From 18e94786894dbd2bda66737f6cbd1dcaeccdf47e Mon Sep 17 00:00:00 2001 From: Viktor Torstensson Date: Thu, 20 Nov 2025 18:59:20 +0100 Subject: [PATCH 1/2] tapd+sqlc: separate SQL migrations from code migs Previously, SQL migrations and code migrations have been designed to be a single migration version, where the SQL migration was run first, followed by a post step migration callback that executed the code migration. Due to a re-definition of the migration design, which separates SQL and code migrations, we need to update tapd to become compatible with the new migration design before updating to the new design. This commit therefore separates the code migrations from migration `33` & `37`, into separate migration files, and adds those two new migrations to the `tapdb/sqlc/migrations` directory. Additionally, since users that have already run the code migrations during migration `33` & `37` will have those migrations re-executed once more after updating to this PR, we test that re-execution of those code migrations will be a no-op for those users, and won't add or change any data in the database. --- tapdb/migrations.go | 2 +- tapdb/migrations_test.go | 184 ++++++++++++++++++ tapdb/post_migration_checks.go | 14 +- ...0048_script_key_type_code_migration.up.sql | 1 + ...9_insert_asset_burns_code_migration.up.sql | 1 + 5 files changed, 194 insertions(+), 8 deletions(-) create mode 100644 tapdb/sqlc/migrations/000048_script_key_type_code_migration.up.sql create mode 100644 tapdb/sqlc/migrations/000049_insert_asset_burns_code_migration.up.sql diff --git a/tapdb/migrations.go b/tapdb/migrations.go index 8deb9fa65..ab9d54edb 100644 --- a/tapdb/migrations.go +++ b/tapdb/migrations.go @@ -24,7 +24,7 @@ const ( // daemon. // // NOTE: This MUST be updated when a new migration is added. - LatestMigrationVersion = 47 + LatestMigrationVersion = 49 ) // DatabaseBackend is an interface that contains all methods our different diff --git a/tapdb/migrations_test.go b/tapdb/migrations_test.go index 4b027199b..cf9641859 100644 --- a/tapdb/migrations_test.go +++ b/tapdb/migrations_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "regexp" + "sort" "strings" "testing" @@ -573,6 +574,10 @@ func TestMigration33(t *testing.T) { // And now that we have test data inserted, we can migrate to the latest // version. + // NOTE: the post-migration check was originally run at migration + // version 33, but was later moved to version 48. Targeting the latest + // migration, will execute post-migration check, but at version 48 + // instead of 33. err := db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( makePostStepCallbacks(db, postMigrationChecks), )) @@ -672,6 +677,116 @@ func TestMigration33(t *testing.T) { ) } +// TestMigration48ScriptKeyTypeReplay makes sure that if the script key type +// backfill already ran for a user (originally at migration 33), the replay at +// migration 48 is a no-op and doesn't rewrite any data. +func TestMigration48ScriptKeyTypeReplay(t *testing.T) { + ctx := context.Background() + + db := NewTestDBWithVersion(t, 32) + + InsertTestdata(t, db.BaseDB, "migrations_test_00033_dummy_data.sql") + + // We simulate that a user previously ran migration 33, which at the + // time also contained the code migration which has now been separated + // to migration 48. + err := db.ExecuteMigrations(TargetVersion(33), WithPostStepCallbacks( + makePostStepCallbacks(db, map[uint]postMigrationCheck{ + 33: determineAndAssignScriptKeyType, + }), + )) + require.NoError(t, err) + + const ( + key1 = "039c571fffcac1a1a7cd3372bd202ad8562f28e48b90f8a4eb714" + + "eca062f576ee6" + key2 = "029c571fffcac1a1a7cd3372bd202ad8562f28e48b90f8a4eb714" + + "eca062f576ee6" + key3 = "03f9cdf1ff7c9fbb0ea3c8533cd7048994f41ea20a79764469c22" + + "aa18aa6696169" + key4 = "027c79b9b26e463895eef5679d8558942c86c4ad2233adef01bc3" + + "e6d540b3653fe" + key5 = "0350aaeb166f4234650d84a2d8a130987aeaf6950206e0905401e" + + "e74ff3f8d18e6" + key6 = "02248bca7dbb12dcf0b490263a1d521691691aa2541842b7472c8" + + "3acac0e88443b" + ) + + expectedKeyTypes := map[string]asset.ScriptKeyType{ + key1: asset.ScriptKeyUnknown, + key2: asset.ScriptKeyBip86, + key3: asset.ScriptKeyScriptPathExternal, + key4: asset.ScriptKeyTombstone, + key5: asset.ScriptKeyScriptPathChannel, + key6: asset.ScriptKeyBurn, + } + + // fetchTypes returns the script key types currently persisted in the + // database keyed by their tweaked hex representation. + fetchTypes := func() map[string]asset.ScriptKeyType { + currentTypes := make(map[string]asset.ScriptKeyType) + + for keyHex := range expectedKeyTypes { + keyBytes, err := hex.DecodeString(keyHex) + require.NoError(t, err) + + dbKey, err := db.BaseDB.FetchScriptKeyByTweakedKey( + ctx, keyBytes, + ) + require.NoError(t, err) + + currentTypes[keyHex] = + extractSqlInt16[asset.ScriptKeyType]( + dbKey.ScriptKey.KeyType, + ) + } + + return currentTypes + } + + // Verify that the database contains the expected script keys after the + // first migration has been run. + require.Equal(t, expectedKeyTypes, fetchTypes()) + + // Now let's change the ScriptKey type for one of the entries, to an + // incorrect value. When the code migration is rerun, this value should + // not be changed despite being incorrect, as the replay of the code + // migration won't act on values which have already been assigned. + keyBytes, err := hex.DecodeString(key5) + require.NoError(t, err) + + dbKey, err := db.BaseDB.FetchScriptKeyByTweakedKey( + ctx, keyBytes, + ) + require.NoError(t, err) + + _, err = db.BaseDB.UpsertScriptKey(ctx, NewScriptKey{ + InternalKeyID: dbKey.InternalKey.KeyID, + TweakedScriptKey: dbKey.ScriptKey.TweakedScriptKey, + Tweak: dbKey.ScriptKey.Tweak, + KeyType: sqlInt16(asset.ScriptKeyBip86), + }) + require.NoError(t, err) + + // Executing the code migration again (now at migration 48) should not + // change or add any new values than the values assigned when the code + // migration was run for migration version 33. + err = db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( + makePostStepCallbacks(db, postMigrationChecks), + )) + require.NoError(t, err) + + // As we changed the value for key5 to an incorrect value, we expect + // that the db still contains that, hence not being equal to the + // expectedKeyTypes. + require.NotEqual(t, expectedKeyTypes, fetchTypes()) + + // If we change the expectedKeyTypes to contain the incorrect value, + // they should however be equal. + expectedKeyTypes[key5] = asset.ScriptKeyBip86 + require.Equal(t, expectedKeyTypes, fetchTypes()) +} + // TestMigration37 tests that the Golang based post-migration check for the // asset burn insertion works as expected. func TestMigration37(t *testing.T) { @@ -685,6 +800,10 @@ func TestMigration37(t *testing.T) { // And now that we have test data inserted, we can migrate to the latest // version. + // NOTE: the post-migration check was originally run at migration + // version 37, but was later moved to version 49. Targeting the latest + // migration, will execute post-migration check, but at version 49 + // instead of 37. err := db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( makePostStepCallbacks(db, postMigrationChecks), )) @@ -696,6 +815,71 @@ func TestMigration37(t *testing.T) { require.Len(t, burns, 5) } +// TestMigration49BurnReplay makes sure that if the asset burn code migration +// already ran for a user (originally at migration 37), the replay at migration +// 49 is a no-op and doesn't insert duplicate burns. +func TestMigration49BurnReplay(t *testing.T) { + ctx := context.Background() + + db := NewTestDBWithVersion(t, 36) + + InsertTestdata(t, db.BaseDB, "migrations_test_00037_dummy_data.sql") + + // The test data inserts 3 burns into the database before the migration + // is run. After the migration is run, 2 more entries will be added. + burnsBefore, err := db.QueryBurns(ctx, QueryBurnsFilters{}) + require.NoError(t, err) + require.Len(t, burnsBefore, 3) + + // We simulate that a user previously ran migration 37, which at the + // time also contained the code migration which has now been separated + // to migration 49. + err = db.ExecuteMigrations(TargetVersion(37), WithPostStepCallbacks( + makePostStepCallbacks(db, map[uint]postMigrationCheck{ + 37: insertAssetBurns, + }), + )) + require.NoError(t, err) + + // Since the migration was insertAssetBurns code migration was run for + // version 37, the database should now contain 5 entries. + burnsAfterFirstMigration, err := db.QueryBurns(ctx, QueryBurnsFilters{}) + require.NoError(t, err) + require.Len(t, burnsAfterFirstMigration, 5) + + normalizeBurns := func(burns []sqlc.QueryBurnsRow) []string { + result := make([]string, 0, len(burns)) + + for _, burn := range burns { + result = append(result, fmt.Sprintf("%x:%x:%x:%d", + burn.AnchorTxid, burn.AssetID, burn.GroupKey, + burn.Amount)) + } + + sort.Strings(result) + + return result + } + + // Execute the rest of the migrations, which will trigger the migration + // version 49 code migration. + err = db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( + makePostStepCallbacks(db, postMigrationChecks), + )) + require.NoError(t, err) + + burnsAfter, err := db.QueryBurns(ctx, QueryBurnsFilters{}) + require.NoError(t, err) + + // Despite that the code migration in migration version 49 was executed + // once more, the asset burns persisted in the database should not have + // changed. + require.Equal( + t, normalizeBurns(burnsAfterFirstMigration), + normalizeBurns(burnsAfter), + ) +} + // TestDirtySqliteVersion tests that if a migration fails and leaves an Sqlite // database backend in a dirty state, any attempts of re-executing migrations on // the db (i.e. restart tapd), will fail with an error indicating that the diff --git a/tapdb/post_migration_checks.go b/tapdb/post_migration_checks.go index 2b8c9b0b2..b78362168 100644 --- a/tapdb/post_migration_checks.go +++ b/tapdb/post_migration_checks.go @@ -18,14 +18,14 @@ import ( ) const ( - // Migration33ScriptKeyType is the version of the migration that - // introduces the script key type. - Migration33ScriptKeyType = 33 + // Migration48ScriptKeyType is the version of the code migration that + // runs the script key type detection/backfill. + Migration48ScriptKeyType = 48 - // Migration37InsertAssetBurns is the version of the migration that + // Migration49InsertAssetBurns is the version of the code migration that // inserts the asset burns into the specific asset burns table by // querying all assets and detecting burns from their witnesses. - Migration37InsertAssetBurns = 37 + Migration49InsertAssetBurns = 49 ) // postMigrationCheck is a function type for a function that performs a @@ -38,8 +38,8 @@ var ( // applied. These functions are used to perform additional checks on the // database state that are not fully expressible in SQL. postMigrationChecks = map[uint]postMigrationCheck{ - Migration33ScriptKeyType: determineAndAssignScriptKeyType, - Migration37InsertAssetBurns: insertAssetBurns, + Migration48ScriptKeyType: determineAndAssignScriptKeyType, + Migration49InsertAssetBurns: insertAssetBurns, } ) diff --git a/tapdb/sqlc/migrations/000048_script_key_type_code_migration.up.sql b/tapdb/sqlc/migrations/000048_script_key_type_code_migration.up.sql new file mode 100644 index 000000000..38740797b --- /dev/null +++ b/tapdb/sqlc/migrations/000048_script_key_type_code_migration.up.sql @@ -0,0 +1 @@ +-- The file intentionally only contains this comment to ensure the file created and picked up in the migration stream. \ No newline at end of file diff --git a/tapdb/sqlc/migrations/000049_insert_asset_burns_code_migration.up.sql b/tapdb/sqlc/migrations/000049_insert_asset_burns_code_migration.up.sql new file mode 100644 index 000000000..38740797b --- /dev/null +++ b/tapdb/sqlc/migrations/000049_insert_asset_burns_code_migration.up.sql @@ -0,0 +1 @@ +-- The file intentionally only contains this comment to ensure the file created and picked up in the migration stream. \ No newline at end of file From 8e4edf677d4eef44757d6e5c84e44e26630a0311 Mon Sep 17 00:00:00 2001 From: Viktor Torstensson Date: Mon, 24 Nov 2025 11:48:43 +0100 Subject: [PATCH 2/2] scripts: ignore missing .down.sql for code migs With the separation of code migrations and SQL migrations in added with the previous commit, it now makes sense to not re-execute a code migration during a database downgrade. Therefore, we need to loosen the strictness of the check-migration-numbering.sh script, to allow for missing .down.sql files for code migrations specifically. This commit implements this change. --- scripts/check-migration-numbering.sh | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/scripts/check-migration-numbering.sh b/scripts/check-migration-numbering.sh index d8a11eed7..031e99379 100755 --- a/scripts/check-migration-numbering.sh +++ b/scripts/check-migration-numbering.sh @@ -9,6 +9,25 @@ if [ ! -d "$migrations_path" ]; then exit 1 fi +# is_sql_migration checks if a .up.sql file contains executable SQL +# statements and not just comments/whitespace/semicolons. The logic mirrors the +# Go helper the `migrate` package dependency uses in the `migrate.go` file. +is_sql_migration() { + local file="$1" + local cleaned + + cleaned=$( + perl -0777 -pe ' + s/^\x{FEFF}//; + s{/\*.*?\*/}{}gs; + s{--[^\r\n]*}{}g; + ' "$file" 2>/dev/null | \ + sed -E 's/[[:space:];]+//g' + ) + + [ -n "$cleaned" ] +} + # Get all unique prefixes (e.g., 000001, always 6 digits) from .up.sql files. prefixes=($(ls "$migrations_path"/*.up.sql 2>/dev/null | \ sed -E 's/.*\/([0-9]{6})_.*\.up\.sql/\1/' | sort)) @@ -34,7 +53,11 @@ for i in "${!prefixes[@]}"; do base_filename=$(ls "$migrations_path/${prefixes[$i]}"_*.up.sql | \ sed -E 's/\.up\.sql//') - if [ ! -f "$base_filename.down.sql" ]; then + # Error if the .up.sql is an SQL migration, but we're missing the + # corresponding .down.sql. This doesn't apply if the .up.sql file is a code + # migration. + if is_sql_migration "$base_filename.up.sql" && \ + [ ! -f "$base_filename.down.sql" ]; then echo "Error: Missing .down.sql file for migration $expected_prefix." exit 1 fi