Pending aggregates: When multiple aggregated attestations only differing by the aggregator index are in the pending queue, only process one of them. by nalepae · Pull Request #16153 · OffchainLabs/prysm

nalepae · 2025-12-17T22:52:48Z

What type of PR is this?
Other

What does this PR do? Why is it needed?
When an (potentially aggregated) attestation is received before the block being voted for, Prysm queues this attestation, then processes the queue when the block has been received.

This behavior is consistent with the Phase0 specification .

[IGNORE] The block being voted for (attestation.data.beacon_block_root) has been seen (via gossip or non-gossip sources) (a client MAY queue attestations for processing once block is retrieved).

Once the block being voted for is processed, previously queued (potentially aggregated) attestations are then processed, and broadcasted.

Processing (potentially aggregated) attestations takes some non negligible time. For this reason, (potentially aggregated) attestations are deduplicated before being introduced into the pending queue, to avoid eventually processing duplicates.

Before this PR, two aggregated attestations were considered duplicated if all of the following conditions were gathered:

Attestations have the same version,
Attestations have the same aggregator index (aka., the same validator aggregated them),
Attestations have the same slot,
Attestations have the same committee index, and
Attestations have the same aggregation bits

Aggregated attestations are then broadcasted.
The final purpose of aggregated attestations is to be packed into the next block by the next proposer.
When packing attestations, the aggregator index is not used any more.

This pull request modifies the deduplication function used in the pending aggregated attestations queue by considering that multiple aggregated attestations only differing by the aggregator index are equivalent (removing 2. of the previous list.)

As a consequence, the count of aggregated attestations to be introduced in the pending queue is reduced from 1 aggregated attestation by aggregator to, in the best case, MAX_COMMITTEE_PER_SLOT=64.

Also, only a single aggregated attestation for a given version, slot, committee index and aggregation bits will be re-broadcasted. This is a correct behavior, since no data to be included in a block will be lost. (We can even say that this will reduce by a bit the total networking volume.)

How to test:

Start a beacon node (preferably, on a slow computer) from a checkpoint.
Filter logs containing Synced new block and Verified and saved pending attestations to pool. (You can pipe logs into grep -E "Synced new block|Verified and saved pending attestations to pool".

In Synced new block logs, monitor the sinceSlotStartTime value. This should monotonically decrease.
In Verified and saved pending attestations to pool, monitor the pendingAggregateAttAndProofCount value. It should be a "honest" value. "honest" is not really quantifiable here, since it depends on the aggregators. But it's likely to be less than 5*MAX_COMMITTEE_PER_SLOT=320.

Which issues(s) does this PR fix?

Partially fixes:

Once out of sync, Prysm struggles to catch up because of enqueued (aggregated) attestations processing. #16160

Other notes for review
Please read commit by commit, with commit messages.
The important commit is b748c04.

Acknowledgements

I have read CONTRIBUTING.md.
I have included a uniquely named changelog fragment file.
I have added a description with sufficient context for reviewers to understand this PR.
I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable).

…ing by the aggregator index are in the pending queue, only process one of them.

potuz

LGTM, would feel better with an extra validation of my comment

potuz · 2025-12-18T12:14:15Z

 		}
 	}

+	initialMissingRootCount := len(missingIndicesByRoot)


this is unrelated with the PR

That's right. That's the reason why it's modified in a totally separated commit.

should we take it out of the PR?
id at least mention it in the github description, as reviewer, it was confusing to read at first

potuz · 2025-12-18T12:20:33Z

-	if a.AggregateAttestationAndProof().GetAggregatorIndex() != b.AggregateAttestationAndProof().GetAggregatorIndex() {
-		return false
-	}


We need to make sure that one of the aggregates is valid if and only iff all other aggregates are valid

Moving to draft until your comment is solved.

With this PR, there is a flaw:
If the first aggregated attestation (for a given slot, committee index and aggregation bits) is received, but with an aggregator index which is not supposed to aggregate for this slot, then this aggregated attestation is inserted into the queue, and all other equivalent aggregated attestations are discarded.

Later, when processing the queue, we finally reject this aggregated attestation because it is not valid.
As a consequence, 0 aggregated attestation of this kind is neither accepted nor broadcasted.
==> This is a bug.

9969761 fixes is issue, by moving all verifications not needing the block (including the aggregator index) before inserting the aggregated attestation into the queue.
Only the verifications needing the block are done when pulling aggregated attestations from the queue.

As a bonus, it considerably reduces the process time when pulling aggregated attestations from the queue.

This reverts commit 9969761.

…y differing by the aggregator index are in the pending queue, only process one of them." This reverts commit b748c04.

…ter.

…ering by aggregator index.

potuz

LGTM

potuz · 2025-12-18T20:22:06Z

+	// Verify attestation target root is consistent with the head root.
+	// This verification is not in the spec, however we guard against it as it opens us up
+	// to weird edge cases during verification. The attestation technically could be used to add value to a block,
+	// but it's invalid in the spirit of the protocol. Here we choose safety over profit.


I don't understand this good. Moving this offline

potuz · 2025-12-19T00:28:02Z

