Skip to content

fix(proto): tail-loss probes should always be ack-eliciting#561

Merged
flub merged 5 commits intomainfrom
flub/pto-always-ack-eliciting
Apr 9, 2026
Merged

fix(proto): tail-loss probes should always be ack-eliciting#561
flub merged 5 commits intomainfrom
flub/pto-always-ack-eliciting

Conversation

@flub
Copy link
Copy Markdown
Collaborator

@flub flub commented Apr 2, 2026

Description

The PacketBuilder gets is_ack_eliciting from the SendableFrames, but
that is empty if data had to be queued especially for the probe. This
would mean that the PacketBuilder::finish_and_track did not note the
correct number of in-flight bytes for the congestion controller and
also did not update the
PacketNumberSpace::time_of_last_ack_eliciting_packet. That field in
turn is used by the PTO calculation as time from which to arm the next
PTO timeout.

To fix this we add information about all frames on whether they are
ack-eliciting or not. And if any ack-eliciting frame is written by the
PacketBuilder it keeps track of this internally.

Tail-loss probes should also not happen for the Data spaces as long as
the handshake has not yet been confirmed. During this time the Initial
and Handshake spaces' tail-loss probes are the ones that drive the
connection forward. And sending tail-loss probes on the Data space already
consumes congestion controller window and pacing tokens. Also you
don't even know if the peer has keys to decrypt them.

Breaking Changes

n/a

Notes & open questions

n/a

The PacketBuilder gets is_ack_eliciting from the SendableFrames, but
that is empty if data had to be queued especially for the probe. This
would mean that the PacketBuilder::finish_and_track did not note the
correct number of in-flight bytes for the congestion controller and
also did not update the
PacketNumberSpace::time_of_last_ack_eliciting_packet. That field in
turn is used by the PTO calculation as time from which to arm the next
PTO timeout.

To fix this we add information about all frames on whether they are
ack-eliciting or not. And if any ack-eliciting frame is written by the
PacketBuilder it keeps track of this internally.
@flub flub marked this pull request as draft April 2, 2026 12:34
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/561/docs/noq/

Last updated: 2026-04-08T16:08:40Z

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Performance Comparison Report

cdce1f8837fffff61503a935e31466b885e987ea - artifacts

Raw Benchmarks (localhost)

Scenario noq upstream Delta CPU (avg/max)
large-single 5644.6 Mbps 8287.8 Mbps -31.9% 96.2% / 107.0%
medium-concurrent 5823.9 Mbps 7987.7 Mbps -27.1% 95.7% / 106.0%
medium-single 4022.1 Mbps 4716.9 Mbps -14.7% 98.4% / 163.0%
small-concurrent 3902.1 Mbps 5296.1 Mbps -26.3% 93.8% / 108.0%
small-single 3635.7 Mbps 4676.4 Mbps -22.3% 93.7% / 108.0%

Netsim Benchmarks (network simulation)

Condition noq upstream Delta
ideal 3184.4 Mbps 4009.6 Mbps -20.6%
lan 782.4 Mbps 810.3 Mbps -3.4%
lossy 69.8 Mbps 55.9 Mbps +25.0%
wan 83.8 Mbps 83.8 Mbps ~0%

Summary

noq is 24.4% slower on average

---
c187201cb12f3629419d35a9cd7ff3d8d94a767b - artifacts

Raw Benchmarks (localhost)

Scenario noq upstream Delta CPU (avg/max)
large-single 5785.7 Mbps 8088.1 Mbps -28.5% 98.6% / 159.0%
medium-concurrent 5715.7 Mbps 7290.7 Mbps -21.6% 97.9% / 159.0%
medium-single 3827.5 Mbps 4625.2 Mbps -17.2% 93.6% / 108.0%
small-concurrent 3889.4 Mbps 5274.4 Mbps -26.3% 95.5% / 109.0%
small-single 3656.9 Mbps 4650.8 Mbps -21.4% 90.5% / 97.9%

Netsim Benchmarks (network simulation)

Condition noq upstream Delta
ideal 3063.1 Mbps 3605.7 Mbps -15.0%
lan 782.0 Mbps 796.4 Mbps -1.8%
lossy 69.8 Mbps 55.9 Mbps +25.0%
wan 83.8 Mbps 83.8 Mbps ~0%

Summary

noq is 22.0% slower on average

---
d8c93053a98715db0206a87615d9451a87931178 - artifacts

No results available

---
545501f11aef8691411f8516cd01febc3e7c1855 - artifacts

Raw Benchmarks (localhost)

Scenario noq upstream Delta CPU (avg/max)
large-single 5416.1 Mbps 7952.1 Mbps -31.9% 90.7% / 96.8%
medium-concurrent 5373.4 Mbps 7839.9 Mbps -31.5% 93.9% / 99.4%
medium-single 4032.2 Mbps 4749.3 Mbps -15.1% 96.0% / 129.0%
small-concurrent 3884.5 Mbps 5338.5 Mbps -27.2% 88.3% / 98.2%
small-single 3427.8 Mbps 4819.3 Mbps -28.9% 92.5% / 101.0%

Netsim Benchmarks (network simulation)

Condition noq upstream Delta
ideal N/A 3922.2 Mbps N/A
lan N/A 803.1 Mbps N/A
lossy N/A 55.9 Mbps N/A
wan N/A 83.8 Mbps N/A

Summary

noq is 27.9% slower on average

@n0bot n0bot bot added this to iroh Apr 2, 2026
@github-project-automation github-project-automation bot moved this to 🚑 Needs Triage in iroh Apr 2, 2026
This is enough to fix the test that was failing. But really #556 also
needs fixing.
@flub flub marked this pull request as ready for review April 2, 2026 17:47
@flub
Copy link
Copy Markdown
Collaborator Author

flub commented Apr 2, 2026

This is ready for review now.

Comment thread noq-proto/src/connection/mod.rs Outdated
Comment thread noq-proto/src/connection/mod.rs Outdated
@dignifiedquire dignifiedquire moved this from 🚑 Needs Triage to 👀 In review in iroh Apr 7, 2026
Comment thread noq-proto/src/frame.rs
Copy link
Copy Markdown
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really happy with the PacketBuilder changes.
Unfortunate that this requires another bugfix (the loss during handshake thing), and that's a lot harder to understand (otherwise I'd have given the 👍 long ago 😅).

Comment thread noq-proto/src/connection/mod.rs
Copy link
Copy Markdown
Collaborator

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge!

@flub flub added this pull request to the merge queue Apr 9, 2026
Merged via the queue into main with commit 1948efc Apr 9, 2026
36 checks passed
@flub flub deleted the flub/pto-always-ack-eliciting branch April 9, 2026 10:47
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in iroh Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

4 participants