ibc: ⛅ place validate_penumbra_client_state in ics02_validation

nb: this commit strictly contains plain code motion. it does not make any changes to the code being moved. while this was not originally placed in the ics02_validation submodule, it feels like it is more connected to that validation logic than the surrounding client counter types. this bears out to be true; it is `pub`, the client counter code isn't changed, and affected imports are in the message handler area.
penumbra-zone · Jan 10, 2024 · 83f95f1 · 83f95f1
1 parent e1a1bda
commit 83f95f1
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 96 deletions.
diff --git a/crates/core/component/ibc/src/component/client_counter.rs b/crates/core/component/ibc/src/component/client_counter.rs
@@ -1,8 +1,5 @@
-use crate::{component::ics02_validation, IBC_PROOF_SPECS};
-use ibc_proto::google::protobuf::Any;
 use ibc_types::core::client::Height;
-use ibc_types::core::connection::{ChainId, ConnectionId};
-use ibc_types::lightclients::tendermint::TrustThreshold;
+use ibc_types::core::connection::ConnectionId;
 use penumbra_proto::{penumbra::core::component::ibc::v1alpha1 as pb, DomainType};
 
 #[derive(Clone, Debug)]
@@ -89,82 +86,3 @@ impl From<ClientConnections> for pb::ClientConnections {
         }
     }
 }
-
-// Check that the trust threshold is:
-//
-// a) non-zero
-// b) greater or equal to 1/3
-// c) strictly less than 1
-fn validate_trust_threshold(trust_threshold: TrustThreshold) -> anyhow::Result<()> {
-    if trust_threshold.denominator() == 0 {
-        anyhow::bail!("trust threshold denominator cannot be zero");
-    }
-
-    if trust_threshold.numerator() * 3 < trust_threshold.denominator() {
-        anyhow::bail!("trust threshold must be greater than 1/3");
-    }
-
-    if trust_threshold.numerator() >= trust_threshold.denominator() {
-        anyhow::bail!("trust threshold must be strictly less than 1");
-    }
-
-    Ok(())
-}
-
-// validate the parameters of an AnyClientState, verifying that it is a valid Penumbra client
-// state.
-pub fn validate_penumbra_client_state(
-    client_state: Any,
-    chain_id: &str,
-    current_height: u64,
-) -> anyhow::Result<()> {
-    let tm_client_state = ics02_validation::get_tendermint_client_state(client_state)?;
-
-    if tm_client_state.frozen_height.is_some() {
-        anyhow::bail!("invalid client state: frozen");
-    }
-
-    // NOTE: Chain ID validation is actually not standardized yet. see
-    // https://github.com/informalsystems/ibc-rs/pull/304#discussion_r503917283
-    let chain_id = ChainId::from_string(chain_id);
-    if chain_id != tm_client_state.chain_id {
-        anyhow::bail!("invalid client state: chain id does not match");
-    }
-
-    // check that the revision number is the same as our chain ID's version
-    if tm_client_state.latest_height().revision_number() != chain_id.version() {
-        anyhow::bail!("invalid client state: revision number does not match");
-    }
-
-    // check that the latest height isn't gte the current block height
-    if tm_client_state.latest_height().revision_height() >= current_height {
-        anyhow::bail!(
-            "invalid client state: latest height is greater than or equal to the current block height"
-        );
-    }
-
-    // check client proof specs match penumbra proof specs
-    if IBC_PROOF_SPECS.clone() != tm_client_state.proof_specs {
-        // allow legacy proof specs without prehash_key_before_comparison
-        let mut spec_with_prehash_key = tm_client_state.proof_specs.clone();
-        spec_with_prehash_key[0].prehash_key_before_comparison = true;
-        spec_with_prehash_key[1].prehash_key_before_comparison = true;
-        if IBC_PROOF_SPECS.clone() != spec_with_prehash_key {
-            anyhow::bail!("invalid client state: proof specs do not match");
-        }
-    }
-
-    // check that the trust level is correct
-    validate_trust_threshold(tm_client_state.trust_level)?;
-
-    // TODO: check that the unbonding period is correct
-    //
-    // - check unbonding period is greater than trusting period
-    if tm_client_state.unbonding_period < tm_client_state.trusting_period {
-        anyhow::bail!("invalid client state: unbonding period is less than trusting period");
-    }
-
-    // TODO: check upgrade path
-
-    Ok(())
-}
diff --git a/crates/core/component/ibc/src/component/ics02_validation.rs b/crates/core/component/ibc/src/component/ics02_validation.rs
@@ -1,16 +1,17 @@
+use crate::IBC_PROOF_SPECS;
 use anyhow::{anyhow, Result};
 use ibc_proto::google::protobuf::Any;
