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

Finished Gossip Block Validation Conditions #2640

Merged
merged 5 commits into from
Sep 28, 2021

Conversation

ethDreamer
Copy link
Member

Finished implementing the gossip block validation conditions.

Copy link
Member

@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.

Looks good, just one TODO to remove

beacon_node/beacon_chain/src/block_verification.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added the merge-f2f Relates to the Oct 2021 Merge F2F label Sep 28, 2021
/// An implementation of the method described in the consensus spec here:
///
/// https://github.com/ethereum/consensus-specs/blob/dev/specs/merge/beacon-chain.md#compute_timestamp_at_slot
fn compute_timestamp_at_slot(&self, slot: Slot) -> Option<u64> {
Copy link
Member

Choose a reason for hiding this comment

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

This PR adds a second implementation of compute_timestamp_at_slot in the SlotClock to avoid accessing the head during block verification just to get the genesis_time. The original implementation still exists and is used in the consensus code, because we don't pass the SlotClock around the consensus code (and to keep the consensus code more similar to the spec). Wondering if people think that seems reasonable? Or should we try to keep a single implementation everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

@paulhauner how does this sound to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think this is a good idea. It's a simple enough function that I think it's worth the trade-off.

Very neat and correct impl too 🏅

@ethDreamer ethDreamer merged commit 0a0deb7 into sigp:merge-f2f Sep 28, 2021
paulhauner pushed a commit that referenced this pull request Oct 1, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Oct 12, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Oct 27, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit to paulhauner/lighthouse that referenced this pull request Nov 3, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Nov 11, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Nov 28, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Nov 28, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
paulhauner pushed a commit that referenced this pull request Dec 2, 2021
* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <[email protected]>
@ethDreamer ethDreamer deleted the block-validation branch December 19, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-f2f Relates to the Oct 2021 Merge F2F
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants