From 4982285e5814368cb0e4b652eff1c4ec8dbc7d3b Mon Sep 17 00:00:00 2001 From: mbaxter Date: Tue, 5 Mar 2024 16:22:41 -0500 Subject: [PATCH] Bugfix - only track intrinsically bad blocks (#6678) Signed-off-by: mbaxter --- .../besu/ethereum/MainnetBlockValidator.java | 62 +++++----- .../besu/ethereum/chain/BadBlockCause.java | 34 ++---- .../besu/ethereum/chain/BadBlockManager.java | 17 +-- .../ethereum/MainnetBlockValidatorTest.java | 107 +++++++++++++++++- .../ethereum/chain/BadBlockManagerTest.java | 32 ------ 5 files changed, 146 insertions(+), 106 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java index a5765b8984a..4938b653534 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java +++ b/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,12 +133,13 @@ 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 receipts = @@ -144,7 +147,7 @@ public BlockProcessingResult validateAndProcessBlock( 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()); } } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockCause.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockCause.java index 41054a646d7..7a014e99cd2 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockCause.java +++ b/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 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 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 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(); } } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java index 46155f94bc2..b13f81c877c 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java +++ b/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 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; - } } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/MainnetBlockValidatorTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/MainnetBlockValidatorTest.java index c29c686e144..ccda0ccb807 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/MainnetBlockValidatorTest.java +++ b/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 getStorageExceptions() { + return Stream.of( + Arguments.of("StorageException", new StorageException("Database closed")), + Arguments.of("MerkleTrieException", new MerkleTrieException("Missing trie node"))); + } + + public static Stream 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()); + } } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/chain/BadBlockManagerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/chain/BadBlockManagerTest.java index 52288a09066..ba027c64361 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/chain/BadBlockManagerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/chain/BadBlockManagerTest.java @@ -19,15 +19,8 @@ import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockchainSetupUtil; -import org.hyperledger.besu.ethereum.trie.MerkleTrieException; -import org.hyperledger.besu.plugin.services.exception.StorageException; - -import java.util.stream.Stream; 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 BadBlockManagerTest { @@ -35,12 +28,6 @@ public class BadBlockManagerTest { final Block block = chainUtil.getBlock(1); final BadBlockManager badBlockManager = new BadBlockManager(); - public static Stream getInternalExceptions() { - return Stream.of( - Arguments.of("StorageException", new StorageException("oops")), - Arguments.of("MerkleTrieException", new MerkleTrieException("fail"))); - } - @Test public void addBadBlock_addsBlock() { BadBlockManager badBlockManager = new BadBlockManager(); @@ -50,25 +37,6 @@ public void addBadBlock_addsBlock() { assertThat(badBlockManager.getBadBlocks()).containsExactly(block); } - @ParameterizedTest(name = "[{index}] {0}") - @MethodSource("getInternalExceptions") - public void addBadBlock_ignoresInternalError(final String caseName, final Exception err) { - BadBlockManager badBlockManager = new BadBlockManager(); - final BadBlockCause cause = BadBlockCause.fromProcessingError(err); - badBlockManager.addBadBlock(block, cause); - - assertThat(badBlockManager.getBadBlocks()).isEmpty(); - } - - @Test - public void addBadBlock_doesNotIgnoreRuntimeException() { - final Exception err = new RuntimeException("oops"); - final BadBlockCause cause = BadBlockCause.fromProcessingError(err); - badBlockManager.addBadBlock(block, cause); - - assertThat(badBlockManager.getBadBlocks()).containsExactly(block); - } - @Test public void reset_clearsBadBlocks() { BadBlockManager badBlockManager = new BadBlockManager();