fix: ensure dials always use the latest PeerInfo from the PeerBook (#312

)

* 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.

* test: add test for get peer info

* refactor: just use id with dialer queue

src/dialer/queue.js

@@ -61,12 +61,11 @@ function createConnectionWithProtocol ({ protocol, connection, callback }) {
 class Queue {
   /**
    * @constructor
-   * @param {PeerInfo} peerInfo
+   * @param {string} peerId
    * @param {Switch} _switch
    */
-  constructor (peerInfo, _switch) {
-    this.peerInfo = peerInfo
-    this.id = peerInfo.id.toB58String()
+  constructor (peerId, _switch) {
+    this.id = peerId
     this.switch = _switch
     this._queue = []
     this.isRunning = false
@@ -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) {

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]
   }
 }

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)

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)