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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a1d5b33
ensure latest height revision number matches chain id revision number
colin-axner Jul 7, 2021
08b9af4
add changelog
colin-axner Jul 7, 2021
84e341f
Update modules/light-clients/07-tendermint/types/client_state_test.go
colin-axner Jul 7, 2021
49bfa8a
Update modules/light-clients/07-tendermint/types/client_state_test.go
colin-axner Jul 7, 2021
53ad6f4
address review suggestions
colin-axner Jul 7, 2021
2d070b6
Merge branch 'colin/240-client-validate' of github.com:cosmos/ibc-go …
colin-axner Jul 7, 2021
1be3f11
Merge branch 'main' into colin/240-client-validate
colin-axner Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions modules/core/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,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-1", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you create the variable for these tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, there was only 2 of them so didn't feel as necessary

// Call ZeroCustomFields on upgraded clients to clear any client-chosen parameters in test-case upgradedClient
upgradedClient = upgradedClient.ZeroCustomFields()

Expand Down Expand Up @@ -698,7 +698,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-1", 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
View file
Open in desktop
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, "latest height revision height cannot be zero")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this should remain. No tendermint height should start at 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add back the tendermint part but the latest height makes things more specific since the frozen height also contains a revision height

}
if cs.TrustingPeriod >= cs.UnbondingPeriod {
return sdkerrors.Wrapf(
Expand Down
8 changes: 7 additions & 1 deletion modules/light-clients/07-tendermint/types/client_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,13 @@ 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,
},

colin-axner marked this conversation as resolved.
Show resolved Hide resolved
{
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
View file
Open in desktop
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