-
Notifications
You must be signed in to change notification settings - Fork 398
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 engine_newPayloadSyncContextV1
#318
Closed
Closed
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
81825fd
Add `engine_newPayloadHeaderV1`
etan-status 029af74
Merge branch 'main' into lc-eph
etan-status 1d42525
Satisfy spellcheck
etan-status d794b90
Change from SSZ to RLP hash
etan-status 08c0030
Cleanup
etan-status 834b36b
Improve wording
etan-status 6054dfb
Synchronize layout
etan-status c86db03
Consistent format to refer to newPayload response
etan-status 53beebe
Add new fields to nullability check
etan-status baa849c
Extend requirement for withdrawals
etan-status c206bc8
Only pass minimal sync context information
etan-status d7be255
Typo
etan-status 17b4222
Always return `SYNCING` for `newPayloadSyncContext`
etan-status 4a09624
Document error object
etan-status b05cb74
Improve grammar
etan-status File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a full
ExecutionPayloadHeader
is required to trigger the beacon sync, onlyblockNumber
andblockHash
are sufficient and rest EL clients can fetch off the wire, since they would anyway need to fetch full block body fortransactions
to eventually execute the block (and if we are being liberal we can includeparentHash
as well)So may be we can call it
ExecutionPayloadHeaderSlim
or somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For LES use case, having access to
logs_bloom
is also useful to determine whether a block contains useful information.The one truly questionable field is the
transactionsRoot
though – it being an SSZ root may make it difficult to get aVALID
/INVALID
classification from the EL without it also knowing how to create and verify those. For that one, I see three options:transactionsRoot
is not useful for any use case, in this case just drop it from the engine API structure, diverging from the beacon chain structure.transactionsRoot
would be useful as an RLP hash for some use cases, in this case extend the beacon chain structure with RLPtransactionsHash
that lives next to thetransactions
in bothExecutionPayload
andExecutionPayloadHeader
.transactionsRoot
is still useful in SSZ format, in this case just keep as is. Maybe for MEV blinded blocks to prove inclusion of particular tx against censoring, but those proofs could also be rooted instateRoot
I guess?On CL light client front, the extra size for full header is not substantially different from including just a couple of the fields, as proof size goes up for individual fields. So, full
ExecutionPayloadHeader
can be expected to be available. Any subset is alright for the engine API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, so...
for execution nodes running
full
protocol, it won't matter what kind oftransactionsRoot
we provide since they would still need to download the block off the wire (as you observed) using their own peers as they do in normal beacon sync when they don't have parents.Now for
les
we could make it easy by having the RLP transactions hash that the blockhash lines up and they don't have to download anything off wire (unless there is a missing header update in which case they will again have to)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ethereum/consensus-specs#3078
Opened a proposal here to add those RLP hashes to the
ExecutionPayload
/ExecutionPayloadHeader
as well, from Capella onward. This would make this much easier, removing the need for RLP computations in CL, or for SSZ computations in EL.