Bugfix - only track intrinsically bad blocks (hyperledger#6678)

Signed-off-by: mbaxter <mbaxter.dev@gmail.com>

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

@@ -105,7 +105,8 @@ public BlockProcessingResult validateAndProcessBlock(
         var retval =
             new BlockProcessingResult(
                 "Parent block with hash " + header.getParentHash() + " not present");
-        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
+        // Blocks should not be marked bad due to missing data
+        handleFailedBlockProcessing(block, retval, false);
         return retval;
       }
       parentHeader = maybeParentHeader.get();
@@ -114,12 +115,13 @@ public BlockProcessingResult validateAndProcessBlock(
           header, parentHeader, context, headerValidationMode)) {
         final String error = String.format("Header validation failed (%s)", headerValidationMode);
         var retval = new BlockProcessingResult(error);
-        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
+        handleFailedBlockProcessing(block, retval, shouldRecordBadBlock);
         return retval;
       }
     } catch (StorageException ex) {
       var retval = new BlockProcessingResult(Optional.empty(), ex);
-      handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
+      // Blocks should not be marked bad due to a local storage failure
+      handleFailedBlockProcessing(block, retval, false);
       return retval;
     }
     try (final var worldState =
@@ -131,20 +133,21 @@ public BlockProcessingResult validateAndProcessBlock(
                 "Unable to process block because parent world state "
                     + parentHeader.getStateRoot()
                     + " is not available");
-        handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
+        // Blocks should not be marked bad due to missing data
+        handleFailedBlockProcessing(block, retval, false);
         return retval;
       }
       var result = processBlock(context, worldState, block);
       if (result.isFailed()) {
-        handleAndLogImportFailure(block, result, shouldRecordBadBlock);
+        handleFailedBlockProcessing(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)) {
           result = new BlockProcessingResult("failed to validate output of imported block");
-          handleAndLogImportFailure(block, result, shouldRecordBadBlock);
+          handleFailedBlockProcessing(block, result, shouldRecordBadBlock);
           return result;
         }
 
@@ -157,53 +160,50 @@ public BlockProcessingResult validateAndProcessBlock(
           .ifPresentOrElse(
               synchronizer -> synchronizer.healWorldState(ex.getMaybeAddress(), ex.getLocation()),
               () ->
-                  handleAndLogImportFailure(
+                  handleFailedBlockProcessing(
                       block,
                       new BlockProcessingResult(Optional.empty(), ex),
-                      shouldRecordBadBlock));
+                      // Do not record bad black due to missing data
+                      false));
       return new BlockProcessingResult(Optional.empty(), ex);
     } catch (StorageException ex) {
       var retval = new BlockProcessingResult(Optional.empty(), ex);
-      handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
+      // Do not record bad block due to a local storage issue
+      handleFailedBlockProcessing(block, retval, false);
       return retval;
     } catch (Exception ex) {
+      // Wrap checked autocloseable exception from try-with-resources
       throw new RuntimeException(ex);
     }
   }
 
