fix: handle Unknown addresses in connect operation's should_accept() #2184
Description
Problem
The 20-peer NAT River test panics in PR #2169's connect operation when the gateway checks should_accept() for peer-to-peer connection attempts.
Panic location: connect.rs:403-411 in RelayContext::should_accept()
fn should_accept(&self, joiner: &PeerKeyLocation) -> bool {
let location = joiner
.location
.unwrap_or_else(|| Location::from_address(&joiner.addr())); // ❌ Line 406 - panics
self.op_manager
.ring
.connection_manager
.should_accept(location, &joiner.peer()) // ❌ Line 410 - panics
}Both joiner.addr() and joiner.peer() panic when the joiner has PeerAddr::Unknown (NAT peers don't know their external address).
Why 6-peer test passed but 20-peer test failed
- 6-peer network: Pure star topology - peers only connect to gateway. The
should_accept()code path for peer-to-peer connections never executes. - 20-peer network: Triggers topology adjustment where peers try to build mesh by connecting to other peers. When gateway receives connect request from NAT peer A trying to reach peer B, it calls
should_accept()with joiner havingPeerAddr::Unknown→ panic.
Test command that reproduces:
cd ~/code/freenet/river/main/cli
FREENET_TEST_DOCKER_NAT=1 FREENET_CORE_PATH=/path/to/pr-2169 \
RIVER_TEST_PEER_COUNT=20 RUST_LOG=info \
cargo test --test message_flow river_message_flow_over_freenet_six_peers_five_rounds \
-- --ignored --test-threads=1 --nocapturePanic output:
[thread] gw0: 'tokio-runtime-worker' panicked at peer_key_location.rs:127:34:
addr() called on PeerKeyLocation with unknown address; use socket_addr() instead
[at] gw0: crates/core/src/ring/connection_manager.rs:162
Root Cause
PR #2169 introduced this bug during wire protocol refactoring. The same class of bug we've been fixing throughout the stack - calling .addr() or .peer() on PeerKeyLocation with Unknown addresses.
Proposed Solution
Change ConnectionManager::should_accept() signature from:
pub fn should_accept(&self, location: Location, peer_id: &PeerId) -> boolTo:
pub fn should_accept(&self, location: Location, joiner: &PeerKeyLocation) -> boolThen use joiner.pub_key() for identity checks and joiner.socket_addr() for safe address access.
Additional Issue
Potentially similar bug at op_state_manager.rs:388:
let peer_ids: Vec<PeerId> = subscribers.into_iter().map(|sub| sub.peer()).collect();Needs investigation - if subscribers can have Unknown addresses, this will also panic.
Context
- Discovered during systematic testing of PR stack refactor(routing): add upstream_addr for connection-based routing #2167-refactor: remove redundant inline timeout check from connect op #2175
- Part of ongoing NAT routing fixes (issue refactor: Restructure peer identity and address handling #2164)
- Related to subscription tree and seeding mechanism work
Affects
PR #2169 (refactor/wire-protocol-cleanup-2164)
[AI-assisted - Claude]