-use ibc_types::lightclients::tendermint::client_state::{
-    ClientState as TendermintClientState, TENDERMINT_CLIENT_STATE_TYPE_URL,
-};
-use ibc_types::lightclients::tendermint::consensus_state::{
-    ConsensusState as TendermintConsensusState, TENDERMINT_CONSENSUS_STATE_TYPE_URL,
-};
-use ibc_types::lightclients::tendermint::header::{
-    Header as TendermintHeader, TENDERMINT_HEADER_TYPE_URL,
-};
-use ibc_types::lightclients::tendermint::misbehaviour::{
-    Misbehaviour as TendermintMisbehavior, TENDERMINT_MISBEHAVIOUR_TYPE_URL,
+use ibc_types::{
+    core::connection::ChainId,
+    lightclients::tendermint::{
+        client_state::{ClientState as TendermintClientState, TENDERMINT_CLIENT_STATE_TYPE_URL},
+        consensus_state::{
+            ConsensusState as TendermintConsensusState, TENDERMINT_CONSENSUS_STATE_TYPE_URL,
+        },
+        header::{Header as TendermintHeader, TENDERMINT_HEADER_TYPE_URL},
+        misbehaviour::{Misbehaviour as TendermintMisbehavior, TENDERMINT_MISBEHAVIOUR_TYPE_URL},
+        TrustThreshold,
+    },
 };
 
 pub fn is_tendermint_header_state(header: &Any) -> bool {
@@ -78,3 +79,82 @@ pub fn get_tendermint_client_state(client_state: Any) -> Result<TendermintClient
         ))
     }
 }
+
+// validate the parameters of an AnyClientState, verifying that it is a valid Penumbra client
+// state.
+pub fn validate_penumbra_client_state(
+    client_state: Any,
+    chain_id: &str,
+    current_height: u64,
+) -> anyhow::Result<()> {
+    let tm_client_state = get_tendermint_client_state(client_state)?;
+
+    if tm_client_state.frozen_height.is_some() {
+        anyhow::bail!("invalid client state: frozen");
+    }
+
+    // NOTE: Chain ID validation is actually not standardized yet. see
+    // https://github.com/informalsystems/ibc-rs/pull/304#discussion_r503917283
+    let chain_id = ChainId::from_string(chain_id);
+    if chain_id != tm_client_state.chain_id {
+        anyhow::bail!("invalid client state: chain id does not match");
+    }
+
+    // check that the revision number is the same as our chain ID's version
+    if tm_client_state.latest_height().revision_number() != chain_id.version() {
+        anyhow::bail!("invalid client state: revision number does not match");
+    }
+
+    // check that the latest height isn't gte the current block height
+    if tm_client_state.latest_height().revision_height() >= current_height {
+        anyhow::bail!(
+            "invalid client state: latest height is greater than or equal to the current block height"
+        );
+    }
+
+    // check client proof specs match penumbra proof specs
+    if IBC_PROOF_SPECS.clone() != tm_client_state.proof_specs {
+        // allow legacy proof specs without prehash_key_before_comparison
+        let mut spec_with_prehash_key = tm_client_state.proof_specs.clone();
+        spec_with_prehash_key[0].prehash_key_before_comparison = true;
+        spec_with_prehash_key[1].prehash_key_before_comparison = true;
+        if IBC_PROOF_SPECS.clone() != spec_with_prehash_key {
+            anyhow::bail!("invalid client state: proof specs do not match");
+        }
+    }
+
+    // check that the trust level is correct
+    validate_trust_threshold(tm_client_state.trust_level)?;
+
+    // TODO: check that the unbonding period is correct
+    //
+    // - check unbonding period is greater than trusting period
+    if tm_client_state.unbonding_period < tm_client_state.trusting_period {
+        anyhow::bail!("invalid client state: unbonding period is less than trusting period");
+    }
+
+    // TODO: check upgrade path
+
+    Ok(())
+}
+
+// Check that the trust threshold is:
+//
+// a) non-zero
+// b) greater or equal to 1/3
+// c) strictly less than 1
+fn validate_trust_threshold(trust_threshold: TrustThreshold) -> anyhow::Result<()> {
+    if trust_threshold.denominator() == 0 {
+        anyhow::bail!("trust threshold denominator cannot be zero");
+    }
+
+    if trust_threshold.numerator() * 3 < trust_threshold.denominator() {
+        anyhow::bail!("trust threshold must be greater than 1/3");
+    }
+
+    if trust_threshold.numerator() >= trust_threshold.denominator() {
+        anyhow::bail!("trust threshold must be strictly less than 1");
+    }
+
+    Ok(())
+}
diff --git a/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs b/crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs
@@ -12,8 +12,8 @@ use penumbra_chain::component::StateReadExt as _;
 use crate::{
     component::{
         client::StateReadExt as _,
-        client_counter::validate_penumbra_client_state,
         connection::{StateReadExt as _, StateWriteExt as _},
+        ics02_validation::validate_penumbra_client_state,
         proof_verification, MsgHandler,
     },
     IBC_COMMITMENT_PREFIX,

diff --git a/crates/core/component/ibc/src/component/msg_handler/connection_open_try.rs b/crates/core/component/ibc/src/component/msg_handler/connection_open_try.rs
@@ -17,9 +17,9 @@ use penumbra_chain::component::StateReadExt as _;
 
 use crate::component::{
     client::StateReadExt as _,
-    client_counter::validate_penumbra_client_state,
     connection::{StateReadExt as _, StateWriteExt as _},
     connection_counter::SUPPORTED_VERSIONS,
+    ics02_validation::validate_penumbra_client_state,
     MsgHandler,
 };