fix: add self-connection guard in should_accept()

[sanity]

Problem

When a joiner connects to a gateway that has no other peers, the connect request can be forwarded back to the joiner itself. Without a guard, the joiner would accept this connection to itself, causing a self-connection.

This scenario occurs during early network bootstrap when:

1. Joiner connects to gateway
2. Gateway has no other peers to forward to
3. Gateway forwards connect request back to joiner
4. Joiner evaluates should_accept() for its own connection request

Solution

Add an early guard in should_accept() that checks if the incoming peer ID matches the node's own peer ID. If they match, the connection is rejected immediately with a warning log.

// Don't accept connections from ourselves
if let Some(own_id) = self.get_peer_key() {
    if &own_id == peer_id {
        tracing::warn!(%peer_id, "should_accept: rejecting self-connection attempt");
        return false;
    }
}

Testing

Added two unit tests:

• rejects_self_connection - verifies that should_accept() returns false when the peer ID matches the node's own ID
• accepts_connection_from_different_peer - verifies normal connections from other peers are still accepted

Fixes

Closes #2139

[AI-assisted - Claude]

[sanity]

@claude Please review this PR and the original issue #2139 it fixes. Verify the fix is correct and complete.

[claude]

Claude finished @sanity's task —— View job

---

PR Review: Self-Connection Guard

I've completed my review of PR #2140 and the original issue #2139. Here's my comprehensive analysis:

---

✅ Overall Assessment: Approved with Minor Notes

The fix is correct and complete for the reported issue. The implementation is clean, well-tested, and placed in the optimal location.

---

🔍 Implementation Review

Guard Logic ✅

The guard is implemented correctly in connection_manager.rs:150-156:

// Don't accept connections from ourselves
if let Some(own_id) = self.get_peer_key() {
    if &own_id == peer_id {
        tracing::warn!(%peer_id, "should_accept: rejecting self-connection attempt");
        return false;
    }
}

Strengths:

• ✅ Placed at the optimal location - should_accept() is the single admission gate for all connection decisions
• ✅ Early exit prevents unnecessary processing and state changes
• ✅ Handles the case where peer_key is not yet set (returns None from get_peer_key())
• ✅ Logs appropriately at warn level for visibility
• ✅ Clean comparison using peer IDs (the authoritative identity)

Existing Safeguards

The codebase already has defense-in-depth at connection_manager.rs:408:

debug_assert!(self.get_peer_key().expect("should be set") != peer);

This debug_assert! in add_connection() would have caught self-connections in debug builds, but:

1. It doesn't prevent the issue in release builds
2. It fires after acceptance logic (too late)
3. Your guard fixes this by rejecting at admission time

---

🧪 Test Coverage

Tests Added ✅

Two unit tests provide solid coverage:

1. rejects_self_connection (lines 658-688) - Verifies rejection when peer ID matches
2. accepts_connection_from_different_peer (lines 690-727) - Ensures normal connections still work

Test Quality:

• ✅ Tests the actual behavior, not implementation details
• ✅ Creates realistic test fixtures with proper keypairs and addresses
• ✅ Verifies both positive and negative cases
• ✅ Tests are isolated and don't depend on external state

Edge Cases Covered

• ✅ Node with peer ID set → Correctly rejects self
• ✅ Node with peer ID set → Correctly accepts others
• ⚠️ Minor: Doesn't explicitly test the case where peer_key is None (joiner before peer ID is set), but this is handled gracefully by the if let Some(own_id) pattern

---

🔗 Related Code Analysis

I checked whether similar guards are needed elsewhere:

1. routing_candidates() (lines 589-627): Already filters the requesting peer from candidates (lines 604-608), preventing routing loops
2. add_connection() (line 408): Has a debug_assert! (mentioned above)
3. Connection forwarding logic: Uses a visited list to prevent loops in operations/connect.rs

Conclusion: No additional guards needed. The fix at should_accept() is the right chokepoint.

---

📝 Code Quality

Positive:

• ✅ Follows Rust idioms (if let Some)
• ✅ Uses structured logging with field interpolation (%peer_id)
• ✅ Consistent with codebase style
• ✅ No performance impact (early exit on match)

Minor Suggestions (non-blocking):

1. Consider testing the peer_key = None path explicitly (though current code handles it correctly)
2. The log message could include the node's own peer ID for debugging: "should_accept: rejecting self-connection (own_id: {own_id}, peer_id: {peer_id})"

---

🎯 Issue Resolution

The fix addresses the exact scenario from issue #2139:

• ✅ Prevents joiner from accepting connections to itself
• ✅ Works during early bootstrap (gateway with no other peers)
• ✅ Fails safely when peer ID isn't set yet

Root Cause Correctly Identified:
The PR description correctly explains that when a gateway has no other peers to forward to, it can forward back to the joiner. Your guard prevents the joiner from accepting this.

---

🚀 Recommendation: Merge

This PR is ready to merge. The fix is:

• Correct - Solves the stated problem
• Complete - No additional changes needed
• Well-tested - Unit tests cover the scenarios
• Safe - No breaking changes or side effects
• Clear - PR description explains the WHY effectively

---

References:

• Guard implementation: crates/core/src/ring/connection_manager.rs:150-156
• Test coverage: crates/core/src/ring/connection_manager.rs:658-727
• Related safeguard: crates/core/src/ring/connection_manager.rs:408

---

|

[iduartgomez]

this fix is ok, but the main issue is that we are not adding self to the skip_list of connect so it wont be a possible target at all in the gateway, we should look into that