refactor: Restructure peer identity and address handling

[sanity]

Peer Identity and Address Restructuring

Problem Statement

Current architecture conflates peer identity (pub_key) with network address (SocketAddr) in PeerId. This causes:

1. Peers embed their own (often incorrect) address in messages
2. NAT peers report loopback/unspecified addresses before learning their external address
3. Connection tracking uses PeerId as key, but transport layer identifies by SocketAddr
4. Workarounds like observed_addr and address correction logic in message handling

Motivation: This refactor was motivated by failures in the Docker NAT test (FREENET_TEST_DOCKER_NAT=1 cargo test --test message_flow river_message_flow_over_freenet) where subscribe operations failed because NAT peers embedded incorrect loopback addresses in messages.

Design Principles

1. Identity vs Address separation: pub_key is the true identity; SocketAddr is "how to reach them now"
2. Connections keyed by address: Transport identifies packets by source address
3. Location derived from address: Ring location is a function of address, computed on demand
4. Request-response routing: Responses route back over the same connection the request came from - no embedded addresses needed
5. Connect is special: Only the Connect protocol needs embedded addresses for NAT hole-punching (establishing new direct connections)

Key Insight: Two Categories of Operations

Category 1: Request-Response (Get, Put, Subscribe, Update)

• Request travels hop-by-hop toward target
• Response travels back the same path via connection-based routing (each hop maintains operation state tracking which connection the request came from)
• No embedded originator address needed - each hop just responds to whoever sent the request
• The "sender" at each hop is just the packet source

Category 2: Connection Establishment (Connect only)

• Joiner's address must travel to acceptor for NAT hole-punching
• Acceptor's address must travel back to joiner
• Embedded addresses required because the goal is to establish a new direct connection, not respond over existing path
• Use PeerAddr::Unknown when originator creates message, first recipient fills from packet source

Proposed Type Hierarchy

/// Explicit representation of peer address state
pub enum PeerAddr {
    /// Address unknown - will be filled in by first recipient from packet source
    Unknown,
    /// Known address  
    Known(SocketAddr),
}

/// Peer info for routing - identity + address
pub struct PeerKeyLocation {
    pub pub_key: TransportPublicKey,  // The true peer identity
    pub addr: PeerAddr,                // Unknown = "fill in from packet source"
}

impl PeerKeyLocation {
    /// Location is derived from address, computed on demand
    pub fn location(&self) -> Option<Location> {
        match &self.addr {
            PeerAddr::Known(addr) => Some(Location::from_address(addr)),
            PeerAddr::Unknown => None,
        }
    }
}

Key changes:

• PeerId removed - was conflating identity with address
• Use TransportPublicKey directly (no wrapper type needed)
• addr uses PeerAddr enum for explicit semantics (Unknown vs Known)
• location removed as field - now a getter that computes from addr

Note on Ord: Current PeerId implements Ord by address as a workaround because TransportPublicKey doesn't implement Ord. The BTreeMap<PeerId, Location> usages can be changed to HashMap<TransportPublicKey, ...> since the ordering isn't semantically important.

Connection Management Changes

p2p_protoc.rs

// Before
connections: HashMap<PeerId, PeerConnChannelSender>

// After
connections: HashMap<SocketAddr, ConnectionEntry>

struct ConnectionEntry {
    sender: PeerConnChannelSender,
    pub_key: Option<TransportPublicKey>,  // Learned from first message
}

⚠️ Race Condition Caution: The current code has careful synchronization around connection access (see TOCTOU handling at lines 380-401). The refactoring must preserve these guarantees when changing the HashMap key.

connection_manager.rs

// Address-based lookups (for skip lists - track addresses we've tried)
skip_lists: HashSet<SocketAddr>,

// Peer info by address (changed from BTreeMap to HashMap - ordering not needed)
peers_by_addr: HashMap<SocketAddr, PeerKeyLocation>,

