Ignore aggregate attestations that are a subset of aggregates we've already seen

CHANGELOG.md

@@ -24,6 +24,7 @@ For information on changes in released versions of Teku, see the [releases page]
 - Added `is_optimistic` field to `/eth/v1/node/syncing` response.
 - Using execution engine endpoint as Eth1 endpoint when latter is not provided.
 - Check `Eth1Address` checksum ([EIP-55](https://eips.ethereum.org/EIPS/eip-55)) if address is mixed-case.
+- Ignore aggregate attestation gossip that does not include any new validators.
 
 ### Bug Fixes
 - Added stricter limits on attestation pool size. 

ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/Constants.java

@@ -32,6 +32,8 @@ public class Constants {
   public static final int VALID_BLOCK_SET_SIZE = 1000;
   // Target holding two slots worth of aggregators (16 aggregators, 64 committees and 2 slots)
   public static final int VALID_AGGREGATE_SET_SIZE = 16 * 64 * 2;
+  // Target 2 different attestation data (aggregators normally agree) for two slots
+  public static final int VALID_ATTESTATION_DATA_SET_SIZE = 2 * 64 * 2;
   public static final int VALID_VALIDATOR_SET_SIZE = 10000;
   public static final int VALID_CONTRIBUTION_AND_PROOF_SET_SIZE = 10000;
   public static final int VALID_SYNC_COMMITTEE_MESSAGE_SET_SIZE = 10000;

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/util/SeenAggregatesCache.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2022 ConsenSys AG.
+ *
+ * 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.
+ */
+
+package tech.pegasys.teku.statetransition.util;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import org.apache.tuweni.bytes.Bytes32;
+import tech.pegasys.teku.infrastructure.collections.LimitedMap;
+import tech.pegasys.teku.infrastructure.collections.LimitedSet;
+import tech.pegasys.teku.infrastructure.ssz.collections.SszBitlist;
+
+public class SeenAggregatesCache {
+
+  private final Map<Bytes32, Set<SszBitlist>> seenAggregationBitsByDataRoot;
+  private final int aggregateSetSize;
+
+  public SeenAggregatesCache(final int rootCacheSize, final int aggregateSetSize) {
+    this.seenAggregationBitsByDataRoot = LimitedMap.create(rootCacheSize);
+    this.aggregateSetSize = aggregateSetSize;
+  }
+
+  public boolean add(final Bytes32 root, final SszBitlist aggregationBits) {
+    final Set<SszBitlist> seenBitlists =
+        seenAggregationBitsByDataRoot.computeIfAbsent(
+            root,
+            // Aim to hold all aggregation bits but have a limit for safety.
+            // Normally we'd have far fewer as we avoid adding subsets.
+            key -> LimitedSet.create(aggregateSetSize));
+    if (isAlreadySeen(seenBitlists, aggregationBits)) {
+      return false;
+    }
+    return seenBitlists.add(aggregationBits);
+  }
+
+  public boolean isAlreadySeen(final Bytes32 root, final SszBitlist aggregationBits) {
+    final Set<SszBitlist> seenAggregates =
+        seenAggregationBitsByDataRoot.getOrDefault(root, Collections.emptySet());
+    return isAlreadySeen(seenAggregates, aggregationBits);
+  }
+
+  private boolean isAlreadySeen(
+      final Set<SszBitlist> seenAggregates, final SszBitlist aggregationBits) {
+    return seenAggregates.stream().anyMatch(seen -> seen.isSuperSetOf(aggregationBits));
+  }
+}

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/validation/AggregateAttestationValidator.java

@@ -16,6 +16,8 @@
 import static java.lang.Math.toIntExact;
 import static tech.pegasys.teku.infrastructure.async.SafeFuture.completedFuture;
 import static tech.pegasys.teku.spec.config.Constants.VALID_AGGREGATE_SET_SIZE;
+import static tech.pegasys.teku.spec.config.Constants.VALID_ATTESTATION_DATA_SET_SIZE;
+import static tech.pegasys.teku.spec.constants.ValidatorConstants.TARGET_AGGREGATORS_PER_COMMITTEE;
 import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.ignore;
 import static tech.pegasys.teku.statetransition.validation.InternalValidationResult.reject;
 
@@ -33,6 +35,7 @@
 import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.collections.LimitedSet;
+import tech.pegasys.teku.infrastructure.ssz.collections.SszBitlist;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.SpecVersion;
@@ -44,13 +47,14 @@
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
 import tech.pegasys.teku.spec.logic.common.util.AsyncBLSSignatureVerifier;
 import tech.pegasys.teku.spec.logic.common.util.AsyncBatchBLSSignatureVerifier;
+import tech.pegasys.teku.statetransition.util.SeenAggregatesCache;
 
 public class AggregateAttestationValidator {
   private static final Logger LOG = LogManager.getLogger();
   private final Set<AggregatorIndexAndEpoch> receivedAggregatorIndexAndEpochs =
       LimitedSet.create(VALID_AGGREGATE_SET_SIZE);
-  private final Set<Bytes32> receivedValidAggregations =
-      LimitedSet.create(VALID_AGGREGATE_SET_SIZE);
+  private final SeenAggregatesCache seenAggregationBits =
+      new SeenAggregatesCache(VALID_ATTESTATION_DATA_SET_SIZE, TARGET_AGGREGATORS_PER_COMMITTEE);
   private final AttestationValidator attestationValidator;
   private final Spec spec;
   private final AsyncBLSSignatureVerifier signatureVerifier;
@@ -65,7 +69,8 @@ public AggregateAttestationValidator(
   }
 
   public void addSeenAggregate(final ValidateableAttestation attestation) {
-    receivedValidAggregations.add(attestation.hashTreeRoot());
+    seenAggregationBits.add(
+        attestation.getData().hashTreeRoot(), attestation.getAttestation().getAggregationBits());
   }
 
   public SafeFuture<InternalValidationResult> validate(final ValidateableAttestation attestation) {
@@ -81,8 +86,10 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
     if (receivedAggregatorIndexAndEpochs.contains(aggregatorIndexAndEpoch)) {
       return completedFuture(ignore("Ignoring duplicate aggregate"));
     }
-    if (receivedValidAggregations.contains(attestation.hashTreeRoot())) {
-      return completedFuture(ignore("Ignoring duplicate aggregate based on hash tree root"));
+
+    final SszBitlist aggregationBits = attestation.getAttestation().getAggregationBits();
+    if (seenAggregationBits.isAlreadySeen(attestation.getData().hashTreeRoot(), aggregationBits)) {
+      return completedFuture(ignore("Ignoring duplicate aggregate based on aggregation bits"));
     }
 
     final AsyncBatchBLSSignatureVerifier signatureVerifier =
@@ -157,8 +164,10 @@ public SafeFuture<InternalValidationResult> validate(final ValidateableAttestati
                         if (!receivedAggregatorIndexAndEpochs.add(aggregatorIndexAndEpoch)) {
                           return ignore("Ignoring duplicate aggregate");
                         }
-                        if (!receivedValidAggregations.add(attestation.hashTreeRoot())) {
-                          return ignore("Ignoring duplicate aggregate based on hash tree root");
+                        if (!seenAggregationBits.add(
+                            attestation.getData().hashTreeRoot(),
+                            attestation.getAttestation().getAggregationBits())) {
+                          return ignore("Ignoring duplicate aggregate based on aggregation bits");
                         }
                         return resultWithState.getResult();
                       });

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/util/SeenAggregatesCacheTest.java

@@ -0,0 +1,119 @@
+/*
+ * Copyright 2022 ConsenSys AG.
+ *
+ * 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.
+ */
+
+package tech.pegasys.teku.statetransition.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.tuweni.bytes.Bytes32;
+import org.junit.jupiter.api.Test;
+import tech.pegasys.teku.infrastructure.ssz.collections.SszBitlist;
+import tech.pegasys.teku.infrastructure.ssz.schema.collections.SszBitlistSchema;
+import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.TestSpecFactory;
+import tech.pegasys.teku.spec.util.DataStructureUtil;
+
+class SeenAggregatesCacheTest {
+
+  private final Spec spec = TestSpecFactory.createDefault();
+  private final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec);
+  private final SszBitlistSchema<SszBitlist> bitlistSchema = SszBitlistSchema.create(10);
+  private final SeenAggregatesCache cache = new SeenAggregatesCache(3, 4);
+
+  @Test
+  void isAlreadySeen_shouldBeTrueWhenValueIsEqual() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.isAlreadySeen(root, bitlist(true, false, true, false))).isTrue();
+  }
+
+  @Test
+  void isAlreadySeen_shouldBeTrueWhenValueIsSubset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.isAlreadySeen(root, bitlist(true, false, false, false))).isTrue();
+  }
+
+  @Test
+  void isAlreadySeen_shouldBeFalseWhenSuperset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.isAlreadySeen(root, bitlist(true, true, false, false))).isFalse();
+  }
+
+  @Test
+  void isAlreadySeen_shouldBeFalseWhenNoIndividualSeenValueIsASuperset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    // Combined we've seen all validators
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+    assertThat(cache.add(root, bitlist(false, true, false, true))).isTrue();
+
+    // But this aggregate isn't a subset of either previously seen value
+    assertThat(cache.isAlreadySeen(root, bitlist(true, true, false, false))).isFalse();
+  }
+
+  @Test
+  void isAlreadySeen_shouldBeFalseWhenRootIsDifferent() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    // Combined we've seen all validators
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    // But this aggregate isn't a subset of either previously seen value
+    assertThat(
+            cache.isAlreadySeen(
+                dataStructureUtil.randomBytes32(), bitlist(true, false, true, false)))
+        .isFalse();
+  }
+
+  @Test
+  void add_shouldBeFalseWhenValueIsEqual() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isFalse();
+  }
+
+  @Test
+  void add_shouldBeFalseWhenValueIsSubset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.add(root, bitlist(true, false, false, false))).isFalse();
+  }
+
+  @Test
+  void add_shouldBeTrueWhenSuperset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+
+    assertThat(cache.add(root, bitlist(true, true, false, false))).isTrue();
+  }
+
+  @Test
+  void add_shouldBeTrueWhenNoIndividualSeenValueIsASuperset() {
+    final Bytes32 root = dataStructureUtil.randomBytes32();
+    // Combined we've seen all validators
+    assertThat(cache.add(root, bitlist(true, false, true, false))).isTrue();
+    assertThat(cache.add(root, bitlist(false, true, false, true))).isTrue();
+
+    // But this aggregate isn't a subset of either previously seen value
+    assertThat(cache.add(root, bitlist(true, true, false, false))).isTrue();
+  }
+
+  private SszBitlist bitlist(final Boolean... values) {
+    return bitlistSchema.of(values);
+  }
+}

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/validation/AggregateAttestationValidatorTest.java

