Subject: [quicwg/base-drafts] Added more test to NCID section (#3535)

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 15:43:17 -0700

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

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

-- Commit Summary --

  * Added more test to NCID section

-- File Changes --

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

-- Patch Links --

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

---

From: Eric Kinnear <[email-masked]>
Date: Thu, 19 Mar 2020 16:33:59 -0700

erickinnear commented on this pull request.

> @@ -5579,8 +5585,8 @@ value.
 An endpoint that receives a NEW_CONNECTION_ID frame with a sequence number
 smaller than the Retire Prior To field of a previously received
 NEW_CONNECTION_ID frame MUST immediately send a corresponding
-RETIRE_CONNECTION_ID frame that retires the newly received connection ID.
-
+RETIRE_CONNECTION_ID frame that retires the newly received connection ID,
+unless it has already done so for that sequence number.

Was the concern about doing this multiple times, or was it about the `immediately` part? That still wasn't clear based on the discussion on the list.

> @@ -5571,6 +5571,12 @@ Retire Prior To field MUST be less than or equal to the Sequence Number field.
 Receiving a value greater than the Sequence Number MUST be treated as a
 connection error of type FRAME_ENCODING_ERROR.
 
+Endpoints that receive a NEW_CONNECTION_ID frame SHOULD NOT retire connection

I honestly find this (and that it is normative language) more confusing than not: it's incredibly specific about the requirement, and that requirement is the exact inverse requirement of Retire Prior To -- I get that that's the point, but the phrasing leads me down a confusing path. 
I went to suggest an alternative for wording (and a new location away from the frame mechanics and as the second to last paragraph of `{{retiring-cids}}`), but I'm struggling to find anything here that doesn't seem to cause additional confusion. 

I'm not quite seeing how any reading of the Retire _PRIOR_ To text leads one to try and retire all of the CIDs that aren't _PRIOR_ to the value received. From the description of the issue you ran into, the problem was less around RPT at all and more than you tried to throw away all other CIDs when you got a NCID frame, which might be something worth mentioning but is not this text.

Text more to that effect would probably go best around the active_cid_limit discussion where we indicate that you should probably keep as many as you said you would be willing for the peer to send you.

-- 
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/3535#pullrequestreview-378158838

---

From: Martin Thomson <[email-masked]>
Date: Thu, 19 Mar 2020 17:13:54 -0700

martinthomson commented on this pull request.

I would also lose the "immediately".  That's not a useful word to use in this context.

+Endpoints that receive a NEW_CONNECTION_ID frame SHOULD NOT retire connection
+IDs with a sequence number equal to or greater than the Retire Prior To field
+solely as a result of receiving the frame. If the peer is trying to maintain a
+stock of provided CIDs, this can result in an infinite loop of
+NEW_CONNECTION_ID and RETIRE_CONNECTION_ID frames.

I don't think that we need this paragraph.

-- 
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/3535#pullrequestreview-378174705

---

From: Martin Thomson <[email-masked]>
Date: Thu, 19 Mar 2020 17:15:12 -0700

If you drop the first paragraph, this is just an editorial fix as far as I'm concerned. 

That is, not sending a frame that was previously sent is not a change.  And - assuming that you dump it - "immediately" has no defined meaning anyway.

-- 
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/3535#issuecomment-601473795

---

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 17:33:13 -0700

martinduke commented on this pull request.

@erickinnear The multiple times part.
@martinthomson good suggestion; I will patch it after I make dinner.

-- 
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/3535#discussion_r395391470

---

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 17:34:57 -0700

I am happy to jettison this paragraph to get the other bit through. The point is to say "here is a stupid thing to do and don't do it". If there's some other wording, or some other place to put it that's better, I'm fine with that too.

-- 
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/3535#discussion_r395391892

---

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 17:36:30 -0700

@martinthomson I'm happy to drop the first paragraph, and I'm glad to hear that it's editorial. Two people on our team took the existing text to mean that we had to send RCID after receiving NCID even though we'd previously retired that sequence number.

Update coming soon.

-- 
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/3535#issuecomment-601478535

---

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 19:23:51 -0700

@martinduke pushed 1 commit.

22c7e1245c0889e7c43944e8c4dc1482175927de  take all the suggestions

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3535/files/2e306e30c55c8a21b1a6818c670fc52c529677cf..22c7e1245c0889e7c43944e8c4dc1482175927de

---

From: martinduke <[email-masked]>
Date: Thu, 19 Mar 2020 19:25:32 -0700

@martinthomson Feel free to change to "editorial"

-- 
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/3535#issuecomment-601499226

---

From: Nick Banks <[email-masked]>
Date: Thu, 19 Mar 2020 20:03:35 -0700

nibanks approved this pull request.

Looks fine to me, though I do think it's better with some kind of language to send it sooner rather than later.

> @@ -5578,9 +5578,9 @@ value.
 
 An endpoint that receives a NEW_CONNECTION_ID frame with a sequence number
 smaller than the Retire Prior To field of a previously received
-NEW_CONNECTION_ID frame MUST immediately send a corresponding
-RETIRE_CONNECTION_ID frame that retires the newly received connection ID.
-
+NEW_CONNECTION_ID frame MUST send a corresponding RETIRE_CONNECTION_ID frame

"promptly send"? Though that's almost the same as immediately.

-- 
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/3535#pullrequestreview-378211773

---

From: ianswett <[email-masked]>
Date: Sat, 21 Mar 2020 12:05:22 -0700

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/3535#pullrequestreview-378943083

---

From: Martin Thomson <[email-masked]>
Date: Sun, 22 Mar 2020 15:03:07 -0700

Yeah, this is fine as is.

-- 
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/3535#discussion_r396146316

---

From: Martin Thomson <[email-masked]>
Date: Sun, 22 Mar 2020 19:52:15 -0700

Merged #3535 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/3535#event-3153433483