fix: use ObservedAddr newtype for NAT routing #2172

sanity · 2025-11-29T15:21:23Z

Problem

Peers behind NAT embed incorrect addresses (e.g., 127.0.0.1:31337) in protocol messages. When responses like ReturnSub need to be routed back, they previously used these embedded addresses instead of the transport-layer observed address, causing messages to be sent to the wrong destination.

Why existing tests didn't catch this

Unit tests and in-memory network tests use localhost addresses that work correctly. The issue only manifests with real NAT traversal where the embedded and observed addresses differ.

Previous Approaches

PR #2167 introduced threading source_addr through operations using raw SocketAddr. Nacho's feedback: "Don't use SocketAddr directly anywhere in the codebase for other things than connecting to the actual socket."

This Solution

Introduces ObservedAddr newtype to wrap transport-layer addresses, making the address semantics explicit at the type level:

Embedded address: What the peer claims (wrong behind NAT)
Observed address: What we see at transport layer (correct for NAT)

Key changes:

transport/mod.rs: Add ObservedAddr newtype with explicit semantics
subscribe.rs: Store upstream_addr: Option<ObservedAddr>, use it for ReturnSub routing
seeding.rs: add_subscriber() takes Option<ObservedAddr> to store correct routing address
ring/mod.rs: Updated wrapper to use ObservedAddr

The conversion back to raw SocketAddr happens at the transport boundary in build_op_result().

Testing

Unit Tests

All subscribe unit tests pass
All connect unit tests pass (fixed peer() calls on Unknown addresses in logging)

Docker NAT Integration Test

minimal_nat_test PASSED: PUT from NAT peer, GET from gateway, state verification

River End-to-End NAT Test

Full River message flow test with 6 Docker NAT peers:

FREENET_CORE_PATH=/path/to/this-branch \
FREENET_TEST_DOCKER_NAT=1 \
cargo test --test message_flow river_message_flow_over_freenet_six_peers_five_rounds

Results:

Network: 6 peers behind simulated NAT (Docker iptables MASQUERADE), 1 gateway
Room create: 1.11s (first attempt)
Invite accept: 85.80ms (first attempt)
Messages sent: 10/10 delivered
Message latency: avg=1478ms, min=214ms, max=2711ms
Verification: Each user (on different peers) confirmed receipt of all messages

This confirms that contract updates (River room messages) propagate correctly across NAT'd peers using the full Freenet subscription/update flow.

Stacked On

This PR is stacked on:

fix: use upstream_addr for subscribe operation NAT routing #2171: fix: use upstream_addr for subscribe operation NAT routing
refactor(connect): remove from/sender fields from ConnectMsg #2169: refactor(connect): remove from/sender fields from ConnectMsg
refactor(routing): add upstream_addr for connection-based routing #2167: refactor(routing): add upstream_addr for connection-based routing

Fixes

Addresses #2164

[AI-assisted - Claude]

Copilot

Pull request overview

This PR introduces the ObservedAddr newtype to fix NAT routing issues where peers behind NAT embed incorrect addresses in protocol messages. The newtype wraps transport-layer addresses and makes address semantics explicit, preventing confusion between embedded (potentially wrong) and observed (correct) addresses.

Key changes:

Introduced ObservedAddr newtype to wrap transport-layer socket addresses with explicit semantics
Updated subscription logic to store and use ObservedAddr for NAT-friendly routing
Modified add_subscriber() API to accept Option<ObservedAddr> parameter for correct address resolution

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`crates/core/src/transport/mod.rs`	Adds `ObservedAddr` newtype with Display and From trait implementations to represent transport-layer observed addresses
`crates/core/src/ring/seeding.rs`	Updates `add_subscriber()` to accept `Option<ObservedAddr>` parameter and use it to replace embedded subscriber addresses when available
`crates/core/src/ring/mod.rs`	Updates wrapper method signature to pass through `ObservedAddr` parameter to seeding manager
`crates/core/src/operations/update.rs`	Passes `None` for `upstream_addr` when subscribing to contracts locally (self-subscription)
`crates/core/src/operations/subscribe.rs`	Changes `upstream_addr` field type to `Option<ObservedAddr>`, updates all `add_subscriber()` calls to pass appropriate address values, and adds debug logging for address tracking
`crates/core/src/node/testing_impl/in_memory.rs`	Updates test setup to pass `None` for `upstream_addr` in test subscriber initialization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-29T19:23:17Z

