Client Discovery Improvements (#3120)

* Client: add explicit discovery startup logging

* Client: add bootnodes format hint on CLI option help

* Client: add option to pass in bootnode.txt file to --bootnodes CLI param, add CLI test

* Client: replace goerli -> holesky in list with networks with activated DNS discovery

* Devp2p: add new confirmed-peer mechanism for a more fine grained peer discovery, reactivated discV4 for client

* Devp2p: add test setup for DPT, initialization, bootstrap(), addPeer() and confirmed/unconfirmed refresh() tests, fix bug in getClosestPeers()

* Client: make onlyConfirmed exception for mainnet (since most peers are mainnet peers and peering then goes quicker)

* Devp2p: increase network resilience for the case that no initial confirmation is possible

* Devp2p: remove peer from confirmed peers list when being removed from DPT, fix tests

* Fix tests

* Client: add missing bootnode.txt test file

packages/client/bin/cli.ts

@@ -102,7 +102,8 @@ const args: ClientOpts = yargs(hideBin(process.argv))
     default: true,
   })
   .option('bootnodes', {
-    describe: 'Comma-separated list of network bootnodes',
+    describe:
+      'Comma-separated list of network bootnodes (format: "enode://<id>@<host:port>,enode://..." ("[?discport=<port>]" not supported) or path to a bootnode.txt file',
     array: true,
   })
   .option('port', {
@@ -811,7 +812,31 @@ async function run() {
   }
 
   logger = getLogger(args)
-  const bootnodes = args.bootnodes !== undefined ? parseMultiaddrs(args.bootnodes) : undefined
+  let bootnodes
+  if (args.bootnodes !== undefined) {
+    // File path passed, read bootnodes from disk
+    if (
+      Array.isArray(args.bootnodes) &&
+      args.bootnodes.length === 1 &&
+      args.bootnodes[0].includes('.txt')
+    ) {
+      const file = readFileSync(args.bootnodes[0], 'utf-8')
+      let nodeURLs = file.split(/\r?\n/).filter((url) => (url !== '' ? true : false))
+      nodeURLs = nodeURLs.map((url) => {
+        const discportIndex = url.indexOf('?discport')
+        if (discportIndex > 0) {
+          return url.substring(0, discportIndex)
+        } else {
+          return url
+        }
+      })
+      bootnodes = parseMultiaddrs(nodeURLs)
+      logger.info(`Reading bootnodes file=${args.bootnodes[0]} num=${nodeURLs.length}`)
+    } else {
+      bootnodes = parseMultiaddrs(args.bootnodes)
+    }
+  }
+
   const multiaddrs = args.multiaddrs !== undefined ? parseMultiaddrs(args.multiaddrs) : undefined
   const mine = args.mine !== undefined ? args.mine : args.dev !== undefined
   const isSingleNode = args.isSingleNode !== undefined ? args.isSingleNode : args.dev !== undefined

packages/client/src/config.ts

@@ -518,7 +518,7 @@ export class Config {
     this.execCommon = common.copy()
 
     this.discDns = this.getDnsDiscovery(options.discDns)
-    this.discV4 = this.getV4Discovery(options.discV4)
+    this.discV4 = options.discV4 ?? true
 
     this.logger = options.logger ?? getLogger({ loglevel: 'error' })
 
@@ -672,16 +672,7 @@ export class Config {
    */
   getDnsDiscovery(option: boolean | undefined): boolean {
     if (option !== undefined) return option
-    const dnsNets = ['goerli', 'sepolia']
+    const dnsNets = ['holesky', 'sepolia']
     return dnsNets.includes(this.chainCommon.chainName())
   }
-
-  /**
-   * Returns specified option or the default setting for whether v4 peer discovery
-   * is enabled based on chainName. `true` for `mainnet`
-   */
-  getV4Discovery(option: boolean | undefined): boolean {
-    if (option !== undefined) return option
-    return this.chainCommon.chainName() === 'mainnet'
-  }
 }

packages/client/src/net/server/rlpxserver.ts

@@ -216,6 +216,7 @@ export class RlpxServer extends Server {
           udpPort: null,
           tcpPort: null,
         },
+        onlyConfirmed: this.config.chainCommon.chainName() === 'mainnet' ? false : true,
         shouldFindNeighbours: this.config.discV4,
         shouldGetDnsPeers: this.config.discDns,
         dnsRefreshQuantity: this.config.maxPeers,
@@ -233,9 +234,16 @@ export class RlpxServer extends Server {
         resolve()
       })
 
+      this.config.events.on(Event.PEER_CONNECTED, (peer) => {
+        this.dpt?.confirmPeer(peer.id)
+      })
+
       if (typeof this.config.port === 'number') {
         this.dpt.bind(this.config.port, '0.0.0.0')
       }
+      this.config.logger.info(
+        `Started discovery service discV4=${this.config.discV4} dns=${this.config.discDns} refreshInterval=${this.refreshInterval}`
+      )
     })
   }
 

