Merge unstable to deneb 20230822
#4649
Merged
Conversation
This file contains 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
## Issue Addressed Closes sigp#4245 ## Proposed Changes - If an SSE channel fills up, send a comment instead of terminating the stream. - Add a CLI flag for scaling up the SSE buffer: `--http-sse-capacity-multiplier N`. ## Additional Info ~~Blocked on sigp#4462. I haven't rebased on that PR yet for initial testing, because it still needs some more work to handle long-running HTTP threads.~~ - [x] Add CLI flag tests.
Since `tolerant_current_epoch` is expected to be either `current_epoch` or `current_epoch+1`, we can eliminate a case here. And added a comment about `compute_historic_attester_duties` , since `RelativeEpoch::from_epoch` will only allow `request_epoch == current_epoch-1` when `request_epoch < current_epoch`.
…ties_sync` (sigp#4617) It seems `post_validator_duties_sync` is the only api which doesn't have its own metric in `duties_service`, this PR adds `metrics::VALIDATOR_DUTIES_SYNC_HTTP_POST` for completeness.
## Proposed Changes This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉 Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy: - Keep both `modern` and `portable` builds for releases/Docker images. - Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits. - Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware). - Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
…uning (sigp#4610) ## Issue Addressed Closes sigp#3210 Closes sigp#3211 ## Proposed Changes - Checkpoint sync from the latest finalized state regardless of its alignment. - Add the `block_root` to the database's split point. This is _only_ added to the in-memory split in order to avoid a schema migration. See `load_split`. - Add a new method to the DB called `get_advanced_state`, which looks up a state _by block root_, with a `state_root` as fallback. Using this method prevents accidental accesses of the split's unadvanced state, which does not exist in the hot DB and is not guaranteed to exist in the freezer DB at all. Previously Lighthouse would look up this state _from the freezer DB_, even if it was required for block/attestation processing, which was suboptimal. - Replace several state look-ups in block and attestation processing with `get_advanced_state` so that they can't hit the split block's unadvanced state. - Do not store any states in the freezer database by default. All states will be deleted upon being evicted from the hot database unless `--reconstruct-historic-states` is set. The anchor info which was previously used for checkpoint sync is used to implement this, including when syncing from genesis. ## Additional Info Needs further testing. I want to stress-test the pruned database under Hydra. The `get_advanced_state` method is intended to become more relevant over time: `tree-states` includes an identically named method that returns advanced states from its in-memory cache. Co-authored-by: realbigsean <seananderson33@gmail.com>
…p#4626) `parent_finalized.epoch + 1 > block_epoch` will never be `true` since as the comment says: ``` A block in epoch `N` cannot contain attestations which would finalize an epoch higher than `N - 1`. ```
## Issue Addressed Fixes a bug in the handling of `--beacon-process-max-workers` which caused it to have no effect. ## Proposed Changes For this PR I channeled @ethDreamer and saw deep into the faulty CLI config -- this bug is almost identical to the one Mark found and fixed in sigp#4622.
## Issue Addressed Wrong link for anvil ## Proposed Changes Fix the link to correct one.
## Issue Addressed This PR remove Antithesis docker build CI workflow. Confirmed with Antithesis team that this is no longer used.
# Conflicts: # beacon_node/beacon_chain/src/builder.rs # beacon_node/beacon_chain/tests/store_tests.rs # beacon_node/client/src/builder.rs # beacon_node/src/config.rs # beacon_node/store/src/hot_cold_store.rs # lighthouse/tests/beacon_node.rs
jimmygchen
commented
Aug 22, 2023
Comment on lines
+426
to
+468
| let store = self | ||
| .store | ||
| .clone() | ||
| .ok_or("weak_subjectivity_state requires a store")?; | ||
| let log = self | ||
| .log | ||
| .as_ref() | ||
| .ok_or("weak_subjectivity_state requires a log")?; | ||
|
|
||
| // Check that the given state lies on an epoch boundary. Due to the database only storing | ||
| // full states on epoch boundaries and at restore points it would be difficult to support | ||
| // starting from a mid-epoch state. | ||
| if weak_subj_slot % TEthSpec::slots_per_epoch() != 0 { | ||
| return Err(format!( | ||
| "Checkpoint state at slot {} is not aligned to epoch start. \ | ||
| Please supply an aligned checkpoint with state.slot % 32 == 0", | ||
| weak_subj_slot, | ||
| )); | ||
| // Ensure the state is advanced to an epoch boundary. | ||
| let slots_per_epoch = TEthSpec::slots_per_epoch(); | ||
| if weak_subj_state.slot() % slots_per_epoch != 0 { | ||
| debug!( | ||
| log, | ||
| "Advancing checkpoint state to boundary"; | ||
| "state_slot" => weak_subj_state.slot(), | ||
| "block_slot" => weak_subj_block.slot(), | ||
| ); | ||
| while weak_subj_state.slot() % slots_per_epoch != 0 { | ||
| per_slot_processing(&mut weak_subj_state, None, &self.spec) | ||
| .map_err(|e| format!("Error advancing state: {e:?}"))?; | ||
| } | ||
| } | ||
|
|
||
| // Prime all caches before storing the state in the database and computing the tree hash | ||
| // root. | ||
| weak_subj_state | ||
| .build_caches(&self.spec) | ||
| .map_err(|e| format!("Error building caches on checkpoint state: {e:?}"))?; | ||
| weak_subj_state | ||
| let weak_subj_state_root = weak_subj_state | ||
| .update_tree_hash_cache() | ||
| .map_err(|e| format!("Error computing checkpoint state root: {:?}", e))?; | ||
|
|
||
| let latest_block_slot = weak_subj_state.latest_block_header().slot; | ||
| let weak_subj_slot = weak_subj_state.slot(); | ||
| let weak_subj_block_root = weak_subj_block.canonical_root(); | ||
|
|
||
| // We can only validate the block root if it exists in the state. We can't calculated it | ||
| // from the `latest_block_header` because the state root might be set to the zero hash. | ||
| if let Ok(state_slot_block_root) = weak_subj_state.get_block_root(latest_block_slot) { | ||
| if weak_subj_block_root != *state_slot_block_root { | ||
| return Err(format!( | ||
| "Snapshot state's most recent block root does not match block, expected: {:?}, got: {:?}", | ||
| weak_subj_block_root, state_slot_block_root | ||
| )); | ||
| } | ||
| // Validate the state's `latest_block_header` against the checkpoint block. | ||
| let state_latest_block_root = weak_subj_state.get_latest_block_root(weak_subj_state_root); | ||
| if weak_subj_block_root != state_latest_block_root { | ||
| return Err(format!( | ||
| "Snapshot state's most recent block root does not match block, expected: {:?}, got: {:?}", | ||
| weak_subj_block_root, state_latest_block_root | ||
| )); |
There was a problem hiding this comment.
This was the conflicting bit, but should be fine to accept unstable changes, as it was built on top of the same checkpoint sync alignment branch.
There was a problem hiding this comment.
Agreed. Looks like you faithfully maintained the logic from unstable (based on the diff here: unstable...jimmygchen:lighthouse:merge-unstable-to-deneb-20230822#diff-44c324564286d739ad29b047b484dc7fd1ba47741a3e07665302dcd10ab83f9c)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue Addressed
There are a few conflicts now with
unstable, so probably worth resolving these now as the deneb PR is going through a review.The main conflicts are the changes in checkpoint alignment #4610. Seeing that the PR had cherry-picked from #4389 (which was the same branch deneb got these changes from), and modifications were made on top of it, I've accepted the conflicting changes from
unstable.