don't add to bad blocks manager on StorageException (#4524)

* don't add to bad blocks manager on StorageException
* add bugfix to changelog
* adds test coverage

Signed-off-by: Justin Florentine <justin+github@florentine.us>

CHANGELOG.md

@@ -22,8 +22,10 @@
 - Corrects emission of blockadded events when rewinding during a re-org. Fix for [#4495](https://github.com/hyperledger/besu/issues/4495)
 - Always return a transaction type for pending transactions [#4364](https://github.com/hyperledger/besu/pull/4364)
 - Avoid a cyclic reference while printing EngineExchangeTransitionConfigurationParameter [#4357](https://github.com/hyperledger/besu/pull/4357)
+- Corrects treating a block as bad on internal error [#4512](https://github.com/hyperledger/besu/issues/4512)
 - In GraphQL update scalar parsing to be variable friendly [#4522](https://github.com/hyperledger/besu/pull/4522)
 
+
 ### Download Links
 
 

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

@@ -107,6 +107,7 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
+          INVALID,
           "Failed to decode transactions from block parameter");
     }
 
@@ -115,6 +116,7 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
+          INVALID,
           "Field extraData must not be null");
     }
 
@@ -141,11 +143,12 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
     // ensure the block hash matches the blockParam hash
     // this must be done before any other check
     if (!newBlockHeader.getHash().equals(blockParam.getBlockHash())) {
-      LOG.debug(
+      String errorMessage =
           String.format(
               "Computed block hash %s does not match block hash parameter %s",
-              newBlockHeader.getBlockHash(), blockParam.getBlockHash()));
-      return respondWith(reqId, blockParam, null, INVALID_BLOCK_HASH);
+              newBlockHeader.getBlockHash(), blockParam.getBlockHash());
+      LOG.debug(errorMessage);
+      return respondWithInvalid(reqId, blockParam, null, INVALID_BLOCK_HASH, errorMessage);
     }
     // do we already have this payload
     if (protocolContext.getBlockchain().getBlockByHash(newBlockHeader.getBlockHash()).isPresent()) {
@@ -153,13 +156,14 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
       return respondWith(reqId, blockParam, blockParam.getBlockHash(), VALID);
     }
     if (mergeCoordinator.isBadBlock(blockParam.getBlockHash())) {
-      return respondWith(
+      return respondWithInvalid(
           reqId,
           blockParam,
           mergeCoordinator
               .getLatestValidHashOfBadBlock(blockParam.getBlockHash())
               .orElse(Hash.ZERO),
-          INVALID);
+          INVALID,
+          "Block already present in bad block manager.");
     }
 
     Optional<BlockHeader> parentHeader =
@@ -170,6 +174,7 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           reqId,
           blockParam,
           mergeCoordinator.getLatestValidAncestor(blockParam.getParentHash()).orElse(null),
+          INVALID,
           "block timestamp not greater than parent");
     }
 
@@ -200,6 +205,7 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
           reqId,
           blockParam,
           Hash.ZERO,
+          INVALID,
           newBlockHeader.getHash() + " did not descend from terminal block");
     }
 
@@ -228,7 +234,11 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
       }
       LOG.debug("New payload is invalid: {}", executionResult.errorMessage.get());
       return respondWithInvalid(
-          reqId, blockParam, latestValidAncestor.get(), executionResult.errorMessage.get());
+          reqId,
+          blockParam,
+          latestValidAncestor.get(),
+          INVALID,
+          executionResult.errorMessage.get());
     }
   }
 
