ICS4: Add ORDERED_ALLOW_TIMEOUT channel type #636
Conversation
| @@ -624,7 +628,7 @@ function recvPacket( | |||
|
|
|||
| // all assertions passed (except sequence check), we can alter state | |||
|
|
|||
| if (channel.order === ORDERED) { | |||
| if (channel.order === ORDERED || channel.order == ORDERED_ALLOW_TIMEOUT) { | |||
| nextSequenceRecv = provableStore.get(nextSequenceRecvPath(packet.destPort, packet.destChannel)) | |||
There was a problem hiding this comment.
I don't understand. If packet sequence 5 times out, even if the channel doesn't close, I don't see any changes here that allow packet sequence 6 to be received
There was a problem hiding this comment.
Yes great catch, this makes the changes less trivial as we need to somehow communicate to destination chain the sequences that have been timed out
| nextSequenceRecv = nextSequenceRecv + 1 | ||
| provableStore.set(nextSequenceRecvPath(packet.destPort, packet.destChannel), nextSequenceRecv) |
There was a problem hiding this comment.
this should be the nextSequenceAck (as it is on the sender side)
|
As colin mentioned this feature will be more complicated than first envisioned. The ack sequence can be correctly incremented when a timeout occurs. However, the receive sequence needs to be incremented on the counterparty chain. Thus, the receiving chain needs to have this information propogated back to it somehow. This will probably require some slightly different relayer behaviour to enable. e.g. We might have to relay the ack sequence to the receiving chain before being able to receive the next packet after timeout. Though, async acks complicates this |
AdityaSripal
changed the title
| // unsupported channel type | ||
| abortTransactionUnless(true) | ||
| } | ||
|
|
||
| // log that a packet has been received |
There was a problem hiding this comment.
As a side note about ICS4, shouldn't we call here the OnRecvPacket() callback function and in case it returns a non-nil ACK, invoke writeAcknowledgement?
| switch channel.order { | ||
| case ORDERED: | ||
| // ordered channel: check that packet has not been received | ||
| abortTransactionUnless(nextSequenceRecv <= packet.sequence) |
There was a problem hiding this comment.
Is not related to the ORDERED_ALLOW_TIMEOUT channels, but shouldn't this condition be abortTransactionUnless(nextSequenceRecv == packet.sequence)? Why do we allow timeout messages regarding future packets. Maybe the packet with nextSequenceRecv will still be received on time, even if another packet with e.g., nextSequenceRecv+1 has timed out.
There was a problem hiding this comment.
Hmm yea that makes sense to me.
| // 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(connection.verifyPacketReceipt( |
There was a problem hiding this comment.
In order for the following property to hold, we need to check for the sequence here, similar to the ORDERED case.
On ordered_allow_timeout channels, packets should be sent and received in the same order: if packet x is sent before packet y by a channel end on chain
A, packet x must be received or timed out before packet y by the corresponding channel end on chainB.
This means that a packet with sequence e.g., 5 cannot be timed out while we are still waiting for an ACK from sequence e.g., 3
There was a problem hiding this comment.
Addressed on line 898
| - On ordered channels, packets should be sent and received in the same order: if packet *x* is sent before packet *y* by a channel end on chain `A`, packet *x* must be received before packet *y* by the corresponding channel end on chain `B`. | ||
| - On unordered channels, packets may be sent and received in any order. Unordered packets, like ordered packets, have individual timeouts specified in terms of the destination chain's height. | ||
| - On *ordered* channels, packets should be sent and received in the same order: if packet *x* is sent before packet *y* by a channel end on chain `A`, packet *x* must be received before packet *y* by the corresponding channel end on chain `B`. If packet *x* is sent before packet *y* by a channel and packet *x* is timed out; then packet *y* and any packet sent after *x* cannot be received. | ||
| - On *ordered_allow_timeout* channels, packets should be sent and received in the same order: if packet *x* is sent before packet *y* by a channel end on chain `A`, packet *x* must be received **or** timed out before packet *y* by the corresponding channel end on chain `B`. |
There was a problem hiding this comment.
@AdityaSripal Does this mean that if a packet x times out, any packet y that was sent after x cannot be received before the timeout period of x elapses?
There was a problem hiding this comment.
Correct. Is it unclear?
| switch channel.order { | ||
| case ORDERED: | ||
| // ordered channel: check that packet has not been received | ||
| // only allow timeout on next sequence so all sequences before the timed out packet can be received |
There was a problem hiding this comment.
This comment is a bit unclear. Did you mean to say "all sequences AFTER the timed out packet can be received"?
There was a problem hiding this comment.
I've clarified. Let me know if it's still unclear
Allow for ORDERED_ALLOW_TIMEOUT, by writing a timeout receipt on destination chain if packet is received after timeout. Once the timeout receipt is written, it can be proven on counterparty chain to timeout packet without closing the channel