packages/client/src/net/server/server.ts

@@ -74,7 +74,7 @@ export class Server {
     const protocols: Protocol[] = Array.from(this.protocols)
     await Promise.all(protocols.map((p) => p.open()))
     this.started = true
-    this.config.logger.info(`Started ${this.name} server.`)
+    this.config.logger.info(`Started ${this.name} server maxPeers=${this.config.maxPeers}`)
     return true
   }
 

packages/client/src/service/service.ts

@@ -198,6 +198,7 @@ export class Service {
       const heapUsed = Math.round(used_heap_size / 1000 / 1000) // MB
       const percentage = Math.round((100 * used_heap_size) / heap_size_limit)
       const msg = `Memory stats usage=${heapUsed} MB percentage=${percentage}%`
+
       if (this._statsCounter % 4 === 0) {
         this.config.logger.info(msg)
         this._statsCounter = 0

packages/client/test/cli/cli.spec.ts

@@ -460,6 +460,21 @@ describe('[CLI]', () => {
     }
     await clientRunHelper(cliArgs, onData)
   }, 30000)
+  it('should start client with file path for bootnodes option', async () => {
+    const cliArgs = ['--bootnodes=./test/testdata/bootnode.txt']
+    const onData = async (
+      message: string,
+      child: ChildProcessWithoutNullStreams,
+      resolve: Function
+    ) => {
+      if (message.includes('Reading bootnodes')) {
+        assert.ok(message.includes('num=2'), 'passing bootnode.txt URL for bootnodes option works')
+        child.kill(9)
+        resolve(undefined)
+      }
+    }
+    await clientRunHelper(cliArgs, onData)
+  }, 30000)
   // test experimental feature options
   it('should start client when passed options for experimental features', async () => {
     const cliArgs = ['--mine=true', '--forceSnapSync=true', '--dev=poa', '--port=30393']

packages/client/test/config.spec.ts

@@ -36,17 +36,6 @@ describe('[Config]', () => {
     assert.equal(config.discV4, true, 'enables DNS peer discovery for mainnet')
   })
 
-  it('peer discovery default testnet settings', () => {
-    let config
-
-    for (const chain of [Chain.Goerli, Chain.Sepolia]) {
-      const common = new Common({ chain })
-      config = new Config({ common })
-      assert.equal(config.discDns, true, `enables DNS peer discovery for ${chain}`)
-      assert.equal(config.discV4, false, `disables V4 peer discovery for ${chain}`)
-    }
-  })
-
   it('--discDns=true/false', () => {
     let common, config, chain
 
@@ -67,11 +56,11 @@ describe('[Config]', () => {
     chain = Chain.Mainnet
     common = new Common({ chain })
     config = new Config({ common, discV4: false })
-    assert.equal(config.discDns, false, `default discV4 setting can be overridden to false`)
+    assert.equal(config.discV4, false, `default discV4 setting can be overridden to false`)
 
     chain = Chain.Goerli
     common = new Common({ chain })
     config = new Config({ common, discV4: true })
-    assert.equal(config.discDns, true, `default discV4 setting can be overridden to true`)
+    assert.equal(config.discV4, true, `default discV4 setting can be overridden to true`)
   })
 })

packages/client/test/testdata/bootnode.txt

@@ -0,0 +1,2 @@
+enode://abc@127.0.0.1:30303
+enode://abc@127.0.0.1:30304

packages/devp2p/src/dpt/dpt.ts

@@ -1,4 +1,4 @@
-import { bytesToInt, randomBytes } from '@ethereumjs/util'
+import { bytesToInt, bytesToUnprefixedHex, randomBytes } from '@ethereumjs/util'
 import { secp256k1 } from 'ethereum-cryptography/secp256k1.js'
 import { EventEmitter } from 'events'
 
@@ -32,6 +32,9 @@ export class DPT {
   protected _dnsNetworks: string[]
   protected _dnsAddr: string
 
+  protected _onlyConfirmed: boolean
+  protected _confirmedPeers: Set<string>
+
   constructor(privateKey: Uint8Array, options: DPTOptions) {
     this.events = new EventEmitter()
     this._privateKey = privateKey
@@ -46,6 +49,9 @@ export class DPT {
     this._dns = new DNS({ dnsServerAddress: this._dnsAddr })
     this._banlist = new BanList()
 
+    this._onlyConfirmed = options.onlyConfirmed ?? false
+    this._confirmedPeers = new Set()
+
     this._kbucket = new KBucket(this.id)
     this._kbucket.events.on('added', (peer: PeerInfo) => this.events.emit('peer:added', peer))
     this._kbucket.events.on('removed', (peer: PeerInfo) => this.events.emit('peer:removed', peer))
@@ -118,6 +124,9 @@ export class DPT {
   async bootstrap(peer: PeerInfo): Promise<void> {
     try {
       peer = await this.addPeer(peer)
+      if (peer.id !== undefined) {
+        this._confirmedPeers.add(bytesToUnprefixedHex(peer.id))
+      }
     } catch (error: any) {
       this.events.emit('error', error)
       return
@@ -148,6 +157,20 @@ export class DPT {
     }
   }
 
+  /**
+   * Add peer to a confirmed list of peers (peers meeting some
+   * level of quality, e.g. being on the same network) to allow
+   * for a more selective findNeighbours request and sending
+   * (with activated `onlyConfirmed` setting)
+   *
+   * @param id Unprefixed hex id
+   */
+  confirmPeer(id: string) {
+    if (this._confirmedPeers.size < 5000) {
+      this._confirmedPeers.add(id)
+    }
+  }
+
   getPeer(obj: string | Uint8Array | PeerInfo) {
     return this._kbucket.get(obj)
   }
@@ -156,11 +179,25 @@ export class DPT {
     return this._kbucket.getAll()
   }
 
+  numPeers() {
+    return this._kbucket.getAll().length
+  }
+
   getClosestPeers(id: Uint8Array) {
-    return this._kbucket.closest(id)
+    let peers = this._kbucket.closest(id)
+    if (this._onlyConfirmed && this._confirmedPeers.size > 0) {
+      peers = peers.filter((peer) =>
+        this._confirmedPeers.has(bytesToUnprefixedHex(peer.id as Uint8Array)) ? true : false
+      )
+    }
+    return peers
   }
 
   removePeer(obj: string | PeerInfo | Uint8Array) {
+    const peer = this._kbucket.get(obj)
+    if (peer?.id !== undefined) {
+      this._confirmedPeers.delete(bytesToUnprefixedHex(peer.id as Uint8Array))
+    }
     this._kbucket.remove(obj)
   }
 
@@ -187,7 +224,14 @@ export class DPT {
         // Randomly distributed selector based on peer ID
         // to decide on subdivided execution
         const selector = bytesToInt((peer.id as Uint8Array).subarray(0, 1)) % 10
-        if (selector === this._refreshIntervalSelectionCounter) {
+        let confirmed = true
+        if (this._onlyConfirmed && this._confirmedPeers.size > 0) {
+          const id = bytesToUnprefixedHex(peer.id as Uint8Array)
+          if (!this._confirmedPeers.has(id)) {
+            confirmed = false
+          }
+        }
+        if (confirmed && selector === this._refreshIntervalSelectionCounter) {
           this._server.findneighbours(peer, randomBytes(64))
         }
       }

packages/devp2p/src/types.ts

@@ -76,6 +76,18 @@ export interface DPTOptions {
    */
   shouldFindNeighbours?: boolean
 
+  /**
+   * Send findNeighbour requests to and only answer with respective peers
+   * being confirmed by calling the `confirmPeer()` method
+   *
+   * (allows for a more selective and noise reduced discovery)
+   *
+   * Note: Bootstrap nodes are confirmed by default.
+   *
+   * Default: false
+   */
+  onlyConfirmed?: boolean
+
   /**
    * Toggles whether or not peers should be discovered by querying EIP-1459 DNS lists
    *

packages/devp2p/test/dpt.spec.ts

@@ -0,0 +1,108 @@
+import { hexToBytes } from '@ethereumjs/util'
+import { afterEach, assert, describe, expect, it, vi } from 'vitest'
+
+import { DPT } from '../src/dpt/index.js'
+
+import type { PeerInfo } from '../src/types.js'
+
+describe('DPT', () => {
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  const privateKey1 = hexToBytes(
+    '0x012e930448c53e0b73edbbbc433e8a741e978cda79be2be039905f538d6247c2'
+  )
+
+  const peers: PeerInfo[] = []
+
+  for (let i = 1; i <= 5; i++) {
+    const id = new Uint8Array([i])
+    const address = '127.0.0.1'
+    const udpPort = 5000 + i
+    const peer: PeerInfo = {
+      id,
+      address,
+      udpPort,
+    }
+    peers.push(peer)
+  }
+
+  class Server {
+    ping() {}
+    findneighbours() {}
+    destroy() {}
+  }
+
+  it('should initialize and add peers', async () => {
+    const dpt = new DPT(privateKey1, {})
+    dpt['_server'] = new Server() as any
+    assert.equal(dpt['_dnsAddr'], '8.8.8.8', 'should initialize with default values')
+
+    dpt['_server'].ping = vi.fn().mockResolvedValue(peers[0])
+    await dpt.bootstrap(peers[0])
+    assert.equal(dpt.numPeers(), 1, 'should add peer on bootstrap()')
+
+    // Attention! Not all peers are called by default in refresh()
+    // (take into account on test design)
+    const spy = vi.spyOn(dpt['_server'], 'findneighbours')
+    await dpt.refresh()
+    expect(spy).toHaveBeenCalledTimes(1)
+
+    dpt['_server'].ping = vi.fn().mockResolvedValue(peers[1])
+    await dpt.addPeer(peers[1])
+    assert.equal(dpt.numPeers(), 2, 'should add another peer on addPeer()')
+
+    assert.equal(
+      dpt.getClosestPeers(peers[0].id!).length,
+      2,
+      'should return all peers on getClosestPeers()'
+    )
+
+    dpt.destroy()
+  })
+
+  it('should only call to confirmed peers on activated flag', async () => {
+    const dpt = new DPT(privateKey1, { onlyConfirmed: true })
+    dpt['_server'] = new Server() as any
+
+    dpt['_server'].ping = vi.fn().mockResolvedValue(peers[0])
+    await dpt.addPeer(peers[0])
+
+    const spy = vi.spyOn(dpt['_server'], 'findneighbours')
+    await dpt.refresh()
+    expect(
+      spy,
+      'call findneighbours on unconfirmed if no confirmed peers yet'
+    ).toHaveBeenCalledTimes(1)
+
+    dpt['_refreshIntervalSelectionCounter'] = 0
+    dpt.confirmPeer('01')
+    await dpt.refresh()
+    expect(spy, 'call findneighbours on confirmed').toHaveBeenCalledTimes(2)
+
+    dpt['_server'].ping = vi.fn().mockResolvedValue(peers[1])
+    await dpt.addPeer(peers[1])
+    assert.equal(
+      dpt.getClosestPeers(peers[0].id!).length,
+      1,
+      'should not return unconfirmed on getClosestPeers()'
+    )
+
+    dpt.confirmPeer('02')
+    assert.equal(
+      dpt.getClosestPeers(peers[0].id!).length,
+      2,
+      'should return confirmed on getClosestPeers()'
+    )
+
+    dpt.removePeer(peers[1])
+    assert.equal(
+      dpt.getClosestPeers(peers[0].id!).length,
+      1,
+      'should work after peers being removed'
+    )
+
+    dpt.destroy()
+  })
+})