From b4fb9b7bf266ba03c4462c0a41b1c2691e4e88d4 Mon Sep 17 00:00:00 2001 From: Vasco Santos Date: Fri, 12 Feb 2021 11:22:11 +0100 Subject: [PATCH 1/3] fix: address book should not emit peer event if no addresses are known --- src/circuit/auto-relay.js | 17 +++++++---- src/index.js | 2 +- src/peer-store/address-book.js | 5 ++++ test/peer-store/address-book.spec.js | 17 +++++++++++ test/relay/auto-relay.node.js | 45 ++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/circuit/auto-relay.js b/src/circuit/auto-relay.js index a99bb14d..c5c5d3e9 100644 --- a/src/circuit/auto-relay.js +++ b/src/circuit/auto-relay.js @@ -231,8 +231,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._addListenRelay(connection, peerId.toB58String()) + await this._tryToListenOnRelay(peerId) // Check if already listening on enough relays if (this._listenRelays.size >= this.maxListeners) { @@ -247,12 +246,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) { @@ -263,6 +261,15 @@ class AutoRelay { log.error(err) } } + + async _tryToListenOnRelay (peerId) { + try { + const connection = await this._libp2p.dial(peerId) + await this._addListenRelay(connection, peerId.toB58String()) + } catch (err) { + log.error(`could not connect and listen on known hop relay ${peerId.toB58String()}`) + } + } } module.exports = AutoRelay diff --git a/src/index.js b/src/index.js index 02fa4f4e..4494ebac 100644 --- a/src/index.js +++ b/src/index.js @@ -700,7 +700,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}`) } } } diff --git a/src/peer-store/address-book.js b/src/peer-store/address-book.js index 595781c0..8b94acc5 100644 --- a/src/peer-store/address-book.js +++ b/src/peer-store/address-book.js @@ -229,6 +229,11 @@ class AddressBook extends Book { const addresses = this._toAddresses(multiaddrs) const id = peerId.toB58String() + // Not add unavailable addresses + if (!addresses.length) { + return this + } + const entry = this.data.get(id) if (entry && entry.addresses) { diff --git a/test/peer-store/address-book.spec.js b/test/peer-store/address-book.spec.js index 48eb7180..512fc8fd 100644 --- a/test/peer-store/address-book.spec.js +++ b/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() diff --git a/test/relay/auto-relay.node.js b/test/relay/auto-relay.node.js index a254b047..4f0aa05c 100644 --- a/test/relay/auto-relay.node.js +++ b/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') @@ -371,6 +372,50 @@ describe('auto-relay', () => { 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', () => { From 828a32d4f578ab8e57970cd8ddb30f1a7838786b Mon Sep 17 00:00:00 2001 From: Vasco Santos Date: Thu, 15 Apr 2021 14:38:25 +0200 Subject: [PATCH 2/3] chore: add jsdoc --- src/circuit/auto-relay.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/circuit/auto-relay.js b/src/circuit/auto-relay.js index c5c5d3e9..613c473e 100644 --- a/src/circuit/auto-relay.js +++ b/src/circuit/auto-relay.js @@ -262,6 +262,9 @@ class AutoRelay { } } + /** + * @param {PeerId} peerId + */ async _tryToListenOnRelay (peerId) { try { const connection = await this._libp2p.dial(peerId) From 3ffeb4ebe6320093d0b8d65a1b3b68f4183cd50f Mon Sep 17 00:00:00 2001 From: Vasco Santos Date: Tue, 20 Apr 2021 11:23:31 +0200 Subject: [PATCH 3/3] chore: apply suggestions from code review Co-authored-by: Irakli Gozalishvili --- src/circuit/auto-relay.js | 20 +++++++++++++++----- src/peer-store/address-book.js | 2 +- test/relay/auto-relay.node.js | 2 ++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/circuit/auto-relay.js b/src/circuit/auto-relay.js index 613c473e..20174e99 100644 --- a/src/circuit/auto-relay.js +++ b/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) } } @@ -258,7 +268,7 @@ class AutoRelay { } } } catch (err) { - log.error(err) + this._onError(err) } } @@ -270,7 +280,7 @@ class AutoRelay { const connection = await this._libp2p.dial(peerId) await this._addListenRelay(connection, peerId.toB58String()) } catch (err) { - log.error(`could not connect and listen on known hop relay ${peerId.toB58String()}`) + this._onError(err, `could not connect and listen on known hop relay ${peerId.toB58String()}`) } } } diff --git a/src/peer-store/address-book.js b/src/peer-store/address-book.js index 8b94acc5..f04ea453 100644 --- a/src/peer-store/address-book.js +++ b/src/peer-store/address-book.js @@ -229,7 +229,7 @@ class AddressBook extends Book { const addresses = this._toAddresses(multiaddrs) const id = peerId.toB58String() - // Not add unavailable addresses + // No addresses to be added if (!addresses.length) { return this } diff --git a/test/relay/auto-relay.node.js b/test/relay/auto-relay.node.js index 4f0aa05c..3b0481a3 100644 --- a/test/relay/auto-relay.node.js +++ b/test/relay/auto-relay.node.js @@ -359,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 @@ -369,6 +370,7 @@ 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) })