diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index cc71a5a5e..f0409a545 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -81,3 +81,4 @@ Special thanks to external contributors on this release: - [light] \#7640 Light Client: fix absence proof verification (@ashcherbakov) - [light] \#7641 Light Client: fix querying against the latest height (@ashcherbakov) - [cli] [#7837](https://github.com/tendermint/tendermint/pull/7837) fix app hash in state rollback. (@yihuang) +- [cli] \#8276 scmigrate: ensure target key is correctly renamed. (@creachadair) diff --git a/go.mod b/go.mod index 3b0004ba0..49db1ba5e 100644 --- a/go.mod +++ b/go.mod @@ -72,6 +72,7 @@ require ( github.com/charithe/durationcheck v0.0.9 // indirect github.com/chavacava/garif v0.0.0-20210405164556-e8a0a408d6af // indirect github.com/containerd/continuity v0.2.1 // indirect + github.com/creachadair/taskgroup v0.3.2 // indirect github.com/daixiang0/gci v0.3.3 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/denis-tingaikin/go-header v0.4.3 // indirect diff --git a/go.sum b/go.sum index 7374c90d0..c0388b7af 100644 --- a/go.sum +++ b/go.sum @@ -223,6 +223,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.0/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsr github.com/cpuguy83/go-md2man/v2 v2.0.1/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= github.com/creachadair/atomicfile v0.2.4 h1:GRjpQLmz/78I4+nBQpGMFrRa9yrL157AUTrA6hnF0YU= github.com/creachadair/atomicfile v0.2.4/go.mod h1:BRq8Une6ckFneYXZQ+kO7p1ZZP3I2fzVzf28JxrIkBc= +github.com/creachadair/taskgroup v0.3.2 h1:zlfutDS+5XG40AOxcHDSThxKzns8Tnr9jnr6VqkYlkM= +github.com/creachadair/taskgroup v0.3.2/go.mod h1:wieWwecHVzsidg2CsUnFinW1faVN4+kq+TDlRJQ0Wbk= github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4= diff --git a/scripts/keymigrate/migrate.go b/scripts/keymigrate/migrate.go index 2061223ad..b53c5e98f 100644 --- a/scripts/keymigrate/migrate.go +++ b/scripts/keymigrate/migrate.go @@ -15,8 +15,8 @@ import ( "math/rand" "runtime" "strconv" - "sync" + "github.com/creachadair/taskgroup" "github.com/google/orderedcode" dbm "github.com/tendermint/tm-db" ) @@ -27,7 +27,7 @@ type ( ) func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { - out := []keyID{} + var out []keyID iter, err := db.Iterator(nil, nil) if err != nil { @@ -43,11 +43,8 @@ func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { continue } - // there's inconsistency around tm-db's handling of - // key copies. - nk := make([]byte, len(k)) - copy(nk, k) - out = append(out, nk) + // Make an explicit copy, since not all tm-db backends do. + out = append(out, []byte(string(k))) } if err = iter.Error(); err != nil { @@ -61,17 +58,6 @@ func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { return out, nil } -func makeKeyChan(keys []keyID) <-chan keyID { - out := make(chan keyID, len(keys)) - defer close(out) - - for _, key := range keys { - out <- key - } - - return out -} - func keyIsLegacy(key keyID) bool { for _, prefix := range []keyID{ // core "store" @@ -111,7 +97,7 @@ func keyIsHash(key keyID) bool { return len(key) == 32 && !bytes.Contains(key, []byte("/")) } -func migarateKey(key keyID) (keyID, error) { +func migrateKey(key keyID) (keyID, error) { switch { case bytes.HasPrefix(key, keyID("H:")): val, err := strconv.Atoi(string(key[2:])) @@ -349,53 +335,23 @@ func Migrate(ctx context.Context, db dbm.DB) error { return err } - numWorkers := runtime.NumCPU() - wg := &sync.WaitGroup{} + var errs []string + g, start := taskgroup.New(func(err error) error { + errs = append(errs, err.Error()) + return err + }).Limit(runtime.NumCPU()) - errs := make(chan error, numWorkers) - - keyCh := makeKeyChan(keys) - - // run migrations. - for i := 0; i < numWorkers; i++ { - wg.Add(1) - go func() { - defer wg.Done() - for key := range keyCh { - err := replaceKey(db, key, migarateKey) - if err != nil { - errs <- err - } - - if ctx.Err() != nil { - return - } + for _, key := range keys { + key := key + start(func() error { + if err := ctx.Err(); err != nil { + return err } - }() + return replaceKey(db, key, migrateKey) + }) } - - // collect and process the errors. - errStrs := []string{} - signal := make(chan struct{}) - go func() { - defer close(signal) - for err := range errs { - if err == nil { - continue - } - errStrs = append(errStrs, err.Error()) - } - }() - - // Wait for everything to be done. - wg.Wait() - close(errs) - <-signal - - // check the error results - if len(errs) != 0 { - return fmt.Errorf("encountered errors during migration: %v", errStrs) + if g.Wait() != nil { + return fmt.Errorf("encountered errors during migration: %q", errs) } - return nil } diff --git a/scripts/keymigrate/migrate_test.go b/scripts/keymigrate/migrate_test.go index 21e9592fb..8f6f30808 100644 --- a/scripts/keymigrate/migrate_test.go +++ b/scripts/keymigrate/migrate_test.go @@ -117,7 +117,7 @@ func TestMigration(t *testing.T) { }) t.Run("Conversion", func(t *testing.T) { for kind, le := range legacyPrefixes { - nk, err := migarateKey(le) + nk, err := migrateKey(le) require.NoError(t, err, kind) require.False(t, keyIsLegacy(nk), kind) } @@ -159,7 +159,7 @@ func TestMigration(t *testing.T) { "UserKey3": []byte("foo/bar/baz/1.2/4"), } for kind, key := range table { - out, err := migarateKey(key) + out, err := migrateKey(key) require.Error(t, err, kind) require.Nil(t, out, kind) } @@ -177,7 +177,7 @@ func TestMigration(t *testing.T) { return nil, errors.New("hi") })) }) - t.Run("KeyDisapears", func(t *testing.T) { + t.Run("KeyDisappears", func(t *testing.T) { db := dbm.NewMemDB() key := keyID("hi") require.NoError(t, db.Set(key, []byte("world"))) @@ -215,17 +215,6 @@ func TestMigration(t *testing.T) { require.False(t, keyIsLegacy(key)) } }) - t.Run("ChannelConversion", func(t *testing.T) { - ch := makeKeyChan([]keyID{ - makeKey(t, "abc", int64(2), int64(42)), - makeKey(t, int64(42)), - }) - count := 0 - for range ch { - count++ - } - require.Equal(t, 2, count) - }) t.Run("Migrate", func(t *testing.T) { _, db := getLegacyDatabase(t) diff --git a/scripts/scmigrate/migrate.go b/scripts/scmigrate/migrate.go index e6eee3d95..ba3de2698 100644 --- a/scripts/scmigrate/migrate.go +++ b/scripts/scmigrate/migrate.go @@ -73,8 +73,6 @@ func sortMigrations(scData []toMigrate) { }) } -func getMigrationsToDelete(in []toMigrate) []toMigrate { return in[1:] } - func getAllSeenCommits(ctx context.Context, db dbm.DB) ([]toMigrate, error) { scKeyPrefix := makeKeyFromPrefix(prefixSeenCommit) iter, err := db.Iterator( @@ -117,6 +115,34 @@ func getAllSeenCommits(ctx context.Context, db dbm.DB) ([]toMigrate, error) { return scData, nil } +func renameRecord(ctx context.Context, db dbm.DB, keep toMigrate) error { + wantKey := makeKeyFromPrefix(prefixSeenCommit) + if bytes.Equal(keep.key, wantKey) { + return nil // we already did this conversion + } + + // This record's key has already been converted to the "new" format, we just + // now need to trim off the tail. + val, err := db.Get(keep.key) + if err != nil { + return err + } + + batch := db.NewBatch() + if err := batch.Delete(keep.key); err != nil { + return err + } + if err := batch.Set(wantKey, val); err != nil { + return err + } + werr := batch.Write() + cerr := batch.Close() + if werr != nil { + return werr + } + return cerr +} + func deleteRecords(ctx context.Context, db dbm.DB, scData []toMigrate) error { // delete all the remaining stale values in a single batch batch := db.NewBatch() @@ -141,20 +167,29 @@ func Migrate(ctx context.Context, db dbm.DB) error { scData, err := getAllSeenCommits(ctx, db) if err != nil { return fmt.Errorf("sourcing tasks to migrate: %w", err) + } else if len(scData) == 0 { + return nil // nothing to do } - // sort earliest->latest commits. + // Sort commits in decreasing order of height. sortMigrations(scData) - // trim the one we want to save: - scData = getMigrationsToDelete(scData) + // Keep and rename the newest seen commit, delete the rest. + // In TM < v0.35 we kept a last-seen commit for each height; in v0.35 we + // retain only the latest. + keep, remove := scData[0], scData[1:] - if len(scData) <= 1 { + if err := renameRecord(ctx, db, keep); err != nil { + return fmt.Errorf("renaming seen commit record: %w", err) + } + + if len(remove) == 0 { return nil } - // write the migration (remove ) - if err := deleteRecords(ctx, db, scData); err != nil { + // Remove any older seen commits. Prior to v0.35, we kept these records for + // all heights, but v0.35 keeps only the latest. + if err := deleteRecords(ctx, db, remove); err != nil { return fmt.Errorf("writing data: %w", err) } diff --git a/scripts/scmigrate/migrate_test.go b/scripts/scmigrate/migrate_test.go index abe12584d..900a15f85 100644 --- a/scripts/scmigrate/migrate_test.go +++ b/scripts/scmigrate/migrate_test.go @@ -1,6 +1,7 @@ package scmigrate import ( + "bytes" "context" "math/rand" "testing" @@ -104,15 +105,6 @@ func TestMigrations(t *testing.T) { assertWellOrderedMigrations(t, testData) }) }) - t.Run("GetMigrationsToDelete", func(t *testing.T) { - for i := 1; i < 100; i++ { - data := appendRandomMigrations([]toMigrate{}, i) - toMigrate := getMigrationsToDelete(data) - if len(data) != len(toMigrate)+1 { - t.Fatalf("migration prep did not save one document [original=%d migrations=%d]", len(data), len(toMigrate)) - } - } - }) t.Run("InvalidMigrations", func(t *testing.T) { if _, err := makeToMigrate(nil); err == nil { t.Fatal("should error for nil migrations") @@ -156,18 +148,24 @@ func TestMigrations(t *testing.T) { // safe to rerun t.Run(test, func(t *testing.T) { if err := Migrate(ctx, db); err != nil { - t.Fatal(err) + t.Fatalf("Migration failed: %v", err) } post, err := getAllSeenCommits(ctx, db) if err != nil { - t.Fatal(err) + t.Fatalf("Fetching seen commits: %v", err) } + if len(post) != 1 { - t.Fatal("migration was not successful") + t.Fatalf("Wrong number of commits: got %d, wanted 1", len(post)) } - if post[0].commit.Height != latestHeight { - t.Fatal("migration did not save correct document") + + wantKey := makeKeyFromPrefix(prefixSeenCommit) + if !bytes.Equal(post[0].key, wantKey) { + t.Errorf("Seen commit key: got %x, want %x", post[0].key, wantKey) + } + if got := post[0].commit.Height; got != latestHeight { + t.Fatalf("Wrong commit height after migration: got %d, wanted %d", got, latestHeight) } }) }