From 9a423720839884461972220438a4d8e4070a1ebb Mon Sep 17 00:00:00 2001 From: achingbrain Date: Tue, 23 Apr 2024 12:55:05 +0100 Subject: [PATCH 1/3] fix: return closest peers from FIND_NODE 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 --- .../kad-dht/src/rpc/handlers/find-node.ts | 18 ++--------- .../kad-dht/src/rpc/handlers/get-providers.ts | 2 +- packages/kad-dht/test/kad-dht.spec.ts | 31 +++++++++++++++++++ .../test/rpc/handlers/find-node.spec.ts | 30 +++++++++--------- packages/kad-dht/test/rpc/index.node.ts | 2 -- 5 files changed, 48 insertions(+), 35 deletions(-) diff --git a/packages/kad-dht/src/rpc/handlers/find-node.ts b/packages/kad-dht/src/rpc/handlers/find-node.ts index 162a8ce775..8ad6bf8c08 100644 --- 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,7 +14,6 @@ export interface FindNodeHandlerInit { export interface FindNodeHandlerComponents { peerId: PeerId - addressManager: AddressManager logger: ComponentLogger } @@ -25,7 +21,6 @@ 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) { @@ -33,7 +28,6 @@ export class FindNodeHandler implements DHTMessageHandler { 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,20 +38,11 @@ export class FindNodeHandler implements DHTMessageHandler { async handle (peerId: PeerId, msg: Message): Promise { 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, @@ -65,6 +50,7 @@ export class FindNodeHandler implements DHTMessageHandler { 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 index ef13fa5353..f8bb968626 100644 --- 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 { 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 index 70d331212c..73df181518 100644 --- 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> = [] + + 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 index 8ca6535470..132278f111 100644 --- a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts +++ b/packages/kad-dht/test/rpc/handlers/find-node.spec.ts @@ -5,7 +5,6 @@ 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' @@ -13,8 +12,6 @@ import { passthroughMapper, removePrivateAddressesMapper, removePublicAddressesM 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 - let addressManager: StubbedInstance beforeEach(async () => { peerId = await createPeerId() sourcePeer = await createPeerId() targetPeer = await createPeerId() peerRouting = Sinon.createStubInstance(PeerRouting) - addressManager = stubInterface() handler = new FindNodeHandler({ peerId, - addressManager, logger: defaultLogger() }, { peerRouting, @@ -44,7 +38,7 @@ describe('rpc - handlers - FindNode', () => { }) }) - it('returns self, if asked for self', async () => { + it('returns nodes close to self but not self, if asked for self', async () => { const msg: Message = { type: T, key: peerId.multihash.bytes, @@ -52,13 +46,18 @@ describe('rpc - handlers - FindNode', () => { 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, + 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 +66,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 +138,6 @@ describe('rpc - handlers - FindNode', () => { handler = new FindNodeHandler({ peerId, - addressManager, logger: defaultLogger() }, { peerRouting, @@ -181,7 +180,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 index c4886d22dc..8233b04f22 100644 --- 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(), - addressManager: stubInterface(), logger: defaultLogger() } components.peerStore = new PersistentPeerStore({ From e74406588ea3b3568a437d31332d50cbbc62e2c9 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Tue, 23 Apr 2024 12:58:22 +0100 Subject: [PATCH 2/3] chore: test for self too --- packages/kad-dht/test/rpc/handlers/find-node.spec.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts b/packages/kad-dht/test/rpc/handlers/find-node.spec.ts index 132278f111..1994e720b3 100644 --- a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts +++ b/packages/kad-dht/test/rpc/handlers/find-node.spec.ts @@ -49,7 +49,14 @@ describe('rpc - handlers - FindNode', () => { peerRouting.getCloserPeersOffline .withArgs(peerId.multihash.bytes, peerId) .resolves([{ - id: targetPeer, + 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'), From a3a6be8fa8018de4b5d02700ec6a1dd322db83d7 Mon Sep 17 00:00:00 2001 From: Alex Potsides Date: Wed, 24 Apr 2024 07:17:32 +0100 Subject: [PATCH 3/3] chore: apply suggestions from code review Co-authored-by: Chad Nehemiah --- packages/kad-dht/test/rpc/handlers/find-node.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts b/packages/kad-dht/test/rpc/handlers/find-node.spec.ts index 1994e720b3..e4ed94d6be 100644 --- a/packages/kad-dht/test/rpc/handlers/find-node.spec.ts +++ b/packages/kad-dht/test/rpc/handlers/find-node.spec.ts @@ -38,7 +38,7 @@ describe('rpc - handlers - FindNode', () => { }) }) - it('returns nodes close to self but not 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,