-
Notifications
You must be signed in to change notification settings - Fork 398
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
ICS004: Remove last packet sent #1026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 🧹
Co-authored-by: Carlos Rodriguez <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the core idea here is that we can remove the lastPacketSent
field because the following statements remain true:
-
'Any packet sent to the channel with a packet sequence number greater than the
lastPacketSent
will be rejected until the upgrade is complete.'- This statement remains valid because, even without the
lastPacketSent
field, the transition to theFLUSHING
state during thestartFlushUpgradeHandshake
operation effectively blocks thesendPacket
function. As a result, no additional packets can be sent over this channel during the upgrade process. - Considering that we were storing the
lastPacketSent
when we were moving to FLUSHING, the system withoutlastPacketSent
achieve the same result.
- This statement remains valid because, even without the
-
We still have a way to check if there are pending inflight packets.
pendingInflightPacket
does not depend onlastPacketSent
. Indeed is designed to monitor the pending in-flight packet verifying the presence/absence of packet commitment given that packet commitments are deleted only when the packet lifecycle is complete. Therefore, if a packet commitment exists on the sender's chain, it indicates that the packet lifecycle is still incomplete.
Am I missing something? If all the above is correct, IMO, removing lastPacketSent
is safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also closes #1015 as it won't be required after this PR 👍
Thanks @AdityaSripal ❤️
closes: #1024
closes: #1015
closes: #1023