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

*: electra versioned aggregations #3482

Merged
merged 9 commits into from
Jan 28, 2025

Conversation

KaloyanTanev
Copy link
Collaborator

@KaloyanTanev KaloyanTanev commented Jan 22, 2025

Change the regular aggregations to versioned aggregations. Given that the attestation structure is changing, the one for aggregations does as well.

Some other small things included in this PR:

  • minor fixes from the previous attestations PR
  • bumping go-eth2-client to its newest electra version
  • fix teku integration tests, using its mekong branch
  • add electra version to more switch cases (not exhaustive, will open separate PR for that)

category: feature
ticket: none

@KaloyanTanev KaloyanTanev marked this pull request as draft January 22, 2025 16:27
@github-actions github-actions bot added the electra PR targeting electra upgrade label Jan 22, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 27.85146% with 544 lines in your changes missing coverage. Please review.

Please upload report for BASE (electra@eab13c1). Learn more about missing BASE report.

Files with missing lines Patch % Lines
core/signeddata.go 16.98% 210 Missing and 10 partials ⚠️
core/unsigneddata.go 28.35% 89 Missing and 7 partials ⚠️
core/ssz.go 49.24% 59 Missing and 8 partials ⚠️
core/validatorapi/router.go 29.62% 57 Missing ⚠️
eth2util/signing/signing.go 0.00% 42 Missing ⚠️
core/validatorapi/validatorapi.go 21.15% 39 Missing and 2 partials ⚠️
app/eth2wrap/eth2wrap_gen.go 0.00% 8 Missing ⚠️
core/fetcher/fetcher.go 45.45% 6 Missing ⚠️
core/dutydb/memory.go 75.00% 2 Missing and 1 partial ⚠️
core/eth2signeddata.go 57.14% 2 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             electra    #3482   +/-   ##
==========================================
  Coverage           ?   55.70%           
==========================================
  Files              ?      218           
  Lines              ?    33725           
  Branches           ?        0           
==========================================
  Hits               ?    18786           
  Misses             ?    12966           
  Partials           ?     1973           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KaloyanTanev KaloyanTanev self-assigned this Jan 28, 2025
@KaloyanTanev KaloyanTanev marked this pull request as ready for review January 28, 2025 11:09
Copy link
Collaborator

@pinebit pinebit left a comment

Choose a reason for hiding this comment

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

Great job

err := unmarshal(typ, body, denebAtts)
if err == nil {
for _, att := range *denebAtts {
// TODO: Data version is not Deneb, it might be anything between Phase0 and Deneb
Copy link
Collaborator

Choose a reason for hiding this comment

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

Flagging this TODO, maybe you forgot to resolve it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, I haven't found a nice way yet... It's in my list for things to fix.

With other versioned structures (like blocks), we have a clunky but functional way of resolving the version - we simply try to parse all versions and whichever succeeds, we deem it is that one. However, for attestations, anything between phase0 and Deneb has the exact same structure, so this method won't really work.

@KaloyanTanev KaloyanTanev added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jan 28, 2025
@KaloyanTanev KaloyanTanev merged commit af7658f into electra Jan 28, 2025
11 of 14 checks passed
@KaloyanTanev KaloyanTanev deleted the kalo/electra-versioned-aggregations branch January 28, 2025 16:36
KaloyanTanev added a commit that referenced this pull request Jan 31, 2025
* Add electra versioned aggregations

* Bump go-eth2-client version and fix TODOs

* Write own switch cases, instead of waiting on go-eth2-client util funcs

* Fix linter

* WIP

* Aggregations fixes and tests updates

* Fix submitAttestations parsing

* Bump go-eth2-client

* Fix teku integration test
KaloyanTanev added a commit that referenced this pull request Feb 5, 2025
* Add electra versioned aggregations

* Bump go-eth2-client version and fix TODOs

* Write own switch cases, instead of waiting on go-eth2-client util funcs

* Fix linter

* WIP

* Aggregations fixes and tests updates

* Fix submitAttestations parsing

* Bump go-eth2-client

* Fix teku integration test
KaloyanTanev added a commit that referenced this pull request Feb 7, 2025
* Add electra versioned aggregations

* Bump go-eth2-client version and fix TODOs

* Write own switch cases, instead of waiting on go-eth2-client util funcs

* Fix linter

* WIP

* Aggregations fixes and tests updates

* Fix submitAttestations parsing

* Bump go-eth2-client

* Fix teku integration test
KaloyanTanev added a commit that referenced this pull request Feb 10, 2025
* Add electra versioned aggregations

* Bump go-eth2-client version and fix TODOs

* Write own switch cases, instead of waiting on go-eth2-client util funcs

* Fix linter

* WIP

* Aggregations fixes and tests updates

* Fix submitAttestations parsing

* Bump go-eth2-client

* Fix teku integration test
KaloyanTanev added a commit that referenced this pull request Feb 24, 2025
* Add electra versioned aggregations

* Bump go-eth2-client version and fix TODOs

* Write own switch cases, instead of waiting on go-eth2-client util funcs

* Fix linter

* WIP

* Aggregations fixes and tests updates

* Fix submitAttestations parsing

* Bump go-eth2-client

* Fix teku integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electra PR targeting electra upgrade merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants