Skip to content

fix: update provider multiaddrs before dial #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 21, 2021
Merged

fix: update provider multiaddrs before dial #286

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b01da5d
fix: udpate provider multiaddrs before dial
achingbrain Jan 19, 2021
db0aa36
chore: do not add provider multiaddrs to address book
achingbrain Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/network.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ class Network {
async findAndConnect (cid, options) {
const connectAttempts = []
for await (const provider of this.findProviders(cid, CONSTANTS.maxProvidersPerRequest, options)) {
this._log('connecting to providers', provider.id.toB58String())
connectAttempts.push(this.connectTo(provider, options))
this._log(`connecting to provider ${provider.id}`)
connectAttempts.push(this.connectTo(provider.id, options))
Copy link
Member

@vasco-santos vasco-santos Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
await Promise.all(connectAttempts)
}
Expand Down