Do not send new payloads to backward sync if initial sync is in progress (#4720)

* Do not involve backward sync until initial sync is done

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

CHANGELOG.md

@@ -21,6 +21,7 @@
 - Remove some log statements that are keeping some objects live in heap for a long time, to reduce the amount of memory required during initial sync [#4705](https://github.com/hyperledger/besu/pull/4705)
 - Add field `type` to Transaction receipt object (eth_getTransactionReceipt) [#4505](https://github.com/hyperledger/besu/issues/4505)
 - Print an overview of configuration and system information at startup [#4451](https://github.com/hyperledger/besu/pull/4451)
+- Do not send new payloads to backward sync if initial sync is in progress [#4720](https://github.com/hyperledger/besu/issues/4720)
 
 ### Bug Fixes
 - Restore updating chain head and finalized block during backward sync [#4718](https://github.com/hyperledger/besu/pull/4718)

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java

@@ -433,15 +433,6 @@ public class MergeCoordinator implements MergeMiningCoordinator, BadChainListene
 
   @Override
   public BlockProcessingResult validateBlock(final Block block) {
-
-    final var chain = protocolContext.getBlockchain();
-    chain
-        .getBlockHeader(block.getHeader().getParentHash())
-        .ifPresentOrElse(
-            blockHeader ->
-                debugLambda(LOG, "Parent of block {} is already present", block::toLogString),
-            () -> backwardSyncContext.syncBackwardsUntil(block));
-
     final var validationResult =
         protocolSchedule
             .getByBlockNumber(block.getHeader().getNumber())

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayload.java

@@ -96,6 +96,11 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
 
     traceLambda(LOG, "blockparam: {}", () -> Json.encodePrettily(blockParam));
 
+    if (mergeContext.get().isSyncing()) {
+      LOG.debug("We are syncing");
+      return respondWith(reqId, blockParam, null, SYNCING);
+    }
+
     final List<Transaction> transactions;
     try {
       transactions =
@@ -182,12 +187,9 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
     final var block =
         new Block(newBlockHeader, new BlockBody(transactions, Collections.emptyList()));
 
-    if (mergeContext.get().isSyncing() || parentHeader.isEmpty()) {
-      LOG.debug(
-          "isSyncing: {} parentHeaderMissing: {}, adding {} to backwardsync",
-          mergeContext.get().isSyncing(),
-          parentHeader.isEmpty(),
-          block.getHash());
+    if (parentHeader.isEmpty()) {
+      debugLambda(
+          LOG, "Parent of block {} is not present, append it to backward sync", block::toLogString);
       mergeCoordinator.appendNewPayloadToSync(block);
 
       return respondWith(reqId, blockParam, null, SYNCING);

ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadTest.java

@@ -351,10 +351,7 @@ public class EngineNewPayloadTest {
   @Test
   public void shouldRespondWithSyncingDuringForwardSync() {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
-    when(blockchain.getBlockByHash(any())).thenReturn(Optional.empty());
     when(mergeContext.isSyncing()).thenReturn(Boolean.TRUE);
-    when(mergeCoordinator.appendNewPayloadToSync(any()))
-        .thenReturn(CompletableFuture.completedFuture(null));
     var resp = resp(mockPayload(mockHeader, Collections.emptyList()));
 
     EnginePayloadStatusResult res = fromSuccessResp(resp);

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContext.java

@@ -118,45 +118,37 @@ public class BackwardSyncContext {
   }
 
   public synchronized CompletableFuture<Void> syncBackwardsUntil(final Hash newBlockHash) {
-    Optional<Status> maybeCurrentStatus = Optional.ofNullable(this.currentBackwardSyncStatus.get());
-    if (isTrusted(newBlockHash)) {
-      return maybeCurrentStatus
-          .map(Status::getCurrentFuture)
-          .orElseGet(() -> CompletableFuture.completedFuture(null));
+    if (!isTrusted(newBlockHash)) {
+      backwardChain.addNewHash(newBlockHash);
     }
-    backwardChain.addNewHash(newBlockHash);
-    return maybeCurrentStatus
-        .map(Status::getCurrentFuture)
-        .orElseGet(
-            () -> {
-              LOG.info("Starting a new backward sync session");
-              Status status = new Status(prepareBackwardSyncFutureWithRetry());
-              this.currentBackwardSyncStatus.set(status);
-              return status.currentFuture;
-            });
+
+    final Status status = getOrStartSyncSession();
+    backwardChain
+        .getBlock(newBlockHash)
+        .ifPresent(
+            newTargetBlock -> status.updateTargetHeight(newTargetBlock.getHeader().getNumber()));
+    return status.currentFuture;
   }
 
   public synchronized CompletableFuture<Void> syncBackwardsUntil(final Block newPivot) {
-    Optional<Status> maybeCurrentStatus = Optional.ofNullable(this.currentBackwardSyncStatus.get());
-    if (isTrusted(newPivot.getHash())) {
-      return maybeCurrentStatus
-          .map(Status::getCurrentFuture)
-          .orElseGet(() -> CompletableFuture.completedFuture(null));
+    if (!isTrusted(newPivot.getHash())) {
+      backwardChain.appendTrustedBlock(newPivot);
     }
-    backwardChain.appendTrustedBlock(newPivot);
-    return maybeCurrentStatus
-        .map(Status::getCurrentFuture)
-        .orElseGet(
-            () -> {
-              LOG.info("Starting a new backward sync session");
-              LOG.info("Backward sync target block is {}", newPivot.toLogString());
-              Status status = new Status(prepareBackwardSyncFutureWithRetry());
-              status.setSyncRange(
-                  getProtocolContext().getBlockchain().getChainHeadBlockNumber(),
-                  newPivot.getHeader().getNumber());
-              this.currentBackwardSyncStatus.set(status);
-              return status.currentFuture;
-            });
+
+    final Status status = getOrStartSyncSession();
+    status.updateTargetHeight(newPivot.getHeader().getNumber());
+    return status.currentFuture;
+  }
+
+  private Status getOrStartSyncSession() {
+    Optional<Status> maybeCurrentStatus = Optional.ofNullable(this.currentBackwardSyncStatus.get());
+    return maybeCurrentStatus.orElseGet(
+        () -> {
+          LOG.info("Starting a new backward sync session");
+          Status newStatus = new Status(prepareBackwardSyncFutureWithRetry());
+          this.currentBackwardSyncStatus.set(newStatus);
+          return newStatus;
+        });
   }
 
   private boolean isTrusted(final Hash hash) {
@@ -418,20 +410,16 @@ public class BackwardSyncContext {
     }
   }
 
-  static class Status {
+  class Status {
     private final CompletableFuture<Void> currentFuture;
+    private final long initialChainHeight;
     private long targetChainHeight;
-    private long initialChainHeight;
 
-    private static long lastLogAt = 0;
+    private long lastLogAt = 0;
 
     public Status(final CompletableFuture<Void> currentFuture) {
       this.currentFuture = currentFuture;
-    }
-
-    public void setSyncRange(final long initialHeight, final long targetHeight) {
-      initialChainHeight = initialHeight;
-      targetChainHeight = targetHeight;
+      this.initialChainHeight = protocolContext.getBlockchain().getChainHeadBlockNumber();
     }
 
     public void updateTargetHeight(final long newTargetHeight) {

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncAlgorithm.java

@@ -63,11 +63,7 @@ public class BackwardsSyncAlgorithm {
               result -> {
                 LOG.info("Backward sync target block is {}", result.toLogString());
                 context.getBackwardChain().removeFromHashToAppend(firstHash.get());
-                context
-                    .getStatus()
-                    .setSyncRange(
-                        context.getProtocolContext().getBlockchain().getChainHeadBlockNumber(),
-                        result.getHeader().getNumber());
+                context.getStatus().updateTargetHeight(result.getHeader().getNumber());
               });
     }
     if (!context.isReady()) {