[tapdb]: add script key type enum, run Golang based post migration checks #1198

guggero · 2024-11-15T19:14:46Z

Depends on golang-migrate/migrate#1253 (might need to maintain our own fork if we decide to go with the approach).

Fixes #1458.
Fixes #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_known boolean, which can now be synthesized with key_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 #1413.

cc @GeorgeTsagk: this mechanism can be used to add asset burns to the new table retroactively.

coveralls · 2025-04-02T20:56:54Z

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%

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%

Totals
Change from base Build 14775721701:	0.05%
Covered Lines:	26840
Relevant Lines:	93365

💛 - Coveralls

guggero · 2025-04-03T14:26:21Z

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.

guggero · 2025-04-04T17:32:00Z

Addressed the final TODO by asserting the new balances in multiple integration test steps.

ffranr

I don't think we need to fork the migrate package (guggero/migrate@f82a988) It would be nice not to have to maintain a fork.

Can't we get away with just reformulating applyMigrations in tapdb/migrations.go?

Roughly:

sqlMigrate, err := migrate.NewWithInstance(
    "migrations", migrateFileServer, dbName, driver,
)

// We will iter over `opts.postStepCallbacks` in ascending version index.
// Get next post migration callback index from `opts.postStepCallbacks`:
// `nextVersion`.
//
// Migrate to that version.
sqlMigrate.Migrate(nextVersion)

// Run callback function using the same driver that we passed into
// `migrate.NewWithInstance`.
//
// Flag version as dirty before callback exec.
driver.SetVersion(nextVersion, true)

err := opts.postStepCallbacks[nextVersion](driver)
if err != nil {...}

// Set version clean given no callback error.
driver.SetVersion(nextVersion, false)

// Handle the next version in `opts.postStepCallbacks` or until target migration version is reached.

The high level idea is to migrate using the migrate in stages and pause to apply post migration callbacks.

Roasbeef · 2025-04-10T01:20:06Z

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.

ffranr · 2025-04-10T11:37:30Z

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.

tapdb/assets_common.go

tapdb/sqlc/migrations/000031_script_key_type.up.sql

asset/asset.go

ffranr · 2025-04-10T14:17:17Z

tapfreighter/chain_porter.go

+		// Burn and tombstone keys are the only keys that we don't
+		// explicitly store in the DB before this point. But we'll want
+		// them to have the correct type when creating the transfer, so
+		// we'll set that now.
+		detectUnSpendableKeys(currentPkg.VirtualPackets)


This seems like an odd place to set the script type in the vout. Can't we just do that when we're constructing the vout (or assigning the burn/tombstone script key to the vout)?

Such a key might come from the RPC interface, set by the user. So it's hard to determine an exact point before that we could reliably do this. But I moved the logic even closer to the database logic, so it is more clear that this only affects what is stored in the DB and not really the transfer itself.

go.mod

ffranr · 2025-04-10T14:28:13Z

tapdb/post_migration_checks.go

