fix: return closest known peers, even if they are not closer (#3182)

Aligns with Kad-DHT spec PR, when responding to a `FIND_NODE` query,
return the `k` known closest peers, excluding the server and the
requester, even if they are not closer than the requester.

Author: achingbrain

packages/kad-dht/src/peer-routing/index.ts

@@ -3,8 +3,6 @@ import { InvalidPublicKeyError, NotFoundError } from '@libp2p/interface'
 import { peerIdFromPublicKey, peerIdFromMultihash } from '@libp2p/peer-id'
 import { Libp2pRecord } from '@libp2p/record'
 import * as Digest from 'multiformats/hashes/digest'
-import { xor as uint8ArrayXor } from 'uint8arrays/xor'
-import { xorCompare as uint8ArrayXorCompare } from 'uint8arrays/xor-compare'
 import { QueryError, InvalidRecordError } from '../errors.js'
 import { MessageType } from '../message/dht.js'
 import { PeerDistanceList } from '../peer-distance-list.js'
@@ -14,13 +12,14 @@ import {
   valueEvent
 } from '../query/events.js'
 import { verifyRecord } from '../record/validators.js'
-import { convertBuffer, convertPeerId, keyForPublicKey } from '../utils.js'
+import { convertBuffer, keyForPublicKey } from '../utils.js'
 import type { DHTRecord, FinalPeerEvent, QueryEvent, Validators } from '../index.js'
 import type { Message } from '../message/dht.js'
 import type { Network, SendMessageOptions } from '../network.js'
 import type { QueryManager, QueryOptions } from '../query/manager.js'
 import type { QueryFunc } from '../query/types.js'
 import type { RoutingTable } from '../routing-table/index.js'
+import type { GetClosestPeersOptions } from '../routing-table/k-bucket.ts'
 import type { ComponentLogger, Logger, Metrics, PeerId, PeerInfo, PeerStore, RoutingOptions } from '@libp2p/interface'
 import type { ConnectionManager } from '@libp2p/interface-internal'
 import type { AbortOptions } from 'it-pushable'
@@ -246,7 +245,7 @@ export class PeerRouting {
     const self = this
 
     const getCloserPeersQuery: QueryFunc = async function * ({ peer, path, peerKadId, signal }) {
-      self.log('getClosestPeers asking %p', peer)
+      self.log('getClosestPeers asking %p', peer.id)
       const request: Partial<Message> = {
         type: MessageType.FIND_NODE,
         key
@@ -336,10 +335,9 @@ export class PeerRouting {
   }
 
   /**
-   * Get the peers in our routing table that are closer than the passed PeerId
-   * to the passed key
+   * Get the peers in our routing table that are closest to the passed key
    */
-  async getCloserPeersOffline (key: Uint8Array, closerThan: PeerId, options?: AbortOptions): Promise<PeerInfo[]> {
+  async getClosestPeersOffline (key: Uint8Array, options?: GetClosestPeersOptions): Promise<PeerInfo[]> {
     const output: PeerInfo[] = []
 
     // try getting the peer directly
@@ -356,19 +354,9 @@ export class PeerRouting {
     } catch {}
 
     const keyKadId = await convertBuffer(key, options)
-    const ids = this.routingTable.closestPeers(keyKadId)
-    const closerThanKadId = await convertPeerId(closerThan, options)
-    const requesterXor = uint8ArrayXor(closerThanKadId, keyKadId)
+    const ids = this.routingTable.closestPeers(keyKadId, options)
 
     for (const peerId of ids) {
-      const peerKadId = await convertPeerId(peerId, options)
-      const peerXor = uint8ArrayXor(peerKadId, keyKadId)
-
-      // only include if peer is closer than requester
-      if (uint8ArrayXorCompare(peerXor, requesterXor) !== -1) {
-        continue
-      }
-
       try {
         output.push(await this.components.peerStore.getInfo(peerId, options))
       } catch (err: any) {
@@ -379,9 +367,9 @@ export class PeerRouting {
     }
 
     if (output.length > 0) {
-      this.log('getCloserPeersOffline found %d peer(s) closer to %b than %p', output.length, key, closerThan)
+      this.log('getClosestPeersOffline returning the %d closest peer(s) %b we know', output.length, key)
     } else {
-      this.log('getCloserPeersOffline could not find peer closer to %b than %p with %d peers in the routing table', key, closerThan, this.routingTable.size)
+      this.log('getClosestPeersOffline could not any peers close to %b with %d peers in the routing table', key, this.routingTable.size)
     }
 
     return output

packages/kad-dht/src/query/manager.ts

@@ -168,7 +168,9 @@ export class QueryManager implements Startable {
       const id = await convertBuffer(key, {
         signal
       })
-      const peers = this.routingTable.closestPeers(id, this.routingTable.kBucketSize)
+      const peers = this.routingTable.closestPeers(id, {
+        count: this.routingTable.kBucketSize
+      })
 
       // split peers into d buckets evenly(ish)
       const peersToQuery = peers.sort(() => {

packages/kad-dht/src/routing-table/index.ts

@@ -7,7 +7,7 @@ import { TypedEventEmitter, setMaxListeners } from 'main-event'
 import * as utils from '../utils.js'
 import { ClosestPeers } from './closest-peers.js'
 import { KBucket, isLeafBucket } from './k-bucket.js'
-import type { Bucket, LeafBucket, Peer } from './k-bucket.js'
+import type { Bucket, GetClosestPeersOptions, LeafBucket, Peer } from './k-bucket.js'
 import type { Network } from '../network.js'
 import type { AbortOptions, ComponentLogger, CounterGroup, Logger, Metric, Metrics, PeerId, PeerStore, Startable, Stream } from '@libp2p/interface'
 import type { Ping } from '@libp2p/ping'
@@ -444,7 +444,9 @@ export class RoutingTable extends TypedEventEmitter<RoutingTableEvents> implemen
    * Retrieve the closest peers to the given kadId
    */
   closestPeer (kadId: Uint8Array): PeerId | undefined {
-    const res = this.closestPeers(kadId, 1)
+    const res = this.closestPeers(kadId, {
+      count: 1
+    })
 
     if (res.length > 0) {
       return res[0]
@@ -456,12 +458,12 @@ export class RoutingTable extends TypedEventEmitter<RoutingTableEvents> implemen
   /**
    * Retrieve the `count`-closest peers to the given kadId
    */
-  closestPeers (kadId: Uint8Array, count = this.kBucketSize): PeerId[] {
+  closestPeers (kadId: Uint8Array, options?: GetClosestPeersOptions): PeerId[] {
     if (this.kb == null) {
       return []
     }
 
-    return [...this.kb.closest(kadId, count)]
+    return [...this.kb.closest(kadId, options)]
   }
 
   /**

packages/kad-dht/src/routing-table/k-bucket.ts

@@ -130,6 +130,11 @@ export function isLeafBucket (obj: any): obj is LeafBucket {
   return Array.isArray(obj?.peers)
 }
 
+export interface GetClosestPeersOptions extends AbortOptions {
+  count?: number
+  exclude?: PeerId[]
+}
+
 /**
  * Implementation of a Kademlia DHT routing table as a prefix binary trie with
  * configurable prefix length, bucket split threshold and size.
@@ -318,10 +323,14 @@ export class KBucket {
    * @param {Uint8Array} id - Contact node id
    * @returns {Generator<Peer, void, undefined>} Array Maximum of n closest contacts to the node id
    */
-  * closest (id: Uint8Array, n: number = this.kBucketSize): Generator<PeerId, void, undefined> {
-    const list = new PeerDistanceList(id, n)
+  * closest (id: Uint8Array, options?: GetClosestPeersOptions): Generator<PeerId, void, undefined> {
+    const list = new PeerDistanceList(id, options?.count ?? this.kBucketSize)
 
     for (const peer of this.toIterable()) {
+      if (options?.exclude?.some(p => p.equals(peer.peerId)) === true) {
+        continue
+      }
+
       list.addWithKadId({ id: peer.peerId, multiaddrs: [] }, peer.kadId)
     }
 

packages/kad-dht/src/rpc/handlers/find-node.ts

@@ -42,38 +42,52 @@ export class FindNodeHandler implements DHTMessageHandler {
    * Process `FindNode` DHT messages
    */
   async handle (peerId: PeerId, msg: Message): Promise<Message> {
-    this.log('incoming request from %p for peers closer to %b', peerId, msg.key)
+    this.log('incoming request from %p for peers close to %b', peerId, msg.key)
+    try {
+      if (msg.key == null) {
+        throw new InvalidMessageError('Invalid FIND_NODE message received - key was missing')
+      }
 
-    if (msg.key == null) {
-      throw new InvalidMessageError('Invalid FIND_NODE message received - key was missing')
-    }
-
-    const closer: PeerInfo[] = await this.peerRouting.getCloserPeersOffline(msg.key, peerId)
+      const closer: PeerInfo[] = await this.peerRouting.getClosestPeersOffline(msg.key, {
+        exclude: [
+        // never tell a peer about itself
+          peerId,
 
-    if (uint8ArrayEquals(this.peerId.toMultihash().bytes, msg.key)) {
-      closer.push({
-        id: this.peerId,
-        multiaddrs: this.addressManager.getAddresses().map(ma => ma.decapsulateCode(protocols('p2p').code))
+          // do not include the server in the results
+          this.peerId
+        ]
       })
-    }
 
-    const response: Message = {
-      type: MessageType.FIND_NODE,
-      clusterLevel: msg.clusterLevel,
-      closer: closer
-        .map(this.peerInfoMapper)
-        .filter(({ multiaddrs }) => multiaddrs.length)
-        .map(peerInfo => ({
-          id: peerInfo.id.toMultihash().bytes,
-          multiaddrs: peerInfo.multiaddrs.map(ma => ma.bytes)
-        })),
-      providers: []
-    }
+      if (uint8ArrayEquals(this.peerId.toMultihash().bytes, msg.key)) {
+        closer.push({
+          id: this.peerId,
+          multiaddrs: this.addressManager.getAddresses().map(ma => ma.decapsulateCode(protocols('p2p').code))
+        })
+      }
 
-    if (response.closer.length === 0) {
-      this.log('could not find any peers closer to %b than %p', msg.key, peerId)
-    }
+      const response: Message = {
+        type: MessageType.FIND_NODE,
+        clusterLevel: msg.clusterLevel,
+        closer: closer
+          .map(this.peerInfoMapper)
+          .filter(({ multiaddrs }) => multiaddrs.length)
+          .map(peerInfo => ({
+            id: peerInfo.id.toMultihash().bytes,
+            multiaddrs: peerInfo.multiaddrs.map(ma => ma.bytes)
+          })),
+        providers: []
+      }
+
+      if (response.closer.length === 0) {
+        this.log('could not find any peers closer to %b for %p', msg.key, peerId)
+      } else {
+        this.log('found %d peers close to %b for %p', response.closer.length, msg.key, peerId)
+      }
 
-    return response
+      return response
+    } catch (err: any) {
+      this.log('error during finding peers closer to %b for %p - %e', msg.key, peerId, err)
+      throw err
+    }
   }
 }

packages/kad-dht/src/rpc/handlers/get-providers.ts

@@ -67,7 +67,7 @@ export class GetProvidersHandler implements DHTMessageHandler {
 
         return info
       })),
-      this.peerRouting.getCloserPeersOffline(msg.key, peerId)
+      this.peerRouting.getClosestPeersOffline(msg.key)
     ])
 
     const response: Message = {

packages/kad-dht/src/rpc/handlers/get-value.ts

@@ -84,7 +84,7 @@ export class GetValueHandler implements DHTMessageHandler {
 
     const [record, closer] = await Promise.all([
       this._checkLocalDatastore(key),
-      this.peerRouting.getCloserPeersOffline(key, peerId)
+      this.peerRouting.getClosestPeersOffline(key)
     ])
 
     if (record != null) {

packages/kad-dht/test/peer-routing.spec.ts

@@ -65,8 +65,7 @@ describe('peer-routing', () => {
     it('should only return DHT servers', async () => {
       const key = Uint8Array.from([0, 1, 2, 3, 4])
       const [
-        serverPeerId,
-        requester
+        serverPeerId
       ] = await getSortedPeers(key)
 
       const serverPeer: Peer = stubInterface<Peer>({
@@ -86,7 +85,7 @@ describe('peer-routing', () => {
         multiaddrs: serverPeer.addresses.map(({ multiaddr }) => multiaddr)
       })
 
-      const closer = await peerRouting.getCloserPeersOffline(key, requester.peerId)
+      const closer = await peerRouting.getClosestPeersOffline(key)
 
       expect(closer).to.have.lengthOf(1)
       expect(closer[0].id).to.equal(serverPeer.id)
@@ -96,8 +95,7 @@ describe('peer-routing', () => {
       const clientPeerId = peerIdFromPrivateKey(await generateKeyPair('Ed25519'))
       const key = clientPeerId.toMultihash().bytes
       const [
-        serverPeerId,
-        requester
+        serverPeerId
       ] = await getSortedPeers(key)
 
       const clientPeer: Peer = stubInterface<Peer>({
@@ -130,58 +128,12 @@ describe('peer-routing', () => {
         multiaddrs: clientPeer.addresses.map(({ multiaddr }) => multiaddr)
       })
 
-      const closer = await peerRouting.getCloserPeersOffline(key, requester.peerId)
+      const closer = await peerRouting.getClosestPeersOffline(key)
 
       expect(closer).to.have.lengthOf(2)
       expect(closer[0].id).to.equal(clientPeer.id)
       expect(closer[1].id).to.equal(serverPeer.id)
     })
-
-    // this is not in the spec
-    it.skip('should only include peers closer than the requesting peer', async () => {
-      const key = Uint8Array.from([0, 1, 2, 3, 4])
-      const [
-        closerPeerId,
-        requester,
-        furtherPeerId
-      ] = await getSortedPeers(key)
-
-      const closerPeer: Peer = stubInterface<Peer>({
-        id: closerPeerId.peerId,
-        addresses: [{
-          isCertified: true,
-          multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4001')
-        }]
-      })
-      const furtherPeer: Peer = stubInterface<Peer>({
-        id: furtherPeerId.peerId,
-        addresses: [{
-          isCertified: true,
-          multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4002')
-        }]
-      })
-
-      init.routingTable.closestPeers.returns([
-        closerPeer.id,
-        furtherPeer.id
-      ])
-
-      components.peerStore.get.withArgs(closerPeer.id).resolves(closerPeer)
-      components.peerStore.getInfo.withArgs(closerPeer.id).resolves({
-        id: closerPeer.id,
-        multiaddrs: closerPeer.addresses.map(({ multiaddr }) => multiaddr)
-      })
-      components.peerStore.get.withArgs(furtherPeer.id).resolves(furtherPeer)
-      components.peerStore.getInfo.withArgs(furtherPeer.id).resolves({
-        id: furtherPeer.id,
-        multiaddrs: furtherPeer.addresses.map(({ multiaddr }) => multiaddr)
-      })
-
-      const closer = await peerRouting.getCloserPeersOffline(key, requester.peerId)
-
-      expect(closer).to.have.lengthOf(1)
-      expect(closer[0].id).to.equal(closerPeer.id)
-    })
   })
 })
 

packages/kad-dht/test/routing-table.spec.ts

@@ -210,7 +210,9 @@ describe('Routing Table', () => {
         const id = ids[random(ids.length - 1)]
         const key = await kadUtils.convertPeerId(id.peerId)
 
-        expect(table.closestPeers(key, 5).length)
+        expect(table.closestPeers(key, {
+          count: 5
+        }).length)
           .to.be.above(0)
       })
     )
@@ -291,7 +293,9 @@ describe('Routing Table', () => {
     }))
 
     const key = await kadUtils.convertPeerId(peers[2].peerId)
-    expect(table.closestPeers(key, 10)).to.have.length(10)
+    expect(table.closestPeers(key, {
+      count: 10
+    })).to.have.length(10)
     await expect(table.find(peers[5].peerId)).to.eventually.be.ok()
     expect(table.size).to.equal(10)
 
@@ -329,7 +333,9 @@ describe('Routing Table', () => {
     await Promise.all(peers.map(async (peer) => { await table.add(peer.peerId) }))
 
     const key = await kadUtils.convertPeerId(peers[2].peerId)
-    expect(table.closestPeers(key, 15)).to.have.length(15)
+    expect(table.closestPeers(key, {
+      count: 15
+    })).to.have.length(15)
   })
 
   it('favours old peers that respond to pings', async () => {