From 212a3bfe83c81da56259eeb915c2ec1a00688506 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 6 Feb 2021 18:34:29 +0100 Subject: [PATCH 1/4] Explicit assumption in packet_from_tx_search_response --- CHANGELOG.md | 9 +++- relayer/src/chain/cosmos.rs | 101 +++++++++++++++++++++--------------- 2 files changed, 68 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07120a2582..b27b9158e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ # Changelog -[comment]: <> (## Unreleased Changes) +## Unreleased Changes + +### BUG FIXES: + +- [relayer-cli] + - Fix wrong acks sent with `tx raw packet-ack` in a 3-chain setup ([#614]) + +[#614]: https://github.com/informalsystems/ibc-rs/issues/614 ## v0.1.0 *February 4, 2021* diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index cb82aa61e9..254d50396c 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -6,6 +6,7 @@ use std::{ use anomaly::fail; use bitcoin::hashes::hex::ToHex; use crossbeam_channel as channel; +use ibc::ics04_channel::events as ChannelEvents; use prost::Message; use prost_types::Any; use tendermint::abci::Path as TendermintABCIPath; @@ -688,7 +689,7 @@ impl Chain for CosmosSDKChain { )) .unwrap(); // todo - let mut events = packet_from_tx_search_response(&request, *seq, &response)? + let mut events = packet_from_tx_search_response(&request, *seq, response)? .map_or(vec![], |v| vec![v]); result.append(&mut events); } @@ -839,56 +840,74 @@ fn packet_query(request: &QueryPacketEventDataRequest, seq: &Sequence) -> Result )) } -// Extract the packet events from the query_tx RPC response. The response includes the full set of events -// from the Tx-es where there is at least one request query match. -// For example, the query request asks for the Tx for packet with sequence 3, and both 3 and 4 were -// committed in one Tx. In this case the response includes the events for 3 and 4. +// Extract the packet events from the query_txs RPC response. For any given +// packet query, there is at most one Tx matching such query. However, a single +// Tx may contain several events, and a single one must match the packet query. +// For example, if we're querying for the packet with sequence 3 and this packet +// was committed a some Tx along with the packet with sequence 4, the response +// will include both packets. For this reason, we iterate all packets in the Tx +// until the one we're looking for is found. fn packet_from_tx_search_response( request: &QueryPacketEventDataRequest, seq: Sequence, - response: &tendermint_rpc::endpoint::tx_search::Response, + mut response: tendermint_rpc::endpoint::tx_search::Response, ) -> Result, Error> { - // TODO: remove loop as `response.txs.len() <= 1` - for r in response.txs.iter() { - let height = r.height; - if height.value() > request.height.revision_height { - continue; - } - - for e in r.tx_result.events.iter() { - if e.type_str != request.event_id.as_str() { - continue; - } - - let res = from_tx_response_event(e); - if res.is_none() { - continue; - } - let event = res.unwrap(); - let packet = match &event { - IBCEvent::SendPacketChannel(send_ev) => Some(&send_ev.packet), - IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(&ack_ev.packet), - _ => None, - }; - - if packet.is_none() { - continue; + assert!( + response.txs.len() <= 1, + "packet_from_tx_search_response: unexpected number of txs" + ); + match response.txs.pop() { + None => Ok(None), + Some(r) => { + let height = r.height; + if height.value() > request.height.revision_height { + return Ok(None); } - let packet = packet.unwrap(); - if packet.source_port != request.source_port_id - || packet.source_channel != request.source_channel_id - || packet.destination_port != request.destination_port_id - || packet.destination_channel != request.destination_channel_id - || packet.sequence != seq - { - continue; + let mut matching = Vec::new(); + for e in r.tx_result.events { + assert_eq!( + e.type_str, + request.event_id.as_str(), + "packet_from_tx_search_response: unexpected event type" + ); + + let res = ChannelEvents::try_from_tx(&e); + if res.is_none() { + continue; + } + let event = res.unwrap(); + let packet = match &event { + IBCEvent::SendPacketChannel(send_ev) => Some(&send_ev.packet), + IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(&ack_ev.packet), + _ => None, + }; + + if packet.is_none() { + continue; + } + + let packet = packet.unwrap(); + if packet.source_port != request.source_port_id + || packet.source_channel != request.source_channel_id + || packet.destination_port != request.destination_port_id + || packet.destination_channel != request.destination_channel_id + || packet.sequence != seq + { + continue; + } + + matching.push(event); } - return Ok(Some(event)); + assert_eq!( + matching.len(), + 1, + "packet_from_tx_search_response: unexpected number of matching packets" + ); + Ok(matching.pop()) } } - Ok(None) } /// Perform a generic `abci_query`, and return the corresponding deserialized response data. From e07f7fd95212ee8ebac31e9c501c76563c63c21f Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 6 Feb 2021 18:37:19 +0100 Subject: [PATCH 2/4] Minimize diff --- relayer/src/chain/cosmos.rs | 91 ++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 46 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 254d50396c..ed3a8149ce 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -856,57 +856,56 @@ fn packet_from_tx_search_response( response.txs.len() <= 1, "packet_from_tx_search_response: unexpected number of txs" ); - match response.txs.pop() { - None => Ok(None), - Some(r) => { - let height = r.height; - if height.value() > request.height.revision_height { - return Ok(None); + if let Some(r) = response.txs.pop() { + let height = r.height; + if height.value() > request.height.revision_height { + return Ok(None); + } + + let mut matching = Vec::new(); + for e in r.tx_result.events { + assert_eq!( + e.type_str, + request.event_id.as_str(), + "packet_from_tx_search_response: unexpected event type" + ); + + let res = ChannelEvents::try_from_tx(&e); + if res.is_none() { + continue; + } + let event = res.unwrap(); + let packet = match &event { + IBCEvent::SendPacketChannel(send_ev) => Some(&send_ev.packet), + IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(&ack_ev.packet), + _ => None, + }; + + if packet.is_none() { + continue; } - let mut matching = Vec::new(); - for e in r.tx_result.events { - assert_eq!( - e.type_str, - request.event_id.as_str(), - "packet_from_tx_search_response: unexpected event type" - ); - - let res = ChannelEvents::try_from_tx(&e); - if res.is_none() { - continue; - } - let event = res.unwrap(); - let packet = match &event { - IBCEvent::SendPacketChannel(send_ev) => Some(&send_ev.packet), - IBCEvent::WriteAcknowledgementChannel(ack_ev) => Some(&ack_ev.packet), - _ => None, - }; - - if packet.is_none() { - continue; - } - - let packet = packet.unwrap(); - if packet.source_port != request.source_port_id - || packet.source_channel != request.source_channel_id - || packet.destination_port != request.destination_port_id - || packet.destination_channel != request.destination_channel_id - || packet.sequence != seq - { - continue; - } - - matching.push(event); + let packet = packet.unwrap(); + if packet.source_port != request.source_port_id + || packet.source_channel != request.source_channel_id + || packet.destination_port != request.destination_port_id + || packet.destination_channel != request.destination_channel_id + || packet.sequence != seq + { + continue; } - assert_eq!( - matching.len(), - 1, - "packet_from_tx_search_response: unexpected number of matching packets" - ); - Ok(matching.pop()) + matching.push(event); } + + assert_eq!( + matching.len(), + 1, + "packet_from_tx_search_response: unexpected number of matching packets" + ); + Ok(matching.pop()) + } else { + Ok(None) } } From 3e95014ed6e1c9e11cff4eb2fe5cbe6df6d181d5 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 6 Feb 2021 18:44:08 +0100 Subject: [PATCH 3/4] Improve wording --- relayer/src/chain/cosmos.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index ed3a8149ce..74ff383fc5 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -841,12 +841,12 @@ fn packet_query(request: &QueryPacketEventDataRequest, seq: &Sequence) -> Result } // Extract the packet events from the query_txs RPC response. For any given -// packet query, there is at most one Tx matching such query. However, a single -// Tx may contain several events, and a single one must match the packet query. +// packet query, there is at most one Tx matching such query. Moreover, a Tx may +// contain several events, but a single one must match the packet query. // For example, if we're querying for the packet with sequence 3 and this packet -// was committed a some Tx along with the packet with sequence 4, the response -// will include both packets. For this reason, we iterate all packets in the Tx -// until the one we're looking for is found. +// was committed in some Tx along with the packet with sequence 4, the response +// will include both packets. For this reason, we iterate all packets in the Tx, +// searching for those that match (which must be a single one). fn packet_from_tx_search_response( request: &QueryPacketEventDataRequest, seq: Sequence, From b0a9aa0a36126cbeec414f5ac659b997cf594594 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 6 Feb 2021 20:29:08 +0100 Subject: [PATCH 4/4] Remove added assert --- relayer/src/chain/cosmos.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 74ff383fc5..fc831dfb6c 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -864,11 +864,9 @@ fn packet_from_tx_search_response( let mut matching = Vec::new(); for e in r.tx_result.events { - assert_eq!( - e.type_str, - request.event_id.as_str(), - "packet_from_tx_search_response: unexpected event type" - ); + if e.type_str != request.event_id.as_str() { + continue; + } let res = ChannelEvents::try_from_tx(&e); if res.is_none() {