Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: deprecate AllowUpdateAfter...check #1511

Merged
merged 4 commits into from
Jun 9, 2022
Merged

fix: deprecate AllowUpdateAfter...check #1511

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed4242a
fix: deprecate AllowUpdateAfter...check
charleenfei Apr 25, 2022
4dc3a15
update IsMatchingClientState
charleenfei Jun 9, 2022
30127d7
rm unnecessary fields in testing
charleenfei Jun 9, 2022
a19bcf3
Merge branch 'main' into fix/rmAllowUpdateAfter
charleenfei Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### State Machine Breaking

### Improvements

* (cleanup) [\#1335](https://github.com/cosmos/ibc-go/pull/1335/) `gofumpt -w -l .` to standardize the code layout more strictly than `go fmt ./...`
* (transfer) [\#1342](https://github.com/cosmos/ibc-go/pull/1342) `DenomTrace` grpc now takes in either an `ibc denom` or a `hash` instead of only accepting a `hash`.
* (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.
Expand All @@ -62,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
* (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

Expand Down
13 changes: 6 additions & 7 deletions docs/architecture/adr-026-ibc-client-recovery-mechanisms.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 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.
Expand All @@ -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
Expand Down
3 changes: 1 addition & 2 deletions docs/ibc/proposals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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

Expand Down
4 changes: 2 additions & 2 deletions docs/ibc/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4935,8 +4935,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 |



Expand Down
32 changes: 11 additions & 21 deletions modules/light-clients/07-tendermint/types/proposal_handle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also update isMatchingClientState as well to set the flags to true so that differences in these values between clients are ignored

ctx sdk.Context, cdc codec.BinaryCodec, subjectClientStore,
substituteClientStore sdk.KVStore, substituteClient exported.ClientState,
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
112 changes: 12 additions & 100 deletions modules/light-clients/07-tendermint/types/proposal_handle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,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"
},
},
Expand All @@ -47,8 +49,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)
Expand Down Expand Up @@ -78,133 +78,45 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() {
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",
name: "PASS: update checks are deprecated, client is frozen and expired",
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",
name: "PASS: update checks are deprecated, not frozen or expired",
AllowUpdateAfterExpiry: false,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: true,
expPass: true,
},
{
name: "PASS: allowed to be updated only after expiry, client is expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
FreezeClient: false,
ExpireClient: true,
ExpireClient: false,
expPass: true,
},
{
name: "allowed to be updated only after expiry, client is frozen and expired",
name: "PASS: update checks are deprecated, not frozen or expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: These fields can be removed now because of the change you made to IsMatchingClient

FreezeClient: true,
ExpireClient: true,
expPass: false,
},
{
name: "allowed to be updated after expiry and misbehaviour, not frozen or expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: false,
ExpireClient: false,
expPass: false,
expPass: true,
},
{
name: "PASS: allowed to be updated after expiry and misbehaviour, client is frozen",
name: "PASS: update checks are deprecated, client is frozen",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
FreezeClient: true,
ExpireClient: false,
expPass: true,
},
{
name: "PASS: allowed to be updated after expiry and misbehaviour, client is expired",
name: "PASS: update checks are deprecated, client is expired",
AllowUpdateAfterExpiry: true,
AllowUpdateAfterMisbehaviour: true,
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 {
Expand Down
Loading