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

Engine API: unify failure mode for mismatched structure versions #337

Merged
merged 9 commits into from Jan 12, 2023

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Dec 20, 2022

Unifies return value for the case when the wrong data structure version is used in V2 method calls in a way that client software MUST return -32602: Invalid params error.

The idea behind this change is to make a future proof unification to this type of failure, and also get rid of abusing INVALID payload status and discrepancy between two methods in handling the same type of error that exists in the current version of the spec.

Note: We may introduce a new custom error type to be returned in the response. I just thought we might not need it but not opposed to it in general.

@ethDreamer
Copy link
Contributor

I like this. Can we add in the same change for the src/engine/experimental/blob-extension.md file as well?

@mkalinin
Copy link
Collaborator Author

I like this. Can we add in the same change for the src/engine/experimental/blob-extension.md file as well?

Done!

@ethDreamer
Copy link
Contributor

I like this. Can we add in the same change for the src/engine/experimental/blob-extension.md file as well?

Done!

Now I'm wondering if we should specify that the V3 methods could accept V1, V2, or V3 objects depending on the fork.
@ajsutton seemed to favor this approach and I can appreciate his reasoning.

@rolfyone
Copy link

This does explicitly seem to exclude that possibility...
Given how the at timestamp X, must call shanghai version etc, it's specifically incompatible with calling v2 if capella is defined but not active.
I definitely would prefer not having to keep v1 functions forever, which this is going to implicitly require that we do. It seemed fairly logical to be able to call v2 functions with null withdrawals prior to capella, and I'm not sure why that provision has been removed.
Now when we're syncing old blocks we'll be needing to toggle functions we call so that we're ensuring we call the right functions, so unless we ever do actually start dropping older blocks, we'll always need V* functions to provide that data to the EL. It's not unmanagable, but it's going to grow into a bit of a mess over time.

@rolfyone
Copy link

As a follow up, I had a chat with Mikhail about the intent here, and hadn't understood fully that we'd be able to call the V2 of a function with the v1 data structure after the timestamp passes, in order to perform things like syncing functions.
I think that answers the key point of how sync would happen, and does mean we can eventually deprecate old functions, which is the main issue I had raised above.
Now that I understand this, I think I'm ok with the intent behind this change...

@mkalinin
Copy link
Collaborator Author

Now I'm wondering if we should specify that the V3 methods could accept V1, V2, or V3 objects depending on the fork.

Good point. I have added V1 struct to the V3 method and think we should get back to it when we have more understanding up to which extent CL will send blocks to EL when doing backfilling. I am in favour of deprecating V1 struct as soon as it becomes possible.

@mkalinin mkalinin merged commit 939255f into ethereum:main Jan 12, 2023
siladu added a commit to hyperledger/besu that referenced this pull request Jan 18, 2023
Update engine API to include some new changes introduced in 
- ethereum/execution-apis#338
- ethereum/execution-apis#337

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
yperbasis pushed a commit to ledgerwatch/erigon that referenced this pull request Jan 18, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
Update engine API to include some new changes introduced in 
- ethereum/execution-apis#338
- ethereum/execution-apis#337

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Update engine API to include some new changes introduced in 
- ethereum/execution-apis#338
- ethereum/execution-apis#337

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com>
Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
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.

None yet

4 participants