Subject: [quicwg/base-drafts] Clarify text around preferred address (#3589)

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 01:00:49 -0700

Add remaining changes from #3353, specifically: 
- Handshake finished -&gt; confirmed
- Move text around to clarify existing requirements around local addresses and CIDs
- Add note nearby when describing use of SPA that preferred address can be used with CIDs other than that specified in the TP (this is already described later in the document)

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

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

-- Commit Summary --

  * Clarify existing requirements on CID use to consolidate expected behavior
  * Make {{issue-cid}} and {{retire-cid}} consistent
  * Clarify handshake confirmed for preferred address, note that any CID is valid for use as mentioned later

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Mon, 20 Apr 2020 02:53:39 -0700

@martinthomson commented on this pull request.

Thanks for putting this together.

I have a few editorial comments, which look worse than they really are.

> +Similarly, an endpoint MUST NOT reuse a connection ID when sending to more than
+one destination address, for example when responding to a change in the address
+of a peer if the packet with the new peer address uses an active connection ID
+that has not been previously used by the peer.

This is a little long and the 'for example' is a little buried.

```suggestion
Similarly, an endpoint MUST NOT reuse a connection ID when sending to a remote
address from which the peer has chosen to use a connection ID that has not
previously been used.  This occurs when an endpoint receives packets that
include a change to both the source address and destination connection ID at the
same time.
```

> +Note that these requirements apply to the endpoints sending packets, as
+unintentional changes in path without a change in connection ID are possible.
+For example, after a period of network inactivity, NAT rebinding might cause
+packets to be sent on a new path when the client resumes sending.  An endpoint
+responds to such an event as described in {{migration-response}}.

```suggestion
These requirements regarding connection ID reuse apply only to the sending of
packets, as unintentional changes in path without a change in connection ID are
possible.  For example, after a period of network inactivity, NAT rebinding
might cause packets to be sent on a new path when the client resumes sending.
An endpoint responds to such an event as described in {{migration-response}}.
```

>  : The sequence number of the connection ID being retired.  See
-  {{retiring-cids}}.
+  {{retire-cid}}.

I can't suggest here (damn GitHub).  But what you want is a semi-colon: '; see {{retire-cid}}.'

> @@ -5610,7 +5619,7 @@ IDs, the endpoint MAY treat that receipt as a connection error of type
 PROTOCOL_VIOLATION.
 
 The Retire Prior To field counts connection IDs established during connection
-setup and the preferred_address transport parameter (see {{retiring-cids}}). The
+setup and the preferred_address transport parameter (see {{retire-cid}}). The

```suggestion
setup and the preferred_address transport parameter; see {{retire-cid}}. The
```

>  : A variable-length integer indicating which connection IDs should be retired.
-  See {{retiring-cids}}.
+  See {{retire-cid}}.

semi-colon here too.

>  {{migrate-validate}}) of that address using the connection ID provided in the
-preferred_address transport parameter.
+preferred_address transport parameter or, if that connection ID has been
+retired, any active connection ID that has not already been used on a different
+path.

I can't suggest here, but maybe "using any previously unused connection ID, taken from either the preferred_address transport parameter or the NEW_CONNECTION_ID frame."

-- 
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/3589#pullrequestreview-396301600

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 20 Apr 2020 05:53:15 -0700

@kazuho commented on this pull request.

I prefer @martinthomson's suggestion. For CIDs provided by NCID frames, we do not require them to be used in particular order, even when the client has received CIDs in order. I would be hesitant to to require in-order use of CID when SPA is involved.

-- 
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/3589#discussion_r411351974

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 20 Apr 2020 05:56:10 -0700

@erickinnear Thank you so much for doing this work. I think the changes are spot-on. I see some improvements suggested by @martinthomson, but otherwise looks perfect. ❤️ 👍 🥇 

-- 
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/3589#pullrequestreview-396425796

---

From: Mike Bishop <[email-masked]>
Date: Mon, 20 Apr 2020 09:05:00 -0700

@MikeBishop approved this pull request.

For the semicolon ones which aren't in text you're actually changing, it might be better to leave them and do a separate editorial PR that cleans these up.  I'm sure the ones that hit the updated reference aren't the only ones.

