Refactor and fix version validation in connection and channel handsha…

…kes (#626) * Streamline codebase around Version validation * Fix versions() check * Add a docstring for versions() check * Simplify version check in conn_open_ack * Remove unnecessary for * Resolve a minor issue * Further pruning * favor slices to `Vec` * Mend feature checks * Fix clippy * Allow empty version for on_chan_open_init_validate * Fix pick() test * Modify pick_version * Edit changelog description * name changes * Move version length check into the new ConnectionEnd constructor * Revert Versions signature * docstring * remove redundant function --------- Co-authored-by: Philippe Laferrière <plafer@protonmail.com>
cosmos · Apr 24, 2023 · fd282cb · fd282cb
1 parent 1b3ad8a
commit fd282cb
Show file tree

Hide file tree

Showing 34 changed files with 226 additions and 140 deletions.
diff --git a/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md b/.changelog/unreleased/breaking-changes/625-refactor-version-validation.md
@@ -0,0 +1,2 @@
+- Refactor and fix version validation in connection and channel handshakes
+([#625](https://github.com/cosmos/ibc-rs/issues/625))
diff --git a/crates/ibc/src/applications/transfer/context.rs b/crates/ibc/src/applications/transfer/context.rs
@@ -1,3 +1,4 @@
+use crate::core::ContextError;
 use crate::prelude::*;
 
 use sha2::{Digest, Sha256};
@@ -134,11 +135,10 @@ pub fn on_chan_open_init_validate(
         });
     }
 
-    if !version.is_empty() && version != &Version::new(VERSION.to_string()) {
-        return Err(TokenTransferError::InvalidVersion {
-            expect_version: Version::new(VERSION.to_string()),
-            got_version: version.clone(),
-        });
+    if !version.is_empty() {
+        version
+            .verify_is_expected(Version::new(VERSION.to_string()))
+            .map_err(ContextError::from)?;
     }
 
     Ok(())
@@ -173,12 +173,10 @@ pub fn on_chan_open_try_validate(
             got_order: order,
         });
     }
-    if counterparty_version != &Version::new(VERSION.to_string()) {
-        return Err(TokenTransferError::InvalidCounterpartyVersion {
-            expect_version: Version::new(VERSION.to_string()),
-            got_version: counterparty_version.clone(),
-        });
-    }
+
+    counterparty_version
+        .verify_is_expected(Version::new(VERSION.to_string()))
+        .map_err(ContextError::from)?;
 
     Ok(())
 }
@@ -202,12 +200,9 @@ pub fn on_chan_open_ack_validate(
     _channel_id: &ChannelId,
     counterparty_version: &Version,
 ) -> Result<(), TokenTransferError> {
-    if counterparty_version != &Version::new(VERSION.to_string()) {
-        return Err(TokenTransferError::InvalidCounterpartyVersion {
-            expect_version: Version::new(VERSION.to_string()),
-            got_version: counterparty_version.clone(),
-        });
-    }
+    counterparty_version
+        .verify_is_expected(Version::new(VERSION.to_string()))
+        .map_err(ContextError::from)?;
 
     Ok(())
 }

diff --git a/crates/ibc/src/applications/transfer/error.rs b/crates/ibc/src/applications/transfer/error.rs
@@ -5,7 +5,6 @@ use ibc_proto::protobuf::Error as TendermintProtoError;
 use uint::FromDecStrErr;
 
 use crate::core::ics04_channel::channel::Order;
-use crate::core::ics04_channel::Version;
 use crate::core::ics24_host::error::ValidationError;
 use crate::core::ics24_host::identifier::{ChannelId, PortId};
 use crate::core::ContextError;
@@ -60,16 +59,6 @@ pub enum TokenTransferError {
         expect_order: Order,
         got_order: Order,
     },
-    /// expected version `{expect_version}` , got `{got_version}`
-    InvalidVersion {
-        expect_version: Version,
-        got_version: Version,
-    },
-    /// expected counterparty version `{expect_version}`, got `{got_version}`
-    InvalidCounterpartyVersion {
-        expect_version: Version,
-        got_version: Version,
-    },
     /// channel cannot be closed
     CantCloseChannel,
     /// failed to deserialize packet data
@@ -132,3 +121,9 @@ impl From<Infallible> for TokenTransferError {
         match e {}
     }
 }
