feat(proto): replace BBR with BBRv3 + breaking Controller API changes by dignifiedquire · Pull Request #611 · n0-computer/noq

dignifiedquire · 2026-04-23T11:42:37Z

Pulls in upstream quinn PR quinn-rs/quinn#2481

Squashes the upstream commits and adapts them.

Closes #603

Upstream commits

f8b25b24b - granular packet events for congestion controllers
52f04ed9b - Pacer queries Controller for pacing rate / send quantum
c022e308a - BBRv3 implementation + MaxFilter
ce60e5b5c - remove old BBR implementation

Breaking Changes

Controller trait:
- on_ack gains pn: u64
- on_congestion_event gains largest_lost: u64
- New optional on_packet_sent, on_packet_lost, on_ack_frequency_update
- New metrics() -> ControllerMetrics { congestion_window, ssthresh, pacing_rate, send_quantum }, used by the pacer
Pacer changes:
- Pacer::delay() now also takes capacity: Option<u64> and pacing_rate: Option<u64> from the controller; CC-supplied values take precedence over the window-derived defaults but are still capped by noq's max_bytes_per_second.

github-actions · 2026-04-23T11:44:40Z

Performance Comparison Report

c737f72a1d71c2c618e862c2aed345b662d5d5b5 - artifacts

No results available

---

032ebeff96f9ffc35928f877ea49bfdccdc1cd8d - artifacts

No results available

---

8b7b16883846412482caf7e3267f55091451852e - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5525.2 Mbps	8019.0 Mbps	-31.1%	96.7% / 148.0%
medium-concurrent	5484.9 Mbps	7593.0 Mbps	-27.8%	97.0% / 150.0%
medium-single	3807.3 Mbps	4766.9 Mbps	-20.1%	92.6% / 101.0%
small-concurrent	3885.2 Mbps	5450.7 Mbps	-28.7%	94.8% / 102.0%
small-single	3627.8 Mbps	4931.0 Mbps	-26.4%	96.9% / 153.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	N/A	4107.1 Mbps	N/A
lan	N/A	810.4 Mbps	N/A
lossy	N/A	55.9 Mbps	N/A
wan	N/A	83.8 Mbps	N/A

Summary

noq is 27.4% slower on average

---

acc3302245eff6e951f138511f3159216d9e7a88 - artifacts

No results available

---

69c68b87b1db92ea798b96e49bd916afbfddee22 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5671.4 Mbps	7965.0 Mbps	-28.8%	98.3% / 151.0%
medium-concurrent	5471.5 Mbps	7700.6 Mbps	-28.9%	97.8% / 151.0%
medium-single	4344.6 Mbps	4749.7 Mbps	-8.5%	93.2% / 102.0%
small-concurrent	3930.5 Mbps	5335.9 Mbps	-26.3%	96.1% / 103.0%
small-single	3620.8 Mbps	4695.3 Mbps	-22.9%	97.3% / 153.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3114.7 Mbps	N/A	N/A
lan	782.4 Mbps	N/A	N/A
lossy	69.8 Mbps	N/A	N/A
wan	83.8 Mbps	N/A	N/A

Summary

noq is 24.3% slower on average

github-actions · 2026-04-23T11:45:29Z

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

Last updated: 2026-04-24T08:35:31Z

Pulls in upstream quinn PR quinn-rs/quinn#2481 Squashes the four upstream commits and adapts them to noq's multipath divergence Closes 603 Upstream commits - f8b25b24b - granular packet events for congestion controllers - 52f04ed9b - Pacer queries Controller for pacing rate / send quantum - c022e308a - BBRv3 implementation + MaxFilter - ce60e5b5c - remove old BBR implementation Breaking changes to `Controller` trait: - `on_ack` gains `pn: u64` - `on_congestion_event` gains `largest_lost: u64` - New optional `on_packet_sent`, `on_packet_lost`, `on_ack_frequency_update` - New `metrics()` -> `ControllerMetrics { congestion_window, ssthresh, pacing_rate, send_quantum }`, used by the pacer Pacer changes: - `Pacer::delay()` now also takes `capacity: Option<u64>` and `pacing_rate: Option<u64>` from the controller; CC-supplied values take precedence over the window-derived defaults but are still capped by noq's `max_bytes_per_second`. Co-authored-by: Tipuch <fiorini751@proton.me>

flub

Overall LGTM, I'd like to tidy up some variable names mostly. We already started diverging from Quinn on those and would be best to stay consistent. I didn't see any problem wrt multipath or other noq-specific things though.

