Restore updating chain head and finalized block during backward sync

CHANGELOG.md

@@ -23,6 +23,7 @@
 - Print an overview of configuration and system information at startup [#4451](https://github.com/hyperledger/besu/pull/4451)
 
 ### Bug Fixes
+- Restore updating chain head and finalized block during backward sync [#4718](https://github.com/hyperledger/besu/pull/4718)
 
 ### Download Links
 
@@ -76,7 +77,6 @@
 https://hyperledger.jfrog.io/hyperledger/besu-binaries/besu/22.10.0/besu-22.10.0.tar.gz  / sha256: 88fb5df567e4ec3547d7d2970cfef00debbd020c0da66b19166d43779b3b2b85
 https://hyperledger.jfrog.io/hyperledger/besu-binaries/besu/22.10.0/besu-22.10.0.zip / sha256: c8e39f7c879409cb9b47f4d3de5e9c521249083830a8c9a45e8a14a319fe195d
 
-
 ## 22.10.0-RC2
 
 ### Breaking Changes

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

@@ -390,33 +390,45 @@ private boolean canRetryBlockCreation(final Throwable throwable) {
   }
 
   @Override
-  public Optional<BlockHeader> getOrSyncHeaderByHash(final Hash blockHash) {
+  public Optional<BlockHeader> getOrSyncHeadByHash(final Hash headHash, final Hash finalizedHash) {
     final var chain = protocolContext.getBlockchain();
-    final var optHeader = chain.getBlockHeader(blockHash);
+    final var maybeHeadHeader = chain.getBlockHeader(headHash);
 
-    if (optHeader.isPresent()) {
-      debugLambda(LOG, "BlockHeader {} is already present", () -> optHeader.get().toLogString());
+    if (maybeHeadHeader.isPresent()) {
+      debugLambda(LOG, "BlockHeader {} is already present", maybeHeadHeader.get()::toLogString);
     } else {
-      debugLambda(LOG, "appending block hash {} to backward sync", blockHash::toHexString);
-      backwardSyncContext.syncBackwardsUntil(blockHash);
+      debugLambda(LOG, "Appending new head block hash {} to backward sync", headHash::toHexString);
+      backwardSyncContext.updateHead(headHash);
+      backwardSyncContext
+          .syncBackwardsUntil(headHash)
+          .thenRun(() -> updateFinalized(finalizedHash));
     }
-    return optHeader;
+    return maybeHeadHeader;
   }
 
-  @Override
-  public Optional<BlockHeader> getOrSyncHeaderByHash(
-      final Hash blockHash, final Hash finalizedBlockHash) {
-    final var chain = protocolContext.getBlockchain();
-    final var optHeader = chain.getBlockHeader(blockHash);
-
-    if (optHeader.isPresent()) {
-      debugLambda(LOG, "BlockHeader {} is already present", () -> optHeader.get().toLogString());
-    } else {
-      debugLambda(LOG, "appending block hash {} to backward sync", blockHash::toHexString);
-      backwardSyncContext.updateHeads(blockHash, finalizedBlockHash);
-      backwardSyncContext.syncBackwardsUntil(blockHash);
+  private void updateFinalized(final Hash finalizedHash) {
+    if (mergeContext
+        .getFinalized()
+        .map(BlockHeader::getHash)
+        .map(finalizedHash::equals)
+        .orElse(Boolean.FALSE)) {
+      LOG.debug("Finalized block already set to {}, nothing to do", finalizedHash);
+      return;
     }
-    return optHeader;
+
+    protocolContext
+        .getBlockchain()
+        .getBlockHeader(finalizedHash)
+        .ifPresentOrElse(
+            finalizedHeader -> {
+              debugLambda(
+                  LOG, "Setting finalized block header to {}", finalizedHeader::toLogString);
+              mergeContext.setFinalized(finalizedHeader);
+            },
+            () ->
+                LOG.warn(
+                    "Internal error, backward sync completed but failed to import finalized block {}",
+                    finalizedHash));
   }
 
   @Override
@@ -693,12 +705,11 @@ private Optional<Hash> findValidAncestor(
 
   @Override
   public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) {
-    LOG.debug(
-        "checking if block {}:{} is ancestor of {}:{}",
-        ancestorBlock.getNumber(),
-        ancestorBlock.getBlockHash(),
-        newBlock.getNumber(),
-        newBlock.getBlockHash());
+    debugLambda(
+        LOG,
+        "checking if block {} is ancestor of {}",
+        ancestorBlock::toLogString,
+        newBlock::toLogString);
 
     // start with self, because descending from yourself is valid
     Optional<BlockHeader> parentOf = Optional.of(newBlock);
@@ -714,10 +725,11 @@ public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader
         && ancestorBlock.getBlockHash().equals(parentOf.get().getBlockHash())) {
       return true;
     } else {
-      LOG.debug(
+      debugLambda(
+          LOG,
           "looped all the way back, did not find ancestor {} of child {}",
-          ancestorBlock.getBlockHash(),
-          newBlock.getBlockHash());
+          ancestorBlock::toLogString,
+          newBlock::toLogString);
       return false;
     }
   }

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

@@ -63,12 +63,10 @@ ForkchoiceResult updateForkChoice(
 
   CompletableFuture<Void> appendNewPayloadToSync(Block newPayload);
 
-  Optional<BlockHeader> getOrSyncHeaderByHash(Hash blockHash);
+  Optional<BlockHeader> getOrSyncHeadByHash(Hash headHash, Hash finalizedHash);
 
   boolean isMiningBeforeMerge();
 
-  Optional<BlockHeader> getOrSyncHeaderByHash(Hash blockHash, Hash finalizedBlockHash);
-
   void addBadBlock(final Block block, Optional<Throwable> maybeCause);
 
   boolean isBadBlock(Hash blockHash);

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

@@ -183,14 +183,8 @@ public CompletableFuture<Void> appendNewPayloadToSync(final Block newPayload) {
   }
 
   @Override
-  public Optional<BlockHeader> getOrSyncHeaderByHash(final Hash blockHash) {
-    return mergeCoordinator.getOrSyncHeaderByHash(blockHash);
-  }
-
-  @Override
-  public Optional<BlockHeader> getOrSyncHeaderByHash(
-      final Hash blockHash, final Hash finalizedBlockHash) {
-    return mergeCoordinator.getOrSyncHeaderByHash(blockHash, finalizedBlockHash);
+  public Optional<BlockHeader> getOrSyncHeadByHash(final Hash headHash, final Hash finalizedHash) {
+    return mergeCoordinator.getOrSyncHeadByHash(headHash, finalizedHash);
   }
 
   @Override

consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java

@@ -698,7 +698,7 @@ public void assertGetOrSyncForBlockAlreadyPresent() {
     BlockHeader mockHeader =
         headerGenerator.parentHash(Hash.fromHexStringLenient("0xdead")).buildHeader();
     when(blockchain.getBlockHeader(mockHeader.getHash())).thenReturn(Optional.of(mockHeader));
-    var res = coordinator.getOrSyncHeaderByHash(mockHeader.getHash());
+    var res = coordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO);
 
     assertThat(res).isPresent();
   }
@@ -710,7 +710,7 @@ public void assertGetOrSyncForBlockNotPresent() {
     when(backwardSyncContext.syncBackwardsUntil(mockHeader.getBlockHash()))
         .thenReturn(CompletableFuture.completedFuture(null));
 
-    var res = coordinator.getOrSyncHeaderByHash(mockHeader.getHash());
+    var res = coordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO);
 
     assertThat(res).isNotPresent();
   }

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

@@ -99,37 +99,40 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
               Optional.of(forkChoice.getHeadBlockHash() + " is an invalid block")));
     }
 
-    Optional<BlockHeader> newHead =
-        mergeCoordinator.getOrSyncHeaderByHash(forkChoice.getHeadBlockHash());
+    final Optional<BlockHeader> maybeNewHead =
+        mergeCoordinator.getOrSyncHeadByHash(
+            forkChoice.getHeadBlockHash(), forkChoice.getFinalizedBlockHash());
 
-    if (newHead.isEmpty()) {
+    if (maybeNewHead.isEmpty()) {
       return syncingResponse(requestId, forkChoice);
     }
 
+    final BlockHeader newHead = maybeNewHead.get();
+
     maybePayloadAttributes.ifPresentOrElse(
         this::logPayload, () -> LOG.debug("Payload attributes are null"));
 
     if (!isValidForkchoiceState(
-        forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead.get())) {
+        forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead)) {
       logForkchoiceUpdatedCall(INVALID, forkChoice);
       return new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_FORKCHOICE_STATE);
     }
 
     // TODO: post-merge cleanup, this should be unnecessary after merge
-    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) {
+    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead)) {
       logForkchoiceUpdatedCall(INVALID, forkChoice);
       return new JsonRpcSuccessResponse(
           requestId,
           new EngineUpdateForkchoiceResult(
               INVALID,
               Hash.ZERO,
               null,
-              Optional.of(newHead.get() + " did not descend from terminal block")));
+              Optional.of(newHead + " did not descend from terminal block")));
     }
 
     ForkchoiceResult result =
         mergeCoordinator.updateForkChoice(
-            newHead.get(),
+            newHead,
             forkChoice.getFinalizedBlockHash(),
             forkChoice.getSafeBlockHash(),
             maybePayloadAttributes.map(
@@ -149,7 +152,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
         maybePayloadAttributes.map(
             payloadAttributes ->
                 mergeCoordinator.preparePayload(
-                    newHead.get(),
+                    newHead,
                     payloadAttributes.getTimestamp(),
                     payloadAttributes.getPrevRandao(),
                     payloadAttributes.getSuggestedFeeRecipient()));

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

@@ -111,7 +111,7 @@ public void shouldReturnSyncingIfMissingNewHead() {
   public void shouldReturnInvalidOnBadTerminalBlock() {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
 
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(false);
     assertSuccessWithPayloadForForkchoiceResult(
@@ -148,7 +148,7 @@ public void shouldReturnSyncingOnHeadNotFound() {
   @Test
   public void shouldReturnValidWithoutFinalizedOrPayload() {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
 
@@ -172,7 +172,7 @@ public void shouldReturnInvalidOnOldTimestamp() {
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
     when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), parent.getHash()))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.updateForkChoice(
             mockHeader, parent.getHash(), parent.getHash(), Optional.empty()))
@@ -202,7 +202,7 @@ public void shouldReturnValidWithNewHeadAndFinalizedNoPayload() {
     BlockHeader mockParent = builder.number(9L).buildHeader();
     BlockHeader mockHeader = builder.number(10L).parentHash(mockParent.getHash()).buildHeader();
     when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
     when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
@@ -217,7 +217,7 @@ public void shouldReturnValidWithNewHeadAndFinalizedNoPayload() {
   @Test
   public void shouldReturnValidWithoutFinalizedWithPayload() {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
 
@@ -254,7 +254,7 @@ public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsUnknown() {
 
     when(blockchain.getBlockHeader(finalizedBlockHash)).thenReturn(Optional.empty());
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalizedBlockHash))
         .thenReturn(Optional.of(newHead));
 
     var resp =
@@ -275,7 +275,7 @@ public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsNotAnAncestorOfN
     when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
     when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized));
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash()))
         .thenReturn(Optional.of(newHead));
     when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(false);
 
@@ -301,7 +301,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeHeadZeroWithFinalizedBlock()
 
     when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent));
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), parent.getBlockHash()))
         .thenReturn(Optional.of(newHead));
 
     var resp =
