simplify UpdateClient proposal (#181)

* simplify UpdateClient proposal to copy over latest consensus state

* fix tests and update godoc

* update docs

* code coverage

* changelog

* remove unused code

* set subject chain-id using substitute

* fix tests

* Update modules/core/02-client/keeper/proposal.go

Co-authored-by: Aditya <[email protected]>

CHANGELOG.md

@@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (02-client) [\#181](https://github.com/cosmos/ibc-go/pull/181) Remove 'InitialHeight' from UpdateClient Proposal. Only copy over latest consensus state from substitute client.
 * (06-solomachine) [\#169](https://github.com/cosmos/ibc-go/pull/169) Change FrozenSequence to boolean in solomachine ClientState. The solo machine proto package has been bumped from `v1` to `v2`.
 * (module/core/02-client) [\#165](https://github.com/cosmos/ibc-go/pull/165) Remove GetFrozenHeight from the ClientState interface. 
 * (modules) [\#166](https://github.com/cosmos/ibc-go/pull/166) Remove GetHeight from the misbehaviour interface. The `consensus_height` attribute has been removed from Misbehaviour events.

docs/architecture/adr-026-ibc-client-recovery-mechanisms.md

@@ -5,6 +5,7 @@
 - 2020/06/23: Initial version
 - 2020/08/06: Revisions per review & to reference version
 - 2021/01/15: Revision to support substitute clients for unfreezing
+- 2021/05/20: Revision to simplify consensus state copying, remove initial height
 
 ## Status
 
@@ -42,11 +43,10 @@ We elect not to deal with chains which have actually halted, which is necessaril
 1. Require Tendermint light clients (ICS 07) to expose the following additional state mutation functions
     1. `Unfreeze()`, which unfreezes a light client after misbehaviour and clears any frozen height previously set
 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. Extend the base `Proposal` with two client identifiers (`string`). 
     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, 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. If this governance proposal passes, the client on trial will be updated to the latest state of the substitute, if and only if:
         1. `allow_governance_override_after_expiry` is true and the client has expired (`Expired()` returns true)
         1. `allow_governance_override_after_misbehaviour` is true and the client has been frozen (`Frozen()` returns true)
             1. In this case, additionally, the client is unfrozen by calling `Unfreeze()`

docs/ibc/proposals.md

@@ -32,11 +32,10 @@ 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. 
+proposal includes the client identifier for the subject and the client identifier for a substitute
+client. Light client implementations may implement custom updating logic, but in most cases, 
+the subject will be updated to the latest consensus state of 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. 

docs/ibc/proto-docs.md

@@ -477,11 +477,9 @@ client.
 
 ### ClientUpdateProposal
 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).
+client's latest consensus state is copied over to the subject client. 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).
 
 
 | Field | Type | Label | Description |
@@ -490,7 +488,6 @@ must also be valid (cannot be expired).
 | `description` | [string](#string) |  | the description of the proposal |
 | `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 |
 
 
 

modules/core/02-client/client/cli/tx.go

@@ -237,12 +237,12 @@ func NewUpgradeClientCmd() *cobra.Command {
 // 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]",
+		Use:   "update-client [subject-client-id] [substitute-client-id] [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.",
+			"Please specify the substitute client the subject client will be updated to.",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx, err := client.GetClientTxContext(cmd)
 			if err != nil {
@@ -262,12 +262,7 @@ func NewCmdSubmitUpdateClientProposal() *cobra.Command {
 			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)
+			content := types.NewClientUpdateProposal(title, description, subjectClientID, substituteClientID)
 
 			from := clientCtx.GetFromAddress()
 

modules/core/02-client/keeper/proposal.go

@@ -11,14 +11,13 @@ import (
 )
 
 // 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
+// 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.
+// store. The substitute must be Active and the subject must not be Active.
 func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdateProposal) error {
 	if p.SubjectClientId == exported.Localhost || p.SubstituteClientId == exported.Localhost {
 		return sdkerrors.Wrap(types.ErrInvalidUpdateClientProposal, "cannot update localhost client with proposal")
@@ -29,16 +28,28 @@ func (k Keeper) ClientUpdateProposal(ctx sdk.Context, p *types.ClientUpdatePropo
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "subject client with ID %s", p.SubjectClientId)
 	}
 
-	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)
+	subjectClientStore := k.ClientStore(ctx, p.SubjectClientId)
+
+	if status := subjectClientState.Status(ctx, subjectClientStore, k.cdc); status == exported.Active {
+		return sdkerrors.Wrap(types.ErrInvalidUpdateClientProposal, "cannot update Active subject client")
 	}
 
 	substituteClientState, found := k.GetClientState(ctx, p.SubstituteClientId)
 	if !found {
 		return sdkerrors.Wrapf(types.ErrClientNotFound, "substitute client with ID %s", p.SubstituteClientId)
 	}
 
-	clientState, err := subjectClientState.CheckSubstituteAndUpdateState(ctx, k.cdc, k.ClientStore(ctx, p.SubjectClientId), k.ClientStore(ctx, p.SubstituteClientId), substituteClientState, p.InitialHeight)
+	if subjectClientState.GetLatestHeight().GTE(substituteClientState.GetLatestHeight()) {
+		return sdkerrors.Wrapf(types.ErrInvalidHeight, "subject client state latest height is greater or equal to substitute client state latest height (%s >= %s)", subjectClientState.GetLatestHeight(), substituteClientState.GetLatestHeight())
+	}
+
+	substituteClientStore := k.ClientStore(ctx, p.SubstituteClientId)
+
+	if status := substituteClientState.Status(ctx, substituteClientStore, k.cdc); status != exported.Active {
+		return sdkerrors.Wrapf(types.ErrClientNotActive, "substitute client is not Active, status is %s", status)
+	}
+
+	clientState, err := subjectClientState.CheckSubstituteAndUpdateState(ctx, k.cdc, subjectClientStore, substituteClientStore, substituteClientState)
 	if err != nil {
 		return err
 	}

modules/core/02-client/keeper/proposal_test.go

@@ -13,7 +13,6 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 	var (
 		subject, substitute                       string
 		subjectClientState, substituteClientState exported.ClientState
-		initialHeight                             types.Height
 		content                                   govtypes.Content
 		err                                       error
 	)
@@ -25,7 +24,7 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 	}{
 		{
 			"valid update client proposal", func() {
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
 			}, true,
 		},
 		{
@@ -37,31 +36,43 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 				newRevisionNumber := tmClientState.GetLatestHeight().GetRevisionNumber() + 1
 
 				tmClientState.LatestHeight = types.NewHeight(newRevisionNumber, tmClientState.GetLatestHeight().GetRevisionHeight())
-				initialHeight = types.NewHeight(newRevisionNumber, initialHeight.GetRevisionHeight())
+
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), substitute, tmClientState.LatestHeight, consState)
+				clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), substitute)
+				ibctmtypes.SetProcessedTime(clientStore, tmClientState.LatestHeight, 100)
+				ibctmtypes.SetProcessedHeight(clientStore, tmClientState.LatestHeight, types.NewHeight(0, 1))
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState)
 
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
 			}, true,
 		},
 		{
 			"cannot use localhost as subject", func() {
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, exported.Localhost, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, exported.Localhost, substitute)
 			}, false,
 		},
 		{
 			"cannot use localhost as substitute", func() {
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, exported.Localhost, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, exported.Localhost)
+			}, false,
+		},
+		{
+			"cannot use solomachine as substitute for tendermint client", func() {
+				solomachine := ibctesting.NewSolomachine(suite.T(), suite.cdc, "solo machine", "", 1)
+				solomachine.Sequence = subjectClientState.GetLatestHeight().GetRevisionHeight() + 1
+				substituteClientState = solomachine.ClientState()
+				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, substituteClientState)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
 			}, false,
 		},
 		{
 			"subject client does not exist", func() {
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, ibctesting.InvalidID, substitute)
 			}, false,
 		},
 		{
 			"substitute client does not exist", func() {
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, ibctesting.InvalidID, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, ibctesting.InvalidID)
 			}, false,
 		},
 		{
@@ -71,7 +82,7 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 				tmClientState.LatestHeight = substituteClientState.GetLatestHeight().(types.Height)
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState)
 
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
 			}, false,
 		},
 		{
@@ -81,7 +92,17 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 				tmClientState.FrozenHeight = types.ZeroHeight()
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState)
 
-				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute, initialHeight)
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
+			}, false,
+		},
+		{
+			"substitute is frozen", func() {
+				tmClientState, ok := substituteClientState.(*ibctmtypes.ClientState)
+				suite.Require().True(ok)
+				tmClientState.FrozenHeight = types.NewHeight(0, 1)
+				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState)
+
+				content = types.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subject, substitute)
 			}, false,
 		},
 	}
@@ -100,7 +121,6 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 			substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupClients(substitutePath)
 			substitute = substitutePath.EndpointA.ClientID
-			initialHeight = types.NewHeight(subjectClientState.GetLatestHeight().GetRevisionNumber(), subjectClientState.GetLatestHeight().GetRevisionHeight()+1)
 
 			// update substitute twice
 			substitutePath.EndpointA.UpdateClient()
@@ -118,7 +138,6 @@ func (suite *KeeperTestSuite) TestClientUpdateProposal() {
 			suite.Require().True(ok)
 			tmClientState.AllowUpdateAfterMisbehaviour = true
 			tmClientState.AllowUpdateAfterExpiry = true
-			tmClientState.FrozenHeight = tmClientState.LatestHeight
 			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitute, tmClientState)
 
 			tc.malleate()

modules/core/02-client/proposal_handler_test.go

@@ -29,7 +29,6 @@ func (suite *ClientTestSuite) TestNewClientUpdateProposalHandler() {
 
 				substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB)
 				suite.coordinator.SetupClients(substitutePath)
-				initialHeight := clienttypes.NewHeight(subjectClientState.GetLatestHeight().GetRevisionNumber(), subjectClientState.GetLatestHeight().GetRevisionHeight()+1)
 
 				// update substitute twice
 				err = substitutePath.EndpointA.UpdateClient()
@@ -50,7 +49,7 @@ func (suite *ClientTestSuite) TestNewClientUpdateProposalHandler() {
 				tmClientState.AllowUpdateAfterMisbehaviour = true
 				suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitutePath.EndpointA.ClientID, tmClientState)
 
-				content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subjectPath.EndpointA.ClientID, substitutePath.EndpointA.ClientID, initialHeight)
+				content = clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subjectPath.EndpointA.ClientID, substitutePath.EndpointA.ClientID)
 			}, true,
 		},
 		{