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

Jump to bottom

UpgradeClient Followup #1 #7457

Merged
merged 19 commits into from
Oct 8, 2020
Merged

UpgradeClient Followup #1 #7457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fb9cd44
require old chain halts before upgrade
AdityaSripal Oct 5, 2020
7dda223
Update proto/ibc/core/client/v1/client.proto
AdityaSripal Oct 5, 2020
37a20af
start address reviews
AdityaSripal Oct 5, 2020
7ad8d73
Apply suggestions from code review
AdityaSripal Oct 6, 2020
ebaad0c
Merge branch 'aditya/upgrade-followups' of github.com:cosmos/cosmos-s…
AdityaSripal Oct 6, 2020
a39437b
address reviews
AdityaSripal Oct 6, 2020
fef7f93
fix merge
AdityaSripal Oct 6, 2020
85817e5
rework upgrade to ensure there is never more than one upgrade client …
AdityaSripal Oct 6, 2020
8d59a8f
fix tests
AdityaSripal Oct 6, 2020
25847e4
fix conditional
AdityaSripal Oct 6, 2020
8e6268a
Merge branch 'master' into aditya/upgrade-followups
AdityaSripal Oct 6, 2020
8ba3d1f
make proto-gen
AdityaSripal Oct 6, 2020
690dfcc
Merge branch 'master' into aditya/upgrade-followups
colin-axner Oct 7, 2020
2671765
remove if statement skipping tests in upgrade keeper test
colin-axner Oct 7, 2020
2d4c5f2
Merge branch 'master' into aditya/upgrade-followups
colin-axner Oct 7, 2020
f656313
address reviews
AdityaSripal Oct 7, 2020
9c1e043
Merge branch 'aditya/upgrade-followups' of github.com:cosmos/cosmos-s…
AdityaSripal Oct 7, 2020
86e5d30
correctly escape and unescape merkle keys
AdityaSripal Oct 7, 2020
cb1556d
add small conditional check
AdityaSripal Oct 7, 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
6 changes: 4 additions & 2 deletions proto/ibc/core/client/v1/client.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ message MsgUpgradeClient {
string client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""];
// upgraded client state
google.protobuf.Any client_state = 2 [(gogoproto.moretags) = "yaml:\"client_state\""];
// height at which old chain halts and upgrades (i.e last block executed)
Height upgrade_height = 3 [(gogoproto.moretags) = "yaml:\"upgrade_height\""];
// proof that old chain committed to new client
bytes proof_upgrade = 3 [(gogoproto.moretags) = "yaml:\"proof_upgrade\""];
bytes proof_upgrade = 4 [(gogoproto.moretags) = "yaml:\"proof_upgrade\""];
// signer address
string signer = 4;
string signer = 5;
}

// MsgSubmitMisbehaviour defines an sdk.Msg type that submits Evidence for
Expand Down
2 changes: 1 addition & 1 deletion proto/ibc/lightclients/tendermint/v1/tendermint.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ message ClientState {
repeated ics23.ProofSpec proof_specs = 8 [(gogoproto.moretags) = "yaml:\"proof_specs\""];

// Path at which next upgraded client will be committed
ibc.core.commitment.v1.MerklePath upgrade_path = 9 [(gogoproto.moretags) = "yaml:\"upgrade_path\""];
string upgrade_path = 9 [(gogoproto.moretags) = "yaml:\"upgrade_path\""];

// This flag, when set to true, will allow governance to recover a client
// which has expired
Expand Down
3 changes: 2 additions & 1 deletion x/distribution/types/genesis.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion x/ibc/core/02-client/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func HandleMsgUpgradeClient(ctx sdk.Context, k keeper.Keeper, msg *types.MsgUpgr
return nil, err
}

if err = k.UpgradeClient(ctx, msg.ClientId, upgradedClient, msg.ProofUpgrade); err != nil {
if err = k.UpgradeClient(ctx, msg.ClientId, upgradedClient, msg.UpgradeHeight, msg.ProofUpgrade); err != nil {
return nil, err
}

Expand Down
46 changes: 32 additions & 14 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 @@ -84,10 +84,11 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
var (
clientA string
upgradedClient exported.ClientState
upgradeHeight exported.Height
msg *clienttypes.MsgUpgradeClient
)

upgradeHeight := clienttypes.NewHeight(1, 1)
newClientHeight := clienttypes.NewHeight(1, 1)

cases := []struct {
name string
Expand All @@ -98,9 +99,13 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
name: "successful upgrade",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, 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)

// upgrade Height is at next block
upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

// commit upgrade store changes and update clients

Expand All @@ -111,9 +116,9 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: true,
Expand All @@ -125,23 +130,32 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
// upgrade Height is at next block
upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

consState := ibctmtypes.NewConsensusState(time.Now(), commitmenttypes.NewMerkleRoot([]byte("app_hash")), []byte("next_vals_hash"))
consAny, err := clienttypes.PackConsensusState(consState)
suite.Require().NoError(err)

msg = &types.MsgUpgradeClient{ClientId: clientA, ClientState: consAny, ProofUpgrade: proofUpgrade, Signer: suite.chainA.SenderAccount.GetAddress().String()}
height, _ := upgradeHeight.(types.Height)

msg = &types.MsgUpgradeClient{ClientId: clientA, ClientState: consAny, UpgradeHeight: &height, ProofUpgrade: proofUpgrade, Signer: suite.chainA.SenderAccount.GetAddress().String()}
},
expPass: false,
},
{
name: "invalid clientstate",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &ibctesting.UpgradePath, false, false)
upgradedClient = ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath, false, false)

// upgrade Height is at next block
upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

// commit upgrade store changes and update clients

Expand All @@ -152,9 +166,9 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
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())
proofUpgrade, _ := suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, proofUpgrade, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: false,
Expand All @@ -163,17 +177,21 @@ func (suite *ClientTestSuite) TestUpgradeClient() {
name: "VerifyUpgrade fails",
setup: func() {

upgradedClient = ibctmtypes.NewClientState("newChainId", ibctmtypes.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod+ibctesting.TrustingPeriod, ibctesting.MaxClockDrift, upgradeHeight, 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)

// upgrade Height is at next block
upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))

// zero custom fields and store in upgrade store
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)

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

msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, nil, suite.chainA.SenderAccount.GetAddress())
msg, err = clienttypes.NewMsgUpgradeClient(clientA, upgradedClient, upgradeHeight, nil, suite.chainA.SenderAccount.GetAddress())
suite.Require().NoError(err)
},
expPass: false,
Expand Down
4 changes: 2 additions & 2 deletions x/ibc/core/02-client/keeper/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H

// UpgradeClient upgrades the client to a new client state if this new client was committed to
// by the old client at the specified upgrade height
func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient exported.ClientState, proofUpgrade []byte) error {
func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient exported.ClientState, upgradeHeight exported.Height, proofUpgrade []byte) error {
clientState, found := k.GetClientState(ctx, clientID)
if !found {
return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", clientID)
Expand All @@ -93,7 +93,7 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
return sdkerrors.Wrapf(types.ErrClientFrozen, "cannot update client with ID %s", clientID)
}

err := clientState.VerifyUpgrade(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, proofUpgrade)
err := clientState.VerifyUpgrade(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade)
if err != nil {
return sdkerrors.Wrapf(err, "cannot upgrade client with ID: %s", clientID)
}
Expand Down
Loading