Hi Ben, Thanks for the review. I've created GitHub issues for each of these, see the links inline.
On Thu, Jan 7, 2021 at 6:17 AM Benjamin Kaduk via Datatracker < [email protected]> wrote: > Benjamin Kaduk has entered the following ballot position for > draft-ietf-quic-recovery-33: No Objection > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html > for more information about IESG DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-quic-recovery/ > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Another well-written QUIC document; thank you! > (The last version of -recovery I read was quite some time ago, perhaps the > -09, and left me scratching my head quite often. This one is quite clear.) > > I put my usual editorial suggestions up at > https://github.com/quicwg/base-drafts/pull/4685 > > Section 5.2 > > > Endpoints SHOULD set the min_rtt to the newest RTT sample after > > persistent congestion is established. [...] > > Earlier we said that min_rtt was the minimum value observed "over the > lifetime of the path", which seems in conflict with this behavior unless > we somehow declare that the path is re-established after a persistent > congestion event. Perhaps we could finesse the wording a bit in the > former location (given that we go on to allow re-establishing (or > refreshing) the min_rtt at other times as well)? (Appendix A.3 might be > affected as well, if changes are made.) > https://github.com/quicwg/base-drafts/issues/4708 > Section 5.3 > > > After the handshake is confirmed, any acknowledgement delays reported > > by the peer that are greater than the peer's max_ack_delay are > > attributed to unintentional but potentially repeating delays, such as > > scheduler latency at the peer or loss of previous acknowledgements. > > I don't think I understand how loss of previous acknowledgements could > induce a peer to delay sending an ack longer than its max_ack_delay > after an ack-eliciting packet. > https://github.com/quicwg/base-drafts/issues/4709 > Section 6.2.1 > > > A sender SHOULD restart its PTO timer every time an ack-eliciting > > packet is sent or acknowledged, when the handshake is confirmed > > (Section 4.1.2 of [QUIC-TLS]), or when Initial or Handshake keys are > > discarded (Section 4.9 of [QUIC-TLS]). [...] > > If I understand correctly, "handshake is confirmed" and "handshake keys > are discarded" occur simultaneously, thus it is redundant to list both. > https://github.com/quicwg/base-drafts/issues/4710 > Section 6.2.3 > > > Endpoints > > that do not cease retransmitting packets in response to > > unauthenticated data risk creating an infinite exchange of packets. > > We may want to be more precise about what "unauthenticated data" means > here, since in -transport we say that even Initial packets are > "authenticated" in some sense. > https://github.com/quicwg/base-drafts/issues/4711 > Section 6.2.4 > > > > When a PTO timer expires, a sender MUST send at least one ack- > > eliciting packet in the packet number space as a probe. [...] > > [...] > > When the PTO timer expires, an ack-eliciting packet MUST be sent. [...] > > These look redundant to me (which, of course, does not necessarily mean > that there is no value in having both). > https://github.com/quicwg/base-drafts/issues/4712 > Section 7.7 > > > Endpoints can implement pacing as they choose. A perfectly paced > > sender spreads packets exactly evenly over time. For a window-based > > congestion controller, such as the one in this document, that rate > > can be computed by averaging the congestion window over the round- > > trip time. Expressed as a rate in bytes: > > I cringed a little when I saw a rate being expressed as a unit > expression with no time denominator, but am not sure how it might be > improved. The best I can come up with is "bytes/time". > https://github.com/quicwg/base-drafts/issues/4713 > Section 8.1 > > I guess in some sense the observed RTT is also a signal under the > control of a (Dolev-Yao) attacker, in addition to the loss and ECN > signals. > https://github.com/quicwg/base-drafts/issues/4714 > Appendix A > > Should we say explicitly that '^' in the pseudocode is used to indicate > exponentiation? > https://github.com/quicwg/base-drafts/issues/4715 Cheers, Lucas On behalf of QUIC WG Chairs