// Reverse lookup: identity to current address
addr_by_pub_key: HashMap<TransportPublicKey, SocketAddr>,

Message Changes by Operation

Connect (needs embedded addresses for NAT hole-punching)

ConnectRequest {
    joiner: PeerKeyLocation,  // joiner.addr = Unknown when joiner creates, filled by gateway
    target: PeerKeyLocation,  // target.addr = Known (we know where we're sending)
}

ConnectResponse {
    acceptor: PeerKeyLocation,  // acceptor.addr = Unknown when acceptor creates, filled by first recipient
    joiner: PeerKeyLocation,    // joiner.addr = Known (known from request)
}

Protocol for Connect:

1. Joiner creates ConnectRequest with joiner.addr = PeerAddr::Unknown
2. Gateway receives packet, fills joiner.addr = PeerAddr::Known(packet_source) before forwarding
3. Request reaches acceptor with correct joiner address
4. Acceptor creates ConnectResponse with acceptor.addr = PeerAddr::Unknown
5. First recipient fills acceptor.addr = PeerAddr::Known(packet_source) before forwarding
6. Response reaches joiner with correct acceptor address
7. Both can now attempt NAT hole-punching with correct addresses

Get, Put, Subscribe, Update (no embedded originator addresses needed)

These operations use connection-based routing:

• Each peer maintains operation state (keyed by transaction ID) that tracks which connection the request came from
• When a response arrives, the peer looks up the transaction and forwards the response back over the original connection
• No embedded address fields needed - routing is implicit via connection state

// Example: Subscribe
RequestSub {
    key: ContractKey,
    target: PeerKeyLocation,  // Where we're routing toward
    // No subscriber field needed - subscriber is tracked via connection state
}

ReturnSub {
    key: ContractKey,
    subscribed: bool,
    // No target field needed - response goes back via connection state
}

Implementation Phases

Phase 1: New Types

1. Create PeerAddr enum with Unknown and Known(SocketAddr) variants
2. Update PeerKeyLocation: replace peer: PeerId with pub_key: TransportPublicKey, use PeerAddr for addr, remove location field and add location() getter
3. Change BTreeMap<PeerId, ...> to HashMap<TransportPublicKey, ...> where applicable
4. Add type alias type PeerId = TransportPublicKey temporarily for gradual migration

Phase 2: Connection Management

1. Change connections HashMap key from PeerId to SocketAddr
2. Add pub_key: Option<TransportPublicKey> to connection entry
3. Update skip lists to use HashSet<SocketAddr>
4. Remove placeholder pub_key logic for transient connections
5. Carefully preserve synchronization guarantees around connection access

Phase 3: Message Handling - Connect

1. Update Connect message types to use PeerAddr for joiner/acceptor
2. Add address fill-in logic for Connect messages only
3. Remove observed_addr workaround (no longer needed)

Phase 4: Message Handling - Other Operations

1. Remove unnecessary sender/subscriber/requester fields from Get, Put, Subscribe, Update
2. Ensure response routing uses connection state, not embedded addresses
3. Remove existing address correction/fixup logic

Phase 5: Cleanup

1. Remove PeerId type alias
2. Update all remaining call sites (~600 usages across 30+ files)

Critical Files

• crates/core/src/node/mod.rs - PeerId definition (to be removed)
• crates/core/src/ring/peer_key_location.rs - PeerKeyLocation changes
• crates/core/src/ring/connection_manager.rs - Connection tracking restructure
• crates/core/src/node/network_bridge/p2p_protoc.rs - Connection HashMap, address fill-in logic
• crates/core/src/operations/connect.rs - Connect message changes
• crates/core/src/operations/subscribe.rs - Remove subscriber field
• crates/core/src/operations/get.rs - Remove sender field
• crates/core/src/operations/put.rs - Remove sender/origin fields
• crates/core/src/operations/update.rs - Remove sender field

Testing

Primary Validation

The Docker NAT test that motivated this refactor must pass:

