Subject: Re: [quicwg/base-drafts] Don't store or retransmit PATH_RESPONSE frames, avoid buffering (#2729)

From: Mike Bishop <[email-masked]>
Date: Thu, 06 Jun 2019 11:07:11 -0700

MikeBishop commented on this pull request.

> @@ -1721,7 +1721,12 @@ it can associate the peer's response with the corresponding PATH_CHALLENGE.
 ## Path Validation Responses
 
 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
-echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
+echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame,
+unless it has PATH_RESPONSE frames buffered for the same destination connection
+ID and wishes to limit memory consumption.
+
+An endpoint MUST NOT store or retransmit a PATH_RESPONSE frame, as a new
+PATH_CHALLENGE frame will be sent if another PATH_RESPONSE frame is needed.

There's no point in putting multiple PATH_RESPONSEs in a single packet, nor in responding to more than one PATH_CHALLENGE from a given packet.  The others can just be ignored.  However, multiple attempts across multiple packets increases resiliency -- if you get 100 PATH_CHALLENGEs in 100 packets, I'd say you send back 100 PATH_RESPONSEs in 100 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/2729#discussion_r291306399

---

From: ianswett <[email-masked]>
Date: Mon, 10 Jun 2019 10:23:46 -0700

ianswett commented on this pull request.

To back up @MikeBishop point, I'd suggest. "If a packet contains multiple PATH_CHALLENGE frames, only one PATH_RESPONSE should be sent in response."

-- 
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/2729#discussion_r292106697

---

From: David Schinazi <[email-masked]>
Date: Mon, 10 Jun 2019 15:39:24 -0700

DavidSchinazi commented on this pull request.

Since there's no point in having multiple challenges in one packet, why not simply forbid it?

-- 
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/2729#discussion_r292218667

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 10 Jun 2019 16:13:21 -0700

erickinnear commented on this pull request.

I'd be fine with forbidding it if we think it's harmful in some way, I'd much prefer that to collapsing multiple PATH_CHALLENGE frames into a single PATH_RESPONSE.

-- 
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/2729#discussion_r292225930

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 10 Jun 2019 16:15:07 -0700

That's part of what @ianswett was attempting to address when we brought this issue up, but I think the idea there was deliberately about rate limiting of the amount of memory you could be required to consume. Not sure how much text we need there... 
@ianswett, thoughts?

-- 
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/2729#discussion_r292226269

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 10 Jun 2019 16:15:35 -0700

Agreed, I like that as well. Will change! 

-- 
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/2729#discussion_r292226391

---

From: ianswett <[email-masked]>
Date: Mon, 10 Jun 2019 19:21:39 -0700

If we forbid multiple PATH_CHALLENGE frames in a single packet, I believe we don't need to add text about buffering.  But I'm not sure if that's an acceptable restriction to the WG.  If not, an alternate option would be to say you SHOULD only send one PATH_CHALLENGE in a packet and you SHOULD only respond to one PATH_CHALLENGE from a single packet.

-- 
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/2729#discussion_r292256204

---

From: MikkelFJ <[email-masked]>
Date: Tue, 11 Jun 2019 00:32:15 -0700

mikkelfj commented on this pull request.

I rather prefer the principle where you can process a frame without knowing the rest of the packet content. Since challenges are not retransmitted with the complexity that could entail, it is simpler to send at most one challenge per packet.

-- 
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/2729#discussion_r292312804

---

From: Dmitri Tikhonov <[email-masked]>
Date: Tue, 11 Jun 2019 05:17:15 -0700

dtikhonov commented on this pull request.

The issue is not important enough to warrant its own "MUST NOT."  If an endpoint wants to send a packetful of _PATH_CHALLENGE_ frames, let it.  The peer is allowed to send only one _PATH_RESPONSE_.

-- 
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/2729#discussion_r292423173

---

From: Mike Bishop <[email-masked]>
Date: Tue, 11 Jun 2019 10:45:57 -0700

Do we have precedent for a fixed number of something allowed in a packet?  We've previously said multiple ACKs isn't best practice, but it's not prohibited.  This feels similar -- adding a new type of prohibition for something not actively harmful seems excessive.  Simply don't require them to be processed, and let implementations choose.

