Allow a conn open ack to succeed in the happy case (#700)

* Allow a conn open ack to succeed * Remove invalid test * update CHANGELOG * Make MsgConnectionOpenAck.counterparty_connection_id not an Option
informalsystems · Feb 23, 2021 · 08db234 · 08db234
1 parent a36abaa
commit 08db234
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 51 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,7 @@
 
 - [ibc]
  - Fix overflow bug in ICS03 client consensus height verification method ([#685])
+ - Allow a conn open ack to succeed in the happy case ([#699])
 
 - [ibc-relayer]
  - [nothing yet]
@@ -44,7 +45,7 @@
 ### BREAKING CHANGES
 
 - [ibc]
- - [nothing yet]
+ - `MsgConnectionOpenAck.counterparty_connection_id` is now a `ConnectionId` instead of an `Option<ConnectionId>`([#700])
 
 - [ibc-relayer]
  - [nothing yet]
@@ -55,6 +56,8 @@
 [#599]: https://github.com/informalsystems/ibc-rs/issues/599
 [#685]: https://github.com/informalsystems/ibc-rs/issues/685
 [#689]: https://github.com/informalsystems/ibc-rs/issues/689
+[#699]: https://github.com/informalsystems/ibc-rs/issues/699
+[#700]: https://github.com/informalsystems/ibc-rs/pull/700
 
 ## v0.1.1
 *February 17, 2021*

diff --git a/modules/src/ics02_client/client_def.rs b/modules/src/ics02_client/client_def.rs
@@ -73,7 +73,7 @@ pub trait ClientDef: Clone {
  height: Height,
  prefix: &CommitmentPrefix,
  proof: &CommitmentProofBytes,
- connection_id: &ConnectionId,
+ connection_id: Option<&ConnectionId>,
  expected_connection_end: &ConnectionEnd,
  ) -> Result<(), Box<dyn std::error::Error>>;
 
@@ -472,7 +472,7 @@ impl ClientDef for AnyClient {
  height: Height,
  prefix: &CommitmentPrefix,
  proof: &CommitmentProofBytes,
- connection_id: &ConnectionId,
+ connection_id: Option<&ConnectionId>,
  expected_connection_end: &ConnectionEnd,
  ) -> Result<(), Box<dyn std::error::Error>> {
  match self {

diff --git a/modules/src/ics03_connection/handler/conn_open_ack.rs b/modules/src/ics03_connection/handler/conn_open_ack.rs
@@ -30,10 +30,15 @@ pub(crate) fn process(
  || old_conn_end.state_matches(&State::TryOpen)
  && old_conn_end.versions().get(0).eq(&Some(msg.version()));
 
- // Check that if the msg's counterparty connection id is not empty then it matches
- // the old connection's counterparty.
- let counterparty_matches = msg.counterparty_connection_id().is_none()
- || old_conn_end.counterparty().connection_id() == msg.counterparty_connection_id();
+ // Check that, if we have a counterparty connection id, then it
+ // matches the one in the message.
+ let counterparty_matches = if let Some(counterparty_connection_id) =
+ old_conn_end.counterparty().connection_id()
+ {
+ &msg.counterparty_connection_id == counterparty_connection_id
+ } else {
+ true
+ };
 
  if state_is_consistent && counterparty_matches {
  Ok(old_conn_end)
@@ -59,8 +64,8 @@ pub(crate) fn process(
  Counterparty::new(
  // The counterparty is the local chain.
  new_conn_end.client_id().clone(), // The local client identifier.
- msg.counterparty_connection_id().cloned(), // This chain's connection id as known on counterparty.
- ctx.commitment_prefix(), // Local commitment prefix.
+ Some(msg.counterparty_connection_id().clone()), // This chain's connection id as known on counterparty.
+ ctx.commitment_prefix(),  // Local commitment prefix.
  ),
  vec![msg.version().clone()],
  new_conn_end.delay_period,
@@ -147,7 +152,7 @@ mod tests {
  client_id.clone(),
  Counterparty::new(
  client_id.clone(),
- msg_ack.counterparty_connection_id().cloned(),
+ Some(msg_ack.counterparty_connection_id().clone()),
  CommitmentPrefix::from(b"ibc".to_vec()),
  ),
  vec![msg_ack.version().clone()],
@@ -164,20 +169,10 @@ mod tests {
  conn_end_prefix.set_state(State::Init);
  conn_end_prefix.set_counterparty(Counterparty::new(
  client_id.clone(),
- msg_ack.counterparty_connection_id().cloned(),
+ Some(msg_ack.counterparty_connection_id().clone()),
  CommitmentPrefix::from(vec![]), // incorrect field
  ));
 
- // A connection end with correct state & prefix, but incorrect counterparty; exercises
- // unsuccessful processing path.
- let mut conn_end_cparty = conn_end_open.clone();
- conn_end_cparty.set_state(State::Init);
- conn_end_cparty.set_counterparty(Counterparty::new(
- client_id.clone(),
- None, // incorrect field
- CommitmentPrefix::from(b"ibc".to_vec()),
- ));
-
  let tests: Vec<Test> = vec![
  Test {
  name: "Successful processing of an Ack message".to_string(),
@@ -209,21 +204,11 @@ mod tests {
  Test {
  name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(),
  ctx: default_context
- .clone()
  .with_client(&client_id, proof_height)
- .with_connection(conn_id.clone(), conn_end_prefix),
- msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
- want_pass: false,
- error_kind: Some(Kind::ConsensusStateVerificationFailure(proof_height))
- },
- Test {
- name: "Processing fails due to mismatching counterparty conn id".to_string(),
- ctx: default_context
- .with_client(&client_id, proof_height)
- .with_connection(conn_id.clone(), conn_end_cparty),
+ .with_connection(conn_id, conn_end_prefix),
  msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)),
  want_pass: false,
- error_kind: Some(Kind::ConnectionMismatch(conn_id))
+ error_kind: Some(Kind::ConsensusStateVerificationFailure(proof_height))
  },
  /*
  Test {

diff --git a/modules/src/ics03_connection/handler/verify.rs b/modules/src/ics03_connection/handler/verify.rs
@@ -96,7 +96,7 @@ pub fn verify_connection_proof(
  proof_height,
  connection_end.counterparty().prefix(),
  proof,
- &connection_end.counterparty().connection_id().unwrap(),
+ connection_end.counterparty().connection_id(),
  expected_conn,
  )
  .map_err(|_| Kind::InvalidProof)?)

diff --git a/modules/src/ics03_connection/msgs/conn_open_ack.rs b/modules/src/ics03_connection/msgs/conn_open_ack.rs
@@ -1,5 +1,4 @@
 use std::convert::{TryFrom, TryInto};
-use std::str::FromStr;
 
 use tendermint::account::Id as AccountId;
 use tendermint_proto::Protobuf;
@@ -22,7 +21,7 @@ pub const TYPE_URL: &str = "/ibc.core.connection.v1.MsgConnectionOpenAck";
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct MsgConnectionOpenAck {
  pub connection_id: ConnectionId,
- pub counterparty_connection_id: Option<ConnectionId>,
+ pub counterparty_connection_id: ConnectionId,
  pub client_state: Option<AnyClientState>,
  pub proofs: Proofs,
  pub version: Version,
@@ -36,8 +35,8 @@ impl MsgConnectionOpenAck {
  }
 
  /// Getter for accessing the counterparty's connection identifier from this message.
- pub fn counterparty_connection_id(&self) -> Option<&ConnectionId> {
- self.counterparty_connection_id.as_ref()
+ pub fn counterparty_connection_id(&self) -> &ConnectionId {
+ &self.counterparty_connection_id
  }
 
  /// Getter for accessing the client state.
@@ -107,18 +106,15 @@ impl TryFrom<RawMsgConnectionOpenAck> for MsgConnectionOpenAck {
  .filter(|x| !x.is_empty())
  .map(CommitmentProofBytes::from);
 
- let counterparty_connection_id = Some(msg.counterparty_connection_id)
- .filter(|x| !x.is_empty())
- .map(|v| FromStr::from_str(v.as_str()))
- .transpose()
- .map_err(|e| Kind::IdentifierError.context(e))?;
-
  Ok(Self {
  connection_id: msg
  .connection_id
  .parse()
  .map_err(|e| Kind::IdentifierError.context(e))?,
- counterparty_connection_id,
+ counterparty_connection_id: msg
+ .counterparty_connection_id
+ .parse()
+ .map_err(|e| Kind::IdentifierError.context(e))?,
  client_state: msg
  .client_state
  .map(AnyClientState::try_from)
@@ -146,9 +142,7 @@ impl From<MsgConnectionOpenAck> for RawMsgConnectionOpenAck {
  fn from(ics_msg: MsgConnectionOpenAck) -> Self {
  RawMsgConnectionOpenAck {
  connection_id: ics_msg.connection_id.as_str().to_string(),
- counterparty_connection_id: ics_msg
- .counterparty_connection_id
- .map_or_else(|| "".to_string(), |v| v.as_str().to_string()),
+ counterparty_connection_id: ics_msg.counterparty_connection_id.as_str().to_string(),
  client_state: ics_msg
  .client_state
  .map_or_else(|| None, |v| Some(v.into())),

diff --git a/modules/src/ics07_tendermint/client_def.rs b/modules/src/ics07_tendermint/client_def.rs
@@ -57,7 +57,7 @@ impl ClientDef for TendermintClient {
  _height: Height,
  _prefix: &CommitmentPrefix,
  _proof: &CommitmentProofBytes,
- _connection_id: &ConnectionId,
+ _connection_id: Option<&ConnectionId>,
  _expected_connection_end: &ConnectionEnd,
  ) -> Result<(), Box<dyn std::error::Error>> {
  todo!()

diff --git a/modules/src/mock/client_def.rs b/modules/src/mock/client_def.rs
@@ -64,7 +64,7 @@ impl ClientDef for MockClient {
  _height: Height,
  _prefix: &CommitmentPrefix,
  _proof: &CommitmentProofBytes,
- _connection_id: &ConnectionId,
+ _connection_id: Option<&ConnectionId>,
  _expected_connection_end: &ConnectionEnd,
  ) -> Result<(), Box<dyn std::error::Error>> {
  Ok(())

diff --git a/relayer/src/connection.rs b/relayer/src/connection.rs
@@ -566,7 +566,7 @@ impl Connection {
 
  let new_msg = MsgConnectionOpenAck {
  connection_id: self.dst_connection_id().clone(),
- counterparty_connection_id: Option::from(self.src_connection_id().clone()),
+ counterparty_connection_id: self.src_connection_id().clone(),
  client_state,
  proofs,
  version: src_connection.versions()[0].clone(),