always atomically update justified and finalized#2727
Merged
Conversation
validation if attestation is from a block message
djrtwo
changed the title
djrtwo
commented
Nov 18, 2021
tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py
Outdated
Show resolved
Hide resolved
…tified_checkpoint_ancestor`
Contributor
|
Also thank @ajsutton for noticing this issue months ago!
|
2 tasks
adiasg
approved these changes
Nov 22, 2021
Contributor
adiasg
left a comment
adiasg
left a comment
There was a problem hiding this comment.
This PR makes 2 changes:
- Only make atomic updates to store
- Allow old attestations from blocks to be processed
Fix for atomic updates to store looks good.
Also good to move ahead with processing old attestations from blocks for now - that's the only way to make atomic updates to the store work in our current testing setup. If that changes in the future, this logic should go through security analysis (esp. for flip-flop attacks).
6 tasks
bors bot
pushed a commit
to sigp/lighthouse
that referenced
this pull request
Dec 13, 2021
## Issue Addressed Resolves: #2741 Includes: #2853 so that we can get ssz static tests passing here on v1.1.6. If we want to merge that first, we can make this diff slightly smaller ## Proposed Changes - Changes the `justified_epoch` and `finalized_epoch` in the `ProtoArrayNode` each to an `Option<Checkpoint>`. The `Option` is necessary only for the migration, so not ideal. But does allow us to add a default logic to `None` on these fields during the database migration. - Adds a database migration from a legacy fork choice struct to the new one, search for all necessary block roots in fork choice by iterating through blocks in the db. - updates related to ethereum/consensus-specs#2727 - We will have to update the persisted forkchoice to make sure the justified checkpoint stored is correct according to the updated fork choice logic. This boils down to setting the forkchoice store's justified checkpoint to the justified checkpoint of the block that advanced the finalized checkpoint to the current one. - AFAICT there's no migration steps necessary for the update to allow applying attestations from prior blocks, but would appreciate confirmation on that - I updated the consensus spec tests to v1.1.6 here, but they will fail until we also implement the proposer score boost updates. I confirmed that the previously failing scenario `new_finalized_slot_is_justified_checkpoint_ancestor` will now pass after the boost updates, but haven't confirmed _all_ tests will pass because I just quickly stubbed out the proposer boost test scenario formatting. - This PR now also includes proposer boosting ethereum/consensus-specs#2730 ## Additional Info I realized checking justified and finalized roots in fork choice makes it more likely that we trigger this bug: ethereum/consensus-specs#2727 It's possible the combination of justified checkpoint and finalized checkpoint in the forkchoice store is different from in any block in fork choice. So when trying to startup our store's justified checkpoint seems invalid to the rest of fork choice (but it should be valid). When this happens we get an `InvalidBestNode` error and fail to start up. So I'm including that bugfix in this branch. Todo: - [x] Fix fork choice tests - [x] Self review - [x] Add fix for ethereum/consensus-specs#2727 - [x] Rebase onto Kintusgi - [x] Fix `num_active_validators` calculation as @michaelsproul pointed out - [x] Clean up db migrations Co-authored-by: realbigsean <seananderson33@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When finalized is updated, the fork choice should always update to the justification that finalized the new checkpoint atomically with the finality update.
This document goes into the problem unearthed and the solution -- https://notes.ethereum.org/YfStc2i6Rgenk_gzhEojfQ?view
Note that this bug can only be triggered on mainnet with >1/3 slashing, and if you are willing to do that, there are plenty of much more interesting attacks you'd probably pull off. This patch is a must prior to the Merge (along with proposer boosting!)
Thank you @realbigsean and @paulhauner for bringing the issue to our attention!
@adiasg's comment copied from below:
This PR makes 2 changes:
Fix for atomic updates to store looks good.
Also good to move ahead with processing old attestations from blocks for now - that's the only way to make atomic updates to the store work in our current testing setup. If that changes in the future, this logic should go through security analysis (esp. for flip-flop attacks).