fix: deprecate AllowUpdateAfter...check (backport #1511) (#1519)

* fix: deprecate AllowUpdateAfter...check (#1511) * fix: deprecate AllowUpdateAfter...check * update IsMatchingClientState * rm unnecessary fields in testing (cherry picked from commit 5e5e2cd) # Conflicts: # CHANGELOG.md # docs/ibc/proposals.md # modules/light-clients/07-tendermint/types/tendermint.pb.go * fix conflicts * Update CHANGELOG.md * Update adr-026-ibc-client-recovery-mechanisms.md * Update adr-026-ibc-client-recovery-mechanisms.md Co-authored-by: Charly <charly@interchain.berlin> Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
cosmos · Jun 12, 2022 · b142ead · b142ead
1 parent 1a3664c
commit b142ead
Show file tree

Hide file tree

Showing 8 changed files with 153 additions and 224 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -52,6 +52,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/keeper) [\#1284](https://github.com/cosmos/ibc-go/pull/1284) Add sanity check for the keepers passed into `ibckeeper.NewKeeper`. `ibckeeper.NewKeeper` now panics if any of the keepers passed in is empty.
 * (transfer) [\#1414](https://github.com/cosmos/ibc-go/pull/1414) Emitting Sender address from `fungible_token_packet` events in `OnRecvPacket` and `OnAcknowledgementPacket`.
 * (modules/core/04-channel) [\#1464](https://github.com/cosmos/ibc-go/pull/1464) Emit a channel close event when an ordered channel is closed.
+* (modules/light-clients/07-tendermint) [\#1118](https://github.com/cosmos/ibc-go/pull/1118) Deprecating `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour`. See ADR-026 for context.
 
 ### Features
 

diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md
@@ -6,6 +6,7 @@
 - 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
+- 2022/04/08: Revision to deprecate AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour
 
 ## Status
 
@@ -35,21 +36,20 @@ Two-thirds of the validator set (the quorum for governance, module participation
 We elect not to deal with chains which have actually halted, which is necessarily Byzantine behaviour and in which case token recovery is not likely possible anyways (in-flight packets cannot be timed-out, but the relative impact of that is minor).
 
 1. Require Tendermint light clients (ICS 07) to be created with the following additional flags
-    1. `allow_governance_override_after_expiry` (boolean, default false)
+    1. `allow_update_after_expiry` (boolean, default true). Note that this flag has been deprecated, it remains to signal intent but checks against this value will not be enforced.
 1. Require Tendermint light clients (ICS 07) to expose the following additional internal query functions
     1. `Expired() boolean`, which returns whether or not the client has passed the trusting period since the last update (in which case no headers can be validated)
 1. Require Tendermint light clients (ICS 07) & solo machine clients (ICS 06) to be created with the following additional flags
-    1. `allow_governance_override_after_misbehaviour` (boolean, default false)
+    1. `allow_update_after_misbehaviour` (boolean, default true). Note that this flag has been deprecated, it remains to signal intent but checks against this value will not be enforced.
 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`). 
     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. 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()`
+    1. If this governance proposal passes, the client on trial will be updated to the latest state of the substitute.
+
+    Previously, `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour` were used to signal the recovery options for an expired or frozen client, and governance proposals were not allowed to overwrite the client if these parameters were set to false. However, this has now been deprecated because a code migration can overwrite the client and consensus states regardless of the value of these parameters. If governance would vote to overwrite a client or consensus state, it is likely that governance would also be willing to perform a code migration to do the same.
 
 
 Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen. 
@@ -62,7 +62,6 @@ This ADR does not address planned upgrades, which are handled separately as per
 
 - Establishes a mechanism for client recovery in the case of expiry
 - Establishes a mechanism for client recovery in the case of misbehaviour
-- Clients can elect to disallow this recovery mechanism if they do not wish to allow for it
 - Constructing an ClientUpdate Proposal is as difficult as creating a new client
 
 ### Negative

diff --git a/docs/ibc/proposals.md b/docs/ibc/proposals.md
@@ -39,3 +39,49 @@ The substitute client is used as a "stand in" while the subject is on trial. It
 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. 
+
+# How to recover an expired client with a governance proposal
+
+See also the relevant documentation: [ADR-026, IBC client recovery mechanisms](../architecture/adr-026-ibc-client-recovery-mechanisms.md)
+
+### Preconditions 
+- The chain is updated with ibc-go >= v1.1.0.
+- The client identifier of an active client for the same counterparty chain.
+- The governance deposit.
+
+## Steps
+
+### Step 1
+
+Check if the client is attached to the expected `chain-id`. For example, for an expired Tendermint client representing the Akash chain the client state looks like this on querying the client state:
+
+```
+{
+  client_id: 07-tendermint-146
+  client_state:
+  '@type': /ibc.lightclients.tendermint.v1.ClientState
+  allow_update_after_expiry: true
+  allow_update_after_misbehaviour: true
+  chain_id: akashnet-2
+}
+```
+
+The client is attached to the expected Akash `chain-id`. Note that although the parameters (`allow_update_after_expiry` and `allow_update_after_misbehaviour`) exist to signal intent, these parameters have been deprecated and will not enforce any checks on the revival of client. See ADR-026 for more context on this deprecation.
+
+### Step 2
+
+If the chain has been updated to ibc-go >= v1.1.0, anyone can submit the governance proposal to recover the client by executing this via cli:
+
+```
+<binary> tx gov submit-proposal update-client <expired-client-id> <active-client-id>
+```
+
+The `<active-client-id>` should be a client identifier on the same chain as the expired or frozen client. This client identifier should connect to the same chain as the expired or frozen client. This means: use the active client that is currently being used to relay packets between the two chains as the replacement client.
+
+After this, it is just a question of who funds the governance deposit and if the chain in question votes yes.
+
+## Important considerations 
+
+Please note that from v1.0.0 of ibc-go it will not be allowed for transactions to go to expired clients anymore, so please update to at least this version to prevent similar issues in the future.
+
+Please also note that if the client on the other end of the transaction is also expired, that client will also need to update. This process updates only one client.
diff --git a/docs/ibc/proto-docs.md b/docs/ibc/proto-docs.md
@@ -3707,8 +3707,8 @@ and a possible frozen height.
 | `latest_height` | [ibc.core.client.v1.Height](#ibc.core.client.v1.Height) |  | Latest height the client was updated to |
 | `proof_specs` | [ics23.ProofSpec](#ics23.ProofSpec) | repeated | Proof specifications used in verifying counterparty state |
 | `upgrade_path` | [string](#string) | repeated | Path at which next upgraded client will be committed. Each element corresponds to the key for a single CommitmentProof in the chained proof. NOTE: ClientState must stored under `{upgradePath}/{upgradeHeight}/clientState` ConsensusState must be stored under `{upgradepath}/{upgradeHeight}/consensusState` For SDK chains using the default upgrade module, upgrade_path should be []string{"upgrade", "upgradedIBCState"}` |
-| `allow_update_after_expiry` | [bool](#bool) |  | This flag, when set to true, will allow governance to recover a client which has expired |
-| `allow_update_after_misbehaviour` | [bool](#bool) |  | This flag, when set to true, will allow governance to unfreeze a client whose chain has experienced a misbehaviour event |
+| `allow_update_after_expiry` | [bool](#bool) |  | **Deprecated.** allow_update_after_expiry is deprecated |
+| `allow_update_after_misbehaviour` | [bool](#bool) |  | **Deprecated.** allow_update_after_misbehaviour is deprecated |
 
 
 

diff --git a/modules/light-clients/07-tendermint/types/proposal_handle.go b/modules/light-clients/07-tendermint/types/proposal_handle.go
@@ -11,18 +11,17 @@ import (
 )
 
 // 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 Status() == Frozen
-// 	2) AllowUpdateAfterExpiry=true and Status() == Expired
+// substitute.
+//
+// AllowUpdateAfterMisbehaviour and AllowUpdateAfterExpiry have been deprecated.
+// Please see ADR 026 for more information.
 //
 // 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.
+// the FrozenHeight to the zero Height.
 func (cs ClientState) CheckSubstituteAndUpdateState(
 	ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore,
 	substituteClientStore sdk.KVStore, substituteClient exported.ClientState,
@@ -38,23 +37,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState(
 		return nil, sdkerrors.Wrap(clienttypes.ErrInvalidSubstitute, "subject client state does not match substitute client state")
 	}
 
-	switch cs.Status(ctx, subjectClientStore, cdc) {
-
-	case exported.Frozen:
-		if !cs.AllowUpdateAfterMisbehaviour {
-			return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unfrozen")
-		}
-
+	if cs.Status(ctx, subjectClientStore, cdc) == exported.Frozen {
 		// unfreeze the client
 		cs.FrozenHeight = clienttypes.ZeroHeight()
-
-	case exported.Expired:
-		if !cs.AllowUpdateAfterExpiry {
-			return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client is not allowed to be unexpired")
-		}
-
-	default:
-		return nil, sdkerrors.Wrap(clienttypes.ErrUpdateClientFailed, "client cannot be updated with proposal")
 	}
 
 	// copy consensus states and processed time from substitute to subject
@@ -100,6 +85,11 @@ func IsMatchingClientState(subject, substitute ClientState) bool {
 	substitute.FrozenHeight = clienttypes.ZeroHeight()
 	subject.ChainId = ""
 	substitute.ChainId = ""
+	// sets both sets of flags to true as these flags have been DEPRECATED, see ADR-026 for more information
+	subject.AllowUpdateAfterExpiry = true
+	substitute.AllowUpdateAfterExpiry = true
+	subject.AllowUpdateAfterMisbehaviour = true
+	substitute.AllowUpdateAfterMisbehaviour = true
 
 	return reflect.DeepEqual(subject, substitute)
 }
diff --git a/modules/light-clients/07-tendermint/types/proposal_handle_test.go b/modules/light-clients/07-tendermint/types/proposal_handle_test.go
@@ -30,10 +30,12 @@ func (suite *TendermintTestSuite) TestCheckSubstituteUpdateStateBasic() {
 		{
 			"non-matching substitute", func() {
 				suite.coordinator.SetupClients(substitutePath)
-				substituteClientState = suite.chainA.GetClientState(substitutePath.EndpointA.ClientID).(*types.ClientState)
-				tmClientState, ok := substituteClientState.(*types.ClientState)
+				substituteClientState, ok := suite.chainA.GetClientState(substitutePath.EndpointA.ClientID).(*types.ClientState)
 				suite.Require().True(ok)
+				// change trusting period so that test should fail
+				substituteClientState.TrustingPeriod = time.Hour * 24 * 7
 
+				tmClientState := substituteClientState
 				tmClientState.ChainId = tmClientState.ChainId + "different chain"
 			},
 		},
@@ -50,8 +52,6 @@ func (suite *TendermintTestSuite) TestCheckSubstituteUpdateStateBasic() {
 
 			suite.coordinator.SetupClients(subjectPath)
 			subjectClientState := suite.chainA.GetClientState(subjectPath.EndpointA.ClientID).(*types.ClientState)
-			subjectClientState.AllowUpdateAfterMisbehaviour = true
-			subjectClientState.AllowUpdateAfterExpiry = true
 
 			// expire subject client
 			suite.coordinator.IncrementTimeBy(subjectClientState.TrustingPeriod)
@@ -74,140 +74,40 @@ func (suite *TendermintTestSuite) TestCheckSubstituteUpdateStateBasic() {
 func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() {
 	testCases := []struct {
 		name                         string
-		AllowUpdateAfterExpiry       bool
-		AllowUpdateAfterMisbehaviour bool
 		FreezeClient                 bool
 		ExpireClient                 bool
 		expPass                      bool
 	}{
 		{
-			name:                         "not allowed to be updated, not frozen or expired",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 false,
-			ExpireClient:                 false,
-			expPass:                      false,
-		},
-		{
-			name:                         "not allowed to be updated, client is frozen",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 true,
-			ExpireClient:                 false,
-			expPass:                      false,
-		},
-		{
-			name:                         "not allowed to be updated, client is expired",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 false,
-			ExpireClient:                 true,
-			expPass:                      false,
-		},
-		{
-			name:                         "not allowed to be updated, client is frozen and expired",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 true,
-			ExpireClient:                 true,
-			expPass:                      false,
-		},
-		{
-			name:                         "allowed to be updated only after misbehaviour, not frozen or expired",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: true,
-			FreezeClient:                 false,
-			ExpireClient:                 false,
-			expPass:                      false,
-		},
-		{
-			name:                         "allowed to be updated only after misbehaviour, client is expired",
-			AllowUpdateAfterExpiry:       false,
-			AllowUpdateAfterMisbehaviour: true,
-			FreezeClient:                 false,
-			ExpireClient:                 true,
-			expPass:                      false,
-		},
-		{
-			name:                         "allowed to be updated only after expiry, not frozen or expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 false,
-			ExpireClient:                 false,
-			expPass:                      false,
-		},
-		{
-			name:                         "allowed to be updated only after expiry, client is frozen",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 true,
-			ExpireClient:                 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,
+			name:                         "PASS: update checks are deprecated, client is frozen and expired",
 			FreezeClient:                 true,
 			ExpireClient:                 true,
 			expPass:                      true,
 		},
 		{
-			name:                         "PASS: allowed to be updated only after expiry, client is expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: false,
+			name:                         "PASS: update checks are deprecated, not frozen or expired",
 			FreezeClient:                 false,
-			ExpireClient:                 true,
+			ExpireClient:                 false,
 			expPass:                      true,
 		},
 		{
-			name:                         "allowed to be updated only after expiry, client is frozen and expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: false,
-			FreezeClient:                 true,
-			ExpireClient:                 true,
-			expPass:                      false,
-		},
-		{
-			name:                         "allowed to be updated after expiry and misbehaviour, not frozen or expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: true,
+			name:                         "PASS: update checks are deprecated, not frozen or expired",
 			FreezeClient:                 false,
 			ExpireClient:                 false,
-			expPass:                      false,
+			expPass:                      true,
 		},
 		{
-			name:                         "PASS: allowed to be updated after expiry and misbehaviour, client is frozen",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: true,
+			name:                         "PASS: update checks are deprecated, client is frozen",
 			FreezeClient:                 true,
 			ExpireClient:                 false,
 			expPass:                      true,
 		},
 		{
-			name:                         "PASS: allowed to be updated after expiry and misbehaviour, client is expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: true,
+			name:                         "PASS: update checks are deprecated, client is expired",
 			FreezeClient:                 false,
 			ExpireClient:                 true,
 			expPass:                      true,
 		},
-		{
-			name:                         "PASS: allowed to be updated after expiry and misbehaviour, client is frozen and expired",
-			AllowUpdateAfterExpiry:       true,
-			AllowUpdateAfterMisbehaviour: true,
-			FreezeClient:                 true,
-			ExpireClient:                 true,
-			expPass:                      true,
-		},
 	}
 
 	for _, tc := range testCases {
@@ -225,8 +125,6 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() {
 			subjectPath := ibctesting.NewPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupClients(subjectPath)
 			subjectClientState := suite.chainA.GetClientState(subjectPath.EndpointA.ClientID).(*types.ClientState)
-			subjectClientState.AllowUpdateAfterExpiry = tc.AllowUpdateAfterExpiry
-			subjectClientState.AllowUpdateAfterMisbehaviour = tc.AllowUpdateAfterMisbehaviour
 
 			// apply freezing or expiry as determined by the test case
 			if tc.FreezeClient {
@@ -247,8 +145,6 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() {
 			substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupClients(substitutePath)
 			substituteClientState := suite.chainA.GetClientState(substitutePath.EndpointA.ClientID).(*types.ClientState)
-			substituteClientState.AllowUpdateAfterExpiry = tc.AllowUpdateAfterExpiry
-			substituteClientState.AllowUpdateAfterMisbehaviour = tc.AllowUpdateAfterMisbehaviour
 			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitutePath.EndpointA.ClientID, substituteClientState)
 
 			// update substitute a few times