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 dependent_root metadata to attestation and block duties #117

Merged
merged 4 commits into from
Nov 27, 2020

Conversation

ajsutton
Copy link
Contributor

Based on a proposal by @paulhauner (#116 (comment)), adds additional dependent_root fields which make it possible for validator clients to accurately determine when a chain reorg or block import has caused previously calculated duties to become invalid. The previous approach only said to monitor chain_reorg events but didn't provide any guarantee that chain_reorg events would be published in all cases that could require invalidating duties. See https://hackmd.io/kutQ7smJRZ-sJNSuY1WWVA#Why-head-must-be-published-for-empty-slots for an explanation of the scenario that is typically missed.

In this variant of the proposal, the target_root is included as a dependent_root metadata property for the attester and proposer duties responses. This avoids the need to break backwards compatibility by changing the data property from an object to an array and establishes a potentially useful concept of explicitly declaring the chain state that a response is dependent on. Which slot the block root must exist in isn't explicitly specified in the response but is defined in the description.

current_duty_dependent_root and previous_duty_dependent_root are then added to the head event, so that validator clients are able to determine when existing duties have been invalided.

head events are then only required when a new block is imported or when a reorg changes the head block, but not when the chain head state progresses due to an empty slot (and beacon nodes are then free to skip processing empty slots).

@ajsutton
Copy link
Contributor Author

This would remove the need for the additional semantics of head and chain_reorg events in #96. Whether to combine the head and chain_reorg events is then a separate question.

@paulhauner
Copy link
Contributor

I am working on this now. I think I want to suggest some changes, but I'm still not sure. Watch this space™

Copy link
Contributor

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

I am working on this now. I think I want to suggest some changes, but I'm still not sure. Watch this space™

I was right to doubt myself, I have no changes to suggest.

Thanks @ajsutton for writing this up, coming up with better names (dependent root) and fixing up my mistake with the proposer dependent root.

I hope block explorers and other external systems can also benefit from this additional piece of information. I suspect it would useful in keying databases.

@mpetrunic mpetrunic requested a review from djrtwo November 20, 2020 08:08
@mpetrunic
Copy link
Contributor

LGTM

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.

great work @ajsutton and @paulhauner

@djrtwo djrtwo merged commit ceb555f into ethereum:master Nov 27, 2020
bors bot pushed a commit to sigp/lighthouse that referenced this pull request Dec 4, 2020
## Issue Addressed

Resolves #1434 (this is the last major feature in the standard spec. There are only a couple of places we may be off-spec due to recent spec changes or ongoing discussion)
Partly addresses #1669
 
## Proposed Changes

- remove the websocket server
- remove the `TeeEventHandler` and `NullEventHandler` 
- add server sent events according to the eth2 API spec

## Additional Info

This is according to the currently unmerged PR here: ethereum/beacon-APIs#117


Co-authored-by: realbigsean <seananderson33@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants