fix: return closest peers from FIND_NODE

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

@@ -1,13 +1,10 @@
 import { CodeError } from '@libp2p/interface'
-import { protocols } from '@multiformats/multiaddr'
-import { equals as uint8ArrayEquals } from 'uint8arrays'
 import { MessageType } from '../../message/dht.js'
 import type { PeerInfoMapper } from '../../index.js'
 import type { Message } from '../../message/dht.js'
 import type { PeerRouting } from '../../peer-routing/index.js'
 import type { DHTMessageHandler } from '../index.js'
 import type { ComponentLogger, Logger, PeerId, PeerInfo } from '@libp2p/interface'
-import type { AddressManager } from '@libp2p/interface-internal'
 
 export interface FindNodeHandlerInit {
   peerRouting: PeerRouting
@@ -17,23 +14,20 @@ export interface FindNodeHandlerInit {
 
 export interface FindNodeHandlerComponents {
   peerId: PeerId
-  addressManager: AddressManager
   logger: ComponentLogger
 }
 
 export class FindNodeHandler implements DHTMessageHandler {
   private readonly peerRouting: PeerRouting
   private readonly peerInfoMapper: PeerInfoMapper
   private readonly peerId: PeerId
-  private readonly addressManager: AddressManager
   private readonly log: Logger
 
   constructor (components: FindNodeHandlerComponents, init: FindNodeHandlerInit) {
     const { peerRouting, logPrefix } = init
 
     this.log = components.logger.forComponent(`${logPrefix}:rpc:handlers:find-node`)
     this.peerId = components.peerId
-    this.addressManager = components.addressManager
     this.peerRouting = peerRouting
     this.peerInfoMapper = init.peerInfoMapper
   }
@@ -44,27 +38,19 @@ export class FindNodeHandler implements DHTMessageHandler {
   async handle (peerId: PeerId, msg: Message): Promise<Message> {
     this.log('incoming request from %p for peers closer to %b', peerId, msg.key)
 
-    let closer: PeerInfo[] = []
-
     if (msg.key == null) {
       throw new CodeError('Invalid FIND_NODE message received - key was missing', 'ERR_INVALID_MESSAGE')
     }
 
-    if (uint8ArrayEquals(this.peerId.toBytes(), msg.key)) {
-      closer = [{
-        id: this.peerId,
-        multiaddrs: this.addressManager.getAddresses().map(ma => ma.decapsulateCode(protocols('p2p').code))
-      }]
-    } else {
-      closer = await this.peerRouting.getCloserPeersOffline(msg.key, peerId)
-    }
+    const closer: PeerInfo[] = await this.peerRouting.getCloserPeersOffline(msg.key, peerId)
 
     const response: Message = {
       type: MessageType.FIND_NODE,
       clusterLevel: msg.clusterLevel,
       closer: closer
         .map(this.peerInfoMapper)
         .filter(({ multiaddrs }) => multiaddrs.length)
+        .filter(({ id }) => !id.equals(this.peerId))
         .map(peerInfo => ({
           id: peerInfo.id.toBytes(),
           multiaddrs: peerInfo.multiaddrs.map(ma => ma.bytes)

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

@@ -40,7 +40,7 @@
 
   async handle (peerId: PeerId, msg: Message): Promise<Message> {
     if (msg.key == null) {
-      throw new CodeError('Invalid FIND_NODE message received - key was missing', 'ERR_INVALID_MESSAGE')
+      throw new CodeError('Invalid GET_PROVIDERS message received - key was missing', 'ERR_INVALID_MESSAGE')
     }
 
     let cid

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

@@ -799,6 +799,37 @@ describe('KadDHT', () => {
 
       expect(res).to.not.be.empty()
     })
+
+    it('should not include itself in getClosestPeers PEER_RESPONSE', async function () {
+      this.timeout(240 * 1000)
+
+      const nDHTs = 30
+      const dhts = await Promise.all(
+        new Array(nDHTs).fill(0).map(async () => tdht.spawn())
+      )
+
+      const connected: Array<Promise<void>> = []
+
+      for (let i = 0; i < dhts.length - 1; i++) {
+        connected.push(tdht.connect(dhts[i], dhts[(i + 1) % dhts.length]))
+      }
+
+      await Promise.all(connected)
+
+      const res = await all(dhts[1].getClosestPeers(dhts[2].components.peerId.toBytes()))
+      expect(res).to.not.be.empty()
+
+      // no peer should include itself in the response, only other peers that it
+      // knows who are closer
+      for (const event of res) {
+        if (event.name !== 'PEER_RESPONSE') {
+          continue
+        }
+
+        expect(event.closer.map(peer => peer.id.toString()))
+          .to.not.include(event.from.toString())
+      }
+    })
   })
 
   describe('errors', () => {

packages/kad-dht/test/rpc/handlers/find-node.spec.ts

@@ -5,16 +5,13 @@ import { peerIdFromBytes } from '@libp2p/peer-id'
 import { multiaddr } from '@multiformats/multiaddr'
 import { expect } from 'aegir/chai'
 import Sinon, { type SinonStubbedInstance } from 'sinon'
-import { stubInterface } from 'sinon-ts'
 import { type Message, MessageType } from '../../../src/message/dht.js'
 import { PeerRouting } from '../../../src/peer-routing/index.js'
 import { FindNodeHandler } from '../../../src/rpc/handlers/find-node.js'
 import { passthroughMapper, removePrivateAddressesMapper, removePublicAddressesMapper } from '../../../src/utils.js'
 import { createPeerId } from '../../utils/create-peer-id.js'
 import type { DHTMessageHandler } from '../../../src/rpc/index.js'
 import type { PeerId } from '@libp2p/interface'
-import type { AddressManager } from '@libp2p/interface-internal'
-import type { StubbedInstance } from 'sinon-ts'
 
 const T = MessageType.FIND_NODE
 
@@ -24,18 +21,15 @@ describe('rpc - handlers - FindNode', () => {
   let targetPeer: PeerId
   let handler: DHTMessageHandler
   let peerRouting: SinonStubbedInstance<PeerRouting>
-  let addressManager: StubbedInstance<AddressManager>
 
   beforeEach(async () => {
     peerId = await createPeerId()
     sourcePeer = await createPeerId()
     targetPeer = await createPeerId()
     peerRouting = Sinon.createStubInstance(PeerRouting)
-    addressManager = stubInterface<AddressManager>()
 
     handler = new FindNodeHandler({
       peerId,
-      addressManager,
       logger: defaultLogger()
     }, {
       peerRouting,
@@ -44,21 +38,33 @@ describe('rpc - handlers - FindNode', () => {
     })
   })
 
-  it('returns self, if asked for self', async () => {
+  it('returns nodes close to self but excludes self, if asked for self', async () => {
     const msg: Message = {
       type: T,
       key: peerId.multihash.bytes,
       closer: [],
       providers: []
     }
 
-    addressManager.getAddresses.returns([
-      multiaddr(`/ip4/127.0.0.1/tcp/4002/p2p/${peerId.toString()}`),
-      multiaddr(`/ip4/192.168.1.5/tcp/4002/p2p/${peerId.toString()}`),
-      multiaddr(`/ip4/221.4.67.0/tcp/4002/p2p/${peerId.toString()}`)
-    ])
+    peerRouting.getCloserPeersOffline
+      .withArgs(peerId.multihash.bytes, peerId)
+      .resolves([{
+        id: targetPeer, // closer peer
+        multiaddrs: [
+          multiaddr('/ip4/127.0.0.1/tcp/4002'),
+          multiaddr('/ip4/192.168.1.5/tcp/4002'),
+          multiaddr('/ip4/221.4.67.0/tcp/4002')
+        ]
+      }, {
+        id: peerId, // self peer
+        multiaddrs: [
+          multiaddr('/ip4/127.0.0.1/tcp/4002'),
+          multiaddr('/ip4/192.168.1.5/tcp/4002'),
+          multiaddr('/ip4/221.4.67.0/tcp/4002')
+        ]
+      }])
 
-    const response = await handler.handle(sourcePeer, msg)
+    const response = await handler.handle(peerId, msg)
 
     if (response == null) {
       throw new Error('No response received from handler')
@@ -67,7 +73,8 @@ describe('rpc - handlers - FindNode', () => {
     expect(response.closer).to.have.length(1)
     const peer = response.closer[0]
 
-    expect(peerIdFromBytes(peer.id).toString()).to.equal(peerId.toString())
+    expect(peerIdFromBytes(peer.id).toString()).to.equal(targetPeer.toString())
+    expect(peer.multiaddrs).to.not.be.empty()
   })
 
   it('returns closer peers', async () => {
@@ -138,7 +145,6 @@ describe('rpc - handlers - FindNode', () => {
 
     handler = new FindNodeHandler({
       peerId,
-      addressManager,
       logger: defaultLogger()
     }, {
       peerRouting,
@@ -181,7 +187,6 @@ describe('rpc - handlers - FindNode', () => {
 
     handler = new FindNodeHandler({
       peerId,
-      addressManager,
       logger: defaultLogger()
     }, {
       peerRouting,

packages/kad-dht/test/rpc/index.node.ts

@@ -24,7 +24,6 @@ import { passthroughMapper } from '../../src/utils.js'
 import { createPeerId } from '../utils/create-peer-id.js'
 import type { Validators } from '../../src/index.js'
 import type { Libp2pEvents, Connection, PeerId, PeerStore } from '@libp2p/interface'
-import type { AddressManager } from '@libp2p/interface-internal'
 import type { Datastore } from 'interface-datastore'
 import type { Duplex, Source } from 'it-stream-types'
 
@@ -45,7 +44,6 @@ describe('rpc', () => {
       peerId,
       datastore,
       peerStore: stubInterface<PeerStore>(),
-      addressManager: stubInterface<AddressManager>(),
       logger: defaultLogger()
     }
     components.peerStore = new PersistentPeerStore({