From 6e65915660f17a499403216032cb4316ec245933 Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Wed, 6 Mar 2024 15:33:31 +0100 Subject: [PATCH] Improve block equivocation check for block publishing V2 API (#8043) --- .../BlockBroadcastValidatorImpl.java | 12 +- .../validation/BlockGossipValidator.java | 109 +++++++++++------- .../validation/InternalValidationResult.java | 42 ++++++- .../validation/ValidationResultCode.java | 7 +- .../BlockBroadcastValidatorTest.java | 63 ++++++---- .../validation/BlockGossipValidatorTest.java | 5 +- 6 files changed, 160 insertions(+), 78 deletions(-) diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorImpl.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorImpl.java index 858a75ac42e..4b9c9b5ea1e 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorImpl.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorImpl.java @@ -24,6 +24,7 @@ import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock; import tech.pegasys.teku.spec.datastructures.validator.BroadcastValidationLevel; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult; +import tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult; public class BlockBroadcastValidatorImpl implements BlockBroadcastValidator { private final BlockGossipValidator blockGossipValidator; @@ -92,7 +93,8 @@ private void buildValidationPipeline(final SignedBeaconBlock block) { .validate(block, true) .thenApply( gossipValidationResult -> { - if (gossipValidationResult.isAccept()) { + if (gossipValidationResult.isAccept() + || gossipValidationResult.isIgnoreAlreadySeen()) { return SUCCESS; } return GOSSIP_FAILURE; @@ -136,11 +138,13 @@ private void buildValidationPipeline(final SignedBeaconBlock block) { } // perform final equivocation validation - if (blockGossipValidator.blockIsFirstBlockWithValidSignatureForSlot(block)) { - return SUCCESS; + if (blockGossipValidator + .performBlockEquivocationCheck(block) + .equals(EquivocationCheckResult.EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER)) { + return FINAL_EQUIVOCATION_FAILURE; } - return FINAL_EQUIVOCATION_FAILURE; + return SUCCESS; }) .propagateTo(broadcastValidationResult); } diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidator.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidator.java index bc8fa6a9075..f6eda8e336e 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidator.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidator.java @@ -15,18 +15,22 @@ import static tech.pegasys.teku.infrastructure.async.SafeFuture.completedFuture; import static tech.pegasys.teku.spec.config.Constants.VALID_BLOCK_SET_SIZE; +import static tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult.BLOCK_ALREADY_SEEN_FOR_SLOT_PROPOSER; +import static tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult.EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER; +import static tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult.FIRST_BLOCK_FOR_SLOT_PROPOSER; import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.ignore; import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.reject; +import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ValidationResultSubCode.IGNORE_ALREADY_SEEN; +import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ValidationResultSubCode.IGNORE_EQUIVOCATION_DETECTED; -import com.google.common.base.Objects; +import java.util.Map; import java.util.Optional; -import java.util.Set; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.bytes.Bytes32; import tech.pegasys.teku.infrastructure.async.SafeFuture; -import tech.pegasys.teku.infrastructure.collections.LimitedSet; +import tech.pegasys.teku.infrastructure.collections.LimitedMap; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.constants.Domain; @@ -43,8 +47,8 @@ public class BlockGossipValidator { private final GossipValidationHelper gossipValidationHelper; private final ReceivedBlockEventsChannel receivedBlockEventsChannelPublisher; - private final Set receivedValidBlockInfoSet = - LimitedSet.createSynchronized(VALID_BLOCK_SET_SIZE); + private final Map receivedValidBlockInfoSet = + LimitedMap.createNonSynchronized(VALID_BLOCK_SET_SIZE); public BlockGossipValidator( final Spec spec, @@ -76,14 +80,18 @@ public SafeFuture validate( private SafeFuture internalValidate( final SignedBeaconBlock block, final boolean isLocallyProduced) { - if (gossipValidationHelper.isSlotFinalized(block.getSlot()) - || !blockIsFirstBlockWithValidSignatureForSlot(block)) { - LOG.trace( - "BlockValidator: Block is either too old or is not the first block with valid signature for " - + "its slot. It will be dropped"); + if (gossipValidationHelper.isSlotFinalized(block.getSlot())) { + LOG.trace("BlockValidator: Block is too old. It will be dropped"); return completedFuture(InternalValidationResult.IGNORE); } + final InternalValidationResult intermediateValidationResult = + equivocationCheckResultToInternalValidationResult(performBlockEquivocationCheck(block)); + + if (!intermediateValidationResult.isAccept()) { + return completedFuture(intermediateValidationResult); + } + if (gossipValidationHelper.isSlotFromFuture(block.getSlot())) { LOG.trace("BlockValidator: Block is from the future. It will be saved for future processing"); return completedFuture(InternalValidationResult.SAVE_FOR_FUTURE); @@ -163,21 +171,57 @@ private SafeFuture internalValidate( // BroadcastValidationLevel.CONSENSUS_EQUIVOCATION is used (one at the beginning of // the blocks import, // another after consensus validation) - final boolean equivocationCheckSuccessful = + final EquivocationCheckResult secondEquivocationCheckResult = isLocallyProduced - ? blockIsFirstBlockWithValidSignatureForSlot(block) - : receivedValidBlockInfoSet.add(new SlotAndProposer(block)); - if (!equivocationCheckSuccessful) { - return ignore( - "Block is not the first with valid signature for its slot. It will be dropped."); - } + ? performBlockEquivocationCheck(false, block) + : performBlockEquivocationCheck(true, block); - return InternalValidationResult.ACCEPT; + return equivocationCheckResultToInternalValidationResult( + secondEquivocationCheckResult); }); } - boolean blockIsFirstBlockWithValidSignatureForSlot(final SignedBeaconBlock block) { - return !receivedValidBlockInfoSet.contains(new SlotAndProposer(block)); + private InternalValidationResult equivocationCheckResultToInternalValidationResult( + final EquivocationCheckResult equivocationCheckResult) { + return switch (equivocationCheckResult) { + case FIRST_BLOCK_FOR_SLOT_PROPOSER -> InternalValidationResult.ACCEPT; + case EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER -> ignore( + IGNORE_EQUIVOCATION_DETECTED, "Equivocating block detected. It will be dropped."); + case BLOCK_ALREADY_SEEN_FOR_SLOT_PROPOSER -> ignore( + IGNORE_ALREADY_SEEN, + "Block is not the first with valid signature for its slot. It will be dropped."); + }; + } + + private synchronized EquivocationCheckResult performBlockEquivocationCheck( + final boolean add, final SignedBeaconBlock block) { + final SlotAndProposer slotAndProposer = new SlotAndProposer(block); + + final Optional maybePreviouslySeenBlockRoot = + Optional.ofNullable(receivedValidBlockInfoSet.get(slotAndProposer)); + + if (maybePreviouslySeenBlockRoot.isEmpty()) { + if (add) { + receivedValidBlockInfoSet.put(slotAndProposer, block.getRoot()); + } + return FIRST_BLOCK_FOR_SLOT_PROPOSER; + } + + if (maybePreviouslySeenBlockRoot.get().equals(block.getRoot())) { + return BLOCK_ALREADY_SEEN_FOR_SLOT_PROPOSER; + } + + return EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER; + } + + EquivocationCheckResult performBlockEquivocationCheck(final SignedBeaconBlock block) { + return performBlockEquivocationCheck(false, block); + } + + public enum EquivocationCheckResult { + FIRST_BLOCK_FOR_SLOT_PROPOSER, + BLOCK_ALREADY_SEEN_FOR_SLOT_PROPOSER, + EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER } private boolean blockSignatureIsValidWithRespectToProposerIndex( @@ -194,30 +238,9 @@ private boolean blockSignatureIsValidWithRespectToProposerIndex( signingRoot, block.getProposerIndex(), block.getSignature(), postState); } - private static class SlotAndProposer { - private final UInt64 slot; - private final UInt64 proposerIndex; - + private record SlotAndProposer(UInt64 slot, UInt64 proposerIndex) { public SlotAndProposer(final SignedBeaconBlock block) { - this.slot = block.getSlot(); - this.proposerIndex = block.getMessage().getProposerIndex(); - } - - @Override - public boolean equals(final Object o) { - if (this == o) { - return true; - } - if (!(o instanceof SlotAndProposer)) { - return false; - } - SlotAndProposer that = (SlotAndProposer) o; - return Objects.equal(slot, that.slot) && Objects.equal(proposerIndex, that.proposerIndex); - } - - @Override - public int hashCode() { - return Objects.hashCode(slot, proposerIndex); + this(block.getSlot(), block.getMessage().getProposerIndex()); } } } diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/InternalValidationResult.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/InternalValidationResult.java index 5478fe51751..95fb28be8aa 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/InternalValidationResult.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/InternalValidationResult.java @@ -17,6 +17,7 @@ import com.google.errorprone.annotations.FormatMethod; import java.util.Objects; import java.util.Optional; +import tech.pegasys.teku.statetransition.validation.ValidationResultCode.ValidationResultSubCode; public class InternalValidationResult { @@ -30,20 +31,33 @@ public class InternalValidationResult { private final ValidationResultCode validationResultCode; private final Optional description; + private final Optional validationResultSubCode; private InternalValidationResult( - final ValidationResultCode validationResultCode, final Optional description) { + final ValidationResultCode validationResultCode, + final Optional validationResultSubCode, + final Optional description) { this.validationResultCode = validationResultCode; + this.validationResultSubCode = validationResultSubCode; this.description = description; } static InternalValidationResult create(final ValidationResultCode validationResultCode) { - return new InternalValidationResult(validationResultCode, Optional.empty()); + return new InternalValidationResult(validationResultCode, Optional.empty(), Optional.empty()); } public static InternalValidationResult create( final ValidationResultCode validationResultCode, final String description) { - return new InternalValidationResult(validationResultCode, Optional.of(description)); + return new InternalValidationResult( + validationResultCode, Optional.empty(), Optional.of(description)); + } + + public static InternalValidationResult create( + final ValidationResultCode validationResultCode, + final ValidationResultSubCode subCode, + final String description) { + return new InternalValidationResult( + validationResultCode, Optional.of(subCode), Optional.of(description)); } @FormatMethod @@ -58,10 +72,25 @@ public static InternalValidationResult ignore( return create(ValidationResultCode.IGNORE, String.format(descriptionTemplate, args)); } + @FormatMethod + public static InternalValidationResult ignore( + final ValidationResultSubCode validationResultSubCode, + final String descriptionTemplate, + final Object... args) { + return create( + ValidationResultCode.IGNORE, + validationResultSubCode, + String.format(descriptionTemplate, args)); + } + public ValidationResultCode code() { return validationResultCode; } + public Optional getValidationResultSubCode() { + return validationResultSubCode; + } + public Optional getDescription() { return description; } @@ -91,6 +120,13 @@ public boolean isIgnore() { return this.validationResultCode.equals(ValidationResultCode.IGNORE); } + public boolean isIgnoreAlreadySeen() { + return isIgnore() + && this.validationResultSubCode + .map(subCode -> subCode.equals(ValidationResultSubCode.IGNORE_ALREADY_SEEN)) + .orElse(false); + } + public boolean isReject() { return this.validationResultCode.equals(ValidationResultCode.REJECT); } diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/ValidationResultCode.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/ValidationResultCode.java index 038d08ea6b0..151af4b978a 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/ValidationResultCode.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/ValidationResultCode.java @@ -17,5 +17,10 @@ public enum ValidationResultCode { ACCEPT, SAVE_FOR_FUTURE, IGNORE, - REJECT + REJECT; + + public enum ValidationResultSubCode { + IGNORE_EQUIVOCATION_DETECTED, + IGNORE_ALREADY_SEEN + } } diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorTest.java index 77a65241836..3d36ea16fcb 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockBroadcastValidatorTest.java @@ -26,6 +26,8 @@ import static tech.pegasys.teku.statetransition.validation.BlockBroadcastValidator.BroadcastValidationResult.FINAL_EQUIVOCATION_FAILURE; import static tech.pegasys.teku.statetransition.validation.BlockBroadcastValidator.BroadcastValidationResult.GOSSIP_FAILURE; import static tech.pegasys.teku.statetransition.validation.BlockBroadcastValidator.BroadcastValidationResult.SUCCESS; +import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ValidationResultSubCode.IGNORE_ALREADY_SEEN; +import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ValidationResultSubCode.IGNORE_EQUIVOCATION_DETECTED; import java.util.Arrays; import java.util.stream.Stream; @@ -41,6 +43,7 @@ import tech.pegasys.teku.spec.datastructures.validator.BroadcastValidationLevel; import tech.pegasys.teku.spec.logic.common.statetransition.results.BlockImportResult; import tech.pegasys.teku.spec.util.DataStructureUtil; +import tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult; public class BlockBroadcastValidatorTest { private final Spec spec = TestSpecFactory.createMinimalPhase0(); @@ -66,6 +69,22 @@ public void shouldReturnSuccessWhenValidationIsGossipAndGossipValidationReturnsA verifyNoMoreInteractions(blockGossipValidator); } + @Test + public void + shouldReturnSuccessWhenValidationIsGossipAndGossipValidationReturnsIgnoreAlreadySeen() { + when(blockGossipValidator.validate(eq(block), eq(true))) + .thenReturn( + SafeFuture.completedFuture( + InternalValidationResult.ignore(IGNORE_ALREADY_SEEN, "already seen"))); + + prepareBlockBroadcastValidator(GOSSIP); + + assertThat(blockBroadcastValidator.getResult()) + .isCompletedWithValueMatching(result -> result.equals(SUCCESS)); + verify(blockGossipValidator).validate(eq(block), eq(true)); + verifyNoMoreInteractions(blockGossipValidator); + } + @Test public void shouldReturnSuccessWhenValidationIsGossipAndGossipValidationReturnsAcceptEvenWhenBlockImportFails() { @@ -150,13 +169,15 @@ public void shouldReturnConsensusFailureImmediatelyWhenConsensusCompleteExceptio verifyNoMoreInteractions(blockGossipValidator); } - @Test - public void shouldReturnSuccessWhenSecondEquivocationCheckIsValidated() { + @ParameterizedTest + @EnumSource(value = EquivocationCheckResult.class) + public void shouldReturnFinalEquivocationFailureOnlyForEquivocatingBlocks( + final EquivocationCheckResult equivocationCheckResult) { when(blockGossipValidator.validate(eq(block), eq(true))) .thenReturn(SafeFuture.completedFuture(InternalValidationResult.ACCEPT)); - when(blockGossipValidator.blockIsFirstBlockWithValidSignatureForSlot(eq(block))) - .thenReturn(Boolean.TRUE); + when(blockGossipValidator.performBlockEquivocationCheck(eq(block))) + .thenReturn(equivocationCheckResult); prepareBlockBroadcastValidator(CONSENSUS_AND_EQUIVOCATION); @@ -168,28 +189,16 @@ public void shouldReturnSuccessWhenSecondEquivocationCheckIsValidated() { blockImportResult.completeExceptionally(new RuntimeException("error")); assertThat(blockBroadcastValidator.getResult()) - .isCompletedWithValueMatching(result -> result.equals(SUCCESS)); - verify(blockGossipValidator).validate(eq(block), eq(true)); - verify(blockGossipValidator).blockIsFirstBlockWithValidSignatureForSlot(eq(block)); - verifyNoMoreInteractions(blockGossipValidator); - } - - @Test - public void shouldReturnFinalEquivocationFailureWhenSecondEquivocationCheckFails() { - when(blockGossipValidator.validate(eq(block), eq(true))) - .thenReturn(SafeFuture.completedFuture(InternalValidationResult.ACCEPT)); - - when(blockGossipValidator.blockIsFirstBlockWithValidSignatureForSlot(eq(block))) - .thenReturn(Boolean.FALSE); - - prepareBlockBroadcastValidator(CONSENSUS_AND_EQUIVOCATION); - - blockBroadcastValidator.onConsensusValidationSucceeded(); - - assertThat(blockBroadcastValidator.getResult()) - .isCompletedWithValueMatching(result -> result.equals(FINAL_EQUIVOCATION_FAILURE)); + .isCompletedWithValueMatching( + result -> { + if (equivocationCheckResult.equals( + EquivocationCheckResult.EQUIVOCATING_BLOCK_FOR_SLOT_PROPOSER)) { + return result.equals(FINAL_EQUIVOCATION_FAILURE); + } + return result.equals(SUCCESS); + }); verify(blockGossipValidator).validate(eq(block), eq(true)); - verify(blockGossipValidator).blockIsFirstBlockWithValidSignatureForSlot(eq(block)); + verify(blockGossipValidator).performBlockEquivocationCheck(eq(block)); verifyNoMoreInteractions(blockGossipValidator); } @@ -199,6 +208,10 @@ private static Stream provideBroadcastValidationsAndGossipFailures() broadcastValidation -> Stream.of( Arguments.of(broadcastValidation, InternalValidationResult.IGNORE), + Arguments.of( + broadcastValidation, + InternalValidationResult.ignore( + IGNORE_EQUIVOCATION_DETECTED, "equivocation detected")), Arguments.of(broadcastValidation, InternalValidationResult.SAVE_FOR_FUTURE))); } diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidatorTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidatorTest.java index 1d6735937ee..ecadad6210d 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidatorTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockGossipValidatorTest.java @@ -14,7 +14,6 @@ package tech.pegasys.teku.statetransition.validation; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static tech.pegasys.teku.infrastructure.unsigned.UInt64.ONE; @@ -43,6 +42,7 @@ import tech.pegasys.teku.spec.generator.ChainBuilder.BlockOptions; import tech.pegasys.teku.spec.logic.common.block.AbstractBlockProcessor; import tech.pegasys.teku.statetransition.block.ReceivedBlockEventsChannel; +import tech.pegasys.teku.statetransition.validation.BlockGossipValidator.EquivocationCheckResult; import tech.pegasys.teku.storage.client.ChainUpdater; import tech.pegasys.teku.storage.client.RecentChainData; import tech.pegasys.teku.storage.storageSystem.InMemoryStorageSystemBuilder; @@ -324,7 +324,8 @@ void shouldNotTrackLocallyProducedBlocks() { storageSystem.chainUpdater().setCurrentSlot(nextSlot); assertResultIsAccept(block, blockGossipValidator.validate(block, true)); - assertTrue(blockGossipValidator.blockIsFirstBlockWithValidSignatureForSlot(block)); + assertThat(blockGossipValidator.performBlockEquivocationCheck(block)) + .isEqualByComparingTo(EquivocationCheckResult.FIRST_BLOCK_FOR_SLOT_PROPOSER); } @TestTemplate