Skip to content

Commit

Permalink
fix: return self in FIND_NODE for self (#2512)
Browse files Browse the repository at this point in the history
Partial revert of #2499

If a node is queried for it's own peer id, return it's own peer
info.

This is necessary because since libp2p/go-libp2p-kad-dht#820
go-libp2p-kad-dht won't add a peer to it's routing tables that
doesn't have any DHT peers that are KAD-futher from it's own ID
already.
  • Loading branch information
achingbrain committed Apr 29, 2024
1 parent 4afd7a9 commit 9e9a32b
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 12 deletions.
2 changes: 1 addition & 1 deletion packages/kad-dht/src/peer-routing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export class PeerRouting {
if (output.length > 0) {
this.log('getCloserPeersOffline found %d peer(s) closer to %b than %p', output.length, key, closerThan)
} else {
this.log('getCloserPeersOffline could not find peer closer to %b than %p', key, closerThan)
this.log('getCloserPeersOffline could not find peer closer to %b than %p with %d peers in the routing table', key, closerThan, this.routingTable.size)
}

return output
Expand Down
14 changes: 13 additions & 1 deletion packages/kad-dht/src/rpc/handlers/find-node.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
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
Expand All @@ -14,20 +17,23 @@ 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
}
Expand All @@ -44,13 +50,19 @@ export class FindNodeHandler implements DHTMessageHandler {

const closer: PeerInfo[] = await this.peerRouting.getCloserPeersOffline(msg.key, peerId)

if (uint8ArrayEquals(this.peerId.toBytes(), msg.key)) {
closer.push({
id: this.peerId,
multiaddrs: this.addressManager.getAddresses().map(ma => ma.decapsulateCode(protocols('p2p').code))
})
}

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)
Expand Down
2 changes: 1 addition & 1 deletion packages/kad-dht/test/kad-dht.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -800,7 +800,7 @@ describe('KadDHT', () => {
expect(res).to.not.be.empty()
})

it('should not include itself in getClosestPeers PEER_RESPONSE', async function () {
it.skip('should not include itself in getClosestPeers PEER_RESPONSE', async function () {
this.timeout(240 * 1000)

const nDHTs = 30
Expand Down
28 changes: 19 additions & 9 deletions packages/kad-dht/test/rpc/handlers/find-node.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ 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

Expand All @@ -21,15 +24,18 @@ 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,
Expand All @@ -38,14 +44,20 @@ describe('rpc - handlers - FindNode', () => {
})
})

it('returns nodes close to self but excludes self, if asked for self', async () => {
it('returns nodes close to self and includes 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'),
multiaddr('/ip4/192.168.1.5/tcp/4002'),
multiaddr('/ip4/221.4.67.0/tcp/4002')
])

peerRouting.getCloserPeersOffline
.withArgs(peerId.multihash.bytes, peerId)
.resolves([{
Expand All @@ -55,13 +67,6 @@ describe('rpc - handlers - FindNode', () => {
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(peerId, msg)
Expand All @@ -70,11 +75,14 @@ describe('rpc - handlers - FindNode', () => {
throw new Error('No response received from handler')
}

expect(response.closer).to.have.length(1)
expect(response.closer).to.have.length(2)
const peer = response.closer[0]

expect(peerIdFromBytes(peer.id).toString()).to.equal(targetPeer.toString())
expect(peer.multiaddrs).to.not.be.empty()

const self = response.closer[1]
expect(peerIdFromBytes(self.id).toString()).to.equal(peerId.toString())
})

it('returns closer peers', async () => {
Expand Down Expand Up @@ -145,6 +153,7 @@ describe('rpc - handlers - FindNode', () => {

handler = new FindNodeHandler({
peerId,
addressManager,
logger: defaultLogger()
}, {
peerRouting,
Expand Down Expand Up @@ -187,6 +196,7 @@ describe('rpc - handlers - FindNode', () => {

handler = new FindNodeHandler({
peerId,
addressManager,
logger: defaultLogger()
}, {
peerRouting,
Expand Down
2 changes: 2 additions & 0 deletions packages/kad-dht/test/rpc/index.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ 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'

Expand All @@ -44,6 +45,7 @@ describe('rpc', () => {
peerId,
datastore,
peerStore: stubInterface<PeerStore>(),
addressManager: stubInterface<AddressManager>(),
logger: defaultLogger()
}
components.peerStore = new PersistentPeerStore({
Expand Down

0 comments on commit 9e9a32b

Please sign in to comment.