Fork choice upgrade #3290
Merged
Fork choice upgrade #3290
Conversation
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
potuz
reviewed
Mar 14, 2023
djrtwo
reviewed
Mar 14, 2023
Contributor
djrtwo
left a comment
djrtwo
left a comment
There was a problem hiding this comment.
very nice! I mainly had some minor input on style and comments
Co-authored-by: Danny Ryan <[email protected]>
bors bot
pushed a commit
to sigp/lighthouse
that referenced
this pull request
Mar 21, 2023
## Issue Addressed NA ## Proposed Changes - Implements ethereum/consensus-specs#3290 - Bumps `ef-tests` to [v1.3.0-rc.4](https://github.com/ethereum/consensus-spec-tests/releases/tag/v1.3.0-rc.4). The `CountRealizedFull` concept has been removed and the `--count-unrealized-full` and `--count-unrealized` BN flags now do nothing but log a `WARN` when used. ## Database Migration Debt This PR removes the `best_justified_checkpoint` from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the `Option`s around the justified and finalized checkpoints on `ProtoNode` whilst we're at it. Those options were added in #2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to `Some` ever since v2.1.0. There's no reason to keep them as options anymore. I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration [over in my repo](https://github.com/paulhauner/lighthouse/tree/fc-pr-18-migration) so we can pick it up after this PR is merged.
etan-status
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
May 5, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290 Note: Some test networks still define the constant, ignoring the config constant for now until it is no longer used.
etan-status
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
May 5, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290 Note: Some test networks still define the constant, ignoring the config constant for now until it is no longer used.
etan-status
added a commit
to etan-status/eth2-networks
that referenced
this pull request
May 5, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290
This was referenced May 5, 2023
etan-status
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
May 5, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290 Note: Some test networks still define the constant, ignoring the config constant for now until it is no longer used.
dapplion
pushed a commit
to gnosischain/configs
that referenced
this pull request
May 14, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290
tersec
pushed a commit
to eth-clients/eth2-networks
that referenced
this pull request
Jun 14, 2023
The `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` constant is no longer used as the bouncing attack fix was removed: ethereum/consensus-specs#3290
haxxpop
reviewed
Jun 21, 2023
| ) | ||
|
|
||
| # If the previous epoch is justified, the block should be pulled-up. In this case, check that unrealized | ||
| # justification is higher than the store and that the voting source is not more than two epochs ago |
Member
There was a problem hiding this comment.
Can you help explain this a bit more? I'm curious about the intuition behind it.
It seems to me that this contradict to the HLMD-GHOST in https://arxiv.org/pdf/2003.03052.pdf
L′ ← set of leaf blocks Bl in G such that (Bj , j) ∈ J(ffgview(Bl))
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
NA - Implements ethereum/consensus-specs#3290 - Bumps `ef-tests` to [v1.3.0-rc.4](https://github.com/ethereum/consensus-spec-tests/releases/tag/v1.3.0-rc.4). The `CountRealizedFull` concept has been removed and the `--count-unrealized-full` and `--count-unrealized` BN flags now do nothing but log a `WARN` when used. This PR removes the `best_justified_checkpoint` from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the `Option`s around the justified and finalized checkpoints on `ProtoNode` whilst we're at it. Those options were added in sigp#2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to `Some` ever since v2.1.0. There's no reason to keep them as options anymore. I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration [over in my repo](https://github.com/paulhauner/lighthouse/tree/fc-pr-18-migration) so we can pick it up after this PR is merged.
etan-status
added a commit
to status-im/nimbus-eth2
that referenced
this pull request
Feb 5, 2026
Since ethereum/consensus-specs#3290 the checks `justified_checkpoint_root` and `best_justified_checkpoint` are not used in consensus-spec tests anymore and can be dropped.
tersec
pushed a commit
to status-im/nimbus-eth2
that referenced
this pull request
Feb 5, 2026
Since ethereum/consensus-specs#3290 the checks `justified_checkpoint_root` and `best_justified_checkpoint` are not used in consensus-spec tests anymore and can be dropped.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
This PR proposes a large fork choice upgrade, including:
SAFE_SLOTS_TO_UPDATE_JUSTIFIEDmark. A detailed explanation of the issue by @fradamt is attached here. We remove the earlier fix, i.e.,Store.best_justified_checkpointandSAFE_SLOTS_TO_UPDATE_JUSTIFIED, leading to a massive simplification of the fork choice spec.AttesterSlashingis received. We strengthen this by also censoring validators who are slashed in the state of theStore.justified_checkpoint.Fork choice test vectors: