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
refactor: move data availability validation out of state transition to allow optimistic sync #5178
Conversation
3fd9506
to
8556227
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
8556227
to
387c252
Compare
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.
While this was changed in the spec we don't really have to change anything right? I really like the need to have to explicitly state if the availability of the blob on import as it forces critical decisions publicly. Then if a block is not available it wont ever be imported to the forkchoice so it is not a concern to it
I can accumulate and make the dataAvailable flags pass upstream to forkchoice's i feel it doesnot belong to block state transition, also feel the same for execution status too that we pass there. |
Moving to draft and adding discussion tag |
Pull request was converted to draft
79a94b7
to
657e1aa
Compare
657e1aa
to
4047185
Compare
4047185
to
f9049ca
Compare
06bb540
to
600e7c7
Compare
@@ -254,7 +253,6 @@ async function getFinalizedState( | |||
state = stateTransition(state, block, { | |||
// Replaying finalized blocks, all data is considered valid | |||
executionPayloadStatus: ExecutionPayloadStatus.valid, | |||
dataAvailableStatus: DataAvailableStatus.available, |
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.
The main purpose of this notation was to force all consumers to explicitly declare assumptions about blobs. We will have to support different paths like regen block replay where we don't want to load blobs, but already know they are available. I'm aware it's technically pointless but being verbose about such critical decisions sounded good to me. I'm fine moving DA check outside of state transition, but this explicit statements should stay IMO
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.
ok sounds good, will make modifications
600e7c7
to
3af1a13
Compare
…nsition rebase fixes retain dataAvailabilityStatus flag in state transition reduce diff correctly propogate availability status while block processing
7436e10
to
2966201
Compare
rebased to resolve conflicts |
🎉 This PR is included in v1.9.0 🎉 |
ref
is_data_available
check to fork-choiceon_block
ethereum/consensus-specs#3185