@@ -328,7 +328,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsUnknown() {
     when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized));
     when(blockchain.getBlockHeader(safeBlockBlockHash)).thenReturn(Optional.empty());
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash()))
         .thenReturn(Optional.of(newHead));
     when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true);
 
@@ -352,7 +352,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotADescendantOfFinal
     when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized));
     when(blockchain.getBlockHeader(safeBlock.getHash())).thenReturn(Optional.of(safeBlock));
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash()))
         .thenReturn(Optional.of(newHead));
     when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true);
     when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(false);
@@ -377,7 +377,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHea
     when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized));
     when(blockchain.getBlockHeader(safeBlock.getHash())).thenReturn(Optional.of(safeBlock));
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.getOrSyncHeaderByHash(newHead.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(newHead.getHash(), finalized.getBlockHash()))
         .thenReturn(Optional.of(newHead));
     when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true);
     when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(true);
@@ -397,7 +397,7 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHea
   public void shouldIgnoreUpdateToOldHeadAndNotPreparePayload() {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
 
-    when(mergeCoordinator.getOrSyncHeaderByHash(mockHeader.getHash()))
+    when(mergeCoordinator.getOrSyncHeadByHash(mockHeader.getHash(), Hash.ZERO))
         .thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
 

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

@@ -93,15 +93,14 @@ public synchronized void prependAncestorsHeader(final BlockHeader blockHeader) {
       headers.put(blockHeader.getHash(), blockHeader);
       return;
     }
-    BlockHeader firstHeader = firstStoredAncestor.get();
+    final BlockHeader firstHeader = firstStoredAncestor.get();
     headers.put(blockHeader.getHash(), blockHeader);
-    chainStorage.put(blockHeader.getHash(), firstStoredAncestor.get().getHash());
+    chainStorage.put(blockHeader.getHash(), firstHeader.getHash());
     firstStoredAncestor = Optional.of(blockHeader);
     debugLambda(
         LOG,
-        "Added header {} on height {} to backward chain led by pivot {} on height {}",
+        "Added header {} to backward chain led by pivot {} on height {}",
         blockHeader::toLogString,
-        blockHeader::getNumber,
         () -> lastStoredPivot.orElseThrow().toLogString(),
         firstHeader::getNumber);
   }
@@ -127,16 +126,23 @@ public synchronized void dropFirstHeader() {
   }
 
   public synchronized void appendTrustedBlock(final Block newPivot) {
-    debugLambda(LOG, "appending trusted block {}", newPivot::toLogString);
+    debugLambda(LOG, "Appending trusted block {}", newPivot::toLogString);
     headers.put(newPivot.getHash(), newPivot.getHeader());
     blocks.put(newPivot.getHash(), newPivot);
     if (lastStoredPivot.isEmpty()) {
       firstStoredAncestor = Optional.of(newPivot.getHeader());
     } else {
       if (newPivot.getHeader().getParentHash().equals(lastStoredPivot.get().getHash())) {
+        debugLambda(
+            LOG,
+            "Added block {} to backward chain led by pivot {} on height {}",
+            newPivot::toLogString,
+            lastStoredPivot.get()::toLogString,
+            firstStoredAncestor.get()::getNumber);
         chainStorage.put(lastStoredPivot.get().getHash(), newPivot.getHash());
       } else {
         firstStoredAncestor = Optional.of(newPivot.getHeader());
+        debugLambda(LOG, "Re-pivoting to new target block {}", newPivot::toLogString);
       }
     }
     lastStoredPivot = Optional.of(newPivot.getHeader());