From a70fe536c2dbdfbe4f031204edcdb4dc8113dcbc Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Thu, 9 Sep 2021 16:56:08 +1000 Subject: [PATCH] Remove the requirement to cache seen attestations (#4332) - AttestationValidator no longer looks for attestations that have been seen, as it is filtered by the gossip cache Signed-off-by: Paul Harris --- .../attestation/AttestationManager.java | 2 - .../validation/AttestationValidator.java | 79 +------------------ .../validation/AttestationValidatorTest.java | 38 ++------- .../pegasys/teku/util/config/Constants.java | 1 - 4 files changed, 8 insertions(+), 112 deletions(-) diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AttestationManager.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AttestationManager.java index a0f2ad46329..872e93910aa 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AttestationManager.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AttestationManager.java @@ -232,8 +232,6 @@ private void sendToSubscribersIfProducedLocally(ValidateableAttestation attestat if (attestation.isAggregate()) { aggregateValidator.addSeenAggregate(attestation); - } else { - attestationValidator.addSeenAttestation(attestation); } notifyAttestationsToSendSubscribers(attestation); diff --git a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java index 7380e796a57..c4c13bd3d89 100644 --- a/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java +++ b/ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java @@ -19,16 +19,12 @@ import static tech.pegasys.teku.infrastructure.unsigned.UInt64.ZERO; import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ACCEPT; import static tech.pegasys.teku.util.config.Constants.ATTESTATION_PROPAGATION_SLOT_RANGE; -import static tech.pegasys.teku.util.config.Constants.VALID_ATTESTATION_SET_SIZE; import it.unimi.dsi.fastutil.ints.IntList; -import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; -import java.util.Set; 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.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.datastructures.attestation.ValidateableAttestation; @@ -41,13 +37,10 @@ import tech.pegasys.teku.util.config.Constants; public class AttestationValidator { - private static final UInt64 MAX_FUTURE_SLOT_ALLOWANCE = UInt64.valueOf(3); private static final UInt64 MAXIMUM_GOSSIP_CLOCK_DISPARITY = UInt64.valueOf(Constants.MAXIMUM_GOSSIP_CLOCK_DISPARITY); - private final Set receivedValidAttestations = - LimitedSet.create(VALID_ATTESTATION_SET_SIZE); private final Spec spec; private final RecentChainData recentChainData; private final AsyncBLSSignatureVerifier signatureVerifier; @@ -70,30 +63,7 @@ public SafeFuture validate( } return singleOrAggregateAttestationChecks( - signatureVerifier, - validateableAttestation, - validateableAttestation.getReceivedSubnetId()) - .thenApply( - result -> { - if (result.code() != ACCEPT) { - return result; - } - - return addAndCheckFirstValidAttestation(attestation); - }); - } - - public void addSeenAttestation(final ValidateableAttestation attestation) { - receivedValidAttestations.add(getValidatorAndTargetEpoch(attestation.getAttestation())); - } - - private InternalValidationResult addAndCheckFirstValidAttestation(final Attestation attestation) { - // The attestation is the first valid attestation received for the participating validator for - // the slot, attestation.data.slot. - if (!receivedValidAttestations.add(getValidatorAndTargetEpoch(attestation))) { - return InternalValidationResult.IGNORE; - } - return InternalValidationResult.ACCEPT; + signatureVerifier, validateableAttestation, validateableAttestation.getReceivedSubnetId()); } private InternalValidationResult singleAttestationChecks(final Attestation attestation) { @@ -103,12 +73,6 @@ private InternalValidationResult singleAttestationChecks(final Attestation attes if (bitCount != 1) { return InternalValidationResult.reject("Attestation has %s bits set instead of 1", bitCount); } - - // The attestation is the first valid attestation received for the participating validator for - // the slot, attestation.data.slot. - if (receivedValidAttestations.contains(getValidatorAndTargetEpoch(attestation))) { - return InternalValidationResult.IGNORE; - } return InternalValidationResult.ACCEPT; } @@ -261,13 +225,6 @@ public SafeFuture> resolveStateForAttestation( } } - private ValidatorAndTargetEpoch getValidatorAndTargetEpoch(final Attestation attestation) { - return new ValidatorAndTargetEpoch( - attestation.getData().getTarget().getEpoch(), - attestation.getData().getIndex(), - attestation.getAggregationBits().streamAllSetBits().findFirst().orElseThrow()); - } - private boolean isCurrentTimeBeforeMinimumAttestationBroadcastTime( final Attestation attestation, final UInt64 currentTimeMillis) { final UInt64 minimumBroadcastTimeMillis = @@ -319,40 +276,6 @@ private UInt64 maximumBroadcastTimeMillis(final UInt64 attestationSlot) { return secondsToMillis(lastAllowedTime).plus(MAXIMUM_GOSSIP_CLOCK_DISPARITY); } - private static class ValidatorAndTargetEpoch { - private final UInt64 targetEpoch; - // Validator is identified via committee index and position to avoid resolving the actual - // validator ID before checking for duplicates - private final UInt64 committeeIndex; - private final int committeePosition; - - private ValidatorAndTargetEpoch( - final UInt64 targetEpoch, final UInt64 committeeIndex, final int committeePosition) { - this.targetEpoch = targetEpoch; - this.committeeIndex = committeeIndex; - this.committeePosition = committeePosition; - } - - @Override - public boolean equals(final Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - final ValidatorAndTargetEpoch that = (ValidatorAndTargetEpoch) o; - return committeePosition == that.committeePosition - && Objects.equals(targetEpoch, that.targetEpoch) - && Objects.equals(committeeIndex, that.committeeIndex); - } - - @Override - public int hashCode() { - return Objects.hash(targetEpoch, committeeIndex, committeePosition); - } - } - private int secondsPerSlot(final UInt64 slot) { return spec.getSecondsPerSlot(slot); } diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationValidatorTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationValidatorTest.java index e3c6af4bd28..6654f145625 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationValidatorTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AttestationValidatorTest.java @@ -19,13 +19,11 @@ import static org.mockito.Mockito.when; import static tech.pegasys.teku.infrastructure.unsigned.UInt64.ONE; import static tech.pegasys.teku.infrastructure.unsigned.UInt64.ZERO; -import static tech.pegasys.teku.spec.datastructures.util.BeaconStateUtil.get_committee_count_per_slot; import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.ACCEPT; import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.IGNORE; import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.REJECT; import static tech.pegasys.teku.statetransition.validation.ValidationResultCode.SAVE_FOR_FUTURE; import static tech.pegasys.teku.util.config.Constants.ATTESTATION_PROPAGATION_SLOT_RANGE; -import static tech.pegasys.teku.util.config.Constants.SLOTS_PER_EPOCH; import java.util.List; import java.util.Optional; @@ -122,11 +120,12 @@ public void shouldReturnValidForValidAttestation() { @Test public void shouldReturnValidForValidAttestation_whenManyBlocksHaveBeenSkipped() { final StateAndBlockSummary head = recentChainData.getChainHead().orElseThrow(); - final UInt64 currentSlot = head.getSlot().plus(SLOTS_PER_EPOCH * 3); + final UInt64 currentSlot = head.getSlot().plus(spec.getSlotsPerEpoch(head.getSlot()) * 3); storageSystem.chainUpdater().setCurrentSlot(currentSlot); final Attestation attestation = - attestationGenerator.validAttestation(head, head.getSlot().plus(SLOTS_PER_EPOCH * 3)); + attestationGenerator.validAttestation( + head, head.getSlot().plus(spec.getSlotsPerEpoch(head.getSlot()) * 3)); assertThat(validate(attestation).code()).isEqualTo(ACCEPT); } @@ -208,36 +207,13 @@ public void shouldRejectAggregatedAttestation() { assertThat(validate(attestation).code()).isEqualTo(REJECT); } - @Test - public void shouldRejectAttestationForSameValidatorAndTargetEpoch() { - final StateAndBlockSummary genesis = recentChainData.getChainHead().orElseThrow(); - chainUpdater.advanceChain(ONE); - - // Slot 1 attestation for the block at slot 1 - final Attestation attestation1 = - attestationGenerator.validAttestation(recentChainData.getChainHead().orElseThrow()); - // Slot 1 attestation from the same validator claiming no block at slot 1 - final Attestation attestation2 = - attestationGenerator - .streamAttestations(genesis, ONE) - .filter(attestation -> hasSameValidators(attestation1, attestation)) - .findFirst() - .orElseThrow(); - - // Sanity check - assertThat(attestation1.getData().getTarget().getEpoch()) - .isEqualTo(attestation2.getData().getTarget().getEpoch()); - assertThat(attestation1.getAggregationBits()).isEqualTo(attestation2.getAggregationBits()); - - assertThat(validate(attestation1).code()).isEqualTo(ACCEPT); - assertThat(validate(attestation2).code()).isEqualTo(IGNORE); - } - @Test public void shouldAcceptAttestationForSameValidatorButDifferentTargetEpoch() { final StateAndBlockSummary genesis = recentChainData.getChainHead().orElseThrow(); final SignedBeaconBlock nextEpochBlock = - chainUpdater.advanceChain(UInt64.valueOf(SLOTS_PER_EPOCH + 1)).getBlock(); + chainUpdater + .advanceChain(UInt64.valueOf(spec.getSlotsPerEpoch(genesis.getSlot()) + 1)) + .getBlock(); // Slot 0 attestation final Attestation attestation1 = attestationGenerator.validAttestation(genesis); @@ -333,7 +309,7 @@ public void shouldRejectAttestationsWithCommitteeIndexNotInTheExpectedRange() { attestation.getAggregationBits(), new AttestationData( data.getSlot(), - get_committee_count_per_slot( + spec.getCommitteeCountPerSlot( blockAndState.getState(), data.getTarget().getEpoch()), data.getBeacon_block_root(), data.getSource(), diff --git a/util/src/main/java/tech/pegasys/teku/util/config/Constants.java b/util/src/main/java/tech/pegasys/teku/util/config/Constants.java index 89379cb6532..1249e3670d6 100644 --- a/util/src/main/java/tech/pegasys/teku/util/config/Constants.java +++ b/util/src/main/java/tech/pegasys/teku/util/config/Constants.java @@ -116,7 +116,6 @@ public class Constants { // Teku Networking Specific public static final int VALID_BLOCK_SET_SIZE = 1000; - public static final int VALID_ATTESTATION_SET_SIZE = 21000; public static final int VALID_AGGREGATE_SET_SIZE = 1000; public static final int VALID_VALIDATOR_SET_SIZE = 10000; public static final int VALID_CONTRIBUTION_AND_PROOF_SET_SIZE = 10000;