refactor(proto): Move PathStatus state to the PacketNumberSpace by flub · Pull Request #780 · n0-computer/noq

flub · 2026-07-30T14:53:53Z

Description

The path status belongs to the number space, not to a specific path
generation.

Part of #591.

Breaking Changes

n/a

Notes & open questions

Some invariants just show again how the current structure is badly
done. But the fallback with unwrap_or_default for PathStatus works
fine.
Also fixes an event being sent for an unknown path, that seems like
a weird choice.

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
This PR was created by a human that thought critically about the
proposed change and wrote an as clear and concise description as
they could.
This PR isn't slop, and is carefully crafted to do have the
intented effect.
cargo make passes locally.

The path status belongs to the number space, not to a specific path generation. Part of #591.

github-actions · 2026-07-30T14:55:49Z

Performance Comparison Report

a87bded9b85a1e5319840c00a5a1884a64e29601 - artifacts

No results available

---

835487aadb1030c1e2ea04fb4c0b7a1b79f151e6 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5347.2 Mbps	8101.9 Mbps	-34.0%	94.7% / 99.0%
medium-concurrent	5258.7 Mbps	7553.1 Mbps	-30.4%	96.8% / 148.0%
medium-single	4025.2 Mbps	4524.8 Mbps	-11.0%	96.7% / 150.0%
small-concurrent	3877.4 Mbps	5328.3 Mbps	-27.2%	93.1% / 102.0%
small-single	3472.1 Mbps	4692.7 Mbps	-26.0%	93.1% / 102.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	2988.7 Mbps	4011.8 Mbps	-25.5%
lan	782.4 Mbps	810.3 Mbps	-3.4%
lossy	69.8 Mbps	59.7 Mbps	+17.0%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 26.3% slower on average

---

f5cc4cb51d8c2df2bd9b47019a68b4d3ca17dda5 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5351.5 Mbps	7989.8 Mbps	-33.0%	98.5% / 150.0%
medium-concurrent	5315.1 Mbps	7740.6 Mbps	-31.3%	92.2% / 97.3%
medium-single	3814.7 Mbps	4470.6 Mbps	-14.7%	92.1% / 102.0%
small-concurrent	3737.3 Mbps	5039.2 Mbps	-25.8%	96.2% / 105.0%
small-single	3426.1 Mbps	4584.6 Mbps	-25.3%	94.5% / 104.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3030.9 Mbps	4021.7 Mbps	-24.6%
lan	782.5 Mbps	810.4 Mbps	-3.4%
lossy	69.8 Mbps	55.9 Mbps	+25.0%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 26.4% slower on average

---

f74477cfd8547967a62c0a642d80754f49945388 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5274.0 Mbps	7838.1 Mbps	-32.7%	92.2% / 97.5%
medium-concurrent	5274.7 Mbps	7975.7 Mbps	-33.9%	94.2% / 99.3%
medium-single	4034.2 Mbps	4749.1 Mbps	-15.1%	93.8% / 102.0%
small-concurrent	3766.0 Mbps	5387.8 Mbps	-30.1%	98.9% / 154.0%
small-single	3468.5 Mbps	4701.7 Mbps	-26.2%	88.9% / 97.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3030.8 Mbps	4024.7 Mbps	-24.7%
lan	782.4 Mbps	824.3 Mbps	-5.1%
lossy	69.8 Mbps	69.9 Mbps	~0%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 27.7% slower on average

github-actions · 2026-07-30T14:56:16Z

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

Last updated: 2026-08-18T11:36:10Z

flub · 2026-07-30T14:59:57Z

The macos failure is not due to this PR I think, it is flaky... which is very interesting. I'll investigate but let it not bother you for the review. The esp32 failure is also something external.

matheus23

Yeah I think this is correct, and the PR looks good besides a nit.

That said, I still really dislike how this is change is entirely inconsequential AFAIU, because path status frames are useless 🙃

matheus23 · 2026-08-03T08:39:58Z

+                // pns can never be None, that would be a logical error.
+                let path_status = self.spaces[SpaceKind::Data]
+                    .number_spaces
+                    .get(path_id)
+                    .map(|pns| pns.local_status())
+                    .unwrap_or_default();


Are we sure we want to use unwrap_or_default() here instead of .expect()?
Just want to note this - I personally slightly lean in favor of failing loudly when invariants are broken, but I'll defer the final decision to you.

I changed this one by changing the function to iterate over the packet number spaces. Which I think is more correct.

matheus23 · 2026-08-03T08:40:16Z

+        // pns can never be None here, that would be a logical error.
+        let pns = self.spaces[SpaceKind::Data].number_spaces.get(&path_id);
+        let status = pns.map(|pns| pns.local_status()).unwrap_or_default();


