fix(kad): exclude the requester from closerPeers

[gmelodie]

Summary

findClosestPeers built the closerPeers list for FIND_NODE, GET_VALUE and GET_PROVIDERS replies while filtering out only self, never the sender. When the sender was in our routing table and landed among the k closest to the target key, we handed it back its own peer id.

That is harmless against a client that filters its own id out, but it hung interop against py-libp2p (v0.6.0): on receiving its own id, py-libp2p opened a stream back to itself, sent FIND_NODE, and blocked waiting for a reply that a node in client mode never sends, stalling the whole lookup until the test timed out.

This PR excludes the requester as well as self, following go-libp2p's closestPeersToQuery. findClosestPeers takes the sender as a requester argument and the three handlers thread stream.peerId into it. It over-fetches from the routing table by the number of excluded keys and truncates back to replication, so dropping the sender does not shrink an otherwise full reply.

FIND_NODE additionally prepends the target itself when we know its addresses, which closes a real gap: a peer in client mode is in nobody's routing table, so findPeer could never resolve one. The target is looked up in the address book rather than the routing table, and is skipped when it is already the closest entry so it is not duplicated.

Affected Areas

• Peer Management / Discovery — which routing-table peers a node hands back in a reply, and whether a client-mode peer can be resolved at all.

• Protocol Logic — closerPeers construction in handleFindNode (find.nim), handleGetValue (get.nim) and handleGetProviders (provider.nim).

Compatibility & Downstream Validation

The behavior change is confined to kad-dht reply construction and only removes an entry that a correct client already discards, so no downstream integration branches were needed:

• Nimbus: N/A
• Waku: N/A
• Codex: N/A

Impact on Library Users

findClosestPeers* is exported and gains a required requester: PeerId parameter — a source-breaking signature change for anyone calling it directly. Every in-tree caller is a DHT handler, and no tests referenced it. There is no wire-format change.

Behaviorally: a node no longer receives itself in closerPeers, and FIND_NODE for a client-mode peer whose addresses we know now resolves instead of returning nothing.

Risk Assessment

• Backward compatibility: wire-compatible. Interop improves rather than regresses, since the removed entry is one every correct client already discards.
• Network behavior: replies still carry up to replication closest peers because the routing-table lookup over-fetches before filtering. FIND_NODE can return one extra entry when the target is prepended.
• Performance: one extra routing-table candidate sorted per reply, and one address-book lookup per FIND_NODE. The exclusion set is a stack array. Negligible.
• Security: none identified. The prepended target is only ever a peer we already hold addresses for.

References

• Closes kad-dht: should closerPeers exclude the requester? #2848
• go-libp2p closestPeersToQuery: https://github.com/libp2p/go-libp2p-kad-dht/blob/cb9639642a8d54f22a49e7b45b66fd7615f2bbf5/dht.go#L751-L779
• kad-dht spec, FIND_NODE / GET_PROVIDERS closerPeers: https://github.com/libp2p/specs/blob/6b6203ee6f62938ce67efdb33498173f475851c0/kad-dht/README.md?plain=1#L490-L505
• py-libp2p interop hang: feat: add nim to kad-dht and enable quic in hole-punch libp2p/unified-testing#119

Additional Notes

The issue asked about handleFindNode and handleGetProviders. handleGetValue builds closerPeers the same way and go excludes the sender there too, so it is included for consistency. The target prepend is FIND_NODE-only, matching go.

Deliberate divergence from go: go prepends the target even when the target is the requester, and its comment justifies this as "per spec: FIND_PEER has a special exception where the target peer MUST be included". The spec says no such thing — it contains no MUST at all, and FIND_NODE states only that closerPeers is "the k closest Peers". Adopting that half would re-arm the very hang this PR fixes: py-libp2p's refresh_routing_table performs a lookup for its own id, and v0.6.0 only filters self out of peerstore-seeded candidates, not out of peers received from a reply. It would dial itself and stall again. So the target is not prepended when it is the requester; a peer wanting its own addresses can use identify. "Find node for own PeerID excludes the requester" guards this.

nim is itself immune to that class of bug from either direction — sortedShortlist (find.nim) skips self when selecting peers to query, and RoutingTable.insert refuses self — but that is no reason to emit it to others.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.19%. Comparing base (8a7b54e) to head (b4fc4de).

Files with missing lines	Patch %	Lines
libp2p/protocols/kademlia/find.nim	84.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2850   +/-   ##
=======================================
  Coverage   81.19%   81.19%           
=======================================
  Files         170      170           
  Lines       30797    30797           
  Branches       12       13    +1     
=======================================
+ Hits        25005    25006    +1     
+ Misses       5792     5791    -1     

Files with missing lines	Coverage Δ
libp2p/protocols/kademlia/get.nim	61.98% <100.00%> (ø)
libp2p/protocols/kademlia/provider.nim	72.25% <100.00%> (ø)
libp2p/protocols/kademlia/find.nim	64.60% <84.00%> (+0.86%)	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rlve]

🚀

[richard-ramos]

While reviewing the diff between our impl and rust/go, i noticed a diff that is probably worth looking into later: we take the closerPeers that we receive and pass them to updatePeers, which insert them in the routing table, the problem is that these peers might not support kademlia at all. Go and Rust delay the insertion to the table until they confirm that they support kademlia.

I'll create a follow up issue

[richard-ramos]

Something to discuss tho:

• In go-libp2p you can find 'client-mode' targets
• In rust, you can find only 'servers'

In here, it seems we are trying both, trying to return clients like go, but then querying them,, however, this may not work for client-mode peers, because after receiving the target, we will send FIND_NODE to it, and client-mode peer will not reply, so we will remove it after retries, and findPeer will return "peer not found".

We probably should create a unit test for this scenario to confirm tho, but would like to know your thoughts

also cc: @rlve

[gmelodie]

In go it seems that an unreachable peer is still considered valid as it can be a client-only node.

By their own words, this is a bug https://github.com/libp2p/go-libp2p-kad-dht/blob/master/routing.go#L678-L687

The fact that a peer that is returned can be a non-DHT server peer and is not identified as such is a bug.