Ensure lookup sync checks caches correctly (#5840)

* Ensure lookup sync checks caches correctly

* fix tests and remove unused method

* Simplify BlockProcessStatus

beacon_node/beacon_chain/src/beacon_block_streamer.rs

@@ -1,4 +1,4 @@
-use crate::{metrics, BeaconChain, BeaconChainError, BeaconChainTypes};
+use crate::{metrics, BeaconChain, BeaconChainError, BeaconChainTypes, BlockProcessStatus};
 use execution_layer::{ExecutionLayer, ExecutionPayloadBodyV1};
 use slog::{crit, debug, error, Logger};
 use std::collections::HashMap;
@@ -410,15 +410,14 @@ impl<T: BeaconChainTypes> BeaconBlockStreamer<T> {
 
     fn check_caches(&self, root: Hash256) -> Option<Arc<SignedBeaconBlock<T::EthSpec>>> {
         if self.check_caches == CheckCaches::Yes {
-            self.beacon_chain
-                .reqresp_pre_import_cache
-                .read()
-                .get(&root)
-                .map(|block| {
+            match self.beacon_chain.get_block_process_status(&root) {
+                BlockProcessStatus::Unknown => None,
+                BlockProcessStatus::NotValidated(block)
+                | BlockProcessStatus::ExecutionValidated(block) => {
                     metrics::inc_counter(&metrics::BEACON_REQRESP_PRE_IMPORT_CACHE_HITS);
-                    block.clone()
-                })
-                .or(self.beacon_chain.early_attester_cache.get_block(root))
+                    Some(block)
+                }
+            }
         } else {
             None
         }

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -337,6 +337,20 @@ struct PartialBeaconBlock<E: EthSpec> {
     bls_to_execution_changes: Vec<SignedBlsToExecutionChange>,
 }
 
+pub enum BlockProcessStatus<E: EthSpec> {
+    /// Block is not in any pre-import cache. Block may be in the data-base or in the fork-choice.
+    Unknown,
+    /// Block is currently processing but not yet validated.
+    NotValidated(Arc<SignedBeaconBlock<E>>),
+    /// Block is fully valid, but not yet imported. It's cached in the da_checker while awaiting
+    /// missing block components.
+    ExecutionValidated(Arc<SignedBeaconBlock<E>>),
+}
+
+pub struct BeaconChainMetrics {
+    pub reqresp_pre_import_cache_len: usize,
+}
+
 pub type LightClientProducerEvent<T> = (Hash256, Slot, SyncAggregate<T>);
 
 pub type BeaconForkChoice<T> = ForkChoice<
@@ -1237,6 +1251,27 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         Ok(self.store.get_blinded_block(block_root)?)
     }
 
+    /// Return the status of a block as it progresses through the various caches of the beacon
+    /// chain. Used by sync to learn the status of a block and prevent repeated downloads /
+    /// processing attempts.
+    pub fn get_block_process_status(&self, block_root: &Hash256) -> BlockProcessStatus<T::EthSpec> {
+        if let Some(block) = self
+            .data_availability_checker
+            .get_execution_valid_block(block_root)
+        {
+            return BlockProcessStatus::ExecutionValidated(block);
+        }
+
+        if let Some(block) = self.reqresp_pre_import_cache.read().get(block_root) {
+            // A block is on the `reqresp_pre_import_cache` but NOT in the
+            // `data_availability_checker` only if it is actively processing. We can expect a future
+            // event with the result of processing
+            return BlockProcessStatus::NotValidated(block.clone());
+        }
+
+        BlockProcessStatus::Unknown
+    }
+
     /// Returns the state at the given root, if any.
     ///
     /// ## Errors
@@ -6630,6 +6665,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             ForkName::Base => Err(Error::UnsupportedFork),
         }
     }
