Skip to content

Commit

Permalink
Fix off-by-one error in AbstractRetryingPeerTask (hyperledger#4254)
Browse files Browse the repository at this point in the history
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
  • Loading branch information
fab-10 authored and freemanzMrojo committed Aug 19, 2022
1 parent 2813ce7 commit 4159147
Show file tree
Hide file tree
Showing 16 changed files with 124 additions and 25 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ protected void executeTask() {
// Return if task is done
return;
}
if (retryCount > maxRetries) {
if (retryCount >= maxRetries) {
result.completeExceptionally(new MaxRetriesReachedException());
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public abstract class RetryingMessageTaskTest<T> extends AbstractMessageTaskTest
protected final int maxRetries;

protected RetryingMessageTaskTest() {
this.maxRetries = 3;
this.maxRetries = 4;
}

@Override
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public void setUp() {
blockchainSetupUtil.getWorldArchive(),
transactionPool,
EthProtocolConfiguration.defaultConfig());
pivotBlockConfirmer = createPivotBlockConfirmer(3, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(3, 2);
}

private PivotBlockConfirmer createPivotBlockConfirmer(
Expand All @@ -108,7 +108,7 @@ private PivotBlockConfirmer createPivotBlockConfirmer(

@Test
public void completeSuccessfully() {
pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);

final Responder responder =
RespondingEthPeer.blockchainResponder(
Expand Down Expand Up @@ -137,7 +137,7 @@ public void completeSuccessfully() {

@Test
public void delayedResponse() {
pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);

final Responder responder =
RespondingEthPeer.blockchainResponder(
Expand Down Expand Up @@ -170,7 +170,7 @@ public void delayedResponse() {

@Test
public void peerTimesOutThenIsUnresponsive() {
pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);

final Responder responder =
RespondingEthPeer.blockchainResponder(
Expand Down Expand Up @@ -210,7 +210,7 @@ public void peerTimesOutThenIsUnresponsive() {

@Test
public void peerTimesOut() {
pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);

final Responder responder =
RespondingEthPeer.blockchainResponder(
Expand Down Expand Up @@ -250,7 +250,7 @@ public void peerTimesOut() {

@Test
public void peerUnresponsive() {
pivotBlockConfirmer = createPivotBlockConfirmer(2, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(2, 2);

final Responder responder =
RespondingEthPeer.blockchainResponder(
Expand Down Expand Up @@ -292,7 +292,7 @@ public void peerUnresponsive() {

@Test
public void headerMismatch() {
pivotBlockConfirmer = createPivotBlockConfirmer(3, 1);
pivotBlockConfirmer = createPivotBlockConfirmer(3, 2);

final Responder responderA =
RespondingEthPeer.blockchainResponder(
Expand Down

0 comments on commit 4159147

Please sign in to comment.