From 28962b5cb27b686ffd1791221b3a554fe98eb097 Mon Sep 17 00:00:00 2001 From: daniellehrner Date: Thu, 9 Apr 2026 13:29:44 +0200 Subject: [PATCH 1/5] check the bad block manager when receiving a new block from the network. This allows to mark a whole fork invalid and signaling this to the CL at the next fcu Signed-off-by: daniellehrner --- ...oiceUpdatedBadAncestorIntegrationTest.java | 200 ++++++++++++++++++ .../eth/sync/BlockPropagationManager.java | 20 ++ .../AbstractBlockPropagationManagerTest.java | 131 ++++++++++++ 3 files changed, 351 insertions(+) create mode 100644 ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java new file mode 100644 index 00000000000..7e9b30eef91 --- /dev/null +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java @@ -0,0 +1,200 @@ +/* + * Copyright contributors to Besu. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine; + +import static java.util.Collections.emptyList; +import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.datatypes.HardforkId.MainnetHardforkId.AMSTERDAM; +import static org.hyperledger.besu.datatypes.HardforkId.MainnetHardforkId.CANCUN; +import static org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.ExecutionEngineJsonRpcMethod.EngineStatus.INVALID; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.hyperledger.besu.consensus.merge.MergeContext; +import org.hyperledger.besu.consensus.merge.blockcreation.MergeCoordinator; +import org.hyperledger.besu.ethereum.ProtocolContext; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; +import org.hyperledger.besu.ethereum.api.jsonrpc.internal.parameters.EngineForkchoiceUpdatedParameter; +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; +import org.hyperledger.besu.ethereum.chain.BadBlockCause; +import org.hyperledger.besu.ethereum.chain.BadBlockManager; +import org.hyperledger.besu.ethereum.chain.MutableBlockchain; +import org.hyperledger.besu.ethereum.core.Block; +import org.hyperledger.besu.ethereum.core.BlockBody; +import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; +import org.hyperledger.besu.ethereum.core.MiningConfiguration; +import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; +import org.hyperledger.besu.ethereum.eth.sync.backwardsync.BackwardSyncContext; +import org.hyperledger.besu.ethereum.eth.transactions.TransactionPool; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; +import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive; + +import java.util.List; +import java.util.Optional; + +import io.vertx.core.Vertx; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Integration test for the engine_forkchoiceUpdated "invalid block as ancestor" flow. + * + *

Stages exercised, using REAL objects (not mocks) for {@link BadBlockManager}, {@link + * MergeCoordinator}, and {@link EngineForkchoiceUpdatedV3}: + * + *

    + *
  1. A bad block B is registered in {@link BadBlockManager} (simulating what {@link + * org.hyperledger.besu.ethereum.MainnetBlockValidator} does on a failed import). + *
  2. {@link MergeCoordinator#onBadChain(Block, List, List)} is called with B and a descendant D + * — simulating the backward-sync code path that fires on {@code + * BackwardSyncContext.emitBadChainEvent}. This should propagate B's "bad" marking to every + * descendant, and record each descendant's {@code latestValidHash} as B's (valid) parent + * hash. + *
  3. A JSON-RPC {@code engine_forkchoiceUpdated} call is invoked with {@code headBlockHash = D}. + * The {@link + * org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine.AbstractEngineForkchoiceUpdated} + * short-circuit at line 115 should fire, returning {@code INVALID} with {@code + * latestValidHash} equal to B's valid parent hash. + *
+ * + *

The existing unit test {@link + * AbstractEngineForkchoiceUpdatedTest#shouldReturnInvalidWithLatestValidHashOnBadBlock} mocks both + * {@code isBadBlock} and {@code getLatestValidHashOfBadBlock}, so it does not verify that {@code + * MergeCoordinator.onBadChain} actually wires descendants up correctly. This test closes that gap + * end-to-end across the {@code onBadChain → BadBlockManager → fcU response} boundary. + */ +public class EngineForkchoiceUpdatedBadAncestorIntegrationTest { + + private static final Vertx vertx = Vertx.vertx(); + + private final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture(); + + private BadBlockManager badBlockManager; + private MutableBlockchain blockchain; + private MergeCoordinator mergeCoordinator; + private EngineForkchoiceUpdatedV3 method; + + @BeforeEach + public void setUp() { + badBlockManager = new BadBlockManager(); + + // Blockchain only needs to serve the bad block's parent header lookup for + // MergeCoordinator.onBadChain; everything else on the fcU short-circuit path avoids it. + blockchain = mock(MutableBlockchain.class); + + final MergeContext mergeContext = mock(MergeContext.class); + when(mergeContext.as(MergeContext.class)).thenReturn(mergeContext); + + final WorldStateArchive worldStateArchive = mock(WorldStateArchive.class); + + final ProtocolContext protocolContext = + new ProtocolContext.Builder() + .withBlockchain(blockchain) + .withWorldStateArchive(worldStateArchive) + .withConsensusContext(mergeContext) + .withBadBlockManager(badBlockManager) + .build(); + + final ProtocolSchedule protocolSchedule = mock(ProtocolSchedule.class); + // Avoid NPE inside MergeCoordinator's getDefaultGasLimit fallback during construction. + when(protocolSchedule.getChainId()).thenReturn(Optional.empty()); + // AbstractEngineForkchoiceUpdated reads these in its constructor; returning empty means the + // fork-supported check is skipped, which matches a non-Cancun/non-Amsterdam test setup. + when(protocolSchedule.milestoneFor(CANCUN)).thenReturn(Optional.empty()); + when(protocolSchedule.milestoneFor(AMSTERDAM)).thenReturn(Optional.empty()); + + final EthScheduler ethScheduler = mock(EthScheduler.class); + final TransactionPool transactionPool = mock(TransactionPool.class); + final BackwardSyncContext backwardSyncContext = mock(BackwardSyncContext.class); + final MiningConfiguration miningConfiguration = MiningConfiguration.newDefault(); + + mergeCoordinator = + new MergeCoordinator( + protocolContext, + protocolSchedule, + ethScheduler, + transactionPool, + miningConfiguration, + backwardSyncContext); + + method = + new EngineForkchoiceUpdatedV3( + vertx, + protocolSchedule, + protocolContext, + mergeCoordinator, + mock(EngineCallListener.class)); + } + + @Test + public void shouldReturnInvalidForForkchoiceUpdateWhenHeadHasBadAncestor() { + // Chain shape: + // validParent (PoS, difficulty=0, known to the blockchain) + // └── badBlock B (rejected by validator, in BadBlockManager) + // └── descendant D (CL's head — only in BadBlockManager via onBadChain) + final BlockHeader validParent = headerBuilder.number(100L).buildHeader(); + final BlockHeader badHeader = + headerBuilder.number(101L).parentHash(validParent.getHash()).buildHeader(); + final Block badBlock = new Block(badHeader, BlockBody.empty()); + final BlockHeader descendantHeader = + headerBuilder.number(102L).parentHash(badHeader.getHash()).buildHeader(); + + // onBadChain looks up the bad block's parent in the blockchain to compute latestValidHash. + when(blockchain.getBlockHeader(validParent.getHash())).thenReturn(Optional.of(validParent)); + + // Stage 1: Simulate MainnetBlockValidator.handleFailedBlockProcessing — add the bad block + // to the manager directly. (The direct add path intentionally does NOT record a + // latestValidHash for B itself; only the descendants in stage 2 get one.) + badBlockManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("BAL mismatch")); + + // Stage 2: Simulate BackwardSyncContext.emitBadChainEvent firing — MergeCoordinator is + // registered as a BadChainListener in its constructor and receives the descendant list. + mergeCoordinator.onBadChain(badBlock, emptyList(), List.of(descendantHeader)); + + // Sanity check: onBadChain propagated "bad" status and latestValidHash to the descendant. + assertThat(mergeCoordinator.isBadBlock(descendantHeader.getHash())).isTrue(); + assertThat(mergeCoordinator.getLatestValidHashOfBadBlock(descendantHeader.getHash())) + .contains(validParent.getHash()); + + // Stage 3: CL sends engine_forkchoiceUpdated with the descendant as the head — simulating the + // case where the CL reuses a head whose ancestry was poisoned while we were backward-syncing. + final JsonRpcResponse response = + invokeForkchoiceUpdated( + new EngineForkchoiceUpdatedParameter( + descendantHeader.getHash(), validParent.getHash(), validParent.getHash())); + + // The short-circuit at AbstractEngineForkchoiceUpdated.java:115 must return INVALID with + // latestValidHash pointing at the last valid ancestor (B's parent), not SYNCING and not + // VALID, without ever touching the blockchain state. + final EngineUpdateForkchoiceResult forkchoiceResult = + (EngineUpdateForkchoiceResult) ((JsonRpcSuccessResponse) response).getResult(); + assertThat(forkchoiceResult.getPayloadStatus().getStatus()).isEqualTo(INVALID); + assertThat(forkchoiceResult.getPayloadStatus().getLatestValidHashAsString()) + .isEqualTo(validParent.getHash().toHexString()); + assertThat(forkchoiceResult.getPayloadStatus().getError()) + .contains(descendantHeader.getHash() + " is an invalid block"); + assertThat(forkchoiceResult.getPayloadId()).isNull(); + } + + private JsonRpcResponse invokeForkchoiceUpdated(final EngineForkchoiceUpdatedParameter fcu) { + return method.response( + new JsonRpcRequestContext( + new JsonRpcRequest("2.0", "engine_forkchoiceUpdatedV3", new Object[] {fcu}))); + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java index 7fde3a17c7f..61d009b143f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java @@ -337,6 +337,14 @@ private void handleNewBlockFromNetwork(final EthMessage message) { .log(); return; } + if (protocolContext.getBadBlockManager().isBadBlock(block.getHash())) { + LOG.atTrace() + .setMessage("Ignoring known-bad block {} from peer {}") + .addArgument(block::toLogString) + .addArgument(message::getPeer) + .log(); + return; + } importOrSavePendingBlock(block, message.getPeer().nodeId()); } catch (final RLPException e) { @@ -391,6 +399,10 @@ private void handleNewBlockHashesFromNetwork(final EthMessage message) { LOG.trace("New block hash from network {} was already imported", announcedBlock); continue; } + if (protocolContext.getBadBlockManager().isBadBlock(announcedBlock.hash())) { + LOG.trace("New block hash from network {} is a known bad block", announcedBlock); + continue; + } if (processingBlocksManager.addRequestedBlock(announcedBlock.hash())) { newBlocks.add(announcedBlock); } else { @@ -625,6 +637,14 @@ CompletableFuture importOrSavePendingBlock(final Block block, final Bytes .addArgument(block::toLogString) .log(); + if (protocolContext.getBadBlockManager().isBadBlock(block.getHash())) { + LOG.atTrace() + .setMessage("Skipping known-bad block {} in importOrSavePendingBlock") + .addArgument(block::toLogString) + .log(); + return CompletableFuture.completedFuture(block); + } + if (!protocolContext.getBlockchain().contains(block.getHeader().getParentHash())) { // Block isn't connected to local chain, save it to pending blocks collection if (savePendingBlock(block, nodeId)) { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java index b493f0534dd..a5161241a16 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java @@ -19,6 +19,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -29,6 +30,7 @@ import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.ConsensusContext; import org.hyperledger.besu.ethereum.ProtocolContext; +import org.hyperledger.besu.ethereum.chain.BadBlockCause; import org.hyperledger.besu.ethereum.chain.BadBlockManager; import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.chain.MutableBlockchain; @@ -861,6 +863,135 @@ public Object answer(final InvocationOnMock invocation) throws Throwable { verify(ethScheduler, times(1)).scheduleSyncWorkerTask(any(Supplier.class)); } + @Test + public void shouldSkipKnownBadBlockOnNewBlockMessage() { + blockchainUtil.importFirstBlocks(2); + final Block firstBlock = blockchainUtil.getBlock(1); + final BadBlockManager badBlocksManager = protocolContext.getBadBlockManager(); + final Block badBlock = + new BlockDataGenerator() + .block( + BlockDataGenerator.BlockOptions.create() + .setBlockNumber(2) + .setParentHash(firstBlock.getHash()) + .setBlockHeaderFunctions(new MainnetBlockHeaderFunctions())); + + // Pre-populate BadBlockManager, simulating an earlier validation failure. + badBlocksManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("test")); + assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); + + blockPropagationManager.start(); + + final RespondingEthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 0); + final NewBlockMessage newBlockMessage = + NewBlockMessage.create(badBlock, Difficulty.ONE, maxMessageSize); + + EthProtocolManagerTestUtil.broadcastMessage(ethProtocolManager, peer, newBlockMessage); + + // The handler should have short-circuited before entering the import pipeline. + verify(processingBlocksManager, never()).addImportingBlock(badBlock.getHash()); + // BadBlockManager should still contain exactly one entry — we didn't re-add it. + assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); + } + + @Test + public void shouldSkipKnownBadBlockOnNewBlockHashesMessage() { + blockchainUtil.importFirstBlocks(2); + final Block firstBlock = blockchainUtil.getBlock(1); + final BadBlockManager badBlocksManager = protocolContext.getBadBlockManager(); + final Block badBlock = + new BlockDataGenerator() + .block( + BlockDataGenerator.BlockOptions.create() + .setBlockNumber(2) + .setParentHash(firstBlock.getHash()) + .setBlockHeaderFunctions(new MainnetBlockHeaderFunctions())); + + // Pre-populate BadBlockManager, simulating an earlier validation failure. + badBlocksManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("test")); + + blockPropagationManager.start(); + + final RespondingEthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 0); + final NewBlockHashesMessage newBlockHashesMessage = + NewBlockHashesMessage.create( + Collections.singletonList( + new NewBlockHashesMessage.NewBlockHash( + badBlock.getHash(), badBlock.getHeader().getNumber()))); + + EthProtocolManagerTestUtil.broadcastMessage(ethProtocolManager, peer, newBlockHashesMessage); + + // The hash announcement should have been filtered out before requesting the body. + verify(processingBlocksManager, never()).addRequestedBlock(badBlock.getHash()); + // No body request should have been issued to the peer. + assertThat(peer.hasOutstandingRequests()).isFalse(); + } + + @SuppressWarnings("unchecked") + @Test + public void importOrSavePendingBlockShouldSkipKnownBadBlock() { + // Mirrors shouldDetectAndCacheInvalidBlocks but pre-populates BadBlockManager to + // exercise the defensive short-circuit at the top of importOrSavePendingBlock. + final EthScheduler ethScheduler = mock(EthScheduler.class); + when(ethScheduler.scheduleSyncWorkerTask(any(Supplier.class))) + .thenAnswer( + new Answer() { + @Override + public Object answer(final InvocationOnMock invocation) throws Throwable { + return invocation.getArgument(0, Supplier.class).get(); + } + }); + + final EthContext ethContext = + new EthContext( + new EthPeers( + () -> protocolSchedule.getByBlockHeader(blockchain.getChainHeadHeader()), + TestClock.fixed(), + metricsSystem, + EthProtocolConfiguration.DEFAULT_MAX_MESSAGE_SIZE, + Collections.emptyList(), + Bytes.random(64), + 25, + 25, + false, + SyncMode.SNAP, + new ForkIdManager(blockchain, Collections.emptyList(), Collections.emptyList())), + new EthMessages(), + ethScheduler, + null); + final BlockPropagationManager blockPropagationManager = + new BlockPropagationManager( + syncConfig, + protocolSchedule, + protocolContext, + ethContext, + syncState, + pendingBlocksManager, + metricsSystem, + blockBroadcaster); + + blockchainUtil.importFirstBlocks(2); + final Block firstBlock = blockchainUtil.getBlock(1); + final BadBlockManager badBlocksManager = protocolContext.getBadBlockManager(); + final Block badBlock = + new BlockDataGenerator() + .block( + BlockDataGenerator.BlockOptions.create() + .setBlockNumber(2) + .setParentHash(firstBlock.getHash()) + .setBlockHeaderFunctions(new MainnetBlockHeaderFunctions())); + + // Pre-populate BadBlockManager, simulating an earlier validation failure. + badBlocksManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("test")); + assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); + + blockPropagationManager.importOrSavePendingBlock(badBlock, NODE_ID_1); + + // The defensive check should have short-circuited before scheduling validation. + verify(ethScheduler, never()).scheduleSyncWorkerTask(any(Supplier.class)); + assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); + } + @Test public void shouldTryWithAnotherPeerWhenFailedDownloadingBlock() { blockchainUtil.importFirstBlocks(2); From ca65d087890f8ef711e8d2643b0bde467855e9b6 Mon Sep 17 00:00:00 2001 From: daniellehrner Date: Fri, 10 Apr 2026 10:40:22 +0200 Subject: [PATCH 2/5] changed log level, improved comments in tests Signed-off-by: daniellehrner --- ...oiceUpdatedBadAncestorIntegrationTest.java | 31 ++++++------------- .../eth/sync/BlockPropagationManager.java | 4 +-- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java index 7e9b30eef91..04ae0d93741 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java @@ -55,29 +55,18 @@ /** * Integration test for the engine_forkchoiceUpdated "invalid block as ancestor" flow. * - *

Stages exercised, using REAL objects (not mocks) for {@link BadBlockManager}, {@link - * MergeCoordinator}, and {@link EngineForkchoiceUpdatedV3}: - * *

    *
  1. A bad block B is registered in {@link BadBlockManager} (simulating what {@link * org.hyperledger.besu.ethereum.MainnetBlockValidator} does on a failed import). *
  2. {@link MergeCoordinator#onBadChain(Block, List, List)} is called with B and a descendant D - * — simulating the backward-sync code path that fires on {@code + * simulating the backward-sync code path that fires on {@code * BackwardSyncContext.emitBadChainEvent}. This should propagate B's "bad" marking to every * descendant, and record each descendant's {@code latestValidHash} as B's (valid) parent * hash. *
  3. A JSON-RPC {@code engine_forkchoiceUpdated} call is invoked with {@code headBlockHash = D}. - * The {@link - * org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine.AbstractEngineForkchoiceUpdated} - * short-circuit at line 115 should fire, returning {@code INVALID} with {@code - * latestValidHash} equal to B's valid parent hash. + * The that should return {@code INVALID} with {@code latestValidHash} equal to B's valid + * parent hash. *
- * - *

The existing unit test {@link - * AbstractEngineForkchoiceUpdatedTest#shouldReturnInvalidWithLatestValidHashOnBadBlock} mocks both - * {@code isBadBlock} and {@code getLatestValidHashOfBadBlock}, so it does not verify that {@code - * MergeCoordinator.onBadChain} actually wires descendants up correctly. This test closes that gap - * end-to-end across the {@code onBadChain → BadBlockManager → fcU response} boundary. */ public class EngineForkchoiceUpdatedBadAncestorIntegrationTest { @@ -112,10 +101,8 @@ public void setUp() { .build(); final ProtocolSchedule protocolSchedule = mock(ProtocolSchedule.class); - // Avoid NPE inside MergeCoordinator's getDefaultGasLimit fallback during construction. + when(protocolSchedule.getChainId()).thenReturn(Optional.empty()); - // AbstractEngineForkchoiceUpdated reads these in its constructor; returning empty means the - // fork-supported check is skipped, which matches a non-Cancun/non-Amsterdam test setup. when(protocolSchedule.milestoneFor(CANCUN)).thenReturn(Optional.empty()); when(protocolSchedule.milestoneFor(AMSTERDAM)).thenReturn(Optional.empty()); @@ -158,28 +145,28 @@ public void shouldReturnInvalidForForkchoiceUpdateWhenHeadHasBadAncestor() { // onBadChain looks up the bad block's parent in the blockchain to compute latestValidHash. when(blockchain.getBlockHeader(validParent.getHash())).thenReturn(Optional.of(validParent)); - // Stage 1: Simulate MainnetBlockValidator.handleFailedBlockProcessing — add the bad block + // Simulate MainnetBlockValidator.handleFailedBlockProcessing — add the bad block // to the manager directly. (The direct add path intentionally does NOT record a // latestValidHash for B itself; only the descendants in stage 2 get one.) badBlockManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("BAL mismatch")); - // Stage 2: Simulate BackwardSyncContext.emitBadChainEvent firing — MergeCoordinator is + // Simulate BackwardSyncContext.emitBadChainEvent firing — MergeCoordinator is // registered as a BadChainListener in its constructor and receives the descendant list. mergeCoordinator.onBadChain(badBlock, emptyList(), List.of(descendantHeader)); - // Sanity check: onBadChain propagated "bad" status and latestValidHash to the descendant. + // onBadChain propagated "bad" status and latestValidHash to the descendant. assertThat(mergeCoordinator.isBadBlock(descendantHeader.getHash())).isTrue(); assertThat(mergeCoordinator.getLatestValidHashOfBadBlock(descendantHeader.getHash())) .contains(validParent.getHash()); - // Stage 3: CL sends engine_forkchoiceUpdated with the descendant as the head — simulating the + // CL sends engine_forkchoiceUpdated with the descendant as the head, simulating the // case where the CL reuses a head whose ancestry was poisoned while we were backward-syncing. final JsonRpcResponse response = invokeForkchoiceUpdated( new EngineForkchoiceUpdatedParameter( descendantHeader.getHash(), validParent.getHash(), validParent.getHash())); - // The short-circuit at AbstractEngineForkchoiceUpdated.java:115 must return INVALID with + // Must return INVALID with // latestValidHash pointing at the last valid ancestor (B's parent), not SYNCING and not // VALID, without ever touching the blockchain state. final EngineUpdateForkchoiceResult forkchoiceResult = diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java index 61d009b143f..8548e44a8d0 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java @@ -298,7 +298,7 @@ private void handleNewBlockFromNetwork(final EthMessage message) { final NewBlockMessage newBlockMessage = NewBlockMessage.readFrom(message.getData()); try { final Block block = newBlockMessage.block(protocolSchedule); - LOG.atTrace() + LOG.atWarn() .setMessage("New block from network {} from peer {}. Current status {}") .addArgument(block::toLogString) .addArgument(message::getPeer) @@ -400,7 +400,7 @@ private void handleNewBlockHashesFromNetwork(final EthMessage message) { continue; } if (protocolContext.getBadBlockManager().isBadBlock(announcedBlock.hash())) { - LOG.trace("New block hash from network {} is a known bad block", announcedBlock); + LOG.warn("New block hash from network {} is a known bad block", announcedBlock); continue; } if (processingBlocksManager.addRequestedBlock(announcedBlock.hash())) { From e8084f9ccafd527b962184e5d629a82279b868bf Mon Sep 17 00:00:00 2001 From: daniellehrner Date: Fri, 10 Apr 2026 10:48:25 +0200 Subject: [PATCH 3/5] changed log level to debug for not important events Signed-off-by: daniellehrner --- .../besu/ethereum/eth/sync/BlockPropagationManager.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java index 8548e44a8d0..f63bfa71f4b 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java @@ -298,7 +298,7 @@ private void handleNewBlockFromNetwork(final EthMessage message) { final NewBlockMessage newBlockMessage = NewBlockMessage.readFrom(message.getData()); try { final Block block = newBlockMessage.block(protocolSchedule); - LOG.atWarn() + LOG.atTrace() .setMessage("New block from network {} from peer {}. Current status {}") .addArgument(block::toLogString) .addArgument(message::getPeer) @@ -338,7 +338,7 @@ private void handleNewBlockFromNetwork(final EthMessage message) { return; } if (protocolContext.getBadBlockManager().isBadBlock(block.getHash())) { - LOG.atTrace() + LOG.atDebug() .setMessage("Ignoring known-bad block {} from peer {}") .addArgument(block::toLogString) .addArgument(message::getPeer) @@ -400,7 +400,7 @@ private void handleNewBlockHashesFromNetwork(final EthMessage message) { continue; } if (protocolContext.getBadBlockManager().isBadBlock(announcedBlock.hash())) { - LOG.warn("New block hash from network {} is a known bad block", announcedBlock); + LOG.debug("New block hash from network {} is a known bad block", announcedBlock); continue; } if (processingBlocksManager.addRequestedBlock(announcedBlock.hash())) { From 14f21de74f7c1dbf9606094e66383fffb7851b8c Mon Sep 17 00:00:00 2001 From: daniellehrner Date: Fri, 10 Apr 2026 11:09:44 +0200 Subject: [PATCH 4/5] addressed pr comments Signed-off-by: daniellehrner --- ...rkchoiceUpdatedBadAncestorIntegrationTest.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java index 04ae0d93741..3a8cdb3c22f 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java @@ -49,6 +49,7 @@ import java.util.Optional; import io.vertx.core.Vertx; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -64,7 +65,9 @@ * descendant, and record each descendant's {@code latestValidHash} as B's (valid) parent * hash. *

  • A JSON-RPC {@code engine_forkchoiceUpdated} call is invoked with {@code headBlockHash = D}. - * The that should return {@code INVALID} with {@code latestValidHash} equal to B's valid + * The bad-block short-circuit in {@link + * org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.engine.AbstractEngineForkchoiceUpdated} + * should fire, returning {@code INVALID} with {@code latestValidHash} equal to B's valid * parent hash. * */ @@ -72,6 +75,11 @@ public class EngineForkchoiceUpdatedBadAncestorIntegrationTest { private static final Vertx vertx = Vertx.vertx(); + @AfterAll + public static void tearDown() { + vertx.close(); + } + private final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture(); private BadBlockManager badBlockManager; @@ -174,8 +182,9 @@ public void shouldReturnInvalidForForkchoiceUpdateWhenHeadHasBadAncestor() { assertThat(forkchoiceResult.getPayloadStatus().getStatus()).isEqualTo(INVALID); assertThat(forkchoiceResult.getPayloadStatus().getLatestValidHashAsString()) .isEqualTo(validParent.getHash().toHexString()); - assertThat(forkchoiceResult.getPayloadStatus().getError()) - .contains(descendantHeader.getHash() + " is an invalid block"); + final String error = forkchoiceResult.getPayloadStatus().getError(); + assertThat(error).contains(descendantHeader.getHash().toString()); + assertThat(error).containsIgnoringCase("invalid"); assertThat(forkchoiceResult.getPayloadId()).isNull(); } From 9d8add1d8da97f4eb96052b0bfde0380c54b77e1 Mon Sep 17 00:00:00 2001 From: daniellehrner Date: Fri, 10 Apr 2026 12:33:06 +0200 Subject: [PATCH 5/5] make tests stricter Signed-off-by: daniellehrner --- ...oiceUpdatedBadAncestorIntegrationTest.java | 39 +++++++++++++++++++ .../AbstractBlockPropagationManagerTest.java | 9 +++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java index 3a8cdb3c22f..b702a58ae6f 100644 --- a/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java +++ b/ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java @@ -24,6 +24,7 @@ import org.hyperledger.besu.consensus.merge.MergeContext; import org.hyperledger.besu.consensus.merge.blockcreation.MergeCoordinator; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequest; import org.hyperledger.besu.ethereum.api.jsonrpc.internal.JsonRpcRequestContext; @@ -188,6 +189,44 @@ public void shouldReturnInvalidForForkchoiceUpdateWhenHeadHasBadAncestor() { assertThat(forkchoiceResult.getPayloadId()).isNull(); } + @Test + public void shouldReturnInvalidWithZeroHashWhenBadBlockParentIsUnknown() { + // Same shape as the previous test, except the bad block's parent is NOT in the blockchain. + // This models a deep backward-sync failure where we never resolved the ancestor — onBadChain + // cannot compute a latestValidHash, so the fcU short-circuit must fall back to Hash.ZERO. + final BlockHeader unknownParent = headerBuilder.number(100L).buildHeader(); + final BlockHeader badHeader = + headerBuilder.number(101L).parentHash(unknownParent.getHash()).buildHeader(); + final Block badBlock = new Block(badHeader, BlockBody.empty()); + final BlockHeader descendantHeader = + headerBuilder.number(102L).parentHash(badHeader.getHash()).buildHeader(); + + // Deliberately leave blockchain.getBlockHeader(unknownParent.getHash()) unstubbed so the + // mock returns Optional.empty() — that's the path that drives maybeLatestValidHash empty. + + badBlockManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("BAL mismatch")); + mergeCoordinator.onBadChain(badBlock, emptyList(), List.of(descendantHeader)); + + // Descendant is still marked bad, but no latestValidHash was recorded for it. + assertThat(mergeCoordinator.isBadBlock(descendantHeader.getHash())).isTrue(); + assertThat(mergeCoordinator.getLatestValidHashOfBadBlock(descendantHeader.getHash())).isEmpty(); + + final JsonRpcResponse response = + invokeForkchoiceUpdated( + new EngineForkchoiceUpdatedParameter( + descendantHeader.getHash(), unknownParent.getHash(), unknownParent.getHash())); + + final EngineUpdateForkchoiceResult forkchoiceResult = + (EngineUpdateForkchoiceResult) ((JsonRpcSuccessResponse) response).getResult(); + assertThat(forkchoiceResult.getPayloadStatus().getStatus()).isEqualTo(INVALID); + assertThat(forkchoiceResult.getPayloadStatus().getLatestValidHashAsString()) + .isEqualTo(Hash.ZERO.toHexString()); + final String error = forkchoiceResult.getPayloadStatus().getError(); + assertThat(error).contains(descendantHeader.getHash().toString()); + assertThat(error).containsIgnoringCase("invalid"); + assertThat(forkchoiceResult.getPayloadId()).isNull(); + } + private JsonRpcResponse invokeForkchoiceUpdated(final EngineForkchoiceUpdatedParameter fcu) { return method.response( new JsonRpcRequestContext( diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java index a5161241a16..8cef5af7642 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java @@ -880,7 +880,8 @@ public void shouldSkipKnownBadBlockOnNewBlockMessage() { badBlocksManager.addBadBlock(badBlock, BadBlockCause.fromValidationFailure("test")); assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); - blockPropagationManager.start(); + final BlockPropagationManager propManager = spy(blockPropagationManager); + propManager.start(); final RespondingEthPeer peer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 0); final NewBlockMessage newBlockMessage = @@ -888,8 +889,10 @@ public void shouldSkipKnownBadBlockOnNewBlockMessage() { EthProtocolManagerTestUtil.broadcastMessage(ethProtocolManager, peer, newBlockMessage); - // The handler should have short-circuited before entering the import pipeline. - verify(processingBlocksManager, never()).addImportingBlock(badBlock.getHash()); + // handleNewBlockFromNetwork must short-circuit before dispatching to importOrSavePendingBlock. + // Checking addImportingBlock alone would also pass via the defensive second check inside + // importOrSavePendingBlock, so verify the dispatch itself never happened. + verify(propManager, never()).importOrSavePendingBlock(any(), any(Bytes.class)); // BadBlockManager should still contain exactly one entry — we didn't re-add it. assertThat(badBlocksManager.getBadBlocks().size()).isEqualTo(1); }