fix(proto): Ignore PATH_CIDS_BLOCKED when it refers to abandoned paths by matheus23 · Pull Request #710 · n0-computer/noq

matheus23 · 2026-06-17T11:24:03Z

Description

When receiving a PATH_CIDS_BLOCKED frame we used to check the next_seq value for protocol compliance like so:

if next_seq.0
    > self
        .local_cid_state
        .get(&path_id)
        .map(|cid_state| cid_state.active_seq().1 + 1)
        .unwrap_or_default()
{
    return Err(TransportError::PROTOCOL_VIOLATION(
        "PATH_CIDS_BLOCKED next sequence number larger than in local state",
    ));
}

But the .unwrap_or_default() would fail if we don't actually store path state for the given path. This can occur when the path has already been abandoned and discarded.
Usually, we wouldn't hit this case because the most likely value for next_seq for PATH_CIDS_BLOCKED is 0, unless it is lost and retransmitted.

The added regression test simulates all of these rare circumstances at once:

All PATH_NEW_CONNECTION_ID frames from server to client are delayed such that opening a path on the client side generates a PATH_CIDS_BLOCKED frame
The PATH_CIDS_BLOCKED frame is lost as well to ensure it is resent with a newer next_seq number after the delayed PATH_NEW_CONNECTION_ID frames have come in
The path is abadoned and discarded on both ends before we send the delayed PATH_CIDS_BLOCKED frame triggering the bug

The fix is to correctly check for !self.abandoned_paths.contains(&path_id) in the above condition.

Breaking Changes

None

Notes & open questions

This bug report is what originally prompted this work: n0-computer/iroh#4347

It should now be fixed.

Change checklist

Self-review.
Tests if relevant.
All breaking changes documented.

PATH_CIDS_BLOCKED

github-actions · 2026-06-17T11:25:07Z

Performance Comparison Report

3a75cc5c28828bd3c90ed6443bfd1ee4a6753fd5 - artifacts

No results available

---

d2d97cf5c1a1db7edea7e8e64863e25d5798cf6e - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5280.0 Mbps	7934.9 Mbps	-33.5%	92.2% / 97.3%
medium-concurrent	5334.9 Mbps	7741.9 Mbps	-31.1%	93.8% / 101.0%
medium-single	4026.0 Mbps	4551.9 Mbps	-11.6%	93.0% / 102.0%
small-concurrent	3786.3 Mbps	5292.3 Mbps	-28.5%	97.2% / 155.0%
small-single	3483.4 Mbps	4733.1 Mbps	-26.4%	99.0% / 154.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3058.9 Mbps	4023.0 Mbps	-24.0%
lan	782.4 Mbps	810.3 Mbps	-3.4%
lossy	69.8 Mbps	68.6 Mbps	+1.9%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 26.5% slower on average

---

c49030feba544e762115a262ef73c9d310e07cf2 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5574.8 Mbps	7935.9 Mbps	-29.8%	96.6% / 150.0%
medium-concurrent	5380.2 Mbps	7717.7 Mbps	-30.3%	97.5% / 149.0%
medium-single	3944.9 Mbps	4546.1 Mbps	-13.2%	99.3% / 151.0%
small-concurrent	3932.7 Mbps	5330.1 Mbps	-26.2%	95.4% / 102.0%
small-single	3475.0 Mbps	4710.8 Mbps	-26.2%	94.1% / 102.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3072.7 Mbps	4070.6 Mbps	-24.5%
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 25.4% slower on average

---

a072a93bbbe27e19ee88358e9a3000e00b66659b - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5346.8 Mbps	7819.7 Mbps	-31.6%	93.9% / 100.0%
medium-concurrent	5500.9 Mbps	7917.8 Mbps	-30.5%	94.0% / 99.0%
medium-single	3817.6 Mbps	4749.1 Mbps	-19.6%	92.9% / 101.0%
small-concurrent	3840.3 Mbps	5378.3 Mbps	-28.6%	100.1% / 154.0%
small-single	3445.1 Mbps	4763.4 Mbps	-27.7%	95.8% / 153.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3016.7 Mbps	N/A	N/A
lan	782.4 Mbps	N/A	N/A
lossy	55.9 Mbps	N/A	N/A
wan	83.8 Mbps	N/A	N/A

Summary

noq is 28.3% slower on average

---

a9e57305b9f428fa89107c6237aa8e50795e24e6 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5339.5 Mbps	7946.0 Mbps	-32.8%	96.4% / 147.0%
medium-concurrent	5335.8 Mbps	7719.6 Mbps	-30.9%	96.8% / 149.0%
medium-single	3819.6 Mbps	4596.7 Mbps	-16.9%	90.2% / 99.0%
small-concurrent	3811.8 Mbps	5030.1 Mbps	-24.2%	95.6% / 104.0%
small-single	3432.2 Mbps	4838.5 Mbps	-29.1%	95.2% / 153.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3092.5 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 27.9% slower on average

github-actions · 2026-06-17T11:26:40Z

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

Last updated: 2026-06-18T12:57:12Z

flub

This... made me check how a PATH_CIDS_BLOCKED frame is built and I don't like it (git says I wrote it 😉). Because it is super weird that the retransmit of that frame uses next_seq=1 in your test.

This logic is just wrong, when a retransmit happens you can't just do active_cid_seq + 1. That works for the first time you send the packet but not for a retransmit.

I still think the easiest is to store the next_seq in Restransmits itself. That way we may send a useless frame on retransmit (it may be no longer blocked), but at least we're not sending something wrong. And I don't think the complexity of working out if we're still blocked when (re)transmitting is worth it, which would be another option.

FWIW PATHS_BLOCKED has a similar issue, but that's certainly out of scope so I'll file an issue for that one.

flub · 2026-06-18T11:54:31Z

-                                .get(&path_id)
-                                .map(|cid_state| cid_state.active_seq().1 + 1)
-                                .unwrap_or_default()
+                        if !self.abandoned_paths.contains(&path_id)


why check if the path is abandoned? isn't this check not only for when the path is drained? i.e. when the path is abandoned we still have our local CIDs for this path and we can still check the protocol violation?

What do you think of the current version?

…path-cids-blocked

flub

Yes, this I think is a good fix. Cleanly ignores a late arriving packet that is otherwise entirely legal.

…0-computer#716) ## Description Please merge this after n0-computer#710 Prior to this change, retransmissions of PATHS_BLOCKED and PATH_CIDS_BLOCKED frames would use "up to date" information for the known maximum remote path ID and CID queue expected next seq number respectively. This is weird, because these frames are informational and meant for debugging, and either we don't send them when they're not needed anymore (e.g. between originally trying to send them and retransmission the maximum remote path ID increased or the CID queue active seq number increased. In both of these cases we wouldn't actually be blocked anymore.), or we just retransmit them with with the "outdated" values that we sent them with on the first transmit. This PR decides to go for the latter. This provides the appropriate debugging information to the remote that we were out of path CIDs *at some point in time*, even if that is now obsolete. --- Apart from that this PR also fixes the fact that we never actually queued PATHS_BLOCKED frames in the first place... ## Breaking Changes None. ## Notes & open questions Arguably, we should not send any of these frames and I personally hate them, they haven't really helped me at all and only caused problems so far. The only good reason I can find to keep sending them is that the spec forces the server side to correctly *handle* them (and produce protocol violations if they're sent incorrectly), and without actually sending them, we would never exercise these code paths and not make sure that said server side checks are correct. (Yes I think that's just unnecessary busywork and wasted bytes/datagrams.) I've also added a test to check that we transmit & retransmit PATHS_BLOCKED frames. Unfortunately it's quite hard to check that the data that is actually sent is the same PATHS_BLOCKED frame as originally constructed. Instead I did a manual check by looking at the logs. Please trust me bro. ## Change checklist - [x] Self-review. - [x] Tests if relevant.

matheus23 self-assigned this Jun 17, 2026

test(proto): Write regression test for receiving delayed

d2d97cf

PATH_CIDS_BLOCKED

matheus23 force-pushed the matheus23/regression-path-cids-blocked branch from 3a75cc5 to d2d97cf Compare June 17, 2026 11:24

fix(proto): Ignore PATH_CIDS_BLOCKED referring to abandoned paths

c49030f

matheus23 changed the title ~~test(proto): Write regression test for receiving delayed PATH_CIDS_BLOCKED~~ fix(proto): Ignore PATH_CIDS_BLOCKED when it refers to abandoned paths Jun 17, 2026

matheus23 requested review from divagant-martian and flub June 17, 2026 11:47

matheus23 marked this pull request as ready for review June 17, 2026 11:47

n0bot Bot added this to iroh Jun 17, 2026

github-project-automation Bot moved this to 🚑 Needs Triage in iroh Jun 17, 2026

flub reviewed Jun 17, 2026

View reviewed changes

Comment thread noq-proto/src/tests/multipath.rs

Add a comment for future us

a072a93

matheus23 requested a review from flub June 18, 2026 10:17

flub reviewed Jun 18, 2026

View reviewed changes

matheus23 added 2 commits June 18, 2026 14:54

Still check the PV when the path is abandoned, but not discarded yet.

a0f852f

Merge remote-tracking branch 'origin/main' into matheus23/regression-…

a9e5730

…path-cids-blocked

matheus23 requested a review from flub June 18, 2026 12:55

flub approved these changes Jun 18, 2026

View reviewed changes

matheus23 mentioned this pull request Jun 18, 2026

fix(proto): Use correct info in retransmission of *_BLOCKED frames #716

Merged

2 tasks

matheus23 added this pull request to the merge queue Jun 18, 2026

Merged via the queue into main with commit 8ca5916 Jun 18, 2026
39 checks passed

matheus23 deleted the matheus23/regression-path-cids-blocked branch June 18, 2026 16:34

github-project-automation Bot moved this from 🚑 Needs Triage to ✅ Done in iroh Jun 18, 2026

Uh oh!

Conversation

matheus23 commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Breaking Changes

Notes & open questions

Change checklist

Uh oh!

github-actions Bot commented Jun 17, 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

Raw Benchmarks (localhost)

Netsim Benchmarks (network simulation)

Summary

Uh oh!

github-actions Bot commented Jun 17, 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!

flub Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

matheus23 Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matheus23 commented Jun 17, 2026 •

edited

Loading

github-actions Bot commented Jun 17, 2026 •

edited

Loading

github-actions Bot commented Jun 17, 2026 •

edited

Loading