-- 
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/2729#discussion_r292581950

---

From: Yang Chi <[email-masked]>
Date: Tue, 11 Jun 2019 16:46:01 -0700

Should there also be changes in the recovery specs? Now that PATH_RESPONSE shouldn't be retransmitted, loss timer may not be set if the only inflight packet has only PATH_RESPONSE frame (unless one wants to waste CPU). But it still is an ack-eliciting packet. In the current recovery doc, as long as ack-eliciting packets are  inflight, loss timer will be set up. So this sounds buggy to me. Does this make sense?

-- 
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/2729#issuecomment-501065019

---

From: ianswett <[email-masked]>
Date: Wed, 12 Jun 2019 06:42:06 -0700

There are other frames that are not retransmitted, and the recovery spec is designed to deal with that.

The second and third paragraph of 5.3.2 discuss this case: https://tools.ietf.org/html/draft-ietf-quic-recovery-20#section-6.3.2

-- 
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/2729#issuecomment-501278700

---

From: Yang Chi <[email-masked]>
Date: Wed, 12 Jun 2019 06:55:41 -0700

(Is that the right section? There is no 5.3.2 and the link was to 6.3.2.)
I don't think 6.3.2 addresses this. The 2nd and 3rd paragraph in 6.3.2 addresses what to send after loss timer fires. My comment is about the loss timer should not even be set in the first place if PATH_RESPONSE is all you have inflight. 

-- 
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/2729#issuecomment-501284453

---

From: ianswett <[email-masked]>
Date: Wed, 12 Jun 2019 07:02:10 -0700

Yes, 6.3.2

If you have inflight bytes, you'd like to remove them eventually.  The recovery document uses the PTO timer to do that.  I would not be comfortable not including PATH_RESPONSE in bytes in flight and I'm not comfortable saying you don't have to worry about removing those bytes, hence the existing text in recovery.

-- 
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/2729#issuecomment-501287424

---

From: Yang Chi <[email-masked]>
Date: Wed, 12 Jun 2019 07:58:23 -0700

Ye that makes sense. Thank 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/2729#issuecomment-501312753

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Jun 2019 19:20:42 -0700

@martinthomson pushed 1 commit.

c205b79ad75d9e67759dcb48994f3aef3b021715  Prohibit sending multiple PATH_RESPONSE

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Jun 2019 19:22:52 -0700

I took the liberty of making the change proposed by Kazuho here.

I did not make a change to prohibit sending multiple PATH_RESPONSE per packet.  As others have pointed out, that makes processing packets for sending more complex than is ideal.  The text allowing these frames to be dropped is fine.

If someone disagrees with this, then I think it would be best to take that discussion up in a new issue.

-- 
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/2729#issuecomment-501943552

---

From: MikkelFJ <[email-masked]>
Date: Thu, 13 Jun 2019 23:14:29 -0700

> An endpoint MUST NOT send more than one PATH_RESPONSE frame in response to one
PATH_CHALLENGE frame. 

With this text, should't PATH_RESPONSE be non-ack-eliciting?

-- 
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/2729#issuecomment-501982572

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 13 Jun 2019 23:32:18 -0700

@mikkelfj
> With this text, should't PATH_RESPONSE be non-ack-eliciting?

I think there is no need for such change, because a frame that's ack-eliciting does not necessary mean that it needs to be retransmitted. 

Quoting section 13.2 of the transport draft, _QUIC packets that are determined to be lost are not retransmitted whole. The same applies to the frames that are contained within lost packets. Instead, the information that might be carried in frames is sent again in new frames as needed._

-- 
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/2729#issuecomment-501986664

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 06:28:05 -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/2729#pullrequestreview-249901955

---

From: David Schinazi <[email-masked]>
Date: Fri, 14 Jun 2019 08:25:14 -0700

I like the new text prohibiting sending multiple PATH_RESPONSE frames. Now that we have that, do we still need `unless it has PATH_RESPONSE frames buffered`? Why would one buffer PATH_RESPONSE frames?

