From 5d0c2cbee4e323d5af9293fb85b103b1d868f150 Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Thu, 21 Mar 2019 11:59:10 +0100 Subject: [PATCH 1/3] fix: ensure dials always use the latest PeerInfo from the PeerBook This fixes an issue where if dial is called with a new instance of PeerInfo, if it is the first dial to that peer, the queue was forever associated with that instance. This is currently the case when Circuit checks the HOP status of a potential relay. This ensures that whenever we dial, we are updating the peer book and using the latest PeerInfo in that dial request. --- src/dialer/queue.js | 4 ++-- src/get-peer-info.js | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/dialer/queue.js b/src/dialer/queue.js index b1c6cf1..cb40342 100644 --- a/src/dialer/queue.js +++ b/src/dialer/queue.js @@ -65,7 +65,6 @@ class Queue { * @param {Switch} _switch */ constructor (peerInfo, _switch) { - this.peerInfo = peerInfo this.id = peerInfo.id.toB58String() this.switch = _switch this._queue = [] @@ -168,8 +167,9 @@ class Queue { this._run() }) + const peerInfo = this.switch._peerBook.get(this.id) let queuedDial = this._queue.shift() - let { connectionFSM, didCreate } = this._getOrCreateConnection(this.peerInfo) + let { connectionFSM, didCreate } = this._getOrCreateConnection(peerInfo) // If the dial expects a ConnectionFSM, we can provide that back now if (queuedDial.useFSM) { diff --git a/src/get-peer-info.js b/src/get-peer-info.js index 7e3a13f..68f38f3 100644 --- a/src/get-peer-info.js +++ b/src/get-peer-info.js @@ -15,9 +15,10 @@ const multiaddr = require('multiaddr') function getPeerInfo (peer, peerBook) { let peerInfo - // Already a PeerInfo instance + // Already a PeerInfo instance, + // add to the peer book and return the latest value if (PeerInfo.isPeerInfo(peer)) { - return peer + return peerBook.put(peer) } // Attempt to convert from Multiaddr instance (not string) From 220e08210477a0c834d6fc0642afb494d0aeaa57 Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Thu, 21 Mar 2019 12:15:15 +0100 Subject: [PATCH 2/3] test: add test for get peer info --- test/get-peer-info.spec.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/get-peer-info.spec.js b/test/get-peer-info.spec.js index f339666..1678dad 100644 --- a/test/get-peer-info.spec.js +++ b/test/get-peer-info.spec.js @@ -63,6 +63,35 @@ describe('Get peer info', () => { }) }) + it('should add a peerInfo to the book', (done) => { + PeerId.createFromJSON(TestPeerInfos[1].id, (err, id) => { + const peerInfo = new PeerInfo(id) + expect(peerBook.has(peerInfo.id.toB58String())).to.eql(false) + + expect(getPeerInfo(peerInfo, peerBook)).to.exist() + expect(peerBook.has(peerInfo.id.toB58String())).to.eql(true) + done(err) + }) + }) + + it('should return the most up to date version of the peer', (done) => { + const ma1 = MultiAddr('/ip4/0.0.0.0/tcp/8080') + const ma2 = MultiAddr('/ip6/::/tcp/8080') + PeerId.createFromJSON(TestPeerInfos[1].id, (err, id) => { + const peerInfo = new PeerInfo(id) + peerInfo.multiaddrs.add(ma1) + expect(getPeerInfo(peerInfo, peerBook)).to.exist() + + const peerInfo2 = new PeerInfo(id) + peerInfo2.multiaddrs.add(ma2) + const returnedPeerInfo = getPeerInfo(peerInfo2, peerBook) + expect(returnedPeerInfo.multiaddrs.toArray()).to.contain.members([ + ma1, ma2 + ]) + done(err) + }) + }) + it('an invalid peer type should throw an error', () => { let func = () => { getPeerInfo('/ip4/127.0.0.1/tcp/1234', peerBook) From afadda0d344e672b802071a599fb336008bb6d39 Mon Sep 17 00:00:00 2001 From: Jacob Heun Date: Thu, 21 Mar 2019 12:56:07 +0100 Subject: [PATCH 3/3] refactor: just use id with dialer queue --- src/dialer/queue.js | 6 +++--- src/dialer/queueManager.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dialer/queue.js b/src/dialer/queue.js index cb40342..8155fca 100644 --- a/src/dialer/queue.js +++ b/src/dialer/queue.js @@ -61,11 +61,11 @@ function createConnectionWithProtocol ({ protocol, connection, callback }) { class Queue { /** * @constructor - * @param {PeerInfo} peerInfo + * @param {string} peerId * @param {Switch} _switch */ - constructor (peerInfo, _switch) { - this.id = peerInfo.id.toB58String() + constructor (peerId, _switch) { + this.id = peerId this.switch = _switch this._queue = [] this.isRunning = false diff --git a/src/dialer/queueManager.js b/src/dialer/queueManager.js index 775dc52..933173e 100644 --- a/src/dialer/queueManager.js +++ b/src/dialer/queueManager.js @@ -48,7 +48,7 @@ class DialQueueManager { getQueue (peerInfo) { const id = peerInfo.id.toB58String() - this._queue[id] = this._queue[id] || new Queue(peerInfo, this.switch) + this._queue[id] = this._queue[id] || new Queue(id, this.switch) return this._queue[id] } }