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

[Jana Iyengar <[e-mail]>]

@janaiyengar commented on this pull request.

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

> +  # 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:
+      // 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.
```

>        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

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

>        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):

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.

>        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):
+    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