fcU: Ignore newHead if it is an ancestor of chain head (hyperledger#4055

) * ignore forkchoice update if new head is an ancestor of the chain head * added draft for CHANGELOG.md * update PR link for CHANGELOG.md Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net> Co-authored-by: Justin Florentine <justin+github@florentine.us>
lacchain · Nov 3, 2023 · bf1d1c3 · bf1d1c3
1 parent 9e97802
commit bf1d1c3
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 - Add TTD and DNS to Sepolia config [#4024](https://github.com/hyperledger/besu/pull/4024)
 - Add terminal block hash and number to Ropsten genesis file [#4026](https://github.com/hyperledger/besu/pull/4026)
 - Return `type` with value `0x0` when serializing legacy transactions [#4027](https://github.com/hyperledger/besu/pull/4027)
+- Ignore `ForkchoiceUpdate` if `newHead` is an ancestor of the chain head [#4055](https://github.com/hyperledger/besu/pull/4055)
 
 ### Bug Fixes
 - Fixed a snapsync issue that can sometimes block the healing step [#3920](https://github.com/hyperledger/besu/pull/3920)

diff --git a/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java
@@ -289,6 +289,12 @@ public ForkchoiceResult updateForkChoice(
     MutableBlockchain blockchain = protocolContext.getBlockchain();
     final Optional<BlockHeader> newFinalized = blockchain.getBlockHeader(finalizedBlockHash);
 
+    if (newHead.getNumber() < blockchain.getChainHeadBlockNumber()
+        && isDescendantOf(newHead, blockchain.getChainHeadHeader())) {
+      LOG.info("Ignoring update to old head");
+      return ForkchoiceResult.withIgnoreUpdateToOldHead(newHead);
+    }
+
     final Optional<Hash> latestValid = getLatestValidAncestor(newHead);
 
     Optional<BlockHeader> parentOfNewHead = blockchain.getBlockHeader(newHead.getParentHash());
@@ -434,7 +440,7 @@ boolean ancestorIsValidTerminalProofOfWork(final BlockHeader blockheader) {
 
     boolean resp =
         parent.filter(header -> isTerminalProofOfWorkBlock(header, protocolContext)).isPresent();
-    LOG.warn(
+    LOG.debug(
         "checking ancestor {} is valid terminal PoW for {}\n {}",
         parent.map(BlockHeader::toLogString).orElse("empty"),
         blockheader.toLogString(),

diff --git a/.../main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java b/.../main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java
@@ -14,6 +14,7 @@
  */
 package org.hyperledger.besu.consensus.merge.blockcreation;
 
+import static org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator.ForkchoiceResult.Status.IGNORE_UPDATE_TO_OLD_HEAD;
 import static org.hyperledger.besu.consensus.merge.blockcreation.MergeMiningCoordinator.ForkchoiceResult.Status.VALID;
 
 import org.hyperledger.besu.datatypes.Address;
@@ -69,7 +70,8 @@ class ForkchoiceResult {
     public enum Status {
       VALID,
       INVALID,
-      INVALID_PAYLOAD_ATTRIBUTES
+      INVALID_PAYLOAD_ATTRIBUTES,
+      IGNORE_UPDATE_TO_OLD_HEAD
     }
 
     private final Status status;
@@ -101,6 +103,15 @@ public static ForkchoiceResult withFailure(
           latestValid);
     }
 
+    public static ForkchoiceResult withIgnoreUpdateToOldHead(final BlockHeader oldHead) {
+      return new ForkchoiceResult(
+          IGNORE_UPDATE_TO_OLD_HEAD,
+          Optional.empty(),
+          Optional.empty(),
+          Optional.empty(),
+          Optional.of(oldHead.getHash()));
+    }
+
     public static ForkchoiceResult withResult(
         final Optional<BlockHeader> newFinalized, final Optional<BlockHeader> newHead) {
       return new ForkchoiceResult(VALID, Optional.empty(), newFinalized, newHead, Optional.empty());

diff --git a/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java
@@ -467,6 +467,37 @@ public void invalidPayloadShouldReturnErrorAndUpdateForkchoiceState() {
     verify(mergeContext).setSafeBlock(lastFinalizedHeader);
   }
 
+  @Test
+  public void forkchoiceUpdateShouldIgnoreAncestorOfChainHead() {
+    BlockHeader terminalHeader = terminalPowBlock();
+    sendNewPayloadAndForkchoiceUpdate(
+        new Block(terminalHeader, BlockBody.empty()), Optional.empty(), Hash.ZERO);
+
+    BlockHeader parentHeader = nextBlockHeader(terminalHeader);
+    Block parent = new Block(parentHeader, BlockBody.empty());
+    sendNewPayloadAndForkchoiceUpdate(parent, Optional.empty(), terminalHeader.getHash());
+
+    BlockHeader childHeader = nextBlockHeader(parentHeader);
+    Block child = new Block(childHeader, BlockBody.empty());
+    sendNewPayloadAndForkchoiceUpdate(child, Optional.empty(), parent.getHash());
+
+    ForkchoiceResult res =
+        coordinator.updateForkChoice(
+            parentHeader,
+            Hash.ZERO,
+            terminalHeader.getHash(),
+            Optional.of(
+                new PayloadAttributes(parentHeader.getTimestamp() + 1, Hash.ZERO, Address.ZERO)));
+
+    assertThat(res.getStatus()).isEqualTo(ForkchoiceResult.Status.IGNORE_UPDATE_TO_OLD_HEAD);
+    assertThat(res.getNewHead().isEmpty()).isTrue();
+    assertThat(res.getLatestValid().isPresent()).isTrue();
+    assertThat(res.getLatestValid().get()).isEqualTo(parentHeader.getHash());
+    assertThat(res.getErrorMessage().isEmpty()).isTrue();
+
+    verify(blockchain, never()).rewindToBlock(any());
+  }
+
   private void sendNewPayloadAndForkchoiceUpdate(
       final Block block, final Optional<BlockHeader> finalizedHeader, final Hash safeHash) {
 

diff --git a/...yperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java b/...yperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdated.java
@@ -142,7 +142,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
                         payloadAttributes.getSuggestedFeeRecipient())));
 
     if (!result.isValid()) {
-      return handleForkchoiceError(requestId, result);
+      return handleNonValidForkchoiceUpdate(requestId, result);
     }
 
     // begin preparing a block if we have a non-empty payload attributes param
@@ -172,7 +172,7 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
             Optional.empty()));
   }
 
-  private JsonRpcResponse handleForkchoiceError(
+  private JsonRpcResponse handleNonValidForkchoiceUpdate(
       final Object requestId, final ForkchoiceResult result) {
     JsonRpcResponse response;
 
@@ -184,14 +184,18 @@ private JsonRpcResponse handleForkchoiceError(
             new JsonRpcSuccessResponse(
                 requestId,
                 new EngineUpdateForkchoiceResult(
-                    INVALID,
-                    latestValid.isPresent() ? latestValid.get() : null,
-                    null,
-                    result.getErrorMessage()));
+                    INVALID, latestValid.orElse(null), null, result.getErrorMessage()));
         break;
       case INVALID_PAYLOAD_ATTRIBUTES:
         response = new JsonRpcErrorResponse(requestId, JsonRpcError.INVALID_PAYLOAD_ATTRIBUTES);
         break;
+      case IGNORE_UPDATE_TO_OLD_HEAD:
+        response =
+            new JsonRpcSuccessResponse(
+                requestId,
+                new EngineUpdateForkchoiceResult(
+                    VALID, latestValid.orElse(null), null, result.getErrorMessage()));
+        break;
       default:
         throw new AssertionError(
             "ForkchoiceResult.Status "

diff --git a/...ledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java b/...ledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedTest.java
@@ -20,6 +20,7 @@
 import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.VALID;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -339,6 +340,41 @@ public void shouldReturnInvalidForkchoiceStateIfSafeBlockIsNotAnAncestorOfNewHea
     assertInvalidForkchoiceState(resp);
   }
 
+  @Test
+  public void shouldIgnoreUpdateToOldHeadAndNotPreparePayload() {
+    BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+
+    when(blockchain.getBlockHeader(any())).thenReturn(Optional.of(mockHeader));
+    when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(mockHeader)).thenReturn(true);
+
+    var ignoreOldHeadUpdateRes = ForkchoiceResult.withIgnoreUpdateToOldHead(mockHeader);
+    when(mergeCoordinator.updateForkChoice(any(), any(), any(), any()))
+        .thenReturn(ignoreOldHeadUpdateRes);
+
+    var payloadParams =
+        new EnginePayloadAttributesParameter(
+            String.valueOf(System.currentTimeMillis()),
+            Bytes32.fromHexStringLenient("0xDEADBEEF").toHexString(),
+            Address.ECREC.toString());
+
+    var resp =
+        (JsonRpcSuccessResponse)
+            resp(
+                new EngineForkchoiceUpdatedParameter(
+                    mockHeader.getBlockHash(), Hash.ZERO, Hash.ZERO),
+                Optional.of(payloadParams));
+
+    var forkchoiceRes = (EngineUpdateForkchoiceResult) resp.getResult();
+
+    verify(mergeCoordinator, never()).preparePayload(any(), any(), any(), any());
+
+    assertThat(forkchoiceRes.getPayloadStatus().getStatus()).isEqualTo(EngineStatus.VALID);
+    assertThat(forkchoiceRes.getPayloadStatus().getError()).isNull();
+    assertThat(forkchoiceRes.getPayloadStatus().getLatestValidHashAsString())
+        .isEqualTo(mockHeader.getHash().toHexString());
+    assertThat(forkchoiceRes.getPayloadId()).isNull();
+  }
+
   private EngineUpdateForkchoiceResult assertSuccessWithPayloadForForkchoiceResult(
       final Optional<EnginePayloadAttributesParameter> payloadParam,
       final ForkchoiceResult forkchoiceResult,