fix(qnt): distinguish may_probe from may_migrate#660
Conversation
The QNT impl did re-use may_migrate to see on whether packets are allowed from a remote. But probing packets do not trigger migration and the two need to be treated separately. Fixes #654.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/660/docs/noq/ Last updated: 2026-05-19T12:53:17Z |
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5643.3 Mbps | 7845.3 Mbps | -28.1% | 97.0% / 98.7% |
| medium-concurrent | 5461.8 Mbps | 7929.7 Mbps | -31.1% | 96.6% / 98.0% |
| medium-single | 3740.1 Mbps | 4719.6 Mbps | -20.8% | 96.2% / 98.5% |
| small-concurrent | 3817.5 Mbps | 5365.6 Mbps | -28.9% | 98.0% / 100.0% |
| small-single | 3600.7 Mbps | 4847.6 Mbps | -25.7% | 96.2% / 98.5% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3060.8 Mbps | 4092.7 Mbps | -25.2% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 26.6% slower on average
| if let Some(hs) = self.state.as_handshake() { | ||
| hs.allow_server_migration | ||
| } else { | ||
| self.n0_nat_traversal.is_negotiated() && self.is_handshake_confirmed() |
There was a problem hiding this comment.
So I'd like to replace this line with false to fix the fact that clients aren't supposed to run migrate(), i.e. servers aren't allowed to migrate and we should not start doing the whole current and previous PathData dance in the client's PathState.
This would fix the slew of rare proptest failures we were generating in the last month (all of the ones that contain two PassiveMigrations): They were all exercising a case where both the client and server would migrate at the same time and then get stuck in a loop.
There was a problem hiding this comment.
Sounds good. Does that mean this fix is independent of this PR? That would mean you don't have to wait on this getting ready enough.
…low probing (n0-computer#663) ## Description Subsumes n0-computer#660 Fixes n0-computer#654 - Separates `peer_may_probe` from `peer_may_migrate` (taken from n0-computer#660 ) - Adds a test that failed before the above change was applied - Makes `peer_may_migrate` return `false` for the client side to avoid triggering migration probing on clients - Adds a proptest regression we've observed a bunch of times that reproduced the above issue but can't be fixed without also depending on n0-computer#660 without breaking other tests. ## Change checklist <!-- Remove any that are not relevant. --> - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [x] Tests if relevant. --------- Co-authored-by: Floris Bruynooghe <flub@n0.computer>
2 participants
Description
The QNT impl did re-use may_migrate to see on whether packets are allowed from a remote. But probing packets do not trigger migration and the two need to be treated separately.
Fixes #654.
Breaking Changes
n/a
Notes & open questions
This is an early draft, I need to properly sanity check. And server/client migration may be involved.
Change checklist