Skip to content

fix: remove getAttestationsForBlockPreElectra #8048

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Draft

fix: remove getAttestationsForBlockPreElectra #8048

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
130 changes: 4 additions & 126 deletions packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import {
ForkName,
ForkSeq,
MAX_ATTESTATIONS,
MAX_ATTESTATIONS_ELECTRA,
MAX_COMMITTEES_PER_SLOT,
MIN_ATTESTATION_INCLUSION_DELAY,
Expand Down Expand Up @@ -45,8 +44,6 @@

type CommitteeIndex = number;

// for pre-electra
type AttestationWithScore = {attestation: Attestation; score: number};
/**
* for electra, this is to consolidate aggregated attestations of the same attestation data into a single attestation to be included in block
* note that this is local definition in this file and it's NOT validator consolidation
Expand Down Expand Up @@ -101,15 +98,6 @@
*/
const MAX_RETAINED_ATTESTATIONS_PER_GROUP_ELECTRA = 8;

/**
* Pre-electra, each slot has 64 committees, and each block has 128 attestations max so in average
* we get 2 attestation per groups.
* Starting from Jan 2024, we have a performance issue getting attestations for a block. Based on the
* fact that lot of groups will have only 1 full participation attestation, increase this number
* a bit higher than average. This also help decrease number of slots to search for attestations.
*/
const MAX_ATTESTATIONS_PER_GROUP = 3;

/**
* For electra, there is on chain aggregation of attestations across committees, so we can just pick up to 8
* attestations per group, sort by scores to get first 8.
Expand Down Expand Up @@ -217,122 +205,12 @@
}

getAttestationsForBlock(fork: ForkName, forkChoice: IForkChoice, state: CachedBeaconStateAllForks): Attestation[] {
// since Jul 2025, only support electra
const forkSeq = ForkSeq[fork];
return forkSeq >= ForkSeq.electra
? this.getAttestationsForBlockElectra(fork, forkChoice, state)
: this.getAttestationsForBlockPreElectra(fork, forkChoice, state);
}

/**
* Get attestations to be included in a block pre-electra. Returns up to $MAX_ATTESTATIONS items
*/
getAttestationsForBlockPreElectra(
fork: ForkName,
forkChoice: IForkChoice,
state: CachedBeaconStateAllForks
): phase0.Attestation[] {
const stateSlot = state.slot;
const stateEpoch = state.epochCtx.epoch;
const statePrevEpoch = stateEpoch - 1;

const notSeenValidatorsFn = getNotSeenValidatorsFn(state);
const validateAttestationDataFn = getValidateAttestationDataFn(forkChoice, state);

const attestationsByScore: AttestationWithScore[] = [];

const slots = Array.from(this.attestationGroupByIndexByDataHexBySlot.keys()).sort((a, b) => b - a);
let minScore = Number.MAX_SAFE_INTEGER;
let slotCount = 0;
slot: for (const slot of slots) {
slotCount++;
const attestationGroupByIndexByDataHash = this.attestationGroupByIndexByDataHexBySlot.get(slot);
// should not happen
if (!attestationGroupByIndexByDataHash) {
throw Error(`No aggregated attestation pool for slot=${slot}`);
}

const epoch = computeEpochAtSlot(slot);
// validateAttestation condition: Attestation target epoch not in previous or current epoch
if (!(epoch === stateEpoch || epoch === statePrevEpoch)) {
continue; // Invalid attestations
}
// validateAttestation condition: Attestation slot not within inclusion window
if (
!(
slot + MIN_ATTESTATION_INCLUSION_DELAY <= stateSlot &&
// Post deneb, attestations are valid for current and previous epoch
(ForkSeq[fork] >= ForkSeq.deneb || stateSlot <= slot + SLOTS_PER_EPOCH)
)
) {
continue; // Invalid attestations
}

const inclusionDistance = stateSlot - slot;
for (const attestationGroupByIndex of attestationGroupByIndexByDataHash.values()) {
for (const [committeeIndex, attestationGroup] of attestationGroupByIndex.entries()) {
const notSeenCommitteeMembers = notSeenValidatorsFn(epoch, slot, committeeIndex);
if (notSeenCommitteeMembers === null || notSeenCommitteeMembers.size === 0) {
continue;
}

if (
slotCount > 2 &&
attestationsByScore.length >= MAX_ATTESTATIONS &&
notSeenCommitteeMembers.size / inclusionDistance < minScore
) {
// after 2 slots, there are a good chance that we have 2 * MAX_ATTESTATIONS attestations and break the for loop early
// if not, we may have to scan all slots in the pool
// if we have enough attestations and the max possible score is lower than scores of `attestationsByScore`, we should skip
// otherwise it takes time to check attestation, add it and remove it later after the sort by score
continue;
}

if (validateAttestationDataFn(attestationGroup.data) !== null) {
continue;
}

// TODO: Is it necessary to validateAttestation for:
// - Attestation committee index not within current committee count
// - Attestation aggregation bits length does not match committee length
//
// These properties should not change after being validate in gossip
// IF they have to be validated, do it only with one attestation per group since same data
// The committeeCountPerSlot can be precomputed once per slot
const getAttestationsResult = attestationGroup.getAttestationsForBlock(
fork,
state.epochCtx.effectiveBalanceIncrements,
notSeenCommitteeMembers,
MAX_ATTESTATIONS_PER_GROUP
);
for (const {attestation, newSeenEffectiveBalance} of getAttestationsResult.result) {
const score = newSeenEffectiveBalance / inclusionDistance;
if (score < minScore) {
minScore = score;
}
attestationsByScore.push({
attestation,
score,
});
}

// Stop accumulating attestations there are enough that may have good scoring
if (attestationsByScore.length >= MAX_ATTESTATIONS * 2) {
break slot;
}
}
}
}

const sortedAttestationsByScore = attestationsByScore.sort((a, b) => b.score - a.score);
const attestationsForBlock: phase0.Attestation[] = [];
for (const [i, attestationWithScore] of sortedAttestationsByScore.entries()) {
if (i >= MAX_ATTESTATIONS) {
break;
}
// attestations could be modified in this op pool, so we need to clone for block
attestationsForBlock.push(ssz.phase0.Attestation.clone(attestationWithScore.attestation));
if (forkSeq < ForkSeq.electra) {
throw Error(`Not support producing block for fork ${fork} slot ${state.slot}`);

Check failure on line 211 in packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (22)

packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts > AggregatedAttestationPool - Altair > incompatible shuffling - incorrect pivot block root 

Error: Not support producing block for fork altair slot 16241
 ❯ AggregatedAttestationPool.getAttestationsForBlock packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts:211:13
 ❯ packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts:158:17

Check failure on line 211 in packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (22)

packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts > AggregatedAttestationPool - Altair > incorrect source 

Error: Not support producing block for fork altair slot 16241
 ❯ AggregatedAttestationPool.getAttestationsForBlock packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts:211:13
 ❯ packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts:147:17

Check failure on line 211 in packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (22)

packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts > AggregatedAttestationPool - Altair > one is seen and one is NOT 

Error: Not support producing block for fork altair slot 16241
 ❯ AggregatedAttestationPool.getAttestationsForBlock packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts:211:13
 ❯ packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts:133:21

Check failure on line 211 in packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (22)

packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts > AggregatedAttestationPool - Altair > all validators are NOT seen 

Error: Not support producing block for fork altair slot 16241
 ❯ AggregatedAttestationPool.getAttestationsForBlock packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts:211:13
 ❯ packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts:133:21

Check failure on line 211 in packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (22)

packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts > AggregatedAttestationPool - Altair > all validators are seen 

Error: Not support producing block for fork altair slot 16241
 ❯ AggregatedAttestationPool.getAttestationsForBlock packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts:211:13
 ❯ packages/beacon-node/test/unit-minimal/chain/opPools/aggregatedAttestationPool.test.ts:135:21
}
return attestationsForBlock;
return this.getAttestationsForBlockElectra(fork, forkChoice, state);
Comment on lines +208 to +213
Copy link

@graphite-app graphite-app bot Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a breaking behavior by throwing an error for pre-electra forks instead of handling them. Since this modifies the public API contract, it should be documented in the changelog to properly notify users of this backward-incompatible change. Consider adding a changelog entry that clearly communicates this interface change and provides migration guidance for users who might still be relying on pre-electra block production.

Spotted by Diamond (based on custom rules)

Is this helpful? React 👍 or 👎 to let us know.

}

/**
Expand Down
Loading