diff --git a/CHANGELOG.md b/CHANGELOG.md index aa9cb95669ec..c39a1780e9aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/{bank,distrib,gov,slashing,staking}) [\#8363](https://github.com/cosmos/cosmos-sdk/issues/8363) Store keys have been modified to allow for variable-length addresses. * (x/ibc) [\#8266](https://github.com/cosmos/cosmos-sdk/issues/8266) Add amino JSON for IBC messages in order to support Ledger text signing. +* (x/ibc) [\#8405](https://github.com/cosmos/cosmos-sdk/pull/8405) Refactor IBC client update governance proposals to use a substitute client to update a frozen or expired client. * (x/evidence) [\#8502](https://github.com/cosmos/cosmos-sdk/pull/8502) `HandleEquivocationEvidence` persists the evidence to state. * (x/gov) [\#7733](https://github.com/cosmos/cosmos-sdk/pull/7733) ADR 037 Implementation: Governance Split Votes diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index 424f0415f60f..66eeaa13ea3f 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -44,7 +44,7 @@ We elect not to deal with chains which have actually halted, which is necessaril 1. Add a new governance proposal type, `ClientUpdateProposal`, in the `x/ibc` module 1. Extend the base `Proposal` with two client identifiers (`string`) and an initial height ('exported.Height'). 1. The first client identifier is the proposed client to be updated. This client must be either frozen or expired. - 1. The second client is a substitute client. It carries all the state for the client which may be updated. It must have identitical client and chain parameters to the client which may be updated (except for latest height and frozen height). It should be continually updated during the voting period. + 1. The second client is a substitute client. It carries all the state for the client which may be updated. It must have identitical client and chain parameters to the client which may be updated (except for latest height, frozen height, and chain-id). It should be continually updated during the voting period. 1. The initial height represents the starting height consensus states which will be copied from the substitute client to the frozen/expired client. 1. If this governance proposal passes, the client on trial will be updated with all the state of the substitute, if and only if: 1. `allow_governance_override_after_expiry` is true and the client has expired (`Expired()` returns true) diff --git a/docs/core/proto-docs.md b/docs/core/proto-docs.md index c98035efc0b0..2e39a4befc9a 100644 --- a/docs/core/proto-docs.md +++ b/docs/core/proto-docs.md @@ -7949,17 +7949,20 @@ client. ### ClientUpdateProposal -ClientUpdateProposal is a governance proposal. If it passes, the client is -updated with the provided header. The update may fail if the header is not -valid given certain conditions specified by the client implementation. +ClientUpdateProposal is a governance proposal. If it passes, the substitute client's +consensus states starting from the 'initial height' are copied over to the subjects +client state. The proposal handler may fail if the subject and the substitute do not +match in client and chain parameters (with exception to latest height, frozen height, and chain-id). +The updated client must also be valid (cannot be expired). | Field | Type | Label | Description | | ----- | ---- | ----- | ----------- | | `title` | [string](#string) | | the title of the update proposal | | `description` | [string](#string) | | the description of the proposal | -| `client_id` | [string](#string) | | the client identifier for the client to be updated if the proposal passes | -| `header` | [google.protobuf.Any](#google.protobuf.Any) | | the header used to update the client if the proposal passes | +| `subject_client_id` | [string](#string) | | the client identifier for the client to be updated if the proposal passes | +| `substitute_client_id` | [string](#string) | | the substitute client identifier for the client standing in for the subject client | +| `initial_height` | [Height](#ibc.core.client.v1.Height) | | the intital height to copy consensus states from the substitute to the subject | diff --git a/docs/ibc/README.md b/docs/ibc/README.md index 22061db94583..6298c119632b 100644 --- a/docs/ibc/README.md +++ b/docs/ibc/README.md @@ -12,6 +12,7 @@ This repository contains reference documentation for the IBC protocol integratio 2. [Integration](./integration.md) 3. [Customization](./custom.md) 4. [Relayer](./relayer.md) +5. [Governance Proposals](./proposals.md) After reading about IBC, head on to the [Building Modules documentation](../building-modules/README.md) to learn more about the process of building modules. diff --git a/docs/ibc/proposals.md b/docs/ibc/proposals.md new file mode 100644 index 000000000000..6bdf9f7051fc --- /dev/null +++ b/docs/ibc/proposals.md @@ -0,0 +1,42 @@ + + +# Governance Proposals + +In uncommon situations, a highly valued client may become frozen due to uncontrollable +circumstances. A highly valued client might have hundreds of channels being actively used. +Some of those channels might have a significant amount of locked tokens used for ICS 20. + +If the one third of the validator set of the chain the client represents decides to collude, +they can sign off on two valid but conflicting headers each signed by the other one third +of the honest validator set. The light client can now be updated with two valid, but conflicting +headers at the same height. The light client cannot know which header is trustworthy and therefore +evidence of such misbehaviour is likely to be submitted resulting in a frozen light client. + +Frozen light clients cannot be updated under any circumstance except via a governance proposal. +Since a quorum of validators can sign arbitrary state roots which may not be valid executions +of the state machine, a governance proposal has been added to ease the complexity of unfreezing +or updating clients which have become "stuck". Without this mechanism, validator sets would need +to construct a state root to unfreeze the client. Unfreezing clients, re-enables all of the channels +built upon that client. This may result in recovery of otherwise lost funds. + +Tendermint light clients may become expired if the trusting period has passed since their +last update. This may occur if relayers stop submitting headers to update the clients. + +An unplanned upgrade by the counterparty chain may also result in expired clients. If the counterparty +chain undergoes an unplanned upgrade, there may be no commitment to that upgrade signed by the validator +set before the chain-id changes. In this situation, the validator set of the last valid update for the +light client is never expected to produce another valid header since the chain-id has changed, which will +ultimately lead the on-chain light client to become expired. + +In the case that a highly valued light client is frozen, expired, or rendered non-updateable, a +governance proposal may be submitted to update this client, known as the subject client. The +proposal includes the client identifier for the subject, the client identifier for a substitute +client, and an initial height to reference the substitute client from. Light client implementations +may implement custom updating logic, but in most cases, the subject will be updated with information +from the substitute client, if the proposal passes. The substitute client is used as a "stand in" +while the subject is on trial. It is best practice to create a substitute client *after* the subject +has become frozen to avoid the substitute from also becoming frozen. An active substitute client +allows headers to be submitted during the voting period to prevent accidental expiry once the proposal +passes. diff --git a/proto/ibc/core/client/v1/client.proto b/proto/ibc/core/client/v1/client.proto index 11d2195aaf61..4c6308bd5415 100644 --- a/proto/ibc/core/client/v1/client.proto +++ b/proto/ibc/core/client/v1/client.proto @@ -33,9 +33,11 @@ message ClientConsensusStates { [(gogoproto.moretags) = "yaml:\"consensus_states\"", (gogoproto.nullable) = false]; } -// ClientUpdateProposal is a governance proposal. If it passes, the client is -// updated with the provided header. The update may fail if the header is not -// valid given certain conditions specified by the client implementation. +// ClientUpdateProposal is a governance proposal. If it passes, the substitute client's +// consensus states starting from the 'initial height' are copied over to the subjects +// client state. The proposal handler may fail if the subject and the substitute do not +// match in client and chain parameters (with exception to latest height, frozen height, and chain-id). +// The updated client must also be valid (cannot be expired). message ClientUpdateProposal { option (gogoproto.goproto_getters) = false; // the title of the update proposal @@ -43,9 +45,11 @@ message ClientUpdateProposal { // the description of the proposal string description = 2; // the client identifier for the client to be updated if the proposal passes - string client_id = 3 [(gogoproto.moretags) = "yaml:\"client_id\""]; - // the header used to update the client if the proposal passes - google.protobuf.Any header = 4; + string subject_client_id = 3 [(gogoproto.moretags) = "yaml:\"subject_client_id\""]; + // the substitute client identifier for the client standing in for the subject client + string substitute_client_id = 4 [(gogoproto.moretags) = "yaml:\"susbtitute_client_id\""]; + // the intital height to copy consensus states from the substitute to the subject + Height initial_height = 5 [(gogoproto.moretags) = "yaml:\"initial_height\"", (gogoproto.nullable) = false]; } // Height is a monotonically increasing data type diff --git a/simapp/app.go b/simapp/app.go index 4db7d53d7755..388c8c50eb8a 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -69,6 +69,7 @@ import ( ibctransfertypes "github.com/cosmos/cosmos-sdk/x/ibc/applications/transfer/types" ibc "github.com/cosmos/cosmos-sdk/x/ibc/core" ibcclient "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client" + ibcclienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" porttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/05-port/types" ibchost "github.com/cosmos/cosmos-sdk/x/ibc/core/24-host" ibckeeper "github.com/cosmos/cosmos-sdk/x/ibc/core/keeper" @@ -309,7 +310,7 @@ func NewSimApp( AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)). AddRoute(distrtypes.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.DistrKeeper)). AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper)). - AddRoute(ibchost.RouterKey, ibcclient.NewClientUpdateProposalHandler(app.IBCKeeper.ClientKeeper)) + AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientUpdateProposalHandler(app.IBCKeeper.ClientKeeper)) app.GovKeeper = govkeeper.NewKeeper( appCodec, keys[govtypes.StoreKey], app.GetSubspace(govtypes.ModuleName), app.AccountKeeper, app.BankKeeper, &stakingKeeper, govRouter, diff --git a/x/ibc/core/02-client/client/cli/tx.go b/x/ibc/core/02-client/client/cli/tx.go index d036a5766d84..78313d21b7a3 100644 --- a/x/ibc/core/02-client/client/cli/tx.go +++ b/x/ibc/core/02-client/client/cli/tx.go @@ -11,7 +11,10 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/version" + govcli "github.com/cosmos/cosmos-sdk/x/gov/client/cli" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) @@ -244,3 +247,69 @@ func NewUpgradeClientCmd() *cobra.Command { return cmd } + +// NewCmdSubmitUpdateClientProposal implements a command handler for submitting an update IBC client proposal transaction. +func NewCmdSubmitUpdateClientProposal() *cobra.Command { + cmd := &cobra.Command{ + Use: "update-client [subject-client-id] [substitute-client-id] [initial-height] [flags]", + Args: cobra.ExactArgs(3), + Short: "Submit an update IBC client proposal", + Long: "Submit an update IBC client proposal along with an initial deposit.\n" + + "Please specify a subject client identifier you want to update..\n" + + "Please specify the substitute client the subject client will use and the initial height to reference the substitute client's state.", + RunE: func(cmd *cobra.Command, args []string) error { + clientCtx, err := client.GetClientTxContext(cmd) + if err != nil { + return err + } + + title, err := cmd.Flags().GetString(govcli.FlagTitle) + if err != nil { + return err + } + + description, err := cmd.Flags().GetString(govcli.FlagDescription) + if err != nil { + return err + } + + subjectClientID := args[0] + substituteClientID := args[1] + + initialHeight, err := types.ParseHeight(args[2]) + if err != nil { + return err + } + + content := types.NewClientUpdateProposal(title, description, subjectClientID, substituteClientID, initialHeight) + + from := clientCtx.GetFromAddress() + + depositStr, err := cmd.Flags().GetString(govcli.FlagDeposit) + if err != nil { + return err + } + deposit, err := sdk.ParseCoinsNormalized(depositStr) + if err != nil { + return err + } + + msg, err := govtypes.NewMsgSubmitProposal(content, deposit, from) + if err != nil { + return err + } + + if err = msg.ValidateBasic(); err != nil { + return err + } + + return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg) + }, + } + + cmd.Flags().String(govcli.FlagTitle, "", "title of proposal") + cmd.Flags().String(govcli.FlagDescription, "", "description of proposal") + cmd.Flags().String(govcli.FlagDeposit, "", "deposit of proposal") + + return cmd +} diff --git a/x/ibc/core/02-client/client/proposal_handler.go b/x/ibc/core/02-client/client/proposal_handler.go new file mode 100644 index 000000000000..63585cbe50b2 --- /dev/null +++ b/x/ibc/core/02-client/client/proposal_handler.go @@ -0,0 +1,8 @@ +package client + +import ( + govclient "github.com/cosmos/cosmos-sdk/x/gov/client" + "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/client/cli" +) + +var ProposalHandler = govclient.NewProposalHandler(cli.NewCmdSubmitUpdateClientProposal, nil) diff --git a/x/ibc/core/02-client/keeper/proposal.go b/x/ibc/core/02-client/keeper/proposal.go index 6b17278e09dd..6d4ff350df32 100644 --- a/x/ibc/core/02-client/keeper/proposal.go +++ b/x/ibc/core/02-client/keeper/proposal.go @@ -10,32 +10,41 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) -// ClientUpdateProposal will try to update the client with the new header if and only if -// the proposal passes. The localhost client is not allowed to be modified with a proposal. +// ClientUpdateProposal will retrieve the subject and substitute client. +// The initial height must be greater than the latest height of the subject +// client. A callback will occur to the subject client state with the client +// prefixed store being provided for both the subject and the substitute client. +// The localhost client is not allowed to be modified with a proposal. The IBC +// client implementations are responsible for validating the parameters of the +// subtitute (enusring they match the subject's parameters) as well as copying +// the necessary consensus states from the subtitute to the subject client +// store. func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdateProposal) error { - if p.ClientId == exported.Localhost { + if p.SubjectClientId == exported.Localhost || p.SubstituteClientId == exported.Localhost { return sdkerrors.Wrap(types.ErrInvalidUpdateClientProposal, "cannot update localhost client with proposal") } - clientState, found := k.GetClientState(ctx, p.ClientId) + subjectClientState, found := k.GetClientState(ctx, p.SubjectClientId) if !found { - return sdkerrors.Wrapf(types.ErrClientNotFound, "cannot update client with ID %s", p.ClientId) + return sdkerrors.Wrapf(types.ErrClientNotFound, "subject client with ID %s", p.SubjectClientId) } - header, err := types.UnpackHeader(p.Header) - if err != nil { - return err + if subjectClientState.GetLatestHeight().GTE(p.InitialHeight) { + return sdkerrors.Wrapf(types.ErrInvalidHeight, "subject client state latest height is greater or equal to initial height (%s >= %s)", subjectClientState.GetLatestHeight(), p.InitialHeight) + } + + substituteClientState, found := k.GetClientState(ctx, p.SubstituteClientId) + if !found { + return sdkerrors.Wrapf(types.ErrClientNotFound, "substitute client with ID %s", p.SubstituteClientId) } - clientState, consensusState, err := clientState.CheckProposedHeaderAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, p.ClientId), header) + clientState, err := subjectClientState.CheckSubstituteAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, p.SubjectClientId), k.ClientStore(ctx, p.SubstituteClientId), substituteClientState, p.InitialHeight) if err != nil { return err } + k.SetClientState(ctx, p.SubjectClientId, clientState) - k.SetClientState(ctx, p.ClientId, clientState) - k.SetClientConsensusState(ctx, p.ClientId, header.GetHeight(), consensusState) - - k.Logger(ctx).Info("client updated after governance proposal passed", "client-id", p.ClientId, "height", clientState.GetLatestHeight().String()) + k.Logger(ctx).Info("client updated after governance proposal passed", "client-id", p.SubjectClientId, "height", clientState.GetLatestHeight().String()) defer func() { telemetry.IncrCounterWithLabels( @@ -43,7 +52,7 @@ func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdatePropo 1, []metrics.Label{ telemetry.NewLabel("client-type", clientState.ClientType()), - telemetry.NewLabel("client-id", p.ClientId), + telemetry.NewLabel("client-id", p.SubjectClientId), telemetry.NewLabel("update-type", "proposal"), }, ) @@ -53,9 +62,9 @@ func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdatePropo ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeUpdateClientProposal, - sdk.NewAttribute(types.AttributeKeyClientID, p.ClientId), + sdk.NewAttribute(types.AttributeKeySubjectClientID, p.SubjectClientId), sdk.NewAttribute(types.AttributeKeyClientType, clientState.ClientType()), - sdk.NewAttribute(types.AttributeKeyConsensusHeight, header.GetHeight().String()), + sdk.NewAttribute(types.AttributeKeyConsensusHeight, clientState.GetLatestHeight().String()), ), ) diff --git a/x/ibc/core/02-client/keeper/proposal_test.go b/x/ibc/core/02-client/keeper/proposal_test.go index ada205402b34..8dbe43f7d7f7 100644 --- a/x/ibc/core/02-client/keeper/proposal_test.go +++ b/x/ibc/core/02-client/keeper/proposal_test.go @@ -10,8 +10,11 @@ import ( func (suite *KeeperTestSuite) TestClientUpdateProposal() { var ( - content *types.ClientUpdateProposal - err error + subject, substitute string + subjectClientState, substituteClientState exported.ClientState + initialHeight clienttypes.Height + content *types.ClientUpdateProposal + err error ) testCases := []struct { @@ -21,53 +24,63 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() { }{ { "valid update client proposal", func() { - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - clientState := suite.chainA.GetClientState(clientA) - - tmClientState, ok := clientState.(*ibctmtypes.ClientState) + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight) + }, true, + }, + { + "subject and substitute use different revision numbers", func() { + tmClientState, ok := substituteClientState.(*ibctmtypes.ClientState) suite.Require().True(ok) - tmClientState.AllowUpdateAfterMisbehaviour = true - tmClientState.FrozenHeight = tmClientState.LatestHeight - suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, tmClientState) + consState, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), substitute, tmClientState.LatestHeight) + suite.Require().True(found) + newRevisionNumber := tmClientState.GetLatestHeight().GetRevisionNumber() + 1 - // use next header for chainB to update the client on chainA - header, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientA) - suite.Require().NoError(err) + tmClientState.LatestHeight = clienttypes.NewHeight(newRevisionNumber, tmClientState.GetLatestHeight().GetRevisionHeight()) + initialHeight = clienttypes.NewHeight(newRevisionNumber, initialHeight.GetRevisionHeight()) + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), substitute, tmClientState.LatestHeight, consState) + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, clientA, header) - suite.Require().NoError(err) + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight) }, true, }, { - "client type does not exist", func() { - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, &ibctmtypes.Header{}) - suite.Require().NoError(err) + "cannot use localhost as subject", func() { + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, exported.Localhost, substitute, initialHeight) }, false, }, { - "cannot update localhost", func() { - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, exported.Localhost, &ibctmtypes.Header{}) - suite.Require().NoError(err) + "cannot use localhost as substitute", func() { + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, exported.Localhost, initialHeight) }, false, }, { - "client does not exist", func() { - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, &ibctmtypes.Header{}) - suite.Require().NoError(err) + "subject client does not exist", func() { + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, substitute, initialHeight) }, false, }, { - "cannot unpack header, header is nil", func() { - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - content = &clienttypes.ClientUpdateProposal{ibctesting.Title, ibctesting.Description, clientA, nil} + "substitute client does not exist", func() { + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, ibctesting.InvalidID, initialHeight) }, false, }, { - "update fails", func() { - header := &ibctmtypes.Header{} - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, clientA, header) - suite.Require().NoError(err) + "subject and substitute have equal latest height", func() { + tmClientState, ok := subjectClientState.(*ibctmtypes.ClientState) + suite.Require().True(ok) + tmClientState.LatestHeight = substituteClientState.GetLatestHeight().(clienttypes.Height) + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight) + }, false, + }, + { + "update fails, client is not frozen or expired", func() { + tmClientState, ok := subjectClientState.(*ibctmtypes.ClientState) + suite.Require().True(ok) + tmClientState.FrozenHeight = clienttypes.ZeroHeight() + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight) }, false, }, } @@ -78,6 +91,30 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() { suite.Run(tc.name, func() { suite.SetupTest() // reset + subject, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + subjectClientState = suite.chainA.GetClientState(subject) + substitute, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + initialHeight = clienttypes.NewHeight(subjectClientState.GetLatestHeight().GetRevisionNumber(), subjectClientState.GetLatestHeight().GetRevisionHeight()+1) + + // update substitute twice + suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + substituteClientState = suite.chainA.GetClientState(substitute) + + tmClientState, ok := subjectClientState.(*ibctmtypes.ClientState) + suite.Require().True(ok) + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + tmClientState.FrozenHeight = tmClientState.LatestHeight + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) + + tmClientState, ok = substituteClientState.(*ibctmtypes.ClientState) + suite.Require().True(ok) + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + tmClientState.FrozenHeight = tmClientState.LatestHeight + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) + tc.malleate() err = suite.chainA.App.IBCKeeper.ClientKeeper.ClientUpdateProposal(suite.chainA.GetContext(), content) diff --git a/x/ibc/core/02-client/proposal_handler_test.go b/x/ibc/core/02-client/proposal_handler_test.go index 91c1451b7076..41b893186d65 100644 --- a/x/ibc/core/02-client/proposal_handler_test.go +++ b/x/ibc/core/02-client/proposal_handler_test.go @@ -24,21 +24,29 @@ func (suite *ClientTestSuite) TestNewClientUpdateProposalHandler() { }{ { "valid update client proposal", func() { - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - clientState := suite.chainA.GetClientState(clientA) + subject, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + subjectClientState := suite.chainA.GetClientState(subject) + substitute, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + initialHeight := clienttypes.NewHeight(subjectClientState.GetLatestHeight().GetRevisionNumber(), subjectClientState.GetLatestHeight().GetRevisionHeight()+1) - tmClientState, ok := clientState.(*ibctmtypes.ClientState) + // update substitute twice + suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + substituteClientState := suite.chainA.GetClientState(substitute) + + tmClientState, ok := subjectClientState.(*ibctmtypes.ClientState) suite.Require().True(ok) tmClientState.AllowUpdateAfterMisbehaviour = true tmClientState.FrozenHeight = tmClientState.LatestHeight - suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, tmClientState) + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) - // use next header for chainB to update the client on chainA - header, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientA) - suite.Require().NoError(err) + // replicate changes to substitute (they must match) + tmClientState, ok = substituteClientState.(*ibctmtypes.ClientState) + suite.Require().True(ok) + tmClientState.AllowUpdateAfterMisbehaviour = true + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) - content, err = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, clientA, header) - suite.Require().NoError(err) + content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight) }, true, }, { diff --git a/x/ibc/core/02-client/types/client.pb.go b/x/ibc/core/02-client/types/client.pb.go index a42ddef4c593..61bdd9bf804d 100644 --- a/x/ibc/core/02-client/types/client.pb.go +++ b/x/ibc/core/02-client/types/client.pb.go @@ -191,18 +191,22 @@ func (m *ClientConsensusStates) GetConsensusStates() []ConsensusStateWithHeight return nil } -// ClientUpdateProposal is a governance proposal. If it passes, the client is -// updated with the provided header. The update may fail if the header is not -// valid given certain conditions specified by the client implementation. +// ClientUpdateProposal is a governance proposal. If it passes, the substitute client's +// consensus states starting from the 'initial height' are copied over to the subjects +// client state. The proposal handler may fail if the subject and the substitute do not +// match in client and chain parameters (with exception to latest height, frozen height, and chain-id). +// The updated client must also be valid (cannot be expired). type ClientUpdateProposal struct { // the title of the update proposal Title string `protobuf:"bytes,1,opt,name=title,proto3" json:"title,omitempty"` // the description of the proposal Description string `protobuf:"bytes,2,opt,name=description,proto3" json:"description,omitempty"` // the client identifier for the client to be updated if the proposal passes - ClientId string `protobuf:"bytes,3,opt,name=client_id,json=clientId,proto3" json:"client_id,omitempty" yaml:"client_id"` - // the header used to update the client if the proposal passes - Header *types.Any `protobuf:"bytes,4,opt,name=header,proto3" json:"header,omitempty"` + SubjectClientId string `protobuf:"bytes,3,opt,name=subject_client_id,json=subjectClientId,proto3" json:"subject_client_id,omitempty" yaml:"subject_client_id"` + // the substitute client identifier for the client standing in for the subject client + SubstituteClientId string `protobuf:"bytes,4,opt,name=substitute_client_id,json=substituteClientId,proto3" json:"substitute_client_id,omitempty" yaml:"susbtitute_client_id"` + // the intital height to copy consensus states from the substitute to the subject + InitialHeight Height `protobuf:"bytes,5,opt,name=initial_height,json=initialHeight,proto3" json:"initial_height" yaml:"initial_height"` } func (m *ClientUpdateProposal) Reset() { *m = ClientUpdateProposal{} } @@ -344,43 +348,48 @@ func init() { func init() { proto.RegisterFile("ibc/core/client/v1/client.proto", fileDescriptor_b6bc4c8185546947) } var fileDescriptor_b6bc4c8185546947 = []byte{ - // 574 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x9c, 0x54, 0xbd, 0x8e, 0xd3, 0x4c, - 0x14, 0x8d, 0x93, 0x7c, 0xd1, 0x66, 0xf2, 0x29, 0x59, 0x99, 0x84, 0xf5, 0xa6, 0xb0, 0xa3, 0xa9, - 0x52, 0xec, 0xda, 0x24, 0x14, 0xa0, 0x74, 0x38, 0x0d, 0x5b, 0x80, 0x82, 0x11, 0x02, 0xd1, 0x44, - 0xfe, 0x99, 0x75, 0x46, 0x38, 0x9e, 0xc8, 0x33, 0x09, 0x9b, 0x37, 0xa0, 0xa4, 0xa4, 0xa0, 0xe0, - 0x09, 0xe8, 0x78, 0x03, 0x8a, 0x2d, 0xb7, 0xa4, 0xb2, 0x50, 0xf2, 0x06, 0x79, 0x02, 0xe4, 0x99, - 0xc9, 0x12, 0x07, 0x22, 0xad, 0xa8, 0x7c, 0x7d, 0xe6, 0xcc, 0xb9, 0xe7, 0xdc, 0x19, 0x0d, 0x30, - 0xb0, 0xe7, 0x5b, 0x3e, 0x49, 0x90, 0xe5, 0x47, 0x18, 0xc5, 0xcc, 0x5a, 0xf4, 0x64, 0x65, 0xce, - 0x12, 0xc2, 0x88, 0xaa, 0x62, 0xcf, 0x37, 0x33, 0x82, 0x29, 0xe1, 0x45, 0xaf, 0xdd, 0x0c, 0x49, - 0x48, 0xf8, 0xb2, 0x95, 0x55, 0x82, 0xd9, 0x3e, 0x0d, 0x09, 0x09, 0x23, 0x64, 0xf1, 0x3f, 0x6f, - 0x7e, 0x69, 0xb9, 0xf1, 0x52, 0x2c, 0xc1, 0xcf, 0x0a, 0x68, 0x5d, 0x04, 0x28, 0x66, 0xf8, 0x12, - 0xa3, 0x60, 0xc8, 0x85, 0x5e, 0x32, 0x97, 0x21, 0xb5, 0x07, 0xaa, 0x42, 0x77, 0x8c, 0x03, 0x4d, - 0xe9, 0x28, 0xdd, 0xaa, 0xdd, 0xdc, 0xa4, 0xc6, 0xf1, 0xd2, 0x9d, 0x46, 0x03, 0x78, 0xbb, 0x04, - 0x9d, 0x23, 0x51, 0x5f, 0x04, 0xea, 0x08, 0xfc, 0x2f, 0x71, 0x9a, 0x49, 0x68, 0xc5, 0x8e, 0xd2, - 0xad, 0xf5, 0x9b, 0xa6, 0x68, 0x6f, 0x6e, 0xdb, 0x9b, 0x4f, 0xe2, 0xa5, 0x7d, 0xb2, 0x49, 0x8d, - 0x7b, 0x39, 0x2d, 0xbe, 0x07, 0x3a, 0x35, 0xff, 0xb7, 0x09, 0xf8, 0x55, 0x01, 0xda, 0x90, 0xc4, - 0x14, 0xc5, 0x74, 0x4e, 0x39, 0xf4, 0x1a, 0xb3, 0xc9, 0x53, 0x84, 0xc3, 0x09, 0x53, 0x1f, 0x83, - 0xca, 0x84, 0x57, 0xdc, 0x5e, 0xad, 0xdf, 0x36, 0xff, 0x9c, 0x88, 0x29, 0xb8, 0x76, 0xf9, 0x3a, - 0x35, 0x0a, 0x8e, 0xe4, 0xab, 0x6f, 0x40, 0xc3, 0xdf, 0xaa, 0xde, 0xc1, 0xeb, 0xe9, 0x26, 0x35, - 0x5a, 0x99, 0x57, 0xb8, 0xb7, 0x0b, 0x3a, 0x75, 0x3f, 0xe7, 0x0e, 0x7e, 0x57, 0x40, 0x4b, 0x4c, - 0x31, 0x6f, 0x9b, 0xfe, 0xcb, 0x3c, 0xaf, 0xc0, 0xf1, 0x5e, 0x43, 0xaa, 0x15, 0x3b, 0xa5, 0x6e, - 0xad, 0x7f, 0xf6, 0xb7, 0xa8, 0x87, 0x06, 0x65, 0x1b, 0x59, 0xf8, 0x4d, 0x6a, 0x9c, 0xc8, 0x5e, - 0x7b, 0x9a, 0xd0, 0x69, 0xe4, 0x53, 0x50, 0xf8, 0x4d, 0x01, 0x4d, 0x11, 0xe3, 0xd5, 0x2c, 0x70, - 0x19, 0x1a, 0x25, 0x64, 0x46, 0xa8, 0x1b, 0xa9, 0x4d, 0xf0, 0x1f, 0xc3, 0x2c, 0x42, 0x22, 0x81, - 0x23, 0x7e, 0xd4, 0x0e, 0xa8, 0x05, 0x88, 0xfa, 0x09, 0x9e, 0x31, 0x4c, 0x62, 0x3e, 0xcb, 0xaa, - 0xb3, 0x0b, 0xe5, 0xd3, 0x97, 0xee, 0x94, 0xfe, 0x2c, 0x3b, 0x5e, 0x37, 0x40, 0x89, 0x56, 0x3e, - 0x7c, 0x36, 0x8e, 0xe4, 0x0c, 0xca, 0x1f, 0xbe, 0x18, 0x85, 0xec, 0x3a, 0x57, 0xe4, 0xed, 0x18, - 0x82, 0x46, 0x82, 0x16, 0x98, 0x62, 0x12, 0x8f, 0xe3, 0xf9, 0xd4, 0x43, 0x09, 0xf7, 0x5c, 0xb6, - 0xdb, 0x9b, 0xd4, 0xb8, 0x2f, 0xfa, 0xee, 0x11, 0xa0, 0x53, 0xdf, 0x22, 0xcf, 0x39, 0x90, 0x13, - 0x91, 0x77, 0xad, 0x78, 0x50, 0x44, 0x10, 0x76, 0x44, 0x84, 0x93, 0xc1, 0x51, 0x66, 0xed, 0x53, - 0x66, 0xef, 0x19, 0xa8, 0x8c, 0xdc, 0xc4, 0x9d, 0xd2, 0x4c, 0xd8, 0x8d, 0x22, 0xf2, 0x1e, 0x05, - 0x63, 0x11, 0x98, 0x6a, 0x4a, 0xa7, 0xd4, 0xad, 0xee, 0x0a, 0xef, 0x11, 0xa0, 0x53, 0x97, 0x88, - 0x38, 0x19, 0x6a, 0xbf, 0xb8, 0x5e, 0xe9, 0xca, 0xcd, 0x4a, 0x57, 0x7e, 0xae, 0x74, 0xe5, 0xe3, - 0x5a, 0x2f, 0xdc, 0xac, 0xf5, 0xc2, 0x8f, 0xb5, 0x5e, 0x78, 0xfb, 0x28, 0xc4, 0x6c, 0x32, 0xf7, - 0x4c, 0x9f, 0x4c, 0x2d, 0x9f, 0xd0, 0x29, 0xa1, 0xf2, 0x73, 0x4e, 0x83, 0x77, 0xd6, 0x95, 0x75, - 0xfb, 0xb6, 0x3c, 0xe8, 0x9f, 0xcb, 0xe7, 0x85, 0x2d, 0x67, 0x88, 0x7a, 0x15, 0x3e, 0xdc, 0x87, - 0xbf, 0x02, 0x00, 0x00, 0xff, 0xff, 0x39, 0xbe, 0xfd, 0x04, 0x7e, 0x04, 0x00, 0x00, + // 641 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x9c, 0x54, 0xbf, 0x6e, 0xd3, 0x40, + 0x1c, 0x8e, 0xd3, 0x34, 0x6a, 0x2e, 0x90, 0x14, 0x93, 0xd0, 0x34, 0x40, 0x2e, 0xba, 0x29, 0x03, + 0xb5, 0x69, 0x18, 0x40, 0xdd, 0x48, 0x96, 0x76, 0x00, 0xb5, 0x87, 0x10, 0x88, 0x25, 0xf8, 0xcf, + 0x35, 0x39, 0x70, 0x7c, 0x91, 0xef, 0x5c, 0x9a, 0x37, 0x60, 0x64, 0x64, 0x60, 0xe0, 0x09, 0x78, + 0x0a, 0x86, 0x8e, 0x95, 0x58, 0x98, 0x2c, 0xd4, 0xbe, 0x81, 0x9f, 0x00, 0xf9, 0xee, 0x92, 0x36, + 0x29, 0x15, 0x88, 0xc9, 0x3f, 0x7f, 0xf7, 0xfd, 0xbe, 0xdf, 0xf7, 0xfb, 0x74, 0x36, 0x80, 0xd4, + 0xf5, 0x6c, 0x8f, 0x45, 0xc4, 0xf6, 0x02, 0x4a, 0x42, 0x61, 0x1f, 0x6d, 0xeb, 0xca, 0x9a, 0x44, + 0x4c, 0x30, 0xd3, 0xa4, 0xae, 0x67, 0x65, 0x04, 0x4b, 0xc3, 0x47, 0xdb, 0xcd, 0xda, 0x90, 0x0d, + 0x99, 0x3c, 0xb6, 0xb3, 0x4a, 0x31, 0x9b, 0x9b, 0x43, 0xc6, 0x86, 0x01, 0xb1, 0xe5, 0x9b, 0x1b, + 0x1f, 0xda, 0x4e, 0x38, 0x55, 0x47, 0xe8, 0x8b, 0x01, 0xea, 0x7b, 0x3e, 0x09, 0x05, 0x3d, 0xa4, + 0xc4, 0xef, 0x4b, 0xa1, 0x17, 0xc2, 0x11, 0xc4, 0xdc, 0x06, 0x25, 0xa5, 0x3b, 0xa0, 0x7e, 0xc3, + 0x68, 0x1b, 0x9d, 0x52, 0xaf, 0x96, 0x26, 0x70, 0x7d, 0xea, 0x8c, 0x83, 0x1d, 0x34, 0x3f, 0x42, + 0x78, 0x4d, 0xd5, 0x7b, 0xbe, 0xb9, 0x0f, 0x6e, 0x68, 0x9c, 0x67, 0x12, 0x8d, 0x7c, 0xdb, 0xe8, + 0x94, 0xbb, 0x35, 0x4b, 0x8d, 0xb7, 0x66, 0xe3, 0xad, 0xa7, 0xe1, 0xb4, 0xb7, 0x91, 0x26, 0xf0, + 0xf6, 0x82, 0x96, 0xec, 0x41, 0xb8, 0xec, 0x5d, 0x98, 0x40, 0xdf, 0x0c, 0xd0, 0xe8, 0xb3, 0x90, + 0x93, 0x90, 0xc7, 0x5c, 0x42, 0xaf, 0xa8, 0x18, 0xed, 0x12, 0x3a, 0x1c, 0x09, 0xf3, 0x09, 0x28, + 0x8e, 0x64, 0x25, 0xed, 0x95, 0xbb, 0x4d, 0xeb, 0x6a, 0x22, 0x96, 0xe2, 0xf6, 0x0a, 0x27, 0x09, + 0xcc, 0x61, 0xcd, 0x37, 0x5f, 0x83, 0xaa, 0x37, 0x53, 0xfd, 0x07, 0xaf, 0x9b, 0x69, 0x02, 0xeb, + 0x99, 0x57, 0xb4, 0xd4, 0x85, 0x70, 0xc5, 0x5b, 0x70, 0x87, 0xbe, 0x1b, 0xa0, 0xae, 0x52, 0x5c, + 0xb4, 0xcd, 0xff, 0x27, 0xcf, 0x63, 0xb0, 0xbe, 0x34, 0x90, 0x37, 0xf2, 0xed, 0x95, 0x4e, 0xb9, + 0xfb, 0xe0, 0x4f, 0xab, 0x5e, 0x17, 0x54, 0x0f, 0x66, 0xcb, 0xa7, 0x09, 0xdc, 0xd0, 0xb3, 0x96, + 0x34, 0x11, 0xae, 0x2e, 0x6e, 0xc1, 0xd1, 0x8f, 0x3c, 0xa8, 0xa9, 0x35, 0x5e, 0x4e, 0x7c, 0x47, + 0x90, 0xfd, 0x88, 0x4d, 0x18, 0x77, 0x02, 0xb3, 0x06, 0x56, 0x05, 0x15, 0x01, 0x51, 0x1b, 0x60, + 0xf5, 0x62, 0xb6, 0x41, 0xd9, 0x27, 0xdc, 0x8b, 0xe8, 0x44, 0x50, 0x16, 0xca, 0x2c, 0x4b, 0xf8, + 0x32, 0x64, 0xee, 0x82, 0x5b, 0x3c, 0x76, 0xdf, 0x11, 0x4f, 0x0c, 0x2e, 0x52, 0x58, 0x91, 0x29, + 0xdc, 0x4b, 0x13, 0xd8, 0x50, 0xce, 0xae, 0x50, 0x10, 0xae, 0x6a, 0xac, 0x3f, 0x0b, 0xe5, 0x00, + 0xd4, 0x78, 0xec, 0x72, 0x41, 0x45, 0x2c, 0xc8, 0x25, 0xb1, 0x82, 0x14, 0x83, 0x69, 0x02, 0xef, + 0xce, 0xc4, 0xb8, 0xbb, 0xcc, 0x42, 0xd8, 0xbc, 0x68, 0x9e, 0x4b, 0xbe, 0x05, 0x15, 0x1a, 0x52, + 0x41, 0x9d, 0x60, 0xa0, 0x2f, 0xd4, 0xea, 0x5f, 0x2f, 0xd4, 0x7d, 0x9d, 0x69, 0x5d, 0x0d, 0x5b, + 0xec, 0x47, 0xf8, 0xa6, 0x06, 0x14, 0x7b, 0xa7, 0xf0, 0xf1, 0x2b, 0xcc, 0x65, 0x1f, 0x5b, 0x51, + 0xdf, 0xdd, 0x3e, 0xa8, 0x46, 0xe4, 0x88, 0x72, 0xca, 0xc2, 0x41, 0x18, 0x8f, 0x5d, 0x12, 0xc9, + 0x44, 0x0b, 0xbd, 0x66, 0x9a, 0xc0, 0x3b, 0x4a, 0x73, 0x89, 0x80, 0x70, 0x65, 0x86, 0x3c, 0x97, + 0xc0, 0x82, 0x88, 0x36, 0x9e, 0xbf, 0x56, 0x64, 0xe6, 0x6c, 0x2e, 0xa2, 0xad, 0xad, 0x65, 0xd6, + 0x3e, 0x67, 0xf6, 0x9e, 0x81, 0xe2, 0xbe, 0x13, 0x39, 0x63, 0x9e, 0x09, 0x3b, 0x41, 0xc0, 0x3e, + 0x10, 0x5f, 0x47, 0xc7, 0x1b, 0x46, 0x7b, 0xa5, 0x53, 0xba, 0x2c, 0xbc, 0x44, 0x40, 0xb8, 0xa2, + 0x11, 0x15, 0x2b, 0xef, 0x1d, 0x9c, 0x9c, 0xb5, 0x8c, 0xd3, 0xb3, 0x96, 0xf1, 0xeb, 0xac, 0x65, + 0x7c, 0x3a, 0x6f, 0xe5, 0x4e, 0xcf, 0x5b, 0xb9, 0x9f, 0xe7, 0xad, 0xdc, 0x9b, 0xc7, 0x43, 0x2a, + 0x46, 0xb1, 0x6b, 0x79, 0x6c, 0x6c, 0x7b, 0x8c, 0x8f, 0x19, 0xd7, 0x8f, 0x2d, 0xee, 0xbf, 0xb7, + 0x8f, 0xed, 0xf9, 0x9f, 0xef, 0x61, 0x77, 0x4b, 0xff, 0xfc, 0xc4, 0x74, 0x42, 0xb8, 0x5b, 0x94, + 0x9f, 0xe5, 0xa3, 0xdf, 0x01, 0x00, 0x00, 0xff, 0xff, 0x95, 0xe2, 0x8e, 0x47, 0x1c, 0x05, 0x00, + 0x00, } func (m *IdentifiedClientState) Marshal() (dAtA []byte, err error) { @@ -534,22 +543,27 @@ func (m *ClientUpdateProposal) MarshalToSizedBuffer(dAtA []byte) (int, error) { _ = i var l int _ = l - if m.Header != nil { - { - size, err := m.Header.MarshalToSizedBuffer(dAtA[:i]) - if err != nil { - return 0, err - } - i -= size - i = encodeVarintClient(dAtA, i, uint64(size)) + { + size, err := m.InitialHeight.MarshalToSizedBuffer(dAtA[:i]) + if err != nil { + return 0, err } + i -= size + i = encodeVarintClient(dAtA, i, uint64(size)) + } + i-- + dAtA[i] = 0x2a + if len(m.SubstituteClientId) > 0 { + i -= len(m.SubstituteClientId) + copy(dAtA[i:], m.SubstituteClientId) + i = encodeVarintClient(dAtA, i, uint64(len(m.SubstituteClientId))) i-- dAtA[i] = 0x22 } - if len(m.ClientId) > 0 { - i -= len(m.ClientId) - copy(dAtA[i:], m.ClientId) - i = encodeVarintClient(dAtA, i, uint64(len(m.ClientId))) + if len(m.SubjectClientId) > 0 { + i -= len(m.SubjectClientId) + copy(dAtA[i:], m.SubjectClientId) + i = encodeVarintClient(dAtA, i, uint64(len(m.SubjectClientId))) i-- dAtA[i] = 0x1a } @@ -711,14 +725,16 @@ func (m *ClientUpdateProposal) Size() (n int) { if l > 0 { n += 1 + l + sovClient(uint64(l)) } - l = len(m.ClientId) + l = len(m.SubjectClientId) if l > 0 { n += 1 + l + sovClient(uint64(l)) } - if m.Header != nil { - l = m.Header.Size() + l = len(m.SubstituteClientId) + if l > 0 { n += 1 + l + sovClient(uint64(l)) } + l = m.InitialHeight.Size() + n += 1 + l + sovClient(uint64(l)) return n } @@ -1206,7 +1222,7 @@ func (m *ClientUpdateProposal) Unmarshal(dAtA []byte) error { iNdEx = postIndex case 3: if wireType != 2 { - return fmt.Errorf("proto: wrong wireType = %d for field ClientId", wireType) + return fmt.Errorf("proto: wrong wireType = %d for field SubjectClientId", wireType) } var stringLen uint64 for shift := uint(0); ; shift += 7 { @@ -1234,11 +1250,43 @@ func (m *ClientUpdateProposal) Unmarshal(dAtA []byte) error { if postIndex > l { return io.ErrUnexpectedEOF } - m.ClientId = string(dAtA[iNdEx:postIndex]) + m.SubjectClientId = string(dAtA[iNdEx:postIndex]) iNdEx = postIndex case 4: if wireType != 2 { - return fmt.Errorf("proto: wrong wireType = %d for field Header", wireType) + return fmt.Errorf("proto: wrong wireType = %d for field SubstituteClientId", wireType) + } + var stringLen uint64 + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return ErrIntOverflowClient + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + stringLen |= uint64(b&0x7F) << shift + if b < 0x80 { + break + } + } + intStringLen := int(stringLen) + if intStringLen < 0 { + return ErrInvalidLengthClient + } + postIndex := iNdEx + intStringLen + if postIndex < 0 { + return ErrInvalidLengthClient + } + if postIndex > l { + return io.ErrUnexpectedEOF + } + m.SubstituteClientId = string(dAtA[iNdEx:postIndex]) + iNdEx = postIndex + case 5: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field InitialHeight", wireType) } var msglen int for shift := uint(0); ; shift += 7 { @@ -1265,10 +1313,7 @@ func (m *ClientUpdateProposal) Unmarshal(dAtA []byte) error { if postIndex > l { return io.ErrUnexpectedEOF } - if m.Header == nil { - m.Header = &types.Any{} - } - if err := m.Header.Unmarshal(dAtA[iNdEx:postIndex]); err != nil { + if err := m.InitialHeight.Unmarshal(dAtA[iNdEx:postIndex]); err != nil { return err } iNdEx = postIndex diff --git a/x/ibc/core/02-client/types/codec.go b/x/ibc/core/02-client/types/codec.go index 8d79dcdaa45c..59a15832bec1 100644 --- a/x/ibc/core/02-client/types/codec.go +++ b/x/ibc/core/02-client/types/codec.go @@ -7,6 +7,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/msgservice" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) @@ -33,6 +34,10 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) { "ibc.core.client.v1.Misbehaviour", (*exported.Misbehaviour)(nil), ) + registry.RegisterImplementations( + (*govtypes.Content)(nil), + &ClientUpdateProposal{}, + ) registry.RegisterImplementations( (*sdk.Msg)(nil), &MsgCreateClient{}, diff --git a/x/ibc/core/02-client/types/errors.go b/x/ibc/core/02-client/types/errors.go index 09dc92959a2b..5b44cd522211 100644 --- a/x/ibc/core/02-client/types/errors.go +++ b/x/ibc/core/02-client/types/errors.go @@ -30,4 +30,6 @@ var ( ErrUpdateClientFailed = sdkerrors.Register(SubModuleName, 23, "unable to update light client") ErrInvalidUpdateClientProposal = sdkerrors.Register(SubModuleName, 24, "invalid update client proposal") ErrInvalidUpgradeClient = sdkerrors.Register(SubModuleName, 25, "invalid client upgrade") + ErrInvalidHeight = sdkerrors.Register(SubModuleName, 26, "invalid height") + ErrInvalidSubstitute = sdkerrors.Register(SubModuleName, 27, "invalid client state substitute") ) diff --git a/x/ibc/core/02-client/types/events.go b/x/ibc/core/02-client/types/events.go index 779e87cee332..d0760ba89c8a 100644 --- a/x/ibc/core/02-client/types/events.go +++ b/x/ibc/core/02-client/types/events.go @@ -9,6 +9,7 @@ import ( // IBC client events const ( AttributeKeyClientID = "client_id" + AttributeKeySubjectClientID = "subject_client_id" AttributeKeyClientType = "client_type" AttributeKeyConsensusHeight = "consensus_height" ) diff --git a/x/ibc/core/02-client/types/proposal.go b/x/ibc/core/02-client/types/proposal.go index 334a9d4599ef..95b10aaf408e 100644 --- a/x/ibc/core/02-client/types/proposal.go +++ b/x/ibc/core/02-client/types/proposal.go @@ -1,10 +1,8 @@ package types import ( - codectypes "github.com/cosmos/cosmos-sdk/codec/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" - host "github.com/cosmos/cosmos-sdk/x/ibc/core/24-host" - "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) const ( @@ -12,24 +10,21 @@ const ( ProposalTypeClientUpdate = "ClientUpdate" ) -var ( - _ govtypes.Content = &ClientUpdateProposal{} - _ codectypes.UnpackInterfacesMessage = ClientUpdateProposal{} -) +var _ govtypes.Content = &ClientUpdateProposal{} -// NewClientUpdateProposal creates a new client update proposal. -func NewClientUpdateProposal(title, description, clientID string, header exported.Header) (*ClientUpdateProposal, error) { - any, err := PackHeader(header) - if err != nil { - return nil, err - } +func init() { + govtypes.RegisterProposalType(ProposalTypeClientUpdate) +} +// NewClientUpdateProposal creates a new client update proposal. +func NewClientUpdateProposal(title, description, subjectClientID, substituteClientID string, initialHeight Height) *ClientUpdateProposal { return &ClientUpdateProposal{ - Title: title, - Description: description, - ClientId: clientID, - Header: any, - }, nil + Title: title, + Description: description, + SubjectClientId: subjectClientID, + SubstituteClientId: substituteClientID, + InitialHeight: initialHeight, + } } // GetTitle returns the title of a client update proposal. @@ -51,20 +46,19 @@ func (cup *ClientUpdateProposal) ValidateBasic() error { return err } - if err := host.ClientIdentifierValidator(cup.ClientId); err != nil { + if cup.SubjectClientId == cup.SubstituteClientId { + return sdkerrors.Wrap(ErrInvalidSubstitute, "subject and substitute client identifiers are equal") + } + if _, _, err := ParseClientIdentifier(cup.SubjectClientId); err != nil { return err } - - header, err := UnpackHeader(cup.Header) - if err != nil { + if _, _, err := ParseClientIdentifier(cup.SubstituteClientId); err != nil { return err } - return header.ValidateBasic() -} + if cup.InitialHeight.IsZero() { + return sdkerrors.Wrap(ErrInvalidHeight, "initial height cannot be zero height") + } -// UnpackInterfaces implements the UnpackInterfacesMessage interface. -func (cup ClientUpdateProposal) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { - var header exported.Header - return unpacker.UnpackAny(cup.Header, &header) + return nil } diff --git a/x/ibc/core/02-client/types/proposal_test.go b/x/ibc/core/02-client/types/proposal_test.go index 5a47cf2f01cb..597e5cf8f801 100644 --- a/x/ibc/core/02-client/types/proposal_test.go +++ b/x/ibc/core/02-client/types/proposal_test.go @@ -5,35 +5,15 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" - ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types" + "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing" ) -func (suite *TypesTestSuite) TestNewUpdateClientProposal() { - p, err := types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, clientID, &ibctmtypes.Header{}) - suite.Require().NoError(err) - suite.Require().NotNil(p) - - p, err = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, clientID, nil) - suite.Require().Error(err) - suite.Require().Nil(p) -} - func (suite *TypesTestSuite) TestValidateBasic() { - // use solo machine header for testing - solomachine := ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, clientID, "", 2) - smHeader := solomachine.CreateHeader() - header, err := types.PackHeader(smHeader) - suite.Require().NoError(err) - - // use a different pointer so we don't modify 'header' - smInvalidHeader := solomachine.CreateHeader() - - // a sequence of 0 will fail basic validation - smInvalidHeader.Sequence = 0 - - invalidHeader, err := types.PackHeader(smInvalidHeader) - suite.Require().NoError(err) + subject, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + subjectClientState := suite.chainA.GetClientState(subject) + substitute, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + initialHeight := types.NewHeight(subjectClientState.GetLatestHeight().GetRevisionNumber(), subjectClientState.GetLatestHeight().GetRevisionHeight()+1) testCases := []struct { name string @@ -42,22 +22,32 @@ func (suite *TypesTestSuite) TestValidateBasic() { }{ { "success", - &types.ClientUpdateProposal{ibctesting.Title, ibctesting.Description, clientID, header}, + types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight), true, }, { "fails validate abstract - empty title", - &types.ClientUpdateProposal{"", ibctesting.Description, clientID, header}, + types.NewClientUpdateProposal("", ibctesting.Description, subject, substitute, initialHeight), + false, + }, + { + "subject and substitute use the same identifier", + types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, subject, initialHeight), + false, + }, + { + "invalid subject clientID", + types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, substitute, initialHeight), false, }, { - "fails to unpack header", - &types.ClientUpdateProposal{ibctesting.Title, ibctesting.Description, clientID, nil}, + "invalid substitute clientID", + types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, ibctesting.InvalidID, initialHeight), false, }, { - "fails header validate basic", - &types.ClientUpdateProposal{ibctesting.Title, ibctesting.Description, clientID, invalidHeader}, + "initial height is zero", + types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, types.ZeroHeight()), false, }, } @@ -74,22 +64,15 @@ func (suite *TypesTestSuite) TestValidateBasic() { } } -// tests a client update proposal can be marshaled and unmarshaled, and the -// client state can be unpacked +// tests a client update proposal can be marshaled and unmarshaled func (suite *TypesTestSuite) TestMarshalClientUpdateProposalProposal() { - _, err := types.PackHeader(&ibctmtypes.Header{}) - suite.Require().NoError(err) - // create proposal - header := suite.chainA.CurrentTMClientHeader() - proposal, err := types.NewClientUpdateProposal("update IBC client", "description", "client-id", header) - suite.Require().NoError(err) + proposal := types.NewClientUpdateProposal("update IBC client", "description", "subject", "substitute", types.NewHeight(1, 0)) // create codec ir := codectypes.NewInterfaceRegistry() types.RegisterInterfaces(ir) govtypes.RegisterInterfaces(ir) - ibctmtypes.RegisterInterfaces(ir) cdc := codec.NewProtoCodec(ir) // marshal message @@ -100,8 +83,4 @@ func (suite *TypesTestSuite) TestMarshalClientUpdateProposalProposal() { newProposal := &types.ClientUpdateProposal{} err = cdc.UnmarshalJSON(bz, newProposal) suite.Require().NoError(err) - - // unpack client state - _, err = types.UnpackHeader(newProposal.Header) - suite.Require().NoError(err) } diff --git a/x/ibc/core/exported/client.go b/x/ibc/core/exported/client.go index 656a233b3a9c..3d552b07724a 100644 --- a/x/ibc/core/exported/client.go +++ b/x/ibc/core/exported/client.go @@ -46,7 +46,7 @@ type ClientState interface { CheckHeaderAndUpdateState(sdk.Context, codec.BinaryMarshaler, sdk.KVStore, Header) (ClientState, ConsensusState, error) CheckMisbehaviourAndUpdateState(sdk.Context, codec.BinaryMarshaler, sdk.KVStore, Misbehaviour) (ClientState, error) - CheckProposedHeaderAndUpdateState(sdk.Context, codec.BinaryMarshaler, sdk.KVStore, Header) (ClientState, ConsensusState, error) + CheckSubstituteAndUpdateState(ctx sdk.Context, cdc codec.BinaryMarshaler, subjectClientStore, substituteClientStore sdk.KVStore, substituteClient ClientState, height Height) (ClientState, error) // Upgrade functions // NOTE: proof heights are not included as upgrade to a new revision is expected to pass only on the last diff --git a/x/ibc/core/spec/01_concepts.md b/x/ibc/core/spec/01_concepts.md index 045508999d39..4347fb674154 100644 --- a/x/ibc/core/spec/01_concepts.md +++ b/x/ibc/core/spec/01_concepts.md @@ -53,11 +53,11 @@ submission. ## ClientUpdateProposal -A governance proposal may be passed to update a specified client with a provided -header. This is useful in unfreezing clients or updating expired clients. Each -client is expected to implement this functionality. A client may choose to disallow -an update by a governance proposal by returning an error in the client state function -'CheckProposedHeaderAndUpdateState'. +A governance proposal may be passed to update a specified client using another client +known as the "substitute client". This is useful in unfreezing clients or updating +expired clients, thereby making the effected channels active again. Each client is +expected to implement this functionality. A client may choose to disallow an update +by a governance proposal by returning an error in the client state function 'CheckSubstituteAndUpdateState'. The localhost client cannot be updated by a governance proposal. @@ -68,9 +68,21 @@ be updated later. The tendermint client has two flags update flags, 'AllowUpdateAfterExpiry' and 'AllowUpdateAfterMisbehaviour'. The former flag can only be used to unexpire clients. The latter flag can be used to unfreeze a client and if necessary it will also unexpire the client. -It is advised to let a client expire if it has become frozen before proposing a new header. -This is to avoid the client from becoming refrozen if the misbehaviour evidence has not -expired. These boolean flags are set upon client creation and cannot be updated later. +It is best practice to initialize a new substitute client instead of using an existing one +This avoids potential issues of the substitute becoming frozen due to misbehaviour or the +subject client becoming refrozen due to misbehaviour not being expired at the time the +proposal passes. These boolean flags are set upon client creation and cannot be updated later. + +The `CheckSubstituteAndUpdateState` function provides the light client with its own client +store, the client store of the substitute, the substitute client state, and the intitial +height that should be used when referring to the substitute client. Most light client +implementations should copy consensus states from the substitute to the subject, but +are not required to do so. Light clients may copy informationa as they deem necessary. + +It is not recommended to use a substitute client in normal operations since the subject +light client will be given unrestricted access to the substitute client store. Governance +should not pass votes which enable byzantine light client modules from modifying the state +of the substitute. ## IBC Client Heights diff --git a/x/ibc/core/spec/03_state_transitions.md b/x/ibc/core/spec/03_state_transitions.md index be3b508b795c..518ff9247b97 100644 --- a/x/ibc/core/spec/03_state_transitions.md +++ b/x/ibc/core/spec/03_state_transitions.md @@ -27,8 +27,9 @@ parameters and if applicable will update to the new light client implementation. ## Client Update Proposal -An Update Client Proposal will unfreeze a client and set an updated `ClientState` and a new -`ConsensusState`. +An Update Client Proposal will unfreeze a client (if necessary) and set an updated `ClientState`. +The light client may make optional modifications to the client prefixed store of the subject client +including copying `ConsensusStates` from the substitute to the subject. ## Connection Open Init diff --git a/x/ibc/light-clients/06-solomachine/spec/01_concepts.md b/x/ibc/light-clients/06-solomachine/spec/01_concepts.md index fd8a4f71b20e..de486b71b1a8 100644 --- a/x/ibc/light-clients/06-solomachine/spec/01_concepts.md +++ b/x/ibc/light-clients/06-solomachine/spec/01_concepts.md @@ -129,17 +129,14 @@ If the update is successful: An update by a governance proposal will only succeed if: -- the header provided is parseable to solo machine header +- the substitute provided is parseable to solo machine client state - the `AllowUpdateAfterProposal` client parameter is set to `true` -- the new header public key does not equal the consensus state public key +- the new consensus state public key does not equal the current consensus state public key If the update is successful: -- the public key is updated -- the diversifier is updated -- the timestamp is updated -- the sequence is updated -- the new consensus state is set in the client state +- the subject client state is updated to the substitute client state +- the subject consensus state is updated to the substitute consensus state - the client is unfrozen (if it was previously frozen) ## Misbehaviour diff --git a/x/ibc/light-clients/06-solomachine/spec/03_state_transitions.md b/x/ibc/light-clients/06-solomachine/spec/03_state_transitions.md index 3ca4b7017acf..48a1e18f1c90 100644 --- a/x/ibc/light-clients/06-solomachine/spec/03_state_transitions.md +++ b/x/ibc/light-clients/06-solomachine/spec/03_state_transitions.md @@ -24,11 +24,8 @@ A successful update of a solo machine light client by a header will result in: A successful update of a solo machine light client by a governance proposal will result in: -- the public key being updated to the new public key provided by the header. -- the diversifier being updated to the new diviersifier provided by the header. -- the timestamp being updated to the new timestamp provided by the header. -- the sequence being set to the new sequence provided by the header. -- the consensus state being updated (consensus state stores the public key, diversifier, and timestamp) +- the client state being updated to the substitute client state +- the consensus state being updated to the substitute consensus state (consensus state stores the public key, diversifier, and timestamp) - the frozen sequence being set to zero (client is unfrozen if it was previously frozen). ## Upgrade diff --git a/x/ibc/light-clients/06-solomachine/types/proposal_handle.go b/x/ibc/light-clients/06-solomachine/types/proposal_handle.go index b55e612b3c96..e38155b23615 100644 --- a/x/ibc/light-clients/06-solomachine/types/proposal_handle.go +++ b/x/ibc/light-clients/06-solomachine/types/proposal_handle.go @@ -10,56 +10,55 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) -// CheckProposedHeaderAndUpdateState updates the consensus state to the header's sequence and -// public key. An error is returned if the client has been disallowed to be updated by a -// governance proposal, the header cannot be casted to a solo machine header, or the current -// public key equals the new public key. -func (cs ClientState) CheckProposedHeaderAndUpdateState( - ctx sdk.Context, cdc codec.BinaryMarshaler, clientStore sdk.KVStore, - header exported.Header, -) (exported.ClientState, exported.ConsensusState, error) { +// CheckSubstituteAndUpdateState verifies that the subject is allowed to be updated by +// a governance proposal and that the substitute client is a solo machine. +// It will update the consensus state to the substitute's consensus state and +// the sequence to the substitute's current sequence. An error is returned if +// the client has been disallowed to be updated by a governance proposal, +// the substitute is not a solo machine, or the current public key equals +// the new public key. +func (cs ClientState) CheckSubstituteAndUpdateState( + ctx sdk.Context, cdc codec.BinaryMarshaler, subjectClientStore, + _ sdk.KVStore, substituteClient exported.ClientState, + _ exported.Height, +) (exported.ClientState, error) { if !cs.AllowUpdateAfterProposal { - return nil, nil, sdkerrors.Wrapf( + return nil, sdkerrors.Wrapf( clienttypes.ErrUpdateClientFailed, "solo machine client is not allowed to updated with a proposal", ) } - smHeader, ok := header.(*Header) + substituteClientState, ok := substituteClient.(*ClientState) if !ok { - return nil, nil, sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, "header type %T, expected %T", header, &Header{}, + return nil, sdkerrors.Wrapf( + clienttypes.ErrInvalidClientType, "substitute client state type %T, expected %T", substituteClient, &ClientState{}, ) } - consensusPublicKey, err := cs.ConsensusState.GetPubKey() + subjectPublicKey, err := cs.ConsensusState.GetPubKey() if err != nil { - return nil, nil, sdkerrors.Wrap(err, "failed to get consensus public key") + return nil, sdkerrors.Wrap(err, "failed to get consensus public key") } - headerPublicKey, err := smHeader.GetPubKey() + substitutePublicKey, err := substituteClientState.ConsensusState.GetPubKey() if err != nil { - return nil, nil, sdkerrors.Wrap(err, "failed to get header public key") + return nil, sdkerrors.Wrap(err, "failed to get substitute client public key") } - if reflect.DeepEqual(consensusPublicKey, headerPublicKey) { - return nil, nil, sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, "new public key in header equals current public key", + if reflect.DeepEqual(subjectPublicKey, substitutePublicKey) { + return nil, sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, "subject and substitute have the same public key", ) } clientState := &cs - consensusState := &ConsensusState{ - PublicKey: smHeader.NewPublicKey, - Diversifier: smHeader.NewDiversifier, - Timestamp: smHeader.Timestamp, - } - - clientState.Sequence = smHeader.Sequence - clientState.ConsensusState = consensusState + // update to substitute parameters + clientState.Sequence = substituteClientState.Sequence + clientState.ConsensusState = substituteClientState.ConsensusState clientState.FrozenSequence = 0 - return clientState, consensusState, nil + return clientState, nil } diff --git a/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go b/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go index da5c815ecbc8..0113da104497 100644 --- a/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go +++ b/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go @@ -7,8 +7,11 @@ import ( ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing" ) -func (suite *SoloMachineTestSuite) TestCheckProposedHeaderAndUpdateState() { - var header exported.Header +func (suite *SoloMachineTestSuite) TestCheckSubstituteAndUpdateState() { + var ( + subjectClientState *types.ClientState + substituteClientState exported.ClientState + ) // test singlesig and multisig public keys for _, solomachine := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} { @@ -19,21 +22,34 @@ func (suite *SoloMachineTestSuite) TestCheckProposedHeaderAndUpdateState() { expPass bool }{ { - "valid header", func() { - header = solomachine.CreateHeader() + "valid substitute", func() { + subjectClientState.AllowUpdateAfterProposal = true }, true, }, { - "nil header", func() { - header = &ibctmtypes.Header{} + "subject not allowed to be updated", func() { + subjectClientState.AllowUpdateAfterProposal = false }, false, }, { - "header does not update public key", func() { - header = &types.Header{ - Sequence: 1, - NewPublicKey: solomachine.ConsensusState().PublicKey, - } + "substitute is not the solo machine", func() { + substituteClientState = &ibctmtypes.ClientState{} + }, false, + }, + { + "subject public key is nil", func() { + subjectClientState.ConsensusState.PublicKey = nil + }, false, + }, + + { + "substitute public key is nil", func() { + substituteClientState.(*types.ClientState).ConsensusState.PublicKey = nil + }, false, + }, + { + "subject and substitute use the same public key", func() { + substituteClientState.(*types.ClientState).ConsensusState.PublicKey = subjectClientState.ConsensusState.PublicKey }, false, }, } @@ -44,46 +60,27 @@ func (suite *SoloMachineTestSuite) TestCheckProposedHeaderAndUpdateState() { suite.Run(tc.name, func() { suite.SetupTest() - clientState := solomachine.ClientState() + subjectClientState = solomachine.ClientState() + subjectClientState.AllowUpdateAfterProposal = true + substitute := ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, "substitute", "testing", 5) + substituteClientState = substitute.ClientState() tc.malleate() - clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID) - - // all cases should always fail if the client has 'AllowUpdateAfterProposal' set to false - clientState.AllowUpdateAfterProposal = false - cs, consState, err := clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, header) - suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) + subjectClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID) + substituteClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), substitute.ClientID) - clientState.AllowUpdateAfterProposal = true - cs, consState, err = clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, header) + updatedClient, err := subjectClientState.CheckSubstituteAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), subjectClientStore, substituteClientStore, substituteClientState, nil) if tc.expPass { suite.Require().NoError(err) - smConsState, ok := consState.(*types.ConsensusState) - suite.Require().True(ok) - smHeader, ok := header.(*types.Header) - suite.Require().True(ok) - - suite.Require().Equal(cs.(*types.ClientState).ConsensusState, consState) - - headerPubKey, err := smHeader.GetPubKey() - suite.Require().NoError(err) - - consStatePubKey, err := smConsState.GetPubKey() - suite.Require().NoError(err) - - suite.Require().Equal(headerPubKey, consStatePubKey) - suite.Require().Equal(smHeader.NewDiversifier, smConsState.Diversifier) - suite.Require().Equal(smHeader.Timestamp, smConsState.Timestamp) - suite.Require().Equal(smHeader.GetHeight().GetRevisionHeight(), cs.(*types.ClientState).Sequence) + suite.Require().Equal(substituteClientState.(*types.ClientState).ConsensusState, updatedClient.(*types.ClientState).ConsensusState) + suite.Require().Equal(substituteClientState.(*types.ClientState).Sequence, updatedClient.(*types.ClientState).Sequence) + suite.Require().Equal(uint64(0), updatedClient.(*types.ClientState).FrozenSequence) } else { suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) + suite.Require().Nil(updatedClient) } }) } diff --git a/x/ibc/light-clients/07-tendermint/types/proposal_handle.go b/x/ibc/light-clients/07-tendermint/types/proposal_handle.go index 4cd3eb376cbd..c64c52b3f810 100644 --- a/x/ibc/light-clients/07-tendermint/types/proposal_handle.go +++ b/x/ibc/light-clients/07-tendermint/types/proposal_handle.go @@ -1,9 +1,7 @@ package types import ( - "time" - - tmtypes "github.com/tendermint/tendermint/types" + "reflect" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" @@ -12,31 +10,52 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" ) -// CheckProposedHeaderAndUpdateState will try to update the client with the new header if and -// only if the proposal passes and one of the following two conditions is satisfied: -// 1) AllowUpdateAfterExpiry=true and Expire(ctx.BlockTime) = true -// 2) AllowUpdateAfterMisbehaviour and IsFrozen() = true -// In case 2) before trying to update the client, the client will be unfrozen by resetting -// the FrozenHeight to the zero Height. If AllowUpdateAfterMisbehaviour is set to true, -// expired clients will also be updated even if AllowUpdateAfterExpiry is set to false. -// Note, that even if the update happens, it may not be successful. The header may fail -// validation checks and an error will be returned in that case. -func (cs ClientState) CheckProposedHeaderAndUpdateState( - ctx sdk.Context, cdc codec.BinaryMarshaler, clientStore sdk.KVStore, - header exported.Header, -) (exported.ClientState, exported.ConsensusState, error) { - tmHeader, ok := header.(*Header) +// CheckSubstituteAndUpdateState will try to update the client with the state of the +// substitute if and only if the proposal passes and one of the following conditions are +// satisfied: +// 1) AllowUpdateAfterMisbehaviour and IsFrozen() = true +// 2) AllowUpdateAfterExpiry=true and Expire(ctx.BlockTime) = true +// +// The following must always be true: +// - The substitute client is the same type as the subject client +// - The subject and substitute client states match in all parameters (expect frozen height, latest height, and chain-id) +// +// In case 1) before updating the client, the client will be unfrozen by resetting +// the FrozenHeight to the zero Height. If a client is frozen and AllowUpdateAfterMisbehaviour +// is set to true, the client will be unexpired even if AllowUpdateAfterExpiry is set to false. +// Note, that even if the subject is updated to the state of the substitute, an error may be +// returned if the updated client state is invalid or the client is expired. +func (cs ClientState) CheckSubstituteAndUpdateState( + ctx sdk.Context, cdc codec.BinaryMarshaler, subjectClientStore, + substituteClientStore sdk.KVStore, substituteClient exported.ClientState, + initialHeight exported.Height, +) (exported.ClientState, error) { + substituteClientState, ok := substituteClient.(*ClientState) if !ok { - return nil, nil, sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, "expected type %T, got %T", &Header{}, header, + return nil, sdkerrors.Wrapf( + clienttypes.ErrInvalidClient, "expected type %T, got %T", &ClientState{}, substituteClient, ) } + // substitute clients are not allowed to be upgraded during the voting period + // If an upgrade passes before the subject client has been updated, a new proposal must be created + // with an initial height that contains the new revision number. + if substituteClientState.GetLatestHeight().GetRevisionNumber() != initialHeight.GetRevisionNumber() { + return nil, sdkerrors.Wrapf( + clienttypes.ErrInvalidHeight, "substitute client revision number must equal initial height revision number (%d != %d)", + substituteClientState.GetLatestHeight().GetRevisionNumber(), initialHeight.GetRevisionNumber(), + ) + } + + if !IsMatchingClientState(cs, *substituteClientState) { + return nil, sdkerrors.Wrap(clienttypes.ErrInvalidSubstitute, "subject client state does not match substitute client state") + } + // get consensus state corresponding to client state to check if the client is expired - consensusState, err := GetConsensusState(clientStore, cdc, cs.GetLatestHeight()) + consensusState, err := GetConsensusState(subjectClientStore, cdc, cs.GetLatestHeight()) if err != nil { - return nil, nil, sdkerrors.Wrapf( - err, "could not get consensus state from clientstore at height: %d", cs.GetLatestHeight(), + return nil, sdkerrors.Wrapf( + err, "unexpected error: could not get consensus state from clientstore at height: %d", cs.GetLatestHeight(), ) } @@ -44,84 +63,72 @@ func (cs ClientState) CheckProposedHeaderAndUpdateState( case cs.IsFrozen(): if !cs.AllowUpdateAfterMisbehaviour { - return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unfrozen") + return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unfrozen") } // unfreeze the client cs.FrozenHeight = clienttypes.ZeroHeight() - // if the client is expired we unexpire the client using softer validation, otherwise - // full validation on the header is performed. - if cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()) { - return cs.unexpireClient(ctx, clientStore, consensusState, tmHeader, ctx.BlockTime()) + case cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()): + if !cs.AllowUpdateAfterExpiry { + return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unexpired") } - // NOTE: the client may be frozen again since the misbehaviour evidence may - // not be expired yet - return cs.CheckHeaderAndUpdateState(ctx, cdc, clientStore, header) - - case cs.AllowUpdateAfterExpiry && cs.IsExpired(consensusState.Timestamp, ctx.BlockTime()): - return cs.unexpireClient(ctx, clientStore, consensusState, tmHeader, ctx.BlockTime()) - default: - return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal") + return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal") } -} - -// unexpireClient checks if the proposed header is sufficient to update an expired client. -// The client is updated if no error occurs. -func (cs ClientState) unexpireClient( - ctx sdk.Context, clientStore sdk.KVStore, consensusState *ConsensusState, header *Header, currentTimestamp time.Time, -) (exported.ClientState, exported.ConsensusState, error) { + // copy consensus states and processed time from substitute to subject + // starting from initial height and ending on the latest height (inclusive) + for i := initialHeight.GetRevisionHeight(); i <= substituteClientState.GetLatestHeight().GetRevisionHeight(); i++ { + height := clienttypes.NewHeight(substituteClientState.GetLatestHeight().GetRevisionNumber(), i) - // the client is expired and either AllowUpdateAfterMisbehaviour or AllowUpdateAfterExpiry - // is set to true so light validation of the header is executed - if err := cs.checkProposedHeader(consensusState, header, currentTimestamp); err != nil { - return nil, nil, err - } + consensusState, err := GetConsensusState(substituteClientStore, cdc, height) + if err != nil { + // not all consensus states will be filled in + continue + } + SetConsensusState(subjectClientStore, cdc, consensusState, height) - newClientState, consensusState := update(ctx, clientStore, &cs, header) - return newClientState, consensusState, nil -} + processedTime, found := GetProcessedTime(substituteClientStore, height) + if !found { + continue + } + SetProcessedTime(subjectClientStore, height, processedTime) -// checkProposedHeader checks if the Tendermint header is valid for updating a client after -// a passed proposal. -// It returns an error if: -// - the header provided is not parseable to tendermint types -// - header height is less than or equal to the latest client state height -// - signed tendermint header is invalid -// - header timestamp is less than or equal to the latest consensus state timestamp -// - header timestamp is expired -// NOTE: header.ValidateBasic is called in the 02-client proposal handler. Additional checks -// on the validator set and the validator set hash are done in header.ValidateBasic. -func (cs ClientState) checkProposedHeader(consensusState *ConsensusState, header *Header, currentTimestamp time.Time) error { - tmSignedHeader, err := tmtypes.SignedHeaderFromProto(header.SignedHeader) - if err != nil { - return sdkerrors.Wrap(err, "signed header in not tendermint signed header type") } - if !header.GetTime().After(consensusState.Timestamp) { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header timestamp is less than or equal to latest consensus state timestamp (%s ≤ %s)", header.GetTime(), consensusState.Timestamp) + cs.LatestHeight = substituteClientState.LatestHeight + + // validate the updated client and ensure it isn't expired + if err := cs.Validate(); err != nil { + return nil, sdkerrors.Wrap(err, "unexpected error: updated subject client state is invalid") } - // assert header height is newer than latest client state - if header.GetHeight().LTE(cs.GetLatestHeight()) { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header height ≤ consensus state height (%s ≤ %s)", header.GetHeight(), cs.GetLatestHeight(), + latestConsensusState, err := GetConsensusState(subjectClientStore, cdc, cs.GetLatestHeight()) + if err != nil { + return nil, sdkerrors.Wrapf( + err, "unexpected error: could not get consensus state for updated subject client from clientstore at height: %d", cs.GetLatestHeight(), ) } - if err := tmSignedHeader.ValidateBasic(cs.GetChainID()); err != nil { - return sdkerrors.Wrap(err, "signed header failed basic validation") + if cs.IsExpired(latestConsensusState.Timestamp, ctx.BlockTime()) { + return nil, sdkerrors.Wrap(clienttypes.ErrInvalidClient, "updated subject client is expired") } - if cs.IsExpired(header.GetTime(), currentTimestamp) { - return sdkerrors.Wrap(clienttypes.ErrInvalidHeader, "header timestamp is already expired") - } + return &cs, nil +} - return nil +// IsMatchingClientState returns true if all the client state parameters match +// except for frozen height, latest height, and chain-id. +func IsMatchingClientState(subject, substitute ClientState) bool { + // zero out parameters which do not need to match + subject.LatestHeight = clienttypes.ZeroHeight() + subject.FrozenHeight = clienttypes.ZeroHeight() + substitute.LatestHeight = clienttypes.ZeroHeight() + substitute.FrozenHeight = clienttypes.ZeroHeight() + subject.ChainId = "" + substitute.ChainId = "" + + return reflect.DeepEqual(subject, substitute) } diff --git a/x/ibc/light-clients/07-tendermint/types/proposal_handle_test.go b/x/ibc/light-clients/07-tendermint/types/proposal_handle_test.go index 6863ad1d1c80..c1fb05413d9d 100644 --- a/x/ibc/light-clients/07-tendermint/types/proposal_handle_test.go +++ b/x/ibc/light-clients/07-tendermint/types/proposal_handle_test.go @@ -1,6 +1,8 @@ package types_test import ( + "time" + clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" "github.com/cosmos/cosmos-sdk/x/ibc/core/exported" "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types" @@ -11,38 +13,120 @@ var ( frozenHeight = clienttypes.NewHeight(0, 1) ) -// sanity checks -func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateStateBasic() { - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - clientState := suite.chainA.GetClientState(clientA).(*types.ClientState) - clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) - - // use nil header - cs, consState, err := clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, nil) - suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) - - clientState.LatestHeight = clientState.LatestHeight.Increment().(clienttypes.Height) - - // consensus state for latest height does not exist - cs, consState, err = clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, suite.chainA.LastHeader) - suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) +func (suite *TendermintTestSuite) TestCheckSubstituteUpdateStateBasic() { + var ( + substitute string + substituteClientState exported.ClientState + initialHeight clienttypes.Height + ) + testCases := []struct { + name string + malleate func() + }{ + { + "solo machine used for substitute", func() { + substituteClientState = ibctesting.NewSolomachine(suite.T(), suite.cdc, "solo machine", "", 1).ClientState() + }, + }, + { + "initial height and substitute revision numbers do not match", func() { + initialHeight = clienttypes.NewHeight(substituteClientState.GetLatestHeight().GetRevisionNumber()+1, 1) + }, + }, + { + "non-matching substitute", func() { + substitute, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + substituteClientState = suite.chainA.GetClientState(substitute).(*types.ClientState) + tmClientState, ok := substituteClientState.(*types.ClientState) + suite.Require().True(ok) + + tmClientState.ChainId = tmClientState.ChainId + "different chain" + }, + }, + { + "updated client is invalid - revision height is zero", func() { + substitute, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + substituteClientState = suite.chainA.GetClientState(substitute).(*types.ClientState) + tmClientState, ok := substituteClientState.(*types.ClientState) + suite.Require().True(ok) + // match subject + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + + // will occur. This case should never occur (caught by upstream checks) + initialHeight = clienttypes.NewHeight(5, 0) + tmClientState.LatestHeight = clienttypes.NewHeight(5, 0) + }, + }, + { + "updated client is expired", func() { + substitute, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + substituteClientState = suite.chainA.GetClientState(substitute).(*types.ClientState) + tmClientState, ok := substituteClientState.(*types.ClientState) + suite.Require().True(ok) + initialHeight = tmClientState.LatestHeight + + // match subject + tmClientState.AllowUpdateAfterMisbehaviour = true + tmClientState.AllowUpdateAfterExpiry = true + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState) + + // update substitute a few times + err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + suite.Require().NoError(err) + substituteClientState = suite.chainA.GetClientState(substitute) + + err = suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) + suite.Require().NoError(err) + + suite.chainA.ExpireClient(tmClientState.TrustingPeriod) + suite.chainB.ExpireClient(tmClientState.TrustingPeriod) + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + + substituteClientState = suite.chainA.GetClientState(substitute) + }, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + + suite.SetupTest() // reset + + subject, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + subjectClientState := suite.chainA.GetClientState(subject).(*types.ClientState) + subjectClientState.AllowUpdateAfterMisbehaviour = true + subjectClientState.AllowUpdateAfterExpiry = true + + // expire subject + suite.chainA.ExpireClient(subjectClientState.TrustingPeriod) + suite.chainB.ExpireClient(subjectClientState.TrustingPeriod) + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + + tc.malleate() + + subjectClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), subject) + substituteClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), substitute) + + updatedClient, err := subjectClientState.CheckSubstituteAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), subjectClientStore, substituteClientStore, substituteClientState, initialHeight) + suite.Require().Error(err) + suite.Require().Nil(updatedClient) + }) + } } // to expire clients, time needs to be fast forwarded on both chainA and chainB. // this is to prevent headers from failing when attempting to update later. -func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { +func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { testCases := []struct { name string AllowUpdateAfterExpiry bool AllowUpdateAfterMisbehaviour bool FreezeClient bool ExpireClient bool - expPassUnfreeze bool // expected result using a header that passes stronger validation - expPassUnexpire bool // expected result using a header that passes weaker validation + expPass bool }{ { name: "not allowed to be updated, not frozen or expired", @@ -50,8 +134,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: false, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "not allowed to be updated, client is frozen", @@ -59,8 +142,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: true, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "not allowed to be updated, client is expired", @@ -68,8 +150,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: false, ExpireClient: true, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "not allowed to be updated, client is frozen and expired", @@ -77,8 +158,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: true, ExpireClient: true, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "allowed to be updated only after misbehaviour, not frozen or expired", @@ -86,17 +166,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: false, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, - }, - { - name: "PASS: allowed to be updated only after misbehaviour, client is frozen", - AllowUpdateAfterExpiry: false, - AllowUpdateAfterMisbehaviour: true, - FreezeClient: true, - ExpireClient: false, - expPassUnfreeze: true, - expPassUnexpire: false, + expPass: false, }, { name: "allowed to be updated only after misbehaviour, client is expired", @@ -104,17 +174,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: false, ExpireClient: true, - expPassUnfreeze: false, - expPassUnexpire: false, - }, - { - name: "allowed to be updated only after misbehaviour, client is frozen and expired", - AllowUpdateAfterExpiry: false, - AllowUpdateAfterMisbehaviour: true, - FreezeClient: true, - ExpireClient: true, - expPassUnfreeze: true, - expPassUnexpire: true, + expPass: false, }, { name: "allowed to be updated only after expiry, not frozen or expired", @@ -122,8 +182,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: false, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "allowed to be updated only after expiry, client is frozen", @@ -131,8 +190,23 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: true, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, + }, + { + name: "PASS: allowed to be updated only after misbehaviour, client is frozen", + AllowUpdateAfterExpiry: false, + AllowUpdateAfterMisbehaviour: true, + FreezeClient: true, + ExpireClient: false, + expPass: true, + }, + { + name: "PASS: allowed to be updated only after misbehaviour, client is frozen and expired", + AllowUpdateAfterExpiry: false, + AllowUpdateAfterMisbehaviour: true, + FreezeClient: true, + ExpireClient: true, + expPass: true, }, { name: "PASS: allowed to be updated only after expiry, client is expired", @@ -140,8 +214,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: false, ExpireClient: true, - expPassUnfreeze: true, - expPassUnexpire: true, + expPass: true, }, { name: "allowed to be updated only after expiry, client is frozen and expired", @@ -149,8 +222,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: false, FreezeClient: true, ExpireClient: true, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "allowed to be updated after expiry and misbehaviour, not frozen or expired", @@ -158,8 +230,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: false, ExpireClient: false, - expPassUnfreeze: false, - expPassUnexpire: false, + expPass: false, }, { name: "PASS: allowed to be updated after expiry and misbehaviour, client is frozen", @@ -167,8 +238,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: true, ExpireClient: false, - expPassUnfreeze: true, - expPassUnexpire: false, + expPass: true, }, { name: "PASS: allowed to be updated after expiry and misbehaviour, client is expired", @@ -176,8 +246,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: false, ExpireClient: true, - expPassUnfreeze: true, - expPassUnexpire: true, + expPass: true, }, { name: "PASS: allowed to be updated after expiry and misbehaviour, client is frozen and expired", @@ -185,8 +254,7 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { AllowUpdateAfterMisbehaviour: true, FreezeClient: true, ExpireClient: true, - expPassUnfreeze: true, - expPassUnexpire: true, + expPass: true, }, } @@ -201,67 +269,67 @@ func (suite *TendermintTestSuite) TestCheckProposedHeaderAndUpdateState() { // start by testing unexpiring the client suite.SetupTest() // reset - // construct client state based on test case parameters - clientA, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - clientState := suite.chainA.GetClientState(clientA).(*types.ClientState) - clientState.AllowUpdateAfterExpiry = tc.AllowUpdateAfterExpiry - clientState.AllowUpdateAfterMisbehaviour = tc.AllowUpdateAfterMisbehaviour + // construct subject using test case parameters + subject, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + subjectClientState := suite.chainA.GetClientState(subject).(*types.ClientState) + subjectClientState.AllowUpdateAfterExpiry = tc.AllowUpdateAfterExpiry + subjectClientState.AllowUpdateAfterMisbehaviour = tc.AllowUpdateAfterMisbehaviour + + // apply freezing or expiry as determined by the test case if tc.FreezeClient { - clientState.FrozenHeight = frozenHeight + subjectClientState.FrozenHeight = frozenHeight } if tc.ExpireClient { - suite.chainA.ExpireClient(clientState.TrustingPeriod) - suite.chainB.ExpireClient(clientState.TrustingPeriod) + suite.chainA.ExpireClient(subjectClientState.TrustingPeriod) + suite.chainB.ExpireClient(subjectClientState.TrustingPeriod) suite.coordinator.CommitBlock(suite.chainA, suite.chainB) } - // use next header for chainB to unfreeze client on chainA - unfreezeClientHeader, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientA) - suite.Require().NoError(err) + // construct the substitute to match the subject client + // NOTE: the substitute is explicitly created after the freezing or expiry occurs, + // primarily to prevent the substitute from becoming frozen. It also should be + // the natural flow of events in practice. The subject will become frozen/expired + // and a substitute will be created along with a governance proposal as a response - clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) - cs, consState, err := clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, unfreezeClientHeader) + substitute, _ := suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + substituteClientState := suite.chainA.GetClientState(substitute).(*types.ClientState) + substituteClientState.AllowUpdateAfterExpiry = tc.AllowUpdateAfterExpiry + substituteClientState.AllowUpdateAfterMisbehaviour = tc.AllowUpdateAfterMisbehaviour + suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, substituteClientState) - if tc.expPassUnfreeze { + initialHeight := substituteClientState.GetLatestHeight() + + // update substitute a few times + for i := 0; i < 3; i++ { + err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, substitute, exported.Tendermint) suite.Require().NoError(err) - suite.Require().Equal(clienttypes.ZeroHeight(), cs.GetFrozenHeight()) - suite.Require().NotNil(consState) - } else { - suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) + // skip a block + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) } - // use next header for chainB to unexpire clients but with empty trusted heights - // and validators. Update chainB time so header won't be expired. - unexpireClientHeader, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientA) - suite.Require().NoError(err) - unexpireClientHeader.TrustedHeight = clienttypes.ZeroHeight() - unexpireClientHeader.TrustedValidators = nil + // get updated substitue + substituteClientState = suite.chainA.GetClientState(substitute).(*types.ClientState) - clientStore = suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) - cs, consState, err = clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, unexpireClientHeader) + subjectClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), subject) + substituteClientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), substitute) + updatedClient, err := subjectClientState.CheckSubstituteAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), subjectClientStore, substituteClientStore, substituteClientState, initialHeight) - if tc.expPassUnexpire { + if tc.expPass { suite.Require().NoError(err) - suite.Require().NotNil(cs) - suite.Require().NotNil(consState) + suite.Require().Equal(clienttypes.ZeroHeight(), updatedClient.GetFrozenHeight()) } else { suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) + suite.Require().Nil(updatedClient) } + }) } } -// test softer validation on headers used for unexpiring clients -func (suite *TendermintTestSuite) TestCheckProposedHeader() { +func (suite *TendermintTestSuite) TestIsMatchingClientState() { var ( - header *types.Header - clientState *types.ClientState - clientA string - err error + subject, substitute string + subjectClientState, substituteClientState *types.ClientState ) testCases := []struct { @@ -270,46 +338,33 @@ func (suite *TendermintTestSuite) TestCheckProposedHeader() { expPass bool }{ { - "success", func() {}, true, + "matching clients", func() { + subjectClientState = suite.chainA.GetClientState(subject).(*types.ClientState) + substituteClientState = suite.chainA.GetClientState(substitute).(*types.ClientState) + }, true, }, { - "invalid signed header", func() { - header.SignedHeader = nil - }, false, + "matching, frozen height is not used in check for equality", func() { + subjectClientState.FrozenHeight = frozenHeight + substituteClientState.FrozenHeight = clienttypes.ZeroHeight() + }, true, }, { - "header time is less than or equal to consensus state timestamp", func() { - consensusState, found := suite.chainA.GetConsensusState(clientA, clientState.GetLatestHeight()) - suite.Require().True(found) - consensusState.(*types.ConsensusState).Timestamp = header.GetTime() - suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, clientState.GetLatestHeight(), consensusState) - - // update block time so client is expired - suite.chainA.ExpireClient(clientState.TrustingPeriod) - suite.chainB.ExpireClient(clientState.TrustingPeriod) - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) - }, false, + "matching, latest height is not used in check for equality", func() { + subjectClientState.LatestHeight = clienttypes.NewHeight(0, 10) + substituteClientState.FrozenHeight = clienttypes.ZeroHeight() + }, true, }, { - "header height is not newer than client state", func() { - consensusState, found := suite.chainA.GetConsensusState(clientA, clientState.GetLatestHeight()) - suite.Require().True(found) - clientState.LatestHeight = header.GetHeight().(clienttypes.Height) - suite.chainA.App.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientA, clientState.GetLatestHeight(), consensusState) - - }, false, + "matching, chain id is different", func() { + subjectClientState.ChainId = "bitcoin" + substituteClientState.ChainId = "ethereum" + }, true, }, { - "signed header failed validate basic - wrong chain ID", func() { - clientState.ChainId = ibctesting.InvalidID - }, false, - }, - { - "header is already expired", func() { - // expire client - suite.chainA.ExpireClient(clientState.TrustingPeriod) - suite.chainB.ExpireClient(clientState.TrustingPeriod) - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + "not matching, trusting period is different", func() { + subjectClientState.TrustingPeriod = time.Duration(time.Hour * 10) + substituteClientState.TrustingPeriod = time.Duration(time.Hour * 1) }, false, }, } @@ -320,37 +375,13 @@ func (suite *TendermintTestSuite) TestCheckProposedHeader() { suite.Run(tc.name, func() { suite.SetupTest() // reset - clientA, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) - clientState = suite.chainA.GetClientState(clientA).(*types.ClientState) - clientState.AllowUpdateAfterExpiry = true - clientState.AllowUpdateAfterMisbehaviour = false - - // expire client - suite.chainA.ExpireClient(clientState.TrustingPeriod) - suite.chainB.ExpireClient(clientState.TrustingPeriod) - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) - - // use next header for chainB to unexpire clients but with empty trusted heights - // and validators. - header, err = suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, clientA) - suite.Require().NoError(err) - header.TrustedHeight = clienttypes.ZeroHeight() - header.TrustedValidators = nil + subject, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + substitute, _ = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) tc.malleate() - clientStore := suite.chainA.App.IBCKeeper.ClientKeeper.ClientStore(suite.chainA.GetContext(), clientA) - cs, consState, err := clientState.CheckProposedHeaderAndUpdateState(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), clientStore, header) + suite.Require().Equal(tc.expPass, types.IsMatchingClientState(*subjectClientState, *substituteClientState)) - if tc.expPass { - suite.Require().NoError(err) - suite.Require().NotNil(cs) - suite.Require().NotNil(consState) - } else { - suite.Require().Error(err) - suite.Require().Nil(cs) - suite.Require().Nil(consState) - } }) } } diff --git a/x/ibc/light-clients/07-tendermint/types/store.go b/x/ibc/light-clients/07-tendermint/types/store.go index 8b2720c5a995..7d6a841b89f1 100644 --- a/x/ibc/light-clients/07-tendermint/types/store.go +++ b/x/ibc/light-clients/07-tendermint/types/store.go @@ -14,6 +14,13 @@ import ( // KeyProcessedTime is appended to consensus state key to store the processed time var KeyProcessedTime = []byte("/processedTime") +// SetConsensusState stores the consensus state at the given height. +func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryMarshaler, consensusState *ConsensusState, height exported.Height) { + key := host.ConsensusStateKey(height) + val := clienttypes.MustMarshalConsensusState(cdc, consensusState) + clientStore.Set(key, val) +} + // GetConsensusState retrieves the consensus state from the client prefixed // store. An error is returned if the consensus state does not exist. func GetConsensusState(store sdk.KVStore, cdc codec.BinaryMarshaler, height exported.Height) (*ConsensusState, error) { 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 e0ba7a2f0b20..5a4a41a17988 100644 --- a/x/ibc/light-clients/09-localhost/types/client_state.go +++ b/x/ibc/light-clients/09-localhost/types/client_state.go @@ -107,12 +107,13 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( return nil, sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "cannot submit misbehaviour to localhost client") } -// CheckProposedHeaderAndUpdateState returns an error. The localhost cannot be modified by +// CheckSubstituteAndUpdateState returns an error. The localhost cannot be modified by // proposals. -func (cs ClientState) CheckProposedHeaderAndUpdateState( - ctx sdk.Context, _ codec.BinaryMarshaler, _ sdk.KVStore, _ exported.Header, -) (exported.ClientState, exported.ConsensusState, error) { - return nil, nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "cannot update localhost client with a proposal") +func (cs ClientState) CheckSubstituteAndUpdateState( + ctx sdk.Context, _ codec.BinaryMarshaler, _, _ sdk.KVStore, + _ exported.ClientState, _ exported.Height, +) (exported.ClientState, error) { + return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "cannot update localhost client with a proposal") } // VerifyUpgradeAndUpdateState returns an error since localhost cannot be upgraded diff --git a/x/ibc/light-clients/09-localhost/types/client_state_test.go b/x/ibc/light-clients/09-localhost/types/client_state_test.go index 13a1367d5c21..bc58f62539be 100644 --- a/x/ibc/light-clients/09-localhost/types/client_state_test.go +++ b/x/ibc/light-clients/09-localhost/types/client_state_test.go @@ -170,10 +170,9 @@ func (suite *LocalhostTestSuite) TestMisbehaviourAndUpdateState() { func (suite *LocalhostTestSuite) TestProposedHeaderAndUpdateState() { clientState := types.NewClientState("chainID", clientHeight) - cs, consState, err := clientState.CheckProposedHeaderAndUpdateState(suite.ctx, nil, nil, nil) + cs, err := clientState.CheckSubstituteAndUpdateState(suite.ctx, nil, nil, nil, nil, nil) suite.Require().Error(err) suite.Require().Nil(cs) - suite.Require().Nil(consState) } func (suite *LocalhostTestSuite) TestVerifyConnectionState() {