DNS peers handled the same as boot nodes #4178

pinges · 2022-07-27T12:28:36Z

Peers discovered through the DNSDaemon are now added to the peerTable. These peers are potentially used (16 are randomly chosen from the peer table) in RecursivePeerRefreshStateas as the initial peers for bonding and then for finding neighbours, etc.

Signed-off-by: Stefan stefan.pingel@consensys.net

Signed-off-by: Stefan <stefan.pingel@consensys.net>

…InitialNodes

Signed-off-by: Stefan <stefan.pingel@consensys.net>

matkt

just a few questions to understand the PR

matkt · 2022-07-28T07:52:59Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

@@ -278,6 +281,14 @@ public void processMessage(final Capability cap, final Message message) {
      return;
    }

+    if (messageData.getSize() > this.maxMessageSize) {


not sure why this modification is present in the PR

.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

matkt · 2022-07-28T07:56:13Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

@@ -347,11 +344,16 @@ DNSDaemonListener createDaemonListener() {
                .build();
        final DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL);
        peers.add(peer);
-        rlpxAgent.connect(peer);


why are we no longer trying to connect to the peer?

you put this back in, right @pinges

It looks like it is just moved to the subsequent block on line 357 after adding to the peer table

Signed-off-by: Stefan <stefan.pingel@consensys.net>

macfarla · 2022-08-02T01:45:28Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

+          messageData.getSize(),
+          this.maxMessageSize);
+    }
+


this is useful, although unrelated to this PR

macfarla · 2022-08-02T01:46:29Z

...ain/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

