From af477d4e44d699a123f3d299354826232b0cef06 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 11 May 2023 12:11:28 -0700 Subject: [PATCH] Fix upgrade-client issues and refactor relevant tests (#673) * Fix upgrade-client bugs and refactor relevant tests * Update ADR06 * Add APIs for storing/retrieving upgraded client/cons states * Add unclog * Remove upgrade APIs to be added in a later PR --- ...ix-upgraded-client-cons-states-encoding.md | 2 + .../385-refactor-upgrade-client-tests.md | 2 + .../671-client-state-conversion-refinement.md | 2 + crates/ibc/Cargo.toml | 3 - .../clients/ics07_tendermint/client_state.rs | 163 ++++++++----- .../ics07_tendermint/consensus_state.rs | 5 - crates/ibc/src/core/handler.rs | 33 +-- crates/ibc/src/core/ics02_client/error.rs | 2 + .../ics02_client/handler/upgrade_client.rs | 218 ++++++++++-------- .../core/ics02_client/msgs/upgrade_client.rs | 92 ++------ .../src/core/ics23_commitment/commitment.rs | 4 + crates/ibc/src/core/ics24_host/path.rs | 20 +- crates/ibc/src/hosts/tendermint.rs | 3 + crates/ibc/src/mock/context.rs | 4 +- .../adr-006-upgrade-client-implementation.md | 48 ++-- 15 files changed, 318 insertions(+), 283 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/672-fix-upgraded-client-cons-states-encoding.md create mode 100644 .changelog/unreleased/improvements/385-refactor-upgrade-client-tests.md create mode 100644 .changelog/unreleased/improvements/671-client-state-conversion-refinement.md diff --git a/.changelog/unreleased/bug-fixes/672-fix-upgraded-client-cons-states-encoding.md b/.changelog/unreleased/bug-fixes/672-fix-upgraded-client-cons-states-encoding.md new file mode 100644 index 000000000..e1d536a1b --- /dev/null +++ b/.changelog/unreleased/bug-fixes/672-fix-upgraded-client-cons-states-encoding.md @@ -0,0 +1,2 @@ +- Encode upgraded client/consensus states for upgrade_client validation using `prost::Message` + from pros ([#672](https://github.com/cosmos/ibc-rs/issues/672)) diff --git a/.changelog/unreleased/improvements/385-refactor-upgrade-client-tests.md b/.changelog/unreleased/improvements/385-refactor-upgrade-client-tests.md new file mode 100644 index 000000000..adc8dcf0f --- /dev/null +++ b/.changelog/unreleased/improvements/385-refactor-upgrade-client-tests.md @@ -0,0 +1,2 @@ +- Refactor tests for upgrade_client implementation + ([#385](https://github.com/cosmos/ibc-rs/issues/385)) diff --git a/.changelog/unreleased/improvements/671-client-state-conversion-refinement.md b/.changelog/unreleased/improvements/671-client-state-conversion-refinement.md new file mode 100644 index 000000000..ca6da1643 --- /dev/null +++ b/.changelog/unreleased/improvements/671-client-state-conversion-refinement.md @@ -0,0 +1,2 @@ +- Exclude `ClientState::new()` checks from proto ClientState conversion + ([#671](https://github.com/cosmos/ibc-rs/issues/671)) diff --git a/crates/ibc/Cargo.toml b/crates/ibc/Cargo.toml index b0c37f65a..a2b1efd3d 100644 --- a/crates/ibc/Cargo.toml +++ b/crates/ibc/Cargo.toml @@ -42,9 +42,6 @@ borsh = ["dep:borsh"] # This feature is required for token transfer (ICS-20) serde = ["dep:serde", "dep:serde_derive", "serde_json", "erased-serde"] -# 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 63d62dba1..704681159 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -13,11 +13,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::{MerklePath, MerkleProof as RawMerkleProof}; -use ibc_proto::ibc::lightclients::tendermint::v1::{ - ClientState as RawTmClientState, ConsensusState as RawTmConsensusState, -}; +use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawTmClientState; use ibc_proto::protobuf::Protobuf; use prost::Message; + use tendermint::chain::id::MAX_LENGTH as MaxChainIdLen; use tendermint::trust_threshold::TrustThresholdFraction as TendermintTrustThresholdFraction; use tendermint_light_client_verifier::options::Options; @@ -41,13 +40,12 @@ use crate::core::ics23_commitment::merkle::{apply_prefix, MerkleProof}; use crate::core::ics23_commitment::specs::ProofSpecs; use crate::core::ics24_host::identifier::{ChainId, ClientId}; use crate::core::ics24_host::path::Path; -use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, ClientUpgradePath}; +use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, UpgradeClientPath}; use crate::core::timestamp::ZERO_DURATION; use crate::Height; use super::client_type as tm_client_type; use super::trust_threshold::TrustThreshold; - use crate::core::{ExecutionContext, ValidationContext}; const TENDERMINT_CLIENT_STATE_TYPE_URL: &str = "/ibc.lightclients.tendermint.v1.ClientState"; @@ -78,6 +76,33 @@ pub struct AllowUpdate { } impl ClientState { + #[allow(clippy::too_many_arguments)] + fn new_without_validation( + chain_id: ChainId, + trust_level: TrustThreshold, + trusting_period: Duration, + unbonding_period: Duration, + max_clock_drift: Duration, + latest_height: Height, + proof_specs: ProofSpecs, + upgrade_path: Vec, + allow_update: AllowUpdate, + ) -> ClientState { + Self { + chain_id, + trust_level, + trusting_period, + unbonding_period, + max_clock_drift, + latest_height, + proof_specs, + upgrade_path, + allow_update, + frozen_height: None, + verifier: ProdVerifier::default(), + } + } + #[allow(clippy::too_many_arguments)] pub fn new( chain_id: ChainId, @@ -90,60 +115,92 @@ impl ClientState { upgrade_path: Vec, allow_update: AllowUpdate, ) -> Result { - if chain_id.as_str().len() > MaxChainIdLen { + let client_state = Self::new_without_validation( + chain_id, + trust_level, + trusting_period, + unbonding_period, + max_clock_drift, + latest_height, + proof_specs, + upgrade_path, + allow_update, + ); + client_state.validate()?; + Ok(client_state) + } + + pub fn with_header(self, header: TmHeader) -> Result { + Ok(ClientState { + latest_height: max(header.height(), self.latest_height), + ..self + }) + } + + pub fn with_frozen_height(self, h: Height) -> Self { + Self { + frozen_height: Some(h), + ..self + } + } + + pub fn validate(&self) -> Result<(), Error> { + if self.chain_id.as_str().len() > MaxChainIdLen { return Err(Error::ChainIdTooLong { - chain_id: chain_id.clone(), - len: chain_id.as_str().len(), + chain_id: self.chain_id.clone(), + len: self.chain_id.as_str().len(), max_len: MaxChainIdLen, }); } // `TrustThreshold` is guaranteed to be in the range `[0, 1)`, but a `TrustThreshold::ZERO` // value is invalid in this context - if trust_level == TrustThreshold::ZERO { + if self.trust_level == TrustThreshold::ZERO { return Err(Error::InvalidTrustThreshold { reason: "ClientState trust-level cannot be zero".to_string(), }); } - let _ = TendermintTrustThresholdFraction::new( - trust_level.numerator(), - trust_level.denominator(), + TendermintTrustThresholdFraction::new( + self.trust_level.numerator(), + self.trust_level.denominator(), ) .map_err(Error::InvalidTendermintTrustThreshold)?; // Basic validation of trusting period and unbonding period: each should be non-zero. - if trusting_period <= Duration::new(0, 0) { + if self.trusting_period <= Duration::new(0, 0) { return Err(Error::InvalidTrustThreshold { reason: format!( - "ClientState trusting period ({trusting_period:?}) must be greater than zero" + "ClientState trusting period ({:?}) must be greater than zero", + self.trusting_period ), }); } - if unbonding_period <= Duration::new(0, 0) { + if self.unbonding_period <= Duration::new(0, 0) { return Err(Error::InvalidTrustThreshold { reason: format!( - "ClientState unbonding period ({unbonding_period:?}) must be greater than zero" + "ClientState unbonding period ({:?}) must be greater than zero", + self.unbonding_period ), }); } - if trusting_period >= unbonding_period { + if self.trusting_period >= self.unbonding_period { return Err(Error::InvalidTrustThreshold { reason: format!( - "ClientState trusting period ({trusting_period:?}) must be smaller than unbonding period ({unbonding_period:?})" + "ClientState trusting period ({:?}) must be smaller than unbonding period ({:?})", self.trusting_period, self.unbonding_period ), }); } - if max_clock_drift <= Duration::new(0, 0) { + if self.max_clock_drift <= Duration::new(0, 0) { return Err(Error::InvalidMaxClockDrift { reason: "ClientState max-clock-drift must be greater than zero".to_string(), }); } - if latest_height.revision_number() != chain_id.version() { + if self.latest_height.revision_number() != self.chain_id.version() { return Err(Error::InvalidLatestHeight { reason: "ClientState latest-height revision number must match chain-id version" .to_string(), @@ -151,14 +208,14 @@ impl ClientState { } // Disallow empty proof-specs - if proof_specs.is_empty() { + if self.proof_specs.is_empty() { return Err(Error::Validation { reason: "ClientState proof-specs cannot be empty".to_string(), }); } // `upgrade_path` itself may be empty, but if not then each key must be non-empty - for (idx, key) in upgrade_path.iter().enumerate() { + for (idx, key) in self.upgrade_path.iter().enumerate() { if key.trim().is_empty() { return Err(Error::Validation { reason: format!( @@ -168,33 +225,7 @@ impl ClientState { } } - Ok(Self { - chain_id, - trust_level, - trusting_period, - unbonding_period, - max_clock_drift, - latest_height, - proof_specs, - upgrade_path, - allow_update, - frozen_height: None, - verifier: ProdVerifier::default(), - }) - } - - pub fn with_header(self, header: TmHeader) -> Result { - Ok(ClientState { - latest_height: max(header.height(), self.latest_height), - ..self - }) - } - - pub fn with_frozen_height(self, h: Height) -> Self { - Self { - frozen_height: Some(h), - ..self - } + Ok(()) } /// Get the refresh time to ensure the state does not expire @@ -249,8 +280,8 @@ impl Ics2ClientState for ClientState { Ok(()) } + // Resets custom fields to zero values (used in `update_client`) fn zero_custom_fields(&mut self) { - // Reset custom fields to zero values self.trusting_period = ZERO_DURATION; self.trust_level = TrustThreshold::ZERO; self.allow_update.after_expiry = false; @@ -264,7 +295,13 @@ impl Ics2ClientState for ClientState { } fn initialise(&self, consensus_state: Any) -> Result, ClientError> { - TmConsensusState::try_from(consensus_state).map(TmConsensusState::into_box) + let tm_consensus_state = TmConsensusState::try_from(consensus_state)?; + if tm_consensus_state.root().is_empty() { + return Err(ClientError::Other { + description: "empty commitment root".into(), + }); + }; + Ok(Box::new(tm_consensus_state)) } fn verify_client_message( @@ -386,10 +423,10 @@ impl Ics2ClientState for ClientState { 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)?; + let mut upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state.clone())?; // Make sure that the consensus type is of Tendermint type `ConsensusState` - let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?; + TmConsensusState::try_from(upgraded_consensus_state.clone())?; // Note: verification of proofs that unmarshalled correctly has been done // while decoding the proto message into a `MsgEnvelope` domain type @@ -418,15 +455,18 @@ impl Ics2ClientState for ClientState { // 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()); + client_upgrade_path.push(UpgradeClientPath::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); + + let mut client_state_value = Vec::new(); + upgraded_client_state + .encode(&mut client_state_value) + .map_err(ClientError::Encode)?; // Verify the proof of the upgraded client state merkle_proof_upgrade_client @@ -442,12 +482,15 @@ impl Ics2ClientState for ClientState { // 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()); + .push(UpgradeClientPath::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); + let mut cons_state_value = Vec::new(); + upgraded_consensus_state + .encode(&mut cons_state_value) + .map_err(ClientError::Encode)?; // Verify the proof of the upgraded consensus state merkle_proof_upgrade_cons_state @@ -657,7 +700,7 @@ impl TryFrom for ClientState { after_misbehaviour: raw.allow_update_after_misbehaviour, }; - let client_state = ClientState::new( + let client_state = ClientState::new_without_validation( chain_id, trust_level, trusting_period, @@ -667,7 +710,7 @@ impl TryFrom for ClientState { raw.proof_specs.into(), raw.upgrade_path, allow_update, - )?; + ); Ok(client_state) } diff --git a/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs b/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs index 62c56b6b0..20384923c 100644 --- a/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/consensus_state.rs @@ -57,11 +57,6 @@ impl TryFrom for ConsensusState { reason: "missing commitment root".into(), })? .hash; - if proto_root.is_empty() { - return Err(Error::InvalidRawClientState { - reason: "empty commitment root".into(), - }); - }; let ibc_proto::google::protobuf::Timestamp { seconds, nanos } = raw.timestamp.ok_or(Error::InvalidRawClientState { diff --git a/crates/ibc/src/core/handler.rs b/crates/ibc/src/core/handler.rs index 768ba2df1..e5094b3c6 100644 --- a/crates/ibc/src/core/handler.rs +++ b/crates/ibc/src/core/handler.rs @@ -185,6 +185,7 @@ mod tests { msgs::transfer::test_util::get_dummy_msg_transfer, msgs::transfer::MsgTransfer, packet::PacketData, PrefixedCoin, MODULE_ID_STR, }; + use crate::core::dispatch; use crate::core::events::{IbcEvent, MessageEvent}; use crate::core::ics02_client::msgs::{ create_client::MsgCreateClient, update_client::MsgUpdateClient, @@ -221,14 +222,12 @@ mod tests { }; use crate::core::ics04_channel::timeout::TimeoutHeight; use crate::core::ics04_channel::Version as ChannelVersion; - use crate::core::ics23_commitment::commitment::test_util::get_dummy_merkle_proof; use crate::core::ics23_commitment::commitment::CommitmentPrefix; use crate::core::ics24_host::identifier::{ChannelId, ClientId, ConnectionId, PortId}; use crate::core::ics24_host::path::CommitmentPath; use crate::core::msgs::MsgEnvelope; use crate::core::router::ModuleId; use crate::core::timestamp::Timestamp; - use crate::core::{dispatch, ValidationContext}; use crate::mock::client_state::MockClientState; use crate::mock::consensus_state::MockConsensusState; use crate::mock::context::MockContext; @@ -538,7 +537,7 @@ mod tests { msg: MsgEnvelope::Client(ClientMsg::UpdateClient(MsgUpdateClient { client_id: client_id.clone(), header: MockHeader::new(update_client_height_after_second_send).into(), - signer: default_signer.clone(), + signer: default_signer, })) .into(), want_pass: true, @@ -579,30 +578,20 @@ mod tests { }, Test { name: "Client upgrade successful".to_string(), - msg: MsgEnvelope::Client(ClientMsg::UpgradeClient(MsgUpgradeClient::new( - client_id.clone(), - MockClientState::new(MockHeader::new(upgrade_client_height)).into(), - MockConsensusState::new(MockHeader::new(upgrade_client_height)).into(), - get_dummy_merkle_proof(), - get_dummy_merkle_proof(), - default_signer.clone(), - ))) + msg: MsgEnvelope::Client(ClientMsg::UpgradeClient( + MsgUpgradeClient::new_dummy(upgrade_client_height) + .with_client_id(client_id.clone()), + )) .into(), - // Temporarily set to false due to the fact that the client - // upgrade is not yet implemented - want_pass: cfg!(feature = "upgrade_client"), + want_pass: true, state_check: None, }, Test { name: "Client upgrade un-successful".to_string(), - msg: MsgEnvelope::Client(ClientMsg::UpgradeClient(MsgUpgradeClient::new( - client_id, - MockClientState::new(MockHeader::new(upgrade_client_height_second)).into(), - MockConsensusState::new(MockHeader::new(upgrade_client_height_second)).into(), - get_dummy_merkle_proof(), - get_dummy_merkle_proof(), - default_signer, - ))) + msg: MsgEnvelope::Client(ClientMsg::UpgradeClient( + MsgUpgradeClient::new_dummy(upgrade_client_height_second) + .with_client_id(client_id), + )) .into(), want_pass: false, state_check: None, diff --git a/crates/ibc/src/core/ics02_client/error.rs b/crates/ibc/src/core/ics02_client/error.rs index 08e30bf46..5ad6299ab 100644 --- a/crates/ibc/src/core/ics02_client/error.rs +++ b/crates/ibc/src/core/ics02_client/error.rs @@ -53,6 +53,8 @@ pub enum ClientError { MissingRawConsensusState, /// invalid client id in the update client message: `{0}` InvalidMsgUpdateClientId(IdentifierError), + /// encode error: `{0}` + Encode(prost::EncodeError), /// 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 f54ca5bd3..3708f0042 100644 --- a/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/upgrade_client.rs @@ -2,16 +2,13 @@ //! use crate::prelude::*; +use crate::core::context::ContextError; use crate::core::events::{IbcEvent, MessageEvent}; use crate::core::ics02_client::client_state::UpdatedState; use crate::core::ics02_client::error::ClientError; use crate::core::ics02_client::events::UpgradeClient; use crate::core::ics02_client::msgs::upgrade_client::MsgUpgradeClient; - -use crate::core::context::ContextError; - use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath}; - use crate::core::{ExecutionContext, ValidationContext}; pub(crate) fn validate(ctx: &Ctx, msg: MsgUpgradeClient) -> Result<(), ContextError> @@ -24,13 +21,6 @@ where ctx.validate_message_signer(&signer)?; - // Temporary has been disabled until we have a better understanding of some design implications - if cfg!(not(feature = "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)?; @@ -108,115 +98,155 @@ where Ok(()) } -#[cfg(feature = "upgrade_client")] #[cfg(test)] mod tests { use super::*; - use crate::core::events::IbcEvent; - use crate::core::ics02_client::handler::upgrade_client::execute; - use crate::core::ics24_host::path::ClientConsensusStatePath; - use crate::core::ValidationContext; - use crate::downcast; - use rstest::*; - use core::str::FromStr; + use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState; + use crate::clients::ics07_tendermint::client_type; + use crate::clients::ics07_tendermint::header::test_util::get_dummy_tendermint_header; - use crate::core::ics02_client::msgs::upgrade_client::MsgUpgradeClient; + use crate::core::ics03_connection::handler::test_util::{Expect, Fixture}; use crate::core::ics24_host::identifier::ClientId; + use crate::downcast; + use crate::Height; + use crate::mock::client_state::client_type as mock_client_type; - use crate::mock::client_state::MockClientState; - use crate::mock::consensus_state::MockConsensusState; use crate::mock::context::MockContext; - use crate::mock::header::MockHeader; - use crate::test_utils::get_dummy_account_id; - use crate::Height; - use super::validate; + enum Ctx { + Default, + WithClient, + } - pub struct Fixture { - pub ctx: MockContext, - pub msg: MsgUpgradeClient, + enum Msg { + Default, + LowUpgradeHeight, + UnknownUpgradedClientStateType, } - #[fixture] - fn fixture() -> Fixture { - let ctx = - MockContext::default().with_client(&ClientId::default(), Height::new(0, 42).unwrap()); + fn msg_upgrade_client_fixture(ctx_variant: Ctx, msg_variant: Msg) -> Fixture { + let client_id = ClientId::new(mock_client_type(), 0).unwrap(); - let msg = MsgUpgradeClient { - client_id: ClientId::default(), - 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: get_dummy_account_id(), + let ctx_default = MockContext::default(); + let ctx_with_client = ctx_default + .clone() + .with_client(&client_id, Height::new(0, 42).unwrap()); + let ctx = match ctx_variant { + Ctx::Default => ctx_default, + Ctx::WithClient => ctx_with_client, }; - Fixture { ctx, msg } - } + let upgrade_height = Height::new(1, 26).unwrap(); + let msg_default = MsgUpgradeClient::new_dummy(upgrade_height); - #[rstest] - fn upgrade_client_ok(fixture: Fixture) { - let Fixture { mut ctx, msg } = fixture; + let low_upgrade_height = Height::new(0, 26).unwrap(); + let msg_with_low_upgrade_height = MsgUpgradeClient::new_dummy(low_upgrade_height); - let res = validate(&ctx, msg.clone()); - assert!(res.is_ok(), "validation happy path"); + let msg_with_unknown_upgraded_cs = MsgUpgradeClient { + client_state: TmClientState::new_dummy_from_header(get_dummy_tendermint_header()) + .into(), + ..msg_default.clone() + }; - let res = execute(&mut ctx, msg.clone()); - assert!(res.is_ok(), "execution happy path"); + let msg = match msg_variant { + Msg::Default => msg_default, + Msg::LowUpgradeHeight => msg_with_low_upgrade_height, + Msg::UnknownUpgradedClientStateType => msg_with_unknown_upgraded_cs, + }; - assert!(matches!( - ctx.events[0], - IbcEvent::Message(MessageEvent::Client) - )); - let upgrade_client_event = downcast!(&ctx.events[1] => IbcEvent::UpgradeClient).unwrap(); - assert_eq!(upgrade_client_event.client_id(), &msg.client_id); - assert_eq!(upgrade_client_event.client_type(), &mock_client_type()); - assert_eq!( - upgrade_client_event.consensus_height(), - &Height::new(1, 26).unwrap() - ); - - let client_state = ctx.client_state(&msg.client_id).unwrap(); - assert_eq!(client_state.as_ref().clone_into(), msg.client_state); - - let consensus_state = ctx - .consensus_state(&ClientConsensusStatePath { - client_id: msg.client_id, - epoch: 1, - height: 26, - }) - .unwrap(); - assert_eq!(consensus_state.as_ref().clone_into(), msg.consensus_state); + Fixture { ctx, msg } } - #[rstest] - fn upgrade_client_fail_nonexisting_client(fixture: Fixture) { - let Fixture { ctx, mut msg } = fixture; + fn upgrade_client_validate(fxt: &Fixture, expect: Expect) { + let Fixture { ctx, msg } = fxt; + let res = validate(ctx, msg.clone()); + let err_msg = fxt.generate_error_msg(&expect, "validation", &res); + + match expect { + Expect::Failure(_) => { + assert!(res.is_err(), "{err_msg}"); + } + Expect::Success => { + assert!(res.is_ok(), "{err_msg}"); + } + }; + } - msg.client_id = ClientId::from_str("nonexistingclient").unwrap(); + fn upgrade_client_execute(fxt: &mut Fixture, expect: Expect) { + let res = execute(&mut fxt.ctx, fxt.msg.clone()); + let err_msg = fxt.generate_error_msg(&expect, "execution", &res); + match expect { + Expect::Failure(err) => { + assert!(res.is_err(), "{err_msg}"); + assert_eq!( + core::mem::discriminant(res.as_ref().unwrap_err()), + core::mem::discriminant(&err.unwrap()) + ); + } + Expect::Success => { + assert!(res.is_ok(), "{err_msg}"); + assert!(matches!( + fxt.ctx.events[0], + IbcEvent::Message(MessageEvent::Client) + )); + let upgrade_client_event = + downcast!(&fxt.ctx.events[1] => IbcEvent::UpgradeClient).unwrap(); + let plan_height = Height::new(1, 26).unwrap(); + + assert_eq!(upgrade_client_event.client_id(), &fxt.msg.client_id); + assert_eq!(upgrade_client_event.client_type(), &mock_client_type()); + assert_eq!(upgrade_client_event.consensus_height(), &plan_height); + + let client_state = fxt.ctx.client_state(&fxt.msg.client_id).unwrap(); + assert_eq!(client_state.as_ref().clone_into(), fxt.msg.client_state); + let consensus_state = fxt + .ctx + .consensus_state(&ClientConsensusStatePath::new( + &fxt.msg.client_id, + &plan_height, + )) + .unwrap(); + assert_eq!( + consensus_state.as_ref().clone_into(), + fxt.msg.consensus_state + ); + } + }; + } - let res = validate(&ctx, msg); - assert!( - res.is_err(), - "validation fails because the client is non-existing" - ); + #[test] + fn msg_upgrade_client_healthy() { + let mut fxt = msg_upgrade_client_fixture(Ctx::WithClient, Msg::Default); + upgrade_client_validate(&fxt, Expect::Success); + upgrade_client_execute(&mut fxt, Expect::Success); } - #[rstest] - fn upgrade_client_fail_low_upgrade_height(fixture: Fixture) { - let Fixture { ctx, mut msg } = fixture; + #[test] + fn upgrade_client_fail_nonexisting_client() { + let fxt = msg_upgrade_client_fixture(Ctx::Default, Msg::Default); + let expected_err = ContextError::ClientError(ClientError::ClientStateNotFound { + client_id: fxt.msg.client_id.clone(), + }); + upgrade_client_validate(&fxt, Expect::Failure(Some(expected_err))); + } - msg.client_state = - MockClientState::new(MockHeader::new(Height::new(0, 26).unwrap())).into(); - msg.consensus_state = - MockConsensusState::new(MockHeader::new(Height::new(0, 26).unwrap())).into(); + #[test] + fn upgrade_client_fail_low_upgrade_height() { + let fxt = msg_upgrade_client_fixture(Ctx::WithClient, Msg::LowUpgradeHeight); + let expected_err = ContextError::ClientError(ClientError::LowUpgradeHeight { + upgraded_height: Height::new(0, 26).unwrap(), + client_height: fxt.ctx.latest_height(), + }); + upgrade_client_validate(&fxt, Expect::Failure(Some(expected_err))); + } - let res = validate(&ctx, msg); - assert!( - res.is_err(), - "validation fails because the upgrade height is too low" - ); + #[test] + fn upgrade_client_fail_unknown_upgraded_client_state() { + let fxt = msg_upgrade_client_fixture(Ctx::WithClient, Msg::UnknownUpgradedClientStateType); + let expected_err = ContextError::ClientError(ClientError::UnknownClientStateType { + client_state_type: client_type().to_string(), + }); + upgrade_client_validate(&fxt, Expect::Failure(Some(expected_err))); } } 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 a9463dbd6..b4bada646 100644 --- a/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs +++ b/crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs @@ -101,53 +101,40 @@ impl TryFrom for MsgUpgradeClient { #[cfg(test)] pub mod test_util { - use ibc_proto::google::protobuf::Any; - use ibc_proto::ibc::core::client::v1::MsgUpgradeClient as RawMsgUpgradeClient; - use ibc_proto::ibc::core::commitment::v1::MerkleProof as RawMerkleProof; - - use crate::{ - core::{ics02_client::height::Height, ics24_host::identifier::ClientId}, - mock::{ - client_state::MockClientState, consensus_state::MockConsensusState, header::MockHeader, - }, - test_utils::{get_dummy_bech32_account, get_dummy_proof}, - }; + use super::*; - use super::MsgUpgradeClient; - use crate::signer::Signer; + use crate::core::ics23_commitment::commitment::test_util::get_dummy_merkle_proof; + use crate::core::{ics02_client::height::Height, ics24_host::identifier::ClientId}; + use crate::mock::client_state::client_type as mock_client_type; + use crate::mock::{ + client_state::MockClientState, consensus_state::MockConsensusState, header::MockHeader, + }; + use crate::test_utils::{get_dummy_account_id, get_dummy_bech32_account, get_dummy_proof}; /// Extends the implementation with additional helper methods. impl MsgUpgradeClient { - pub fn new( - client_id: ClientId, - client_state: Any, - consensus_state: Any, - proof_upgrade_client: RawMerkleProof, - proof_upgrade_consensus_state: RawMerkleProof, - signer: Signer, - ) -> Self { + pub fn new_dummy(upgrade_height: Height) -> Self { MsgUpgradeClient { - client_id, - client_state, - consensus_state, - proof_upgrade_client, - proof_upgrade_consensus_state, - signer, + client_id: ClientId::new(mock_client_type(), 0).unwrap(), + client_state: MockClientState::new(MockHeader::new(upgrade_height)).into(), + consensus_state: MockConsensusState::new(MockHeader::new(upgrade_height)).into(), + proof_upgrade_client: get_dummy_merkle_proof(), + proof_upgrade_consensus_state: get_dummy_merkle_proof(), + signer: get_dummy_account_id(), } } - // /// Setter for `client_id`. Amenable to chaining, since it consumes the input message. - // pub fn with_client_id(self, client_id: ClientId) -> Self { - // MsgUpgradeClient { client_id, ..self } - // } + pub fn with_client_id(self, client_id: ClientId) -> Self { + MsgUpgradeClient { client_id, ..self } + } } /// Returns a dummy `RawMsgUpgradeClient`, for testing only! - pub fn get_dummy_raw_msg_upgrade_client(height: Height) -> RawMsgUpgradeClient { + pub fn get_dummy_raw_msg_upgrade_client(upgrade_height: Height) -> RawMsgUpgradeClient { RawMsgUpgradeClient { - client_id: "tendermint".parse().unwrap(), - client_state: Some(MockClientState::new(MockHeader::new(height)).into()), - consensus_state: Some(MockConsensusState::new(MockHeader::new(height)).into()), + client_id: mock_client_type().to_string(), + client_state: Some(MockClientState::new(MockHeader::new(upgrade_height)).into()), + consensus_state: Some(MockConsensusState::new(MockHeader::new(upgrade_height)).into()), proof_upgrade_client: get_dummy_proof(), proof_upgrade_consensus_state: get_dummy_proof(), signer: get_dummy_bech32_account(), @@ -157,41 +144,12 @@ pub mod test_util { #[cfg(test)] mod tests { - - use ibc_proto::ibc::core::client::v1::MsgUpgradeClient as RawMsgUpgradeClient; - - use crate::{ - core::{ - ics02_client::{height::Height, msgs::upgrade_client::MsgUpgradeClient}, - ics23_commitment::commitment::test_util::get_dummy_merkle_proof, - ics24_host::identifier::ClientId, - }, - mock::{ - client_state::MockClientState, consensus_state::MockConsensusState, header::MockHeader, - }, - test_utils::get_dummy_account_id, - }; + use super::*; + use crate::core::ics02_client::height::Height; #[test] fn msg_upgrade_client_serialization() { - let client_id: ClientId = "tendermint".parse().unwrap(); - let signer = get_dummy_account_id(); - - let height = Height::new(1, 1).unwrap(); - - let client_state = MockClientState::new(MockHeader::new(height)); - let consensus_state = MockConsensusState::new(MockHeader::new(height)); - - let proof = get_dummy_merkle_proof(); - - let msg = MsgUpgradeClient::new( - client_id, - client_state.into(), - consensus_state.into(), - proof.clone(), - proof, - signer, - ); + let msg = MsgUpgradeClient::new_dummy(Height::new(1, 1).unwrap()); let raw: RawMsgUpgradeClient = RawMsgUpgradeClient::from(msg.clone()); let msg_back = MsgUpgradeClient::try_from(raw.clone()).unwrap(); let raw_back: RawMsgUpgradeClient = RawMsgUpgradeClient::from(msg_back.clone()); diff --git a/crates/ibc/src/core/ics23_commitment/commitment.rs b/crates/ibc/src/core/ics23_commitment/commitment.rs index c7f772507..e3ea05b88 100644 --- a/crates/ibc/src/core/ics23_commitment/commitment.rs +++ b/crates/ibc/src/core/ics23_commitment/commitment.rs @@ -42,6 +42,10 @@ impl CommitmentRoot { pub fn into_vec(self) -> Vec { self.bytes } + + pub fn is_empty(&self) -> bool { + self.bytes.is_empty() + } } impl From> for CommitmentRoot { diff --git a/crates/ibc/src/core/ics24_host/path.rs b/crates/ibc/src/core/ics24_host/path.rs index ee6d1b34b..24dc1bb48 100644 --- a/crates/ibc/src/core/ics24_host/path.rs +++ b/crates/ibc/src/core/ics24_host/path.rs @@ -36,7 +36,7 @@ pub enum Path { Commitment(CommitmentPath), Ack(AckPath), Receipt(ReceiptPath), - Upgrade(ClientUpgradePath), + UpgradeClient(UpgradeClientPath), } #[cfg_attr( @@ -356,7 +356,7 @@ impl ReceiptPath { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] /// Paths that are specific for client upgrades. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)] -pub enum ClientUpgradePath { +pub enum UpgradeClientPath { #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_STATE}")] UpgradedClientState(u64), #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_CONSENSUS_STATE}")] @@ -824,9 +824,9 @@ fn parse_upgrades(components: &[&str]) -> Option { }; match last { - UPGRADED_CLIENT_STATE => Some(ClientUpgradePath::UpgradedClientState(height).into()), + UPGRADED_CLIENT_STATE => Some(UpgradeClientPath::UpgradedClientState(height).into()), UPGRADED_CLIENT_CONSENSUS_STATE => { - Some(ClientUpgradePath::UpgradedClientConsensusState(height).into()) + Some(UpgradeClientPath::UpgradedClientConsensusState(height).into()) } _ => None, } @@ -1186,7 +1186,9 @@ mod tests { assert_eq!( parse_upgrades(&components), - Some(Path::Upgrade(ClientUpgradePath::UpgradedClientState(0))), + Some(Path::UpgradeClient(UpgradeClientPath::UpgradedClientState( + 0 + ))), ); let path = "upgradedIBCState/0/upgradedConsState"; @@ -1194,8 +1196,8 @@ mod tests { assert_eq!( parse_upgrades(&components), - Some(Path::Upgrade( - ClientUpgradePath::UpgradedClientConsensusState(0) + Some(Path::UpgradeClient( + UpgradeClientPath::UpgradedClientConsensusState(0) )), ) } @@ -1208,7 +1210,7 @@ mod tests { assert!(path.is_ok()); assert_eq!( path.unwrap(), - Path::Upgrade(ClientUpgradePath::UpgradedClientState(0)), + Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(0)), ); } @@ -1220,7 +1222,7 @@ mod tests { assert!(path.is_ok()); assert_eq!( path.unwrap(), - Path::Upgrade(ClientUpgradePath::UpgradedClientConsensusState(0)), + Path::UpgradeClient(UpgradeClientPath::UpgradedClientConsensusState(0)), ); } } diff --git a/crates/ibc/src/hosts/tendermint.rs b/crates/ibc/src/hosts/tendermint.rs index 22263fa6e..72c1fd684 100644 --- a/crates/ibc/src/hosts/tendermint.rs +++ b/crates/ibc/src/hosts/tendermint.rs @@ -6,6 +6,7 @@ use ibc_proto::google::protobuf::Any; use crate::clients::ics07_tendermint::client_state::ClientState as TmClientState; use crate::core::ics02_client::client_state::ClientState; +use crate::core::ics02_client::error::ClientError; use crate::core::ics03_connection::error::ConnectionError; use crate::core::ics23_commitment::specs::ProofSpecs; use crate::core::ics24_host::identifier::ChainId; @@ -30,6 +31,8 @@ pub trait ValidateSelfClientContext { }) .map_err(ContextError::ConnectionError)?; + tm_client_state.validate().map_err(ClientError::from)?; + tm_client_state.confirm_not_frozen()?; let self_chain_id = self.chain_id(); diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 9f423dc42..b60e9dbc8 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -590,8 +590,7 @@ impl MockContext { ) } - #[inline] - fn latest_height(&self) -> Height { + pub fn latest_height(&self) -> Height { self.history .last() .expect("history cannot be empty") @@ -731,6 +730,7 @@ impl ValidationContext for MockContext { fn decode_client_state(&self, client_state: Any) -> Result, ContextError> { if let Ok(client_state) = TmClientState::try_from(client_state.clone()) { + client_state.validate().map_err(ClientError::from)?; Ok(client_state.into_box()) } else if let Ok(client_state) = MockClientState::try_from(client_state.clone()) { Ok(client_state.into_box()) diff --git a/docs/architecture/adr-006-upgrade-client-implementation.md b/docs/architecture/adr-006-upgrade-client-implementation.md index d90884622..05cf58f6f 100644 --- a/docs/architecture/adr-006-upgrade-client-implementation.md +++ b/docs/architecture/adr-006-upgrade-client-implementation.md @@ -3,33 +3,35 @@ ## Changelog * 2023-01-25 Initial Proposal +* 2023-01-30 [Accepted](https://github.com/cosmos/ibc-rs/pull/383) +* 2023-05-10 Finalize Implementation ## Context -The ability to upgrade is a crucial feature for IBC-connected chains, as it +The ability to upgrade is a crucial feature for IBC-connected chains, which 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 chain-wide logic behind upgrading light clients, list requisites -for validation and execution steps, determine the boundary between basic and +be affected by some upgrades, but some may require the relevant client be +upgraded as well to keep high-value connections to other chains secure. For +having this capability, chains that implement `IBC-rs` may bring various +concerns and characteristics than Tendermint-based 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 implementation. This record aims 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 -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. +In this section, we first introduce the rules that are mainly derived from the +IBC protocol, and the review of IBC-go implementation. Next, we will provide a +detailed outline of the upgrade client process for Tendermint chains. Finally, +we will explain how we have implemented this rationale in IBC-rs. ### Chain-wide Upgrade Rules -#### Chain supports IBC upgrades? +#### Chain supports IBC client upgrades? * IBC currently **ONLY** supports planned upgrades that are committed to in advance by the upgrading chain, in order for counterparty clients to maintain @@ -74,7 +76,7 @@ and illustrate an example of client-specific upgrade. ### Upgrade Tendermint Clients -This section is based on the `IBC-Go` procedure for client upgrades with a few +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 @@ -318,9 +320,11 @@ previous section as mentioned: }; upgraded_tm_client_state.zero_custom_fields(); - let client_state_value = - Protobuf::::encode_vec(&upgraded_tm_client_state) - .map_err(ClientError::Encode)?; + + let mut client_state_value = Vec::new(); + upgraded_client_state + .encode(&mut client_state_value) + .map_err(ClientError::Encode)?; // Verify the proof of the upgraded client state merkle_proof_upgrade_client @@ -345,7 +349,9 @@ previous section as mentioned: key_path: cons_upgrade_path, }; - let cons_state_value = Protobuf::::encode_vec(&upgraded_tm_cons_state) + let mut cons_state_value = Vec::new(); + upgraded_consensus_state + .encode(&mut cons_state_value) .map_err(ClientError::Encode)?; // Verify the proof of the upgraded consensus state @@ -390,14 +396,14 @@ previous section as mentioned: ## Status -Proposed +Accepted ## Consequences ### Positive * Resolve the issue of unimplemented upgrade client message in `IBC-rs` -* Keep tendermint upgrade client implementation close to the `IBC-Go` +* Keep tendermint upgrade client implementation close to the `IBC-go` ### Negative