@@ -38,6 +38,7 @@
 import tech.pegasys.teku.core.AttestationGenerator;
 import tech.pegasys.teku.core.ChainBuilder;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
+import tech.pegasys.teku.infrastructure.ssz.collections.SszBitlist;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.SpecVersion;
@@ -47,6 +48,7 @@
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBlockAndState;
 import tech.pegasys.teku.spec.datastructures.blocks.StateAndBlockSummary;
 import tech.pegasys.teku.spec.datastructures.interop.MockStartValidatorKeyPairFactory;
+import tech.pegasys.teku.spec.datastructures.operations.AggregateAndProof;
 import tech.pegasys.teku.spec.datastructures.operations.AggregateAndProof.AggregateAndProofSchema;
 import tech.pegasys.teku.spec.datastructures.operations.Attestation;
 import tech.pegasys.teku.spec.datastructures.operations.AttestationData;
@@ -118,7 +120,7 @@ class AggregateAttestationValidatorTest {
 
   private final AsyncBLSSignatureVerifier signatureVerifier =
       AsyncBLSSignatureVerifier.wrap(BLSSignatureVerifier.SIMPLE);
-  private final AggregateAttestationValidator validator =
+  private AggregateAttestationValidator validator =
       new AggregateAttestationValidator(spec, attestationValidator, signatureVerifier);
   private SignedBlockAndState bestBlock;
   private SignedBlockAndState genesis;
@@ -139,6 +141,12 @@ public void setUp() {
     bestBlock = chainUpdater.addNewBestBlock();
   }
 
+  private void disableSignatureVerification() {
+    validator =
+        new AggregateAttestationValidator(
+            spec, attestationValidator, AsyncBLSSignatureVerifier.wrap(BLSSignatureVerifier.NO_OP));
+  }
+
   @Test
   public void shouldReturnValidForValidAggregate() {
     final StateAndBlockSummary chainHead = storageSystem.getChainHead();
@@ -301,6 +309,83 @@ public void shouldOnlyAcceptFirstAggregateWithSameHashTreeRootWhenPassedSeenAggr
         .isCompletedWithValueMatching(InternalValidationResult::isIgnore);
   }
 
+  @Test
+  void shouldIgnoreAggregateWhenAlreadySeenAllAttestingValidators() {
+    disableSignatureVerification();
+    final StateAndBlockSummary chainHead = storageSystem.getChainHead();
+    final SignedAggregateAndProof validAggregate = generator.validAggregateAndProof(chainHead);
+    final AttestationData attestationData = validAggregate.getMessage().getAggregate().getData();
+    final ValidateableAttestation smallAttestation =
+        createValidAggregate(ONE, attestationData, true, false, false);
+    final ValidateableAttestation largeAttestation =
+        createValidAggregate(
+            validAggregate.getMessage().getIndex(), attestationData, true, true, true);
+
+    validator.addSeenAggregate(largeAttestation);
+    assertThat(validator.validate(smallAttestation))
+        .isCompletedWithValueMatching(InternalValidationResult::isIgnore);
+  }
+
+  @Test
+  void shouldAcceptAggregateWhenNotAllAttestingValidatorsSeen() {
+    disableSignatureVerification();
+    final StateAndBlockSummary chainHead = storageSystem.getChainHead();
+    final AggregateAndProof validAggregate =
+        generator.validAggregateAndProof(chainHead).getMessage();
+    final AttestationData attestationData = validAggregate.getAggregate().getData();
+    final ValidateableAttestation smallAttestation =
+        createValidAggregate(ONE, attestationData, true, false, false);
+    final ValidateableAttestation largeAttestation =
+        createValidAggregate(validAggregate.getIndex(), attestationData, true, true, true);
+
+    validator.addSeenAggregate(smallAttestation);
+    assertThat(validator.validate(largeAttestation))
+        .isCompletedWithValueMatching(InternalValidationResult::isAccept);
+  }
+
+  @Test
+  void shouldAcceptAggregateWhenNoSupersetOfValidatorsSeen() {
+    disableSignatureVerification();
+    final StateAndBlockSummary chainHead = storageSystem.getChainHead();
+    final AggregateAndProof validAggreagte =
+        generator.validAggregateAndProof(chainHead).getMessage();
+    final AttestationData attestationData = validAggreagte.getAggregate().getData();
+    final ValidateableAttestation smallAttestation1 =
+        createValidAggregate(ONE, attestationData, true, false, true);
+    final ValidateableAttestation smallAttestation2 =
+        createValidAggregate(UInt64.valueOf(2), attestationData, false, true, true);
+    final ValidateableAttestation attestation =
+        createValidAggregate(validAggreagte.getIndex(), attestationData, true, true, false);
+
+    validator.addSeenAggregate(smallAttestation1);
+    validator.addSeenAggregate(smallAttestation2);
+
+    // Accept because neither of the small attestations includes both the validators this one does
+    assertThat(validator.validate(attestation))
+        .isCompletedWithValueMatching(InternalValidationResult::isAccept);
+  }
+
+  private ValidateableAttestation createValidAggregate(
+      final UInt64 validatorIndex,
+      final AttestationData attestationData,
+      final Boolean... aggregationBits) {
+    final SszBitlist aggregateBits =
+        aggregateAndProofSchema
+            .getAttestationSchema()
+            .getAggregationBitsSchema()
+            .of(aggregationBits);
+    final Attestation attestation =
+        aggregateAndProofSchema
+            .getAttestationSchema()
+            .create(aggregateBits, attestationData, BLSSignature.empty());
+    final SignedAggregateAndProof signedAggregate =
+        signedAggregateAndProofSchema.create(
+            aggregateAndProofSchema.create(validatorIndex, attestation, BLSSignature.empty()),
+            BLSSignature.empty());
+    whenAttestationIsValid(signedAggregate);
+    return ValidateableAttestation.aggregateFromValidator(spec, signedAggregate);
+  }
+
   @Test
   public void shouldAcceptAggregateWithSameSlotAndDifferentAggregatorIndex() {
     final StateAndBlockSummary chainHead = storageSystem.getChainHead();