cd ~/code/freenet/river/main/cli
FREENET_TEST_DOCKER_NAT=1 FREENET_CORE_PATH=/path/to/freenet-core cargo test --test message_flow river_message_flow_over_freenet -- --nocapture

This test:

• Spins up gateway and peers in Docker containers with NAT simulation
• Tests that peers behind NAT can subscribe and exchange messages via riverctl
• Validates the complete flow: connect → subscribe → put → get

Additional Tests

1. Existing unit tests should continue to pass
2. freenet-test-network multi-peer tests validate routing still works
3. Local mode tests (without NAT) ensure no regressions

Post-Implementation Verification Checklist

After implementation, verify the following invariants hold:

Address Resolution

• No Unknown addresses after first hop: Any message with PeerAddr::Unknown must have it resolved to PeerAddr::Known before forwarding. Add debug assertions that panic if a message is about to be forwarded with Unknown still present.
• Address fill-in happens exactly once: The first recipient fills in the address; subsequent hops should never see Unknown.
• No loopback/unspecified addresses in forwarded messages: Ensure 127.0.0.1 or 0.0.0.0 never appear in messages after the first hop.

Connection Management

• Connection HashMap consistency: connections (keyed by SocketAddr) and any reverse lookups (by pub_key) stay in sync during insert/remove operations.
• No placeholder pub_keys: The old pattern of using gateway's own pub_key as placeholder for unknown peers should be completely removed.
• Transient connections properly tracked: Transient connections identified by address, with pub_key learned from first message.

Operation State

• Transaction state tracks return path: Each operation correctly stores which connection the request arrived on, enabling response routing.
• Connection drops handled gracefully: If a connection drops mid-operation, the operation fails cleanly rather than routing to wrong peer.

Message Format

• No orphaned address fields: Sender/subscriber/requester fields removed from non-Connect operations actually used connection-based routing.
• Connect messages have correct addresses: After full message traversal, joiner and acceptor addresses are correct for NAT hole-punching.

End-to-End Validation

• Docker NAT test passes: FREENET_TEST_DOCKER_NAT=1 cargo test --test message_flow river_message_flow_over_freenet completes successfully with both peers able to subscribe and exchange messages.
• Multi-peer network test passes: freenet-test-network tests with multiple peers work correctly.

Logging/Debugging

• Trace logs show address resolution: Add tracing for when Unknown → Known transition happens.
• Connection identity updates logged: When a connection's pub_key is learned, log it for debugging.

---

[AI-assisted - Claude]

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-enhancement (95% confidence)
• E-hard (90% confidence)
• A-networking (95% confidence)
• S-needs-design (85% confidence)

Reasoning: This is a planned refactor to separate cryptographic identity from network address and to change connection management and message handling — an improvement/architecture change rather than a bug or new feature, so T-enhancement is appropriate. The change touches many core files and ~600 usages, warrants significant effort (E-hard). It primarily affects networking/peer connection logic (A-networking) and involves migration strategies and developer-facing API changes (A-developer-xp). The issue proposes a concrete design and phased implementation, so it should be flagged as needing design review (S-needs-design). Confidence reflects the clear scope and impact described in the report.

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.

[sanity]

@claude Please review this plan and provide feedback

[claude]

Claude finished @sanity's task —— View job

---

Design Review Complete

Todo List

• Read current implementation files
• Analyze proposed design
• Provide detailed feedback

---

Overall Assessment

This is a well-reasoned refactoring plan that addresses real architectural issues in the codebase. The separation of identity from address is sound, and the phased approach is appropriate for the scope. However, there are several areas that need clarification and adjustment before implementation.

---

Key Strengths

1. Clear Problem Identification

The core issues are real and well-documented:

• PeerId does conflate identity with address (lines 1220-1254 in node/mod.rs)
• The PartialEq and Hash implementations only compare addr, ignoring pub_key (lines 1226-1235)
• NAT peers do report incorrect addresses initially