This is more precise, but still feels a little obtuse.  Would it be clearer to switch this into an exception?
```suggestion
Similarly, an endpoint MUST NOT reuse a connection ID when sending to more than
one destination address.  However, if an endpoint receives packets from a new
source address with the same destination connection ID, it MAY continue to use
the current connection ID with the new address.
```

> @@ -5654,7 +5663,7 @@ RETIRE_CONNECTION_ID frames contain the following fields:
 Sequence Number:

```suggestion
: The sequence number of the connection ID being retired; see
```

-- 
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/3589#pullrequestreview-396590152

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 12:45:55 -0700

@erickinnear commented on this pull request.

Thanks! I was struggling to find a way to word that nicely, this is much better.

-- 
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/3589#discussion_r411644109

---

From: Martin Thomson <[email-masked]>
Date: Mon, 20 Apr 2020 15:07:17 -0700

I'm happy with Mike's suggestion.

-- 
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/3589#discussion_r411724260

---

From: ianswett <[email-masked]>
Date: Mon, 20 Apr 2020 19:32:35 -0700

@ianswett approved this pull request.

Some small nits, but this looks good.

Is rationale useful here?   ie "as this indicates the peer did not intentionally change its address."?

 PROTOCOL_VIOLATION.

I thought parens were fine?  Personally, I prefer parens, so I'm curious why prefer semicolons?

I think they're used quite widely, so if you want to consistently move to semicolons, I'd suggest a single cleanup PR to switch everything over.

-- 
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/3589#pullrequestreview-396944598

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:25:54 -0700

@erickinnear pushed 1 commit.

3cf8eaa6b912fd715691bbdb660967554c01d61a  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3589/files/0e1664e9680b22789418c61579c2ce7d1dee0336..3cf8eaa6b912fd715691bbdb660967554c01d61a

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:28:43 -0700

5509f1d2cd36a7c3dd780cf3722338b2c0348680  Review comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3589/files/3cf8eaa6b912fd715691bbdb660967554c01d61a..5509f1d2cd36a7c3dd780cf3722338b2c0348680

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:31:19 -0700

-  {{retiring-cids}}.
+  {{retire-cid}}.

Done in 3cf8eaa

-- 
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/3589#discussion_r411860302

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:31:28 -0700

-  See {{retiring-cids}}.
+  See {{retire-cid}}.

Done in 3cf8eaa

-- 
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/3589#discussion_r411860380

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:32:05 -0700

Fixed in 5509f1d

-- 
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/3589#discussion_r411860622

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:37:16 -0700

Thanks! This is nice wording, added in 5509f1d.
 
Re: rationale, I think that's mostly covered by two notes indicating that this can happen.
This one, the immediate next paragraph after the sentence in question:
```
These requirements regarding connection ID reuse apply only to the sending of
packets, as unintentional changes in path without a change in connection ID are
possible.  For example, after a period of network inactivity, NAT rebinding
might cause packets to be sent on a new path when the client resumes sending.
An endpoint responds to such an event as described in {{migration-response}}.
```
and, immediately after the other place we talk about this kind of interaction: 
```
Not all changes of peer address are intentional, or active, migrations. The peer
could experience NAT rebinding: a change of address due to a middlebox, usually
a NAT, allocating a new outgoing port or even a new outgoing IP address for a
flow.  An endpoint MUST perform path validation ({{migrate-validate}}) if it
detects any change to a peer's address, unless it has previously validated that
address.
```

-- 
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/3589#discussion_r411862410

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:39:29 -0700

We've moved a bunch of stuff to semicolons (I did like parenthesis a bit too), so I kept these consistent since we're touching those lines here already, and I will send a separate PR that just changes some of the others that aren't related to any of the changes in this 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/3589#issuecomment-616947495

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 Apr 2020 21:42:28 -0700

Thanks for the excellent comments and suggestions! Applied all the changes, should cover everything currently outstanding.

-- 
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/3589#issuecomment-616948230

---

From: David Schinazi <[email-masked]>
Date: Tue, 21 Apr 2020 09:24:45 -0700

@DavidSchinazi 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/3589#pullrequestreview-397455430