Do not exit ChainDownloader in case of a generic CancellationException (#3319)

* Do not exit ChainDownloader in case of a generic CancellationException

There right way of halting a ChainDownloader is only via the cancel method,
no special action should be taken on a generic CancellationException, that
could be triggered by peer tasks that run in the pipeline, since in that
case the error is transient and the download should restart.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

CHANGELOG.md

@@ -14,6 +14,7 @@
 ### Bug Fixes
 - Fix regression on cors-origin star value
 - Fix for ethFeeHistory accepting hex values for blockCount
+- Fix a sync issue, when the chain downloader incorrectly shutdown when a task in the pipeline is cancelled. [#3319](https://github.com/hyperledger/besu/pull/3319)
 
 ## 22.1.0-RC2
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/PipelineChainDownloader.java

@@ -123,9 +123,7 @@ public class PipelineChainDownloader implements ChainDownloader {
       syncState.disconnectSyncTarget(DisconnectReason.BREACH_OF_PROTOCOL);
     }
 
-    if (!cancelled.get()
-        && syncTargetManager.shouldContinueDownloading()
-        && !(ExceptionUtils.rootCause(error) instanceof CancellationException)) {
+    if (!cancelled.get() && syncTargetManager.shouldContinueDownloading()) {
       logDownloadFailure("Chain download failed. Restarting after short delay.", error);
       // Allowing the normal looping logic to retry after a brief delay.
       return scheduler.scheduleFutureTask(() -> completedFuture(null), PAUSE_AFTER_ERROR_DURATION);
@@ -138,9 +136,9 @@ public class PipelineChainDownloader implements ChainDownloader {
 
   private void logDownloadFailure(final String message, final Throwable error) {
     final Throwable rootCause = ExceptionUtils.rootCause(error);
-    if (rootCause instanceof CancellationException || rootCause instanceof InterruptedException) {
-      LOG.trace(message, error);
-    } else if (rootCause instanceof EthTaskException) {
+    if (rootCause instanceof CancellationException
+        || rootCause instanceof InterruptedException
+        || rootCause instanceof EthTaskException) {
       LOG.debug(message, error);
     } else if (rootCause instanceof InvalidBlockException) {
       LOG.warn(message, error);

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/PipelineChainDownloaderTest.java

@@ -62,11 +62,13 @@ public class PipelineChainDownloaderTest {
   @Mock private SyncState syncState;
   private final BlockHeader commonAncestor = new BlockHeaderTestFixture().buildHeader();
   private SyncTarget syncTarget;
+  private SyncTarget syncTarget2;
   private PipelineChainDownloader chainDownloader;
 
   @Before
   public void setUp() {
     syncTarget = new SyncTarget(peer1, commonAncestor);
+    syncTarget2 = new SyncTarget(peer2, commonAncestor);
     chainDownloader =
         new PipelineChainDownloader(
             syncState,
@@ -171,7 +173,6 @@ public class PipelineChainDownloaderTest {
 
     final CompletableFuture<Void> result = chainDownloader.start();
 
-    final SyncTarget syncTarget2 = new SyncTarget(peer2, commonAncestor);
     verify(syncTargetManager).findSyncTarget(Optional.empty());
 
     // Setup expectation for second time round.
@@ -254,6 +255,40 @@ public class PipelineChainDownloaderTest {
     assertCancelled(result);
   }
 
+  @Test
+  public void shouldRetryIfNotCancelledAndPipelineTaskThrowsException() {
+    when(syncTargetManager.shouldContinueDownloading()).thenReturn(true);
+
+    final CompletableFuture<Void> pipelineFuture1 = new CompletableFuture<>();
+    final CompletableFuture<Void> pipelineFuture2 = new CompletableFuture<>();
+
+    when(syncTargetManager.findSyncTarget(Optional.empty()))
+        .thenReturn(completedFuture(syncTarget))
+        .thenReturn(completedFuture(syncTarget2));
+
+    expectPipelineCreation(syncTarget, downloadPipeline);
+    expectPipelineCreation(syncTarget2, downloadPipeline2);
+
+    when(scheduler.startPipeline(downloadPipeline)).thenReturn(pipelineFuture1);
+    when(scheduler.startPipeline(downloadPipeline2)).thenReturn(pipelineFuture2);
+
+    final CompletableFuture<Void> result = chainDownloader.start();
+
+    // A task in the pipeline is cancelled, causing the pipeline to abort.
+    final Exception taskException =
+        new RuntimeException(
+            "Async operation failed", new ExecutionException(new CancellationException()));
+    pipelineFuture1.completeExceptionally(taskException);
+
+    assertThat(result).isNotDone();
+
+    // Second time round, there are no task errors in the pipeline, and the download can complete.
+    when(syncTargetManager.shouldContinueDownloading()).thenReturn(false);
+    pipelineFuture2.complete(null);
+
+    assertThat(result).isDone();
+  }
+
   @Test
   public void shouldDisconnectSyncTargetOnInvalidBlockException_finishedDownloading_cancelled() {
     testInvalidBlockHandling(true, true);