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

[sanity]

⚠️ PR Stack Notice

This PR is the base of a 4-PR stack. The test_three_node_network_connectivity test may fail on this PR but passes in #2169 and later.

Why: This PR introduces the foundation for connection-based routing, but the wire protocol still embeds from/sender fields in messages. PR #2169 completes the cleanup by:

• Removing from field from ConnectMsg::Request (sender inferred from transport)
• Removing sender field from ConnectMsg::Response
• Simplifying Joiner enum to use PeerKeyLocation directly
• Changing visited from Vec<PeerKeyLocation> to Vec<SocketAddr>

These changes are necessary for proper response routing. The test passes in #2169, #2171, and #2172.

Merge order: #2167 → #2169 → #2171 → #2172

---

Problem

Issue #2164 - Current architecture conflates peer identity with network address, causing NAT traversal failures. Peers embed their own (often incorrect) addresses in messages, and the routing layer relies on these embedded addresses rather than using the transport-layer source address.

Solution

This PR implements Phases 1-4a of #2164:

Phase 1-2: Type System Restructuring

• Add PeerAddr enum (Unknown/Known) for explicit address state
• Restructure PeerKeyLocation to separate identity from address
• Change connections HashMap key from PeerId to SocketAddr
• Change LiveTransactionTracker key from PeerId to SocketAddr

Phase 3: NetworkBridge Refactor

• Update NetworkBridge trait to use SocketAddr instead of PeerId
• Thread source_addr from transport layer through message handling

Phase 4a: Connection-Based Routing Foundation

• Add upstream_addr field to all operations (Get, Put, Subscribe, Update)
• Add source_addr parameter to Operation::load_or_init and process_message
• Remove rewrite_sender_addr hack - no longer needed with real source address
• Add target_addr to OperationResult for explicit routing decisions

Key Design Principles

1. Identity vs Address separation: pub_key is identity; SocketAddr is "how to reach them"
2. Connections keyed by address: Transport identifies packets by source address
3. Request-response routing: Responses route back via stored upstream_addr
4. Connect is special: Only Connect protocol needs embedded addresses for NAT hole-punching

Changes

