Stop adding bad proposed block to bad block manager (#5082)

* Add flag to distinguish when bad block is a proposed block

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add flag to distinguish when bad block is a proposed block

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* javadoc

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add shouldRecordBadBlock flag to stop adding proposed bad blocks to the bad block manager

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add overload to validateAndProcessBlock with shouldRecordBadBlock flag

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add unit test to ensure flag is being used to add blocks to bad block manager

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add unit test to ensure we don't add our own proposed bad block to the bad block manager

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Add debug log when badBlock is not added

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Undo change done in the first approach

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Improve log

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Improve test names

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Improve javadoc of validateProposedBlock method

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Remove method from interface

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

* Change validateProposedBlock access modifier to private

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

---------

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>

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

@@ -270,7 +270,7 @@ public PayloadIdentifier preparePayload(
             .createBlock(Optional.of(Collections.emptyList()), prevRandao, timestamp, withdrawals)
             .getBlock();
 
-    BlockProcessingResult result = validateBlock(emptyBlock);
+    BlockProcessingResult result = validateProposedBlock(emptyBlock);
     if (result.isSuccessful()) {
       mergeContext.putPayloadById(
           payloadIdentifier, new BlockWithReceipts(emptyBlock, result.getReceipts()));
@@ -401,7 +401,7 @@ private void evaluateNewBlock(
 
     if (isBlockCreationCancelled(payloadIdentifier)) return;
 
-    final var resultBest = validateBlock(bestBlock);
+    final var resultBest = validateProposedBlock(bestBlock);
     if (resultBest.isSuccessful()) {
 
       if (isBlockCreationCancelled(payloadIdentifier)) return;
@@ -494,6 +494,22 @@ public BlockProcessingResult validateBlock(final Block block) {
     return validationResult;
   }
 
+  private BlockProcessingResult validateProposedBlock(final Block block) {
+    final var validationResult =
+        protocolSchedule
+            .getByBlockHeader(block.getHeader())
+            .getBlockValidator()
+            .validateAndProcessBlock(
+                protocolContext,
+                block,
+                HeaderValidationMode.FULL,
+                HeaderValidationMode.NONE,
+                false,
+                false);
+
+    return validationResult;
+  }
+
   @Override
   public BlockProcessingResult rememberBlock(final Block block) {
     debugLambda(LOG, "Remember block {}", block::toLogString);

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

@@ -317,6 +317,26 @@ public void exceptionDuringBuildingBlockShouldNotBeInvalid()
     verify(willThrow, never()).addBadBlock(any(), any());
   }
 
+  @Test
+  public void shouldNotRecordProposedBadBlockToBadBlockManager()
+      throws ExecutionException, InterruptedException {
+    // set up invalid parent to simulate one of the many conditions that can cause a block
+    // validation to fail
+    final BlockHeader invalidParentHeader = new BlockHeaderTestFixture().buildHeader();
+
+    blockCreationTask.get();
+
+    coordinator.preparePayload(
+        invalidParentHeader,
+        System.currentTimeMillis() / 1000,
+        Bytes32.ZERO,
+        suggestedFeeRecipient,
+        Optional.empty());
+
+    verify(badBlockManager, never()).addBadBlock(any(), any());
+    assertThat(badBlockManager.getBadBlocks().size()).isEqualTo(0);
+  }
+
   @Test
   public void shouldContinueBuildingBlocksUntilFinalizeIsCalled()
       throws InterruptedException, ExecutionException {

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/BlockValidator.java

@@ -35,6 +35,14 @@ BlockProcessingResult validateAndProcessBlock(
       final HeaderValidationMode ommerValidationMode,
       final boolean shouldPersist);
 
+  BlockProcessingResult validateAndProcessBlock(
+      final ProtocolContext context,
+      final Block block,
+      final HeaderValidationMode headerValidationMode,
+      final HeaderValidationMode ommerValidationMode,
+      final boolean shouldPersist,
+      final boolean shouldRecordBadBlock);
+
   boolean fastBlockValidation(
       final ProtocolContext context,
       final Block block,

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java

@@ -71,7 +71,8 @@ public BlockProcessingResult validateAndProcessBlock(
       final Block block,
       final HeaderValidationMode headerValidationMode,
       final HeaderValidationMode ommerValidationMode) {
-    return validateAndProcessBlock(context, block, headerValidationMode, ommerValidationMode, true);
+    return validateAndProcessBlock(
+        context, block, headerValidationMode, ommerValidationMode, true, true);
   }
 
   @Override
@@ -81,6 +82,18 @@ public BlockProcessingResult validateAndProcessBlock(
       final HeaderValidationMode headerValidationMode,
       final HeaderValidationMode ommerValidationMode,
       final boolean shouldPersist) {
+    return validateAndProcessBlock(
+        context, block, headerValidationMode, ommerValidationMode, shouldPersist, true);
+  }
+
+  @Override
+  public BlockProcessingResult validateAndProcessBlock(
+      final ProtocolContext context,
+      final Block block,
+      final HeaderValidationMode headerValidationMode,
+      final HeaderValidationMode ommerValidationMode,
+      final boolean shouldPersist,
+      final boolean shouldRecordBadBlock) {
 
     final BlockHeader header = block.getHeader();
     final BlockHeader parentHeader;
@@ -93,20 +106,20 @@ public BlockProcessingResult validateAndProcessBlock(
         var retval =
             new BlockProcessingResult(
                 "Parent block with hash " + header.getParentHash() + " not present");
-        handleAndLogImportFailure(block, retval);
+        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
         return retval;
       }
       parentHeader = maybeParentHeader.get();
 
       if (!blockHeaderValidator.validateHeader(
           header, parentHeader, context, headerValidationMode)) {
         var retval = new BlockProcessingResult("header validation rule violated, see logs");
-        handleAndLogImportFailure(block, retval);
+        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
         return retval;
       }
     } catch (StorageException ex) {
       var retval = new BlockProcessingResult(Optional.empty(), ex);
-      handleAndLogImportFailure(block, retval);
+      handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
       return retval;
     }
 
@@ -129,19 +142,19 @@ public BlockProcessingResult validateAndProcessBlock(
                 "Unable to process block because parent world state "
                     + parentHeader.getStateRoot()
                     + " is not available");
-        handleAndLogImportFailure(block, retval);
+        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
         return retval;
       }
       var result = processBlock(context, worldState, block);
       if (result.isFailed()) {
-        handleAndLogImportFailure(block, result);
+        handleAndLogImportFailure(block, result, shouldRecordBadBlock);
         return result;
       } else {
         List<TransactionReceipt> receipts =
             result.getYield().map(BlockProcessingOutputs::getReceipts).orElse(new ArrayList<>());
         if (!blockBodyValidator.validateBody(
             context, block, receipts, worldState.rootHash(), ommerValidationMode)) {
-          handleAndLogImportFailure(block, result);
+          handleAndLogImportFailure(block, result, shouldRecordBadBlock);
           return new BlockProcessingResult("failed to validate output of imported block");
         }
         if (result instanceof GoQuorumBlockProcessingResult) {
@@ -174,19 +187,23 @@ public BlockProcessingResult validateAndProcessBlock(
               synchronizer -> synchronizer.healWorldState(ex.getMaybeAddress(), ex.getLocation()),
               () ->
                   handleAndLogImportFailure(
-                      block, new BlockProcessingResult(Optional.empty(), ex)));
+                      block,
+                      new BlockProcessingResult(Optional.empty(), ex),
+                      shouldRecordBadBlock));
       return new BlockProcessingResult(Optional.empty(), ex);
     } catch (StorageException ex) {
       var retval = new BlockProcessingResult(Optional.empty(), ex);
-      handleAndLogImportFailure(block, retval);
+      handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
       return retval;
     } catch (Exception ex) {
       throw new RuntimeException(ex);
     }
   }
 
   private void handleAndLogImportFailure(
-      final Block invalidBlock, final BlockValidationResult result) {
+      final Block invalidBlock,
+      final BlockValidationResult result,
+      final boolean shouldRecordBadBlock) {
     if (result.causedBy().isPresent()) {
       LOG.info(
           "Invalid block {}: {}, caused by {}",
@@ -201,7 +218,11 @@ private void handleAndLogImportFailure(
         LOG.info("Invalid block {}", invalidBlock.toLogString());
       }
     }
-    badBlockManager.addBadBlock(invalidBlock, result.causedBy());
+    if (shouldRecordBadBlock) {
+      badBlockManager.addBadBlock(invalidBlock, result.causedBy());
+    } else {
+      LOG.debug("Invalid block {} not added to badBlockManager ", invalidBlock.toLogString());
+    }
   }
 
   /**

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/MainnetBlockValidatorTest.java

@@ -223,4 +223,48 @@ public void shouldReturnBadBlockBasedOnTheHash() {
         HeaderValidationMode.DETACHED_ONLY);
     assertThat(badBlockManager.getBadBlock(badBlock.getHash())).containsSame(badBlock);
   }
+
+  @Test
+  public void when_shouldRecordBadBlockIsFalse_Expect_BlockNotAddedToBadBlockManager() {
+    when(blockchain.getBlockHeader(any(Hash.class)))
+        .thenReturn(Optional.of(new BlockHeaderTestFixture().buildHeader()));
+    mainnetBlockValidator.validateAndProcessBlock(
+        protocolContext,
+        badBlock,
+        HeaderValidationMode.DETACHED_ONLY,
+        HeaderValidationMode.DETACHED_ONLY,
+        false,
+        false);
+
+    assertThat(badBlockManager.getBadBlocks().size()).isEqualTo(0);
+  }
+
+  @Test
+  public void when_shouldRecordBadBlockIsTrue_Expect_BlockAddedToBadBlockManager() {
+    when(blockchain.getBlockHeader(any(Hash.class)))
+        .thenReturn(Optional.of(new BlockHeaderTestFixture().buildHeader()));
+    mainnetBlockValidator.validateAndProcessBlock(
+        protocolContext,
+        badBlock,
+        HeaderValidationMode.DETACHED_ONLY,
+        HeaderValidationMode.DETACHED_ONLY,
+        false,
+        true);
+
+    assertThat(badBlockManager.getBadBlocks().size()).isEqualTo(1);
+  }
+
+  @Test
+  public void when_shouldRecordBadBlockIsNotSet_Expect_BlockAddedToBadBlockManager() {
+    when(blockchain.getBlockHeader(any(Hash.class)))
+        .thenReturn(Optional.of(new BlockHeaderTestFixture().buildHeader()));
+    mainnetBlockValidator.validateAndProcessBlock(
+        protocolContext,
+        badBlock,
+        HeaderValidationMode.DETACHED_ONLY,
+        HeaderValidationMode.DETACHED_ONLY,
+        false);
+
+    assertThat(badBlockManager.getBadBlocks().size()).isEqualTo(1);
+  }
 }