Backwards Sync now adding blocks to BadBlocksManager

Signed-off-by: Jiri Peinlich <jiri.peinlich@gmail.com>

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

@@ -508,4 +508,14 @@ private boolean isPayloadAttributesValid(
   interface MergeBlockCreatorFactory {
     MergeBlockCreator forParams(BlockHeader header, Optional<Address> feeRecipient);
   }
+
+  @Override
+  public boolean isBadBlock(final Hash blockHash) {
+    final BadBlockManager badBlocksManager =
+        protocolSchedule
+            .getByBlockNumber(protocolContext.getBlockchain().getChainHeadBlockNumber())
+            .getBadBlocksManager();
+    return badBlocksManager.getBadBlock(blockHash).isPresent()
+        || badBlocksManager.getBadHash(blockHash).isPresent();
+  }
 }

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

@@ -61,6 +61,8 @@ ForkchoiceResult updateForkChoice(
 
   Optional<BlockHeader> getOrSyncHeaderByHash(Hash blockHash, Hash finalizedBlockHash);
 
+  boolean isBadBlock(Hash blockHash);
+
   class ForkchoiceResult {
     public enum Status {
       VALID,

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

@@ -193,4 +193,9 @@ public boolean isMiningBeforeMerge() {
   public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader newBlock) {
     return mergeCoordinator.isDescendantOf(ancestorBlock, newBlock);
   }
+
+  @Override
+  public boolean isBadBlock(final Hash blockHash) {
+    return mergeCoordinator.isBadBlock(blockHash);
+  }
 }

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

@@ -134,23 +134,22 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
               "Computed block hash %s does not match block hash parameter %s",
               newBlockHeader.getBlockHash(), blockParam.getBlockHash()));
       return respondWith(reqId, null, INVALID_BLOCK_HASH);
-    } else {
-      // do we already have this payload
-      if (protocolContext
-          .getBlockchain()
-          .getBlockByHash(newBlockHeader.getBlockHash())
-          .isPresent()) {
-        LOG.debug("block already present");
-        return respondWith(reqId, blockParam.getBlockHash(), VALID);
-      }
-
-      Optional<BlockHeader> parentHeader =
-          protocolContext.getBlockchain().getBlockHeader(blockParam.getParentHash());
-      if (parentHeader.isPresent()
-          && (blockParam.getTimestamp() <= parentHeader.get().getTimestamp())) {
-        LOG.info("method parameter timestamp not greater than parent");
-        return new JsonRpcErrorResponse(reqId, JsonRpcError.INVALID_PARAMS);
-      }
+    }
+    // do we already have this payload
+    if (protocolContext.getBlockchain().getBlockByHash(newBlockHeader.getBlockHash()).isPresent()) {
+      LOG.debug("block already present");
+      return respondWith(reqId, blockParam.getBlockHash(), VALID);
+    }
+    if (mergeCoordinator.isBadBlock(blockParam.getParentHash())) {
+      return respondWith(reqId, Hash.ZERO, INVALID);
+    }
+
+    Optional<BlockHeader> parentHeader =
+        protocolContext.getBlockchain().getBlockHeader(blockParam.getParentHash());
+    if (parentHeader.isPresent()
+        && (blockParam.getTimestamp() <= parentHeader.get().getTimestamp())) {
+      LOG.info("method parameter timestamp not greater than parent");
+      return new JsonRpcErrorResponse(reqId, JsonRpcError.INVALID_PARAMS);
     }
 
     final var block =

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

@@ -282,6 +282,19 @@ public void shouldRespondWithInvalidIfExtraDataIsNull() {
     assertThat(res.getError()).isEqualTo("Field extraData must not be null");
   }
 
