Connection proofs refactor (#170)

* conn_open_confirm refactor

* conn_open_ack handler

* disable mbt

* ConnOpenConfirm handler

Remove connection::verify.rs

* conn_open_ack: readability

* remove ConnectionOpenTry::with_previous_connection_id

* Make previous_connection_id deprecated

* remove mbt

* Remove hold_oldest_height check

* changelog

* Output logs for handlers again

* conn_open_ack: apply naming convention

* conn_open_try apply naming convention

* conn_open_init apply naming convention

* conn_open_try naming fix

* conn_open_confirm apply naming convention

* Document naming convention

* fmt

* comments

* Update crates/ibc/src/core/ics03_connection/handler.rs

Co-authored-by: Shoaib Ahmed <[email protected]>
Signed-off-by: Philippe Laferrière <[email protected]>

* Move naming convention docstring

* fix

* update deprecated annotation

Signed-off-by: Philippe Laferrière <[email protected]>
Co-authored-by: Shoaib Ahmed <[email protected]>

.changelog/unreleased/breaking-changes/156-connection-crossing-hellos

@@ -0,0 +1,3 @@
+- Remove crossing hellos logic from connection handshake. Breaking changes in 
+  connection message types.
+  ([#156](https://github.com/cosmos/ibc-rs/issues/156)).

.changelog/unreleased/bug-fixes/168-consensus-height.md

@@ -0,0 +1,2 @@
+- Connection consensus state proof verification now properly uses `consensus_height`
+  ([#168](https://github.com/cosmos/ibc-rs/issues/168)).

.changelog/unreleased/improvements/165-refactor-connection-proofs

@@ -0,0 +1,2 @@
+- Refactor connection proofs
+  ([#165](https://github.com/cosmos/ibc-rs/issues/165)).

crates/ibc/Cargo.toml

@@ -72,8 +72,3 @@ modelator = "0.4.2"
 sha2 = { version = "0.10.6" }
 tendermint-rpc = { version = "=0.25.0", features = ["http-client", "websocket-client"] }
 tendermint-testgen = { version = "=0.25.0" } # Needed for generating (synthetic) light blocks.
-
-[[test]]
-name = "mbt"
-path = "tests/mbt.rs"
-required-features = ["mocks"]

crates/ibc/src/core/ics02_client/client_state.rs

@@ -101,11 +101,11 @@ pub trait ClientState:
     #[allow(clippy::too_many_arguments)]
     fn verify_client_consensus_state(
         &self,
-        height: Height,
-        prefix: &CommitmentPrefix,
+        proof_height: Height,
+        counterparty_prefix: &CommitmentPrefix,
         proof: &CommitmentProofBytes,
         root: &CommitmentRoot,
-        client_id: &ClientId,
+        counterparty_client_id: &ClientId,
         consensus_height: Height,
         expected_consensus_state: &dyn ConsensusState,
     ) -> Result<(), Error>;
@@ -114,11 +114,11 @@ pub trait ClientState:
     #[allow(clippy::too_many_arguments)]
     fn verify_connection_state(
         &self,
-        height: Height,
-        prefix: &CommitmentPrefix,
+        proof_height: Height,
+        counterparty_prefix: &CommitmentPrefix,
         proof: &CommitmentProofBytes,
         root: &CommitmentRoot,
-        connection_id: &ConnectionId,
+        counterparty_connection_id: &ConnectionId,
         expected_connection_end: &ConnectionEnd,
     ) -> Result<(), Error>;
 
@@ -139,8 +139,8 @@ pub trait ClientState:
     #[allow(clippy::too_many_arguments)]
     fn verify_client_full_state(
         &self,
-        height: Height,
-        prefix: &CommitmentPrefix,
+        proof_height: Height,
+        counterparty_prefix: &CommitmentPrefix,
         proof: &CommitmentProofBytes,
         root: &CommitmentRoot,
         client_id: &ClientId,

crates/ibc/src/core/ics03_connection/context.rs

@@ -6,14 +6,16 @@ use crate::core::ics02_client::client_state::ClientState;
 use crate::core::ics02_client::consensus_state::ConsensusState;
 use crate::core::ics03_connection::connection::ConnectionEnd;
 use crate::core::ics03_connection::error::Error;
-use crate::core::ics03_connection::handler::{ConnectionIdState, ConnectionResult};
+use crate::core::ics03_connection::handler::ConnectionResult;
 use crate::core::ics03_connection::version::{get_compatible_versions, pick_version, Version};
 use crate::core::ics23_commitment::commitment::CommitmentPrefix;
 use crate::core::ics24_host::identifier::{ClientId, ConnectionId};
 use crate::prelude::*;
 use crate::Height;
 use ibc_proto::google::protobuf::Any;
 
+use super::handler::ConnectionIdState;
+
 /// A context supplying all the necessary read-only dependencies for processing any `ConnectionMsg`.
 pub trait ConnectionReader {
     /// Returns the ConnectionEnd for the given identifier `conn_id`.
@@ -28,6 +30,7 @@ pub trait ConnectionReader {
     /// Returns the current height of the local chain.
     fn host_current_height(&self) -> Height;
 
+    #[deprecated(since = "0.20.0")]
     /// Returns the oldest height available on the local chain.
     fn host_oldest_height(&self) -> Height;
 

crates/ibc/src/core/ics03_connection/handler.rs

@@ -1,4 +1,5 @@
-//! This module implements the processing logic for ICS3 (connection open handshake) messages.
+//! This module implements the processing logic for ICS3 (connection open
+//! handshake) messages.
 
 use crate::core::ics03_connection::connection::ConnectionEnd;
 use crate::core::ics03_connection::context::ConnectionReader;
@@ -12,8 +13,6 @@ pub mod conn_open_confirm;
 pub mod conn_open_init;
 pub mod conn_open_try;
 
-pub mod verify;
-
 /// Defines the possible states of a connection identifier in a `ConnectionResult`.
 #[derive(Clone, Debug)]
 pub enum ConnectionIdState {

crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs

@@ -4,92 +4,135 @@ use crate::core::ics03_connection::connection::{ConnectionEnd, Counterparty, Sta
 use crate::core::ics03_connection::context::ConnectionReader;
 use crate::core::ics03_connection::error::Error;
 use crate::core::ics03_connection::events::Attributes;
-use crate::core::ics03_connection::handler::verify::{
-    check_client_consensus_height, verify_proofs,
-};
-use crate::core::ics03_connection::handler::{ConnectionIdState, ConnectionResult};
+use crate::core::ics03_connection::handler::ConnectionResult;
 use crate::core::ics03_connection::msgs::conn_open_ack::MsgConnectionOpenAck;
 use crate::events::IbcEvent;
 use crate::handler::{HandlerOutput, HandlerResult};
 use crate::prelude::*;
 
+use super::ConnectionIdState;
+
+/// Per our convention, this message is processed on chain A.
 pub(crate) fn process(
-    ctx: &dyn ConnectionReader,
+    ctx_a: &dyn ConnectionReader,
     msg: MsgConnectionOpenAck,
 ) -> HandlerResult<ConnectionResult, Error> {
     let mut output = HandlerOutput::builder();
 
-    // If a consensus proof is present, check that the consensus height (for
-    // client proof) in the message is not too advanced nor too old.
-    if let Some(consensus_height) = msg.consensus_height() {
-        check_client_consensus_height(ctx, consensus_height)?;
+    if msg.consensus_height_of_a_on_b > ctx_a.host_current_height() {
+        return Err(Error::invalid_consensus_height(
+            msg.consensus_height_of_a_on_b,
+            ctx_a.host_current_height(),
+        ));
     }
 
-    // Validate the connection end.
-    let mut conn_end = ctx.connection_end(&msg.connection_id)?;
-    // A connection end must be Init or TryOpen; otherwise we return an error.
-    let state_is_consistent = conn_end.state_matches(&State::Init)
-        && conn_end.versions().contains(&msg.version)
-        || conn_end.state_matches(&State::TryOpen)
-            && conn_end.versions().get(0).eq(&Some(&msg.version));
-
-    if !state_is_consistent {
-        // Old connection end is in incorrect state, propagate the error.
-        return Err(Error::connection_mismatch(msg.connection_id));
-    }
+    ///////////////////////////////////////////////////////////
+    // validate_self_client() verification goes here
+    // See [issue](https://github.com/cosmos/ibc-rs/issues/162)
+    ///////////////////////////////////////////////////////////
 
-    // Set the connection ID of the counterparty
-    let prev_counterparty = conn_end.counterparty();
-    let counterparty = Counterparty::new(
-        prev_counterparty.client_id().clone(),
-        Some(msg.counterparty_connection_id.clone()),
-        prev_counterparty.prefix().clone(),
-    );
-    conn_end.set_state(State::Open);
-    conn_end.set_version(msg.version.clone());
-    conn_end.set_counterparty(counterparty);
+    let conn_end_on_a = ctx_a.connection_end(&msg.conn_id_on_a)?;
+    if !(conn_end_on_a.state_matches(&State::Init)
+        && conn_end_on_a.versions().contains(&msg.version))
+    {
+        return Err(Error::connection_mismatch(msg.conn_id_on_a));
+    }
 
     // Proof verification.
-    let expected_conn = {
-        // The counterparty is the local chain.
-        let counterparty = Counterparty::new(
-            conn_end.client_id().clone(),    // The local client identifier.
-            Some(msg.connection_id.clone()), // This chain's connection id as known on counterparty.
-            ctx.commitment_prefix(),         // Local commitment prefix.
-        );
+    {
+        let client_state_of_b_on_a = ctx_a.client_state(conn_end_on_a.client_id())?;
+        let consensus_state_of_b_on_a =
+            ctx_a.client_consensus_state(conn_end_on_a.client_id(), msg.proofs_height_on_b)?;
 
-        ConnectionEnd::new(
-            State::TryOpen,
-            conn_end.counterparty().client_id().clone(),
-            counterparty,
-            vec![msg.version.clone()],
-            conn_end.delay_period(),
-        )
-    };
+        let prefix_on_a = ctx_a.commitment_prefix();
+        let prefix_on_b = conn_end_on_a.counterparty().prefix();
+        let client_id_on_a = conn_end_on_a.client_id();
+        let client_id_on_b = conn_end_on_a.counterparty().client_id();
+
+        {
+            let conn_id_on_b = conn_end_on_a
+                .counterparty()
+                .connection_id()
+                .ok_or_else(Error::invalid_counterparty)?;
+            let expected_conn_end_on_b = ConnectionEnd::new(
+                State::TryOpen,
+                client_id_on_b.clone(),
+                Counterparty::new(
+                    client_id_on_a.clone(),
+                    Some(msg.conn_id_on_a.clone()),
+                    prefix_on_a,
+                ),
+                vec![msg.version.clone()],
+                conn_end_on_a.delay_period(),
+            );
+
+            client_state_of_b_on_a
+                .verify_connection_state(
+                    msg.proofs_height_on_b,
+                    prefix_on_b,
+                    &msg.proof_conn_end_on_b,
+                    consensus_state_of_b_on_a.root(),
+                    conn_id_on_b,
+                    &expected_conn_end_on_b,
+                )
+                .map_err(Error::verify_connection_state)?;
+        }
 
-    // 2. Pass the details to the verification function.
-    verify_proofs(
-        ctx,
-        msg.client_state,
-        msg.proofs.height(),
-        &conn_end,
-        &expected_conn,
-        &msg.proofs,
-    )?;
-
-    output.log("success: connection verification passed");
-
-    let result = ConnectionResult {
-        connection_id: msg.connection_id,
-        connection_id_state: ConnectionIdState::Reused,
-        connection_end: conn_end,
+        client_state_of_b_on_a
+            .verify_client_full_state(
+                msg.proofs_height_on_b,
+                prefix_on_b,
+                &msg.proof_client_state_of_a_on_b,
+                consensus_state_of_b_on_a.root(),
+                client_id_on_b,
+                msg.client_state_of_a_on_b,
+            )
+            .map_err(|e| {
+                Error::client_state_verification_failure(conn_end_on_a.client_id().clone(), e)
+            })?;
+
+        let expected_consensus_state_of_a_on_b =
+            ctx_a.host_consensus_state(msg.consensus_height_of_a_on_b)?;
+        client_state_of_b_on_a
+            .verify_client_consensus_state(
+                msg.proofs_height_on_b,
+                prefix_on_b,
+                &msg.proof_consensus_state_of_a_on_b,
+                consensus_state_of_b_on_a.root(),
+                conn_end_on_a.counterparty().client_id(),
+                msg.consensus_height_of_a_on_b,
+                expected_consensus_state_of_a_on_b.as_ref(),
+            )
+            .map_err(|e| Error::consensus_state_verification_failure(msg.proofs_height_on_b, e))?;
+    }
+
+    // Success
+    let result = {
+        let new_conn_end_on_a = {
+            let mut counterparty = conn_end_on_a.counterparty().clone();
+            counterparty.connection_id = Some(msg.conn_id_on_b.clone());
+
+            let mut new_conn_end_on_a = conn_end_on_a;
+            new_conn_end_on_a.set_state(State::Open);
+            new_conn_end_on_a.set_version(msg.version.clone());
+            new_conn_end_on_a.set_counterparty(counterparty);
+            new_conn_end_on_a
+        };
+
+        ConnectionResult {
+            connection_id: msg.conn_id_on_a,
+            connection_id_state: ConnectionIdState::Reused,
+            connection_end: new_conn_end_on_a,
+        }
     };
 
     let event_attributes = Attributes {
         connection_id: Some(result.connection_id.clone()),
         ..Default::default()
     };
+
     output.emit(IbcEvent::OpenAckConnection(event_attributes.into()));
+    output.log("success: conn_open_ack verification passed");
 
     Ok(output.with_result(result))
 }
@@ -126,12 +169,12 @@ mod tests {
 
         let msg_ack =
             MsgConnectionOpenAck::try_from(get_dummy_raw_msg_conn_open_ack(10, 10)).unwrap();
-        let conn_id = msg_ack.connection_id.clone();
-        let counterparty_conn_id = msg_ack.counterparty_connection_id.clone();
+        let conn_id = msg_ack.conn_id_on_a.clone();
+        let counterparty_conn_id = msg_ack.conn_id_on_b.clone();
 
         // Client parameters -- identifier and correct height (matching the proof height)
         let client_id = ClientId::from_str("mock_clientid").unwrap();
-        let proof_height = msg_ack.proofs.height();
+        let proof_height = msg_ack.proofs_height_on_b;
 
         // Parametrize the host chain to have a height at least as recent as the
         // the height of the proofs in the Ack msg.
@@ -150,7 +193,7 @@ mod tests {
             client_id.clone(),
             Counterparty::new(
                 client_id.clone(),
-                Some(msg_ack.counterparty_connection_id.clone()),
+                Some(msg_ack.conn_id_on_b.clone()),
                 CommitmentPrefix::try_from(b"ibc".to_vec()).unwrap(),
             ),
             vec![msg_ack.version.clone()],