From 1f68e84d297293eeea6a170df1d91880a5b9e3de Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Jul 2024 07:34:04 -0700 Subject: [PATCH] imp: 2nd try, place serialize_path under the ClientStateCommon --- ibc-clients/cw-context/src/handlers.rs | 11 +- .../src/client_state/common.rs | 113 ++++++++++++++- .../src/client_state/validation.rs | 131 +----------------- .../ics02-client/context/src/client_state.rs | 44 +++--- ibc-core/ics02-client/context/src/context.rs | 6 +- .../src/handler/upgrade_client.rs | 18 ++- .../src/handler/conn_open_ack.rs | 12 +- .../src/handler/conn_open_confirm.rs | 5 +- .../src/handler/conn_open_try.rs | 10 +- .../src/handler/acknowledgement.rs | 4 +- .../src/handler/chan_close_confirm.rs | 2 +- .../src/handler/chan_open_ack.rs | 2 +- .../src/handler/chan_open_confirm.rs | 2 +- .../src/handler/chan_open_try.rs | 2 +- .../ics04-channel/src/handler/recv_packet.rs | 4 +- ibc-core/ics04-channel/src/handler/timeout.rs | 4 +- .../src/handler/timeout_on_close.rs | 6 +- .../traits/client_state_common.rs | 32 +++++ .../traits/client_state_validation.rs | 23 --- ibc-derive/src/utils.rs | 5 + .../testapp/ibc/clients/mock/client_state.rs | 45 +++--- .../tests/core/ics02_client/create_client.rs | 4 +- 22 files changed, 255 insertions(+), 230 deletions(-) diff --git a/ibc-clients/cw-context/src/handlers.rs b/ibc-clients/cw-context/src/handlers.rs index 3ad414231..4b4798d1e 100644 --- a/ibc-clients/cw-context/src/handlers.rs +++ b/ibc-clients/cw-context/src/handlers.rs @@ -103,13 +103,20 @@ where SudoMsg::VerifyUpgradeAndUpdateState(msg) => { let msg = VerifyUpgradeAndUpdateStateMsg::try_from(msg)?; - client_state.verify_upgrade_client( - self, + let client_cons_state_path = ClientConsensusStatePath::new( client_id.clone(), + client_state.latest_height().revision_number(), + client_state.latest_height().revision_height(), + ); + + let consensus_state = self.consensus_state(&client_cons_state_path)?; + + client_state.verify_upgrade_client( msg.upgrade_client_state.clone(), msg.upgrade_consensus_state.clone(), msg.proof_upgrade_client, msg.proof_upgrade_consensus_state, + consensus_state.root(), )?; client_state.update_state_on_upgrade( diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index aee889214..bfadd70c4 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -1,7 +1,7 @@ use ibc_client_tendermint_types::{client_type as tm_client_type, ClientState as ClientStateType}; use ibc_core_client::context::client_state::ClientStateCommon; use ibc_core_client::context::consensus_state::ConsensusState; -use ibc_core_client::types::error::ClientError; +use ibc_core_client::types::error::{ClientError, UpgradeClientError}; use ibc_core_client::types::Height; use ibc_core_commitment_types::commitment::{ CommitmentPrefix, CommitmentProofBytes, CommitmentRoot, @@ -10,9 +10,10 @@ 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::PathBytes; +use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath}; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; +use ibc_primitives::ToVec; use super::ClientState; use crate::consensus_state::ConsensusState as TmConsensusState; @@ -34,6 +35,38 @@ impl ClientStateCommon for ClientState { validate_proof_height(self.inner(), proof_height) } + fn verify_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + proof_upgrade_client: CommitmentProofBytes, + 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(UpgradeClientPath::UpgradedClientState(last_height))?; + + let upgrade_consensus_path_bytes = + self.serialize_path(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, + root, + ) + } + + fn serialize_path(&self, path: impl Into) -> Result { + Ok(path.into().to_string().into_bytes().into()) + } + fn verify_membership( &self, prefix: &CommitmentPrefix, @@ -109,6 +142,82 @@ pub fn validate_proof_height( Ok(()) } +/// 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. +/// +/// Note that this function is typically implemented as part of the +/// [`ClientStateCommon`] trait, but has been made a standalone function +/// in order to make the ClientState APIs more flexible. +#[allow(clippy::too_many_arguments)] +pub fn verify_upgrade_client( + client_state: &ClientStateType, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + proof_upgrade_client: CommitmentProofBytes, + proof_upgrade_consensus_state: CommitmentProofBytes, + 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` + let upgraded_tm_client_state = ClientState::try_from(upgraded_client_state.clone())?; + + // Make sure that the consensus type is of Tendermint type `ConsensusState` + TmConsensusState::try_from(upgraded_consensus_state.clone())?; + + let latest_height = client_state.latest_height; + let upgraded_tm_client_state_height = upgraded_tm_client_state.latest_height(); + + // 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 latest_height >= upgraded_tm_client_state_height { + Err(UpgradeClientError::LowUpgradeHeight { + upgraded_height: latest_height, + client_height: upgraded_tm_client_state_height, + })? + } + + // 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 { + description: "cannot upgrade client as no upgrade path has been set".to_string(), + }); + }; + + let upgrade_path_prefix = CommitmentPrefix::try_from(upgrade_path[0].clone().into_bytes()) + .map_err(ClientError::InvalidCommitmentProof)?; + + // Verify the proof of the upgraded client state + verify_membership::( + &client_state.proof_specs, + &upgrade_path_prefix, + &proof_upgrade_client, + root, + upgrade_client_path_bytes, + upgraded_client_state.to_vec(), + )?; + + // Verify the proof of the upgraded consensus state + verify_membership::( + &client_state.proof_specs, + &upgrade_path_prefix, + &proof_upgrade_consensus_state, + root, + upgrade_consensus_path_bytes, + upgraded_consensus_state.to_vec(), + )?; + + Ok(()) +} + /// Verify membership of the given value against the client's merkle proof. /// /// Note that this function is typically implemented as part of the diff --git a/ibc-clients/ics07-tendermint/src/client_state/validation.rs b/ibc-clients/ics07-tendermint/src/client_state/validation.rs index 49576eeca..116b84dd8 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/validation.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/validation.rs @@ -2,29 +2,21 @@ use ibc_client_tendermint_types::{ ClientState as ClientStateType, ConsensusState as ConsensusStateType, Header as TmHeader, Misbehaviour as TmMisbehaviour, TENDERMINT_HEADER_TYPE_URL, TENDERMINT_MISBEHAVIOUR_TYPE_URL, }; -use ibc_core_client::context::client_state::{ClientStateCommon, ClientStateValidation}; -use ibc_core_client::context::consensus_state::ConsensusState; -use ibc_core_client::context::{ClientValidationContext, Convertible, ExtClientValidationContext}; -use ibc_core_client::types::error::{ClientError, UpgradeClientError}; +use ibc_core_client::context::client_state::ClientStateValidation; +use ibc_core_client::context::{Convertible, ExtClientValidationContext}; +use ibc_core_client::types::error::ClientError; use ibc_core_client::types::Status; -use ibc_core_commitment_types::commitment::{CommitmentPrefix, CommitmentProofBytes}; -use ibc_core_commitment_types::proto::ics23::{HostFunctionsManager, HostFunctionsProvider}; use ibc_core_host::types::identifiers::ClientId; -use ibc_core_host::types::path::{ClientConsensusStatePath, UpgradeClientPath}; +use ibc_core_host::types::path::ClientConsensusStatePath; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; -use ibc_primitives::ToVec; use tendermint::crypto::default::Sha256; use tendermint::crypto::Sha256 as Sha256Trait; use tendermint::merkle::MerkleHash; use tendermint_light_client_verifier::{ProdVerifier, Verifier}; -use super::{ - check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update, verify_membership, - ClientState, -}; +use super::{check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update, ClientState}; use crate::client_state::{verify_header, verify_misbehaviour}; -use crate::consensus_state::ConsensusState as TmConsensusState; impl ClientStateValidation for ClientState where @@ -79,26 +71,6 @@ where status(self.inner(), ctx, client_id) } - fn verify_upgrade_client( - &self, - ctx: &V, - client_id: ClientId, - upgraded_client_state: Any, - upgraded_consensus_state: Any, - proof_upgrade_client: CommitmentProofBytes, - proof_upgrade_consensus_state: CommitmentProofBytes, - ) -> Result<(), ClientError> { - verify_upgrade_client::( - self.inner(), - ctx, - client_id, - upgraded_client_state, - upgraded_consensus_state, - proof_upgrade_client, - proof_upgrade_consensus_state, - ) - } - fn check_substitute(&self, _ctx: &V, substitute_client_state: Any) -> Result<(), ClientError> { check_substitute::(self.inner(), substitute_client_state) } @@ -257,99 +229,6 @@ where Ok(Status::Active) } -/// 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. -/// -/// Note that this function is typically implemented as part of the -/// [`ClientStateCommon`] trait, but has been made a standalone function -/// in order to make the ClientState APIs more flexible. -pub fn verify_upgrade_client( - client_state: &ClientStateType, - ctx: &V, - client_id: ClientId, - upgraded_client_state: Any, - upgraded_consensus_state: Any, - proof_upgrade_client: CommitmentProofBytes, - proof_upgrade_consensus_state: CommitmentProofBytes, -) -> Result<(), ClientError> -where - V: ClientValidationContext, - H: HostFunctionsProvider, -{ - // Read the latest consensus state from the host chain store. - let old_client_cons_state_path = ClientConsensusStatePath::new( - client_id.clone(), - client_state.latest_height.revision_number(), - client_state.latest_height.revision_height(), - ); - let old_consensus_state = ctx - .consensus_state(&old_client_cons_state_path) - .map_err(|_| ClientError::ConsensusStateNotFound { - client_id, - height: client_state.latest_height, - })?; - - // Make sure that the client type is of Tendermint type `ClientState` - let upgraded_tm_client_state = ClientState::try_from(upgraded_client_state.clone())?; - - // Make sure that the consensus type is of Tendermint type `ConsensusState` - TmConsensusState::try_from(upgraded_consensus_state.clone())?; - - let latest_height = client_state.latest_height; - let upgraded_tm_client_state_height = upgraded_tm_client_state.latest_height(); - - // 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 latest_height >= upgraded_tm_client_state_height { - Err(UpgradeClientError::LowUpgradeHeight { - upgraded_height: latest_height, - client_height: upgraded_tm_client_state_height, - })? - } - - // 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 { - description: "cannot upgrade client as no upgrade path has been set".to_string(), - }); - }; - - let upgrade_path_prefix = CommitmentPrefix::try_from(upgrade_path[0].clone().into_bytes()) - .map_err(ClientError::InvalidCommitmentProof)?; - - let last_height = latest_height.revision_height(); - - // Verify the proof of the upgraded client state - verify_membership::( - &client_state.proof_specs, - &upgrade_path_prefix, - &proof_upgrade_client, - old_consensus_state.root(), - ctx.serialize_path(UpgradeClientPath::UpgradedClientState(last_height))?, - upgraded_client_state.to_vec(), - )?; - - // Verify the proof of the upgraded consensus state - verify_membership::( - &client_state.proof_specs, - &upgrade_path_prefix, - &proof_upgrade_consensus_state, - old_consensus_state.root(), - ctx.serialize_path(UpgradeClientPath::UpgradedClientConsensusState(last_height))?, - upgraded_consensus_state.to_vec(), - )?; - - Ok(()) -} - /// Check that the subject and substitute client states match as part of /// the client recovery validation step. /// diff --git a/ibc-core/ics02-client/context/src/client_state.rs b/ibc-core/ics02-client/context/src/client_state.rs index 8a53c9d8b..71ea91e37 100644 --- a/ibc-core/ics02-client/context/src/client_state.rs +++ b/ibc-core/ics02-client/context/src/client_state.rs @@ -6,7 +6,7 @@ use ibc_core_commitment_types::commitment::{ CommitmentPrefix, CommitmentProofBytes, CommitmentRoot, }; use ibc_core_host_types::identifiers::{ClientId, ClientType}; -use ibc_core_host_types::path::PathBytes; +use ibc_core_host_types::path::{Path, PathBytes}; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; @@ -33,6 +33,28 @@ pub trait ClientStateCommon: Convertible { /// Validate that the client is at a sufficient height fn validate_proof_height(&self, proof_height: Height) -> Result<(), ClientError>; + /// 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, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + proof_upgrade_client: CommitmentProofBytes, + proof_upgrade_consensus_state: CommitmentProofBytes, + root: &CommitmentRoot, + ) -> Result<(), ClientError>; + + /// Determines how a path should be serialized into a `PathBytes` object. + fn serialize_path(&self, path: impl Into) -> Result; + // Verify_membership is a generic proof verification method which verifies a // proof of the existence of a value at a given Path. fn verify_membership( @@ -103,26 +125,6 @@ where /// Returns the status of the client. Only Active clients are allowed to process packets. fn status(&self, ctx: &V, client_id: &ClientId) -> Result; - /// 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, - ctx: &V, - client_id: ClientId, - upgraded_client_state: Any, - upgraded_consensus_state: Any, - proof_upgrade_client: CommitmentProofBytes, - proof_upgrade_consensus_state: CommitmentProofBytes, - ) -> Result<(), ClientError>; - /// Verifies whether the calling (subject) client state matches the substitute /// client state for the purposes of client recovery. /// diff --git a/ibc-core/ics02-client/context/src/context.rs b/ibc-core/ics02-client/context/src/context.rs index 8c44e7eec..972ec547e 100644 --- a/ibc-core/ics02-client/context/src/context.rs +++ b/ibc-core/ics02-client/context/src/context.rs @@ -1,7 +1,7 @@ use ibc_core_client_types::Height; use ibc_core_handler_types::error::ContextError; use ibc_core_host_types::identifiers::ClientId; -use ibc_core_host_types::path::{ClientConsensusStatePath, ClientStatePath, Path, PathBytes}; +use ibc_core_host_types::path::{ClientConsensusStatePath, ClientStatePath}; use ibc_primitives::prelude::*; use ibc_primitives::Timestamp; @@ -39,10 +39,6 @@ pub trait ClientValidationContext: Sized { client_id: &ClientId, height: &Height, ) -> Result<(Timestamp, Height), ContextError>; - - fn serialize_path(&self, path: impl Into) -> Result { - Ok(path.into().to_string().into_bytes().into()) - } } /// Defines the methods that all client `ExecutionContext`s (precisely the diff --git a/ibc-core/ics02-client/src/handler/upgrade_client.rs b/ibc-core/ics02-client/src/handler/upgrade_client.rs index 7c45edff4..a072988a2 100644 --- a/ibc-core/ics02-client/src/handler/upgrade_client.rs +++ b/ibc-core/ics02-client/src/handler/upgrade_client.rs @@ -1,10 +1,12 @@ //! Protocol logic specific to processing ICS2 messages of type `MsgUpgradeAnyClient`. //! use ibc_core_client_context::prelude::*; +use ibc_core_client_types::error::ClientError; use ibc_core_client_types::events::UpgradeClient; use ibc_core_client_types::msgs::MsgUpgradeClient; use ibc_core_handler_types::error::ContextError; use ibc_core_handler_types::events::{IbcEvent, MessageEvent}; +use ibc_core_host::types::path::ClientConsensusStatePath; use ibc_core_host::{ExecutionContext, ValidationContext}; use ibc_primitives::prelude::*; @@ -28,14 +30,26 @@ where .status(client_val_ctx, &client_id)? .verify_is_active()?; + // Read the latest consensus state from the host chain store. + let old_client_cons_state_path = ClientConsensusStatePath::new( + client_id.clone(), + old_client_state.latest_height().revision_number(), + old_client_state.latest_height().revision_height(), + ); + let old_consensus_state = client_val_ctx + .consensus_state(&old_client_cons_state_path) + .map_err(|_| ClientError::ConsensusStateNotFound { + client_id, + height: old_client_state.latest_height(), + })?; + // Validate the upgraded client state and consensus state and verify proofs against the root old_client_state.verify_upgrade_client( - client_val_ctx, - client_id, msg.upgraded_client_state.clone(), msg.upgraded_consensus_state, msg.proof_upgrade_client, msg.proof_upgrade_consensus_state, + old_consensus_state.root(), )?; Ok(()) diff --git a/ibc-core/ics03-connection/src/handler/conn_open_ack.rs b/ibc-core/ics03-connection/src/handler/conn_open_ack.rs index eb626eaf1..f6f4d31f0 100644 --- a/ibc-core/ics03-connection/src/handler/conn_open_ack.rs +++ b/ibc-core/ics03-connection/src/handler/conn_open_ack.rs @@ -84,9 +84,6 @@ where let prefix_on_b = vars.conn_end_on_a.counterparty().prefix(); { - let path_bytes = - client_val_ctx_a.serialize_path(ConnectionPath::new(&msg.conn_id_on_b))?; - let expected_conn_end_on_b = ConnectionEnd::new( State::TryOpen, vars.client_id_on_b().clone(), @@ -99,6 +96,9 @@ where vars.conn_end_on_a.delay_period(), )?; + let path_bytes = + client_state_of_b_on_a.serialize_path(ConnectionPath::new(&msg.conn_id_on_b))?; + client_state_of_b_on_a .verify_membership( prefix_on_b, @@ -110,8 +110,8 @@ where .map_err(ConnectionError::VerifyConnectionState)?; } - let path_bytes = - client_val_ctx_a.serialize_path(ClientStatePath::new(vars.client_id_on_b().clone()))?; + let path_bytes = client_state_of_b_on_a + .serialize_path(ClientStatePath::new(vars.client_id_on_b().clone()))?; client_state_of_b_on_a .verify_membership( @@ -138,7 +138,7 @@ where msg.consensus_height_of_a_on_b.revision_height(), ); - let path_bytes = client_val_ctx_a + let path_bytes = client_state_of_b_on_a .serialize_path(Path::ClientConsensusState(client_cons_state_path_on_b))?; client_state_of_b_on_a diff --git a/ibc-core/ics03-connection/src/handler/conn_open_confirm.rs b/ibc-core/ics03-connection/src/handler/conn_open_confirm.rs index 8e5428013..741c207dc 100644 --- a/ibc-core/ics03-connection/src/handler/conn_open_confirm.rs +++ b/ibc-core/ics03-connection/src/handler/conn_open_confirm.rs @@ -61,8 +61,6 @@ where let prefix_on_a = conn_end_on_b.counterparty().prefix(); let prefix_on_b = ctx_b.commitment_prefix(); - let path_bytes = client_val_ctx_b.serialize_path(ConnectionPath::new(conn_id_on_a))?; - let expected_conn_end_on_a = ConnectionEnd::new( State::Open, client_id_on_a.clone(), @@ -75,6 +73,9 @@ where conn_end_on_b.delay_period(), )?; + let path_bytes = + client_state_of_a_on_b.serialize_path(ConnectionPath::new(conn_id_on_a))?; + client_state_of_a_on_b .verify_membership( prefix_on_a, diff --git a/ibc-core/ics03-connection/src/handler/conn_open_try.rs b/ibc-core/ics03-connection/src/handler/conn_open_try.rs index 2ca2fe705..a4909d607 100644 --- a/ibc-core/ics03-connection/src/handler/conn_open_try.rs +++ b/ibc-core/ics03-connection/src/handler/conn_open_try.rs @@ -84,9 +84,6 @@ where let prefix_on_b = ctx_b.commitment_prefix(); { - let path_bytes = - client_val_ctx_b.serialize_path(ConnectionPath::new(&vars.conn_id_on_a))?; - let expected_conn_end_on_a = ConnectionEnd::new( State::Init, client_id_on_a.clone(), @@ -95,6 +92,9 @@ where msg.delay_period, )?; + let path_bytes = + client_state_of_a_on_b.serialize_path(ConnectionPath::new(&vars.conn_id_on_a))?; + client_state_of_a_on_b .verify_membership( prefix_on_a, @@ -107,7 +107,7 @@ where } let path_bytes = - client_val_ctx_b.serialize_path(ClientStatePath::new(client_id_on_a.clone()))?; + client_state_of_a_on_b.serialize_path(ClientStatePath::new(client_id_on_a.clone()))?; client_state_of_a_on_b .verify_membership( @@ -134,7 +134,7 @@ where msg.consensus_height_of_b_on_a.revision_height(), ); - let path_bytes = client_val_ctx_b.serialize_path(client_cons_state_path_on_a)?; + let path_bytes = client_state_of_a_on_b.serialize_path(client_cons_state_path_on_a)?; client_state_of_a_on_b .verify_membership( diff --git a/ibc-core/ics04-channel/src/handler/acknowledgement.rs b/ibc-core/ics04-channel/src/handler/acknowledgement.rs index d3d6994ab..a35461494 100644 --- a/ibc-core/ics04-channel/src/handler/acknowledgement.rs +++ b/ibc-core/ics04-channel/src/handler/acknowledgement.rs @@ -188,10 +188,10 @@ where let ack_path_on_b = AckPath::new(&packet.port_id_on_b, &packet.chan_id_on_b, packet.seq_on_a); - let path_bytes = client_val_ctx_a.serialize_path(ack_path_on_b)?; - verify_conn_delay_passed(ctx_a, msg.proof_height_on_b, &conn_end_on_a)?; + let path_bytes = client_state_of_b_on_a.serialize_path(ack_path_on_b)?; + // Verify the proof for the packet against the chain store. client_state_of_b_on_a .verify_membership( diff --git a/ibc-core/ics04-channel/src/handler/chan_close_confirm.rs b/ibc-core/ics04-channel/src/handler/chan_close_confirm.rs index fda78eab2..2339e7a7a 100644 --- a/ibc-core/ics04-channel/src/handler/chan_close_confirm.rs +++ b/ibc-core/ics04-channel/src/handler/chan_close_confirm.rs @@ -149,7 +149,7 @@ where )?; let chan_end_path_on_a = ChannelEndPath::new(port_id_on_a, chan_id_on_a); - let path_bytes = client_val_ctx_b.serialize_path(chan_end_path_on_a)?; + let path_bytes = client_state_of_a_on_b.serialize_path(chan_end_path_on_a)?; // Verify the proof for the channel state against the expected channel end. // A counterparty channel id of None in not possible, and is checked by validate_basic in msg. diff --git a/ibc-core/ics04-channel/src/handler/chan_open_ack.rs b/ibc-core/ics04-channel/src/handler/chan_open_ack.rs index 2855d95cf..38c43c057 100644 --- a/ibc-core/ics04-channel/src/handler/chan_open_ack.rs +++ b/ibc-core/ics04-channel/src/handler/chan_open_ack.rs @@ -142,7 +142,7 @@ where )?; let chan_end_path_on_b = ChannelEndPath::new(port_id_on_b, &msg.chan_id_on_b); - let path_bytes = client_val_ctx_a.serialize_path(chan_end_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(chan_end_path_on_b)?; // Verify the proof for the channel state against the expected channel end. // A counterparty channel id of None in not possible, and is checked by validate_basic in msg. diff --git a/ibc-core/ics04-channel/src/handler/chan_open_confirm.rs b/ibc-core/ics04-channel/src/handler/chan_open_confirm.rs index 7e96d96b9..0cc71d9f9 100644 --- a/ibc-core/ics04-channel/src/handler/chan_open_confirm.rs +++ b/ibc-core/ics04-channel/src/handler/chan_open_confirm.rs @@ -149,7 +149,7 @@ where )?; let chan_end_path_on_a = ChannelEndPath::new(port_id_on_a, chan_id_on_a); - let path_bytes = client_val_ctx_b.serialize_path(chan_end_path_on_a)?; + let path_bytes = client_state_of_a_on_b.serialize_path(chan_end_path_on_a)?; // Verify the proof for the channel state against the expected channel end. // A counterparty channel id of None in not possible, and is checked in msg. diff --git a/ibc-core/ics04-channel/src/handler/chan_open_try.rs b/ibc-core/ics04-channel/src/handler/chan_open_try.rs index d985b3e84..a147b95d9 100644 --- a/ibc-core/ics04-channel/src/handler/chan_open_try.rs +++ b/ibc-core/ics04-channel/src/handler/chan_open_try.rs @@ -168,7 +168,7 @@ where )?; let chan_end_path_on_a = ChannelEndPath::new(&port_id_on_a, &chan_id_on_a); - let path_bytes = client_val_ctx_b.serialize_path(chan_end_path_on_a)?; + let path_bytes = client_state_of_a_on_b.serialize_path(chan_end_path_on_a)?; // Verify the proof for the channel state against the expected channel end. // A counterparty channel id of None in not possible, and is checked by validate_basic in msg. diff --git a/ibc-core/ics04-channel/src/handler/recv_packet.rs b/ibc-core/ics04-channel/src/handler/recv_packet.rs index 10522db89..635eaaf9e 100644 --- a/ibc-core/ics04-channel/src/handler/recv_packet.rs +++ b/ibc-core/ics04-channel/src/handler/recv_packet.rs @@ -206,10 +206,10 @@ where msg.packet.seq_on_a, ); - let path_bytes = client_val_ctx_b.serialize_path(commitment_path_on_a)?; - verify_conn_delay_passed(ctx_b, msg.proof_height_on_a, &conn_end_on_b)?; + let path_bytes = client_state_of_a_on_b.serialize_path(commitment_path_on_a)?; + // Verify the proof for the packet against the chain store. client_state_of_a_on_b .verify_membership( diff --git a/ibc-core/ics04-channel/src/handler/timeout.rs b/ibc-core/ics04-channel/src/handler/timeout.rs index 3f0d12856..def90b0b3 100644 --- a/ibc-core/ics04-channel/src/handler/timeout.rs +++ b/ibc-core/ics04-channel/src/handler/timeout.rs @@ -221,7 +221,7 @@ where let seq_recv_path_on_b = SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b); - let path_bytes = client_val_ctx_a.serialize_path(seq_recv_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(seq_recv_path_on_b)?; client_state_of_b_on_a.verify_membership( conn_end_on_a.counterparty().prefix(), @@ -238,7 +238,7 @@ where msg.packet.seq_on_a, ); - let path_bytes = client_val_ctx_a.serialize_path(receipt_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(receipt_path_on_b)?; client_state_of_b_on_a.verify_non_membership( conn_end_on_a.counterparty().prefix(), diff --git a/ibc-core/ics04-channel/src/handler/timeout_on_close.rs b/ibc-core/ics04-channel/src/handler/timeout_on_close.rs index fefb7117f..df2530fb9 100644 --- a/ibc-core/ics04-channel/src/handler/timeout_on_close.rs +++ b/ibc-core/ics04-channel/src/handler/timeout_on_close.rs @@ -104,7 +104,7 @@ where let chan_end_path_on_b = ChannelEndPath(port_id_on_b, chan_id_on_b.clone()); - let path_bytes = client_val_ctx_a.serialize_path(chan_end_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(chan_end_path_on_b)?; // Verify the proof for the channel state against the expected channel end. // A counterparty channel id of None in not possible, and is checked by validate_basic in msg. @@ -133,7 +133,7 @@ where let seq_recv_path_on_b = SeqRecvPath::new(&packet.port_id_on_b, &packet.chan_id_on_b); - let path_bytes = client_val_ctx_a.serialize_path(seq_recv_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(seq_recv_path_on_b)?; client_state_of_b_on_a.verify_membership( conn_end_on_a.counterparty().prefix(), @@ -150,7 +150,7 @@ where msg.packet.seq_on_a, ); - let path_bytes = client_val_ctx_a.serialize_path(receipt_path_on_b)?; + let path_bytes = client_state_of_b_on_a.serialize_path(receipt_path_on_b)?; client_state_of_b_on_a.verify_non_membership( conn_end_on_a.counterparty().prefix(), diff --git a/ibc-derive/src/client_state/traits/client_state_common.rs b/ibc-derive/src/client_state/traits/client_state_common.rs index 3284eb220..34f593405 100644 --- a/ibc-derive/src/client_state/traits/client_state_common.rs +++ b/ibc-derive/src/client_state/traits/client_state_common.rs @@ -35,6 +35,18 @@ pub(crate) fn impl_ClientStateCommon( quote! {validate_proof_height(cs, proof_height)}, imports, ); + let verify_upgrade_client_impl = delegate_call_in_match( + client_state_enum_name, + enum_variants.iter(), + quote! {verify_upgrade_client(cs, upgraded_client_state, upgraded_consensus_state, proof_upgrade_client, proof_upgrade_consensus_state, root)}, + imports, + ); + let serialize_path_impl = delegate_call_in_match( + client_state_enum_name, + enum_variants.iter(), + quote! {serialize_path(cs, path)}, + imports, + ); let verify_membership_impl = delegate_call_in_match( client_state_enum_name, enum_variants.iter(), @@ -58,6 +70,7 @@ pub(crate) fn impl_ClientStateCommon( let ClientType = imports.client_type(); let ClientError = imports.client_error(); let Height = imports.height(); + let Path = imports.path(); let PathBytes = imports.path_bytes(); quote! { @@ -85,6 +98,25 @@ pub(crate) fn impl_ClientStateCommon( } } + fn verify_upgrade_client( + &self, + upgraded_client_state: #Any, + upgraded_consensus_state: #Any, + proof_upgrade_client: #CommitmentProofBytes, + proof_upgrade_consensus_state: #CommitmentProofBytes, + root: &#CommitmentRoot, + ) -> core::result::Result<(), #ClientError> { + match self { + #(#verify_upgrade_client_impl),* + } + } + + fn serialize_path(&self, path: impl Into<#Path>) -> core::result::Result<#PathBytes, #ClientError> { + match self { + #(#serialize_path_impl),* + } + } + fn verify_membership( &self, prefix: &#CommitmentPrefix, diff --git a/ibc-derive/src/client_state/traits/client_state_validation.rs b/ibc-derive/src/client_state/traits/client_state_validation.rs index 9c28307aa..263411b15 100644 --- a/ibc-derive/src/client_state/traits/client_state_validation.rs +++ b/ibc-derive/src/client_state/traits/client_state_validation.rs @@ -37,14 +37,6 @@ pub(crate) fn impl_ClientStateValidation( imports, ); - let verify_upgrade_client_impl = delegate_call_in_match( - client_state_enum_name, - enum_variants.iter(), - opts, - quote! {verify_upgrade_client(cs, ctx, client_id, upgraded_client_state, upgraded_consensus_state, proof_upgrade_client, proof_upgrade_consensus_state)}, - imports, - ); - let check_substitute_impl = delegate_call_in_match( client_state_enum_name, enum_variants.iter(), @@ -59,7 +51,6 @@ pub(crate) fn impl_ClientStateValidation( let ClientError = imports.client_error(); let ClientStateValidation = imports.client_state_validation(); let Status = imports.status(); - let CommitmentProofBytes = imports.commitment_proof_bytes(); // The types we need for the generated code. let HostClientState = client_state_enum_name; @@ -106,20 +97,6 @@ pub(crate) fn impl_ClientStateValidation( } } - fn verify_upgrade_client( - &self, - ctx: &#V, - client_id: #ClientId, - upgraded_client_state: #Any, - upgraded_consensus_state: #Any, - proof_upgrade_client: #CommitmentProofBytes, - proof_upgrade_consensus_state: #CommitmentProofBytes, - ) -> core::result::Result<(), #ClientError> { - match self { - #(#verify_upgrade_client_impl),* - } - } - fn check_substitute( &self, ctx: &#V, diff --git a/ibc-derive/src/utils.rs b/ibc-derive/src/utils.rs index 82cf91335..e028b677f 100644 --- a/ibc-derive/src/utils.rs +++ b/ibc-derive/src/utils.rs @@ -45,6 +45,11 @@ impl Imports { quote! {#Prefix::commitment_types::commitment::CommitmentProofBytes} } + pub fn path(&self) -> TokenStream { + let Prefix = self.prefix(); + quote! {#Prefix::host::types::path::Path} + } + pub fn path_bytes(&self) -> TokenStream { let Prefix = self.prefix(); quote! {#Prefix::host::types::path::PathBytes} diff --git a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs index afef24949..6319b4b64 100644 --- a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs +++ b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs @@ -9,7 +9,7 @@ use ibc::core::commitment_types::commitment::{ }; use ibc::core::handler::types::error::ContextError; use ibc::core::host::types::identifiers::{ClientId, ClientType}; -use ibc::core::host::types::path::{ClientConsensusStatePath, ClientStatePath, PathBytes}; +use ibc::core::host::types::path::{ClientConsensusStatePath, ClientStatePath, Path, PathBytes}; use ibc::core::primitives::prelude::*; use ibc::core::primitives::Timestamp; use ibc::primitives::proto::{Any, Protobuf}; @@ -184,6 +184,29 @@ impl ClientStateCommon for MockClientState { Ok(()) } + fn serialize_path(&self, path: impl Into) -> Result { + Ok(path.into().to_string().into_bytes().into()) + } + + fn verify_upgrade_client( + &self, + upgraded_client_state: Any, + upgraded_consensus_state: Any, + _proof_upgrade_client: CommitmentProofBytes, + _proof_upgrade_consensus_state: CommitmentProofBytes, + _root: &CommitmentRoot, + ) -> Result<(), ClientError> { + let upgraded_mock_client_state = Self::try_from(upgraded_client_state)?; + MockConsensusState::try_from(upgraded_consensus_state)?; + if self.latest_height() >= upgraded_mock_client_state.latest_height() { + return Err(UpgradeClientError::LowUpgradeHeight { + upgraded_height: self.latest_height(), + client_height: upgraded_mock_client_state.latest_height(), + })?; + } + Ok(()) + } + fn verify_membership( &self, _prefix: &CommitmentPrefix, @@ -287,26 +310,6 @@ where Ok(Status::Active) } - fn verify_upgrade_client( - &self, - _ctx: &V, - _client_id: ClientId, - upgraded_client_state: Any, - _upgraded_consensus_state: Any, - _proof_upgrade_client: CommitmentProofBytes, - _proof_upgrade_consensus_state: CommitmentProofBytes, - ) -> Result<(), ClientError> { - let upgraded_mock_client_state = Self::try_from(upgraded_client_state)?; - // MockConsensusState::try_from(upgraded_consensus_state)?; - if self.latest_height() >= upgraded_mock_client_state.latest_height() { - return Err(UpgradeClientError::LowUpgradeHeight { - upgraded_height: self.latest_height(), - client_height: upgraded_mock_client_state.latest_height(), - })?; - } - Ok(()) - } - fn check_substitute(&self, _ctx: &V, _substitute_client_state: Any) -> Result<(), ClientError> { Ok(()) } diff --git a/tests-integration/tests/core/ics02_client/create_client.rs b/tests-integration/tests/core/ics02_client/create_client.rs index ec284f27d..170baf42f 100644 --- a/tests-integration/tests/core/ics02_client/create_client.rs +++ b/tests-integration/tests/core/ics02_client/create_client.rs @@ -195,7 +195,7 @@ fn test_tm_create_client_proof_verification_ok() { &ctx_tm.ibc_store().commitment_prefix(), &proof, &root, - client_validation_ctx_mk + tm_client_state .serialize_path(next_client_seq_path.clone()) .expect("path"), serde_json::to_vec(&next_client_seq_value).expect("valid json serialization"), @@ -209,7 +209,7 @@ fn test_tm_create_client_proof_verification_ok() { &ctx_tm.ibc_store().commitment_prefix(), &proof, &root, - client_validation_ctx_mk + tm_client_state .serialize_path(next_client_seq_path.clone()) .expect("path"), serde_json::to_vec(&(next_client_seq_value + 1)).expect("valid json serialization"),