Introduce a cap to reputation score increase (#4230)

Signed-off-by: Fabio Di Fabio <[email protected]>

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## 22.7.1
 
 ### Additions and Improvements
+- Introduce a cap to reputation score increase [#4230](https://github.com/hyperledger/besu/pull/4230)
 - Add experimental CLI option for `--Xp2p-peer-lower-bound` [#4200](https://github.com/hyperledger/besu/pull/4200)
 
 ### Bug Fixes

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

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.ethereum.eth.manager;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import org.hyperledger.besu.ethereum.p2p.rlpx.wire.messages.DisconnectMessage.DisconnectReason;
 
 import java.util.Map;
@@ -30,21 +32,34 @@
 import org.slf4j.LoggerFactory;
 
 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 = 150;
+  static final int DEFAULT_INITIAL_SCORE = 100;
   private static final Logger LOG = LoggerFactory.getLogger(PeerReputation.class);
   private static final int TIMEOUT_THRESHOLD = 3;
   private static final int USELESS_RESPONSE_THRESHOLD = 5;
-  static final long USELESS_RESPONSE_WINDOW_IN_MILLIS =
-      TimeUnit.MILLISECONDS.convert(1, TimeUnit.MINUTES);
 
   private final ConcurrentMap<Integer, AtomicInteger> timeoutCountByRequestType =
       new ConcurrentHashMap<>();
   private final Queue<Long> uselessResponseTimes = new ConcurrentLinkedQueue<>();
 
-  private static final int DEFAULT_SCORE = 100;
   private static final int SMALL_ADJUSTMENT = 1;
   private static final int LARGE_ADJUSTMENT = 10;
 
-  private long score = DEFAULT_SCORE;
+  private int score;
+
+  private final int maxScore;
+
+  public PeerReputation() {
+    this(DEFAULT_INITIAL_SCORE, DEFAULT_MAX_SCORE);
+  }
+
+  public PeerReputation(final int initialScore, final int maxScore) {
+    checkArgument(initialScore <= maxScore, "Inital score must be less than or equal to max score");
+    this.maxScore = maxScore;
+    this.score = initialScore;
+  }
 
   public Optional<DisconnectReason> recordRequestTimeout(final int requestCode) {
     final int newTimeoutCount = getOrCreateTimeoutCount(requestCode).incrementAndGet();
@@ -86,7 +101,9 @@ public Optional<DisconnectReason> recordUselessResponse(final long timestamp) {
   }
 
   public void recordUsefulResponse() {
-    score += SMALL_ADJUSTMENT;
+    if (score < maxScore) {
+      score = Math.min(maxScore, score + SMALL_ADJUSTMENT);
+    }
   }
 
   private boolean shouldRemove(final Long timestamp, final long currentTimestamp) {
@@ -100,6 +117,10 @@ public String toString() {
 
   @Override
   public int compareTo(final @Nonnull PeerReputation otherReputation) {
-    return Long.compare(this.score, otherReputation.score);
+    return Integer.compare(this.score, otherReputation.score);
+  }
+
+  public int getScore() {
+    return score;
   }
 }

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

@@ -365,7 +365,9 @@ public void compareTo_withDifferentNodeId() {
   @Test
   public void recordUsefullResponse() {
     final EthPeer peer = createPeer();
+    peer.recordUselessResponse("bodies");
     final EthPeer peer2 = createPeer();
+    peer2.recordUselessResponse("bodies");
     peer.recordUsefulResponse();
     assertThat(peer.getReputation().compareTo(peer2.getReputation())).isGreaterThan(0);
   }

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

@@ -24,7 +24,14 @@
 
 public class PeerReputationTest {
 
-  private final PeerReputation reputation = new PeerReputation();
+  private static final int INITIAL_SCORE = 25;
+  private static final int MAX_SCORE = 50;
+  private final PeerReputation reputation = new PeerReputation(INITIAL_SCORE, MAX_SCORE);
+
+  @Test(expected = java.lang.IllegalArgumentException.class)
+  public void shouldThrowOnInvalidInitialScore() {
+    new PeerReputation(2, 1);
+  }
 
   @Test
   public void shouldOnlyDisconnectWhenTimeoutLimitReached() {
@@ -84,6 +91,14 @@ public void shouldDiscardEmptyResponseRecordsAfterTimeWindowElapses() {
   @Test
   public void shouldIncreaseScore() {
     reputation.recordUsefulResponse();
-    assertThat(reputation.compareTo(new PeerReputation())).isGreaterThan(0);
+    assertThat(reputation.getScore()).isGreaterThan(INITIAL_SCORE);
+  }
+
+  @Test
+  public void shouldNotIncreaseScoreOverMax() {
+    for (int i = 0; i <= MAX_SCORE + 1; i++) {
+      reputation.recordUsefulResponse();
+    }
+    assertThat(reputation.getScore()).isEqualTo(MAX_SCORE);
   }
 }