Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove faulty receipt check during recv_packet_validate #1337

Merged
merged 3 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- [ibc-core] Remove faulty receipt check during `recv_packet_validate`
([#1336](https://github.com/cosmos/ibc-rs/issues/1336)).
19 changes: 6 additions & 13 deletions ibc-core/ics04-channel/src/handler/recv_packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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.
seanchen1991 marked this conversation as resolved.
Show resolved Hide resolved

// Case where the recvPacket is successful and an
// acknowledgement will be written (not a no-op)
validate_write_acknowledgement(ctx_b, msg)?;
Expand Down
15 changes: 15 additions & 0 deletions ibc-core/ics04-channel/types/src/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
}

/// 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(
Expand All @@ -36,6 +40,17 @@
#[derive(Clone, Debug)]
pub enum Receipt {
Ok,
None,

Check warning on line 43 in ibc-core/ics04-channel/types/src/packet.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/types/src/packet.rs#L43

Added line #L43 was not covered by tests
}

impl Receipt {
pub fn is_ok(&self) -> bool {
matches!(self, Receipt::Ok)
}

pub fn is_none(&self) -> bool {
matches!(self, Receipt::None)
}

Check warning on line 53 in ibc-core/ics04-channel/types/src/packet.rs

View check run for this annotation

Codecov / codecov/patch

ibc-core/ics04-channel/types/src/packet.rs#L51-L53

Added lines #L51 - L53 were not covered by tests
}

impl core::fmt::Display for PacketMsgType {
Expand Down
9 changes: 8 additions & 1 deletion ibc-core/ics24-host/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,14 @@ pub trait ValidationContext {
commitment_path: &CommitmentPath,
) -> Result<PacketCommitment, ContextError>;

/// 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<Receipt, ContextError>;

/// Returns the packet acknowledgement for the given store path
Expand Down
2 changes: 1 addition & 1 deletion ibc-query/src/core/channel/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@

// Receipt only has one enum
seanchen1991 marked this conversation as resolved.
Show resolved Hide resolved
// 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)?;

Check warning on line 284 in ibc-query/src/core/channel/query.rs

View check run for this annotation

Codecov / codecov/patch

ibc-query/src/core/channel/query.rs#L284

Added line #L284 was not covered by tests

let proof_height = match request.query_height {
Some(height) => height,
Expand Down
20 changes: 7 additions & 13 deletions ibc-testkit/src/testapp/ibc/core/core_ctx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,20 +211,14 @@
}

fn get_packet_receipt(&self, receipt_path: &ReceiptPath) -> Result<Receipt, ContextError> {
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)

Check warning on line 218 in ibc-testkit/src/testapp/ibc/core/core_ctx.rs

View check run for this annotation

Codecov / codecov/patch

ibc-testkit/src/testapp/ibc/core/core_ctx.rs#L218

Added line #L218 was not covered by tests
} else {
Ok(Receipt::None)
}
}

fn get_packet_acknowledgement(
Expand Down
3 changes: 3 additions & 0 deletions tests-integration/tests/core/ics04_channel/recv_packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading