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

From: Jana Iyengar <[email-masked]>
Date: Wed, 01 Jul 2020 22:47:04 -0700

@janaiyengar commented on this pull request.

Doing this in chunks.

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

This is supposed to say that the IP protocol number indicates UDP. We discussed this when adding the text, and couldn't come up with a better way to say it.

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

I don't feel great about this ... it is slightly better than the old text but it's still unclear exactly what is meant. How about "This identifier is set by an endpoint for its peer ..."

 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.

They are very slightly different -- this para says that an endpoint that wants to stop retransmitting MUST send a RESET_STREAM first. The earlier para says that a RESET_STREAM is sent instead of a retransmission.

That said, I think this is basically redundant. I would replace the language in the earlier para with the one from here, which is more accurate. We don't need the endpoint to send RESET_STREAM every time it detects a loss. Only that it send a RESET_STREAM before stopping retrnamissions.

-- 
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-441360363

---

From: Mike Bishop <[email-masked]>
Date: Thu, 02 Jul 2020 06:31:45 -0700

@MikeBishop pushed 1 commit.

f9fdab10ce23e66de11ac3447b1040cef39fe24c  For for for

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/f7b6aab88c9b98000bd91ae3ed817188ffb21fe9..f9fdab10ce23e66de11ac3447b1040cef39fe24c

---

From: Mike Bishop <[email-masked]>
Date: Thu, 02 Jul 2020 06:39:57 -0700

4a967aa11e096c98f927bd5a9eb29dd6c170edc0  Clarify STOP_SENDING in "Data Sent"

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/f9fdab10ce23e66de11ac3447b1040cef39fe24c..4a967aa11e096c98f927bd5a9eb29dd6c170edc0

---

From: Mike Bishop <[email-masked]>
Date: Thu, 02 Jul 2020 06:41:54 -0700

@MikeBishop commented on this pull request.

QUIC is defined (per invariants) to run over UDP.  I don't think including a constant in the definition of the tuple gains you any uniqueness, and stating that it's a UDP port already emphasizes what the protocol has to be.

That's workable, but the point of my edit is not to imply the CID is 1:1 with the connection itself.  Perhaps we use similar words to the version change being proposed, "Opaque bytes that permit the recipient to associate a packet with a connection.  Each endpoint can supply one or more sequences of bytes for its peer to use in sending packets."?

I think the real point is that you don't have to send a RESET_STREAM immediately if you've already sent everything and closed the stream, but you SHOULD send a RESET_STREAM if any data later gets lost.  See if this new text works better.

-- 
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#discussion_r448998529

---

From: Martin Thomson <[email-masked]>
Date: Thu, 02 Jul 2020 22:57:18 -0700

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

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

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?

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

---

From: Mike Bishop <[email-masked]>
Date: Tue, 07 Jul 2020 15:33:20 -0700

It reads to me like baggage; it's only notable because it didn't used to be true.  On a fresh read without the mental baggage, it just feels strange to call *that one type* out.  The alternative to me would be identifying the first one for each type, but since that's simply the low-order bits with the higher bits zero, it felt superfluous.

-- 
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#discussion_r451181154

---

From: Mike Bishop <[email-masked]>
Date: Tue, 07 Jul 2020 15:35:45 -0700

The trouble is that in the -tls draft, these packet types do use the thing we call "packet protection."  Happy to reword here, but I think saying the packets don't have something they clearly do have isn't the right solution.

-- 
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#discussion_r451182015

---

From: Martin Thomson <[email-masked]>
Date: Tue, 07 Jul 2020 17:36:15 -0700

I see your point.  Let's move this to a new issue. 

-- 
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#discussion_r451217706

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Jul 2020 08:03:37 -0700

@MikeBishop pushed 2 commits.

c0d8a453b56a7f51ea56beae22d12073dd227b9b  Incorporating feedback
af4f6bc3959128e04f28ae07695f58f62caaaabc  Merge remote-tracking branch 'origin/master' into transport/mbishop_wglc

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/4a967aa11e096c98f927bd5a9eb29dd6c170edc0..af4f6bc3959128e04f28ae07695f58f62caaaabc

---

From: Mike Bishop <[email-masked]>
Date: Thu, 09 Jul 2020 08:03:41 -0700

I've adjusted the preceding paragraph to note that each stream space begins at the minimum value and increases monotonically, since that's effectively what we're trying to communicate here.

Reverted and opened #3887.

Tightened scope a bit.

Okay; reverted.

I'm unclear on what makes them different.  I've been using the distinction that where the reference is explicitly part of the sentence ("defined in {{ref}}") it doesn't need parentheses, but when it's just dropping a reference mid-sentence ("UDP {{ref}} tells us that") then it is parenthetical.  But yes, we should decide on something uniform and reconcile.

-- 
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#discussion_r452261627

---

From: Martin Thomson <[email-masked]>
Date: Thu, 09 Jul 2020 17:28:54 -0700

