Subject: [quicwg/base-drafts] Add OnPacketReceived to pseudocode (#3653)

From: ianswett <[email-masked]>
Date: Wed, 13 May 2020 06:48:14 -0700

Pseudocode for re-arming the PTO when &quot;If no additional data can be sent, the server&#39;s PTO timer MUST NOT be armed until datagrams have been received from the client, because packets sent on PTO count against the anti-amplification limit. &quot;
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add OnPacketReceived to pseudocode

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Wed, 13 May 2020 06:49:34 -0700

@ianswett pushed 1 commit.

2a479f634819f531129ab49ae871f23ed9e13940  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/4444017fe3f1f1eb1baf0b00e545b8913be94c7c..2a479f634819f531129ab49ae871f23ed9e13940

---

From: Martin Thomson <[email-masked]>
Date: Wed, 13 May 2020 17:58:43 -0700

@martinthomson commented on this pull request.

Thanks for doing this.  I have one tweak that I think you need and maybe a tweak that you want.

The first is about the argument and what information is passed.  I think that this is just a notification from the main packet processing code, so it doesn't need to include packet processing.

The second is more involved and would involve adding more state variables.

> @@ -1142,6 +1142,21 @@ Pseudocode for OnPacketSent follows:
      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Packet
+
+When a packet is received, it may allow the server to send
+if the server was previously amplification limited.
+
+Psuedocode for OnPacketReceived follows:
+
+~~~
+OnPacketReceived(packet):
+  // If this packet unblocks the server, arm the

This is fine.  The alternative I considered was:

```
  if (server and peer address is not validated):
    was_blocked = aa_bytes_sent >= aa_bytes_received * 3
    aa_bytes_received += datagram.size()
    if was_blocked:
        SetLossDetectionTimer()
```
But this is fine.  In order for that to work, you need to have global state variables for aa_bytes_sent and aa_bytes_received, which you don't otherwise have.

> @@ -1142,6 +1142,21 @@ Pseudocode for OnPacketSent follows:
      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Packet
+
+When a packet is received, it may allow the server to send
+if the server was previously amplification limited.
+
+Psuedocode for OnPacketReceived follows:
+
+~~~
+OnPacketReceived(packet):

I would drop the argument to this.  Because this needs to be a datagram not a packet.

That means the function at the end should be called ProcessPackets(datagram) if you keep it.  But I would suggest that you don't need to do packet processing in this logic: this is just a function where the loss recovery code is told about what is happening in the main 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/3653#pullrequestreview-411395226

---

From: Marten Seemann <[email-masked]>
Date: Wed, 13 May 2020 19:39:59 -0700

@marten-seemann commented on this pull request.

      SetLossDetectionTimer()
 ~~~

Pseudo, not psuedo. 

      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Packet
+
+When a packet is received, it may allow the server to send
+if the server was previously amplification limited.
+
+Psuedocode for OnPacketReceived follows:
+
+~~~
+OnPacketReceived(packet):
+  // If this packet unblocks the server, arm the
+  // timer to ensure forward progress resumes
+  if (server was at anti-amplification limit):
+    SetLossDetectionTimer()
+  DecryptAndProcessFrames()

This doesn’t seem to belong in recovery. I think you’re trying to point out that even undecryptable packets unblock the server, right?

-- 
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/3653#pullrequestreview-411426042

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 13 May 2020 20:00:41 -0700

@janaiyengar commented on this pull request.

      SetLossDetectionTimer()
 ~~~

Yeah, I think this should be in the comments above.

      SetLossDetectionTimer()
 ~~~

```suggestion
  // PTO timer to avoid deadlock.
```

      SetLossDetectionTimer()
 ~~~

+1. You don't need to process packets  in here, just say up above that this function is called even if the received packet is undecryptable.

-- 
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/3653#pullrequestreview-411432230

---

From: ianswett <[email-masked]>
Date: Sat, 16 May 2020 12:44:45 -0700

@ianswett commented on this pull request.

      SetLossDetectionTimer()
 ~~~

Thanks!

-- 
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/3653#discussion_r426184659

---

From: ianswett <[email-masked]>
Date: Sat, 16 May 2020 12:50:09 -0700

75aeb2ffb9deaa7bb33ad35b7a6960388f39844a  Update based on comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/2a479f634819f531129ab49ae871f23ed9e13940..75aeb2ffb9deaa7bb33ad35b7a6960388f39844a

---

From: ianswett <[email-masked]>
Date: Sat, 16 May 2020 12:50:26 -0700

Thanks everyone, updated based on comments.  PTAL.

      SetLossDetectionTimer()
 ~~~

Yeah, I was hoping to avoid that because I think it'd be a substantial amount of extra pseudocode.

      SetLossDetectionTimer()
 ~~~

Method renamed and comment updated.

-- 
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/3653#pullrequestreview-413103713

---

From: Martin Thomson <[email-masked]>
Date: Sun, 17 May 2020 17:11:58 -0700

@martinthomson approved this pull request.

A rewording suggestion; and a minor error correction.

      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Datagram
+
+When a datagram is received, it may allow the server to send
+if the server was previously amplification limited, even if
+none of the packets in it can be processed.
+
+Pseudocode for OnDatagramReceived follows:
+
+~~~
+OnDatagramReceived(datagram):
+  // If this packet unblocks the server, arm the

```suggestion
  // If this datagram unblocks the server, arm the
```

> +When a datagram is received, it may allow the server to send
+if the server was previously amplification limited, even if
+none of the packets in it can be processed.

```suggestion
Receiving a datagram might allow the server to send additional data if the
server was previously blocked by the anti-amplification limits; see
{{before-address-validation}}. This is necessary even if none of the packets in
the datagram are successfully processed.
```

-- 
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/3653#pullrequestreview-413212951

---

From: ianswett <[email-masked]>
Date: Sun, 17 May 2020 17:19:36 -0700

54a7e018d34939d9bb42a9ea657f75415c10d907  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/75aeb2ffb9deaa7bb33ad35b7a6960388f39844a..54a7e018d34939d9bb42a9ea657f75415c10d907

---

From: ianswett <[email-masked]>
Date: Sun, 17 May 2020 17:22:08 -0700

3bdc0ed6e05a308ee501baf6bcbad206e9231f03  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/54a7e018d34939d9bb42a9ea657f75415c10d907..3bdc0ed6e05a308ee501baf6bcbad206e9231f03

---

From: ianswett <[email-masked]>
Date: Sun, 17 May 2020 17:23:29 -0700

9be18130577abf419d42efe0d3dae6f04184c00e  Remove ref to a section which does not exist.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/3bdc0ed6e05a308ee501baf6bcbad206e9231f03..9be18130577abf419d42efe0d3dae6f04184c00e

---

From: ianswett <[email-masked]>
Date: Sun, 17 May 2020 17:25:37 -0700

I removed the reference because I couldn't find that section and removed "additional data", because the amplification factor applies to all packets, not just data or ack-eliciting 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/3653#discussion_r426322796

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 18 May 2020 19:44:28 -0700

> +Receiving a datagram might allow the server to send if the
+server was previously blocked by the anti-amplification limits.
+This is necessary even if none of the packets in the datagram
+are successfully processed.

```suggestion
When a server is  blocked by anti-amplification limits, receiving
a datagram unblocks it, even if none of the packets in the datagram
are successfully processed. In such a case, the PTO timer is now set,
since data is in flight and probes can be 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/3653#pullrequestreview-414060771

---

From: ianswett <[email-masked]>
Date: Tue, 19 May 2020 07:27:24 -0700

ee306ab8f36e504f8571cb269198eeaf0abf78f9  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/9be18130577abf419d42efe0d3dae6f04184c00e..ee306ab8f36e504f8571cb269198eeaf0abf78f9

---

From: ianswett <[email-masked]>
Date: Tue, 19 May 2020 07:29:05 -0700

04e787fb731e380aaa79dc236064cfb294449dcc  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/ee306ab8f36e504f8571cb269198eeaf0abf78f9..04e787fb731e380aaa79dc236064cfb294449dcc

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 19 May 2020 15:03:43 -0700

> @@ -1142,6 +1142,22 @@ Pseudocode for OnPacketSent follows:
      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Datagram
+
+When a server is blocked by anti-amplification limits, receiving
+a datagram unblocks it, even if none of the packets in the
+datagram are successfully processed. In such a case, the PTO
+timer may need to be armed.

When would it not be armed?

-- 
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/3653#pullrequestreview-414856015

---

From: ianswett <[email-masked]>
Date: Tue, 19 May 2020 15:19:08 -0700

SetLossDetectionTimer()
 ~~~

I thought a padded ACK-only packet could hit the limit without bytes in flight, but I realize that each full-sized ACK allows 3 packets to be sent.  Updating text.

-- 
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/3653#discussion_r427632916

---

From: ianswett <[email-masked]>
Date: Tue, 19 May 2020 15:20:04 -0700

49ddf7fba915e7aabc815239718a83a720363e28  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653/files/04e787fb731e380aaa79dc236064cfb294449dcc..49ddf7fba915e7aabc815239718a83a720363e28

---

From: ianswett <[email-masked]>
Date: Tue, 19 May 2020 15:33:28 -0700

Merged #3653 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/3653#event-3353963595