Minor fixes for ibc tendermint (#7916)

* remove nil checks in getter functions for ibc tm header

* remove redundant registration

* add upgrade height version height check

* fix err msg

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

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{},
-	)
 }

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
 }
 

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() {

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())

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() {