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

docs: use MsgRecoverClient in docs #4580

Merged
merged 3 commits into from
Sep 6, 2023
Merged

docs: use MsgRecoverClient in docs #4580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
720a30d
docs: recover client update
colin-axner Sep 5, 2023
9efba33
Update docs/ibc/proposals.md
colin-axner Sep 5, 2023
59be787
Apply suggestions from code review
colin-axner Sep 6, 2023
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
9 changes: 7 additions & 2 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 @@ -8,6 +8,7 @@
- 2021/05/20: Revision to simplify consensus state copying, remove initial height
- 2022/04/08: Revision to deprecate AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour
- 2022/07/15: Revision to allow updating of TrustingPeriod
- 2023/09/05: Revision to migrate from gov v1beta1 to gov v1

## Status

Expand Down Expand Up @@ -44,16 +45,19 @@ We elect not to deal with chains which have actually halted, which is necessaril
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. Add a new governance proposal with `MsgRecoverClient`.
1. Create a new Msg with two client identifiers (`string`) and a signer.
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.
1. The signer must be the authority set for the ibc module.

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.

In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the `TrustingPeriod` of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to update this client parameter.

In versions older than ibc-go v8, `MsgRecoverClient` was a governance proposal type `ClientUpdateProposal`. It has been removed and replaced by `MsgRecoverClient` in the migration from goverance v1beta1 to governacne v1.

Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set to 2/3 of the `UnbondingPeriod`.

Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen.
Expand Down Expand Up @@ -83,3 +87,4 @@ No neutral consequences.
- [Prior discussion](https://github.com/cosmos/ics/issues/421)
- [Epoch number discussion](https://github.com/cosmos/ics/issues/439)
- [Upgrade plan discussion](https://github.com/cosmos/ics/issues/445)
- [Migration from gov v1beta1 to gov v1](https://github.com/cosmos/ibc-go/issues/3672)
34 changes: 10 additions & 24 deletions docs/ibc/proposals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ See also the relevant documentation: [ADR-026, IBC client recovery mechanisms](.

## Preconditions

- The chain is updated with ibc-go >= v1.1.0.
- There exists an active client (with a known client identifier) for the same counterparty chain as the expired client.
- The governance deposit.

Expand All @@ -76,11 +75,10 @@ The client is attached to the expected Akash `chain-id`. Note that although the

### 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.
Anyone can submit the governance proposal to recover the client by executing the following via CLI.
If the chain is on an ibc-go version older than v8, please see the [relevant documentation](https://ibc.cosmos.network/v6.1.0/ibc/proposals.html).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason linking v7.3.0 wasn't working for me

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, that's not good... Looks like the docs for the latest releases are not properly deployed...


> Note that the Cosmos SDK has updated how governance proposals are submitted in SDK v0.46, now requiring to pass a .json proposal file

- From SDK v0.46.x onwards
- From ibc-go v8 onwards

```shell
<binary> tx gov submit-proposal [path-to-proposal-json]
Expand All @@ -92,30 +90,20 @@ If the chain has been updated to ibc-go >= v1.1.0, anyone can submit the governa
{
"messages": [
{
"@type": "/ibc.core.client.v1.ClientUpdateProposal",
"title": "title_string",
"description": "description_string",
"@type": "/ibc.core.client.v1.MsgRecoverClient",
"subject_client_id": "expired_client_id_string",
"substitute_client_id": "active_client_id_string"
"substitute_client_id": "active_client_id_string",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't verified manually. If someone thinks that is important, I'll let you take the lead 🙂

I did use the gov cli cmd to format this:

Example:
$ simd tx gov submit-proposal path/to/proposal.json

Where proposal.json contains:

{
  // array of proto-JSON-encoded sdk.Msgs
  "messages": [
    {
      "@type": "/cosmos.bank.v1beta1.MsgSend",
      "from_address": "cosmos1...",
      "to_address": "cosmos1...",
      "amount":[{"denom": "stake","amount": "10"}]
    }
  ],
  // metadata can be any of base64 encoded, raw text, stringified json, IPFS link to json
  // see below for example metadata
  "metadata": "4pIMOgIGx1vZGU=",
  "deposit": "10stake",
  "title": "My proposal",
  "summary": "A short summary of my proposal",
  "expedited": false
}

Copy link
Contributor

@DimitrisJim DimitrisJim Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn't this just be simd tx ibc client "subject-client" "substitute-client" --title="..."

this suggestion comes after looking at sdk's docs for x/upgrade which uses gov v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's 2 cli's that support submission of recovering a client. Using gov's submit-proposal cli and the one we implemented. I actually ended up testing recovery via the gov cli using the e2e targetting cli's. Personally I prefer the json file approach, otherwise you input all the args as flags

"signer": "<gov-address>"
}
],
"metadata": "<metadata>",
"deposit": "10stake"
"title": "My proposal",
"summary": "A short summary of my proposal",
"expedited": false
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add an example using the recover-client CLI that we are adding now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I find the submit-proposal to be easier UI then submitting all these extra fields with flags. I'll leave it up to y'all to push the extra example if you'd like

Alternatively there's a legacy command (that is no longer recommended though):

```shell
<binary> tx gov submit-legacy-proposal update-client <expired-client-id> <active-client-id>
```

- Until SDK v0.45.x

```shell
<binary> tx gov submit-proposal update-client <expired-client-id> <active-client-id>
```

The `<expired-client-id>` identifier is the proposed client to be updated. This client must be either frozen or expired.

The `<active-client-id>` represents a substitute client. It carries all the state for the client which may be updated. It must have identical 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.
Expand All @@ -124,6 +112,4 @@ After this, all that remains is deciding who funds the governance deposit and en

## 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed as no chains are on a pre ibc-go v1 version


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.
Please 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.