Modify IBC client governance unfreezing to reflect ADR changes (#8405)

* update proto files

* make proto-gen

* update clienttypes

* update localhost and solo machine

* refactor tm client proposal handling

* copy metadata

* self review fixes

* update 02-client keeper tests

* fix 02-client type tests

* fix localhost and solomachine tests

* begin updating tm tests

* partially fix tm tests

* increase codecov

* add more tests

* add changelog

* update specs

* add docs

* fix test

* modify adr

* allow modified chain-ids

* add CLI command

* fix typos

* fix lint

* Apply suggestions from code review

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* update docs, rm example

* Update docs/ibc/proposals.md

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

* update height checks to reflect chain-id changes cc @AdityaSripal

* Apply suggestions from code review

Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

* Apply suggestions from code review

Co-authored-by: Aditya <adityasripal@gmail.com>

* address most of @AdityaSripal suggestions

* update docs per review suggestions

* Update x/ibc/core/02-client/types/proposal.go

* add proposal handler

* register proposal type

* register proposal on codec

* fix routing

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>
Co-authored-by: Aditya <adityasripal@gmail.com>

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
 

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)

docs/core/proto-docs.md

@@ -7949,17 +7949,20 @@ client.
 <a name="ibc.core.client.v1.ClientUpdateProposal"></a>
 
 ### 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 |
 
 
 

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.

docs/ibc/proposals.md

@@ -0,0 +1,42 @@
+<!--
+order: 5
+-->
+
+# 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. 

proto/ibc/core/client/v1/client.proto

@@ -33,19 +33,23 @@ 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
   string title = 1;
   // 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

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,

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
+}

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)

x/ibc/core/02-client/keeper/proposal.go

@@ -10,40 +10,49 @@ 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(
 			[]string{"ibc", "client", "update"},
 			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()),
 		),
 	)