fix(ibc-client-tendermint-types): drop frozen height check in TryFrom of TmClientState (#1062)

* Fix issues related to frozen height of tendermint client.

* fix: ease frozen height check within TryFrom for TmClientState

* nit: set correct vallue for the frozen height in test

* nit

* use rstest cases

* add check for is_frozen

---------

Co-authored-by: Rivers Yang <[email protected]>
Co-authored-by: Ranadeep Biswas <[email protected]>

Author: 3 people

.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))

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);

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<String>,
+        frozen_height: Option<Height>,
         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<RawTmClientState> 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<RawTmClientState> for ClientState {
             latest_height,
             raw.proof_specs.into(),
             raw.upgrade_path,
+            frozen_height,
             allow_update,
         );
 
@@ -324,12 +322,7 @@ impl From<ClientState> 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,

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"),
-        }
-    }
 }