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

* fix: deprecate AllowUpdateAfter...check (#1511)

* fix: deprecate AllowUpdateAfter...check

* update IsMatchingClientState

* rm unnecessary fields in testing

(cherry picked from commit 5e5e2cd)

# Conflicts:
#	CHANGELOG.md

* fix conflicts

* Update CHANGELOG.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>

Author: 3 people

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
 

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

docs/ibc/proposals.md

@@ -46,7 +46,6 @@ See also the relevant documentation: [ADR-026, IBC client recovery mechanisms](.
 
 ### Preconditions 
 - The chain is updated with ibc-go >= v1.1.0.
-- Recovery parameters are set to `true` for the Tendermint light client (this determines if a governance proposal can be used). If the recovery parameters are set to `false`, recovery will require custom migration code.
 - The client identifier of an active client for the same counterparty chain.
 - The governance deposit.
 
@@ -67,7 +66,7 @@ Check if the client is attached to the expected `chain-id`. For example, for an
 }
 ```
 
-The client is attached to the expected Akash `chain-id` and the recovery parameters (`allow_update_after_expiry` and `allow_update_after_misbehaviour`) are set to `true`.
+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
 

docs/ibc/proto-docs.md

@@ -4023,8 +4023,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 |
 
 
 

modules/light-clients/07-tendermint/types/proposal_handle.go

@@ -12,18 +12,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,
@@ -39,23 +38,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
@@ -101,6 +86,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)
 }

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