scmigrate: ensure target key is correctly renamed (#8276)

Prior to v0.35, the keys for seen-commit records included the applicable
height.  In v0.35 and beyond, we only keep the record for the latest height,
and its key does not include the height.

Update the seen-commit migration to ensure that the record we retain after
migration is correctly renamed to omit the height from its key.

Update the test cases to check for this condition after migrating.

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)

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

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=

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
 }

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)
 

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

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