2. Sound Design Principles

The five principles are architecturally correct:

• Identity/address separation aligns with networking best practices
• Address-based connection tracking matches transport reality
• Computed location prevents stale data
• Request-response routing insight is valid

3. Two Categories Insight is Partially Correct

The distinction between Connect (needs addresses for hole-punching) vs other operations (request-response) is insightful, but needs refinement (see concerns below).

---

Critical Concerns

1. The "No Embedded Addresses" Claim Needs Verification

The plan states that Get/Put/Subscribe/Update don't need embedded originator addresses because "responses route back over the same connection." However, examining the code reveals:

Current Implementation Evidence:

• subscribe.rs:106: upstream_subscriber: Option<PeerKeyLocation> - Subscribe does track upstream subscriber
• get.rs: The operation maintains routing state with peer information
• These operations traverse multiple hops and may need to route responses back through a chain

Question for clarification:

1. In a multi-hop scenario (e.g., A → B → C → target), how does the response get back to A without some form of path tracking?
2. Are you proposing to replace explicit address fields with implicit connection-based routing at each hop?
3. What happens if intermediate connections drop during the operation?

Recommendation: Before removing address fields from these messages, document the exact response routing mechanism. If relying on connection state, ensure robustness against connection churn.

2. Connection Management Design Has Gaps

The proposed connection tracking in p2p_protoc.rs:

// After
connections: HashMap<SocketAddr, ConnectionEntry>

struct ConnectionEntry {
    sender: PeerConnChannelSender,
    pub_key: Option<TransportPublicKey>,  // Learned from first message
}

Issues:

1. TOCTOU race condition: Current code at p2p_protoc.rs:383-401 shows careful handling of concurrent access. Changing the key from PeerId to SocketAddr without equivalent synchronization will introduce races.

2. Public key learning timing: When is pub_key learned? The current PeerId is created upfront from handshake data. If pub_key is None initially, how do you prevent accepting multiple connections from different keys on the same address?

3. Reverse lookup efficiency: The proposed addr_by_pub_key: HashMap<TransportPublicKey, SocketAddr> needs careful maintenance. What happens when:

   • A peer reconnects from a different address (NAT rebinding)?
   • Two peers attempt to connect from the same address (proxy/NAT)?

Recommendation: Expand Phase 2 to include detailed synchronization strategy and race condition analysis.

3. PeerKeyLocation Changes Need More Detail

Proposed change:

pub struct PeerKeyLocation {
    pub pub_key: TransportPublicKey,  
    pub addr: Option<SocketAddr>,      
}

Current usage analysis:

• peer_key_location.rs:10: Currently has peer: PeerId and location: Option<Location>
• connection_manager.rs:24: Used extensively in BTreeMap<PeerId, Location>
• The type is used in Ord, PartialOrd, Hash implementations

Missing from plan:

1. How does PeerKeyLocation implement Ord/Hash after the change? By pub_key? By addr?
2. What breaks when addr is None? The plan says "None = fill in my address from packet source" but who calls the location() getter if addr is None?
3. Current code has a TODO comment (line 11) saying location shouldn't be Option when this struct is used - your changes make the situation more complex with two Options.

Recommendation: Provide concrete before/after code for PeerKeyLocation including all trait implementations and explain the semantics when addr is None.

4. "Option<SocketAddr>" Semantics Are Unclear

The plan proposes addr: Option<SocketAddr> with these semantics:

• None when "referring to myself and I don't know my external address yet"
• First recipient fills from packet source

Problems:

1. Identity confusion: How do you distinguish "peer X with unknown address" from "myself with unknown address"? Both would be addr: None.

2. Gateway bootstrapping: In connection_manager.rs:78-87, gateways derive location from their configured address. If gateway messages have addr: None, what location do they advertise?

3. Handshake implications: The handshake code (p2p_protoc.rs:258-266) carefully manages peer identity. How does this interact with messages that have partial peer info?

