Skip to content
/ besu Public
forked from hyperledger/besu

Commit

Permalink
[hyperledger#6301] Track bad block cause (hyperledger#6622)
Browse files Browse the repository at this point in the history
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
  • Loading branch information
mbaxter authored and amsmota committed Apr 16, 2024
1 parent 80c44f6 commit ea16ab7
Show file tree
Hide file tree
Showing 23 changed files with 845 additions and 315 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
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.BadBlockCause;
import org.hyperledger.besu.ethereum.chain.BadBlockManager;
import org.hyperledger.besu.ethereum.chain.Blockchain;
import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
Expand Down Expand Up @@ -781,7 +782,7 @@ public void onBadChain(
final Block badBlock,
final List<Block> badBlockDescendants,
final List<BlockHeader> badBlockHeaderDescendants) {
LOG.trace("Adding bad block {} and all its descendants", badBlock.getHash());
LOG.trace("Mark descendents of bad block {} as bad", badBlock.getHash());
final BadBlockManager badBlockManager = protocolContext.getBadBlockManager();

final Optional<BlockHeader> parentHeader =
Expand All @@ -791,12 +792,11 @@ public void onBadChain(
? Optional.of(parentHeader.get().getHash())
: Optional.empty();

badBlockManager.addBadBlock(badBlock, Optional.empty());

// Bad block has already been marked, but we need to mark the bad block's descendants
badBlockDescendants.forEach(
block -> {
LOG.trace("Add descendant block {} to bad blocks", block.getHash());
badBlockManager.addBadBlock(block, Optional.empty());
badBlockManager.addBadBlock(block, BadBlockCause.fromBadAncestorBlock(badBlock));
maybeLatestValidHash.ifPresent(
latestValidHash ->
badBlockManager.addLatestValidHash(block.getHash(), latestValidHash));
Expand All @@ -805,7 +805,7 @@ public void onBadChain(
badBlockHeaderDescendants.forEach(
header -> {
LOG.trace("Add descendant header {} to bad blocks", header.getHash());
badBlockManager.addBadHeader(header);
badBlockManager.addBadHeader(header, BadBlockCause.fromBadAncestorBlock(badBlock));
maybeLatestValidHash.ifPresent(
latestValidHash ->
badBlockManager.addLatestValidHash(header.getHash(), latestValidHash));
Expand Down Expand Up @@ -835,16 +835,10 @@ protected interface MergeBlockCreatorFactory {
MergeBlockCreator forParams(BlockHeader header, Optional<Address> feeRecipient);
}

@Override
public void addBadBlock(final Block block, final Optional<Throwable> maybeCause) {
protocolContext.getBadBlockManager().addBadBlock(block, maybeCause);
}

@Override
public boolean isBadBlock(final Hash blockHash) {
final BadBlockManager badBlockManager = protocolContext.getBadBlockManager();
return badBlockManager.getBadBlock(blockHash).isPresent()
|| badBlockManager.getBadHash(blockHash).isPresent();
return badBlockManager.isBadBlock(blockHash);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,6 @@ ForkchoiceResult updateForkChoice(
*/
boolean isMiningBeforeMerge();

/**
* Add bad block.
*
* @param block the block
* @param maybeCause the maybe cause
*/
void addBadBlock(final Block block, Optional<Throwable> maybeCause);

/**
* Is bad block.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,6 @@ public boolean isDescendantOf(final BlockHeader ancestorBlock, final BlockHeader
return mergeCoordinator.isDescendantOf(ancestorBlock, newBlock);
}

@Override
public void addBadBlock(final Block block, final Optional<Throwable> maybeCause) {
mergeCoordinator.addBadBlock(block, maybeCause);
}

@Override
public boolean isBadBlock(final Hash blockHash) {
return mergeCoordinator.isBadBlock(blockHash);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,6 @@ public void exceptionDuringBuildingBlockShouldNotBeInvalid()
Optional.empty(),
Optional.empty());

verify(willThrow, never()).addBadBlock(any(), any());
blockCreationTask.get();

ArgumentCaptor<PayloadWrapper> payloadWrapper = ArgumentCaptor.forClass(PayloadWrapper.class);
Expand All @@ -345,7 +344,6 @@ public void exceptionDuringBuildingBlockShouldNotBeInvalid()
// this only verifies that adding the bad block didn't happen through the mergeCoordinator, it
// still may be called directly.
verify(badBlockManager, never()).addBadBlock(any(), any());
verify(willThrow, never()).addBadBlock(any(), any());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.BadBlockResult;
import org.hyperledger.besu.ethereum.api.jsonrpc.internal.results.BlockResultFactory;
import org.hyperledger.besu.ethereum.chain.BadBlockCause;
import org.hyperledger.besu.ethereum.chain.BadBlockManager;
import org.hyperledger.besu.ethereum.core.Block;
import org.hyperledger.besu.ethereum.core.BlockDataGenerator;
Expand All @@ -37,7 +38,6 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -95,8 +95,10 @@ public void shouldReturnCorrectResponse() {
.setBlockHeaderFunctions(new MainnetBlockHeaderFunctions())
.setParentHash(parentBlock.getHash()));

badBlockManager.addBadBlock(badBlockWithTransaction, Optional.empty());
badBlockManager.addBadBlock(badBlockWoTransaction, Optional.empty());
badBlockManager.addBadBlock(
badBlockWithTransaction, BadBlockCause.fromValidationFailure("failed"));
badBlockManager.addBadBlock(
badBlockWoTransaction, BadBlockCause.fromValidationFailure("failed"));

final JsonRpcRequestContext request =
new JsonRpcRequestContext(new JsonRpcRequest("2.0", "debug_traceBlock", new Object[] {}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcResponse;
import org.hyperledger.besu.ethereum.api.jsonrpc.internal.response.JsonRpcSuccessResponse;
import org.hyperledger.besu.ethereum.api.query.BlockchainQueries;
import org.hyperledger.besu.ethereum.chain.BadBlockCause;
import org.hyperledger.besu.ethereum.chain.BadBlockManager;
import org.hyperledger.besu.ethereum.chain.Blockchain;
import org.hyperledger.besu.ethereum.core.Block;
Expand Down Expand Up @@ -99,7 +100,7 @@ public void shouldTraceTheTransactionUsingTheTransactionTracer() {
final List<String> paths = new ArrayList<>();
paths.add("path-1");

badBlockManager.addBadBlock(block, Optional.empty());
badBlockManager.addBadBlock(block, BadBlockCause.fromValidationFailure("failed"));

when(blockchainQueries.getBlockchain()).thenReturn(blockchain);
when(transactionTracer.traceTransactionToFile(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.lenient;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -59,7 +58,6 @@
import org.hyperledger.besu.ethereum.eth.manager.EthPeers;
import org.hyperledger.besu.ethereum.mainnet.BodyValidation;
import org.hyperledger.besu.ethereum.mainnet.DepositsValidator;
import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
import org.hyperledger.besu.ethereum.trie.MerkleTrieException;
Expand All @@ -82,17 +80,6 @@
@ExtendWith(MockitoExtension.class)
public abstract class AbstractEngineNewPayloadTest extends AbstractScheduledApiTest {

@FunctionalInterface
interface MethodFactory {
AbstractEngineNewPayload create(
final Vertx vertx,
final ProtocolSchedule protocolSchedule,
final ProtocolContext protocolContext,
final MergeMiningCoordinator mergeCoordinator,
final EthPeers ethPeers,
final EngineCallListener engineCallListener);
}

protected AbstractEngineNewPayload method;
protected Optional<Bytes32> maybeParentBeaconBlockRoot = Optional.empty();

Expand Down Expand Up @@ -229,8 +216,6 @@ public void shouldNotReturnInvalidOnStorageException() {

fromErrorResp(resp);
verify(engineCallListener, times(1)).executionEngineCalled();
verify(mergeCoordinator, times(0)).addBadBlock(any(), any());
// verify mainnetBlockValidator does not add to bad block manager
}

@Test
Expand All @@ -247,7 +232,6 @@ public void shouldNotReturnInvalidOnHandledMerkleTrieException() {
var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList()));

verify(engineCallListener, times(1)).executionEngineCalled();
verify(mergeCoordinator, times(0)).addBadBlock(any(), any());

fromErrorResp(resp);
}
Expand All @@ -265,7 +249,6 @@ public void shouldNotReturnInvalidOnThrownMerkleTrieException() {
var resp = resp(mockEnginePayload(mockHeader, Collections.emptyList()));

verify(engineCallListener, times(1)).executionEngineCalled();
verify(mergeCoordinator, never()).addBadBlock(any(), any());

fromErrorResp(resp);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.hyperledger.besu.ethereum;

import org.hyperledger.besu.ethereum.chain.BadBlockCause;
import org.hyperledger.besu.ethereum.chain.BadBlockManager;
import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
import org.hyperledger.besu.ethereum.core.Block;
Expand Down Expand Up @@ -111,7 +112,8 @@ public BlockProcessingResult validateAndProcessBlock(

if (!blockHeaderValidator.validateHeader(
header, parentHeader, context, headerValidationMode)) {
var retval = new BlockProcessingResult("header validation rule violated, see logs");
final String error = String.format("Header validation failed (%s)", headerValidationMode);
var retval = new BlockProcessingResult(error);
handleAndLogImportFailure(block, retval, shouldRecordBadBlock);
return retval;
}
Expand Down Expand Up @@ -141,8 +143,9 @@ public BlockProcessingResult validateAndProcessBlock(
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);
return new BlockProcessingResult("failed to validate output of imported block");
return result;
}

return new BlockProcessingResult(
Expand Down Expand Up @@ -187,7 +190,18 @@ private void handleAndLogImportFailure(
}
}
if (shouldRecordBadBlock) {
badBlockManager.addBadBlock(invalidBlock, result.causedBy());
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());
}
Expand Down Expand Up @@ -216,12 +230,14 @@ public boolean fastBlockValidation(
final HeaderValidationMode ommerValidationMode) {
final BlockHeader header = block.getHeader();
if (!blockHeaderValidator.validateHeader(header, context, headerValidationMode)) {
badBlockManager.addBadBlock(block, Optional.empty());
String description = String.format("Failed header validation (%s)", headerValidationMode);
badBlockManager.addBadBlock(block, BadBlockCause.fromValidationFailure(description));
return false;
}

if (!blockBodyValidator.validateBodyLight(context, block, receipts, ommerValidationMode)) {
badBlockManager.addBadBlock(block, Optional.empty());
badBlockManager.addBadBlock(
block, BadBlockCause.fromValidationFailure("Failed body validation (light)"));
return false;
}
return true;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright 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
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*
*/
package org.hyperledger.besu.ethereum.chain;

import org.hyperledger.besu.ethereum.core.Block;

import java.util.Optional;

public class BadBlockCause {
public enum BadBlockReason {
// Standard spec-related validation failures
SPEC_VALIDATION_FAILURE,
// When an unexpected exception occurs during block processing
EXCEPTIONAL_BLOCK_PROCESSING,
// This block is bad because it descends from a bad block
DESCENDS_FROM_BAD_BLOCK,
}

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());
}

public static BadBlockCause fromValidationFailure(final String failureMessage) {
return new BadBlockCause(
BadBlockReason.SPEC_VALIDATION_FAILURE, failureMessage, Optional.empty());
}

private BadBlockCause(BadBlockReason reason, String description, Optional<Throwable> exception) {
this.reason = reason;
this.description = description;
this.exception = exception;
}

public BadBlockReason getReason() {
return reason;
}

public String getDescription() {
return description;
}

public Optional<Throwable> getException() {
return exception;
}

@Override
public String toString() {
return "BadBlockCause{"
+ "reason="
+ reason
+ ", description='"
+ description
+ '\''
+ ", exception="
+ exception
+ '}';
}
}
Loading

0 comments on commit ea16ab7

Please sign in to comment.