-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
EIP-4788: Beacon state root in EVM #4788
EIP-4788: Beacon state root in EVM #4788
Conversation
All tests passed; auto-merging...(pass) eip-4788.md
|
I would still favor using the slot number and not the block number, and copying the most recent block hash into multiple slots if there are skips the same way as the consensus chain history accumulator does it (see the Reasons:
I don't think the extra complexity of having an SSTORE loop is that large; it's a simple loop, and the beacon chain already does it. |
If I'm reading it correctly, it appears that |
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.
gm
EIPS/eip-beacon_state_root_in_evm.md
Outdated
|
||
## Abstract | ||
|
||
Commit to the state root of the beacon chain in the `ommers` field in the post-merge execution block. Reflect the changes in the `ommersHash` field of the execution block header. |
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.
Recommend referring to these fields by their position, since the names are meaningless (especially post-merge since it is just a fixed value). You could say something like "in position x
, which formerly held the ommersHash
".
There is no discussions-to URL yet so posting this comment here: |
The primary goal here is to get the beacon accumulator into the execution block header (or at least committed to there). Execution clients already have this functionality to include ommer headers (and commit to them via the This EIP suggests to reuse this machinery as it can simply be used as is, without having to introduce further development and testing requirements for client developers going into Shanghai (that already has a number of EIPS already competing for scarce developer attention). There are a couple of alternatives if we don't want to go this route:
I want to get client developer input on this point to see what the appetite is for the various options. |
I agree with all of your reasoning -- the concern is efficient reads and writes in the case where we have many skipped slots. Unlikely to happen on mainnet, but we should still consider worst case behavior. Will think this over some more... |
updated! https://ethereum-magicians.org/t/eip-4788-beacon-state-root-in-evm/8281 feel free to migrate this there if you want :)
we could go either way here -- the important context is that all of the block roots are committed to in the beacon state (so only a few extra hashes in a merkle proof to go from state root to any block root) and so it is fairly easy to e.g. prove some RANDAO value in a block via the state root. the opposite also holds: each block has its state root, so we could just as easily use the block root and then (with a few extra hashes) reach the state root and then prove anything in the state we wanted... my personal feeling is that there is more in the state that a user would want to make proofs against (e.g. finalized or justified blocks for light client-esque bridges) and so we should favor that use case more (by requiring fewer hashes to process in a proof) -- knowing this do you still see a case for block roots? |
c65626b
to
459342e
Compare
459342e
to
8809ca0
Compare
|
CI blocked on:
|
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.
No blockers, but recommend applying or commenting on the remaining feedback.
|
||
Beginning at the execution timestamp `FORK_TIMESTAMP`, execution clients **MUST**: | ||
|
||
1. set the value of the `ommers` field in the block to an RLP list with one element: the 32 byte [hash tree root](https://github.com/ethereum/consensus-specs/blob/dev/ssz/simple-serialize.md#merkleization) of the [beacon state](https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#beaconstate) from the previous slot to this block. |
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.
These should link to specific commit hashes rather than files in a dev
branch. Also, are these not available in the primary consensus spec yet (hence why you are linking to dev
rather than main
/master
)?
## Test Cases | ||
|
||
TODO | ||
|
||
## Reference Implementation | ||
|
||
TODO | ||
|
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.
## Test Cases | |
TODO | |
## Reference Implementation | |
TODO |
These sections are optional, recommend just removing them until you have something useful to include.
agreeed |
* Add EIP for beacon state root in EVM * fix typo Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Expose the beacon state root in the EVM by including it in each execution block and introduce an opcode to facilitate reading the roots.