Skip to content
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.

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

Add the full libp2p default bootstrap peer list #226

Merged
merged 1 commit into from
Jan 29, 2019

Conversation

anacrolix
Copy link
Contributor

This is useful for two specific cases in the libp2p code base at present. go-libp2p-examples/chat-with-rendezvous, and the go-libp2p-daemon.

@ghost ghost assigned anacrolix Jan 23, 2019
@ghost ghost added the status/in-progress In progress label Jan 23, 2019
@anacrolix anacrolix requested a review from geoah January 23, 2019 06:09
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Are they used anywhere in the code?
We might be better off with the bootstrappers in a separate package and not inside the dht package.

@Stebalien
Copy link
Member

This list doesn't belong in this package. There was a start to a bootstrap package (https://github.com/florianlenz/go-libp2p-bootstrap) but it never got a full review, IIRC.

The idea was that the bootstrap package would be passed to the DHT so the DHT can bootstrap on demand. (this is also one of my motivating use cases for a "service" system)

@anacrolix
Copy link
Contributor Author

There's no use of the global bootstrap nodes in the DHT code directly (fortunately no unit tests appear to be attempting to interoperate with the global network). The bootstrap addresses are strongly correlated with the DHT, and do belong here. Whether or not they're an alias for something in another package in the future is open and can be decided later. Users of the DHT will wonder where they should bootstrap from, and this is it. I strongly suspect necessary changes in the future that correct the coupling between the routing table and the host connection manager will make the importance of this bootstrap list more apparent.

@Stebalien
Copy link
Member

Fair enough. My concern was that we'll likely want to share these addresses between multiple packages but you're right, we can move them and alias. These are the bootstrap nodes known to support this DHT so it makes sense to reference them from here.

@vyzo, does that argument make sense (third party opinion)?

@vyzo
Copy link
Contributor

vyzo commented Jan 23, 2019

Let's have them on a separate package.

@anacrolix
Copy link
Contributor Author

@anacrolix
Copy link
Contributor Author

I'm ready to merge this. The Go ecosystem for libp2p is already too fragmented, and breaking this out is tedious and will reduce development inertia.

@raulk
Copy link
Member

raulk commented Jan 28, 2019

So while a bootstrap package makes sense, we'd need to model something like a matrix that associates bootstrap nodes with services they actually bootstrap into. That requires some thought.

In the meantime, no other libp2p service requires a bootstrap. I agree about self-containing these nodes here for now, and aliasing to elsewhere in the future.

@lanzafame
Copy link
Contributor

lanzafame commented Jan 28, 2019

The Go ecosystem for libp2p is already too fragmented, and breaking this out is tedious and will reduce development inertia.

Though I agree, this change has nothing to do with a Kademlia DHT implementation and more about how to bootstrap to the libp2p DHT. Do we plan to duplicate this code to the coral implementation?

EDIT: unless these bootstraps nodes will only work with a kad dht, then I have no issue with this PR. 😄

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Was gonna submit an approval, but let's finish the discussion.

@@ -10,8 +10,35 @@ import (
goprocess "github.com/jbenet/goprocess"
peer "github.com/libp2p/go-libp2p-peer"
routing "github.com/libp2p/go-libp2p-routing"
multiaddr "github.com/multiformats/go-multiaddr"
Copy link
Member

Choose a reason for hiding this comment

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

nit: lo and behold, convention is to alias multiaddr as ma. Change would be appreciated for consistency.

@raulk
Copy link
Member

raulk commented Jan 28, 2019

@lanzafame Yeah, Coral is nowhere ready to be deployed. When the moment comes to factor these hosts out (if ever, even... we might decide we want dedicated hostnames:ports for specific services, for scalability and infrastructure reasons!), we can alias.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Future infrastructure of bootstrap nodes ran by PL for other services is uncertain. There's no point holding this change back; if we decide to factor out, we can always alias.

@anacrolix anacrolix merged commit bebd753 into master Jan 29, 2019
@ghost ghost removed the status/in-progress In progress label Jan 29, 2019
@ghost
Copy link

ghost commented Feb 6, 2019

This changeset adds an implicit dependency on go-multiaddr-dns, because otherwise /dnsaddr multiaddrs aren't supported and this will panic.

@ghost
Copy link

ghost commented Feb 6, 2019

I'm thinking this list should simply be where the rest of the libp2p default settings are.

@ghost
Copy link

ghost commented Feb 6, 2019

This changeset adds an implicit dependency on go-multiaddr-dns, because otherwise /dnsaddr multiaddrs aren't supported and this will panic.

Nevermind -- go-multiaddr-net pulls in go-multiaddr-dns, so it's going to be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants