diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index 36f57b15686..a5af86551bb 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -197,7 +197,7 @@ private boolean registerDisconnect( disconnectCallbacks.forEach(callback -> callback.onDisconnect(peer)); peer.handleDisconnect(); abortPendingRequestsAssignedToDisconnectedPeers(); - LOG.debug("Disconnected EthPeer {}...", peer.getShortNodeId()); + LOG.debug("Disconnected EthPeer {}", peer.getShortNodeId()); LOG.trace("Disconnected EthPeer {}", peer); } } @@ -391,7 +391,7 @@ public void disconnectWorstUselessPeer() { peer -> { LOG.atDebug() .setMessage( - "disconnecting peer {}... Waiting for better peers. Current {} of max {}") + "disconnecting peer {}. Waiting for better peers. Current {} of max {}") .addArgument(peer::getShortNodeId) .addArgument(this::peerCount) .addArgument(this::getMaxPeers) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputation.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputation.java index ff7617b0f46..e4c263ea783 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputation.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputation.java @@ -34,13 +34,10 @@ public class PeerReputation implements Comparable { static final long USELESS_RESPONSE_WINDOW_IN_MILLIS = TimeUnit.MILLISECONDS.convert(1, TimeUnit.MINUTES); - static final int DEFAULT_MAX_SCORE = 200; - // how much above the initial score you need to be to not get disconnected for timeouts/useless - // responses - private final int hasBeenUsefulThreshold; + static final int DEFAULT_MAX_SCORE = 150; static final int DEFAULT_INITIAL_SCORE = 100; private static final Logger LOG = LoggerFactory.getLogger(PeerReputation.class); - private static final int TIMEOUT_THRESHOLD = 5; + private static final int TIMEOUT_THRESHOLD = 3; private static final int USELESS_RESPONSE_THRESHOLD = 5; private final ConcurrentMap timeoutCountByRequestType = @@ -48,7 +45,8 @@ public class PeerReputation implements Comparable { private final Queue uselessResponseTimes = new ConcurrentLinkedQueue<>(); private static final int SMALL_ADJUSTMENT = 1; - private static final int LARGE_ADJUSTMENT = 5; + private static final int LARGE_ADJUSTMENT = 10; + private int score; private final int maxScore; @@ -61,37 +59,22 @@ public PeerReputation(final int initialScore, final int maxScore) { checkArgument( initialScore <= maxScore, "Initial score must be less than or equal to max score"); this.maxScore = maxScore; - this.hasBeenUsefulThreshold = Math.min(maxScore, initialScore + 10); this.score = initialScore; } public Optional recordRequestTimeout(final int requestCode) { final int newTimeoutCount = getOrCreateTimeoutCount(requestCode).incrementAndGet(); if (newTimeoutCount >= TIMEOUT_THRESHOLD) { - score -= LARGE_ADJUSTMENT; - // don't trigger disconnect if this peer has a sufficiently high reputation score - if (peerHasNotBeenUseful()) { - LOG.debug( - "Disconnection triggered by {} repeated timeouts for requestCode {}, peer score {}", - newTimeoutCount, - requestCode, - score); - return Optional.of(DisconnectReason.TIMEOUT); - } - - LOG.trace( - "Not triggering disconnect for {} repeated timeouts for requestCode {} because peer has high score {}", + LOG.debug( + "Disconnection triggered by {} repeated timeouts for requestCode {}", newTimeoutCount, - requestCode, - score); + requestCode); + score -= LARGE_ADJUSTMENT; + return Optional.of(DisconnectReason.TIMEOUT); } else { score -= SMALL_ADJUSTMENT; + return Optional.empty(); } - return Optional.empty(); - } - - private boolean peerHasNotBeenUseful() { - return score < hasBeenUsefulThreshold; } public void resetTimeoutCount(final int requestCode) { @@ -113,19 +96,12 @@ public Optional recordUselessResponse(final long timestamp) { } if (uselessResponseTimes.size() >= USELESS_RESPONSE_THRESHOLD) { score -= LARGE_ADJUSTMENT; - // don't trigger disconnect if this peer has a sufficiently high reputation score - if (peerHasNotBeenUseful()) { - LOG.debug( - "Disconnection triggered by exceeding useless response threshold, score {}", score); - return Optional.of(DisconnectReason.USELESS_PEER); - } - LOG.trace( - "Not triggering disconnect for exceeding useless response threshold because peer has high score {}", - score); + LOG.debug("Disconnection triggered by exceeding useless response threshold"); + return Optional.of(DisconnectReason.USELESS_PEER); } else { score -= SMALL_ADJUSTMENT; + return Optional.empty(); } - return Optional.empty(); } public void recordUsefulResponse() { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputationTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputationTest.java index f49abee8bb9..9706369a069 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputationTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/PeerReputationTest.java @@ -36,8 +36,6 @@ public void shouldThrowOnInvalidInitialScore() { @Test public void shouldOnlyDisconnectWhenTimeoutLimitReached() { - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).contains(TIMEOUT); @@ -47,11 +45,6 @@ public void shouldOnlyDisconnectWhenTimeoutLimitReached() { public void shouldTrackTimeoutsSeparatelyForDifferentRequestTypes() { assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); - - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); @@ -64,8 +57,6 @@ public void shouldResetTimeoutCountForRequestType() { assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_HEADERS)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); - assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty(); assertThat(reputation.recordRequestTimeout(EthPV62.GET_BLOCK_BODIES)).isEmpty();