Subject: [quicwg/base-drafts] Editorial minor issues in receovery-31 found in AD review. (#4196)

From: Magnus Westerlund <[email-masked]>
Date: Mon, 12 Oct 2020 09:31:19 -0700

This issue will list a number of minor editorial things that should be checked and likely addressed with some editorial fixes:

1. Section 5.3: "These delays are computed using the ACK Delay field of the ACK frame as described  in Section 19.3 of [QUIC-TRANSPORT]." This text implies that the computation of the delay is described in Section 19.3 of QUIC-TRANSPORT. However, I don't think it is described the calculation, only the definition of the field. Please reformulate to imply only field definition not calculation. 
2. Section 5.3: "On the first RTT sample for the network path, that sample is used as rtt_sample. This ensures that the first measurement erases the history of any persisted or default values." I think this intended to cover cases where path changes are detected. But it is not well expressed. Does this need a reformulation and a reference into QUIC-TRANSPORT where it states when such a reset should occur?
3. Section 6.1: "Either its packet number is kPacketThreshold smaller than an acknowledged packet" Nitpicking, assuming no PN gaps in transmission. Does that need a note?
4. Section 6.2.4: "An endpoint MAY send up to two full-sized datagrams containing ack-eliciting packets, to avoid an expensive consecutive PTO expiration due to a single lost datagram or transmit data from multiple packet number spaces." This last part, I assume intended to generate additional datagrams to prevent single loss to impact. However, due to coalescing this is unclear in this context.
5. Section 7: "Endpoints can unilaterally choose a different algorithm to use, such as Cubic ([RFC8312])." I think this sentence should be explicit that in needs to be sender-based algorithms. I think we need to have the requirement on performing congestion control on the sender unless there has been explicit agreement with the receiver that they will do it. 
6. Section 7.2: "Endpoints SHOULD use an initial congestion window of 10 times the maximum datagram size (max_datagram_size), limited to the larger of 14720 bytes or twice the maximum datagram size." "maximum datagram size" is not used in Transport there need to be alignment here of what name to use. Transport has Maximum Packet Size, which appears to match the definition in the appendix of max_datagram_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/4196

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 14:25:18 -0700

Thanks for your careful review, @gloinul -- these are good finds!  I've addressed your comments below and in #4213. I'll address point 6 in a separate PR.

1. Section 19.3 has the following text, which does tell how to compute acknowledgement delay from the ACK Delay field:
```
ACK Delay:
    A variable-length integer encoding the acknowledgement delay in microseconds; see Section 13.2.5. 
    It is decoded by multiplying the value in the field by 2 to the power of the ack_delay_exponent transport
    parameter sent by the sender of the ACK frame; see Section 18.2. Compared to simply expressing the
    delay as an integer, this encoding allows for a larger range of values within the same number of bytes,
    at the cost of lower resolution.
```
I've replaced "computed" with "decoded" in the recovery document to make it clearer.

4. Coalescing applies to packets, not to datagrams. We've explicitly stated that a sender could send 2 datagrams to avoid any effects of coalescing. I think the text is pretty clear on this: "An endpoint MAY send up to two full-sized datagrams containing ack-eliciting packets". 

5. I've added "sender-side" to make the controller more explicit. To your point about making this a sender requirement, that is covered in the transport doc (end of [Section 13.3](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-13.3-7)):
```
Upon detecting losses, a sender MUST take appropriate congestion control action.
The details of loss detection and congestion control are described in [QUIC-RECOVERY].
```

-- 
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/4196#issuecomment-708668433

---

From: Lars Eggert <[email-masked]>
Date: Thu, 15 Oct 2020 04:25:24 -0700

Based on #4213 and #4214, labeling this as "editorial"

-- 
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/4196#issuecomment-709217744

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 15 Oct 2020 05:31:09 -0700

I think these issues are resolved once the two PRs are completely ready. 

-- 
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/4196#issuecomment-709283826

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 15 Oct 2020 12:56:52 -0700

Closed #4196 via #4213.

-- 
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/4196#event-3883332135