feat(proto): Send PATH_ABANDON on the abandoned path itself - #509
Merged
Merged
Conversation
so let's check in totally broken code.
This stopped us from going to the next space to coalesce the Initial with the Handshake packet on the server-side. And probably lots of other stuff
Turns out this was exactly the same case as SendableFrames::space_id_only.
Slightly sad to have to make an explicit exception for this, at least currently.
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
Two bits of PR review: - Move the scheduling to a separate function. - Avoid an allocation, but this has some tradeoffs. - I now have two loops that look for the PathId by doing `next_path_id = self.path.keys().find(|i| **i > path_id).copied();`. It might be possible to fold the MTU discovery in the main poll loop, MTU packets would get a slightly higher priority but probably not really harmful overall. - I now need to do the computation for `have_validated_status_available_space many more times. I'm not sure how much the compiler manages to remove all of that. Is it smart enough to figure out that `have_validate_status_available_space` won't change between the calls and does it move it out? Does it make the iteration as fast as the previous version? On the other hand, we now have some situations where we don't have to compute the scheduling information, and no longer need to compute it for all paths if we don't send on the last path. What do you think, which version is better (though I also adopted @matheus23's feedback about splitting it off to a function, but that doesn't affect this really. It does make the diff a little bit more though)? As an aside, in working out of how scheduling should work it was really helpful to have to extremely explicit as a bunch of data that's computed up-front. But it's fair that now we know this is how it should work that we can implement it in the most optimal way.
The ordering of these frames was a bit too eager. REACH_OUT is important timing-wise. But it is not more important than HANDSHAKE_DONE, PING, IMMEDIATE_ACK, ACK, ACK_FREQUENCY and should anyway not be sent on a path that also needs PATH_CHALLENGE. PATH_RESPONSE could be the one exception but it is also small. If one of those frames do end up in the same packet as REACH_OUT there will still be place for the REACH_OUT frame. The CRYPTO frame is left after it, because after the handshake that is only carrying auxiliarry non-time-sensitive information, and REACH_OUT is also only possible in the data space kind. OBSERVED_ADDRESS is definitely not that high priority, it might need to move back even further.
If there is no other validated path then we allow sending PATH_ABANDON on the path to be abandoned itself.
flub
marked this pull request as draft
March 16, 2026 18:09
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/509/docs/noq/ Last updated: 2026-03-20T11:28:31Z |
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5515.0 Mbps | 7948.2 Mbps | -30.6% | 99.0% / 181.0% |
| medium-concurrent | 5476.3 Mbps | 7589.6 Mbps | -27.8% | 95.3% / 122.0% |
| medium-single | 3927.6 Mbps | 4666.3 Mbps | -15.8% | 92.5% / 101.0% |
| small-concurrent | 3853.8 Mbps | 5083.8 Mbps | -24.2% | 91.9% / 99.4% |
| small-single | 3578.1 Mbps | 4674.3 Mbps | -23.5% | 96.4% / 125.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2939.4 Mbps | N/A | N/A |
| lan | 782.5 Mbps | N/A | N/A |
| lossy | 69.8 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 25.4% slower on average
be2840d430ab339b550748db568e3f49807fbc08 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5436.3 Mbps | 8001.7 Mbps | -32.1% | 96.5% / 126.0% |
| medium-concurrent | 5746.3 Mbps | 7842.9 Mbps | -26.7% | 95.1% / 126.0% |
| medium-single | 4408.2 Mbps | 4749.1 Mbps | -7.2% | 88.2% / 96.8% |
| small-concurrent | 3892.6 Mbps | 5308.1 Mbps | -26.7% | 87.1% / 97.0% |
| small-single | 3493.1 Mbps | 4770.8 Mbps | -26.8% | 89.1% / 97.2% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2919.3 Mbps | N/A | N/A |
| lan | 782.5 Mbps | N/A | N/A |
| lossy | 69.8 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 25.1% slower on average
c4501fbcb6135189b4e1abf41f982365f71839f1 - artifacts
No results available
c7296ce54c106c8808afeac611b16ffb9a722269 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5635.8 Mbps | 7885.3 Mbps | -28.5% | 94.5% / 108.0% |
| medium-concurrent | 5389.8 Mbps | 7979.1 Mbps | -32.5% | 95.2% / 108.0% |
| medium-single | 3707.3 Mbps | 4693.0 Mbps | -21.0% | 95.3% / 108.0% |
| small-concurrent | 3797.9 Mbps | 5278.0 Mbps | -28.0% | 100.4% / 166.0% |
| small-single | 3550.8 Mbps | 4767.0 Mbps | -25.5% | 92.9% / 111.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3184.5 Mbps | 3939.2 Mbps | -19.2% |
| lan | 796.4 Mbps | 810.3 Mbps | -1.7% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 26.1% slower on average
dignifiedquire
added a commit
that referenced
this pull request
Mar 18, 2026
PR #444 (packet scheduling): verify unvalidated paths don't carry stream data — only PATH_CHALLENGE/RESPONSE. Stream data should flow exclusively on validated Available paths. PR #509 (PATH_ABANDON on self): verify PATH_ABANDON is sent on the abandoned path itself when no other validated path exists. Also verify PATH_ABANDON is delivered to remote after network change replaces the only validated path.
flub
marked this pull request as ready for review
March 20, 2026 11:23
matheus23
approved these changes
Mar 20, 2026
dignifiedquire
approved these changes
Mar 20, 2026
cuzic
added a commit
to cuzic/noq
that referenced
this pull request
Aug 11, 2026
Compared against how the pre-existing codebase actually references issues in comments (e.g. 'PATH_ABANDON on the abandoned path itself when no other path exists (n0-computer#509).', 'Recover storage from these by compacting (n0-computer#700)') -- the convention is a bare '(#NNN)' at the end of the relevant sentence, not a 'noqNNN:' prefix at the start. Reworded every doc/comment this PR chain added that used the latter style to match. Also fixed two doc comments that still referenced a hypothetical 'PathData network_path/transmit_path split' follow-up instead of the actual n0-computer#787 that now exists, and a stray duplicated blank doc line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
If there is no other validated path then we allow sending PATH_ABANDON
on the path to be abandoned itself.
Closes #399.
Breaking Changes
n/a
Notes & open questions
Please review the logic changes carefully, I had to modify all
conditions and we need to be sure I only added a condition without
modifying previous logic.
I have removed the test because this is blocking other work. I will
PR the test separately. The test succeeds by checking the logs
manually, so the change is correct. There are however changes needed
to be able to assert that in the test.