From dbde4bf53a1b0db47cff84aa52d2a0e15e0d628a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Jun 2022 15:53:32 +1000 Subject: [PATCH] Expose fork choice locks --- .../src/attestation_verification.rs | 6 +- beacon_node/beacon_chain/src/beacon_chain.rs | 67 ++-- .../beacon_chain/src/beacon_proposer_cache.rs | 1 + .../beacon_chain/src/block_verification.rs | 41 ++- .../beacon_chain/src/canonical_head.rs | 325 ++---------------- .../beacon_chain/src/execution_payload.rs | 16 +- beacon_node/http_api/src/lib.rs | 12 +- testing/ef_tests/src/cases/fork_choice.rs | 14 +- 8 files changed, 133 insertions(+), 349 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index ce4a9b442c4..63af6ab9e11 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -977,6 +977,7 @@ fn verify_head_block_is_known( ) -> Result { let block_opt = chain .canonical_head + .fork_choice_read_lock() .get_block(&attestation.data.beacon_block_root) .or_else(|| { chain @@ -1244,7 +1245,10 @@ where // processing an attestation that does not include our latest finalized block in its chain. // // We do not delay consideration for later, we simply drop the attestation. - if !chain.canonical_head.contains_block(&target.root) + if !chain + .canonical_head + .fork_choice_read_lock() + .contains_block(&target.root) && !chain.early_attester_cache.contains_block(target.root) { return Err(Error::UnknownTargetRoot(target.root)); diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0120eca75e4..39c232982a6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1348,6 +1348,7 @@ impl BeaconChain { let execution_status = self .canonical_head + .fork_choice_read_lock() .get_block_execution_status(&head_block_root) .ok_or(Error::AttestationHeadNotInForkChoice(head_block_root))?; @@ -1400,6 +1401,7 @@ impl BeaconChain { let beacon_block_root = attestation.data.beacon_block_root; match self .canonical_head + .fork_choice_read_lock() .get_block_execution_status(&beacon_block_root) { // The attestation references a block that is not in fork choice, it must be @@ -1566,6 +1568,7 @@ impl BeaconChain { // Only attest to a block if it is fully verified (i.e. not optimistic or invalid). match self .canonical_head + .fork_choice_read_lock() .get_block_execution_status(&beacon_block_root) { Some(execution_status) if execution_status.is_valid_or_irrelevant() => (), @@ -1770,6 +1773,7 @@ impl BeaconChain { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_PROCESS_ATTESTATION_TIMES); self.canonical_head + .fork_choice_write_lock() .on_attestation( self.slot()?, verified.indexed_attestation(), @@ -2009,9 +2013,14 @@ impl BeaconChain { // pivot block is the same as the current state's pivot block. If it is, then the // attestation's shuffling is the same as the current state's. // To account for skipped slots, find the first block at *or before* the pivot slot. - let pivot_block_root = self - .canonical_head - .get_ancestor_at_or_below_slot(block_root, pivot_slot); + let fork_choice_lock = self.canonical_head.fork_choice_read_lock(); + let pivot_block_root = fork_choice_lock + .proto_array() + .core_proto_array() + .iter_block_roots(block_root) + .find(|(_, slot)| *slot <= pivot_slot) + .map(|(block_root, _)| block_root); + drop(fork_choice_lock); match pivot_block_root { Some(root) => root == state_pivot_block_root, @@ -2624,6 +2633,7 @@ impl BeaconChain { // Load the parent proto block for later use. let parent_proto_block = self .canonical_head + .fork_choice_read_lock() .get_block(&signed_block.parent_root()) .ok_or_else(|| BlockError::ParentUnknown(signed_block.clone()))?; @@ -2773,9 +2783,7 @@ impl BeaconChain { // // It is important to avoid interleaving this write-lock with any other locks, *especially* // the `canonical_head.cached_head` lock. - let mut fork_choice_write_lock = self - .canonical_head - .block_processing_fork_choice_write_lock(); + let mut fork_choice_write_lock = self.canonical_head.fork_choice_write_lock(); // Register the new block with the fork choice service. { @@ -2800,17 +2808,19 @@ impl BeaconChain { } // Register each indexed attestation in the block with the fork choice service. - match fork_choice_write_lock.on_attestations( - current_slot, - &indexed_attestations, - AttestationFromBlock::True, - ) { - Ok(()) => Ok(()), - // Ignore invalid attestations whilst importing attestations from a block. The - // block might be very old and therefore the attestations useless to fork choice. - Err(ForkChoiceError::InvalidAttestation(_)) => Ok(()), - Err(e) => Err(BlockError::BeaconChainError(e.into())), - }?; + for indexed_attestation in indexed_attestations { + match fork_choice_write_lock.on_attestation( + current_slot, + &indexed_attestation, + AttestationFromBlock::True, + ) { + Ok(()) => Ok(()), + // Ignore invalid attestations whilst importing attestations from a block. The + // block might be very old and therefore the attestations useless to fork choice. + Err(ForkChoiceError::InvalidAttestation(_)) => Ok(()), + Err(e) => Err(BlockError::BeaconChainError(e.into())), + }?; + } // If the block is recent enough and it was not optimistically imported, check to see if it // becomes the head block. If so, apply it to the early attester cache. This will allow @@ -3555,7 +3565,12 @@ impl BeaconChain { let inner_op = op.clone(); let fork_choice_result = self .spawn_blocking_handle( - move || chain.canonical_head.on_invalid_execution_payload(&inner_op), + move || { + chain + .canonical_head + .fork_choice_write_lock() + .on_invalid_execution_payload(&inner_op) + }, "invalid_payload_fork_choice_update", ) .await?; @@ -3591,7 +3606,12 @@ impl BeaconChain { let chain = self.clone(); let justified_block = self .spawn_blocking_handle( - move || chain.canonical_head.get_justified_block(), + move || { + chain + .canonical_head + .fork_choice_read_lock() + .get_justified_block() + }, "invalid_payload_fork_choice_get_justified", ) .await??; @@ -3627,7 +3647,9 @@ impl BeaconChain { } pub fn block_is_known_to_fork_choice(&self, root: &Hash256) -> bool { - self.canonical_head.contains_block(root) + self.canonical_head + .fork_choice_read_lock() + .contains_block(root) } /// Determines the beacon proposer for the next slot. If that proposer is registered in the @@ -3978,6 +4000,7 @@ impl BeaconChain { move || { chain .canonical_head + .fork_choice_write_lock() .on_valid_execution_payload(head_block_root) }, "update_execution_engine_invalid_payload", @@ -4074,6 +4097,7 @@ impl BeaconChain { Ok(false) } else { self.canonical_head + .fork_choice_read_lock() .is_optimistic_block(&block.canonical_root()) .map_err(BeaconChainError::ForkChoiceError) } @@ -4099,6 +4123,7 @@ impl BeaconChain { Ok(false) } else { self.canonical_head + .fork_choice_read_lock() .is_optimistic_block_no_fallback(&head_block.canonical_root()) .map_err(BeaconChainError::ForkChoiceError) } @@ -4128,6 +4153,7 @@ impl BeaconChain { Ok(false) } else { self.canonical_head + .fork_choice_read_lock() .is_optimistic_block_no_fallback(block_root) .map_err(BeaconChainError::ForkChoiceError) } @@ -4269,6 +4295,7 @@ impl BeaconChain { { let head_block = self .canonical_head + .fork_choice_read_lock() .get_block(&head_block_root) .ok_or(Error::MissingBeaconBlock(head_block_root))?; diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index a4c9621bf3f..346896883cc 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -154,6 +154,7 @@ pub fn compute_proposer_duties_from_head( let execution_status = chain .canonical_head + .fork_choice_read_lock() .get_block_execution_status(&head_block_root) .ok_or(BeaconChainError::HeadMissingFromForkChoice(head_block_root))?; diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index de6e354de5f..4c1e017a3eb 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -672,7 +672,11 @@ impl GossipVerifiedBlock { // reboot if the `observed_block_producers` cache is empty. In that case, without this // check, we will load the parent and state from disk only to find out later that we // already know this block. - if chain.canonical_head.contains_block(&block_root) { + if chain + .canonical_head + .fork_choice_read_lock() + .contains_block(&block_root) + { return Err(BlockError::BlockIsAlreadyKnown); } @@ -1019,7 +1023,11 @@ impl ExecutionPendingBlock { parent: PreProcessingSnapshot, chain: &Arc>, ) -> Result> { - if let Some(parent) = chain.canonical_head.get_block(&block.parent_root()) { + if let Some(parent) = chain + .canonical_head + .fork_choice_read_lock() + .get_block(&block.parent_root()) + { // Reject any block where the parent has an invalid payload. It's impossible for a valid // block to descend from an invalid parent. if parent.execution_status.is_invalid() { @@ -1200,12 +1208,15 @@ impl ExecutionPendingBlock { let is_optimistic_candidate = chain .spawn_blocking_handle( move || { - inner_chain.canonical_head.is_optimistic_candidate_block( - current_slot, - block_slot, - &block_parent_root, - &inner_chain.spec, - ) + inner_chain + .canonical_head + .fork_choice_read_lock() + .is_optimistic_candidate_block( + current_slot, + block_slot, + &block_parent_root, + &inner_chain.spec, + ) }, "validate_merge_block_optimistic_candidate", ) @@ -1446,6 +1457,7 @@ pub fn check_block_is_finalized_descendant( ) -> Result<(), BlockError> { if chain .canonical_head + .fork_choice_read_lock() .is_descendant_of_finalized(block.parent_root()) { Ok(()) @@ -1512,7 +1524,11 @@ pub fn check_block_relevancy( // Check if the block is already known. We know it is post-finalization, so it is // sufficient to check the fork choice. - if chain.canonical_head.contains_block(&block_root) { + if chain + .canonical_head + .fork_choice_read_lock() + .contains_block(&block_root) + { return Err(BlockError::BlockIsAlreadyKnown); } @@ -1541,6 +1557,7 @@ fn verify_parent_block_is_known( ) -> Result<(ProtoBlock, Arc>), BlockError> { if let Some(proto_block) = chain .canonical_head + .fork_choice_read_lock() .get_block(&block.message().parent_root()) { Ok((proto_block, block)) @@ -1576,7 +1593,11 @@ fn load_parent( // because it will revert finalization. Note that the finalized block is stored in fork // choice, so we will not reject any child of the finalized block (this is relevant during // genesis). - if !chain.canonical_head.contains_block(&block.parent_root()) { + if !chain + .canonical_head + .fork_choice_read_lock() + .contains_block(&block.parent_root()) + { return Err(BlockError::ParentUnknown(block)); } diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index 7edd71d9cdc..4499e33b08d 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -1,64 +1,35 @@ //! This module provides all functionality for finding the canonical head, updating all necessary //! components see (e.g. caches) and also maintaining a cached head block and state. //! -//! ## Usage +//! For practically all applications, the "canonical head" can be read using +//! `beacon_chain.canonical_head.cached_head()`. //! -//! This module primarily provides the following: +//! The canonical head can be updated using `beacon_chain.recompute_head()`. //! -//! ### The `CanonicalHead` struct. +//! ## Deadlock safety //! -//! Use this to access values from fork choice or from the `cached_head`, which is a cached block -//! and state from the last time fork choice was run. -//! -//! ### The `BeaconChain::recompute_head` method. -//! -//! This method was formally known as `BeaconChain::fork_choice`. It runs the fork choice -//! algorithm and then enshrines the result as the "canonical head". This involves updating the -//! `cached_head` so we always have the head block and state on hand. It also involves pruning -//! caches, sending SSE events, pruning the database and other things. -//! -//! ## The Three Rules -//! -//! There are three locks managed by this function: +//! This module contains three locks: //! //! 1. `RwLock`: Contains `proto_array` fork choice. //! 2. `RwLock`: Contains a cached block/state from the last run of `proto_array`. //! 3. `Mutex<()>`: Is used to prevent concurrent execution of `BeaconChain::recompute_head`. //! -//! The code in this module is designed specifically to prevent dead-locks through improper use of -//! these locks. There are three primary "rules" which, if followed, will prevent *other modules* from -//! causing dead-locks. The rules are: -//! -//! Rule #1: Never expose a *read or write* lock for `RwLock` outside this module. -//! Rule #2: Functions external to this module may hold a *read lock* for `RwLock` -//! (never a write-lock). -//! Rule #3: Never expose a read or write lock for `Mutex<()>` outside this module. -//! -//! Since users can only access a `RwLock` outside this function, they cannot interleave -//! the other two locks and cause a deadlock. Whilst we maintain the three rules, external functions -//! are dead-lock safe. Unfortunately, this module has no such guarantees, proceed with extreme -//! caution when managing locks in this module. -//! -//! The downside of Rule #1 is that we must re-expose every function on `BeaconForkChoice` on the -//! `CanonicalHead`. This prevents users from being able to hold and interleave the fork choice lock -//! with the canonical head lock. It's annoying when working on this file, but hopefully the -//! long-term safety benefits will pay off. +//! This module has to take great efforts to avoid causing a deadlock with these three methods. Any +//! developers working in this module should tread carefully and seek a detailed review. //! -//! Like all good rules, we have some exceptions. The first violates rule #1 via exposing the -//! `BlockProcessingForkChoiceWriteLock`. This exposes a write-lock on the `BeaconForkChoice` for -//! use during block processing. We *need* an exclusive lock here so we block access to fork choice -//! until we've written to the database; this helps prevent corruption. This struct is *clearly* -//! labelled for use only with block processing and it has a limited set of functionality to give -//! this module control over what happens with it. +//! To encourage safe use of this module, it should **only ever return a read or write lock for the +//! fork choice lock (lock 1)**. Whilst public functions might utilise locks (2) and (3), the +//! fundamental `RwLockWriteGuard` or `RwLockReadGuard` should never be exposed. This prevents +//! external functions from acquiring these locks in conflicting orders and causing a deadlock. //! //! ## Design Considerations //! //! We separate the `BeaconForkChoice` and `CachedHead` into two `RwLocks` because we want to ensure //! fast access to the `CachedHead`. If we were to put them both under the same lock, we would need //! to take an exclusive write-lock on it in order to run `ForkChoice::get_head`. This can take tens -//! of milliseconds and would block all downstream functions that want to know the head block root. -//! This is unacceptable for fast-responding functions like the networking stack. Believe me, I have -//! tried to put them under the same lock and it did not work well :( +//! of milliseconds and would block all downstream functions that want to know simple things like +//! the head block root. This is unacceptable for fast-responding functions like the networking +//! stack. use crate::persisted_fork_choice::PersistedForkChoice; use crate::{ @@ -69,13 +40,10 @@ use crate::{ events::ServerSentEventHandler, metrics, validator_monitor::{get_slot_delay_ms, timestamp_now}, - BeaconChain, BeaconChainError as Error, BeaconChainTypes, BeaconSnapshot, ForkChoiceError, + BeaconChain, BeaconChainError as Error, BeaconChainTypes, BeaconSnapshot, }; use eth2::types::{EventKind, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead}; -use fork_choice::{ - AttestationFromBlock, ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, - InvalidationOperation, PayloadVerificationStatus, ProtoBlock, -}; +use fork_choice::{ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock}; use itertools::process_results; use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard}; use slog::{crit, debug, error, warn, Logger}; @@ -222,76 +190,6 @@ impl CachedHead { } } -/// This struct provides a write-lock on the `BeaconForkChoice` is is **only for use during block -/// processing**. -/// -/// It provides a limited set of functionality that is required for processing blocks and -/// maintaining consistency between the database and fork choice. -pub struct BlockProcessingForkChoiceWriteLock<'a, T: BeaconChainTypes> { - fork_choice: RwLockWriteGuard<'a, BeaconForkChoice>, -} - -impl<'a, T: BeaconChainTypes> BlockProcessingForkChoiceWriteLock<'a, T> { - /// Get a `ProtoBlock` from proto array. Contains a limited, but useful set of information about - /// the block. - pub fn get_block(&self, block_root: &Hash256) -> Option { - self.fork_choice.get_block(block_root) - } - - /// Apply a block to fork choice. - #[allow(clippy::too_many_arguments)] - pub fn on_block>( - &mut self, - current_slot: Slot, - block: BeaconBlockRef, - block_root: Hash256, - block_delay: Duration, - state: &BeaconState, - payload_verification_status: PayloadVerificationStatus, - spec: &ChainSpec, - ) -> Result<(), ForkChoiceError> { - self.fork_choice.on_block( - current_slot, - block, - block_root, - block_delay, - state, - payload_verification_status, - spec, - ) - } - - /// Apply some attestations to fork choice. - pub fn on_attestations( - &mut self, - current_slot: Slot, - attestations: &[IndexedAttestation], - is_from_block: AttestationFromBlock, - ) -> Result<(), ForkChoiceError> { - for indexed_attestation in attestations { - self.fork_choice - .on_attestation(current_slot, indexed_attestation, is_from_block)? - } - Ok(()) - } - - /// Recompute the head of the beacon chain. - /// - /// ## Note - /// - /// Exposing this function means that the `canonical_head.fork_choice` can get ahead of - /// `canonical_head.cached_head`! We deem this to be OK, it's required for use to achieve the - /// `early_attester_cache` and we ensure that we make minimal assumptions about fork choice - /// being at the same place as the `cached_head`. - pub fn get_head( - &mut self, - current_slot: Slot, - spec: &ChainSpec, - ) -> Result { - self.fork_choice.get_head(current_slot, spec) - } -} - /// Represents the "canonical head" of the beacon chain. /// /// The `cached_head` is elected by the `fork_choice` algorithm contained in this struct. @@ -345,15 +243,15 @@ impl CanonicalHead { /// save-point. pub(crate) fn restore_from_store( &self, - // We don't actually *need* the block processing guard, but we pass it because the only - // place that calls this function is block processing and we'll get a deadlock if it isn't - // dropped before this function runs. - block_processing_guard: BlockProcessingForkChoiceWriteLock, + // We don't actually need this value, however it's always present when we call this function + // and it needs to be dropped to prevent a dead-lock. Requiring it to be passed here is + // defensive programming. + fork_choice_write_lock: RwLockWriteGuard>, store: &BeaconStore, spec: &ChainSpec, ) -> Result<(), Error> { - // Failing to drop this will result in a dead-lock. - drop(block_processing_guard); + // Drop the lock to prevent a deadlock later in the function. + drop(fork_choice_write_lock); let fork_choice = >::load_fork_choice(store.clone(), spec)? .ok_or(Error::MissingPersistedForkChoice)?; @@ -389,25 +287,13 @@ impl CanonicalHead { Ok(()) } - /// Only for use in block processing. Do not use this function unless you are *certain* you know - /// what you are doing. - /// - /// See `BlockProcessingForkChoiceWriteLock` for more detail. - pub(crate) fn block_processing_fork_choice_write_lock( - &self, - ) -> BlockProcessingForkChoiceWriteLock { - BlockProcessingForkChoiceWriteLock { - fork_choice: self.fork_choice_write_lock(), - } - } - /// Returns the execution status of the block at the head of the beacon chain. /// /// This will only return `Err` in the scenario where `self.fork_choice` has advanced /// significantly past the cached `head_snapshot`. In such a scenario is it likely prudent to /// run `BeaconChain::recompute_head` to update the cached values. pub fn head_execution_status(&self) -> Result { - let head_block_root = self.cached_head.read().head_block_root(); + let head_block_root = self.cached_head().head_block_root(); self.fork_choice .read() .get_block_execution_status(&head_block_root) @@ -426,178 +312,21 @@ impl CanonicalHead { /// Access a write-lock for the cached head. /// - /// This function **must not be made public**. (See "Rule #2") + /// This function is **not safe** to be public. See the module-level documentation for more + /// information about protecting from deadlocks. fn cached_head_write_lock(&self) -> RwLockWriteGuard> { self.cached_head.write() } /// Access a read-lock for fork choice. - /// - /// This function **must not be made public**. (See "Rule #1") - fn fork_choice_read_lock(&self) -> RwLockReadGuard> { + pub fn fork_choice_read_lock(&self) -> RwLockReadGuard> { self.fork_choice.read() } - /// Access a read-lock for fork choice. - /// - /// This function **must only be used in testing**. (See "Rule #1") - pub fn fork_choice_read_lock_testing_only(&self) -> RwLockReadGuard> { - self.fork_choice_read_lock() - } - /// Access a write-lock for fork choice. - /// - /// This function **must not be made public**. (See "Rule #1") - fn fork_choice_write_lock(&self) -> RwLockWriteGuard> { + pub fn fork_choice_write_lock(&self) -> RwLockWriteGuard> { self.fork_choice.write() } - - /// Access a write-lock for fork choice. - /// - /// This function **must only be used in testing**. (See "Rule #1") - pub fn fork_choice_write_lock_testing_only(&self) -> RwLockWriteGuard> { - self.fork_choice_write_lock() - } - - /// Update fork choice to inform it about a valid execution payload. - /// - /// Mutates fork choice. - pub fn on_valid_execution_payload(&self, block_root: Hash256) -> Result<(), ForkChoiceError> { - self.fork_choice_write_lock() - .on_valid_execution_payload(block_root) - } - - /// Update fork choice to inform it about an invalid execution payload. - /// - /// Mutates fork choice. - pub fn on_invalid_execution_payload( - &self, - op: &InvalidationOperation, - ) -> Result<(), ForkChoiceError> { - self.fork_choice_write_lock() - .on_invalid_execution_payload(op) - } - - /// Returns `true` if fork choice is aware of a block with `block_root`. - /// - /// Does not mutate fork choice. - pub fn contains_block(&self, block_root: &Hash256) -> bool { - self.fork_choice_read_lock().contains_block(block_root) - } - - /// Returns the `ProtoBlock` identified by `block_root`, if known to fork choice. - /// - /// Does not mutate fork choice. - pub fn get_block(&self, block_root: &Hash256) -> Option { - self.fork_choice_read_lock().get_block(block_root) - } - - /// Returns the `ExecutionStatus` for the `block_root`, if known to fork choice. - /// - /// Does not mutate fork choice. - pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { - self.fork_choice_read_lock() - .get_block_execution_status(block_root) - } - - /// Returns the `ProtoBlock` of the justified block. - /// - /// This *may not* be the same block as `self.cached_head.justified_checkpoint`, since it uses - /// fork choice and it might be ahead of the cached head. - /// - /// Does not mutate fork choice. - pub fn get_justified_block(&self) -> Result { - self.fork_choice_read_lock().get_justified_block() - } - - /// Returns `true` if some block with the given parameters is safe to be imported - /// optimistically. - /// - /// Does not mutate fork choice. - pub fn is_optimistic_candidate_block( - &self, - current_slot: Slot, - block_slot: Slot, - block_parent_root: &Hash256, - spec: &ChainSpec, - ) -> Result { - self.fork_choice_read_lock().is_optimistic_candidate_block( - current_slot, - block_slot, - block_parent_root, - spec, - ) - } - - /// See `ForkChoice::is_optimistic_block` for documentation. - /// - /// Does not mutate fork choice. - pub fn is_optimistic_block(&self, block_root: &Hash256) -> Result { - self.fork_choice_read_lock().is_optimistic_block(block_root) - } - - /// See `ForkChoice::is_optimistic_block_no_fallback` for documentation. - /// - /// Does not mutate fork choice. - pub fn is_optimistic_block_no_fallback( - &self, - block_root: &Hash256, - ) -> Result { - self.fork_choice_read_lock() - .is_optimistic_block_no_fallback(block_root) - } - - /// Returns `true` if the `block_root` is a known descendant of the finalized block. - /// - /// The finalized block used is per fork choice and might be later than (but not conflicting - /// with) `self.cached_head.finalized_checkpoint`. - /// - /// Does not mutate fork choice. - pub fn is_descendant_of_finalized(&self, block_root: Hash256) -> bool { - self.fork_choice_read_lock() - .is_descendant_of_finalized(block_root) - } - - /// Applies an attestation to fork choice. - /// - /// Mutates fork choice. - pub fn on_attestation( - &self, - current_slot: Slot, - attestation: &IndexedAttestation, - is_from_block: AttestationFromBlock, - ) -> Result<(), ForkChoiceError> { - self.fork_choice_write_lock() - .on_attestation(current_slot, attestation, is_from_block) - } - - /// Gets the ancestor of `block_root` at a slot equal to or less than `target_slot`, if any. - /// - /// Does not mutate fork choice. - pub fn get_ancestor_at_or_below_slot( - &self, - block_root: &Hash256, - target_slot: Slot, - ) -> Option { - self.fork_choice_read_lock() - .proto_array() - .core_proto_array() - .iter_block_roots(block_root) - .find(|(_, slot)| *slot <= target_slot) - .map(|(block_root, _)| block_root) - } - - /// Returns the core `ProtoArray` struct as JSON. Useful for the HTTP API. - /// - /// Does not mutate fork choice. - pub fn proto_array_json(&self) -> Result { - serde_json::to_value( - &self - .fork_choice_read_lock() - .proto_array() - .core_proto_array(), - ) - } } impl BeaconChain { diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 6bc056f1071..a427a71db34 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -200,12 +200,15 @@ pub async fn validate_merge_block<'a, T: BeaconChainTypes>( let is_optimistic_candidate = chain .spawn_blocking_handle( move || { - inner_chain.canonical_head.is_optimistic_candidate_block( - current_slot, - block_slot, - &block_parent_root, - &inner_chain.spec, - ) + inner_chain + .canonical_head + .fork_choice_read_lock() + .is_optimistic_candidate_block( + current_slot, + block_slot, + &block_parent_root, + &inner_chain.spec, + ) }, "validate_merge_block_optimistic_candidate", ) @@ -408,6 +411,7 @@ where move || { inner_chain .canonical_head + .fork_choice_read_lock() .get_block(&finalized_checkpoint.root) }, "prepare_execution_payload_finalized_hash", diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index afc935d9df0..7d3141001b9 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2558,14 +2558,12 @@ pub fn serve( .and(chain_filter.clone()) .and_then(|chain: Arc>| { blocking_task(move || { - let json = chain.canonical_head.proto_array_json().map_err(|e| { - warp_utils::reject::custom_server_error(format!( - "Unable to serialize proto array: {:?}", - e - )) - })?; Ok::<_, warp::Rejection>(warp::reply::json(&api_types::GenericResponseRef::from( - &json, + chain + .canonical_head + .fork_choice_read_lock() + .proto_array() + .core_proto_array(), ))) }) }); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 29f411a5d92..4f9f4dacad3 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -318,7 +318,7 @@ impl Tester { self.harness .chain .canonical_head - .fork_choice_write_lock_testing_only() + .fork_choice_write_lock() .update_time(slot) .unwrap(); } @@ -375,7 +375,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_write_lock_testing_only() + .fork_choice_write_lock() .on_block( self.harness.chain.slot().unwrap(), block.message(), @@ -460,7 +460,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_read_lock_testing_only() + .fork_choice_read_lock() .justified_checkpoint(); assert_checkpoints_eq("justified_checkpoint", head_checkpoint, fc_checkpoint); @@ -477,7 +477,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_read_lock_testing_only() + .fork_choice_read_lock() .justified_checkpoint(); assert_checkpoints_eq("justified_checkpoint_root", head_checkpoint, fc_checkpoint); @@ -495,7 +495,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_read_lock_testing_only() + .fork_choice_read_lock() .finalized_checkpoint(); assert_checkpoints_eq("finalized_checkpoint", head_checkpoint, fc_checkpoint); @@ -511,7 +511,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_read_lock_testing_only() + .fork_choice_read_lock() .best_justified_checkpoint(); check_equal( "best_justified_checkpoint", @@ -528,7 +528,7 @@ impl Tester { .harness .chain .canonical_head - .fork_choice_read_lock_testing_only() + .fork_choice_read_lock() .proposer_boost_root(); check_equal( "proposer_boost_root",