fix: duplicated block attestations #7455
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7455 +/- ##
=========================================
Coverage 50.25% 50.25%
=========================================
Files 602 602
Lines 40401 40416 +15
Branches 2204 2203 -1
=========================================
+ Hits 20305 20313 +8
- Misses 20056 20063 +7
Partials 40 40 |
nflaig
reviewed
Feb 12, 2025
| @@ -385,19 +383,22 @@ export class AggregatedAttestationPool { | |||
| if (score < minScore) { | |||
| minScore = score; | |||
| } | |||
| consolidations.push({...consolidation, score}); | |||
|
|
|||
| consolidations.set(consolidation, score); | |||
There was a problem hiding this comment.
so this would override the record? might be better to just continue if it already exists.
But why do we have duplicates in sameAttDataCons shouldn't this be aggregated earlier into a single one?
There was a problem hiding this comment.
so this would override the record? might be better to just continue if it already exists.
That's the whole point. The current implementation would insert the same consolidation with different score into consolidations, hence the duplication.
But why do we have duplicates in sameAttDataCons shouldn't this be aggregated earlier into a single one?
The aggregation earlier only aggregates the SignedAggregateAndProofs from the same committee. Notice how they are stored by committee index and MatchingDataAttestationGroup only store aggregated attestations from the same committee.
private readonly attestationGroupByIndexByDataHexBySlot = new MapDef<
Slot,
Map<DataRootHex, Map<CommitteeIndex, MatchingDataAttestationGroup>>>
| .slice(0, MAX_ATTESTATIONS_ELECTRA); | ||
|
|
There was a problem hiding this comment.
Noticed we are not cloning here
lodestar/packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Lines 288 to 289 in e45e0eb
but might be fine due to aggregateConsolidation
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
|
It took me a while to understand what Suppose you have committee 100 and committee 101 both contain members who vote for the same attestation data. Then they will share the same In the first iteration of the above for loop, and then each In the second iteration, and again where each element's |
…ol.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com>
|
Can confirm this PR fixes the issue. https://dora.pectra-devnet-6.ethpandaops.io/slot/66426 no longer sees duplicates. |
|
@ensi321 could you reproduce the issue in a unit test? |
nflaig
approved these changes
Feb 13, 2025
Merged
|
🎉 This PR is included in v1.27.0 🎉 |
wemeetagain
pushed a commit
that referenced
this pull request
Feb 18, 2025
Follow up on #7455 (comment) to add unit test of the changes introduced in #7455. --------- Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Raise by Teku team on discord https://discord.com/channels/595666850260713488/1338793076491026486/1338793635386228756,
Lodestar will generate duplicated attestations and include them in the block body when proposing.
For example https://dora.pectra-devnet-6.ethpandaops.io/slot/54933 has 6 copies of the same attestation with signature
0xae6b928e4866d5a43ae6d4ced869e3aa53f38617d42da5862c76e9a928942783a108e4e281d208766b8a9b2adb286aff0e0af7c14f24d1b013f4ccb47c000a11c256112fab37945d2e5bb3671b997a23b1d57d67d3fe69835ecc06f1f57b1210.This is due to
getAttestationsForBlockElectraputting multiple copies of the same attestations inconsolidationswhen there are attestations coming from different committees with the same attestation data.The above attestation having 6 copies because the attestation contains 6 committees (0, 2, 3, 6, 7 and 12).
Proposal
Remove
AttestationsConsolidation.scoreand convertconsolidationsto aMap<AttestationsConsolidation, number>()to track the score while eliminating duplicates.