Subject: [quicwg/base-drafts] Minor fixes for the recovery draft (#3813)

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 01 Jul 2020 22:21:25 -0700

- In A.8 `#` should be `//`.
- Both "in-flight" and "in flight" are used. It's better to uniform them to "in-flight".
- The same for "count toward" and "count towards".
- The packet field `ack_eliciting` is not referred. It should be removed.
- The packet field `sent_bytes` is defined but the field name actually used is `size`. `.size` should be `.sent_bytes`.
- In Sec 6.2.3 "For example, clients can coalesce an Initial packet containing PING and PADDING frames with a 0-RTT data packet and a server can coalesce an Initial packet containing a PING frame with one or more packets in its first flight".  Singular vs plural: client*s* and *a* server. "Clients" should be "a client".
- Should `max_datagram_size` be `max_udp_payload_size` for clarity?
- In A.6, I think that `OnDatagramReceived` should be `OnPacketReceived` since `OnPacketSent` is used in A.5.

-- 
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/3813

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 01 Jul 2020 22:30:14 -0700

- In A.7, `// DetectNewlyAckedPackets finds packets` should be `// DetectAndRemoveAckedPackets finds packets`. Or this comment is not necessary at all.

-- 
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/3813#issuecomment-652791694

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 01 Jul 2020 22:34:51 -0700

> - The packet field `ack_eliciting` is not referred. It should be removed.

Rather than removing it, I guess it's better to define `IncludesAckEliciting` explicitly and refer `.ack_eliciting` in 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/issues/3813#issuecomment-652793205

---

From: Kazu Yamamoto <[email-masked]>
Date: Wed, 01 Jul 2020 22:43:18 -0700

> - In A.7, `// DetectNewlyAckedPackets finds packets` should be `// DetectAndRemoveAckedPackets finds packets`. Or this comment is not necessary at all.

I cannot find the definition of `DetectAndRemoveAckedPackets`. (I was confused with `DetectAndRemoveLostPackets`.)

-- 
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/3813#issuecomment-652796033

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 02 Jul 2020 02:27:16 -0700

In B.5, `acked_packet` of `acked_packet.size` is used only here. It should probably be `packet.size`.

-- 
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/3813#issuecomment-652897057

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 02 Jul 2020 20:34:35 -0700

In A.7, `now -` should be `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/issues/3813#issuecomment-653323010

---

From: Kazu Yamamoto <[email-masked]>
Date: Thu, 02 Jul 2020 22:11:55 -0700

In A.7:

```
latest_rtt = now - sent_packets[pn_space][ack.largest_acked].time_sent
```

is also strange. `sent_packets[pn_space][ack.largest_acked]` is not available here since it was removed by `DetectAndRemoveAckedPackets`.

-- 
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/3813#issuecomment-653352251

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 03 Jul 2020 00:09:06 -0700

`kInitialRtt` is not used anywhere. Should `initial_rtt` in A.4 be `kInitialRtt`?

-- 
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/3813#issuecomment-653390278

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 03 Jul 2020 01:13:29 -0700

B.8: "Invoked from `DetectLostPackets` when packets are deemed lost."

But `OnPacketsLost` is called from `OnLossDetectionTimeout` and `DetectLostPackets` is not defined.

-- 
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/3813#issuecomment-653416274

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 03 Jul 2020 22:57:43 -0700

A.9: `bytes_in_flight`, which is defined in B, is used.

-- 
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/3813#issuecomment-653725932

---

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 05 Jul 2020 20:38:39 -0700

The `return` in `for` in `GetPtoTimeAndSpace` suggests me that the semantics of `return` in `for` returns a value form the function. In `OnPacketsAcked`, `return` is also used in `for`. But in this case, it seems to me that the semantics of this `return` goes to the next item.

-- 
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/3813#issuecomment-653998174

---

From: Kazu Yamamoto <[email-masked]>
Date: Sun, 05 Jul 2020 20:40:08 -0700

In A, `for` is not followed by `(`. But on some places in B, `for` is followed by `(`.

-- 
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/3813#issuecomment-653998488

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 06 Jul 2020 18:46:46 -0700

@janaiyengar I would like to see fixed pseudo code so that I can put my implementation work forward.

-- 
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/3813#issuecomment-654550119

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 06 Jul 2020 23:40:11 -0700

I would like to know how to implement `AreAllPacketsLost` and `DetectLostPackets`.

-- 
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/3813#issuecomment-654634614

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 07 Jul 2020 22:18:36 -0700

> I would like to know how to implement `AreAllPacketsLost` and `DetectLostPackets`.

`DetectLostPackets` was fixed via 2961b77dc1742f337343134bf455405688eb47c3.

-- 
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/3813#issuecomment-655291515

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 07 Jul 2020 22:42:04 -0700

@ianswett Some inconsistencies would be added via #3540.

-- 
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/3813#issuecomment-655298973

---

From: Kazu Yamamoto <[email-masked]>
Date: Fri, 10 Jul 2020 01:10:24 -0700

`max_ack_delay` is initialized to 0 but not updated anywhere.

-- 
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/3813#issuecomment-656546828

---

From: Kazu Yamamoto <[email-masked]>
Date: Mon, 13 Jul 2020 19:27:35 -0700

> `max_ack_delay` is initialized to 0 but not updated anywhere.

- [ ] I would like to have a comment that `max_ack_delay` is set when TP is exchanged.

- [ ] `max_ack_delay` is used in three places in the pseudo code. `InPersistentCongestion` uses non-0 value for Initial and Handshake. Other two places looks 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/issues/3813#issuecomment-657928497