Subject: [quicwg/base-drafts] Full (mostly) editorial pass (#1665)

From: mirjak <[email-masked]>
Date: Wed, 15 Aug 2018 16:18:09 +0000 (UTC)

This PR includes a bunch of small changes after a full review of the doc. Some are nits, many are mostly editorial or clarifications, but a few changes might be more than that. Please provide feedback!
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1665

-- Commit Summary --

  * Editorial pass
  * Full editorial pass

-- File Changes --

    M draft-ietf-quic-transport.md (523)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1665.patch
https://github.com/quicwg/base-drafts/pull/1665.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665

---

From: mirjak <[email-masked]>
Date: Wed, 15 Aug 2018 09:23:51 -0700

@mirjak pushed 1 commit.

3f61ebc  tailing spaces...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665/files/835b1e53d834d5e2960cbc1215a8a73f49106ade..3f61ebcf4bbe3b40b5998e6cbe6e4041ecfb7174

---

From: mirjak <[email-masked]>
Date: Wed, 15 Aug 2018 09:26:23 -0700

@mirjak pushed 1 commit.

dee34a1  still tailing spaces...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665/files/3f61ebcf4bbe3b40b5998e6cbe6e4041ecfb7174..dee34a1c9119e79177f013a059a6275a4edd6a1b

---

From: ianswett <[email-masked]>
Date: Wed, 15 Aug 2018 09:56:37 -0700

ianswett commented on this pull request.

This PR may be too large to land, but I'll leave that call to Martin.  There are some good editorial fixes in here, so thanks for taking a pass at the draft.

>  protocol, including the conceptual design, wire format, and mechanisms of the
 QUIC protocol for connection establishment, stream multiplexing, stream and
 connection-level flow control, connection migration, and data reliability.
 
-Accompanying documents describe QUIC's loss detection and congestion control
-{{QUIC-RECOVERY}}, and the use of TLS 1.3 for key negotiation {{QUIC-TLS}}.
+An accompanying document describes QUIC's loss detection and congestion control
+{{QUIC-RECOVERY}}. As these are sender-side only mechanisms they can be changed

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

> @@ -182,7 +188,15 @@ QUIC packet:
 
 : A well-formed UDP payload that can be parsed by a QUIC receiver.
 
-QUIC is a name, not an acronym.
+0-RTT:
+
+: QUIC packets containing application payload data that are encrypted with

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."

> @@ -293,8 +307,8 @@ Long headers are used for packets that are sent prior to the completion of
 version negotiation and establishment of 1-RTT keys. Once both conditions are
 met, a sender switches to sending packets using the short header
 ({{short-header}}).  The long form allows for special packets - such as the
-Version Negotiation packet - to be represented in this uniform fixed-length
-packet format. Packets that use the long header contain the following fields:
+Version Negotiation packet - to be represented in this uniform packet format.

It's actually weird to use version negotiation as an example here, because there's no packet number for version negotiation.  And I think there's no length either?

> @@ -380,7 +394,8 @@ covers the both the Packet Number and Payload fields, both of which are
 confidentiality protected and initially of unknown length.  The size of the
 Payload field is learned once the packet number protection is removed.
 
-Senders can sometimes coalesce multiple packets into one UDP datagram.  See
+The length field is used to enable senders to coalesce multiple long-header

nit: "is used to enable" -> enables

> @@ -466,16 +481,16 @@ Protected Payload:
 
 : Packets with a short header always include a 1-RTT protected payload.
 
-The header form and connection ID field of a short header packet are
-version-independent.  The remaining fields are specific to the selected QUIC
-version.  See {{QUIC-INVARIANTS}} for details on how packets from different
-versions of QUIC are interpreted.
+The header form and connection ID field, if present, of a short header packet

A 0-byte CID is always present and the extra clause makes this read more awkwardly, so I'd prefer keeping this as is.

> @@ -889,23 +908,24 @@ Packets protected with 0-RTT keys use a type value of 0x7C.  The connection ID
 fields for a 0-RTT packet MUST match the values used in the Initial packet
 ({{packet-initial}}).
 
-The client can send 0-RTT packets after receiving an Initial
-{{packet-initial}} or Handshake ({{packet-handshake}}) packet, if that
+The client can send further 0-RTT packets after receiving an Initial

I prefer it without further.  If you want to add a word, I'd suggest "more"

>  packet does not complete the handshake.  Even if the client receives a
 different connection ID in the Handshake packet, it MUST continue to
 use the same Destination Connection ID for 0-RTT packets, see
 {{connection-id-encoding}}.
 
-The version field for protected packets is the current QUIC version.
+The version field for protected packets contains the current QUIC version.

I'm not sure this is correct?

 The packet number field contains a packet number, which has additional
 confidentiality protection that is applied after packet protection is applied
 (see {{QUIC-TLS}} for details).  The underlying packet number increases with
 each packet sent, see {{packet-numbers}} for details.
 
-The payload is protected using authenticated encryption.  {{QUIC-TLS}} describes
-packet protection in detail.  After decryption, the plaintext consists of a
-sequence of frames, as described in {{frames}}.
+The content of 0-RTT and 1-RTT packets is protected using authenticated

I believe everything but INITIAL and RETRY are authenticated, so I think this change is not an improvement.

> @@ -914,14 +934,13 @@ A sender can coalesce multiple QUIC packets (typically a Cryptographic Handshake
 packet and a Protected packet) into one UDP datagram.  This can reduce the
 number of UDP datagrams needed to send application data during the handshake and
 immediately afterwards. It is not necessary for senders to coalesce
-packets, though failing to do so will require sending a significantly
-larger number of datagrams during the handshake. Receivers MUST
-be able to process coalesced packets.
+packets, though not doing it leads larger number of datagrams during the

"though not doing so leads to a larger number of datagrams.."

> @@ -1184,7 +1203,9 @@ endpoint cannot send a frame of a type that is unknown to its peer.
 An extension to QUIC that wishes to use a new type of frame MUST first ensure
 that a peer is able to understand the frame.  An endpoint can use a transport
 parameter to signal its willingness to receive one or more extension frame types
-with the one transport parameter.
+with the one transport parameter. Otherwise it is assumed that enpoints have

I'm not sure we want to imply or endorse the use of out of band negotiation, so I'd stick with the existing text.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#pullrequestreview-146520920

---

From: Nick Banks <[email-masked]>
Date: Wed, 15 Aug 2018 17:02:06 +0000 (UTC)

nibanks requested changes on this pull request.

Full review of the new text. Most changes look good to me. A couple changes I think are necessary though.

 
+areversion-independent.  The remaining fields are specific to the selected

`areversion` -> `are version`

+encryption. {{QUIC-TLS}}vdescribes packet protection in detail.  After

`vdescribes` -> ` describes`

-The payload is protected using authenticated encryption.  {{QUIC-TLS}} describes
-packet protection in detail.  After decryption, the plaintext consists of a
-sequence of frames, as described in {{frames}}.
+The content of 0-RTT and 1-RTT packets is protected using authenticated
+encryption. {{QUIC-TLS}}vdescribes packet protection in detail.  After
+decryption, the plaintext consistsvof a sequence of frames, as described in

`consistsvof` -> `consists of`

`leads larger number` -> `leads to larger number`

 Senders SHOULD coalesce packets in order of increasing encryption levels
 (Initial, Handshake, 0-RTT, 1-RTT), as this makes it more likely the receiver
 will be able to process all the packets in a single pass.  A packet with a short
-header does not include a length, so it will always be the last packet included
+header does not include a length, so it can only be the last packet included

Personally, I think `will always` is better in this case.

> @@ -996,7 +1014,7 @@ response to connection migration ({{migration}}). NEW_CONNECTION_ID frames
 
 The packet number is an integer in the range 0 to 2^62-1. The value is used in
 determining the cryptographic nonce for packet encryption.  Each endpoint
-maintains a separate packet number for sending and receiving.
+maintains separate packet number spaces for sending and receiving.

This should be reverted or at least modified more. Before, it was trying to say that packet numbers are different in each direction and should be tracked separately. Now, it's just talking about packet number spaces. It should probably say there are separate packet numbers in each direction, and they are tracked for each different packet number space.

> @@ -1114,7 +1132,8 @@ Stateless Reset do not contain frames.
 Protected payloads MUST contain at least one frame, and MAY contain multiple
 frames and multiple frame types.
 
-Frames MUST fit within a single QUIC packet and MUST NOT span a QUIC packet
+Frames MUST fit within a single QUIC packet, and therefore within a single
+UDP datagram, and MUST NOT span a QUIC packet
 boundary. Each frame begins with a Frame Type, indicating its type, followed by

Fix up the following line wrapping, so this new line doesn't terminate so early.

 An extension to QUIC that wishes to use a new type of frame MUST first ensure
 that a peer is able to understand the frame.  An endpoint can use a transport
 parameter to signal its willingness to receive one or more extension frame types
-with the one transport parameter.
+with the one transport parameter. Otherwise it is assumed that enpoints have
+knowledge about which (extension) frame types are supported by the otherwise, e.g.
+through out of band negotiation.

I'm confused about this additional text. It seems to suggest an entirely new notion of exchanging extension support knowledge out of band. I think this type of text should be in a different PR.

>  deployment-specific) method which will allow packets with that connection ID to
 be routed back to the endpoint and identified by the endpoint upon receipt.
 
 A zero-length connection ID MAY be used when the connection ID is not needed for
