-
Notifications
You must be signed in to change notification settings - Fork 1k
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 proposer index to BeaconBlock #1626
Conversation
fb98f52
to
757f5a3
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.
Did an initial pass and it looks good. It's a substantial change however, so I like to do a second pass, check if there are more cases where we need to change things. And think about edge-cases like the genesis latest-header proposer index.
Sounds good. Added a couple of additional sanity block tests to cover the combinations on how the index might be messed up |
def process_randao(state: BeaconState, body: BeaconBlockBody) -> None:
epoch = get_current_epoch(state)
# Verify RANDAO reveal
proposer = state.validators[get_beacon_proposer_index(state)]
... could be refactored to def process_randao(state: BeaconState, block: BeaconBlock) -> None:
epoch = get_current_epoch(state)
# Verify RANDAO reveal
proposer = block.proposer_index
randao_reveal = block.body.randao_reveal
... |
@hwwhww I decided to make the changes minimally invasive and to only check that the specified proposer index in the block matches the expected in state once One place that could also be refactored is Since proposer shuffling lookup will almost certainly be memoized, this is safe and doesn't really add a cost to processing |
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.
LGTM, happy to see this! It will make cases that trust block-roots, but are unaware of the state, aware of proposers. This is good for tooling/bridges/etc.
Note for implementers though: careful, don't trust blocks blindly, follow spec and verify the proposer index.
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.
Agreed with minimal changes here and the clients can still optimize it if they want.
LGTM!
Addresses #1601
proposer_index
toBeaconBlock
andBeaconBlockHeader
proposer_index
fromProposerSlashing
and instead assert that both headers have same proposer indexComment from @protolambda below copied here for visibility