-func pendingAggregatesAreEqual(a, b ethpb.SignedAggregateAttAndProof) bool {
+// pendingAggregatesAreEqual checks if two pending aggregate attestations are equal.
+// If filterOnAggregatorIndex is false, two aggregates only differ by their aggregator index will be considered equal.
+func pendingAggregatesAreEqual(a, b ethpb.SignedAggregateAttAndProof, filterOnAggregatorIndex bool) bool {


for the record I don't mind boolean controllers, but even some linters would recommend here a named wrapper that has the extra validation. I think it's fine as is.

Fixed in 4eb55cd.

potuz · 2025-12-19T00:29:27Z

+
 		locker.Lock()
+		defer locker.Unlock()
+


I wonder if this deadlock ever happened in the wild

terencechain · 2025-12-19T00:26:57Z

 		}
 	}

+	initialMissingRootCount := len(missingIndicesByRoot)


should we take it out of the PR?
id at least mention it in the github description, as reviewer, it was confusing to read at first

terencechain · 2025-12-19T00:36:17Z

+	durationAtts := time.Since(startAtts)
+
+	log.WithFields(logrus.Fields{
+		"blockRoot":                       fmt.Sprintf("%#x", blockRoot),


it looks to me we are just passing blockRoot as an argument just for logging purpose, but you dont really need to pass it, we could get the blockRoot from the attestation it self, it's just attestation.data.block_root

Fixed in df3b0da.

terencechain · 2025-12-19T00:39:45Z

+	if filterOnAggregatorIndex {
+		if a.AggregateAttestationAndProof().GetAggregatorIndex() != b.AggregateAttestationAndProof().GetAggregatorIndex() {
+			return false
+		}


you may want to add a unit test just for this case as i haven't really see them yet

Fixed in 7dfaa90.

nalepae · 2025-12-19T09:51:42Z

#16153 (comment)

Yeah actually it is in a totally separate commit (that's why I asked to read commit by commit).
For the context, when initially working on this PR, I thought the bottleneck was the sidecars fetching process, not the attestation handling. That's why I added this field :)

meny3437-blip

@meny3437-blip

nalepae added 7 commits December 15, 2025 12:46

SidecarParentSeen: Write the parent root in the error message.

68a28bb

Requesting blocks by root": Display one log per peer (with the pid)

8ad1b9d

processPendingBlocksQueue: User the defer pattern.

bd71128

processPendingBlocks: Log duration

86387d7

requestDirectSidecarsFromPeers: Improve logs

04a95d7

Add att duration.

84830bf

pendingAttsLimit: Move to const.

3b8f5f0

nalepae changed the title ~~Not been seen~~ Stop considering the aggregator index to compare 2 aggregated attestations in the pending queue. Dec 17, 2025

nalepae added 2 commits December 18, 2025 10:00

Pending aggregates: When multiple aggregated attestations only differ…

b748c04

…ing by the aggregator index are in the pending queue, only process one of them.

Add changelog

e94e7c1

nalepae force-pushed the not-been-seen branch from 37ecd5a to e94e7c1 Compare December 18, 2025 09:01

nalepae changed the title ~~Stop considering the aggregator index to compare 2 aggregated attestations in the pending queue.~~ Pending aggregates: When multiple aggregated attestations only differing by the aggregator index are in the pending queue, only process one of them. Dec 18, 2025

nalepae marked this pull request as ready for review December 18, 2025 09:59

potuz reviewed Dec 18, 2025

View reviewed changes

nalepae marked this pull request as draft December 18, 2025 13:48

Fix Potuz's comment.

9969761

nalepae force-pushed the not-been-seen branch from ac485e3 to 9969761 Compare December 18, 2025 15:09

nalepae marked this pull request as ready for review December 18, 2025 15:52

nalepae added 4 commits December 18, 2025 23:18

Revert "Fix Potuz's comment."

d3a3279

This reverts commit 9969761.

Revert "Pending aggregates: When multiple aggregated attestations onl…

e2db419

…y differing by the aggregator index are in the pending queue, only process one of them." This reverts commit b748c04.

processPendingBlocks: Add slotIndex field in log.

668918f

processPendingBlocks: Split the for loop in two parts

ed9dd00

nalepae force-pushed the not-been-seen branch from e06a6c1 to ed9dd00 Compare December 18, 2025 22:42

nalepae added 2 commits December 19, 2025 00:33

pendingAggregatesAreEqual: Add the filterOnAggregatorIndex parame…

84ce0f8

…ter.

processAttestations: Avoid processing multiple aggregates only diff…

0ce9c60

…ering by aggregator index.

nalepae force-pushed the not-been-seen branch from 697a234 to 0ce9c60 Compare December 18, 2025 23:52

potuz previously approved these changes Dec 19, 2025

View reviewed changes

terencechain reviewed Dec 19, 2025

View reviewed changes

Fix Potuz's comment.

4eb55cd

nalepae dismissed potuz’s stale review via 4eb55cd December 19, 2025 10:00

nalepae added 2 commits December 19, 2025 11:11

Fix Terence's comment.

df3b0da

Fix Terence's comment.

7dfaa90

potuz approved these changes Dec 19, 2025

View reviewed changes

terencechain approved these changes Dec 19, 2025

View reviewed changes

nalepae added this pull request to the merge queue Dec 19, 2025

Merged via the queue into develop with commit 2ac30f5 Dec 19, 2025
18 checks passed

nalepae deleted the not-been-seen branch December 19, 2025 14:14

nalepae mentioned this pull request Dec 22, 2025

Prysm 7.0.0 not syncing mainnet #16093

Closed

meny3437-blip reviewed Feb 15, 2026

View reviewed changes

Uh oh!

Conversation

nalepae commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potuz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potuz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalepae commented Dec 19, 2025

Uh oh!

Uh oh!

meny3437-blip left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nalepae commented Dec 17, 2025 •

edited

Loading