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

EIP-7742 Engine API Changes #7840

Closed
wants to merge 9 commits into from

Conversation

siladu
Copy link
Contributor

@siladu siladu commented Oct 31, 2024

PR description

This makes Pectra require EIP-7742 fields so will break any pectra devnets that exclude 7742. @daniellehrner will that be an issue? I could temporarily disable the engine api validation to avoid this if so.

This makes Besu usable for interop (has been tested with lodestar so far), however there are still some missing pieces that may cause 7742 to break depending on how maxBlobsPerBlock is set:

  • The max isn't wired through to block creation so that still assumes Cancun's 6 blobs max.
  • The 4844 block/tx max blob validation (via newPayload) hasn't been skipped for Prague so a newPayload with > 6 blobs may fail

In other words, in order for Besu to work for a Pectra interop we would require a EIP-7742 compatible CL and a max blob value of <= 6.

This PR was big enough already and I think uncontentious, whereas the implementations for the points above may require more discussion.

Also block creation wiring was required in this PR in order to satisfy CodeDelegationTransactionAcceptanceTest

Part of #7605

Spec: ethereum/execution-apis#574

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

.map(Withdrawal::hashCode)
.reduce(1, (a, b) -> a ^ (b * 31)))
.orElse(0)
<< 32
Copy link
Contributor Author

@siladu siladu Nov 1, 2024

Choose a reason for hiding this comment

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

Note, added a (<< 32) left shift for withdrawals to keep the hashcodes evenly distributed by using the same pattern as the other fields.

This change impacts the shanghai and cancun ATs

Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth extracting this to a method? it's a bit hard to read indented over so many lines

Copy link

github-actions bot commented Dec 2, 2024

This pr is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the Stale label Dec 2, 2024
@siladu siladu removed the Stale label Dec 4, 2024
@siladu siladu force-pushed the eip-7742_engine-api branch 2 times, most recently from 7683fc3 to 852fbf9 Compare December 5, 2024 05:40
@siladu siladu marked this pull request as ready for review December 6, 2024 06:24
@siladu siladu requested a review from daniellehrner December 6, 2024 06:29
@daniellehrner
Copy link
Contributor

daniellehrner commented Dec 6, 2024

@siladu Breaking existing testnets isn't an issue. Mekong will not be updated, it's spec stays frozen, and devnet-4 has been shut down already

@siladu siladu force-pushed the eip-7742_engine-api branch from 7cd15a5 to 2b8fc09 Compare December 10, 2024 06:28
.map(Withdrawal::hashCode)
.reduce(1, (a, b) -> a ^ (b * 31)))
.orElse(0)
<< 32
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth extracting this to a method? it's a bit hard to read indented over so many lines

if (payloadAttributes.getTargetBlobsPerBlock() != null) {
message += ", targetBlobsPerBlock: {}";
builder = builder.setMessage(message).addArgument(payloadAttributes::getTargetBlobsPerBlock);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why not log getMaxBlobsPerBlock here too

@siladu
Copy link
Contributor Author

siladu commented Dec 17, 2024

Removed from Pectra

@siladu siladu closed this Dec 17, 2024
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.

3 participants