Refactor BackwardsSyncAlgorithm.pickNextStep for readability (#6775)

* Refactor BackwardsSyncAlgorithm.pickNextStep for readability ...mostly extracting methods - Result null check - Remove redundant BannedMethod suppressions - Add sonar config back in that was removed by mistake Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
hyperledger · Mar 25, 2024 · bb9ba13 · bb9ba13
1 parent 2eca4d5
commit bb9ba13
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 36 deletions.
diff --git a/.../main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgorithm.java b/.../main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgorithm.java
@@ -63,40 +63,7 @@ public CompletableFuture<Void> executeBackwardsSync(final Void unused) {
   public CompletableFuture<Void> pickNextStep() {
     final Optional<Hash> firstHash = context.getBackwardChain().getFirstHashToAppend();
     if (firstHash.isPresent()) {
-      final CompletableFuture<Void> syncStep = new CompletableFuture<>();
-      executeSyncStep(firstHash.get())
-          .whenComplete(
-              (result, error) -> {
-                if (error != null) {
-                  if (error instanceof CompletionException
-                      && error.getCause() instanceof MaxRetriesReachedException) {
-                    context.getBackwardChain().removeFromHashToAppend(firstHash.get());
-                    LOG.atWarn()
-                        .setMessage(
-                            "Unable to retrieve block {} from any peer, with {} peers available. Could be a reorged block. Waiting for the next block from the consensus client to try again.")
-                        .addArgument(firstHash.get())
-                        .addArgument(context.getEthContext().getEthPeers().peerCount())
-                        .addArgument(context.getBackwardChain().getFirstHashToAppend())
-                        .log();
-                    LOG.atDebug()
-                        .setMessage("Removing hash {} from hashesToAppend")
-                        .addArgument(firstHash.get())
-                        .log();
-                    syncStep.complete(null);
-                  } else {
-                    syncStep.completeExceptionally(error);
-                  }
-                } else {
-                  LOG.atDebug()
-                      .setMessage("Backward sync target block is {}")
-                      .addArgument(result::toLogString)
-                      .log();
-                  context.getBackwardChain().removeFromHashToAppend(firstHash.get());
-                  context.getStatus().updateTargetHeight(result.getHeader().getNumber());
-                  syncStep.complete(null);
-                }
-              });
-      return syncStep;
+      return handleSyncStep(firstHash.get());
     }
     if (!context.isReady()) {
       return waitForReady();
@@ -137,6 +104,59 @@ public CompletableFuture<Void> pickNextStep() {
     return executeBackwardAsync(firstAncestorHeader);
   }
 
+  private CompletableFuture<Void> handleSyncStep(final Hash firstHash) {
+    final CompletableFuture<Void> syncStep = new CompletableFuture<>();
+    executeSyncStep(firstHash)
+        .whenComplete(
+            (result, error) -> {
+              if (error != null) {
+                handleSyncStepError(error, firstHash, syncStep);
+              } else {
+                handleSyncStepSuccess(result, firstHash, syncStep);
+              }
+            });
+    return syncStep;
+  }
+
+  private void handleSyncStepSuccess(
+      final Block result, final Hash firstHash, final CompletableFuture<Void> syncStep) {
+    if (result == null) {
+      LOG.atWarn().setMessage("Unexpected null result in for hash {}").addArgument(firstHash).log();
+      syncStep.completeExceptionally(new BackwardSyncException("Unexpected null result", true));
+    } else {
+      LOG.atDebug()
+          .setMessage("Backward sync target block is {}")
+          .addArgument(result::toLogString)
+          .log();
+      context.getBackwardChain().removeFromHashToAppend(firstHash);
+      context.getStatus().updateTargetHeight(result.getHeader().getNumber());
+      syncStep.complete(null);
+    }
+  }
+
+  private void handleSyncStepError(
+      final Throwable error, final Hash firstHash, final CompletableFuture<Void> syncStep) {
+    if (error instanceof CompletionException
+        && error.getCause() instanceof MaxRetriesReachedException) {
+      handleEthPeerMaxRetriesException(firstHash);
+      syncStep.complete(null);
+    } else {
+      syncStep.completeExceptionally(error);
+    }
+  }
+
+  private void handleEthPeerMaxRetriesException(final Hash firstHash) {
+    context.getBackwardChain().removeFromHashToAppend(firstHash);
+    LOG.atWarn()
+        .setMessage(
+            "Unable to retrieve block {} from any peer, with {} peers available. Could be a reorged block. Waiting for the next block from the consensus client to try again.")
+        .addArgument(firstHash)
+        .addArgument(context.getEthContext().getEthPeers().peerCount())
+        .addArgument(context.getBackwardChain().getFirstHashToAppend())
+        .log();
+    LOG.atDebug().setMessage("Removing hash {} from hashesToAppend").addArgument(firstHash).log();
+  }
+
   @VisibleForTesting
   public CompletableFuture<Void> executeProcessKnownAncestors() {
     return new ProcessKnownAncestorsStep(context, context.getBackwardChain()).executeAsync();

diff --git a/...est/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java b/...est/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java
@@ -442,7 +442,6 @@ public void shouldFailAfterMaxNumberOfRetries() {
     }
   }
 
-  @SuppressWarnings("BannedMethod")
   @Test
   public void whenBlockNotFoundInPeers_shouldRemoveBlockFromQueueAndProgressInNextSession() {
     // This scenario can happen due to a reorg
@@ -466,7 +465,6 @@ public void whenBlockNotFoundInPeers_shouldRemoveBlockFromQueueAndProgressInNext
         .isEqualTo(remoteBlockchain.getBlockByNumber(reorgBlockHeight).orElseThrow());
   }
 
-  @SuppressWarnings("BannedMethod")
   @Test
   public void
       whenBlockNotFoundInPeers_shouldRemoveBlockFromQueueAndProgressWithQueueInSameSession() {

diff --git a/gradle.properties b/gradle.properties
@@ -13,3 +13,4 @@ org.gradle.jvmargs=-Xmx4g \
 --add-opens jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
 --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
 # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134
+systemProp.sonar.gradle.skipCompile=true