Return Invalid forkchoice state if fcU contains invalid finalizedBlockHash or safeBlockHash (#3828)

* added acceptance tests

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>

* updated EngineForkchoiceUpdated to return INVALID_FORKCHOICE_STATE when finalized block hash or safe block hash are unknown

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>

* fixed tests and added new ones for invalid forkchoice state and saving safe block

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/04_invalid_safeblock_hash.json

@@ -0,0 +1,24 @@
+{
+  "request": {
+    "jsonrpc": "2.0",
+    "method": "engine_forkchoiceUpdatedV1",
+    "params": [
+      {
+        "headBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "safeBlockHash": "0x00000051470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04abcdef",
+        "finalizedBlockHash": "0x3b8fb240d288781d4aac94d3fd16809ee413bc99294a085798a589dae51ddd4a"
+      },
+      null
+    ],
+    "id": 67
+  },
+  "response": {
+    "jsonrpc": "2.0",
+    "id": 67,
+    "error": {
+      "code": -38002,
+      "message": "Invalid forkchoice state"
+    }
+  },
+  "statusCode": 200
+}

acceptance-tests/tests/src/test/resources/jsonrpc/engine/test-cases/05_invalid_finalized_block_hash.json

@@ -0,0 +1,24 @@
+{
+  "request": {
+    "jsonrpc": "2.0",
+    "method": "engine_forkchoiceUpdatedV1",
+    "params": [
+      {
+        "headBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "safeBlockHash": "0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858",
+        "finalizedBlockHash": "0x00000040d288781d4aac94d3fd16809ee413bc99294a085798a589dae5abcdef"
+      },
+      null
+    ],
+    "id": 67
+  },
+  "response": {
+    "jsonrpc": "2.0",
+    "id": 67,
+    "error": {
+      "code": -38002,
+      "message": "Invalid forkchoice state"
+    }
+  },
+  "statusCode": 200
+}

besu/src/main/java/org/hyperledger/besu/controller/MergeBesuControllerBuilder.java

@@ -124,6 +124,11 @@ public class MergeBesuControllerBuilder extends BesuControllerBuilder {
         .flatMap(blockchain::getBlockHeader)
         .ifPresent(mergeContext::setFinalized);
 
+    blockchain
+        .getSafeBlock()
+        .flatMap(blockchain::getBlockHeader)
+        .ifPresent(mergeContext::setSafeBlock);
+
     if (terminalBlockNumber.isPresent() && terminalBlockHash.isPresent()) {
       Optional<BlockHeader> termBlock = blockchain.getBlockHeader(terminalBlockNumber.getAsLong());
       mergeContext.setTerminalPoWBlock(termBlock);

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/MergeContext.java

@@ -49,6 +49,10 @@ public interface MergeContext extends ConsensusContext {
 
   Optional<BlockHeader> getFinalized();
 
+  void setSafeBlock(final BlockHeader blockHeader);
+
+  Optional<BlockHeader> getSafeBlock();
+
   Optional<BlockHeader> getTerminalPoWBlock();
 
   void setTerminalPoWBlock(Optional<BlockHeader> hashAndNumber);
@@ -59,7 +63,7 @@ public interface MergeContext extends ConsensusContext {
 
   Optional<Block> retrieveBlockById(final PayloadIdentifier payloadId);
 
-  void fireNewForkchoiceMessageEvent(
+  void fireNewUnverifiedForkchoiceMessageEvent(
       final Hash headBlockHash,
       final Optional<Hash> maybeFinalizedBlockHash,
       final Hash safeBlockHash);

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PostMergeContext.java

@@ -51,6 +51,7 @@ public class PostMergeContext implements MergeContext {
 
   // latest finalized block
   private final AtomicReference<BlockHeader> lastFinalized = new AtomicReference<>();
+  private final AtomicReference<BlockHeader> lastSafeBlock = new AtomicReference<>();
   private final AtomicReference<Optional<BlockHeader>> terminalPoWBlock =
       new AtomicReference<>(Optional.empty());
 
@@ -138,7 +139,7 @@ public class PostMergeContext implements MergeContext {
   }
 
   @Override
-  public void fireNewForkchoiceMessageEvent(
+  public void fireNewUnverifiedForkchoiceMessageEvent(
       final Hash headBlockHash,
       final Optional<Hash> maybeFinalizedBlockHash,
       final Hash safeBlockHash) {
@@ -161,6 +162,16 @@ public class PostMergeContext implements MergeContext {
     return Optional.ofNullable(lastFinalized.get());
   }
 
+  @Override
+  public void setSafeBlock(final BlockHeader blockHeader) {
+    lastSafeBlock.set(blockHeader);
+  }
+
+  @Override
+  public Optional<BlockHeader> getSafeBlock() {
+    return Optional.ofNullable(lastSafeBlock.get());
+  }
+
   @Override
   public Optional<BlockHeader> getTerminalPoWBlock() {
     return terminalPoWBlock.get();

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/TransitionContext.java

@@ -84,11 +84,11 @@ public class TransitionContext implements MergeContext {
   }
 
   @Override
-  public void fireNewForkchoiceMessageEvent(
+  public void fireNewUnverifiedForkchoiceMessageEvent(
       final Hash headBlockHash,
       final Optional<Hash> maybeFinalizedBlockHash,
       final Hash safeBlockHash) {
-    postMergeContext.fireNewForkchoiceMessageEvent(
+    postMergeContext.fireNewUnverifiedForkchoiceMessageEvent(
         headBlockHash, maybeFinalizedBlockHash, safeBlockHash);
   }
 
@@ -107,6 +107,16 @@ public class TransitionContext implements MergeContext {
     return postMergeContext.getFinalized();
   }
 
+  @Override
+  public void setSafeBlock(final BlockHeader blockHeader) {
+    postMergeContext.setSafeBlock(blockHeader);
+  }
+
+  @Override
+  public Optional<BlockHeader> getSafeBlock() {
+    return postMergeContext.getSafeBlock();
+  }
+
   @Override
   public Optional<BlockHeader> getTerminalPoWBlock() {
     return this.postMergeContext.getTerminalPoWBlock();

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

@@ -254,23 +254,12 @@ public class MergeCoordinator implements MergeMiningCoordinator {
 
   @Override
   public ForkchoiceResult updateForkChoice(
-      final Hash headBlockHash, final Hash finalizedBlockHash) {
+      final BlockHeader newHead, final Hash finalizedBlockHash, final Hash safeBlockHash) {
     MutableBlockchain blockchain = protocolContext.getBlockchain();
     Optional<BlockHeader> currentFinalized = mergeContext.getFinalized();
     final Optional<BlockHeader> newFinalized = blockchain.getBlockHeader(finalizedBlockHash);
-    BlockHeader newHead = blockchain.getBlockHeader(headBlockHash).orElse(null);
-    if (newHead == null) {
-      return ForkchoiceResult.withFailure(
-          String.format("not able to find new head block %s", headBlockHash), Optional.empty());
-    }
+
     final Optional<Hash> latestValid = getLatestValidAncestor(newHead);
-    if (newFinalized.isEmpty() && !finalizedBlockHash.equals(Hash.ZERO)) {
-      // we should only fail to find when it's the special value 0x000..000
-      return ForkchoiceResult.withFailure(
-          String.format(
-              "should've been able to find block hash %s but couldn't", finalizedBlockHash),
-          latestValid);
-    }
 
     if (currentFinalized.isPresent()
         && newFinalized.isPresent()
@@ -282,8 +271,6 @@ public class MergeCoordinator implements MergeMiningCoordinator {
           latestValid);
     }
 
-    // ensure we have headBlock:
-
     // ensure new head is descendant of finalized
     Optional<String> descendantError =
         newFinalized
@@ -316,6 +303,14 @@ public class MergeCoordinator implements MergeMiningCoordinator {
           mergeContext.setFinalized(blockHeader);
         });
 
+    blockchain
+        .getBlockHeader(safeBlockHash)
+        .ifPresent(
+            newSafeBlock -> {
+              blockchain.setSafeBlock(safeBlockHash);
+              mergeContext.setSafeBlock(newSafeBlock);
+            });
+
     return ForkchoiceResult.withResult(newFinalized, Optional.of(newHead));
   }
 
@@ -458,9 +453,10 @@ public class MergeCoordinator implements MergeMiningCoordinator {
                     .orElse(Optional.empty()));
   }
 
-  private boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) {
+  @Override
+  public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) {
     LOG.debug(
-        "checking if finalized block {}:{} is ancestor of {}:{}",
+        "checking if block {}:{} is ancestor of {}:{}",
         ancestorBlock.getNumber(),
         ancestorBlock.getBlockHash(),
         newBlock.getNumber(),

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

@@ -35,7 +35,8 @@ public interface MergeMiningCoordinator extends MiningCoordinator {
 
   Result executeBlock(final Block block);
 
-  ForkchoiceResult updateForkChoice(final Hash headBlockHash, final Hash finalizedBlockHash);
+  ForkchoiceResult updateForkChoice(
+      final BlockHeader newHead, final Hash finalizedBlockHash, final Hash safeBlockHash);
 
   Optional<Hash> getLatestValidAncestor(Hash blockHash);
 
@@ -43,6 +44,8 @@ public interface MergeMiningCoordinator extends MiningCoordinator {
 
   boolean latestValidAncestorDescendsFromTerminal(final BlockHeader blockHeader);
 
+  boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock);
+
   boolean isBackwardSyncing();
 
   CompletableFuture<Void> appendNewPayloadToSync(Block newPayload);

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

@@ -139,8 +139,8 @@ public class TransitionCoordinator extends TransitionUtils<MiningCoordinator>
 
   @Override
   public ForkchoiceResult updateForkChoice(
-      final Hash headBlockHash, final Hash finalizedBlockHash) {
-    return mergeCoordinator.updateForkChoice(headBlockHash, finalizedBlockHash);
+      final BlockHeader newHead, final Hash finalizedBlockHash, final Hash safeBlockHash) {
+    return mergeCoordinator.updateForkChoice(newHead, finalizedBlockHash, safeBlockHash);
   }
 
   @Override
@@ -178,4 +178,9 @@ public class TransitionCoordinator extends TransitionUtils<MiningCoordinator>
   public boolean isMiningBeforeMerge() {
     return mergeCoordinator.isMiningBeforeMerge();
   }
+
+  @Override
+  public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) {
+    return mergeCoordinator.isDescendantOf(ancestorBlock, newBlock);
+  }
 }

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

@@ -142,7 +142,8 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     coordinator.executeBlock(child);
 
     ForkchoiceResult result =
-        coordinator.updateForkChoice(childHeader.getHash(), terminalHeader.getHash());
+        coordinator.updateForkChoice(
+            childHeader, terminalHeader.getHash(), terminalHeader.getHash());
 
     assertThat(result.isSuccessful()).isFalse();
     assertThat(result.getErrorMessage()).isPresent();
@@ -151,6 +152,8 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
 
     verify(blockchain, never()).setFinalized(childHeader.getHash());
     verify(mergeContext, never()).setFinalized(childHeader);
+    verify(blockchain, never()).setSafeBlock(childHeader.getHash());
+    verify(mergeContext, never()).setSafeBlock(childHeader);
 
     assertThat(this.coordinator.latestValidAncestorDescendsFromTerminal(child.getHeader()))
         .isTrue();
@@ -216,10 +219,13 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     coordinator.executeBlock(headBlock);
 
-    coordinator.updateForkChoice(headBlock.getHash(), firstFinalizedBlock.getHash());
+    coordinator.updateForkChoice(
+        headBlockHeader, firstFinalizedBlock.getHash(), firstFinalizedBlock.getHash());
 
     verify(blockchain).setFinalized(firstFinalizedBlock.getHash());
     verify(mergeContext).setFinalized(firstFinalizedHeader);
+    verify(blockchain).setSafeBlock(firstFinalizedBlock.getHash());
+    verify(mergeContext).setSafeBlock(firstFinalizedHeader);
   }
 
   @Test
@@ -241,10 +247,13 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
     coordinator.executeBlock(headBlock);
 
-    coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
+    coordinator.updateForkChoice(
+        headBlockHeader, lastFinalizedBlock.getHash(), lastFinalizedBlock.getHash());
 
     verify(blockchain).setFinalized(lastFinalizedBlock.getHash());
     verify(mergeContext).setFinalized(lastFinalizedHeader);
+    verify(blockchain).setSafeBlock(lastFinalizedBlock.getHash());
+    verify(mergeContext).setSafeBlock(lastFinalizedHeader);
   }
 
   @Test
@@ -270,11 +279,15 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
         .thenReturn(Optional.of(lastFinalizedHeader));
     when(blockchain.getBlockHeader(headBlockHeader.getHash()))
         .thenReturn(Optional.of(headBlockHeader));
-    var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
+    var res =
+        coordinator.updateForkChoice(
+            headBlockHeader, lastFinalizedBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isFailed()).isTrue();
 
     verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
     verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
+    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
+    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
   }
 
   @Test
@@ -300,37 +313,16 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
         .thenReturn(Optional.of(lastFinalizedHeader));
     when(blockchain.getBlockHeader(headBlockHeader.getHash()))
         .thenReturn(Optional.of(headBlockHeader));
-    var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
+    var res =
+        coordinator.updateForkChoice(
+            headBlockHeader, lastFinalizedBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isSuccessful()).isFalse();
     assertThat(res.isFailed()).isTrue();
 
     verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
     verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
-  }
-
-  @Test
-  public void updateForkChoiceShouldFailIfHeadBlockNotFound() {
-    BlockHeader terminalHeader = terminalPowBlock();
-    coordinator.executeBlock(new Block(terminalHeader, BlockBody.empty()));
-
-    BlockHeader prevFinalizedHeader = nextBlockHeader(terminalHeader);
-    Block prevFinalizedBlock = new Block(prevFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(prevFinalizedBlock);
-
-    when(mergeContext.getFinalized()).thenReturn(Optional.of(prevFinalizedHeader));
-
-    BlockHeader lastFinalizedHeader = nextBlockHeader(prevFinalizedHeader);
-    Block lastFinalizedBlock = new Block(lastFinalizedHeader, BlockBody.empty());
-    coordinator.executeBlock(lastFinalizedBlock);
-
-    BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
-    Block headBlock = new Block(headBlockHeader, BlockBody.empty());
-    // note this block is not executed, so not known by us
-    var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
-    assertThat(res.isFailed()).isTrue();
-
-    verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
-    verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
+    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
+    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
   }
 
   @Test
@@ -355,12 +347,16 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     when(blockchain.getBlockHeader(headBlockHeader.getHash()))
         .thenReturn(Optional.of(headBlockHeader))
         .thenReturn(Optional.of(headBlockHeader));
-    var res = coordinator.updateForkChoice(headBlock.getHash(), lastFinalizedBlock.getHash());
+    var res =
+        coordinator.updateForkChoice(
+            headBlockHeader, lastFinalizedBlock.getHash(), lastFinalizedBlock.getHash());
     assertThat(res.isSuccessful()).isFalse();
     assertThat(res.isFailed()).isTrue();
 
     verify(blockchain, never()).setFinalized(lastFinalizedBlock.getHash());
     verify(mergeContext, never()).setFinalized(lastFinalizedHeader);
+    verify(blockchain, never()).setSafeBlock(lastFinalizedBlock.getHash());
+    verify(mergeContext, never()).setSafeBlock(lastFinalizedHeader);
   }
 
   @Test

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

@@ -30,6 +30,8 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EngineForkchoiceUpdatedParameter;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadAttributesParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcError;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.EngineUpdateForkchoiceResult;
@@ -60,6 +62,7 @@ public class EngineForkchoiceUpdated extends ExecutionEngineJsonRpcMethod {
 
   @Override
   public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) {
+    final Object requestId = requestContext.getRequest().getId();
 
     final EngineForkchoiceUpdatedParameter forkChoice =
         requestContext.getRequiredParameter(0, EngineForkchoiceUpdatedParameter.class);
@@ -68,93 +71,138 @@ public class EngineForkchoiceUpdated extends ExecutionEngineJsonRpcMethod {
 
     Optional<Hash> maybeFinalizedHash =
         Optional.ofNullable(forkChoice.getFinalizedBlockHash())
-            .filter(finalized -> !Hash.ZERO.equals(finalized));
+            .filter(finalized -> !finalized.isZero());
 
-    mergeContext.fireNewForkchoiceMessageEvent(
+    mergeContext.fireNewUnverifiedForkchoiceMessageEvent(
         forkChoice.getHeadBlockHash(), maybeFinalizedHash, forkChoice.getSafeBlockHash());
 
     if (mergeContext.isSyncing()) {
-      return new JsonRpcSuccessResponse(
-          requestContext.getRequest().getId(),
-          new EngineUpdateForkchoiceResult(SYNCING, null, null, Optional.empty()));
+      return syncingResponse(requestId);
+    }
+
+    Optional<BlockHeader> newHead =
+        protocolContext.getBlockchain().getBlockHeader(forkChoice.getHeadBlockHash());
+
+    if (newHead.isEmpty()) {
+      Optional.ofNullable(forkChoice.getHeadBlockHash())
+          .filter(hash -> !hash.equals(Hash.ZERO))
+          .ifPresent(mergeCoordinator::getOrSyncHeaderByHash);
+
+      return syncingResponse(requestId);
     }
 
     LOG.info(
-        "Consensus fork-choice-update: head: {}, finalized: {}",
+        "Consensus fork-choice-update: head: {}, finalized: {}, safeBlockHash: {}",
         forkChoice.getHeadBlockHash(),
-        forkChoice.getFinalizedBlockHash());
+        forkChoice.getFinalizedBlockHash(),
+        forkChoice.getSafeBlockHash());
 
-    Optional<BlockHeader> newHead =
-        protocolContext.getBlockchain().getBlockHeader(forkChoice.getHeadBlockHash());
+    if (!isValidForkchoiceState(
+        forkChoice.getSafeBlockHash(), forkChoice.getFinalizedBlockHash(), newHead.get())) {
+      return new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_FORKCHOICE_STATE);
+    }
 
-    Optional<BlockHeader> finalizedHead =
-        maybeFinalizedHash.flatMap(protocolContext.getBlockchain()::getBlockHeader);
+    // TODO: post-merge cleanup, this should be unnecessary after merge
+    if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) {
+      return new JsonRpcSuccessResponse(
+          requestId,
+          new EngineUpdateForkchoiceResult(
+              INVALID_TERMINAL_BLOCK,
+              null,
+              null,
+              Optional.of(newHead.get() + " did not descend from terminal block")));
+    }
 
-    if (newHead.isPresent() && (maybeFinalizedHash.isEmpty() || finalizedHead.isPresent())) {
+    ForkchoiceResult result =
+        mergeCoordinator.updateForkChoice(
+            newHead.get(), forkChoice.getFinalizedBlockHash(), forkChoice.getSafeBlockHash());
+
+    if (result.isFailed()) {
+      final Optional<Hash> latestValid = result.getLatestValid();
+      return new JsonRpcSuccessResponse(
+          requestId,
+          new EngineUpdateForkchoiceResult(
+              INVALID,
+              latestValid.isPresent() ? latestValid.get() : null,
+              null,
+              result.getErrorMessage()));
+    }
 
-      // TODO: post-merge cleanup, this should be unnecessary after merge
-      if (!mergeCoordinator.latestValidAncestorDescendsFromTerminal(newHead.get())) {
-        return new JsonRpcSuccessResponse(
-            requestContext.getRequest().getId(),
-            new EngineUpdateForkchoiceResult(
-                INVALID_TERMINAL_BLOCK,
-                null,
-                null,
-                Optional.of(newHead.get() + " did not descend from terminal block")));
+    // begin preparing a block if we have a non-empty payload attributes param
+    Optional<PayloadIdentifier> payloadId =
+        optionalPayloadAttributes.map(
+            payloadAttributes ->
+                mergeCoordinator.preparePayload(
+                    newHead.get(),
+                    payloadAttributes.getTimestamp(),
+                    payloadAttributes.getPrevRandao(),
+                    payloadAttributes.getSuggestedFeeRecipient()));
+
+    payloadId.ifPresent(
+        pid ->
+            debugLambda(
+                LOG,
+                "returning identifier {} for requested payload {}",
+                pid::toHexString,
+                () -> optionalPayloadAttributes.map(EnginePayloadAttributesParameter::serialize)));
+
+    return new JsonRpcSuccessResponse(
+        requestId,
+        new EngineUpdateForkchoiceResult(
+            VALID,
+            result.getNewHead().map(BlockHeader::getHash).orElse(null),
+            payloadId.orElse(null),
+            Optional.empty()));
+  }
+
+  private boolean isValidForkchoiceState(
+      final Hash safeBlockHash, final Hash finalizedBlockHash, final BlockHeader newBlock) {
+    Optional<BlockHeader> maybeFinalizedBlock = Optional.empty();
+
+    if (!finalizedBlockHash.isZero()) {
+      maybeFinalizedBlock = protocolContext.getBlockchain().getBlockHeader(finalizedBlockHash);
+
+      // if the finalized block hash is not zero, we always need to have its block, because we
+      // only do this check once we have finished syncing
+      if (maybeFinalizedBlock.isEmpty()) {
+        return false;
       }
 
-      // update fork choice
-      ForkchoiceResult result =
-          mergeCoordinator.updateForkChoice(
-              forkChoice.getHeadBlockHash(), forkChoice.getFinalizedBlockHash());
-
-      // only build a block if forkchoice was successful
-      if (result.isSuccessful()) {
-        // begin preparing a block if we have a non-empty payload attributes param
-        Optional<PayloadIdentifier> payloadId =
-            optionalPayloadAttributes.map(
-                payloadAttributes ->
-                    mergeCoordinator.preparePayload(
-                        newHead.get(),
-                        payloadAttributes.getTimestamp(),
-                        payloadAttributes.getPrevRandao(),
-                        payloadAttributes.getSuggestedFeeRecipient()));
-
-        payloadId.ifPresent(
-            pid ->
-                debugLambda(
-                    LOG,
-                    "returning identifier {} for requested payload {}",
-                    pid::toHexString,
-                    () ->
-                        optionalPayloadAttributes.map(
-                            EnginePayloadAttributesParameter::serialize)));
-
-        return new JsonRpcSuccessResponse(
-            requestContext.getRequest().getId(),
-            new EngineUpdateForkchoiceResult(
-                VALID,
-                result.getNewHead().map(BlockHeader::getHash).orElse(null),
-                payloadId.orElse(null),
-                Optional.empty()));
-      } else if (result.isFailed()) {
-        final Optional<Hash> latestValid = result.getLatestValid();
-        return new JsonRpcSuccessResponse(
-            requestContext.getRequest().getId(),
-            new EngineUpdateForkchoiceResult(
-                INVALID,
-                latestValid.isPresent() ? latestValid.get() : null,
-                null,
-                result.getErrorMessage()));
+      // a valid finalized block must be an ancestor of the new head
+      if (!mergeCoordinator.isDescendantOf(maybeFinalizedBlock.get(), newBlock)) {
+        return false;
       }
     }
 
-    Optional.ofNullable(forkChoice.getHeadBlockHash())
-        .filter(hash -> !hash.equals(Hash.ZERO))
-        .ifPresent(mergeCoordinator::getOrSyncHeaderByHash);
+    // A zero value is only allowed, if the transition block is not yet finalized.
+    // Once we have at least one finalized block, the transition block has either been finalized
+    // directly
+    // or through one of its descendants.
+    if (safeBlockHash.isZero()) {
+      return finalizedBlockHash.isZero();
+    }
+
+    final Optional<BlockHeader> maybeSafeBlock =
+        protocolContext.getBlockchain().getBlockHeader(safeBlockHash);
+
+    // if the safe block hash is not zero, we always need to have its block, because we
+    // only do this check once we have finished syncing
+    if (maybeSafeBlock.isEmpty()) {
+      return false;
+    }
+
+    // a valid safe block must be a descendant of the finalized block
+    if (maybeFinalizedBlock.isPresent()
+        && !mergeCoordinator.isDescendantOf(maybeFinalizedBlock.get(), maybeSafeBlock.get())) {
+      return false;
+    }
+
+    // a valid safe block must be an ancestor of the new block
+    return mergeCoordinator.isDescendantOf(maybeSafeBlock.get(), newBlock);
+  }
 
+  private JsonRpcResponse syncingResponse(final Object requestId) {
     return new JsonRpcSuccessResponse(
-        requestContext.getRequest().getId(),
-        new EngineUpdateForkchoiceResult(SYNCING, null, null, Optional.empty()));
+        requestId, new EngineUpdateForkchoiceResult(SYNCING, null, null, Optional.empty()));
   }
 }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcError.java

@@ -72,6 +72,7 @@ public enum JsonRpcError {
   // Execution engine failures
   UNKNOWN_PAYLOAD(-32001, "Payload does not exist / is not available"),
   INVALID_TERMINAL_BLOCK(-32002, "Terminal block doesn't satisfy terminal block conditions"),
+  INVALID_FORKCHOICE_STATE(-38002, "Invalid forkchoice state"),
 
   // Miner failures
   COINBASE_NOT_SET(-32010, "Coinbase not set. Unable to start mining without a coinbase"),

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

@@ -38,6 +38,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EngineForkchoiceUpdatedParameter;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EnginePayloadAttributesParameter;
+import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcErrorResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponseType;
 import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
@@ -105,6 +106,7 @@ public class EngineForkchoiceUpdatedTest {
 
     when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(false);
+    when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
     assertSuccessWithPayloadForForkchoiceResult(
         Optional.empty(), mock(ForkchoiceResult.class), INVALID_TERMINAL_BLOCK);
   }
@@ -120,6 +122,7 @@ public class EngineForkchoiceUpdatedTest {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
     when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
 
     assertSuccessWithPayloadForForkchoiceResult(
         Optional.empty(),
@@ -140,8 +143,9 @@ public class EngineForkchoiceUpdatedTest {
     when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent));
     //    when(blockchain.getChainHeadHeader()).thenReturn(parent);
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
     when(mergeContext.isSyncing()).thenReturn(false);
-    when(mergeCoordinator.updateForkChoice(mockHeader.getHash(), parent.getHash()))
+    when(mergeCoordinator.updateForkChoice(mockHeader, parent.getHash(), parent.getHash()))
         .thenReturn(
             ForkchoiceResult.withFailure(
                 "new head timestamp not greater than parent", Optional.of(parent.getHash())));
@@ -166,6 +170,7 @@ public class EngineForkchoiceUpdatedTest {
     BlockHeader mockHeader = builder.number(10L).parentHash(mockParent.getHash()).buildHeader();
     when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
 
     assertSuccessWithPayloadForForkchoiceResult(
         Optional.empty(),
@@ -178,6 +183,7 @@ public class EngineForkchoiceUpdatedTest {
     BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
     when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
     when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(any(), any())).thenReturn(true);
 
     var payloadParams =
         new EnginePayloadAttributesParameter(
@@ -200,13 +206,145 @@ public class EngineForkchoiceUpdatedTest {
     assertThat(res.getPayloadId()).isEqualTo(mockPayloadId.toHexString());
   }
 
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsUnknown() {
+    BlockHeader newHead = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    Hash finalizedBlockHash = Hash.hash(Bytes32.fromHexStringLenient("0x424abcdef"));
+
+    when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
+    when(blockchain.getBlockHeader(finalizedBlockHash)).thenReturn(Optional.empty());
+    when(mergeContext.isSyncing()).thenReturn(false);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), finalizedBlockHash, finalizedBlockHash),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfFinalizedBlockIsNotAnAncestorOfNewHead() {
+    BlockHeader finalized = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader newHead = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+
+    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.isDescendantOf(finalized, newHead)).thenReturn(false);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), finalized.getBlockHash(), finalized.getBlockHash()),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfSafeHeadZeroWithFinalizedBlock() {
+    BlockHeader parent = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader newHead =
+        new BlockHeaderTestFixture()
+            .baseFeePerGas(Wei.ONE)
+            .parentHash(parent.getHash())
+            .timestamp(parent.getTimestamp())
+            .buildHeader();
+
+    when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
+    when(blockchain.getBlockHeader(parent.getHash())).thenReturn(Optional.of(parent));
+    when(mergeContext.isSyncing()).thenReturn(false);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), parent.getBlockHash(), Hash.ZERO),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsUnknown() {
+    BlockHeader finalized = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader newHead =
+        new BlockHeaderTestFixture()
+            .baseFeePerGas(Wei.ONE)
+            .parentHash(finalized.getHash())
+            .timestamp(finalized.getTimestamp())
+            .buildHeader();
+    Hash safeBlockBlockHash = Hash.hash(Bytes32.fromHexStringLenient("0x424abcdef"));
+
+    when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
+    when(blockchain.getBlockHeader(finalized.getHash())).thenReturn(Optional.of(finalized));
+    when(blockchain.getBlockHeader(safeBlockBlockHash)).thenReturn(Optional.empty());
+    when(mergeContext.isSyncing()).thenReturn(false);
+    when(mergeCoordinator.isDescendantOf(finalized, newHead)).thenReturn(true);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), finalized.getBlockHash(), safeBlockBlockHash),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotADescendantOfFinalized() {
+    BlockHeader finalized = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader newHead = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader safeBlock = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+
+    when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
+    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.isDescendantOf(finalized, newHead)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(false);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), finalized.getBlockHash(), safeBlock.getBlockHash()),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
+  @Test
+  public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHead() {
+    BlockHeader finalized = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader newHead = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+    BlockHeader safeBlock = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+
+    when(blockchain.getBlockHeader(newHead.getHash())).thenReturn(Optional.of(newHead));
+    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.isDescendantOf(finalized, newHead)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(finalized, safeBlock)).thenReturn(true);
+    when(mergeCoordinator.isDescendantOf(safeBlock, newHead)).thenReturn(false);
+
+    var resp =
+        resp(
+            new EngineForkchoiceUpdatedParameter(
+                newHead.getBlockHash(), finalized.getBlockHash(), safeBlock.getBlockHash()),
+            Optional.empty());
+
+    assertInvalidForkchoiceState(resp);
+  }
+
   private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult(
       final Optional<EnginePayloadAttributesParameter> payloadParam,
       final ForkchoiceResult forkchoiceResult,
       final EngineStatus expectedStatus) {
 
     // result from mergeCoordinator has no new finalized, new head:
-    when(mergeCoordinator.updateForkChoice(any(Hash.class), any(Hash.class)))
+    when(mergeCoordinator.updateForkChoice(
+            any(BlockHeader.class), any(Hash.class), any(Hash.class)))
         .thenReturn(forkchoiceResult);
     var resp =
         resp(new EngineForkchoiceUpdatedParameter(mockHash, mockHash, mockHash), payloadParam);
@@ -231,7 +369,8 @@ public class EngineForkchoiceUpdatedTest {
     }
 
     // assert that listeners are always notified
-    verify(mergeContext).fireNewForkchoiceMessageEvent(mockHash, Optional.of(mockHash), mockHash);
+    verify(mergeContext)
+        .fireNewUnverifiedForkchoiceMessageEvent(mockHash, Optional.of(mockHash), mockHash);
 
     return res;
   }
@@ -255,4 +394,12 @@ public class EngineForkchoiceUpdatedTest {
         .map(EngineUpdateForkchoiceResult.class::cast)
         .get();
   }
+
+  private void assertInvalidForkchoiceState(final JsonRpcResponse resp) {
+    assertThat(resp.getType()).isEqualTo(JsonRpcResponseType.ERROR);
+
+    var errorResp = (JsonRpcErrorResponse) resp;
+    assertThat(errorResp.getError().getCode()).isEqualTo(-38002);
+    assertThat(errorResp.getError().getMessage()).isEqualTo("Invalid forkchoice state");
+  }
 }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/Blockchain.java

@@ -44,6 +44,13 @@ public interface Blockchain {
    */
   Optional<Hash> getFinalized();
 
+  /**
+   * Return the last safe block hash if present.
+   *
+   * @return The hash of the last safe block
+   */
+  Optional<Hash> getSafeBlock();
+
   /**
    * Return the block number of the head of the canonical chain.
    *

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BlockchainStorage.java

@@ -32,6 +32,8 @@ public interface BlockchainStorage {
 
   Optional<Hash> getFinalized();
 
+  Optional<Hash> getSafeBlock();
+
   Optional<BlockHeader> getBlockHeader(Hash blockHash);
 
   Optional<BlockBody> getBlockBody(Hash blockHash);
@@ -66,6 +68,8 @@ public interface BlockchainStorage {
 
     void setFinalized(Hash blockHash);
 
+    void setSafeBlock(Hash blockHash);
+
     void removeBlockHash(long blockNumber);
 
     void removeTransactionLocation(Hash transactionHash);

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/DefaultBlockchain.java

@@ -199,6 +199,11 @@ public class DefaultBlockchain implements MutableBlockchain {
     return blockchainStorage.getFinalized();
   }
 
+  @Override
+  public Optional<Hash> getSafeBlock() {
+    return blockchainStorage.getSafeBlock();
+  }
+
   @Override
   public Hash getChainHeadHash() {
     return chainHeader.getHash();
@@ -530,6 +535,13 @@ public class DefaultBlockchain implements MutableBlockchain {
     updater.commit();
   }
 
+  @Override
+  public void setSafeBlock(final Hash blockHash) {
+    final var updater = blockchainStorage.updater();
+    updater.setSafeBlock(blockHash);
+    updater.commit();
+  }
+
   private void updateCacheForNewCanonicalHead(final Block block, final Difficulty uInt256) {
     chainHeader = block.getHeader();
     totalDifficulty = uInt256;

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/MutableBlockchain.java

@@ -58,4 +58,11 @@ public interface MutableBlockchain extends Blockchain {
    * @param blockHash The hash of the last finalized block.
    */
   void setFinalized(final Hash blockHash);
+
+  /**
+   * Set the hash of the last safe block.
+   *
+   * @param blockHash The hash of the last safe block.
+   */
+  void setSafeBlock(final Hash blockHash);
 }

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/storage/keyvalue/KeyValueStoragePrefixedKeyBlockchainStorage.java

@@ -44,6 +44,8 @@ public class KeyValueStoragePrefixedKeyBlockchainStorage implements BlockchainSt
       Bytes.wrap("forkHeads".getBytes(StandardCharsets.UTF_8));
   private static final Bytes FINALIZED_BLOCK_HASH_KEY =
       Bytes.wrap("finalizedBlockHash".getBytes(StandardCharsets.UTF_8));
+  private static final Bytes SAFE_BLOCK_HASH_KEY =
+      Bytes.wrap("safeBlockHash".getBytes(StandardCharsets.UTF_8));
 
   private static final Bytes VARIABLES_PREFIX = Bytes.of(1);
   static final Bytes BLOCK_HEADER_PREFIX = Bytes.of(2);
@@ -79,6 +81,11 @@ public class KeyValueStoragePrefixedKeyBlockchainStorage implements BlockchainSt
     return get(VARIABLES_PREFIX, FINALIZED_BLOCK_HASH_KEY).map(this::bytesToHash);
   }
 
+  @Override
+  public Optional<Hash> getSafeBlock() {
+    return get(VARIABLES_PREFIX, SAFE_BLOCK_HASH_KEY).map(this::bytesToHash);
+  }
+
   @Override
   public Optional<BlockHeader> getBlockHeader(final Hash blockHash) {
     return get(BLOCK_HEADER_PREFIX, blockHash)
@@ -186,6 +193,11 @@ public class KeyValueStoragePrefixedKeyBlockchainStorage implements BlockchainSt
       set(VARIABLES_PREFIX, FINALIZED_BLOCK_HASH_KEY, blockHash);
     }
 
+    @Override
+    public void setSafeBlock(final Hash blockHash) {
+      set(VARIABLES_PREFIX, SAFE_BLOCK_HASH_KEY, blockHash);
+    }
+
     @Override
     public void removeBlockHash(final long blockNumber) {
       remove(BLOCK_HASH_PREFIX, UInt256.valueOf(blockNumber));

ethereum/referencetests/src/main/java/org/hyperledger/besu/ethereum/referencetests/ReferenceTestBlockchain.java

@@ -60,6 +60,8 @@ public class ReferenceTestBlockchain implements Blockchain {
       "Chain head is inherently non-deterministic. The block currently being processed should be treated as the chain head.";
   private static final String FINALIZED_ERROR =
       "Finalized block is inherently non-deterministic. The block currently being processed should be treated as the finalized block.";
+  private static final String SAFE_BLOCK_ERROR =
+      "Safe block is inherently non-deterministic. The block currently being processed should be treated as the safe block.";
   private final Map<Hash, BlockHeader> hashToHeader = new HashMap<>();
 
   public ReferenceTestBlockchain() {
@@ -100,6 +102,11 @@ public class ReferenceTestBlockchain implements Blockchain {
     throw new NonDeterministicOperationException(FINALIZED_ERROR);
   }
 
+  @Override
+  public Optional<Hash> getSafeBlock() {
+    throw new NonDeterministicOperationException(SAFE_BLOCK_ERROR);
+  }
+
   @Override
   public long getChainHeadBlockNumber() {
     throw new NonDeterministicOperationException(CHAIN_HEAD_ERROR);