Fix banning (static peers) because of late response in eth/66 (#4377)

* RequestManager: Lessen penalty for late responses with request ID

If the communication with a peer uses the eth/66 protocol, requests have
an accompanying request ID. If the response to any of these requests was
late, we would disconnect and ban the peer mmediately. This is too
excessive a punishment.

A late response is typically already punished with a timeout before (too
many of which and we disconnect). This commit changes the immediate
banning of the peer to just considering the late response useless
(again, too many of which and we disconnect).

Note that in eth/65, the lack of a request ID would mean we would just
consider the late response to be useless (or process it anyway). This
commit therefore brings the punishment of late responses more in line
with what it used to be before request IDs were introduced.

Closes #4320

Signed-off-by: Pietje Puk <pietje@pietjepuk.net>

* PeerDenylistManager: Do not ban static/maintained peers

These types of peers are added manually by the user, and have a certain
trusted status. We should therefore not ban them.

Note that we will still disconnect from these peers when they exhibit
undesirable behavior (e.g. repeated timeouts). We will however continue
to reconnect to them.

Signed-off-by: Pietje Puk <pietje@pietjepuk.net>

Signed-off-by: Pietje Puk <pietje@pietjepuk.net>
Co-authored-by: Pietje Puk <pietje@pietjepuk.net>

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

@@ -79,12 +79,12 @@ public class RequestManager {
         Optional.ofNullable(responseStreams.get(requestIdAndEthMessage.getKey()))
             .ifPresentOrElse(
                 responseStream -> responseStream.processMessage(requestIdAndEthMessage.getValue()),
-                // disconnect on incorrect requestIds
+                // Consider incorrect requestIds to be a useless response; too
+                // many of these and we will disconnect.
                 () -> {
-                  LOG.debug(
+                  peer.recordUselessResponse("Request ID incorrect");
-                      "Request ID incorrect (BREACH_OF_PROTOCOL), disconnecting peer {}", peer);
-                  peer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL);
                 });
+
       } else {
         // otherwise iterate through all of them
         streams.forEach(stream -> stream.processMessage(ethMessage.getData()));

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

@@ -494,7 +494,8 @@ public class DefaultP2PNetwork implements P2PNetwork {
       // Set up permissions
       // Fold peer reputation into permissions
       final PeerPermissionsDenylist misbehavingPeers = PeerPermissionsDenylist.create(500);
-      final PeerDenylistManager reputationManager = new PeerDenylistManager(misbehavingPeers);
+      final PeerDenylistManager reputationManager =
+          new PeerDenylistManager(misbehavingPeers, maintainedPeers);
       peerPermissions = PeerPermissions.combine(peerPermissions, misbehavingPeers);
 
       final MutableLocalNode localNode =

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

@@ -14,6 +14,7 @@
  */
 package org.hyperledger.besu.ethereum.p2p.network;
 
+import org.hyperledger.besu.ethereum.p2p.peers.MaintainedPeers;
 import org.hyperledger.besu.ethereum.p2p.permissions.PeerPermissionsDenylist;
 import org.hyperledger.besu.ethereum.p2p.rlpx.DisconnectCallback;
 import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection;
@@ -36,8 +37,12 @@ public class PeerDenylistManager implements DisconnectCallback {
 
   private final PeerPermissionsDenylist denylist;
 
-  public PeerDenylistManager(final PeerPermissionsDenylist denylist) {
+  private final MaintainedPeers maintainedPeers;
+
+  public PeerDenylistManager(
+      final PeerPermissionsDenylist denylist, final MaintainedPeers maintainedPeers) {
     this.denylist = denylist;
+    this.maintainedPeers = maintainedPeers;
   }
 
   @Override
@@ -46,10 +51,17 @@ public class PeerDenylistManager implements DisconnectCallback {
       final DisconnectReason reason,
       final boolean initiatedByPeer) {
     if (shouldBlock(reason, initiatedByPeer)) {
+      if (maintainedPeers.contains(connection.getPeer())) {
+        LOG.debug(
+            "Skip adding maintained peer {} to peer denylist for reason {}",
+            connection,
+            reason.name());
+      } else {
         LOG.debug("Added peer {} to peer denylist for reason {}", connection, reason.name());
         denylist.add(connection.getPeer());
       }
     }
+  }
 
   private boolean shouldBlock(final DisconnectReason reason, final boolean initiatedByPeer) {
     return (!initiatedByPeer && locallyTriggeredDisconnectReasons.contains(reason))

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManagerTest.java

@@ -20,6 +20,7 @@ import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.ethereum.p2p.peers.DefaultPeer;
 import org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl;
+import org.hyperledger.besu.ethereum.p2p.peers.MaintainedPeers;
 import org.hyperledger.besu.ethereum.p2p.peers.Peer;
 import org.hyperledger.besu.ethereum.p2p.permissions.PeerPermissions;
 import org.hyperledger.besu.ethereum.p2p.permissions.PeerPermissionsDenylist;
@@ -34,10 +35,11 @@ public class PeerDenylistManagerTest {
   private final Peer localNode = generatePeer();
   private final PeerDenylistManager peerDenylistManager;
   private final PeerPermissionsDenylist denylist;
+  private final MaintainedPeers maintainedPeers = new MaintainedPeers();
 
   public PeerDenylistManagerTest() {
     denylist = PeerPermissionsDenylist.create();
-    peerDenylistManager = new PeerDenylistManager(denylist);
+    peerDenylistManager = new PeerDenylistManager(denylist, maintainedPeers);
   }
 
   @Test
@@ -69,6 +71,16 @@ public class PeerDenylistManagerTest {
     checkPermissions(denylist, peer.getPeer(), true);
   }
 
+  @Test
+  public void doesNotDenylistMaintainedPeer() {
+    final PeerConnection peer = generatePeerConnection();
+    maintainedPeers.add(peer.getPeer());
+
+    checkPermissions(denylist, peer.getPeer(), true);
+    peerDenylistManager.onDisconnect(peer, DisconnectReason.BREACH_OF_PROTOCOL, false);
+    checkPermissions(denylist, peer.getPeer(), true);
+  }
+
   @Test
   public void denylistIncompatiblePeer() {
     final PeerConnection peer = generatePeerConnection();