fix: make downgrade verification work again (#13936)

CHANGELOG.md

@@ -181,6 +181,7 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf
 
 ### Bug Fixes
 
+* (x/upgrade) [#13936](https://github.com/cosmos/cosmos-sdk/pull/13936) Make downgrade verification work again
 * (x/group) [#13742](https://github.com/cosmos/cosmos-sdk/pull/13742) Fix `validate-genesis` when group policy accounts exist.
 * (x/auth) [#13838](https://github.com/cosmos/cosmos-sdk/pull/13838) Fix calling `String()` and `MarshalYAML` panics when pubkey is set on a `BaseAccount`. 
 * (rosetta) [#13583](https://github.com/cosmos/cosmos-sdk/pull/13583) Misc fixes for cosmos-rosetta.

x/upgrade/abci.go

@@ -25,6 +25,28 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
 
 	plan, found := k.GetUpgradePlan(ctx)
 
+	if !k.DowngradeVerified() {
+		k.SetDowngradeVerified(true)
+		// This check will make sure that we are using a valid binary.
+		// It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade.
+		// 1. If there is no scheduled upgrade.
+		// 2. If the plan is not ready.
+		// 3. If the plan is ready and skip upgrade height is set for current height.
+		if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) {
+			lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx)
+			if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) {
+				var appVersion uint64
+
+				cp := ctx.ConsensusParams()
+				if cp != nil && cp.Version != nil {
+					appVersion = cp.Version.App
+				}
+
+				panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan))
+			}
+		}
+	}
+
 	if !found {
 		return
 	}
@@ -70,28 +92,6 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
 		ctx.Logger().Error(downgradeMsg)
 		panic(downgradeMsg)
 	}
-
-	if !k.DowngradeVerified() {
-		k.SetDowngradeVerified(true)
-		lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx)
-		// This check will make sure that we are using a valid binary.
-		// It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade.
-		// 1. If there is no scheduled upgrade.
-		// 2. If the plan is not ready.
-		// 3. If the plan is ready and skip upgrade height is set for current height.
-		if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) {
-			if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) {
-				var appVersion uint64
-
-				cp := ctx.ConsensusParams()
-				if cp != nil && cp.Version != nil {
-					appVersion = cp.Version.App
-				}
-
-				panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan))
-			}
-		}
-	}
 }
 
 // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed.

x/upgrade/abci_test.go

@@ -483,3 +483,91 @@ func TestBinaryVersion(t *testing.T) {
 		}
 	}
 }
+
+func TestDowngradeVerification(t *testing.T) {
+	// could not use setupTest() here, because we have to use the same key
+	// for the two keepers.
+	encCfg := moduletestutil.MakeTestEncodingConfig(upgrade.AppModuleBasic{})
+	key := sdk.NewKVStoreKey(types.StoreKey)
+	testCtx := testutil.DefaultContextWithDB(s.T(), key, sdk.NewTransientStoreKey("transient_test"))
+	ctx := testCtx.Ctx.WithBlockHeader(tmproto.Header{Time: time.Now(), Height: 10})
+
+	skip := map[int64]bool{}
+	tempDir := t.TempDir()
+	k := keeper.NewKeeper(skip, key, encCfg.Codec, tempDir, nil, authtypes.NewModuleAddress(govtypes.ModuleName).String())
+	m := upgrade.NewAppModule(k)
+	handler := upgrade.NewSoftwareUpgradeProposalHandler(k)
+
+	// submit a plan.
+	planName := "downgrade"
+	err := handler(ctx, &types.SoftwareUpgradeProposal{Title: "test", Plan: types.Plan{Name: planName, Height: ctx.BlockHeight() + 1}})
+	require.NoError(t, err)
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
+
+	// set the handler.
+	k.SetUpgradeHandler(planName, func(ctx sdk.Context, plan types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+		return vm, nil
+	})
+
+	// successful upgrade.
+	req := abci.RequestBeginBlock{Header: ctx.BlockHeader()}
+	require.NotPanics(t, func() {
+		m.BeginBlock(ctx, req)
+	})
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
+
+	testCases := map[string]struct{
+		preRun      func(keeper.Keeper, sdk.Context, string)
+		expectPanic bool
+	}{
+		"valid binary": {
+			preRun: func(k keeper.Keeper, ctx sdk.Context, name string) {
+				k.SetUpgradeHandler(planName, func(ctx sdk.Context, plan types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+					return vm, nil
+				})
+			},
+		},
+		"downgrade with an active plan": {
+			preRun: func(k keeper.Keeper, ctx sdk.Context, name string) {
+				handler := upgrade.NewSoftwareUpgradeProposalHandler(k)
+				err := handler(ctx, &types.SoftwareUpgradeProposal{Title: "test", Plan: types.Plan{Name: "another" + planName, Height: ctx.BlockHeight() + 1}})
+				require.NoError(t, err, name)
+			},
+			expectPanic: true,
+		},
+		"downgrade without any active plan": {
+			expectPanic: true,
+		},
+	}
+
+	for name, tc := range testCases {
+		ctx, _ := ctx.CacheContext()
+
+		// downgrade. now keeper does not have the handler.
+		k := keeper.NewKeeper(skip, key, encCfg.Codec, tempDir, nil, authtypes.NewModuleAddress(govtypes.ModuleName).String())
+		m := upgrade.NewAppModule(k)
+
+		// assertions
+		lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx)
+		require.Equal(t, planName, lastAppliedPlan)
+		require.False(t, k.HasHandler(planName))
+		require.False(t, k.DowngradeVerified())
+		_, found := k.GetUpgradePlan(ctx)
+		require.False(t, found)
+
+		if tc.preRun != nil {
+			tc.preRun(k, ctx, name)
+		}
+
+		req := abci.RequestBeginBlock{Header: ctx.BlockHeader()}
+		if tc.expectPanic {
+			require.Panics(t, func() {
+				m.BeginBlock(ctx, req)
+			}, name)
+		} else {
+			require.NotPanics(t, func() {
+				m.BeginBlock(ctx, req)
+			}, name)
+		}
+	}
+}