Re: [quicwg/base-drafts] Editorial fixes to ECN text (#2189)

[Martin Thomson <[e-mail]>]

martinthomson approved this pull request.

Changes LGTM, though I have a new failure mode for ECN to add.

Also, this will rot my new patch, but I can look after that.

> @@ -3031,33 +3031,28 @@ an ACK frame without ECN feedback, the endpoint stops setting ECT codepoints in
 subsequent IP packets, with the expectation that either the network path or the
 peer no longer supports ECN.
 
-To reduce the risk of non-standard compliant ECN markings affecting the
-operation of an endpoint, an endpoint verifies the counts it receives when it
-receives new acknowledgements:
+Network devices that corrupt or apply non-standard ECN markings might result in
+reduced throughput or other undesirable side-effects.  To reduce this risk, an
+endpoint uses the following steps to verify the counts it receives in an ACK
+frame.  Note that the counts MUST NOT be verified if the ACK frame does not

```suggestion
frame.  Counts MUST NOT be verified if the ACK frame does not
```

Now we don't say why this is any more.  Is that OK?

There's a perverse situation in which this results in another problem with this logic.

That situation is where the largest acknowledged is lower on packet that is sent later.  AND that packet acknowledges more packets so that the counts are higher.  AND that packet is reordered ahead of packets with higher largest acknowledged.  This results in the verification failing on the second packet to arrive (counts are lower because it's an older ACK, but the largest acknowledged is higher).

A fix is to never decrease the largest acknowledged when sending an ACK frame.  Another is to disable validation if the packet containing the ACK is not higher-numbered than the last packet containing an ACK.  The question is whether we care enough to recommend either fix...

@gloinul, WDYT?

-- 
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/2189#pullrequestreview-185423232