-routing and the address/port tuple of packets is sufficient to associate them to
-a connection. An endpoint whose peer has selected a zero-length connection ID
-MUST continue to use a zero-length connection ID for the lifetime of the
-connection and MUST NOT send packets from any other local address.
+routing and the address/port tuple of packets is assumed to be sufficiently stable
+to associate them to a connection. An endpoint whose peer has selected a
+zero-length connection ID MUST continue to use a zero-length connection ID for the
+lifetime of the connection and cannot send packets from any other local address
+without breaking the connection.

I don't think this text needs to be changed. I prefer how it was before.

>  Servers MUST drop incoming packets under all other circumstances. They
 SHOULD send a Stateless Reset ({{stateless-reset}}) if a connection ID
 is present in the header.
 
 ## Version Negotiation
 
 Version negotiation ensures that client and server agree to a QUIC version
-that is mutually supported. A server sends a Version Negotiation packet in
-response to each packet that might initiate a new connection, see
+that is mutually supported independent of the implemented version. A server

Should there be a comma after `mutually supported`?

> @@ -1578,20 +1599,26 @@ language from Section 3 of {{!TLS13=I-D.ietf-tls-tls13}}.
 
 The `extension_data` field of the quic_transport_parameters extension defined in
 {{QUIC-TLS}} contains a TransportParameters value.  TLS encoding rules are
-therefore used to encode the transport parameters.
+therefore used to encode the transport parameters. As show in

`As show` -> `As shown`

 QUIC encodes transport parameters into a sequence of octets, which are then
 included in the cryptographic handshake.  Once the handshake completes, the
 transport parameters declared by the peer are available.  Each endpoint
 validates the value provided by its peer.  In particular, version negotiation
 MUST be validated (see {{version-validation}}) before the connection
-establishment is considered properly complete.
+establishment is considered properly complete. Each transport parameter consists
+of an 2-byte parameter ID and an optional parameter value.

And a length, right?

> @@ -1661,7 +1688,7 @@ disable_migration (0x0009):
 : The endpoint does not support connection migration ({{migration}}). Peers MUST
   NOT send any packets, including probing packets ({{probing}}), from a local
   address other than that used to perform the handshake.  This parameter is a
-  zero-length value.
+  zero-length value. Values other than 0 and 1 are invalid.

There is no 'value' in the case. It's zero-length.

 An endpoint enters a closing period after initiating an immediate close
-({{immediate-close}}).  While closing, an endpoint MUST NOT send packets unless
-they contain a CONNECTION_CLOSE or APPLICATION_CLOSE frame (see
-{{immediate-close}} for details).
+({{immediate-close}}).  In the closing period, an endpoint MUST NOT send

I think `During the closing period` sounds better than `In the closing period`.

 An endpoint enters a closing period after initiating an immediate close
-({{immediate-close}}).  While closing, an endpoint MUST NOT send packets unless
-they contain a CONNECTION_CLOSE or APPLICATION_CLOSE frame (see
-{{immediate-close}} for details).
+({{immediate-close}}).  In the closing period, an endpoint MUST NOT send
+packets unless they only contain a CONNECTION_CLOSE or APPLICATION_CLOSE frame

I disagree with the addition of the word `only` here.

> @@ -2773,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.
+traffic analysis for protected packets. PADDING frames need to be acknowleged.

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

> @@ -3415,7 +3454,10 @@ acknowledged again.
 Because ACK frames are not sent in response to ACK-only packets, a receiver that
 is only sending ACK frames will only receive acknowledgements for its packets
 if the sender includes them in packets with non-ACK frames.  A sender SHOULD
