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
Add a Stream Migration spec #406
base: master
Are you sure you want to change the base?
Add a Stream Migration spec #406
Changes from 1 commit
4fc768f
b0db416
dac9a8d
37a86ff
6f03926
406171f
99f055e
c0fd69f
eef233e
49c0597
4e1f106
a380dc4
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.
/libp2p/stream-migration/1.0.0
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.
/libp2p/stream-migration/
, we can still version this later if necessary.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.
The protocol name can't include the stream ID. The stream ID is the payload of this protocol.
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.
Why not? This way you wouldn't need a separate payload message.
The initiator would send the stream-migration protocol ID+stream id, then can send the underlying protocol right away without having to wait for a response.
The responder would read the stream migration protocol and ack it (by echoing back as in multistream select. Note this doc doesn't say this part yet.), then continue negotiating the underlying protocol. If the other node for some reason doesn't support stream-migration (even if we thought it did), it would echo back na, and continue as before.
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.
ah, it could be because we cache the protocols seen on the other side, correct?
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.
Yes, exactly. Even worse, we send an Identify Delta messages for every new protocol that we add.
Also, logically speaking, the stream ID is not part of the protocol ID, but it's a payload of the stream migration protocol. Counting the bytes on the wire, it makes no (or at least not more than a few byte) difference if it's in the protocol name or in the payload.
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.
Not sure this was mentioned before: For the sake of evolvability of the protocol in the future, can the stream ID be send embedded in a Protobuf? That way we can add new fields in the future.
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.
Good call out. I did this in the poc, but I’ll update the spec to include this.