From 8b05dd47c8c93408ac687fc3bf4980644fb3aabc Mon Sep 17 00:00:00 2001 From: pietjepuk2 <75548277+pietjepuk2@users.noreply.github.com> Date: Thu, 6 Oct 2022 16:40:47 +0200 Subject: [PATCH] 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 * 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 Signed-off-by: Pietje Puk Co-authored-by: Pietje Puk --- .../ethereum/eth/manager/RequestManager.java | 8 ++++---- .../p2p/network/DefaultP2PNetwork.java | 3 ++- .../p2p/network/PeerDenylistManager.java | 18 +++++++++++++++--- .../p2p/network/PeerDenylistManagerTest.java | 14 +++++++++++++- 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/RequestManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/RequestManager.java index cefcdab1ff..17028726c4 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/RequestManager.java +++ b/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( - "Request ID incorrect (BREACH_OF_PROTOCOL), disconnecting peer {}", peer); - peer.disconnect(DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL); + peer.recordUselessResponse("Request ID incorrect"); }); + } else { // otherwise iterate through all of them streams.forEach(stream -> stream.processMessage(ethMessage.getData())); diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java index 44d783fa39..dee16a5b29 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/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 = diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManager.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManager.java index 17d1d11fc6..bf98a0bb3f 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManager.java +++ b/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,8 +51,15 @@ public class PeerDenylistManager implements DisconnectCallback { final DisconnectReason reason, final boolean initiatedByPeer) { if (shouldBlock(reason, initiatedByPeer)) { - LOG.debug("Added peer {} to peer denylist for reason {}", connection, reason.name()); - denylist.add(connection.getPeer()); + 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()); + } } } diff --git a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManagerTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManagerTest.java index 8ebd40e6a7..38435d9d53 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/PeerDenylistManagerTest.java +++ b/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();