-bundle ACK frames with other frames when possible.
+bundle ACK frames with other frames when possible. A receiver that would only
+send ACK frames therefore can trigger an ACK from the sender by sending a
+frames that needs to be acknowledged. Knowing which ACKs have been received

Again, frames aren't acknowledged.

> @@ -3808,7 +3852,8 @@ containing that information is acknowledged.
 * A liveness or path validation check using PATH_CHALLENGE frames is sent
   periodically until a matching PATH_RESPONSE frame is received or until there
   is no remaining need for liveness or path validation checking. PATH_CHALLENGE
-  frames include a different payload each time they are sent.
+  frames include a different payload each time they are sent and are therefore
+  not-retransmittable.

You cannot retransmit the exact same PATH_CHALLENGE frames? That is change from my previous understanding.

> +                       | Create Bidirectional Stream (Sending)
+                       | Recv MAX_STREAM_DATA
+                       | Create Higher-Numbered Stream
+                       v
+                   +-------+ Send STOP_SENDING
+                 .-|       |-------------------------------.
+Send             | | Recv  |                               |
+ MAX_STREAM_SIZE '>|       | Recv RST_STREAM               |
+                   +-------+-----------------------.       |
+                       |                           |       |
+                       | Recv STREAM + FIN         |       |
+                       v                           |       v
+                   +-------+                       |   +-------+
+                   | Size  |                       |   | Wait  |
+                   | Known + Recv RST_STREAM       |   | STOP  |
+                   +-------+---------------------->|   +-------+

What is this `Wait STOP` state?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#pullrequestreview-146517164

---

From: Nick Banks <[email-masked]>
Date: Wed, 15 Aug 2018 10:04:46 -0700

nibanks commented on this pull request.

I think it is. `The version field … contains the`

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210337197

---

From: Mike Bishop <[email-masked]>
Date: Wed, 15 Aug 2018 12:29:03 -0700

MikeBishop requested changes on this pull request.

Quite a mixed bag.  You hit on a few places where there are useful improvements to make.  However, several of the changes seem to add text unnecessarily, and a few really ought to be separate PRs.

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}}."

-Accompanying documents describe QUIC's loss detection and congestion control
-{{QUIC-RECOVERY}}, and the use of TLS 1.3 for key negotiation {{QUIC-TLS}}.
+An accompanying document describes QUIC's loss detection and congestion control
+{{QUIC-RECOVERY}}. As these are sender-side only mechanisms they can be changed
+without negotiating a new QUIC version. This document specifies version
+0x00000001 of QUIC which uses TLS1.3 for key negotiation as described in

Dropped a space ("TLS 1.3").

+QUIC is a name, not an acronym.

The consensus before was to put this in the definitions section.  It feels out-of-place here.

-QUIC version 1 conforms to the protocol invariants in {{QUIC-INVARIANTS}}.
+All versions of QUIC MUST conform to the protocol invariants in
+{{QUIC-INVARIANTS}}.

No, this isn't a normative requirement.  You could say that "All versions of QUIC conform....", purely for the reader's information.

No, there's not.  I don't think this bears on @mirjak's change itself, but we should update this paragraph.

I think "multiple packets" was fine.  The section being referenced has the details about what combinations will actually work.

> @@ -736,11 +751,11 @@ the Source Connection ID includes the connection ID that the sender of the
 packet wishes to use (see {{connection-id}}). The server MUST use consistent
 Source Connection IDs during the handshake.
 
-On first receiving an Initial or Retry packet from the server, the client uses
-the Source Connection ID supplied by the server as the Destination Connection ID
-for subsequent packets.  Once a client has received an Initial packet from the
-server, it MUST discard any packet it receives with a different Source
-Connection ID.
+On first receiving an Initial or Retry packet from the server, the client MUST

While not inaccurate, is there a reason to make this change?  This is a statement of how the protocol works, which still describes the correct behavior.

> @@ -842,18 +857,21 @@ used to carry acknowledgments and cryptographic handshake messages from the
 server and client.
 
 A server sends its cryptographic handshake in one or more Handshake packets in
-response to an Initial packet if it does not send a Retry packet.  Once a client
-has received a Handshake packet from a server, it uses Handshake packets to send
-subsequent cryptographic handshake messages and acknowledgments to the server.
+response to an Initial packet (in addition to the server Initial packet) if it

This parenthetical doesn't seem to add anything.  The server *does* send an Initial packet, true, but it's not related to how the cryptographic handshake is carried.

 The Destination Connection ID field in a Handshake packet contains a connection
-ID that is chosen by the recipient of the packet; the Source Connection ID
-includes the connection ID that the sender of the packet wishes to use (see
-{{connection-id-encoding}}).
+ID that is chosen by the other endpoint of this connection as indicated in
+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}}).

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

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

"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.)

> @@ -873,7 +891,8 @@ static handshake keys or the 0-RTT keys are sent with long headers; all packets
 protected with 1-RTT keys are sent with short headers.  The different packet
 types explicitly indicate the encryption level and therefore the keys that are
 used to remove packet protection.  0-RTT and 1-RTT protected packets share a
-single packet number space.
+single packet number space. Initial and Handshake packets both have their own

both => each; perhaps combine these:  "...single packet number space, while Initial and Handshake packets each...."

or "additional", or "can continue sending"

No, because "protected" packets includes short-header packets, which don't have a version field.  This inverts the original meaning, which is that whenever a version field is present, it always indicates this version.

Why the switch from "payload" as used elsewhere to "content"?  The payload of each protected packet is protected with authenticated encryption.  That's true of Initial and Handshake as well, though the Initial key is not secret enough to put much faith in the authentication.

If you're touching this section, I feel like these MUSTs are excessive.  It's not that it's prohibited, it's that there's no mechanism for doing so.  Packets contain sequences of whole frames and cannot span datagrams.  Therefore, frames also cannot span packets.

Agreed -- revert.

> @@ -1207,18 +1228,21 @@ distinguish it from other connections.  A connection ID can be either 0 octets
 in length, or between 4 and 18 octets (inclusive).  Connection IDs are selected
 independently in each direction.
 