+  @Test
+  public void shouldReturnInvalidWhenBadBlock() {
+    when(mergeCoordinator.isBadBlock(any(Hash.class))).thenReturn(true);
+    BlockHeader mockHeader = new BlockHeaderTestFixture().baseFeePerGas(Wei.ONE).buildHeader();
+
+    var resp = resp(mockPayload(mockHeader, Collections.emptyList()));
+
+    EnginePayloadStatusResult res = fromSuccessResp(resp);
+    assertThat(res.getLatestValidHash()).contains(Hash.ZERO);
+    assertThat(res.getStatusAsString()).isEqualTo(INVALID.name());
+    assertThat(res.getError()).isNull();
+  }
+
   private JsonRpcResponse resp(final EnginePayloadParameter payload) {
     return method.response(
         new JsonRpcRequestContext(

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

@@ -16,6 +16,7 @@
 
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.core.Block;
+import org.hyperledger.besu.ethereum.core.BlockHeader;
 
 import java.util.Collection;
 import java.util.Optional;
@@ -27,6 +28,8 @@ public class BadBlockManager {
 
   private final Cache<Hash, Block> badBlocks =
       CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
+  private final Cache<Hash, BlockHeader> badHeaders =
+      CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();
 
   /**
    * Add a new invalid block.
@@ -57,4 +60,12 @@ public Collection<Block> getBadBlocks() {
   public Optional<Block> getBadBlock(final Hash hash) {
     return Optional.ofNullable(badBlocks.getIfPresent(hash));
   }
+
+  public void addBadHeader(final BlockHeader header) {
+    badHeaders.put(header.getHash(), header);
+  }
+
+  public Optional<BlockHeader> getBadHash(final Hash blockHash) {
+    return Optional.ofNullable(badHeaders.getIfPresent(blockHash));
+  }
 }

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

@@ -19,6 +19,7 @@
 import static org.slf4j.LoggerFactory.getLogger;
 
 import org.hyperledger.besu.datatypes.Hash;
+import org.hyperledger.besu.ethereum.chain.BadBlockManager;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockHeaderFunctions;
@@ -188,4 +189,17 @@ public synchronized void addNewHash(final Hash newBlockHash) {
   public synchronized Optional<Hash> getFirstHashToAppend() {
     return Optional.ofNullable(hashesToAppend.poll());
   }
+
+  public void addBadChainToManager(final BadBlockManager badBlocksManager, final Hash hash) {
+    final Optional<Hash> ancestor = chainStorage.get(hash);
+    while (ancestor.isPresent()) {
+      final Optional<Block> block = blocks.get(ancestor.get());
+      if (block.isPresent()) {
+        badBlocksManager.addBadBlock(block.get());
+      } else {
+        final Optional<BlockHeader> blockHeader = headers.get(ancestor.get());
+        blockHeader.ifPresent(badBlocksManager::addBadHeader);
+      }
+    }
+  }
 }

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

@@ -21,6 +21,7 @@
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.BlockValidator;
 import org.hyperledger.besu.ethereum.ProtocolContext;
+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.BlockHeader;
@@ -244,13 +245,28 @@ protected Void saveBlock(final Block block) {
                 block,
                 HeaderValidationMode.FULL,
                 HeaderValidationMode.NONE);
-    optResult.blockProcessingOutputs.ifPresent(
-        result -> {
-          traceLambda(LOG, "Block {} was validated, going to import it", block::toLogString);
-          result.worldState.persist(block.getHeader());
-          this.getProtocolContext().getBlockchain().appendBlock(block, result.receipts);
-          possiblyMoveHead(block);
-        });
+    if (optResult.blockProcessingOutputs.isPresent()) {
+      traceLambda(LOG, "Block {} was validated, going to import it", block::toLogString);
+      optResult.blockProcessingOutputs.get().worldState.persist(block.getHeader());
+      this.getProtocolContext()
+          .getBlockchain()
+          .appendBlock(block, optResult.blockProcessingOutputs.get().receipts);
+      possiblyMoveHead(block);
+    } else {
+      final BadBlockManager badBlocksManager =
+          protocolSchedule
+              .getByBlockNumber(getProtocolContext().getBlockchain().getChainHeadBlockNumber())
+              .getBadBlocksManager();
+      badBlocksManager.addBadBlock(block);
+      getBackwardChain().addBadChainToManager(badBlocksManager, block.getHash());
+      throw new BackwardSyncException(
+          "Cannot save block "
+              + block.getHash()
+              + " because of "
+              + optResult.errorMessage.orElseThrow());
+    }
+    optResult.blockProcessingOutputs.ifPresent(result -> {});
+
     return null;
   }
 

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

@@ -63,32 +63,33 @@ public CompletableFuture<Void> pickNextStep() {
     }
     runFinalizedSuccessionRule(
         context.getProtocolContext().getBlockchain(), context.findMaybeFinalized());
-    final Optional<BlockHeader> firstAncestorHeader =
+    final Optional<BlockHeader> possibleFirstAncestorHeader =
         context.getBackwardChain().getFirstAncestorHeader();
-    if (firstAncestorHeader.isEmpty()) {
+    if (possibleFirstAncestorHeader.isEmpty()) {
       this.finished = true;
       LOG.info("The Backward sync is done...");
       context.getBackwardChain().clear();
       return CompletableFuture.completedFuture(null);
     }
     final MutableBlockchain blockchain = context.getProtocolContext().getBlockchain();
-    if (blockchain.contains(firstAncestorHeader.get().getHash())) {
+    final BlockHeader firstAncestorHeader = possibleFirstAncestorHeader.get();
+    if (blockchain.contains(firstAncestorHeader.getHash())) {
       return executeProcessKnownAncestors();
     }
-    if (blockchain.getChainHead().getHeight() > firstAncestorHeader.get().getNumber()) {
+    if (blockchain.getChainHead().getHeight() > firstAncestorHeader.getNumber()) {
       debugLambda(
           LOG,
           "Backward reached below previous head {} : {}",
           () -> blockchain.getChainHead().toLogString(),
-          () -> firstAncestorHeader.get().toLogString());
+          firstAncestorHeader::toLogString);
     }
 
-    if (finalBlockConfirmation.finalHeaderReached(firstAncestorHeader.get())) {
+    if (finalBlockConfirmation.finalHeaderReached(firstAncestorHeader)) {
       LOG.info("Backward sync reached final header, starting Forward sync");
       return executeForwardAsync();
     }
 
-    return executeBackwardAsync(firstAncestorHeader.get());
+    return executeBackwardAsync(firstAncestorHeader);
   }
 
   @VisibleForTesting

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java

@@ -23,6 +23,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
@@ -31,6 +32,7 @@
 import org.hyperledger.besu.ethereum.BlockValidator;
 import org.hyperledger.besu.ethereum.BlockValidator.Result;
 import org.hyperledger.besu.ethereum.ProtocolContext;
+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.BlockDataGenerator;
@@ -59,6 +61,7 @@
 import org.junit.runner.RunWith;
 import org.mockito.Answers;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.Spy;
 import org.mockito.junit.MockitoJUnitRunner;
 
@@ -324,4 +327,22 @@ public void shouldProcessExceptionsCorrectly() {
         new RuntimeException(new BackwardSyncException("shouldNotThrow", true)));
     context.processException(new RuntimeException(new RuntimeException("shouldNotThrow")));
   }
+
+  @Test
+  public void makeSureWeRememberBadBlocks() {
+    Block block = Mockito.mock(Block.class);
+    when(block.getHash()).thenReturn(Hash.ZERO);
+    doReturn(blockValidator).when(context).getBlockValidatorForBlock(any());
+    Result result = new Result("custom error");
+    doReturn(result).when(blockValidator).validateAndProcessBlock(any(), any(), any(), any());
+
+    final BadBlockManager manager = mock(BadBlockManager.class);
+    doReturn(manager).when(mockProtocolSpec).getBadBlocksManager();
+
+    assertThatThrownBy(() -> context.saveBlock(block))
+        .isInstanceOf(BackwardSyncException.class)
+        .hasMessageContaining("custom error");
+
+    Mockito.verify(manager).addBadBlock(block);
+  }
 }