fix: add self to connect skip_list when selecting forwarding targets #2146
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
When selecting forwarding targets for connect requests, explicitly exclude ourselves from candidates using SkipListWithSelf. This ensures a node never selects itself as a forwarding target, even if self wasn't properly added to the visited list by upstream callers. This complements the defense-in-depth guard added in PR #2140 which rejects self-connections in should_accept(). Now we prevent self from being a candidate in the first place. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential self-connection issue by ensuring nodes never select themselves as forwarding targets when routing connect requests. The fix introduces SkipListWithSelf, which explicitly excludes both the node's own peer ID and previously visited peers from candidate selection, regardless of whether self was properly added to the visited list by upstream callers.
Key Changes:
- Renamed
VisitedPeerIdstoSkipListWithSelfwith an addedself_peerfield - Updated
Containstrait implementations to check both self and visited peers - Added comprehensive unit test to verify self-exclusion behavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iduartgomez
reviewed
Nov 25, 2025
| // Use SkipListWithSelf to explicitly exclude ourselves from candidates, | ||
| // ensuring we never select ourselves as a forwarding target even if | ||
| // self wasn't added to visited by upstream callers. | ||
| let skip = SkipListWithSelf { |
Collaborator
There was a problem hiding this comment.
future suggestion: should we refactor this to be used in other ops as well?
iduartgomez
approved these changes
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
PR #2140 added a defense-in-depth guard in
should_accept()to reject self-connections. However, as Nacho pointed out in review feedback, the root fix should happen earlier: when a gateway picks candidates to forward a connect request, it should never even consider itself as a target.The issue was that
select_next_hop()used a skip list built only from thevisitedpeers in the incoming request. If for any reasonselfwasn't properly added tovisitedby upstream callers, the node could select itself as a forwarding target.This Solution
Introduce
SkipListWithSelf, a skip list that explicitly combines visited peers with the current node's own peer ID. This ensures:should_accept()guard from fix: add self-connection guard in should_accept() #2140 remains as defense-in-depthThe key insight is that excluding self should not depend on upstream callers correctly populating the visited list - it should be a hard guarantee at the point where we select candidates.
Changes
VisitedPeerIdswithSkipListWithSelfinselect_next_hop()SkipListWithSelfchecks both&target == self.self_peerAND the visited listTesting
skip_list_with_self_excludes_self_and_visitedtestAddresses
Feedback from @iduartgomez on https://github.com/freenet/freenet-core/pull/2140/files#r2559449449
[AI-assisted - Claude]