From 78d7273bfe3ff69bfa7ccdff1ccb06f94f72579a Mon Sep 17 00:00:00 2001 From: joao Date: Thu, 20 Jun 2024 16:56:30 +1000 Subject: [PATCH 1/3] Add multihop support for `tx chan-open-ack` * Replace call to `tx_chan_cmd!` macro with an inline function * Add `build_multihop_chan_open_ack()` --- crates/relayer-cli/src/commands/tx/channel.rs | 305 ++++++++++++++++-- crates/relayer/src/channel.rs | 120 ++++++- 2 files changed, 391 insertions(+), 34 deletions(-) diff --git a/crates/relayer-cli/src/commands/tx/channel.rs b/crates/relayer-cli/src/commands/tx/channel.rs index 98f066144b..16696fe6d2 100644 --- a/crates/relayer-cli/src/commands/tx/channel.rs +++ b/crates/relayer-cli/src/commands/tx/channel.rs @@ -392,8 +392,8 @@ impl Runnable for TxChanOpenTryCmd { Err(e) => Output::error(e).exit(), }; - let mut a_side_hops = Vec::new(); - let mut b_side_hops = Vec::new(); + let mut a_side_hops = Vec::new(); // Hops from --src-chain towards --dst-chain + let mut b_side_hops = Vec::new(); // Hops from --dst-chain towards --src-chain // Determine if the channel is a multihop channel, i.e, connection_hops > 1 if init_channel.connection_hops().len() > 1 { @@ -706,35 +706,284 @@ pub struct TxChanOpenAckCmd { impl Runnable for TxChanOpenAckCmd { fn run(&self) { - tx_chan_cmd!( - "ChanOpenAck", - build_chan_open_ack_and_send, - self, - |chains: ChainHandlePair, dst_connection: ConnectionEnd| { - Channel { - connection_delay: Default::default(), - ordering: Ordering::default(), - a_side: ChannelSide::new( - chains.src, - ClientId::default(), - ConnectionId::default(), - None, - self.src_port_id.clone(), - Some(self.src_chan_id.clone()), - None, + let config = app_config(); + + // Set a global registry to retrieve or spawn chain handles + set_global_registry(SharedRegistry::new((*app_config()).clone())); + + let chains = match ChainHandlePair::spawn(&config, &self.src_chain_id, &self.dst_chain_id) { + Ok(chains) => chains, + Err(e) => Output::error(e).exit(), + }; + + // Attempt to retrieve --dst-connection + let dst_connection = match chains.dst.query_connection( + QueryConnectionRequest { + connection_id: self.dst_conn_id.clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((connection, _)) => connection, + Err(e) => Output::error(e).exit(), + }; + + // Retrieve the ChannelEnd in TRYOPEN state (--src-channel) from --src-chain + let tryopen_channel = match chains.src.query_channel( + QueryChannelRequest { + port_id: self.src_port_id.clone(), + channel_id: self.src_chan_id.clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((channel, _)) => channel, + Err(e) => Output::error(e).exit(), + }; + + let mut a_side_hops = Vec::new(); // Hops from --src-chain towards --dst-chain + let mut b_side_hops = Vec::new(); // Hops from --dst-chain towards --src-chain + + // Determine if the channel is a multihop channel, i.e, connection_hops > 1 + if tryopen_channel.connection_hops().len() > 1 { + // In the case of multihop channels, we must build the channel path from --dst-chain + // to --src-chain by leveraging the information stored in the ChannelEnd in TRYOPEN state, + // which contains the path from --src-chain to --dst-chain. We must traverse the path from + // --src-chain to --dst-chain, and, for each connection hop, obtain the counterparty + // connection and use it to build the same hop from the opposite direction. + let mut chain_id = chains.src.id().clone(); + + for a_side_connection_id in tryopen_channel.connection_hops() { + let chain_handle = match spawn_chain_runtime(&config, &chain_id) { + Ok(handle) => handle, + Err(e) => Output::error(e).exit(), + }; + + let a_side_hop_connection = match chain_handle.query_connection( + QueryConnectionRequest { + connection_id: a_side_connection_id.clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((connection, _)) => connection, + Err(e) => Output::error(e).exit(), + }; + + let a_side_hop_conn_client_state = match chain_handle.query_client_state( + QueryClientStateRequest { + client_id: a_side_hop_connection.client_id().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((client_state, _)) => client_state, + Err(e) => Output::error(e).exit(), + }; + + // Obtain the counterparty ConnectionId and ChainId for the current connection hop + // towards b_side + let counterparty_conn_id = a_side_hop_connection + .counterparty() + .connection_id() + .unwrap_or_else(|| { + Output::error(ConnectionError::missing_counterparty_connection_id()).exit() + }); + + let counterparty_chain_id = a_side_hop_conn_client_state.chain_id().clone(); + + let counterparty_handle = match spawn_chain_runtime(&config, &counterparty_chain_id) + { + Ok(handle) => handle, + Err(e) => Output::error(e).exit(), + }; + + // Retrieve the counterparty connection + let counterparty_connection = match counterparty_handle.query_connection( + QueryConnectionRequest { + connection_id: counterparty_conn_id.clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((connection, _)) => connection, + Err(e) => Output::error(e).exit(), + }; + + a_side_hops.push(ConnectionHop { + connection: IdentifiedConnectionEnd::new( + a_side_connection_id.clone(), + a_side_hop_connection.clone(), ), - b_side: ChannelSide::new( - chains.dst, - dst_connection.client_id().clone(), - self.dst_conn_id.clone(), - None, - self.dst_port_id.clone(), - Some(self.dst_chan_id.clone()), - None, + src_chain_id: chain_id.clone(), + dst_chain_id: a_side_hop_conn_client_state.chain_id(), + }); + + // Build the current hop from the opposite direction + b_side_hops.push(ConnectionHop { + connection: IdentifiedConnectionEnd::new( + counterparty_conn_id.clone(), + counterparty_connection, ), - } + src_chain_id: a_side_hop_conn_client_state.chain_id(), + dst_chain_id: chain_id.clone(), + }); + + // Update chain_id to point to the next chain in the channel path + // from a_side towards b_side + chain_id = a_side_hop_conn_client_state.chain_id().clone(); } - ); + } else { + // If the channel path corresponds to a single-hop channel, there is only one + // connection hop from --dst-chain to --src-chain and vice-versa. + + // Get the single ConnectionId from a_side (--src-chain) to b_side (--dst-chain) + // from the connection_hops field of the ChannelEnd in TRYOPEN state + let a_side_connection_id = + tryopen_channel + .connection_hops() + .first() + .unwrap_or_else(|| { + Output::error(ChannelError::missing_connection_hops( + self.src_chan_id.clone(), + self.src_chain_id.clone(), + )) + .exit() + }); + + let a_side_hop_connection = match chains.src.query_connection( + QueryConnectionRequest { + connection_id: a_side_connection_id.clone().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((connection, _)) => connection, + Err(e) => Output::error(e).exit(), + }; + + let a_side_hop_conn_client_state = match chains.src.query_client_state( + QueryClientStateRequest { + client_id: a_side_hop_connection.client_id().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((client_state, _)) => client_state, + Err(e) => Output::error(e).exit(), + }; + + a_side_hops.push(ConnectionHop { + connection: IdentifiedConnectionEnd::new( + a_side_connection_id.clone(), + a_side_hop_connection, + ), + src_chain_id: chains.src.id(), + dst_chain_id: a_side_hop_conn_client_state.chain_id(), + }); + + let b_side_hop_connection = match chains.dst.query_connection( + QueryConnectionRequest { + connection_id: self.dst_conn_id.clone().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((connection, _)) => connection, + Err(e) => Output::error(e).exit(), + }; + + // Retrieve the state of the client underlying the hop connection + let b_side_hop_conn_client_state = match chains.dst.query_client_state( + QueryClientStateRequest { + client_id: b_side_hop_connection.client_id().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) { + Ok((client_state, _)) => client_state, + Err(e) => Output::error(e).exit(), + }; + + // Build the single hop from --dst-chain to --src-chain + b_side_hops.push(ConnectionHop { + connection: IdentifiedConnectionEnd::new( + self.dst_conn_id.clone(), + b_side_hop_connection.clone(), + ), + src_chain_id: chains.dst.id(), + dst_chain_id: b_side_hop_conn_client_state.chain_id().clone(), + }); + } + + // The connection hops were assembled while traversing from --src-chain towards --dst-chain. + // Reverse them to obtain the path from --dst-chain to --src-chain. Single hops remain unchanged + // when reversed. + b_side_hops.reverse(); + + // Ensure that the reverse channel path that starts at --dst-chain correctly leads to --src-chain + if let Some(last_hop) = &b_side_hops.last() { + if last_hop.dst_chain_id != chains.src.id() { + Output::error(Error::ics33_hops_destination_mismatch( + chains.src.id(), + chains.dst.id(), + last_hop.dst_chain_id.clone(), + )) + .exit() + } + } + + // FIXME: For now, pass Some(_) to connection_hops if there are multiple hops and None if there is a single one. + // This allows us to keep using existing structs as they are defined (with the single `connection_id` field) while also including + // the new `connection_hops` field. When multiple hops are present, pass Some(_) to connection_hops and use that. + // When a single hop is present, pass None to connection_hops and use the connection_id stored in `ChannelSide`. + let a_side_hops = match a_side_hops.len() { + 0 => Output::error("At least one connection hop is required for opening a channel.") + .exit(), + 1 => None, + _ => Some(ConnectionHops::new(a_side_hops)), + }; + + let b_side_hops = match b_side_hops.len() { + 0 => Output::error("At least one connection hop is required for opening a channel.") + .exit(), + 1 => None, + _ => Some(ConnectionHops::new(b_side_hops)), + }; + + let channel = Channel { + ordering: Ordering::default(), + a_side: ChannelSide::new( + chains.src, + ClientId::default(), + ConnectionId::default(), + a_side_hops, + self.src_port_id.clone(), + Some(self.src_chan_id.clone()), + None, + ), + b_side: ChannelSide::new( + chains.dst, + dst_connection.client_id().clone(), + self.dst_conn_id.clone(), + b_side_hops, + self.dst_port_id.clone(), + Some(self.dst_chan_id.clone()), + None, + ), + connection_delay: Default::default(), + }; + + info!("message {}: {}", "ChanOpenAck", channel); + + let res: Result = channel + .build_chan_open_ack_and_send() + .map_err(Error::channel); + + match res { + Ok(receipt) => Output::success(receipt).exit(), + Err(e) => Output::error(e).exit(), + } } } diff --git a/crates/relayer/src/channel.rs b/crates/relayer/src/channel.rs index 82c4d54e40..d6a716cb5e 100644 --- a/crates/relayer/src/channel.rs +++ b/crates/relayer/src/channel.rs @@ -1002,8 +1002,8 @@ impl Channel { .dst_channel_id() .ok_or_else(ChannelError::missing_counterparty_channel_id)?; - // If there is a channel present on the destination chain, - // the counterparty should look like this: + // If there is a channel end on the destination chain, the channel end's + // counterparty should match this: let counterparty = Counterparty::new(self.src_port_id().clone(), self.src_channel_id().cloned()); @@ -1015,6 +1015,7 @@ impl Channel { _ => State::Uninitialized, }; + // The channel configuration as it should be in the destination let dst_expected_channel = ChannelEnd::new( highest_state, self.ordering, @@ -1024,7 +1025,7 @@ impl Channel { 0, ); - // Retrieve existing channel + // Retrieve existing channel from the destination let (dst_channel, _) = self .dst_chain() .query_channel( @@ -1037,8 +1038,8 @@ impl Channel { ) .map_err(|e| ChannelError::query(self.dst_chain().id(), e))?; - // Check if a channel is expected to exist on destination chain - // A channel must exist on destination chain for Ack and Confirm Tx-es to succeed + // Require the channel end to exist, i.e, require the state to differ from State::Uninitialized. + // A channel end must exist on the destination for Ack and Confirm Txs to succeed. if dst_channel.state_matches(&State::Uninitialized) { return Err(ChannelError::missing_channel_on_destination()); } @@ -1633,6 +1634,109 @@ impl Channel { } } + pub fn build_multihop_chan_open_ack(&self) -> Result, ChannelError> { + // Source and destination channel IDs must be specified + let src_channel_id = self + .src_channel_id() + .ok_or_else(ChannelError::missing_local_channel_id)?; + let dst_channel_id = self + .dst_channel_id() + .ok_or_else(ChannelError::missing_counterparty_channel_id)?; + + // Check that the destination chain will accept the Ack message + // self.validated_expected_channel(ChannelMsgType::OpenAck)?; + + // Channel must exist on source + let (src_channel, _) = self + .src_chain() + .query_channel( + QueryChannelRequest { + port_id: self.src_port_id().clone(), + channel_id: src_channel_id.clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) + .map_err(|e| ChannelError::query(self.src_chain().id(), e))?; + + // // The channel end in src_chain must be in 'Init' state + // if !src_channel.state_matches(&State::Init) { + // return Err(ChannelError::unexpected_channel_state( + // src_channel_id.clone(), + // State::Init, + // *src_channel.state(), + // )); + // } + + self.dst_chain() + .query_connection( + QueryConnectionRequest { + connection_id: self.dst_connection_id().clone(), + height: QueryHeight::Latest, + }, + IncludeProof::No, + ) + .map_err(|e| ChannelError::query(self.dst_chain().id(), e))?; + + // Update the clients along the channel path and store the heights necessary for querying + // multihop proofs. 'proof_heights' contains the height at which proofs should be queried, + // ordered from the sending chain to the penultimate chain in the channel path. In order to + // verify proofs queried at the height 'proof_query_height' stored in 'proof_heights', the + // client on the chain that receives the proof must possess the consensus state corresponding + // to height 'proof_query_height + 1' of the chain on which the proofs were queried. + let proof_heights = self.update_channel_path_clients()?; + + // Get the proof heights for the last hop in the channel path, which connects the penultimate + // chain to the destination. + let last_hop_heights = + proof_heights + .last() + .ok_or(ChannelError::missing_multihop_proof_heights( + src_channel_id.clone(), + self.src_chain().id(), + ))?; + + // Build the message to update the client on the channel path's destination chain. Because + // proofs are queried at height 'last_hop_heights.proof_query_height' in the penultimate chain, + // update the client that tracks the penultimate chain in the destination using height + // 'last_hop_heights.proof_query_height' + 1 to allow for proof verification. + let mut msgs = + self.build_update_client_on_last_hop(last_hop_heights.proof_query_height.increment())?; + + let multihop_proofs = self.build_multihop_channel_proofs(&proof_heights)?; + + let multihop_proof_bytes = prost::Message::encode_to_vec(&multihop_proofs); + + let proofs = ibc_relayer_types::proofs::Proofs::new( + CommitmentProofBytes::try_from(multihop_proof_bytes).unwrap(), + None, + None, + None, + None, + last_hop_heights.proof_query_height.increment(), + ) + .map_err(ChannelError::malformed_proof)?; + + // Get signer + let signer = self + .dst_chain() + .get_signer() + .map_err(|e| ChannelError::fetch_signer(self.dst_chain().id(), e))?; + + // Build the domain type message + let new_msg = MsgChannelOpenAck { + port_id: self.dst_port_id().clone(), + channel_id: dst_channel_id.clone(), + counterparty_channel_id: src_channel_id.clone(), + counterparty_version: src_channel.version().clone(), + proofs, + signer, + }; + + msgs.push(new_msg.to_any()); + Ok(msgs) + } + pub fn build_chan_open_ack(&self) -> Result, ChannelError> { // Source and destination channel IDs must be specified let src_channel_id = self @@ -1706,7 +1810,11 @@ impl Channel { fn do_build_chan_open_ack_and_send( channel: &Channel, ) -> Result { - let dst_msgs = channel.build_chan_open_ack()?; + let dst_msgs = if channel.a_side.connection_hops.is_some() { + channel.build_multihop_chan_open_ack()? + } else { + channel.build_chan_open_try()? + }; let tm = TrackedMsgs::new_static(dst_msgs, "ChannelOpenAck"); From f0bfc75401258f0103105e2ed56cbc116c593e8b Mon Sep 17 00:00:00 2001 From: joao Date: Thu, 20 Jun 2024 17:37:54 +1000 Subject: [PATCH 2/3] Fix failing CI tests * Replace incorrect call to `build_chan_open_try()` with a call to `build_chan_open_ack()` within `do_build_chan_open_ack_and_send()` method --- crates/relayer/src/channel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/relayer/src/channel.rs b/crates/relayer/src/channel.rs index d6a716cb5e..ede070079c 100644 --- a/crates/relayer/src/channel.rs +++ b/crates/relayer/src/channel.rs @@ -1813,7 +1813,7 @@ impl Channel { let dst_msgs = if channel.a_side.connection_hops.is_some() { channel.build_multihop_chan_open_ack()? } else { - channel.build_chan_open_try()? + channel.build_chan_open_ack()? }; let tm = TrackedMsgs::new_static(dst_msgs, "ChannelOpenAck"); From 22a5a469f01e68c13db5b06e261bb5f89787f14a Mon Sep 17 00:00:00 2001 From: joao Date: Fri, 21 Jun 2024 14:40:10 +1000 Subject: [PATCH 3/3] Modify `validated_expected_channel()` to support multiple connection hops * Add a more descriptive message to `ChannelError::UnexpectedChannelState` * Improve comments --- crates/relayer-cli/src/commands/tx/channel.rs | 4 +- crates/relayer/src/channel.rs | 38 +++++++++---------- crates/relayer/src/channel/error.rs | 3 +- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/crates/relayer-cli/src/commands/tx/channel.rs b/crates/relayer-cli/src/commands/tx/channel.rs index 16696fe6d2..9374830ba6 100644 --- a/crates/relayer-cli/src/commands/tx/channel.rs +++ b/crates/relayer-cli/src/commands/tx/channel.rs @@ -782,7 +782,7 @@ impl Runnable for TxChanOpenAckCmd { }; // Obtain the counterparty ConnectionId and ChainId for the current connection hop - // towards b_side + // towards b_side/destination let counterparty_conn_id = a_side_hop_connection .counterparty() .connection_id() @@ -916,7 +916,7 @@ impl Runnable for TxChanOpenAckCmd { }); } - // The connection hops were assembled while traversing from --src-chain towards --dst-chain. + // The connection hops were assembled while traversing from --src-chain(a_side) --dst-chain(b_side). // Reverse them to obtain the path from --dst-chain to --src-chain. Single hops remain unchanged // when reversed. b_side_hops.reverse(); diff --git a/crates/relayer/src/channel.rs b/crates/relayer/src/channel.rs index ede070079c..de4bfcce94 100644 --- a/crates/relayer/src/channel.rs +++ b/crates/relayer/src/channel.rs @@ -1003,7 +1003,7 @@ impl Channel { .ok_or_else(ChannelError::missing_counterparty_channel_id)?; // If there is a channel end on the destination chain, the channel end's - // counterparty should match this: + // counterparty should match the specified source chain, i.e, the following: let counterparty = Counterparty::new(self.src_port_id().clone(), self.src_channel_id().cloned()); @@ -1015,12 +1015,19 @@ impl Channel { _ => State::Uninitialized, }; - // The channel configuration as it should be in the destination + // The configuration of the channel end in the destination chain should match the following let dst_expected_channel = ChannelEnd::new( highest_state, self.ordering, counterparty, - vec![self.dst_connection_id().clone()], + // FIXME: This is a temporary workaround while --connection-hops has not yet replaced + // --dst-connection in the tx CLI. If connection_hops are 'None' pass '--dst-connection' + // to the field 'ChannelEnd.connection_hops' for now. + self.b_side + .connection_hops + .as_ref() + .map(|hops| hops.connection_ids()) + .unwrap_or_else(|| vec![self.dst_connection_id().clone()]), Version::empty(), 0, ); @@ -1383,10 +1390,11 @@ impl Channel { ) .map_err(|e| ChannelError::query(self.src_chain().id(), e))?; - // The channel end in src_chain must be in 'Init' state + // The channel end in src_chain must be in 'INIT' state to send a 'MsgChannelOpenTry' if !src_channel.state_matches(&State::Init) { return Err(ChannelError::unexpected_channel_state( src_channel_id.clone(), + self.src_chain().id().clone(), State::Init, *src_channel.state(), )); @@ -1515,10 +1523,11 @@ impl Channel { ) .map_err(|e| ChannelError::query(self.src_chain().id(), e))?; - // The channel end in src_chain must be in `Init` state + // The channel end in src_chain must be in 'INIT' state to send a 'MsgChannelOpenTry' if !src_channel.state_matches(&State::Init) { return Err(ChannelError::unexpected_channel_state( src_channel_id.clone(), + self.src_chain().id().clone(), State::Init, *src_channel.state(), )); @@ -1643,9 +1652,6 @@ impl Channel { .dst_channel_id() .ok_or_else(ChannelError::missing_counterparty_channel_id)?; - // Check that the destination chain will accept the Ack message - // self.validated_expected_channel(ChannelMsgType::OpenAck)?; - // Channel must exist on source let (src_channel, _) = self .src_chain() @@ -1659,14 +1665,8 @@ impl Channel { ) .map_err(|e| ChannelError::query(self.src_chain().id(), e))?; - // // The channel end in src_chain must be in 'Init' state - // if !src_channel.state_matches(&State::Init) { - // return Err(ChannelError::unexpected_channel_state( - // src_channel_id.clone(), - // State::Init, - // *src_channel.state(), - // )); - // } + // Check that the destination chain will accept the MsgChannelOpenAck + self.validated_expected_channel(ChannelMsgType::OpenAck)?; self.dst_chain() .query_connection( @@ -1746,9 +1746,6 @@ impl Channel { .dst_channel_id() .ok_or_else(ChannelError::missing_counterparty_channel_id)?; - // Check that the destination chain will accept the Ack message - self.validated_expected_channel(ChannelMsgType::OpenAck)?; - // Channel must exist on source let (src_channel, _) = self .src_chain() @@ -1762,6 +1759,9 @@ impl Channel { ) .map_err(|e| ChannelError::query(self.src_chain().id(), e))?; + // Check that the destination chain will accept the MsgChannelOpenAck + self.validated_expected_channel(ChannelMsgType::OpenAck)?; + // Connection must exist on destination self.dst_chain() .query_connection( diff --git a/crates/relayer/src/channel/error.rs b/crates/relayer/src/channel/error.rs index 5ed2b99d42..c17396f480 100644 --- a/crates/relayer/src/channel/error.rs +++ b/crates/relayer/src/channel/error.rs @@ -136,10 +136,11 @@ define_error! { UnexpectedChannelState { channel_id: ChannelId, + chain_id: ChainId, expected_state: State, channel_state: State, } - | e | { format_args!("expected state of channel '{}' to be '{}', but found '{}'", e.channel_id, e.expected_state, e.channel_state) }, + | e | { format_args!("expected state of channel '{}' on chain '{}' to be '{}', but found '{}'", e.channel_id, e.chain_id, e.expected_state, e.channel_state) }, MalformedProof [ ProofError ]