Improve single block/blob logging (#4579)

* remove closure from `check_availability_mayb_import`

* impove logging, add wrapper struct to requested ids

* improve logging

* only log if we're in deneb. Only delay lookup if we're in deneb

* fix bug in missing components check

beacon_node/network/src/sync/block_lookups/common.rs

@@ -374,7 +374,7 @@ impl<L: Lookup, T: BeaconChainTypes> RequestState<L, T> for BlobRequestState<L,
 
     fn new_request(&self) -> BlobsByRootRequest {
         BlobsByRootRequest {
-            blob_ids: VariableList::from(self.requested_ids.clone()),
+            blob_ids: self.requested_ids.clone().into(),
         }
     }
 
@@ -402,7 +402,7 @@ impl<L: Lookup, T: BeaconChainTypes> RequestState<L, T> for BlobRequestState<L,
                     Err(LookupVerifyError::UnrequestedBlobId)
                 } else {
                     // State should remain downloading until we receive the stream terminator.
-                    self.requested_ids.retain(|id| *id != received_id);
+                    self.requested_ids.remove(&received_id);
                     let blob_index = blob.index;
 
                     if blob_index >= T::EthSpec::max_blobs_per_block() as u64 {

beacon_node/network/src/sync/block_lookups/mod.rs

@@ -96,7 +96,7 @@ pub struct BlockLookups<T: BeaconChainTypes> {
 
     single_block_lookups: FnvHashMap<Id, SingleBlockLookup<Current, T>>,
 
-    da_checker: Arc<DataAvailabilityChecker<T>>,
+    pub(crate) da_checker: Arc<DataAvailabilityChecker<T>>,
 
     /// The logger for the import manager.
     log: Logger,
@@ -126,10 +126,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
         cx: &mut SyncNetworkContext<T>,
     ) {
         let lookup = self.new_current_lookup(block_root, None, &[peer_source], cx);
+
         if let Some(lookup) = lookup {
+            let msg = "Searching for block";
+            lookup_creation_logging(msg, &lookup, peer_source, &self.log);
             self.trigger_single_lookup(lookup, cx);
         }
     }
+
     /// Creates a lookup for the block with the given `block_root`.
     ///
     /// The request is not immediately triggered, and should be triggered by a call to
@@ -142,6 +146,8 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     ) {
         let lookup = self.new_current_lookup(block_root, None, &[peer_source], cx);
         if let Some(lookup) = lookup {
+            let msg = "Initialized delayed lookup for block";
+            lookup_creation_logging(msg, &lookup, peer_source, &self.log);
             self.add_single_lookup(lookup)
         }
     }
@@ -155,13 +161,18 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     pub fn search_child_block(
         &mut self,
         block_root: Hash256,
-        child_components: Option<CachedChildComponents<T::EthSpec>>,
-        peer_source: &[PeerShouldHave],
+        child_components: CachedChildComponents<T::EthSpec>,
+        peer_source: PeerShouldHave,
         cx: &mut SyncNetworkContext<T>,
     ) {
-        let lookup = self.new_current_lookup(block_root, child_components, peer_source, cx);
-        if let Some(lookup) = lookup {
-            self.trigger_single_lookup(lookup, cx);
+        if child_components.is_missing_components() {
+            let lookup =
+                self.new_current_lookup(block_root, Some(child_components), &[peer_source], cx);
+            if let Some(lookup) = lookup {
+                let msg = "Searching for components of a block with unknown parent";
+                lookup_creation_logging(msg, &lookup, peer_source, &self.log);
+                self.trigger_single_lookup(lookup, cx);
+            }
         }
     }
 
@@ -175,13 +186,18 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     pub fn search_child_delayed(
         &mut self,
         block_root: Hash256,
-        child_components: Option<CachedChildComponents<T::EthSpec>>,
-        peer_source: &[PeerShouldHave],
+        child_components: CachedChildComponents<T::EthSpec>,
+        peer_source: PeerShouldHave,
         cx: &mut SyncNetworkContext<T>,
     ) {
-        let lookup = self.new_current_lookup(block_root, child_components, peer_source, cx);
-        if let Some(lookup) = lookup {
-            self.add_single_lookup(lookup)
+        if child_components.is_missing_components() {
+            let lookup =
+                self.new_current_lookup(block_root, Some(child_components), &[peer_source], cx);
+            if let Some(lookup) = lookup {
+                let msg = "Initialized delayed lookup for block with unknown parent";
+                lookup_creation_logging(msg, &lookup, peer_source, &self.log);
+                self.add_single_lookup(lookup)
+            }
         }
     }
 
@@ -218,6 +234,22 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     pub fn trigger_lookup_by_root(&mut self, block_root: Hash256, cx: &SyncNetworkContext<T>) {
         self.single_block_lookups.retain(|_id, lookup| {
             if lookup.block_root() == block_root {
+                if lookup.da_checker.is_deneb() {
+                    let blob_indices = lookup.blob_request_state.requested_ids.indices();
+                    debug!(
+                        self.log,
+                        "Triggering delayed single lookup";
+                        "block" => ?block_root,
+                        "blob_indices" => ?blob_indices
+                    );
+                } else {
+                    debug!(
+                        self.log,
+                        "Triggering delayed single lookup";
+                        "block" => ?block_root,
+                    );
+                }
+
                 if let Err(e) = lookup.request_block_and_blobs(cx) {
                     debug!(self.log, "Delayed single block lookup failed";
                         "error" => ?e,
@@ -271,13 +303,6 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
             return None;
         }
 
-        debug!(
-            self.log,
-            "Searching for block";
-            "peer_id" => ?peers,
-            "block" => ?block_root
-        );
-
         Some(SingleBlockLookup::new(
             block_root,
             child_components,
@@ -583,7 +608,6 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
             &mut parent_lookup,
         ) {
             Ok(()) => {
-                debug!(self.log, "Requesting parent"; &parent_lookup);
                 self.parent_lookups.push(parent_lookup);
             }
             Err(e) => {
@@ -1435,10 +1459,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
             Err(e) => {
                 self.handle_parent_request_error(&mut parent_lookup, cx, e);
             }
-            Ok(_) => {
-                debug!(self.log, "Requesting parent"; &parent_lookup);
-                self.parent_lookups.push(parent_lookup)
-            }
+            Ok(_) => self.parent_lookups.push(parent_lookup),
         }
 
         // We remove and add back again requests so we want this updated regardless of outcome.
@@ -1460,3 +1481,29 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
         self.parent_lookups.drain(..).len()
     }
 }
+
+fn lookup_creation_logging<L: Lookup, T: BeaconChainTypes>(
+    msg: &str,
+    lookup: &SingleBlockLookup<L, T>,
+    peer_source: PeerShouldHave,
+    log: &Logger,
+) {
+    let block_root = lookup.block_root();
+    if lookup.da_checker.is_deneb() {
+        let blob_indices = lookup.blob_request_state.requested_ids.indices();
+        debug!(
+            log,
+            "{}", msg;
+            "peer_id" => ?peer_source,
+            "block" => ?block_root,
+            "blob_indices" => ?blob_indices
+        );
+    } else {
+        debug!(
+            log,
+            "{}", msg;
+            "peer_id" => ?peer_source,
+            "block" => ?block_root,
+        );
+    }
+}

beacon_node/network/src/sync/block_lookups/parent_lookup.rs

@@ -193,11 +193,11 @@ impl<T: BeaconChainTypes> ParentLookup<T> {
     ) -> Result<Option<R::VerifiedResponseType>, ParentVerifyError> {
         let expected_block_root = self.current_parent_request.block_root();
         let request_state = R::request_state_mut(&mut self.current_parent_request);
-        let root_and_block = request_state.verify_response(expected_block_root, block)?;
+        let root_and_verified = request_state.verify_response(expected_block_root, block)?;
 
         // check if the parent of this block isn't in the failed cache. If it is, this chain should
         // be dropped and the peer downscored.
-        if let Some(parent_root) = root_and_block
+        if let Some(parent_root) = root_and_verified
             .as_ref()
             .and_then(|block| R::get_parent_root(block))
         {
@@ -207,7 +207,7 @@ impl<T: BeaconChainTypes> ParentLookup<T> {
             }
         }
 
-        Ok(root_and_block)
+        Ok(root_and_verified)
     }
 
     pub fn add_peers(&mut self, peer_source: &[PeerShouldHave]) {

beacon_node/network/src/sync/block_lookups/single_block_lookup.rs

@@ -5,9 +5,12 @@ use crate::sync::network_context::SyncNetworkContext;
 use beacon_chain::block_verification_types::RpcBlock;
 use beacon_chain::data_availability_checker::{AvailabilityCheckError, DataAvailabilityChecker};
 use beacon_chain::BeaconChainTypes;
+use lighthouse_network::rpc::methods::MaxRequestBlobSidecars;
 use lighthouse_network::{PeerAction, PeerId};
 use slog::{trace, Logger};
+use ssz_types::VariableList;
 use std::collections::HashSet;
+use std::fmt::Debug;
 use std::marker::PhantomData;
 use std::sync::Arc;
 use store::Hash256;
@@ -257,7 +260,7 @@ impl<L: Lookup, T: BeaconChainTypes> SingleBlockLookup<L, T> {
 
     /// Updates this request with the most recent picture of which blobs still need to be requested.
     pub fn update_blobs_request(&mut self) {
-        self.blob_request_state.requested_ids = self.missing_blob_ids()
+        self.blob_request_state.requested_ids = self.missing_blob_ids().into()
     }
 
     /// If `unknown_parent_components` is `Some`, we know block components won't hit the data
@@ -319,12 +322,42 @@ impl<L: Lookup, T: BeaconChainTypes> SingleBlockLookup<L, T> {
     }
 }
 
+#[derive(Clone, Default)]
+pub struct RequestedBlobIds(Vec<BlobIdentifier>);
+
+impl From<Vec<BlobIdentifier>> for RequestedBlobIds {
+    fn from(value: Vec<BlobIdentifier>) -> Self {
+        Self(value)
+    }
+}
+
+impl Into<VariableList<BlobIdentifier, MaxRequestBlobSidecars>> for RequestedBlobIds {
+    fn into(self) -> VariableList<BlobIdentifier, MaxRequestBlobSidecars> {
+        VariableList::from(self.0)
+    }
+}
+
+impl RequestedBlobIds {
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+    pub fn contains(&self, blob_id: &BlobIdentifier) -> bool {
+        self.0.contains(blob_id)
+    }
+    pub fn remove(&mut self, blob_id: &BlobIdentifier) {
+        self.0.retain(|id| id != blob_id)
+    }
+    pub fn indices(&self) -> Vec<u64> {
+        self.0.iter().map(|id| id.index).collect()
+    }
+}
+
 /// The state of the blob request component of a `SingleBlockLookup`.
 pub struct BlobRequestState<L: Lookup, T: EthSpec> {
     /// The latest picture of which blobs still need to be requested. This includes information
     /// from both block/blobs downloaded in the network layer and any blocks/blobs that exist in
     /// the data availability checker.
-    pub requested_ids: Vec<BlobIdentifier>,
+    pub requested_ids: RequestedBlobIds,
     /// Where we store blobs until we receive the stream terminator.
     pub blob_download_queue: FixedBlobSidecarList<T>,
     pub state: SingleLookupRequestState,
@@ -430,6 +463,16 @@ impl<E: EthSpec> CachedChildComponents<E> {
             .filter_map(|(i, blob_opt)| blob_opt.as_ref().map(|_| i))
             .collect::<HashSet<_>>()
     }
+
+    pub fn is_missing_components(&self) -> bool {
+        self.downloaded_block
+            .as_ref()
+            .map(|block| {
+                block.num_expected_blobs()
+                    != self.downloaded_blobs.iter().filter(|b| b.is_some()).count()
+            })
+            .unwrap_or(true)
+    }
 }
 
 /// Object representing the state of a single block or blob lookup request.
@@ -562,7 +605,7 @@ impl<L: Lookup, T: BeaconChainTypes> slog::Value for SingleBlockLookup<L, T> {
         serializer.emit_arguments("hash", &format_args!("{}", self.block_root()))?;
         serializer.emit_arguments(
             "blob_ids",
-            &format_args!("{:?}", self.blob_request_state.requested_ids),
+            &format_args!("{:?}", self.blob_request_state.requested_ids.indices()),
         )?;
         serializer.emit_arguments(
             "block_request_state.state",

beacon_node/network/src/sync/block_lookups/tests.rs

@@ -1306,8 +1306,8 @@ mod deneb_only {
                         block_root = child_root;
                         bl.search_child_block(
                             child_root,
-                            Some(CachedChildComponents::new(Some(child_block), None)),
-                            &[PeerShouldHave::Neither(peer_id)],
+                            CachedChildComponents::new(Some(child_block), None),
+                            PeerShouldHave::Neither(peer_id),
                             &mut cx,
                         );
 
@@ -1344,8 +1344,8 @@ mod deneb_only {
                         *blobs.index_mut(0) = Some(child_blob);
                         bl.search_child_block(
                             child_root,
-                            Some(CachedChildComponents::new(None, Some(blobs))),
-                            &[PeerShouldHave::Neither(peer_id)],
+                            CachedChildComponents::new(None, Some(blobs)),
+                            PeerShouldHave::Neither(peer_id),
                             &mut cx,
                         );