Conversation
noq#738: on a dual-stack socket, the same real peer/interface can be represented as either plain IPv4 or IPv4-mapped-IPv6, and comparing these representations wrongly reports them as different, breaking multipath path validation (confirmed on real Android hardware with physical Wi-Fi/cellular interfaces). Rather than patching comparison call sites, normalize both remote and local_ip to the connection's established socket family at every point a FourTuple enters Connection-owned state from outside: - noq_proto::Connection::open_path / open_path_ensure normalize the caller-supplied FourTuple before matching or storing it. Path 0 is never touched here, preserving family autodetection -- these are only for additional multipath paths, which by definition come after the connection's family is already established. This also means embedders that call these proto-level APIs directly (bypassing the noq wrapper crate) get the invariant for free. - The noq wrapper's normalize_network_path() now delegates address-family detection to conn.is_ipv6() instead of independently re-deriving it, collapsing two separately-computed notions of the connection's family into one. - noq/src/endpoint.rs normalizes RecvMeta's local_ip to match the decoded remote sockaddr's family before constructing the FourTuple handed to noq-proto, so incoming datagrams are consistent too. is_ipv6() itself is fixed once at Connection::new() from the initial path's remote family (see the field's doc comment for why this must not be re-derived live from current path membership, and the documented limitation around Endpoint::rebind() not being signaled through to noq-proto::Connection). With every entry point normalized, there is no longer any need for comparison-time canonicalization: FourTuple keeps #[derive(Hash, Eq, PartialEq, Copy, Clone)] unchanged (plain structural equality, no risk to downstream consumers like iroh that use FourTuple as a HashMap/HashSet key), and every FourTuple comparison in noq-proto is a plain == with no special-casing.
open_path_normalizes_ipv4_mapped_addrs_to_connection_family: connects over a routing table that only knows the IPv4-mapped-IPv6 form of a second client/server address pair, then opens a path with the plain-IPv4 representation of that same pair. Verified (by temporarily reverting the normalization) that this fails on unfixed code: the test harness's own routing simulation drops every PATH_CHALLENGE with 'no route from client to server', since the plain-IPv4 source/destination doesn't match any route -- the deterministic-test-harness analogue of a real dual-stack socket sending from/to the wrong representation.
… branch Unrelated to noq#738 -- just two import-wrap hunks that cargo fmt --check flagged in files this branch already had open.
5 tasks
I had only run a narrower cargo test/clippy subset before, not the project's actual Makefile.toml dev-flow (format-check, check, clippy, doc, test, proptests-extralight, all workspace-wide with --all-features). Running the real thing surfaced two rustdoc issues the narrower checks missed: - normalize_network_path()'s doc comment linked to Transmit::destination and Transmit::src_ip, but Transmit isn't in scope in this file -- fixed to proto::Transmit::destination / proto::Transmit::src_ip. Also re-ran cargo fmt with this project's actual rustfmt config (comment_width=100, wrap_comments=true, imports_granularity=Crate, from Makefile.toml) instead of plain defaults. This reflowed one over-width comment and, notably, reverted two import-wrap edits from an earlier pass in this branch's history that turned out to be wrong under the project's real config (imports_granularity=Crate wants those collapsed to one line, not wrapped).
Extends open_path_normalizes_ipv4_mapped_addrs_to_connection_family to cover the exact shape divagant-martian asked for on the sibling PR (n0-computer#784): a client address reachable in one representation on the way out but not the other on the way back. Investigated first (see PR comment for the full writeup) whether there's a noq-proto code path where the server independently re-derives a different reply destination than the literal remote it just received -- found none; every send path (build_transmit, PathResponses, migration) uses the stored/observed FourTuple as-is. The actual bug is upstream of that: without this branch's fix, calling open_path/open_path_ensure with a plain-IPv4 FourTuple on an already-IPv6-family connection stores that mismatched representation in PathData.network_path itself, and every later send for that path inherits it. The route table now permits the outbound (client-to-server) leg via the plain-IPv4 representation but only the mapped representation on the return leg. Verified by temporarily reverting the fix: the test fails with 'no route from server to client for packet packet.destination=1.1.1.99:4433' (server tries to reply to the unnormalized plain-IPv4 remote it stored); restoring the fix, the stored path is normalized to the mapped form up front and the reply routes successfully. Raw logs from both runs are in the PR comment.
Author
|
Investigated whether there's a So the closest faithful reproduction I could build: a routing table where the outbound (client→server) leg is reachable via the plain-IPv4 representation, but the return leg only routes via the mapped one. Without the fix, the unnormalized plain-IPv4 path representation gets stored and the server's reply is dropped. With the fix, the stored representation is normalized to mapped up front and the reply succeeds — same routing table both times. Full logs ( Restored-fix run: |
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.
cuzic
added a commit
to cuzic/noq
that referenced
this pull request
Aug 11, 2026
into this PR Adds the remaining piece from the sibling exploration in n0-computer#787: caller- supplied `remote` addresses passed to `open_path`/`open_path_ensure` are now normalized to the connection's established socket family too, the same way `local_ip` already was. This is scoped narrowly, exactly as n0-computer#787 worked out: `Connection::new` (path 0, which establishes the family in the first place), `handle_event`'s incoming datagram arm, and `handle_first_packet` are all left untouched for `remote` -- their remote addresses come from the OS's own recvfrom-equivalent, which for a single bound socket already reports peer addresses in one consistent representation. With every FourTuple that ever enters Connection-owned state now consistently normalized -- both remote (this commit) and local_ip (already normalized at all five entry points) -- structural equality just works everywhere. Delete FourTuple::is_same_remote()/ canonical_remote() entirely and revert every comparison site (early_discard_packet, PATH_CHALLENGE-on-active-path detection, OBSERVED_ADDR matching, the peer-migration trigger, PathResponses::push, is_probably_same_path) to plain ==/!=. There is no comparison-time canonicalization hack left anywhere in noq-proto. Added regression coverage exercised directly through Connection::open_path/open_path_ensure (bypassing the noq wrapper): normalization for both a dual-stack and an IPv4-only connection, and an asymmetric-routing test (ManyToManyRouting with the outbound leg reachable via plain IPv4 but the return leg only via mapped IPv4-in-IPv6) proving this actually closes a real gap -- verified by temporarily reverting the remote normalization and confirming failure first (a path that times out and never validates), then confirming it passes restored. This is the last piece n0-computer#787 explored separately; consolidating it here so n0-computer#784 is the complete fix and n0-computer#787 can close as superseded.
Author
|
Consolidated this into #784 -- the remote-normalization approach explored here (normalize at |
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
1 participant
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
Alternative to #784 for the same issue (#738), for comparison — happy to drop whichever approach you don't prefer.
On a dual-stack socket,
FourTuple { remote, local_ip }can represent the same real peer/interface as either plain IPv4 or IPv4-mapped-IPv6, and comparing these representations wrongly reports them as different, breaking multipath path validation (confirmed on real Android hardware with physical Wi-Fi/cellular interfaces).Per @matheus23's review on #784: rather than patching comparison call sites, this normalizes
remoteandlocal_ipto the connection's established socket family at every point aFourTupleentersConnection-owned state from outside —open_path/open_path_ensure(both proto-level, so embedders bypassing thenoqwrapper get the invariant too) and incoming datagram handling. Path 0 is never touched, so address-family autodetection is preserved.is_ipv6()itself is fixed once atConnection::new()from the initial path's remote family, instead of being re-derived live from current path membership (which could otherwise drift mid-connection and reintroduce a variant of the same bug — see the field's doc comment).With every entry point normalized,
FourTuplekeeps#[derive(Hash, Eq, PartialEq, Copy, Clone)]unchanged and every comparison innoq-protois a plain==— nois_same_remote/is_same_local_ip-style helpers anywhere.Known gap:
Endpoint::rebind()changing the underlying socket's address family mid-connection isn't signaled tonoq-proto::Connectionat all today (itsConnectionEventInnerhas no rebind concept), sois_ipv6()won't adapt to that. Documented on the field rather than silently left unhandled; wiring up a real signal for it felt like a separate, bigger change.Breaking Changes
None.
is_ipv6()onnoq_proto::Connectionis nowpubinstead ofpub(crate)(needed so thenoqwrapper can delegate to it as the single source of truth), which is additive.Notes & open questions
Open to feedback on whether
Endpoint::rebind()signaling intonoq-proto::Connection(the known gap above) should be tackled here or as a separate follow-up.Change checklist
cargo test -p noq-proto(389 passed),cargo test -p noq(33 passed), clippy and fmt clean.