Subject: [quicwg/base-drafts] Don't verify ECN counts for out of order ACKs (#2215)

From: ianswett <[email-masked]>
Date: Thu, 20 Dec 2018 07:10:40 -0800

Fixes #2198
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Don't verify ECN counts for out of order ACKs

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2215.patch
https://github.com/quicwg/base-drafts/pull/2215.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/2215

---

From: ianswett <[email-masked]>
Date: Thu, 20 Dec 2018 07:12:38 -0800

There's probably slightly better phrasing for the packet number clause, so feel free to suggest something.

-- 
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/2215#issuecomment-449030568

---

From: ianswett <[email-masked]>
Date: Thu, 20 Dec 2018 14:07:45 -0800

Closed #2215.

-- 
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/2215#event-2039515712

---

From: ianswett <[email-masked]>
Date: Thu, 20 Dec 2018 14:07:44 -0800

Jana's talked me through this and now I realize that this is broken in a larger sense.  As we've previously discussed, any time an acknowledgement is dropped, ECN validation MAY fail.

-- 
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/2215#issuecomment-449150906

---

From: ianswett <[email-masked]>
Date: Thu, 20 Dec 2018 14:10:44 -0800

Reopened #2215.

-- 
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/2215#event-2039520661

---

From: janaiyengar <[email-masked]>
Date: Thu, 20 Dec 2018 14:46:00 -0800

After more discussion, I agree that this does fix the problem. I think the text could be moved to a better location, but I'm fine with this for now.

-- 
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/2215#issuecomment-449160164

---

From: janaiyengar <[email-masked]>
Date: Thu, 20 Dec 2018 14:46:09 -0800

janaiyengar approved this pull request.

-- 
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/2215#pullrequestreview-187215314

---

From: MikkelFJ <[email-masked]>
Date: Thu, 20 Dec 2018 22:55:59 +0000 (UTC)

"Processed in different order than it was sent" is a bit vague. If there is reordering you don't know which frame is the one that is out of order. There needs to be a clear selection criteria such as not processing a frame in packet with a PN lower than a previously processed.

-- 
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/2215#issuecomment-449162263

---

From: janaiyengar <[email-masked]>
Date: Thu, 20 Dec 2018 15:15:40 -0800

@janaiyengar pushed 1 commit.

221567aaaee99d1db6d6c4ef83cebc52fa15d284  smithing

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2215/files/4152d7b55ad227206f03ad7793373d7dc75eb210..221567aaaee99d1db6d6c4ef83cebc52fa15d284

---

From: janaiyengar <[email-masked]>
Date: Thu, 20 Dec 2018 15:16:26 -0800

@ianswett: I've reworded and moved the text. See what you think.

-- 
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/2215#issuecomment-449168819

---

From: Martin Thomson <[email-masked]>
Date: Thu, 20 Dec 2018 17:47:37 -0800

martinthomson requested changes on this pull request.

I really don't like adding a hard coupling like this to the design.  I would be comfortable with a SHOULD NOT, explaining that if you don't do that, you might disable ECN unnecessarily in some rare cases.

> @@ -3065,6 +3064,10 @@ to be greater than the number of packets acknowledged in an ACK frame.  When
 this happens, and if verification succeeds, the local reference counts MUST be
 increased to match the counts in the ACK frame.
 
+Processing counts out of order can result in verification failure.  An endpoint
+MUST NOT perform this verification if the ACK frame is received in a packet with
+packet number lower than a previously received ACK frame.

With the proposed change, add:

> Verifying based on ACK frames that arrive out of order can result in disabling ECN unnecessarily.

> @@ -3065,6 +3064,10 @@ to be greater than the number of packets acknowledged in an ACK frame.  When
 this happens, and if verification succeeds, the local reference counts MUST be
 increased to match the counts in the ACK frame.
 
+Processing counts out of order can result in verification failure.  An endpoint
+MUST NOT perform this verification if the ACK frame is received in a packet with

```suggestion
SHOULD NOT perform this verification if the ACK frame is received in a packet with
```

-- 
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/2215#pullrequestreview-187248817

---

From: janaiyengar <[email-masked]>
Date: Fri, 21 Dec 2018 12:15:21 -0800

786d360adaff0368c666fe964225539982a1f10d  martin's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2215/files/221567aaaee99d1db6d6c4ef83cebc52fa15d284..786d360adaff0368c666fe964225539982a1f10d

---

From: janaiyengar <[email-masked]>
Date: Fri, 21 Dec 2018 12:15:47 -0800

@martinthomson : I've incorporated your comments.

-- 
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/2215#issuecomment-449488064

---

From: ianswett <[email-masked]>
Date: Fri, 21 Dec 2018 13:10:46 -0800

This still LGTM.

-- 
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/2215#issuecomment-449499224

---

From: Martin Thomson <[email-masked]>
Date: Sun, 23 Dec 2018 20:02:43 -0800

Merged #2215 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/2215#event-2042720771

---

From: Martin Thomson <[email-masked]>
Date: Sun, 23 Dec 2018 20:03:10 -0800

Thanks for considering my input.

-- 
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/2215#issuecomment-449683346