Make fork id the default and try to recover the DiscoveryPeer for incoming connections from the PeerTable

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java

@@ -37,7 +37,7 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
   private final String DNS_DISCOVERY_SERVER_OVERRIDE_FLAG = "--Xp2p-dns-discovery-server";
   private final String DISCOVERY_PROTOCOL_V5_ENABLED = "--Xv5-discovery-enabled";
   /** The constant FILTER_ON_ENR_FORK_ID. */
-  public static final String FILTER_ON_ENR_FORK_ID = "--Xfilter-on-enr-fork-id";
+  public static final String FILTER_ON_ENR_FORK_ID = "--filter-on-enr-fork-id";
 
   @CommandLine.Option(
       names = INITIATE_CONNECTIONS_FREQUENCY_FLAG,
@@ -76,9 +76,9 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
   @CommandLine.Option(
       names = FILTER_ON_ENR_FORK_ID,
       hidden = true,
-      defaultValue = "false",
+      defaultValue = "true",
       description = "Whether to enable filtering of peers based on the ENR field ForkId)")
-  private final Boolean filterOnEnrForkId = false;
+  private final Boolean filterOnEnrForkId = NetworkingConfiguration.DEFAULT_FILTER_ON_ENR_FORK_ID;
 
   @CommandLine.Option(
       hidden = true,

besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java

@@ -134,7 +134,7 @@ public void checkFilterByForkIdNotSet() {
 
     final NetworkingOptions options = cmd.getNetworkingOptions();
     final NetworkingConfiguration networkingConfig = options.toDomainObject();
-    assertThat(networkingConfig.getDiscovery().isFilterOnEnrForkIdEnabled()).isEqualTo(false);
+    assertThat(networkingConfig.getDiscovery().isFilterOnEnrForkIdEnabled()).isEqualTo(true);
 
     assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
     assertThat(commandOutput.toString(UTF_8)).isEmpty();

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

@@ -139,6 +139,7 @@ public EthPeers(
         "peer_limit",
         "The maximum number of peers this node allows to connect",
         () -> peerUpperBound);
+
     connectedPeersCounter =
         metricsSystem.createCounter(
             BesuMetricCategory.PEERS, "connected_total", "Total number of peers connected");

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

@@ -110,7 +110,7 @@ public EthProtocolManager(
 
     this.blockBroadcaster = new BlockBroadcaster(ethContext);
 
-    supportedCapabilities =
+    this.supportedCapabilities =
         calculateCapabilities(synchronizerConfiguration, ethereumWireProtocolConfiguration);
 
     // Run validators
@@ -380,11 +380,12 @@ public void handleNewConnection(final PeerConnection connection) {
   @Override
   public boolean shouldConnect(final Peer peer, final boolean incoming) {
     if (peer.getForkId().map(forkId -> forkIdManager.peerCheck(forkId)).orElse(true)) {
-      LOG.trace("ForkId OK or not available");
+      LOG.debug("ForkId OK or not available for peer {}", peer.getId());
       if (ethPeers.shouldConnect(peer, incoming)) {
         return true;
       }
     }
+    LOG.debug("ForkId check failed for peer {}", peer.getId());
     return false;
   }
 
@@ -412,35 +413,30 @@ private void handleStatusMessage(final EthPeer peer, final Message message) {
     try {
       if (!status.networkId().equals(networkId)) {
         LOG.atDebug()
-            .setMessage("Mismatched network id: {}, EthPeer {}...")
+            .setMessage("Mismatched network id: {}, peer {}")
             .addArgument(status.networkId())
-            .addArgument(peer.getShortNodeId())
-            .log();
-        LOG.atTrace()
-            .setMessage("Mismatched network id: {}, EthPeer {}")
-            .addArgument(status.networkId())
-            .addArgument(peer)
+            .addArgument(LOG.isTraceEnabled() ? peer : peer.getShortNodeId())
             .log();
         peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
       } else if (!forkIdManager.peerCheck(forkId) && status.protocolVersion() > 63) {
         LOG.debug(
             "{} has matching network id ({}), but non-matching fork id: {}",
-            peer,
+            LOG.isTraceEnabled() ? peer : peer.getShortNodeId(),
             networkId,
             forkId);
         peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
       } else if (forkIdManager.peerCheck(status.genesisHash())) {
         LOG.debug(
             "{} has matching network id ({}), but non-matching genesis hash: {}",
-            peer,
+            LOG.isTraceEnabled() ? peer : peer.getShortNodeId(),
             networkId,
             status.genesisHash());
         peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
       } else if (mergePeerFilter.isPresent()
           && mergePeerFilter.get().disconnectIfPoW(status, peer)) {
         LOG.atDebug()
             .setMessage("Post-merge disconnect: peer still PoW {}")
-            .addArgument(peer.getShortNodeId())
+            .addArgument(LOG.isTraceEnabled() ? peer : peer.getShortNodeId())
             .log();
         handleDisconnect(peer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED, false);
       } else {

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/DiscoveryConfiguration.java

@@ -32,7 +32,7 @@ public class DiscoveryConfiguration {
   private List<EnodeURL> bootnodes = new ArrayList<>();
   private String dnsDiscoveryURL;
   private boolean discoveryV5Enabled = false;
-  private boolean filterOnEnrForkId = false;
+  private boolean filterOnEnrForkId = NetworkingConfiguration.DEFAULT_FILTER_ON_ENR_FORK_ID;
 
   public static DiscoveryConfiguration create() {
     return new DiscoveryConfiguration();

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java

@@ -23,6 +23,7 @@ public class NetworkingConfiguration {
   public static final int DEFAULT_INITIATE_CONNECTIONS_FREQUENCY_SEC = 30;
   public static final int DEFAULT_CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_SEC = 60;
   public static final int DEFAULT_PEER_LOWER_BOUND = 25;
+  public static final boolean DEFAULT_FILTER_ON_ENR_FORK_ID = true;
 
   private DiscoveryConfiguration discovery = new DiscoveryConfiguration();
   private RlpxConfiguration rlpx = new RlpxConfiguration();

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

@@ -26,6 +26,7 @@
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerDiscoveryController;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerRequirement;
+import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerTable;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.PingPacketData;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.TimerUtil;
 import org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl;
@@ -81,6 +82,7 @@ public abstract class PeerDiscoveryAgent {
   private final MetricsSystem metricsSystem;
   private final RlpxAgent rlpxAgent;
   private final ForkIdManager forkIdManager;
+  private final PeerTable peerTable;
 
   /* The peer controller, which takes care of the state machine of peers. */
   protected Optional<PeerDiscoveryController> controller = Optional.empty();
@@ -109,7 +111,8 @@ protected PeerDiscoveryAgent(
       final MetricsSystem metricsSystem,
       final StorageProvider storageProvider,
       final ForkIdManager forkIdManager,
-      final RlpxAgent rlpxAgent) {
+      final RlpxAgent rlpxAgent,
+      final PeerTable peerTable) {
     this.metricsSystem = metricsSystem;
     checkArgument(nodeKey != null, "nodeKey cannot be null");
     checkArgument(config != null, "provided configuration cannot be null");
@@ -130,6 +133,7 @@ protected PeerDiscoveryAgent(
     this.forkIdManager = forkIdManager;
     this.forkIdSupplier = () -> forkIdManager.getForkIdForChainHead().getForkIdAsBytesList();
     this.rlpxAgent = rlpxAgent;
+    this.peerTable = peerTable;
   }
 
   protected abstract TimerUtil createTimer();
@@ -263,9 +267,9 @@ private PeerDiscoveryController createController(final DiscoveryPeer localNode)
         .peerRequirement(PeerRequirement.combine(peerRequirements))
         .peerPermissions(peerPermissions)
         .metricsSystem(metricsSystem)
-        .forkIdManager(forkIdManager)
         .filterOnEnrForkId((config.isFilterOnEnrForkIdEnabled()))
         .rlpxAgent(rlpxAgent)
+        .peerTable(peerTable)
         .build();
   }
 

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

@@ -23,6 +23,7 @@
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.Packet;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerDiscoveryController;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor;
+import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerTable;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.TimerUtil;
 import org.hyperledger.besu.ethereum.p2p.discovery.internal.VertxTimerUtil;
 import org.hyperledger.besu.ethereum.p2p.permissions.PeerPermissions;
@@ -73,7 +74,8 @@ public VertxPeerDiscoveryAgent(
       final MetricsSystem metricsSystem,
       final StorageProvider storageProvider,
       final ForkIdManager forkIdManager,
-      final RlpxAgent rlpxAgent) {
+      final RlpxAgent rlpxAgent,
+      final PeerTable peerTable) {
     super(
         nodeKey,
         config,
@@ -82,7 +84,8 @@ public VertxPeerDiscoveryAgent(
         metricsSystem,
         storageProvider,
         forkIdManager,
-        rlpxAgent);
+        rlpxAgent,
+        peerTable);
     checkArgument(vertx != null, "vertx instance cannot be null");
     this.vertx = vertx;
 

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

@@ -21,8 +21,6 @@
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 import org.hyperledger.besu.cryptoservices.NodeKey;
-import org.hyperledger.besu.ethereum.forkid.ForkId;
-import org.hyperledger.besu.ethereum.forkid.ForkIdManager;
 import org.hyperledger.besu.ethereum.p2p.discovery.DiscoveryPeer;
 import org.hyperledger.besu.ethereum.p2p.discovery.PeerDiscoveryStatus;
 import org.hyperledger.besu.ethereum.p2p.peers.Peer;
@@ -129,7 +127,6 @@ public class PeerDiscoveryController {
   private final DiscoveryProtocolLogger discoveryProtocolLogger;
   private final LabelledMetric<Counter> interactionCounter;
   private final LabelledMetric<Counter> interactionRetryCounter;
-  private final ForkIdManager forkIdManager;
   private final boolean filterOnEnrForkId;
   private final RlpxAgent rlpxAgent;
 
@@ -161,7 +158,6 @@ private PeerDiscoveryController(
       final PeerPermissions peerPermissions,
       final MetricsSystem metricsSystem,
       final Optional<Cache<Bytes, Packet>> maybeCacheForEnrRequests,
-      final ForkIdManager forkIdManager,
       final boolean filterOnEnrForkId,
       final RlpxAgent rlpxAgent) {
     this.timerUtil = timerUtil;
@@ -197,11 +193,11 @@ private PeerDiscoveryController(
             "discovery_interaction_retry_count",
             "Total number of interaction retries performed",
             "type");
+
     this.cachedEnrRequests =
         maybeCacheForEnrRequests.orElse(
             CacheBuilder.newBuilder().maximumSize(50).expireAfterWrite(10, SECONDS).build());
 
-    this.forkIdManager = forkIdManager;
     this.filterOnEnrForkId = filterOnEnrForkId;
   }
 
@@ -314,6 +310,7 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) {
     }
 
     final DiscoveryPeer peer = resolvePeer(sender);
+    final Bytes peerId = peer.getId();
     switch (packet.getType()) {
       case PING:
         if (peerPermissions.allowInboundBonding(peer)) {
@@ -333,10 +330,10 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) {
                   if (filterOnEnrForkId) {
                     requestENR(peer);
                   }
-                  bondingPeers.invalidate(peer.getId());
+                  bondingPeers.invalidate(peerId);
                   addToPeerTable(peer);
                   recursivePeerRefreshState.onBondingComplete(peer);
-                  Optional.ofNullable(cachedEnrRequests.getIfPresent(peer.getId()))
+                  Optional.ofNullable(cachedEnrRequests.getIfPresent(peerId))
                       .ifPresent(cachedEnrRequest -> processEnrRequest(peer, cachedEnrRequest));
                 });
         break;
@@ -360,12 +357,12 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) {
         if (PeerDiscoveryStatus.BONDED.equals(peer.getStatus())) {
           processEnrRequest(peer, packet);
         } else if (PeerDiscoveryStatus.BONDING.equals(peer.getStatus())) {
-          LOG.trace("ENR_REQUEST cached for bonding peer Id: {}", peer.getId());
+          LOG.trace("ENR_REQUEST cached for bonding peer Id: {}", peerId);
           // Due to UDP, it may happen that we receive the ENR_REQUEST just before the PONG.
           // Because peers want to send the ENR_REQUEST directly after the pong.
           // If this happens we don't want to ignore the request but process when bonded.
           // this cache allows to keep the request and to respond after having processed the PONG
-          cachedEnrRequests.put(peer.getId(), packet);
+          cachedEnrRequests.put(peerId, packet);
         }
         break;
       case ENR_RESPONSE:
@@ -376,26 +373,6 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) {
                       packet.getPacketData(ENRResponsePacketData.class);
                   final NodeRecord enr = packetData.get().getEnr();
                   peer.setNodeRecord(enr);
-
-                  final Optional<ForkId> maybeForkId = peer.getForkId();
-                  if (maybeForkId.isPresent()) {
-                    if (forkIdManager.peerCheck(maybeForkId.get())) {
-                      connectOnRlpxLayer(peer);
-                      LOG.debug(
-                          "Peer {} PASSED fork id check. ForkId received: {}",
-                          sender.getId(),
-                          maybeForkId.get());
-                    } else {
-                      LOG.debug(
-                          "Peer {} FAILED fork id check. ForkId received: {}",
-                          sender.getId(),
-                          maybeForkId.get());
-                    }
-                  } else {
-                    // if the peer hasn't sent the ForkId try to connect to it anyways
-                    connectOnRlpxLayer(peer);
-                    LOG.debug("No fork id sent by peer: {}", peer.getId());
-                  }
                 });
         break;
     }
@@ -431,9 +408,7 @@ private boolean addToPeerTable(final DiscoveryPeer peer) {
 
     if (peer.getStatus() != PeerDiscoveryStatus.BONDED) {
       peer.setStatus(PeerDiscoveryStatus.BONDED);
-      if (!filterOnEnrForkId) {
-        connectOnRlpxLayer(peer);
-      }
+      connectOnRlpxLayer(peer);
     }
 
     final PeerTable.AddResult result = peerTable.tryAdd(peer);
@@ -560,8 +535,6 @@ void bond(final DiscoveryPeer peer) {
    */
   @VisibleForTesting
   void requestENR(final DiscoveryPeer peer) {
-    peer.setStatus(PeerDiscoveryStatus.ENR_REQUESTED);
-
     final Consumer<PeerInteractionState> action =
         interaction -> {
           final ENRRequestPacketData data = ENRRequestPacketData.create();
@@ -838,18 +811,13 @@ public static class Builder {
 
     private Cache<Bytes, Packet> cachedEnrRequests =
         CacheBuilder.newBuilder().maximumSize(50).expireAfterWrite(10, SECONDS).build();
-    private ForkIdManager forkIdManager;
     private RlpxAgent rlpxAgent;
 
     private Builder() {}
 
     public PeerDiscoveryController build() {
       validate();
 
-      if (peerTable == null) {
-        peerTable = new PeerTable(this.nodeKey.getPublicKey().getEncodedBytes(), 16);
-      }
-
       return new PeerDiscoveryController(
           nodeKey,
           localPeer,
@@ -864,7 +832,6 @@ public PeerDiscoveryController build() {
           peerPermissions,
           metricsSystem,
           Optional.of(cachedEnrRequests),
-          forkIdManager,
           filterOnEnrForkId,
           rlpxAgent);
     }
@@ -875,8 +842,8 @@ private void validate() {
       validateRequiredDependency(timerUtil, "TimerUtil");
       validateRequiredDependency(workerExecutor, "AsyncExecutor");
       validateRequiredDependency(metricsSystem, "MetricsSystem");
-      validateRequiredDependency(forkIdManager, "ForkIdManager");
       validateRequiredDependency(rlpxAgent, "RlpxAgent");
+      validateRequiredDependency(peerTable, "PeerTable");
     }
 
     private void validateRequiredDependency(final Object object, final String name) {
@@ -970,11 +937,5 @@ public Builder rlpxAgent(final RlpxAgent rlpxAgent) {
       this.rlpxAgent = rlpxAgent;
       return this;
     }
-
-    public Builder forkIdManager(final ForkIdManager forkIdManager) {
-      checkNotNull(forkIdManager);
-      this.forkIdManager = forkIdManager;
-      return this;
-    }
   }
 }