From c153b23ca0c82a0324c026746d5b9f8333298a26 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 17 Aug 2023 00:28:12 +0300 Subject: [PATCH] Move check for upgraded client state height to 02-client layer. --- modules/core/02-client/keeper/client.go | 9 ++++++ modules/core/02-client/keeper/client_test.go | 29 +++++++++++++++++++ .../light-clients/07-tendermint/upgrade.go | 12 ++------ 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 34b8ff8c528f..2cfe42d4641a 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -9,6 +9,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" + ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -128,6 +129,14 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e return errorsmod.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status) } + // last height of current counterparty chain must be client's latest height + lastHeight := clientState.GetLatestHeight() + + if !upgradedClient.GetLatestHeight().GT(lastHeight) { + return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", + upgradedClient.GetLatestHeight(), lastHeight) + } + if err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore, upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState, ); err != nil { diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 99dcd8616471..071d399b73e9 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -437,6 +437,35 @@ func (suite *KeeperTestSuite) TestUpgradeClient() { }, expPass: false, }, + { + name: "unsuccessful upgrade: upgraded height is not greater than current height", + setup: func() { + // last Height is at next block + lastHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1)) + + // zero custom fields and store in upgrade store + err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedClientBz) + suite.Require().NoError(err) + err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedConsStateBz) + suite.Require().NoError(err) + + // change upgradedClient client-specified parameters + tmClient := upgradedClient.(*ibctm.ClientState) + tmClient.LatestHeight = clienttypes.NewHeight(0, 1) + upgradedClient = tmClient + + suite.coordinator.CommitBlock(suite.chainB) + err = path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID) + suite.Require().True(found) + + proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetRevisionHeight())), cs.GetLatestHeight().GetRevisionHeight()) + proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetRevisionHeight())), cs.GetLatestHeight().GetRevisionHeight()) + }, + expPass: false, + }, } for _, tc := range testCases { diff --git a/modules/light-clients/07-tendermint/upgrade.go b/modules/light-clients/07-tendermint/upgrade.go index bd91ac438d84..efa377ac2930 100644 --- a/modules/light-clients/07-tendermint/upgrade.go +++ b/modules/light-clients/07-tendermint/upgrade.go @@ -12,7 +12,6 @@ import ( clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" commitmenttypes "github.com/cosmos/ibc-go/v7/modules/core/23-commitment/types" - ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -36,14 +35,6 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( return errorsmod.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set") } - // last height of current counterparty chain must be client's latest height - lastHeight := cs.GetLatestHeight() - - if !upgradedClient.GetLatestHeight().GT(lastHeight) { - return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s", - upgradedClient.GetLatestHeight(), lastHeight) - } - // upgraded client state and consensus state must be IBC tendermint client state and consensus state // this may be modified in the future to upgrade to a new IBC tendermint type // counterparty must also commit to the upgraded consensus state at a sub-path under the upgrade path specified @@ -67,6 +58,9 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( return errorsmod.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal consensus state merkle proof: %v", err) } + // last height of current counterparty chain must be client's latest height + lastHeight := cs.GetLatestHeight() + // Must prove against latest consensus state to ensure we are verifying against latest upgrade plan // This verifies that upgrade is intended for the provided revision, since committed client must exist // at this consensus state