From 040fb7167faae8f74e604543cd397cc385f5645b Mon Sep 17 00:00:00 2001 From: yito88 Date: Mon, 5 Aug 2024 15:45:42 +0200 Subject: [PATCH 01/12] fix upgrade_path prefix --- .../ics07-tendermint/src/client_state/common.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index c4726871e..35f1a2765 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -189,13 +189,14 @@ pub fn verify_upgrade_client( // Check to see if the upgrade path is set let mut upgrade_path = client_state.upgrade_path.clone(); - if upgrade_path.pop().is_none() { - return Err(ClientError::ClientSpecific { + let upgrade_path = upgrade_path + .pop() + .ok_or_else(|| ClientError::ClientSpecific { description: "cannot upgrade client as no upgrade path has been set".to_string(), - }); - }; + })?; - let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path[0].clone().into_bytes()); + let upgrade_path_prefix = CommitmentPrefix::try_from(upgrade_path.into_bytes()) + .map_err(ClientError::InvalidCommitmentProof)?; // Verify the proof of the upgraded client state verify_membership::( From 1dfa27223c3a91058d6bc442a31ae659148cb10f Mon Sep 17 00:00:00 2001 From: yito88 Date: Mon, 5 Aug 2024 20:48:17 +0200 Subject: [PATCH 02/12] fix conversion --- ibc-clients/ics07-tendermint/src/client_state/common.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index 35f1a2765..a4009f260 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -195,8 +195,7 @@ pub fn verify_upgrade_client( description: "cannot upgrade client as no upgrade path has been set".to_string(), })?; - let upgrade_path_prefix = CommitmentPrefix::try_from(upgrade_path.into_bytes()) - .map_err(ClientError::InvalidCommitmentProof)?; + let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path.into_bytes()); // Verify the proof of the upgraded client state verify_membership::( From a2fa682ee59278d3a399df05ac476d487562a9a7 Mon Sep 17 00:00:00 2001 From: yito88 Date: Mon, 5 Aug 2024 20:51:31 +0200 Subject: [PATCH 03/12] add more check --- ibc-clients/ics07-tendermint/src/client_state/common.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index a4009f260..d1ac4401c 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -196,6 +196,11 @@ pub fn verify_upgrade_client( })?; let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path.into_bytes()); + if upgrade_path_prefix.is_empty() { + return Err(ClientError::InvalidCommitmentProof( + CommitmentError::EmptyCommitmentPrefix, + )); + } // Verify the proof of the upgraded client state verify_membership::( From 30fa65406bd35fa5db0c047e16357ff260d51a60 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 7 Aug 2024 00:31:04 +0200 Subject: [PATCH 04/12] user-defined upgrade path --- .../src/client_state/common.rs | 63 +++++----- .../cosmos/src/upgrade_proposal/context.rs | 10 +- .../cosmos/src/upgrade_proposal/handler.rs | 7 +- ibc-core/ics24-host/types/src/path.rs | 113 +++++++++++++----- ibc-query/src/core/client/query.rs | 33 +++-- ibc-query/src/core/client/types/request.rs | 6 + 6 files changed, 160 insertions(+), 72 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index d1ac4401c..ee6253924 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -11,7 +11,9 @@ use ibc_core_commitment_types::merkle::{MerklePath, MerkleProof}; use ibc_core_commitment_types::proto::ics23::{HostFunctionsManager, HostFunctionsProvider}; use ibc_core_commitment_types::specs::ProofSpecs; use ibc_core_host::types::identifiers::ClientType; -use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath}; +use ibc_core_host::types::path::{ + Path, PathBytes, UpgradeClientStatePath, UpgradeConsensusStatePath, UPGRADED_IBC_STATE, +}; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; use ibc_primitives::ToVec; @@ -44,24 +46,13 @@ impl ClientStateCommon for ClientState { proof_upgrade_consensus_state: CommitmentProofBytes, root: &CommitmentRoot, ) -> Result<(), ClientError> { - let last_height = self.latest_height().revision_height(); - - let upgrade_client_path_bytes = self.serialize_path(Path::UpgradeClient( - UpgradeClientPath::UpgradedClientState(last_height), - ))?; - - let upgrade_consensus_path_bytes = self.serialize_path(Path::UpgradeClient( - UpgradeClientPath::UpgradedClientConsensusState(last_height), - ))?; - verify_upgrade_client::( self.inner(), upgraded_client_state, upgraded_consensus_state, proof_upgrade_client, proof_upgrade_consensus_state, - upgrade_client_path_bytes, - upgrade_consensus_path_bytes, + self.latest_height(), root, ) } @@ -163,8 +154,7 @@ pub fn verify_upgrade_client( upgraded_consensus_state: Any, proof_upgrade_client: CommitmentProofBytes, proof_upgrade_consensus_state: CommitmentProofBytes, - upgrade_client_path_bytes: PathBytes, - upgrade_consensus_path_bytes: PathBytes, + last_height: Height, root: &CommitmentRoot, ) -> Result<(), ClientError> { // Make sure that the client type is of Tendermint type `ClientState` @@ -187,20 +177,37 @@ pub fn verify_upgrade_client( } // Check to see if the upgrade path is set - let mut upgrade_path = client_state.upgrade_path.clone(); - - let upgrade_path = upgrade_path - .pop() - .ok_or_else(|| ClientError::ClientSpecific { - description: "cannot upgrade client as no upgrade path has been set".to_string(), - })?; + let upgrade_path = &client_state.upgrade_path; + let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() { + 0 => (CommitmentPrefix::empty(), UPGRADED_IBC_STATE), + 1 => (CommitmentPrefix::empty(), upgrade_path[0].as_ref()), + 2 => ( + upgrade_path[0].as_bytes().to_vec().into(), + upgrade_path[1].as_ref(), + ), + _ => { + return Err(ClientError::ClientSpecific { + description: "cannot upgrade client as too long upgrade path has been set" + .to_string(), + }) + } + }; - let upgrade_path_prefix = CommitmentPrefix::from(upgrade_path.into_bytes()); - if upgrade_path_prefix.is_empty() { - return Err(ClientError::InvalidCommitmentProof( - CommitmentError::EmptyCommitmentPrefix, - )); - } + let upgrade_client_path_bytes = Path::UpgradeClientState(UpgradeClientStatePath { + upgrade_path: upgrade_path.to_string(), + height: last_height.revision_height(), + }) + .to_string() + .into_bytes() + .into(); + + let upgrade_consensus_path_bytes = Path::UpgradeConsensusState(UpgradeConsensusStatePath { + upgrade_path: upgrade_path.to_string(), + height: last_height.revision_height(), + }) + .to_string() + .into_bytes() + .into(); // Verify the proof of the upgraded client state verify_membership::( diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs index 056bd4e46..1328355e8 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs +++ b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/context.rs @@ -7,7 +7,7 @@ use ibc_core_client_context::ClientValidationContext; use ibc_core_client_types::error::UpgradeClientError; -use ibc_core_host_types::path::UpgradeClientPath; +use ibc_core_host_types::path::{UpgradeClientStatePath, UpgradeConsensusStatePath}; use super::Plan; @@ -28,13 +28,13 @@ pub trait UpgradeValidationContext { /// Returns the upgraded client state at the specified upgrade path. fn upgraded_client_state( &self, - upgrade_path: &UpgradeClientPath, + upgrade_path: &UpgradeClientStatePath, ) -> Result, UpgradeClientError>; /// Returns the upgraded consensus state at the specified upgrade path. fn upgraded_consensus_state( &self, - upgrade_path: &UpgradeClientPath, + upgrade_path: &UpgradeConsensusStatePath, ) -> Result, UpgradeClientError>; } @@ -50,14 +50,14 @@ pub trait UpgradeExecutionContext: UpgradeValidationContext { /// Stores the upgraded client state at the specified upgrade path. fn store_upgraded_client_state( &mut self, - upgrade_path: UpgradeClientPath, + upgrade_path: UpgradeClientStatePath, client_state: UpgradedClientStateRef, ) -> Result<(), UpgradeClientError>; /// Stores the upgraded consensus state at the specified upgrade path. fn store_upgraded_consensus_state( &mut self, - upgrade_path: UpgradeClientPath, + upgrade_path: UpgradeConsensusStatePath, consensus_state: UpgradedConsensusStateRef, ) -> Result<(), UpgradeClientError>; } diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs index 8c7ee72cf..542863faf 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs +++ b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs @@ -1,6 +1,6 @@ use ibc_client_tendermint::types::ClientState as TmClientState; use ibc_core_client_types::error::UpgradeClientError; -use ibc_core_host_types::path::UpgradeClientPath; +use ibc_core_host_types::path::{UpgradeClientStatePath, UPGRADED_IBC_STATE}; use ibc_primitives::prelude::*; use tendermint::abci::Event as TmEvent; @@ -37,7 +37,10 @@ where ctx.schedule_upgrade(plan.clone())?; - let upgraded_client_state_path = UpgradeClientPath::UpgradedClientState(plan.height); + let upgraded_client_state_path = UpgradeClientStatePath { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height: plan.height, + }; ctx.store_upgraded_client_state(upgraded_client_state_path, client_state.into())?; diff --git a/ibc-core/ics24-host/types/src/path.rs b/ibc-core/ics24-host/types/src/path.rs index bd6b120de..dd6a9c02f 100644 --- a/ibc-core/ics24-host/types/src/path.rs +++ b/ibc-core/ics24-host/types/src/path.rs @@ -99,7 +99,8 @@ pub enum Path { Commitment(CommitmentPath), Ack(AckPath), Receipt(ReceiptPath), - UpgradeClient(UpgradeClientPath), + UpgradeClientState(UpgradeClientStatePath), + UpgradeConsensusState(UpgradeConsensusStatePath), } #[cfg_attr( @@ -693,13 +694,31 @@ impl ReceiptPath { derive(borsh::BorshSerialize, borsh::BorshDeserialize) )] #[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 UpgradeClientPath { - #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_STATE}")] - UpgradedClientState(u64), - #[display(fmt = "{UPGRADED_IBC_STATE}/{_0}/{UPGRADED_CLIENT_CONSENSUS_STATE}")] - UpgradedClientConsensusState(u64), +#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_STATE}")] +pub struct UpgradeClientStatePath { + pub upgrade_path: String, + pub height: u64, +} + +#[cfg_attr( + feature = "parity-scale-codec", + derive( + parity_scale_codec::Encode, + parity_scale_codec::Decode, + scale_info::TypeInfo + ) +)] +#[cfg_attr( + feature = "borsh", + derive(borsh::BorshSerialize, borsh::BorshDeserialize) +)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Display)] +#[display(fmt = "{upgrade_path}/{height}/{UPGRADED_CLIENT_CONSENSUS_STATE}")] +pub struct UpgradeConsensusStatePath { + pub upgrade_path: String, + pub height: u64, } #[cfg_attr( @@ -760,7 +779,8 @@ impl FromStr for Path { .or_else(|| parse_commitments(&components)) .or_else(|| parse_acks(&components)) .or_else(|| parse_receipts(&components)) - .or_else(|| parse_upgrades(&components)) + .or_else(|| parse_upgrade_client_state(&components)) + .or_else(|| parse_upgrade_consensus_state(&components)) .ok_or(PathError::ParseFailure { path: s.to_string(), }) @@ -1084,28 +1104,52 @@ fn parse_receipts(components: &[&str]) -> Option { ) } -fn parse_upgrades(components: &[&str]) -> Option { +fn parse_upgrade_client_state(components: &[&str]) -> Option { if components.len() != 3 { return None; } - let first = *components.first()?; + let last = *components.last()?; - if first != UPGRADED_IBC_STATE { + if last != UPGRADED_CLIENT_STATE { return None; } - let last = *components.last()?; + let upgrade_path = components.first()?.to_string(); - let height = components[1].parse::().ok()?; + let height = u64::from_str(components[1]).ok()?; - match last { - UPGRADED_CLIENT_STATE => Some(UpgradeClientPath::UpgradedClientState(height).into()), - UPGRADED_CLIENT_CONSENSUS_STATE => { - Some(UpgradeClientPath::UpgradedClientConsensusState(height).into()) + Some( + UpgradeClientStatePath { + upgrade_path, + height, } - _ => None, + .into(), + ) +} + +fn parse_upgrade_consensus_state(components: &[&str]) -> Option { + if components.len() != 3 { + return None; + } + + let last = *components.last()?; + + if last != UPGRADED_CLIENT_CONSENSUS_STATE { + return None; } + + let upgrade_path = components.first()?.to_string(); + + let height = u64::from_str(components[1]).ok()?; + + Some( + UpgradeConsensusStatePath { + upgrade_path, + height, + } + .into(), + ) } #[cfg(test)] @@ -1207,11 +1251,17 @@ mod tests { )] #[case( "upgradedIBCState/0/upgradedClient", - Path::UpgradeClient(UpgradeClientPath::UpgradedClientState(0)) + Path::UpgradeClientState(UpgradeClientStatePath { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height: 0, + }) )] #[case( "upgradedIBCState/0/upgradedConsState", - Path::UpgradeClient(UpgradeClientPath::UpgradedClientConsensusState(0)) + Path::UpgradeConsensusState(UpgradeConsensusStatePath { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height: 0, + }) )] fn test_successful_parsing(#[case] path_str: &str, #[case] path: Path) { // can be parsed into Path @@ -1419,25 +1469,30 @@ mod tests { } #[test] - fn test_parse_upgrades_fn() { + fn test_parse_upgrade_client_state_fn() { let path = "upgradedIBCState/0/upgradedClient"; let components: Vec<&str> = path.split('/').collect(); assert_eq!( - parse_upgrades(&components), - Some(Path::UpgradeClient(UpgradeClientPath::UpgradedClientState( - 0 - ))), + parse_upgrade_client_state(&components), + Some(Path::UpgradeClientState(UpgradeClientStatePath { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height: 0, + })), ); + } + #[test] + fn test_parse_upgrade_consensus_state_fn() { let path = "upgradedIBCState/0/upgradedConsState"; let components: Vec<&str> = path.split('/').collect(); assert_eq!( - parse_upgrades(&components), - Some(Path::UpgradeClient( - UpgradeClientPath::UpgradedClientConsensusState(0) - )), + parse_upgrade_consensus_state(&components), + Some(Path::UpgradeConsensusState(UpgradeConsensusStatePath { + upgrade_path: UPGRADED_CLIENT_STATE.to_string(), + height: 0, + })), ) } } diff --git a/ibc-query/src/core/client/query.rs b/ibc-query/src/core/client/query.rs index 076aa8329..27e64a33f 100644 --- a/ibc-query/src/core/client/query.rs +++ b/ibc-query/src/core/client/query.rs @@ -4,11 +4,12 @@ use ibc::core::client::context::client_state::ClientStateValidation; use ibc::core::client::context::ClientValidationContext; use ibc::core::client::types::error::ClientError; use ibc::core::host::types::path::{ - ClientConsensusStatePath, ClientStatePath, Path, UpgradeClientPath, + ClientConsensusStatePath, ClientStatePath, Path, UpgradeClientStatePath, + UpgradeConsensusStatePath, UPGRADED_IBC_STATE, }; use ibc::core::host::{ConsensusStateRef, ValidationContext}; use ibc::cosmos_host::upgrade_proposal::{UpgradeValidationContext, UpgradedConsensusStateRef}; -use ibc::primitives::prelude::format; +use ibc::primitives::prelude::{format, ToString}; use ibc::primitives::proto::Any; use super::{ @@ -205,6 +206,12 @@ where I: ValidationContext, U: UpgradeValidationContext + ProvableContext, { + let upgrade_path = match &request.upgrade_path { + Some(path) if !path.is_empty() => path, + _ => UPGRADED_IBC_STATE, + } + .to_string(); + let upgrade_revision_height = match request.upgrade_height { Some(height) => height.revision_height(), None => { @@ -215,8 +222,10 @@ where } }; - let upgraded_client_state_path = - UpgradeClientPath::UpgradedClientState(upgrade_revision_height); + let upgraded_client_state_path = UpgradeClientStatePath { + upgrade_path, + height: upgrade_revision_height, + }; let upgraded_client_state = upgrade_ctx .upgraded_client_state(&upgraded_client_state_path) @@ -230,7 +239,7 @@ where let proof = upgrade_ctx .get_proof( proof_height, - &Path::UpgradeClient(upgraded_client_state_path), + &Path::UpgradeClientState(upgraded_client_state_path), ) .ok_or_else(|| { QueryError::proof_not_found(format!( @@ -256,6 +265,12 @@ where U: UpgradeValidationContext + ProvableContext, UpgradedConsensusStateRef: Into, { + let upgrade_path = match &request.upgrade_path { + Some(path) if !path.is_empty() => path, + _ => UPGRADED_IBC_STATE, + } + .to_string(); + let upgrade_revision_height = match request.upgrade_height { Some(height) => height.revision_height(), None => { @@ -266,8 +281,10 @@ where } }; - let upgraded_consensus_state_path = - UpgradeClientPath::UpgradedClientConsensusState(upgrade_revision_height); + let upgraded_consensus_state_path = UpgradeConsensusStatePath { + upgrade_path, + height: upgrade_revision_height, + }; let upgraded_consensus_state = upgrade_ctx .upgraded_consensus_state(&upgraded_consensus_state_path) @@ -281,7 +298,7 @@ where let proof = upgrade_ctx .get_proof( proof_height, - &Path::UpgradeClient(upgraded_consensus_state_path), + &Path::UpgradeConsensusState(upgraded_consensus_state_path), ) .ok_or_else(|| { QueryError::proof_not_found(format!( diff --git a/ibc-query/src/core/client/types/request.rs b/ibc-query/src/core/client/types/request.rs index 1cc5e4769..8648c69bc 100644 --- a/ibc-query/src/core/client/types/request.rs +++ b/ibc-query/src/core/client/types/request.rs @@ -198,6 +198,8 @@ impl From for QueryClientParamsRequest { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct QueryUpgradedClientStateRequest { + /// The upgrade path + pub upgrade_path: Option, /// Height at which the chain is scheduled to halt for upgrade pub upgrade_height: Option, /// The height at which to query the upgraded client state. If not provided, @@ -208,6 +210,7 @@ pub struct QueryUpgradedClientStateRequest { impl From for QueryUpgradedClientStateRequest { fn from(_request: RawUpgradedClientStateRequest) -> Self { Self { + upgrade_path: None, upgrade_height: None, query_height: None, } @@ -220,6 +223,8 @@ impl From for QueryUpgradedClientStateRequest { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] pub struct QueryUpgradedConsensusStateRequest { + /// The upgrade path + pub upgrade_path: Option, /// The height at which the chain is scheduled to halt for upgrade. pub upgrade_height: Option, /// The height at which to query the upgraded consensus state. If not @@ -230,6 +235,7 @@ pub struct QueryUpgradedConsensusStateRequest { impl From for QueryUpgradedConsensusStateRequest { fn from(_request: RawUpgradedConsensusStateRequest) -> Self { Self { + upgrade_path: None, upgrade_height: None, query_height: None, } From ba6814bd17120b2bf94966a92fdd9ebf974fc1b1 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 7 Aug 2024 11:03:18 +0200 Subject: [PATCH 05/12] fix a test --- ibc-core/ics24-host/types/src/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics24-host/types/src/path.rs b/ibc-core/ics24-host/types/src/path.rs index dd6a9c02f..f019d49a7 100644 --- a/ibc-core/ics24-host/types/src/path.rs +++ b/ibc-core/ics24-host/types/src/path.rs @@ -1490,7 +1490,7 @@ mod tests { assert_eq!( parse_upgrade_consensus_state(&components), Some(Path::UpgradeConsensusState(UpgradeConsensusStatePath { - upgrade_path: UPGRADED_CLIENT_STATE.to_string(), + upgrade_path: UPGRADED_IBC_STATE.to_string(), height: 0, })), ) From 4fdf7729e7657a57e8d44092b1f094d5c2230a24 Mon Sep 17 00:00:00 2001 From: Yuji Ito Date: Wed, 7 Aug 2024 21:58:45 +0200 Subject: [PATCH 06/12] Update ibc-clients/ics07-tendermint/src/client_state/common.rs Co-authored-by: Sean Chen Signed-off-by: Yuji Ito --- ibc-clients/ics07-tendermint/src/client_state/common.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index ee6253924..86a306f8f 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -176,7 +176,12 @@ pub fn verify_upgrade_client( })? } - // Check to see if the upgrade path is set + // The client state's upgrade path vector needs to parsed into a tuple in the form + // of `(upgrade_path_prefix, upgrade_path)`. Given the length of the client + // state's upgrade path vector, the following determinations are made: + // 0: The tuple defaults to an empty commitment prefix and `UPGRADED_IBC_STATE` as the path. + // 1: The commitment prefix is left empty and the upgrade path is used as-is. + // 2: The commitment prefix and upgrade path are both taken as-is. let upgrade_path = &client_state.upgrade_path; let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() { 0 => (CommitmentPrefix::empty(), UPGRADED_IBC_STATE), From eeade2a252f6f0ab7e759c3d9040413926bca033 Mon Sep 17 00:00:00 2001 From: Yuji Ito Date: Wed, 7 Aug 2024 21:58:50 +0200 Subject: [PATCH 07/12] Update ibc-clients/ics07-tendermint/src/client_state/common.rs Co-authored-by: Sean Chen Signed-off-by: Yuji Ito --- ibc-clients/ics07-tendermint/src/client_state/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index 86a306f8f..85e5ff551 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -192,7 +192,7 @@ pub fn verify_upgrade_client( ), _ => { return Err(ClientError::ClientSpecific { - description: "cannot upgrade client as too long upgrade path has been set" + description: "upgrade client failed: upgrade path is too long" .to_string(), }) } From 0fb6f93a2b96e58b82c631af0f38a336342fef82 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 7 Aug 2024 22:10:12 +0200 Subject: [PATCH 08/12] fmt --- ibc-clients/ics07-tendermint/src/client_state/common.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index 85e5ff551..8f577216c 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -192,8 +192,7 @@ pub fn verify_upgrade_client( ), _ => { return Err(ClientError::ClientSpecific { - description: "upgrade client failed: upgrade path is too long" - .to_string(), + description: "upgrade client failed: upgrade path is too long".to_string(), }) } }; From 283f23a898ac0c12c12b59cb5fb1f1f125bdb382 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 8 Aug 2024 22:14:25 +0200 Subject: [PATCH 09/12] disallow empty upgrade_path --- .../src/client_state/common.rs | 90 ++++++++++--------- .../cosmos/src/upgrade_proposal/handler.rs | 5 +- ibc-core/ics24-host/types/src/path.rs | 20 +++++ 3 files changed, 69 insertions(+), 46 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index 8f577216c..3ddbb9d38 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -12,7 +12,7 @@ use ibc_core_commitment_types::proto::ics23::{HostFunctionsManager, HostFunction use ibc_core_commitment_types::specs::ProofSpecs; use ibc_core_host::types::identifiers::ClientType; use ibc_core_host::types::path::{ - Path, PathBytes, UpgradeClientStatePath, UpgradeConsensusStatePath, UPGRADED_IBC_STATE, + Path, PathBytes, UpgradeClientStatePath, UpgradeConsensusStatePath, }; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; @@ -46,13 +46,54 @@ impl ClientStateCommon for ClientState { proof_upgrade_consensus_state: CommitmentProofBytes, root: &CommitmentRoot, ) -> Result<(), ClientError> { + let last_height = self.latest_height().revision_height(); + + // The client state's upgrade path vector needs to parsed into a tuple in the form + // of `(upgrade_path_prefix, upgrade_path)`. Given the length of the client + // state's upgrade path vector, the following determinations are made: + // 1: The commitment prefix is left empty and the upgrade path is used as-is. + // 2: The commitment prefix and upgrade path are both taken as-is. + let upgrade_path = &self.inner().upgrade_path; + let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() { + 0 => { + return Err(ClientError::ClientSpecific { + description: "cannot upgrade client as no upgrade path has been set" + .to_string(), + }); + } + 1 => (CommitmentPrefix::empty(), upgrade_path[0].clone()), + 2 => ( + upgrade_path[0].as_bytes().to_vec().into(), + upgrade_path[1].clone(), + ), + _ => { + return Err(ClientError::ClientSpecific { + description: "upgrade client failed: upgrade path is too long".to_string(), + }) + } + }; + + let upgrade_client_path_bytes = + self.serialize_path(Path::UpgradeClientState(UpgradeClientStatePath { + upgrade_path: upgrade_path.clone(), + height: last_height, + }))?; + + let upgrade_consensus_path_bytes = + self.serialize_path(Path::UpgradeConsensusState(UpgradeConsensusStatePath { + upgrade_path, + height: last_height, + }))?; + verify_upgrade_client::( self.inner(), upgraded_client_state, upgraded_consensus_state, proof_upgrade_client, proof_upgrade_consensus_state, - self.latest_height(), + upgrade_path_prefix, + upgrade_client_path_bytes, + upgrade_consensus_path_bytes, root, ) } @@ -154,7 +195,9 @@ pub fn verify_upgrade_client( upgraded_consensus_state: Any, proof_upgrade_client: CommitmentProofBytes, proof_upgrade_consensus_state: CommitmentProofBytes, - last_height: Height, + upgrade_path_prefix: CommitmentPrefix, + upgrade_client_path_bytes: PathBytes, + upgrade_consensus_path_bytes: PathBytes, root: &CommitmentRoot, ) -> Result<(), ClientError> { // Make sure that the client type is of Tendermint type `ClientState` @@ -171,48 +214,11 @@ pub fn verify_upgrade_client( // the height if latest_height >= upgraded_tm_client_state_height { Err(UpgradeClientError::LowUpgradeHeight { - upgraded_height: latest_height, - client_height: upgraded_tm_client_state_height, + upgraded_height: upgraded_tm_client_state_height, + client_height: latest_height, })? } - // The client state's upgrade path vector needs to parsed into a tuple in the form - // of `(upgrade_path_prefix, upgrade_path)`. Given the length of the client - // state's upgrade path vector, the following determinations are made: - // 0: The tuple defaults to an empty commitment prefix and `UPGRADED_IBC_STATE` as the path. - // 1: The commitment prefix is left empty and the upgrade path is used as-is. - // 2: The commitment prefix and upgrade path are both taken as-is. - let upgrade_path = &client_state.upgrade_path; - let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() { - 0 => (CommitmentPrefix::empty(), UPGRADED_IBC_STATE), - 1 => (CommitmentPrefix::empty(), upgrade_path[0].as_ref()), - 2 => ( - upgrade_path[0].as_bytes().to_vec().into(), - upgrade_path[1].as_ref(), - ), - _ => { - return Err(ClientError::ClientSpecific { - description: "upgrade client failed: upgrade path is too long".to_string(), - }) - } - }; - - let upgrade_client_path_bytes = Path::UpgradeClientState(UpgradeClientStatePath { - upgrade_path: upgrade_path.to_string(), - height: last_height.revision_height(), - }) - .to_string() - .into_bytes() - .into(); - - let upgrade_consensus_path_bytes = Path::UpgradeConsensusState(UpgradeConsensusStatePath { - upgrade_path: upgrade_path.to_string(), - height: last_height.revision_height(), - }) - .to_string() - .into_bytes() - .into(); - // Verify the proof of the upgraded client state verify_membership::( &client_state.proof_specs, diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs index 542863faf..c7cea4cd1 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs +++ b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs @@ -37,10 +37,7 @@ where ctx.schedule_upgrade(plan.clone())?; - let upgraded_client_state_path = UpgradeClientStatePath { - upgrade_path: UPGRADED_IBC_STATE.to_string(), - height: plan.height, - }; + let upgraded_client_state_path = UpgradeClientStatePath::new_with_default_path(plan.height); ctx.store_upgraded_client_state(upgraded_client_state_path, client_state.into())?; diff --git a/ibc-core/ics24-host/types/src/path.rs b/ibc-core/ics24-host/types/src/path.rs index f019d49a7..61ce31813 100644 --- a/ibc-core/ics24-host/types/src/path.rs +++ b/ibc-core/ics24-host/types/src/path.rs @@ -701,6 +701,16 @@ pub struct UpgradeClientStatePath { pub height: u64, } +impl UpgradeClientStatePath { + /// Create with the default upgrade path + pub fn new_with_default_path(height: u64) -> Self { + Self { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height, + } + } +} + #[cfg_attr( feature = "parity-scale-codec", derive( @@ -721,6 +731,16 @@ pub struct UpgradeConsensusStatePath { pub height: u64, } +impl UpgradeConsensusStatePath { + /// Create with the default upgrade path + pub fn new_with_default_path(height: u64) -> Self { + Self { + upgrade_path: UPGRADED_IBC_STATE.to_string(), + height, + } + } +} + #[cfg_attr( feature = "parity-scale-codec", derive( From 45fe2e5e0d39a11edcf02bf58b02eb40e73e21a9 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 8 Aug 2024 22:17:44 +0200 Subject: [PATCH 10/12] remove unused import --- ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs index c7cea4cd1..3f27bad12 100644 --- a/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs +++ b/ibc-core/ics24-host/cosmos/src/upgrade_proposal/handler.rs @@ -1,6 +1,6 @@ use ibc_client_tendermint::types::ClientState as TmClientState; use ibc_core_client_types::error::UpgradeClientError; -use ibc_core_host_types::path::{UpgradeClientStatePath, UPGRADED_IBC_STATE}; +use ibc_core_host_types::path::UpgradeClientStatePath; use ibc_primitives::prelude::*; use tendermint::abci::Event as TmEvent; From cc4bff4625c164cd3d70ceccc9cb889c7f3dd69a Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 8 Aug 2024 22:46:44 +0200 Subject: [PATCH 11/12] add InvalidUpgradePath --- .../ics07-tendermint/src/client_state/common.rs | 15 ++++++++------- ibc-core/ics02-client/types/src/error.rs | 2 ++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index 3ddbb9d38..b0af93e68 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -56,10 +56,10 @@ impl ClientStateCommon for ClientState { let upgrade_path = &self.inner().upgrade_path; let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() { 0 => { - return Err(ClientError::ClientSpecific { - description: "cannot upgrade client as no upgrade path has been set" - .to_string(), - }); + return Err(UpgradeClientError::InvalidUpgradePath { + reason: "no upgrade path has been set".to_string(), + } + .into()); } 1 => (CommitmentPrefix::empty(), upgrade_path[0].clone()), 2 => ( @@ -67,9 +67,10 @@ impl ClientStateCommon for ClientState { upgrade_path[1].clone(), ), _ => { - return Err(ClientError::ClientSpecific { - description: "upgrade client failed: upgrade path is too long".to_string(), - }) + return Err(UpgradeClientError::InvalidUpgradePath { + reason: "upgrade path is too long".to_string(), + } + .into()); } }; diff --git a/ibc-core/ics02-client/types/src/error.rs b/ibc-core/ics02-client/types/src/error.rs index 14e86274e..c5ea46242 100644 --- a/ibc-core/ics02-client/types/src/error.rs +++ b/ibc-core/ics02-client/types/src/error.rs @@ -151,6 +151,8 @@ pub enum UpgradeClientError { upgraded_height: Height, client_height: Height, }, + /// Invalid upgrade path: `{reason}` + InvalidUpgradePath { reason: String }, /// invalid upgrade proposal: `{reason}` InvalidUpgradeProposal { reason: String }, /// invalid upgrade plan: `{reason}` From 1dd9be8c1cdc773e6b5b0b3609f3390a9a69eb9b Mon Sep 17 00:00:00 2001 From: yito88 Date: Fri, 9 Aug 2024 10:25:09 +0200 Subject: [PATCH 12/12] add changelogs --- .changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md | 3 +++ .changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md | 3 +++ 2 files changed, 6 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md create mode 100644 .changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md diff --git a/.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md b/.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md new file mode 100644 index 000000000..7bfaeedb1 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1297-fix-upgrade-path-check.md @@ -0,0 +1,3 @@ +- [ibc-client-tendermint] Fix client verification panic on upgrades when the + `upgrade_path` size is 1. + ([\#1297](https://github.com/cosmos/ibc-rs/issues/1297)) diff --git a/.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md b/.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md new file mode 100644 index 000000000..324daf793 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1303-fix-upgrade-path-check.md @@ -0,0 +1,3 @@ +- [ibc-client-tendermint] Use the user-defined upgrade path for client upgrades, +instead of defaulting to `UPGRADED_IBC_STATE`. + ([\#1303](https://github.com/cosmos/ibc-rs/issues/1303))