fix: remove aggregation bits from seen attestation cache (#7265)

* fix: remove aggregation bits from seen attestation cache

* Allow passing null as aggregationBits to test pre-electra case

* Only create aggregationBits once for the first attestation

* Avoid second getSingleTrueBit call

Author: nflaig

packages/beacon-node/src/api/impl/beacon/pool/index.ts

@@ -105,15 +105,16 @@ export function getBeaconPoolApi({
             // when a validator is configured with multiple beacon node urls, this attestation data may come from another beacon node
             // and the block hasn't been in our forkchoice since we haven't seen / processing that block
             // see https://github.com/ChainSafe/lodestar/issues/5098
-            const {indexedAttestation, subnet, attDataRootHex, committeeIndex, aggregationBits} =
+            const {indexedAttestation, subnet, attDataRootHex, committeeIndex, committeeValidatorIndex, committeeSize} =
               await validateGossipFnRetryUnknownRoot(validateFn, network, chain, slot, beaconBlockRoot);
 
             if (network.shouldAggregate(subnet, slot)) {
               const insertOutcome = chain.attestationPool.add(
                 committeeIndex,
                 attestation,
                 attDataRootHex,
-                aggregationBits
+                committeeValidatorIndex,
+                committeeSize
               );
               metrics?.opPool.attestationPoolInsertOutcome.inc({insertOutcome});
             }

packages/beacon-node/src/chain/opPools/attestationPool.ts

@@ -109,7 +109,8 @@ export class AttestationPool {
     committeeIndex: CommitteeIndex,
     attestation: SingleAttestation,
     attDataRootHex: RootHex,
-    aggregationBits: BitArray | null
+    committeeValidatorIndex: number,
+    committeeSize: number
   ): InsertOutcome {
     const slot = attestation.data.slot;
     const fork = this.config.getForkName(slot);
@@ -149,10 +150,10 @@ export class AttestationPool {
     const aggregate = aggregateByIndex.get(committeeIndex);
     if (aggregate) {
       // Aggregate mutating
-      return aggregateAttestationInto(aggregate, attestation, aggregationBits);
+      return aggregateAttestationInto(aggregate, attestation, committeeValidatorIndex);
     }
     // Create new aggregate
-    aggregateByIndex.set(committeeIndex, attestationToAggregate(attestation, aggregationBits));
+    aggregateByIndex.set(committeeIndex, attestationToAggregate(attestation, committeeValidatorIndex, committeeSize));
     return InsertOutcome.NewData;
   }
 
@@ -224,13 +225,12 @@ export class AttestationPool {
 function aggregateAttestationInto(
   aggregate: AggregateFast,
   attestation: SingleAttestation,
-  aggregationBits: BitArray | null
+  committeeValidatorIndex: number
 ): InsertOutcome {
   let bitIndex: number | null;
 
   if (isElectraSingleAttestation(attestation)) {
-    assert.notNull(aggregationBits, "aggregationBits missing post-electra");
-    bitIndex = aggregationBits.getSingleTrueBit();
+    bitIndex = committeeValidatorIndex;
   } else {
     bitIndex = attestation.aggregationBits.getSingleTrueBit();
   }
@@ -250,12 +250,15 @@ function aggregateAttestationInto(
 /**
  * Format `contribution` into an efficient `aggregate` to add more contributions in with aggregateContributionInto()
  */
-function attestationToAggregate(attestation: SingleAttestation, aggregationBits: BitArray | null): AggregateFast {
+function attestationToAggregate(
+  attestation: SingleAttestation,
+  committeeValidatorIndex: number,
+  committeeSize: number
+): AggregateFast {
   if (isElectraSingleAttestation(attestation)) {
-    assert.notNull(aggregationBits, "aggregationBits missing post-electra to generate aggregate");
     return {
       data: attestation.data,
-      aggregationBits,
+      aggregationBits: BitArray.fromSingleBit(committeeSize, committeeValidatorIndex),
       committeeBits: BitArray.fromSingleBit(MAX_COMMITTEES_PER_SLOT, attestation.committeeIndex),
       signature: signatureFromBytesNoCheck(attestation.signature),
     };

packages/beacon-node/src/chain/seenCache/seenAttestationData.ts

@@ -11,7 +11,6 @@ type AttDataBase64 = string;
 export type AttestationDataCacheEntry = {
   // part of shuffling data, so this does not take memory
   committeeValidatorIndices: Uint32Array;
-  // TODO: remove this? this is available in SingleAttestation
   committeeIndex: CommitteeIndex;
   // IndexedAttestationData signing root, 32 bytes
   signingRoot: Uint8Array;
@@ -21,8 +20,6 @@ export type AttestationDataCacheEntry = {
   // for example in a mainnet node subscribing to all subnets, attestations are processed up to 20k per slot
   attestationData: phase0.AttestationData;
   subnet: number;
-  // aggregationBits only populates post-electra. Pre-electra can use get it directly from attestationOrBytes
-  aggregationBits: BitArray | null;
 };
 
 export enum RejectReason {

packages/beacon-node/src/chain/validation/attestation.ts

@@ -69,7 +69,8 @@ export type AttestationValidationResult = {
   subnet: number;
   attDataRootHex: RootHex;
   committeeIndex: CommitteeIndex;
-  aggregationBits: BitArray | null; // Field populated post-electra only
+  committeeValidatorIndex: number;
+  committeeSize: number;
 };
 
 export type AttestationOrBytes = ApiAttestation | GossipAttestation;
@@ -325,6 +326,7 @@ async function validateAttestationNoSignatureCheck(
   }
 
   let aggregationBits: BitArray | null = null;
+  let committeeValidatorIndex: number | null = null;
   if (!isForkPostElectra(fork)) {
     // [REJECT] The attestation is unaggregated -- that is, it has exactly one participating validator
     // (len([bit for bit in attestation.aggregation_bits if bit]) == 1, i.e. exactly 1 bit is set).
@@ -344,11 +346,7 @@ async function validateAttestationNoSignatureCheck(
         code: AttestationErrorCode.NOT_EXACTLY_ONE_AGGREGATION_BIT_SET,
       });
     }
-  } else {
-    // Populate aggregationBits if cached post-electra, else we populate later
-    if (attestationOrCache.cache && attestationOrCache.cache.aggregationBits !== null) {
-      aggregationBits = attestationOrCache.cache.aggregationBits;
-    }
+    committeeValidatorIndex = bitIndex;
   }
 
   let committeeValidatorIndices: Uint32Array;
@@ -407,10 +405,9 @@ async function validateAttestationNoSignatureCheck(
   if (!isForkPostElectra(fork)) {
     // The validity of aggregation bits are already checked above
     assert.notNull(aggregationBits);
-    const bitIndex = aggregationBits.getSingleTrueBit();
-    assert.notNull(bitIndex);
+    assert.notNull(committeeValidatorIndex);
 
-    validatorIndex = committeeValidatorIndices[bitIndex];
+    validatorIndex = committeeValidatorIndices[committeeValidatorIndex];
     // [REJECT] The number of aggregation bits matches the committee size
     // -- i.e. len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot, data.index)).
     // > TODO: Is this necessary? Lighthouse does not do this check.
@@ -434,17 +431,12 @@ async function validateAttestationNoSignatureCheck(
 
     // [REJECT] The attester is a member of the committee -- i.e.
     // `attestation.attester_index in get_beacon_committee(state, attestation.data.slot, index)`.
-    // If `aggregationBitsElectra` exists, that means we have already cached it. No need to check again
-    if (aggregationBits === null) {
-      // Position of the validator in its committee
-      const committeeValidatorIndex = committeeValidatorIndices.indexOf(validatorIndex);
-      if (committeeValidatorIndex === -1) {
-        throw new AttestationError(GossipAction.REJECT, {
-          code: AttestationErrorCode.ATTESTER_NOT_IN_COMMITTEE,
-        });
-      }
-
-      aggregationBits = BitArray.fromSingleBit(committeeValidatorIndices.length, committeeValidatorIndex);
+    // Position of the validator in its committee
+    committeeValidatorIndex = committeeValidatorIndices.indexOf(validatorIndex);
+    if (committeeValidatorIndex === -1) {
+      throw new AttestationError(GossipAction.REJECT, {
+        code: AttestationErrorCode.ATTESTER_NOT_IN_COMMITTEE,
+      });
     }
   }
 
@@ -517,7 +509,6 @@ async function validateAttestationNoSignatureCheck(
         // root of AttestationData was already cached during getIndexedAttestationSignatureSet
         attDataRootHex,
         attestationData: attData,
-        aggregationBits: isForkPostElectra(fork) ? aggregationBits : null,
       });
     }
   }
@@ -553,7 +544,8 @@ async function validateAttestationNoSignatureCheck(
     signatureSet,
     validatorIndex,
     committeeIndex,
-    aggregationBits: isForkPostElectra(fork) ? aggregationBits : null,
+    committeeValidatorIndex,
+    committeeSize: committeeValidatorIndices.length,
   };
 }
 

packages/beacon-node/src/network/processor/gossipHandlers.ts

@@ -637,8 +637,14 @@ function getBatchHandlers(modules: ValidatorFnsModules, options: GossipHandlerOp
         results.push(null);
 
         // Handler
-        const {indexedAttestation, attDataRootHex, attestation, committeeIndex, aggregationBits} =
-          validationResult.result;
+        const {
+          indexedAttestation,
+          attDataRootHex,
+          attestation,
+          committeeIndex,
+          committeeValidatorIndex,
+          committeeSize,
+        } = validationResult.result;
         metrics?.registerGossipUnaggregatedAttestation(gossipHandlerParams[i].seenTimestampSec, indexedAttestation);
 
         try {
@@ -650,7 +656,8 @@ function getBatchHandlers(modules: ValidatorFnsModules, options: GossipHandlerOp
               committeeIndex,
               attestation,
               attDataRootHex,
-              aggregationBits
+              committeeValidatorIndex,
+              committeeSize
             );
             metrics?.opPool.attestationPoolInsertOutcome.inc({insertOutcome});
           }

packages/beacon-node/test/unit/chain/opPools/attestationPool.test.ts

@@ -24,6 +24,9 @@ describe("AttestationPool", () => {
   const clockStub = getMockedClock();
   vi.spyOn(clockStub, "secFromSlot").mockReturnValue(0);
 
+  const committeeValidatorIndex = 0;
+  const committeeSize = 128;
+
   const cutOffSecFromSlot = (2 / 3) * config.SECONDS_PER_SLOT;
 
   // Mock attestations
@@ -37,10 +40,10 @@ describe("AttestationPool", () => {
     signature: validSignature,
   };
   const electraAttestation: electra.Attestation = {
-    ...ssz.electra.Attestation.defaultValue(),
-    committeeBits: BitArray.fromSingleBit(MAX_COMMITTEES_PER_SLOT, electraSingleAttestation.committeeIndex),
+    aggregationBits: BitArray.fromSingleBit(committeeSize, committeeValidatorIndex),
     data: electraAttestationData,
     signature: validSignature,
+    committeeBits: BitArray.fromSingleBit(MAX_COMMITTEES_PER_SLOT, electraSingleAttestation.committeeIndex),
   };
   const phase0AttestationData: phase0.AttestationData = {
     ...ssz.phase0.AttestationData.defaultValue(),
@@ -60,9 +63,14 @@ describe("AttestationPool", () => {
 
   it("add correct electra attestation", () => {
     const committeeIndex = 0;
-    const aggregationBits = ssz.electra.Attestation.fields.aggregationBits.defaultValue();
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraSingleAttestation.data));
-    const outcome = pool.add(committeeIndex, electraSingleAttestation, attDataRootHex, aggregationBits);
+    const outcome = pool.add(
+      committeeIndex,
+      electraSingleAttestation,
+      attDataRootHex,
+      committeeValidatorIndex,
+      committeeSize
+    );
 
     expect(outcome).equal(InsertOutcome.NewData);
     expect(pool.getAggregate(electraAttestationData.slot, committeeIndex, attDataRootHex)).toEqual(electraAttestation);
@@ -71,7 +79,7 @@ describe("AttestationPool", () => {
   it("add correct phase0 attestation", () => {
     const committeeIndex = null;
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(phase0Attestation.data));
-    const outcome = pool.add(committeeIndex, phase0Attestation, attDataRootHex, null);
+    const outcome = pool.add(committeeIndex, phase0Attestation, attDataRootHex, committeeValidatorIndex, committeeSize);
 
     expect(outcome).equal(InsertOutcome.NewData);
     expect(pool.getAggregate(phase0AttestationData.slot, committeeIndex, attDataRootHex)).toEqual(phase0Attestation);
@@ -82,18 +90,18 @@ describe("AttestationPool", () => {
 
   it("add electra attestation without committee index", () => {
     const committeeIndex = null;
-    const aggregationBits = ssz.electra.Attestation.fields.aggregationBits.defaultValue();
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraSingleAttestation.data));
 
-    expect(() => pool.add(committeeIndex, electraSingleAttestation, attDataRootHex, aggregationBits)).toThrow();
+    expect(() =>
+      pool.add(committeeIndex, electraSingleAttestation, attDataRootHex, committeeValidatorIndex, committeeSize)
+    ).toThrow();
     expect(pool.getAggregate(electraAttestationData.slot, committeeIndex, attDataRootHex)).toBeNull();
   });
 
   it("add phase0 attestation with committee index", () => {
     const committeeIndex = 0;
-    const aggregationBits = ssz.electra.Attestation.fields.aggregationBits.defaultValue();
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(phase0Attestation.data));
-    const outcome = pool.add(committeeIndex, phase0Attestation, attDataRootHex, aggregationBits);
+    const outcome = pool.add(committeeIndex, phase0Attestation, attDataRootHex, committeeValidatorIndex, committeeSize);
 
     expect(outcome).equal(InsertOutcome.NewData);
     expect(pool.getAggregate(phase0AttestationData.slot, committeeIndex, attDataRootHex)).toEqual(phase0Attestation);
@@ -104,15 +112,14 @@ describe("AttestationPool", () => {
 
   it("add electra attestation with phase0 slot", () => {
     const electraAttestationDataWithPhase0Slot = {...ssz.phase0.AttestationData.defaultValue(), slot: GENESIS_SLOT};
-    const aggregationBits = ssz.electra.Attestation.fields.aggregationBits.defaultValue();
     const singleAttestation = {
       ...ssz.electra.SingleAttestation.defaultValue(),
       data: electraAttestationDataWithPhase0Slot,
       signature: validSignature,
     };
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraAttestationDataWithPhase0Slot));
 
-    expect(() => pool.add(0, singleAttestation, attDataRootHex, aggregationBits)).toThrow();
+    expect(() => pool.add(0, singleAttestation, attDataRootHex, committeeValidatorIndex, committeeSize)).toThrow();
   });
 
   it("add phase0 attestation with electra slot", () => {
@@ -127,6 +134,6 @@ describe("AttestationPool", () => {
     };
     const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(phase0AttestationDataWithElectraSlot));
 
-    expect(() => pool.add(0, attestation, attDataRootHex, null)).toThrow();
+    expect(() => pool.add(0, attestation, attDataRootHex, committeeValidatorIndex, committeeSize)).toThrow();
   });
 });