From 83f95f120a8b801b5e8c239295e44a1a04078472 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Wed, 10 Jan 2024 09:14:41 -0500 Subject: [PATCH] =?UTF-8?q?ibc:=20=E2=9B=85=20place=20validate=5Fpenumbra?= =?UTF-8?q?=5Fclient=5Fstate=20in=20ics02=5Fvalidation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../ibc/src/component/client_counter.rs | 84 +-------------- .../ibc/src/component/ics02_validation.rs | 102 ++++++++++++++++-- .../msg_handler/connection_open_ack.rs | 2 +- .../msg_handler/connection_open_try.rs | 2 +- 4 files changed, 94 insertions(+), 96 deletions(-) diff --git a/crates/core/component/ibc/src/component/client_counter.rs b/crates/core/component/ibc/src/component/client_counter.rs index 5b0a7b08b3..70d0673498 100644 --- 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 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 index 82f22bf470..144f4a7abe 100644 --- 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 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 index 7dbddbff96..1b3757ec6d 100644 --- 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 index 6f83273c1d..e7c5b12d09 100644 --- 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, };