ICS 04: fix timeout conditions for ordered (without/with allow timeout) channels #977

crodriguezvega · 2023-05-30T21:07:06Z

closes: #965

crodriguezvega · 2023-05-30T21:09:08Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -964,7 +964,7 @@ function timeoutPacket(
        // ordered channel: check that packet has not been received
        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(nextSequenceRecv <= packet.sequence)


See ibc-go.

I think its clearer to write

abortTransactionUnless(packet.Sequence >= nextSequenceRecv)

crodriguezvega · 2023-05-30T21:10:03Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+    switch channel.order {
+      case ORDERED:
+        // ordered channel: check that packet has not been received
+        abortTransactionUnless(nextSequenceRecv <= packet.sequence)


I changed the ordered to match what the implementation does and what the spec also does for regular packet timeouts.

crodriguezvega · 2023-05-30T21:11:21Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+
+      // NOTE: For ORDERED_ALLOW_TIMEOUT, the relayer must first attempt the receive on the destination chain
+      // before the timeout receipt can be written and subsequently proven on the sender chain in timeoutPacket
+      case ORDERED_ALLOW_TIMEOUT:


I split ORDERED and ORDERED_ALLOW_TIMEOUT because conditions are different.

crodriguezvega · 2023-05-30T21:12:14Z

spec/core/ics-004-channel-and-packet-semantics/README.md

-        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
-        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(nextSequenceRecv > packet.sequence)


For both packet timeouts and for timeout on close the nextSequenceRecv on the counterparty would have been incremented by one before the timeout receipt is written.

I believe this was correct as it was

The reason for changing it is that in line 709 inside recvPacket the nextSequenceRecv is incremented by 1 before the timeout receipt is written:

nextSequenceRecv = nextSequenceRecv + 1 provableStore.set(nextSequenceRecvPath(packet.destPort, packet.destChannel), nextSequenceRecv)

So if I relayer tries to timeout the packet, the nextSequenceRecv in the timeout message will be > than packet.sequence, won't it?

Same thing for the condition checked in timeoutOnClose.

Is this reasoning correct, @AdityaSripal?

@AdityaSripal for timing out a packet if the channel is open, should the condition still be that the relayer must attempt to receive the packet (as the comment above this line says)? If that's the case, then I guess it's ok to not support the situation where packet.sequence is >= nextSequenceRecv.

AdityaSripal · 2023-06-12T15:21:27Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -964,7 +964,7 @@ function timeoutPacket(
        // ordered channel: check that packet has not been received
        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(nextSequenceRecv <= packet.sequence)


I think its clearer to write

abortTransactionUnless(packet.Sequence >= nextSequenceRecv)

AdityaSripal · 2023-06-12T15:35:44Z

spec/core/ics-004-channel-and-packet-semantics/README.md

-        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
-        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(nextSequenceRecv > packet.sequence)


I believe this was correct as it was

AdityaSripal · 2023-06-12T15:36:35Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+      // NOTE: For ORDERED_ALLOW_TIMEOUT, the relayer must first attempt the receive on the destination chain
+      // before the timeout receipt can be written and subsequently proven on the sender chain in timeoutPacket
+      case ORDERED_ALLOW_TIMEOUT:
+        abortTransactionUnless(nextSequenceRecv > packet.sequence)


This should be an equals

…annot attempt receive

crodriguezvega · 2023-10-02T19:46:46Z

spec/core/ics-004-channel-and-packet-semantics/README.md

-        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
-        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(nextSequenceRecv > packet.sequence)


@AdityaSripal for timing out a packet if the channel is open, should the condition still be that the relayer must attempt to receive the packet (as the comment above this line says)? If that's the case, then I guess it's ok to not support the situation where packet.sequence is >= nextSequenceRecv.

crodriguezvega · 2023-10-02T19:48:06Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+        // Otherwise, if packet.sequence < nextSequenceRecv, then the relayer has attempted
+        // to receive the packet on the destination chain, and nextSequenceRecv has been incremented.
+        // In this situation, verify the presence of timeout receipt. 
+        if packet.sequence < nextSequenceRecv {


If packet.sequence >= nextSequenceRecv, then timeout, otherwise verify there is a timeout receipt.

…unction-timeoutpacket-and-timeoutonclose

crodriguezvega · 2023-10-02T20:00:01Z

spec/core/ics-004-channel-and-packet-semantics/README.md

-        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
-        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(packet.sequence < nextSequenceRecv)


@AdityaSripal if a channel is open and the relayer must first attempt to receive the packet on destination chain, I guess it is ok to assume that nextSequenceRecv will have been incremented and therefore is > packet.sequence?

No we will actually want the equals here because I think it makes sense to enforce that timeout happens in order as well.

Should the condition be then abortTransactionUnless(packet.sequence == nextSequenceRecv - 1) since nextSequenceRecv will have been incremented when attempting to receive packet with packet.sequence?

AdityaSripal

Thanks Carlos, I think its still important to ensure ordering in the timeout logic. Perhaps we should even add this to timeoutOnClose. Also I believe the else clause in the ordered_allow_timeout case is missing.

AdityaSripal · 2023-10-05T13:23:26Z

spec/core/ics-004-channel-and-packet-semantics/README.md

-        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
-        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
+        abortTransactionUnless(packet.sequence < nextSequenceRecv)


No we will actually want the equals here because I think it makes sense to enforce that timeout happens in order as well.

AdityaSripal · 2023-10-05T13:30:11Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -1190,8 +1190,7 @@ function timeoutPacket(
        // ordered channel: check that packet has not been received
        // only allow timeout on next sequence so all sequences before the timed out packet are processed (received/timed out)
        // before this packet times out
-        abortTransactionUnless(nextSequenceRecv == packet.sequence)
-
+        abortTransactionUnless(packet.sequence >= nextSequenceRecv)


Again I think this should remain as is. It would enforce that the only packet that gets timed out is the first in line

spec/core/ics-004-channel-and-packet-semantics/README.md

AdityaSripal

Thank you @crodriguezvega !

fix timeout conditions for ordered (without/with allow timeout) channels

30bbb34

crodriguezvega requested review from mpoke, AdityaSripal and cwgoes as code owners May 30, 2023 21:07

crodriguezvega linked an issue May 30, 2023 that may be closed by this pull request

ICS04: Something confusing about function timeoutPacket and timeoutOnClose #965

Closed

crodriguezvega requested review from colin-axner and angbrav as code owners May 30, 2023 21:07

crodriguezvega commented May 30, 2023

View reviewed changes

crodriguezvega mentioned this pull request May 30, 2023

ICS04: Something confusing about function timeoutPacket and timeoutOnClose #965

Closed

AdityaSripal requested changes Jun 12, 2023

View reviewed changes

crodriguezvega added 2 commits October 2, 2023 21:43

fix support for timing out packets if channel is closed and relayer c…

98f5a0f

…annot attempt receive

reverse condition

d7e5d78

crodriguezvega commented Oct 2, 2023

View reviewed changes

Merge branch 'main' into carlos/965-ics04-something-confusing-about-f…

abfbfb5

…unction-timeoutpacket-and-timeoutonclose

crodriguezvega commented Oct 2, 2023

View reviewed changes

fix alignment

3c47477

AdityaSripal requested changes Oct 5, 2023

View reviewed changes

crodriguezvega added 2 commits October 30, 2023 21:04

review comments

dcf39dd

Update README.md

fa37176

AdityaSripal approved these changes Nov 1, 2023

View reviewed changes

AdityaSripal merged commit cd6ae64 into main Nov 1, 2023
2 checks passed

AdityaSripal deleted the carlos/965-ics04-something-confusing-about-function-timeoutpacket-and-timeoutonclose branch November 1, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICS 04: fix timeout conditions for ordered (without/with allow timeout) channels #977

ICS 04: fix timeout conditions for ordered (without/with allow timeout) channels #977

crodriguezvega commented May 30, 2023

crodriguezvega May 30, 2023

AdityaSripal Jun 12, 2023

crodriguezvega May 30, 2023 •

edited

Loading

crodriguezvega May 30, 2023

crodriguezvega May 30, 2023

AdityaSripal Jun 12, 2023

crodriguezvega Jun 13, 2023 •

edited

Loading

crodriguezvega Oct 2, 2023

AdityaSripal Jun 12, 2023

AdityaSripal Jun 12, 2023

AdityaSripal Jun 12, 2023

crodriguezvega Oct 2, 2023

crodriguezvega Oct 2, 2023

crodriguezvega Oct 2, 2023

AdityaSripal Oct 5, 2023

crodriguezvega Oct 31, 2023

AdityaSripal left a comment

AdityaSripal Oct 5, 2023

AdityaSripal Oct 5, 2023

AdityaSripal left a comment

ICS 04: fix timeout conditions for ordered (without/with allow timeout) channels #977

ICS 04: fix timeout conditions for ordered (without/with allow timeout) channels #977

Conversation

crodriguezvega commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

crodriguezvega May 30, 2023 •

edited

Loading

crodriguezvega Jun 13, 2023 •

edited

Loading