fix: advertise dialable addresses to DHT and harden NAT traversal#71
Closed
mickvandijke wants to merge 1 commit into
Closed
fix: advertise dialable addresses to DHT and harden NAT traversal#71mickvandijke wants to merge 1 commit into
mickvandijke wants to merge 1 commit into
Conversation
Splits the NAT-traversal and dialable-address work from the original PR #70 bundle. Root cause and fix below; other correctness fixes bundled. ## Root cause `DhtNetworkManager::local_dht_node()` built its self-entry from the static `NodeConfig::listen_addrs()` derivation, which returns wildcard IPs (`0.0.0.0` / `::`) and the configured port (`0` for ephemeral binds). Receivers' `dialable_addresses()` filter then rejected the entry, so DHT-based peer discovery returned **zero contactable addresses** for any node. Connections only succeeded via side channels (static bootstrap, in-memory cache, relay fallback) — exactly the sporadic-failure pattern reported by `saorsa-node-lmdb`. ## Fix `local_dht_node()` is now an `async fn` that sources its self-entry addresses from `TransportHandle::observed_external_addresses()` — the post-NAT addresses observed via QUIC `OBSERVED_ADDRESS` frames, one per local interface that has an observation on a multi-homed host. An intermediate iteration added a `UdpSocket::connect`-then- `getsockname` helper (`primary_local_ip`) to substitute wildcard IPs, but it was dropped in a follow-up because it ran blocking syscalls inside an async context and returned RFC1918 addresses behind home NAT; the OBSERVED_ADDRESS-only flow supersedes it and needs no interface probing. If no source produces an address, the published self-entry has an empty `addresses` vec. This is the right answer at the publish layer: better to admit "I don't know how to be reached yet" than to lie with a bind-side wildcard. The empty window closes naturally once the first peer connects and OBSERVED_ADDRESS flows. ## Other NAT fixes bundled - **Event-driven relay/address updates** — relay-established and peer-address-update events drain via `tokio::select!` over `TransportHandle::recv_relay_established()` / `recv_peer_address_update()` instead of the previous 1-second polling loop, eliminating the race window where outbound DHT queries advertised no relay address for up to a second after relay setup. - **Identity exchange timeout bumped to 15 s** — `send_dht_request` computes its identity-exchange timeout as `min(request_timeout, IDENTITY_EXCHANGE_TIMEOUT)`; with the constant at 5 s the effective budget was always 5 s, too tight for cross-region or congested cellular links where the two-RTT exchange plus ML-DSA-65 verification can blow past with retransmits. Bumped to 15 s to match the bootstrap budget. - **Observed-address cache fallback** — saorsa-transport exposes the externally-observed address only via active connections, so a node whose every connection has just dropped has no answer for `observed_external_address()`. This introduces `ObservedAddressCache`: an in-memory, frequency- and recency-aware cache populated by `P2pEvent::ExternalAddressDiscovered` events. When the live source is empty, the cache returns the most-frequently-observed address among recent entries, breaking ties by recency. NAT-rebind handling via a two-pass selection (recent window wins over stale counts). Keyed by `(local_bind, observed)` so multi-homed hosts don't cross-pollute observations from different interfaces. The cache is reset on process restart — a fresh node re-discovers its current address from live connections rather than trusting stale state. - **Bridge task split + bounded forwarder channels** — the DHT bridge previously ran `find_closest_nodes_network` inline inside the select loop on the relay branch, blocking the peer-address-update branch for the duration of the iterative lookup; the underlying forwarder mpsc channels were `unbounded_channel` so the queue could grow without limit during the stall. Detached the lookup + publish into its own spawned task per relay event so the select loop keeps polling, and replaced both forwarder channels with bounded `channel(ADDRESS_EVENT_CHANNEL_CAPACITY)` plus a per-event drop counter. - **`dial_addresses()` helper** — loops through every dialable address until one succeeds. Replaces single-address dial sites in `bootstrap_from_peers`, `spawn_bucket_refresh_task`, `trigger_self_lookup`, `find_closest_nodes_network` parallel-query block, and `send_dht_request` fallback so a stale NAT binding on entry #1 no longer kills the dial when entry #2 would have worked. ## Regression tests `tests/dht_self_advertisement.rs` adds six `#[tokio::test]` cases that pin the published self-entry behaviour: - **Loopback bind path** (single-node, `local: true`): - `local_mode_publishes_dialable_loopback_address` - `local_mode_published_self_entry_matches_runtime_listen_addrs` - `local_mode_never_publishes_port_zero` - **Wildcard / OBSERVED_ADDRESS path** (`local: false`): - `wildcard_bind_with_no_peers_publishes_empty_self_entry` — pins the "don't lie when you don't know" contract. - `wildcard_bind_publishes_observed_address_after_peer_connection` — primary regression test for the OBSERVED_ADDRESS flow. - `observed_address_cache_serves_fallback_after_connection_drop` — pins the cache fallback behaviour after all live connections drop. All six FAIL on `rc-2026.4.1` with concrete evidence (e.g. `[Quic(0.0.0.0:0)]` published as the only address) and PASS here. `ObservedAddressCache` ships with 9 unit tests covering empty, single, repeated, frequency, recency tiebreak, NAT-rebinding fallback, long-quiet fallback, eviction, and re-observation-on-full. ## chore: drop needless struct update in IPDiversityConfig test Both fields of `IPDiversityConfig` are explicitly set, so the trailing `..IPDiversityConfig::default()` triggers `clippy::needless_update` under `cargo clippy --all-targets`. Test code only; no functional change. ## Verification - `cargo build --lib`: clean - `cargo fmt --check`: clean - `cargo clippy --lib -- -D warnings -D clippy::unwrap_used -D clippy::expect_used`: clean - `cargo clippy --all-targets -- -D warnings`: clean - `cargo test --lib`: 282 passed, 0 failed - `cargo test --test dht_self_advertisement`: 6/6 passed - `cargo test --test two_node_messaging`: 7/7 passed - `cargo test --test node_lifecycle`: 17/17 passed This is one of two PRs that split the original PR #70, which bundled this NAT-traversal hardening with two 1000-node scale fixes in the DHT touch path and the inbound message dispatcher. The other PR covers the 1000-node perf work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+448
to
+459
| pub fn observed_external_addresses(&self) -> Vec<SocketAddr> { | ||
| let mut out: Vec<SocketAddr> = self.dual_node.get_observed_external_addresses(); | ||
| let cached = self | ||
| .observed_address_cache | ||
| .lock() | ||
| .most_frequent_recent_per_local_bind(); | ||
| for addr in cached { | ||
| if !out.contains(&addr) { | ||
| out.push(addr); | ||
| } | ||
| } | ||
| out |
There was a problem hiding this comment.
The doc comment says the cache is used "for each (local_bind, observed) partition that has no live observation yet", implying it only fills the gap when a bind has no live data. The implementation deduplicates by address value only — not by local bind — so if a bind currently has a live observation but the cache also holds a stale address for that same bind (e.g. after a NAT rebinding), both addresses are published. Peers reaching dial_addresses will try the stale one and fail, then succeed on the fresh one, so there is no correctness impact. However, the doc and the code should be aligned: if the per-bind fallback intent is correct, the merge loop could skip cache entries whose local bind already has a live entry in out.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/transport_handle.rs
Line: 448-459
Comment:
**Cache merged unconditionally alongside live observations**
The doc comment says the cache is used "for each `(local_bind, observed)` partition that has **no live observation yet**", implying it only fills the gap when a bind has no live data. The implementation deduplicates by address value only — not by local bind — so if a bind currently has a live observation but the cache also holds a stale address for that same bind (e.g. after a NAT rebinding), both addresses are published. Peers reaching `dial_addresses` will try the stale one and fail, then succeed on the fresh one, so there is no correctness impact. However, the doc and the code should be aligned: if the per-bind fallback intent is correct, the merge loop could skip cache entries whose local bind already has a live entry in `out`.
How can I resolve this? If you propose a fix, please make it concise.
3 tasks
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.
Splits the NAT-traversal and dialable-address work out of #70 so it can
be reviewed and merged independently from the two 1000-node perf fixes
in the original bundle.
Summary
DhtNetworkManager::local_dht_node()is now anasync fnthat sourcesits self-entry from
TransportHandle::observed_external_addresses()(QUIC
OBSERVED_ADDRESSframes) instead of the wildcardNodeConfig::listen_addrs()derivation. This fixes the sporadic-failurepattern reported by saorsa-node-lmdb where DHT-based peer discovery
returned zero contactable addresses for any node.
via
tokio::select!over newTransportHandle::recv_*accessorsinstead of the previous 1 s polling loop.
budget) so cross-region and congested cellular links stop timing out
during the ML-DSA-65 handshake.
ObservedAddressCache: in-memory, frequency/recency-aware fallbackfor when every live connection has dropped. Keyed by
(local_bind, observed)so multi-homed hosts don't cross-polluteobservations from different interfaces. Reset on process restart.
longer blocks the peer-address-update branch on
find_closest_nodes_network, and the forwarder mpsc channels are nowbounded with a per-event drop counter.
dial_addresses()helper: loops through every dialable addressuntil one succeeds. Replaces 5 single-address dial sites so a stale NAT
binding on entry style: Fix formatting issues to resolve CI failures #1 no longer kills the dial when entry Comparison between saorsa and libp2p or iroh #2 would have
worked.
clippy::needless_updatefix inIPDiversityConfigtest code.Regression tests
tests/dht_self_advertisement.rsadds six#[tokio::test]cases thatpin the published self-entry behaviour — three on the loopback bind path,
three on the wildcard / OBSERVED_ADDRESS path (including the primary
regression test for the OBSERVED_ADDRESS flow and the cache fallback
after connection drop). All six FAIL on
rc-2026.4.1and PASS here.ObservedAddressCacheships with 9 unit tests covering empty, single,repeated, frequency, recency tiebreak, NAT-rebinding fallback, long-quiet
fallback, eviction, and re-observation-on-full.
Verification
cargo build --lib— cleancargo fmt --check— cleancargo clippy --lib -- -D warnings -D clippy::unwrap_used -D clippy::expect_used— cleancargo clippy --all-targets -- -D warnings— cleancargo test --lib— 282 passed, 0 failedcargo test --test dht_self_advertisement— 6/6 passedcargo test --test two_node_messaging— 7/7 passedcargo test --test node_lifecycle— 17/17 passedSplit from #70
This PR and its sibling replace the original #70, which bundled this
NAT-traversal hardening with two 1000-node scale fixes in the DHT touch
path and the inbound message dispatcher. Byte-for-byte the union of this
PR and the sibling equals the content of #70 (verified by merging the two
split branches and diffing against the original HEAD).
The sibling PR covers the 1000-node perf work.
🤖 Generated with Claude Code
Greptile Summary
This PR fixes the root cause of DHT-based peer discovery returning zero contactable addresses by sourcing
local_dht_node()from QUICOBSERVED_ADDRESSframes and runtime listen addresses rather than the staticNodeConfig::listen_addrs()wildcard derivation. Supporting changes include:ObservedAddressCache(frequency+recency fallback surviving connection drops), bounded mpsc channels replacing unbounded queues, event-driven DHT bridge replacing a 1 s poll loop,dial_addresses()trying every advertised address instead of the first, and the identity-exchange timeout raised to 15 s to absorb cross-region ML-DSA-65 handshake cost.Confidence Score: 5/5
Safe to merge — all remaining findings are P2 style/doc concerns with no correctness impact
No P0/P1 defects found. The core algorithms (OBSERVED_ADDRESS sourcing, ObservedAddressCache frequency+recency selection, dial_addresses fallback loop, bounded mpsc with drop counters) are logically correct. The single P2 finding (doc vs. implementation gap in the cache-merge deduplication in observed_external_addresses()) has no behavioral impact because dial_addresses retries all candidates. The PR ships 6 integration tests + 9 cache unit tests that all pass, and all four CI suites (clippy, fmt, lib tests, integration tests) are confirmed green.
src/transport_handle.rs lines 448–459 (cache-merge doc/behavior gap)
Important Files Changed
Sequence Diagram
sequenceDiagram participant T as saorsa-transport participant C as ObservedAddressCache participant TH as TransportHandle participant DHT as DhtNetworkManager participant Peer as Remote Peer T->>C: ExternalAddressDiscovered(addr)<br/>via forwarder task Note over C: records (local_bind, observed)<br/>frequency + recency T->>TH: RelayEstablished(relay_addr)<br/>bounded mpsc [256] TH-->>DHT: recv_relay_established() DHT->>DHT: tokio::spawn self-lookup task DHT->>Peer: PublishAddress(relay_addr) to K closest T->>TH: PeerAddressUpdated(peer, adv)<br/>bounded mpsc [256] TH-->>DHT: recv_peer_address_update() DHT->>DHT: touch_node_typed(relay address) DHT->>TH: local_dht_node() [async] TH->>T: get_observed_external_addresses() T-->>TH: live observations (per stack) TH->>C: most_frequent_recent_per_local_bind() C-->>TH: cached fallback (deduped) TH-->>DHT: merged address list DHT->>Peer: FIND_NODE response with self-entryPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: advertise dialable addresses to DHT..." | Re-trigger Greptile