revert (#6232)

Signed-off-by: [email protected] <[email protected]>
hyperledger · Dec 5, 2023 · ee810eb · ee810eb
1 parent e3db18e
commit ee810eb
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 48 deletions.
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
@@ -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
@@ -34,21 +34,19 @@
 public class PeerReputation implements Comparable<PeerReputation> {
   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<Integer, AtomicInteger> timeoutCountByRequestType =
       new ConcurrentHashMap<>();
   private final Queue<Long> 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<DisconnectReason> 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<DisconnectReason> 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
@@ -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();