I think that the prevailing use of external references - those that appear in brackets - is both inline "defined in [REF]" and not as an automatic parenthetical, but an annotation of a noun in cases like "such as UDP [REF]", which you would convert to something like `<a href="REF">UDP</a>` were this text.  That's harder to codify though.  In many ways this is a limitation of markdown, but I don't see it as a serious shortcoming, because fixes (such as those in HTML or XML) just transfer the problem to the presentation layer.

-- 
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#discussion_r452560291

---

From: Mike Bishop <[email-masked]>
Date: Fri, 10 Jul 2020 13:16:59 -0700

Ah, while I'm looking at the text output and saying "'UDP [UDP]' looks a bit strange; let's fix that."

-- 
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#discussion_r453058144

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 10 Jul 2020 14:56:23 -0700

@dtikhonov commented on this pull request.

 Client:
 
 : The endpoint initiating a QUIC connection.
 
 Server:
 
-: The endpoint accepting incoming QUIC connections.
+: The endpoint accepting an incoming QUIC connection.

👎 : the proposed version sounds worse.

-- 
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-446721485

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 10 Jul 2020 14:57:32 -0700

Connection ID:
 
 : An opaque identifier that is used to identify a QUIC connection at an
-  endpoint.  Each endpoint sets a value for its peer to include in packets sent
-  towards the endpoint.
+  endpoint.  Each endpoint selects one or more Connection IDs for its peer to

s/selects/specifies/ or s/selects/provides/

-- 
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-446721895

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 10 Jul 2020 14:58:28 -0700

> @@ -240,7 +240,7 @@ Application:
 
 ## Notational Conventions
 
-Packet and frame diagrams in this document use a bespoke format. The purpose of
+Packet and frame diagrams in this document use a custom format. The purpose of

An enthusiastic 👍 

-- 
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-446722240

---

From: Mike Bishop <[email-masked]>
Date: Fri, 10 Jul 2020 17:23:09 -0700

Client:

 Server:

Can you expand on why?  I'm adjusting it because nothing in the server role requires it to be accepting multiple connections.  We speak about clients initiating "a QUIC connection," and for symmetry should talk about a server accepting an individual QUIC connection.  Dealing with multiple simultaneous connections is something clients and servers both might do, but it's not fundamental to the definition of either client or server.

-- 
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#discussion_r453130041

---

From: Dmitri Tikhonov <[email-masked]>
Date: Sat, 11 Jul 2020 06:59:08 -0700

Client:

 Server:

You are right: nothing in the server role mandates that the server accept more than one connection.  On the other hand, in the majority of cases, from literature to practical implementations, a server does accept more than one connection.  Hence, my own mental model is that of "one server, many clients."  I predict this model is shared by the majority of readers, which is why I dislike the proposed change in wording.

This is not to say that the new version is bad; it is just worse in comparison.  The former definition causes less mental load because it fits the mental model.

While we are on this sentence: perhaps drop "incoming?"

-- 
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#discussion_r453197698

---

From: Martin Thomson <[email-masked]>
Date: Sun, 12 Jul 2020 20:44:54 -0700

@martinthomson approved this pull request.

I'm happy with this.

 Client:

 Server:

```suggestion
: The endpoint that accepts a QUIC connection.
```

> @@ -205,28 +205,28 @@ Out-of-order packet:
 
 Endpoint:
 
-: An entity that can participate in a QUIC connection by generating,
-  receiving, and processing QUIC packets. There are only two types of endpoint
-  in QUIC: client and server.
+: An entity that can participate in a QUIC connection by generating, receiving,
+  and processing QUIC packets. There are only two types of endpoint in QUIC:
+  client and server.
 
 Client:

```suggestion
: The endpoint that initiates a QUIC connection.
```

-- 
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-446963416

---

From: Mike Bishop <[email-masked]>
Date: Mon, 13 Jul 2020 11:55:35 -0700

8c4fe3d99fb185ac0ced223c4b9405fde94db45d  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/af4f6bc3959128e04f28ae07695f58f62caaaabc..8c4fe3d99fb185ac0ced223c4b9405fde94db45d

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Jul 2020 00:27:40 -0700

@martinthomson pushed 1 commit.

8b60a676470b4eb51beb3cef6c5c73606f1f90f3  Merge branch 'master' into transport/mbishop_wglc

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/8c4fe3d99fb185ac0ced223c4b9405fde94db45d..8b60a676470b4eb51beb3cef6c5c73606f1f90f3

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Jul 2020 00:33:25 -0700

4bbc69fa482ab5811a2b094413ab66e6220c8162  Fix grammar, wrap

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/8b60a676470b4eb51beb3cef6c5c73606f1f90f3..4bbc69fa482ab5811a2b094413ab66e6220c8162

---

From: Mike Bishop <[email-masked]>
Date: Tue, 14 Jul 2020 08:00:58 -0700

f2e30d93b472ab3059e24c3ea91d48607e39f772  Could swear I fixed this, which probably means a conflict later

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3805/files/4bbc69fa482ab5811a2b094413ab66e6220c8162..f2e30d93b472ab3059e24c3ea91d48607e39f772

---

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Jul 2020 15:43:52 -0700

Merged #3805 into master.

-- 
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#event-3546192908