Use head when computing status for slot

sigp · May 10, 2022 · ce4c520 · ce4c520
1 parent b3ab8a3
commit ce4c520
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 3 deletions.
diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs
@@ -4236,6 +4236,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     ///
     /// Returns `Ok(false)` if the block is pre-Bellatrix, or has `ExecutionStatus::Valid`.
     /// Returns `Ok(true)` if the block has `ExecutionStatus::Optimistic`.
+    ///
+    /// This function will return an error if `head_block` is not present in the fork choice store
+    /// and so should only be used on the head block or when the block *should* be present in the
+    /// fork choice store.
+    ///
+    /// There is a potential race condition when syncing where the block_root of `head_block` could
+    /// be pruned from the fork choice store before being read.
     pub fn is_optimistic_head_block(
         &self,
         head_block: &SignedBeaconBlock<T::EthSpec>,
@@ -4256,6 +4263,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     ///
     /// Returns `Ok(false)` if the head block is pre-Bellatrix, or has `ExecutionStatus::Valid`.
     /// Returns `Ok(true)` if the head block has `ExecutionStatus::Optimistic`.
+    ///
+    /// There is a potential race condition when syncing where the block root of `head_info` could
+    /// be pruned from the fork choice store before being read.
     pub fn is_optimistic_head(
         &self,
         head_info: Option<&HeadInfo>,

diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs
@@ -125,14 +125,28 @@ impl BlockId {
         let execution_optimistic = match self.0 {
             // Genesis block is inherently verified.
             CoreBlockId::Genesis => false,
+            // Head, Finalized and Justified are determined based on their respective statuses.
             CoreBlockId::Head => chain
                 .is_optimistic_head_block(&block)
                 .map_err(warp_utils::reject::beacon_chain_error)?,
-            // Slot, Finalized and Justified are determined based on the current head.
-            CoreBlockId::Slot(_) | CoreBlockId::Finalized | CoreBlockId::Justified => chain
+            // Note that `Justified` should always be present in fork choice,
+            // so using `is_optimistic_head_block` should be fine here. Although there is a small
+            // risk of `Justified` being pruned from the fork choice store before its status is
+            // computed.
+            CoreBlockId::Justified => chain
                 .is_optimistic_head_block(&block)
                 .map_err(warp_utils::reject::beacon_chain_error)?,
-            // If the root is explicitly given, we can determine based on fork-choice.
+            // Since `is_optimistic_block` falls back to the status of the finalized block, using
+            // it should minimize the impacts of the possible race condition.
+            CoreBlockId::Finalized => chain
+                .is_optimistic_block(&block)
+                .map_err(warp_utils::reject::beacon_chain_error)?,
+            // If the slot is supplied we cannot use `block`. Instead we compute the
+            // head and use that to determine the status.
+            CoreBlockId::Slot(_) => chain
+                .is_optimistic_head(None)
+                .map_err(warp_utils::reject::beacon_chain_error)?,
+            // If the root is explicitly given, compute its status directly.
             CoreBlockId::Root(_) => chain
                 .is_optimistic_block(&block)
                 .map_err(warp_utils::reject::beacon_chain_error)?,