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

Add support for Electra #613

Merged
merged 56 commits into from
Feb 19, 2025
Merged

Add support for Electra #613

merged 56 commits into from
Feb 19, 2025

Conversation

jtraglia
Copy link
Collaborator

@jtraglia jtraglia commented Apr 29, 2024

πŸ“ Summary

This PR makes the necessary changes for Electra.

β›± Motivation and Context

πŸ“š References

Other Electra branches of dependencies:


βœ… I have run these commands

  • make lint
  • make test-race
  • go mod tidy
  • I have seen and agree to CONTRIBUTING.md

@jtraglia jtraglia marked this pull request as ready for review May 9, 2024 14:32
@@ -425,6 +453,8 @@ which is sufficient data to set the bid of the builder. The `Transactions`
and `Withdrawals` fields are required to construct the full SignedBeaconBlock
and are parsed asynchronously.

TODO(JWT): Does this need to be updated? It hasn't been updated for Deneb.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be separate upgrade added deneb support here #618

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good idea. Thank you for making that PR. I will remove this comment.

@jtraglia
Copy link
Collaborator Author

I'm working on testing this with kurtosis this week. There are some minor issues. Most importantly, I forgot that we needed to add Electra support to flashbots/builder so simulation works. Will mark as a draft for now.

@jtraglia jtraglia marked this pull request as draft May 15, 2024 18:54
@@ -878,6 +879,9 @@ func TestCheckSubmissionPayloadAttrs(t *testing.T) {
for _, tc := range cases {
t.Run(tc.description, func(t *testing.T) {
_, _, backend := startTestBackend(t)
backend.relay.capellaEpoch = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe setting this should be part of startTestBackend

// deneb specific logging
if payload.Deneb != nil {
if payload.Version >= spec.DataVersionDeneb {
blobs, err := payload.Blobs()
Copy link
Collaborator

@avalonche avalonche May 16, 2024

Choose a reason for hiding this comment

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

the purpose of the GetBlockSubmissionInfo struct is so we don't need to extract each field individually and handle the error once. you should be able to access these fields directly e.g. submission.Blobs

return ErrHeaderHTRMismatch
}

if len(bb.Electra.Message.Body.BlobKZGCommitments) != len(payload.Electra.BlobsBundle.Commitments) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

potential to extract to a separate function with BlobKZGCommitments as input

withdrawalsRoot phase0.Root
parentBeaconRoot *phase0.Root
payloadAttributes beaconclient.PayloadAttributes
depositReceiptsRoot phase0.Root
Copy link
Collaborator

Choose a reason for hiding this comment

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

has https://github.com/attestantio/go-eth2-client/blob/master/api/v1/payloadattributesevent.go been updated for latest payload attributes?
If so it can replace the types https://github.com/flashbots/mev-boost-relay/blob/main/beaconclient/prod_beacon_instance.go#L63-L83 which need updating to accept the new payload attributes. You'll also need to update processPayloadAttributes in service.go with the new payload attributes

@avalonche
Copy link
Collaborator

πŸ‘ feel free to fork the builder to build images for kurtosis. Let me know if you need me to rebase the builder from the latest geth electra branch

@jtraglia
Copy link
Collaborator Author

Let me know if you need me to rebase the builder from the latest geth electra branch

Yes please. That would be very useful! This is latest geth electra branch:

Copy link
Contributor

@ryanschneider ryanschneider left a comment

Choose a reason for hiding this comment

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

overall lgtm, @avalonche are any of your comments blockers?

Comment on lines +458 to +459
TODO(JWT): Does this need to be updated? It hasn't been updated for Deneb.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TODO(JWT): Does this need to be updated? It hasn't been updated for Deneb.

@ryanschneider ryanschneider marked this pull request as ready for review February 6, 2025 20:16
@ryanschneider
Copy link
Contributor

@metachris please take a look. For context this version has been used successfully in both builder-playground and kurtosis alongside the latest rbuilder devnet-6 versions.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 15.92179% with 301 lines in your changes missing coverage. Please review.

Project coverage is 32.64%. Comparing base (1d4b461) to head (a32d82a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
common/types_spec.go 3.10% 121 Missing and 4 partials ⚠️
services/api/service.go 20.28% 49 Missing and 6 partials ⚠️
common/types.go 0.00% 25 Missing ⚠️
common/utils.go 26.47% 19 Missing and 6 partials ⚠️
services/api/utils.go 0.00% 25 Missing ⚠️
datastore/redis.go 58.00% 21 Missing ⚠️
database/typesconv.go 5.00% 18 Missing and 1 partial ⚠️
services/api/blocksim_ratelimiter.go 0.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   33.68%   32.64%   -1.05%     
==========================================
  Files          42       42              
  Lines        6682     6990     +308     
==========================================
+ Hits         2251     2282      +31     
- Misses       4156     4418     +262     
- Partials      275      290      +15     
Flag Coverage Ξ”
unittests 32.64% <15.92%> (-1.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@@ -47,10 +47,10 @@ jobs:
uses: actions/checkout@v3

- name: Install gofumpt
run: go install mvdan.cc/gofumpt@v0.4.0
run: go install mvdan.cc/gofumpt@latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

static pinning is better because otherwise at some point CI might just break.

Copy link
Collaborator

@metachris metachris left a comment

Choose a reason for hiding this comment

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

lgtm

@jtraglia jtraglia merged commit 13160be into main Feb 19, 2025
3 of 4 checks passed
@jtraglia jtraglia deleted the electra branch February 19, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants