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

Integrate request validation in EnginveValidator #13849

Open
mattsse opened this issue Jan 18, 2025 · 3 comments · May be fixed by #13858
Open

Integrate request validation in EnginveValidator #13849

mattsse opened this issue Jan 18, 2025 · 3 comments · May be fixed by #13858
Assignees
Labels
A-consensus Related to the consensus engine A-rpc Related to the RPC implementation C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Jan 18, 2025

Describe the feature

I think this is an alternative to

#13831

and

alloy-rs/op-alloy#395

currently we do:

validate_execution_requests(&execution_requests)?;

empty requests should be enforced anyway via the hash check but makes sense to add an additional check for this directly in engine API validation

we should move validate_execution_requests into

pub trait EngineValidator<Types: EngineTypes>: PayloadValidator {

cc @emhane

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Jan 18, 2025
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started A-rpc Related to the RPC implementation A-consensus Related to the consensus engine and removed S-needs-triage This issue needs to be labelled labels Jan 18, 2025
@hoank101
Copy link
Contributor

Hi i can do it

@mattsse
Copy link
Collaborator Author

mattsse commented Jan 18, 2025

assigned, ty!

@emhane
Copy link
Member

emhane commented Jan 18, 2025

@mattsse can you please explain how this makes the code run differently for optimism and ethereum? this check would have to be moved into reth-optimism-payload-builder and reth-ethereum-payload-builder in order to run a different validation on EL requests wrt network

as for the linked issue #13831, this is smthg we want in the end anyway, even if it doesn't block inclusion of the EL req eip in next op hf. eventually we want to use the op-alloy trait engine api in op-reth, not the same as ethereum, so it needs to be exposed at sdk level. low priority though, but very helpful if impl ♡

as for the linked op-alloy issue, to fix the blanket impl of OpEngineApi is still relevant, otherwise it breaks spec. EL reqs must be checked to be empty. Ref ethereum-optimism/specs#525.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine A-rpc Related to the RPC implementation C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants