Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ICS3: Add Connection Upgrade Spec #621
ICS3: Add Connection Upgrade Spec #621
Changes from 4 commits
e0fa2e1
cb0b5c5
067956e
dc00b01
82f7286
26169bd
b7ab84d
d71f150
e398dbb
b92cced
e7ff7bf
bd795ae
da255ad
d5588c8
3d30b28
37916cd
f483d1f
7239327
6f7f572
758e5ab
67f2a03
c40ab29
a152627
6a59321
a4c9fdc
7359894
849059a
c261dd3
1948a82
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Interesting, I'd think you'd want to leave the connection unchanged until the upgrade process successfully completes. Otherwise you could potentially disrupt packet flow if the connection is in UPGRADE_INIT instead of OPEN?
I'd expect you to store the proposed upgrade connection under a proposed upgrade connection path. Use that for proofs and then if successful update the connection on ack/confirm?
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.
I don't want the case where chainA has its connectionEnd OPEN with the old parameters, and chainB has its connectionEnd OPEN with the new parameters as that may cause unexpected consequences.
If a connection is
OPEN
on both ends it should be a valid connection always.So either both ends are
OPEN
running the old parameters, or both ends areOPEN
running the new parameters.The only way to accomplish this is to temporarily close the connection on both sides (INIT and TRY), then reopen after the upgrade is complete on either end (ACK and CONFIRM).
This does mean packet flow is temporarily halted but i believe this is fine. Once the connection is back up, the relayers can either receive or timeout the pending packets
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.
This is also why I specify a timeout, because I don't want the "temporary pause" to last arbitrarily long
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.
yeah, this makes sense to me
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.
Before changing the local connection state to OPEN, shouldn't we check again that the remote connection is compatible with the local one? During
connUpgradeAck
, the remote side could restore the connection, which means moving to a connection with state OPEN. Just verifying that the remote side is OPEN may not be enough.connUpgradeConfirm
should also contain a proof thatUpgradeError
was not set by the remote side.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.
Ahh thank you, nice catch