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

Upgrade-Client Followup #2 #7460

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Upgrade-Client Followup #2 #7460

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15b8dcc
enforce client-chosen parameters are persisted across upgrades in ten…
AdityaSripal Oct 5, 2020
b3c1dfd
Merge branch 'master' into aditya/upgrade-followup-2
AdityaSripal Oct 5, 2020
153a464
Update x/ibc/light-clients/07-tendermint/types/upgrade.go
colin-axner Oct 6, 2020
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
29 changes: 0 additions & 29 deletions x/ibc/core/02-client/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
Expand Down Expand Up @@ -119,34 +118,6 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
},
expPass: true,
},
{
name: "successful upgrade to different client type",
setup: func() {

// previous chain committed to the change
upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.chainA.App.AppCodec(), clientA, "diversifier", 1).ClientState()
soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
// change sequence to be higher height than latest current client height
soloClient.Sequence = 100000000000
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), soloClient)

// commit upgrade store changes and update clients

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: true,
},
{
name: "invalid upgrade: msg.ClientState does not contain valid clientstate",
setup: func() {
Expand Down
29 changes: 1 addition & 28 deletions x/ibc/core/02-client/keeper/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types"
commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
localhosttypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/09-localhost/types"
ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
Expand Down Expand Up @@ -248,7 +247,7 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
expPass bool
}{
{
name: "successful upgrade to a new tendermint client",
name: "successful upgrade",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
Expand All @@ -268,32 +267,6 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
},
expPass: true,
},
{
name: "successful upgrade to a solomachine client",
setup: func() {
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
// previous chain committed to the change
upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
// change sequence to be higher height than latest current client height
soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())
},
expPass: true,
},
{
name: "client state not found",
setup: func() {
Expand Down
27 changes: 27 additions & 0 deletions x/ibc/light-clients/07-tendermint/types/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package types

import (
"reflect"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand All @@ -25,6 +27,11 @@ func (cs ClientState) VerifyUpgrade(
if cs.UpgradePath == nil {
return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
}
tmClient, ok := upgradedClient.(*ClientState)
if !ok {
return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
&ClientState{}, upgradedClient)
}

if !upgradedClient.GetLatestHeight().GT(cs.GetLatestHeight()) {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgrade client height %s must be greater than current client height %s",
Expand Down Expand Up @@ -54,5 +61,25 @@ func (cs ClientState) VerifyUpgrade(
return sdkerrors.Wrap(err, "could not retrieve latest consensus state")
}

tmCommittedClient, ok := committedClient.(*ClientState)
if !ok {
return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T",
&ClientState{}, upgradedClient)
}

// Relayer must keep all client-chosen parameters the same as the previous client
// compare relayer-provided client state against expected client state
// all chain-chosen parameters come from committed client, all client-chosen parameters
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
// come from current client
expectedClient := NewClientState(
tmCommittedClient.ChainId, cs.TrustLevel, cs.TrustingPeriod, tmCommittedClient.UnbondingPeriod,
cs.MaxClockDrift, tmCommittedClient.LatestHeight, tmCommittedClient.ProofSpecs, tmCommittedClient.UpgradePath,
cs.AllowUpdateAfterExpiry, cs.AllowUpdateAfterMisbehaviour,
)
if !reflect.DeepEqual(expectedClient, tmClient) {
return sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "upgraded client does not maintain previous chosen parameters. expected: %v got: %v",
expectedClient, tmClient)
}

return merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), *cs.UpgradePath, bz)
}
102 changes: 25 additions & 77 deletions x/ibc/light-clients/07-tendermint/types/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package types_test

import (
"fmt"

commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/23-commitment/types"
"github.com/cosmos/cosmos-sdk/x/ibc/core/exported"
solomachinetypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/06-solomachine/types"
"github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
Expand All @@ -22,7 +23,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
expPass bool
}{
{
name: "successful upgrade to a new tendermint client",
name: "successful upgrade",
setup: func() {

upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
Expand All @@ -43,15 +44,15 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
expPass: true,
},
{
name: "successful upgrade to a new tendermint client with different client chosen parameters",
name: "successful upgrade with different client chosen parameters",
setup: func() {

upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)

// change upgradedClient client-specified parameters
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, true)
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
Expand All @@ -60,67 +61,17 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
},
expPass: true,
},
{
name: "successful upgrade to a solomachine client",
setup: func() {
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
// previous chain committed to the change
upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
// change sequence to be higher height than latest current client height
soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.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(), cs.GetLatestHeight().GetEpochHeight())
},
expPass: true,
},
{
name: "successful upgrade to a solomachine client with different client-chosen parameters",
setup: func() {
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
// previous chain committed to the change
upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
// change sequence to be higher height than latest current client height
soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), soloClient)

// change client-specified parameter
soloClient.AllowUpdateAfterProposal = true

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
suite.Require().NoError(err)

cs, found = suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)
// Previous client-chosen parameters must be the same as upgraded client chosen parameters
tmClient, _ := cs.(*types.ClientState)
oldClient := types.NewClientState(tmClient.ChainId, types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, tmClient.LatestHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)
suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, oldClient)

proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
},
expPass: true,
},
{
name: "unsuccessful upgrade to a new tendermint client: chain-specified paramaters do not match committed client",
name: "unsuccessful upgrade: chain-specified paramaters do not match committed client",
setup: func() {

upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
Expand All @@ -142,53 +93,45 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
expPass: false,
},
{

name: "unsuccessful upgrade to a new solomachine client: chain-specified paramaters do not match committed client",
name: "unsuccessful upgrade: client-specified parameters do not match previous client",
setup: func() {
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

// demonstrate that VerifyUpgrade allows for arbitrary changes to clienstate structure so long as
// previous chain committed to the change
upgradedClient = ibctesting.NewSolomachine(suite.T(), suite.cdc, clientA, "diversifier", 1).ClientState()
soloClient, _ := upgradedClient.(*solomachinetypes.ClientState)
// change sequence to be higher height than latest current client height
soloClient.Sequence = cs.GetLatestHeight().GetEpochHeight() + 100
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)

// change chain-specified parameters from committed values
soloClient.Sequence = 10000
// change upgradedClient client-specified parameters
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, ubdPeriod, ubdPeriod+trustingPeriod, maxClockDrift+5, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, true, false)

suite.coordinator.CommitBlock(suite.chainB)
err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
suite.Require().NoError(err)

cs, found = suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
suite.Require().True(found)

proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
},
expPass: false,
},
{
name: "unsuccessful upgrade to a new tendermint client: proof is empty",
name: "unsuccessful upgrade: proof is empty",
setup: func() {
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
proofUpgrade = []byte{}
},
expPass: false,
},
{
name: "unsuccessful upgrade to a new tendermint client: proof unmarshal failed",
name: "unsuccessful upgrade: proof unmarshal failed",
setup: func() {
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
proofUpgrade = []byte("proof")
},
expPass: false,
},
{
name: "unsuccessful upgrade to a new tendermint client: proof verification failed",
name: "unsuccessful upgrade: proof verification failed",
setup: func() {
// create but do not store upgraded client
upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
Expand All @@ -197,11 +140,12 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
suite.Require().True(found)

proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
fmt.Printf("%#v\n", proofUpgrade)
},
expPass: false,
},
{
name: "unsuccessful upgrade to a new tendermint client: upgrade path is nil",
name: "unsuccessful upgrade: upgrade path is nil",
setup: func() {

upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
Expand All @@ -227,7 +171,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
expPass: false,
},
{
name: "unsuccessful upgrade to a new tendermint client: upgraded height is not greater than current height",
name: "unsuccessful upgrade: upgraded height is not greater than current height",
setup: func() {

upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, height, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
Expand All @@ -251,6 +195,10 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {

for _, tc := range testCases {
tc := tc

// reset suite
suite.SetupTest()

clientA, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, ibctesting.Tendermint)

tc.setup()
Expand Down