4512 invalid badblock (hyperledger#4554)

* don't add to bad blocks manager on StorageException * support for MerklePatriciaTrie exceptions Signed-off-by: Justin Florentine <justin+github@florentine.us>
noirgif · Oct 29, 2022 · fe79c02 · fe79c02
1 parent b322ef6
commit fe79c02
Show file tree

Hide file tree

Showing 37 changed files with 758 additions and 456 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,7 @@
 
 ### Bug Fixes
 - Fixed default fromBlock value and improved parameter interpetation in eth_getLogs RPC handler [#4513](https://github.com/hyperledger/besu/pull/4513)
-
+- Corrects treating a block as bad on internal error during either validation or processing [#4512](https://github.com/hyperledger/besu/issues/4512)
 ## 22.10.0-RC2
 
 ### Breaking Changes

diff --git a/...s/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/validation/MessageValidator.java b/...s/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/validation/MessageValidator.java
@@ -81,7 +81,7 @@ private boolean validateBlock(final Block block) {
         blockValidator.validateAndProcessBlock(
             protocolContext, block, HeaderValidationMode.LIGHT, HeaderValidationMode.FULL);
 
-    if (validationResult.blockProcessingOutputs.isEmpty()) {
+    if (validationResult.isFailed()) {
       LOG.info(
           "Invalid Proposal message, block did not pass validation. Reason {}",
           validationResult.errorMessage);

diff --git a/...rc/test/java/org/hyperledger/besu/consensus/ibft/statemachine/RoundChangeManagerTest.java b/...rc/test/java/org/hyperledger/besu/consensus/ibft/statemachine/RoundChangeManagerTest.java
@@ -36,9 +36,8 @@
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.crypto.NodeKeyUtils;
 import org.hyperledger.besu.datatypes.Address;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.BlockValidator;
-import org.hyperledger.besu.ethereum.BlockValidator.BlockProcessingOutputs;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.Util;
 
@@ -79,7 +78,7 @@ public void setup() {
 
     final BlockValidator blockValidator = mock(BlockValidator.class);
     when(blockValidator.validateAndProcessBlock(any(), any(), any(), any()))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     final RoundChangePayloadValidator.MessageValidatorForHeightFactory messageValidatorFactory =
         mock(RoundChangePayloadValidator.MessageValidatorForHeightFactory.class);

diff --git a/...ft/src/test/java/org/hyperledger/besu/consensus/ibft/validation/MessageValidatorTest.java b/...ft/src/test/java/org/hyperledger/besu/consensus/ibft/validation/MessageValidatorTest.java
@@ -35,9 +35,8 @@
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.crypto.NodeKeyUtils;
 import org.hyperledger.besu.datatypes.Address;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.BlockValidator;
-import org.hyperledger.besu.ethereum.BlockValidator.BlockProcessingOutputs;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
 import org.hyperledger.besu.ethereum.core.AddressHelpers;
@@ -99,7 +98,7 @@ public void setup() {
             mock(MutableBlockchain.class), mock(WorldStateArchive.class), mockBftCtx);
 
     when(blockValidator.validateAndProcessBlock(any(), any(), any(), any()))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     when(roundChangeCertificateValidator.validateProposalMessageMatchesLatestPrepareCertificate(
             any(), any()))
@@ -154,7 +153,7 @@ public void ifProposalConsistencyChecksFailProposalIsIllegal() {
   @Test
   public void blockValidationFailureFailsValidation() {
     when(blockValidator.validateAndProcessBlock(any(), any(), any(), any()))
-        .thenReturn(new Result("Failed"));
+        .thenReturn(new BlockProcessingResult("Failed"));
 
     final Proposal proposalMsg =
         messageFactory.createProposal(roundIdentifier, block, Optional.empty());

diff --git a/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/...ge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java
@@ -23,7 +23,7 @@
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.datatypes.Wei;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.blockcreation.BlockCreator.BlockCreationResult;
 import org.hyperledger.besu.ethereum.chain.BadBlockManager;
@@ -210,8 +210,8 @@ public PayloadIdentifier preparePayload(
             .createBlock(Optional.of(Collections.emptyList()), prevRandao, timestamp)
             .getBlock();
 
-    Result result = validateBlock(emptyBlock);
-    if (result.blockProcessingOutputs.isPresent()) {
+    BlockProcessingResult result = validateBlock(emptyBlock);
+    if (result.getYield().isPresent()) {
       mergeContext.putPayloadById(payloadIdentifier, emptyBlock);
       debugLambda(
           LOG,
@@ -327,7 +327,7 @@ private void evaluateNewBlock(
     if (isBlockCreationCancelled(payloadIdentifier)) return;
 
     final var resultBest = validateBlock(bestBlock);
-    if (resultBest.blockProcessingOutputs.isPresent()) {
+    if (resultBest.getYield().isPresent()) {
 
       if (isBlockCreationCancelled(payloadIdentifier)) return;
 
@@ -395,7 +395,8 @@ private Void logSyncException(final Hash blockHash, final Throwable exception) {
   }
 
   @Override
-  public Result validateBlock(final Block block) {
+  public BlockProcessingResult validateBlock(final Block block) {
+
     final var chain = protocolContext.getBlockchain();
     chain
         .getBlockHeader(block.getHeader().getParentHash())
@@ -415,18 +416,15 @@ public Result validateBlock(final Block block) {
                 HeaderValidationMode.NONE,
                 false);
 
-    validationResult.errorMessage.ifPresent(errMsg -> addBadBlock(block));
-
     return validationResult;
   }
 
   @Override
-  public Result rememberBlock(final Block block) {
+  public BlockProcessingResult rememberBlock(final Block block) {
     debugLambda(LOG, "Remember block {}", block::toLogString);
     final var chain = protocolContext.getBlockchain();
     final var validationResult = validateBlock(block);
-    validationResult.blockProcessingOutputs.ifPresent(
-        result -> chain.storeBlock(block, result.receipts));
+    validationResult.getYield().ifPresent(result -> chain.storeBlock(block, result.getReceipts()));
     return validationResult;
   }
 

diff --git a/.../main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java b/.../main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeMiningCoordinator.java
@@ -19,7 +19,7 @@
 
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.blockcreation.MiningCoordinator;
 import org.hyperledger.besu.ethereum.core.Block;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
@@ -41,9 +41,9 @@ default boolean isCompatibleWithEngineApi() {
     return true;
   }
 
-  Result rememberBlock(final Block block);
+  BlockProcessingResult rememberBlock(final Block block);
 
-  Result validateBlock(final Block block);
+  BlockProcessingResult validateBlock(final Block block);
 
   ForkchoiceResult updateForkChoice(
       final BlockHeader newHead,

diff --git a/...c/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java b/...c/main/java/org/hyperledger/besu/consensus/merge/blockcreation/TransitionCoordinator.java
@@ -18,7 +18,7 @@
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.datatypes.Wei;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.blockcreation.MiningCoordinator;
 import org.hyperledger.besu.ethereum.blockcreation.PoWMiningCoordinator;
 import org.hyperledger.besu.ethereum.chain.PoWObserver;
@@ -137,12 +137,12 @@ public PayloadIdentifier preparePayload(
   }
 
   @Override
-  public Result rememberBlock(final Block block) {
+  public BlockProcessingResult rememberBlock(final Block block) {
     return mergeCoordinator.rememberBlock(block);
   }
 
   @Override
-  public Result validateBlock(final Block block) {
+  public BlockProcessingResult validateBlock(final Block block) {
     return mergeCoordinator.validateBlock(block);
   }
 

diff --git a/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/...rc/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java
@@ -778,7 +778,7 @@ public void invalidPayloadShouldReturnErrorAndUpdateForkchoiceState() {
 
     BlockHeader headBlockHeader = nextBlockHeader(lastFinalizedHeader);
     Block headBlock = new Block(headBlockHeader, BlockBody.empty());
-    assertThat(coordinator.rememberBlock(headBlock).blockProcessingOutputs).isPresent();
+    assertThat(coordinator.rememberBlock(headBlock).getYield()).isPresent();
 
     var res =
         coordinator.updateForkChoice(
@@ -832,7 +832,7 @@ public void forkchoiceUpdateShouldIgnoreAncestorOfChainHead() {
   private void sendNewPayloadAndForkchoiceUpdate(
       final Block block, final Optional<BlockHeader> finalizedHeader, final Hash safeHash) {
 
-    assertThat(coordinator.rememberBlock(block).blockProcessingOutputs).isPresent();
+    assertThat(coordinator.rememberBlock(block).getYield()).isPresent();
     assertThat(
             coordinator
                 .updateForkChoice(

diff --git a/...erge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeReorgTest.java b/...erge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeReorgTest.java
@@ -24,7 +24,7 @@
 import org.hyperledger.besu.consensus.merge.PostMergeContext;
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Wei;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.GenesisState;
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
@@ -147,13 +147,15 @@ public void reorgsAcrossTDDToDifferentTargetsWhenNotFinal() {
   }
 
   private void appendBlock(final Block block) {
-    final Result result = coordinator.validateBlock(block);
-
-    result.blockProcessingOutputs.ifPresentOrElse(
-        outputs -> blockchain.appendBlock(block, outputs.receipts),
-        () -> {
-          throw new RuntimeException(result.errorMessage.get());
-        });
+    final BlockProcessingResult result = coordinator.validateBlock(block);
+
+    result
+        .getYield()
+        .ifPresentOrElse(
+            outputs -> blockchain.appendBlock(block, outputs.getReceipts()),
+            () -> {
+              throw new RuntimeException(result.errorMessage.get());
+            });
   }
 
   private List<Block> subChain(

diff --git a/...rc/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalPayloadValidator.java b/...rc/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalPayloadValidator.java
@@ -122,7 +122,7 @@ private boolean validateBlock(final Block block) {
         blockValidator.validateAndProcessBlock(
             protocolContext, block, HeaderValidationMode.LIGHT, HeaderValidationMode.FULL);
 
-    if (validationResult.blockProcessingOutputs.isEmpty()) {
+    if (!validationResult.isSuccessful()) {
       LOG.info(
           "{}: block did not pass validation. Reason {}",
           ERROR_PREFIX,

diff --git a/...main/java/org/hyperledger/besu/consensus/qbft/validation/RoundChangeMessageValidator.java b/...main/java/org/hyperledger/besu/consensus/qbft/validation/RoundChangeMessageValidator.java
@@ -81,7 +81,7 @@ private boolean validateBlock(final Block block) {
         blockValidator.validateAndProcessBlock(
             protocolContext, block, HeaderValidationMode.LIGHT, HeaderValidationMode.FULL);
 
-    if (validationResult.blockProcessingOutputs.isEmpty()) {
+    if (!validationResult.isSuccessful()) {
       LOG.info(
           "{}: block did not pass validation. Reason {}",
           ERROR_PREFIX,

diff --git a/...est/java/org/hyperledger/besu/consensus/qbft/validation/ProposalPayloadValidatorTest.java b/...est/java/org/hyperledger/besu/consensus/qbft/validation/ProposalPayloadValidatorTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 ConsenSys AG.
+ * Copyright 2022 Hyperledger Besu Contributors.
  *
  * 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
@@ -37,9 +37,8 @@
 import org.hyperledger.besu.crypto.NodeKeyUtils;
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.datatypes.Hash;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.BlockValidator;
-import org.hyperledger.besu.ethereum.BlockValidator.BlockProcessingOutputs;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
 import org.hyperledger.besu.ethereum.core.Block;
@@ -105,7 +104,7 @@ public void validationPassesWhenProposerAndRoundMatchAndBlockIsValid() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isTrue();
   }
@@ -129,7 +128,7 @@ public void validationPassesWhenBlockRoundDoesNotMatchProposalRound() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isTrue();
   }
@@ -152,7 +151,7 @@ public void validationFailsWhenBlockFailsValidation() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result("Failed"));
+        .thenReturn(new BlockProcessingResult("Failed"));
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isFalse();
   }
@@ -228,7 +227,7 @@ public void validationFailsForBlockWithIncorrectHeight() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isFalse();
   }
@@ -262,7 +261,7 @@ public void validationForCmsFailsWhenCmsFailsValidation() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
     when(cmsValidator.validate(eq(cms), eq(hashWithoutCms))).thenReturn(false);
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isFalse();
@@ -297,7 +296,7 @@ public void validationForCmsPassesWhenCmsIsValid() {
             eq(block),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
     when(cmsValidator.validate(eq(cms), eq(hashWithoutCms))).thenReturn(true);
 
     assertThat(payloadValidator.validate(proposal.getSignedPayload())).isTrue();

diff --git a/...t/src/test/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidatorTest.java b/...t/src/test/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidatorTest.java
@@ -37,9 +37,8 @@
 import org.hyperledger.besu.consensus.qbft.payload.PreparedRoundMetadata;
 import org.hyperledger.besu.consensus.qbft.payload.RoundChangePayload;
 import org.hyperledger.besu.datatypes.Hash;
+import org.hyperledger.besu.ethereum.BlockProcessingResult;
 import org.hyperledger.besu.ethereum.BlockValidator;
-import org.hyperledger.besu.ethereum.BlockValidator.BlockProcessingOutputs;
-import org.hyperledger.besu.ethereum.BlockValidator.Result;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
 import org.hyperledger.besu.ethereum.core.Block;
@@ -106,7 +105,7 @@ public void setup() {
             any(),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result(new BlockProcessingOutputs(null, null)));
+        .thenReturn(new BlockProcessingResult(Optional.empty()));
 
     roundItems.put(ROUND_ID.ZERO, createRoundSpecificItems(0));
     roundItems.put(ROUND_ID.ONE, createRoundSpecificItems(1));
@@ -158,7 +157,7 @@ public void validationFailsIfBlockIsInvalid() {
             any(),
             eq(HeaderValidationMode.LIGHT),
             eq(HeaderValidationMode.FULL)))
-        .thenReturn(new Result("Failed"));
+        .thenReturn(new BlockProcessingResult("Failed"));
 
     assertThat(roundItem.messageValidator.validate(proposal)).isFalse();
   }