+
+impl From<ContextError> for TokenTransferError {
+    fn from(err: ContextError) -> TokenTransferError {
+        Self::ContextError(err)
+    }
+}
diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs
@@ -317,11 +317,13 @@ pub trait ValidationContext: Router {
     /// connection handshake protocol prefers.
     fn pick_version(
         &self,
-        supported_versions: &[ConnectionVersion],
         counterparty_candidate_versions: &[ConnectionVersion],
     ) -> Result<ConnectionVersion, ContextError> {
-        pick_version(supported_versions, counterparty_candidate_versions)
-            .map_err(ContextError::ConnectionError)
+        let version = pick_version(
+            &self.get_compatible_versions(),
+            counterparty_candidate_versions,
+        )?;
+        Ok(version)
     }
 
     /// Returns the ChannelEnd for the given `port_id` and `chan_id`.

diff --git a/crates/ibc/src/core/context/acknowledgement.rs b/crates/ibc/src/core/context/acknowledgement.rs
@@ -206,7 +206,8 @@ mod tests {
             ),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         Fixture {
             ctx,

diff --git a/crates/ibc/src/core/context/chan_close_confirm.rs b/crates/ibc/src/core/context/chan_close_confirm.rs
@@ -135,7 +135,8 @@ mod tests {
             ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         let msg_chan_close_confirm = MsgChannelCloseConfirm::try_from(
             get_dummy_raw_msg_chan_close_confirm(client_consensus_state_height.revision_height()),

diff --git a/crates/ibc/src/core/context/chan_close_init.rs b/crates/ibc/src/core/context/chan_close_init.rs
@@ -133,7 +133,8 @@ mod tests {
             ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         let msg_chan_close_init =
             MsgChannelCloseInit::try_from(get_dummy_raw_msg_chan_close_init()).unwrap();

diff --git a/crates/ibc/src/core/context/chan_open_ack.rs b/crates/ibc/src/core/context/chan_open_ack.rs
@@ -149,7 +149,8 @@ mod tests {
             ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         let msg =
             MsgChannelOpenAck::try_from(get_dummy_raw_msg_chan_open_ack(proof_height)).unwrap();

diff --git a/crates/ibc/src/core/context/chan_open_confirm.rs b/crates/ibc/src/core/context/chan_open_confirm.rs
@@ -156,7 +156,8 @@ mod tests {
             ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         let msg =
             MsgChannelOpenConfirm::try_from(get_dummy_raw_msg_chan_open_confirm(proof_height))

diff --git a/crates/ibc/src/core/context/chan_open_init.rs b/crates/ibc/src/core/context/chan_open_init.rs
@@ -162,7 +162,8 @@ mod tests {
             msg_conn_init.counterparty.clone(),
             get_compatible_versions(),
             msg_conn_init.delay_period,
-        );
+        )
+        .unwrap();
 
         Fixture {
             context,

diff --git a/crates/ibc/src/core/context/chan_open_try.rs b/crates/ibc/src/core/context/chan_open_try.rs
@@ -170,7 +170,8 @@ mod tests {
             ConnectionCounterparty::try_from(get_dummy_raw_counterparty(Some(0))).unwrap(),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         // We're going to test message processing against this message.
         // Note: we make the counterparty's channel_id `None`.

diff --git a/crates/ibc/src/core/context/recv_packet.rs b/crates/ibc/src/core/context/recv_packet.rs
@@ -224,7 +224,8 @@ mod tests {
             ),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         Fixture {
             context,

diff --git a/crates/ibc/src/core/context/timeout.rs b/crates/ibc/src/core/context/timeout.rs
@@ -238,7 +238,8 @@ mod tests {
             ),
             get_compatible_versions(),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         Fixture {
             ctx,

diff --git a/crates/ibc/src/core/handler.rs b/crates/ibc/src/core/handler.rs
@@ -524,7 +524,8 @@ mod tests {
                     ),
                     vec![ConnVersion::default()],
                     Duration::MAX,
-                ),
+                )
+                .unwrap(),
             );
         let module = DummyTransferModule::new();
 

diff --git a/crates/ibc/src/core/ics03_connection/connection.rs b/crates/ibc/src/core/ics03_connection/connection.rs
@@ -227,7 +227,7 @@ impl TryFrom<RawConnectionEnd> for ConnectionEnd {
             return Err(ConnectionError::EmptyProtoConnectionEnd);
         }
 
-        Ok(Self::new(
+        Self::new(
             state,
             value
                 .client_id
@@ -243,7 +243,7 @@ impl TryFrom<RawConnectionEnd> for ConnectionEnd {
                 .map(Version::try_from)
                 .collect::<Result<Vec<_>, _>>()?,
             Duration::from_nanos(value.delay_period),
-        ))
+        )
     }
 }
 
@@ -270,14 +270,21 @@ impl ConnectionEnd {
         counterparty: Counterparty,
         versions: Vec<Version>,
         delay_period: Duration,
-    ) -> Self {
-        Self {
+    ) -> Result<Self, ConnectionError> {
+        // Note: `versions`'s semantics vary based on the `State` of the connection:
+        // + Init: contains the set of compatible versions,
+        // + TryOpen/Open: contains the single version chosen by the handshake protocol.
+        if state != State::Init && versions.len() != 1 {
+            return Err(ConnectionError::InvalidVersionLength);
+        }
+
+        Ok(Self {
             state,
             client_id,
             counterparty,
             versions,
             delay_period,
-        }
+        })
     }
 
     /// Getter for the state of this connection end.

diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs
@@ -31,12 +31,18 @@ pub enum ConnectionError {
     EmptyProtoConnectionEnd,
     /// empty supported versions
     EmptyVersions,
-    /// empty supported features
-    EmptyFeatures,
-    /// no common version
-    NoCommonVersion,
+    /// single version must be negotiated on connection before opening channel
+    InvalidVersionLength,
     /// version \"`{version}`\" not supported
     VersionNotSupported { version: Version },
+    /// no common version
+    NoCommonVersion,
+    /// empty supported features
+    EmptyFeatures,
+    /// feature \"`{feature}`\" not supported
+    FeatureNotSupported { feature: String },
+    /// no common features
+    NoCommonFeatures,
     /// missing proof height
     MissingProofHeight,
     /// missing consensus height

diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs
@@ -45,9 +45,10 @@ where
 
     ctx_a.validate_self_client(msg.client_state_of_a_on_b.clone())?;
 
-    if !(vars.conn_end_on_a.state_matches(&State::Init)
-        && vars.conn_end_on_a.versions().contains(&msg.version))
-    {
+    msg.version
+        .verify_is_supported(vars.conn_end_on_a.versions())?;
+
+    if !vars.conn_end_on_a.state_matches(&State::Init) {
         return Err(ConnectionError::ConnectionMismatch {
             connection_id: msg.conn_id_on_a.clone(),
         }
@@ -88,7 +89,7 @@ where
                 ),
                 vec![msg.version.clone()],
                 vars.conn_end_on_a.delay_period(),
-            );
+            )?;
 
             client_state_of_b_on_a
                 .verify_membership(
@@ -261,7 +262,8 @@ mod tests {
             ),
             vec![msg.version.clone()],
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         // A connection end with incorrect state `Open`; will be part of the context.
         let mut conn_end_open = default_conn_end.clone();

diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs
@@ -74,7 +74,7 @@ where
             ),
             conn_end_on_b.versions().to_vec(),
             conn_end_on_b.delay_period(),
-        );
+        )?;
 
         client_state_of_a_on_b
             .verify_membership(
@@ -213,7 +213,8 @@ mod tests {
             counterparty,
             ValidationContext::get_compatible_versions(&ctx_default),
             ZERO_DURATION,
-        );
+        )
+        .unwrap();
 
         let mut correct_conn_end = incorrect_conn_end_state.clone();
         correct_conn_end.set_state(State::TryOpen);

diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs
@@ -3,7 +3,6 @@ use crate::prelude::*;
 
 use crate::core::context::ContextError;
 use crate::core::ics03_connection::connection::{ConnectionEnd, Counterparty, State};
-use crate::core::ics03_connection::error::ConnectionError;
 use crate::core::ics03_connection::events::OpenInit;
 use crate::core::ics03_connection::msgs::conn_open_init::MsgConnectionOpenInit;
 use crate::core::ics24_host::identifier::ConnectionId;
@@ -20,9 +19,7 @@ where
     client_state_of_b_on_a.confirm_not_frozen()?;
 
     if let Some(version) = msg.version {
-        if !ctx_a.get_compatible_versions().contains(&version) {
-            return Err(ConnectionError::VersionNotSupported { version }.into());
-        }
+        version.verify_is_supported(&ctx_a.get_compatible_versions())?;
     }
 
     Ok(())
@@ -32,16 +29,12 @@ pub(crate) fn execute<Ctx>(ctx_a: &mut Ctx, msg: MsgConnectionOpenInit) -> Resul
 where
     Ctx: ExecutionContext,
 {
-    let versions = match msg.version {
-        Some(version) => {
-            if ctx_a.get_compatible_versions().contains(&version) {
-                Ok(vec![version])
-            } else {
-                Err(ConnectionError::VersionNotSupported { version })
-            }
-        }
-        None => Ok(ctx_a.get_compatible_versions()),
-    }?;
+    let versions = if let Some(version) = msg.version {
+        version.verify_is_supported(&ctx_a.get_compatible_versions())?;
+        vec![version]
+    } else {
+        ctx_a.get_compatible_versions()
+    };
 
     let conn_end_on_a = ConnectionEnd::new(
         State::Init,
@@ -53,7 +46,8 @@ where
         ),
         versions,
         msg.delay_period,
-    );
+    )
+    .unwrap();
 
     // Construct the identifier for the new connection.
     let conn_id_on_a = ConnectionId::new(ctx_a.connection_counter()?);
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- Refactor and fix version validation in connection and channel handshakes
		([#625](https://github.com/cosmos/ibc-rs/issues/625))