-
Notifications
You must be signed in to change notification settings - Fork 781
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
Network Update for Weak Subjectivity Sync #2561
Conversation
f89b69b
to
626b0e8
Compare
cd8c1e7
to
ce13ae8
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.
Posting this partial review so it doesn't get eaten by github
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.
Seems to work really well, and I can't find any major faults! Happy to merge down!
Just a few minor nitpicks
beacon_node/network/src/beacon_processor/worker/sync_methods.rs
Outdated
Show resolved
Hide resolved
); | ||
"invalid_signature".into() | ||
} | ||
HistoricalBlockError::ValidatorPubkeyCacheTimeout => { |
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.
I had to add this error type when adding signature verification. Unlike the other errors this is our fault rather than the peer's, so we probably shouldn't penalise them, although I realise that might be hard to tweak.
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.
Yeah. This is a bit more work.
I think maybe we leave this, unless we expect this to occur regularly. Maybe we see how it goes in the wild and if its necessary, I can add in extra conditions where we don't penalize the peer.
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.
I've set the timeout super long so it really shouldn't happen unless the hardware is getting completely slogged. Should be fine to leave as-is.
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.
Perfect! Let's merge this!
## Issue Addressed Closes #1891 Closes #1784 ## Proposed Changes Implement checkpoint sync for Lighthouse, enabling it to start from a weak subjectivity checkpoint. ## Additional Info - [x] Return unavailable status for out-of-range blocks requested by peers (#2561) - [x] Implement sync daemon for fetching historical blocks (#2561) - [x] Verify chain hashes (either in `historical_blocks.rs` or the calling module) - [x] Consistency check for initial block + state - [x] Fetch the initial state and block from a beacon node HTTP endpoint - [x] Don't crash fetching beacon states by slot from the API - [x] Background service for state reconstruction, triggered by CLI flag or API call. Considered out of scope for this PR: - Drop the requirement to provide the `--checkpoint-block` (this would require some pretty heavy refactoring of block verification) Co-authored-by: Diva M <[email protected]>
This adds the network part to the weak subjectivity sync.