From 1fb3547a78e5dfa2c9bba99261a1f3a90de9bb7f Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 1 Feb 2023 21:28:39 +0330 Subject: [PATCH] Implement `verify_upgrade_and_update_state` for Tendermint Client (#349) * Implement verify_upgrade_and_update_state * Construct new_client_state and new_consensus_state * Split off verification and execution steps * Revise some namings and comments * Comment out some upgrade tests * Update mock tests related to ugrade_client * Add changelog entry * Refactor verify_upgrade_client to use Path and Encode error type * Remove pub before UPGRADE const * Rewrite upgrade_client unit tests * Add unbonding period validation step * Set root of new consensus state with sentinel value * Revise some comments * Refactor upgrade method to zero_custom_fields * Mend fields of new client state * Disable upgrade client handler * Fix issue with cargo test * Flip upgrade_client feature flag * Remove unnecessary unbonding period check --------- Signed-off-by: Farhad Shabani --- .../19-implement-verify-upgrade-client.md | 2 + crates/ibc/Cargo.toml | 3 + .../clients/ics07_tendermint/client_state.rs | 191 +++++++-- .../ibc/src/core/ics02_client/client_state.rs | 32 +- crates/ibc/src/core/ics02_client/error.rs | 2 + .../ics02_client/handler/upgrade_client.rs | 379 ++++++++++-------- .../core/ics02_client/msgs/upgrade_client.rs | 7 + crates/ibc/src/core/ics26_routing/handler.rs | 4 +- crates/ibc/src/mock/client_state.rs | 39 +- .../adr-006-upgrade-client-implementation.md | 39 +- 10 files changed, 446 insertions(+), 252 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/19-implement-verify-upgrade-client.md diff --git a/.changelog/unreleased/breaking-changes/19-implement-verify-upgrade-client.md b/.changelog/unreleased/breaking-changes/19-implement-verify-upgrade-client.md new file mode 100644 index 000000000..5f3498c23 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/19-implement-verify-upgrade-client.md @@ -0,0 +1,2 @@ +- Implement `verify_upgrade_and_update_state` method for Tendermint clients + ([#19](https://github.com/cosmos/ibc-rs/issues/19)). diff --git a/crates/ibc/Cargo.toml b/crates/ibc/Cargo.toml index f70bf6a3b..d028a2665 100644 --- a/crates/ibc/Cargo.toml +++ b/crates/ibc/Cargo.toml @@ -45,6 +45,9 @@ serde = ["dep:serde", "dep:serde_derive", "serde_json", "erased-serde"] # This feature guards the unfinished implementation of ADR 5. val_exec_ctx = [] +# This feature guards the unfinished implementation of the `UpgradeClient` handler. +upgrade_client = [] + # This feature grants access to development-time mocking libraries, such as `MockContext` or `MockHeader`. # Depends on the `testgen` suite for generating Tendermint light blocks. mocks = ["tendermint-testgen", "tendermint/clock", "cfg-if", "parking_lot"] diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index b0aecf3aa..fb226707c 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -5,8 +5,10 @@ use core::time::Duration; use ibc_proto::google::protobuf::Any; use ibc_proto::ibc::core::client::v1::Height as RawHeight; -use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof; -use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawTmClientState; +use ibc_proto::ibc::core::commitment::v1::{MerklePath, MerkleProof as RawMerkleProof}; +use ibc_proto::ibc::lightclients::tendermint::v1::{ + ClientState as RawTmClientState, ConsensusState as RawTmConsensusState, +}; use ibc_proto::protobuf::Protobuf; use prost::Message; use tendermint::chain::id::MAX_LENGTH as MaxChainIdLen; @@ -15,13 +17,12 @@ use tendermint_light_client_verifier::options::Options; use tendermint_light_client_verifier::types::{TrustedBlockState, UntrustedBlockState}; use tendermint_light_client_verifier::{ProdVerifier, Verifier}; +use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState; use crate::clients::ics07_tendermint::consensus_state::ConsensusState as TmConsensusState; use crate::clients::ics07_tendermint::error::{Error, IntoResult}; use crate::clients::ics07_tendermint::header::{Header as TmHeader, Header}; use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehaviour; -use crate::core::ics02_client::client_state::{ - ClientState as Ics2ClientState, UpdatedState, UpgradeOptions as CoreUpgradeOptions, -}; +use crate::core::ics02_client::client_state::{ClientState as Ics2ClientState, UpdatedState}; use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::context::ClientReader; @@ -38,8 +39,8 @@ use crate::core::ics23_commitment::merkle::{apply_prefix, MerkleProof}; use crate::core::ics23_commitment::specs::ProofSpecs; use crate::core::ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId}; use crate::core::ics24_host::path::{ - AcksPath, ChannelEndsPath, ClientConsensusStatePath, ClientStatePath, CommitmentsPath, - ConnectionsPath, ReceiptsPath, SeqRecvsPath, + AcksPath, ChannelEndsPath, ClientConsensusStatePath, ClientStatePath, ClientUpgradePath, + CommitmentsPath, ConnectionsPath, ReceiptsPath, SeqRecvsPath, }; use crate::core::ics24_host::Path; use crate::timestamp::{Timestamp, ZERO_DURATION}; @@ -351,14 +352,6 @@ impl ClientState { } } -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct UpgradeOptions { - pub unbonding_period: Duration, -} - -impl CoreUpgradeOptions for UpgradeOptions {} - impl Ics2ClientState for ClientState { fn chain_id(&self) -> ChainId { self.chain_id.clone() @@ -376,17 +369,7 @@ impl Ics2ClientState for ClientState { self.frozen_height } - fn upgrade( - &mut self, - upgrade_height: Height, - upgrade_options: &dyn CoreUpgradeOptions, - chain_id: ChainId, - ) { - let upgrade_options = upgrade_options - .as_any() - .downcast_ref::() - .expect("UpgradeOptions not of type Tendermint"); - + fn zero_custom_fields(&mut self) { // Reset custom fields to zero values self.trusting_period = ZERO_DURATION; self.trust_level = TrustThreshold::ZERO; @@ -394,11 +377,6 @@ impl Ics2ClientState for ClientState { self.allow_update.after_misbehaviour = false; self.frozen_height = None; self.max_clock_drift = ZERO_DURATION; - - // Upgrade the client state - self.latest_height = upgrade_height; - self.unbonding_period = upgrade_options.unbonding_period; - self.chain_id = chain_id; } fn expired(&self, elapsed: Duration) -> bool { @@ -876,13 +854,154 @@ impl Ics2ClientState for ClientState { }) } - fn verify_upgrade_and_update_state( + /// Perform client-specific verifications and check all data in the new + /// client state to be the same across all valid Tendermint clients for the + /// new chain. + /// + /// You can learn more about how to upgrade IBC-connected SDK chains in + /// [this](https://ibc.cosmos.network/main/ibc/upgrades/quick-guide.html) + /// guide + fn verify_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + proof_upgrade_client: RawMerkleProof, + proof_upgrade_consensus_state: RawMerkleProof, + root: &CommitmentRoot, + ) -> Result<(), ClientError> { + // Make sure that the client type is of Tendermint type `ClientState` + let mut upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?; + + // Make sure that the consensus type is of Tendermint type `ConsensusState` + let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; + + // Note: verification of proofs that unmarshalled correctly has been done + // while decoding the proto message into a `MsgEnvelope` domain type + let merkle_proof_upgrade_client = MerkleProof::from(proof_upgrade_client); + let merkle_proof_upgrade_cons_state = MerkleProof::from(proof_upgrade_consensus_state); + + // Make sure the latest height of the current client is not greater then + // the upgrade height This condition checks both the revision number and + // the height + 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(), + }); + } + + // Check to see if the upgrade path is set + let mut upgrade_path = self.upgrade_path.clone(); + if upgrade_path.pop().is_none() { + return Err(ClientError::ClientSpecific { + description: "cannot upgrade client as no upgrade path has been set".to_string(), + }); + }; + + 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, + }; + + upgraded_tm_client_state.zero_custom_fields(); + 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)?; + + // 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)?; + + Ok(()) + } + + // Commit the new client state and consensus state to the store + fn update_state_with_upgrade_client( &self, - _consensus_state: Any, - _proof_upgrade_client: RawMerkleProof, - _proof_upgrade_consensus_state: RawMerkleProof, + upgraded_client_state: Any, + upgraded_consensus_state: Any, ) -> Result { - unimplemented!() + let upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?; + let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; + + // Frozen height is set to None fo the new client state + let new_frozen_height = None; + + // Construct new client state and consensus state relayer chosen client + // parameters are ignored. All chain-chosen parameters come from + // committed client, all client-chosen parameters come from current + // client. + 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, + self.allow_update, + new_frozen_height, + )?; + + // The new consensus state is merely used as a trusted kernel against + // which headers on the new chain can be verified. The root is just a + // stand-in sentinel value as it cannot be known in advance, thus no + // proof verification will pass. The timestamp and the + // NextValidatorsHash of the consensus state is the blocktime and + // NextValidatorsHash of the last block committed by the old chain. This + // will allow the first block of the new chain to be verified against + // the last validators of the old chain so long as it is submitted + // within the TrustingPeriod of this client. + // NOTE: 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. The next consensus state submitted using update + // will be usable for packet-verification. + 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, + ); + + Ok(UpdatedState { + client_state: new_client_state.into_box(), + consensus_state: new_consensus_state.into_box(), + }) } fn verify_client_consensus_state( diff --git a/crates/ibc/src/core/ics02_client/client_state.rs b/crates/ibc/src/core/ics02_client/client_state.rs index b5e75112c..ec2cfb9b2 100644 --- a/crates/ibc/src/core/ics02_client/client_state.rs +++ b/crates/ibc/src/core/ics02_client/client_state.rs @@ -63,12 +63,7 @@ pub trait ClientState: /// Helper function to verify the upgrade client procedure. /// Resets all fields except the blockchain-specific ones, /// and updates the given fields. - fn upgrade( - &mut self, - upgrade_height: Height, - upgrade_options: &dyn UpgradeOptions, - chain_id: ChainId, - ); + fn zero_custom_fields(&mut self); /// Convert into a boxed trait object fn into_box(self) -> Box @@ -112,11 +107,30 @@ pub trait ClientState: misbehaviour: Any, ) -> Result, ContextError>; - fn verify_upgrade_and_update_state( + /// Verify the upgraded client and consensus states and validate proofs + /// against the given root. + /// + /// NOTE: proof heights are not included as upgrade to a new revision is + /// expected to pass only on the last height committed by the current + /// revision. Clients are responsible for ensuring that the planned last + /// height of the current revision is somehow encoded in the proof + /// verification process. This is to ensure that no premature upgrades + /// occur, since upgrade plans committed to by the counterparty may be + /// cancelled or modified before the last planned height. + fn verify_upgrade_client( &self, - consensus_state: Any, + upgraded_client_state: Any, + upgraded_consensus_state: Any, proof_upgrade_client: MerkleProof, proof_upgrade_consensus_state: MerkleProof, + root: &CommitmentRoot, + ) -> Result<(), ClientError>; + + // Update the client state and consensus state in the store with the upgraded ones. + fn update_state_with_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, ) -> Result; /// Verification functions as specified in: @@ -274,8 +288,6 @@ pub fn downcast_client_state(h: &dyn ClientState) -> Option<&CS h.as_any().downcast_ref::() } -pub trait UpgradeOptions: AsAny {} - pub struct UpdatedState { pub client_state: Box, pub consensus_state: Box, diff --git a/crates/ibc/src/core/ics02_client/error.rs b/crates/ibc/src/core/ics02_client/error.rs index ceaca144e..d35d0f786 100644 --- a/crates/ibc/src/core/ics02_client/error.rs +++ b/crates/ibc/src/core/ics02_client/error.rs @@ -49,6 +49,8 @@ pub enum ClientError { MissingRawConsensusState, /// invalid client id in the update client message: `{0}` InvalidMsgUpdateClientId(ValidationError), + /// Encode error: `{0}` + Encode(TendermintProtoError), /// decode error: `{0}` Decode(prost::DecodeError), /// invalid client identifier error: `{0}` diff --git a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs index 1fc36065b..ebb0bed6e 100644 --- a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs @@ -35,22 +35,57 @@ where { let MsgUpgradeClient { client_id, .. } = msg; - // Read client state from the host chain store. + // Temporary has been disabled until we have a better understanding of some design implications + if cfg!(feature = "disable_upgrade_client") { + return Err(ContextError::ClientError(ClientError::Other { + description: "upgrade_client feature is not supported".to_string(), + })); + } + + // Read the current latest client state from the host chain store. let old_client_state = ctx.client_state(&client_id)?; + // Check if the client is frozen. if old_client_state.is_frozen() { - return Err(ClientError::ClientFrozen { client_id }.into()); + return Err(ContextError::ClientError(ClientError::ClientFrozen { + client_id, + })); } - let upgrade_client_state = ctx.decode_client_state(msg.client_state)?; - - if old_client_state.latest_height() >= upgrade_client_state.latest_height() { - return Err(ClientError::LowUpgradeHeight { - upgraded_height: old_client_state.latest_height(), - client_height: upgrade_client_state.latest_height(), - } - .into()); - } + // Read the latest consensus state from the host chain store. + 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, + })?; + + // Check if the latest consensus state is within the trust period. + if old_client_state.expired(duration) { + return Err(ContextError::ClientError( + ClientError::HeaderNotWithinTrustPeriod { + latest_time: old_consensus_state.timestamp(), + update_time: now, + }, + )); + }; + + // Validate the upgraded client state and consensus state and verify proofs against the root + old_client_state.verify_upgrade_client( + msg.client_state.clone(), + msg.consensus_state.clone(), + msg.proof_upgrade_client.clone(), + msg.proof_upgrade_consensus_state, + old_consensus_state.root(), + )?; Ok(()) } @@ -62,19 +97,13 @@ where { let MsgUpgradeClient { client_id, .. } = msg; - let upgrade_client_state = ctx.decode_client_state(msg.client_state)?; + let old_client_state = ctx.client_state(&client_id)?; let UpdatedState { client_state, consensus_state, - } = upgrade_client_state.verify_upgrade_and_update_state( - msg.consensus_state.clone(), - msg.proof_upgrade_client.clone(), - msg.proof_upgrade_consensus_state, - )?; - - // Not implemented yet: https://github.com/informalsystems/ibc-rs/issues/722 - // todo!() + } = old_client_state + .update_state_with_upgrade_client(msg.client_state.clone(), msg.consensus_state)?; ctx.store_client_state(ClientStatePath(client_id.clone()), client_state.clone())?; ctx.store_consensus_state( @@ -98,52 +127,77 @@ pub(crate) fn process( let mut output = HandlerOutput::builder(); let MsgUpgradeClient { client_id, .. } = msg; - // Read client state from the host chain store. + // Temporary has been disabled until we have a better understanding of some design implications + if !cfg!(feature = "upgrade_client") { + return Err(ClientError::Other { + description: "upgrade_client feature is not supported".to_string(), + }); + } + + // Read the current latest client state from the host chain store. let old_client_state = ctx.client_state(&client_id)?; + // Check if the client is frozen. if old_client_state.is_frozen() { return Err(ClientError::ClientFrozen { client_id }); } - let upgrade_client_state = ctx.decode_client_state(msg.client_state)?; - - if old_client_state.latest_height() >= upgrade_client_state.latest_height() { - return Err(ClientError::LowUpgradeHeight { - upgraded_height: old_client_state.latest_height(), - client_height: upgrade_client_state.latest_height(), + // Read the latest consensus state from the host chain store. + 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, + })?; + + // Check if the latest consensus state is within the trust period. + if old_client_state.expired(duration) { + return Err(ClientError::HeaderNotWithinTrustPeriod { + latest_time: old_consensus_state.timestamp(), + update_time: now, }); - } + }; - let UpdatedState { - client_state, - consensus_state, - } = upgrade_client_state.verify_upgrade_and_update_state( + // Validate the upgraded client state and consensus state and verify proofs against the root + old_client_state.verify_upgrade_client( + msg.client_state.clone(), msg.consensus_state.clone(), msg.proof_upgrade_client.clone(), - msg.proof_upgrade_consensus_state, + msg.proof_upgrade_consensus_state.clone(), + old_consensus_state.root(), )?; - // Not implemented yet: https://github.com/informalsystems/ibc-rs/issues/722 - // todo!() - - let client_type = client_state.client_type(); - let consensus_height = client_state.latest_height(); + // Create updated new client state and consensus state + let UpdatedState { + client_state, + consensus_state, + } = old_client_state + .update_state_with_upgrade_client(msg.client_state.clone(), msg.consensus_state)?; let result = ClientResult::Upgrade(UpgradeClientResult { client_id: client_id.clone(), - client_state, + client_state: client_state.clone(), consensus_state, }); output.emit(IbcEvent::UpgradeClient(UpgradeClient::new( client_id, - client_type, - consensus_height, + client_state.client_type(), + client_state.latest_height(), ))); Ok(output.with_result(result)) } +#[cfg(feature = "upgrade_client")] #[cfg(test)] mod tests { use crate::events::IbcEvent; @@ -157,7 +211,6 @@ mod tests { use crate::core::ics02_client::msgs::upgrade_client::MsgUpgradeClient; use crate::core::ics02_client::msgs::ClientMsg; use crate::core::ics24_host::identifier::ClientId; - use crate::handler::HandlerOutput; use crate::mock::client_state::client_type as mock_client_type; use crate::mock::client_state::MockClientState; use crate::mock::consensus_state::MockConsensusState; @@ -167,135 +220,131 @@ mod tests { use crate::Height; #[test] - fn test_upgrade_client_ok() { + fn upgrade_client_msg_processing() { + struct Test { + name: String, + ctx: MockContext, + msg: MsgUpgradeClient, + want_pass: bool, + } let client_id = ClientId::default(); let signer = get_dummy_account_id(); - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); - - let msg = MsgUpgradeClient { - client_id: client_id.clone(), - client_state: MockClientState::new(MockHeader::new(Height::new(1, 26).unwrap())).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26).unwrap())) - .into(), - proof_upgrade_client: Default::default(), - proof_upgrade_consensus_state: Default::default(), - signer, - }; - - let output = dispatch(&ctx, ClientMsg::UpgradeClient(msg.clone())); - - match output { - Ok(HandlerOutput { - result, - events: _, - log, - }) => { - assert!(log.is_empty()); - // Check the result - match result { - Upgrade(upg_res) => { - assert_eq!(upg_res.client_id, client_id); - assert_eq!(upg_res.client_state.as_ref().clone_into(), msg.client_state) + let tests: Vec = vec![ + Test { + name: "Processing succeeds".to_string(), + ctx: ctx.clone(), + msg: MsgUpgradeClient { + client_id: client_id.clone(), + client_state: MockClientState::new(MockHeader::new( + Height::new(1, 26).unwrap(), + )) + .into(), + consensus_state: MockConsensusState::new(MockHeader::new( + Height::new(1, 26).unwrap(), + )) + .into(), + proof_upgrade_client: Default::default(), + proof_upgrade_consensus_state: Default::default(), + signer: signer.clone(), + }, + want_pass: true, + }, + Test { + name: "Processing fails for non existing client".to_string(), + ctx: ctx.clone(), + msg: MsgUpgradeClient { + client_id: ClientId::from_str("nonexistingclient").unwrap(), + client_state: MockClientState::new(MockHeader::new( + Height::new(1, 26).unwrap(), + )) + .into(), + consensus_state: MockConsensusState::new(MockHeader::new( + Height::new(1, 26).unwrap(), + )) + .into(), + proof_upgrade_client: Default::default(), + proof_upgrade_consensus_state: Default::default(), + signer: signer.clone(), + }, + want_pass: false, + }, + Test { + name: "Processing fails for low upgrade height".to_string(), + ctx, + msg: MsgUpgradeClient { + client_id: client_id.clone(), + client_state: MockClientState::new(MockHeader::new( + Height::new(0, 26).unwrap(), + )) + .into(), + consensus_state: MockConsensusState::new(MockHeader::new( + Height::new(0, 26).unwrap(), + )) + .into(), + proof_upgrade_client: Default::default(), + proof_upgrade_consensus_state: Default::default(), + signer, + }, + want_pass: false, + }, + ] + .into_iter() + .collect(); + + for test in tests { + let output = dispatch(&test.ctx, ClientMsg::UpgradeClient(test.msg.clone())); + let test_name = test.name; + match (test.want_pass, output) { + (true, Ok(output)) => { + let upgrade_client_event = + downcast!(output.events.first().unwrap() => IbcEvent::UpgradeClient) + .unwrap(); + assert_eq!(upgrade_client_event.client_id(), &client_id); + assert_eq!(upgrade_client_event.client_type(), &mock_client_type()); + assert_eq!( + upgrade_client_event.consensus_height(), + &Height::new(1, 26).unwrap() + ); + assert!(output.log.is_empty()); + match output.result { + Upgrade(upg_res) => { + assert_eq!(upg_res.client_id, client_id); + assert_eq!( + upg_res.client_state.as_ref().clone_into(), + test.msg.client_state + ); + assert_eq!( + upg_res.consensus_state.as_ref().clone_into(), + test.msg.consensus_state + ); + } + _ => panic!("upgrade handler result has incorrect type"), } - _ => panic!("upgrade handler result has incorrect type"), } - } - Err(err) => { - panic!("unexpected error: {}", err); - } - } - } - - #[test] - fn test_upgrade_nonexisting_client() { - let client_id = ClientId::from_str("mockclient1").unwrap(); - let signer = get_dummy_account_id(); - - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); - - let msg = MsgUpgradeClient { - client_id: ClientId::from_str("nonexistingclient").unwrap(), - client_state: MockClientState::new(MockHeader::new(Height::new(1, 26).unwrap())).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(1, 26).unwrap())) - .into(), - proof_upgrade_client: Default::default(), - proof_upgrade_consensus_state: Default::default(), - signer, - }; - - let output = dispatch(&ctx, ClientMsg::UpgradeClient(msg.clone())); - - match output { - Err(ClientError::ClientNotFound { client_id }) => { - assert_eq!(client_id, msg.client_id); - } - _ => { - panic!("expected ClientNotFound error, instead got {:?}", output); - } - } - } - - #[test] - fn test_upgrade_client_low_height() { - let client_id = ClientId::default(); - let signer = get_dummy_account_id(); - - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); - - let msg = MsgUpgradeClient { - client_id, - client_state: MockClientState::new(MockHeader::new(Height::new(0, 26).unwrap())).into(), - consensus_state: MockConsensusState::new(MockHeader::new(Height::new(0, 26).unwrap())) - .into(), - proof_upgrade_client: Default::default(), - proof_upgrade_consensus_state: Default::default(), - signer, - }; - - let output = dispatch(&ctx, ClientMsg::UpgradeClient(msg.clone())); - - match output { - Err(ClientError::LowUpgradeHeight { - upgraded_height, - client_height, - }) => { - assert_eq!(upgraded_height, Height::new(0, 42).unwrap()); - assert_eq!( - client_height, - MockClientState::try_from(msg.client_state) - .unwrap() - .latest_height() - ); - } - _ => { - panic!("expected LowUpgradeHeight error, instead got {:?}", output); + (true, Err(e)) => panic!("unexpected error for test {test_name}, {e:?}"), + (false, Err(e)) => match e { + ClientError::ClientNotFound { client_id } => { + assert_eq!(client_id, test.msg.client_id) + } + ClientError::LowUpgradeHeight { + upgraded_height, + client_height, + } => { + assert_eq!(upgraded_height, Height::new(0, 42).unwrap()); + assert_eq!( + client_height, + MockClientState::try_from(test.msg.client_state) + .unwrap() + .latest_height() + ); + } + _ => panic!("unexpected error for test {test_name}, {e:?}"), + }, + (false, Ok(e)) => { + panic!("unexpected success for test {test_name}, result: {e:?}") + } } } } - - #[test] - fn test_upgrade_client_event() { - let client_id = ClientId::default(); - let signer = get_dummy_account_id(); - - let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap()); - - let upgrade_height = Height::new(1, 26).unwrap(); - let msg = MsgUpgradeClient { - client_id: client_id.clone(), - client_state: MockClientState::new(MockHeader::new(upgrade_height)).into(), - consensus_state: MockConsensusState::new(MockHeader::new(upgrade_height)).into(), - proof_upgrade_client: Default::default(), - proof_upgrade_consensus_state: Default::default(), - signer, - }; - - let output = dispatch(&ctx, ClientMsg::UpgradeClient(msg)).unwrap(); - let upgrade_client_event = - downcast!(output.events.first().unwrap() => IbcEvent::UpgradeClient).unwrap(); - assert_eq!(upgrade_client_event.client_id(), &client_id); - assert_eq!(upgrade_client_event.client_type(), &mock_client_type()); - assert_eq!(upgrade_client_event.consensus_height(), &upgrade_height); - } } diff --git a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs index 754d220ae..22e129fa0 100644 --- a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs @@ -21,11 +21,18 @@ pub(crate) const TYPE_URL: &str = "/ibc.core.client.v1.MsgUpgradeClient"; /// A type of message that triggers the upgrade of an on-chain (IBC) client. #[derive(Clone, Debug, PartialEq)] pub struct MsgUpgradeClient { + // client unique identifier pub client_id: ClientId, + // Upgraded client state pub client_state: Any, + // Upgraded consensus state, only contains enough information + // to serve as a basis of trust in update logic pub consensus_state: Any, + // proof that old chain committed to new client pub proof_upgrade_client: RawMerkleProof, + // proof that old chain committed to new consensus state pub proof_upgrade_consensus_state: RawMerkleProof, + // signer address pub signer: Signer, } diff --git a/crates/ibc/src/core/ics26_routing/handler.rs b/crates/ibc/src/core/ics26_routing/handler.rs index 1f11e73fe..fbb7c9705 100644 --- a/crates/ibc/src/core/ics26_routing/handler.rs +++ b/crates/ibc/src/core/ics26_routing/handler.rs @@ -583,7 +583,9 @@ mod tests { default_signer.clone(), ))) .into(), - want_pass: true, + // Temporarily set to false due to the fact that the client + // upgrade is not yet implemented + want_pass: cfg!(feature = "upgrade_client"), state_check: None, }, Test { diff --git a/crates/ibc/src/mock/client_state.rs b/crates/ibc/src/mock/client_state.rs index 06e925175..9f3993758 100644 --- a/crates/ibc/src/mock/client_state.rs +++ b/crates/ibc/src/mock/client_state.rs @@ -2,14 +2,13 @@ use crate::prelude::*; use alloc::collections::btree_map::BTreeMap as HashMap; use core::time::Duration; -use dyn_clone::clone_box; use ibc_proto::ibc::core::commitment::v1::MerkleProof; use ibc_proto::google::protobuf::Any; use ibc_proto::ibc::mock::ClientState as RawMockClientState; use ibc_proto::protobuf::Protobuf; -use crate::core::ics02_client::client_state::{ClientState, UpdatedState, UpgradeOptions}; +use crate::core::ics02_client::client_state::{ClientState, UpdatedState}; use crate::core::ics02_client::client_type::ClientType; use crate::core::ics02_client::consensus_state::ConsensusState; use crate::core::ics02_client::context::ClientReader; @@ -167,12 +166,7 @@ impl ClientState for MockClientState { self.frozen_height } - fn upgrade( - &mut self, - _upgrade_height: Height, - _upgrade_options: &dyn UpgradeOptions, - _chain_id: ChainId, - ) { + fn zero_custom_fields(&mut self) { unimplemented!() } @@ -283,16 +277,35 @@ impl ClientState for MockClientState { Ok(new_state.into_box()) } - fn verify_upgrade_and_update_state( + fn verify_upgrade_client( &self, - consensus_state: Any, + upgraded_client_state: Any, + upgraded_consensus_state: Any, _proof_upgrade_client: MerkleProof, _proof_upgrade_consensus_state: MerkleProof, + _root: &CommitmentRoot, + ) -> Result<(), ClientError> { + let upgraded_mock_client_state = MockClientState::try_from(upgraded_client_state)?; + MockConsensusState::try_from(upgraded_consensus_state)?; + if self.latest_height() >= upgraded_mock_client_state.latest_height() { + return Err(ClientError::LowUpgradeHeight { + upgraded_height: self.latest_height(), + client_height: upgraded_mock_client_state.latest_height(), + }); + } + Ok(()) + } + + fn update_state_with_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, ) -> Result { - let consensus_state = MockConsensusState::try_from(consensus_state)?; + let mock_client_state = MockClientState::try_from(upgraded_client_state)?; + let mock_consensus_state = MockConsensusState::try_from(upgraded_consensus_state)?; Ok(UpdatedState { - client_state: clone_box(self), - consensus_state: consensus_state.into_box(), + client_state: mock_client_state.into_box(), + consensus_state: mock_consensus_state.into_box(), }) } diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index 90df47b14..d90884622 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -97,8 +97,9 @@ supported by `IBC-rs`: 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. + break some counterparty clients (Consider the case where the + `UnbondingPeriod` falls below the `TrustingPeriod`). 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) @@ -170,12 +171,12 @@ validations (SV) and lastly execution (E) steps as follows: 2. (BV) Check if the latest consensus state is within the trust period 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 + 5. (SV) Match any Tendermint chain specified parameter in upgraded client + such as ChainID, UnbondingPeriod, and ProofSpecs with the committed client + 6. (SV) Verify that the upgradedConsensusState be of a Tendermint `ConsensusState` type - 6. (SV) Check the height of the committed client state is not greater than + 7. (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 that the client-customizable fields must be zeroed out for this check @@ -289,11 +290,14 @@ previous section as mentioned: let upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?; ``` -5. ```rust +5. This check has been done as part of step 4 while creating an instance of + `UpgradedClientState` in the domain type. + +6. ```rust let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; ``` -6. ```rust +7. ```rust if self.latest_height() >= upgraded_tm_client_state.latest_height() { return Err(ClientError::LowUpgradeHeight { upgraded_height: self.latest_height(), @@ -302,25 +306,6 @@ previous section as mentioned: } ``` -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();