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

From: janaiyengar <[email-masked]>
Date: Fri, 14 Dec 2018 18:55:07 -0800

Follow-up to #2113, and some other consistency checks.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2189

-- Commit Summary --

  * Editorial fixes to ECN text

-- File Changes --

    M draft-ietf-quic-transport.md (67)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2189.patch
https://github.com/quicwg/base-drafts/pull/2189.diff

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

---

From: Martin Thomson <[email-masked]>
Date: Sun, 16 Dec 2018 18:41:25 -0800

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

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 17 Dec 2018 05:10:09 -0800

gloinul commented on this pull request.

To avoid all issues let us check so that the ACK frames are not reordered. For example rewrite this check to: "Counts MUST NOT be verified if the ACK frame arrived in an QUIC packet whose packet number is not the highest received so far." 

-* The increase in ECT(0) and ECT(1) counters MUST be at least the number of QUIC
-  packets newly acknowledged that were sent with the corresponding codepoint
-  minus the increase in the CE counter. This detects network remarking between
-  ECT(0) and ECT(1).
-
-* The total increase in ECT(0), ECT(1), and CE counters reported in the ACK
-  frame MUST be at least the total number of QUIC packets newly acknowledged in
-  this ACK frame. This detects if the network changes ECT(0), ECT(1) or CE to
+* The total increase in ECT(0), ECT(1), and CE counts MUST be no smaller than
+  the total number of QUIC packets newly acknowledged in this ACK frame.  This

I suggest that you add the below (new after "frame").
"... in this ACK frame that was marked as ECT."
The reasons is that if a sender for some reason don't mark all packets, which is not required they need to check what was sent. 

-- 
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-185578251

---

From: Martin Thomson <[email-masked]>
Date: Mon, 17 Dec 2018 20:02:39 -0800

martinthomson commented on this pull request.

Creating any coupling between frames and packets will be annoying.  We've resisted attempts to do that in the past.

There are two conditions necessary: first that counters never decrease when put in packets with higher numbers, second: that no check is performed if the packet number is lower.  I don't think that we have the first condition either (though it is strongly implied, we don't insist upon it for other frame types).  

-- 
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#discussion_r242400838

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 14:08:39 -0800

janaiyengar commented on this pull request.

We can add a requirement that a receiver MUST NOT decrease the largest acked. I cannot see a reason (let alone a good one) why a receiver would do this. QUIC does not allow reneging of acked data, so decreasing the largest acked is strictly bad. That resolves this issue, right?

-- 
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#discussion_r242724075

---

From: ianswett <[email-masked]>
Date: Tue, 18 Dec 2018 22:13:17 +0000 (UTC)

ianswett commented on this pull request.

Hmm, this seems like a strong requirement we should think through a bit more.  At some point, I was thinking some very constrained implementations might just ACK the N most recent packets, and with reordering, that could cause largest acked to decrease.  Maybe that's not a compelling use case, but we should be sure we're ok with never decreasing 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/pull/2189#discussion_r242725472

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 14:13:21 -0800

I'd like to do this in a separate PR. This PR is editorial. While adding a requirement on largest acked shouldn't surprise anyone, it's probably best to make it a separate thing.

-- 
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#discussion_r242725495

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 14:13:47 -0800

@janaiyengar pushed 1 commit.

061cba3809316d79735e552e0d0364d0275530df  comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2189/files/c96dda790322376f29f72b7e70320565d7b80059..061cba3809316d79735e552e0d0364d0275530df

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 22:18:05 +0000 (UTC)

I've addressed the comments, and am merging. I'm happy to address the issue raised about largest acked in a separate issue (#2205).

-- 
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#issuecomment-448392434

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 22:18:12 +0000 (UTC)

Merged #2189 into master.

-- 
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#event-2034254804