Recommendation: Define precise state machine for address resolution:

• When is addr None?
• Who fills it in and when?
• What happens if it remains None (error case)?
• How does this interact with transient connections?

5. Phase Dependencies and Ordering

The plan proposes:

1. Phase 1: New types
2. Phase 2: Connection management
3. Phase 3: Connect messages
4. Phase 4: Other operations
5. Phase 5: Cleanup

Concern: Phase 2 (connection management) and Phase 3 (Connect) have circular dependencies. The connection HashMap needs to handle Connect messages, but Connect message handling depends on connection tracking changes.

Recommendation: Consider splitting Phase 2 into:

• Phase 2a: Internal connection tracking changes (no message protocol changes)
• Phase 2b: Connection tracking synchronized with message changes
  Then Phase 3 can cleanly build on 2a.

---

Moderate Concerns

6. Backward Compatibility Not Addressed

The plan doesn't mention protocol versioning or migration strategy. Questions:

1. Can nodes running old code interoperate with new code?
2. Is this a flag-day protocol change requiring coordinated upgrade?
3. How are serialized messages affected?

Current code has version checking (p2p_protoc.rs:192). Ensure migration path is documented.

7. Testing Strategy Insufficient

The plan mentions:

1. Existing tests should continue to pass

This is insufficient for a refactoring of this scope. Need:

• Unit tests for new PeerKeyLocation semantics
• Integration tests for address fill-in logic
• Negative tests for race conditions in connection management
• NAT traversal tests with partial address information

Recommendation: Add Phase 0 for test expansion before code changes.

8. Performance Implications Not Analyzed

Changes to connection HashMap keys (PeerId → SocketAddr) may affect:

• Lookup performance (SocketAddr is smaller but requires different hashing)
• Memory usage (additional reverse lookup map)
• Lock contention patterns (more fine-grained lookups?)

Recommendation: Run benchmarks on connection lookup hot paths before/after.

---

Minor Issues

9. Terminology Inconsistency

The plan uses:

• "peer identity" = pub_key
• "PeerId" = the struct being removed
• "PeerKeyLocation" = the replacement

But then proposes:

Add type alias type PeerId = TransportPublicKey temporarily

This will be confusing during transition. Consider using PeerIdentity for the type alias to avoid confusion.

10. Message Examples Need Update

The proposed ConnectRequest/ConnectResponse examples show the new structure, but don't show the "before" state for comparison. For reviewability, show side-by-side before/after for each message type.

11. Skip List Changes Incomplete

The plan mentions:

Update skip lists to use HashSet<SocketAddr>

But skip lists track "peers we've already tried". If a peer changes addresses mid-operation, should both addresses be in the skip list? Current code uses PeerId which is address-based (line 1226-1235), so this is actually no change - clarify this.

---

Questions for Clarification

1. Multi-hop response routing: In A → B → C flow, how exactly does C's response reach A without embedded path info?

2. Transient connections: How do transient connections (tracked separately in connection_manager.rs:32) interact with the new model? They have location: Option<Location> but what about address/pub_key?

3. Location derivation: If location is always derived from address, what about test scenarios that want deterministic locations? (Current code has this_location: Option<Location> for testing - line 127 in p2p_protoc.rs)

4. Subscriber tracking: ring module has subscribers_of() method. How does subscriber identity work after this change?

5. Connection failure handling: If connection keyed by SocketAddr drops, and peer reconnects from different address, how is continuity maintained?

---

Recommendations for Next Steps

Before Implementation:

1. Write detailed ADR (Architecture Decision Record) covering:

   • Response routing mechanism without embedded addresses
   • Connection identity vs. connection addressing model
   • State machine for address resolution (None → Some)
   • Migration/compatibility strategy

2. Create proof-of-concept for Connection management changes:

   • Implement HashMap<SocketAddr, ...> change in isolation
   • Add comprehensive tests for race conditions
   • Measure performance impact

3. Document message flow diagrams for:

   • Connect operation (with address fill-in)
   • Multi-hop Get/Put (showing response routing)
   • NAT peer joining (address discovery)

4. Expand Phase breakdown:

   • Add Phase 0: Test expansion
   • Split Phase 2 into 2a/2b
   • Add rollback criteria for each phase

During Implementation:

5. Feature flag the changes to allow gradual rollout and easy rollback

6. Add explicit invariant checks in debug builds:

   • Assert connection HashMap stays consistent with reverse lookup
   • Validate address is filled before transmission
   • Check for self-connection attempts

---

Conclusion

This refactoring addresses real architectural debt and the overall direction is sound. However, the plan currently has insufficient detail in critical areas (response routing, connection management races, address resolution state machine) that could lead to subtle bugs during implementation.

Verdict: Needs refinement before implementation

I recommend:

1. ✅ Proceed with this refactoring direction
2. ⚠️ Address the critical concerns above first
3. 📝 Expand the design document with answers to clarification questions
4. 🧪 Add testing phase before code changes

The phased approach is good, but needs more granularity and explicit dependencies between phases.

---

[sanity]

Response to Review Feedback

Thanks for the thorough review. Here's how we've addressed the concerns:

Critical Concern #1: Multi-hop Response Routing

Reviewer asked: "In A → B → C → target, how exactly does C's response reach A without embedded path info?"

Answer: Each hop maintains operation state keyed by transaction ID. When B forwards a request from A to C, B stores that this transaction arrived via its connection to A. When the response comes back from C, B looks up the transaction and routes the response back over the connection to A. This is connection-based routing, not address-based. The upstream_subscriber: Option<PeerKeyLocation> found in the code is part of this operation state tracking - but the address component isn't needed for routing since we use the connection directly.

Critical Concern #2: Connection Management Races

Valid concern. Added a caution note in the plan: "The current code has careful synchronization around connection access (see TOCTOU handling at lines 380-401). The refactoring must preserve these guarantees when changing the HashMap key."

Critical Concern #3: PeerKeyLocation Ord/Hash

Clarified: Current Ord implementation by address is a workaround because TransportPublicKey doesn't implement Ord. The BTreeMap<PeerId, Location> usages can become HashMap<TransportPublicKey, ...> since ordering isn't semantically required.

Critical Concern #4: Option Semantics

Improved: Changed from Option<SocketAddr> to explicit PeerAddr enum:

pub enum PeerAddr {
    Unknown,           // "Fill in my address from packet source"
    Known(SocketAddr), // Address is known
}

This makes the semantics explicit. Unknown only appears when a peer refers to themselves and doesn't know their external address. First recipient fills it in before forwarding.

Critical Concern #5: Phase Dependencies

The phases are sequential by design - each builds on the previous. Phase 2 (connection management) changes internal data structures, Phase 3 (Connect messages) adds the address fill-in logic that uses those structures. No circular dependency since the HashMap key change is independent of message format changes.

Moderate Concerns

• Backward Compatibility: Breaking changes are acceptable at this stage - we have no users yet as basic functionality is still being implemented.
• Testing: Added Docker NAT test as the primary validation since its failure motivated this refactor. Also added comprehensive post-implementation verification checklist.

Skip Lists

The reviewer said skip lists using SocketAddr is "no change" - this is actually incorrect. We explicitly chose SocketAddr for skip lists (per discussion with maintainer) to allow retrying the same peer if their address changes. This IS an intentional change.

---

[AI-assisted - Claude]

[iduartgomez]

Quick comment. If we are doing this change encode in the type system PrivateAddr vs PublicAddr so it will never will get mixed up. The issue started when we mixed them up, the socket addr in peerId was always supposed to be the public one but the refactors have broken that.

The public key is not a stable identity either though since it will change across peer restarts potentially, so we shouldn't relay on that for anything.