Re: [quicwg/base-drafts] Gorry's ECN rewrite (#4059)

[ianswett <[e-mail]>]

@ianswett requested changes on this pull request.

Some suggestions, some of which I don't have suggestions for.

>  
 
 ### ECN Counts
 
-On receiving a QUIC packet with an ECT or CE codepoint, an ECN-enabled endpoint
-that can access the ECN codepoints from the enclosing IP packet increases the
-corresponding ECT(0), ECT(1), or CE count, and includes these counts in
-subsequent ACK frames; see {{generating-acks}} and {{frame-ack}}.
-
-An IP packet that results in no QUIC packets being processed does not increase
-ECN counts.  A QUIC packet detected by a receiver as a duplicate does not
-affect the receiver's local ECN codepoint counts; see {{security-ecn}} for
-relevant security concerns.
+Use of ECN requires the receiving endpoint to read the ECN codepoint from an IP

nit: There's sending ECN and receiving ECN.  I believe there are platforms where receiving is possible, but sending is not.   As such, I'm unclear if "Use of ECN" means a sender negotiating it and receiving the feedback in ACK frames or something else?  Possible suggestion below.

```suggestion
In order for the sender to use ECN in its congestion controller, the sender needs to be able to set the ECN codepoint in the IP header and the receiver needs to be able to to read the ECN codepoint from an IP
```


>  
-Coalesced packets (see {{packet-coalesce}}) mean that several packets can share
-the same IP header.  The ECN counts for the ECN codepoint received in the
-associated IP header are incremented once for each QUIC packet, not per
-enclosing IP packet or UDP datagram.
+On receiving a QUIC packet with an ECT(0), ECT(1) or CE codepoint, an

nit: QUIC packets don't have ECT codepoints, IP packets do.

```suggestion
On receiving an IP packet with an ECT(0), ECT(1) or CE codepoint, an
```

>  
-Coalesced packets (see {{packet-coalesce}}) mean that several packets can share
-the same IP header.  The ECN counts for the ECN codepoint received in the
-associated IP header are incremented once for each QUIC packet, not per
-enclosing IP packet or UDP datagram.
+On receiving a QUIC packet with an ECT(0), ECT(1) or CE codepoint, an
+ECN-enabled endpoint accesses the ECN codepoint from the enclosing IP packet and

```suggestion
ECN-enabled endpoint accesses the ECN codepoint and
```

> +ECN counts.  Coalesced QUIC packets (see {{packet-coalesce}}) share the same IP
+header.  The ECN counts for the ECN codepoint received in the associated IP
+header are incremented once for each QUIC packet.

I find this structure a bit awkward, so suggestion below.

```suggestion
ECN counts.  Coalesced QUIC packets (see {{packet-coalesce}}) share the same IP
header so the ECN counts are incremented once for each QUIC packet.
```

>  
-### ECN Validation {#ecn-validation}
+For example, if one each of an Initial, 0-RTT, Handshake, and 1-RTT QUIC packet

This is a bad example as there's no practical use case where 0-RTT would be coalesced with 1-RTT as one would never have both keys at once.  I understand the point of the example, but I think it's important to keep to practical examples.

```suggestion
For example, if one each of an Initial, Handshake, and 1-RTT QUIC packet
```

>  
-It is possible for faulty network devices to corrupt or erroneously drop packets
-with ECN markings.  To provide robust connectivity in the presence of such
-devices, each endpoint independently validates ECN counts and disables ECN if
-the path is not showing consistent support for ECN.
+An ECN count is not incremented when no QUIC packets from the received IP

```suggestion
ECN counts are only incremented when QUIC packets from the received IP
```

> +packet are processed. A QUIC packet detected by a receiver as a duplicate also
+does not increase an ECN count; see {{security-ecn}} for relevant security

You don't process duplicate packets, so suggestion below.

```suggestion
packet are processed. As such, duplicate QUIC packets are not processed and
do not increase ECN counts; see {{security-ecn}} for relevant security
```

>  
-* If all packets that were sent with the ECT(0) codepoint are eventually deemed
-  lost (Section 6 of {{QUIC-RECOVERY}}), validation is deemed to have failed.
+If an endpoint has cause to expect that IP packets with an ECT codepoint might

Setting ECN on 10 packets if you think they're going to be lost seems odd and I can't imagine anyone would do that.  This text is such a departure from the existing text, I'm unsure how to fix it.

> +An endpoint thus attempts to use ECN and validates this for each new connection,
+when switching to a server's preferred address, and on active connection
+migration to a new path.  {{ecn-alg}} describes one possible algorithm.

```suggestion
An endpoint negotiates ECN for each new connection or new path.
{{ecn-alg}} describes one possible algorithm.
```

>  
-ECN validation fails if the sum of the increase in ECT(0) and ECN-CE counts is
-less than the number of newly acknowledged packets that were originally sent
+ECN validation also fails if the sum of the increase in ECT(0) and ECN-CE counts

undo

```suggestion
ECN validation also if the sum of the increase in ECT(0) and ECN-CE counts
```

>  with an ECT(0) marking.  Similarly, ECN validation fails if the sum of the
 increases to ECT(1) and ECN-CE counts is less than the number of newly
 acknowledged packets sent with an ECT(1) marking.  These checks can detect
-removal of ECN markings in the network.
+remarking of ECN-CE markings by the network.

remarking and removal are completely different and I believe removal is correct.  I'm unsure what's meant by remarking.

```suggestion
removal of ECN-CE markings by the network.
```

> +an ACK frame. This is why ECN counts are permitted to be larger than the value
+corresponding to the largest acknowledged packet number.

This is unfortunately incorrect.  One could in theory start with a packet number of 1 million and it should not affect correctness.

```suggestion
an ACK frame. This is why ECN counts are permitted to be larger than the total
number of acknowledged packets within a packet number space.
```

>  
-Processing ECN counts out of order can result in validation failure.  An
-endpoint SHOULD skip ECN validation on an ACK frame that does not increase the
-largest acknowledged packet number.
+ECN validation can fail if the received total count for either ECT(0) or ECT(1)

Can fail?  Isn't it required to fail?

>  
-Processing ECN counts out of order can result in validation failure.  An
-endpoint SHOULD skip ECN validation on an ACK frame that does not increase the
-largest acknowledged packet number.
+ECN validation can fail if the received total count for either ECT(0) or ECT(1)
+exceeds the total number of packets sent with each corresponding ECT codepoint.
+In particular, validation will fail when an endpoint receives a non-zero ECN
+count corresponding to an ECT codepoint that it never applied.  This check
+detects when packets are remarked to ECT(0) or ECT(1) in the network.

```suggestion
detects when packets are marked ECT(0) or ECT(1) by the network.
```

-- 
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/4059#pullrequestreview-485432064