Subject: [quicwg/base-drafts] The assertion in OnPacketsLost can fail (#4172)

From: Martin Thomson <[email-masked]>
Date: Wed, 30 Sep 2020 03:24:27 -0700

This is a little convoluted, but here's the logic.

The loss recovery and congestion control both have separate states for tracking when the first RTT sample arrived.  In practice you would likely mix those and share RTT estimation states.  The RTT estimation is performed on the loss recovery side, but the results are used in the congestion control side, primarily for determining the PTO for persistent congestion. 

The first RTT sample on the loss recovery side is set when the first ack-eliciting packet is acknowledged.

The congestion control side declares an RTT estimate available when the first ACK arrives.

That difference means that if an acknowledgment only acknowledges non-ack-eliciting packets AND there is a loss (which seems plausible in that case), THEN the congestion control side will think that there is an RTT estimate when there is not.  This leads to this assertion in OnPacketsLost failing:

```python
  // Only consider packets sent after getting an RTT sample.
  assert(first_rtt_sample != 0)
```

I was just in the process of changing my "detect_persistent_congestion" implementation to switch from:

```rust
if first_rtt_sample.is_none() {
    return;
}
```
to 

```rust
debug_assert!(first_rtt_sample.is_some());
```

And when I traced the logic back I found this bug.

The fix is easy:

```python
  // Only consider packets sent after getting an RTT sample.
  if (first_rtt_sample == 0):
    return
```

-- 
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/issues/4172

---

From: ianswett <[email-masked]>
Date: Wed, 30 Sep 2020 12:33:45 -0700

non-ack-eliciting packets don't elicit an ACK, so you should never receive an ACK that only newly acknowledges non-ack-eliciting packets.

However, given there may be buggy peers, the change from an assert to an if is fine with me.

-- 
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/issues/4172#issuecomment-701599221

---

From: Martin Thomson <[email-masked]>
Date: Wed, 30 Sep 2020 16:28:51 -0700

I thought about the non-ack-eliciting thing.  There could be cases (though likely only early in each packet number space) where an non-ack-eliciting packet has been received and the receiver needs to send other stuff, so they include an ACK.  I don't think that our stack would, and what I see as a natural interpretation of the spec wouldn't necessarily lead to that, but it isn't expressly forbidden.

-- 
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/issues/4172#issuecomment-701699318

---

From: Nick Banks <[email-masked]>
Date: Wed, 30 Sep 2020 16:34:19 -0700

Generally, you could definitely receive an ACK for only non-ACK-eliciting packets if the peer has other data to send and decided to package up ACKs with that data. MsQuic does this. It will send any ACKs it has queued up if a packet is built for other reasons (e.g. stream data, control frames, etc.). Now, I'm not sure this can end up happening for the first RTT sample though. Perhaps.

-- 
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/issues/4172#issuecomment-701700840

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 30 Sep 2020 23:00:06 -0700

Closed #4172 via #4174.

-- 
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/issues/4172#event-3827643189