diff --git a/x/ibc/light-clients/07-tendermint/types/codec.go b/x/ibc/light-clients/07-tendermint/types/codec.go index f0ff8b2f56..5d876c8fe0 100644 --- a/x/ibc/light-clients/07-tendermint/types/codec.go +++ b/x/ibc/light-clients/07-tendermint/types/codec.go @@ -24,8 +24,4 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { (*exported.Misbehaviour)(nil), &Misbehaviour{}, ) - registry.RegisterImplementations( - (*exported.Header)(nil), - &Header{}, - ) } diff --git a/x/ibc/light-clients/07-tendermint/types/header.go b/x/ibc/light-clients/07-tendermint/types/header.go index e128abea1c..40858c7350 100644 --- a/x/ibc/light-clients/07-tendermint/types/header.go +++ b/x/ibc/light-clients/07-tendermint/types/header.go @@ -30,20 +30,16 @@ func (h Header) ClientType() string { // GetHeight returns the current height. It returns 0 if the tendermint // header is nil. +// NOTE: the header.Header is checked to be non nil in ValidateBasic. func (h Header) GetHeight() exported.Height { - if h.Header == nil { - return clienttypes.ZeroHeight() - } version := clienttypes.ParseChainID(h.Header.ChainID) return clienttypes.NewHeight(version, uint64(h.Header.Height)) } // GetTime returns the current block timestamp. It returns a zero time if // the tendermint header is nil. +// NOTE: the header.Header is checked to be non nil in ValidateBasic. func (h Header) GetTime() time.Time { - if h.Header == nil { - return time.Time{} - } return h.Header.Time } diff --git a/x/ibc/light-clients/07-tendermint/types/header_test.go b/x/ibc/light-clients/07-tendermint/types/header_test.go index cda3047be2..2dbba94b97 100644 --- a/x/ibc/light-clients/07-tendermint/types/header_test.go +++ b/x/ibc/light-clients/07-tendermint/types/header_test.go @@ -13,17 +13,11 @@ import ( func (suite *TendermintTestSuite) TestGetHeight() { header := suite.chainA.LastHeader suite.Require().NotEqual(uint64(0), header.GetHeight()) - - header.Header = nil - suite.Require().Equal(clienttypes.ZeroHeight(), header.GetHeight()) } func (suite *TendermintTestSuite) TestGetTime() { header := suite.chainA.LastHeader suite.Require().NotEqual(time.Time{}, header.GetTime()) - - header.Header = nil - suite.Require().Equal(time.Time{}, header.GetTime()) } func (suite *TendermintTestSuite) TestHeaderValidateBasic() { diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade.go b/x/ibc/light-clients/07-tendermint/types/upgrade.go index 38373f1a95..dcd3cd1a5f 100644 --- a/x/ibc/light-clients/07-tendermint/types/upgrade.go +++ b/x/ibc/light-clients/07-tendermint/types/upgrade.go @@ -18,8 +18,10 @@ import ( // in client state that must be the same across all valid Tendermint clients for the new chain. // VerifyUpgrade will return an error if: // - the upgradedClient is not a Tendermint ClientState +// - the lastest height of the client state does not have the same version number or has a greater +// height than the committed client. // - the height of upgraded client is not greater than that of current client -// - the latest height of the new client does not match the height in committed client +// - the latest height of the new client does not match or is greater than the height in committed client // - any Tendermint chain specified parameter in upgraded client such as ChainID, UnbondingPeriod, // and ProofSpecs do not match parameters set by committed client func (cs ClientState) VerifyUpgradeAndUpdateState( @@ -40,6 +42,13 @@ func (cs ClientState) VerifyUpgradeAndUpdateState( cs.GetLatestHeight().GetVersionNumber(), upgradeHeight.GetVersionNumber()) } + // UpgradeHeight must be greater than or equal to current client state height + if cs.GetLatestHeight().GetVersionHeight() > upgradeHeight.GetVersionHeight() { + return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version height at which upgrade occurs must be greater than or equal to current client height (%d > %d)", + cs.GetLatestHeight().GetVersionHeight(), upgradeHeight.GetVersionHeight(), + ) + } + if !upgradedClient.GetLatestHeight().GT(cs.GetLatestHeight()) { return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be greater than current client height %s", upgradedClient.GetLatestHeight(), cs.GetLatestHeight()) diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade_test.go b/x/ibc/light-clients/07-tendermint/types/upgrade_test.go index 06b5032732..6bffb19408 100644 --- a/x/ibc/light-clients/07-tendermint/types/upgrade_test.go +++ b/x/ibc/light-clients/07-tendermint/types/upgrade_test.go @@ -102,6 +102,32 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { }, expPass: false, }, + { + name: "unsuccessful upgrade: upgrade height version height is less than the current client version height", + setup: func() { + + upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false) + + // upgrade Height is at next block + upgradeHeight = clienttypes.NewHeight(10, uint64(suite.chainB.GetContext().BlockHeight()-1)) + + // zero custom fields and store in upgrade store + suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetVersionHeight()), upgradedClient) + + // commit upgrade store changes and update clients + + suite.coordinator.CommitBlock(suite.chainB) + err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, exported.Tendermint) + suite.Require().NoError(err) + + cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA) + suite.Require().True(found) + + proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight()) + }, + expPass: false, + }, + { name: "unsuccessful upgrade: chain-specified parameters do not match committed client", setup: func() {