+		if _, ok := burnKeys[serializedKey]; ok {
+			newType = asset.ScriptKeyBurn
+		} else {
+			guessedType := scriptKey.GuessType()


Same here with the term "guess". Is there an aspect of this that's uncertain that I'm not seeing?

There is some uncertainty to it. For example with channel related keys. We can detect the funding output key, since that's the static OP_TRUE (at least before we added the universe collision avoidance that now creates unique keys for each asset ID, but we assume that only happens in the release that also contains this PR, where we explicitly set the type on key creation). But we can't detect any to_local or to_remote or HTLC related channel keys. Those will be shown as ScriptKeyScriptPathExternal which isn't 100% correct.
So users who had taproot asset channels that force closed before this PR will see such script keys. But at least they should also show as "spent", so hopefully that won't bother anyone.

tapdb/post_migration_checks.go

ffranr · 2025-04-10T15:00:01Z

asset/asset.go

 func (s *ScriptKey) DeclaredAsKnown() bool {
-	return s.TweakedScriptKey != nil && s.TweakedScriptKey.DeclaredKnown
+	return s.TweakedScriptKey != nil && s.Type != ScriptKeyUnknown


Maybe ScriptKeyUnknown should be renamed to ScriptKeyTypeUnknown, because the type of script key is the thing that is unknown, even for a tracked script key. Have i understood that correctly?

Yeah, I guess so. But then as a consequence we should also put the word Type into the values of the pseudo-enum, so ScriptKeyTypeBip86, ScriptKeyTypeScriptPathExternal and so on, which makes them even longer.
But since we have an explicit Golang type that also has the word Type in it, that also provides some context to it. So as long as we don't have any naming collision I'm opting to keep the current naming.

rpcserver.go

ffranr · 2025-04-10T15:12:53Z

tapdb/assets_store.go

+	assetBalancesFilter.ExcludeScriptKeyType = sqlInt16(
+		asset.ScriptKeyScriptPathChannel,
+	)
+


Do we also need to exclude burn and tombstone here? I would expect ExcludeScriptKeyType to be a list—same in the SQL query. But perhaps we can defer making it a list for now and still remain correct?

I would argue no. Burn keys are something the user explicitly creates, so IMO they should be shown if the user selects with_spent. And tombstones are created behind the scenes, BUT they have have a relevance to the user as they might lock up BTC in an otherwise empty UTXO (see #1458 that is fixed by this PR).
The only type of script key that IMO should always be hidden from the user (as otherwise they will cause confusion and questions) are channel related keys and their assets. Sure, the user creates those channels, so it is the result of a user action. But for any balance related calls we should not include them, as otherwise the assets might be counted twice (once off-chain and once on-chain). And especially force close output related script keys should be hidden, as they are always swept into "normal" outputs anyway under the hood.

lightninglabs-deploy · 2025-04-24T16:38:45Z

@guggero, remember to re-request review from reviewers when ready

GeorgeTsagk

LGTM 💥

tapdb/sqlc/queries/addrs.sql

tapchannel/aux_closer.go

tapdb/post_migration_checks.go

taprpc/taprootassets.proto

Roasbeef

Great work! Very comprehensive change, and now we have a method of doing optional Go migrations as well.

Only potential blocker I identified is if we need to change the defaults on the CLI due to the new filtration semantics. We may also need to modify some balance checks/assertions in the litd channel tests as well. Can do both/either of those as a follow up.

LGTM 🦃

Roasbeef · 2025-04-29T23:59:51Z

tapdb/sqlc/queries/assets.sql

@@ -135,6 +135,9 @@ SELECT seedling_id, asset_name, asset_type, asset_version, asset_supply,
    sqlc.embed(assets_meta),
    emission_enabled, batch_id, 
    group_genesis_id, group_anchor_id, group_tapscript_root,
+    -- TODO(guggero): We should use sqlc.embed() for the script key and internal
+    -- key fields, but we can't because it's a LEFT JOIN. We should check if the
+    -- LEFT JOIN is actually necessary or if we always have keys for seedlings.


Good q...

Perhaps this is from before we had an explicit asset meta?

Looking at it now, the only portion that def needs a left join is for the group keys, as not every seedling actually has a group key.

I think this is a quirk of the minting logic. At some point during the minting process there can be seedlings that don't yet have a script key defined, but we still want to be able to store them. I changed this to a normal join and tests started failing, so I didn't look into it more closely.

Roasbeef · 2025-04-30T00:04:06Z

tapdb/sqlc/migrations/000033_script_key_type.up.sql

+-- will mean the type is not known. Existing script keys will be inspected by
+-- the post-migration step Golang code that determines the type of each script
+-- key.
+ALTER TABLE script_keys ADD COLUMN key_type SMALLINT;


Non-blocking, but an alternative here would be the enum table pattern. So a new script_key_type table w/ set values (burn, transfer, channel, etc) and this new column refs that. This let's us put the mapping of string type to integer within the db itself.

Tradeoff is potentially a join to match thing sup if just the primary key is stored here. Adding new types also then is a migration, albeit just one that inserts a single row into this new table.

Just using an integer type here and putting the mapping in the Go layer as is don here certainly simpler.

Right, but what would the benefits of that be? IMO it doesn't make the data more easy to read "by eye", meaning that without the join it would still be a non-self-explanatory number.
And on top of that we'd need to string matching to map the values to Golang constants. Or would the idea be to use string-based constants on the Golang level?

tapdb/sqlc/queries/assets.sql

asset/asset.go

Roasbeef · 2025-04-30T00:17:12Z

asset/asset.go

+	// If there is a known tweak, then we know that this is a script path
+	// key. We never return the channel type, since those keys should always
+	// be declared properly, and we never should need to guess their type.
+	if s.HasScriptPath() {


Seems that for tap channel keys, we'd miss out on a more precise label here. Unless we always take a code path that skips this routine.

Yes, that's correct. For already existing channel keys (excluding the funding OP_TRUE key, as that's detectable), we couldn't assign the correct value.
But those should only include force close outputs. And they should be spent already, so hopefully not causing too much confusion for users.

rpcserver.go

taprpc/marshal.go

Roasbeef · 2025-04-30T00:39:42Z

tapdb/assets_store.go

-	assetBalancesFilter.ExcludeKey = excludeKey.PubKey.SerializeCompressed()
+		// If the user explicitly wants to see the channel related asset
+		// balances, we need to set the exclude type to NULL.
+		if t == asset.ScriptKeyScriptPathChannel {


But this doesn't allow them to only query for the channel related balances?

Hmm, not sure I understand... We always set the ExcludeScriptKeyType to channel keys above. But if the user explicitly wants to see channel keys, we set the ExcludeScriptKeyType to NULL, disabling the exclude.

tapdb/sqlc/queries/assets.sql

Roasbeef · 2025-04-30T00:40:54Z

itest/burn_test.go

-	)
+	allAssets, err := t.tapd.ListAssets(ctxt, &taprpc.ListAssetRequest{
+		IncludeSpent:  true,
+		ScriptKeyType: allScriptKeysQuery,


Do we need to change CLI defaults as a result of this?

Also something to call out in the release notes as a change in behavior here.

No, the defaults remain the same. But I added a new --script_key_type flag to all relevant commands so the users can also filter on the command line.

guggero · 2025-04-30T18:24:23Z

Had to add a commit to point to the script-key-migrations branch in litd: lightninglabs/lightning-terminal#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.

With this commit we add a new numeric type for the type of a script key. This will be a Golang enum/const that's going to be assigned manually when declaring a key as known. For existing keys, we'll add a new mechanism in the following commits that runs on startup after we detect a SQL migration was applied that will attempt the detection of the type of those keys.

With this commit we add a type enum to the script key. This will mostly be used on the database level to filter assets when listing or calculating balances. We can only be 100% certain about the type of a key in the case of BIP-0086 or where we explicitly create them in the tapchannel package. Anything else is a bit of a guess, since the keys are coming over the RPC interface. So we just assume any key that has a non-empty script path is an externally-defined (external app) script spend. This heuristic should be good enough to filter out channel related assets or assets that can't be spent without external instructions in balance RPC calls.

In order for burn/tombstone keys to be stored with the correct type, we need to detect them before storing the transfer outputs to the database. We can't set the script key type before, because we're carrying around the script key in a virtual packet, where that information isn't available.

We'll want to re-use that function in an upcoming commit, without needing to instantiate a full asset store.

The previous commit forced us to update some gRPC/REST related libraries, which cause a diff in the generated RPC stubs.

Specifying a script key as known now requires the user to set a specific type.

Since we can now declare more than just knowledge of a script key by giving it a specific type, we no longer need the boolean flag that was added as a temporary workaround.

With an explicit type for a script key now being used, we can turn the key based matching into a type based matching, which will also be more generic for unique funding script keys that are required for grouped asset channel funding.

To make sure our itests still work, we need to query assets with all possible script key types.

We add a new AssetBalances function that makes sure that the output from all the following RPCs is aligned: - ListAssets - ListBalances (both grouped by asset ID or group key) - ListUtxos

This commit adds a new --script_key_type flag to the asset related commands, where supported by the corresponding RPC. If nothing is specified, the previous "show all assets" behavior is kept.

We need to update the way we assert balances in the integration tests, because with the changes in this PR we no longer show channel funding related asset outputs by default.

dstadulis assigned guggero Nov 15, 2024

dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 15, 2024

guggero modified the milestones: v0.5 (v0.4.2 rename), v0.6 - Post MVP Features Dec 5, 2024

guggero force-pushed the script-key-migrations branch 3 times, most recently from 33717eb to 1b9a8cc Compare March 28, 2025 18:23

guggero force-pushed the script-key-migrations branch from 1b9a8cc to d85ebb9 Compare April 2, 2025 20:34

guggero force-pushed the script-key-migrations branch from d85ebb9 to c63abbe Compare April 3, 2025 14:25

guggero marked this pull request as ready for review April 3, 2025 14:25

guggero requested review from gijswijs and Roasbeef April 3, 2025 14:27

levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Apr 3, 2025

guggero force-pushed the script-key-migrations branch from c63abbe to 450bb48 Compare April 4, 2025 17:30

levmi requested review from ffranr and removed request for Roasbeef April 7, 2025 15:45

ffranr reviewed Apr 8, 2025

View reviewed changes

ffranr reviewed Apr 10, 2025

View reviewed changes

ffranr requested review from GeorgeTsagk and removed request for gijswijs April 22, 2025 18:47

guggero force-pushed the script-key-migrations branch 2 times, most recently from de58c89 to 31347c7 Compare April 24, 2025 09:54

guggero force-pushed the script-key-migrations branch from 31347c7 to 03cdc3b Compare April 25, 2025 08:52

guggero mentioned this pull request Apr 25, 2025

mod: add replace directive to compile tapd lightninglabs/loop#929

Merged

GeorgeTsagk approved these changes Apr 25, 2025

View reviewed changes

tapdb/sqlc/queries/addrs.sql Show resolved Hide resolved

tapchannel/aux_closer.go Show resolved Hide resolved

tapdb/post_migration_checks.go Outdated Show resolved Hide resolved

taprpc/taprootassets.proto Show resolved Hide resolved

taprpc/taprootassets.proto Outdated Show resolved Hide resolved

Roasbeef approved these changes Apr 30, 2025

View reviewed changes

guggero force-pushed the script-key-migrations branch 3 times, most recently from a67a5e9 to c4317f3 Compare April 30, 2025 16:50

guggero mentioned this pull request Apr 30, 2025

itest: update balance assertions after changes to script key type handling lightninglabs/lightning-terminal#1053

Merged

guggero added 19 commits May 1, 2025 14:56

tapdb: fix comments and args

42bcae4

tapdb: use embeddings for script key

90cce94

tapcfg+tapdb: move and export DatabaseBackend

36a2010

tapdb: turn dbAssetsToChainAssets into function

0c94492

We'll want to re-use that function in an upcoming commit, without needing to instantiate a full asset store.

tapdb: add postmigration checks

5d263ee

taprpc: re-compile after update of protobuf library

83451bd

The previous commit forced us to update some gRPC/REST related libraries, which cause a diff in the generated RPC stubs.

taprpc+rpcserver: allow user to set script key type

8bda951

Specifying a script key as known now requires the user to set a specific type.

multi: remove now unused DeclaredKnown field

f65f32b

Since we can now declare more than just knowledge of a script key by giving it a specific type, we no longer need the boolean flag that was added as a temporary workaround.

taprpc: add script key type to asset

a662f64

multi: add script key type filter to queries

4b74c99

multi: turn coin selection type into script key filter

8881ec2

tapdb: change script key exclusion mechanism

862af8d

With an explicit type for a script key now being used, we can turn the key based matching into a type based matching, which will also be more generic for unique funding script keys that are required for grouped asset channel funding.

itest: query for all asset script types

2b55f82

To make sure our itests still work, we need to query assets with all possible script key types.

itest: add new balance assertions to multiple tests

414edc5

We add a new AssetBalances function that makes sure that the output from all the following RPCs is aligned: - ListAssets - ListBalances (both grouped by asset ID or group key) - ListUtxos

cmd/commands: add script key flag to commands

482130e

This commit adds a new --script_key_type flag to the asset related commands, where supported by the corresponding RPC. If nothing is specified, the previous "show all assets" behavior is kept.

GitHub: update litd itest branch

7b38305

We need to update the way we assert balances in the integration tests, because with the changes in this PR we no longer show channel funding related asset outputs by default.

guggero force-pushed the script-key-migrations branch from c4317f3 to 7b38305 Compare May 1, 2025 12:57

guggero merged commit 8fb78e2 into main May 1, 2025
18 checks passed

github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board May 1, 2025

guggero deleted the script-key-migrations branch May 1, 2025 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tapdb]: add script key type enum, run Golang based post migration checks #1198

[tapdb]: add script key type enum, run Golang based post migration checks #1198

guggero commented Nov 15, 2024 •

edited

Loading

coveralls commented Apr 2, 2025 •

edited

Loading

guggero commented Apr 3, 2025

guggero commented Apr 4, 2025

ffranr left a comment •

edited

Loading

Roasbeef commented Apr 10, 2025

ffranr commented Apr 10, 2025 •

edited

Loading

ffranr Apr 10, 2025

guggero Apr 25, 2025

ffranr Apr 10, 2025

guggero Apr 25, 2025

ffranr Apr 10, 2025

guggero Apr 25, 2025

ffranr Apr 10, 2025

guggero Apr 25, 2025

lightninglabs-deploy commented Apr 24, 2025

GeorgeTsagk left a comment

Roasbeef left a comment

Roasbeef Apr 29, 2025

guggero Apr 30, 2025

Roasbeef Apr 30, 2025

guggero Apr 30, 2025

Roasbeef Apr 30, 2025

guggero Apr 30, 2025

Roasbeef Apr 30, 2025

guggero Apr 30, 2025

Roasbeef Apr 30, 2025

guggero Apr 30, 2025

guggero commented Apr 30, 2025

[tapdb]: add script key type enum, run Golang based post migration checks #1198

[tapdb]: add script key type enum, run Golang based post migration checks #1198

Conversation

guggero commented Nov 15, 2024 • edited Loading

coveralls commented Apr 2, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14775742039

Details

💛 - Coveralls

guggero commented Apr 3, 2025

guggero commented Apr 4, 2025

ffranr left a comment • edited Loading

Choose a reason for hiding this comment

Roasbeef commented Apr 10, 2025

ffranr commented Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Apr 24, 2025

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Apr 30, 2025

guggero commented Nov 15, 2024 •

edited

Loading

coveralls commented Apr 2, 2025 •

edited

Loading

ffranr left a comment •

edited

Loading

ffranr commented Apr 10, 2025 •

edited

Loading