Subject: [quicwg/base-drafts] Use ACK-eliciting packets in flight to identify anti-deadlock probes (#4848)

From: Benjamin Saunders <[email-masked]>
Date: Sun, 21 Mar 2021 15:06:33 -0700

If a client&#39;s first Handshake ACK is padded (e.g. due to being
coalesced with an Initial packet) and the server&#39;s first flight is
incomplete due to anti-amplification limits, an anti-deadlock probe
may be necessary even though bytes_in_flight is nonzero.

Fixes #4846 .
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Use ACK-eliciting packets in flight to identify anti-deadlock probes

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Sun, 21 Mar 2021 16:42:28 -0700

@martinthomson approved this pull request.

This looks reasonable to me.  It's just a pseudocode fix, so the harm is limited.

> @@ -1557,7 +1557,7 @@ OnLossDetectionTimeout():
     SetLossDetectionTimer()
     return
 
-  if (bytes_in_flight > 0):
+  if (any ack-eliciting packets in flight):

I wonder if it is worth inverting this check here, so that the condition matches the above one.  It's a negative, but the any/no disparity is odd.  The other call site in SetLossDetectionTimer uses "no ack-eliciting packets in flight" as well.

-- 
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/4848#pullrequestreview-617076968

---

From: Benjamin Saunders <[email-masked]>
Date: Sun, 21 Mar 2021 18:36:52 -0700

@Ralith pushed 1 commit.

2bd9bb1d698ec16a0048b2a304a443533bd5442e  Flip condition around for consistency

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/b63bc5ac3724249bcb61698239dfbc130c796ca2..2bd9bb1d698ec16a0048b2a304a443533bd5442e

---

From: Benjamin Saunders <[email-masked]>
Date: Sun, 21 Mar 2021 18:37:05 -0700

@Ralith commented on this pull request.

     SetLossDetectionTimer()
     return

Good thought; applied.

-- 
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/4848#discussion_r598377095

---

From: ianswett <[email-masked]>
Date: Sun, 21 Mar 2021 19:14:28 -0700

@ianswett commented on this pull request.

I think line 1493 should be "ack-eliciting" instead of "in-flight" as well?

> @@ -1480,8 +1480,8 @@ GetLossTimeAndSpace():
 GetPtoTimeAndSpace():
   duration = (smoothed_rtt + max(4 * rttvar, kGranularity))
       * (2 ^ pto_count)
-  // Arm PTO from now when there are no inflight packets.
-  if (no in-flight packets):
+  // Arm PTO from now when there are no ACK-eliciting packets.
+  if (no ack-eliciting packets in flight):

Ack-eliciting packets are defined, so I'd use that.

```suggestion
  if (no ack-eliciting 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/4848#pullrequestreview-617104764

---

From: Benjamin Saunders <[email-masked]>
Date: Sun, 21 Mar 2021 19:23:20 -0700

GetPtoTimeAndSpace():

I prefer this as-is, as it's explicit that we're talking about ACK-eliciting packets that have been transmitted but neither acknowledged nor declared lost. The exact wording is also already used elsewhere, e.g. line 1529.

I appreciate that "in flight" is being a bit overloaded here, so switching all three cases over to a different wording might be nice, but it's not immediately obvious to me what that might be.

-- 
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/4848#discussion_r598387931

---

From: Martin Thomson <[email-masked]>
Date: Sun, 21 Mar 2021 20:25:08 -0700

@martinthomson commented on this pull request.

 GetPtoTimeAndSpace():

If you want something positive the condition is "all ack-eliciting packets sent by this endpoint have either been acknowledged or marked lost".  So maybe "all ack-eliciting packets accounted for".

-- 
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/4848#discussion_r598402978

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 11:44:17 -0700

GetPtoTimeAndSpace():

"accounted for" seems less precise/precedented than "in flight" to me, at least.

-- 
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/4848#discussion_r598985342

---

From: ianswett <[email-masked]>
Date: Mon, 22 Mar 2021 11:53:10 -0700

GetPtoTimeAndSpace():

Thanks for clarifying why you wanted to state "in flight" as well as ack-eliciting.  I think the extra clarity is good.  However, now the comment doesn't quite match up.

I agree the "accounted for" is less precise than "in flight" in this case.

-- 
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/4848#discussion_r598991471

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 11:58:16 -0700

@Ralith pushed 1 commit.

4dd253aef55a05a895560de1486d1aba916e1dfb  Tweak comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/2bd9bb1d698ec16a0048b2a304a443533bd5442e..4dd253aef55a05a895560de1486d1aba916e1dfb

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 11:58:24 -0700

GetPtoTimeAndSpace():

Tweaked the comment to match.

-- 
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/4848#discussion_r598995168

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 12:00:32 -0700

@Ralith pushed 1 commit.

33c19b13a9ea9578e1a23b06aac1178cd2cc7b06  Tweak comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/4dd253aef55a05a895560de1486d1aba916e1dfb..33c19b13a9ea9578e1a23b06aac1178cd2cc7b06

---

From: ianswett <[email-masked]>
Date: Mon, 22 Mar 2021 12:56:52 -0700

@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/4848#pullrequestreview-617927457

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 17:12:17 -0700

@Ralith pushed 1 commit.

6942fde73ce2621412dcada96461f7539f4c8a87  Adjust packet space scan to match

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/33c19b13a9ea9578e1a23b06aac1178cd2cc7b06..6942fde73ce2621412dcada96461f7539f4c8a87

---

From: Benjamin Saunders <[email-masked]>
Date: Mon, 22 Mar 2021 17:12:41 -0700

> I think line 1493 should be "ack-eliciting" instead of "in-flight" as well?

Whoops, missed that comment somehow. I think that makes sense; added.

-- 
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/4848#issuecomment-804485229

---

From: ianswett <[email-masked]>
Date: Tue, 23 Mar 2021 11:33:50 -0700

Thanks for the extra update.

Now that I read this pseudocode, I believe there's a potential deadlock if the CWND is consumed by in flight packets that are not ack-eliciting(ie: PADDING only).

If I read correctly, the PTO will not be set.  In the past, we've said to be careful with padding-only packets for this reason, but if one had a very small CWND, it wouldn't be that difficult to get into this situation by losing a few packets at the wrong time.

-- 
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/4848#pullrequestreview-618940473

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 23 Mar 2021 12:25:12 -0700

I'm not seeing it. I don't see an anti-amplification deadlock, because in the case you describe, no ACK-eliciting packets are in flight, so the anti-deadlock PTO is set. Once `PeerCompletedAddressValidation()` is true, then I don't see why the PTO *should* be set because there's nothing to be judged lost, and no deadlock because the server can transmit freely.

-- 
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/4848#issuecomment-805170580

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Mar 2021 15:41:24 -0700

> +  // Arm PTO from now when there are no ACK-eliciting packets in
+  // flight.

```suggestion
  // Arm PTO when there are no ACK-eliciting packets in flight.
