Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure latest height revision number matches chain id revision number #241

Merged
merged 7 commits into from
Jul 7, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,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.
Expand Down
2 changes: 1 addition & 1 deletion modules/core/02-client/keeper/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
)

newClientHeight := clienttypes.NewHeight(1, 1)
newChainId := "newChainId-1"

cases := []struct {
name string
Expand All @@ -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()

Expand Down Expand Up @@ -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()

Expand Down
8 changes: 7 additions & 1 deletion modules/light-clients/07-tendermint/types/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down
13 changes: 9 additions & 4 deletions modules/light-clients/07-tendermint/types/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import (
ibctesting "github.com/cosmos/ibc-go/testing"
)

var (
newChainId = "newChainId-1"
)

func (suite *TendermintTestSuite) TestVerifyUpgrade() {
var (
upgradedClient exported.ClientState
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down