diff --git a/x/ibc/core/02-client/keeper/client.go b/x/ibc/core/02-client/keeper/client.go index 83ecedc53548..5a6bb78d240a 100644 --- a/x/ibc/core/02-client/keeper/client.go +++ b/x/ibc/core/02-client/keeper/client.go @@ -118,21 +118,22 @@ 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, upgradeHeight, proofUpgrade) + updatedClientState, updatedConsensusState, err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, clientID), upgradedClient, upgradeHeight, proofUpgrade) if err != nil { return sdkerrors.Wrapf(err, "cannot upgrade client with ID %s", clientID) } - k.SetClientState(ctx, clientID, upgradedClient) + k.SetClientState(ctx, clientID, updatedClientState) + k.SetClientConsensusState(ctx, clientID, updatedClientState.GetLatestHeight(), updatedConsensusState) - k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String()) + k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", updatedClientState.GetLatestHeight().String()) defer func() { telemetry.IncrCounterWithLabels( []string{"ibc", "client", "upgrade"}, 1, []metrics.Label{ - telemetry.NewLabel("client-type", clientState.ClientType()), + telemetry.NewLabel("client-type", updatedClientState.ClientType()), telemetry.NewLabel("client-id", clientID), }, ) @@ -143,8 +144,8 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e sdk.NewEvent( types.EventTypeUpgradeClient, sdk.NewAttribute(types.AttributeKeyClientID, clientID), - sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()), - sdk.NewAttribute(types.AttributeKeyConsensusHeight, upgradedClient.GetLatestHeight().String()), + sdk.NewAttribute(types.AttributeKeyClientType, updatedClientState.ClientType()), + sdk.NewAttribute(types.AttributeKeyConsensusHeight, updatedClientState.GetLatestHeight().String()), ), ) diff --git a/x/ibc/core/exported/client.go b/x/ibc/core/exported/client.go index 8c7e89b33e7a..a46c8da0b25b 100644 --- a/x/ibc/core/exported/client.go +++ b/x/ibc/core/exported/client.go @@ -39,14 +39,14 @@ type ClientState interface { CheckProposedHeaderAndUpdateState(sdk.Context, codec.BinaryMarshaler, sdk.KVStore, Header) (ClientState, ConsensusState, error) // Upgrade functions - VerifyUpgrade( + VerifyUpgradeAndUpdateState( ctx sdk.Context, cdc codec.BinaryMarshaler, store sdk.KVStore, newClient ClientState, upgradeHeight Height, proofUpgrade []byte, - ) error + ) (ClientState, ConsensusState, error) // Utility function that zeroes out any client customizable fields in client state // Ledger enforced fields are maintained while all custom fields are zero values // Used to verify upgrades diff --git a/x/ibc/light-clients/06-solomachine/types/client_state.go b/x/ibc/light-clients/06-solomachine/types/client_state.go index 34cb91964bc0..4a6fb8696205 100644 --- a/x/ibc/light-clients/06-solomachine/types/client_state.go +++ b/x/ibc/light-clients/06-solomachine/types/client_state.go @@ -74,12 +74,12 @@ func (cs ClientState) ZeroCustomFields() exported.ClientState { ) } -// VerifyUpgrade returns an error since solomachine client does not support upgrades -func (cs ClientState) VerifyUpgrade( +// VerifyUpgradeAndUpdateState returns an error since solomachine client does not support upgrades +func (cs ClientState) VerifyUpgradeAndUpdateState( _ sdk.Context, _ codec.BinaryMarshaler, _ sdk.KVStore, _ exported.ClientState, _ exported.Height, _ []byte, -) error { - return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client") +) (exported.ClientState, exported.ConsensusState, error) { + return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client") } // VerifyClientState verifies a proof of the client state of the running chain diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade.go b/x/ibc/light-clients/07-tendermint/types/upgrade.go index 28f02950d1f6..34e00f9ccdd3 100644 --- a/x/ibc/light-clients/07-tendermint/types/upgrade.go +++ b/x/ibc/light-clients/07-tendermint/types/upgrade.go @@ -3,7 +3,6 @@ package types import ( "fmt" "net/url" - "reflect" "strings" "github.com/cosmos/cosmos-sdk/codec" @@ -14,7 +13,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) -// VerifyUpgrade checks if the upgraded client has been committed by the current client +// VerifyUpgradeAndUpdateState checks if the upgraded client has been committed by the current client // It will zero out all client-specific fields (e.g. TrustingPeriod and verify all data // in client state that must be the same across all valid Tendermint clients for the new chain. // VerifyUpgrade will return an error if: @@ -23,42 +22,36 @@ import ( // - the latest height of the new client does not match the height in committed client // - any Tendermint chain specified parameter in upgraded client such as ChainID, UnbondingPeriod, // and ProofSpecs do not match parameters set by committed client -func (cs ClientState) VerifyUpgrade( +func (cs ClientState) VerifyUpgradeAndUpdateState( ctx sdk.Context, cdc codec.BinaryMarshaler, clientStore sdk.KVStore, upgradedClient exported.ClientState, upgradeHeight exported.Height, proofUpgrade []byte, -) error { +) (exported.ClientState, exported.ConsensusState, error) { if cs.UpgradePath == "" { - return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set") + return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set") } upgradePath, err := constructUpgradeMerklePath(cs.UpgradePath, upgradeHeight) if err != nil { - return sdkerrors.Wrapf(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, unescaping key with URL format failed: %v", err) + return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, unescaping key with URL format failed: %v", err) } // UpgradeHeight must be in same version as client state height if cs.GetLatestHeight().GetVersionNumber() != upgradeHeight.GetVersionNumber() { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version at which upgrade occurs must be same as current client version. expected version %d, got %d", + return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version at which upgrade occurs must be same as current client version. expected version %d, got %d", cs.GetLatestHeight().GetVersionNumber(), upgradeHeight.GetVersionNumber()) } - 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, "upgraded client height %s must be greater than current client height %s", + return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "upgraded client height %s must be greater than current client height %s", upgradedClient.GetLatestHeight(), cs.GetLatestHeight()) } if len(proofUpgrade) == 0 { - return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "proof of upgrade is empty") + return nil, nil, sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "proof of upgrade is empty") } var merkleProof commitmenttypes.MerkleProof if err := cdc.UnmarshalBinaryBare(proofUpgrade, &merkleProof); err != nil { - return sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal merkle proof: %v", err) + return nil, nil, sdkerrors.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal merkle proof: %v", err) } // counterparty chain must commit the upgraded client with all client-customizable fields zeroed out @@ -66,7 +59,7 @@ func (cs ClientState) VerifyUpgrade( committedClient := upgradedClient.ZeroCustomFields() bz, err := codec.MarshalAny(cdc, committedClient) if err != nil { - return sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err) + return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err) } // Must prove against latest consensus state to ensure we are verifying against latest upgrade plan @@ -74,34 +67,38 @@ func (cs ClientState) VerifyUpgrade( // at this consensus state consState, err := GetConsensusState(clientStore, cdc, upgradeHeight) if err != nil { - return sdkerrors.Wrap(err, "could not retrieve consensus state for upgradeHeight") + return nil, nil, sdkerrors.Wrap(err, "could not retrieve consensus state for upgradeHeight") } if cs.IsExpired(consState.Timestamp, ctx.BlockTime()) { - return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "cannot upgrade an expired client") + return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidClient, "cannot upgrade an expired client") } tmCommittedClient, ok := committedClient.(*ClientState) if !ok { - return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "upgraded client must be Tendermint client. expected: %T got: %T", + return nil, nil, 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. + // Relayer chosen client parameters are ignored. // All chain-chosen parameters come from committed client, all client-chosen parameters - // come from current client - expectedClient := NewClientState( + // come from current client. + updatedClientState := NewClientState( tmCommittedClient.ChainId, cs.TrustLevel, cs.TrustingPeriod, tmCommittedClient.UnbondingPeriod, cs.MaxClockDrift, tmCommittedClient.LatestHeight, tmCommittedClient.ConsensusParams, 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) + + if err := updatedClientState.Validate(); err != nil { + return nil, nil, sdkerrors.Wrap(err, "updated client state failed basic validation") + } + + if err := merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradePath, bz); err != nil { + return nil, nil, err } - return merkleProof.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradePath, bz) + // TODO: Return valid consensus state https://github.com/cosmos/cosmos-sdk/issues/7708 + return updatedClientState, &ConsensusState{}, nil } // construct MerklePath from upgradePath diff --git a/x/ibc/light-clients/07-tendermint/types/upgrade_test.go b/x/ibc/light-clients/07-tendermint/types/upgrade_test.go index bb2108508d23..fea9f09cb201 100644 --- a/x/ibc/light-clients/07-tendermint/types/upgrade_test.go +++ b/x/ibc/light-clients/07-tendermint/types/upgrade_test.go @@ -317,6 +317,31 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { }, expPass: false, }, + { + name: "unsuccessful upgrade: updated unbonding period is equal to trusting period", + setup: func() { + + upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, trustingPeriod, maxClockDrift, newClientHeight, ibctesting.DefaultConsensusParams, commitmenttypes.GetSDKSpecs(), 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(), int64(upgradeHeight.GetVersionHeight()), upgradedClient) + + // commit upgrade store changes and update clients + + suite.coordinator.CommitBlock(suite.chainB) + err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, exported.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(int64(upgradeHeight.GetVersionHeight())), cs.GetLatestHeight().GetVersionHeight()) + }, + expPass: false, + }, } for _, tc := range testCases { @@ -332,7 +357,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { cs := suite.chainA.GetClientState(clientA) clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) - err := cs.VerifyUpgrade( + clientState, consensusState, err := cs.VerifyUpgradeAndUpdateState( suite.chainA.GetContext(), suite.cdc, clientStore, @@ -343,8 +368,14 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { if tc.expPass { suite.Require().NoError(err, "verify upgrade failed on valid case: %s", tc.name) + suite.Require().NotNil(clientState, "verify upgrade failed on valid case: %s", tc.name) + suite.Require().NotNil(consensusState, "verify upgrade failed on valid case: %s", tc.name) } else { suite.Require().Error(err, "verify upgrade passed on invalid case: %s", tc.name) + suite.Require().Nil(clientState, "verify upgrade passed on invalid case: %s", tc.name) + + suite.Require().Nil(consensusState, "verify upgrade passed on invalid case: %s", tc.name) + } } } diff --git a/x/ibc/light-clients/09-localhost/types/client_state.go b/x/ibc/light-clients/09-localhost/types/client_state.go index eb6931147672..32b799fccd39 100644 --- a/x/ibc/light-clients/09-localhost/types/client_state.go +++ b/x/ibc/light-clients/09-localhost/types/client_state.go @@ -102,12 +102,12 @@ func (cs ClientState) CheckProposedHeaderAndUpdateState( return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "cannot update localhost client with a proposal") } -// VerifyUpgrade returns an error since localhost cannot be upgraded -func (cs ClientState) VerifyUpgrade( +// VerifyUpgradeAndUpdateState returns an error since localhost cannot be upgraded +func (cs ClientState) VerifyUpgradeAndUpdateState( _ sdk.Context, _ codec.BinaryMarshaler, _ sdk.KVStore, _ exported.ClientState, _ exported.Height, _ []byte, -) error { - return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client") +) (exported.ClientState, exported.ConsensusState, error) { + return nil, nil, sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade localhost client") } // VerifyClientState verifies that the localhost client state is stored locally