Subject: [quicwg/base-drafts] WGLC review nits for Recovery (#3877)

From: Mike Bishop <[email-masked]>
Date: Wed, 08 Jul 2020 13:20:15 -0700

Again, mostly nits.  Lots of parentheses.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * WGLC review nits

-- File Changes --

    M draft-ietf-quic-recovery.md (126)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3877.patch
https://github.com/quicwg/base-drafts/pull/3877.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/3877

---

From: Mike Bishop <[email-masked]>
Date: Wed, 08 Jul 2020 13:25:58 -0700

@MikeBishop pushed 1 commit.

7c7a75a193467119ebe42806694f42bdc1acb539  Revert one

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3877/files/3b24fdff91cd12c6da486ed48d52152f29928de2..7c7a75a193467119ebe42806694f42bdc1acb539

---

From: Mike Bishop <[email-masked]>
Date: Wed, 08 Jul 2020 13:26:00 -0700

@MikeBishop commented on this pull request.

> @@ -103,12 +103,12 @@ TCP implementations.
 
 Definitions of terms that are used in this document:
 
-Ack-eliciting Frames:
+Ack-eliciting frames:

I'd either title-case or sentence-case these terms, but right now it's mixed.

> @@ -338,7 +338,7 @@ mean variation.
 The calculation of smoothed_rtt uses path latency after adjusting RTT samples
 for acknowledgement delays. These delays are computed using the ACK Delay
 field of the ACK frame as described in Section 19.3 of {{QUIC-TRANSPORT}}.
-For packets sent in the ApplicationData packet number space, a peer limits
+For packets sent in the Application Data packet number space, a peer limits

This is two separate words in the other drafts; I changed it in the prose but left it in the pseudo-code, since it's an enum value there.

> @@ -430,13 +430,13 @@ thresholds to minimize recovery latency.
 
 The RECOMMENDED initial value for the packet reordering threshold
 (kPacketThreshold) is 3, based on best practices for TCP loss detection
-{{?RFC5681}} {{?RFC6675}}.  Implementations SHOULD NOT use a packet threshold
-less than 3, to keep in line with TCP {{?RFC5681}}.
+({{?RFC5681}}, {{?RFC6675}}).  Implementations SHOULD NOT use a packet threshold
+less than 3, to keep in line with TCP ({{?RFC5681}}).

"to keep in line with" felt awkward, but I didn't come up with a rephrasing I was confident of.  Maybe "comparable to TCP"?

> @@ -711,18 +710,18 @@ before Initial packets, early 0-RTT packets will be declared lost, but that
 is expected to be infrequent.
 
 It is expected that keys are discarded after packets encrypted with them would
-be acknowledged or declared lost.  Initial secrets however might be destroyed

This "might" is a MUST.

-- 
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/3877#pullrequestreview-445099674

---

From: ianswett <[email-masked]>
Date: Wed, 15 Jul 2020 18:59:01 -0700

@ianswett approved this pull request.

Thanks for this PR, it LG, but I have some suggestions(mostly to existing text) and comments.

Maybe "In order to remain similar to TCP, implementations SHOULD NOT use a packet threshold less than 3."?

> @@ -587,7 +587,7 @@ received from the client, because packets sent on PTO count against the
 anti-amplification limit. Note that the server could fail to validate the
 client's address even if 0-RTT is accepted.
 
-Since the server could be blocked until more packets are received from the
+Since the server could be blocked until more bytes are received from the

In this case, I prefer packets, since that infers that there are bytes received intended for the specific connection.

> @@ -711,18 +711,18 @@ before Initial packets, early 0-RTT packets will be declared lost, but that
 
 It is expected that keys are discarded after packets encrypted with them would
-be acknowledged or declared lost.  Initial secrets however might be destroyed
-sooner, as soon as handshake keys are available; see Section 4.11.1 of
-{{QUIC-TLS}}.
+be acknowledged or declared lost.  However, Initial secrets are destroyed as

```suggestion
be acknowledged or declared lost.  However, Initial secrets are discarded as
```

We could add a MUST, but there's no reason given the TLS draft has a MUST, unless we're trying to move the TLS references to informational rather than normative?

> +soon as handshake keys are available to both client and server; see Section
+4.11.1 of {{QUIC-TLS}}.

```suggestion
soon as handshake keys are proven to be available to both client and server;
see Section 4.11.1 of {{QUIC-TLS}}.
```

-- 
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/3877#pullrequestreview-449453013

---

From: ianswett <[email-masked]>
Date: Wed, 15 Jul 2020 19:01:59 -0700

@ianswett pushed 1 commit.

80019413ff190246786cdf10df3445da9a2c36d9  Merge branch 'master' into recovery/mbishop_wglc

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3877/files/7c7a75a193467119ebe42806694f42bdc1acb539..80019413ff190246786cdf10df3445da9a2c36d9

---

From: Mike Bishop <[email-masked]>
Date: Tue, 21 Jul 2020 06:58:17 -0700

f5443e87531d974f0c9cc913572f764dfa87dcec  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/3877/files/80019413ff190246786cdf10df3445da9a2c36d9..f5443e87531d974f0c9cc913572f764dfa87dcec

---

From: Mike Bishop <[email-masked]>
Date: Tue, 21 Jul 2020 07:00:48 -0700

The trouble with that is, they don't have to be validly formed packets, do they?  Just attributable to a single connection, which might well be by address.

>  For the purposes of avoiding amplification prior to address validation, servers MUST count all of the payload bytes received in datagrams that are uniquely attributed to a single connection. This includes datagrams that contain packets that are successfully processed and datagrams that contain packets that are all discarded.

Maybe meet in the middle on "datagrams"?

-- 
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/3877#discussion_r458117158

---

From: Mike Bishop <[email-masked]>
Date: Tue, 21 Jul 2020 14:01:16 +0000 (UTC)

44fdab5abd2e05295308f97eb6c86c618d7b1e3b  Ian's suggested wording

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3877/files/f5443e87531d974f0c9cc913572f764dfa87dcec..44fdab5abd2e05295308f97eb6c86c618d7b1e3b

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 21 Jul 2020 19:15:58 +0000 (UTC)

@janaiyengar commented on this pull request.

Thanks, @MikeBishop! Just one comment but LGTM otherwise.

 
-Ack-eliciting Frames:
+Ack-eliciting frames:

I think sentence-case is better.

I like datagrams here -- that is the same language in the transport draft.

-- 
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/3877#pullrequestreview-452751543

---

From: Mike Bishop <[email-masked]>
Date: Tue, 21 Jul 2020 13:19:49 -0700

c75e8fa078ca497824b556afc0cf6024faf72413  Datagrams it is

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3877/files/44fdab5abd2e05295308f97eb6c86c618d7b1e3b..c75e8fa078ca497824b556afc0cf6024faf72413

---

From: ianswett <[email-masked]>
Date: Tue, 21 Jul 2020 14:06:43 -0700

@ianswett commented on this pull request.

Good point, datagrams is the correct answer I believe.

-- 
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/3877#discussion_r458387730

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 21 Jul 2020 14:19:59 -0700

@janaiyengar approved this pull request.

-- 
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/3877#pullrequestreview-452837260

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 21 Jul 2020 14:21:23 -0700

Merged #3877 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/3877#event-3573179873