Skip to content

Commit

Permalink
fix: remove aggregation bits from seen attestation cache (#7265)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@nflaig
nflaig authored Nov 29, 2024
1 parent 17b15cd commit 957684f
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 51 deletions.
5 changes: 3 additions & 2 deletions packages/beacon-node/src/api/impl/beacon/pool/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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});
}
Expand Down
21 changes: 12 additions & 9 deletions packages/beacon-node/src/chain/opPools/attestationPool.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
}
Expand All @@ -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),
};
Expand Down
3 changes: 0 additions & 3 deletions packages/beacon-node/src/chain/seenCache/seenAttestationData.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
36 changes: 14 additions & 22 deletions packages/beacon-node/src/chain/validation/attestation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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).
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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,
});
}
}

Expand Down Expand Up @@ -517,7 +509,6 @@ async function validateAttestationNoSignatureCheck(
// root of AttestationData was already cached during getIndexedAttestationSignatureSet
attDataRootHex,
attestationData: attData,
aggregationBits: isForkPostElectra(fork) ? aggregationBits : null,
});
}
}
Expand Down Expand Up @@ -553,7 +544,8 @@ async function validateAttestationNoSignatureCheck(
signatureSet,
validatorIndex,
committeeIndex,
aggregationBits: isForkPostElectra(fork) ? aggregationBits : null,
committeeValidatorIndex,
committeeSize: committeeValidatorIndices.length,
};
}

Expand Down
13 changes: 10 additions & 3 deletions packages/beacon-node/src/network/processor/gossipHandlers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -650,7 +656,8 @@ function getBatchHandlers(modules: ValidatorFnsModules, options: GossipHandlerOp
committeeIndex,
attestation,
attDataRootHex,
aggregationBits
committeeValidatorIndex,
committeeSize
);
metrics?.opPool.attestationPoolInsertOutcome.inc({insertOutcome});
}
Expand Down
31 changes: 19 additions & 12 deletions packages/beacon-node/test/unit/chain/opPools/attestationPool.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(),
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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", () => {
Expand All @@ -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();
});
});

0 comments on commit 957684f

Please sign in to comment.