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

ICS004 Channel Upgrades - In flight packets #725

Closed
damiannolan opened this issue Apr 29, 2022 · 3 comments
Closed

ICS004 Channel Upgrades - In flight packets #725

damiannolan opened this issue Apr 29, 2022 · 3 comments
Assignees
Labels
improvement Improvement or enhancement to make specs more comprehensible tao Transport, authentication, & ordering layer.

Comments

@damiannolan
Copy link
Member

damiannolan commented Apr 29, 2022

From reading the specification for ICS004 Channel Upgrades I found there to be no mention of what may happen to any number of packets that may try to be sent or are in-flight at the time the upgrade handshake takes place.

Would it be such that the state of a channel-end would change to UPGRADE_INIT or UPGRADE_TRY for example, and this would cause packet recvs to fail at this point in the ibc-go implementation.

I see the code in ibc-go's SendPacket just ensure the channel is not closed. Do we want to allow SendPacket to still be called when an upgrade takes place, so that it could potentially be relayed post upgrade? And if so, is there any concerns around this?

  1. Is there any concerns regarding in-flight packets during a channel upgrade that haven't been explored?
  2. I think it could be potentially useful to mention this in the spec either way, that is, if there is no problems regarding this.

Would be interested to hear the thoughts of the team on this? cc. @AdityaSripal
Feel free to ack and close this issue if there is no problem, thanks!

@AdityaSripal
Copy link
Member

AdityaSripal commented May 2, 2022

Great questions @damiannolan

Yes, it is intended that packets in-flight are "paused" until the upgrade is complete. This is to ensure that packet flow cannot happen while the channel is in a temporarily invalid state (e.g. one end is on v1, and the other is on v2). See discussion here: #621 (comment) (this is for connection upgradability but analogous rationale for channels)

Is there any concerns regarding in-flight packets during a channel upgrade that haven't been explored?

  • Once the upgrade completes, the packet flow of in-flight packets can resume. It's possible that some in-flight packets will have reached their timeout because of the upgrade but that's totally fine.
  • At the channel level, upgrades can only change certain channel struct fields along with the application packet processing logic. So it is possible after an upgrade, in-flight packet data is no longer parsable. This again is not a big issue, since it will result in error acks on the sent packets. If application developers want to avoid this, they should make their upgrades backwards compatible.

I think neither of these are issues but we should definitely discuss this in the spec so that developers are aware of possible

Do we want to allow SendPacket to still be called when an upgrade takes place, so that it could potentially be relayed post upgrade?

I do believe this is still useful since at least then we are only delaying the relaying and processing of packets. Packets can still get queued and then sent out once the upgrade completes. This is a useful feature I believe, and is in-line with what we already have with optimistic sends during the initial channel handshake.

The one issue here, is of course it's possible that some of these SendPackets will be unparsable after the upgrade but this is again an issue we already have and it is neither catastrophic and app developers can choose to avoid it if they want.

This should also be discussed in the spec so that it is visible to app developers.

TL;DR: There aren't issues wrt channel upgradability, but we should document the effect of upgrades on in-flight packets in the spec

@AdityaSripal AdityaSripal self-assigned this May 2, 2022
@crodriguezvega crodriguezvega added the tao Transport, authentication, & ordering layer. label May 3, 2022
@damiannolan
Copy link
Member Author

Thanks for the detailed response @AdityaSripal

@crodriguezvega crodriguezvega added the improvement Improvement or enhancement to make specs more comprehensible label Apr 3, 2023
@crodriguezvega
Copy link
Contributor

Closed by #967.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement or enhancement to make specs more comprehensible tao Transport, authentication, & ordering layer.
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants