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

Packet number echo with fixed-length, 32-bit packet and echo numbers #393

Closed

Conversation

britram
Copy link
Contributor

@britram britram commented Mar 13, 2017

Version of #391 (replacing #368), adds optional packet number echo to the short header, but makes both packet number and packet number echo fields in the short header 32 bits. Implemented largely by removing complexity from #391.

and/or timestamps.

The Packet Number Echo field SHOULD be present on 1-RTT packets containing at
least one ACK frame (see {{frame-ack}}). For efficiency's sake, it MAY be
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason hat the Echo should be in packets containing ACK frames? Since the Echo is only used by middleboxes, for whom the payload is opaque anyway, it shouldn't matter which packet the Echo is sent on.
Even though it doesn't reveal a lot about the packet contents, I'd prefer to not divulge any information about the frame types contained in a packet, unless there's a compelling reason to do so.

The Packet Number Echo field SHOULD be present on 1-RTT packets containing at
least one ACK frame (see {{frame-ack}}). For efficiency's sake, it MAY be
omitted if a 1-RTT packet containing an ACK frame has already been sent during a
time interval equal to the sender's current estimate of the RTT.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a big difference. When a peer is sending stream data, the receiver will generate an ACK for every two packets received. On high bandwidth links, this means that a lot of ACKs are sent per RTT.
I think we should be explicit about this and just require one Echo per RTT.

Copy link
Contributor

Choose a reason for hiding this comment

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

or alternatively, you could remove the largest_acked out of the ACK frame and require this on all packets that carry an ACK frame.

wire, only the least significant 32 bits of the packet number are transmitted
over the wire. The actual packet number for each packet is reconstructed at the
receiver based on the largest packet number received on a successfully
authenticated packet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for a receiver to reconstruct the echoed packet number? If I understand this PR correctly, this value is of no interest for the end points, but only for middleboxes.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. As Brian pointed out, this change refers to the actual packet number. I assumed it referred to the Echo, since there were changes in this PR (which probably need a separate issue then, don't they?).

@britram
Copy link
Contributor Author

britram commented Mar 13, 2017 via email

@britram
Copy link
Contributor Author

britram commented Mar 13, 2017 via email

@janaiyengar janaiyengar added the needs-discussion An issue that needs more discussion before we can resolve it. label Mar 13, 2017
@britram
Copy link
Contributor Author

britram commented Mar 15, 2017

thinking about it a bit more, I think the "do we want variable length packet numbers?" discussion is completely orthogonal to the "do we want packet number echo?" discussion, so let's talk about this in #391 for now...

@britram
Copy link
Contributor Author

britram commented Jun 14, 2017

Let's talk about this on #631 -- in any case #609 is a better proposal for the RTT part of this.

@britram britram closed this Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport needs-discussion An issue that needs more discussion before we can resolve it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants