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.

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

Jump to bottom

feat: pre-electra support from attestation pool #6998

Merged
merged 14 commits into from
Aug 16, 2024
Merged

feat: pre-electra support from attestation pool #6998

merged 14 commits into from
Aug 16, 2024

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Aug 2, 2024 •
edited by wemeetagain
Loading

  • Attestation pool
    • Add type check to ensure adding and getting correct attestation type according to the fork
    • aggregateByIndexByRootBySlot is modified to allow null committee index to only handle pre-electra attestations. null committee index is not allowed post-electra attestations
  • Aggregated attestation pool
    • Add type check to ensure adding and getting correct attestation type according to the fork
  • Gossip
    • beacon_attestation in gossip queue is indexed by attestation data instead of attestation data + committee bits.
  • API
    • Adjust implementations of several APIs to handle correct attestation type: getPoolAttestations, getPoolAttestationsV2, getAggregatedAttestation

nflaig
nflaig reviewed Aug 5, 2024
View reviewed changes
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

General approach looks good to me, haven't reviewed in detail

packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved
Base automatically changed from nc/attestation-api-v2 to electra-fork-rebasejul30 August 5, 2024 19:53
@wemeetagain
Copy link
Member

lgtm

@ensi321 ensi321 marked this pull request as ready for review August 6, 2024 13:46
@ensi321 ensi321 requested a review from a team as a code owner August 6, 2024 13:46
nflaig
nflaig reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Few more comments

packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/api/impl/validator/index.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/chain/chain.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Outdated Show resolved Hide resolved
@g11tech g11tech force-pushed the electra-fork-rebasejul30 branch 3 times, most recently from f83cbf3 to 72b8074 Compare August 9, 2024 16:00
nflaig
nflaig reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

LGTM - just a few remarks

packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Outdated Show resolved Hide resolved
packages/beacon-node/src/chain/opPools/attestationPool.ts Show resolved Hide resolved
packages/beacon-node/src/util/sszBytes.ts Outdated Show resolved Hide resolved
packages/beacon-node/test/perf/chain/opPools/aggregatedAttestationPool.test.ts Outdated Show resolved Hide resolved
@nflaig nflaig mentioned this pull request Aug 12, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Aug 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 14.28571% with 24 lines in your changes missing coverage. Please review.

Please upload report for BASE (electra-fork-rebasejul30@72b8074). Learn more about missing BASE report.

Additional details and impacted files 
@@                     Coverage Diff                     @@
##             electra-fork-rebasejul30    #6998   +/-   ##
===========================================================
  Coverage                            ?   49.35%           
===========================================================
  Files                               ?      589           
  Lines                               ?    39186           
  Branches                            ?     2238           
===========================================================
  Hits                                ?    19342           
  Misses                              ?    19803           
  Partials                            ?       41

@ensi321 ensi321 mentioned this pull request Aug 13, 2024
Open
12 tasks
@nflaig nflaig mentioned this pull request Aug 14, 2024
Merged
@wemeetagain
Copy link
Member

looks like last round of review to be addressed, then we can merge this

nflaig
nflaig approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

LGTM

@ensi321 ensi321 merged commit 55062cd into electra-fork-rebasejul30 Aug 16, 2024
14 of 17 checks passed
@ensi321 ensi321 deleted the nc/attestation-pool branch August 16, 2024 14:40
g11tech pushed a commit that referenced this pull request Aug 23, 2024
@ensi321 @nflaig
@twoeths @g11tech
feat: pre-electra support from attestation pool (#6998)
6c17a67 
* Initial commit

* Update packages/beacon-node/src/chain/chain.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/api/impl/validator/index.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Add unit test for attestation pool

* fix: getSeenAttDataKey apis (#7009)

* fix: getSeenAttDataKey apis

* chore: use ForkName instead of ForkSeq

* Update packages/beacon-node/src/util/sszBytes.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Update error message

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Move determining post-electra fork out of loops

---------

Co-authored-by: Nico Flaig <[email protected]>
Co-authored-by: twoeths <[email protected]>
g11tech pushed a commit that referenced this pull request Aug 27, 2024
@ensi321 @nflaig
@twoeths @g11tech
feat: pre-electra support from attestation pool (#6998)
7d900c5 
* Initial commit

* Update packages/beacon-node/src/chain/chain.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/api/impl/validator/index.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Add unit test for attestation pool

* fix: getSeenAttDataKey apis (#7009)

* fix: getSeenAttDataKey apis

* chore: use ForkName instead of ForkSeq

* Update packages/beacon-node/src/util/sszBytes.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Update error message

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Move determining post-electra fork out of loops

---------

Co-authored-by: Nico Flaig <[email protected]>
Co-authored-by: twoeths <[email protected]>
philknows pushed a commit that referenced this pull request Sep 3, 2024
@ensi321 @nflaig
@twoeths @philknows
feat: pre-electra support from attestation pool (#6998)
b572236 
* Initial commit

* Update packages/beacon-node/src/chain/chain.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/api/impl/validator/index.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Add unit test for attestation pool

* fix: getSeenAttDataKey apis (#7009)

* fix: getSeenAttDataKey apis

* chore: use ForkName instead of ForkSeq

* Update packages/beacon-node/src/util/sszBytes.ts

Co-authored-by: Nico Flaig <[email protected]>

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Update error message

* Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

Co-authored-by: Nico Flaig <[email protected]>

* address comment

* Move determining post-electra fork out of loops

---------

Co-authored-by: Nico Flaig <[email protected]>
Co-authored-by: twoeths <[email protected]>
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.22.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twoeths twoeths twoeths left review comments

@nflaig nflaig nflaig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ensi321 @wemeetagain @twoeths @nflaig