Re: [quicwg/base-drafts] Massive batch of probably-editorial nits (#3805)

[Martin Thomson <[e-mail]>]

@martinthomson commented on this pull request.

These are pretty uniformly good, except for a few cases.

I found one genuine error, and have a few suggestions.

I do question the change to add parentheses for references to citations, that is `({{?FOO}})` as opposed to `{{?FOO}}`.  The extra line noise in output is a little difficult to parse.  Maybe we need to agree on an editorial policy regarding this.

> +: When used without qualification, the tuple of IP version, IP address, and UDP
+  port number that represents one end of a network path.

I think that Mike's edit is fine.  We can ignore the fixed parts of this, which includes that it is QUIC.

> +  endpoint.  Each endpoint sets one or more values for its peer to include in
+  packets sent towards the endpoint.

I think that we should be saying "selects one or more connection IDs".  I might have misread the suggestion here like Jana and read it to mean that a connection ID is "one or more values".

> @@ -235,12 +235,12 @@ Stream:
 
 Application:
 
-: An entity that uses QUIC to send and receive data.
+: An entity running on an endpoint that uses QUIC to send and receive data.

I'm mildly negative on this.  This says too much about how the application and stack are related.

> @@ -357,7 +357,6 @@ Within each type, streams are created with numerically increasing stream IDs.  A
 stream ID that is used out of order results in all streams of that type with
 lower-numbered stream IDs also being opened.
 
-The first bidirectional stream opened by the client has a stream ID of 0.

Removed on what basis?  I know that this *seems* intuitive, and we do say 0..2^62-1 but an explicit statement that identifies the first value isn't harmful, is it?

Or are you saying that we have too much baggage and we shouldn't let that show so much?

>  
 An endpoint SHOULD copy the error code from the STOP_SENDING frame to the
 RESET_STREAM frame it sends, but MAY use any application error code.  The
 endpoint that sends a STOP_SENDING frame MAY ignore the error code carried in
 any RESET_STREAM frame it receives.
 
-If the STOP_SENDING frame is received on a stream that is already in the
-"Data Sent" state, an endpoint that wishes to cease retransmission of
-previously-sent STREAM frames on that stream MUST first send a RESET_STREAM
-frame.

I like the new text.  Thanks.

> @@ -1165,9 +1155,9 @@ discarded if they indicate a different protocol version than that of the
 connection, or if the removal of packet protection is unsuccessful once the
 expected keys are available.
 
-Invalid packets without packet protection, such as Initial, Retry, or Version
-Negotiation, MAY be discarded.  An endpoint MUST generate a connection error if
-it commits changes to state before discovering an error.
+Invalid packets without effective confidentiality protection, such as Initial,

! The operative protection provided here is data origin authentication, which is why I chose to talk about packet protection instead (rather than introduce people to a new concept).

> @@ -1189,11 +1179,10 @@ that packet.
 ### Server Packet Handling {#server-pkt-handling}
 
 If a server receives a packet that indicates an unsupported version but is large
-enough to initiate a new connection for any one supported version,
-the server SHOULD send a Version Negotiation packet as described in
-{{send-vn}}. Servers MAY limit the number of packets that it responds to with a
-Version Negotiation packet. Servers MUST drop smaller packets that specify
-unsupported versions.
+enough to initiate a new connection for any one supported version, the server

```suggestion
enough to initiate a new connection for any supported version, the server
```

> @@ -2496,9 +2478,8 @@ ensure connection stability. This section describes the protocol for migrating a
 connection to a preferred server address.
 
 Migrating a connection to a new server address mid-connection is left for future
-work. If a client receives packets from a new server address not indicated by
-the preferred_address transport parameter, the client SHOULD discard these
-packets.
+work. If a client receives packets from a new server address when the client has
+not initiated a migration, the client SHOULD discard these packets.

This is slightly different.  It's not as tight as it was, because it could be read to allow any server address to be used if the client decides to migrate.  That's probably not your intent.

> +included.  Including older packets reduces the chance of spurious
+retransmissions caused by losing previously sent ACK frames, at the cost of
+larger ACK frames.

```suggestion
included.  Including acknowledgments for older packets reduces the chance of
spurious retransmissions caused by losing previously sent ACK frames, at the
cost of larger ACK frames.
```

>  such as once per round trip, to elicit an ACK from the peer.
 
-A receiver MUST NOT bundle an ack-eliciting frame with packets that would
+A receiver MUST NOT send an ack-eliciting frame in all packets that would

I agree.  (Though it was probably me that deleted it.)

>  
 * If all packets that were sent with the ECT(0) codepoint are eventually deemed
-  lost {{QUIC-RECOVERY}}, validation is deemed to have failed.
+  lost ({{QUIC-RECOVERY}}), validation is deemed to have failed.

```suggestion
  lost (Section 6 of {{QUIC-RECOVERY}}), validation is deemed to have failed.
```

>  
 To reduce the chances of misinterpreting congestive loss as packets dropped by a
 faulty network element, an endpoint could set the ECT(0) codepoint for only the
 first ten outgoing packets on a path, or for a period of three RTTs, whichever
 occurs first.
 
 Other methods of probing paths for ECN support are possible, as are different
-marking strategies. Implementations MAY use other methods defined in RFCs; see
-{{?RFC8311}}. Implementations that use the ECT(1) codepoint need to perform ECN
-validation using ECT(1) counts.
+marking strategies. Implementations MAY use other methods; see {{?RFC8311}}.

The defined in RFCs is a restatement of the requirements in RFC 8311 that I was specifically requested to include.  I know that it is clumsy, but it's an important point (and apparently a touchy one).

> @@ -3924,7 +3907,8 @@ endpoints risk datagrams being lost if they send packets larger than the
 smallest allowed maximum packet size of 1200 bytes.
 
 UDP datagrams MUST NOT be fragmented at the IP layer.  In IPv4
-{{!IPv4=RFC0791}}, the DF bit MUST be set to prevent fragmentation on the path.
+({{!IPv4=RFC0791}}), the DF bit MUST be set to prevent fragmentation on the

I generally haven't bothered with parentheses for citations in these cases, as they get brackets.

> @@ -4008,8 +3992,8 @@ off-path injection as specified in {{!RFC8201}} and Section 5.2 of {{!RFC8085}}.
 This validation SHOULD use the quoted packet supplied in the payload of an ICMP
 message to associate the message with a corresponding transport connection (see
 Section 4.6.1 of {{!DPLPMTUD}}).  ICMP message validation MUST include matching
-IP addresses and UDP ports {{!RFC8085}} and, when possible, connection IDs to an
-active QUIC session.  The endpoint SHOULD ignore all ICMP messages that fail
+IP addresses and UDP ports ({{!RFC8085}}) and, when possible, connection IDs to

Unlike above, this would seem to require the parenthetical.

> @@ -5825,8 +5810,8 @@ NEW_CONNECTION_ID frames contain the following fields:
 
 Sequence Number:
 
-: The sequence number assigned to the connection ID by the sender.  See
-  {{issue-cid}}.
+: The sequence number assigned to the connection ID by the sender, encoded as a
+  variable-length integer.  See {{issue-cid}}.

or
```suggestion
  variable-length integer; see {{issue-cid}}.
```

-- 
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/3805#pullrequestreview-442152935