lightninglabs
[tapdb]: add script key type enum, run Golang based post migration checks
Depends on https://github.com/golang-migrate/migrate/pull/1253 (might need to maintain our own fork if we decide to go with the approach).
Fixes https://github.com/lightninglabs/taproot-assets/issues/1458. Fixes https://github.com/lightninglabs/taproot-assets/issues/1162.
Adds a specific key_type field to the script key table, which allows us to distinguish between the following types of keys:
- Unknown (--> replaces the previous
declared_knownboolean, which can now be synthesized withkey_type != Unknown) - Bip-86
- Script path external (user-defined key with script paths)
- Burn
- Tombstone
- Taproot Asset Channel keys
This allows us to correctly filter out channel related keys when showing balances (e.g. ListBalances, ListUtxos or ListAssets) and also allows us to do more custom coin selection.
This is required for cleanly filter out channel related script keys implemented in https://github.com/lightninglabs/taproot-assets/pull/1413.
cc @GeorgeTsagk: this mechanism can be used to add asset burns to the new table retroactively.
Pull Request Test Coverage Report for Build 14775742039
Details
- 361 of 1468 (24.59%) changed or added relevant lines in 42 files are covered.
- 26 unchanged lines in 12 files lost coverage.
- Overall coverage increased (+0.05%) to 28.747%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| itest/utils.go | 0 | 1 | 0.0% |
| tapcfg/server.go | 0 | 1 | 0.0% |
| tapchannel/aux_funding_controller.go | 0 | 1 | 0.0% |
| tapdb/asset_minting.go | 7 | 9 | 77.78% |
| tapfreighter/wallet.go | 0 | 2 | 0.0% |
| tapchannel/commitment.go | 0 | 3 | 0.0% |
| taprpc/priceoraclerpc/price_oracle.pb.gw.go | 0 | 3 | 0.0% |
| tapdb/assets_store.go | 63 | 68 | 92.65% |
| address/book.go | 0 | 6 | 0.0% |
| tapdb/postgres.go | 0 | 6 | 0.0% |
| <!-- | Total: | 361 | 1468 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| asset/asset.go | 1 | 46.84% |
| tapchannel/aux_sweeper.go | 1 | 0.0% |
| tapdb/asset_minting.go | 1 | 63.16% |
| tapdb/assets_common.go | 1 | 76.75% |
| tapdb/assets_store.go | 1 | 64.64% |
| tapfreighter/wallet.go | 1 | 0.0% |
| address/address.go | 2 | 69.55% |
| commitment/tap.go | 2 | 71.36% |
| itest/assertions.go | 2 | 0.0% |
| taprpc/marshal.go | 2 | 0.0% |
| <!-- | Total: | 26 |
| Totals | |
|---|---|
| Change from base Build 14775721701: | 0.05% |
| Covered Lines: | 26840 |
| Relevant Lines: | 93365 |
💛 - Coveralls
I finally got this down to a single TODO: Add more integration tests. But other than that this contains all functionality that I wanted to add (and that we need). So asking for a first round of review now.
Addressed the final TODO by asserting the new balances in multiple integration test steps.
The high level idea is to migrate using the migrate in stages and pause to apply post migration callbacks.
With your formulation, what happens if we upgrade to version 3 from version 2 (2->3), then as we're executing the call back for post version 2, the daemon crashes?
IIUC, we'll start from version 3, and skip the post step call back of version 2.
What we want here is that the call back is executed in the exact same context as the migration to version 2 in order to retain atomicity. The db shouldn't move to version 2 until the call back has successfully been executed.
The high level idea is to migrate using the migrate in stages and pause to apply post migration callbacks.
With your formulation, what happens if we upgrade to version 3 from version 2 (2->3), then as we're executing the call back for post version 2, the daemon crashes?
IIUC, we'll start from version 3, and skip the post step call back of version 2.
What we want here is that the call back is executed in the exact same context as the migration to version 2 in order to retain atomicity. The db shouldn't move to version 2 until the call back has successfully been executed.
@Roasbeef I agree that my approach isn't atomic, since the callback wouldn't run within the same database transaction. But I believe that's also the case with the current state of the migration package fork solution.
That said, the fork solution does allow the callback to run while holding the same driver lock, which is still valuable. So I think forking the package is probably the best path forward.
EDIT: The callback is executed within a transaction, but that doesn't happen within migrate or the forked migrate package.
@guggero, remember to re-request review from reviewers when ready
Had to add a commit to point to the script-key-migrations branch in litd: https://github.com/lightninglabs/lightning-terminal/pull/1053
Was able to re-use a lot of the new itest balance assertion code, which is nice.
@Roasbeef feel free to merge this PR if you feel like your questions have been addressed satisfactorily.