Subject: [quicwg/base-drafts] recovery: persistent congestion and ack-eliciting (#4421)

From: msvoelker <[email-masked]>
Date: Wed, 02 Dec 2020 23:21:10 -0800

Removes the word ack-eliciting in the descriptive text to persistent congestion. It also contains minor text changes in the following of that section.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * recovery: persistent congestion and ack-eliciting

-- File Changes --

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

-- Patch Links --

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

---

From: msvoelker <[email-masked]>
Date: Wed, 02 Dec 2020 23:28:16 -0800

@msvoelker commented on this pull request.

Explanations to my changes in-line.

> @@ -990,17 +989,20 @@ congestion without depending on PTO expiration.
 
 ### Establishing Persistent Congestion
 
-A sender establishes persistent congestion on receiving an acknowledgement if at
-least two ack-eliciting packets are declared lost, and:
+A sender establishes persistent congestion after the receipt of an

I think it is better to write _after_. The check for persistent congestion might happen directly when receiving an ack, but it might also happen after the loss detection timer fires.

 * the duration between the send times of these two packets exceeds the
   persistent congestion duration ({{pc-duration}}); and
 
 * a prior RTT sample existed when both packets were sent.
 
+The two packets to consider need to be ack-eliciting, because otherwise a sender
+cannot expect to receive an acknowledgement after a specific time.

I tried to explain, why the first and last packet need to be ack-eliciting as far as I understand 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/4421#pullrequestreview-543666779

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 03 Dec 2020 10:51:18 -0800

@janaiyengar commented on this pull request.

This looks good, but I think we can add a MUST here -- I've suggested text. This is implied at the moment, but it's best to make it explicit. I don't think  this changes any requirements, so I would still think of this as editorial.

@ianswett?

```suggestion
These two packets MUST be ack-eliciting, since a receiver is required to
acknowledge only ack-eliciting packets within its maximum ack delay; see Section
13.2 of {{QUIC-TRANSPORT}}.
```

-- 
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/4421#pullrequestreview-544271620

---

From: ianswett <[email-masked]>
Date: Mon, 07 Dec 2020 13:12:32 -0800

@ianswett commented on this pull request.

Looking good, one further suggestion.

-* all packets, across all packet number spaces, sent between these two send
-  times are declared lost;
+* all packets, across all packet number spaces, sent between the send times of
+  these two packets are declared lost;

This is an existing problem, but I'd suggest
```suggestion
  two ack-eliciting packets are declared lost;
```

-- 
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/4421#pullrequestreview-546539820

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Dec 2020 20:36:50 -0800

@janaiyengar pushed 1 commit.

decd1ddc5f8235cbf2798fbb905cd1db92751a00  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/4421/files/a8d9ffe11366b3466576c166788873bfb4cd42aa..decd1ddc5f8235cbf2798fbb905cd1db92751a00

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Dec 2020 20:39:08 -0800

I'm taking the suggested changes and this PR. Thanks, @msvoelker !

-- 
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/4421#issuecomment-740370398

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 07 Dec 2020 20:39:18 -0800

Merged #4421 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/4421#event-4082823751

---

From: msvoelker <[email-masked]>
Date: Mon, 07 Dec 2020 22:41:48 -0800

All three bullet points referred to the "at least two ack-eliciting packets" (1) mentioned in the text before the bullet points. In my understanding of the new text, the ack-eliciting packets (2) mentioned in the first bullet point could be other packets than (1). Maybe (2) are not even declared lost.
Was this the intention or is my understanding just wrong?

-- 
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/4421#discussion_r538073397

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 08 Dec 2020 14:03:38 -0800

Thanks for catching this -- I've sent out #4429 

-- 
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/4421#discussion_r538841116