Subject: [quicwg/base-drafts] Editorial suggestions from Benjamin Kaduk (recovery) (#4685)

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 22:13:30 -0800

I made some proposed tweaks in my local copy as I was doing my IESG review.  There&#39;s a couple places where I&#39;m not sure if it is actually editorial or the proposal actually changes the meaning (and thus should be rejected).
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * kaduk/iesg-evaluation

-- File Changes --

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

-- Patch Links --

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

---

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 22:17:18 -0800

@kaduk commented on this pull request.

> @@ -354,7 +355,7 @@ frequently observable.
 ## Estimating smoothed_rtt and rttvar {#smoothed-rtt}
 
 smoothed_rtt is an exponentially-weighted moving average of an endpoint's RTT
-samples, and rttvar is the variation in the RTT samples, estimated using a
+samples, and rttvar represents the variation in the RTT samples, estimated using a

(if it's "estimated", then it's not "is")

> @@ -767,7 +768,7 @@ initial RTT estimate.
 
 ## Discarding Keys and Packet State {#discarding-packets}
 
-When packet protection keys are discarded (see Section 4.9 of {{QUIC-TLS}}),
+When packet protection keys are discarded from an entire encryption level (see Section 4.9 of {{QUIC-TLS}}),

I believe that we refer to the 1-RTT key update procedure as "discarding" the old generation of keys.  My understanding is that the loss of recovery state described here does not apply to 1-RTT key update.

> @@ -1433,7 +1437,7 @@ UpdateRtt(ack_delay):
   // min_rtt ignores acknowledgment delay.
   min_rtt = min(min_rtt, latest_rtt)
   // Limit ack_delay by max_ack_delay after handshake
-  // confirmation. Note that ack_delay is 0 for
+  // confirmation. Note that ack_delay is clamped to 0 for

ack_delay is an input to this function, and I didn't remember any external constraint that the provided function argument would be zero in this case.

> @@ -1526,7 +1530,7 @@ SetLossDetectionTimer():
     loss_detection_timer.cancel()
     return
 
-  // Determine which PN space to arm PTO for.

stale comment?

> @@ -1783,7 +1788,7 @@ OnCongestionEvent(sent_time):
 
   // Enter recovery period.
   congestion_recovery_start_time = now()
-  ssthresh = congestion_window * kLossReductionFactor
+  ssthresh = congestion_window * (1 - kLossReductionFactor)

I tried to triple check this, but there is not much to compare against.
If the loss reduction factor is supposed to indicate the amount of **reduction**, we need this change.  When the value is 0.5 it doesn't matter, of course...

-- 
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/4685#pullrequestreview-563233569

---

From: ianswett <[email-masked]>
Date: Thu, 07 Jan 2021 12:57:25 -0800

@ianswett commented on this pull request.

Let's just remove this comment.

-- 
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/4685#discussion_r553583360

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 13:01:19 -0800

Do you want me to be making updates to the PR, or can you take care of anything that's needed?

-- 
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/4685#discussion_r553585099

---

From: ianswett <[email-masked]>
Date: Thu, 07 Jan 2021 13:05:59 -0800

I'm happy to do it.

-- 
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/4685#discussion_r553586990

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 13:08:45 -0800

Thank you!

-- 
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/4685#discussion_r553588221

---

From: ianswett <[email-masked]>
Date: Thu, 07 Jan 2021 13:11:02 -0800

> @@ -364,12 +365,12 @@ ACK frames as described in Section 19.3 of {{QUIC-TRANSPORT}}.
 The peer might report acknowledgment delays that are larger than the peer's
 max_ack_delay during the handshake (Section 13.2.1 of {{QUIC-TRANSPORT}}). To
 account for this, the endpoint SHOULD ignore max_ack_delay until the handshake
-is confirmed (Section 4.1.2 of {{QUIC-TLS}}). When they occur, these large
+is confirmed (as defined in Section 4.1.2 of {{QUIC-TLS}}). When they occur, these large

```suggestion
is confirmed, as defined in Section 4.1.2 of {{QUIC-TLS}}. When they occur, these large
```

-- 
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/4685#pullrequestreview-563828201

---

From: ianswett <[email-masked]>
Date: Thu, 07 Jan 2021 13:11:07 -0800

@ianswett pushed 1 commit.

1f1d5a20440190be3ff3bb09e552a4edbaf2e7ef  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/1f161d91035ddc41bcdfe1da8f8d819479f9e31d..1f1d5a20440190be3ff3bb09e552a4edbaf2e7ef

---

From: Martin Thomson <[email-masked]>
Date: Thu, 07 Jan 2021 17:30:10 -0800

@martinthomson commented on this pull request.

The factor is multiplicative in cubic (0.7 there), so 1-B is likely to be problematic.

-- 
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/4685#discussion_r553689321

---

From: Martin Thomson <[email-masked]>
Date: Thu, 07 Jan 2021 17:31:04 -0800

It is if the definition of the term is an representation of an estimate, which is what it is.

-- 
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/4685#discussion_r553689551

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Jan 2021 19:08:32 -0800

@marten-seemann commented on this pull request.

> @@ -784,7 +785,7 @@ If a server accepts 0-RTT, but does not buffer 0-RTT packets that arrive
 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
+It is expected that keys are discarded after all packets encrypted with them would

I'm not sure if this is correct. You could receive HANDSHAKE_DONE without having received all ACKs for Handshake packets.

> @@ -1300,7 +1304,7 @@ largest_acked_packet\[kPacketNumberSpace]:
 : The largest packet number acknowledged in the packet number space so far.
 
 loss_time\[kPacketNumberSpace]:
-: The time at which the next packet in that packet number space will be
+: The earliest time at which an additional packet in that packet number space will be

What was wrong with the original sentence?

 

```suggestion
When Initial and Handshake packet protection keys are discarded (see Section 4.9 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/4685#pullrequestreview-563974588

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 20:37:26 -0800

"next" could be ambiguous about applying to the time axis or the packet number axis.

-- 
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/4685#discussion_r553737156

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 20:38:35 -0800

I don't think the comma is appropriate if the claim is that rttvar is defined to be the estimated variation of the RTT sample.

-- 
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/4685#discussion_r553737416

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 20:38:59 -0800

@kaduk pushed 1 commit.

7da60a4549d1997e68e86fc3cdb0bb426f745706  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/1f1d5a20440190be3ff3bb09e552a4edbaf2e7ef..7da60a4549d1997e68e86fc3cdb0bb426f745706

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Jan 2021 20:41:12 -0800

As packet numbers are used sequentially, those axis should be effectively equivalent.

-- 
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/4685#discussion_r553738009

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 20:45:43 -0800

Hmm.  I considered s/all/any/ but am not sure that helps.  The best idea I have right now (just before bedtime...) is to add to the next sentence that Handshake keys are discarded when the handshake is confirmed.

-- 
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/4685#discussion_r553739125

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 20:47:17 -0800

So we should change the definition of kLossReductionFactor to be something like "scaling factor applied to reduce the congestion window when loss is detected"?

-- 
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/4685#discussion_r553739519

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 06:22:23 -0800

e2d600a620be3106583e5629fe1dddc615a318bc  Remove stale comments from pseudocode

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/7da60a4549d1997e68e86fc3cdb0bb426f745706..e2d600a620be3106583e5629fe1dddc615a318bc

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 06:34:49 -0800

9ebebec90f3db6739573fc281e6d5b6969f1d199  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/e2d600a620be3106583e5629fe1dddc615a318bc..9ebebec90f3db6739573fc281e6d5b6969f1d199

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:04:38 -0800

27828eaefcdfea57a254af5c547b0228e74fdc55  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/9ebebec90f3db6739573fc281e6d5b6969f1d199..27828eaefcdfea57a254af5c547b0228e74fdc55

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:17:11 -0800

747f2df86d5d21ceee6893af2a701a9dce62b1c5  Initial and Handshake keys are discarded early

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/27828eaefcdfea57a254af5c547b0228e74fdc55..747f2df86d5d21ceee6893af2a701a9dce62b1c5

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:24:07 -0800

ce76342e85032c483555365daa3de88cdd818d56  kLossReductionFactor

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/747f2df86d5d21ceee6893af2a701a9dce62b1c5..ce76342e85032c483555365daa3de88cdd818d56

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:25:58 -0800

dce8d4830a8d0be51b76bead80e6019553b0a16d  Update draft-ietf-quic-recovery.md

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

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:28:47 -0800

51c33cb555ca5c547a3bb324ef55667e950c4631  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/dce8d4830a8d0be51b76bead80e6019553b0a16d..51c33cb555ca5c547a3bb324ef55667e950c4631

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:30:51 -0800

> @@ -609,7 +610,7 @@ or acknowledged, when the handshake is confirmed (Section 4.1.2 of
 of the round-trip time and for the correct packet across packet number spaces.
 
 When a PTO timer expires, the PTO backoff MUST be increased, resulting in the
-PTO period being set to twice its current value. The PTO backoff factor is reset
+PTO period being set to twice its current value, in each packet number space. The PTO backoff factor is reset

There's only one PTO alarm/timeout, so reverting this change.

I can change this to "and rttvar is the mean variation in the RTT samples." if that's preferable?

 

I think there's a note about how key update in QUIC is slightly different from TLS, so I believe the existing text is more correct.  @martinthomson to confirm.

This sentence is stale, removed.

I'll update the description to be less ambiguous.

+1 to Marten's comment, reverting most of it, but changing will to can, since timers can wake up late.

-- 
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/4685#pullrequestreview-563829512

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:32:28 -0800

6aed6d7f2c7a6114294bf93e846ceac6e464ca1f  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/51c33cb555ca5c547a3bb324ef55667e950c4631..6aed6d7f2c7a6114294bf93e846ceac6e464ca1f

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:34:34 -0800

1a45d842269928ceabe7d482ee78ebc09996ff50  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/6aed6d7f2c7a6114294bf93e846ceac6e464ca1f..1a45d842269928ceabe7d482ee78ebc09996ff50

---

From: kaduk <[email-masked]>
Date: Fri, 08 Jan 2021 12:49:46 -0800

Okay, sorry for the confusion.  I was just keying off of:

>   Even when there are ack-
   eliciting packets in-flight in multiple packet number spaces, the
   exponential increase in probe timeout occurs across all spaces to
   prevent excess load on the network.  For example, a timeout in the
   Initial packet number space doubles the length of the timeout in the
   Handshake 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/4685#discussion_r554183875

---

From: ianswett <[email-masked]>
Date: Fri, 08 Jan 2021 12:58:21 -0800

Yeah, in this case across all is not quite the same as for each, but I agree the difference is subtle.

-- 
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/4685#discussion_r554187619

---

From: Martin Thomson <[email-masked]>
Date: Sun, 10 Jan 2021 17:41:49 -0800

@martinthomson approved this pull request.

> @@ -132,7 +132,8 @@ the encryption level and includes a packet sequence number (referred to below as
 a packet number).  The encryption level indicates the packet number space, as
 described in {{QUIC-TRANSPORT}}.  Packet numbers never repeat within a packet
 number space for the lifetime of a connection.  Packet numbers are sent in
-monotonically increasing order within a space, preventing ambiguity.
+monotonically increasing order within a space, preventing ambiguity.  It is
+permitted for some packet numbers to never be used, leaving intentional gaps.

Do we now also have to acknowledge the fact that the loss recovery algorithms assume the absence of gaps?

> +can no longer be acknowledged because their acknowledgments cannot be processed
+anymore. The sender MUST discard all recovery state associated with those packets

```suggestion
can no longer be acknowledged because their acknowledgments cannot be processed.
The sender MUST discard all recovery state associated with those packets
```

-- 
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/4685#pullrequestreview-564925638

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:10:28 -0800

@janaiyengar commented on this pull request.

The algorithm does not assume the absence of gaps -- Section 6.1 says: `The packet was sent kPacketThreshold packets before an acknowledged packet`. The pseudo-code does assume it though, and where it does, it is stated in a comment (see A.10).

All in all, I think this addition is safe.

-- 
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/4685#discussion_r554775003

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:10:46 -0800

@janaiyengar pushed 1 commit.

c3a2231b78208313d7f5cecab0b3292ca3d5cf8d  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/1a45d842269928ceabe7d482ee78ebc09996ff50..c3a2231b78208313d7f5cecab0b3292ca3d5cf8d

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:12:03 -0800

@janaiyengar approved this pull request.

I think all of these are good. @ianswett -- feel free to merge after you've had a chance to review.

-- 
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/4685#pullrequestreview-565086589

---

From: kaduk <[email-masked]>
Date: Mon, 11 Jan 2021 00:00:33 -0800

My expectations matched what Jana describes.

-- 
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/4685#discussion_r554863464

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Jan 2021 16:37:22 -0800

> +samples, and rttvar represents the variation in the RTT samples, estimated using
+a mean variation.

```suggestion
samples, and rttvar estimates the variation in the RTT samples using a mean
variation.
```

-- 
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/4685#pullrequestreview-566810637

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jan 2021 16:37:52 -0800

@ianswett 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/4685#pullrequestreview-566810950

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Jan 2021 16:38:19 -0800

> +packet number space that was acknowledged.  This might result in erroneously
+declaring persistent congestion when including all packet number spaces would
+not have declared persistent connection, but will not lead to a failure to
+detect persistent congestion.

```suggestion
packet number space that was acknowledged.  This might result in erroneously
declaring persistent congestion.
```

-- 
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/4685#pullrequestreview-566811074

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 12 Jan 2021 16:38:59 -0800

```suggestion
packet number space that was acknowledged.  This might result in erroneously
declaring persistent congestion, but it will not lead to a failure to detect
persistent congestion. 
```

-- 
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/4685#pullrequestreview-566811308

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jan 2021 16:43:22 -0800

0f65c23979e8eadd9a1bbc4d1fca806fc5018760  Merge branch 'master' into recovery-iesg-eval

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/c3a2231b78208313d7f5cecab0b3292ca3d5cf8d..0f65c23979e8eadd9a1bbc4d1fca806fc5018760

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jan 2021 17:18:10 -0800

0247ef01087f52ed925ba8f7c9e269050b10a81e  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/4685/files/0f65c23979e8eadd9a1bbc4d1fca806fc5018760..0247ef01087f52ed925ba8f7c9e269050b10a81e

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jan 2021 17:18:28 -0800

-- 
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/4685#pullrequestreview-566825425

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jan 2021 17:22:00 -0800

d2a8670fcd88f56584012ef4bbb3b6a122c6db38  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4685/files/0247ef01087f52ed925ba8f7c9e269050b10a81e..d2a8670fcd88f56584012ef4bbb3b6a122c6db38

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 12 Jan 2021 17:23:34 -0800

Merged #4685 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/4685#event-4198956848

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Jan 2021 17:23:35 -0800

-- 
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/4685#pullrequestreview-566827314