From 7ea5cae43d30f395bd3fc45cdb85303137de7347 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 7 Aug 2024 07:46:09 -0700 Subject: [PATCH] fix: prevent expired client creation (#1294) * fix: prevent expired client creation * chore: add changelog * chore: apply review comment Co-authored-by: Sean Chen Signed-off-by: Farhad Shabani * imp: associate consensus_state_status signatures with ConsensusState * Fix a word in doc comment --------- Signed-off-by: Farhad Shabani Co-authored-by: Sean Chen --- .../1088-prevent-expired-client-creation.md | 3 + .../src/client_state/common.rs | 60 +++- .../src/client_state/validation.rs | 22 +- .../ics02-client/context/src/client_state.rs | 7 +- .../ics02-client/src/handler/create_client.rs | 4 +- .../traits/client_state_common.rs | 5 +- ...header.json => expired_signed_header.json} | 0 .../src/data/json/valid_signed_header.json | 64 +++++ .../src/fixtures/clients/tendermint.rs | 20 +- .../fixtures/core/client/msg_create_client.rs | 4 +- .../testapp/ibc/clients/mock/client_state.rs | 26 +- ibc-testkit/src/testapp/ibc/core/types.rs | 5 +- .../tests/core/ics02_client/create_client.rs | 259 ++++++++++++------ .../tests/core/ics02_client/recover_client.rs | 31 ++- .../tests/core/ics02_client/upgrade_client.rs | 5 +- 15 files changed, 391 insertions(+), 124 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md rename ibc-testkit/src/data/json/{signed_header.json => expired_signed_header.json} (100%) create mode 100644 ibc-testkit/src/data/json/valid_signed_header.json diff --git a/.changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md b/.changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md new file mode 100644 index 000000000..104afd07d --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1088-prevent-expired-client-creation.md @@ -0,0 +1,3 @@ +- [ibc-core-client] Prevent expired client creations by ensuring that consensus + state's timestamp is within the trusting period. +- ([\#1088](https://github.com/cosmos/ibc-rs/issues/1088)) diff --git a/ibc-clients/ics07-tendermint/src/client_state/common.rs b/ibc-clients/ics07-tendermint/src/client_state/common.rs index c4726871e..d32225f4e 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/common.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/common.rs @@ -1,8 +1,10 @@ +use core::time::Duration; + 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, UpgradeClientError}; -use ibc_core_client::types::Height; +use ibc_core_client::types::{Height, Status}; use ibc_core_commitment_types::commitment::{ CommitmentPrefix, CommitmentProofBytes, CommitmentRoot, }; @@ -14,14 +16,22 @@ use ibc_core_host::types::identifiers::ClientType; use ibc_core_host::types::path::{Path, PathBytes, UpgradeClientPath}; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; -use ibc_primitives::ToVec; +use ibc_primitives::{Timestamp, ToVec}; use super::ClientState; use crate::consensus_state::ConsensusState as TmConsensusState; impl ClientStateCommon for ClientState { - fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError> { - verify_consensus_state(consensus_state) + fn verify_consensus_state( + &self, + consensus_state: Any, + host_timestamp: &Timestamp, + ) -> Result<(), ClientError> { + verify_consensus_state( + consensus_state, + host_timestamp, + self.inner().trusting_period, + ) } fn client_type(&self) -> ClientType { @@ -111,7 +121,11 @@ impl ClientStateCommon for ClientState { /// 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_consensus_state(consensus_state: Any) -> Result<(), ClientError> { +pub fn verify_consensus_state( + consensus_state: Any, + host_timestamp: &Timestamp, + trusting_period: Duration, +) -> Result<(), ClientError> { let tm_consensus_state = TmConsensusState::try_from(consensus_state)?; if tm_consensus_state.root().is_empty() { @@ -120,9 +134,45 @@ pub fn verify_consensus_state(consensus_state: Any) -> Result<(), ClientError> { }); }; + if consensus_state_status(&tm_consensus_state, host_timestamp, trusting_period)?.is_expired() { + return Err(ClientError::ClientNotActive { + status: Status::Expired, + }); + } + Ok(()) } +/// Determines the `Status`, whether it is `Active` or `Expired`, of a consensus +/// state, using its timestamp, the host's timestamp, and the trusting period. +pub fn consensus_state_status( + consensus_state: &CS, + host_timestamp: &Timestamp, + trusting_period: Duration, +) -> Result { + if !host_timestamp.is_set() { + return Err(ClientError::Other { + description: "host timestamp is none".into(), + }); + } + + // Note: if the `duration_since()` is `None`, indicating that the latest + // consensus state is in the future, then we don't consider the client + // to be expired. + if let Some(elapsed_since_latest_consensus_state) = + host_timestamp.duration_since(&consensus_state.timestamp()) + { + // Note: The equality is considered as expired to stay consistent with + // the check in tendermint-rs, where a header at `trusted_header_time + + // trusting_period` is considered expired. + if elapsed_since_latest_consensus_state >= trusting_period { + return Ok(Status::Expired); + } + } + + Ok(Status::Active) +} + /// Validate the given proof height against the client state's latest height, returning /// an error if the proof height is greater than the latest height of the client state. /// diff --git a/ibc-clients/ics07-tendermint/src/client_state/validation.rs b/ibc-clients/ics07-tendermint/src/client_state/validation.rs index 4a7f7f6a3..b1a028749 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/validation.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/validation.rs @@ -15,7 +15,10 @@ 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, ClientState}; +use super::{ + check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update, + consensus_state_status, ClientState, +}; use crate::client_state::{verify_header, verify_misbehaviour}; impl ClientStateValidation for ClientState @@ -218,18 +221,13 @@ where // to be expired. let now = ctx.host_timestamp()?; - if let Some(elapsed_since_latest_consensus_state) = - now.duration_since(&latest_consensus_state.timestamp().into()) - { - // Note: The equality is considered as expired to stay consistent with - // the check in tendermint-rs, where a header at `trusted_header_time + - // trusting_period` is considered expired. - if elapsed_since_latest_consensus_state >= client_state.trusting_period { - return Ok(Status::Expired); - } - } + let status = consensus_state_status( + &latest_consensus_state.into(), + &now, + client_state.trusting_period, + )?; - Ok(Status::Active) + Ok(status) } /// Check that the subject and substitute client states match as part of diff --git a/ibc-core/ics02-client/context/src/client_state.rs b/ibc-core/ics02-client/context/src/client_state.rs index 3464e37cb..bbcc24aa7 100644 --- a/ibc-core/ics02-client/context/src/client_state.rs +++ b/ibc-core/ics02-client/context/src/client_state.rs @@ -9,6 +9,7 @@ use ibc_core_host_types::identifiers::{ClientId, ClientType}; use ibc_core_host_types::path::{Path, PathBytes}; use ibc_primitives::prelude::*; use ibc_primitives::proto::Any; +use ibc_primitives::Timestamp; use crate::context::{ClientExecutionContext, ClientValidationContext}; use crate::Convertible; @@ -22,7 +23,11 @@ pub trait ClientStateCommon: Convertible { /// /// Notably, an implementation should verify that it can properly /// deserialize the object into the expected format. - fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError>; + fn verify_consensus_state( + &self, + consensus_state: Any, + host_timestamp: &Timestamp, + ) -> Result<(), ClientError>; /// Type of client associated with this state (eg. Tendermint) fn client_type(&self) -> ClientType; diff --git a/ibc-core/ics02-client/src/handler/create_client.rs b/ibc-core/ics02-client/src/handler/create_client.rs index 4cb3d34f2..d6413596f 100644 --- a/ibc-core/ics02-client/src/handler/create_client.rs +++ b/ibc-core/ics02-client/src/handler/create_client.rs @@ -41,7 +41,9 @@ where .into()); }; - client_state.verify_consensus_state(consensus_state)?; + let host_timestamp = ctx.host_timestamp()?; + + client_state.verify_consensus_state(consensus_state, &host_timestamp)?; if client_val_ctx.client_state(&client_id).is_ok() { return Err(ClientError::ClientStateAlreadyExists { client_id }.into()); 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 0184d9a23..8a1cb6dc3 100644 --- a/ibc-derive/src/client_state/traits/client_state_common.rs +++ b/ibc-derive/src/client_state/traits/client_state_common.rs @@ -14,7 +14,7 @@ pub(crate) fn impl_ClientStateCommon( let verify_consensus_state_impl = delegate_call_in_match( client_state_enum_name, enum_variants.iter(), - quote! { verify_consensus_state(cs, consensus_state) }, + quote! { verify_consensus_state(cs, consensus_state, &host_timestamp) }, imports, ); let client_type_impl = delegate_call_in_match( @@ -82,12 +82,13 @@ pub(crate) fn impl_ClientStateCommon( let ClientType = imports.client_type(); let ClientError = imports.client_error(); let Height = imports.height(); + let Timestamp = imports.timestamp(); let Path = imports.path(); let PathBytes = imports.path_bytes(); quote! { impl #ClientStateCommon for #HostClientState { - fn verify_consensus_state(&self, consensus_state: #Any) -> Result<(), #ClientError> { + fn verify_consensus_state(&self, consensus_state: #Any, host_timestamp: &#Timestamp) -> Result<(), #ClientError> { match self { #(#verify_consensus_state_impl),* } diff --git a/ibc-testkit/src/data/json/signed_header.json b/ibc-testkit/src/data/json/expired_signed_header.json similarity index 100% rename from ibc-testkit/src/data/json/signed_header.json rename to ibc-testkit/src/data/json/expired_signed_header.json diff --git a/ibc-testkit/src/data/json/valid_signed_header.json b/ibc-testkit/src/data/json/valid_signed_header.json new file mode 100644 index 000000000..13c278961 --- /dev/null +++ b/ibc-testkit/src/data/json/valid_signed_header.json @@ -0,0 +1,64 @@ +{ + "header": { + "version": { + "block": "0", + "app": "0" + }, + "chain_id": "test-chain-1", + "height": "20", + "time": "2030-11-02T15:04:10Z", + "last_block_id": { + "hash": "15F15EF50BDE2018F4B129A827F90C18222C757770C8295EB8EE7BF50E761BC0", + "part_set_header": { + "total": 1, + "hash": "077E16D720F9AA656EBFD7F3FB31A4A35E1F2F4EBEBB123642BED45535D88AD5" + } + }, + "last_commit_hash": "D5439DD65D45EF1E51412691BCF2F6741D48AC1325572E08D48BD72F80669E70", + "data_hash": "", + "validators_hash": "26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564", + "next_validators_hash": "26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564", + "consensus_hash": "048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F", + "app_hash": "6170705F68617368", + "last_results_hash": "", + "evidence_hash": "", + "proposer_address": "026CC7B6F3E62F789DBECEC59766888B5464737D" + }, + "commit": { + "height": "20", + "round": 1, + "block_id": { + "hash": "5E87BD3A35C62D06138273453AF49C7728E4F8FB4CAFB0784F4816D5052AA349", + "part_set_header": { + "total": 1, + "hash": "DC797E9C450AE5FD0D8000E31672BE3EE97B6C0A3BD69239187F75C00C39D72B" + } + }, + "signatures": [ + { + "block_id_flag": 3, + "validator_address": "01F527D77D3FFCC4FCFF2DDC2952EEA5414F2A33", + "timestamp": "2019-11-02T15:04:15Z", + "signature": "SMBYJYr7yHbqmScg7jUhSGyis/gOMvwkO4mlEmKt9NKJ+aZgETFgIIFnExCju0VidMyy9mUwMQ/b+EoVL1NwAQ==" + }, + { + "block_id_flag": 2, + "validator_address": "026CC7B6F3E62F789DBECEC59766888B5464737D", + "timestamp": "2019-11-02T15:04:15Z", + "signature": "Bn8cV8WlqGjezZcFgQey0ikBshIHw56jpuzWssO2/0HOpMC1Mr2mJ5mxEd4mpsQ4PwenpGtrLJv11PFK6aJLBw==" + }, + { + "block_id_flag": 2, + "validator_address": "03A238BCAF7D1626DFE8A4AFB9448D00B7A3D2E2", + "timestamp": "2019-11-02T15:04:15Z", + "signature": "mhgrDo+fV5GaS34CZn//zgNx24H21Ila9io5HPgxeEWbG24eGLp0GAAlGPryT9bdv5LXPUq8wZpUHxAcyhvZAQ==" + }, + { + "block_id_flag": 2, + "validator_address": "03EC0413849A3311A5341E7A69D6C544E9A30310", + "timestamp": "2019-11-02T15:04:15Z", + "signature": "+g5Rhm8Utshm0SN2gxKeRrqsJyUO32BxDqTVu0wHzaJz0cFJWZxA2JaYxaf6EzuQULkwp+oh6u3xboy/e5wHAg==" + } + ] + } +} \ No newline at end of file diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index aaa49a5d7..89d870f72 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -112,12 +112,24 @@ impl ClientStateConfig { } #[cfg(feature = "serde")] -pub fn dummy_tendermint_header() -> tendermint::block::Header { +pub fn dummy_valid_tendermint_header() -> tendermint::block::Header { use tendermint::block::signed_header::SignedHeader; serde_json::from_str::(include_str!(concat!( env!("CARGO_MANIFEST_DIR"), - "/src/data/json/signed_header.json" + "/src/data/json/valid_signed_header.json" + ))) + .expect("Never fails") + .header +} + +#[cfg(feature = "serde")] +pub fn dummy_expired_tendermint_header() -> tendermint::block::Header { + use tendermint::block::signed_header::SignedHeader; + + serde_json::from_str::(include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/src/data/json/expired_signed_header.json" ))) .expect("Never fails") .header @@ -147,7 +159,7 @@ pub fn dummy_ics07_header() -> Header { // Build a SignedHeader from a JSON file. let shdr = serde_json::from_str::(include_str!(concat!( env!("CARGO_MANIFEST_DIR"), - "/src/data/json/signed_header.json" + "/src/data/json/valid_signed_header.json" ))) .expect("Never fails"); @@ -222,7 +234,7 @@ mod tests { #[test] fn tm_client_state_from_header_healthy() { // check client state creation path from a tendermint header - let tm_header = dummy_tendermint_header(); + let tm_header = dummy_valid_tendermint_header(); let tm_client_state_from_header = dummy_tm_client_state_from_header(tm_header); let any_from_header = Any::from(tm_client_state_from_header.clone()); let tm_client_state_from_any = ClientStateType::try_from(any_from_header); diff --git a/ibc-testkit/src/fixtures/core/client/msg_create_client.rs b/ibc-testkit/src/fixtures/core/client/msg_create_client.rs index bc1cfbc31..a585e2f86 100644 --- a/ibc-testkit/src/fixtures/core/client/msg_create_client.rs +++ b/ibc-testkit/src/fixtures/core/client/msg_create_client.rs @@ -3,13 +3,13 @@ use ibc::core::client::types::proto::v1::MsgCreateClient as RawMsgCreateClient; use ibc::primitives::proto::Any; use crate::fixtures::clients::tendermint::{ - dummy_tendermint_header, dummy_tm_client_state_from_header, + dummy_tm_client_state_from_header, dummy_valid_tendermint_header, }; use crate::fixtures::core::signer::dummy_bech32_account; /// Returns a dummy `RawMsgCreateClient`, for testing purposes only! pub fn dummy_raw_msg_create_client() -> RawMsgCreateClient { - let tm_header = dummy_tendermint_header(); + let tm_header = dummy_valid_tendermint_header(); let tm_client_state = dummy_tm_client_state_from_header(tm_header.clone()); 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 ddcb0c684..46009a70d 100644 --- a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs +++ b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs @@ -1,6 +1,7 @@ use core::str::FromStr; use core::time::Duration; +use ibc::clients::tendermint::client_state::consensus_state_status; use ibc::core::client::context::prelude::*; use ibc::core::client::types::error::{ClientError, UpgradeClientError}; use ibc::core::client::types::{Height, Status}; @@ -160,8 +161,29 @@ pub trait MockClientContext { } impl ClientStateCommon for MockClientState { - fn verify_consensus_state(&self, consensus_state: Any) -> Result<(), ClientError> { - let _mock_consensus_state = MockConsensusState::try_from(consensus_state)?; + fn verify_consensus_state( + &self, + consensus_state: Any, + host_timestamp: &Timestamp, + ) -> Result<(), ClientError> { + let mock_consensus_state = MockConsensusState::try_from(consensus_state)?; + + let consensus_state_timestamp = mock_consensus_state.timestamp(); + + if !consensus_state_timestamp.is_set() { + return Err(ClientError::InvalidConsensusStateTimestamp { + time1: *host_timestamp, + time2: consensus_state_timestamp, + }); + } + + if consensus_state_status(&mock_consensus_state, host_timestamp, self.trusting_period)? + .is_expired() + { + return Err(ClientError::ClientNotActive { + status: Status::Expired, + }); + } Ok(()) } diff --git a/ibc-testkit/src/testapp/ibc/core/types.rs b/ibc-testkit/src/testapp/ibc/core/types.rs index a1296cf6a..4bd314c60 100644 --- a/ibc-testkit/src/testapp/ibc/core/types.rs +++ b/ibc-testkit/src/testapp/ibc/core/types.rs @@ -188,7 +188,10 @@ where ibc_store.store.commit().expect("no error"); ibc_store.store_host_consensus_state( ibc_store.store.current_height(), - MockHeader::default().into_consensus_state().into(), + MockHeader::default() + .with_current_timestamp() + .into_consensus_state() + .into(), ); ibc_store.store_ibc_commitment_proof( ibc_store.store.current_height(), diff --git a/tests-integration/tests/core/ics02_client/create_client.rs b/tests-integration/tests/core/ics02_client/create_client.rs index 4e5dd9e77..71b40718c 100644 --- a/tests-integration/tests/core/ics02_client/create_client.rs +++ b/tests-integration/tests/core/ics02_client/create_client.rs @@ -1,4 +1,3 @@ -use basecoin_store::impls::InMemoryStore; use ibc::clients::tendermint::types::{ client_type as tm_client_type, ConsensusState as TmConsensusState, }; @@ -14,13 +13,17 @@ use ibc::core::handler::types::msgs::MsgEnvelope; use ibc::core::host::types::identifiers::ClientId; use ibc::core::host::types::path::{ClientConsensusStatePath, NextClientSequencePath}; use ibc::core::host::{ClientStateRef, ValidationContext}; +use ibc_core_client_types::Status; use ibc_query::core::context::ProvableContext; use ibc_testkit::context::{MockContext, TendermintContext}; -#[cfg(feature = "serde")] -use ibc_testkit::fixtures::clients::tendermint::dummy_tendermint_header; use ibc_testkit::fixtures::clients::tendermint::dummy_tm_client_state_from_header; +#[cfg(feature = "serde")] +use ibc_testkit::fixtures::clients::tendermint::{ + dummy_expired_tendermint_header, dummy_valid_tendermint_header, +}; use ibc_testkit::fixtures::core::context::TestContextConfig; use ibc_testkit::fixtures::core::signer::dummy_account_id; +use ibc_testkit::fixtures::{Expect, Fixture}; use ibc_testkit::testapp::ibc::clients::mock::client_state::{ client_type as mock_client_type, MockClientState, }; @@ -28,109 +31,201 @@ use ibc_testkit::testapp::ibc::clients::mock::consensus_state::MockConsensusStat use ibc_testkit::testapp::ibc::clients::mock::header::MockHeader; use ibc_testkit::testapp::ibc::clients::{AnyClientState, AnyConsensusState}; use ibc_testkit::testapp::ibc::core::router::MockRouter; -use ibc_testkit::testapp::ibc::core::types::{DefaultIbcStore, LightClientBuilder, MockIbcStore}; +use ibc_testkit::testapp::ibc::core::types::{DefaultIbcStore, LightClientBuilder}; +use ibc_testkit::utils::year_2023; use test_log::test; -#[test] -fn test_create_client_ok() { - let mut ctx = DefaultIbcStore::default(); - let mut router = MockRouter::new_with_transfer(); - let signer = dummy_account_id(); - let height = Height::new(0, 42).unwrap(); - - let msg = MsgCreateClient::new( - MockClientState::new(MockHeader::new(height)).into(), - MockConsensusState::new(MockHeader::new(height)).into(), - signer, - ); - - let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg.clone())); +enum Ctx { + Default, +} - let client_type = mock_client_type(); - let client_id = client_type.build_client_id(ctx.client_counter().unwrap()); +#[allow(warnings)] +enum Msg { + ValidMockHeader, + ExpiredMockHeader, + ValidTendermintHeader, + ExpiredTendermintHeader, + FrozenTendermintHeader, + NoHostTimestamp, +} - let res = validate(&ctx, &router, msg_envelope.clone()); +fn create_client_fixture(ctx_variant: Ctx, msg_variant: Msg) -> Fixture { + let ctx = match ctx_variant { + Ctx::Default => DefaultIbcStore::default(), + }; - assert!(res.is_ok(), "validation happy path"); + let signer = dummy_account_id(); + let height = Height::new(0, 42).unwrap(); - let res = execute(&mut ctx, &mut router, msg_envelope); + let msg = match msg_variant { + Msg::ValidMockHeader => { + let header = MockHeader::new(height).with_current_timestamp(); - assert!(res.is_ok(), "execution happy path"); + MsgCreateClient::new( + MockClientState::new(header).into(), + MockConsensusState::new(header).into(), + signer, + ) + } + Msg::ExpiredMockHeader => { + let header = MockHeader::new(height).with_timestamp(year_2023()); + + MsgCreateClient::new( + MockClientState::new(header).into(), + MockConsensusState::new(header).into(), + signer, + ) + } + Msg::ValidTendermintHeader => { + let tm_header = dummy_valid_tendermint_header(); + + MsgCreateClient::new( + dummy_tm_client_state_from_header(tm_header.clone()).into(), + TmConsensusState::from(tm_header).into(), + signer, + ) + } + Msg::ExpiredTendermintHeader => { + let tm_header = dummy_expired_tendermint_header(); + + MsgCreateClient::new( + dummy_tm_client_state_from_header(tm_header.clone()).into(), + TmConsensusState::from(tm_header).into(), + signer, + ) + } + Msg::FrozenTendermintHeader => { + let tm_header = dummy_valid_tendermint_header(); + + MsgCreateClient::new( + dummy_tm_client_state_from_header(tm_header.clone()) + .inner() + .clone() + .with_frozen_height(Height::min(0)) + .into(), + TmConsensusState::from(tm_header).into(), + signer, + ) + } + Msg::NoHostTimestamp => { + let header = MockHeader::new(height); + + MsgCreateClient::new( + MockClientState::new(header).into(), + MockConsensusState::new(header).into(), + signer, + ) + } + }; - let expected_client_state = - ClientStateRef::::try_from(msg.client_state).unwrap(); - assert_eq!(expected_client_state.client_type(), client_type); - assert_eq!(ctx.client_state(&client_id).unwrap(), expected_client_state); + Fixture { ctx, msg } } -#[cfg(feature = "serde")] -#[test] -fn test_tm_create_client_ok() { - let signer = dummy_account_id(); - - let mut ctx = DefaultIbcStore::default(); +fn create_client_validate(fxt: &Fixture, expect: Expect) { + let router = MockRouter::new_with_transfer(); + let msg_envelope = MsgEnvelope::from(ClientMsg::from(fxt.msg.clone())); + let res = validate(&fxt.ctx, &router, msg_envelope); + let err_msg = fxt.generate_error_msg(&expect, "validation", &res); + match expect { + Expect::Failure(err) => match err { + Some(_e) => { + assert!(matches!(res, Err(_e))); + } + _ => { + assert!(res.is_err(), "{err_msg}"); + } + }, + Expect::Success => { + assert!(res.is_ok(), "{err_msg}"); + } + } +} +fn create_client_execute(fxt: &mut Fixture, expect: Expect) { let mut router = MockRouter::new_with_transfer(); + let msg_envelope = MsgEnvelope::from(ClientMsg::from(fxt.msg.clone())); + let expected_client_state = + ClientStateRef::::try_from(fxt.msg.client_state.clone()).unwrap(); - let tm_header = dummy_tendermint_header(); - - let tm_client_state = dummy_tm_client_state_from_header(tm_header.clone()).into(); + let client_type = match expected_client_state { + AnyClientState::Mock(_) => mock_client_type(), + AnyClientState::Tendermint(_) => tm_client_type(), + }; + let client_id = client_type.build_client_id(fxt.ctx.client_counter().unwrap()); + let res = execute(&mut fxt.ctx, &mut router, msg_envelope); + let err_msg = fxt.generate_error_msg(&expect, "execution", &res); + match expect { + Expect::Failure(_) => { + assert!(res.is_err(), "{err_msg}") + } + Expect::Success => { + assert_eq!( + fxt.ctx.client_state(&client_id).unwrap(), + expected_client_state + ); + assert!(res.is_ok(), "{err_msg}"); + } + } +} - let client_type = tm_client_type(); - let client_id = client_type.build_client_id(ctx.client_counter().unwrap()); +#[test] +fn test_create_mock_client_ok() { + let mut fxt = create_client_fixture(Ctx::Default, Msg::ValidMockHeader); + create_client_validate(&fxt, Expect::Success); + create_client_execute(&mut fxt, Expect::Success); +} - let msg = MsgCreateClient::new( - tm_client_state, - TmConsensusState::from(tm_header).into(), - signer, +#[test] +fn test_create_expired_mock_client() { + let fxt = create_client_fixture(Ctx::Default, Msg::ExpiredMockHeader); + create_client_validate( + &fxt, + Expect::Failure(Some(ContextError::ClientError( + ClientError::ClientNotActive { + status: Status::Expired, + }, + ))), ); +} - let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg.clone())); - - let res = validate(&ctx, &router, msg_envelope.clone()); - - assert!(res.is_ok(), "tendermint client validation happy path"); - - let res = execute(&mut ctx, &mut router, msg_envelope); - - assert!(res.is_ok(), "tendermint client execution happy path"); - - let expected_client_state = - ClientStateRef::>::try_from(msg.client_state).unwrap(); - assert_eq!(expected_client_state.client_type(), client_type); - assert_eq!(ctx.client_state(&client_id).unwrap(), expected_client_state); +#[test] +fn test_create_mock_client_without_timestamp() { + let fxt = create_client_fixture(Ctx::Default, Msg::NoHostTimestamp); + create_client_validate(&fxt, Expect::Failure(None)); } #[cfg(feature = "serde")] #[test] -fn test_invalid_frozen_tm_client_creation() { - let signer = dummy_account_id(); - - let ctx = DefaultIbcStore::default(); - - let router = MockRouter::new_with_transfer(); - - let tm_header = dummy_tendermint_header(); - - let tm_client_state = dummy_tm_client_state_from_header(tm_header.clone()) - .inner() - .clone() - .with_frozen_height(Height::min(0)); +fn test_create_tm_client_ok() { + let mut fxt = create_client_fixture(Ctx::Default, Msg::ValidTendermintHeader); + create_client_validate(&fxt, Expect::Success); + create_client_execute(&mut fxt, Expect::Success); +} - let msg = MsgCreateClient::new( - tm_client_state.into(), - TmConsensusState::from(tm_header).into(), - signer, +#[cfg(feature = "serde")] +#[test] +fn test_create_expired_tm_client() { + let fxt = create_client_fixture(Ctx::Default, Msg::ExpiredTendermintHeader); + create_client_validate( + &fxt, + Expect::Failure(Some(ContextError::ClientError( + ClientError::ClientNotActive { + status: Status::Expired, + }, + ))), ); +} - let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg)); - - let res = validate(&ctx, &router, msg_envelope); - - assert!(matches!( - res, - Err(ContextError::ClientError(ClientError::ClientFrozen { .. })) - )) +#[cfg(feature = "serde")] +#[test] +fn test_create_frozen_tm_client() { + let fxt = create_client_fixture(Ctx::Default, Msg::FrozenTendermintHeader); + create_client_validate( + &fxt, + Expect::Failure(Some(ContextError::ClientError(ClientError::ClientFrozen { + description: "the client is frozen".to_string(), + }))), + ); } #[test] diff --git a/tests-integration/tests/core/ics02_client/recover_client.rs b/tests-integration/tests/core/ics02_client/recover_client.rs index f40f15f52..0b6dffd6c 100644 --- a/tests-integration/tests/core/ics02_client/recover_client.rs +++ b/tests-integration/tests/core/ics02_client/recover_client.rs @@ -10,6 +10,7 @@ use ibc::core::host::types::identifiers::ClientId; use ibc::core::host::types::path::ClientConsensusStatePath; use ibc::core::host::ValidationContext; use ibc::core::primitives::Signer; +use ibc_primitives::Timestamp; use ibc_testkit::context::{MockContext, TendermintContext}; use ibc_testkit::fixtures::core::context::TestContextConfig; use ibc_testkit::fixtures::core::signer::dummy_account_id; @@ -42,11 +43,16 @@ fn setup_client_recovery_fixture( substitute_trusting_period: Duration, substitute_height: Height, ) -> Fixture { - let mut ctx = TendermintContext::default(); + let latest_timestamp = Timestamp::now(); + + let mut ctx_a: TendermintContext = TestContextConfig::builder() + .latest_timestamp(latest_timestamp) + .build(); // create a ctx_b let ctx_b: MockContext = TestContextConfig::builder() .latest_height(substitute_height) + .latest_timestamp(latest_timestamp) .build(); let signer = dummy_account_id(); @@ -68,9 +74,11 @@ fn setup_client_recovery_fixture( let subject_msg_envelope = MsgEnvelope::from(ClientMsg::from(msg)); let client_type = mock_client_type(); - let subject_client_id = client_type.build_client_id(ctx.ibc_store().client_counter().unwrap()); + let subject_client_id = + client_type.build_client_id(ctx_a.ibc_store().client_counter().unwrap()); - ctx.dispatch(subject_msg_envelope) + ctx_a + .dispatch(subject_msg_envelope) .expect("create subject client execution"); let substitute_client_header = ctx_b @@ -90,17 +98,18 @@ fn setup_client_recovery_fixture( let substitute_msg_envelope = MsgEnvelope::from(ClientMsg::from(msg)); let substitute_client_id = - client_type.build_client_id(ctx.ibc_store().client_counter().unwrap()); + client_type.build_client_id(ctx_a.ibc_store().client_counter().unwrap()); - ctx.dispatch(substitute_msg_envelope) + ctx_a + .dispatch(substitute_msg_envelope) .expect("create substitute client execution"); let subject_client_trusted_timestamp = (subject_client_header.timestamp() + subject_trusting_period).expect("no error"); // Let the subject client state expire. - while ctx.latest_timestamp() <= subject_client_trusted_timestamp { - ctx.advance_block_height(); + while ctx_a.latest_timestamp() <= subject_client_trusted_timestamp { + ctx_a.advance_block_height(); } // at this point, the subject client should be expired. @@ -108,7 +117,7 @@ fn setup_client_recovery_fixture( // to the fixture arguments Fixture { - ctx, + ctx: ctx_a, subject_client_id, substitute_client_id, signer, @@ -117,8 +126,10 @@ fn setup_client_recovery_fixture( #[rstest] fn test_recover_client_ok() { - let subject_trusting_period = Duration::from_secs(DEFAULT_BLOCK_TIME_SECS); - let substitute_trusting_period = Duration::from_secs(DEFAULT_BLOCK_TIME_SECS) * 10; + // NOTE: The trusting periods are extended by 1 second as clients expire + // when the elapsed time since a trusted header MATCHES the trusting period. + let subject_trusting_period = Duration::from_secs(DEFAULT_BLOCK_TIME_SECS + 1); + let substitute_trusting_period = Duration::from_secs(DEFAULT_BLOCK_TIME_SECS + 1) * 10; let subject_height = Height::new(0, 42).unwrap(); let substitute_height = Height::new(0, 43).unwrap(); diff --git a/tests-integration/tests/core/ics02_client/upgrade_client.rs b/tests-integration/tests/core/ics02_client/upgrade_client.rs index 015bf8921..c59a7686a 100644 --- a/tests-integration/tests/core/ics02_client/upgrade_client.rs +++ b/tests-integration/tests/core/ics02_client/upgrade_client.rs @@ -10,7 +10,7 @@ use ibc::core::handler::types::msgs::MsgEnvelope; use ibc::core::host::types::path::ClientConsensusStatePath; use ibc_testkit::context::MockContext; use ibc_testkit::fixtures::clients::tendermint::{ - dummy_tendermint_header, dummy_tm_client_state_from_header, + dummy_tm_client_state_from_header, dummy_valid_tendermint_header, }; use ibc_testkit::fixtures::core::client::dummy_msg_upgrade_client; use ibc_testkit::fixtures::{Expect, Fixture}; @@ -51,7 +51,8 @@ fn msg_upgrade_client_fixture(ctx_variant: Ctx, msg_variant: Msg) -> Fixture