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

Sequence for Connection IDs, RETIRE #1829

Merged
merged 3 commits into from
Oct 3, 2018
Merged

Sequence for Connection IDs, RETIRE #1829

merged 3 commits into from
Oct 3, 2018

Conversation

MikeBishop
Copy link
Contributor

Suggestions for your PR, as discussed.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Oct 3, 2018
@@ -1249,8 +1252,11 @@ using the NEW_CONNECTION_ID frame ({{frame-new-connection-id}}).
### Issuing Connection IDs

The initial connection ID issued by an endpoint is the Source Connection ID
during the handshake. Subsequent connection IDs are communicated to the peer
using NEW_CONNECTION_ID frames ({{frame-new-connection-id}}).
during the handshake. The sequence number of the initial connection ID is 0. If
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it really necessary to allocate IDs to connection IDs during the handshake? To me, it makes more sense to only allocate IDs to those that are post-handshake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would result in complications with non-unique 5-tuples if I understand you correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the handshake connection IDs exist, but they don't need an identifier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the id of the id, that is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid that if the entire CID is sent in the RETIRE_CID frame. That would mean, instead, that issuers need to stop using the contained CID for the current connection iff they are and absorb duplicates by silently consuming RETIRE messages for CIDs which aren't associated with the connection.

By having the sequence number in the RETIRE_CID frame, we make it shorter (one varint) and enable using the same deduplicating code as the consumer of NEW_CID messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the use of seq num here. It does serve the dedup purpose, and is easier on implementations as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these are given numbers so they can be retired? If so, that makes sense.

the preferred_address transport parameter is sent, the sequence number of the
supplied connection ID is 1. Subsequent connection IDs are communicated to the
peer using NEW_CONNECTION_ID frames ({{frame-new-connection-id}}), and the
sequence number on each newly-issued connection ID MUST increase by 1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should say something about CIDs on Retry (that they don't really count).

@janaiyengar janaiyengar merged commit 391641e into ncid Oct 3, 2018
janaiyengar added a commit that referenced this pull request Oct 3, 2018
* Add sequence number to NCID frame

* Sequence for Connection IDs, RETIRE (#1829)

* Connection ID changes

* Discuss pre-handshake CIDs

* not contains
@martinthomson martinthomson deleted the ncid_bis branch October 26, 2018 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants