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..40e230d61 --- /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/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 b1f755711..1c6001e22 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; @@ -62,6 +61,7 @@ impl ClientState { latest_height: Height, proof_specs: ProofSpecs, upgrade_path: Vec, + frozen_height: Option, allow_update: AllowUpdate, ) -> Self { Self { @@ -74,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, @@ -100,6 +102,7 @@ impl ClientState { latest_height, proof_specs, upgrade_path, + None, // New valid client must not be frozen. allow_update, ); client_state.validate()?; @@ -283,13 +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 - 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 @@ -308,6 +305,7 @@ impl TryFrom for ClientState { latest_height, raw.proof_specs.into(), raw.upgrade_path, + frozen_height, allow_update, ); @@ -324,12 +322,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, diff --git a/ibc-testkit/src/fixtures/clients/tendermint.rs b/ibc-testkit/src/fixtures/clients/tendermint.rs index 03828d78f..39f8de82f 100644 --- a/ibc-testkit/src/fixtures/clients/tendermint.rs +++ b/ibc-testkit/src/fixtures/clients/tendermint.rs @@ -169,16 +169,27 @@ pub fn dummy_ics07_header() -> Header { mod tests { use ibc::primitives::proto::Any; + use rstest::rstest; use super::*; - #[test] - fn tm_client_state_conversions_healthy() { + #[rstest] + // try conversions for when the client is not frozen + #[case::valid_client(0, 0, false)] + // try conversions for when the client is frozen + #[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, + revision_height, + }; + // 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( @@ -189,11 +200,21 @@ 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() ); + } + #[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 +226,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"), - } - } }