-The primary function of a connection ID is to ensure that changes in addressing
-at lower protocol layers (UDP, IP, and below) don't cause packets for a QUIC
-connection to be delivered to the wrong endpoint.  Each endpoint selects
-connection IDs using an implementation-specific (and perhaps
+The primary function of a connection ID is to ensure packets can still be
+assotiated with the right connection at the receiver if changes in sender

associated with the correct connection

> @@ -1207,18 +1228,21 @@ distinguish it from other connections.  A connection ID can be either 0 octets
 in length, or between 4 and 18 octets (inclusive).  Connection IDs are selected
 independently in each direction.
 
-The primary function of a connection ID is to ensure that changes in addressing
-at lower protocol layers (UDP, IP, and below) don't cause packets for a QUIC
-connection to be delivered to the wrong endpoint.  Each endpoint selects
-connection IDs using an implementation-specific (and perhaps
+The primary function of a connection ID is to ensure packets can still be
+assotiated with the right connection at the receiver if changes in sender
+addressing at lower protocol layers (UDP, IP, and below) occur. The Connection ID
+is visibe to the path to also enable the network to deliver packets for a QUIC

"visible... to ~also~ enable"; perhaps fold the path and load-balancer comments together and just say "to enable load balancers to deliver packets for a QUIC connection to the correct endpoint despite address changes."

> @@ -1303,34 +1327,34 @@ the packet.  Servers SHOULD NOT attempt to decode or decrypt a packet from an
 unknown version, but instead send a Version Negotiation packet, provided that
 the packet is sufficiently long.
 
-Servers MUST drop other packets that contain unsupported versions.
+Servers MUST drop packets without sending a Version Negotiation packet when the
+received packet contains unsupported versions and is too small
+to be an Initial packet for some version supported by the server.

"Initial packet" is a version-specific concept.  "...and is too small to create a new connection in any version supported by the server."  However, I see this in the old text too, so I'll file a separate issue.

A packet can't contain multiple versions, so "when the received packet indicates an unsupported version".  (I probably would have reworded the old text as well, but multiple versions in multiple packets is somewhat okay.)

-Packets with a supported version, or no version field, are matched to
-a connection as described in {{packet-handling}}. If not matched, the
-server continues below.

What was the rationale for moving this text below processing of unmatched packets from a new connection?  You only want to do the Initial/0-RTT checks if a packet is unmatched.

-If a server isn't currently accepting any new connections, it SHOULD send a
-Handshake packet containing a CONNECTION_CLOSE frame with error code
-SERVER_BUSY.
-
-If the packet is a 0-RTT packet, the server MAY buffer a limited
-number of these packets in anticipation of a late-arriving Initial
-Packet. Clients are forbidden from sending Handshake packets prior to
+If the packet is a 0-RTT packet with a supported version number, the server MAY

If you've reached this point in the flow, it has a supported version.  Unsupported versions, regardless of packet type, have already been rejected or dropped.

 ## Version Negotiation
 
 Version negotiation ensures that client and server agree to a QUIC version
-that is mutually supported. A server sends a Version Negotiation packet in
-response to each packet that might initiate a new connection, see
+that is mutually supported independent of the implemented version. A server
+can send a Version Negotiation packet in response to each packet that might
+initiate a new connection and is sufficiently large, see also

- Looks like Martin slipped a comma splice by me.  😉 This should be a semicolon instead.
- I don't think the addition of "also" adds anything here.
- The change from "sends" to "can send" is probably okay, but it implies a slightly different behavior on the part of servers than we'd previously discussed.

> @@ -1363,12 +1387,8 @@ selects an acceptable protocol version from the list provided by the server.
 The client then attempts to create a connection using that version.  Though the
 contents of the Initial packet the client sends might not change in
 response to version negotiation, a client MUST increase the packet number it
-uses on every packet it sends.  Packets MUST continue to use long headers and
-MUST include the new negotiated protocol version.
-
-The client MUST use the long header format and include its selected version on
-all packets until it has 1-RTT keys and it has received a packet from the server
-which is not a Version Negotiation packet.
+uses on every packet it sends. The client MUST use the long header format and
+include its selected version on all packets until it has 1-RTT keys.

For context, before we'd agreed that version implied handshake protocol, we said that the requirement for switching to short-header was the completion of both version negotiation *and* 1-RTT key availability, and this text reflects that.  (Consider a handshake protocol where there's a shared dictionary of static keys that gets rotated periodically, and the only message prior to 1-RTT is the client's unilateral declaration "I'm using key A1B2C3 from your key table."  The client can then send "1-RTT" short-header packets on its first flight.)

However, in the current version (which this is describing), version negotiation always completes prior to 1-RTT key establishment, so this is probably okay.

>  a 1232 octet QUIC packet payload.  This includes overheads that reduce the space
 available to the cryptographic handshake protocol.
 
-The CRYPTO frame can be sent in different packet number spaces.  CRYPTO frames
+The CRYPTO frames can be sent in different packet number spaces.  CRYPTO frames

I'd do "CRYPTO frames" or the original text.

+{{figure-transport-parameters}} TransportParameters consists of either an

Comma after the figure reference.

The value isn't optional -- it's length-prefixed, and the length might be zero.

How would the value be 0 *or* 1 if its length is zero?

> @@ -1719,11 +1746,11 @@ type TRANSPORT_PARAMETER_ERROR.
 ### Values of Transport Parameters for 0-RTT {#zerortt-parameters}
 
 A client that attempts to send 0-RTT data MUST remember the transport parameters
-used by the server.  The transport parameters that the server advertises during
-connection establishment apply to all connections that are resumed using the
-keying material established during that handshake.  Remembered transport
-parameters apply to the new connection until the handshake completes and new
-transport parameters from the server can be provided.
+provided by the server in the initial handshake.  The transport parameters that

"initial handshake" might be unclear, given that previous session might also have been a resumption.  Perhaps "previous" instead?

> @@ -1757,27 +1784,28 @@ for transport parameters cannot be supported.
 
 ### New Transport Parameters
 
-New transport parameters can be used to negotiate new protocol behavior.  An
-endpoint MUST ignore transport parameters that it does not support.  Absence of
-a transport parameter therefore disables any optional protocol feature that is
-negotiated using the parameter.
+New transport parameters can be used by the client to negotiate new protocol

I think you're identifying a distinction here between "negotiate" and "advertise" which is potentially useful, but I'm not sure the text you've written makes good use of that distinction.

Some extensions need only be advertised:  "I support this feature; if you do too, feel free to use it."  Actual changes to existing protocol behavior require confirmation, which can be accomplished by advertisements from both sides.  Choices between multiple options require true negotiation, where the client offers a set of choices and the server picks one (or none).

Only clients can begin a negotiation for breaking protocol changes, but either side can unilaterally advertise support for non-breaking optional features.

> @@ -1757,27 +1784,28 @@ for transport parameters cannot be supported.
 
 ### New Transport Parameters
 
-New transport parameters can be used to negotiate new protocol behavior.  An
-endpoint MUST ignore transport parameters that it does not support.  Absence of
-a transport parameter therefore disables any optional protocol feature that is
-negotiated using the parameter.
+New transport parameters can be used by the client to negotiate new protocol
+behavior.  An endpoint MUST ignore transport parameters that it does not support.
+Absence of a transport parameter as provided by the server therefore disables
+any optional protocol feature that is negotiated using the parameter. A server
+cannot negotiation new features but can indicate the support of a new feature

"negotiate"

> @@ -1757,27 +1784,28 @@ for transport parameters cannot be supported.
 
 ### New Transport Parameters
 
-New transport parameters can be used to negotiate new protocol behavior.  An
-endpoint MUST ignore transport parameters that it does not support.  Absence of
-a transport parameter therefore disables any optional protocol feature that is
-negotiated using the parameter.
+New transport parameters can be used by the client to negotiate new protocol
+behavior.  An endpoint MUST ignore transport parameters that it does not support.
+Absence of a transport parameter as provided by the server therefore disables
+any optional protocol feature that is negotiated using the parameter. A server
+cannot negotiation new features but can indicate the support of a new feature
+that is ready to be used by the client. Howeverm it cannot assume that the client

However**,**

> @@ -1757,27 +1784,28 @@ for transport parameters cannot be supported.
 
 ### New Transport Parameters
 
-New transport parameters can be used to negotiate new protocol behavior.  An
-endpoint MUST ignore transport parameters that it does not support.  Absence of
-a transport parameter therefore disables any optional protocol feature that is
-negotiated using the parameter.
+New transport parameters can be used by the client to negotiate new protocol
+behavior.  An endpoint MUST ignore transport parameters that it does not support.
+Absence of a transport parameter as provided by the server therefore disables
+any optional protocol feature that is negotiated using the parameter. A server
+cannot negotiation new features but can indicate the support of a new feature
+that is ready to be used by the client. Howeverm it cannot assume that the client
+understand the parameter and is ready to use that feature.

"understand**s** ... and will use that feature."

 New transport parameters can be registered according to the rules in
 {{iana-transport-parameters}}.

 ### Version Negotiation Validation {#version-validation}
 
-Though the cryptographic handshake has integrity protection, two forms of QUIC
-version downgrade are possible.  In the first, an attacker replaces the QUIC
-version in the Initial packet.  In the second, a fake Version Negotiation packet
-is sent by an attacker.  To protect against these attacks, the transport
-parameters include three fields that encode version information.  These
-parameters are used to retroactively authenticate the choice of version (see
-{{version-negotiation}}).
-
-The cryptographic handshake provides integrity protection for the negotiated
-version as part of the transport parameters (see {{transport-parameters}}).  As
+Theoretically, two forms of QUIC version downgrade are possible.  In the first,

Why "theoretically"?  Are they not actually possible?

 New transport parameters can be registered according to the rules in
 {{iana-transport-parameters}}.

 ### Version Negotiation Validation {#version-validation}
 
-Though the cryptographic handshake has integrity protection, two forms of QUIC
-version downgrade are possible.  In the first, an attacker replaces the QUIC
-version in the Initial packet.  In the second, a fake Version Negotiation packet
-is sent by an attacker.  To protect against these attacks, the transport
-parameters include three fields that encode version information.  These
-parameters are used to retroactively authenticate the choice of version (see
-{{version-negotiation}}).
-
-The cryptographic handshake provides integrity protection for the negotiated
-version as part of the transport parameters (see {{transport-parameters}}).  As
+Theoretically, two forms of QUIC version downgrade are possible.  In the first,
+an attacker replaces the QUIC version in the Initial packet.  In the second,
+a fake Version Negotiation packet is sent by an attacker.  To protect against
+these attacks, the cryptographic handshake provides integrity protection for
+the negotiated version as part of the transport parameters. As described in

This is somewhat different from what I would have understood from integrity protection.  Perhaps "reconfirms"?

> -Though the cryptographic handshake has integrity protection, two forms of QUIC
-version downgrade are possible.  In the first, an attacker replaces the QUIC
-version in the Initial packet.  In the second, a fake Version Negotiation packet
-is sent by an attacker.  To protect against these attacks, the transport
-parameters include three fields that encode version information.  These
-parameters are used to retroactively authenticate the choice of version (see
-{{version-negotiation}}).
-
-The cryptographic handshake provides integrity protection for the negotiated
-version as part of the transport parameters (see {{transport-parameters}}).  As
+Theoretically, two forms of QUIC version downgrade are possible.  In the first,
+an attacker replaces the QUIC version in the Initial packet.  In the second,
+a fake Version Negotiation packet is sent by an attacker.  To protect against
+these attacks, the cryptographic handshake provides integrity protection for
+the negotiated version as part of the transport parameters. As described in
+{{transport-parameters}} the transport parameters include three fields that

}}**,**

 New transport parameters can be registered according to the rules in
 {{iana-transport-parameters}}.

 ### Version Negotiation Validation {#version-validation}
 
