fix: use observed external address in ConnectResponse target #2142
Conversation
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
ConnectResponse was being sent to the joiner's original (possibly private/NAT) address instead of the observed external address. This prevented NAT users from joining the network. Added response_target field to RelayActions that captures the joiner's updated address after observed_addr processing. The response now uses this target address when sending acceptance back to the joiner. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
sanity
added a commit
that referenced
this pull request
Nov 25, 2025
The previous self-connection guard only checked peer_key, which can be None early during initialization. This caused self-connections to slip through when combined with PR #2142's response_target change. The fix adds a secondary check that compares pub_key directly. Since pub_key is always set in ConnectionManager, this catches self-connections even when peer_key is not yet initialized. This fixes the test_three_node_network_connectivity failure in the release/v0.1.39 CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Nov 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
When a joiner behind NAT sends a ConnectRequest to join the network, the gateway/relay observes the joiner's public external address. However, when sending the ConnectResponse back to the joiner, the code was incorrectly using the original
fromparameter (the joiner's private/NAT address) instead of the updated address with the observed external socket.This caused ConnectResponse messages to be sent to unreachable private addresses (e.g., 192.168.x.x), preventing NAT users from successfully joining the network.
Root Cause
In
process_message, when handlingConnectMsg::Request:actions.observed_addresscorrectly captures the updated joiner addresstarget: from.clone()wherefromis the original request senderThe
observed_addressfield inRelayActionsis only set once (guarded by!self.observed_sent), so using it directly would fail on subsequent calls. We needed a dedicated field that persists the response target.Solution
response_target: Option<PeerKeyLocation>toRelayActionsstructresponse_targetto the updatedself.request.joinerwhen accepting a connection (after observed address processing)response_target.unwrap_or_else(|| from.clone())when creating the ConnectResponseThis ensures the response is sent to the joiner's observed external address when available, falling back to the original address for backward compatibility.
Testing
Added
connect_response_uses_observed_address_not_privatetest that:response_targetcontains the observed public address, not privateAll existing tests pass.
Fixes #2141
[AI-assisted - Claude]