matheus23 · 2026-08-03T08:40:37Z

                && !self.abandoned_paths.contains(path_id)
                && path.data.validated
-                && path.data.local_status() == PathStatus::Available
+                && pns.map(|pns| pns.local_status()).unwrap_or_default() == PathStatus::Available


And here. (And probably more of those above)

divagant-martian

The path status belongs to the number space, not to a specific path
generation.

Not really, the packet number space is indexed as Initial, Handshake, Data(PathId) conceptually at least. So now PathId::Zero might have, based on code structure, three different path statuses: one for initial, one for handshake, one in the data space. So this change isn't exactly enforcing an invariant. It's changing a bad place for another bad place.

And it's not a free change either, as now site calls that used to be a single hashmap lookup are now two. For example:

        let have_validated_status_available_space = self.paths.iter().any(|(path_id, path)| {
            // pns can never be None here, that would be a logical error.
            let pns = self.spaces[SpaceKind::Data].number_spaces.get(path_id);

If the point it to remove it from path generation the place to put it would be the PathState (PathData's parent), not PacketNumerSpace

flub · 2026-08-18T10:00:57Z

Not really, the packet number space is indexed as Initial, Handshake, Data(PathId) conceptually at least. So now PathId::Zero might have, based on code structure, three different path statuses: one for initial, one for handshake, one in the data space. So this change isn't exactly enforcing an invariant. It's changing a bad place for another bad place.

Initial and Handshake space can never carry any multipath packets. I think currently it is best to add multipath-specific fields to the PacketNumberSpace and not use them in the early spaces. That is what I was currently pursuing in #591. The other alternative is to have different types of PacketNumberSpace for the Initial+Handshake and Data(PathId) spaces, but I fear that is too big a step.

Also bear in mind that those fields are also unused when multipath is not negotiated. And that will be the case wherever we put them. So I don't think having a field unused in some cases is that bad of a choice.

And it's not a free change either, as now site calls that used to be a single hashmap lookup are now two.

Yes, I think the storage of PacketNumberSpaces being inside the PacketSpace is a mistake. I would like to move PacketNumberSpaces up to the connection, and eventually maybe outside of once we end up doing this refactor, we've long talked about it.

If the point it to remove it from path generation the place to put it would be the PathState (PathData's parent), not PacketNumerSpace

As you say, conceptually this belongs to all Data(PathId) packet number spaces. So it belongs to a field for those structs, but that struct is currently shared with the Initial and Handshake space.

I don't think PathState is the right place to start moving things into either. I've even been wondering if PathState can disappear entirely: we can store PacketNumberSpace::current_path and PacketNumberSpace::previous_path as u64 types which are indexes into a map-like structure of PathData or something which itself is also stored on Connection. But this isn't fully thought through yet, especially the invariants of which spaces can share PathData and which not would still not be enforced. Perhaps some other kind of pointer would make more sense, or not even any pointer if possible with in-place data somehow.

flub · 2026-08-18T10:04:23Z

I've updated the comment to make it clearer that the PacketNumberSpace::status field is unused for some spaces and when multipath is not negotiated.

I don't really like this per-se, but this changes the iteration to find all paths to be over the packet number spaces, which *is* more correct I think.

flub added 3 commits July 30, 2026 16:48

refactor(proto): Move PathStatus state to the PacketNumberSpace

a87bded

The path status belongs to the number space, not to a specific path generation. Part of #591.

clippy

7b9e0ce

format

835487a

flub requested review from divagant-martian and matheus23 and removed request for divagant-martian July 30, 2026 14:58

n0bot Bot added this to iroh Jul 30, 2026

github-project-automation Bot moved this to 🚑 Needs Triage in iroh Jul 30, 2026

matheus23 approved these changes Aug 3, 2026

View reviewed changes

divagant-martian requested changes Aug 3, 2026

View reviewed changes

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

Expand the comment a bit

f5cc4cb

more expects

f74477c

I don't really like this per-se, but this changes the iteration to find all paths to be over the packet number spaces, which *is* more correct I think.

Conversation

flub commented Jul 30, 2026

Description

Breaking Changes

Notes & open questions

Change checklist

Uh oh!

github-actions Bot commented Jul 30, 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

Raw Benchmarks (localhost)

Netsim Benchmarks (network simulation)

Summary

Uh oh!

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

Uh oh!

Uh oh!

flub commented Jul 30, 2026

Uh oh!

matheus23 left a comment

Choose a reason for hiding this comment

Uh oh!

matheus23 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

flub Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

matheus23 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

matheus23 Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

divagant-martian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub commented Aug 18, 2026

Uh oh!

flub commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 30, 2026 •

edited

Loading

github-actions Bot commented Jul 30, 2026 •

edited

Loading

divagant-martian left a comment •

edited

Loading