ensure latest height revision number matches chain id revision number (…

…#241) * ensure latest height revision number matches chain id revision number fix tests as well * add changelog * Update modules/light-clients/07-tendermint/types/client_state_test.go * Update modules/light-clients/07-tendermint/types/client_state_test.go * address review suggestions
cosmos · Jul 7, 2021 · bc5c659 · bc5c659
1 parent 5d0cb6c
commit bc5c659
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (07-tendermint) [\#241](https://github.com/cosmos/ibc-go/pull/241) Ensure tendermint client state latest height revision number matches chain id revision number.
 * (07-tendermint) [\#234](https://github.com/cosmos/ibc-go/pull/234) Use sentinel value for the consensus state root set during a client upgrade. This prevents genesis validation from failing.
 * (modules) [\#223](https://github.com/cosmos/ibc-go/pull/223) Use correct Prometheus format for metric labels.
 * (06-solomachine) [\#214](https://github.com/cosmos/ibc-go/pull/214) Disable defensive timestamp check in SendPacket for solo machine clients.

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
@@ -394,7 +394,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
  tc := tc
  path = ibctesting.NewPath(suite.chainA, suite.chainB)
  suite.coordinator.SetupClients(path)
- upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+ upgradedClient = ibctmtypes.NewClientState("newChainId-1", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
  upgradedClient = upgradedClient.ZeroCustomFields()
  upgradedClientBz, err = types.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
  suite.Require().NoError(err)

diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go
@@ -647,6 +647,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
  )
 
  newClientHeight := clienttypes.NewHeight(1, 1)
+ newChainId := "newChainId-1"
 
  cases := []struct {
  name string
@@ -657,7 +658,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
  name: "successful upgrade",
  setup: func() {
 
- upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+ upgradedClient = ibctmtypes.NewClientState(newChainId, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
  // Call ZeroCustomFields on upgraded clients to clear any client-chosen parameters in test-case upgradedClient
  upgradedClient = upgradedClient.ZeroCustomFields()
 
@@ -698,7 +699,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
  name: "VerifyUpgrade fails",
  setup: func() {
 
- upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
+ upgradedClient = ibctmtypes.NewClientState(newChainId, ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
  // Call ZeroCustomFields on upgraded clients to clear any client-chosen parameters in test-case upgradedClient
  upgradedClient = upgradedClient.ZeroCustomFields()
 

diff --git a/modules/light-clients/07-tendermint/types/client_state.go b/modules/light-clients/07-tendermint/types/client_state.go
@@ -122,8 +122,14 @@ func (cs ClientState) Validate() error {
  if cs.MaxClockDrift == 0 {
  return sdkerrors.Wrap(ErrInvalidMaxClockDrift, "max clock drift cannot be zero")
  }
+
+ // the latest height revision number must match the chain id revision number
+ if cs.LatestHeight.RevisionNumber != clienttypes.ParseChainID(cs.ChainId) {
+ return sdkerrors.Wrapf(ErrInvalidHeaderHeight,
+ "latest height revision number must match chain id revision number (%d != %d)", cs.LatestHeight.RevisionNumber, clienttypes.ParseChainID(cs.ChainId))
+ }
  if cs.LatestHeight.RevisionHeight == 0 {
- return sdkerrors.Wrapf(ErrInvalidHeaderHeight, "tendermint revision height cannot be zero")
+ return sdkerrors.Wrapf(ErrInvalidHeaderHeight, "tendermint client's latest height revision height cannot be zero")
  }
  if cs.TrustingPeriod >= cs.UnbondingPeriod {
  return sdkerrors.Wrapf(

diff --git a/modules/light-clients/07-tendermint/types/client_state_test.go b/modules/light-clients/07-tendermint/types/client_state_test.go
@@ -129,7 +129,12 @@ func (suite *TendermintTestSuite) TestValidate() {
  expPass: false,
  },
  {
- name: "invalid height",
+ name: "invalid revision number",
+ clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.NewHeight(1, 1), commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
+ expPass: false,
+ },
+ {
+ name: "invalid revision height",
  clientState: types.NewClientState(chainID, types.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, clienttypes.ZeroHeight(), commitmenttypes.GetSDKSpecs(), upgradePath, false, false),
  expPass: false,
  },

diff --git a/modules/light-clients/07-tendermint/types/upgrade_test.go b/modules/light-clients/07-tendermint/types/upgrade_test.go
@@ -10,6 +10,10 @@ import (
  ibctesting "github.com/cosmos/ibc-go/testing"
 )
 
+var (
+ newChainId = "newChainId-1"
+)
+
 func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  var (
  upgradedClient exported.ClientState
@@ -54,6 +58,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  name: "successful upgrade to same revision",
  setup: func() {
  upgradedHeight := clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+2))
+ // don't use -1 suffix in chain id
  upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradedHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
  upgradedClient = upgradedClient.ZeroCustomFields()
  upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
@@ -109,7 +114,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  name: "unsuccessful upgrade: committed client does not have zeroed custom fields",
  setup: func() {
  // non-zeroed upgrade client
- upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+ upgradedClient = types.NewClientState(newChainId, types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
  upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
  suite.Require().NoError(err)
 
@@ -167,7 +172,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedConsStateBz)
 
  // change upgradedClient client-specified parameters
- upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, lastHeight, commitmenttypes.GetSDKSpecs(), upgradePath, true, false)
+ upgradedClient = types.NewClientState(newChainId, types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, lastHeight, commitmenttypes.GetSDKSpecs(), upgradePath, true, false)
 
  suite.coordinator.CommitBlock(suite.chainB)
  err := path.EndpointA.UpdateClient()
@@ -398,7 +403,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  name: "unsuccessful upgrade: final client is not valid",
  setup: func() {
  // new client has smaller unbonding period such that old trusting period is no longer valid
- upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+ upgradedClient = types.NewClientState(newChainId, types.DefaultTrustLevel, trustingPeriod, trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
  upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
  suite.Require().NoError(err)
 
@@ -433,7 +438,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
  path = ibctesting.NewPath(suite.chainA, suite.chainB)
 
  suite.coordinator.SetupClients(path)
- upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+ upgradedClient = types.NewClientState(newChainId, types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
  upgradedClient = upgradedClient.ZeroCustomFields()
  upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient)
  suite.Require().NoError(err)