@@ -93,6 +93,7 @@ private boolean reachedMaximumNumberOfRounds() {
  }

  private void addInitialPeers(final List<DiscoveryPeer> initialPeers) {
+    LOG.debug("INITIAL PEERS: {}", initialPeers);


another log not needed

macfarla · 2022-08-02T01:47:37Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

@@ -347,11 +344,16 @@ DNSDaemonListener createDaemonListener() {
                .build();
        final DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL);
        peers.add(peer);
-        rlpxAgent.connect(peer);


you put this back in, right @pinges

.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

Signed-off-by: Stefan <stefan.pingel@consensys.net>

…InitialNodes

jflo

Tested and a slight initial speedup in gained peers was observed.

garyschulte · 2022-08-02T19:45:52Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java

+   * @param limit The amount of results to return.
+   * @return The <code>limit</code> closest peers, at most.
+   */
+  public List<DiscoveryPeer> nearestBondedPeers(final Bytes target, final int limit) {


I presume this is necessary in order to not give remote clients a list of unverified peers from DNSDaemon lookups?

This is using the definition of distance from the devP2P spec to find the limit number of closest peers to the target.
This is needed to respond to a peer asking for neighbours.

garyschulte

retroactive LGTM

mbaxter · 2022-08-09T19:48:46Z

.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

-    final PeerTable.AddResult result = peerTable.tryAdd(peer);
-    if (result.getOutcome() == PeerTable.AddResult.AddOutcome.SELF) {
-      return false;
-    }
-
    // Reset the last seen timestamp.


(bug) This change is problematic - if we discover our own node, we will now attempt to connect to ourselves because we're no longer short-circuiting the notifyPeerBonded logic below. The call to notifyPeerBonded dispatches an event which will cause the RlpxAgent to attempt a new connection.

This check is not needed anymore. The peerPermissions.isallowedInPeerTable takes care of this, as the local node is checked in the PeerPermissionsDenylist isPermitted() method.

mbaxter · 2022-08-09T19:55:05Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

+    if (messageData.getSize() > this.maxMessageSize) {
+      LOG.debug(
+          "Peer {} sent a message with size {}, larger than the max message size {}",
+          ethPeer,
+          messageData.getSize(),
+          this.maxMessageSize);
+    }


We've had 2 tickets and 3 PRs recently touch this logic, and now we're silently modifying it again in an unrelated PR.
Related Issues:

Besu peers send messages exceeding the 10MB message size limit #3867

Increase default maximum size for p2p messages #4119

We should not be including these kinds of changes mixed in with other functional changes. There's also no explanation for why we're changing this again here.

Note: I'm not saying we should remove this, just that as a practice we should not be lumping in unrelated functional changes into a PR with no documentation on why the change is being made.

This is just adding debug info so we can see which clients are sending these messages

I've removed that. The only time I saw it in the logs was from an older Besu version!

mbaxter · 2022-08-09T19:58:27Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java

+  public Optional<PeerDiscoveryController> getPeerDiscoveryController() {
+    return controller;
+  }
+


The PeerDiscoveryController is marked as internal and is not intended to be accessed from outside the discovery package. Any functionally that is needed should be exposed through PeerDiscoveryAgent.

mbaxter · 2022-08-09T20:01:16Z

.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

+  public boolean addToPeerTable(final DiscoveryPeer peer) {
+    final PeerTable.AddResult result = peerTable.tryAdd(peer);
+
+    if (result.getOutcome() == PeerTable.AddResult.AddOutcome.SELF) {
+      return false;
+    }


(bug) This method bypasses peerPermissions.

Yes, this was missed. I will create a new PR to add this check.

mbaxter · 2022-08-09T20:08:54Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java

@@ -193,7 +211,6 @@ private void buildBloomFilter() {
  public List<DiscoveryPeer> nearestPeers(final Bytes target, final int limit) {
    final Bytes keccak256 = Hash.keccak256(target);
    return streamAllPeers()
-        .filter(p -> p.getStatus() == PeerDiscoveryStatus.BONDED)


Why has this method been modified? And why did we create the very similar nearestBondedPeers? It looks like the functional effect of this is to make the periodic peer searches run across all peers, not just bonded peers. Perhaps this is desired, but it should be addressed in a separate PR.

I suspect the reason this was added is because we're now directly injecting peers into the discovery table instead of bonding with them first. I would suggest reverting these changes, and attempting to bond with the DNS peers rather than directly injecting them.

This would be cleaner, I guess. I will do that in a separate PR.

mbaxter · 2022-08-09T20:20:27Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

-                      60000L,
+                      1800000L,


If the call to the DNS server fails, this introduces a big time lag before we try again.

good point. maybe somewhere in between?

mbaxter · 2022-08-09T20:23:25Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

+        if (peerDiscoveryController.isPresent()) {
+          final PeerDiscoveryController controller = peerDiscoveryController.get();
+          LOG.debug("Adding {} DNS peers to PeerTable", peers.size());
+          peers.forEach(controller::addToPeerTable);


As mentioned previously, we should be calling PeerDiscoveryAgent.bond here instead of directly injecting peers into the discovery peer table.

* DNS peers handled the same as boot nodes * make sure that non bonded peers can be used as initial peers * try to connect to DNS nodes Signed-off-by: Stefan <stefan.pingel@consensys.net> Co-authored-by: Justin Florentine <justin+github@florentine.us>

pinges added 5 commits July 27, 2022 22:13

DNS peers handled the same as boot nodes

35dc5ba

Signed-off-by: Stefan <stefan.pingel@consensys.net>

fix unit test

6cccae8

Signed-off-by: Stefan <stefan.pingel@consensys.net>

spotless

789044f

Signed-off-by: Stefan <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into addEnpNodesTo…

543960e

…InitialNodes

make sure that non bonded peers can be used as initial peers

ec9e307

Signed-off-by: Stefan <stefan.pingel@consensys.net>

macfarla added mainnet peering labels Jul 27, 2022

matkt reviewed Jul 28, 2022

View reviewed changes

pinges added 2 commits July 28, 2022 18:57

try to connect to DNS nodes

521022b

Signed-off-by: Stefan <stefan.pingel@consensys.net>

spotless

4ec655e

Signed-off-by: Stefan <stefan.pingel@consensys.net>

iamhsk added the TeamRevenant GH issues worked on by Revenant Team label Aug 1, 2022

macfarla reviewed Aug 2, 2022

View reviewed changes

.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java Show resolved Hide resolved

pinges added 2 commits August 2, 2022 17:27

clean up

da258ef

Signed-off-by: Stefan <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into addEnpNodesTo…

e885f20

…InitialNodes

jflo approved these changes Aug 2, 2022

View reviewed changes

Merge branch 'main' into addEnpNodesToInitialNodes

54bc957

garyschulte reviewed Aug 2, 2022

View reviewed changes

jflo merged commit 42cfff0 into hyperledger:main Aug 2, 2022

garyschulte approved these changes Aug 2, 2022

View reviewed changes

pinges deleted the addEnpNodesToInitialNodes branch August 2, 2022 23:21

pinges self-assigned this Aug 3, 2022

mbaxter reviewed Aug 9, 2022

View reviewed changes

pinges mentioned this pull request Aug 16, 2022

cleanup from PR 4178 #4266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DNS peers handled the same as boot nodes #4178

DNS peers handled the same as boot nodes #4178

pinges commented Jul 27, 2022

matkt left a comment

matkt Jul 28, 2022

matkt Jul 28, 2022

macfarla Aug 2, 2022

garyschulte Aug 2, 2022

macfarla Aug 2, 2022

macfarla Aug 2, 2022

macfarla Aug 2, 2022

jflo left a comment

garyschulte Aug 2, 2022

pinges Aug 16, 2022 •

edited

Loading

garyschulte left a comment

mbaxter Aug 9, 2022

pinges Aug 16, 2022 •

edited

Loading

mbaxter Aug 9, 2022

pinges Aug 16, 2022

pinges Aug 16, 2022

mbaxter Aug 9, 2022

pinges Aug 16, 2022

mbaxter Aug 9, 2022

pinges Aug 16, 2022

mbaxter Aug 9, 2022

pinges Aug 16, 2022

mbaxter Aug 9, 2022

macfarla Aug 16, 2022

mbaxter Aug 9, 2022

pinges Aug 16, 2022

DNS peers handled the same as boot nodes #4178

DNS peers handled the same as boot nodes #4178

Conversation

pinges commented Jul 27, 2022

matkt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jflo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

garyschulte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinges Aug 16, 2022 •

edited

Loading

pinges Aug 16, 2022 •

edited

Loading