-Though the cryptographic handshake has integrity protection, two forms of QUIC
-version downgrade are possible.  In the first, an attacker replaces the QUIC
-version in the Initial packet.  In the second, a fake Version Negotiation packet
-is sent by an attacker.  To protect against these attacks, the transport
-parameters include three fields that encode version information.  These
-parameters are used to retroactively authenticate the choice of version (see
-{{version-negotiation}}).
-
-The cryptographic handshake provides integrity protection for the negotiated

By removing this statement, it's no longer obvious why using transport parameters is more reliable than the original transmission of these values.

> @@ -1805,10 +1833,11 @@ QUIC versions that the server supports.
 
 The negotiated_version field is the version that is in use.  This MUST be set by
 the server to the value that is on the Initial packet that it accepts (not an
-Initial packet that triggers a Retry or Version Negotiation packet).  A client
-that receives a negotiated_version that does not match the version of QUIC that
-is in use MUST terminate the connection with a VERSION_NEGOTIATION_ERROR error
-code.
+Initial packet that triggers a Retry or Version Negotiation packet).
+
+A client that receives a negotiated_version that does not match the version of

This is a continuation of the same thought; I'm not sure the added paragraph break is an improvement.

> @@ -1985,14 +2014,16 @@ validation - but a server SHOULD limit the data it sends toward an unvalidated
 address.  Successful completion of the cryptographic handshake implicitly
 provides proof that the client has received packets from the server.
 
