From 731be713797d41204ab2f83b6cb706f4ad63d90c Mon Sep 17 00:00:00 2001 From: Aditya Date: Thu, 17 Dec 2020 17:32:19 +0000 Subject: [PATCH] Fix IBC upgrade: manually tested (#8187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 4 ++-- x/upgrade/abci.go | 2 +- x/upgrade/abci_test.go | 3 ++- x/upgrade/keeper/keeper.go | 12 ++++++------ x/upgrade/keeper/keeper_test.go | 5 ++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade.go b/x/ibc/light-clients/07-tendermint/types/upgrade.go index b75af53a80..397e9cfd83 100644 --- a/x/ibc/light-clients/07-tendermint/types/upgrade.go +++ b/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 diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 9ca8f1a3f9..fa95c4c4aa 100644 --- a/x/upgrade/abci.go +++ b/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) { diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 20ca573d13..caa5e5a3c9 100644 --- a/x/upgrade/abci_test.go +++ b/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) } diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 05fe6bd0f7..0cf619cf36 100644 --- a/x/upgrade/keeper/keeper.go +++ b/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 } diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index f22edc00d6..6e91ef3c23 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/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) }