Modify IBC client governance unfreezing to reflect ADR changes

CHANGELOG.md

@@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### State Machine Breaking
 
 * (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.
 
 ### Improvements
 

docs/core/proto-docs.md

@@ -7565,17 +7565,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 and frozen height).
+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,44 @@
+<!--
+order: 5
+-->
+
+# Governance Proposals
+
+In uncommon situtations, a highly valued client may become frozen due to uncontrollable 
+circumstances. For example, the chain the light client represents might fork into two
+new chains. The light client will now have two valid, but conflicting headers at the same
+height. 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 validators can arbitarily agree to make state transitions that defy the written code, a 
+governance proposal has been added to ease the complexity of unfreezing or updating clients
+which have become "stuck". Unfreezing clients, re-enables all of the channels built upon that
+client. This may result in recovery of otherwise lost funds. 
+
+In the case that a highly valued light client is frozen, expired, or rendered unupdateable, 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. 
+
+Example:
+
+There exists a very common client called "ethereum-0" which is a light client for the Ethereum chain.
+Ethereum undergoes a fork, creating two valid headers. As a result, misbehaviour evidence is submitted 
+to "ethereum-0" rendering it frozen. The Cosmos Hub decides that "Ethereum 2.0" fork is the desired 
+counterparty chain for this client. The proposer of the governance proposal to unfreeze "ethereum-0"
+then creates a new "ethereum-{N}" client with the exact same parameters (except for latest height, 
+frozen height, and chain-id). Since the substitute client was created *after* the fork, there are
+no conflicting headers to freeze the client. During the voting period, the substitute client can
+constantly be updated to prevent the subject from being expired at the end of the voting period, 
+if the vote passes. If the vote passed, the "ethereum-0" client would have an updated chain-id 
+and consensus states copied directly from the substitute client's store. 
+
+
+

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 and frozen height).
+// 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

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

@@ -10,40 +10,60 @@ 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.
+//
+// NOTE: Substitute clients with revision numbers not equal to the revision
+// number of the subject client is explicityly disallowed. We cannot support
+// this until there is a way to range query for the all the consensus
+// states which occurred between two IBC Revision heights.
+// https://github.com/cosmos/cosmos-sdk/issues/7712
 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)
+	}
+
+	// substitute clients with height across revision numbers is not allowed
+	if subjectClientState.GetLatestHeight().GetRevisionNumber() != substituteClientState.GetLatestHeight().GetRevisionNumber() {
+		return sdkerrors.Wrapf(types.ErrInvalidHeight, "subject client state and substitute client state must have the same revision number (%d != %d)", subjectClientState.GetLatestHeight().GetRevisionNumber(), substituteClientState.GetLatestHeight().GetRevisionNumber())
 	}
 
-	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 +73,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()),
 		),
 	)
 

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,65 @@ 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)
+				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,
 		},
 		{
-			"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,
+		},
+
+		{
+			"subject and substitute use different revision numbers", func() {
+				tmClientState, ok := substituteClientState.(*ibctmtypes.ClientState)
+				suite.Require().True(ok)
+				tmClientState.LatestHeight = clienttypes.NewHeight(tmClientState.GetLatestHeight().GetRevisionNumber()+1, tmClientState.GetLatestHeight().GetRevisionHeight())
+				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState)
+
+				content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
+			}, false,
+		},
+		{
+			"update fails, client is not frozen or expired", func() {
+				content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
 			}, false,
 		},
 	}
@@ -78,6 +93,16 @@ 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)
+
 			tc.malleate()
 
 			err = suite.chainA.App.IBCKeeper.ClientKeeper.ClientUpdateProposal(suite.chainA.GetContext(), content)