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/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java

@@ -1025,7 +1025,8 @@ protected EthProtocolManager createEthProtocolManager(
         synchronizerConfiguration,
         scheduler,
         genesisConfig.getForkBlockNumbers(),
-        genesisConfig.getForkTimestamps());
+        genesisConfig.getForkTimestamps(),
+        metricsSystem);
   }
 
   /**

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

@@ -58,6 +58,11 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import org.apache.tuweni.bytes.Bytes;
+import org.hyperledger.besu.metrics.BesuMetricCategory;
+import org.hyperledger.besu.metrics.ObservableMetricsSystem;
+import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem;
+import org.hyperledger.besu.plugin.services.metrics.Counter;
+import org.hyperledger.besu.plugin.services.metrics.LabelledMetric;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -79,6 +84,7 @@ public class EthProtocolManager implements ProtocolManager, MinedBlockObserver {
   private final BlockBroadcaster blockBroadcaster;
   private final List<PeerValidator> peerValidators;
   private final Optional<MergePeerFilter> mergePeerFilter;
+  private final LabelledMetric<Counter> forkIdCounter;
 
   public EthProtocolManager(
       final Blockchain blockchain,
@@ -93,7 +99,8 @@ public EthProtocolManager(
       final Optional<MergePeerFilter> mergePeerFilter,
       final SynchronizerConfiguration synchronizerConfiguration,
       final EthScheduler scheduler,
-      final ForkIdManager forkIdManager) {
+      final ForkIdManager forkIdManager,
+      final ObservableMetricsSystem metricsSystem) {
     this.networkId = networkId;
     this.peerValidators = peerValidators;
     this.scheduler = scheduler;
@@ -110,7 +117,7 @@ public EthProtocolManager(
 
     this.blockBroadcaster = new BlockBroadcaster(ethContext);
 
-    supportedCapabilities =
+    this.supportedCapabilities =
         calculateCapabilities(synchronizerConfiguration, ethereumWireProtocolConfiguration);
 
     // Run validators
@@ -125,6 +132,12 @@ public EthProtocolManager(
         transactionPool,
         ethMessages,
         ethereumWireProtocolConfiguration);
+
+    this.forkIdCounter = metricsSystem.createLabelledCounter(
+            BesuMetricCategory.NETWORK,
+            "discovery_fork_id_counter",
+            "total number of successful, failed fork id checks, as well as fork id not present",
+            "name");
   }
 
   @VisibleForTesting
@@ -158,24 +171,26 @@ public EthProtocolManager(
             blockchain,
             Collections.emptyList(),
             Collections.emptyList(),
-            ethereumWireProtocolConfiguration.isLegacyEth64ForkIdEnabled()));
+            ethereumWireProtocolConfiguration.isLegacyEth64ForkIdEnabled()),
+            new NoOpMetricsSystem());
   }
 
   public EthProtocolManager(
-      final Blockchain blockchain,
-      final BigInteger networkId,
-      final WorldStateArchive worldStateArchive,
-      final TransactionPool transactionPool,
-      final EthProtocolConfiguration ethereumWireProtocolConfiguration,
-      final EthPeers ethPeers,
-      final EthMessages ethMessages,
-      final EthContext ethContext,
-      final List<PeerValidator> peerValidators,
-      final Optional<MergePeerFilter> mergePeerFilter,
-      final SynchronizerConfiguration synchronizerConfiguration,
-      final EthScheduler scheduler,
-      final List<Long> blockNumberForks,
-      final List<Long> timestampForks) {
+          final Blockchain blockchain,
+          final BigInteger networkId,
+          final WorldStateArchive worldStateArchive,
+          final TransactionPool transactionPool,
+          final EthProtocolConfiguration ethereumWireProtocolConfiguration,
+          final EthPeers ethPeers,
+          final EthMessages ethMessages,
+          final EthContext ethContext,
+          final List<PeerValidator> peerValidators,
+          final Optional<MergePeerFilter> mergePeerFilter,
+          final SynchronizerConfiguration synchronizerConfiguration,
+          final EthScheduler scheduler,
+          final List<Long> blockNumberForks,
+          final List<Long> timestampForks,
+          final ObservableMetricsSystem metricsSystem) {
     this(
         blockchain,
         networkId,
@@ -193,7 +208,9 @@ public EthProtocolManager(
             blockchain,
             blockNumberForks,
             timestampForks,
-            ethereumWireProtocolConfiguration.isLegacyEth64ForkIdEnabled()));
+            ethereumWireProtocolConfiguration.isLegacyEth64ForkIdEnabled()),
+            metricsSystem
+    );
   }
 
   public EthContext ethContext() {
@@ -411,39 +428,39 @@ private void handleStatusMessage(final EthPeer peer, final Message message) {
     peer.getConnection().getPeer().setForkId(forkId);
     try {
       if (!status.networkId().equals(networkId)) {
+        forkIdCounter.labels("FAIL").inc(); // because fork id check would fail
         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) {
+        forkIdCounter.labels("FAIL").inc();
         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())) {
+        forkIdCounter.labels("FAIL").inc();
         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)) {
+        forkIdCounter.labels("NA").inc();
         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 {
+        forkIdCounter.labels("SUCC").inc();
         LOG.debug(
             "Received status message from {}: {} with connection {}",
             peer,

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java

@@ -1232,7 +1232,8 @@ private EthProtocolManager createEthManager(
             Optional.empty(),
             syncConfig,
             mock(EthScheduler.class),
-            mock(ForkIdManager.class))) {
+            mock(ForkIdManager.class),
+                new NoOpMetricsSystem())) {
 
       return ethManager;
     }

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTestUtil.java

@@ -105,7 +105,8 @@ public static EthProtocolManager create(
         mergePeerFilter,
         mock(SynchronizerConfiguration.class),
         ethScheduler,
-        new ForkIdManager(blockchain, Collections.emptyList(), Collections.emptyList(), false));
+        new ForkIdManager(blockchain, Collections.emptyList(), Collections.emptyList(), false),
+            new NoOpMetricsSystem());
   }
 
   public static EthProtocolManager create(
@@ -154,7 +155,8 @@ public static EthProtocolManager create(
         Optional.empty(),
         mock(SynchronizerConfiguration.class),
         ethScheduler,
-        forkIdManager);
+        forkIdManager,
+            new NoOpMetricsSystem());
   }
 
   public static EthProtocolManager create(final Blockchain blockchain) {

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactoryTest.java

@@ -267,7 +267,8 @@ private void setupInitialSyncPhase(final boolean hasInitialSyncPhase) {
             Optional.empty(),
             mock(SynchronizerConfiguration.class),
             mock(EthScheduler.class),
-            mock(ForkIdManager.class));
+            mock(ForkIdManager.class),
+                new NoOpMetricsSystem());
   }
 
   @Test

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();
@@ -266,6 +270,7 @@ private PeerDiscoveryController createController(final DiscoveryPeer localNode)
         .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/DiscoveryProtocolLogger.java

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.ethereum.p2p.discovery.internal;
 
+import org.hyperledger.besu.ethereum.forkid.ForkId;
+import org.hyperledger.besu.ethereum.p2p.discovery.DiscoveryPeer;
 import org.hyperledger.besu.ethereum.p2p.peers.Peer;
 import org.hyperledger.besu.metrics.BesuMetricCategory;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
@@ -28,6 +30,7 @@
   private static final Logger LOG = LoggerFactory.getLogger(DiscoveryProtocolLogger.class);
   private final LabelledMetric<Counter> outgoingMessageCounter;
   private final LabelledMetric<Counter> incomingMessageCounter;
+  private final LabelledMetric<Counter> forkIdCounter;
 
   public DiscoveryProtocolLogger(final MetricsSystem metricsSystem) {
     outgoingMessageCounter =
@@ -42,6 +45,11 @@
             "discovery_messages_inbound",
             "Total number of P2P discovery messages received",
             "name");
+    forkIdCounter = metricsSystem.createLabelledCounter(
+            BesuMetricCategory.NETWORK,
+            "discovery_fork_id_counter",
+            "total number of successful, failed fork id checks, as well as fork id not present",
+            "name");
   }
 
   void logSendingPacket(final Peer peer, final Packet packet) {
@@ -64,6 +72,30 @@
         packet);
   }
 
+  void logForkIdSuccess(final Peer peer, final ForkId forkId) {
+    forkIdCounter.labels("SUCC").inc();
+    LOG.trace("ForkId successfully checked for peer {}, fork id {}", peer.getId().slice(0,16), forkId);
+  }
+
+  void logForkIdFailure(final Peer peer, final ForkId forkId) {
+    forkIdCounter.labels("FAIL").inc();
+    LOG.trace("ForkId check failed for peer {}, fork id {}", peer.getId().slice(0,16), forkId);
+  }
+
+  void logForkIdNotSent(final Peer peer) {
+    forkIdCounter.labels("NOSE").inc();
+    LOG.trace("ForkId not sent by peer {}", peer.getId().slice(0,16));
+  }
+
+  void logForkIdNotRequestedt(final Peer peer) {
+    forkIdCounter.labels("NORE").inc();
+  }
+
+  void logForkIdRequestedt(final DiscoveryPeer peer) {
+    forkIdCounter.labels("REQU").inc();
+    LOG.trace("ForkId requested from peer {}", peer.getId().slice(0,16));
+  }
+
   private String shortenPacketType(final Packet packet) {
     switch (packet.getType()) {
       case PING:
@@ -81,4 +113,5 @@
     }
     return null;
   }
+
 }