Fix off-by-one error in AbstractRetryingPeerTask (#4254)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
pull/4280/head
Fabio Di Fabio 2 years ago committed by GitHub
parent d2968509ba
commit e0d4da2917
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 2
      CHANGELOG.md
  2. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetAccountRangeFromPeerTask.java
  3. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetBytecodeFromPeerTask.java
  4. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetStorageRangeFromPeerTask.java
  5. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/RetryingGetTrieNodeFromPeerTask.java
  6. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingPeerTask.java
  7. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetHeadersEndingAtFromPeerByHashTask.java
  8. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetNodeDataFromPeerTask.java
  9. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotBlockRetriever.java
  10. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/CompleteBlocksTask.java
  11. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/DownloadHeaderSequenceTask.java
  12. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/GetReceiptsForHeadersTask.java
  13. 2
      ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/RetryingGetHeaderFromPeerByHashTask.java
  14. 10
      ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/ethtaskutils/RetryingMessageTaskTest.java
  15. 99
      ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingPeerTaskTest.java
  16. 14
      ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotBlockConfirmerTest.java

@ -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

@ -43,7 +43,7 @@ public class 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;

@ -41,7 +41,7 @@ public class RetryingGetBytecodeFromPeerTask extends AbstractRetryingPeerTask<Ma
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;

@ -45,7 +45,7 @@ public class 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;

@ -40,7 +40,7 @@ public class RetryingGetTrieNodeFromPeerTask extends AbstractRetryingPeerTask<Ma
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;

@ -81,7 +81,7 @@ public abstract class AbstractRetryingPeerTask<T> extends AbstractEthTask<T> {
// Return if task is done
return;
}
if (retryCount > maxRetries) {
if (retryCount >= maxRetries) {
result.completeExceptionally(new MaxRetriesReachedException());
return;
}

@ -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);

@ -40,7 +40,7 @@ public class RetryingGetNodeDataFromPeerTask extends AbstractRetryingPeerTask<Ma
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;

@ -39,7 +39,7 @@ import org.slf4j.LoggerFactory;
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;

@ -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;

@ -54,7 +54,7 @@ import org.slf4j.LoggerFactory;
*/
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;

@ -42,7 +42,7 @@ import org.slf4j.LoggerFactory;
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;

@ -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;

@ -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 abstract class RetryingMessageTaskTest<T> extends AbstractMessageTaskTest
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 abstract class RetryingMessageTaskTest<T> extends AbstractMessageTaskTest
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

@ -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);
}
}
}
}

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

Loading…
Cancel
Save