-
Notifications
You must be signed in to change notification settings - Fork 891
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
[Issue 1278] Rework static peer discovery handling #1292
[Issue 1278] Rework static peer discovery handling #1292
Conversation
Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net>
Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net>
Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net>
Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net>
Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net>
@@ -265,6 +265,7 @@ public void awaitStop() { | |||
@Override | |||
public boolean addMaintainConnectionPeer(final Peer peer) { | |||
final boolean wasAdded = maintainedPeers.add(peer); | |||
peerDiscoveryAgent.bond(peer); |
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.
Does this mean that we require discovery to be enabled on peers that we want to add using static-nodes.json file or AddPeer API? Because I feel like most uses that are statically mapping their peers might not have discovery enabled (and might not want 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.
No - we try to create a DiscoveryPeer
to initiate bonding with. The DiscoveryPeer.from
helper will only return a peer if discovery appears to be enabled. The bonding process consists of sending a PING
to that peer, and waiting for a response. Only if the peer is running discovery and responds do we add the peer to our table. If the peer doesn't respond, we'll retry a few times and then stop.
Users can explicitly indicate that discovery is disabled by supplying enode urls with discport=0
, which will cause us to skip the bonding attempt.
PR description
Rework discovery-level handling of static peers to fix a few issues:
dropPeer
logicFixed Issue(s)
Fixes #1278