Fix off-by-one error in AbstractRetryingPeerTask

CHANGELOG.md

@@ -9,7 +9,7 @@
 
 ### Bug Fixes
 - Fixes off-by-one error for mainnet TTD fallback [#4223](https://github.com/hyperledger/besu/pull/4223)
-
+- Fix off-by-one error in AbstractRetryingPeerTask [#4254](https://github.com/hyperledger/besu/pull/4254)
 
 ## 22.7.0
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetAccountRangeFromPeerTask.java

@@ -43,7 +43,7 @@ private RetryingGetAccountRangeFromPeerTask(
       final BlockHeader blockHeader,
       final MetricsSystem metricsSystem) {
     super(
-        ethContext, 3, data -> data.accounts().isEmpty() && data.proofs().isEmpty(), metricsSystem);
+        ethContext, 4, data -> data.accounts().isEmpty() && data.proofs().isEmpty(), metricsSystem);
     this.ethContext = ethContext;
     this.startKeyHash = startKeyHash;
     this.endKeyHash = endKeyHash;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetBytecodeFromPeerTask.java

@@ -41,7 +41,7 @@ private RetryingGetBytecodeFromPeerTask(
       final List<Bytes32> codeHashes,
       final BlockHeader blockHeader,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, Map::isEmpty, metricsSystem);
+    super(ethContext, 4, Map::isEmpty, metricsSystem);
     this.ethContext = ethContext;
     this.codeHashes = codeHashes;
     this.blockHeader = blockHeader;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetStorageRangeFromPeerTask.java

@@ -45,7 +45,7 @@ private RetryingGetStorageRangeFromPeerTask(
       final Bytes32 endKeyHash,
       final BlockHeader blockHeader,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, data -> data.proofs().isEmpty() && data.slots().isEmpty(), metricsSystem);
+    super(ethContext, 4, data -> data.proofs().isEmpty() && data.slots().isEmpty(), metricsSystem);
     this.ethContext = ethContext;
     this.accountHashes = accountHashes;
     this.startKeyHash = startKeyHash;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetTrieNodeFromPeerTask.java

@@ -40,7 +40,7 @@ private RetryingGetTrieNodeFromPeerTask(
       final Map<Bytes, List<Bytes>> paths,
       final BlockHeader blockHeader,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, Map::isEmpty, metricsSystem);
+    super(ethContext, 4, Map::isEmpty, metricsSystem);
     this.ethContext = ethContext;
     this.paths = paths;
     this.blockHeader = blockHeader;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingPeerTask.java

@@ -81,7 +81,7 @@ protected void executeTask() {
       // Return if task is done
       return;
     }
-    if (retryCount > maxRetries) {
+    if (retryCount >= maxRetries) {
       result.completeExceptionally(new MaxRetriesReachedException());
       return;
     }

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetHeadersEndingAtFromPeerByHashTask.java

@@ -44,7 +44,7 @@ public class RetryingGetHeadersEndingAtFromPeerByHashTask
       final Hash referenceHash,
       final int count,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, List::isEmpty, metricsSystem);
+    super(ethContext, 4, List::isEmpty, metricsSystem);
     this.protocolSchedule = protocolSchedule;
     this.count = count;
     checkNotNull(referenceHash);

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetNodeDataFromPeerTask.java

@@ -40,7 +40,7 @@ private RetryingGetNodeDataFromPeerTask(
       final Collection<Hash> hashes,
       final long pivotBlockNumber,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, data -> false, metricsSystem);
+    super(ethContext, 4, data -> false, metricsSystem);
     this.ethContext = ethContext;
     this.hashes = new HashSet<>(hashes);
     this.pivotBlockNumber = pivotBlockNumber;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotBlockRetriever.java

@@ -39,7 +39,7 @@
 public class PivotBlockRetriever {
 
   private static final Logger LOG = LoggerFactory.getLogger(PivotBlockRetriever.class);
-  public static final int MAX_QUERY_RETRIES_PER_PEER = 3;
+  public static final int MAX_QUERY_RETRIES_PER_PEER = 4;
   private static final int DEFAULT_MAX_PIVOT_BLOCK_RESETS = 250;
   private static final int SUSPICIOUS_NUMBER_OF_RETRIES = 5;
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksTask.java

@@ -49,7 +49,7 @@ public class CompleteBlocksTask extends AbstractRetryingPeerTask<List<Block>> {
   private static final Logger LOG = LoggerFactory.getLogger(CompleteBlocksTask.class);
 
   private static final int MIN_SIZE_INCOMPLETE_LIST = 1;
-  private static final int DEFAULT_RETRIES = 3;
+  private static final int DEFAULT_RETRIES = 4;
 
   private final EthContext ethContext;
   private final ProtocolSchedule protocolSchedule;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/DownloadHeaderSequenceTask.java

@@ -54,7 +54,7 @@
  */
 public class DownloadHeaderSequenceTask extends AbstractRetryingPeerTask<List<BlockHeader>> {
   private static final Logger LOG = LoggerFactory.getLogger(DownloadHeaderSequenceTask.class);
-  private static final int DEFAULT_RETRIES = 3;
+  private static final int DEFAULT_RETRIES = 4;
 
   private final EthContext ethContext;
   private final ProtocolContext protocolContext;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/GetReceiptsForHeadersTask.java

@@ -42,7 +42,7 @@
 public class GetReceiptsForHeadersTask
     extends AbstractRetryingPeerTask<Map<BlockHeader, List<TransactionReceipt>>> {
   private static final Logger LOG = LoggerFactory.getLogger(GetReceiptsForHeadersTask.class);
-  private static final int DEFAULT_RETRIES = 3;
+  private static final int DEFAULT_RETRIES = 4;
 
   private final EthContext ethContext;
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/RetryingGetHeaderFromPeerByHashTask.java

@@ -44,7 +44,7 @@ public class RetryingGetHeaderFromPeerByHashTask
       final EthContext ethContext,
       final Hash referenceHash,
       final MetricsSystem metricsSystem) {
-    super(ethContext, 3, List::isEmpty, metricsSystem);
+    super(ethContext, 4, List::isEmpty, metricsSystem);
     this.protocolSchedule = protocolSchedule;
     checkNotNull(referenceHash);
     this.referenceHash = referenceHash;

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/ethtaskutils/RetryingMessageTaskTest.java

@@ -38,7 +38,7 @@ public abstract class RetryingMessageTaskTest<T> extends AbstractMessageTaskTest
   protected final int maxRetries;
 
   protected RetryingMessageTaskTest() {
-    this.maxRetries = 3;
+    this.maxRetries = 4;
   }
 
   @Override
@@ -76,8 +76,8 @@ public void failsWhenPeerReturnsPartialResultThenStops() {
     respondingPeer.respond(partialResponder);
     assertThat(future.isDone()).isFalse();
 
-    // Respond max times with no data
-    respondingPeer.respondTimes(emptyResponder, maxRetries);
+    // Respond max times - 1 with no data
+    respondingPeer.respondTimes(emptyResponder, maxRetries - 1);
     assertThat(future).isNotDone();
 
     // Next retry should fail
@@ -205,8 +205,8 @@ public void failsWhenPeersSendEmptyResponses() {
 
     assertThat(future.isDone()).isFalse();
 
-    // Respond max times
-    respondingPeer.respondTimes(responder, maxRetries);
+    // Respond max times - 1
+    respondingPeer.respondTimes(responder, maxRetries - 1);
     assertThat(future).isNotDone();
 
     // Next retry should fail

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingPeerTaskTest.java

@@ -0,0 +1,99 @@
+/*
+ * Copyright Hyperledger Besu Contributors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.ethereum.eth.manager.task;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
+
+import org.hyperledger.besu.ethereum.eth.manager.EthContext;
+import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
+import org.hyperledger.besu.ethereum.eth.manager.exceptions.MaxRetriesReachedException;
+import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem;
+import org.hyperledger.besu.plugin.services.MetricsSystem;
+
+import java.util.Objects;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AbstractRetryingPeerTaskTest {
+
+  @Mock EthContext ethContext;
+  MetricsSystem metricsSystem = new NoOpMetricsSystem();
+
+  @Test
+  public void shouldSuccessAtFirstTryIfNoTaskFailures()
+      throws InterruptedException, ExecutionException {
+    final int maxRetries = 2;
+    TaskThatFailsSometimes task = new TaskThatFailsSometimes(0, maxRetries);
+    CompletableFuture<Boolean> result = task.run();
+    assertThat(result.get()).isTrue();
+    assertThat(task.executions).isEqualTo(1);
+  }
+
+  @Test
+  public void shouldSuccessIfTaskFailOnlyOnce() throws InterruptedException, ExecutionException {
+    final int maxRetries = 2;
+    TaskThatFailsSometimes task = new TaskThatFailsSometimes(1, maxRetries);
+    CompletableFuture<Boolean> result = task.run();
+    assertThat(result.get()).isTrue();
+    assertThat(task.executions).isEqualTo(2);
+  }
+
+  @Test
+  public void shouldFailAfterMaxRetriesExecutions() throws InterruptedException {
+    final int maxRetries = 2;
+    TaskThatFailsSometimes task = new TaskThatFailsSometimes(maxRetries, maxRetries);
+    CompletableFuture<Boolean> result = task.run();
+    assertThat(result.isCompletedExceptionally()).isTrue();
+    assertThat(task.executions).isEqualTo(maxRetries);
+    try {
+      result.get();
+    } catch (ExecutionException ee) {
+      assertThat(ee).hasCauseExactlyInstanceOf(MaxRetriesReachedException.class);
+      return;
+    }
+    failBecauseExceptionWasNotThrown(MaxRetriesReachedException.class);
+  }
+
+  private class TaskThatFailsSometimes extends AbstractRetryingPeerTask<Boolean> {
+    final int initialFailures;
+    int executions = 0;
+    int failures = 0;
+
+    protected TaskThatFailsSometimes(final int initialFailures, final int maxRetries) {
+      super(ethContext, maxRetries, Objects::isNull, metricsSystem);
+      this.initialFailures = initialFailures;
+    }
+
+    @Override
+    protected CompletableFuture<Boolean> executePeerTask(final Optional<EthPeer> assignedPeer) {
+      executions++;
+      if (failures < initialFailures) {
+        failures++;
+        return CompletableFuture.completedFuture(null);
+      } else {
+        result.complete(Boolean.TRUE);
+        return CompletableFuture.completedFuture(Boolean.TRUE);
+      }
+    }
+  }
+}

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotBlockConfirmerTest.java

@@ -90,7 +90,7 @@ public void setUp() {
             blockchainSetupUtil.getWorldArchive(),
             transactionPool,
             EthProtocolConfiguration.defaultConfig());
-    pivotBlockConfirmer = createPivotBlockConfirmer(3, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(3, 2);
   }
 
   private PivotBlockConfirmer createPivotBlockConfirmer(
@@ -108,7 +108,7 @@ private PivotBlockConfirmer createPivotBlockConfirmer(
 
   @Test
   public void completeSuccessfully() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);
 
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
@@ -137,7 +137,7 @@ public void completeSuccessfully() {
 
   @Test
   public void delayedResponse() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);
 
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
@@ -170,7 +170,7 @@ public void delayedResponse() {
 
   @Test
   public void peerTimesOutThenIsUnresponsive() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);
 
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
@@ -210,7 +210,7 @@ public void peerTimesOutThenIsUnresponsive() {
 
   @Test
   public void peerTimesOut() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);
 
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
@@ -250,7 +250,7 @@ public void peerTimesOut() {
 
   @Test
   public void peerUnresponsive() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);
 
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
@@ -292,7 +292,7 @@ public void peerUnresponsive() {
 
   @Test
   public void headerMismatch() {
-    pivotBlockConfirmer = createPivotBlockConfirmer(3, 1);
+    pivotBlockConfirmer = createPivotBlockConfirmer(3, 2);
 
     final Responder responderA =
         RespondingEthPeer.blockchainResponder(