diff --git a/CHANGELOG.md b/CHANGELOG.md index 233e7b0f86..1047b12e8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,16 +21,24 @@ - Fix for mismatching `bitcoin` dep ([#525]) - [modules] - - Clean the validate_basic method ([#94]) + - Clean the validate_basic method ([#94]) + - MsgConnectionOpenAck testing improvements ([#306]) - Update to `tendermint-rs` v0.17.1 ([#517]) +### BUG FIXES: + +- [modules] + - Fix for storing ClientType upon 'create-client' ([#513]) + [#94]: https://github.com/informalsystems/ibc-rs/issues/94 +[#306]: https://github.com/informalsystems/ibc-rs/issues/306 [#470]: https://github.com/informalsystems/ibc-rs/issues/470 [#500]: https://github.com/informalsystems/ibc-rs/issues/500 [#505]: https://github.com/informalsystems/ibc-rs/issues/505 [#507]: https://github.com/informalsystems/ibc-rs/issues/507 [#511]: https://github.com/informalsystems/ibc-rs/pull/511 +[#513]: https://github.com/informalsystems/ibc-rs/issues/513 [#514]: https://github.com/informalsystems/ibc-rs/issues/514 [#517]: https://github.com/informalsystems/ibc-rs/issues/517 [#525]: https://github.com/informalsystems/ibc-rs/issues/525 diff --git a/modules/src/ics02_client/context.rs b/modules/src/ics02_client/context.rs index a40c124e19..b346de0ba2 100644 --- a/modules/src/ics02_client/context.rs +++ b/modules/src/ics02_client/context.rs @@ -19,12 +19,6 @@ pub trait ClientReader { /// Defines the write-only part of ICS2 (client functions) context. pub trait ClientKeeper { - fn store_client_type( - &mut self, - client_id: ClientId, - client_type: ClientType, - ) -> Result<(), Error>; - fn store_client_result( &mut self, handler_res: ClientResult, @@ -32,6 +26,7 @@ pub trait ClientKeeper { match handler_res { Create(res) => { let client_id = self.next_client_id(); + self.store_client_type(client_id.clone(), res.client_type)?; self.store_client_state(client_id.clone(), res.client_state.clone())?; self.store_consensus_state( client_id.clone(), @@ -54,12 +49,21 @@ pub trait ClientKeeper { fn next_client_id(&mut self) -> ClientId; + /// Called upon successful client creation + fn store_client_type( + &mut self, + client_id: ClientId, + client_type: ClientType, + ) -> Result<(), Error>; + + /// Called upon successful client creation and update fn store_client_state( &mut self, client_id: ClientId, client_state: AnyClientState, ) -> Result<(), Error>; + /// Called upon successful client creation and update fn store_consensus_state( &mut self, client_id: ClientId, diff --git a/modules/src/ics03_connection/connection.rs b/modules/src/ics03_connection/connection.rs index 5759b83027..e26708ead9 100644 --- a/modules/src/ics03_connection/connection.rs +++ b/modules/src/ics03_connection/connection.rs @@ -109,6 +109,11 @@ impl ConnectionEnd { self.state = new_state; } + /// Setter for the `counterparty` field. + pub fn set_counterparty(&mut self, new_cparty: Counterparty) { + self.counterparty = new_cparty; + } + /// Setter for the `version` field. /// TODO: A ConnectionEnd should only store one version. pub fn set_version(&mut self, new_version: Version) { diff --git a/modules/src/ics03_connection/error.rs b/modules/src/ics03_connection/error.rs index d858508809..aaa30a360a 100644 --- a/modules/src/ics03_connection/error.rs +++ b/modules/src/ics03_connection/error.rs @@ -6,7 +6,7 @@ pub type Error = anomaly::Error; use crate::ics24_host::identifier::{ClientId, ConnectionId}; use crate::Height; -#[derive(Clone, Debug, Error)] +#[derive(Clone, Debug, Error, Eq, PartialEq)] pub enum Kind { #[error("connection state unknown")] InvalidState(i32), @@ -74,14 +74,14 @@ pub enum Kind { #[error("client proof must be present")] NullClientProof, - #[error("the client is frozen")] - FrozenClient, + #[error("the client running locally is frozen")] + FrozenClient(ClientId), #[error("the connection proof verification failed")] ConnectionVerificationFailure, - #[error("the expected consensus state could not be retrieved")] - MissingClientConsensusState, + #[error("the consensus state at height {0} for client id {1} could not be retrieved")] + MissingClientConsensusState(Height, ClientId), #[error("the local consensus state could not be retrieved")] MissingLocalConsensusState, @@ -89,8 +89,8 @@ pub enum Kind { #[error("the consensus proof verification failed (height: {0})")] ConsensusStateVerificationFailure(Height), - #[error("the client state proof verification failed")] - ClientStateVerificationFailure, + #[error("the client state proof verification failed for client id: {0}")] + ClientStateVerificationFailure(ClientId), } impl Kind { diff --git a/modules/src/ics03_connection/handler/conn_open_ack.rs b/modules/src/ics03_connection/handler/conn_open_ack.rs index b76d91fefc..e4259b2077 100644 --- a/modules/src/ics03_connection/handler/conn_open_ack.rs +++ b/modules/src/ics03_connection/handler/conn_open_ack.rs @@ -96,7 +96,7 @@ mod tests { use crate::handler::EventType; use crate::ics03_connection::connection::{ConnectionEnd, Counterparty, State}; - use crate::ics03_connection::context::ConnectionReader; + use crate::ics03_connection::error::Kind; use crate::ics03_connection::handler::{dispatch, ConnectionResult}; use crate::ics03_connection::msgs::conn_open_ack::test_util::get_dummy_msg_conn_open_ack; use crate::ics03_connection::msgs::conn_open_ack::MsgConnectionOpenAck; @@ -114,115 +114,119 @@ mod tests { ctx: MockContext, msg: ConnectionMsg, want_pass: bool, + error_kind: Option, } - let client_id = ClientId::from_str("mock_clientid").unwrap(); let msg_ack = MsgConnectionOpenAck::try_from(get_dummy_msg_conn_open_ack()).unwrap(); - let counterparty = Counterparty::new( - client_id.clone(), - msg_ack.counterparty_connection_id().cloned(), - CommitmentPrefix::from(vec![]), - ); + let conn_id = msg_ack.connection_id.clone(); - let incorrect_context = MockContext::default(); + // 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(); - // A connection end (with incorrect state `Open`) that will be part of the context. - let incorrect_conn_end_state = ConnectionEnd::new( - State::Open, - client_id.clone(), - counterparty, - incorrect_context.get_compatible_versions(), - 0_u64, + // Parametrize the host chain to have a height at least as recent as the + // the height of the proofs in the Ack msg. + let default_context = MockContext::new( + ChainId::new("mockgaia".to_string(), 1), + HostType::Mock, + 5, + Height::new(1, msg_ack.proofs().height().increment().revision_height), ); - // A connection end (with correct state but incorrect versions); exercises unsuccessful - // processing path. - let mut incorrect_conn_end_vers = incorrect_conn_end_state.clone(); - incorrect_conn_end_vers.set_state(State::Init); - - // A connection end (with correct versions and correct state, but incorrect prefix for the - // counterparty) that will be part of the context to exercise unsuccessful path. - let mut incorrect_conn_end_prefix = incorrect_conn_end_state.clone(); - incorrect_conn_end_prefix.set_state(State::Init); - incorrect_conn_end_prefix.set_version(msg_ack.version().clone()); - - // Build a connection end that will exercise the successful path. - let correct_counterparty = Counterparty::new( - client_id.clone(), - msg_ack.counterparty_connection_id().cloned(), - CommitmentPrefix::from(b"ibc".to_vec()), - ); - let correct_conn_end = ConnectionEnd::new( + // A connection end that will exercise the successful path. + let default_conn_end = ConnectionEnd::new( State::Init, client_id.clone(), - correct_counterparty, + Counterparty::new( + client_id.clone(), + msg_ack.counterparty_connection_id().cloned(), + CommitmentPrefix::from(b"ibc".to_vec()), + ), vec![msg_ack.version().clone()], 0_u64, ); - // Parametrize the (correct) host chain to have a height at least as recent as the - // the height of the proofs in the Ack msg. - let correct_context = MockContext::new( - ChainId::new("mockgaia".to_string(), 1), - HostType::Mock, - 5, - Height::new(1, msg_ack.proofs().height().increment().revision_height), - ); + // A connection end with incorrect state `Open`; will be part of the context. + let mut conn_end_open = default_conn_end.clone(); + conn_end_open.set_state(State::Open); // incorrect field + + // A connection end with correct state, but incorrect prefix for the + // counterparty; will be part of the context to exercise unsuccessful path. + let mut conn_end_prefix = conn_end_open.clone(); + conn_end_prefix.set_state(State::Init); + conn_end_prefix.set_counterparty(Counterparty::new( + client_id.clone(), + msg_ack.counterparty_connection_id().cloned(), + 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 = vec![ Test { - name: "Processing fails due to missing connection in context".to_string(), - ctx: correct_context.clone(), + name: "Successful processing of an Ack message".to_string(), + ctx: default_context + .clone() + .with_client(&client_id, proof_height) + .with_connection(conn_id.clone(), default_conn_end.clone()), msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), - want_pass: false, + want_pass: true, + error_kind: None, }, Test { - name: "Processing fails due to connections mismatch (incorrect state)".to_string(), - ctx: correct_context - .clone() - .with_client(&client_id, Height::new(0, 10)) - .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_state), + name: "Processing fails because the connection does not exist in the context".to_string(), + ctx: MockContext::default(), // Empty context msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), want_pass: false, + error_kind: Some(Kind::UninitializedConnection(conn_id.clone())), }, Test { - name: "Processing fails due to connections mismatch (incorrect versions)" - .to_string(), - ctx: correct_context + name: "Processing fails due to connections mismatch (incorrect 'open' state)".to_string(), + ctx: default_context .clone() - .with_client(&client_id, Height::new(0, 10)) - .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_vers), + .with_client(&client_id, proof_height) + .with_connection(conn_id.clone(), conn_end_open), msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), want_pass: false, + error_kind: Some(Kind::ConnectionMismatch(conn_id.clone())) }, Test { name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(), - ctx: correct_context + ctx: default_context .clone() - .with_client(&client_id, Height::new(0, 10)) - .with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_prefix), + .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 MissingLocalConsensusState".to_string(), - ctx: incorrect_context - .with_client(&client_id, Height::new(0, 10)) - .with_connection(msg_ack.connection_id().clone(), correct_conn_end.clone()), + 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), msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), want_pass: false, + error_kind: Some(Kind::ConnectionMismatch(conn_id.clone())) }, Test { - name: "Successful processing of Ack message".to_string(), - ctx: correct_context - .with_client(&client_id, Height::new(0, 10)) - .with_connection(msg_ack.connection_id().clone(), correct_conn_end), - msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())), - want_pass: true, + name: "Processing fails due to MissingLocalConsensusState".to_string(), + ctx: MockContext::default() + .with_client(&client_id, proof_height) + .with_connection(conn_id, default_conn_end), + msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)), + want_pass: false, + error_kind: Some(Kind::MissingLocalConsensusState) }, - ] - .into_iter() - .collect(); + ]; for test in tests { let res = dispatch(&test.ctx, test.msg.clone()); @@ -248,6 +252,7 @@ mod tests { } } Err(e) => { + println!("Error for {:?} was {:#?}", test.name, e.kind()); assert_eq!( test.want_pass, false, @@ -257,6 +262,14 @@ mod tests { test.ctx.clone(), e, ); + // Verify that the error kind matches + if let Some(expected_kind) = test.error_kind { + assert_eq!( + &expected_kind, + e.kind(), + "conn_open_ack: expected error kind mismatches thrown error kind" + ) + } } } } diff --git a/modules/src/ics03_connection/handler/verify.rs b/modules/src/ics03_connection/handler/verify.rs index 7cce94d2ac..c26c52679a 100644 --- a/modules/src/ics03_connection/handler/verify.rs +++ b/modules/src/ics03_connection/handler/verify.rs @@ -70,9 +70,7 @@ pub fn verify_connection_proof( // The client must not be frozen. if client_state.is_frozen() { - return Err(Kind::FrozenClient - .context(connection_end.client_id().to_string()) - .into()); + return Err(Kind::FrozenClient(connection_end.client_id().clone()).into()); } // The client must have the consensus state for the height where this proof was created. @@ -80,9 +78,11 @@ pub fn verify_connection_proof( .client_consensus_state(connection_end.client_id(), proof_height) .is_none() { - return Err(Kind::MissingClientConsensusState - .context(connection_end.client_id().to_string()) - .into()); + return Err(Kind::MissingClientConsensusState( + proof_height, + connection_end.client_id().clone(), + ) + .into()); } let client_def = AnyClient::from_client_type(client_state.client_type()); @@ -102,6 +102,13 @@ pub fn verify_connection_proof( .map_err(|_| Kind::InvalidProof)?) } +/// Verifies the client `proof` from a connection handshake message, typically from a +/// `MsgConnectionOpenTry` or a `MsgConnectionOpenAck`. The `expected_client_state` argument is a +/// representation for a client of the current chain (the chain handling the current message), which +/// is running on the counterparty chain (the chain which sent this message). This method does a +/// complete verification: that the client state the counterparty stores is valid (i.e., not frozen, +/// at the same revision as the current chain, with matching chain identifiers, etc) and that the +/// `proof` is correct. pub fn verify_client_proof( ctx: &dyn ConnectionReader, connection_end: &ConnectionEnd, @@ -114,12 +121,14 @@ pub fn verify_client_proof( .client_state(connection_end.client_id()) .ok_or_else(|| Kind::MissingClient(connection_end.client_id().clone()))?; - // TODO: Client frozen check? + if client_state.is_frozen() { + return Err(Kind::FrozenClient(connection_end.client_id().clone()).into()); + } let consensus_state = ctx .client_consensus_state(connection_end.client_id(), proof_height) .ok_or_else(|| { - Kind::MissingClientConsensusState.context(connection_end.client_id().to_string()) + Kind::MissingClientConsensusState(proof_height, connection_end.client_id().clone()) })?; let client_def = AnyClient::from_client_type(client_state.client_type()); @@ -134,8 +143,9 @@ pub fn verify_client_proof( proof, &expected_client_state, ) - .map_err(|_| { - Kind::ClientStateVerificationFailure.context(connection_end.client_id().to_string()) + .map_err(|e| { + Kind::ClientStateVerificationFailure(connection_end.client_id().clone()) + .context(e.to_string()) })?) } @@ -151,9 +161,7 @@ pub fn verify_consensus_proof( .ok_or_else(|| Kind::MissingClient(connection_end.client_id().clone()))?; if client_state.is_frozen() { - return Err(Kind::FrozenClient - .context(connection_end.client_id().to_string()) - .into()); + return Err(Kind::FrozenClient(connection_end.client_id().clone()).into()); } // Fetch the expected consensus state from the historical (local) header data. diff --git a/modules/src/ics03_connection/msgs/conn_open_ack.rs b/modules/src/ics03_connection/msgs/conn_open_ack.rs index 80eed41bd7..dc3b0e01a8 100644 --- a/modules/src/ics03_connection/msgs/conn_open_ack.rs +++ b/modules/src/ics03_connection/msgs/conn_open_ack.rs @@ -35,7 +35,7 @@ impl MsgConnectionOpenAck { &self.connection_id } - /// Getter for accessing the connection identifier of this message. + /// 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() } diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 3fdb3c3f37..803d383df9 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -544,6 +544,14 @@ impl ClientReader for MockContext { } impl ClientKeeper for MockContext { + fn next_client_id(&mut self) -> ClientId { + let prefix = ClientId::default().to_string(); + let suffix = self.client_ids_counter; + self.client_ids_counter += 1; + + ClientId::from_str(format!("{}-{}", prefix, suffix).as_str()).unwrap() + } + fn store_client_type( &mut self, client_id: ClientId, @@ -559,14 +567,6 @@ impl ClientKeeper for MockContext { Ok(()) } - fn next_client_id(&mut self) -> ClientId { - let prefix = ClientId::default().to_string(); - let suffix = self.client_ids_counter; - self.client_ids_counter += 1; - - ClientId::from_str(format!("{}-{}", prefix, suffix).as_str()).unwrap() - } - fn store_client_state( &mut self, client_id: ClientId,