Subject: [quicwg/base-drafts] Split PTO calculation into a dedicated method (#3681)

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 09:00:49 -0700

As @martinthomson pointed out in #3666, it was far too complex and error-prone as-is.

Fixes #3564 and #3674 by calculating the PTO timeout for each PN space and picking the earliest, as well as checking that there are inflight packets before calculating PTO for a PN space.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Split PTO calculation into a dedicated method

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 09:03:29 -0700

@ianswett pushed 1 commit.

d0050a08a17463fbf9f3bf25b61b50a982b4a07f  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/3681/files/290a5113d74c36b112d3ada80edc2f3ede23325b..d0050a08a17463fbf9f3bf25b61b50a982b4a07f

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 09:07:50 -0700

65b036d2c0437e81e0533a76e863bebfaa3207a3  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/3681/files/d0050a08a17463fbf9f3bf25b61b50a982b4a07f..65b036d2c0437e81e0533a76e863bebfaa3207a3

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 09:17:54 -0700

@ianswett commented on this pull request.

>    space = Initial
   for pn_space in [ Handshake, ApplicationData ]:
-    if (times[pn_space] != 0 &&
-        (time == 0 || times[pn_space] < time) &&
-        # Skip ApplicationData until handshake completion.
-        (pn_space != ApplicationData ||
-          IsHandshakeComplete()):
-      time = times[pn_space];
+    if (time == 0 || loss_time[pn_space] < time):
+      time = loss_times[pn_space];

```suggestion
      time = loss_time[pn_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/3681#pullrequestreview-415501201

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 09:17:59 -0700

2630bfdfe743909fb7bd3a92a41bd5f794cc503d  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/3681/files/65b036d2c0437e81e0533a76e863bebfaa3207a3..2630bfdfe743909fb7bd3a92a41bd5f794cc503d

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 10:20:28 -0700

83727ca4f10a4d3866ea026efff3cb833111d1f8  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/3681/files/2630bfdfe743909fb7bd3a92a41bd5f794cc503d..83727ca4f10a4d3866ea026efff3cb833111d1f8

---

From: ianswett <[email-masked]>
Date: Wed, 20 May 2020 12:21:49 -0700

f10875ed05f2edd3fe121631bf89c4a877204fb5  Fix errors

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3681/files/83727ca4f10a4d3866ea026efff3cb833111d1f8..f10875ed05f2edd3fe121631bf89c4a877204fb5

---

From: Martin Thomson <[email-masked]>
Date: Wed, 20 May 2020 17:23:56 -0700

@martinthomson commented on this pull request.

Yeah, this is much clearer.

>        space = pn_space
   return time, space
 
+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):
+    return now() + duration, Initial

```suggestion
    return (now() + duration), Initial
```

> +    if (timeout >
+          time_of_last_ack_eliciting_packet[space] + duration):
+      timeout =
+          time_of_last_ack_eliciting_packet[space] + duration

```suggestion
    t = time_of_last_ack_eliciting_packet[space] + duration
    if (t < timeout):
      timeout = t
```

>        space = pn_space
   return time, space
 
+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):
+    return now() + duration, Initial
+  timeout = infinite
+  pn_space = Initial

using space and pn_space is awkward, how about naming this pto_space (and maybe then pto_time for timeout)

-- 
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/3681#pullrequestreview-415799805

---

From: Martin Thomson <[email-masked]>
Date: Wed, 20 May 2020 17:25:26 -0700

FWIW, the problem with exponentially increasing max_ack_delay thing is pretty obvious here.  I think that I might choose not to implement that bit :)

-- 
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/3681#issuecomment-631806905

---

From: ianswett <[email-masked]>
Date: Thu, 21 May 2020 07:47:52 -0700

242e38d3b7e6170b59b347de6d8b3686af356c11  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/3681/files/f10875ed05f2edd3fe121631bf89c4a877204fb5..242e38d3b7e6170b59b347de6d8b3686af356c11

---

From: ianswett <[email-masked]>
Date: Thu, 21 May 2020 07:50:28 -0700

7a330219f1b116f3bc1ddbcf8cb8fd8fdd7355b8  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/3681/files/242e38d3b7e6170b59b347de6d8b3686af356c11..7a330219f1b116f3bc1ddbcf8cb8fd8fdd7355b8

---

From: ianswett <[email-masked]>
Date: Thu, 21 May 2020 07:50:46 -0700

return time, space
 
+GetPtoTimeAndSpace():

Sg, changed to pto_timeout and pto_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/3681#discussion_r428700548

---

From: Martin Thomson <[email-masked]>
Date: Tue, 26 May 2020 15:55:31 -0700

@martinthomson 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/3681#pullrequestreview-418733134

---

From: Marten Seemann <[email-masked]>
Date: Tue, 26 May 2020 22:58:22 -0700

@marten-seemann requested changes on this pull request.

   return time, space
 
+GetPtoTimeAndSpace():

I believe you're missing a pair of brackets here.

   return time, space
 
+GetPtoTimeAndSpace():
+    return (now() + duration), Initial

This breaks the following case: As a client, I need to send a Handshake packet to unblock the server's amplification limit, if the Initial packet number space was already dropped.

-- 
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/3681#pullrequestreview-418844024

---

From: ianswett <[email-masked]>
Date: Wed, 27 May 2020 07:36:31 -0700

return time, space
 
+GetPtoTimeAndSpace():

Good catch, thanks.

-- 
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/3681#discussion_r431186068

---

From: ianswett <[email-masked]>
Date: Wed, 27 May 2020 07:38:24 -0700

return time, space
 
+GetPtoTimeAndSpace():

Thanks.  The amplification factor is the gift that keeps on giving.

-- 
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/3681#discussion_r431187955

---

From: ianswett <[email-masked]>
Date: Wed, 27 May 2020 07:39:50 -0700

6ce4f9b1841f8f2296e7420b08a928a41f7f2a5a  Marten's suggestions

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3681/files/7a330219f1b116f3bc1ddbcf8cb8fd8fdd7355b8..6ce4f9b1841f8f2296e7420b08a928a41f7f2a5a

---

From: Marten Seemann <[email-masked]>
Date: Wed, 27 May 2020 17:36:12 -0700

@marten-seemann 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/3681#pullrequestreview-419701561

---

From: ianswett <[email-masked]>
Date: Thu, 28 May 2020 07:32:50 -0700

820497e882f930dcdc5d3ae7401add631ec0ea38  Merge branch 'master' into ianswett-split-pto-pseudocode

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3681/files/6ce4f9b1841f8f2296e7420b08a928a41f7f2a5a..820497e882f930dcdc5d3ae7401add631ec0ea38

---

From: martinduke <[email-masked]>
Date: Thu, 28 May 2020 16:26:32 -0700

@martinduke approved this pull request.

LGTM

-- 
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/3681#pullrequestreview-420558166

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 29 May 2020 18:29:35 -0700

@janaiyengar commented on this pull request.

This is much cleaner -- thanks for working on it! A few comments.

+  if (no in-flight packets):
+    if (has handshake keys):
+      return (now() + duration), Handshake
+    else:
+      return (now() + duration), Initial
+  pto_timeout = infinite
+  pto_space = Initial
+  for space in [ Initial, Handshake, ApplicationData ]:
+    if (no in-flight packets in space):
+        continue;
+    if space == ApplicationData:
+      // Skip ApplicationData until handshake complete.
+      if (handshake is not complete):
+        return pto_timeout, pto_space
+      // ApplicationData include the max_ack_delay in PTO.

```suggestion
      // Include max_ack_delay and backoff for ApplicationData.
```

   return time, space
 
+GetPtoTimeAndSpace():
+  duration = (smoothed_rtt + max(4 * rttvar, kGranularity))
+      * (2 ^ pto_count)
+  # Arm PTO from now when there are no inflight packets

```suggestion
  // Arm PTO from now when there are no inflight packets.
```

   return time, space
 
+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):

Assuming this is for the amplification protection, something's missing here. Perhaps this should be 
`if (is_client && no packets in flight && !PeerCompletedAddressValidation())`.
Otherwise, you never get into the case below on line 1293.

   return time, space
 
+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):
+    if (has handshake keys):
+      return (now() + duration), Handshake
+    else:
+      return (now() + duration), Initial
+  pto_timeout = infinite
+  pto_space = Initial
+  for space in [ Initial, Handshake, ApplicationData ]:
+    if (no in-flight packets in space):
+        continue;
+    if space == ApplicationData:

```suggestion
    if (space == ApplicationData):
```

-- 
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/3681#pullrequestreview-421361725

---

From: ianswett <[email-masked]>
Date: Sat, 30 May 2020 13:58:39 -0700

829501299ef329a19ab52f9f0702a85e144c18a7  Add an assert

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3681/files/820497e882f930dcdc5d3ae7401add631ec0ea38..829501299ef329a19ab52f9f0702a85e144c18a7

---

From: ianswett <[email-masked]>
Date: Sat, 30 May 2020 13:59:51 -0700

92c2e22eaa7bce92d055dba4bf72ecfbc505e5dc  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/3681/files/829501299ef329a19ab52f9f0702a85e144c18a7..92c2e22eaa7bce92d055dba4bf72ecfbc505e5dc

---

From: ianswett <[email-masked]>
Date: Sat, 30 May 2020 14:04:15 -0700

return time, space
 
+GetPtoTimeAndSpace():

I added an assert that for !PeerCompletedAddressValidation().

I think the condition you're mentioning is:
"if (no in-flight packets in space):
    continue;"

That's per PN-space, not for the connection as a whole.

-- 
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/3681#discussion_r432886719

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 30 May 2020 19:11:44 -0700

return time, space
 
+GetPtoTimeAndSpace():

never mind, I thought the code below to do the early return when no packets are in flight after address validation was removed. It's still there, so all good.

-- 
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/3681#discussion_r432902747

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 30 May 2020 19:11:51 -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/3681#pullrequestreview-421455918

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 30 May 2020 19:12:02 -0700

Merged #3681 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/3681#event-3390218016