-
Notifications
You must be signed in to change notification settings - Fork 109
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
Implement peer discovery #69
Conversation
@@ -166,7 +166,7 @@ func (w *Watcher) pollNextBlock() error { | |||
if err == ethereum.NotFound { | |||
log.WithFields(log.Fields{ | |||
"blockNumber": nextBlockNumber, | |||
}).Info("block header not found") | |||
}).Trace("block header not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabioberger This log was getting pretty noisy. I think it is always expected to happen while we are waiting for the next block to be mined. Are you okay with changing this to Trace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally, thanks for making that change. It is an expected condition.
@@ -253,6 +388,5 @@ func (n *Node) receive(ctx context.Context) (*Message, error) { | |||
// Close closes the Node and any active connections. | |||
func (n *Node) Close() error { | |||
n.cancel() | |||
n.sub.Cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're passing in n.ctx
to n.sub
during initialization so it now closes automatically when we call n.cancel
.
c2ada23
to
50df6ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback off the top of my head. Let me know if you'd like a deeper review!
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
if err := n.host.Connect(connectCtx, *peerInfo); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connect
will establish a connection and trigger the notifees on protocols like the DHT. Those protocols will in turn open streams to the connected peer to test whether the other party supports the protocol, and will perform any init logic in consequence (e.g. adding the peer to a routing table). Until that happens, you won't have a working DHT node. Since the notifees are triggered asynchronously, so you should add like a 2 second wait after all dials completed.
We are speccing out a local event bus (will tag you in that issue) that will allow us to be deterministic instead of probabilistic in the near future (you'll listen to an event instead of relying on a timing condition), but until that's a thing, this is probably the safest path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll add a 2 second wait after dialing all the bootstrap peers for now. Also wanted to point out that the chat-with-rendezvous example doesn't do this: https://github.com/libp2p/go-libp2p-examples/blob/master/chat-with-rendezvous/chat.go. Should we add a time.Sleep
there as well?
Instead of protecting local connections, we now tag peers who speak the 0x-mesh-floodsub protocol. This gives them a slightly positive score and makes them less likely to be removed by the Connection Manager.
588a554
to
dff51cb
Compare
@fabioberger removed WIP tag. We will probably make improvements to peer discovery and peer ranking over time, but this PR is ready for final review. |
Fixes #72 |
core/notifee.go
Outdated
func (n *notifee) OpenedStream(network p2pnet.Network, stream p2pnet.Stream) { | ||
go func() { | ||
// HACK(albrow): When the stream is initially opened, the protocol is not set. | ||
// For now, we have to manually poll until it is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a PR or issue you could add a link to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a link to this issue: libp2p/go-libp2p#467. I'm not 100% sure if it will help us avoid this hack; it depends on how exactly the event bus is implemented.
This PR adds support for peer discovery via go-libp2p-kad-dht and go-libp2p-discovery. It also adds a new environment variable
USE_BOOTSTRAP_LIST
. If set to "true", it uses the IPFS default bootstrap list to bootstrap the DHT.WIP right now because of some weird behavior when using the bootstrap list. If you set it to "false", you can test peer discovery by following these steps:
AddPeer
RPC method.AddPeer
RPC method.