Subject: Re: [quicwg/base-drafts] Include handshake anti-deadlock logic in pseudocode (#2281)

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Jan 2019 14:49:26 -0800

The x3 rule does refer to simple bytes as opposed to bytes in flight as calculated for the purposes of congestion control.  So PADDING is completely sufficient for the client.

My point was that this extra "code" exists only for the purposes of driving that x3 rule, and is only necessary when the client is sending Initial packets.  If it is sending Handshake packets, there is no need because any Handshake packet of any size is sufficient to validate the client address and disable the x3 rule.

(BTW, this wasn't directly a comment on this PR, but on the cited text as well as the code.)

-- 
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/2281#issuecomment-450762843

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 01 Jan 2019 15:12:00 -0800

> BTW, this wasn't directly a comment on this PR, but on the cited text as well as the code.

Right, but I think the currently-required behavior is justified. Consider this case:
1. Client sends Initial w/ CRYPTO
2. Server sends Initial w/ CRYPTO, ACK, followed by Handshake flight; Handshake flight is lost in full
3. Client establishes Handshake keys; sends Initial w/ ACK
4. Server Handshake packets continue to be lost until the 3x budget is spent.

Because the client's crypto packets have all been ACKed, and it has received no further ack-eliciting packets, it has nothing to send.

If the client's second Initial was also lost, meaning the budget is only about 3600 bytes, this can happen almost immediately; a single Quinn server Handshake flight is larger than that.

-- 
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/2281#issuecomment-450764183

---

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Jan 2019 15:44:25 -0800

The client can get Handshake keys in this case, but it won't be sending Handshake packets until it has all of the server Handshake packets, so I think we're still in the original state.  This is still the case that there are outstanding Initial packets.

If the client were to be sending Handshake packets, this rule - as written - would still apply, but my point is that it isn't necessary.

-- 
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/2281#issuecomment-450765960

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 01 Jan 2019 16:03:52 -0800

> it won't be sending Handshake packets until it has all of the server Handshake packets

Is that accurate? Transport §17.6 has:
> Once a client has received a Handshake packet from a server, it uses Handshake packets to send subsequent cryptographic handshake messages and acknowledgments to the server.

It seems that client might reasonably send Handshake-level ACKs (for example) prior to receiving the server's full flight even under ideal network conditions.

> This is still the case that there are outstanding Initial packets.

Ah, it wasn't clear to me that the client's second, ack-only, Initial packet qualified as "in flight"/"outstanding" here. Even then, the client discards the Initial space after first sending a Handshake packet, which might not be ack-eliciting, and might become lost, so a padding-only packet might still be necessary.

-- 
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/2281#issuecomment-450767005

---

From: Martin Thomson <[email-masked]>
Date: Tue, 01 Jan 2019 16:09:14 -0800

Right, I had forgotten.  Handshake ACKs are sent earlier.  Either way, once that starts, the risk of deadlocking is gone.

> Ah, it wasn't clear to me that the client's second, ack-only, Initial packet qualified as "in flight"/"outstanding" here. 

Good point.  The current text says "When crypto packets are sent, the sender MUST set a timer for the crypto timeout period."  And crypto packets don't include ACK-only Initial packets.

That suggests a bigger problem with that text.  I would suggest then that the condition is "Initial packets outstanding and no packet from a higher encryption level ever sent".

-- 
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/2281#issuecomment-450767309

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 01 Jan 2019 16:21:40 -0800

> Either way, once that starts, the risk of deadlocking is gone.

The risk of deadlocking remains so long as the client has not sent any ack-eliciting, i.e. retransmittable, Handshake packets and has not received any Handshake ACKs, because it is possible that all of the client's Handshake packets might be lost, at which point the server may reach the 3x threshold and cease transmitting while the client has nothing to send.

-- 
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/2281#issuecomment-450767920

---

From: Benjamin Saunders <[email-masked]>
Date: Thu, 03 Jan 2019 16:31:53 -0800

On review, the client certainly doesn't need to pad Handshake level anti-deadlock packets to 1200 bytes, though it must send them so long as it hasn't seen an ACK or sent anything that it would otherwise be required to retransmit.

-- 
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/2281#issuecomment-451322057

---

From: ianswett <[email-masked]>
Date: Sun, 06 Jan 2019 13:06:34 -0800

ianswett commented on this pull request.

> @@ -778,13 +778,7 @@ Pseudocode for SetLossDetectionTimer follows:
 
 ~~~
  SetLossDetectionTimer():
-    // Don't arm timer if there are no ack-eliciting packets
-    // in flight.
-    if (no ack-eliciting packets in flight):
-      loss_detection_timer.cancel()
-      return
-
-    if (crypto packets are in flight):
+    if (crypto packets are in flight || handshaking client):

I agree the pseudocode doesn't match the text, but I think the term "handshaking client" is not specific enough a definition to use in the pseudocode.

-- 
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/2281#pullrequestreview-189644317

---

From: Benjamin Saunders <[email-masked]>
Date: Sun, 06 Jan 2019 13:40:50 -0800

Ralith commented on this pull request.

 ~~~
  SetLossDetectionTimer():

Yeah, I wasn't sure what to go with. Should I introduce a new defined variable?

-- 
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/2281#discussion_r245523723

---

From: Benjamin Saunders <[email-masked]>
Date: Fri, 11 Jan 2019 21:06:11 -0800

Updated to use an explicit variable, and to use unpadded packets at Handshake level.

-- 
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/2281#issuecomment-453720326

---

From: ianswett <[email-masked]>
Date: Tue, 15 Jan 2019 20:53:09 +0000 (UTC)

Thanks, I think this is heading in the right direction.  I'd like it to be cleaner, but this isn't a clean mechanism.

> @@ -699,6 +703,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
     largest_acked_packet = max(largest_acked_packet,
                                ack.largest_acked)
 
+    if (from Handshake packet):

```suggestion
    if (ack from Handshake packet):
```

> @@ -699,6 +703,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
     largest_acked_packet = max(largest_acked_packet,
                                ack.largest_acked)
 
+    if (from Handshake packet):

Can you add a short comment like "Once the client receives an ACK in a Handshake packet, it knows the server considers its address verified."?

> @@ -823,6 +831,12 @@ Pseudocode for OnLossDetectionTimeout follows:
        // Crypto retransmission timeout.
        RetransmitUnackedCryptoData()
        crypto_count++
+     else if (unverified_client):
+       if (handshake keys available)
+         SendOnePacket()
+       else:
+         SendPaddedPacket()

```suggestion
         SendOnePaddedPacket()
```

-- 
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/2281#pullrequestreview-192855977

---

From: Martin Thomson <[email-masked]>
Date: Tue, 15 Jan 2019 16:24:19 -0800

martinthomson commented on this pull request.

I don't think that we should be using acknowledgments to drive state at this level.  I realize that this is less optimal, but I don't think that we need this variable.

The client can know that it is unverified if it is sending Initial packets and not Handshake (or greater).  That should be enough.  As for the test in SetLossDetectionTimer() or LossDetectionTimeout(), how is it possible for a crypto packet to be in flight, where this condition would be true?

-- 
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/2281#discussion_r248113199

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 15 Jan 2019 17:45:58 -0800

+    if (from Handshake packet):

> The client can know that it is unverified if it is sending Initial packets and not Handshake (or greater).

That's not true, though: the client doesn't know it's been validated until it's seen an ACK using Handshake keys (or greater). The server needs to see a Handshake packet to validate the client, and until the client's seen such an ACK it can't be sure that any Handshake packets it might have sent so far weren't lost.

> As for the test in SetLossDetectionTimer() or LossDetectionTimeout(), how is it possible for a crypto packet to be in flight, where this condition would be true?

I'm not sure I understand the question. The tests in those functions are to ensure that a client sends packets to either earn anti-amplification credit or validate its address even when it has no information to send.

-- 
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/2281#discussion_r248127126

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 15 Jan 2019 18:01:12 -0800

Added @ianswett's tweaks and relaxed the validation check slightly, since a client could conceivably receive a 1-RTT ACK before a Handshake ACK due to even a single packet's worth of reordering.

-- 
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/2281#issuecomment-454622710

---

From: Martin Thomson <[email-masked]>
Date: Tue, 15 Jan 2019 19:26:25 -0800

Sorry, I should proof-read these.  I should have said that the client can assume that it is unverified for longer than it actually is.  The cost, for a client that is sending Handshake packets is small and for Initial packets, the time is well bounded.

-- 
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/2281#discussion_r248141158

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 15 Jan 2019 19:49:27 -0800

Are you suggesting that we ditch `unverified_client` in favor of checking whether the endpoint is a client that doesn't yet have 1-RTT? I think that works as a simplification, if I'm understanding correctly.

-- 
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/2281#discussion_r248144053

---

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

I think that works.  For the most part "do I have crypto packets in flight" covers the rest of it.  Though "in flight" might be wrong, and it might be better as "are any crypto packets that I have sent, but are unacknowledged".

The cost is that the client might keep this timer well past the point it is needed when acknowledgments are lost, but the simplification is pretty big.

-- 
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/2281#discussion_r248147254

---

From: janaiyengar <[email-masked]>
Date: Thu, 17 Jan 2019 17:39:56 -0800

janaiyengar commented on this pull request.

Should this be happening in an issue? I think we're close here, but I would recommend moving this discussion to an issue if it continues.

FWIW, I'm in agreement with @martinthomson that it's better to drive logic here using the client's state instead of the received 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/2281#pullrequestreview-193919308