From 3055776e895bba32551ebef11b1cbdac31f0f620 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 12 Sep 2024 11:51:01 -0700 Subject: [PATCH 1/3] fix: remove faulty receipt check + improve docs around --- .../ics04-channel/src/handler/recv_packet.rs | 19 ++++++------------ ibc-core/ics04-channel/types/src/packet.rs | 15 ++++++++++++++ ibc-core/ics24-host/src/context.rs | 9 ++++++++- ibc-query/src/core/channel/query.rs | 2 +- ibc-testkit/src/testapp/ibc/core/core_ctx.rs | 20 +++++++------------ .../tests/core/ics04_channel/recv_packet.rs | 3 +++ 6 files changed, 40 insertions(+), 28 deletions(-) diff --git a/ibc-core/ics04-channel/src/handler/recv_packet.rs b/ibc-core/ics04-channel/src/handler/recv_packet.rs index aa22d3ec9c..457a3513f4 100644 --- a/ibc-core/ics04-channel/src/handler/recv_packet.rs +++ b/ibc-core/ics04-channel/src/handler/recv_packet.rs @@ -50,7 +50,7 @@ where let packet = &msg.packet; let receipt_path_on_b = ReceiptPath::new(&packet.port_id_on_b, &packet.chan_id_on_b, packet.seq_on_a); - ctx_b.get_packet_receipt(&receipt_path_on_b).is_ok() + ctx_b.get_packet_receipt(&receipt_path_on_b)?.is_ok() } Order::Ordered => { let seq_recv_path_on_b = @@ -248,18 +248,11 @@ where } } Order::Unordered => { - let receipt_path_on_b = ReceiptPath::new( - &msg.packet.port_id_on_a, - &msg.packet.chan_id_on_a, - msg.packet.seq_on_a, - ); - let packet_rec = ctx_b.get_packet_receipt(&receipt_path_on_b); - match packet_rec { - Ok(_receipt) => {} - Err(ContextError::PacketError(PacketError::PacketReceiptNotFound { sequence })) - if sequence == msg.packet.seq_on_a => {} - Err(e) => return Err(e), - } + // Note: We don't check for the packet receipt here because another + // relayer may have already relayed the packet. If that's the case, + // we want to avoid failing the transaction and consuming + // unnecessary fees. Therefore, IBC core treats this as a no-op. + // Case where the recvPacket is successful and an // acknowledgement will be written (not a no-op) validate_write_acknowledgement(ctx_b, msg)?; diff --git a/ibc-core/ics04-channel/types/src/packet.rs b/ibc-core/ics04-channel/types/src/packet.rs index d07746b773..53feb671d4 100644 --- a/ibc-core/ics04-channel/types/src/packet.rs +++ b/ibc-core/ics04-channel/types/src/packet.rs @@ -20,6 +20,10 @@ pub enum PacketMsgType { } /// Packet receipt, used over unordered channels. +/// +/// If the receipt is present in the host's state, it's marked as `Ok`, +/// indicating the packet has already been processed. If the receipt is absent, +/// it's marked as `None`, meaning the packet has not been received. #[cfg_attr( feature = "parity-scale-codec", derive( @@ -36,6 +40,17 @@ pub enum PacketMsgType { #[derive(Clone, Debug)] pub enum Receipt { Ok, + None, +} + +impl Receipt { + pub fn is_ok(&self) -> bool { + matches!(self, Receipt::Ok) + } + + pub fn is_none(&self) -> bool { + matches!(self, Receipt::None) + } } impl core::fmt::Display for PacketMsgType { diff --git a/ibc-core/ics24-host/src/context.rs b/ibc-core/ics24-host/src/context.rs index da858968d6..44451c4f53 100644 --- a/ibc-core/ics24-host/src/context.rs +++ b/ibc-core/ics24-host/src/context.rs @@ -112,7 +112,14 @@ pub trait ValidationContext { commitment_path: &CommitmentPath, ) -> Result; - /// Returns the packet receipt for the given store path + /// Returns the packet receipt for the given store path. This receipt is + /// used to acknowledge the successful processing of a received packet, and + /// must not be pruned. + /// + /// If the receipt is present in the host's state, return `Receipt::Ok`, + /// indicating the packet has already been processed. If the receipt is + /// absent, return `Receipt::None`, indicating the packet has not been + /// received. fn get_packet_receipt(&self, receipt_path: &ReceiptPath) -> Result; /// Returns the packet acknowledgement for the given store path diff --git a/ibc-query/src/core/channel/query.rs b/ibc-query/src/core/channel/query.rs index 793ce57008..c6f01eb4f8 100644 --- a/ibc-query/src/core/channel/query.rs +++ b/ibc-query/src/core/channel/query.rs @@ -281,7 +281,7 @@ where // Receipt only has one enum // Unreceived packets are not stored - let packet_receipt_data = ibc_ctx.get_packet_receipt(&receipt_path); + let packet_receipt_data = ibc_ctx.get_packet_receipt(&receipt_path)?; let proof_height = match request.query_height { Some(height) => height, diff --git a/ibc-testkit/src/testapp/ibc/core/core_ctx.rs b/ibc-testkit/src/testapp/ibc/core/core_ctx.rs index a475f90d7d..e6ee86872f 100644 --- a/ibc-testkit/src/testapp/ibc/core/core_ctx.rs +++ b/ibc-testkit/src/testapp/ibc/core/core_ctx.rs @@ -211,20 +211,14 @@ where } fn get_packet_receipt(&self, receipt_path: &ReceiptPath) -> Result { - Ok(self + if self .packet_receipt_store - .is_path_set( - StoreHeight::Pending, - &ReceiptPath::new( - &receipt_path.port_id, - &receipt_path.channel_id, - receipt_path.sequence, - ), - ) - .then_some(Receipt::Ok) - .ok_or(PacketError::PacketReceiptNotFound { - sequence: receipt_path.sequence, - })?) + .is_path_set(StoreHeight::Pending, receipt_path) + { + Ok(Receipt::Ok) + } else { + Ok(Receipt::None) + } } fn get_packet_acknowledgement( diff --git a/tests-integration/tests/core/ics04_channel/recv_packet.rs b/tests-integration/tests/core/ics04_channel/recv_packet.rs index de8d1b2870..e86d815bef 100644 --- a/tests-integration/tests/core/ics04_channel/recv_packet.rs +++ b/tests-integration/tests/core/ics04_channel/recv_packet.rs @@ -142,6 +142,9 @@ fn recv_packet_validate_happy_path(fixture: Fixture) { packet.seq_on_a, ); + // Note: For unordered channels, there's no need to set a packet receipt. + // The validation will pass whether the receipt exists or not. + let msg_envelope = MsgEnvelope::from(PacketMsg::from(msg)); let res = validate(&context.ibc_store, &router, msg_envelope); From d3edff3a024a9fe0b5f9e13c59a8febdeec611f4 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 12 Sep 2024 12:06:12 -0700 Subject: [PATCH 2/3] chore: add unclog --- ...6-remove-faulty-receipt-check-during-recv-packet-validate.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1336-remove-faulty-receipt-check-during-recv-packet-validate.md diff --git a/.changelog/unreleased/bug-fixes/1336-remove-faulty-receipt-check-during-recv-packet-validate.md b/.changelog/unreleased/bug-fixes/1336-remove-faulty-receipt-check-during-recv-packet-validate.md new file mode 100644 index 0000000000..ea9421ae4e --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1336-remove-faulty-receipt-check-during-recv-packet-validate.md @@ -0,0 +1,2 @@ +- [ibc-core] Remove faulty receipt check during `recv_packet_validate` + ([#1336](https://github.com/cosmos/ibc-rs/issues/1336)). From 6a66300d502f91a88666f53ef97b529844c1b43e Mon Sep 17 00:00:00 2001 From: Sean Chen Date: Thu, 12 Sep 2024 14:17:56 -0500 Subject: [PATCH 3/3] Clarify some comments --- ibc-core/ics04-channel/src/handler/recv_packet.rs | 2 +- ibc-query/src/core/channel/query.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/ibc-core/ics04-channel/src/handler/recv_packet.rs b/ibc-core/ics04-channel/src/handler/recv_packet.rs index 457a3513f4..781b142f06 100644 --- a/ibc-core/ics04-channel/src/handler/recv_packet.rs +++ b/ibc-core/ics04-channel/src/handler/recv_packet.rs @@ -251,7 +251,7 @@ where // Note: We don't check for the packet receipt here because another // relayer may have already relayed the packet. If that's the case, // we want to avoid failing the transaction and consuming - // unnecessary fees. Therefore, IBC core treats this as a no-op. + // unnecessary fees. // Case where the recvPacket is successful and an // acknowledgement will be written (not a no-op) diff --git a/ibc-query/src/core/channel/query.rs b/ibc-query/src/core/channel/query.rs index c6f01eb4f8..475aaefacc 100644 --- a/ibc-query/src/core/channel/query.rs +++ b/ibc-query/src/core/channel/query.rs @@ -279,7 +279,6 @@ where { let receipt_path = ReceiptPath::new(&request.port_id, &request.channel_id, request.sequence); - // Receipt only has one enum // Unreceived packets are not stored let packet_receipt_data = ibc_ctx.get_packet_receipt(&receipt_path)?;