Subject: [quicwg/base-drafts] Move version before packet number (#939)

From: ianswett <[email-masked]>
Date: Thu, 16 Nov 2017 10:31:00 +0000 (UTC)

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

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

-- Commit Summary --

  * Move version before packet number

-- File Changes --

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

-- Patch Links --

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

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Nov 2017 10:43:03 +0000 (UTC)

MikeBishop requested changes on this pull request.

One thing to fix; otherwise looks good.

> @@ -395,17 +395,17 @@ Connection ID:
 : Octets 1 through 8 contain the connection ID. {{connection-id}} describes the
   use of this field in more detail.
 
-Packet Number:
-
-: Octets 9 to 12 contain the packet number.  {{packet-numbers}} describes the
-  use of packet numbers.
-
 Version:
 
 : Octets 13 to 16 contain the selected protocol version.  This field indicates

You'll need to renumber the octets, 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/939#pullrequestreview-77073613

---

From: ianswett <[email-masked]>
Date: Thu, 16 Nov 2017 10:45:19 +0000 (UTC)

@ianswett pushed 1 commit.

bebdc35  Update octets

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/939/files/6e51418b754fb00b6c6bbf1da66383daa73cc174..bebdc35c052d69e0cac86fccece9668c74d89f2a

---

From: ianswett <[email-masked]>
Date: Thu, 16 Nov 2017 10:46:51 +0000 (UTC)

ianswett commented on this pull request.

-Packet Number:
-

Thanks Mike, fixed

-- 
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/939#discussion_r151378876

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Nov 2017 23:50:24 +0000 (UTC)

MikeBishop 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/939#pullrequestreview-77302670

---

From: janaiyengar <[email-masked]>
Date: Tue, 28 Nov 2017 23:14:00 +0000 (UTC)

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/939#pullrequestreview-79690074

---

From: ianswett <[email-masked]>
Date: Wed, 29 Nov 2017 02:08:45 +0000 (UTC)

b3eb7d3  Update so the packet number must be 0 in VN

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

---

From: janaiyengar <[email-masked]>
Date: Wed, 29 Nov 2017 02:27:11 +0000 (UTC)

janaiyengar commented on this pull request.

You also need to change text in Section 5.3 that currently states that packet number is echoed in VN packets.

-- 
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/939#pullrequestreview-79721212

---

From: ianswett <[email-masked]>
Date: Wed, 29 Nov 2017 02:30:25 +0000 (UTC)

6d19a06  Address Jana's comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/939/files/b3eb7d358efc9d972e73ce75582a17b59293e1d4..6d19a069b87821e9e1853634c7aefe73d14f5d10

---

From: janaiyengar <[email-masked]>
Date: Wed, 29 Nov 2017 02:36:05 +0000 (UTC)

> @@ -515,8 +515,8 @@ A Version Negotiation packet has long headers with a type value of 0x01 and is
 sent only by servers.  The Version Negotiation packet is a response to a client
 packet that contains a version that is not supported by the server.
 
-The packet number, connection ID and version fields echo corresponding values
-from the triggering client packet.  This allows clients some assurance that the
+The connection ID and version fields echo corresponding values from the
+triggering client packet.  This allows clients some assurance that the
 server received the packet and that the Version Negotiation packet was not
 carried in a packet with a spoofed source address.

I would add something like "The packet number field SHOULD be set to 0 and will be ignored by the receiver."

> @@ -940,9 +940,8 @@ successfully receives a response or it abandons the connection attempt.
 ### Handling Version Negotiation Packets {#handle-vn}
 
 When the client receives a Version Negotiation packet, it first checks that the
-packet number and connection ID match the values the client sent in a previous
-packet on the same connection.  If this check fails, the packet MUST be
-discarded.
+packet number is 0 and the connection ID matches the connection ID the client

I don't think you need to check the packet number being 0. It's enough to check the connection ID and I would ignore the packet number at the receiver.

-- 
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/939#pullrequestreview-79722226

---

From: ianswett <[email-masked]>
Date: Wed, 29 Nov 2017 02:42:24 +0000 (UTC)

b74a493  No longer check the packet number

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/939/files/6d19a069b87821e9e1853634c7aefe73d14f5d10..b74a493786ecd5410a03cce8867a3e1d653c6ae5

---

From: ianswett <[email-masked]>
Date: Wed, 29 Nov 2017 02:43:29 +0000 (UTC)

I think it's fine not to check the packet number(as updated), but if so, then advising what to set there seems unnecessary?

-- 
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/939#discussion_r153681866

---

From: janaiyengar <[email-masked]>
Date: Wed, 29 Nov 2017 02:45:25 +0000 (UTC)

The sender has to put something in the packet number, so I think it's better to be explicit about what it should do.

-- 
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/939#discussion_r153682056

---

From: janaiyengar <[email-masked]>
Date: Wed, 29 Nov 2017 02:46:54 +0000 (UTC)

One comment pending -- I think it's better to specify what a sender should do, since it's better to be explicit, and you don't want to have to read about receiver behavior to infer what a sender should do -- but LGTM otherwise.

-- 
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/939#pullrequestreview-79723655

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 03:08:54 +0000 (UTC)

martinthomson commented on this pull request.

Note that if we decide to have a shorter packet header for Version Negotiation, a 0 is harmless. It advertises support for a version that cannot be supported, so it can be ignored.

I was going to suggest having explicit text about ignoring the packet number, but we might just remove it, so don't bother just yet.

-- 
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/939#discussion_r153684470

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 03:08:57 +0000 (UTC)

Merged #939.

-- 
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/939#event-1362725792