[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 14270907739

Details

357 of 1400 (25.5%) changed or added relevant lines in 39 files are covered.
33 unchanged lines in 12 files lost coverage.
Overall coverage increased (+0.08%) to 28.424%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
itest/utils.go	0	1	0.0%
tapcfg/server.go	0	1	0.0%
tapdb/asset_minting.go	7	9	77.78%
tapfreighter/wallet.go	0	2	0.0%
taprpc/priceoraclerpc/price_oracle.pb.gw.go	0	3	0.0%
tapchannel/aux_funding_controller.go	0	4	0.0%
tapdb/assets_store.go	66	71	92.96%
address/book.go	0	6	0.0%
tapdb/postgres.go	0	6	0.0%
taprpc/priceoraclerpc/price_oracle.pb.go	2	9	22.22%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	1	71.59%
tapchannel/aux_sweeper.go	1	0.0%
tapdb/asset_minting.go	1	63.16%
tapdb/assets_common.go	1	76.31%
tapdb/assets_store.go	1	63.91%
tapfreighter/wallet.go	1	0.0%
address/mock.go	2	97.39%
itest/assertions.go	2	0.0%
taprpc/marshal.go	2	0.0%
tapfreighter/chain_porter.go	4	0.0%

Totals
Change from base Build 14267448079:	0.08%
Covered Lines:	25975
Relevant Lines:	91384

💛 - 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.

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

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.

ffranr · 2025-04-10T13:27:18Z

tapdb/assets_common.go

+		result = asset.ScriptKey{
+			TweakedScriptKey: &asset.TweakedScriptKey{
+				RawKey: keychain.KeyDescriptor{
+					KeyLocator: locator,
+				},
+				Tweak:         sk.Tweak,
+				DeclaredKnown: extractBool(sk.DeclaredKnown),
 			},
-		},
-		DeclaredKnown: extractBool(dbKey.DeclaredKnown),
+		}
+		err error
+	)
+
+	if len(sk.TweakedScriptKey) == 0 {
+		return result, fmt.Errorf("tweaked script key is empty")
+	}
+
+	if len(ik.RawKey) == 0 {
+		return result, fmt.Errorf("internal raw key is empty")
+	}


I believe that when returning an error, the asset.ScriptKey return value should remain unset. In this case, result is at least partially populated.

Also, I think you should move these function argument checks to the top of the function, before declaring locator and result.

ffranr · 2025-04-10T13:40:19Z

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

+-- will mean the type is not known. Existing script keys at the time of this
+-- migration will be updated at startup after the migration is applied.


I don't think this is true anymore:

Existing script keys at the time of this migration will be updated at startup after the migration is applied.

The script key type value will be set as part of migration.

ffranr · 2025-04-10T14:03:01Z

asset/asset.go

+// GuessType tries to guess the type of the script key based on the information
+// available.
+func (s *ScriptKey) GuessType() ScriptKeyType {


I think "guess" is misleading here. This method appears to classify decisively whenever possible, but it never seems to make a prediction when uncertainty would be warranted. I suggest we rename to Type().

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)?

ffranr · 2025-04-10T14:20:12Z

go.mod

-replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.33.0-hex-display
+replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.34.2-hex-display
+
+replace github.com/golang-migrate/migrate/v4 => github.com/guggero/migrate/v4 v4.0.0-20250328181905-0e91936b8a3b


Should be a fork under the LL org, IMO. Similar to github.com/lightninglabs/protobuf-go-hex-display I suppose. Would be nice to standardise how LL handles its forked packages.

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?

ffranr · 2025-04-10T14:29:00Z

tapdb/post_migration_checks.go

+		postStepCallbacks[version] = func(m *migrate.Migration,
+			_ database.Driver) error {
+
+			return txDb.ExecTx(
+				ctx, &writeTxOpts, func(q sqlc.Querier) error {
+					return runCheck(m, q)
+				},
+			)
+		}


ah nice! The callback is executed in a transaction here.

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?

ffranr · 2025-04-10T15:06:04Z

rpcserver.go

@@ -2302,21 +2302,21 @@ func (r *rpcServer) FundVirtualPsbt(ctx context.Context,

 // unmarshalCoinSelectType converts an RPC select type into a native one.
 func unmarshalCoinSelectType(
-	coinSelectType wrpc.CoinSelectType) (tapsend.CoinSelectType, error) {
+	coinSelectType wrpc.CoinSelectType) (fn.Option[asset.ScriptKeyType],


func doc comment could be updated

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?

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 added 15 commits April 4, 2025 17:15

tapdb: fix comments and args

1059442

tapdb: use embeddings for script key

e655078

tapcfg+tapdb: move and export DatabaseBackend

a72e74a

tapdb: turn dbAssetsToChainAssets into function

ea5bfbe

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

tapdb: add postmigration checks

642c0e0

taprpc: re-compile after update of protobuf library

914f109

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

f8f67b1

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

multi: remove now unused DeclaredKnown field

8c230eb

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

e3903d0

multi: add script key type filter to queries

e1d3fe3

multi: turn coin selection type into script key filter

fa1a94e

tapdb: change script key exclusion mechanism

946cdf6

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.

guggero added 2 commits April 4, 2025 19:30

itest: query for all asset script types

a3c79c2

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

450bb48

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

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

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

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

ffranr Apr 10, 2025

		-- will mean the type is not known. Existing script keys at the time of this
		-- migration will be updated at startup after the migration is applied.

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

Are you sure you want to change the base?

[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 14270907739

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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