fix: address book should not emit peer event if no addresses are known #887

src/circuit/auto-relay.js

@@ -31,6 +31,7 @@ const {
  *
  * @typedef {Object} AutoRelayOptions
  * @property {number} [maxListeners = 1] - maximum number of relays to listen.
+ * @property {(error: Error, msg?: string) => {}} [onError]
  */
 
 class AutoRelay {
@@ -40,7 +41,7 @@ class AutoRelay {
    * @class
    * @param {AutoRelayProperties & AutoRelayOptions} props
    */
-  constructor ({ libp2p, maxListeners = 1 }) {
+  constructor ({ libp2p, maxListeners = 1, onError }) {
     this._libp2p = libp2p
     this._peerId = libp2p.peerId
     this._peerStore = libp2p.peerStore
@@ -60,6 +61,15 @@ class AutoRelay {
 
     this._peerStore.on('change:protocols', this._onProtocolChange)
     this._connectionManager.on('peer:disconnect', this._onPeerDisconnected)
+
+    /**
+     * @param {Error} error
+     * @param {string} [msg]
+     */
+    this._onError = (error, msg) => {
+      log.error(msg || error)
+      onError && onError(error, msg)
+    }
   }
 
   /**
@@ -107,7 +117,7 @@ class AutoRelay {
         await this._addListenRelay(connection, id)
       }
     } catch (err) {
-      log.error(err)
+      this._onError(err)
     }
   }
 
@@ -160,7 +170,7 @@ class AutoRelay {
       await this._transportManager.listen([new Multiaddr(listenAddr)])
       // Announce multiaddrs will update on listen success by TransportManager event being triggered
     } catch (err) {
-      log.error(err)
+      this._onError(err)
       this._listenRelays.delete(id)
     }
   }
@@ -231,8 +241,7 @@ class AutoRelay {
 
     // Try to listen on known peers that are not connected
     for (const peerId of knownHopsToDial) {
-      const connection = await this._libp2p.dial(peerId)
+      await this._tryToListenOnRelay(peerId)
-      await this._addListenRelay(connection, peerId.toB58String())
 
       // Check if already listening on enough relays
       if (this._listenRelays.size >= this.maxListeners) {
@@ -247,12 +256,11 @@ class AutoRelay {
         if (!provider.multiaddrs.length) {
           continue
         }
+
         const peerId = provider.id
-
         this._peerStore.addressBook.add(peerId, provider.multiaddrs)
-        const connection = await this._libp2p.dial(peerId)
 
-        await this._addListenRelay(connection, peerId.toB58String())
+        await this._tryToListenOnRelay(peerId)
 
         // Check if already listening on enough relays
         if (this._listenRelays.size >= this.maxListeners) {
@@ -260,7 +268,19 @@ class AutoRelay {
         }
       }
     } catch (err) {
-      log.error(err)
+      this._onError(err)
+    }
+  }
+
+  /**
+   * @param {PeerId} peerId
+   */
+  async _tryToListenOnRelay (peerId) {
+    try {
+      const connection = await this._libp2p.dial(peerId)
+      await this._addListenRelay(connection, peerId.toB58String())
+    } catch (err) {
+      this._onError(err, `could not connect and listen on known hop relay ${peerId.toB58String()}`)
     }
   }
 }

src/index.js

@@ -698,7 +698,7 @@ class Libp2p extends EventEmitter {
         try {
           await this.dialer.connectToPeer(peerId)
         } catch (err) {
-          log.error('could not connect to discovered peer', err)
+          log.error(`could not connect to discovered peer ${peerId.toB58String()} with ${err}`)
         }
       }
     }

src/peer-store/address-book.js

@@ -229,6 +229,11 @@ class AddressBook extends Book {
     const addresses = this._toAddresses(multiaddrs)
     const id = peerId.toB58String()
 
+    // No addresses to be added
+    if (!addresses.length) {
+      return this
+    }
+
     const entry = this.data.get(id)
 
     if (entry && entry.addresses) {

test/peer-store/address-book.spec.js

@@ -186,6 +186,23 @@ describe('addressBook', () => {
       throw new Error('invalid multiaddr should throw error')
     })
 
+    it('does not emit event if no addresses are added', async () => {
+      const defer = pDefer()
+
+      peerStore.on('peer', () => {
+        defer.reject()
+      })
+
+      ab.add(peerId, [])
+
+      // Wait 50ms for incorrect second event
+      setTimeout(() => {
+        defer.resolve()
+      }, 50)
+
+      await defer.promise
+    })
+
     it('adds the new content and emits change event', () => {
       const defer = pDefer()
 

test/relay/auto-relay.node.js

@@ -3,6 +3,7 @@
 
 const { expect } = require('aegir/utils/chai')
 const delay = require('delay')
+const pDefer = require('p-defer')
 const pWaitFor = require('p-wait-for')
 const sinon = require('sinon')
 const nock = require('nock')
@@ -358,6 +359,7 @@ describe('auto-relay', () => {
       expect(relayLibp2p1.connectionManager.size).to.equal(1)
 
       // Spy on dial
+      sinon.spy(autoRelay1, '_tryToListenOnRelay')
       sinon.spy(relayLibp2p1, 'dial')
 
       // Remove peer used as relay from peerStore and disconnect it
@@ -368,9 +370,54 @@ describe('auto-relay', () => {
 
       // Wait for other peer connected to be added as listen addr
       await pWaitFor(() => relayLibp2p1.transportManager.listen.callCount === 2)
+      expect(autoRelay1._tryToListenOnRelay.callCount).to.equal(1)
       expect(autoRelay1._listenRelays.size).to.equal(1)
       expect(relayLibp2p1.connectionManager.size).to.eql(1)
     })
+
+    it('should not fail when trying to dial unreachable peers to add as hop relay and replaced removed ones', async () => {
+      const defer = pDefer()
+      // Spy if a connected peer is being added as listen relay
+      sinon.spy(autoRelay1, '_addListenRelay')
+      sinon.spy(relayLibp2p1.transportManager, 'listen')
+
+      // Discover one relay and connect
+      relayLibp2p1.peerStore.addressBook.add(relayLibp2p2.peerId, relayLibp2p2.multiaddrs)
+      await relayLibp2p1.dial(relayLibp2p2.peerId)
+
+      // Discover an extra relay and connect to gather its Hop support
+      relayLibp2p1.peerStore.addressBook.add(relayLibp2p3.peerId, relayLibp2p3.multiaddrs)
+      await relayLibp2p1.dial(relayLibp2p3.peerId)
+
+      // Wait for both peer to be attempted to added as listen relay
+      await pWaitFor(() => autoRelay1._addListenRelay.callCount === 2)
+      expect(autoRelay1._listenRelays.size).to.equal(1)
+      expect(relayLibp2p1.connectionManager.size).to.equal(2)
+
+      // Only one will be used for listeninng
+      expect(relayLibp2p1.transportManager.listen.callCount).to.equal(1)
+
+      // Disconnect not used listen relay
+      await relayLibp2p1.hangUp(relayLibp2p3.peerId)
+
+      expect(autoRelay1._listenRelays.size).to.equal(1)
+      expect(relayLibp2p1.connectionManager.size).to.equal(1)
+
+      // Stub dial
+      sinon.stub(relayLibp2p1, 'dial').callsFake(() => {
+        defer.resolve()
+        return Promise.reject(new Error('failed to dial'))
+      })
+
+      // Remove peer used as relay from peerStore and disconnect it
+      relayLibp2p1.peerStore.delete(relayLibp2p2.peerId)
+      await relayLibp2p1.hangUp(relayLibp2p2.peerId)
+      expect(autoRelay1._listenRelays.size).to.equal(0)
+      expect(relayLibp2p1.connectionManager.size).to.equal(0)
+
+      // Wait for failed dial
+      await defer.promise
+    })
   })
 
   describe('flows with 2 max listeners', () => {