-  private void handleAndLogImportFailure(
-      final Block invalidBlock,
+  private void handleFailedBlockProcessing(
+      final Block failedBlock,
       final BlockValidationResult result,
       final boolean shouldRecordBadBlock) {
     if (result.causedBy().isPresent()) {
+      // Block processing failed exceptionally, we cannot assume the block was intrinsically invalid
       LOG.info(
-          "Invalid block {}: {}, caused by {}",
-          invalidBlock.toLogString(),
+          "Failed to process block {}: {}, caused by {}",
+          failedBlock.toLogString(),
           result.errorMessage,
           result.causedBy().get());
       LOG.debug("with stack", result.causedBy().get());
     } else {
       if (result.errorMessage.isPresent()) {
-        LOG.info("Invalid block {}: {}", invalidBlock.toLogString(), result.errorMessage);
+        LOG.info("Invalid block {}: {}", failedBlock.toLogString(), result.errorMessage);
       } else {
-        LOG.info("Invalid block {}", invalidBlock.toLogString());
+        LOG.info("Invalid block {}", failedBlock.toLogString());
+      }
+
+      if (shouldRecordBadBlock) {
+        // Result.errorMessage should not be empty on failure, but add a default to be safe
+        String description = result.errorMessage.orElse("Unknown cause");
+        final BadBlockCause cause = BadBlockCause.fromValidationFailure(description);
+        badBlockManager.addBadBlock(failedBlock, cause);
+      } else {
+        LOG.debug("Invalid block {} not added to badBlockManager ", failedBlock.toLogString());
       }
-    }
-    if (shouldRecordBadBlock) {
-      BadBlockCause cause =
-          result
-              .causedBy()
-              .map(BadBlockCause::fromProcessingError)
-              .orElseGet(
-                  () -> {
-                    // Result.errorMessage should not be empty on failure, but add a default to be
-                    // safe
-                    String description = result.errorMessage.orElse("Unknown cause");
-                    return BadBlockCause.fromValidationFailure(description);
-                  });
-      badBlockManager.addBadBlock(invalidBlock, cause);
-    } else {
-      LOG.debug("Invalid block {} not added to badBlockManager ", invalidBlock.toLogString());
     }
   }
 

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

@@ -17,7 +17,7 @@
 
 import org.hyperledger.besu.ethereum.core.Block;
 
-import java.util.Optional;
+import com.google.common.base.MoreObjects;
 
 public class BadBlockCause {
   public enum BadBlockReason {
@@ -31,29 +31,20 @@ public enum BadBlockReason {
 
   private final BadBlockReason reason;
   private final String description;
-  private final Optional<Throwable> exception;
-
-  public static BadBlockCause fromProcessingError(final Throwable t) {
-    final String description = t.getLocalizedMessage();
-    return new BadBlockCause(
-        BadBlockReason.EXCEPTIONAL_BLOCK_PROCESSING, description, Optional.of(t));
-  }
 
   public static BadBlockCause fromBadAncestorBlock(final Block badAncestor) {
     final String description =
         String.format("Descends from bad block %s", badAncestor.toLogString());
-    return new BadBlockCause(BadBlockReason.DESCENDS_FROM_BAD_BLOCK, description, Optional.empty());
+    return new BadBlockCause(BadBlockReason.DESCENDS_FROM_BAD_BLOCK, description);
   }
 
   public static BadBlockCause fromValidationFailure(final String failureMessage) {
-    return new BadBlockCause(
-        BadBlockReason.SPEC_VALIDATION_FAILURE, failureMessage, Optional.empty());
+    return new BadBlockCause(BadBlockReason.SPEC_VALIDATION_FAILURE, failureMessage);
   }
 
-  private BadBlockCause(BadBlockReason reason, String description, Optional<Throwable> exception) {
+  private BadBlockCause(final BadBlockReason reason, final String description) {
     this.reason = reason;
     this.description = description;
-    this.exception = exception;
   }
 
   public BadBlockReason getReason() {
@@ -64,20 +55,11 @@ public String getDescription() {
     return description;
   }
 
-  public Optional<Throwable> getException() {
-    return exception;
-  }
-
   @Override
   public String toString() {
-    return "BadBlockCause{"
-        + "reason="
-        + reason
-        + ", description='"
-        + description
-        + '\''
-        + ", exception="
-        + exception
-        + '}';
+    return MoreObjects.toStringHelper(this)
+        .add("reason", reason)
+        .add("description", description)
+        .toString();
   }
 }

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

@@ -17,8 +17,6 @@
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
-import org.hyperledger.besu.ethereum.trie.MerkleTrieException;
-import org.hyperledger.besu.plugin.services.exception.StorageException;
 
 import java.util.Collection;
 import java.util.Optional;
@@ -48,10 +46,8 @@ public class BadBlockManager {
    */
   public void addBadBlock(final Block badBlock, final BadBlockCause cause) {
     // TODO(#6301) Expose bad block with cause through BesuEvents
-    if (badBlock != null && !isInternalError(cause)) {
-      LOG.debug("Register bad block {} with cause: {}", badBlock.toLogString(), cause);
-      this.badBlocks.put(badBlock.getHash(), badBlock);
-    }
+    LOG.debug("Register bad block {} with cause: {}", badBlock.toLogString(), cause);
+    this.badBlocks.put(badBlock.getHash(), badBlock);
   }
 
   public void reset() {
@@ -101,13 +97,4 @@ public void addLatestValidHash(final Hash blockHash, final Hash latestValidHash)
   public Optional<Hash> getLatestValidHash(final Hash blockHash) {
     return Optional.ofNullable(latestValidHashes.getIfPresent(blockHash));
   }
-
-  private boolean isInternalError(final BadBlockCause cause) {
-    if (cause.getException().isEmpty()) {
-      return false;
-    }
-    // As new "internal only" types of exception are discovered, add them here.
-    Throwable causedBy = cause.getException().get();
-    return causedBy instanceof StorageException || causedBy instanceof MerkleTrieException;
-  }
 }

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

@@ -19,6 +19,7 @@
 import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
@@ -34,13 +35,19 @@
 import org.hyperledger.besu.ethereum.mainnet.BlockHeaderValidator;
 import org.hyperledger.besu.ethereum.mainnet.BlockProcessor;
 import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode;
+import org.hyperledger.besu.ethereum.trie.MerkleTrieException;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
+import org.hyperledger.besu.plugin.services.exception.StorageException;
 
 import java.util.Collections;
 import java.util.Optional;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 public class MainnetBlockValidatorTest {
 
@@ -61,6 +68,19 @@ public class MainnetBlockValidatorTest {
       new MainnetBlockValidator(
           blockHeaderValidator, blockBodyValidator, blockProcessor, badBlockManager);
 
+  public static Stream<Arguments> getStorageExceptions() {
+    return Stream.of(
+        Arguments.of("StorageException", new StorageException("Database closed")),
+        Arguments.of("MerkleTrieException", new MerkleTrieException("Missing trie node")));
+  }
+
+  public static Stream<Arguments> getBlockProcessingErrors() {
+    return Stream.of(
+        Arguments.of("StorageException", new StorageException("Database closed")),
+        Arguments.of("MerkleTrieException", new MerkleTrieException("Missing trie node")),
+        Arguments.of("RuntimeException", new RuntimeException("Oops")));
+  }
+
   @BeforeEach
   public void setup() {
     chainUtil.importFirstBlocks(4);
@@ -116,7 +136,7 @@ public void validateAndProcessBlock_whenParentBlockNotPresent() {
 
     final String expectedError = "Parent block with hash " + parentHash + " not present";
     assertValidationFailed(result, expectedError);
-    assertBadBlockIsTracked(block);
+    assertNoBadBlocks();
   }
 
   @Test
@@ -173,7 +193,7 @@ public void validateAndProcessBlock_whenParentWorldStateNotAvailable() {
             + blockParent.getHeader().getStateRoot()
             + " is not available";
     assertValidationFailed(result, expectedError);
-    assertBadBlockIsTracked(block);
+    assertNoBadBlocks();
   }
 
   @Test
@@ -193,6 +213,80 @@ public void validateAndProcessBlock_whenProcessBlockFails() {
     assertBadBlockIsTracked(block);
   }
 
+  @Test
+  public void validateAndProcessBlock_whenStorageExceptionThrownGettingParent() {
+    final Throwable storageException = new StorageException("Database closed");
+    final Hash parentHash = blockParent.getHash();
+    doThrow(storageException).when(blockchain).getBlockHeader(eq(parentHash));
+
+    BlockProcessingResult result =
+        mainnetBlockValidator.validateAndProcessBlock(
+            protocolContext,
+            block,
+            HeaderValidationMode.DETACHED_ONLY,
+            HeaderValidationMode.DETACHED_ONLY);
+
+    assertValidationFailedExceptionally(result, storageException);
+    assertNoBadBlocks();
+  }
+
+  @ParameterizedTest(name = "[{index}] {0}")
+  @MethodSource("getStorageExceptions")
+  public void validateAndProcessBlock_whenStorageExceptionThrownProcessingBlock(
+      final String caseName, final Exception storageException) {
+    doThrow(storageException)
+        .when(blockProcessor)
+        .processBlock(eq(blockchain), any(MutableWorldState.class), eq(block));
+
+    BlockProcessingResult result =
+        mainnetBlockValidator.validateAndProcessBlock(
+            protocolContext,
+            block,
+            HeaderValidationMode.DETACHED_ONLY,
+            HeaderValidationMode.DETACHED_ONLY);
+
+    assertValidationFailedExceptionally(result, storageException);
+    assertNoBadBlocks();
+  }
+
+  @ParameterizedTest(name = "[{index}] {0}")
+  @MethodSource("getStorageExceptions")
+  public void validateAndProcessBlock_whenStorageExceptionThrownGettingWorldState(
+      final String caseName, final Exception storageException) {
+    final BlockHeader parentHeader = blockParent.getHeader();
+    doThrow(storageException).when(worldStateArchive).getMutable(eq(parentHeader), anyBoolean());
+
+    BlockProcessingResult result =
+        mainnetBlockValidator.validateAndProcessBlock(
+            protocolContext,
+            block,
+            HeaderValidationMode.DETACHED_ONLY,
+            HeaderValidationMode.DETACHED_ONLY);
+
+    assertValidationFailedExceptionally(result, storageException);
+    assertNoBadBlocks();
+  }
+
+  @ParameterizedTest(name = "[{index}] {0}")
+  @MethodSource("getBlockProcessingErrors")
+  public void validateAndProcessBlock_whenProcessBlockYieldsExceptionalResult(
+      final String caseName, final Exception cause) {
+    final BlockProcessingResult exceptionalResult =
+        new BlockProcessingResult(Optional.empty(), cause);
+    when(blockProcessor.processBlock(eq(blockchain), any(MutableWorldState.class), eq(block)))
+        .thenReturn(exceptionalResult);
+
+    BlockProcessingResult result =
+        mainnetBlockValidator.validateAndProcessBlock(
+            protocolContext,
+            block,
+            HeaderValidationMode.DETACHED_ONLY,
+            HeaderValidationMode.DETACHED_ONLY);
+
+    assertValidationFailedExceptionally(result, cause);
+    assertNoBadBlocks();
+  }
+
   @Test
   public void validateAndProcessBlock_withShouldRecordBadBlockFalse() {
     when(blockProcessor.processBlock(eq(blockchain), any(MutableWorldState.class), eq(block)))
@@ -305,4 +399,13 @@ private void assertValidationFailed(
     assertThat(result.errorMessage).isPresent();
     assertThat(result.errorMessage.get()).containsIgnoringWhitespaces(expectedError);
   }
+
+  private void assertValidationFailedExceptionally(
+      final BlockProcessingResult result, final Throwable exception) {
+    assertThat(result.isFailed()).isTrue();
+    assertThat(result.causedBy()).containsSame(exception);
+    assertThat(result.errorMessage).isPresent();
+    assertThat(result.errorMessage.get())
+        .containsIgnoringWhitespaces(exception.getLocalizedMessage());
+  }
 }