```

-- 
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/4848#pullrequestreview-619145472

---

From: ianswett <[email-masked]>
Date: Tue, 23 Mar 2021 15:52:04 -0700

+  // flight.

I like dropping a line, but I think the existing text is slightly clearer, since this is the only time we set PTO from now().

-- 
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/4848#discussion_r600011950

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 23 Mar 2021 16:36:10 -0700

@Ralith pushed 1 commit.

95208252bb3a820ecf45a49082e216d69999c546  Fix old typo

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/6942fde73ce2621412dcada96461f7539f4c8a87..95208252bb3a820ecf45a49082e216d69999c546

---

From: Martin Thomson <[email-masked]>
Date: Tue, 23 Mar 2021 16:38:34 -0700

+  // flight.

Sure, how about this then...

> // Arm PTO if no ACK-eliciting packets are in flight.

-- 
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/4848#discussion_r600033364

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 23 Mar 2021 16:39:30 -0700

+  // flight.

(Whoops, github didn't display your response at first...)

I actually found the original wording difficult to parse as a well-formed sentence. "From now when" is weird. We could rephrase it as "based on the current time" or something but that's getting wordy, and I don't feel like it's really adding anything over the psuedocode.

-- 
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/4848#discussion_r600033703

---

From: ianswett <[email-masked]>
Date: Tue, 23 Mar 2021 18:04:34 -0700

GetPtoTimeAndSpace():

This method doesn't actually arm the PTO, and the whole method is about PTO, so maybe it'd be best to remove the comment?

Alternatively, you could change it to:
"Base the PTO from now when no ACK eliciting packets are in flight to avoid it being set too early."

-- 
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/4848#discussion_r600077590

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 26 Mar 2021 16:56:11 -0700

@janaiyengar approved this pull request.

I think this is a fine change. Thanks, @Ralith !

-- 
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/4848#pullrequestreview-622593767

---

From: Benjamin Saunders <[email-masked]>
Date: Fri, 26 Mar 2021 18:13:56 -0700

@Ralith pushed 1 commit.

660d8d20dde6ca4f8ab45298a16cce64b1fb2f9e  Reword comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4848/files/95208252bb3a820ecf45a49082e216d69999c546..660d8d20dde6ca4f8ab45298a16cce64b1fb2f9e

---

From: Benjamin Saunders <[email-masked]>
Date: Fri, 26 Mar 2021 18:14:06 -0700

+  // flight.

Made another pass at the wording here.

-- 
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/4848#discussion_r602648311

---

From: ianswett <[email-masked]>
Date: Sat, 27 Mar 2021 06:10:28 -0700

Thanks, nice improvement.

-- 
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/4848#pullrequestreview-622648451

---

From: ianswett <[email-masked]>
Date: Tue, 30 Mar 2021 12:07:10 -0700

Merged #4848 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/4848#event-4529790342