diff --git a/.github/actions/lit-setup/action.yml b/.github/actions/lit-setup/action.yml index 557a6b1ce..5478d935c 100644 --- a/.github/actions/lit-setup/action.yml +++ b/.github/actions/lit-setup/action.yml @@ -24,6 +24,7 @@ runs: run: | go mod edit -replace=github.com/lightninglabs/taproot-assets=../ go mod edit -replace=github.com/lightninglabs/taproot-assets/taprpc=../taprpc + go mod edit -replace=github.com/golang-migrate/migrate/v4=github.com/lightninglabs/migrate/v4@v4.18.2-9023d66a-fork-pr-2.0.20251211093704-71c1eef09789 go mod tidy shell: bash diff --git a/go.mod b/go.mod index 8dbbefe8f..db624c636 100644 --- a/go.mod +++ b/go.mod @@ -213,7 +213,7 @@ replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-d // We are using a fork of the migration library with custom functionality that // did not yet make it into the upstream repository. -replace github.com/golang-migrate/migrate/v4 => github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2 +replace github.com/golang-migrate/migrate/v4 => github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2.0.20251211093704-71c1eef09789 // Note this is a temproary replace and will be removed when taprpc is tagged. replace github.com/lightninglabs/taproot-assets/taprpc => ./taprpc diff --git a/go.sum b/go.sum index 52f68fc17..1f1d69baa 100644 --- a/go.sum +++ b/go.sum @@ -1142,8 +1142,8 @@ github.com/lightninglabs/lightning-node-connect/mailbox v1.0.1 h1:RWmohykp3n/DTM github.com/lightninglabs/lightning-node-connect/mailbox v1.0.1/go.mod h1:NYtNexZE9gO1eOeegTxmIW9fqanl7eZ9cOrE9yewSAk= github.com/lightninglabs/lndclient v0.20.0-5 h1:CSTJxMoE9aTI92QKJseh1G4l9jqdN7X1JrDyXdbbmNo= github.com/lightninglabs/lndclient v0.20.0-5/go.mod h1:gBtIFPGmC2xIspGIv/G5+HiPSGJsFD8uIow7Oke1HFI= -github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2 h1:eFjp1dIB2BhhQp/THKrjLdlYuPugO9UU4kDqu91OX/Q= -github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2/go.mod h1:99BKpIi6ruaaXRM1A77eqZ+FWPQ3cfRa+ZVy5bmWMaY= +github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2.0.20251211093704-71c1eef09789 h1:7kX7vUgHUazAHcCJ6uzBDa4/2MEGEbMEfa01GtfqmTQ= +github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2.0.20251211093704-71c1eef09789/go.mod h1:99BKpIi6ruaaXRM1A77eqZ+FWPQ3cfRa+ZVy5bmWMaY= github.com/lightninglabs/neutrino v0.16.1 h1:5Kz4ToxncEVkpKC6fwUjXKtFKJhuxlG3sBB3MdJTJjs= github.com/lightninglabs/neutrino v0.16.1/go.mod h1:L+5UAccpUdyM7yDgmQySgixf7xmwBgJtOfs/IP26jCs= github.com/lightninglabs/neutrino/cache v1.1.3 h1:rgnabC41W+XaPuBTQrdeFjFCCAVKh1yctAgmb3Se9zA= diff --git a/tapdb/migrations.go b/tapdb/migrations.go index 9ac1a92df..d3718e8ca 100644 --- a/tapdb/migrations.go +++ b/tapdb/migrations.go @@ -67,15 +67,17 @@ var ( // migrationOption is a functional option that can be passed to migrate related // methods to modify their behavior. type migrateOptions struct { - latestVersion fn.Option[uint] - postStepCallbacks map[uint]migrate.PostStepCallback + latestVersion fn.Option[uint] + programmaticMigrations map[uint]migrate.ProgrammaticMigrEntry } // defaultMigrateOptions returns a new migrateOptions instance with default // settings. func defaultMigrateOptions() *migrateOptions { return &migrateOptions{ - postStepCallbacks: make(map[uint]migrate.PostStepCallback), + programmaticMigrations: make( + map[uint]migrate.ProgrammaticMigrEntry, + ), } } @@ -91,18 +93,15 @@ func WithLatestVersion(version uint) MigrateOpt { } } -// WithPostStepCallbacks is an option that can be used to set a map of -// PostStepCallback functions that can be used to execute a Golang based -// migration step after a SQL based migration step has been executed. The key is -// the migration version and the value is the callback function that should be -// run _after_ the step was executed (but before the version is marked as -// cleanly executed). An error returned from the callback will cause the -// migration to fail and the step to be marked as dirty. -func WithPostStepCallbacks( - postStepCallbacks map[uint]migrate.PostStepCallback) MigrateOpt { +// WithProgrammaticMigrations is an option that can be used to set a map of +// ProgrammaticMigrEntry functions that can be used to execute a Golang based +// migration step. The key is the migration version and the value is the +// Golang migration function entry that should be run for the migration version. +func WithProgrammaticMigrations( + programmaticMigrs map[uint]migrate.ProgrammaticMigrEntry) MigrateOpt { return func(o *migrateOptions) { - o.postStepCallbacks = postStepCallbacks + o.programmaticMigrations = programmaticMigrs } } @@ -160,7 +159,9 @@ func applyMigrations(fs fs.FS, driver database.Driver, path, dbName string, // above. sqlMigrate, err := migrate.NewWithInstance( "migrations", migrateFileServer, dbName, driver, - migrate.WithPostStepCallbacks(opts.postStepCallbacks), + migrate.WithProgrammaticMigrations( + opts.programmaticMigrations, + ), ) if err != nil { return err diff --git a/tapdb/migrations_test.go b/tapdb/migrations_test.go index 0c344dd5e..4d1b1a0c7 100644 --- a/tapdb/migrations_test.go +++ b/tapdb/migrations_test.go @@ -2,6 +2,7 @@ package tapdb import ( "context" + "database/sql" "encoding/hex" "errors" "fmt" @@ -574,7 +575,7 @@ func TestMigration31(t *testing.T) { ) } -// TestMigration33 tests that the Golang based post migration check for the +// TestMigration33 tests that the Golang based programmatic migration for the // script key type detection works as expected. It verifies that the script key // types are detected correctly and that the migration to version 31 works as // expected. @@ -589,12 +590,12 @@ 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 + // NOTE: the programmatic migration was originally run at migration // version 33, but was later moved to version 50. Targeting the latest - // migration, will execute post-migration check, but at version 50 + // migration will execute the programmatic migration at version 50 // instead of 33. - err := db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( - makePostStepCallbacks(db, postMigrationChecks), + err := db.ExecuteMigrations(TargetLatest, WithProgrammaticMigrations( + makeProgrammaticMigrations(db, programmaticMigrations, true), )) require.NoError(t, err) @@ -703,15 +704,24 @@ func TestMigration50ScriptKeyTypeReplay(t *testing.T) { 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 programmatic migration which has now been - // separated to migration 50. - callbacks := map[uint]postMigrationCheck{ - OldScriptKeyTypeMigr: determineAndAssignScriptKeyType, - } + // time contained both the SQL and the programmatic migration. To avoid + // mixing SQL and programmatic migrations in the same version as that + // is no longer allowed by the migrate dependency, we first run only the + // SQL migrations up to version 33, then execute the programmatic + // migration manually. + err := db.ExecuteMigrations(TargetVersion(OldScriptKeyTypeMigr)) + require.NoError(t, err) - err := db.ExecuteMigrations( - TargetVersion(OldScriptKeyTypeMigr), - WithPostStepCallbacks(makePostStepCallbacks(db, callbacks)), + // Manually execute the programmatic migration, which was initially run + // at migration version 33 (post the SQL migration). + txDb := NewTransactionExecutor( + db, func(tx *sql.Tx) sqlc.Querier { return db.WithTx(tx) }, + ) + err = txDb.ExecTx( + ctx, &AssetStoreTxOptions{}, + func(q sqlc.Querier) error { + return determineAndAssignScriptKeyType(ctx, q) + }, ) require.NoError(t, err) @@ -791,8 +801,10 @@ func TestMigration50ScriptKeyTypeReplay(t *testing.T) { // should not change or add any new values than the values assigned when // the programmatic migration was run for migration version 33. err = db.ExecuteMigrations(TargetVersion(Migration50ScriptKeyType), - WithPostStepCallbacks( - makePostStepCallbacks(db, postMigrationChecks), + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db, programmaticMigrations, true, + ), ), ) require.NoError(t, err) @@ -808,7 +820,7 @@ func TestMigration50ScriptKeyTypeReplay(t *testing.T) { require.Equal(t, expectedKeyTypes, fetchTypes()) } -// TestMigration37 tests that the Golang based post-migration check for the +// TestMigration37 tests that the Golang based programmatic migration for the // asset burn insertion works as expected. func TestMigration37(t *testing.T) { ctx := context.Background() @@ -821,12 +833,12 @@ 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 + // NOTE: the programmatic migration was originally run at migration // version 37, but was later moved to version 51. Targeting the latest - // migration, will execute post-migration check, but at version 51 + // migration will execute the programmatic migration at version 51 // instead of 37. - err := db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( - makePostStepCallbacks(db, postMigrationChecks), + err := db.ExecuteMigrations(TargetLatest, WithProgrammaticMigrations( + makeProgrammaticMigrations(db, programmaticMigrations, true), )) require.NoError(t, err) @@ -853,14 +865,24 @@ func TestMigration51BurnReplay(t *testing.T) { require.Len(t, burnsBefore, 3) // We simulate that a user previously ran migration 37, which at the - // time also contained the programmatic migration which has now been - // separated to migration 51. - err = db.ExecuteMigrations(TargetVersion(OldInsertAssetBurnsMigr), - WithPostStepCallbacks( - makePostStepCallbacks(db, map[uint]postMigrationCheck{ - OldInsertAssetBurnsMigr: insertAssetBurns, - }), - ), + // time contained both the SQL and the programmatic migration. To avoid + // mixing SQL and programmatic migrations in the same version as that is + // no longer allowed by the migrate dependency, we first run only the + // SQL migrations up to version 37, then execute the programmatic + // migration manually. + err = db.ExecuteMigrations(TargetVersion(OldInsertAssetBurnsMigr)) + require.NoError(t, err) + + // Manually execute the programmatic migration, which was initially run + // at migration version 37 (post the SQL migration). + txDb := NewTransactionExecutor( + db, func(tx *sql.Tx) sqlc.Querier { return db.WithTx(tx) }, + ) + err = txDb.ExecTx( + ctx, &AssetStoreTxOptions{}, + func(q sqlc.Querier) error { + return insertAssetBurns(ctx, q) + }, ) require.NoError(t, err) @@ -887,8 +909,10 @@ func TestMigration51BurnReplay(t *testing.T) { // Execute the rest of the migrations, which will trigger the migration // version 51 programmatic migration. err = db.ExecuteMigrations(TargetVersion(Migration51InsertAssetBurns), - WithPostStepCallbacks( - makePostStepCallbacks(db, postMigrationChecks), + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db, programmaticMigrations, true, + ), ), ) require.NoError(t, err) @@ -909,28 +933,36 @@ func TestMigration51BurnReplay(t *testing.T) { // 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 // database is in a dirty state. This is regardless of whether the failing -// migration is the latest migration or an intermediate migration. +// migration is the targeted migration or an intermediate migration. +// +// NOTE: This test defines custom programmatic migrations for version 50 & 51, +// with the resetVersionOnError flag set to false, simulating that the +// programmatic migrations at those version errors, leaving the DB in a +// dirty state. func TestDirtySqliteVersion(t *testing.T) { var ( err error testError = errors.New("test error") - // testPostMigrationChecks1 is a map that will trigger a - // migration callback for migration 2 which always returns an + // testProgrammaticMigrations1 is a map that will trigger a + // programmatic for migration 50 which always returns an // error. This is used to simulate an intermediate migration // that fails and leaves the db in a dirty state. - testPostMigrationChecks1 = map[uint]postMigrationCheck{ - 2: func(ctx context.Context, q sqlc.Querier) error { + testProgrammaticMigrations1 = map[uint]programmaticMigration{ + Migration50ScriptKeyType: func(ctx context.Context, + q sqlc.Querier) error { + return testError }, } - // testPostMigrationChecks2 is a map that will trigger a - // migration callback for the latest migration which always - // returns an error. This is used to simulate that the latest - // migration fails and leaves the db in a dirty state. - testPostMigrationChecks2 = map[uint]postMigrationCheck{ - LatestMigrationVersion: func(ctx context.Context, + // testProgrammaticMigrations2 is a map that will trigger a + // migration callback for the targeted migration (i.e. latest) + // which always returns an error. This is used to simulate that + // the latest migration fails and leaves the db in a dirty + // state. + testProgrammaticMigrations2 = map[uint]programmaticMigration{ + Migration51InsertAssetBurns: func(ctx context.Context, q sqlc.Querier) error { return testError @@ -945,20 +977,31 @@ func TestDirtySqliteVersion(t *testing.T) { db1 := NewTestSqliteDBWithVersion(t, 1) // As intend that the failing migration version should be an - // intermediate migration, we use the testPostMigrationChecks1 when + // intermediate migration, we use the testProgrammaticMigrations1 when // executing the migration. Note that we use the `backupAndMigrate` func // as the MigrationTarget, to simulate what is used in production for - // Sqlite database backends. - err = db1.ExecuteMigrations(db1.backupAndMigrate, WithPostStepCallbacks( - makePostStepCallbacks(db1, testPostMigrationChecks1), - )) + // Sqlite database backends. As set the migration version 51 as the + // latest migration version, that version will be targeted. + err = db1.ExecuteMigrations(db1.backupAndMigrate, + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db1, testProgrammaticMigrations1, false, + ), + ), + WithLatestVersion(Migration51InsertAssetBurns), + ) require.ErrorIs(t, err, testError) // If we now attempt to execute migrations again, it should fail with an // error indicating that the db is in a dirty state. - err = db1.ExecuteMigrations(db1.backupAndMigrate, WithPostStepCallbacks( - makePostStepCallbacks(db1, testPostMigrationChecks1), - )) + err = db1.ExecuteMigrations(db1.backupAndMigrate, + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db1, testProgrammaticMigrations1, false, + ), + ), + WithLatestVersion(Migration51InsertAssetBurns), + ) require.ErrorContains(t, err, "database is in a dirty state") // Next, we'll test that if the **latest** migration fails and leaves @@ -967,17 +1010,29 @@ func TestDirtySqliteVersion(t *testing.T) { db2 := NewTestSqliteDBWithVersion(t, 1) // As we intend that the failing migration version should be the latest - // migration, we now use the testPostMigrationChecks2 when executing + // migration, we now use the testProgrammaticMigrations2 when executing // the migration. - err = db2.ExecuteMigrations(db2.backupAndMigrate, WithPostStepCallbacks( - makePostStepCallbacks(db2, testPostMigrationChecks2), - )) + // Note that we're targeting the same version that the programmatic + // migration is defined for, i.e. and is therefore the latest version. + err = db2.ExecuteMigrations(db2.backupAndMigrate, + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db2, testProgrammaticMigrations2, false, + ), + ), + WithLatestVersion(Migration51InsertAssetBurns), + ) require.ErrorIs(t, err, testError) // If we now attempt to execute migrations again, it should fail with an // error indicating that the db is in a dirty state. - err = db2.ExecuteMigrations(db2.backupAndMigrate, WithPostStepCallbacks( - makePostStepCallbacks(db2, testPostMigrationChecks2), - )) + err = db2.ExecuteMigrations(db2.backupAndMigrate, + WithProgrammaticMigrations( + makeProgrammaticMigrations( + db2, testProgrammaticMigrations2, false, + ), + ), + WithLatestVersion(Migration51InsertAssetBurns), + ) require.ErrorContains(t, err, "database is in a dirty state") } diff --git a/tapdb/postgres.go b/tapdb/postgres.go index 0a406046b..2eb0d51f0 100644 --- a/tapdb/postgres.go +++ b/tapdb/postgres.go @@ -132,8 +132,10 @@ func NewPostgresStore(cfg *PostgresConfig) (*PostgresStore, error) { // schemas based on our embedded in-memory file system. if !cfg.SkipMigrations { err := s.ExecuteMigrations( - TargetLatest, WithPostStepCallbacks( - makePostStepCallbacks(s, postMigrationChecks), + TargetLatest, WithProgrammaticMigrations( + makeProgrammaticMigrations( + s, programmaticMigrations, true, + ), ), ) if err != nil { diff --git a/tapdb/post_migration_checks.go b/tapdb/programmatic_migrations.go similarity index 81% rename from tapdb/post_migration_checks.go rename to tapdb/programmatic_migrations.go index 93ce53102..c47e32cd0 100644 --- a/tapdb/post_migration_checks.go +++ b/tapdb/programmatic_migrations.go @@ -29,27 +29,29 @@ const ( Migration51InsertAssetBurns = 51 ) -// postMigrationCheck is a function type for a function that performs a -// post-migration check on the database. -type postMigrationCheck func(context.Context, sqlc.Querier) error +// programmaticMigration is a function type for a function that performs a +// Golang-based migration on the database. +type programmaticMigration func(context.Context, sqlc.Querier) error var ( - // postMigrationChecks is a map of functions that are run after the - // database migration with the version specified in the key has been - // applied. These functions are used to perform additional checks on the + // programmaticMigrations is a map of functions that are run in the + // database migration stream with the version specified in the key. + // These functions are used to perform additional checks on the // database state that are not fully expressible in SQL. - postMigrationChecks = map[uint]postMigrationCheck{ + programmaticMigrations = map[uint]programmaticMigration{ Migration50ScriptKeyType: determineAndAssignScriptKeyType, Migration51InsertAssetBurns: insertAssetBurns, } ) -// makePostStepCallbacks turns the post migration checks into a map of post -// step callbacks that can be used with the migrate package. The keys of the map -// are the migration versions, and the values are the callbacks that will be -// executed after the migration with the corresponding version is applied. -func makePostStepCallbacks(db DatabaseBackend, - checks map[uint]postMigrationCheck) map[uint]migrate.PostStepCallback { +// makeProgrammaticMigrations turns the programmatic migrations into a map of +// ProgrammaticMigrEntry instances that can be used with the migrate package. +// The keys of the map are the migration versions, and the values are the +// programmatic migrations that will be executed as part of the migration with +// the corresponding version. +func makeProgrammaticMigrations(db DatabaseBackend, + migrations map[uint]programmaticMigration, + resetVersionOnError bool) map[uint]migrate.ProgrammaticMigrEntry { var ( ctx = context.Background() @@ -61,22 +63,29 @@ func makePostStepCallbacks(db DatabaseBackend, writeTxOpts AssetStoreTxOptions ) - postStepCallbacks := make(map[uint]migrate.PostStepCallback) - for version, check := range checks { - runCheck := func(m *migrate.Migration, q sqlc.Querier) error { - log.Infof("Running post-migration check for version %d", - version) + programmaticMigrEntries := make(map[uint]migrate.ProgrammaticMigrEntry) + + for version, migration := range migrations { + version := version + migration := migration + + runMigration := func(_ *migrate.Migration, + q sqlc.Querier) error { + + log.Infof("Running programmatic migration "+ + "for version %d", version) start := time.Now() - err := check(ctx, q) + err := migration(ctx, q) if err != nil { - return fmt.Errorf("post-migration "+ - "check failed for version %d: "+ - "%w", version, err) + return fmt.Errorf("programmatic "+ + "migration failed for version "+ + "%d: %w", version, err) } - log.Infof("Post-migration check for version %d "+ - "completed in %v", version, time.Since(start)) + log.Infof("Programmatic migration for version "+ + "%d completed in %v", version, + time.Since(start)) return nil } @@ -85,18 +94,25 @@ func makePostStepCallbacks(db DatabaseBackend, // we use migrate.NewWithInstance() to create the migration // instance from our already instantiated database backend that // is also passed into this function. - postStepCallbacks[version] = func(m *migrate.Migration, - _ database.Driver) error { - - return txDb.ExecTx( - ctx, &writeTxOpts, func(q sqlc.Querier) error { - return runCheck(m, q) + programmaticMigrEntries[version] = + migrate.ProgrammaticMigrEntry{ + ResetVersionOnError: resetVersionOnError, + ProgrammaticMigr: func(m *migrate.Migration, + _ database.Driver) error { + + return txDb.ExecTx( + ctx, &writeTxOpts, + func(q sqlc.Querier) error { + return runMigration( + m, q, + ) + }, + ) }, - ) - } + } } - return postStepCallbacks + return programmaticMigrEntries } // determineAndAssignScriptKeyType attempts to detect the type of the script diff --git a/tapdb/sqlite.go b/tapdb/sqlite.go index e114c5b54..888b64b61 100644 --- a/tapdb/sqlite.go +++ b/tapdb/sqlite.go @@ -149,8 +149,10 @@ func NewSqliteStore(cfg *SqliteConfig) (*SqliteStore, error) { // schemas based on our embedded in-memory file system. if !cfg.SkipMigrations { err := s.ExecuteMigrations( - s.backupAndMigrate, WithPostStepCallbacks( - makePostStepCallbacks(s, postMigrationChecks), + s.backupAndMigrate, WithProgrammaticMigrations( + makeProgrammaticMigrations( + s, programmaticMigrations, true, + ), ), ) if err != nil { @@ -307,8 +309,10 @@ func NewTestSqliteDBWithVersion(t testing.TB, version uint) *SqliteStore { require.NoError(t, err) err = sqlDB.ExecuteMigrations( - TargetVersion(version), WithPostStepCallbacks( - makePostStepCallbacks(sqlDB, postMigrationChecks), + TargetVersion(version), WithProgrammaticMigrations( + makeProgrammaticMigrations( + sqlDB, programmaticMigrations, true, + ), ), ) require.NoError(t, err)