Subject: [quicwg/base-drafts] ACK of non-existent packet is illegal (#2302)

From: Martin Thomson <[email-masked]>
Date: Sun, 06 Jan 2019 15:56:14 -0800

This is stacked on top of #2197.  So it looks worse than it really is.  Review [the last diff](https://github.com/quicwg/base-drafts/commit/69378cd3cba339a9584052323e4b2cf03314ddca).

Closes #2298.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Reorganize ACK section
  * Port #2245
  * Stop using 'fields' for ACK Range and Gap, tweak structure a little
  * Merge branch 'master' into shuffle-ack
  * ACK of non-existent packet is illegal

-- File Changes --

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

-- Patch Links --

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

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 16:29:13 -0800

kazuho commented on this pull request.

>  acknowledgment of its ACK frames, with the knowledge this could cause the sender
 to unnecessarily retransmit some data.  Standard QUIC {{QUIC-RECOVERY}}
 algorithms declare packets lost after sufficiently newer packets are
 acknowledged.  Therefore, the receiver SHOULD repeatedly acknowledge newly
 received packets in preference to packets received in the past.
 
+An endpoint SHOULD treat receipt an acknowledgment for a packet it did not send

receipt "of"?

-- 
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/2302#pullrequestreview-189652652

---

From: Martin Thomson <[email-masked]>
Date: Sun, 06 Jan 2019 16:30:39 -0800

@martinthomson pushed 1 commit.

d872c09a8ef35f027dfbda024407bd73cd0d0371  missing

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2302/files/69378cd3cba339a9584052323e4b2cf03314ddca..d872c09a8ef35f027dfbda024407bd73cd0d0371

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 06 Jan 2019 16:55:42 -0800

kazuho 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/2302#pullrequestreview-189654112

---

From: ianswett <[email-masked]>
Date: Sun, 06 Jan 2019 20:00:52 -0800

ianswett commented on this pull request.

Some suggestions

> @@ -2856,24 +2856,30 @@ needing acknowledgement are received.  The sender can use the receiver's
 Strategies and implications of the frequency of generating acknowledgments are
 discussed in more detail in {{QUIC-RECOVERY}}.
 
-To limit ACK Blocks to those that have not yet been received by the sender,
-the receiver SHOULD track which ACK frames have been acknowledged.  The
-receiver SHOULD exclude already acknowledged packets from future ACK frames
-whenever these packets would unnecessarily contribute to the ACK frame size.
+To limit the ranges of acknowledged packet numbers to those that have not yet

```suggestion
To limit the ranges of acknowledged packets to those that have not yet
```

+An endpoint SHOULD treat receipt of an acknowledgment for a packet it did not
+send as a connection error of type PROTOCOL_VIOLATION, if it is able to detect

As @marten-seemann said elsewhere, it'd be nice to have a new error code for this.

-- 
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/2302#pullrequestreview-189671385

---

From: Martin Thomson <[email-masked]>
Date: Mon, 07 Jan 2019 22:57:34 +0000 (UTC)

martinthomson commented on this pull request.

How so?  It should never happen...

-- 
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/2302#discussion_r245827160

---

From: Martin Thomson <[email-masked]>
Date: Mon, 07 Jan 2019 15:15:16 -0800

d337fcfcbe16fa4f11b6725d9fc5e8e69d5bad48  acknowledged packets, not numbers

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2302/files/d872c09a8ef35f027dfbda024407bd73cd0d0371..d337fcfcbe16fa4f11b6725d9fc5e8e69d5bad48

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Jan 2019 01:01:07 +0000 (UTC)

marten-seemann commented on this pull request.

None of our errors (except for NO_ERROR) should ever happen. We have error codes for exactly those cases where things happen that are not supposed to happen.

-- 
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/2302#discussion_r245849218

---

From: janaiyengar <[email-masked]>
Date: Mon, 07 Jan 2019 17:03:46 -0800

janaiyengar commented on this pull request.

I don't have a strong opinion, but PROTOCOL_VIOLATION seems like a fine error for this.

-- 
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/2302#discussion_r245849623

---

From: Marten Seemann <[email-masked]>
Date: Mon, 07 Jan 2019 17:10:11 -0800

We had PROTOCOL_VIOLATIONs come up in interop testing a couple of times. The problem is, it tells you **exactly nothing** about what's going wrong.

gQUIC was (relatively) easy to debug, partly because it has a very long list of error codes. At some point (and I don't remember when and why), we decided to shrink that list to just around 10 error codes. Now we don't even have a single error code left for loss-recovery related errors - it's all just a PROTOCOL_VIOLATION.

-- 
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/2302#discussion_r245850590

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 07 Jan 2019 17:16:09 -0800

@marten-seemann Shouldn't we be using Reason Phrase for debugging? It provides the endpoint to decide how granular the error reports can be, without requiring all the stack handle the errors the same way.

-- 
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/2302#discussion_r245851457

---

From: Marten Seemann <[email-masked]>
Date: Mon, 07 Jan 2019 17:33:43 -0800

Actually, I think of the reason phrase as giving more details about the error, e.g. when exceeding the stream limit, I'd send the error code STREAM_LIMIT_ERROR, and then the reason phrase might say "Peer tried to open stream 124, but stream limit was 116.".

If we say that all information should go into the reason phrase, then why do we have FLOW_CONTROL_ERROR, STREAM_LIMIT_ERROR, FINAL_OFFSET_ERROR, FRAME_ENCODING_ERROR and TRANSPORT_PARAMETER_ERROR? They're certainly just protocol violations of one sort or another.

-- 
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/2302#discussion_r245854197

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Jan 2019 15:55:46 -0800

6480343739b66463979f1c5340c40aa59aec8cca  Merge branch 'master' into ack-nonexistent-packet

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2302/files/d337fcfcbe16fa4f11b6725d9fc5e8e69d5bad48..6480343739b66463979f1c5340c40aa59aec8cca

---

From: ianswett <[email-masked]>
Date: Tue, 08 Jan 2019 15:57:32 -0800

ianswett 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/2302#pullrequestreview-190512186

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Jan 2019 15:59:20 -0800

Merged #2302 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/2302#event-2061665936

---

From: janaiyengar <[email-masked]>
Date: Tue, 08 Jan 2019 23:59:47 +0000 (UTC)

Let's do a separate discussion about error codes. I don't think it's necessary here, and if this is an important new error code, let's introduce that in a new issue/PR.

-- 
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/2302#discussion_r246211578