Subject: [quicwg/base-drafts] Loss recovery pseudocode can erroneously set PTO in the past (#4846)

From: Benjamin Saunders <[email-masked]>
Date: Fri, 19 Mar 2021 12:46:04 -0700

If the server's first flight is interrupted by anti-amplification, the client's second flight may consist only of ACKs in the Initial and Handshake spaces. At this point, Initial state is discarded on the client, and no ack-eliciting data has been sent in the Handshake space. `GetPtoTimeAndSpace` dictates that the PTO be set to `time_of_last_ack_eliciting_packet[space] + duration`, but `time_of_last_ack_eliciting_packet` still has its initial value of 0. The desired behavior here is to schedule an anti-amplification deadlock prevention probe in the future, so the logic used when there are no in-flight packets should probably apply here as well. This could be accomplished by special-casing the behavior when `time_of_last_ack_eliciting_packet[space] == 0` to use `now()` instead, or perhaps by adjusting the global `if (no in-flight packets):` test to `if (no ack-eliciting packets):` if that wouldn't have other undesired effects.

-- 
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/issues/4846

---

From: Benjamin Saunders <[email-masked]>
Date: Fri, 19 Mar 2021 17:15:44 -0700

Similarly, `OnLossDetectionTimeout` also uses `bytes_in_flight > 0` to judge whether a PTO is a real loss probe or an anti-deadlock packet, but this is incorrect in the case illustrated above.

-- 
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/issues/4846#issuecomment-803203390

---

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

If I'm understanding correctly, the key issue is that the pseudocode is not robust to cases when Handshake packets are padded, which allows bytes_in_flight to be > 0, but no ack-eliciting packets are in-flight AND the server may still be blocked by the amplification limit?

I'll have to think about this more, but if you want to propose a PR, that would be helpful.

-- 
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/issues/4846#issuecomment-803665224

---

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

Yeah, exactly. PR drafted based on the fix we've applied in Quinn.

-- 
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/issues/4846#issuecomment-803667973

---

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

Closed #4846 via #4848.

-- 
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/issues/4846#event-4529790354