fix: favour providers peerInfo over sender peerInfo in ADD_PROVIDER (#129)

Author: dirkmc

src/rpc/handlers/add-provider.js

@@ -30,6 +30,7 @@ module.exports = (dht) => {
       return callback(errcode(new Error(errMsg), 'ERR_INVALID_CID'))
     }
 
+    let foundProvider = false
     msg.providerPeers.forEach((pi) => {
       // Ignore providers not from the originator
       if (!pi.id.isEqual(peer.id)) {
@@ -45,10 +46,20 @@ module.exports = (dht) => {
       log('received provider %s for %s (addrs %s)', peer.id.toB58String(), cid.toBaseEncodedString(), pi.multiaddrs.toArray().map((m) => m.toString()))
 
       if (!dht._isSelf(pi.id)) {
+        foundProvider = true
         dht.peerBook.put(pi)
+        dht.providers.addProvider(cid, pi.id, callback)
       }
     })
 
-    dht.providers.addProvider(cid, peer.id, callback)
+    // Previous versions of the JS DHT sent erroneous providers in the
+    // `providerPeers` field. In order to accommodate older clients that have
+    // this bug, we fall back to assuming the originator is the provider if
+    // we can't find any valid providers in the payload.
+    // https://github.com/libp2p/js-libp2p-kad-dht/pull/127
+    // https://github.com/libp2p/js-libp2p-kad-dht/issues/128
+    if (!foundProvider) {
+      dht.providers.addProvider(cid, peer.id, callback)
+    }
   }
 }

test/rpc/handlers/add-provider.spec.js

@@ -66,16 +66,17 @@ describe('rpc - handlers - AddProvider', () => {
     }))
   })
 
-  it('ignore providers not from the originator', (done) => {
+  it('ignore providers that do not match the sender', (done) => {
     const cid = values[0].cid
 
     const msg = new Message(Message.TYPES.ADD_PROVIDER, cid.buffer, 0)
-    const sender = peers[0]
-    sender.multiaddrs.add('/ip4/127.0.0.1/tcp/1234')
-    const other = peers[1]
+    const sender = _.cloneDeep(peers[0])
+    const provider = _.cloneDeep(peers[0])
+    provider.multiaddrs.add('/ip4/127.0.0.1/tcp/1234')
+    const other = _.cloneDeep(peers[1])
     other.multiaddrs.add('/ip4/127.0.0.1/tcp/2345')
     msg.providerPeers = [
-      sender,
+      provider,
       other
     ]
 
@@ -84,30 +85,32 @@ describe('rpc - handlers - AddProvider', () => {
       (cb) => dht.providers.getProviders(cid, cb),
       (provs, cb) => {
         expect(provs).to.have.length(1)
-        expect(provs[0].id).to.eql(sender.id.id)
-        const bookEntry = dht.peerBook.get(sender.id)
+        expect(provs[0].id).to.eql(provider.id.id)
+        const bookEntry = dht.peerBook.get(provider.id)
+        // Favour peerInfo from payload over peerInfo from sender
         expect(bookEntry.multiaddrs.toArray()).to.eql(
-          sender.multiaddrs.toArray()
+          provider.multiaddrs.toArray()
         )
         cb()
       }
     ], done)
   })
 
-  it('ignore providers with no multiaddrs', (done) => {
+  it('fall back to sender if providers have no multiaddrs', (done) => {
     const cid = values[0].cid
     const msg = new Message(Message.TYPES.ADD_PROVIDER, cid.buffer, 0)
     const sender = _.cloneDeep(peers[0])
-    sender.multiaddrs.clear()
-    msg.providerPeers = [sender]
+    const provider = _.cloneDeep(peers[0])
+    provider.multiaddrs.clear()
+    msg.providerPeers = [provider]
 
     waterfall([
       (cb) => handler(dht)(sender, msg, cb),
       (cb) => dht.providers.getProviders(cid, cb),
       (provs, cb) => {
+        expect(dht.peerBook.has(provider.id)).to.equal(false)
         expect(provs).to.have.length(1)
-        expect(provs[0].id).to.eql(sender.id.id)
-        expect(dht.peerBook.has(sender.id)).to.equal(false)
+        expect(provs[0].id).to.eql(provider.id.id)
         cb()
       }
     ], done)