-- 
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/2729#issuecomment-502152235

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 10:53:53 -0700

I'd be happy with saying SHOULD only send one and SHOULD only respond to one. Generally, I'm not a huge fan of collapsing multiple path challenges, but if we combine that with "don't send something that will get collapsed" then if that happens its your own fault.

-- 
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/2729#discussion_r293914787

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:03:18 -0700

@erickinnear pushed 1 commit.

f00f3d111f9ca642081e9c9dd2ce7c117081055b  Only one path response should be sent in response to a packet containing any number of path challenge frames

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

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:06:09 -0700

Added text for SHOULD NOT send/reply to the appropriate sections, as with @ianswett and @MikeBishop's suggestions.

-- 
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/2729#discussion_r293919172

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:06:54 -0700

Added text for this and removed the "buffered" sentence. 

-- 
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/2729#discussion_r293919440

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:07:11 -0700

Changed (updated, and thanks @martinthomson!)

-- 
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/2729#discussion_r293919544

---

From: MikkelFJ <[email-masked]>
Date: Fri, 14 Jun 2019 11:07:44 -0700

This latest commit reintroduces the need to be aware ofther other challenge frames in a packet. @martinthomson sensibly removed that in the previous commit.

I ought to suffice to respond at most once per frame and say SHOULD not send more than one challenge per packet.

-- 
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/2729#issuecomment-502209393

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:09:02 -0700

Added a reference to the retransmission of information section (bringing that closer to the relevant "what to do with these frames bits" is most of the point of this PR), should cover the remaining conversations here.

With that, I think this is ready, please take a look.

-- 
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/2729#issuecomment-502209813

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:14:27 -0700

If we want a single/sided SHOULD, I'm happy with that. I'd also prefer to avoid being aware of multiple frames in a packet. Will change.

-- 
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/2729#issuecomment-502211542

---

From: Eric Kinnear <[email-masked]>
Date: Fri, 14 Jun 2019 11:16:00 -0700

@erickinnear pushed 2 commits.

950ab2255168c827670234a01f8211ea68090d35  Merge branch 'master' into dont_retransmit_path_response
bef17653caa29d28e96fa825ad34ef4ce8345d70  Remove requirement on responders to PATH_CHALLENGE

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

---

From: David Schinazi <[email-masked]>
Date: Fri, 14 Jun 2019 11:45:23 -0700

DavidSchinazi approved this pull request.

Latest text 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/2729#pullrequestreview-250065898

---

From: ianswett <[email-masked]>
Date: Fri, 14 Jun 2019 13:12:45 -0700

-- 
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/2729#pullrequestreview-250100889

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 15 Jun 2019 00:10:01 -0700

janaiyengar commented on this pull request.

> @@ -1743,6 +1744,10 @@ it can associate the peer's response with the corresponding PATH_CHALLENGE.
 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
 echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
 
+An endpoint MUST NOT send more than one PATH_RESPONSE frame in response to one
+PATH_CHALLENGE frame (see {{retransmission-of-information}}).  A new
+PATH_CHALLENGE frame will be sent if another PATH_RESPONSE frame is needed.

Suggested rephrase: The peer is expected to send more PATH_CHALLENGE frames as necessary to evoke additional PATH_RESPONSE frames.

-- 
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/2729#pullrequestreview-250182219

---

From: Jana Iyengar <[email-masked]>
Date: Sat, 15 Jun 2019 00:10:16 -0700

janaiyengar approved this pull request.

One editorial suggestion, but 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/2729#pullrequestreview-250182226

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 17 Jun 2019 10:32:22 -0700

3c0d1f7297091d0e0cff31d7850fa1c557f2d130  Update with Jana's wording suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2729/files/bef17653caa29d28e96fa825ad34ef4ce8345d70..3c0d1f7297091d0e0cff31d7850fa1c557f2d130

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 17 Jun 2019 10:32:41 -0700

Nice! Updated in both places for consistency (3c0d1f7).

-- 
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/2729#discussion_r294408449

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 17 Jun 2019 10:34:14 -0700

@mnot, @larseggert This should be ready to go to the list

-- 
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/2729#issuecomment-502779358