Fix IBC upgrade: manually tested (#8187)

* commit at plan.Height key

* fix upgrade tests

* remove debugging prints

* Update x/ibc/light-clients/07-tendermint/types/upgrade.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

x/ibc/light-clients/07-tendermint/types/upgrade.go

@@ -83,7 +83,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	// construct clientState Merkle path
 	upgradeClientPath := constructUpgradeClientMerklePath(cs.UpgradePath, lastHeight)
 	if err := merkleProofClient.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeClientPath, bz); err != nil {
-		return nil, nil, err
+		return nil, nil, sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty())
 	}
 
 	// Verify consensus state proof
@@ -94,7 +94,7 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 	// construct consensus state Merkle path
 	upgradeConsStatePath := constructUpgradeConsStateMerklePath(cs.UpgradePath, lastHeight)
 	if err := merkleProofConsState.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeConsStatePath, bz); err != nil {
-		return nil, nil, err
+		return nil, nil, sdkerrors.Wrapf(err, "consensus state proof failed. Path: %s", upgradeConsStatePath.Pretty())
 	}
 
 	// Construct new client state and consensus state

x/upgrade/abci.go

@@ -41,7 +41,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
 			Timestamp:          ctx.BlockTime(),
 			NextValidatorsHash: ctx.BlockHeader().NextValidatorsHash,
 		}
-		k.SetUpgradedConsensusState(ctx, ctx.BlockHeight(), upgradedConsState)
+		k.SetUpgradedConsensusState(ctx, plan.Height, upgradedConsState)
 	}
 	// To make sure clear upgrade is executed at the same block
 	if plan.ShouldExecute(ctx) {

x/upgrade/abci_test.go

@@ -131,7 +131,8 @@ func VerifyDoIBCLastBlock(t *testing.T) {
 	req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
 	s.module.BeginBlock(newCtx, req)
 
-	consState, err := s.keeper.GetUpgradedConsensusState(newCtx, s.ctx.BlockHeight())
+	// plan Height is at ctx.BlockHeight+1
+	consState, err := s.keeper.GetUpgradedConsensusState(newCtx, s.ctx.BlockHeight()+1)
 	require.NoError(t, err)
 	require.Equal(t, &ibctmtypes.ConsensusState{Timestamp: newCtx.BlockTime(), NextValidatorsHash: nextValsHash}, consState)
 }

x/upgrade/keeper/keeper.go

@@ -89,15 +89,15 @@ func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error {
 		if err != nil {
 			return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "could not unpack clientstate: %v", err)
 		}
-		// sets the new upgraded client in last height committed on this chain is at plan.Height - 1,
+		// sets the new upgraded client in last height committed on this chain is at plan.Height,
 		// since the chain will panic at plan.Height and new chain will resume at plan.Height
-		return k.SetUpgradedClient(ctx, plan.Height-1, clientState)
+		return k.SetUpgradedClient(ctx, plan.Height, clientState)
 	}
 	return nil
 }
 
 // SetUpgradedClient sets the expected upgraded client for the next version of this chain at the last height the current chain will commit.
-func (k Keeper) SetUpgradedClient(ctx sdk.Context, lastHeight int64, cs ibcexported.ClientState) error {
+func (k Keeper) SetUpgradedClient(ctx sdk.Context, planHeight int64, cs ibcexported.ClientState) error {
 	store := ctx.KVStore(k.storeKey)
 
 	// zero out any custom fields before setting
@@ -107,7 +107,7 @@ func (k Keeper) SetUpgradedClient(ctx sdk.Context, lastHeight int64, cs ibcexpor
 		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "could not marshal clientstate: %v", err)
 	}
 
-	store.Set(types.UpgradedClientKey(lastHeight), bz)
+	store.Set(types.UpgradedClientKey(planHeight), bz)
 	return nil
 }
 
@@ -129,14 +129,14 @@ func (k Keeper) GetUpgradedClient(ctx sdk.Context, height int64) (ibcexported.Cl
 
 // SetUpgradedConsensusState set the expected upgraded consensus state for the next version of this chain
 // using the last height committed on this chain.
-func (k Keeper) SetUpgradedConsensusState(ctx sdk.Context, lastHeight int64, cs ibcexported.ConsensusState) error {
+func (k Keeper) SetUpgradedConsensusState(ctx sdk.Context, planHeight int64, cs ibcexported.ConsensusState) error {
 	store := ctx.KVStore(k.storeKey)
 	bz, err := clienttypes.MarshalConsensusState(k.cdc, cs)
 	if err != nil {
 		return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "could not marshal consensus state: %v", err)
 	}
 
-	store.Set(types.UpgradedConsStateKey(lastHeight), bz)
+	store.Set(types.UpgradedConsStateKey(planHeight), bz)
 	return nil
 }
 

x/upgrade/keeper/keeper_test.go

@@ -161,7 +161,6 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
 			},
 			expPass: true,
 		},
-
 		{
 			name: "unsuccessful schedule: invalid plan",
 			plan: types.Plan{
@@ -235,12 +234,12 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
 			if tc.expPass {
 				s.Require().NoError(err, "valid test case failed")
 				if tc.plan.UpgradedClientState != nil {
-					got, err := s.app.UpgradeKeeper.GetUpgradedClient(s.ctx, tc.plan.Height-1)
+					got, err := s.app.UpgradeKeeper.GetUpgradedClient(s.ctx, tc.plan.Height)
 					s.Require().NoError(err)
 					s.Require().Equal(clientState, got, "upgradedClient not equal to expected value")
 				} else {
 					// check that upgraded client is empty if latest plan does not specify an upgraded client
-					got, err := s.app.UpgradeKeeper.GetUpgradedClient(s.ctx, tc.plan.Height-1)
+					got, err := s.app.UpgradeKeeper.GetUpgradedClient(s.ctx, tc.plan.Height)
 					s.Require().Error(err)
 					s.Require().Nil(got)
 				}