From 70c5d9378ffd1efce6fcdd2c6bdff264560d7f60 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 5 May 2023 12:08:16 +0100 Subject: [PATCH 1/3] fix: do not auto-dial peers in the dial queue Similar to #1730, filter auto-dial peers by peers already in the dial queue. The dial queue will join any pre-existing dial attempts for the selected peers, but it just creates unnecessary work and adds noise to the logs and filtering them out beforehand is cheap so just do that. --- src/connection-manager/auto-dial.ts | 13 ++- src/connection-manager/dial-queue.ts | 11 ++- test/connection-manager/auto-dial.spec.ts | 110 ++++++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) diff --git a/src/connection-manager/auto-dial.ts b/src/connection-manager/auto-dial.ts index 5f9b58854a..25fc19eb90 100644 --- a/src/connection-manager/auto-dial.ts +++ b/src/connection-manager/auto-dial.ts @@ -1,7 +1,7 @@ import { logger } from '@libp2p/logger' import type { PeerStore } from '@libp2p/interface-peer-store' import type { ConnectionManager } from '@libp2p/interface-connection-manager' -import { PeerMap } from '@libp2p/peer-collections' +import { PeerMap, PeerSet } from '@libp2p/peer-collections' import PQueue from 'p-queue' import { AUTO_DIAL_CONCURRENCY, AUTO_DIAL_INTERVAL, AUTO_DIAL_PRIORITY, MIN_CONNECTIONS } from './constants.js' import type { Startable } from '@libp2p/interfaces/startable' @@ -103,6 +103,12 @@ export class AutoDial implements Startable { const connections = this.connectionManager.getConnectionsMap() const numConnections = connections.size + const dialQueue = new PeerSet( + // @ts-expect-error boolean filter removes falsy peer IDs + this.connectionManager.getDialQueue() + .map(queue => queue.peerId) + .filter(Boolean) + ) // Already has enough connections if (numConnections >= this.minConnections) { @@ -127,6 +133,11 @@ export class AutoDial implements Startable { return false } + // remove peers we are already dialling + if (dialQueue.has(peer.id)) { + return false + } + return true }) diff --git a/src/connection-manager/dial-queue.ts b/src/connection-manager/dial-queue.ts index 963330dc7d..5f0ee5f881 100644 --- a/src/connection-manager/dial-queue.ts +++ b/src/connection-manager/dial-queue.ts @@ -232,7 +232,7 @@ export class DialQueue { signal.clear() }) .catch(err => { - log.error('dial failed to %s', addrsToDial.map(({ multiaddr }) => multiaddr.toString()).join(', '), err) + log.error('dial failed to %s', pendingDial.multiaddrs.map(ma => ma.toString()).join(', '), err) // Error is a timeout if (signal.aborted) { @@ -373,7 +373,14 @@ export class DialQueue { gatedAdrs.push(addr) } - return gatedAdrs.sort(this.addressSorter) + const sortedGatedAddrs = gatedAdrs.sort(this.addressSorter) + + // make sure we actually have some addresses to dial + if (sortedGatedAddrs.length === 0) { + throw new CodeError('The connection gater denied all addresses in the dial request', codes.ERR_NO_VALID_ADDRESSES) + } + + return sortedGatedAddrs } private async performDial (pendingDial: PendingDial, options: DialOptions = {}): Promise { diff --git a/test/connection-manager/auto-dial.spec.ts b/test/connection-manager/auto-dial.spec.ts index d1c7b656b1..57b2dbb9af 100644 --- a/test/connection-manager/auto-dial.spec.ts +++ b/test/connection-manager/auto-dial.spec.ts @@ -11,6 +11,7 @@ import type { PeerStore, Peer } from '@libp2p/interface-peer-store' import { multiaddr } from '@multiformats/multiaddr' import { EventEmitter } from '@libp2p/interfaces/events' import { PeerMap } from '@libp2p/peer-collections' +import type { Connection } from '@libp2p/interface-connection' describe('auto-dial', () => { let autoDialler: AutoDial @@ -51,6 +52,7 @@ describe('auto-dial', () => { const connectionManager = stubInterface() connectionManager.getConnectionsMap.returns(new PeerMap()) + connectionManager.getDialQueue.returns([]) autoDialler = new AutoDial({ peerStore, @@ -69,4 +71,112 @@ describe('auto-dial', () => { expect(connectionManager.openConnection.calledWith(peerWithAddress.id)).to.be.true() expect(connectionManager.openConnection.calledWith(peerWithoutAddress.id)).to.be.false() }) + + it('should not dial connected peers', async () => { + const connectedPeer: Peer = { + id: await createEd25519PeerId(), + protocols: [], + addresses: [{ + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4001'), + isCertified: true + }], + metadata: new Map(), + tags: new Map() + } + const unConnectedPeer: Peer = { + id: await createEd25519PeerId(), + protocols: [], + addresses: [{ + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4002'), + isCertified: true + }], + metadata: new Map(), + tags: new Map() + } + + const peerStore = stubInterface() + + peerStore.all.returns(Promise.resolve([ + connectedPeer, unConnectedPeer + ])) + + const connectionMap = new PeerMap() + connectionMap.set(connectedPeer.id, [stubInterface()]) + const connectionManager = stubInterface() + connectionManager.getConnectionsMap.returns(connectionMap) + connectionManager.getDialQueue.returns([]) + + autoDialler = new AutoDial({ + peerStore, + connectionManager, + events: new EventEmitter() + }, { + minConnections: 10 + }) + autoDialler.start() + await autoDialler.autoDial() + + await pWaitFor(() => connectionManager.openConnection.callCount === 1) + await delay(1000) + + expect(connectionManager.openConnection.callCount).to.equal(1) + expect(connectionManager.openConnection.calledWith(unConnectedPeer.id)).to.be.true() + expect(connectionManager.openConnection.calledWith(connectedPeer.id)).to.be.false() + }) + + it('should not dial peers already in the dial queue', async () => { + // peers with protocols are dialled before peers without protocols + const peerInDialQueue: Peer = { + id: await createEd25519PeerId(), + protocols: [], + addresses: [{ + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4001'), + isCertified: true + }], + metadata: new Map(), + tags: new Map() + } + const peerNotInDialQueue: Peer = { + id: await createEd25519PeerId(), + protocols: [], + addresses: [{ + multiaddr: multiaddr('/ip4/127.0.0.1/tcp/4002'), + isCertified: true + }], + metadata: new Map(), + tags: new Map() + } + + const peerStore = stubInterface() + + peerStore.all.returns(Promise.resolve([ + peerInDialQueue, peerNotInDialQueue + ])) + + const connectionManager = stubInterface() + connectionManager.getConnectionsMap.returns(new PeerMap()) + connectionManager.getDialQueue.returns([{ + id: 'foo', + peerId: peerInDialQueue.id, + multiaddrs: [], + status: 'queued' + }]) + + autoDialler = new AutoDial({ + peerStore, + connectionManager, + events: new EventEmitter() + }, { + minConnections: 10 + }) + autoDialler.start() + await autoDialler.autoDial() + + await pWaitFor(() => connectionManager.openConnection.callCount === 1) + await delay(1000) + + expect(connectionManager.openConnection.callCount).to.equal(1) + expect(connectionManager.openConnection.calledWith(peerNotInDialQueue.id)).to.be.true() + expect(connectionManager.openConnection.calledWith(peerInDialQueue.id)).to.be.false() + }) }) From c02682cfa1e217a24a1227a2c73b6740e2c7fdde Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 5 May 2023 12:11:13 +0100 Subject: [PATCH 2/3] chore: linting --- src/connection-manager/dial-queue.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/connection-manager/dial-queue.ts b/src/connection-manager/dial-queue.ts index 5f0ee5f881..4ec2c50b8e 100644 --- a/src/connection-manager/dial-queue.ts +++ b/src/connection-manager/dial-queue.ts @@ -375,8 +375,8 @@ export class DialQueue { const sortedGatedAddrs = gatedAdrs.sort(this.addressSorter) - // make sure we actually have some addresses to dial - if (sortedGatedAddrs.length === 0) { + // make sure we actually have some addresses to dial + if (sortedGatedAddrs.length === 0) { throw new CodeError('The connection gater denied all addresses in the dial request', codes.ERR_NO_VALID_ADDRESSES) } From f8ccf08dea71e28e7f732125808562ec2939d5ef Mon Sep 17 00:00:00 2001 From: achingbrain Date: Fri, 5 May 2023 17:19:06 +0100 Subject: [PATCH 3/3] chore: update connection manager stub --- test/connection-manager/auto-dial.spec.ts | 32 +++++++++++++---------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/test/connection-manager/auto-dial.spec.ts b/test/connection-manager/auto-dial.spec.ts index 57b2dbb9af..d1622c0c10 100644 --- a/test/connection-manager/auto-dial.spec.ts +++ b/test/connection-manager/auto-dial.spec.ts @@ -50,9 +50,10 @@ describe('auto-dial', () => { peerWithAddress, peerWithoutAddress ])) - const connectionManager = stubInterface() - connectionManager.getConnectionsMap.returns(new PeerMap()) - connectionManager.getDialQueue.returns([]) + const connectionManager = stubInterface({ + getConnectionsMap: new PeerMap(), + getDialQueue: [] + }) autoDialler = new AutoDial({ peerStore, @@ -102,9 +103,11 @@ describe('auto-dial', () => { const connectionMap = new PeerMap() connectionMap.set(connectedPeer.id, [stubInterface()]) - const connectionManager = stubInterface() - connectionManager.getConnectionsMap.returns(connectionMap) - connectionManager.getDialQueue.returns([]) + + const connectionManager = stubInterface({ + getConnectionsMap: connectionMap, + getDialQueue: [] + }) autoDialler = new AutoDial({ peerStore, @@ -153,14 +156,15 @@ describe('auto-dial', () => { peerInDialQueue, peerNotInDialQueue ])) - const connectionManager = stubInterface() - connectionManager.getConnectionsMap.returns(new PeerMap()) - connectionManager.getDialQueue.returns([{ - id: 'foo', - peerId: peerInDialQueue.id, - multiaddrs: [], - status: 'queued' - }]) + const connectionManager = stubInterface({ + getConnectionsMap: new PeerMap(), + getDialQueue: [{ + id: 'foo', + peerId: peerInDialQueue.id, + multiaddrs: [], + status: 'queued' + }] + }) autoDialler = new AutoDial({ peerStore,