diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java index 477cc6d2f6d..e548bf4dd61 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java @@ -17,15 +17,17 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.concurrent.RejectedExecutionException; import java.util.function.Supplier; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.tuweni.bytes.Bytes32; +import tech.pegasys.teku.bls.impl.BlsException; import tech.pegasys.teku.ethereum.events.SlotEventsChannel; import tech.pegasys.teku.infrastructure.async.SafeFuture; import tech.pegasys.teku.infrastructure.exceptions.ExceptionUtil; import tech.pegasys.teku.infrastructure.logging.EventLogger; +import tech.pegasys.teku.infrastructure.ssz.InvalidValueSchemaException; +import tech.pegasys.teku.infrastructure.ssz.sos.SszDeserializeException; import tech.pegasys.teku.infrastructure.subscribers.Subscribers; import tech.pegasys.teku.infrastructure.time.TimeProvider; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -35,6 +37,8 @@ import tech.pegasys.teku.spec.datastructures.epbs.versions.gloas.SignedExecutionPayloadEnvelope; import tech.pegasys.teku.spec.datastructures.execution.ExecutionPayloadSummary; import tech.pegasys.teku.spec.datastructures.validator.BroadcastValidationLevel; +import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.BlockProcessingException; +import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.StateTransitionException; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult.FailureReason; import tech.pegasys.teku.statetransition.blobs.BlockEventsListener; @@ -55,6 +59,28 @@ public class BlockManager extends Service ReceivedExecutionPayloadEventsChannel { private static final Logger LOG = LogManager.getLogger(); + /** + * An internal error is, by default, not a proof that a block is invalid: it is most likely caused + * by a local failure (resource exhaustion, a transient infrastructure issue or a bug). Marking + * the block as invalid in those cases makes us reject the canonical chain, along with all its + * descendants, until the entry is evicted from the invalid blocks cache. + * + *

So only errors which can exclusively be attributed to the content of the block itself are + * considered as a proof of invalidity. + */ + private static final List> INVALID_BLOCK_INTERNAL_ERRORS = + List.of( + // the state transition rejected the block + StateTransitionException.class, + BlockProcessingException.class, + // the block contains malformed SSZ data or makes the post state violate its schema + SszDeserializeException.class, + InvalidValueSchemaException.class, + // the block contains a malformed BLS public key or signature + BlsException.class, + // a value in the block caused an overflow or underflow while processing it + ArithmeticException.class); + private final RecentChainData recentChainData; private final BlockImporter blockImporter; private final BlockEventsListener blockEventsListener; @@ -380,7 +406,7 @@ private SafeFuture handleBlockImport( logFailedBlockImport(block, result.getFailureReason()); if (result .getFailureCause() - .map(this::internalErrorToBeConsiderAsInvalidBlock) + .map(BlockManager::internalErrorToBeConsiderAsInvalidBlock) .orElse(false)) { dropInvalidBlock(block, result); } @@ -447,12 +473,10 @@ private List removeBlocksPendingParentExecutionPayloadDependi return pendingBlockPool.removeBlocksWaitingForParentExecutionPayload(parentRoot); } - private boolean internalErrorToBeConsiderAsInvalidBlock(final Throwable internalError) { - if (internalError instanceof RejectedExecutionException - || ExceptionUtil.hasCause(internalError, RejectedExecutionException.class)) { - return false; - } - return true; + private static boolean internalErrorToBeConsiderAsInvalidBlock(final Throwable internalError) { + // hasCause also checks the exception itself + return INVALID_BLOCK_INTERNAL_ERRORS.stream() + .anyMatch(errorType -> ExceptionUtil.hasCause(internalError, errorType)); } private void logFailedBlockImport( diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java index d8ae2ac94d3..7c48bf72000 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java @@ -55,12 +55,17 @@ import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.stream.Stream; import org.apache.tuweni.bytes.Bytes32; import org.junit.jupiter.api.AfterEach; 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; import org.mockito.Answers; import tech.pegasys.teku.bls.BLSSignatureVerifier; +import tech.pegasys.teku.bls.impl.BlsException; import tech.pegasys.teku.infrastructure.async.AsyncRunner; import tech.pegasys.teku.infrastructure.async.ExceptionThrowingFutureSupplier; import tech.pegasys.teku.infrastructure.async.SafeFuture; @@ -70,9 +75,12 @@ import tech.pegasys.teku.infrastructure.logging.EventLogger; import tech.pegasys.teku.infrastructure.metrics.SettableLabelledGauge; import tech.pegasys.teku.infrastructure.metrics.StubMetricsSystem; +import tech.pegasys.teku.infrastructure.ssz.InvalidValueSchemaException; +import tech.pegasys.teku.infrastructure.ssz.sos.SszDeserializeException; import tech.pegasys.teku.infrastructure.time.StubTimeProvider; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.kzg.NoOpKZG; +import tech.pegasys.teku.service.serviceutils.ServiceCapacityExceededException; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.TestSpecFactory; import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar; @@ -87,6 +95,8 @@ import tech.pegasys.teku.spec.generator.ChainBuilder.BlockOptions; import tech.pegasys.teku.spec.logic.common.statetransition.availability.AvailabilityChecker; import tech.pegasys.teku.spec.logic.common.statetransition.availability.DataAndValidationResult; +import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.BlockProcessingException; +import tech.pegasys.teku.spec.logic.common.statetransition.exceptions.StateTransitionException; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult.FailureReason; import tech.pegasys.teku.spec.util.DataStructureUtil; @@ -479,26 +489,46 @@ public void onGossipedBlock_unattachedFutureBlock() { assertThat(pendingBlocks.contains(nextNextBlock)).isTrue(); } - @Test - public void onGossipedBlock_onKnownInternalErrorsShouldNotMarkAsInvalid() { - final RecentChainData localRecentChainData = mock(RecentChainData.class); - blockManager = setupBlockManagerWithMockRecentChainData(localRecentChainData, false); + static Stream internalErrorsNotProvingBlockIsInvalid() { + return Stream.of( + Arguments.of(new RejectedExecutionException("full")), + Arguments.of(new RuntimeException("wrapped", new RejectedExecutionException("full"))), + Arguments.of(new OutOfMemoryError("Java heap space")), + Arguments.of(new ServiceCapacityExceededException("queue is full")), + Arguments.of(new IllegalStateException("unexpected")), + Arguments.of(new RuntimeException("unknown"))); + } - final UInt64 nextSlot = GENESIS_SLOT.plus(UInt64.ONE); - final SignedBeaconBlock nextBlock = - localChain.chainBuilder().generateBlockAtSlot(nextSlot).getBlock(); - incrementSlot(); + static Stream internalErrorsProvingBlockIsInvalid() { + return Stream.of( + Arguments.of(new StateTransitionException("state transition failed")), + Arguments.of(new BlockProcessingException("block processing failed")), + Arguments.of(new SszDeserializeException("malformed ssz")), + Arguments.of(new InvalidValueSchemaException("value doesn't match the schema")), + Arguments.of(new BlsException("invalid public key")), + Arguments.of(new ArithmeticException("uint64 overflow")), + Arguments.of(new RuntimeException("wrapped", new BlockProcessingException("invalid")))); + } - doAnswer(invocation -> SafeFuture.failedFuture(new RejectedExecutionException("full"))) - .when(asyncRunner) - .runAsync((ExceptionThrowingFutureSupplier) any()); + @ParameterizedTest(name = "{0}") + @MethodSource("internalErrorsNotProvingBlockIsInvalid") + public void onGossipedBlock_onInternalErrorShouldNotMarkAsInvalid(final Throwable internalError) { + final SignedBeaconBlock nextBlock = setupBlockFailingImportWith(internalError); assertThatBlockImport(nextBlock).isCompletedWithValueMatching(result -> !result.isSuccessful()); assertThat(invalidBlockRoots).isEmpty(); } - @Test - public void onGossipedBlock_onInternalErrorsShouldMarkAsInvalid() { + @ParameterizedTest(name = "{0}") + @MethodSource("internalErrorsProvingBlockIsInvalid") + public void onGossipedBlock_onInternalErrorShouldMarkAsInvalid(final Throwable internalError) { + final SignedBeaconBlock nextBlock = setupBlockFailingImportWith(internalError); + + assertThatBlockImport(nextBlock).isCompletedWithValueMatching(result -> !result.isSuccessful()); + assertThat(invalidBlockRoots).containsOnlyKeys(nextBlock.getRoot()); + } + + private SignedBeaconBlock setupBlockFailingImportWith(final Throwable internalError) { final RecentChainData localRecentChainData = mock(RecentChainData.class); blockManager = setupBlockManagerWithMockRecentChainData(localRecentChainData, false); @@ -507,12 +537,11 @@ public void onGossipedBlock_onInternalErrorsShouldMarkAsInvalid() { localChain.chainBuilder().generateBlockAtSlot(nextSlot).getBlock(); incrementSlot(); - doAnswer(invocation -> SafeFuture.failedFuture(new RuntimeException("unknown"))) + doAnswer(invocation -> SafeFuture.failedFuture(internalError)) .when(asyncRunner) .runAsync((ExceptionThrowingFutureSupplier) any()); - assertThatBlockImport(nextBlock).isCompletedWithValueMatching(result -> !result.isSuccessful()); - assertThat(invalidBlockRoots).containsOnlyKeys(nextBlock.getRoot()); + return nextBlock; } @Test