flub · 2026-04-23T12:49:44Z

-                    self.process_ecn(now, space, path, newly_acked.len() as u64, ecn, sent);
+                    let pn_space = self.spaces[space].for_path(path);
+                    let sent = pn_space.largest_acked_packet_sent;
+                    let largest_sent = pn_space.largest_acked_packet.unwrap();


I'm not particularly happy with this unwrap. IIUC this works because new_largest is true and therefore the code above just inserted it. It should be at least an expect() with a message to that effect. But maybe there's a better way. E.g. new_largest could be an Option<pn> maybe, so that this if becomes an if let Some(pn) (not tried this)?

Aside: these variable names are also pretty bad, I had to look them up to make sense of this. I even came very close to improve them before, but failed to find a good alternative. This time I can come up with a (hopefully) sensible suggestion though:

PacketNumberSpace::largest_acked_packet_pn (was largest_acked_packet)

PacketNumberSpace::largest_acked_packet_send_time (was largest_acked_packet_sent)

But I'm fine if you'd rather not mix that in this PR. OTOH I won't complain if you do.

flub · 2026-04-23T12:52:11Z

        newly_acked: u64,
        ecn: frame::EcnCounts,
        largest_sent_time: Instant,
+        largest_sent: u64,


nit: largest_sent_pn would be better. And, err, I won't mind if you tidy up newly_acked into newly_acked_pn.

flub · 2026-04-23T12:56:34Z

        now: Instant,
        sent: Instant,
        bytes: u64,
+        pn: u64,


Hum, I realise this is probably upstream... but let's improve naming because our experience so far has shown that these names do get confusing and consistency helps. I think @matheus23 advocated for pn before over packet_number and that seems like it would be the right compromise for the variables here. So:

on_sent(..., largest_pn: u64)

on_packet_sent(..., pn: u64)

on_ack(..., pn: u64)

on_congestion_event(..., largest_lost_pn: u64)

on_packet_lost(..., pn: u64, ...)

And also use the same variable names in all the impls.

flub · 2026-04-23T12:58:47Z

    // Not timing-aware, so it's safe to call this for inferred acks, such as arise from
    // high-latency handshakes
-    fn on_packet_acked(&mut self, now: Instant, path_id: PathId, info: SentPacket) {
+    fn on_packet_acked(&mut self, now: Instant, path_id: PathId, packet: u64, info: SentPacket) {


packet -> pn

dignifiedquire force-pushed the bbrv3-from-quinn-2481 branch from c737f72 to 032ebef Compare April 23, 2026 11:44

dignifiedquire requested a review from flub April 23, 2026 12:00

dignifiedquire force-pushed the bbrv3-from-quinn-2481 branch from 032ebef to 8b7b168 Compare April 23, 2026 12:02

n0bot Bot added this to iroh Apr 23, 2026

github-project-automation Bot moved this to 🚑 Needs Triage in iroh Apr 23, 2026

flub requested changes Apr 23, 2026

View reviewed changes

github-project-automation Bot moved this from 🚑 Needs Triage to 🏗 In progress in iroh Apr 23, 2026

dignifiedquire added 3 commits April 24, 2026 10:27

apply CR

c917189

CR: rename spaces fields

acc3302

remove one panic

69c68b8

dignifiedquire requested a review from flub April 24, 2026 08:33

flub approved these changes Apr 24, 2026

View reviewed changes

dignifiedquire added this pull request to the merge queue Apr 24, 2026

Merged via the queue into main with commit 0ac6b46 Apr 24, 2026
37 of 38 checks passed

dignifiedquire deleted the bbrv3-from-quinn-2481 branch April 24, 2026 09:29

github-project-automation Bot moved this from 🏗 In progress to ✅ Done in iroh Apr 24, 2026

flub mentioned this pull request May 16, 2026

fix(bbr3): bbr3 controller was not accounting correctly #657

Closed

4 tasks

Conversation

dignifiedquire commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes

Uh oh!

github-actions Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance Comparison Report

Raw Benchmarks (localhost)

Netsim Benchmarks (network simulation)

Summary

Raw Benchmarks (localhost)

Netsim Benchmarks (network simulation)

Summary

Uh oh!

github-actions Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dignifiedquire commented Apr 23, 2026 •

edited

Loading

github-actions Bot commented Apr 23, 2026 •

edited

Loading

github-actions Bot commented Apr 23, 2026 •

edited

Loading