• ring/peer_key_location.rs: PeerAddr enum, restructured PeerKeyLocation
• ring/live_tx.rs: Changed tracker key to SocketAddr
• node/network_bridge.rs: Updated trait to use SocketAddr
• node/network_bridge/p2p_protoc.rs: Thread source_addr, remove address rewriting
• operations/*.rs: Add upstream_addr, update signatures for source_addr
• message.rs: Remove rewrite_sender_addr methods

Testing

• All unit tests pass
• Docker NAT test: 100% connectivity
• Integration tests pass in refactor(connect): remove from/sender fields from ConnectMsg #2169 (after wire protocol cleanup)

Next Steps (PR #2169)

• Remove sender fields from wire messages (Get, Put, Subscribe, Update, Connect)
• Change skip_list to use SocketAddr

Part of #2164

[AI-assisted - Claude]

[iduartgomez]

Dont use SocketAddr directly anywhere in the codebase directly for other thing than connecting to the actual socket.

If you make invalid states (eg using private ips for external connections) unrepresentable at compile time it will prevent a lot of this potential errors we are fixing with endless chains of refactors.

Encode the usage of addresses in the type system via newtypes (eg AdvertisedAddr instead of SocketAddr)

[sanity]

Dont use SocketAddr directly anywhere in the codebase directly for other thing than connecting to the actual socket.

If you make invalid states (eg using private ips for external connections) unrepresentable at compile time it will prevent a lot of this potential errors we are fixing with endless chains of refactors.

Encode the usage of addresses in the type system via newtypes (eg AdvertisedAddr instead of SocketAddr)

I can use an ObservedAddr wrapper for SocketAddr - I'm trying to avoid relying on what a peer advertises as its own addr in messages since the message recipient has a more reliable way to determine the peer's external address from the socket info.

[Copilot]

Pull request overview

This PR implements connection-based routing for NAT traversal by separating peer identity from network addresses. The changes restructure PeerKeyLocation to use an explicit PeerAddr enum and modify connection tracking to key by SocketAddr instead of PeerId.

Key Changes:

• Introduced PeerAddr enum (Unknown/Known) for explicit address state management
• Changed LiveTransactionTracker and connection maps to use SocketAddr keys
• Updated NetworkBridge trait to route by SocketAddr instead of PeerId
• Added upstream_addr field to operations for connection-based routing
• Threaded source_addr through operation handlers for NAT-aware routing

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/core/src/transport/mod.rs	Added ObservedAddr newtype for transport-layer addresses
crates/core/src/ring/peer_key_location.rs	Major restructuring with PeerAddr enum and new accessor methods
crates/core/src/ring/live_tx.rs	Changed to track transactions by SocketAddr
crates/core/src/ring/connection_manager.rs	Updated connection management to use SocketAddr keys
crates/core/src/operations/*.rs	Added upstream_addr field and source_addr parameter to operations
crates/core/src/node/network_bridge/*.rs	Updated NetworkBridge trait and implementations for SocketAddr routing
crates/core/src/tracing/mod.rs	Updated to use new peer() accessor method
crates/core/src/operations/subscribe/tests.rs	Updated test code to use new PeerKeyLocation constructors

---

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

[Copilot]

Attempting to mutate an immutable value. sender_mut.peer() returns a PeerId by value (not a mutable reference), so trying to assign to sender_mut.peer().addr will fail to compile. This should likely be updating the sender_mut's peer_addr field directly instead of trying to mutate through the peer() method.

Suggested change

	if sender_mut.peer().addr.ip().is_unspecified() {
	sender_mut.peer().addr = remote_addr;
	if sender_mut.peer.addr.ip().is_unspecified() {
	sender_mut.peer.addr = remote_addr;

[sanity]

Good catch! Fixed in 09ed113 - now using socket_addr().is_some_and(|a| a.ip().is_unspecified()) for the check and set_addr(remote_addr) for the mutation.

[AI-assisted - Claude]

[sanity]

@iduartgomez

Dont use SocketAddr directly anywhere in the codebase directly for other thing than connecting to the actual socket.

Should be fixed now.

[sanity]

@claude please do a code review of this PR. This is part 1 of a 4-PR stack implementing connection-based routing for NAT traversal (#2164).

[claude]

Claude finished @sanity's task —— View job

---

Code Review - Connection-Based Routing (PR #2167)

Reviewing part 1 of the 4-PR stack for connection-based routing (issue #2164). This PR establishes the foundation for connection-based routing by restructuring the type system and threading source address information through the codebase.

---

Overall Assessment

✅ Architecture: The refactoring successfully separates peer identity from network addresses, which is essential for NAT traversal. The design principles are sound.

⚠️ Concerns: There are several issues around type safety, API design, and potential runtime bugs that should be addressed.

---

Critical Issues

1. Newtype Usage is Inconsistent ⚠️

The PR introduces ObservedAddr as a newtype wrapper for transport-layer addresses (great!), but the type system doesn't enforce its use consistently:

Issue: PeerKeyLocation still uses raw SocketAddr in PeerAddr::Known(SocketAddr) instead of leveraging newtypes.

Example from crates/core/src/ring/peer_key_location.rs:21:

pub enum PeerAddr {
    Unknown,
    Known(SocketAddr),  // ⚠️ Should this be Known(ObservedAddr)?
}

Why this matters: The review comment from @iduartgomez about not using SocketAddr directly is valid. Different address types have different semantics:

• ObservedAddr - Seen at transport layer (ground truth for NAT)
• AdvertisedAddr - What a peer claims in protocol messages
• ExternalAddr - Verified public address

Mixing raw SocketAddr makes it easy to accidentally use advertised addresses where observed addresses are required.

Recommendation: Consider introducing additional newtype wrappers:

• ObservedAddr (already exists) - addresses from UDP packet source
• AdvertisedAddr - addresses embedded in protocol messages
• Use these types in PeerAddr::Known and throughout the codebase

This would make invalid states (like using private IPs for external connections) unrepresentable at compile time.

---

2. addr() Method Panics on Unknown Address 🔴

Location: crates/core/src/ring/peer_key_location.rs:124-130

pub fn addr(&self) -> SocketAddr {
    match &self.peer_addr {
        PeerAddr::Known(addr) => *addr,
        PeerAddr::Unknown => panic!(
            "addr() called on PeerKeyLocation with unknown address; use socket_addr() instead"
        ),
    }
}

Problems:

1. Runtime panic risk: This introduces a footgun where calling addr() on a PeerKeyLocation with unknown address will panic at runtime
2. API design: Having both addr() (panicking) and socket_addr() (safe) is confusing

Impact: High - panics in production are serious failures

Recommendation:

• Option A (safer): Deprecate addr() entirely and require all callers to use socket_addr(), handling the None case explicitly
• Option B (pragmatic): Add #[track_caller] to addr() and comprehensive documentation, but audit all call sites to ensure unknown addresses can't occur

---

3. Connection Manager Still Uses PeerId Keys

Location: crates/core/src/ring/connection_manager.rs

The PR description says "Change connections HashMap key from PeerId to SocketAddr", but looking at the code:

pub(crate) struct ConnectionManager {
    pending_reservations: Arc<RwLock<BTreeMap<PeerId, Location>>>,
    pub(super) location_for_peer: Arc<RwLock<BTreeMap<PeerId, Location>>>,
    // ...
}

Issue: The connection manager still keys several internal data structures by PeerId rather than SocketAddr:

• pending_reservations - BTreeMap<PeerId, Location>
• location_for_peer - BTreeMap<PeerId, Location>

Why this matters: If the goal is connection-based routing (keyed by transport address), these should be keyed by SocketAddr to avoid identity/address confusion.

Found in code:

• connection_manager.rs:135 - should_accept(location: Location, peer_id: &PeerId)
• connection_manager.rs:168 - location_for_peer.read().get(peer_id)
• connection_manager.rs:246 - record_pending_location(&self, peer_id: &PeerId, location: Location)

Recommendation: Either:

1. Complete the refactor to use SocketAddr keys throughout the connection manager (may be in follow-up PRs)
2. Document why PeerId keys are still necessary and add a comment explaining the migration path

---

Medium-Priority Issues

4. LiveTransactionTracker - Good Refactor! ✅

Location: crates/core/src/ring/live_tx.rs

The change from PeerId to SocketAddr keys here is correct and well-done:

pub struct LiveTransactionTracker {
    tx_per_peer: Arc<DashMap<SocketAddr, Vec<Transaction>>>,
}

Rationale: Transactions are indeed tied to network connections (transport-level), not cryptographic identities. This aligns with connection-based routing principles.

---

5. upstream_addr Field - Correct Foundation ✅

Location: Operations files (get.rs:48, put.rs:30, etc.)

Adding upstream_addr: Option<ObservedAddr> to operation structs is the right approach:

pub(crate) struct GetOp {
    pub id: Transaction,
    state: Option<GetState>,
    result: Option<GetResult>,
    stats: Option<Box<GetStats>>,
    upstream_addr: Option<ObservedAddr>,  // ✅ Good addition
}

Why: This enables request-response routing where responses go back to the observed source address.

Note: The field is currently Option<ObservedAddr> to handle local operations. Consider documenting when this is None vs Some.

---

6. target_addr in OperationResult - Explicit Routing ✅

Location: crates/core/src/operations/mod.rs:49-54

pub(crate) struct OperationResult {
    pub return_msg: Option<NetMessage>,
    pub target_addr: Option<SocketAddr>,  // ✅ Explicit routing target
    pub state: Option<OpEnum>,
}

Good: Explicit routing decisions are now made at the operation level rather than embedded in messages. This is clean separation of concerns.

Question: Should target_addr be Option<ObservedAddr> for type safety, or is raw SocketAddr acceptable here since it's an internal routing decision?

---

7. P2pConnManager Connection Tracking

Location: crates/core/src/node/network_bridge/p2p_protoc.rs:140-161

Good refactoring to use SocketAddr as connection keys:

struct ConnectionEntry {
    sender: PeerConnChannelSender,
    pub_key: Option<TransportPublicKey>,
}

pub(in crate::node) struct P2pConnManager {
    /// Connections indexed by socket address
    connections: HashMap<SocketAddr, ConnectionEntry>,
    /// Reverse lookup: public key -> socket address
    addr_by_pub_key: HashMap<TransportPublicKey, SocketAddr>,
    // ...
}

Concern: The dual mapping (connections by addr, addr_by_pub_key reverse lookup) needs to stay synchronized. Consider:

• Helper methods that update both maps atomically
• Debug assertions to verify consistency
• Tests for edge cases (reconnection with same pubkey, address reuse, etc.)

---

8. peer() Method Returns PeerId (Transitional)

Location: crates/core/src/ring/peer_key_location.rs:147-149

pub fn peer(&self) -> PeerId {
    PeerId::new(self.addr(), self.pub_key.clone())
}

Issue: This method constructs a PeerId by calling addr(), which panics if address is unknown.

Impact: Combined with issue #2 above, this is a potential panic source.

Recommendation: Either:

1. Make this return Option<PeerId> and use socket_addr()
2. Document clearly that this should only be called when address is known
3. Add #[track_caller] for better panic traces

---

Minor Issues / Style

9. Test Failure Expectation

The PR description mentions:

The test_three_node_network_connectivity test may fail on this PR but passes in #2169

Concern: Merging code with known test failures is risky. Even if the fix is in a later PR, CI should pass on each commit.

Recommendation: Either:

1. Temporarily skip the failing test with #[ignore] and a TODO-MUST-FIX comment linking to refactor(connect): remove from/sender fields from ConnectMsg #2169
2. Ensure the test passes by including necessary changes from refactor(connect): remove from/sender fields from ConnectMsg #2169

---

10. Documentation Gaps

Missing docs:

• When is PeerAddr::Unknown expected vs PeerAddr::Known?
• What's the lifecycle of an unknown address becoming known?
• When should callers use addr() vs socket_addr()?

Recommendation: Add module-level documentation explaining:

• The identity vs address separation
• NAT traversal flow (unknown → observed → known)
• API usage guidelines

---

Positive Aspects ✅

1. Clean type system refactor - PeerAddr enum makes address state explicit
2. ObservedAddr newtype - Good semantic distinction for transport-layer addresses
3. Connection-based foundation - Threading source_addr through operations is the right approach
4. LiveTransactionTracker refactor - Correctly uses SocketAddr keys
5. Test coverage - Connection manager has good self-connection and different-peer tests

---

Recommendations Summary

Must Fix Before Merge

1. ⚠️ Address the addr() panic issue - either remove the method or add #[track_caller] and audit call sites
2. ⚠️ Fix or skip the failing test_three_node_network_connectivity test
3. ⚠️ Document the dual mapping synchronization strategy in P2pConnManager

Should Fix Before Merge

4. 📝 Add module documentation explaining PeerAddr lifecycle and API usage
5. 📝 Document why ConnectionManager still uses PeerId keys (if intentional)

Consider for Follow-up PRs

6. 💡 Extend newtype usage: introduce AdvertisedAddr to fully address @iduartgomez's concern
7. 💡 Complete ConnectionManager refactor to use SocketAddr keys throughout
8. 💡 Add consistency assertions/tests for dual mappings in P2pConnManager

---

Overall: This is a substantial architectural improvement that lays good groundwork for NAT traversal. The main concerns are around type safety (newtype usage) and the panicking addr() method. With the critical issues addressed, this will be a solid foundation for the remaining PRs in the stack.

---

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

---

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

[iduartgomez]

Dont use SocketAddr directly anywhere in the codebase directly for other thing than connecting to the actual socket.
If you make invalid states (eg using private ips for external connections) unrepresentable at compile time it will prevent a lot of this potential errors we are fixing with endless chains of refactors.
Encode the usage of addresses in the type system via newtypes (eg AdvertisedAddr instead of SocketAddr)

I can use an ObservedAddr wrapper for SocketAddr - I'm trying to avoid relying on what a peer advertises as its own addr in messages since the message recipient has a more reliable way to determine the peer's external address from the socket info.

Yes sorry that's what I meant, observed addr. Thanks.

[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 covnersions/usages etc. would be good.

[iduartgomez]

we are still using SocketAddr here

[iduartgomez]

we are still using SocketAddr here

[iduartgomez]

addr: ObservedAddress

[sanity]

@iduartgomez Regarding your inline comments about using SocketAddr directly - the ObservedAddr newtype work has been moved to PR #2172 (higher in the stack) to maintain proper PR separation.

This PR (#2167) now focuses solely on adding the upstream_addr field for connection-based routing. The newtype wrapper will be introduced in #2172 where it belongs.

[AI-assisted - Claude]

[iduartgomez]

Can this happen? Should we log it? Should we error out?

[iduartgomez]

Can this happen? Should we log it? Should we error out?

[sanity]

Yes, this can happen - it's the current code path used by all operations (Connect/Put/Get/Subscribe/Update) in PR #2167. The NetMessageV1::* handlers all pass None for source_addr as a transitional state.

This legacy path will be removed in PR #2169 when we complete the wire protocol cleanup and switch to connection-based routing everywhere.

For now in #2167, I don't think we need to log it - it's expected behavior during this transition. Once #2169 lands and we switch to ObservedAddr everywhere, these legacy paths will be deleted.

[AI-assisted - Claude]