@@ -237,6 +247,10 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
       final EnginePayloadParameter param,
       final Hash latestValidHash,
       final EngineStatus status) {
+    if (INVALID.equals(status) || INVALID_BLOCK_HASH.equals(status)) {
+      throw new IllegalArgumentException(
+          "Don't call respondWith() with invalid status of " + status.toString());
+    }
     debugLambda(
         LOG,
         "New payload: number: {}, hash: {}, parentHash: {}, latestValidHash: {}, status: {}",
@@ -256,7 +270,12 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
       final Object requestId,
       final EnginePayloadParameter param,
       final Hash latestValidHash,
+      final EngineStatus invalidStatus,
       final String validationError) {
+    if (!INVALID.equals(invalidStatus) && !INVALID_BLOCK_HASH.equals(invalidStatus)) {
+      throw new IllegalArgumentException(
+          "Don't call respondWithInvalid() with non-invalid status of " + invalidStatus.toString());
+    }
     final String invalidBlockLogMessage =
         String.format(
             "Invalid new payload: number: %s, hash: %s, parentHash: %s, latestValidHash: %s, status: %s, validationError: %s",
@@ -264,7 +283,7 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
             param.getBlockHash(),
             param.getParentHash(),
             latestValidHash == null ? null : latestValidHash.toHexString(),
-            INVALID.name(),
+            invalidStatus.name(),
             validationError);
     // always log invalid at DEBUG
     LOG.debug(invalidBlockLogMessage);
@@ -275,7 +294,8 @@ public class EngineNewPayload extends ExecutionEngineJsonRpcMethod {
     }
     return new JsonRpcSuccessResponse(
         requestId,
-        new EnginePayloadStatusResult(INVALID, latestValidHash, Optional.of(validationError)));
+        new EnginePayloadStatusResult(
+            invalidStatus, latestValidHash, Optional.of(validationError)));
   }
 
   private void logImportedBlockInfo(final Block block, final double timeInS) {

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

@@ -235,6 +235,8 @@ public class EngineNewPayloadTest {
 
     fromErrorResp(resp);
     verify(engineCallListener, times(1)).executionEngineCalled();
+    verify(mergeCoordinator, times(0)).addBadBlock(any());
+    // verify mainnetBlockValidator does not add to bad block manager
   }
 
   @Test
@@ -362,7 +364,7 @@ public class EngineNewPayloadTest {
     EnginePayloadStatusResult res = fromSuccessResp(resp);
     assertThat(res.getLatestValidHash()).contains(Hash.ZERO);
     assertThat(res.getStatusAsString()).isEqualTo(INVALID.name());
-    assertThat(res.getError()).isNull();
+    assertThat(res.getError()).isEqualTo("Block already present in bad block manager.");
     verify(engineCallListener, times(1)).executionEngineCalled();
   }
 

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

@@ -150,8 +150,9 @@ public class MainnetBlockValidator implements BlockValidator {
       final Block invalidBlock, final String reason, final BlockProcessor.Result result) {
     if (result.causedBy().isPresent()) {
       LOG.info("{}. Block {}, caused by {}", reason, invalidBlock.toLogString(), result.causedBy());
-      // TODO: if it's an internal error, don't add it
-      badBlockManager.addBadBlock(invalidBlock);
+      if (!result.internalError()) {
+        badBlockManager.addBadBlock(invalidBlock);
+      }
       return new Result(reason, result.causedBy().get());
     } else {
       LOG.info("{}. Block {}", reason, invalidBlock.toLogString());

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BlockProcessor.java

@@ -22,6 +22,7 @@ import org.hyperledger.besu.ethereum.core.MutableWorldState;
 import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.TransactionReceipt;
 import org.hyperledger.besu.ethereum.privacy.storage.PrivateMetadataUpdater;
+import org.hyperledger.besu.plugin.services.exception.StorageException;
 
 import java.util.List;
 import java.util.Optional;
@@ -65,6 +66,17 @@ public interface BlockProcessor {
     default Optional<Throwable> causedBy() {
       return Optional.empty();
     }
+
+    default boolean internalError() {
+      if (causedBy().isPresent()) {
+        Throwable t = causedBy().get();
+        // As new "internal only" types of exception are discovered, add them here.
+        if (t instanceof StorageException) {
+          return true;
+        }
+      }
+      return false;
+    }
   }
 
   /**

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

@@ -36,6 +36,7 @@ import org.hyperledger.besu.ethereum.mainnet.BlockProcessor;
 import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode;
 import org.hyperledger.besu.ethereum.mainnet.MainnetBlockHeaderFunctions;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
+import org.hyperledger.besu.plugin.services.exception.StorageException;
 
 import java.util.Collections;
 import java.util.List;
@@ -257,6 +258,59 @@ public class MainnetBlockValidatorTest {
     assertThat(badBlockManager.getBadBlocks()).isEmpty();
   }
 
+  @Test
+  public void shouldNotCacheWhenInternalError() {
+    when(blockchain.getBlockHeader(any(Hash.class)))
+        .thenReturn(Optional.of(new BlockHeaderTestFixture().buildHeader()));
+    when(blockHeaderValidator.validateHeader(
+            any(BlockHeader.class),
+            any(BlockHeader.class),
+            eq(protocolContext),
+            eq(HeaderValidationMode.DETACHED_ONLY)))
+        .thenReturn(true);
+    when(worldStateArchive.getMutable(any(Hash.class), any(Hash.class)))
+        .thenReturn(Optional.of(mock(MutableWorldState.class)));
+    when(blockProcessor.processBlock(eq(blockchain), any(MutableWorldState.class), eq(badBlock)))
+        .thenReturn(
+            new BlockProcessor.Result() {
+              @SuppressWarnings("unchecked")
+              @Override
+              public List<TransactionReceipt> getReceipts() {
+                return Collections.EMPTY_LIST;
+              }
+
+              @SuppressWarnings("unchecked")
+              @Override
+              public List<TransactionReceipt> getPrivateReceipts() {
+                return Collections.EMPTY_LIST;
+              }
+
+              @Override
+              public boolean isSuccessful() {
+                return false;
+              }
+
+              @Override
+              public Optional<Throwable> causedBy() {
+                return Optional.of(new StorageException("database bedlam"));
+              }
+            });
+    when(blockBodyValidator.validateBody(
+            eq(protocolContext),
+            eq(badBlock),
+            any(),
+            any(),
+            eq(HeaderValidationMode.DETACHED_ONLY)))
+        .thenReturn(true);
+    assertThat(badBlockManager.getBadBlocks().size()).isEqualTo(0);
+    mainnetBlockValidator.validateAndProcessBlock(
+        protocolContext,
+        badBlock,
+        HeaderValidationMode.DETACHED_ONLY,
+        HeaderValidationMode.DETACHED_ONLY);
+    assertThat(badBlockManager.getBadBlocks()).isEmpty();
+  }
+
   @Test
   public void shouldReturnBadBlockBasedOnTheHash() {
     when(blockchain.getBlockHeader(any(Hash.class)))