Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reduce getSingleTrueBit() calls #7209

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/beacon-node/src/api/impl/beacon/pool/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,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} = await validateGossipFnRetryUnknownRoot(
validateFn,
network,
chain,
slot,
beaconBlockRoot
);
const {indexedAttestation, subnet, attDataRootHex, committeeIndex, participationIndex} =
await validateGossipFnRetryUnknownRoot(validateFn, network, chain, slot, beaconBlockRoot);

if (network.shouldAggregate(subnet, slot)) {
const insertOutcome = chain.attestationPool.add(committeeIndex, attestation, attDataRootHex);
const insertOutcome = chain.attestationPool.add(
committeeIndex,
participationIndex,
attestation,
attDataRootHex
);
metrics?.opPool.attestationPoolInsertOutcome.inc({insertOutcome});
}

Expand Down
13 changes: 8 additions & 5 deletions packages/beacon-node/src/chain/opPools/attestationPool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,12 @@ export class AttestationPool {
* - Valid committeeIndex
* - Valid data
*/
add(committeeIndex: CommitteeIndex, attestation: Attestation, attDataRootHex: RootHex): InsertOutcome {
add(
committeeIndex: CommitteeIndex,
participationIndex: number,
attestation: Attestation,
attDataRootHex: RootHex
): InsertOutcome {
const slot = attestation.data.slot;
const fork = this.config.getForkName(slot);
const lowestPermissibleSlot = this.lowestPermissibleSlot;
Expand Down Expand Up @@ -144,7 +149,7 @@ export class AttestationPool {
const aggregate = aggregateByIndex.get(committeeIndex);
if (aggregate) {
// Aggregate mutating
return aggregateAttestationInto(aggregate, attestation);
return aggregateAttestationInto(aggregate, attestation, participationIndex);
}
// Create new aggregate
aggregateByIndex.set(committeeIndex, attestationToAggregate(attestation));
Expand Down Expand Up @@ -216,9 +221,7 @@ export class AttestationPool {
/**
* Aggregate a new attestation into `aggregate` mutating it
*/
function aggregateAttestationInto(aggregate: AggregateFast, attestation: Attestation): InsertOutcome {
const bitIndex = attestation.aggregationBits.getSingleTrueBit();

function aggregateAttestationInto(aggregate: AggregateFast, attestation: Attestation, bitIndex: number): InsertOutcome {
// Should never happen, attestations are verified against this exact condition before
assert.notNull(bitIndex, "Invalid attestation in pool, not exactly one bit set");

Expand Down
2 changes: 2 additions & 0 deletions packages/beacon-node/src/chain/validation/attestation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export type AttestationValidationResult = {
subnet: number;
attDataRootHex: RootHex;
committeeIndex: CommitteeIndex;
participationIndex: number;
};

export type AttestationOrBytes = ApiAttestation | GossipAttestation;
Expand Down Expand Up @@ -505,6 +506,7 @@ async function validateAttestationNoSignatureCheck(
signatureSet,
validatorIndex,
committeeIndex,
participationIndex: bitIndex,
};
}

Expand Down
10 changes: 8 additions & 2 deletions packages/beacon-node/src/network/processor/gossipHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -633,14 +633,20 @@ function getBatchHandlers(modules: ValidatorFnsModules, options: GossipHandlerOp
results.push(null);

// Handler
const {indexedAttestation, attDataRootHex, attestation, committeeIndex} = validationResult.result;
const {indexedAttestation, attDataRootHex, attestation, committeeIndex, participationIndex} =
validationResult.result;
metrics?.registerGossipUnaggregatedAttestation(gossipHandlerParams[i].seenTimestampSec, indexedAttestation);

try {
// Node may be subscribe to extra subnets (long-lived random subnets). For those, validate the messages
// but don't add to attestation pool, to save CPU and RAM
if (aggregatorTracker.shouldAggregate(subnet, indexedAttestation.data.slot)) {
const insertOutcome = chain.attestationPool.add(committeeIndex, attestation, attDataRootHex);
const insertOutcome = chain.attestationPool.add(
committeeIndex,
participationIndex,
attestation,
attDataRootHex
);
metrics?.opPool.attestationPoolInsertOutcome.inc({insertOutcome});
}
} catch (e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe("AttestationPool", () => {
};

let pool: AttestationPool;
const participationIndex = 0;

beforeEach(() => {
pool = new AttestationPool(config, clockStub, cutOffSecFromSlot);
Expand All @@ -52,7 +53,7 @@ describe("AttestationPool", () => {
it("add correct electra attestation", () => {
const committeeIndex = 0;
const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraAttestation.data));
const outcome = pool.add(committeeIndex, electraAttestation, attDataRootHex);
const outcome = pool.add(committeeIndex, participationIndex, electraAttestation, attDataRootHex);

expect(outcome).equal(InsertOutcome.NewData);
expect(pool.getAggregate(electraAttestationData.slot, committeeIndex, attDataRootHex)).toEqual(electraAttestation);
Expand All @@ -61,7 +62,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);
const outcome = pool.add(committeeIndex, participationIndex, phase0Attestation, attDataRootHex);

expect(outcome).equal(InsertOutcome.NewData);
expect(pool.getAggregate(phase0AttestationData.slot, committeeIndex, attDataRootHex)).toEqual(phase0Attestation);
Expand All @@ -74,14 +75,14 @@ describe("AttestationPool", () => {
const committeeIndex = null;
const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraAttestation.data));

expect(() => pool.add(committeeIndex, electraAttestation, attDataRootHex)).toThrow();
expect(() => pool.add(committeeIndex, participationIndex, electraAttestation, attDataRootHex)).toThrow();
expect(pool.getAggregate(electraAttestationData.slot, committeeIndex, attDataRootHex)).toBeNull();
});

it("add phase0 attestation with committee index", () => {
const committeeIndex = 0;
const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(phase0Attestation.data));
const outcome = pool.add(committeeIndex, phase0Attestation, attDataRootHex);
const outcome = pool.add(committeeIndex, participationIndex, phase0Attestation, attDataRootHex);

expect(outcome).equal(InsertOutcome.NewData);
expect(pool.getAggregate(phase0AttestationData.slot, committeeIndex, attDataRootHex)).toEqual(phase0Attestation);
Expand All @@ -99,7 +100,7 @@ describe("AttestationPool", () => {
};
const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(electraAttestationDataWithPhase0Slot));

expect(() => pool.add(0, attestation, attDataRootHex)).toThrow();
expect(() => pool.add(0, participationIndex, attestation, attDataRootHex)).toThrow();
});

it("add phase0 attestation with electra slot", () => {
Expand All @@ -114,6 +115,6 @@ describe("AttestationPool", () => {
};
const attDataRootHex = toHexString(ssz.phase0.AttestationData.hashTreeRoot(phase0AttestationDataWithElectraSlot));

expect(() => pool.add(0, attestation, attDataRootHex)).toThrow();
expect(() => pool.add(0, participationIndex, attestation, attDataRootHex)).toThrow();
});
});
Loading