Skip to content

Commit

Permalink
Only create aggregationBits once for the first attestation
Browse files Browse the repository at this point in the history
  • Loading branch information
nflaig committed Nov 29, 2024
1 parent 96850fe commit 9d17239
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 31 deletions.
5 changes: 3 additions & 2 deletions packages/beacon-node/src/api/impl/beacon/pool/index.ts
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
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
12 changes: 7 additions & 5 deletions packages/beacon-node/src/chain/validation/attestation.ts
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;
committeeValidatorIndex: number;
committeeSize: number;
};

export type AttestationOrBytes = ApiAttestation | GossipAttestation;
Expand Down Expand Up @@ -398,6 +399,7 @@ async function validateAttestationNoSignatureCheck(
}

let validatorIndex: number;
let committeeValidatorIndex: number;

if (!isForkPostElectra(fork)) {
// The validity of aggregation bits are already checked above
Expand All @@ -406,6 +408,7 @@ async function validateAttestationNoSignatureCheck(
assert.notNull(bitIndex);

validatorIndex = committeeValidatorIndices[bitIndex];
committeeValidatorIndex = bitIndex;
// [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 @@ -419,14 +422,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)`.
// Position of the validator in its committee
const committeeValidatorIndex = committeeValidatorIndices.indexOf(validatorIndex);
committeeValidatorIndex = committeeValidatorIndices.indexOf(validatorIndex);
if (committeeValidatorIndex === -1) {
throw new AttestationError(GossipAction.REJECT, {
code: AttestationErrorCode.ATTESTER_NOT_IN_COMMITTEE,
});
}

aggregationBits = BitArray.fromSingleBit(committeeValidatorIndices.length, committeeValidatorIndex);
}

// LH > verify_middle_checks
Expand Down Expand Up @@ -531,7 +532,8 @@ async function validateAttestationNoSignatureCheck(
signatureSet,
validatorIndex,
committeeIndex,
aggregationBits,
committeeValidatorIndex,
committeeSize: committeeValidatorIndices.length,
};
}

Expand Down
13 changes: 10 additions & 3 deletions packages/beacon-node/src/network/processor/gossipHandlers.ts
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
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 9d17239

Please sign in to comment.