-
Notifications
You must be signed in to change notification settings - Fork 205
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
Cleanup packet number text #288
Conversation
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.
Nice clarifications.
draft-ietf-quic-transport.md
Outdated
a packet number size that is able to represent 4 times more packet numbers than | ||
the endpoint has currently outstanding. A packet is outstanding if it sent but | ||
has neither been acknowledged nor been marked as lost (see {{QUIC-RECOVERY}}). | ||
As a result, the size of the packet number encoding is at least two more than |
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.
size in bits?
draft-ietf-quic-transport.md
Outdated
continue to have the VERSION flag set and MUST include the new negotiated | ||
protocol version. | ||
version, the client selects that version and reattempts to created a connection | ||
using that version. The client MUST update packet numbers when attempting to |
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'm not sure what "client MUST update packet numbers" means?
I would change "reattempts to created a connection using that version." to "attempts to create a new connection using that version." and then remove this sentence, because I think it's clear a new connection has a new packet number space.
I think we should specify whether the connection id should be the same? I believe it should be, to ensure the routing is the same.
We should also specify what happens if the version negotiation packet includes the version you just tried to use. I believe we should not try again.
draft-ietf-quic-transport.md
Outdated
The packet number is a 64-bit unsigned number and is used as part of a | ||
cryptographic nonce for packet encryption. Each endpoint maintains a separate | ||
packet number for sending and receiving. The packet number for sending MUST | ||
increase by one after sending any packet. |
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.
At least one?
draft-ietf-quic-transport.md
Outdated
To enable unambiguous reconstruction of the packet number, an endpoint MUST use | ||
a packet number size that is able to represent 4 times more packet numbers than | ||
the endpoint has currently outstanding. A packet is outstanding if it sent but | ||
has neither been acknowledged nor been marked as lost (see {{QUIC-RECOVERY}}). |
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.
Do we really want to define "outstanding" by the number of packets w/o ACKs? I'd rather define it by the number of packet numbers between the current packet number and the earliest packet w/o ACKs.
For a high bandwidth, high latency connection, if I have packet 10 "delayed", but packets 11-1033 have been acked, do I really only have 1 packet "outstanding" and therefore can use the smallest number of bits to encode the following packet number?
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.
@igorlord, I agree, as does Ryan. But I'm trying to keep this PR strictly editorial and I think that this is what the original text is trying to say. I will create a follow-up PR that makes the change you suggest.
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.
LGTM -- the new text is much better!
This builds on #283 and it includes fixes the infidelities I noticed but did not fix during the process of creating that PR.
This is intended to be editorial, but it does remove the use of "in flight" as noted in #286. I have defined "outstanding packets" to be the packets that aren't acknowledged or lost and made the formula count those packets. I think that's probably not the right thing, and the definition in #286 (comment) is probably closer to what is intended, but I was trying my best to avoid changing text and this seemed like the right compromise.
If I'm wrong and this isn't editorial, feel free to tell me what to write so that it can remain so.
Closes #285 by removing the offending text.