Fix upgrade-client issues and refactor relevant tests (#673)

* Fix upgrade-client bugs and refactor relevant tests

* Update ADR06

* Add APIs for storing/retrieving upgraded client/cons states

* Add unclog

* Remove upgrade APIs to be added in a later PR

.changelog/unreleased/bug-fixes/672-fix-upgraded-client-cons-states-encoding.md

@@ -0,0 +1,2 @@
+- Encode upgraded client/consensus states for upgrade_client validation using `prost::Message`
+  from pros ([#672](https://github.com/cosmos/ibc-rs/issues/672))

.changelog/unreleased/improvements/385-refactor-upgrade-client-tests.md

@@ -0,0 +1,2 @@
+- Refactor tests for upgrade_client implementation
+  ([#385](https://github.com/cosmos/ibc-rs/issues/385))

.changelog/unreleased/improvements/671-client-state-conversion-refinement.md

@@ -0,0 +1,2 @@
+- Exclude `ClientState::new()` checks from proto ClientState conversion
+  ([#671](https://github.com/cosmos/ibc-rs/issues/671))

crates/ibc/Cargo.toml

@@ -42,9 +42,6 @@ borsh = ["dep:borsh"]
 # This feature is required for token transfer (ICS-20)
 serde = ["dep:serde", "dep:serde_derive", "serde_json", "erased-serde"]
 
-# This feature guards the unfinished implementation of the `UpgradeClient` handler.
-upgrade_client = []
-
 # This feature grants access to development-time mocking libraries, such as `MockContext` or `MockHeader`.
 # Depends on the `testgen` suite for generating Tendermint light blocks.
 mocks = ["tendermint-testgen", "tendermint/clock", "cfg-if", "parking_lot"]

crates/ibc/src/clients/ics07_tendermint/client_state.rs

@@ -13,11 +13,10 @@ use core::time::Duration;
 use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::core::client::v1::Height as RawHeight;
 use ibc_proto::ibc::core::commitment::v1::{MerklePath, MerkleProof as RawMerkleProof};
-use ibc_proto::ibc::lightclients::tendermint::v1::{
-    ClientState as RawTmClientState, ConsensusState as RawTmConsensusState,
-};
+use ibc_proto::ibc::lightclients::tendermint::v1::ClientState as RawTmClientState;
 use ibc_proto::protobuf::Protobuf;
 use prost::Message;
+
 use tendermint::chain::id::MAX_LENGTH as MaxChainIdLen;
 use tendermint::trust_threshold::TrustThresholdFraction as TendermintTrustThresholdFraction;
 use tendermint_light_client_verifier::options::Options;
@@ -41,13 +40,12 @@ use crate::core::ics23_commitment::merkle::{apply_prefix, MerkleProof};
 use crate::core::ics23_commitment::specs::ProofSpecs;
 use crate::core::ics24_host::identifier::{ChainId, ClientId};
 use crate::core::ics24_host::path::Path;
-use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, ClientUpgradePath};
+use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, UpgradeClientPath};
 use crate::core::timestamp::ZERO_DURATION;
 use crate::Height;
 
 use super::client_type as tm_client_type;
 use super::trust_threshold::TrustThreshold;
-
 use crate::core::{ExecutionContext, ValidationContext};
 
 const TENDERMINT_CLIENT_STATE_TYPE_URL: &str = "/ibc.lightclients.tendermint.v1.ClientState";
@@ -78,6 +76,33 @@ pub struct AllowUpdate {
 }
 
 impl ClientState {
+    #[allow(clippy::too_many_arguments)]
+    fn new_without_validation(
+        chain_id: ChainId,
+        trust_level: TrustThreshold,
+        trusting_period: Duration,
+        unbonding_period: Duration,
+        max_clock_drift: Duration,
+        latest_height: Height,
+        proof_specs: ProofSpecs,
+        upgrade_path: Vec<String>,
+        allow_update: AllowUpdate,
+    ) -> ClientState {
+        Self {
+            chain_id,
+            trust_level,
+            trusting_period,
+            unbonding_period,
+            max_clock_drift,
+            latest_height,
+            proof_specs,
+            upgrade_path,
+            allow_update,
+            frozen_height: None,
+            verifier: ProdVerifier::default(),
+        }
+    }
+
     #[allow(clippy::too_many_arguments)]
     pub fn new(
         chain_id: ChainId,
@@ -90,75 +115,107 @@ impl ClientState {
         upgrade_path: Vec<String>,
         allow_update: AllowUpdate,
     ) -> Result<ClientState, Error> {
-        if chain_id.as_str().len() > MaxChainIdLen {
+        let client_state = Self::new_without_validation(
+            chain_id,
+            trust_level,
+            trusting_period,
+            unbonding_period,
+            max_clock_drift,
+            latest_height,
+            proof_specs,
+            upgrade_path,
+            allow_update,
+        );
+        client_state.validate()?;
+        Ok(client_state)
+    }
+
+    pub fn with_header(self, header: TmHeader) -> Result<Self, Error> {
+        Ok(ClientState {
+            latest_height: max(header.height(), self.latest_height),
+            ..self
+        })
+    }
+
+    pub fn with_frozen_height(self, h: Height) -> Self {
+        Self {
+            frozen_height: Some(h),
+            ..self
+        }
+    }
+
+    pub fn validate(&self) -> Result<(), Error> {
+        if self.chain_id.as_str().len() > MaxChainIdLen {
             return Err(Error::ChainIdTooLong {
-                chain_id: chain_id.clone(),
-                len: chain_id.as_str().len(),
+                chain_id: self.chain_id.clone(),
+                len: self.chain_id.as_str().len(),
                 max_len: MaxChainIdLen,
             });
         }
 
         // `TrustThreshold` is guaranteed to be in the range `[0, 1)`, but a `TrustThreshold::ZERO`
         // value is invalid in this context
-        if trust_level == TrustThreshold::ZERO {
+        if self.trust_level == TrustThreshold::ZERO {
             return Err(Error::InvalidTrustThreshold {
                 reason: "ClientState trust-level cannot be zero".to_string(),
             });
         }
 
-        let _ = TendermintTrustThresholdFraction::new(
-            trust_level.numerator(),
-            trust_level.denominator(),
+        TendermintTrustThresholdFraction::new(
+            self.trust_level.numerator(),
+            self.trust_level.denominator(),
         )
         .map_err(Error::InvalidTendermintTrustThreshold)?;
 
         // Basic validation of trusting period and unbonding period: each should be non-zero.
-        if trusting_period <= Duration::new(0, 0) {
+        if self.trusting_period <= Duration::new(0, 0) {
             return Err(Error::InvalidTrustThreshold {
                 reason: format!(
-                    "ClientState trusting period ({trusting_period:?}) must be greater than zero"
+                    "ClientState trusting period ({:?}) must be greater than zero",
+                    self.trusting_period
                 ),
             });
         }
 
-        if unbonding_period <= Duration::new(0, 0) {
+        if self.unbonding_period <= Duration::new(0, 0) {
             return Err(Error::InvalidTrustThreshold {
                 reason: format!(
-                    "ClientState unbonding period ({unbonding_period:?}) must be greater than zero"
+                    "ClientState unbonding period ({:?}) must be greater than zero",
+                    self.unbonding_period
                 ),
             });
         }
 
-        if trusting_period >= unbonding_period {
+        if self.trusting_period >= self.unbonding_period {
             return Err(Error::InvalidTrustThreshold {
                 reason: format!(
-                "ClientState trusting period ({trusting_period:?}) must be smaller than unbonding period ({unbonding_period:?})"
+                "ClientState trusting period ({:?}) must be smaller than unbonding period ({:?})", self.trusting_period, self.unbonding_period
             ),
             });
         }
 
-        if max_clock_drift <= Duration::new(0, 0) {
+        if self.max_clock_drift <= Duration::new(0, 0) {
             return Err(Error::InvalidMaxClockDrift {
                 reason: "ClientState max-clock-drift must be greater than zero".to_string(),
             });
         }
 
-        if latest_height.revision_number() != chain_id.version() {
+        if self.latest_height.revision_number() != self.chain_id.version() {
             return Err(Error::InvalidLatestHeight {
                 reason: "ClientState latest-height revision number must match chain-id version"
                     .to_string(),
             });
         }
 
         // Disallow empty proof-specs
-        if proof_specs.is_empty() {
+        if self.proof_specs.is_empty() {
             return Err(Error::Validation {
                 reason: "ClientState proof-specs cannot be empty".to_string(),
             });
         }
 
         // `upgrade_path` itself may be empty, but if not then each key must be non-empty
-        for (idx, key) in upgrade_path.iter().enumerate() {
+        for (idx, key) in self.upgrade_path.iter().enumerate() {
             if key.trim().is_empty() {
                 return Err(Error::Validation {
                     reason: format!(
@@ -168,33 +225,7 @@ impl ClientState {
             }
         }
 
-        Ok(Self {
-            chain_id,
-            trust_level,
-            trusting_period,
-            unbonding_period,
-            max_clock_drift,
-            latest_height,
-            proof_specs,
-            upgrade_path,
-            allow_update,
-            frozen_height: None,
-            verifier: ProdVerifier::default(),
-        })
-    }
-
-    pub fn with_header(self, header: TmHeader) -> Result<Self, Error> {
-        Ok(ClientState {
-            latest_height: max(header.height(), self.latest_height),
-            ..self
-        })
-    }
-
-    pub fn with_frozen_height(self, h: Height) -> Self {
-        Self {
-            frozen_height: Some(h),
-            ..self
-        }
+        Ok(())
     }
 
     /// Get the refresh time to ensure the state does not expire
@@ -249,8 +280,8 @@ impl Ics2ClientState for ClientState {
         Ok(())
     }
 
+    // Resets custom fields to zero values (used in `update_client`)
     fn zero_custom_fields(&mut self) {
-        // Reset custom fields to zero values
         self.trusting_period = ZERO_DURATION;
         self.trust_level = TrustThreshold::ZERO;
         self.allow_update.after_expiry = false;
@@ -264,7 +295,13 @@ impl Ics2ClientState for ClientState {
     }
 
     fn initialise(&self, consensus_state: Any) -> Result<Box<dyn ConsensusState>, ClientError> {
-        TmConsensusState::try_from(consensus_state).map(TmConsensusState::into_box)
+        let tm_consensus_state = TmConsensusState::try_from(consensus_state)?;
+        if tm_consensus_state.root().is_empty() {
+            return Err(ClientError::Other {
+                description: "empty commitment root".into(),
+            });
+        };
+        Ok(Box::new(tm_consensus_state))
     }
 
     fn verify_client_message(
@@ -386,10 +423,10 @@ impl Ics2ClientState for ClientState {
         root: &CommitmentRoot,
     ) -> Result<(), ClientError> {
         // Make sure that the client type is of Tendermint type `ClientState`
-        let mut upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state)?;
+        let mut upgraded_tm_client_state = TmClientState::try_from(upgraded_client_state.clone())?;
 
         // Make sure that the consensus type is of Tendermint type `ConsensusState`
-        let upgraded_tm_cons_state = TmConsensusState::try_from(upgraded_consensus_state)?;
+        TmConsensusState::try_from(upgraded_consensus_state.clone())?;
 
         // Note: verification of proofs that unmarshalled correctly has been done
         // while decoding the proto message into a `MsgEnvelope` domain type
@@ -418,15 +455,18 @@ impl Ics2ClientState for ClientState {
 
         // Construct the merkle path for the client state
         let mut client_upgrade_path = upgrade_path.clone();
-        client_upgrade_path.push(ClientUpgradePath::UpgradedClientState(last_height).to_string());
+        client_upgrade_path.push(UpgradeClientPath::UpgradedClientState(last_height).to_string());
 
         let client_upgrade_merkle_path = MerklePath {
             key_path: client_upgrade_path,
         };
 
         upgraded_tm_client_state.zero_custom_fields();
-        let client_state_value =
-            Protobuf::<RawTmClientState>::encode_vec(&upgraded_tm_client_state);
+
+        let mut client_state_value = Vec::new();
+        upgraded_client_state
+            .encode(&mut client_state_value)
+            .map_err(ClientError::Encode)?;
 
         // Verify the proof of the upgraded client state
         merkle_proof_upgrade_client
@@ -442,12 +482,15 @@ impl Ics2ClientState for ClientState {
         // Construct the merkle path for the consensus state
         let mut cons_upgrade_path = upgrade_path;
         cons_upgrade_path
-            .push(ClientUpgradePath::UpgradedClientConsensusState(last_height).to_string());
+            .push(UpgradeClientPath::UpgradedClientConsensusState(last_height).to_string());
         let cons_upgrade_merkle_path = MerklePath {
             key_path: cons_upgrade_path,
         };
 
-        let cons_state_value = Protobuf::<RawTmConsensusState>::encode_vec(&upgraded_tm_cons_state);
+        let mut cons_state_value = Vec::new();
+        upgraded_consensus_state
+            .encode(&mut cons_state_value)
+            .map_err(ClientError::Encode)?;
 
         // Verify the proof of the upgraded consensus state
         merkle_proof_upgrade_cons_state
@@ -657,7 +700,7 @@ impl TryFrom<RawTmClientState> for ClientState {
             after_misbehaviour: raw.allow_update_after_misbehaviour,
         };
 
-        let client_state = ClientState::new(
+        let client_state = ClientState::new_without_validation(
             chain_id,
             trust_level,
             trusting_period,
@@ -667,7 +710,7 @@ impl TryFrom<RawTmClientState> for ClientState {
             raw.proof_specs.into(),
             raw.upgrade_path,
             allow_update,
-        )?;
+        );
 
         Ok(client_state)
     }

crates/ibc/src/clients/ics07_tendermint/consensus_state.rs

@@ -57,11 +57,6 @@ impl TryFrom<RawConsensusState> for ConsensusState {
                 reason: "missing commitment root".into(),
             })?
             .hash;
-        if proto_root.is_empty() {
-            return Err(Error::InvalidRawClientState {
-                reason: "empty commitment root".into(),
-            });
-        };
 
         let ibc_proto::google::protobuf::Timestamp { seconds, nanos } =
             raw.timestamp.ok_or(Error::InvalidRawClientState {