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

Make Engine *V3s handle V3 requests only #5845

Merged
merged 9 commits into from
Jul 18, 2023
Merged

Make Engine *V3s handle V3 requests only #5845

merged 9 commits into from
Jul 18, 2023

Conversation

flcl42
Copy link
Contributor

@flcl42 flcl42 commented Jun 21, 2023 •
edited

Requires #5596 to be merged

Implements ethereum/execution-apis #418

engine_getPayloadV3, engine_newPayloadV3 to handle Cancun payloads only.

Changes

  • Make all fields required
  • Return -38005: Unsupported fork, when V3 is called for a pre-Cancun payload

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Requires Cancun fork

@flcl42 flcl42 force-pushed the feature/eip-4844-v3-handles-v3-only branch 2 times, most recently from dfcb6bf to 78bd6dd Compare June 27, 2023 15:13
@flcl42 flcl42 marked this pull request as ready for review June 27, 2023 15:15
@flcl42 flcl42 changed the base branch from feature/eip-4844-v6 to feature/eip-4844-gas-calculations June 27, 2023 15:15
@flcl42 flcl42 force-pushed the feature/eip-4844-v3-handles-v3-only branch from 78bd6dd to f856077 Compare June 27, 2023 15:17
marcindsobczak
marcindsobczak approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

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

Looks correct!

@flcl42 flcl42 force-pushed the feature/eip-4844-gas-calculations branch from 0830df3 to fbdf0a9 Compare June 30, 2023 13:07
Base automatically changed from feature/eip-4844-gas-calculations to master June 30, 2023 13:27
@flcl42 flcl42 force-pushed the feature/eip-4844-v3-handles-v3-only branch from f856077 to f431233 Compare June 30, 2023 13:42
@flcl42 flcl42 marked this pull request as draft June 30, 2023 15:42
LukaszRozmej
LukaszRozmej requested changes Jul 4, 2023
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.HelperFunctions.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/GetPayloadV2Result.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Handlers/GetPayloadHandlerBase.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.Cancun.cs Outdated Show resolved Hide resolved
@LukaszRozmej
Copy link
Member

If we are going class hierarchy - which we now can and should! - let's use polymorphism. Previous design prevented us from doing that.

@LukaszRozmej LukaszRozmej mentioned this pull request Jul 7, 2023
Closed
12 tasks
@LukaszRozmej
Copy link
Member

LukaszRozmej commented Jul 7, 2023 •
edited

Or maybe we shouldn't have class hierarchies in engine API parameter classes? This would make multi-dimensional extensions hard.
This spans 3 things now:

  1. This PR
  2. Eip 4788/beacon parent block root in evm #5476
  3. Optimism extensions: https://github.com/NethermindEth/nethermind/blob/a44018a1d37cc4227a5ad88d95a16ead8bf2b931/src/Nethermind/Nethermind.Optimism/OptimismPayloadAttributes.cs
  4. Potentially in future other orthogonal extensions to Engine API.

Now imagine that for now, Optimism doesn't need 4844 and #5476 fields, but at some point, it starts to need it. It will be hard to model the data classes correctly.

Options:

  1. Duplicate the classes and check for optimism extensions with interfaces.
  2. Design a more sophisticated method of composing extensions to requests.
  3. Keep only one message type in main repo and just use null'able fields (what we do now)
  4. Ignore for now :)

@flcl42 flcl42 marked this pull request as ready for review July 12, 2023 12:42
@flcl42 flcl42 requested a review from LukaszRozmej July 12, 2023 13:03
@flcl42
Merge branch 'feature/eip-4844-v3-handles-v3-only' of github.com:Neth…
b71b755 
…ermindEth/nethermind into feature/eip-4844-v3-handles-v3-only
LukaszRozmej
LukaszRozmej reviewed Jul 14, 2023
View reviewed changes
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

comments + #5932

src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.HelperFunctions.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V3.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V3.cs Show resolved Hide resolved
@LukaszRozmej
Introduce ExecutionPayloadParams concept to be able to better compose… (
eac4723 
#5932)

* Introduce ExecutionPayloadParams concept to be able to better compose and validate complex method signatures

* whitespace

* fix CreateBlockRequest

* Revert "fix CreateBlockRequest"

This reverts commit 21c72e4.

* fix

* fixes

* fix

* fix whitespace

* support V1 error codes
@flcl42 flcl42 requested a review from LukaszRozmej July 18, 2023 09:20
@flcl42 flcl42 merged commit 429094d into master Jul 18, 2023
61 checks passed
@flcl42 flcl42 deleted the feature/eip-4844-v3-handles-v3-only branch July 18, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarekM25 MarekM25 MarekM25 approved these changes

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@marcindsobczak marcindsobczak marcindsobczak approved these changes

Assignees
No one assigned
Labels
eip-4844
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flcl42 @LukaszRozmej @MarekM25 @marcindsobczak