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.

Sign up for GitHub

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

Jump to bottom

Full (mostly) editorial pass #1665

Closed
wants to merge 5 commits into from
Closed

Full (mostly) editorial pass #1665

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56d6954
Editorial pass
mirjak Aug 10, 2018
835b1e5
Full editorial pass
mirjak Aug 15, 2018
3f61ebc
tailing spaces...
mirjak Aug 15, 2018
dee34a1
still tailing spaces...
mirjak Aug 15, 2018
70e97ec
Comments form @ianswett, @nibanks, and @MikeBishop
mirjak Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 11 additions & 11 deletions draft-ietf-quic-transport.md
View file
Open in desktop
Expand Up @@ -139,8 +139,8 @@ connection-level flow control, connection migration, and data reliability.

An accompanying document describes QUIC's loss detection and congestion control
{{QUIC-RECOVERY}}. As these are sender-side only mechanisms they can be changed
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the sentence "As these are sender-side..." is adding anything. Is there something you think is missing here?

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 the point of that sentence is that the endpoints don't both need to be following that RFC for it to be this version of QUIC. It's a legitimate comment, though I might frame it differently.

"Each implementation of QUIC will unilaterally select a loss detection and congestion control strategy. A recommended approach is described in {{QUIC-RECOVERY}}."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the point was also to provide a reason why this is in a different doc. I used Mike's wording.

without negotiating a new QUIC version. This document specifies version
0x00000001 of QUIC which uses TLS1.3 for key negotiation as described in
without negotiating a new QUIC version. This document specifies version
0x00000001 of QUIC which uses TLS1.3 for key negotiation as described in
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropped a space ("TLS 1.3").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

{{QUIC-TLS}}. Key negotiation and encryption is described in a separate
docuement to make changes to only this part easier in future versions.

Expand Down Expand Up @@ -192,11 +192,11 @@ QUIC packet:

: QUIC packets containing application payload data that are encrypted with
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically 0-RTT packets contain application data and some CRYPTO data, so I'd be inclined to just say "QUIC packets encrypted with a key derived from a previous QUIC connection to the same endpoint."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

a key derived from previous QUIC connection to the same endpoint.

1-RTT:

: QUIC packets that are encrypted with keys derived from the QUIC
handshake.
handshake.


## Notational Conventions
Expand Down Expand Up @@ -868,7 +868,7 @@ Source Connection ID field of the previously received Initial packet from that
endpoint; the Source Connection ID includes the connection ID that the
sender of the packet wishes to use (see {{connection-id-encoding}}).
Copy link
Contributor

Choose a reason for hiding this comment

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

The added text makes this less readable. I'd revert this paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, problem here is that the mechanism is not well described at this point of the doc.


The first Handshake packet sent by a server contains a packet number of 0.
The first Handshake packet sent by a server contains a packet number of 0.
Handshake packets are their own packet number space. See section
{{packet-numbers}} on packet number spaces. Packet numbers are incremented
Copy link
Contributor

Choose a reason for hiding this comment

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

"See X on Y" feels awkward to me. Also, this will produce "section Section a.b.c" when compiled. I'd make this a parenthetical and either shorten this all the way:

...are their own packet number space. (See {{packet-numbers}}.)  Packet numbers are....

...or expand it:

(See {{packet numbers}} for more information on packet number spaces.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

normally for other Handshake packets.
Expand Down Expand Up @@ -2018,7 +2018,7 @@ The client SHOULD allow for additional Retry packets being sent in
response to Initial packets sent containing a token. There are several
situations in which the server might not be able to use the previously
generated token to validate the client's address and must send a new
Retry.
Retry.

A reasonable limit to the number of tries the client allows for, before giving
up, is 3. That is, the client MUST echo the address validation token from a
Expand Down Expand Up @@ -2812,7 +2812,7 @@ encoding.
The PADDING frame (type=0x00) has no semantic value. PADDING frames can be used
to increase the size of a packet. Padding can be used to increase an initial
client packet to the minimum required size, or to provide protection against
traffic analysis for protected packets. PADDING frames need to be acknowleged.
traffic analysis for protected packets. PADDING frames need to be acknowleged.
Copy link
Member

Choose a reason for hiding this comment

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

Frames are not acknowledged. Packets are. I don't think this text is needed either way. All packets are acknowledged.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to what @nibanks said, the presence of PADDING frames does not cause a packet to elicit an acknowledgement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right this wording is wrong in any case. However, I now realized that the doc is actually inconsitent about PADDING frames. In section 8.4.2 it say "PADDING frames generate acknowledgements". That might be a separate PR. Removed that sentence for now.


A PADDING frame has no content. That is, a PADDING frame consists of the single
octet that identifies the frame as a PADDING frame.
Expand Down Expand Up @@ -4089,7 +4089,7 @@ data to a peer.
+-------+
| Ready | Send RST_STREAM
| |-----------------------.
+-------+ |
+-------+ |
| |
| Send STREAM / |
| STREAM_BLOCKED |
Expand Down Expand Up @@ -4177,7 +4177,7 @@ application protocol some of which cannot be observed by the sender.
| Create Bidirectional Stream (Sending)
| Recv MAX_STREAM_DATA
| Create Higher-Numbered Stream
v
v
+-------+ Send STOP_SENDING
.-| |-------------------------------.
Send | | Recv | |
Expand All @@ -4190,8 +4190,8 @@ Send | | Recv | |
| Size | | | Wait |
| Known + Recv RST_STREAM | | STOP |
+-------+---------------------->| +-------+
Copy link
Member

Choose a reason for hiding this comment

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

What is this Wait STOP state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the state where incoming data is being discarded, but the solicited RST_STREAM hasn't been received yet. We originally said that STOP_SENDING didn't change the state, just hinted to the other side that it should do so.

I don't object to making this a defined state, but if so, then the description needs to be added below as well. Let's make this a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i initially though about transition to "Data Recvd" on reception of STOP_SENDING but that would not capture that you are actually waiting for a RST_STREAM.

The text is mostly there it only needs to name the new state if people agree.

Copy link
Member

Choose a reason for hiding this comment

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

That sort of change has to be a separate PR.

| | |
| Recv All Data | | Recv
| | |
| Recv All Data | | Recv
v v | RST_STREAM
+-------+ +------- |
| Data | Recv RST_STREAM | Reset |<--'
Expand Down