fix(halo/app): dump old height (#1976)

When dupmping upgrade info the disk in the "old binary" use-case, ensure
that we set the original upgrade height, not current. This would have
caused the new binary to re-run store upgrade loaders.

issue: #1834

halo/app/app.go

@@ -169,16 +169,13 @@ func newApp(
 		app.ModuleManager.OrderEndBlockers = endBlockers
 		app.SetEndBlocker(app.EndBlocker)
 
-		// Also dump upgrade info to disk if binary is too old.
-		// Cosmos upgrade only dumps when doing the upgrade.
+		// Wrap upgrade module preblocker to dump upgrade info to disk if binary is too old.
+		// By default, it only dumps when doing the upgrade.
 		app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 			resp, err := app.PreBlocker(ctx, req)
-			if lastAppliedUpgrade, ok := isErrWrongVersion(err); ok {
-				height := sdk.UnwrapSDKContext(ctx).BlockHeight()
-				err := app.UpgradeKeeper.DumpUpgradeInfoToDisk(height, utypes.Plan{Name: lastAppliedUpgrade})
-				if err != nil {
-					// Best effort just log
-					log.Warn(ctx, "Failed writing upgrade info", err)
+			if isErrOldBinary(err) {
+				if err := dumpLastAppliedUpgradeInfo(ctx, app.UpgradeKeeper); err != nil {
+					log.Error(ctx, "Failed writing last applied upgrade info", err)
 				}
 			}
 
@@ -222,10 +219,40 @@ func (a App) ClientContext(ctx context.Context) client.Context {
 		WithCodec(a.appCodec)
 }
 
-// isErrWrongVersion returns the last applied upgrade if the error is due to a wrong app version, else it returns false.
-func isErrWrongVersion(err error) (string, bool) {
+// dumpLastAppliedUpgradeInfo dumps the last applied upgrade info to disk.
+// This is a workaround for halovisor to auto upgrade binaries
+// after snapsyncing to a post-upgrade state using a pre-upgrade (old) binary.
+func dumpLastAppliedUpgradeInfo(ctx sdk.Context, keeper *upgradekeeper.Keeper) error {
+	name, height, err := keeper.GetLastCompletedUpgrade(ctx)
+	if err != nil {
+		return errors.Wrap(err, "get last completed upgrade")
+	}
+
+	// Note that we need to ensure that the next binary doesn't actually run any
+	// store loader upgrades on startup, it was already done during the upgrade.
+	// We therefore ensure height isn't current.
+
+	current := sdk.UnwrapSDKContext(ctx).BlockHeight()
+	if height >= current { // Sanity check that the upgrade was in the past.
+		return errors.New("unexpected last upgrade height [BUG]")
+	}
+
+	err = keeper.DumpUpgradeInfoToDisk(height, utypes.Plan{
+		Name:   name,
+		Height: height,
+	})
+	if err != nil {
+		return errors.Wrap(err, "dump upgrade info")
+	}
+
+	return nil
+}
+
+// isErrOldBinary returns the true if the error is due to the
+// upgrade module detecting the binary is too old.
+func isErrOldBinary(err error) bool {
 	if err == nil {
-		return "", false
+		return false
 	}
 
 	// Get the root cause of the error.
@@ -238,17 +265,17 @@ func isErrWrongVersion(err error) (string, bool) {
 		cause = next
 	}
 
-	var ignore int
-	var lastAppliedUpgrade string
+	var d int
+	var s string
 	i, err := fmt.Fscanf(
 		strings.NewReader(cause.Error()),
 		"wrong app version %d, upgrade handler is missing for %s upgrade plan",
-		&ignore, &lastAppliedUpgrade)
+		&d, &s)
 	if err != nil || i != 2 {
-		return "", false
+		return false
 	}
 
-	return lastAppliedUpgrade, true
+	return true
 }
 
 var (

halo/app/app_internal_test.go

@@ -11,13 +11,12 @@ import (
 )
 
 //nolint:forbidigo // We use cosmos errors explicitly.
-func TestIsErrWrongVersion(t *testing.T) {
+func TestIsErrOldBinary(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
-		name               string
-		err                error
-		want               bool
-		lastAppliedUpgrade string
+		name string
+		err  error
+		want bool
 	}{
 		{
 			name: "nil error",
@@ -30,27 +29,24 @@ func TestIsErrWrongVersion(t *testing.T) {
 			want: false,
 		},
 		{
-			name:               "wrong version error",
-			err:                fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", 99, "test"),
-			want:               true,
-			lastAppliedUpgrade: "test",
+			name: "wrong version error",
+			err:  fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", 99, "test"),
+			want: true,
 		},
 		{
 			name: "wrapped wrong version error",
 			err: errors.Wrap(
 				fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", 98, "wrapped"),
 				"wrapper"),
-			want:               true,
-			lastAppliedUpgrade: "wrapped",
+			want: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			lastAppliedUpgrade, ok := isErrWrongVersion(tt.err)
+			ok := isErrOldBinary(tt.err)
 			require.Equal(t, tt.want, ok)
-			require.Equal(t, tt.lastAppliedUpgrade, lastAppliedUpgrade)
 		})
 	}
 }