crates/core/src/operations/subscribe.rs

+                    // ALWAYS use the transport-level source address when available.
+                    // This is critical for NAT peers: they may embed a "known" but wrong address
+                    // (e.g., 127.0.0.1:31337 for loopback). The transport address is the only
+                    // reliable way to route responses back through the NAT.
                    let mut subscriber = subscriber.clone();
-                    if subscriber.peer_addr.is_unknown() {
-                        if let Some(addr) = source_addr {
-                            subscriber.set_addr(addr);
-                            tracing::debug!(
-                                tx = %id,
-                                %key,
-                                subscriber_addr = %addr,
-                                "subscribe: filled subscriber address from source_addr"
-                            );
-                        }
+                    if let Some(addr) = source_addr {
+                        subscriber.set_addr(addr);
+                        tracing::debug!(
+                            tx = %id,
+                            %key,
+                            subscriber_addr = %addr,
+                            "subscribe: using transport source_addr for subscriber"
+                        );
                    }


The address modification here is redundant. The subscriber.set_addr(addr) call on line 422 updates the subscriber's address, but then add_subscriber() (called at line 466) receives self.upstream_addr as a separate parameter and creates a new PeerKeyLocation with that address (see seeding.rs line 118-122), discarding this modification.

Since add_subscriber() handles the address replacement internally when upstream_addr is provided, this direct modification is unnecessary and creates confusion about where the address is actually being set. Consider removing the subscriber.set_addr(addr) call and relying solely on the upstream_addr parameter.

The set_addr call does appear redundant since add_subscriber() replaces the address anyway. However, it's useful for the debug log immediately after (line 423-428) which shows the transport source address - this helps with debugging NAT routing issues.

That said, I could simplify by logging source_addr directly rather than modifying subscriber first. Will leave as-is for now since the behavior is correct, but marking as a future cleanup opportunity.

[AI-assisted - Claude]

If the fgoal is to debug, just debug, dont do side effects just for debugging. (!)

Copilot · 2025-11-29T19:23:17Z

crates/core/src/operations/subscribe.rs

+                tracing::info!(
+                    tx = %id,
+                    ?source_addr,
+                    "subscribe: load_or_init creating new op with source_addr as upstream_addr"
+                );


This debug log should use tracing::debug! instead of tracing::info!. The message "subscribe: load_or_init creating new op with source_addr as upstream_addr" is implementation-level detail useful for debugging, not a significant event that should be logged at info level. Other similar operational details in this file use debug! (e.g., lines 158, 197, 431).

Fixed - changed to debug! in a0196fe.

[AI-assisted - Claude]

Copilot · 2025-11-29T19:23:18Z

crates/core/src/operations/subscribe.rs

+                        tracing::info!(
+                            tx = %id,
+                            %key,
+                            upstream_addr = ?self.upstream_addr,
+                            "subscribe: creating ReturnSub with upstream_addr"
+                        );


This debug log should use tracing::debug! instead of tracing::info!. The message "subscribe: creating ReturnSub with upstream_addr" is implementation-level detail useful for debugging, not a significant event that should be logged at info level. Other similar operational details in this file use debug! (e.g., lines 423, 431).

Fixed - changed to debug! in a0196fe.

[AI-assisted - Claude]

Copilot · 2025-11-29T19:23:18Z

crates/core/src/operations/subscribe.rs

+    tracing::info!(
+        tx = %id,
+        msg_type = ?msg.as_ref().map(|m| std::any::type_name_of_val(m)),
+        ?upstream_addr,
+        ?target_addr,
+        "build_op_result: computed target_addr"
+    );


This debug log should use tracing::debug! instead of tracing::info!. The message "build_op_result: computed target_addr" is implementation-level detail useful for debugging, not a significant event that should be logged at info level. This function is called for every operation result and would generate excessive info-level logs.

Fixed - changed to debug! in a0196fe.

[AI-assisted - Claude]

- Line 748 (SeekNode): Pass None instead of self.upstream_addr because subscriber address was already corrected by Gateway. Using upstream_addr would overwrite with the forwarder's address. - Line 932 (ReturnSub): Pass None instead of self.upstream_addr because sender was looked up from source_addr and already has correct address. Using upstream_addr would use the original requester's address instead of the provider's. Addresses Claude code review feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

- Remove duplicate get_peer_location_by_addr functions - Fix socket_addr() calls on SocketAddr (use dereference instead) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

The acceptor's PeerAddr is intentionally Unknown (NAT scenario) since acceptors don't know their external address until the first recipient fills it in from the packet source. Changed tracing statements to use pub_key() instead of peer() to avoid panics on unknown addresses. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T05:26:20Z

crates/core/src/operations/get.rs

+                    let sender = sender_from_addr.clone().expect(
+                        "ReturnGet requires sender lookup from connection - source_addr should resolve to known peer",
+                    );


This changes error handling from graceful degradation to a panic. Previously, if sender_from_addr lookup failed, the code would log a warning and return an error, allowing the node to continue. Now it uses expect() which will panic and crash the node if the lookup fails. The previous error handling approach was better. Consider restoring the if let Some(sender) = sender_from_addr.clone() else { ... } pattern or using .ok_or_else() with proper error propagation.

Suggested change

let sender = sender_from_addr.clone().expect(

"ReturnGet requires sender lookup from connection - source_addr should resolve to known peer",

);

let sender = if let Some(sender) = sender_from_addr.clone() {

sender

} else {

tracing::warn!(

tx = %id,

%key,

"ReturnGet: Could not resolve sender from address; aborting operation"

);

return Err(ErrorKind::NodeError(

"ReturnGet requires sender lookup from connection - source_addr could not be resolved to known peer".into(),

).into());

};

+1 remove the unwrap

Copilot · 2025-12-01T05:26:20Z

crates/core/src/operations/subscribe.rs

+                            return Ok(OperationResult {
+                                target_addr: return_msg.target_addr(),
+                                return_msg: Some(NetMessage::from(return_msg)),
+                                state: None,
+                            });


The error path for max subscribers creates a ReturnSub but uses return_msg.target_addr() for routing, which extracts the address from subscriber. This is inconsistent with the success path (lines 540-544) which uses self.upstream_addr for routing. For NAT peers, subscriber may contain an incorrect embedded address (e.g., 127.0.0.1:31337), so this should use build_op_result(self.id, None, Some(return_msg), self.upstream_addr) instead to ensure proper NAT routing.

Suggested change

return Ok(OperationResult {

target_addr: return_msg.target_addr(),

return_msg: Some(NetMessage::from(return_msg)),

state: None,

});

return build_op_result(self.id, None, Some(return_msg), self.upstream_addr);

Copilot · 2025-12-01T05:26:21Z

crates/core/src/operations/subscribe.rs

+                        OperationResult {
+                            target_addr: return_msg.target_addr(),
+                            return_msg: Some(NetMessage::from(return_msg)),
+                            state: None,
+                        }


This change removes the use of upstream_addr for routing ReturnSub responses in SeekNode error handling. Previously, the code captured self.upstream_addr and used build_op_result to route responses correctly for NAT scenarios. Now it directly constructs OperationResult using return_msg.target_addr(), which extracts the address from the subscriber field. This creates the same NAT routing issue that this PR is supposed to fix: for NAT peers, subscriber may contain an incorrect embedded address. The closure should be restored to use: build_op_result(*id, None, Some(return_msg), self.upstream_addr).

Suggested change

OperationResult {

target_addr: return_msg.target_addr(),

return_msg: Some(NetMessage::from(return_msg)),

state: None,

}

build_op_result(*id, None, Some(return_msg), self.upstream_addr)

Copilot · 2025-12-01T05:26:21Z

crates/core/src/operations/connect.rs

+                        if let Some(upstream) = &source_addr {
+                            network_bridge
+                                .send(*upstream, NetMessage::V1(NetMessageV1::Connect(msg)))
+                                .await?;
+                        }


The added null check for source_addr is redundant and uses the wrong variable. Line 817 already unwraps source_addr into upstream_addr, so if we reach this point, source_addr is guaranteed to be Some. The code should use upstream_addr directly (which is a SocketAddr, not an Option) instead of re-checking source_addr: network_bridge.send(upstream_addr, NetMessage::V1(NetMessageV1::Connect(msg))).await?;

Suggested change

if let Some(upstream) = &source_addr {

network_bridge

.send(*upstream, NetMessage::V1(NetMessageV1::Connect(msg)))

.await?;

}

network_bridge

.send(upstream_addr, NetMessage::V1(NetMessageV1::Connect(msg)))

.await?;

Copilot · 2025-12-01T05:26:21Z

crates/core/src/operations/connect.rs

+                        if let Some(upstream) = &source_addr {
+                            network_bridge
+                                .send(
+                                    *upstream,
+                                    NetMessage::V1(NetMessageV1::Connect(response_msg)),
+                                )
+                                .await?;
+                        }


Same issue as above: the null check for source_addr is redundant and uses the wrong variable. Line 817 already unwraps source_addr into upstream_addr, so the code should use upstream_addr directly: network_bridge.send(upstream_addr, NetMessage::V1(NetMessageV1::Connect(response_msg))).await?;

Suggested change

if let Some(upstream) = &source_addr {

network_bridge

.send(

*upstream,

NetMessage::V1(NetMessageV1::Connect(response_msg)),

)

.await?;

}

network_bridge

.send(

upstream_addr,

NetMessage::V1(NetMessageV1::Connect(response_msg)),

)

.await?;

iduartgomez · 2025-12-01T15:38:06Z

crates/core/src/operations/connect.rs

-                        network_bridge
-                            .send(upstream_addr, NetMessage::V1(NetMessageV1::Connect(msg)))
-                            .await?;
+                        if let Some(upstream) = &source_addr {


what happens if source_addr is not none? can it even be none?

iduartgomez · 2025-12-01T15:38:46Z

crates/core/src/operations/connect.rs

-                                NetMessage::V1(NetMessageV1::Connect(response_msg)),
-                            )
-                            .await?;
+                        if let Some(upstream) = &source_addr {


what happens if source_addr is not none? can it even be none?

iduartgomez · 2025-12-01T15:41:28Z

crates/core/src/operations/update.rs


        if let Some(target) = remote_target {
-            // Subscribe to the contract
+            // Subscribe to the contract - sender is ourselves, no upstream_addr needed


"sender is opurselves" but this comes from "remote_target" ? Soemthing is off here.

Reverts commit aaf3e3b which incorrectly added ObservedAddr newtype in PR #2167. This work belongs in PR #2172 (higher in the stack). Maintains proper PR stack separation and avoids merge conflicts. [AI-assisted - Claude]

sanity · 2025-12-01T20:29:55Z

@iduartgomez Addressing all your feedback:

1. subscribe.rs:429 - Debug side effects

You're right - the debug trace at line 434 shows subscriber.peer() AFTER potentially modifying subscriber at line 422. This is confusing because it's not clear whether the trace shows the original or modified value.

Fix needed: Move the debug trace to before the modification, or split into two traces (before/after).

2. connect.rs:833 & 879 - source_addr nullability

Can source_addr be None? Yes - for locally-initiated operations (client requests), there's no inbound source address.

What happens if None? Currently, the ObservedAddress message is silently dropped (no upstream to route through). This could hide bugs.

Fix needed: Add explicit handling:

let Some(upstream) = source_addr else {
    tracing::warn!("ObservedAddress has no upstream - locally initiated?");
    return Ok(...);
};

3. get.rs:860 - Remove unwrap

Agreed. The .expect() can panic.

Fix needed: Return error instead:

let Some(sender) = sender_from_addr.clone() else {
    tracing::warn!("ReturnGet without sender lookup");
    return Err(OpError::invalid_transition(self.id));
};

4. update.rs:1008 - Confusing comment

The comment "sender is ourselves" is misleading. Looking at line 1005, sender comes from the function parameter (the peer requesting the update), not ourselves.

Fix needed: Clarify:

// Subscribe on behalf of the requesting peer (no upstream_addr - direct registration)

I'll prepare fixes for all these issues.

[AI-assisted - Claude]

@iduartgomez

Implements changes requested by @iduartgomez: 1. subscribe.rs: Move debug trace before subscriber modification - Log original address before updating from source_addr - Prevents confusion about which address is being used 2. connect.rs: Add explicit None handling for source_addr - ObservedAddress and ConnectResponse messages now check for upstream - Warn when locally-initiated operations lack source_addr 3. get.rs: Replace expect() with proper error handling (4 instances) - RequestGet, SeekNode, and ReturnGet (2 cases) now use Option unwrapping - Return OpError::invalid_transition instead of panicking 4. update.rs: Clarify confusing comment about sender - "sender is ourselves" → "on behalf of the requesting peer" - Accurately reflects that sender comes from function parameter All changes maintain existing behavior while improving error handling and code clarity for NAT routing scenarios. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

sanity · 2025-12-01T20:40:24Z

@iduartgomez - Implemented all requested fixes in commit 8bd39fc:

1. subscribe.rs:416-438 - Moved debug trace before modification

Now logs original subscriber address before updating from source_addr
Prevents confusion about which address is being displayed

2. connect.rs:825-848, 882-901 - Added explicit None handling

ObservedAddress and ConnectResponse now check for upstream before sending
Warns when locally-initiated operations lack source_addr

3. get.rs:552-558, 710-716, 858-864, 1131-1137 - Replaced expect() with error handling

Fixed 4 instances: RequestGet, SeekNode, and ReturnGet (2 cases)
Returns OpError::invalid_transition instead of panicking
Applied same pattern consistently across all message types

4. update.rs:1008 - Clarified comment

Changed "sender is ourselves" → "on behalf of the requesting peer"
Accurately reflects that sender comes from function parameter

[AI-assisted - Claude]

Merges branch fix/seeding-subscriber-nat-2164 to bring in Nacho's address validation fixes: - Better tracing for subscribe address updates - Clearer comments for subscription registration 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

iduartgomez · 2025-12-01T22:46:22Z

crates/core/src/ring/mod.rs

        &self,
        contract: &ContractKey,
        subscriber: PeerKeyLocation,
+        upstream_addr: Option<ObservedAddr>,


why is this optional ?

iduartgomez · 2025-12-01T22:48:49Z

crates/core/src/operations/subscribe.rs

                            "subscribe: registering provider/subscription source"
                        );
-                        if op_manager.ring.add_subscriber(key, sender.clone()).is_err() {
+                        // Pass None: sender was already looked up from source_addr (line ~866),


this is really confussing, whenever you add a subscriber, yolu should know its external address. this shouldn't be optional, and that address should be clearly identified and embedded in the message.

iduartgomez

A change I do not see here, but in the configuration, wherever socket addresses are created for the different connections, also typing them strictly to avoid illegal conversions/usages etc. would be good.

A lof of the confussion comes from makign illegal states representable, encode all usages of socket addresses out of the transport module by their provenance (are they observed? are they private coming from config?) and only accept the types in the messages you would be expecting (e.g. messages sent over the network shouldn't ever contain advertised addresses, sicne those may be private).

PR #2172 added detailed tracing that calls subscriber.peer() BEFORE filling in the address from source_addr. This causes a panic when the subscriber has PeerAddr::Unknown (NAT peers). The correct order (from PR #2171) is: 1. Check if address is unknown 2. Fill in address from source_addr 3. THEN call .peer() for tracing This regression was introduced when merging PR #2172's tracing improvements without preserving the correct order of operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Consolidates changes from PRs #2172, #2174, and #2175: This builds on PR #2191 (wire protocol cleanup) and adds: - Fix seeding/subscribe operations to handle PeerAddr::Unknown for NAT scenarios - Gateway properly fills in observed addresses from packet source - Improved subscriber address tracking in seeding manager - Update live_tx and connection tests for new address model NOTE: This PR requires review - previous PRs (#2174, #2175) had CHANGES_REQUESTED from Nacho. Submitting consolidated changes for fresh review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Adds NAT address handling to subscribe/seeding operations: - Subscribers with PeerAddr::Unknown have their address filled in by gateway - Gateway observes real UDP source address and updates subscriber address - SeedingManager tracks subscriber addresses properly - live_tx tests updated for new address model - In-memory testing infrastructure updated for PeerAddr Supersedes PRs #2172, #2174, #2175 (which had changes requested). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

sanity closed this Nov 29, 2025

sanity reopened this Nov 29, 2025

sanity changed the base branch from main to fix/subscribe-nat-routing-2164 November 29, 2025 16:59

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from 0c2c4c7 to 1b8127d Compare November 29, 2025 17:00

sanity mentioned this pull request Nov 29, 2025

fix: use pub_key for PeerId equality to fix NAT connection lookup #2163

Closed

3 tasks

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from 1b8127d to 91e03e6 Compare November 29, 2025 17:29

sanity requested review from Copilot and iduartgomez November 29, 2025 19:18

Copilot started reviewing on behalf of sanity November 29, 2025 19:19 View session

Copilot finished reviewing on behalf of sanity November 29, 2025 19:22

Copilot AI reviewed Nov 29, 2025

View reviewed changes

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from 9df9c39 to bb74066 Compare November 29, 2025 19:49

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from 91e03e6 to 11a7d16 Compare November 29, 2025 19:52

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from bb74066 to eea52ed Compare November 29, 2025 21:23

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch 2 times, most recently from 7f119f2 to b398ba7 Compare November 29, 2025 22:00

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from 1c90c91 to 0a9183a Compare November 29, 2025 22:38

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from 79f3b59 to a0196fe Compare November 29, 2025 22:38

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from 0a9183a to 2906500 Compare November 29, 2025 23:34

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from a0196fe to cd529c9 Compare November 29, 2025 23:36

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from 2906500 to 4ba0035 Compare November 29, 2025 23:43

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from cd529c9 to a56562b Compare November 29, 2025 23:43

sanity force-pushed the fix/subscribe-nat-routing-2164 branch from 4ba0035 to f849c3a Compare November 30, 2025 00:14

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch 2 times, most recently from bfdc2cf to 9aad6f5 Compare November 30, 2025 00:57

This was referenced Nov 30, 2025

fix: allow parallel connection attempts (max 3 concurrent) #2174

Merged

refactor(routing): add upstream_addr for connection-based routing #2167

Merged

sanity requested review from Copilot and netsirius November 30, 2025 04:45

Copilot started reviewing on behalf of sanity November 30, 2025 04:46 View session

sanity and others added 5 commits November 30, 2025 20:36

ci: retrigger CI

379ccee

ci: retrigger CI

741ad39

fix: resolve compilation errors after rebase

8614d58

- Remove duplicate get_peer_location_by_addr functions - Fix socket_addr() calls on SocketAddr (use dereference instead) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

sanity force-pushed the fix/seeding-subscriber-nat-2164 branch from daa05b9 to 9db2219 Compare December 1, 2025 02:36

sanity requested a review from Copilot December 1, 2025 05:18

Copilot started reviewing on behalf of sanity December 1, 2025 05:18 View session

Copilot finished reviewing on behalf of sanity December 1, 2025 05:25

Copilot AI reviewed Dec 1, 2025

View reviewed changes

iduartgomez reviewed Dec 1, 2025

View reviewed changes

iduartgomez requested changes Dec 1, 2025

View reviewed changes

Base automatically changed from fix/subscribe-nat-routing-2164 to refactor/wire-protocol-cleanup-2164 December 2, 2025 00:11

sanity deleted the branch refactor/wire-protocol-cleanup-2164 December 2, 2025 00:17

sanity closed this Dec 2, 2025

sanity mentioned this pull request Dec 2, 2025

refactor: wire protocol cleanup and NAT routing improvements #2191

Merged

sanity mentioned this pull request Dec 2, 2025

fix: handle NAT address in subscribe and seeding operations #2192

Closed

sanity mentioned this pull request Dec 2, 2025

fix: handle NAT address in subscribe and seeding operations #2193

Open

-                    let sender = sender_from_addr.clone().expect(
-                        "ReturnGet requires sender lookup from connection - source_addr should resolve to known peer",
-                    );
+                    let sender = if let Some(sender) = sender_from_addr.clone() {
+                        sender
+                    } else {
+                        tracing::warn!(
+                            tx = %id,
+                            %key,
+                            "ReturnGet: Could not resolve sender from address; aborting operation"
+                        );
+                        return Err(ErrorKind::NodeError(
+                            "ReturnGet requires sender lookup from connection - source_addr could not be resolved to known peer".into(),
+                        ).into());
+                    };

Uh oh!

fix: use ObservedAddr newtype for NAT routing #2172

fix: use ObservedAddr newtype for NAT routing #2172

Uh oh!

Conversation

sanity commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Why existing tests didn't catch this

Previous Approaches

This Solution

Testing

Unit Tests

Docker NAT Integration Test

River End-to-End NAT Test

Stacked On

Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanity commented Dec 1, 2025

1. subscribe.rs:429 - Debug side effects

2. connect.rs:833 & 879 - source_addr nullability

3. get.rs:860 - Remove unwrap

4. update.rs:1008 - Confusing comment

Uh oh!

sanity commented Dec 1, 2025

Uh oh!

sanity commented Nov 29, 2025 •

edited

Loading