Merge pull request #1097 from ethereumjs/1459-tuning

Add node discovery mode flags & define network-based defaults

Author: holgerd77

packages/client/bin/cli.ts

@@ -93,6 +93,14 @@ const args = require('yargs')
       boolean: true,
       default: Config.DEBUGCODE_DEFAULT,
     },
+    discDns: {
+      describe: 'Query EIP-1459 DNS TXT records for peer discovery',
+      boolean: true,
+    },
+    discV4: {
+      describe: 'Use v4 ("findneighbour" node requests) for peer discovery',
+      boolean: true,
+    },
   })
   .locale('en_EN').argv
 
@@ -174,6 +182,8 @@ async function run() {
     dnsAddr: args.dnsAddr,
     dnsNetworks: args.dnsNetworks,
     debugCode: args.debugCode,
+    discDns: args.discDns,
+    discV4: args.discV4,
   })
   logger = config.logger
 

packages/client/lib/config.ts

@@ -123,6 +123,20 @@ export interface ConfigOptions {
    * (meant to be used internally for the most part)
    */
   debugCode?: boolean
+
+  /**
+   * Query EIP-1459 DNS TXT records for peer discovery
+   *
+   * Default: `true` for testnets, false for mainnet
+   */
+  discDns?: boolean
+
+  /**
+   * Use v4 ("findneighbour" node requests) for peer discovery
+   *
+   * Default: `false` for testnets, true for mainnet
+   */
+  discV4?: boolean
 }
 
 export class Config {
@@ -154,6 +168,8 @@ export class Config {
   public readonly maxPeers: number
   public readonly dnsAddr: string
   public readonly debugCode: boolean
+  public readonly discDns: boolean
+  public readonly discV4: boolean
 
   public readonly chainCommon: Common
   public readonly execCommon: Common
@@ -181,6 +197,9 @@ export class Config {
     this.chainCommon = Object.assign(Object.create(Object.getPrototypeOf(common)), common)
     this.execCommon = Object.assign(Object.create(Object.getPrototypeOf(common)), common)
 
+    this.discDns = this.getDnsDiscovery(options.discDns)
+    this.discV4 = this.getV4Discovery(options.discV4)
+
     if (options.logger) {
       if (options.loglevel) {
         throw new Error('Config initialization with both logger and loglevel options not allowed')
@@ -244,4 +263,23 @@ export class Config {
     const dataDir = `${this.datadir}/${networkDirName}/state`
     return dataDir
   }
+
+  /**
+   * Returns specified option or the default setting for whether DNS-based peer discovery
+   * is enabled based on chainName. `true` for ropsten, rinkeby, and goerli
+   */
+  getDnsDiscovery(option: boolean | undefined): boolean {
+    if (option !== undefined) return option
+    const dnsNets = ['ropsten', 'rinkeby', 'goerli']
+    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/lib/net/server/rlpxserver.ts

@@ -150,8 +150,10 @@ export class RlpxServer extends Server {
     })
 
     // DNS peers
-    const dnsPeers = (await this.dpt?.getDnsPeers()) ?? []
-    promises = promises.concat(dnsPeers.map((node) => self.dpt!.bootstrap(node)))
+    if (this.config.discDns) {
+      const dnsPeers = (await this.dpt?.getDnsPeers()) ?? []
+      promises = promises.concat(dnsPeers.map((node) => self.dpt!.bootstrap(node)))
+    }
 
     for (const promise of promises) {
       try {
@@ -219,7 +221,8 @@ export class RlpxServer extends Server {
         udpPort: null,
         tcpPort: null,
       },
-      shouldFindNeighbours: this.dnsNetworks.length ? false : true,
+      shouldFindNeighbours: this.config.discV4,
+      shouldGetDnsPeers: this.config.discDns,
       dnsRefreshQuantity: this.config.maxPeers,
       dnsNetworks: this.dnsNetworks,
       dnsAddr: this.config.dnsAddr,

packages/client/test/config.spec.ts

@@ -1,5 +1,6 @@
 import tape from 'tape-catch'
 import { Config } from '../lib/config'
+import Common from '@ethereumjs/common'
 
 tape('[Config]', (t) => {
   t.test('Initialization with default parameters', (t) => {
@@ -25,4 +26,54 @@ tape('[Config]', (t) => {
     t.equal(config.getStateDataDirectory(), './datadir/mainnet/state')
     t.end()
   })
+
+  t.test('peer discovery default mainnet setting', (t) => {
+    const common = new Common({ chain: 'mainnet' })
+    const config = new Config({ common })
+    t.equal(config.discDns, false, 'disables DNS peer discovery for mainnet')
+    t.equal(config.discV4, true, 'enables DNS peer discovery for mainnet')
+    t.end()
+  })
+
+  t.test('peer discovery default testnet settings', (t) => {
+    let config
+
+    for (const chain of ['rinkeby', 'goerli', 'ropsten']) {
+      const common = new Common({ chain })
+      config = new Config({ common })
+      t.equal(config.discDns, true, `enables DNS peer discovery for ${chain}`)
+      t.equal(config.discV4, false, `disables V4 peer discovery for ${chain}`)
+    }
+    t.end()
+  })
+
+  t.test('--discDns=true/false', (t) => {
+    let common, config, chain
+
+    chain = 'mainnet'
+    common = new Common({ chain })
+    config = new Config({ common, discDns: true })
+    t.equal(config.discDns, true, `default discDns setting can be overridden to true`)
+
+    chain = 'rinkeby'
+    common = new Common({ chain })
+    config = new Config({ common, discDns: false })
+    t.equal(config.discDns, false, `default discDns setting can be overridden to false`)
+    t.end()
+  })
+
+  t.test('--discV4=true/false', (t) => {
+    let common, config, chain
+
+    chain = 'mainnet'
+    common = new Common({ chain })
+    config = new Config({ common, discV4: false })
+    t.equal(config.discDns, false, `default discV4 setting can be overridden to false`)
+
+    chain = 'rinkeby'
+    common = new Common({ chain })
+    config = new Config({ common, discV4: true })
+    t.equal(config.discDns, true, `default discV4 setting can be overridden to true`)
+    t.end()
+  })
 })

packages/client/test/net/server/rlpxserver.spec.ts

@@ -90,7 +90,7 @@ tape('[RlpxServer]', async (t) => {
 
   t.test('should bootstrap with dns acquired peers', async (t) => {
     const dnsPeerInfo = { address: '10.0.0.5', udpPort: 1234, tcpPort: 1234 }
-    const config = new Config({ loglevel: 'error', transports: [] })
+    const config = new Config({ loglevel: 'error', transports: [], discDns: true })
     const server = new RlpxServer({
       config,
       dnsNetworks: ['enrtree:A'],

packages/devp2p/src/dpt/dpt.ts

@@ -55,6 +55,13 @@ export interface DPTOptions {
    */
   shouldFindNeighbours?: boolean
 
+  /**
+   * Toggles whether or not peers should be discovered by querying EIP-1459 DNS lists
+   *
+   * Default: false
+   */
+  shouldGetDnsPeers?: boolean
+
   /**
    * Max number of candidate peers to retrieve from DNS records when
    * attempting to discover new nodes
@@ -87,6 +94,7 @@ export class DPT extends EventEmitter {
   private _refreshIntervalId: NodeJS.Timeout
   private _refreshIntervalSelectionCounter: number = 0
   private _shouldFindNeighbours: boolean
+  private _shouldGetDnsPeers: boolean
   private _dnsRefreshQuantity: number
   private _dnsNetworks: string[]
   private _dnsAddr: string
@@ -97,6 +105,7 @@ export class DPT extends EventEmitter {
     this.privateKey = Buffer.from(privateKey)
     this._id = pk2id(Buffer.from(publicKeyCreate(this.privateKey, false)))
     this._shouldFindNeighbours = options.shouldFindNeighbours === false ? false : true
+    this._shouldGetDnsPeers = options.shouldGetDnsPeers ?? false
     // By default, tries to connect to 12 new peers every 3s
     this._dnsRefreshQuantity = Math.floor((options.dnsRefreshQuantity ?? 25) / 2)
     this._dnsNetworks = options.dnsNetworks ?? []
@@ -117,9 +126,15 @@ export class DPT extends EventEmitter {
     })
     this._server.once('listening', () => this.emit('listening'))
     this._server.once('close', () => this.emit('close'))
-    this._server.on('peers', (peers) => this._onServerPeers(peers))
     this._server.on('error', (err) => this.emit('error', err))
 
+    // When not using peer neighbour discovery we don't add peers here
+    // because it results in duplicate calls for the same targets
+    this._server.on('peers', (peers) => {
+      if (!this._shouldFindNeighbours) return
+      this._addPeerBatch(peers)
+    })
+
     // By default calls refresh every 3s
     const refreshIntervalSubdivided = Math.floor((options.refreshInterval ?? ms('60s')) / 10)
     this._refreshIntervalId = setInterval(() => this.refresh(), refreshIntervalSubdivided)
@@ -155,12 +170,7 @@ export class DPT extends EventEmitter {
     }
   }
 
-  _onServerPeers(peers: PeerInfo[]): void {
-    // We don't want this to run when using
-    // dns - it's fired in the server.on:peer hook
-    // and results in duplicate addition attempts
-    if (!this._shouldFindNeighbours) return
-
+  _addPeerBatch(peers: PeerInfo[]): void {
     const DIFF_TIME_MS = 200
     let ms = 0
     for (const peer of peers) {
@@ -251,16 +261,16 @@ export class DPT extends EventEmitter {
       }
     }
 
-    const dnsPeers = await this.getDnsPeers()
+    if (this._shouldGetDnsPeers) {
+      const dnsPeers = await this.getDnsPeers()
 
-    debug(
-      `.refresh() (Adding ${dnsPeers.length}) from DNS tree, (${
-        this.getPeers().length
-      } current peers in table)`
-    )
+      debug(
+        `.refresh() Adding ${dnsPeers.length} from DNS tree, (${
+          this.getPeers().length
+        } current peers in table)`
+      )
 
-    for (const peer of dnsPeers) {
-      await this.bootstrap(peer)
+      this._addPeerBatch(dnsPeers)
     }
   }
 }

packages/devp2p/test/integration/util.ts

@@ -37,6 +37,8 @@ export function getTestDPTsWithDns(numDPTs: number) {
       timeout: 100,
       refreshInterval: 10,
       dnsNetworks: [testdata.dns.enrTree],
+      shouldFindNeighbours: false,
+      shouldGetDnsPeers: true,
     })
     dpt.bind(basePort + i)
     dpts.push(dpt)