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

Add randao to execution payload #2479

Merged
merged 8 commits into from
Jun 23, 2021

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Jun 11, 2021

Adds the most recent randao_mix into execution payload.
This PR is a follow up to the corresponding discussion that took place in Merge Implementers' Call 5.

The value of new randao field in ExecutionPayload must be assigned to difficulty field of derived ExecutionBlock. And further randao value is exposed by the DIFFICULTY opcode in the EVM. This way allows to preserve randomness property of the value returned by this opcode. Potentially, DIFFICULTY opcode will be renamed to RANDAO and is one of the post-Merge cleanups.

There two reasons why randao should be an explicit part of the payload:
- ExecutionPayload is the only input required to build corresponding ExecutionBlock
- further compatibility with stateless execution

UPD
The main idea of this particular solution is to exclude EVM changes from the Merge scope. An implementation where randao value is passed alongside with the payload and utilized by EVM context directly without requirement of being a part of ExecutionBlock could be done after the Merge within other EVM changes.

Should be rebased after #2472 gets merged

specs/merge/validator.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice! a bit confused about Bytes32() in the helpers for randao reveal but otherwise, looking good

specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/helpers/block.py Outdated Show resolved Hide resolved
@mkalinin mkalinin marked this pull request as ready for review June 17, 2021 15:28
@JustinDrake JustinDrake added the Bellatrix CL+EL Merge label Jun 20, 2021
@JustinDrake
Copy link
Collaborator

suggested polishing:

Screenshot 2021-06-20 at 20 06 59

Screenshot 2021-06-20 at 20 07 51

@djrtwo
Copy link
Contributor

djrtwo commented Jun 21, 2021

I'm okay with the suggested cleanups.

Although it is an important design goal for execution layer to be parallelizable, I don't think the added param moves the needle too much on making that clear to the reader. I would suggest noting the parallelizability in the header comment (or pre-function text) of process_execution_payload and make it clear there if there are any important considerations.

@mkalinin
Copy link
Collaborator Author

Thanks everyone for your inputs! I've accepted suggested polishing with related comment about randao_mix dependency in process_execution_payload and its impact on ability to process the payload in parallel. I am ok with this change now and it can be merged once approved.

@djrtwo djrtwo merged commit 00afb34 into ethereum:dev Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bellatrix CL+EL Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants