From a6b44886fd08b93cabfba453df02cddccc35098d Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 26 Jan 2023 22:02:32 -0800 Subject: [PATCH 1/7] Initial ADR-006 draft --- .../adr-006-upgrade-client-implementation.md | 420 ++++++++++++++++++ 1 file changed, 420 insertions(+) create mode 100644 docs/architecture/adr-006-upgrade-client-implementation.md diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md new file mode 100644 index 000000000..581e6cc91 --- /dev/null +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -0,0 +1,420 @@ +# ADR 006: Upgrade client implementation + +## Changelog + +* 2023-01-25 Initial Proposal + +## Context + +The ability to upgrade is crucial for IBC-connected chains. It allows them to +evolve and improve without any restrictions. The IBC module may not be affected +by some upgrades, but some may require it to be upgraded as well to maintain +high-value connections to other chains secure. For having this capability, +chains that implement `IBC-rs` may bring various concerns and characteristics +than Tendermint chains leading to different ways for upgrading their clients. +However there are generic rules that apply to all and can serve as a framework +for any `IBC-rs` implementation. On this basis, this record aims to justify the +logic behind upgrading a light client, determine the boundary between +client-wide and client-specific upgrade processes, list requisites for +validation and execution steps, and explain Tendermint's upgrade client +implementation within the +[ics07_tendermint](../../crates/ibc/src/clients/ics07_tendermint). + +## Decision + +In this section, after presenting rules that mainly derived from the IBC +protocol and the review of the `IBC-Go` implementation, the upgrade client +process for Tendermint chains is outlined in detail. Then, the design and +implementation of this rationale in `IBC-rs` is explained. + +### Chain-wide Upgrade Rules + +#### Chain supports IBC upgrades? + +* IBC currently **ONLY** supports planned upgrades that are committed to in + advance by the upgrading chain, in order for counterparty clients to maintain + their connections securely. +* An IBC upgrade **MUST** be performed if upgrading an IBC-connected chain + breaks the counterparty IBC client. +* There **MUST** be a proof verification process to check upgraded client and + consensus states against the host chain's state. +* Clients **MUST** ensure that the planned last height of the current revision + is somehow encoded in the proof verification process. This prevents premature + upgrades, as the counterparty may cancel or modify upgrade plans before the + last planned height. +* Chain upgrades **MUST NOT** result in changing + [ClientState](../../crates/ibc/src/core/ics02_client/client_state.rs#ClientState) + or + [ConsensusState](../../crates/ibc/src/core/ics02_client/consensus_state.rs#ConsensusState) + implementations +* It is **UP TO** the chain's architecture how updated client and consensus + states are committed, either through decentralized approaches, like governance + or centralized methods, like a multisig account, etc. +* Upon commitment, chain's store **MUST** contain updated client and consensus + states, which can be retrieved using respective upgrade paths + +#### IBC Handler accepts upgrade? + +After ensuring that the chain upgrades is supported by IBC, the general +validation and execution steps that apply to all chains are as follows. The +criteria for classifying a validation as client-wide (also can be known as +basic) or client-specific was whether that IBC handler can perform that action +just using its own context data, which are available upon calling interfaces +provided by `ClientState` and `ConsensusState` traits, like `is_frozen()`, +`latest_height()`, etc. + +* Latest client state **MUST NOT** be frozen +* Latest consensus state **MUST** be within the trusting period of the latest + client state +* Verification proofs **MUST** successfully be decoded into the domain types + +Any other requisite beyond the above rules are considered client-specific. Next, +we go through the upgrade client process for Tendermint chains to justify the +logic and illustrate an example of client-specific upgrade process. + +### Upgrade Tendermint Clients + +This section is based on the `IBC-Go` procedure for client upgrades with a few +modifications to comply with the `IBC-rs` design patterns. + +#### Acceptable Client State Upgrades + +Below list enumerates upgrades on Tendermint chains that would break +counterparty IBC Tendermint clients and indicates whether or not the change is +supported by `IBC-rs`: + +* S: Supported, P: Partially Supported, U: Unsupported + +1. (S) Changing the `ChainId` +2. (P) Changing the `UnbondingPeriod`: chains may increase the unbonding period + with no issues. However, decreasing the unbonding period may irreversibly + break some counterparty clients. Thus, it is not recommended that chains + reduce the unbonding period. +3. (S) Changing the `Height` (resetting to 0): as long as chains remember to + increment the revision number in their chain-id. +4. (S) Changing the `ProofSpecs`: this should be changed if the proof structure + needed to verify IBC proofs is changed across the upgrade. Ex: Switching from + an IAVL store, to a SimpleTree Store. +5. (S) Changing the `UpgradePath`: this might involve changing the key under + which upgraded clients and consensus states are stored in the upgrade store, + or even migrating the upgrade store itself. +6. (U) Migrating the IBC store: the store location is negotiated by the + connection. +7. (S) Upgrading to a backwards compatible version of IBC +8. (U) Upgrading to a non-backwards compatible version of IBC: the version is + negotiated on connection handshake. +9. (U) Changing parameters that are customizable by relayers like `TrustLevel` + and `TrustingPeriod`, `max_clock_drift` +10. (P) Changing the Tendermint LightClient algorithm: Changes to the light + client algorithm that do not change the + [ClientState](../../crates/ibc/src/clients/ics07_tendermint/client_state.rs#ClientState) + or + [ConsensusState](../../crates/ibc/src/clients/ics07_tendermint/consensus_state.rs#ConsensusState) + struct may be supported, provided that the counterparty is also upgraded to + support the new light client algorithm. Changes that require updating the + `ClientState` and `ConsensusState` structs themselves are theoretically + possible by providing a path to translate an older ClientState struct into + the new ClientState struct; however this is not currently implemented. + +#### Upgrade Process Step-by-step + +An IBC-connected Tendermint chain will take the following steps to completely +upgrade its own chain and counterparty's IBC client: + +1. Upgrade chain through governance + 1. Create a 02-client + [UpgradeProposal](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/proto-docs.md#upgradeproposal) + with an `UpgradePlan` and a new IBC `ClientState` in the + `UpgradedClientState` field with the following remarks: + * The `UpgradePlan` must specify an upgrade height only (no upgrade + time) + * The `ClientState` should only include the fields common to all valid + clients and zero out any client-customizable fields (such as + `TrustingPeriod`). + 2. Vote on and pass the `UpgradeProposal` + 3. Commit `UpgradedClient` by the upgrade module under the following key: + + ```md + upgrade/UpgradedIBCState/{upgradeHeight}/upgradedClient + ``` + + 4. Commit an initial consensus state by upgrade module on the block right + before the upgrade height for the next chain under the following key: + + ```md + upgrade/UpgradedIBCState/{upgradeHeight}/upgradedConsState + ``` + +2. Submit an upgrade client message to the counterparty chain by a relayer + 1. Wait for the upgrading chain to reach the upgrade height and halt + 2. Query a full node for the proofs of `UpgradedClient` and + `UpgradedConsensusState` at the last height of the old chain + 3. Update the counterparty client to the last height of the old chain using + the `UpdateClient` msg + 4. Submit an `UpgradeClient` msg to the counterparty chain with the + `UpgradedClient`, `UpgradedConsensusState` and their respective proofs + +3. Process the upgrade message on the counterparty chain
IBC handler upon +receiving an `MsgUpgradeClient` message performs basic validations (BV), +client-specific validations (SV) and lastly execution (E) steps as follows. + 1. (BV) Check that the current client is not frozen + 2. (BV) Check if the latest consensus state is within the trust period + 3. (BV) Decode proofs and check that they are valid + 4. (SV) Verify that the upgradedClient be of a Tendermint `ClientState` type + 5. (SV) Verify that the upgradedConsensusState be of a Tendermint + `ConsensusState` type + 6. (SV) Check the latest height of the current client state does not have the + same revision number or has a greater height than the committed client + 7. (SV) Match any Tendermint chain specified parameter in upgraded client + such as ChainID, UnbondingPeriod, and ProofSpecs with the committed client + 8. (SV) Verify that the upgrading chain did indeed commit to the upgraded + client state at the upgrade height by provided proof + 9. (SV) Verify that the upgrading chain did indeed commit to the upgraded + consensus state at the upgrade height by provided proof + 10. (E) Upgrade client to the new client by retaining old client-customizable + parameters (sent by relayers) such `TrustingPeriod`, `TrustLevel`, + `MaxClockDrift` and adopt the new chain-specified fields such as + `UnbondingPeriod`, `ChainId`, `UpgradePath`, etc. + 11. (E) Upgrade consensus state with a stand-in sentinel value for root + Note: The upgraded consensus state serve purely as a basis of trust for + future `UpdateClientMsgs`, and therefore does not require a root for proof + verification. Also, we do not set processed time for this consensus state + since this consensus state should not be used for packet verification as + the root is empty. To ensure the connection can be used for relaying + packets, relayers must submit an `UpdateClientMsg` with a header from the + new chain. + +4. Submit an `UpdateClient` msg by a relayer to the counterparty chain with a + header from the newly upgraded chain + +#### Decisions + +Whenever the IBC handler receives an `MsgUpgradeClient`, it dispatches the +decoded message to the router and triggers the +[process](../../crates/ibc/src/core/ics02_client/handler/upgrade_client.rs#process) +function of `upgrade_client` handler, which would go through the steps outlined +in 3rd section of [Upgrade Process Step-by-Step]. Just note that the `process` +function will be rendered into `validate` and `execute` functions due to ongoing +changes associated with ADR-005, and to align with that one of the decisions +made is to split off the `verify_upgrade_and_update_state` method of +`ClientState` trait into: + +* `verify_upgrade_client` interface + + ```rust + fn verify_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + proof_upgrade_client: MerkleProof, + proof_upgrade_consensus_state: MerkleProof, + root: &CommitmentRoot, + ) -> Result<(), ClientError>; + ``` + +* And `update_state_with_upgrade_client` interface + + ```rust + fn update_state_with_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + ) -> Result; + ``` + +Listed below are the code snippets that correspond to the third step of the +previous section as mentioned: + +* **Basic Validations** + +1. ```rust + if old_client_state.is_frozen() { + return Err(ContextError::ClientError(ClientError::ClientFrozen { + client_id, + })); + } + ``` + +2. ```rust + let old_consensus_state = ctx + .consensus_state(&client_id, &old_client_state.latest_height()) + .map_err(|_| ClientError::ConsensusStateNotFound { + client_id: client_id.clone(), + height: old_client_state.latest_height(), + })?; + + let now = ctx.host_timestamp()?; + let duration = now + .duration_since(&old_consensus_state.timestamp()) + .ok_or_else(|| ClientError::InvalidConsensusStateTimestamp { + time1: old_consensus_state.timestamp(), + time2: now, + })?; + + if old_client_state.expired(duration) { + return Err(ClientError::HeaderNotWithinTrustPeriod { + latest_time: old_consensus_state.timestamp(), + update_time: now, + }); + }; + ``` + +3. ```rust + // Decode the proto message into a domain message inside deliver() function + // include the proofs in the message + let envelope = decode(message)?; + ``` + +* **Chain-specific Validations** + +4. ```rust + let upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?; + ``` + +5. ```rust + let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; + ``` + +6. ```rust + if self.latest_height() >= upgraded_tm_client_state.latest_height() { + return Err(ClientError::LowUpgradeHeight { + upgraded_height: self.latest_height(), + client_height: upgraded_tm_client_state.latest_height(), + }); + } + ``` + +7. ```rust + // Ensure that the new unbonding period is not shorter than the current's client unbonding period + if self.unbonding_period > upgraded_tm_client_state.unbonding_period { + return Err(ClientError::ClientSpecific { + description: + "cannot upgrade client with a shorter unbonding period than the current one" + .to_string(), + }); + } + + // Check to see if the upgrade path is set + let mut upgrade_path = upgraded_tm_client_state.clone().upgrade_path; + if upgrade_path.pop().is_none() { + return Err(ClientError::ClientSpecific { + description: "cannot upgrade client as no upgrade path has been set".to_string(), + }); + }; + ``` + +8. ```rust + let last_height = self.latest_height().revision_height(); + + // Construct the merkle path for the client state + let mut client_upgrade_path = upgrade_path.clone(); + client_upgrade_path.push(ClientUpgradePath::UpgradedClientState(last_height).to_string()); + + let client_upgrade_merkle_path = MerklePath { + key_path: client_upgrade_path, + }; + + let client_state_value = + Protobuf::::encode_vec(&upgraded_tm_client_state) + .map_err(ClientError::Encode)?; + + // Verify the proof of the upgraded client state + merkle_proof_upgrade_client + .verify_membership( + &self.proof_specs, + root.clone().into(), + client_upgrade_merkle_path, + client_state_value, + 0, + ) + .map_err(ClientError::Ics23Verification)?; + ``` + +9. ```rust + let last_height = self.latest_height().revision_height(); + + // Construct the merkle path for the consensus state + let mut cons_upgrade_path = upgrade_path; + cons_upgrade_path + .push(ClientUpgradePath::UpgradedClientConsensusState(last_height).to_string()); + let cons_upgrade_merkle_path = MerklePath { + key_path: cons_upgrade_path, + }; + + let cons_state_value = Protobuf::::encode_vec(&upgraded_tm_cons_state) + .map_err(ClientError::Encode)?; + + // Verify the proof of the upgraded consensus state + merkle_proof_upgrade_cons_state + .verify_membership( + &self.proof_specs, + root.clone().into(), + cons_upgrade_merkle_path, + cons_state_value, + 0, + ) + .map_err(ClientError::Ics23Verification)?; + ``` + +* **Executions** + +10. ```rust + let upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?; + let new_client_state = TmClientState::new( + upgraded_tm_client_state.chain_id, + self.trust_level, + self.trusting_period, + upgraded_tm_client_state.unbonding_period, + self.max_clock_drift, + upgraded_tm_client_state.latest_height, + upgraded_tm_client_state.proof_specs, + upgraded_tm_client_state.upgrade_path, + upgraded_tm_client_state.allow_update, + upgraded_tm_client_state.frozen_height, + )?; + ``` + +11. ```rust + let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; + let sentinel_root = "sentinel_root".as_bytes().to_vec(); + let new_consensus_state = TmConsensusState::new( + sentinel_root.into(), + upgraded_tm_cons_state.timestamp, + upgraded_tm_cons_state.next_validators_hash, + ); + ``` + +## Status + +Proposed + +## Consequences + +### Positive + +* Resolve the issue of unimplemented upgrade client message in `IBC-rs` +* Keep tendermint upgrade client implementation close to the `IBC-Go` + +### Negative + +* This proposal might not cover upgrade edge cases that arise from specific + chain architectures. Thus, there should be further investigation in such + cases. + +### Neutral + +* As a general fact, upgrading processes are tricky by nature, requiring much + more care from developers and support from IBC maintainers + +## References + +* [How to Upgrade IBC Chains and their + Clients](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/quick-guide.md) +* [IBC Client Developer Guide to + Upgrades](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/developer-guide.md) +* [cosmos/cosmos-sdk/Issue 7367: Upgrade + Client](https://github.com/cosmos/cosmos-sdk/pull/7367) +* [cosmos/ibc-go/Issue 2501: Create importable workflow for chains to run + upgrade tests](https://github.com/cosmos/ibc-go/issues/2501) +* [Hermes relayer documentation: Client + Upgrade](https://hermes.informal.systems/documentation/commands/upgrade/index.html) From 3a755547ddf507918855d4287cdfed9f3b8572bb Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 27 Jan 2023 06:57:33 -0800 Subject: [PATCH 2/7] Apply some revisions --- .../adr-006-upgrade-client-implementation.md | 63 ++++++++++--------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 581e6cc91..83b979e86 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -6,16 +6,16 @@ ## Context -The ability to upgrade is crucial for IBC-connected chains. It allows them to -evolve and improve without any restrictions. The IBC module may not be affected -by some upgrades, but some may require it to be upgraded as well to maintain -high-value connections to other chains secure. For having this capability, -chains that implement `IBC-rs` may bring various concerns and characteristics -than Tendermint chains leading to different ways for upgrading their clients. -However there are generic rules that apply to all and can serve as a framework -for any `IBC-rs` implementation. On this basis, this record aims to justify the -logic behind upgrading a light client, determine the boundary between -client-wide and client-specific upgrade processes, list requisites for +The ability to upgrade is a crucial feature for IBC-connected chains, as it +enables them to evolve and improve without limitations. The IBC module may +not be affected by some upgrades, but some may require it to be upgraded as well +to maintain high-value connections to other chains secure. For having this +capability, chains that implement `IBC-rs` may bring various concerns and +characteristics than Tendermint chains leading to different ways for upgrading +their clients. However there are general rules that apply to all and can serve +as a framework for any `IBC-rs` implementation. On this basis, this record aims +to justify the logic behind upgrading a light client, determine the boundary +between client-wide and client-specific upgrade processes, list requisites for validation and execution steps, and explain Tendermint's upgrade client implementation within the [ics07_tendermint](../../crates/ibc/src/clients/ics07_tendermint). @@ -38,10 +38,6 @@ implementation of this rationale in `IBC-rs` is explained. breaks the counterparty IBC client. * There **MUST** be a proof verification process to check upgraded client and consensus states against the host chain's state. -* Clients **MUST** ensure that the planned last height of the current revision - is somehow encoded in the proof verification process. This prevents premature - upgrades, as the counterparty may cancel or modify upgrade plans before the - last planned height. * Chain upgrades **MUST NOT** result in changing [ClientState](../../crates/ibc/src/core/ics02_client/client_state.rs#ClientState) or @@ -66,11 +62,16 @@ provided by `ClientState` and `ConsensusState` traits, like `is_frozen()`, * Latest client state **MUST NOT** be frozen * Latest consensus state **MUST** be within the trusting period of the latest client state -* Verification proofs **MUST** successfully be decoded into the domain types +* Received update message including verification proofs **MUST** successfully be + decoded into the domain types +* Clients **MUST** ensure that the planned last height of the current revision + is somehow encoded in the proof verification process. This prevents premature + upgrades, as the counterparty may cancel or modify upgrade plans before the + last planned height. Any other requisite beyond the above rules are considered client-specific. Next, -we go through the upgrade client process for Tendermint chains to justify the -logic and illustrate an example of client-specific upgrade process. +we go through the upgrade process for Tendermint clients to justify the logic +and illustrate an example of client-specific upgrade. ### Upgrade Tendermint Clients @@ -145,7 +146,7 @@ upgrade its own chain and counterparty's IBC client: upgrade/UpgradedIBCState/{upgradeHeight}/upgradedConsState ``` -2. Submit an upgrade client message to the counterparty chain by a relayer +2. Submit an upgrade client message by relayers to the counterparty chain 1. Wait for the upgrading chain to reach the upgrade height and halt 2. Query a full node for the proofs of `UpgradedClient` and `UpgradedConsensusState` at the last height of the old chain @@ -155,11 +156,11 @@ upgrade its own chain and counterparty's IBC client: `UpgradedClient`, `UpgradedConsensusState` and their respective proofs 3. Process the upgrade message on the counterparty chain
IBC handler upon -receiving an `MsgUpgradeClient` message performs basic validations (BV), -client-specific validations (SV) and lastly execution (E) steps as follows. +receiving a `MsgUpgradeClient` message performs basic validations (BV), +client-specific validations (SV) and lastly execution (E) steps as follows: 1. (BV) Check that the current client is not frozen 2. (BV) Check if the latest consensus state is within the trust period - 3. (BV) Decode proofs and check that they are valid + 3. (BV) Check if the message containing proofs decoded successfully 4. (SV) Verify that the upgradedClient be of a Tendermint `ClientState` type 5. (SV) Verify that the upgradedConsensusState be of a Tendermint `ConsensusState` type @@ -193,13 +194,13 @@ Whenever the IBC handler receives an `MsgUpgradeClient`, it dispatches the decoded message to the router and triggers the [process](../../crates/ibc/src/core/ics02_client/handler/upgrade_client.rs#process) function of `upgrade_client` handler, which would go through the steps outlined -in 3rd section of [Upgrade Process Step-by-Step]. Just note that the `process` -function will be rendered into `validate` and `execute` functions due to ongoing -changes associated with ADR-005, and to align with that one of the decisions -made is to split off the `verify_upgrade_and_update_state` method of -`ClientState` trait into: +in 3rd section of [Upgrade Process Step-by-Step](#upgrade-process-step-by-step). +Just note that the `process` function will be rendered into `validate` and +`execute` functions due to ongoing changes associated with ADR-005, and to align +with that one of the decisions made is to split off the +`verify_upgrade_and_update_state` method of `ClientState` trait into: -* `verify_upgrade_client` interface +* `verify_upgrade_client` method ```rust fn verify_upgrade_client( @@ -212,7 +213,7 @@ made is to split off the `verify_upgrade_and_update_state` method of ) -> Result<(), ClientError>; ``` -* And `update_state_with_upgrade_client` interface +* And `update_state_with_upgrade_client` method ```rust fn update_state_with_upgrade_client( @@ -412,8 +413,10 @@ Proposed Clients](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/quick-guide.md) * [IBC Client Developer Guide to Upgrades](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/developer-guide.md) -* [cosmos/cosmos-sdk/Issue 7367: Upgrade - Client](https://github.com/cosmos/cosmos-sdk/pull/7367) +* [cosmos/ibc-go/Issue 445: IBC upgrade plan + summary](https://github.com/cosmos/ibc/issues/445) +* [cosmos/cosmos-sdk/PR 7367: Upgrade + Client](https://github.com/cosmos/cosmos-sdk/åpull/7367) * [cosmos/ibc-go/Issue 2501: Create importable workflow for chains to run upgrade tests](https://github.com/cosmos/ibc-go/issues/2501) * [Hermes relayer documentation: Client From 64cca7e0fe5865f26bd9a222fc9d8a2e53afc05f Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 27 Jan 2023 08:07:31 -0800 Subject: [PATCH 3/7] Add a description for zero_custom_fields --- .../adr-006-upgrade-client-implementation.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 83b979e86..01badfcf7 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -170,6 +170,7 @@ client-specific validations (SV) and lastly execution (E) steps as follows: such as ChainID, UnbondingPeriod, and ProofSpecs with the committed client 8. (SV) Verify that the upgrading chain did indeed commit to the upgraded client state at the upgrade height by provided proof + Note: client-customizable fields must be zeroed out for this check 9. (SV) Verify that the upgrading chain did indeed commit to the upgraded consensus state at the upgrade height by provided proof 10. (E) Upgrade client to the new client by retaining old client-customizable @@ -223,6 +224,12 @@ with that one of the decisions made is to split off the ) -> Result; ``` +There is also a need to move away from `upgrade` method of `ClientState` trait +leaving only zeroed out fields through `zero_custom_fields()`. New state +commitment should be done in `update_state_with_upgrade_client` and stored via +`store_client_result` of keeper context, so the upgrade part is no longer +necessary. + Listed below are the code snippets that correspond to the third step of the previous section as mentioned: @@ -315,6 +322,7 @@ previous section as mentioned: key_path: client_upgrade_path, }; + upgraded_tm_client_state.zero_custom_fields(); let client_state_value = Protobuf::::encode_vec(&upgraded_tm_client_state) .map_err(ClientError::Encode)?; @@ -385,6 +393,8 @@ previous section as mentioned: ); ``` +```rust + ## Status Proposed From 1b4dd3115e36682e4486d0f18ca3f3f2bc92e2fd Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 27 Jan 2023 13:34:45 -0800 Subject: [PATCH 4/7] Apply some revisions --- .../adr-006-upgrade-client-implementation.md | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 01badfcf7..965c64c89 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -87,35 +87,36 @@ supported by `IBC-rs`: * S: Supported, P: Partially Supported, U: Unsupported 1. (S) Changing the `ChainId` -2. (P) Changing the `UnbondingPeriod`: chains may increase the unbonding period - with no issues. However, decreasing the unbonding period may irreversibly - break some counterparty clients. Thus, it is not recommended that chains - reduce the unbonding period. -3. (S) Changing the `Height` (resetting to 0): as long as chains remember to +2. (S) Changing the `Height` (resetting to 0): as long as chains remember to increment the revision number in their chain-id. -4. (S) Changing the `ProofSpecs`: this should be changed if the proof structure +3. (S) Changing the `ProofSpecs`: this should be changed if the proof structure needed to verify IBC proofs is changed across the upgrade. Ex: Switching from an IAVL store, to a SimpleTree Store. -5. (S) Changing the `UpgradePath`: this might involve changing the key under +4. (S) Changing the `UpgradePath`: this might involve changing the key under which upgraded clients and consensus states are stored in the upgrade store, or even migrating the upgrade store itself. -6. (U) Migrating the IBC store: the store location is negotiated by the - connection. -7. (S) Upgrading to a backwards compatible version of IBC -8. (U) Upgrading to a non-backwards compatible version of IBC: the version is - negotiated on connection handshake. -9. (U) Changing parameters that are customizable by relayers like `TrustLevel` - and `TrustingPeriod`, `max_clock_drift` -10. (P) Changing the Tendermint LightClient algorithm: Changes to the light +5. (S) Upgrading to a backwards compatible version of IBC +6. (P) Changing the `UnbondingPeriod`: chains may increase the unbonding period + with no issues. However, decreasing the unbonding period may irreversibly + break some counterparty clients. Thus, it is not recommended that chains + reduce the unbonding period. +7. (P) Changing the Tendermint LightClient algorithm: Changes to the light client algorithm that do not change the [ClientState](../../crates/ibc/src/clients/ics07_tendermint/client_state.rs#ClientState) or [ConsensusState](../../crates/ibc/src/clients/ics07_tendermint/consensus_state.rs#ConsensusState) - struct may be supported, provided that the counterparty is also upgraded to - support the new light client algorithm. Changes that require updating the - `ClientState` and `ConsensusState` structs themselves are theoretically - possible by providing a path to translate an older ClientState struct into - the new ClientState struct; however this is not currently implemented. + struct abstraction may be supported, provided that the counterparty is also + upgraded to support the new light client algorithm. Changes that require + updating the `ClientState` and `ConsensusState` structs themselves are + theoretically possible by providing a path to translate an older ClientState + struct into the new ClientState struct; however this is not currently + implemented. +8. (U) Migrating the IBC store: the store location is negotiated by the + connection. +9. (U) Upgrading to a non-backwards compatible version of IBC: the version is + negotiated on connection handshake. +10. (U) Changing parameters that are customizable by relayers like `TrustLevel` + and `TrustingPeriod`, `max_clock_drift` #### Upgrade Process Step-by-step @@ -155,9 +156,9 @@ upgrade its own chain and counterparty's IBC client: 4. Submit an `UpgradeClient` msg to the counterparty chain with the `UpgradedClient`, `UpgradedConsensusState` and their respective proofs -3. Process the upgrade message on the counterparty chain
IBC handler upon -receiving a `MsgUpgradeClient` message performs basic validations (BV), -client-specific validations (SV) and lastly execution (E) steps as follows: +3. Process the upgrade message on the counterparty chain upon receiving a +`MsgUpgradeClient` message performs basic validations (BV), client-specific +validations (SV) and lastly execution (E) steps as follows: 1. (BV) Check that the current client is not frozen 2. (BV) Check if the latest consensus state is within the trust period 3. (BV) Check if the message containing proofs decoded successfully @@ -177,7 +178,7 @@ client-specific validations (SV) and lastly execution (E) steps as follows: parameters (sent by relayers) such `TrustingPeriod`, `TrustLevel`, `MaxClockDrift` and adopt the new chain-specified fields such as `UnbondingPeriod`, `ChainId`, `UpgradePath`, etc. - 11. (E) Upgrade consensus state with a stand-in sentinel value for root + 11. (E) Upgrade consensus state with a stand-in sentinel value for root Note: The upgraded consensus state serve purely as a basis of trust for future `UpdateClientMsgs`, and therefore does not require a root for proof verification. Also, we do not set processed time for this consensus state @@ -393,8 +394,6 @@ previous section as mentioned: ); ``` -```rust - ## Status Proposed From d59ddfe9c718ded5121c479e562338b9ebcc2c36 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 30 Jan 2023 10:05:23 -0800 Subject: [PATCH 5/7] Apply review comments * Remove trusting period bound for general MUSTs * Revise clause related to proof verification * Add a NOTE for chain-level upgrade steps * Reword height check clause --- .../adr-006-upgrade-client-implementation.md | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 965c64c89..7e3f6ff5b 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -7,9 +7,9 @@ ## Context The ability to upgrade is a crucial feature for IBC-connected chains, as it -enables them to evolve and improve without limitations. The IBC module may -not be affected by some upgrades, but some may require it to be upgraded as well -to maintain high-value connections to other chains secure. For having this +enables them to evolve and improve without limitations. The IBC module may not +be affected by some upgrades, but some may require it to be upgraded as well to +maintain high-value connections to other chains secure. For having this capability, chains that implement `IBC-rs` may bring various concerns and characteristics than Tendermint chains leading to different ways for upgrading their clients. However there are general rules that apply to all and can serve @@ -60,14 +60,12 @@ provided by `ClientState` and `ConsensusState` traits, like `is_frozen()`, `latest_height()`, etc. * Latest client state **MUST NOT** be frozen -* Latest consensus state **MUST** be within the trusting period of the latest - client state * Received update message including verification proofs **MUST** successfully be decoded into the domain types -* Clients **MUST** ensure that the planned last height of the current revision - is somehow encoded in the proof verification process. This prevents premature - upgrades, as the counterparty may cancel or modify upgrade plans before the - last planned height. +* Clients **MUST** only accept upgrades at the planned last height of the + current revision which is somehow encoded in the proof verification process. + This prevents premature upgrades, as the counterparty may cancel or modify + upgrade plans before the last planned height. Any other requisite beyond the above rules are considered client-specific. Next, we go through the upgrade process for Tendermint clients to justify the logic @@ -121,7 +119,9 @@ supported by `IBC-rs`: #### Upgrade Process Step-by-step An IBC-connected Tendermint chain will take the following steps to completely -upgrade its own chain and counterparty's IBC client: +upgrade its own chain and counterparty's IBC client. Note that the chain-level +upgrade instruction (1) is not a part of the IBC protocol. It is provided for +the sake of the big picture and as an example of how the upgrade process can be. 1. Upgrade chain through governance 1. Create a 02-client @@ -165,29 +165,29 @@ validations (SV) and lastly execution (E) steps as follows: 4. (SV) Verify that the upgradedClient be of a Tendermint `ClientState` type 5. (SV) Verify that the upgradedConsensusState be of a Tendermint `ConsensusState` type - 6. (SV) Check the latest height of the current client state does not have the - same revision number or has a greater height than the committed client + 6. (SV) Check the height of the committed client state is not greater than + the latest height of the current client state 7. (SV) Match any Tendermint chain specified parameter in upgraded client such as ChainID, UnbondingPeriod, and ProofSpecs with the committed client 8. (SV) Verify that the upgrading chain did indeed commit to the upgraded - client state at the upgrade height by provided proof - Note: client-customizable fields must be zeroed out for this check + client state at the upgrade height by provided proof Note: + client-customizable fields must be zeroed out for this check 9. (SV) Verify that the upgrading chain did indeed commit to the upgraded consensus state at the upgrade height by provided proof 10. (E) Upgrade client to the new client by retaining old client-customizable parameters (sent by relayers) such `TrustingPeriod`, `TrustLevel`, `MaxClockDrift` and adopt the new chain-specified fields such as `UnbondingPeriod`, `ChainId`, `UpgradePath`, etc. - 11. (E) Upgrade consensus state with a stand-in sentinel value for root - Note: The upgraded consensus state serve purely as a basis of trust for - future `UpdateClientMsgs`, and therefore does not require a root for proof + 11. (E) Upgrade consensus state with a stand-in sentinel value for root Note: + The upgraded consensus state serve purely as a basis of trust for future + `UpdateClientMsgs`, and therefore does not require a root for proof verification. Also, we do not set processed time for this consensus state since this consensus state should not be used for packet verification as the root is empty. To ensure the connection can be used for relaying packets, relayers must submit an `UpdateClientMsg` with a header from the new chain. -4. Submit an `UpdateClient` msg by a relayer to the counterparty chain with a +1. Submit an `UpdateClient` msg by a relayer to the counterparty chain with a header from the newly upgraded chain #### Decisions From 92e0334803b5e0363a1c5274b9194a3096e36f3a Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 30 Jan 2023 11:51:59 -0800 Subject: [PATCH 6/7] Add description for sentinel value --- .../adr-006-upgrade-client-implementation.md | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 7e3f6ff5b..f394e626c 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -170,7 +170,7 @@ validations (SV) and lastly execution (E) steps as follows: 7. (SV) Match any Tendermint chain specified parameter in upgraded client such as ChainID, UnbondingPeriod, and ProofSpecs with the committed client 8. (SV) Verify that the upgrading chain did indeed commit to the upgraded - client state at the upgrade height by provided proof Note: + client state at the upgrade height by provided proof. Note that the client-customizable fields must be zeroed out for this check 9. (SV) Verify that the upgrading chain did indeed commit to the upgraded consensus state at the upgrade height by provided proof @@ -178,16 +178,18 @@ validations (SV) and lastly execution (E) steps as follows: parameters (sent by relayers) such `TrustingPeriod`, `TrustLevel`, `MaxClockDrift` and adopt the new chain-specified fields such as `UnbondingPeriod`, `ChainId`, `UpgradePath`, etc. - 11. (E) Upgrade consensus state with a stand-in sentinel value for root Note: - The upgraded consensus state serve purely as a basis of trust for future - `UpdateClientMsgs`, and therefore does not require a root for proof - verification. Also, we do not set processed time for this consensus state - since this consensus state should not be used for packet verification as - the root is empty. To ensure the connection can be used for relaying - packets, relayers must submit an `UpdateClientMsg` with a header from the - new chain. - -1. Submit an `UpdateClient` msg by a relayer to the counterparty chain with a + 11. (E) Upgrade consensus state with a stand-in sentinel value for root. Note + that the upgraded consensus state serves purely as a basis of trust for + future `UpdateClientMsgs`, and therefore it does not require a root for + proof verification and it is not used for packet verifications as well. + That sentinel value serves as a temporary substitute until the root of new + chain gets available by `UpdateClientMsg`. It is set by module with a + distinct, easily recognizable value to reduce the risk of bugs. Thereby, + we do not also set a processed time for this consensus state. To ensure + the connection can be used for relaying packets, relayers must submit an + `UpdateClientMsg` with a header from the new chain. + +4. Submit an `UpdateClient` msg by a relayer to the counterparty chain with a header from the newly upgraded chain #### Decisions From ec94b512018aec6c5b8d5947426ccf781734673f Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 30 Jan 2023 21:00:42 -0800 Subject: [PATCH 7/7] Clarify basic vs upgrade-specific validations + notice for upgradedConsState --- .../adr-006-upgrade-client-implementation.md | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index f394e626c..90df47b14 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -14,10 +14,10 @@ capability, chains that implement `IBC-rs` may bring various concerns and characteristics than Tendermint chains leading to different ways for upgrading their clients. However there are general rules that apply to all and can serve as a framework for any `IBC-rs` implementation. On this basis, this record aims -to justify the logic behind upgrading a light client, determine the boundary -between client-wide and client-specific upgrade processes, list requisites for -validation and execution steps, and explain Tendermint's upgrade client -implementation within the +to justify the chain-wide logic behind upgrading light clients, list requisites +for validation and execution steps, determine the boundary between basic and +upgrade-specific validations by an IBC handler, and explain Tendermint's upgrade +client implementation within the [ics07_tendermint](../../crates/ibc/src/clients/ics07_tendermint). ## Decision @@ -46,18 +46,17 @@ implementation of this rationale in `IBC-rs` is explained. * It is **UP TO** the chain's architecture how updated client and consensus states are committed, either through decentralized approaches, like governance or centralized methods, like a multisig account, etc. -* Upon commitment, chain's store **MUST** contain updated client and consensus +* Upon commitment, chain's store **MUST** contain upgraded client and consensus states, which can be retrieved using respective upgrade paths #### IBC Handler accepts upgrade? After ensuring that the chain upgrades is supported by IBC, the general validation and execution steps that apply to all chains are as follows. The -criteria for classifying a validation as client-wide (also can be known as -basic) or client-specific was whether that IBC handler can perform that action -just using its own context data, which are available upon calling interfaces -provided by `ClientState` and `ConsensusState` traits, like `is_frozen()`, -`latest_height()`, etc. +criteria for classifying a validation as basic or upgrade-specific was whether +that IBC handler can perform that check just using its own contextual data, +which are available upon calling interfaces provided by `ClientState` and +`ConsensusState` traits, like `is_frozen()`, `latest_height()`, etc. * Latest client state **MUST NOT** be frozen * Received update message including verification proofs **MUST** successfully be @@ -66,6 +65,8 @@ provided by `ClientState` and `ConsensusState` traits, like `is_frozen()`, current revision which is somehow encoded in the proof verification process. This prevents premature upgrades, as the counterparty may cancel or modify upgrade plans before the last planned height. +* Latest consensus state **MUST** be within the trusting period of the latest + client state, which for clients without a trusting period is not applicable. Any other requisite beyond the above rules are considered client-specific. Next, we go through the upgrade process for Tendermint clients to justify the logic @@ -121,7 +122,9 @@ supported by `IBC-rs`: An IBC-connected Tendermint chain will take the following steps to completely upgrade its own chain and counterparty's IBC client. Note that the chain-level upgrade instruction (1) is not a part of the IBC protocol. It is provided for -the sake of the big picture and as an example of how the upgrade process can be. +the sake of the big picture and as a reference to follow the upgrade process +from the very beginning when a proposal is initiated to when the upgrade message +is entirely handled. 1. Upgrade chain through governance 1. Create a 02-client @@ -147,17 +150,21 @@ the sake of the big picture and as an example of how the upgrade process can be. upgrade/UpgradedIBCState/{upgradeHeight}/upgradedConsState ``` + Notice that since the `UpgradedConsensusState` will not be available at + the upgrade path prior to this height, relayers cannot submit a valid + upgrade message as the proof verification would fail + 2. Submit an upgrade client message by relayers to the counterparty chain 1. Wait for the upgrading chain to reach the upgrade height and halt 2. Query a full node for the proofs of `UpgradedClient` and `UpgradedConsensusState` at the last height of the old chain 3. Update the counterparty client to the last height of the old chain using the `UpdateClient` msg - 4. Submit an `UpgradeClient` msg to the counterparty chain with the + 4. Submit a `MsgUpgradeClient` message to the counterparty chain with the `UpgradedClient`, `UpgradedConsensusState` and their respective proofs 3. Process the upgrade message on the counterparty chain upon receiving a -`MsgUpgradeClient` message performs basic validations (BV), client-specific +`MsgUpgradeClient` message performs basic validations (BV), upgrade-specific validations (SV) and lastly execution (E) steps as follows: 1. (BV) Check that the current client is not frozen 2. (BV) Check if the latest consensus state is within the trust period