fix: return closest peers from FIND_NODE (#2499)

The `FIND_NODE` DHT operation should return the closest peers the node knows to the value. It does not need to return itself in the list because the calling peer already knows about it. Fixes #2450 --------- Co-authored-by: Chad Nehemiah <[email protected]>
libp2p · Apr 24, 2024 · 0d55966 · 0d55966
1 parent 0d5d966
commit 0d55966
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 35 deletions.
diff --git a/packages/kad-dht/src/rpc/handlers/find-node.ts b/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)

diff --git a/packages/kad-dht/src/rpc/handlers/get-providers.ts b/packages/kad-dht/src/rpc/handlers/get-providers.ts
@@ -40,7 +40,7 @@ export class GetProvidersHandler implements DHTMessageHandler {
 
   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

diff --git a/packages/kad-dht/test/kad-dht.spec.ts b/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', () => {

diff --git a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts b/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,

diff --git a/packages/kad-dht/test/rpc/index.node.ts b/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({