+
+    pub fn metrics(&self) -> BeaconChainMetrics {
+        BeaconChainMetrics {
+            reqresp_pre_import_cache_len: self.reqresp_pre_import_cache.read().len(),
+        }
+    }
 }
 
 impl<T: BeaconChainTypes> Drop for BeaconChain<T> {

beacon_node/beacon_chain/src/data_availability_checker.rs

@@ -86,17 +86,12 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> {
 
     /// Checks if the block root is currenlty in the availability cache awaiting import because
     /// of missing components.
-    pub fn has_execution_valid_block(&self, block_root: &Hash256) -> bool {
+    pub fn get_execution_valid_block(
+        &self,
+        block_root: &Hash256,
+    ) -> Option<Arc<SignedBeaconBlock<T::EthSpec>>> {
         self.availability_cache
-            .has_execution_valid_block(block_root)
-    }
-
-    /// Return the required blobs `block_root` expects if the block is currenlty in the cache.
-    pub fn num_expected_blobs(&self, block_root: &Hash256) -> Option<usize> {
-        self.availability_cache
-            .peek_pending_components(block_root, |components| {
-                components.and_then(|components| components.num_expected_blobs())
-            })
+            .get_execution_valid_block(block_root)
     }
 
     /// Return the set of imported blob indexes for `block_root`. Returns None if there is no block

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

@@ -44,7 +44,7 @@ use ssz_types::{FixedVector, VariableList};
 use std::num::NonZeroUsize;
 use std::{collections::HashSet, sync::Arc};
 use types::blob_sidecar::BlobIdentifier;
-use types::{BlobSidecar, ChainSpec, Epoch, EthSpec, Hash256};
+use types::{BlobSidecar, ChainSpec, Epoch, EthSpec, Hash256, SignedBeaconBlock};
 
 /// This represents the components of a partially available block
 ///
@@ -544,12 +544,19 @@ impl<T: BeaconChainTypes> OverflowLRUCache<T> {
     }
 
     /// Returns true if the block root is known, without altering the LRU ordering
-    pub fn has_execution_valid_block(&self, block_root: &Hash256) -> bool {
-        if let Some(pending_components) = self.critical.read().peek_pending_components(block_root) {
-            pending_components.executed_block.is_some()
-        } else {
-            false
-        }
+    pub fn get_execution_valid_block(
+        &self,
+        block_root: &Hash256,
+    ) -> Option<Arc<SignedBeaconBlock<T::EthSpec>>> {
+        self.critical
+            .read()
+            .peek_pending_components(block_root)
+            .and_then(|pending_components| {
+                pending_components
+                    .executed_block
+                    .as_ref()
+                    .map(|block| block.block_cloned())
+            })
     }
 
     /// Fetch a blob from the cache without affecting the LRU ordering

beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs

@@ -37,6 +37,10 @@ impl<E: EthSpec> DietAvailabilityPendingExecutedBlock<E> {
         &self.block
     }
 
+    pub fn block_cloned(&self) -> Arc<SignedBeaconBlock<E>> {
+        self.block.clone()
+    }
+
     pub fn num_blobs_expected(&self) -> usize {
         self.block
             .message()

beacon_node/beacon_chain/src/lib.rs

@@ -62,9 +62,10 @@ pub mod validator_pubkey_cache;
 
 pub use self::beacon_chain::{
     AttestationProcessingOutcome, AvailabilityProcessingStatus, BeaconBlockResponse,
-    BeaconBlockResponseWrapper, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult,
-    ForkChoiceError, LightClientProducerEvent, OverrideForkchoiceUpdate, ProduceBlockVerification,
-    StateSkipConfig, WhenSlotSkipped, INVALID_FINALIZED_MERGE_TRANSITION_BLOCK_SHUTDOWN_REASON,
+    BeaconBlockResponseWrapper, BeaconChain, BeaconChainTypes, BeaconStore, BlockProcessStatus,
+    ChainSegmentResult, ForkChoiceError, LightClientProducerEvent, OverrideForkchoiceUpdate,
+    ProduceBlockVerification, StateSkipConfig, WhenSlotSkipped,
+    INVALID_FINALIZED_MERGE_TRANSITION_BLOCK_SHUTDOWN_REASON,
     INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON,
 };
 pub use self::beacon_snapshot::BeaconSnapshot;

beacon_node/beacon_chain/src/metrics.rs

@@ -1192,6 +1192,7 @@ pub fn scrape_for_metrics<T: BeaconChainTypes>(beacon_chain: &BeaconChain<T>) {
     }
 
     let attestation_stats = beacon_chain.op_pool.attestation_stats();
+    let chain_metrics = beacon_chain.metrics();
 
     set_gauge_by_usize(
         &BLOCK_PROCESSING_SNAPSHOT_CACHE_SIZE,
@@ -1200,7 +1201,7 @@ pub fn scrape_for_metrics<T: BeaconChainTypes>(beacon_chain: &BeaconChain<T>) {
 
     set_gauge_by_usize(
         &BEACON_REQRESP_PRE_IMPORT_CACHE_SIZE,
-        beacon_chain.reqresp_pre_import_cache.read().len(),
+        chain_metrics.reqresp_pre_import_cache_len,
     );
 
     let da_checker_metrics = beacon_chain.data_availability_checker.metrics();