fix: update provider multiaddrs before dial #286
Conversation
The output of `libp2p.contentRouting.findProviders()` is `AsyncIterable<{ id: PeerId, multiaddrs: Multiaddr[] }>`
and not `AsyncIterable<PeerId>` as the code seems to assume.
In order to ensure we can successfully dial a content provider we've not
dialled before, add their multiaddrs to the peer store before dialling them.
achingbrain
changed the title
| connectAttempts.push(this.connectTo(provider, options)) | ||
|
|
||
| this._libp2p.peerStore.addressBook.set(provider.id, provider.multiaddrs) | ||
| connectAttempts.push(this.connectTo(provider.id, options)) |
There was a problem hiding this comment.
Libp2p should be responsible for this. In the DHT we add discovered multiaddrs to the addressBook when we do findPeer, but apparently not when we do findProviders. In the delegated side of things, we don't add them at all.
So, I think we should follow exactly the same approach as with the peer-discovery modules, which libp2p adds to the AddressBook. In this case, we should add the multiaddrs to the peerStore on findProviders, findPeer and getClosestPeers. We can later remove some redundant addressBooks adds on the DHT, but we need to be careful with this as DHT queries might discover important peers to store that might not be returned in these APIs.
What do you think? Do you want to get that in libp2p, or should I do it?
A side note, we should use add here as we might already know other multiaddrs of the peer that we can use in the following connect. Only identify should use set at this moment, as this is the source of truth
There was a problem hiding this comment.
I've added the storing of multiaddrs to findProviders here: libp2p/js-libp2p#865 also made it a lot faster by returning results as they come in instead of waiting for one of the lookups to finish first.
I'll expand it to perform the same operations for findPeer and getClosestPeers tomorrow..
The output of
libp2p.contentRouting.findProviders()isAsyncIterable<{ id: PeerId, multiaddrs: Multiaddr[] }>and not
AsyncIterable<PeerId>as the code seems to assume.In order to ensure we can successfully dial a content provider we've not
dialled before, add their multiaddrs to the peer store before dialling them.
@vasco-santos is this the right place to do this? Seems like it should happen in libp2p?