-The client should allow for additional Retry packets being sent in
+The client SHOULD allow for additional Retry packets being sent in

I agree that we want to get rid of the lowercase "should" to avoid confusion, but given that this is a MUST in the next paragraph, I don't think capitalizing it is the way to go.  Perhaps "needs to"?

> @@ -2015,7 +2046,9 @@ Thus, a resumption token SHOULD include an expiration time.  The server MAY
 include either an explicit expiration time or an issued timestamp and
 dynamically calculate the expiration time.  It is also unlikely that the client
 port number is the same on two different connections; validating the port is
-therefore unlikely to be successful.
+therefore unlikely to be successful. It may also be possible that the client's
+IP address has changed, there it is also not recommended to validate the IP
+address.

This is a substantive change to our recommendations, and I disagree with it.  You issue tokens precisely because it allows you to skip the round-trip of validating the address if they haven't changed (or haven't changed much).

> @@ -2290,20 +2323,20 @@ validation of the address of the spurious migration to be abandoned.
 ### Loss Detection and Congestion Control {#migration-cc}
 
 The capacity available on the new path might not be the same as the old path.
-Packets sent on the old path SHOULD NOT contribute to congestion control or RTT
-estimation for the new path.
-
-On confirming a peer's ownership of its new address, an endpoint SHOULD
+On confirming a peer's ownership of its new address, an endpoint MUST

This changes a "SHOULD" followed by a "MUST NOT" into a "MUST ... unless".  That reduces the scope of the latitude we leave to implementers.

> -An endpoint MUST NOT return to the send rate used for the previous path unless
-it is reasonably sure that the previous send rate is valid for the new path.
-For instance, a change in the client's port number is likely indicative of a
-rebinding in a middlebox and not a complete change in path.  This determination
-likely depends on heuristics, which could be imperfect; if the new path capacity
-is significantly reduced, ultimately this relies on the congestion controller
-responding to congestion signals and reducing send rates appropriately.
+the new path unless it is reasonably sure that the actually network path did not
+change. For instance, a change in the client's port number is likely indicative
+of a rebinding in a middlebox and not a complete change in path.  This
+determination likely depends on heuristics, which could be imperfect; if the new
+path capacity is significantly reduced, ultimately this relies on the congestion
+controller responding to congestion signals and reducing send rates
+appropriately.
+
+If the congestion controller has been reset this means that packets sent on the

reset, ~this means that~ packets

> @@ -2560,11 +2594,16 @@ properly continue the connection.  An endpoint that wishes to communicate a
 fatal connection error MUST use a closing frame if it has sufficient state to do
 so.
 
-To support this process, a token is sent by endpoints.  The token is carried in
-the NEW_CONNECTION_ID frame sent by either peer, and servers can specify the
-stateless_reset_token transport parameter during the handshake (clients cannot
-because their transport parameters don't have confidentiality protection).  This
-value is protected by encryption, so only client and server know this value.
+To support this process, a token is sent by the endpoints that can be recovered
+or regenerated by the endpoint based on the information available in each packet
+even if the connection state was lost. This provides a protect mechanism to
+identify Stateless Reset packets from an attacker.

"protect mechanism" is missing something, but I'm not sure what.  It doesn't permit identifying SRs generated by an attacker, because they'll just look like random noise.  I'd drop this last sentence, though your expansion on our assumptions about the token might be useful.

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

And "a frames".

>  not maximally packed, to avoid sending out large numbers of small packets.  An
-implementation may use knowledge about application sending behavior or
+implementation MAY use knowledge about application sending behavior or

I don't know that this needs to be normative.  If you want to avoid the lower-case may, perhaps "can"?

> @@ -3713,11 +3755,11 @@ latency.
 ## Packet Processing and Acknowledgment {#processing-and-ack}
 
 A packet MUST NOT be acknowledged until packet protection has been successfully
-removed and all frames contained in the packet have been processed.  Any stream
-state transitions triggered by the frame MUST have occurred.  For STREAM frames,
-this means the data has been enqueued in preparation to be received by the
-application protocol, but it does not require that data is delivered and
-consumed.
+removed and all frames contained in the packet have been processed, including
+any stream state transitions triggered by the frame, as described in section

remove "section"

-* Application data sent in STREAM frames is retransmitted in new STREAM frames
-  unless the endpoint has sent a RST_STREAM for that stream.  Once an endpoint
-  sends a RST_STREAM frame, no further STREAM frames are needed.
+* Application data sent in STREAM frames is retransmitted if detected as lost,
+  e.g. according to the rules in {{QUIC-RECOVERY}}, in new STREAM frames unless

Having "in new STREAM frames" after the comma reads oddly.  Perhaps this needs to become its own sentence?  "New STREAM frames containing the lost data are generated, unless the endpoint has sent a RST_STREAM for that stream."

No, you can't.  You can try a fresh challenge, but it's not supposed to be the same payload.

I'm not sure this addition is useful.

> +                        +-------+                       |
+                            |                           |
+                            | Send STREAM /             |
+                            |      STREAM_BLOCKED       |
+                            v                           |
+                        +-------+                       |
+Send STREAM /         .-|       |                       |
+     STREAM_BLOCKED / | | Send  | Send RST_STREAM       |
+Recv MAX_STREAM_DATA  '>|       |---------------------->|
+                        +-------+                       |
+                            |                           |
+                            | Send STREAM + FIN /       |
+                            | Recv STOP_SENDING         |
+                            v                           v
+                      .-+-------+                   +-------+
+    Recv STOP_SEDNING | | Data  | Send RST_STREAM   | Reset |

SENDING

> +                        +-------+                       |
+                            |                           |
+                            | Send STREAM + FIN /       |
+                            | Recv STOP_SENDING         |
+                            v                           v
+                      .-+-------+                   +-------+
+    Recv STOP_SEDNING | | Data  | Send RST_STREAM   | Reset |
+                      '>| Sent  +------------------>| Sent  |
+                        +-------+                   +-------+
+                            |                           |
+                            | Recv All ACKs             | Recv ACK
+                            v                           v
+                        +-------+                   +-------+
+                        | Data  |                   | Reset |
+                        | Recvd |                   | Recvd |
+                        +-------+                   +-------+

I don't think that the self-transitions are needed.  Those events simply don't cause changes to the current stream state, just like many other events that aren't shown.

> @@ -4078,7 +4125,7 @@ in preparation for sending.
 
 The sending part of a bidirectional stream initiated by a peer (type 0 for a
 server, type 1 for a client) enters the "Ready" state if the receiving part
-enters the "Recv" state.
+enters the "Recv" state (see Figure {{fig-stream-recv-states}}).

~Figure~

> -       | App Read All Data         | App Read RST
-       v                           v
-   +-------+                   +-------+
-   | Data  |                   | Reset |
-   | Read  |                   | Read  |
-   +-------+                   +-------+
+                       o
+                       | Recv STREAM / STREAM_BLOCKED / RST_STREAM

Same comment about self-transitions.

+                       | Recv MAX_STREAM_DATA

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.

> @@ -4400,9 +4448,9 @@ STREAM frames ensures that loss recovery, congestion control, and flow control
 operate effectively.
 
 CRYPTO frames SHOULD be prioritized over other streams prior to the completion
-of the cryptographic handshake.  This includes the retransmission of the second
-flight of client handshake messages, that is, the TLS Finished and any client
-authentication messages.
+of the cryptographic handshake.  In {{QUIC-TLS}} this includes the retransmission
+of the second flight of client handshake messages, that is the TLS Finished and
+any client authentication messages.

Addition of QUIC-TLS reference seems unnecessary, and the removal of the comma is incorrect.

> @@ -4450,7 +4498,8 @@ with the Stream ID set appropriately. A receiver could use the current offset of
 data consumed to determine the flow control offset to be advertised.  A receiver
 MAY send MAX_STREAM_DATA frames in multiple packets in order to make sure that
 the sender receives an update before running out of flow control credit, even if
-one of the packets is lost.
+one of the packets is lost. As soons as the packet with the MAX_STREAM_DATA
+frame is acknowlegded, there is no need to send the same max stream limit agian.

Was this not obvious before?

> @@ -4486,7 +4535,7 @@ stream before the RST_STREAM frame, and the receiver MUST use the final offset
 to account for all bytes sent on the stream in its connection level flow
 controller.
 
-### Response to a RST_STREAM
+## Response to a RST_STREAM

These are subsections of "Other Considerations."  If you're going to make them stand-alone sections, I'd move them before that section.  However, I'm more inclined to leave them as-is pending @janaiyengar and @martinthomson's general rearranging of the draft.

> @@ -4495,7 +4544,7 @@ RST_STREAM an endpoint will choose to stop sending data in its own direction. If
 the sender of a RST_STREAM wishes to explicitly state that no future data will
 be processed, that endpoint MAY send a STOP_SENDING frame at the same time.
 
-### Data Limit Increments {#fc-credit}
+## Data Limit Increments {#fc-credit}

Same.

> @@ -4524,7 +4573,7 @@ peer-initiated streams close.  A receiver MAY also advance the maximum stream ID
 based on current activity, system conditions, and other environmental factors.

-### Blocking on Flow Control {#blocking}
+## Blocking on Flow Control {#blocking}

Same.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#pullrequestreview-146544328

---

From: mirjak <[email-masked]>
Date: Thu, 16 Aug 2018 09:55:34 +0000 (UTC)

mirjak commented on this pull request.

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210539292

---

From: mirjak <[email-masked]>
Date: Thu, 16 Aug 2018 04:41:57 -0700

@mirjak pushed 1 commit.

70e97ec  Comments form @ianswett, @nibanks, and @MikeBishop

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665/files/dee34a1c9119e79177f013a059a6275a4edd6a1b..70e97ec0ef373b71bf55ed56e052d5e9227892ba

---

From: mirjak <[email-masked]>
Date: Thu, 16 Aug 2018 04:42:49 -0700

fixed

done.

K, moved it back but I find it weird there.

 

Done

Yes, that needs a separate fix.

I guess it does hurt to be more precise, no? Fixed wording.

 

Okay, removed.

 

fixed

I thought making this normative makes it more clear...

 server and client.

At this point in the doc it is actually not very clear to the reader that the server does send the Initial. I thought that would at least help a bit, however, I guess some restructuring of the doc would help more...

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

Done

Done.

used additional

not sure that was the original meaning but if so, we should probably say this explicitly? "If present, the version field for protected packets contains the current QUIC version."...?

How is that authenticated at all?

Actually payload is not used uniformly. Up to this point in the doc the word content was used. I did not double-check for the rest of the text. Probably worth using one common term. 

fixed

fixed

fixed

done.

hm... 

Can you proposed a wording?

reworded and removed the normative language. I there there is, potentially normative, text elsewhere also saying that packets cannot span multiple datagrams. 

i guess we do not have to say it that explicitly but using the wording "an endpoint CAN use TPs" implies that there is another way to use extension frames and I say we should say something about it. 

fixed

reworded

Why? There are two changes. I added "assumed to be" which I think is more correct because you never know. And removed the normative MUST because that can also not be guarantee because something underneath could change the address.

reworded

 ## Version Negotiation

done

 ## Version Negotiation

I just if you would want to rate limit version negotiation, this is more correct.

?

done

changed

 

can you maybe propose different wording?

 

fixed

 

fixed

 

just because it could use it, it does not mean it will.

 

I was just merging it with the next paragraph. I just think the first sentence ("version downgrades are possible") was not quite correct because, while it is possible you will detect it and close the connection which kind of means it is not possible... Please propose a different wording!

 

you will detect them and close the connection. Does that mean it possible or not possible?

 

Not sure what you want me to change but I did not change the wording I just merged to paragraphs into one.

done

I split it because all other paragraphs talk about server and client behavior separately. 

Are these two different thing? Being prepared to receive multiple retries or sending multiple Initial packets based on one received retry packet. I had to read that multiple time but that is what I understand. Otherwise we need to rephrase that.

I think the previous text was just confusing and probably not correct.

done

done

What about: "This mechanism protects against forged Stateless Retry packets that could otherwise enable an off-path attacker to close a connection." Any better?

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.

reworded

Was not sure here. Changed to can for unambiguity.

done

done

+                            |                           |
+                            | Send STREAM /             |
+                            |      STREAM_BLOCKED       |
+                            v                           |
+                        +-------+                       |
+Send STREAM /         .-|       |                       |

done

+                            |                           |

I added it because that fit with the text describing which messages can be send when.

+                       | Recv MAX_STREAM_DATA

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. 

 operate effectively.

this sentence is very TLS specfic and bundles the two doc more then needed if left as a general statement. 

Readded the comma...

the point here is that I am actually unhappy with the "in multiple packets". How much is multiple? i would assume something like 3 but others might think they are only on the safe side if they send it e.g. 20 times... I just wanted to make sure nobody gets the idea that it is most safe to just always send it. I would be more happier if you could just give a number instead of saying "in multiple packets".

Some of them were subsection, some were new section, however, i also do not understand why they were subsection. The way it was, was just really confusing and this was the easier fix.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210540608

---

From: Nick Banks <[email-masked]>
Date: Thu, 16 Aug 2018 13:29:07 +0000 (UTC)

Maybe just keep the working from before and prepend `Per packet number space, `.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210595231

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Aug 2018 18:02:17 +0000 (UTC)

MikeBishop commented on this pull request.

I think sending the current value in each packet is a reasonable strategy, albeit verbose if you have a lot of streams.  It's certainly a good, even recommended, optimization to suppress that if it hasn't changed and one of them has been ACK'd.  But I think the larger point is that it's okay to duplicate, and how much to duplicate is entirely under the implementation's control.

Also, "again."

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210688936

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Aug 2018 18:07:17 +0000 (UTC)

The original text, "The CRYPTO frame" talks about the CRYPTO frame as a unique type.  "CRYPTO frames" talks about the class of frames of that type.  Either would work here.  "The CRYPTO frames" sounds like a discrete set of frames which are being partitioned into the spaces.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210690517

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Aug 2018 11:07:34 -0700

Also, "As shown"

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210690608

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Aug 2018 11:09:52 -0700

That works.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#discussion_r210691284

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Aug 2018 12:03:25 -0700

## Version Negotiation
 
 Version negotiation ensures that client and server agree to a QUIC version
-that is mutually supported. A server sends a Version Negotiation packet in
-response to each packet that might initiate a new connection, see
+that is mutually supported, independent of the implemented version. A server
+can send a Version Negotiation packet in response to each packet that might
+initiate a new connection and is sufficiently large; see 

Trailing whitespace here.

> @@ -2015,7 +2047,7 @@ Thus, a resumption token SHOULD include an expiration time.  The server MAY
+therefore unlikely to be successful. 

Trailing whitespace here.

-Accompanying documents describe QUIC's loss detection and congestion control
-{{QUIC-RECOVERY}}, and the use of TLS 1.3 for key negotiation {{QUIC-TLS}}.
-
-QUIC version 1 conforms to the protocol invariants in {{QUIC-INVARIANTS}}.
+An accompanying document describes QUIC's loss detection and congestion control
+{{QUIC-RECOVERY}}. Each implementation of QUIC will unilaterally select a loss
+detection and congestion control strategy. A recommended approach is described
+in {{QUIC-RECOVERY}}. This document specifies version 0x00000001 of QUIC which

The first sentence is superfluous given the second and third.

> @@ -182,8 +186,17 @@ QUIC packet:

-QUIC is a name, not an acronym.
+0-RTT:
+
+: QUIC packets encrypted with a key derived from previous QUIC connection

from **a** previous connection

Or explicitly make that statement about long-header packets.

I agree, we should converge.  Filed #1676 to track.

Then perhaps that's the statement that needs to be tightened.  Given the known-schema format of QUIC frames, I'd argue that you never send a frame that isn't supported by default in the negotiated version or explicitly indicated by a transport parameter.  It's hypothetically possible to use out-of-band knowledge, but that's a short path to things blowing up.

> @@ -1303,34 +1326,35 @@ the packet.  Servers SHOULD NOT attempt to decode or decrypt a packet from an
 
-Servers MUST drop other packets that contain unsupported versions.
+Servers MUST drop packets without sending a Version Negotiation packet when the
+received packet indicates an unsupported versions and is too small

version~s~

>  receiving a server response, so servers SHOULD ignore any such packets.
 
+Packets with a supported version, or no version field, are matched to
+a connection as described in {{packet-handling}}.

I still think this paragraph, plus the "If not matched..." sentence that was removed, belongs before line 1334.  Once we know it's a supported version, checking whether the packet belongs to an existing connection comes before checking whether its an Initial.

Same comment from before still applies:  If you've reached this point in the flow, it has a supported version. Unsupported versions, regardless of packet type, have already been rejected or dropped.

> @@ -1719,11 +1747,11 @@ type TRANSPORT_PARAMETER_ERROR.
 
+provided by the server in the preivous handshake.  The transport parameters that

"previous"

 

Given that we don't have any negotiated parameters right now and we've previously been reluctant to add them, I don't want to bake specifics into this draft.  Maybe we settle for alluding to the possibility and let someone define the specifics if it's ever needed?

"New transport parameters can be used to advertise support for optional features or negotiate changes to protocol behavior.  An endpoint MUST ignore transport parameters that it does not support.  Absence of a transport parameter indicates that a peer does not support the associated optional protocol feature."

 

Maybe "Downgrade attacks on QUIC can be attempted in two ways."

 

I see.  How about "To protect against these attacks, the cryptographic handshake reconfirms the negotiated version as part of the transport parameters, which are integrity protected.  As described in...."

That's not the way I read it when it was part of the same paragraph.  I read it as needing to tolerate at least three rounds of Initial-Retry-Initial-Retry-Initial-Retry-Initial....  Now that you've split it off, it could easily be read as a requirement for how many times each Initial gets retransmitted before the connection fails, and a note that a Retry resets your counter.

Both things probably need to be said.

Yeah, there's not a ton of wiggle room between choosing not to implement the SHOULD and still complying with the MUST NOT.  I'm okay with the change, unless anyone else objects.

> @@ -3415,7 +3454,11 @@ acknowledged again.
 Because ACK frames are not sent in response to ACK-only packets, a receiver that
 is only sending ACK frames will only receive acknowledgements for its packets
 if the sender includes them in packets with non-ACK frames.  A sender SHOULD
-bundle ACK frames with other frames when possible.
+bundle ACK frames with other frames when possible. A receiver that would only
+send ACK frames therefore can trigger an acknowledgement from the sender by
+sending a packets that contains another frame that would elicit an

a packet~s~

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#pullrequestreview-146962858

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Aug 2018 23:17:40 -0700

martinthomson commented on this pull request.

I think that I might like 50% of these changes.  There are lots of simple things, lots of good changes, lots of new text, but some of the changes are difficult.

This is going to take a while to digest in its current form.  It would be much easier if it were multiple PRs.

I'm OK with large sets of changes if they are all trivial, but it's very hard to extricate easy changes (the final capitalization of SHOULD is simple and good), from more complex ones (adding new states and transitions).

@MikeBishop, your example is a highly simplified view of 0-RTT :)

+                       | Recv MAX_STREAM_DATA

That sort of change has to be a separate PR.

"soons" "acknowlegded"

The new text is redundant though.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1665#pullrequestreview-147106864