From 806a66892bfa3016d38376257fd3d318a3c4d677 Mon Sep 17 00:00:00 2001 From: Rivers Yang Date: Wed, 17 Jan 2024 17:11:42 +0800 Subject: [PATCH 1/6] Fix issues related to frozen height of tendermint client. --- .../types/src/client_state.rs | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index b1f755711..182096ac2 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -12,7 +12,6 @@ use ibc_core_host_types::identifiers::ChainId; use ibc_primitives::prelude::*; use ibc_primitives::ZERO_DURATION; use ibc_proto::google::protobuf::Any; -use ibc_proto::ibc::core::client::v1::Height as RawHeight; use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawTmClientState; use ibc_proto::Protobuf; use tendermint::chain::id::MAX_LENGTH as MaxChainIdLen; @@ -283,13 +282,16 @@ impl TryFrom for ClientState { // In `RawClientState`, a `frozen_height` of `0` means "not frozen". // See: // https://github.com/cosmos/ibc-go/blob/8422d0c4c35ef970539466c5bdec1cd27369bab3/modules/light-clients/07-tendermint/types/client_state.go#L74 - if raw - .frozen_height - .and_then(|h| Height::try_from(h).ok()) - .is_some() - { - return Err(Error::FrozenHeightNotAllowed); - } + // + // Comment out this check for now to allow fixing the client state. + // + // if raw + // .frozen_height + // .and_then(|h| Height::try_from(h).ok()) + // .is_some() + // { + // return Err(Error::FrozenHeightNotAllowed); + // } // We use set this deprecated field just so that we can properly convert // it back in its raw form @@ -299,8 +301,8 @@ impl TryFrom for ClientState { after_misbehaviour: raw.allow_update_after_misbehaviour, }; - let client_state = Self::new_without_validation( - chain_id, + let mut client_state = Self::new_without_validation( + chain_id.clone(), trust_level, trusting_period, unbonding_period, @@ -311,6 +313,12 @@ impl TryFrom for ClientState { allow_update, ); + // Restore the frozen height if it was set, allow fixing the client state. + if let Some(frozen_height) = raw.frozen_height { + client_state = client_state.with_frozen_height( + Height::try_from(frozen_height).unwrap_or(Height::min(chain_id.revision_number())), + ); + }; Ok(client_state) } } @@ -324,12 +332,7 @@ impl From for RawTmClientState { trusting_period: Some(value.trusting_period.into()), unbonding_period: Some(value.unbonding_period.into()), max_clock_drift: Some(value.max_clock_drift.into()), - frozen_height: Some(value.frozen_height.map(|height| height.into()).unwrap_or( - RawHeight { - revision_number: 0, - revision_height: 0, - }, - )), + frozen_height: value.frozen_height.map(|height| height.into()), latest_height: Some(value.latest_height.into()), proof_specs: value.proof_specs.into(), upgrade_path: value.upgrade_path, From d715869a724028fbdf5eb9879ae7ababd0073047 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 25 Jan 2024 14:52:19 -0800 Subject: [PATCH 2/6] fix: ease frozen height check within TryFrom for TmClientState --- ...ease-frozen-height-check-ics07-try-from.md | 3 ++ .../types/src/client_state.rs | 26 ++++--------- .../src/fixtures/clients/tendermint.rs | 38 ++++++++++--------- 3 files changed, 31 insertions(+), 36 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md diff --git a/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md b/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md new file mode 100644 index 000000000..29e3d329b --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md @@ -0,0 +1,3 @@ +- [ibc-client-tendermint-types] Ease frozen Height check in the tendermint + ClientState protobuf deserialization + ([\#1061](https://github.com/cosmos/ibc-rs/issues/1061)) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 182096ac2..809f83f7f 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -61,6 +61,7 @@ impl ClientState { latest_height: Height, proof_specs: ProofSpecs, upgrade_path: Vec, + frozen_height: Option, allow_update: AllowUpdate, ) -> Self { Self { @@ -73,11 +74,13 @@ impl ClientState { proof_specs, upgrade_path, allow_update, - frozen_height: None, + frozen_height, verifier: ProdVerifier::default(), } } + /// Constructs a new Tendermint `ClientState` by given parameters and checks + /// if the parameters are valid. #[allow(clippy::too_many_arguments)] pub fn new( chain_id: ChainId, @@ -99,6 +102,7 @@ impl ClientState { latest_height, proof_specs, upgrade_path, + None, // New valid client must not be frozen. allow_update, ); client_state.validate()?; @@ -282,16 +286,7 @@ impl TryFrom for ClientState { // In `RawClientState`, a `frozen_height` of `0` means "not frozen". // See: // https://github.com/cosmos/ibc-go/blob/8422d0c4c35ef970539466c5bdec1cd27369bab3/modules/light-clients/07-tendermint/types/client_state.go#L74 - // - // Comment out this check for now to allow fixing the client state. - // - // if raw - // .frozen_height - // .and_then(|h| Height::try_from(h).ok()) - // .is_some() - // { - // return Err(Error::FrozenHeightNotAllowed); - // } + let frozen_height = raw.frozen_height.and_then(|h| Height::try_from(h).ok()); // We use set this deprecated field just so that we can properly convert // it back in its raw form @@ -301,7 +296,7 @@ impl TryFrom for ClientState { after_misbehaviour: raw.allow_update_after_misbehaviour, }; - let mut client_state = Self::new_without_validation( + let client_state = Self::new_without_validation( chain_id.clone(), trust_level, trusting_period, @@ -310,15 +305,10 @@ impl TryFrom for ClientState { latest_height, raw.proof_specs.into(), raw.upgrade_path, + frozen_height, allow_update, ); - // Restore the frozen height if it was set, allow fixing the client state. - if let Some(frozen_height) = raw.frozen_height { - client_state = client_state.with_frozen_height( - Height::try_from(frozen_height).unwrap_or(Height::min(chain_id.revision_number())), - ); - }; Ok(client_state) } } diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index 03828d78f..cb7e84da9 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -172,13 +172,9 @@ mod tests { use super::*; - #[test] - fn tm_client_state_conversions_healthy() { + fn try_tm_client_state_conversions(frozen_height: RawHeight) { // check client state creation path from a proto type - let tm_client_state_from_raw = dummy_tm_client_state_from_raw(RawHeight { - revision_number: 0, - revision_height: 0, - }); + let tm_client_state_from_raw = dummy_tm_client_state_from_raw(frozen_height); assert!(tm_client_state_from_raw.is_ok()); let any_from_tm_client_state = Any::from( @@ -193,7 +189,25 @@ mod tests { tm_client_state_from_raw.expect("Never fails"), tm_client_state_from_any.expect("Never fails").into() ); + } + + #[test] + fn tm_client_state_conversions_healthy() { + // try conversions for when the client is not frozen + try_tm_client_state_conversions(RawHeight { + revision_number: 0, + revision_height: 0, + }); + // try conversions for when the client is frozen + try_tm_client_state_conversions(RawHeight { + revision_number: 0, + revision_height: 10, + }); + } + + #[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_client_state_from_header = dummy_tm_client_state_from_header(tm_header); @@ -205,16 +219,4 @@ mod tests { tm_client_state_from_any.expect("Never fails").into() ); } - - #[test] - fn tm_client_state_malformed_with_frozen_height() { - let tm_client_state_from_raw = dummy_tm_client_state_from_raw(RawHeight { - revision_number: 0, - revision_height: 10, - }); - match tm_client_state_from_raw { - Err(Error::FrozenHeightNotAllowed) => {} - _ => panic!("Expected to fail with FrozenHeightNotAllowed error"), - } - } } From c1e78b17a861fdc0038a422dd3fc3a8f45d62bef Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 25 Jan 2024 15:08:30 -0800 Subject: [PATCH 3/6] nit: set correct vallue for the frozen height in test --- ibc-clients/ics07-tendermint/src/client_state.rs | 5 +++++ ibc-clients/ics07-tendermint/types/src/client_state.rs | 2 +- ibc-testkit/src/fixtures/clients/tendermint.rs | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/ibc-clients/ics07-tendermint/src/client_state.rs b/ibc-clients/ics07-tendermint/src/client_state.rs index f9c7eb332..f4e4390ba 100644 --- a/ibc-clients/ics07-tendermint/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/src/client_state.rs @@ -401,6 +401,11 @@ where _client_message: Any, _update_kind: &UpdateKind, ) -> Result<(), ClientError> { + // NOTE: frozen height is set to `Height {revision_height: 0, + // revision_number: 1}` and it is the same for all misbehaviour. This + // aligns with the + // [`ibc-go`](https://github.com/cosmos/ibc-go/blob/0e3f428e66d6fc0fc6b10d2f3c658aaa5000daf7/modules/light-clients/07-tendermint/misbehaviour.go#L18-L19) + // implementation. let frozen_client_state = self.0.clone().with_frozen_height(Height::min(0)); let wrapped_frozen_client_state = ClientState::from(frozen_client_state); diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 809f83f7f..1c6001e22 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -297,7 +297,7 @@ impl TryFrom for ClientState { }; let client_state = Self::new_without_validation( - chain_id.clone(), + chain_id, trust_level, trusting_period, unbonding_period, diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index cb7e84da9..fa8e189bf 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -202,7 +202,7 @@ mod tests { // try conversions for when the client is frozen try_tm_client_state_conversions(RawHeight { revision_number: 0, - revision_height: 10, + revision_height: 1, }); } From 351079975795958adcf7fa7f9baf978c6125a928 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 25 Jan 2024 15:13:54 -0800 Subject: [PATCH 4/6] nit --- .../bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md b/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md index 29e3d329b..40e230d61 100644 --- a/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md +++ b/.changelog/unreleased/bug-fixes/1061-ease-frozen-height-check-ics07-try-from.md @@ -1,3 +1,3 @@ - [ibc-client-tendermint-types] Ease frozen Height check in the tendermint - ClientState protobuf deserialization + `ClientState` protobuf deserialization ([\#1061](https://github.com/cosmos/ibc-rs/issues/1061)) From 3bd6494013075c5d7c2b9fb9265d083cda2d3653 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 26 Jan 2024 12:54:11 +0100 Subject: [PATCH 5/6] use rstest cases --- .../src/fixtures/clients/tendermint.rs | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index fa8e189bf..0f0bbcecd 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -169,10 +169,24 @@ pub fn dummy_ics07_header() -> Header { mod tests { use ibc::primitives::proto::Any; + use rstest::rstest; use super::*; - fn try_tm_client_state_conversions(frozen_height: RawHeight) { + #[rstest] + // try conversions for when the client is not frozen + #[case::valid_client(0, 0)] + // try conversions for when the client is frozen + #[case::frozen_client(0, 1)] + fn tm_client_state_conversions_healthy( + #[case] revision_number: u64, + #[case] revision_height: u64, + ) { + let frozen_height = RawHeight { + revision_number, + revision_height, + }; + // check client state creation path from a proto type let tm_client_state_from_raw = dummy_tm_client_state_from_raw(frozen_height); assert!(tm_client_state_from_raw.is_ok()); @@ -191,21 +205,6 @@ mod tests { ); } - #[test] - fn tm_client_state_conversions_healthy() { - // try conversions for when the client is not frozen - try_tm_client_state_conversions(RawHeight { - revision_number: 0, - revision_height: 0, - }); - - // try conversions for when the client is frozen - try_tm_client_state_conversions(RawHeight { - revision_number: 0, - revision_height: 1, - }); - } - #[test] fn tm_client_state_from_header_healthy() { // check client state creation path from a tendermint header From dbcecb1492790829cad525ef1b832c5fa1ceb903 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 26 Jan 2024 12:59:57 +0100 Subject: [PATCH 6/6] add check for is_frozen --- ibc-testkit/src/fixtures/clients/tendermint.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index 0f0bbcecd..39f8de82f 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -175,12 +175,13 @@ mod tests { #[rstest] // try conversions for when the client is not frozen - #[case::valid_client(0, 0)] + #[case::valid_client(0, 0, false)] // try conversions for when the client is frozen - #[case::frozen_client(0, 1)] + #[case::frozen_client(0, 1, true)] fn tm_client_state_conversions_healthy( #[case] revision_number: u64, #[case] revision_height: u64, + #[case] is_frozen: bool, ) { let frozen_height = RawHeight { revision_number, @@ -199,6 +200,13 @@ mod tests { ); let tm_client_state_from_any = ClientStateType::try_from(any_from_tm_client_state); assert!(tm_client_state_from_any.is_ok()); + assert_eq!( + Some(is_frozen), + tm_client_state_from_any + .as_ref() + .map(|x| x.is_frozen()) + .ok() + ); assert_eq!( tm_client_state_from_raw.expect("Never fails"), tm_client_state_from_any.expect("Never fails").into()