From 77cb6359e135192d84b5f68070351c517315b30c Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 29 Sep 2025 13:15:20 +1000 Subject: [PATCH 01/14] More proposer shuffling cleanup (WIP) --- beacon_node/beacon_chain/src/beacon_chain.rs | 18 ++++++++++++++++ .../beacon_chain/src/beacon_proposer_cache.rs | 5 +++-- .../beacon_chain/src/block_verification.rs | 2 -- beacon_node/beacon_chain/tests/store_tests.rs | 21 ++++++++++++++++--- beacon_node/http_api/src/proposer_duties.rs | 17 ++++++++------- 5 files changed, 48 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index afbf3278fe0..9fdea603b1c 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -6553,6 +6553,24 @@ impl BeaconChain { } } + /// This function provides safe and efficient multi-threaded access to the beacon proposer cache. + /// + /// The arguments are: + /// + /// - `shuffling_decision_block`: The block root of the decision block for the desired proposer + /// shuffling. This should be computed using one of the methods for computing proposer + /// shuffling decision roots, e.g. `BeaconState::proposer_shuffling_decision_root_at_epoch`. + /// - `proposal_epoch`: The epoch at which the proposer shuffling is required. + /// - `accessor`: A closure to run against the proposers for the selected epoch. Usually this + /// closure just grabs a single proposer, or takes the vec of proposers for the epoch. + /// - `state_provider`: A closure to compute a state suitable for determining the shuffling. + /// This closure is evaluated lazily ONLY in the case + /// + /// This function makes use of closures in order to efficiently handle concurrent accesses to + /// the cache. + /// + /// The error type is polymorphic, if in doubt you can use `BeaconChainError`. You might need + /// to use a turbofish if type inference can't work it out. pub fn with_proposer_cache + From>( &self, shuffling_decision_block: Hash256, diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index a64b4981cc4..f0874038c61 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -17,6 +17,7 @@ use smallvec::SmallVec; use state_processing::state_advance::partial_state_advance; use std::num::NonZeroUsize; use std::sync::Arc; +use tracing::instrument; use types::non_zero_usize::new_non_zero_usize; use types::{ BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, Fork, Hash256, Slot, Unsigned, @@ -199,8 +200,7 @@ pub fn compute_proposer_duties_from_head( .map_err(BeaconChainError::from)?; let dependent_root = state - // The only block which decides its own shuffling is the genesis block. - .proposer_shuffling_decision_root(chain.genesis_block_root, &chain.spec) + .proposer_shuffling_decision_root(head_block_root, &chain.spec) .map_err(BeaconChainError::from)?; Ok((indices, dependent_root, execution_status, state.fork())) @@ -214,6 +214,7 @@ pub fn compute_proposer_duties_from_head( /// - No-op if `state.current_epoch() == target_epoch`. /// - It must be the case that `state.canonical_root() == state_root`, but this function will not /// check that. +#[instrument(skip_all, fields(?state_root, %target_epoch, state_slot = %state.slot()), level = "debug")] pub fn ensure_state_can_determine_proposers_for_epoch( state: &mut BeaconState, state_root: Hash256, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index d0ed8258e55..691293b2000 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -950,8 +950,6 @@ impl GossipVerifiedBlock { let proposer_shuffling_decision_block = parent_block.proposer_shuffling_root_for_child_block(block_epoch, &chain.spec); - // We assign to a variable instead of using `if let Some` directly to ensure we drop the - // write lock before trying to acquire it again in the `else` clause. let block_slot = block.slot(); let mut opt_parent = None; let proposer = chain.with_proposer_cache::<_, BlockError>( diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index cd4032f55d9..1c0fda16962 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -1273,19 +1273,34 @@ async fn proposer_shuffling_root_consistency_test( #[tokio::test] async fn proposer_shuffling_root_consistency_same_epoch() { let spec = test_spec::(); - proposer_shuffling_root_consistency_test(spec, 32, 39).await; + proposer_shuffling_root_consistency_test( + spec, + 4 * E::slots_per_epoch(), + 5 * E::slots_per_epoch() - 1, + ) + .await; } #[tokio::test] async fn proposer_shuffling_root_consistency_next_epoch() { let spec = test_spec::(); - proposer_shuffling_root_consistency_test(spec, 32, 47).await; + proposer_shuffling_root_consistency_test( + spec, + 4 * E::slots_per_epoch(), + 6 * E::slots_per_epoch() - 1, + ) + .await; } #[tokio::test] async fn proposer_shuffling_root_consistency_two_epochs() { let spec = test_spec::(); - proposer_shuffling_root_consistency_test(spec, 32, 55).await; + proposer_shuffling_root_consistency_test( + spec, + 4 * E::slots_per_epoch(), + 7 * E::slots_per_epoch() - 1, + ) + .await; } #[tokio::test] diff --git a/beacon_node/http_api/src/proposer_duties.rs b/beacon_node/http_api/src/proposer_duties.rs index ceac60cbad1..78f99c475ce 100644 --- a/beacon_node/http_api/src/proposer_duties.rs +++ b/beacon_node/http_api/src/proposer_duties.rs @@ -103,14 +103,6 @@ fn try_proposer_duties_from_cache( let head_block = &head.snapshot.beacon_block; let head_block_root = head.head_block_root(); let head_epoch = head_block.slot().epoch(T::EthSpec::slots_per_epoch()); - let head_decision_root = head - .snapshot - .beacon_state - .proposer_shuffling_decision_root(head_block_root, &chain.spec) - .map_err(warp_utils::reject::beacon_state_error)?; - let execution_optimistic = chain - .is_optimistic_or_invalid_head_block(head_block) - .map_err(warp_utils::reject::unhandled_error)?; // This code path can't handle requests for past epochs. if head_epoch > request_epoch { @@ -119,6 +111,15 @@ fn try_proposer_duties_from_cache( ))); } + let head_decision_root = head + .snapshot + .beacon_state + .proposer_shuffling_decision_root_at_epoch(request_epoch, head_block_root, &chain.spec) + .map_err(warp_utils::reject::beacon_state_error)?; + let execution_optimistic = chain + .is_optimistic_or_invalid_head_block(head_block) + .map_err(warp_utils::reject::unhandled_error)?; + chain .beacon_proposer_cache .lock() From 59b873b2834252c6f895a5dca7d041375a67c21a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 29 Sep 2025 13:28:49 +1000 Subject: [PATCH 02/14] Doc lint --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9fdea603b1c..0e977bdd91b 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -6562,9 +6562,9 @@ impl BeaconChain { /// shuffling decision roots, e.g. `BeaconState::proposer_shuffling_decision_root_at_epoch`. /// - `proposal_epoch`: The epoch at which the proposer shuffling is required. /// - `accessor`: A closure to run against the proposers for the selected epoch. Usually this - /// closure just grabs a single proposer, or takes the vec of proposers for the epoch. + /// closure just grabs a single proposer, or takes the vec of proposers for the epoch. /// - `state_provider`: A closure to compute a state suitable for determining the shuffling. - /// This closure is evaluated lazily ONLY in the case + /// This closure is evaluated lazily ONLY in the case /// /// This function makes use of closures in order to efficiently handle concurrent accesses to /// the cache. From e9b7a5100c8968d1d0879917a2a6ff6148e3ed83 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 29 Sep 2025 13:29:01 +1000 Subject: [PATCH 03/14] Use explicit epoch in compute_proposer_duties_from_head --- beacon_node/beacon_chain/src/beacon_proposer_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index f0874038c61..06c0f0d21b5 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -200,7 +200,7 @@ pub fn compute_proposer_duties_from_head( .map_err(BeaconChainError::from)?; let dependent_root = state - .proposer_shuffling_decision_root(head_block_root, &chain.spec) + .proposer_shuffling_decision_root_at_epoch(request_epoch, head_block_root, &chain.spec) .map_err(BeaconChainError::from)?; Ok((indices, dependent_root, execution_status, state.fork())) From 5aba2ecd3e8341ff951aaf18ffe4f86c7835ab41 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 29 Sep 2025 15:58:41 +1000 Subject: [PATCH 04/14] Update the state advance --- .../beacon_chain/src/state_advance_timer.rs | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index 27c2c7c0a11..b19425a8bc8 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -365,25 +365,40 @@ fn advance_head(beacon_chain: &Arc>) -> Resu .build_committee_cache(RelativeEpoch::Next, &beacon_chain.spec) .map_err(BeaconChainError::from)?; - // If the `pre_state` is in a later epoch than `state`, pre-emptively add the proposer shuffling - // for the state's current epoch and the committee cache for the state's next epoch. + // The state root is required to prime the proposer cache AND for writing it to disk. + let advanced_state_root = state.update_tree_hash_cache()?; + + // If the `pre_state` is in a later epoch than `state`, pre-emptively update the proposer + // shuffling and attester shuffling caches. if initial_epoch < state.current_epoch() { - // Update the proposer cache. - // - // We supply the `head_block_root` as the decision block since the prior `if` statement guarantees - // the head root is the latest block from the prior epoch. - beacon_chain - .beacon_proposer_cache - .lock() - .insert( - state.current_epoch(), - head_block_root, - state - .get_beacon_proposer_indices(state.current_epoch(), &beacon_chain.spec) - .map_err(BeaconChainError::from)?, - state.fork(), - ) - .map_err(BeaconChainError::from)?; + // Include the proposer shuffling from the current epoch, which is likely to be useful + // pre-Fulu, and probably redundant post-Fulu (it should already have been in the cache). + let current_epoch_decision_root = state.proposer_shuffling_decision_root_at_epoch( + state.current_epoch(), + head_block_root, + &beacon_chain.spec, + )?; + beacon_chain.with_proposer_cache( + current_epoch_decision_root, + state.current_epoch(), + |_| Ok(()), + || Ok::<_, Error>((advanced_state_root, state.clone())), + )?; + + // For epochs *greater than* the Fulu fork epoch, we have also determined the proposer + // shuffling for the next epoch. + let next_epoch = state.next_epoch()?; + let next_epoch_decision_root = state.proposer_shuffling_decision_root_at_epoch( + next_epoch, + head_block_root, + &beacon_chain.spec, + )?; + beacon_chain.with_proposer_cache( + next_epoch_decision_root, + next_epoch, + |_| Ok(()), + || Ok::<_, Error>((advanced_state_root, state.clone())), + )?; // Update the attester cache. let shuffling_id = @@ -438,7 +453,6 @@ fn advance_head(beacon_chain: &Arc>) -> Resu // even if we race with the deletion of this state by the finalization pruning code, the worst // case is we end up with a finalized state stored, that will get pruned the next time pruning // runs. - let advanced_state_root = state.update_tree_hash_cache()?; beacon_chain.store.put_state(&advanced_state_root, &state)?; debug!( From ffe1c82879d5bb9b4892284ccf4324cd47bb3158 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 30 Sep 2025 13:35:17 +1000 Subject: [PATCH 05/14] Start reverting the extra advance added in #8121 --- beacon_node/beacon_chain/src/beacon_chain.rs | 11 ++++++----- .../beacon_chain/src/beacon_proposer_cache.rs | 14 +++++--------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0e977bdd91b..ef87545a044 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -6624,11 +6624,12 @@ impl BeaconChain { } let proposers = state.get_beacon_proposer_indices(proposal_epoch, &self.spec)?; - Ok::<_, E>(EpochBlockProposers::new( - proposal_epoch, - state.fork(), - proposers, - )) + + // Use fork_at_epoch rather than the state's fork, because post-Fulu we may not have + // advanced the state completely into the new epoch. + let fork = self.spec.fork_at_epoch(proposal_epoch); + + Ok::<_, E>(EpochBlockProposers::new(proposal_epoch, fork, proposers)) })?; // Run the accessor function on the computed epoch proposers. diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index 06c0f0d21b5..6effce49f8b 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -203,7 +203,11 @@ pub fn compute_proposer_duties_from_head( .proposer_shuffling_decision_root_at_epoch(request_epoch, head_block_root, &chain.spec) .map_err(BeaconChainError::from)?; - Ok((indices, dependent_root, execution_status, state.fork())) + // Use fork_at_epoch rather than the state's fork, because post-Fulu we may not have advanced + // the state completely into the new epoch. + let fork = chain.spec.fork_at_epoch(request_epoch); + + Ok((indices, dependent_root, execution_status, fork)) } /// If required, advance `state` to the epoch required to determine proposer indices in `target_epoch`. @@ -235,14 +239,6 @@ pub fn ensure_state_can_determine_proposers_for_epoch( if state.current_epoch() > maximum_epoch { Err(BeaconStateError::SlotOutOfBounds.into()) } else if state.current_epoch() >= minimum_epoch { - if target_epoch > state.current_epoch() { - let target_slot = target_epoch.start_slot(E::slots_per_epoch()); - - // Advance the state into the same epoch as the block. Use the "partial" method since state - // roots are not important for proposer/attester shuffling. - partial_state_advance(state, Some(state_root), target_slot, spec) - .map_err(BeaconChainError::from)?; - } Ok(()) } else { // State's current epoch is less than the minimum epoch. From 5fcaeb2fbbe9a4c0c6466c68d92a4c9b8e1748e9 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 30 Sep 2025 18:04:15 +1000 Subject: [PATCH 06/14] Spicy test and fix fork_at_epoch (I think) --- beacon_node/beacon_chain/tests/store_tests.rs | 41 +++++++++++++++++++ consensus/types/src/chain_spec.rs | 14 +++++-- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 1c0fda16962..522c77b6398 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -13,6 +13,7 @@ use beacon_chain::test_utils::{ use beacon_chain::{ BeaconChain, BeaconChainError, BeaconChainTypes, BeaconSnapshot, BlockError, ChainConfig, NotifyExecutionLayer, ServerSentEventHandler, WhenSlotSkipped, + beacon_proposer_cache::compute_proposer_duties_from_head, data_availability_checker::MaybeAvailableBlock, historical_blocks::HistoricalBlockError, migrate::MigratorConfig, }; @@ -1516,6 +1517,46 @@ async fn proposer_shuffling_changing_with_lookahead() { ); } +#[tokio::test] +async fn proposer_duties_from_head_fulu() { + let spec = ForkName::Fulu.make_genesis_spec(E::default_spec()); + + let db_path = tempdir().unwrap(); + let store = get_store_generic(&db_path, Default::default(), spec.clone()); + let validators_keypairs = + types::test_utils::generate_deterministic_keypairs(LOW_VALIDATOR_COUNT); + let harness = TestHarness::builder(MinimalEthSpec) + .spec(spec.into()) + .keypairs(validators_keypairs) + .fresh_disk_store(store) + .mock_execution_layer() + .build(); + let spec = &harness.chain.spec; + + let initial_blocks = E::slots_per_epoch() * 3; + + // Build chain out to parent block. + let initial_slots: Vec = (1..=initial_blocks).map(Into::into).collect(); + let (state, state_root) = harness.get_current_state_and_root(); + let all_validators = harness.get_all_validators(); + let (_, _, head_block_root, head_state) = harness + .add_attested_blocks_at_slots(state, state_root, &initial_slots, &all_validators) + .await; + + // Compute the proposer duties at the next epoch from the head + let next_epoch = head_state.next_epoch().unwrap(); + let (_indices, dependent_root, _, fork) = + compute_proposer_duties_from_head(next_epoch, &harness.chain).unwrap(); + + assert_eq!( + dependent_root, + head_state + .proposer_shuffling_decision_root_at_epoch(next_epoch, head_block_root.into(), spec) + .unwrap() + ); + assert_eq!(fork, head_state.fork()); +} + // Ensure blocks from abandoned forks are pruned from the Hot DB #[tokio::test] async fn prunes_abandoned_fork_between_two_finalized_checkpoints() { diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 7916e9fcdb1..19000fc012f 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -476,15 +476,23 @@ impl ChainSpec { /// Returns a full `Fork` struct for a given epoch. pub fn fork_at_epoch(&self, epoch: Epoch) -> Fork { let current_fork_name = self.fork_name_at_epoch(epoch); - let previous_fork_name = current_fork_name.previous_fork().unwrap_or(ForkName::Base); - let epoch = self + + let fork_epoch = self .fork_epoch(current_fork_name) .unwrap_or_else(|| Epoch::new(0)); + // At genesis the Fork is initialised with two copies of the same value for both + // `previous_version` and `current_version` (see `initialize_beacon_state_from_eth1`). + let previous_fork_name = if fork_epoch == 0 { + current_fork_name + } else { + current_fork_name.previous_fork().unwrap_or(ForkName::Base) + }; + Fork { previous_version: self.fork_version_for_name(previous_fork_name), current_version: self.fork_version_for_name(current_fork_name), - epoch, + epoch: fork_epoch, } } From 3520fa44447ab8be9daf52257dfc0dbbf9d3a9a2 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 1 Oct 2025 16:40:56 +1000 Subject: [PATCH 07/14] Improve cache miss logging --- beacon_node/beacon_chain/src/beacon_chain.rs | 21 ++++++++++++------- .../beacon_chain/src/state_advance_timer.rs | 18 ++++++++++++++-- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ef87545a044..487fd6c5789 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4726,6 +4726,11 @@ impl BeaconChain { // efficient packing of execution blocks. Err(Error::SkipProposerPreparation) } else { + debug!( + ?shuffling_decision_root, + epoch = %proposal_epoch, + "Proposer shuffling cache miss for proposer prep" + ); let head = self.canonical_head.cached_head(); Ok(( head.head_state_root(), @@ -6564,7 +6569,9 @@ impl BeaconChain { /// - `accessor`: A closure to run against the proposers for the selected epoch. Usually this /// closure just grabs a single proposer, or takes the vec of proposers for the epoch. /// - `state_provider`: A closure to compute a state suitable for determining the shuffling. - /// This closure is evaluated lazily ONLY in the case + /// This closure is evaluated lazily ONLY in the case that a cache miss occurs. It is + /// recommended for code that wants to keep track of cache misses to produce a log and/or + /// increment a metric inside this closure . /// /// This function makes use of closures in order to efficiently handle concurrent accesses to /// the cache. @@ -6589,12 +6596,6 @@ impl BeaconChain { // If it is already initialised, then `get_or_try_init` will return immediately without // executing the initialisation code at all. let epoch_block_proposers = cache_entry.get_or_try_init(|| { - debug!( - ?shuffling_decision_block, - %proposal_epoch, - "Proposer shuffling cache miss" - ); - // Fetch the state on-demand if the required epoch was missing from the cache. // If the caller wants to not compute the state they must return an error here and then // catch it at the call site. @@ -6629,6 +6630,12 @@ impl BeaconChain { // advanced the state completely into the new epoch. let fork = self.spec.fork_at_epoch(proposal_epoch); + debug!( + ?shuffling_decision_block, + epoch = %proposal_epoch, + "Priming proposer shuffling cache" + ); + Ok::<_, E>(EpochBlockProposers::new(proposal_epoch, fork, proposers)) })?; diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index b19425a8bc8..eaa3d46d7ca 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -382,7 +382,14 @@ fn advance_head(beacon_chain: &Arc>) -> Resu current_epoch_decision_root, state.current_epoch(), |_| Ok(()), - || Ok::<_, Error>((advanced_state_root, state.clone())), + || { + debug!( + shuffling_decision_root = ?current_epoch_decision_root, + epoch = %state.current_epoch(), + "Computing current epoch proposer shuffling in state advance" + ); + Ok::<_, Error>((advanced_state_root, state.clone())) + }, )?; // For epochs *greater than* the Fulu fork epoch, we have also determined the proposer @@ -397,7 +404,14 @@ fn advance_head(beacon_chain: &Arc>) -> Resu next_epoch_decision_root, next_epoch, |_| Ok(()), - || Ok::<_, Error>((advanced_state_root, state.clone())), + || { + debug!( + shuffling_decision_root = ?next_epoch_decision_root, + epoch = %next_epoch, + "Computing next epoch proposer shuffling in state advance" + ); + Ok::<_, Error>((advanced_state_root, state.clone())) + }, )?; // Update the attester cache. From fc3a5e9ad4001e2df4ec54b19d4d4066c1b0b30d Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 1 Oct 2025 18:37:55 +1000 Subject: [PATCH 08/14] State advance more aggressively --- .../beacon_chain/src/state_advance_timer.rs | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index eaa3d46d7ca..b6caa39c48a 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -33,7 +33,7 @@ use types::{AttestationShufflingId, BeaconStateError, EthSpec, Hash256, Relative /// /// This avoids doing unnecessary work whilst the node is syncing or has perhaps been put to sleep /// for some period of time. -const MAX_ADVANCE_DISTANCE: u64 = 4; +const MAX_ADVANCE_DISTANCE: u64 = 256; /// Similarly for fork choice: avoid the fork choice lookahead during sync. /// @@ -295,25 +295,6 @@ fn advance_head(beacon_chain: &Arc>) -> Resu .get_advanced_hot_state(head_block_root, current_slot, head_block_state_root)? .ok_or(Error::HeadMissingFromSnapshotCache(head_block_root))?; - // Protect against advancing a state more than a single slot. - // - // Advancing more than one slot without storing the intermediate state would corrupt the - // database. Future works might store intermediate states inside this function. - match state.slot().cmp(&state.latest_block_header().slot) { - std::cmp::Ordering::Equal => (), - std::cmp::Ordering::Greater => { - return Err(Error::StateAlreadyAdvanced { - block_root: head_block_root, - }); - } - std::cmp::Ordering::Less => { - return Err(Error::BadStateSlot { - _block_slot: state.latest_block_header().slot, - _state_slot: state.slot(), - }); - } - } - let initial_slot = state.slot(); let initial_epoch = state.current_epoch(); From 59cbaba5b640500578b8332752d4f5ff10702856 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 1 Oct 2025 18:53:04 +1000 Subject: [PATCH 09/14] Remove unused errors --- .../beacon_chain/src/state_advance_timer.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/beacon_node/beacon_chain/src/state_advance_timer.rs b/beacon_node/beacon_chain/src/state_advance_timer.rs index b6caa39c48a..b10edf23369 100644 --- a/beacon_node/beacon_chain/src/state_advance_timer.rs +++ b/beacon_node/beacon_chain/src/state_advance_timer.rs @@ -49,17 +49,7 @@ enum Error { HeadMissingFromSnapshotCache(#[allow(dead_code)] Hash256), BeaconState(#[allow(dead_code)] BeaconStateError), Store(#[allow(dead_code)] store::Error), - MaxDistanceExceeded { - current_slot: Slot, - head_slot: Slot, - }, - StateAlreadyAdvanced { - block_root: Hash256, - }, - BadStateSlot { - _state_slot: Slot, - _block_slot: Slot, - }, + MaxDistanceExceeded { current_slot: Slot, head_slot: Slot }, } impl From for Error { @@ -180,9 +170,6 @@ async fn state_advance_timer( error = ?e, "Failed to advance head state" ), - Err(Error::StateAlreadyAdvanced { block_root }) => { - debug!(?block_root, "State already advanced on slot") - } Err(Error::MaxDistanceExceeded { current_slot, head_slot, From a4ab920ff50c30f1cca4c51edd58760cfc869865 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 2 Oct 2025 13:04:13 +1000 Subject: [PATCH 10/14] Add more instrumentation for op pool --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 487fd6c5789..7b556b84676 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5238,16 +5238,20 @@ impl BeaconChain { None }; + let slashings_and_exits_span = debug_span!("get_slashings_and_exits").entered(); let (mut proposer_slashings, mut attester_slashings, mut voluntary_exits) = self.op_pool.get_slashings_and_exits(&state, &self.spec); + drop(slashings_and_exits_span); let eth1_data = state.eth1_data().clone(); let deposits = vec![]; + let bls_changes_span = debug_span!("get_bls_to_execution_changes").entered(); let bls_to_execution_changes = self .op_pool .get_bls_to_execution_changes(&state, &self.spec); + drop(bls_changes_span); // Iterate through the naive aggregation pool and ensure all the attestations from there // are included in the operation pool. From 97da066f9199978f9fe878aad044db4469af6923 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 13 Oct 2025 12:58:37 +1100 Subject: [PATCH 11/14] Add regression test for tolerant current epoch case --- .../http_api/tests/interactive_tests.rs | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 1398d8c72fe..9732a62a2f0 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -940,3 +940,106 @@ async fn queue_attestations_from_http() { attestation_future.await.unwrap(); } + +// Test that a request for next epoch proposer duties suceeds when the current slot clock is within +// gossip clock disparity (500ms) of the new epoch. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn proposer_duties_with_gossip_tolerance() { + let validator_count = 24; + let spec = ForkName::Fulu.make_genesis_spec(E::default_spec()); + + let tester = InteractiveTester::::new(Some(spec.clone()), validator_count).await; + let harness = &tester.harness; + let client = &tester.client; + + let num_initial = 4 * E::slots_per_epoch() - 1; + let next_epoch_start_slot = Slot::new(num_initial + 1); + + harness.advance_slot(); + harness + .extend_chain_with_sync( + num_initial as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + SyncCommitteeStrategy::NoValidators, + LightClientStrategy::Disabled, + ) + .await; + + assert_eq!(harness.chain.slot().unwrap(), num_initial); + + // Set the clock to just before the next epoch. + harness.chain.slot_clock.advance_time( + Duration::from_secs(spec.seconds_per_slot) - spec.maximum_gossip_clock_disparity(), + ); + assert_eq!( + harness + .chain + .slot_clock + .now_with_future_tolerance(spec.maximum_gossip_clock_disparity()) + .unwrap(), + next_epoch_start_slot + ); + + let head_state = harness.get_current_state(); + let head_block_root = harness.head_block_root(); + let tolerant_current_epoch = next_epoch_start_slot.epoch(E::slots_per_epoch()); + + // This is a regression test for the bug described here: + // https://github.com/sigp/lighthouse/pull/8130/files#r2386594566 + // + // To trigger it, we need to prime the proposer shuffling cache with an incorrect entry which + // the previous code would be liable to lookup due to the bugs in its decision root calculation. + let wrong_decision_root = head_state + .proposer_shuffling_decision_root(head_block_root, &spec) + .unwrap(); + let wrong_proposer_indices = vec![0; E::slots_per_epoch() as usize]; + harness + .chain + .beacon_proposer_cache + .lock() + .insert( + tolerant_current_epoch, + wrong_decision_root, + wrong_proposer_indices.clone(), + head_state.fork(), + ) + .unwrap(); + + // Request the proposer duties. + let proposer_duties_tolerant_current_epoch = client + .get_validator_duties_proposer(tolerant_current_epoch) + .await + .unwrap(); + + assert_eq!( + proposer_duties_tolerant_current_epoch.dependent_root, + head_state + .proposer_shuffling_decision_root_at_epoch(tolerant_current_epoch, Hash256::ZERO, &spec) + .unwrap() + ); + assert_ne!( + proposer_duties_tolerant_current_epoch + .data + .iter() + .map(|data| data.validator_index as usize) + .collect::>(), + wrong_proposer_indices, + ); + + // We should get the exact same result after properly advancing into the epoch. + harness + .chain + .slot_clock + .advance_time(spec.maximum_gossip_clock_disparity()); + assert_eq!(harness.chain.slot().unwrap(), next_epoch_start_slot); + let proposer_duties_current_epoch = client + .get_validator_duties_proposer(tolerant_current_epoch) + .await + .unwrap(); + + assert_eq!( + proposer_duties_tolerant_current_epoch, + proposer_duties_current_epoch + ); +} From 3171c124406775118e4f5bf323f5b82e56237b32 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 13 Oct 2025 13:31:06 +1100 Subject: [PATCH 12/14] Add some tests at Gloas fork epoch --- beacon_node/beacon_chain/tests/store_tests.rs | 81 ++++++++++++++++++- consensus/types/src/chain_spec.rs | 4 +- 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 522c77b6398..f7d426bd39e 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -13,8 +13,11 @@ use beacon_chain::test_utils::{ use beacon_chain::{ BeaconChain, BeaconChainError, BeaconChainTypes, BeaconSnapshot, BlockError, ChainConfig, NotifyExecutionLayer, ServerSentEventHandler, WhenSlotSkipped, - beacon_proposer_cache::compute_proposer_duties_from_head, - data_availability_checker::MaybeAvailableBlock, historical_blocks::HistoricalBlockError, + beacon_proposer_cache::{ + compute_proposer_duties_from_head, ensure_state_can_determine_proposers_for_epoch, + }, + data_availability_checker::MaybeAvailableBlock, + historical_blocks::HistoricalBlockError, migrate::MigratorConfig, }; use logging::create_test_tracing_subscriber; @@ -1557,6 +1560,80 @@ async fn proposer_duties_from_head_fulu() { assert_eq!(fork, head_state.fork()); } +/// Test that we can compute the proposer shuffling for the Gloas fork epoch itself using lookahead! +#[tokio::test] +async fn proposer_lookahead_gloas_fork_epoch() { + let gloas_fork_epoch = Epoch::new(4); + let mut spec = ForkName::Fulu.make_genesis_spec(E::default_spec()); + spec.gloas_fork_epoch = Some(gloas_fork_epoch); + + let db_path = tempdir().unwrap(); + let store = get_store_generic(&db_path, Default::default(), spec.clone()); + let validators_keypairs = + types::test_utils::generate_deterministic_keypairs(LOW_VALIDATOR_COUNT); + let harness = TestHarness::builder(MinimalEthSpec) + .spec(spec.into()) + .keypairs(validators_keypairs) + .fresh_disk_store(store) + .mock_execution_layer() + .build(); + let spec = &harness.chain.spec; + + let initial_blocks = (gloas_fork_epoch - 1) + .start_slot(E::slots_per_epoch()) + .as_u64(); + + // Build chain out to parent block. + let initial_slots: Vec = (1..=initial_blocks).map(Into::into).collect(); + let (state, state_root) = harness.get_current_state_and_root(); + let all_validators = harness.get_all_validators(); + let (_, _, head_block_root, mut head_state) = harness + .add_attested_blocks_at_slots(state, state_root, &initial_slots, &all_validators) + .await; + let head_state_root = head_state.canonical_root().unwrap(); + + // Check that we have access to the next epoch shuffling according to + // `ensure_state_can_determine_proposers_for_epoch`. + ensure_state_can_determine_proposers_for_epoch( + &mut head_state, + head_state_root, + gloas_fork_epoch, + &spec, + ) + .unwrap(); + assert_eq!(head_state.current_epoch(), gloas_fork_epoch - 1); + + // Compute the proposer duties at the fork epoch from the head. + let (indices, dependent_root, _, fork) = + compute_proposer_duties_from_head(gloas_fork_epoch, &harness.chain).unwrap(); + + assert_eq!( + dependent_root, + head_state + .proposer_shuffling_decision_root_at_epoch( + gloas_fork_epoch, + head_block_root.into(), + spec + ) + .unwrap() + ); + assert_ne!(fork, head_state.fork()); + assert_eq!(fork, spec.fork_at_epoch(gloas_fork_epoch)); + + // Build a block in the Gloas fork epoch and assert that the shuffling does not change. + let gloas_slots = vec![gloas_fork_epoch.start_slot(E::slots_per_epoch())]; + let (_, _, gloas_head_block_root, mut gloas_head_state) = harness + .add_attested_blocks_at_slots(head_state, head_state_root, &gloas_slots, &all_validators) + .await; + + let (no_lookahead_indices, no_lookahead_dependent_root, _, no_lookahead_fork) = + compute_proposer_duties_from_head(gloas_fork_epoch, &harness.chain).unwrap(); + + assert_eq!(no_lookahead_indices, indices); + assert_eq!(no_lookahead_dependent_root, dependent_root); + assert_eq!(no_lookahead_fork, fork); +} + // Ensure blocks from abandoned forks are pruned from the Hot DB #[tokio::test] async fn prunes_abandoned_fork_between_two_finalized_checkpoints() { diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 08b5f8630b9..421655777e7 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -3018,9 +3018,11 @@ mod yaml_tests { fn proposer_shuffling_decision_root_around_epoch_boundary() { type E = MainnetEthSpec; let fulu_fork_epoch = 5; + let gloas_fork_epoch = 10; let spec = { let mut spec = ForkName::Electra.make_genesis_spec(E::default_spec()); spec.fulu_fork_epoch = Some(Epoch::new(fulu_fork_epoch)); + spec.gloas_fork_epoch = Some(Epoch::new(gloas_fork_epoch)); Arc::new(spec) }; @@ -3034,7 +3036,7 @@ mod yaml_tests { } // For epochs after Fulu, the decision slot is the end of the epoch two epochs prior. - for epoch in ((fulu_fork_epoch + 1)..(fulu_fork_epoch + 10)).map(Epoch::new) { + for epoch in ((fulu_fork_epoch + 1)..=(gloas_fork_epoch + 1)).map(Epoch::new) { assert_eq!( spec.proposer_shuffling_decision_slot::(epoch), (epoch - 1).start_slot(E::slots_per_epoch()) - 1 From ff5edd08c50d7db3ced659cfe13753baf2d36f36 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 13 Oct 2025 13:40:57 +1100 Subject: [PATCH 13/14] Fix lints --- beacon_node/beacon_chain/tests/store_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index f7d426bd39e..b367b6d4374 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -1598,7 +1598,7 @@ async fn proposer_lookahead_gloas_fork_epoch() { &mut head_state, head_state_root, gloas_fork_epoch, - &spec, + spec, ) .unwrap(); assert_eq!(head_state.current_epoch(), gloas_fork_epoch - 1); @@ -1622,7 +1622,7 @@ async fn proposer_lookahead_gloas_fork_epoch() { // Build a block in the Gloas fork epoch and assert that the shuffling does not change. let gloas_slots = vec![gloas_fork_epoch.start_slot(E::slots_per_epoch())]; - let (_, _, gloas_head_block_root, mut gloas_head_state) = harness + let (_, _, _, _) = harness .add_attested_blocks_at_slots(head_state, head_state_root, &gloas_slots, &all_validators) .await; From a263157e25ae2f178f607ce540705eba0fab6add Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 20 Oct 2025 10:19:08 +1100 Subject: [PATCH 14/14] Improve tests --- beacon_node/beacon_chain/tests/store_tests.rs | 2 +- beacon_node/http_api/tests/interactive_tests.rs | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index b367b6d4374..f14b87a1b2b 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -1571,7 +1571,7 @@ async fn proposer_lookahead_gloas_fork_epoch() { let store = get_store_generic(&db_path, Default::default(), spec.clone()); let validators_keypairs = types::test_utils::generate_deterministic_keypairs(LOW_VALIDATOR_COUNT); - let harness = TestHarness::builder(MinimalEthSpec) + let harness = TestHarness::builder(E::default()) .spec(spec.into()) .keypairs(validators_keypairs) .fresh_disk_store(store) diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 9732a62a2f0..94b773c32da 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -946,10 +946,10 @@ async fn queue_attestations_from_http() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn proposer_duties_with_gossip_tolerance() { let validator_count = 24; - let spec = ForkName::Fulu.make_genesis_spec(E::default_spec()); - let tester = InteractiveTester::::new(Some(spec.clone()), validator_count).await; + let tester = InteractiveTester::::new(None, validator_count).await; let harness = &tester.harness; + let spec = &harness.spec; let client = &tester.client; let num_initial = 4 * E::slots_per_epoch() - 1; @@ -991,7 +991,7 @@ async fn proposer_duties_with_gossip_tolerance() { // To trigger it, we need to prime the proposer shuffling cache with an incorrect entry which // the previous code would be liable to lookup due to the bugs in its decision root calculation. let wrong_decision_root = head_state - .proposer_shuffling_decision_root(head_block_root, &spec) + .proposer_shuffling_decision_root(head_block_root, spec) .unwrap(); let wrong_proposer_indices = vec![0; E::slots_per_epoch() as usize]; harness @@ -1015,7 +1015,11 @@ async fn proposer_duties_with_gossip_tolerance() { assert_eq!( proposer_duties_tolerant_current_epoch.dependent_root, head_state - .proposer_shuffling_decision_root_at_epoch(tolerant_current_epoch, Hash256::ZERO, &spec) + .proposer_shuffling_decision_root_at_epoch( + tolerant_current_epoch, + head_block_root, + spec + ) .unwrap() ); assert_ne!(