Merge remote-tracking branch 'sigp/unstable' into gloas-lookup-sync-fixes

Reconciles unstable's #9383 (Deprecate blob lookup sync) with this PR's
rewritten lookup architecture by removing blob lookup from the new arch:
Deneb/Electra block lookups complete on the block alone (the merged
da_checker makes them available without blobs), and DataDownload::Blobs,
blob_lookup_request, SyncRequestId::SingleBlob, BlockProcessType::SingleBlob,
the process_rpc_blobs lookup cluster, and blob lookup tests are removed.
Range-sync blobs and blob serving are kept.

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

@@ -1,9 +1,7 @@
 use crate::sync::block_lookups::single_block_lookup::{
     LookupRequestError, SingleBlockLookup, SingleLookupRequestState,
 };
-use crate::sync::block_lookups::{
-    BlobRequestState, BlockRequestState, CustodyRequestState, PeerId,
-};
+use crate::sync::block_lookups::{BlockRequestState, CustodyRequestState, PeerId};
 use crate::sync::manager::BlockProcessType;
 use crate::sync::network_context::{LookupRequestResult, SyncNetworkContext};
 use beacon_chain::BeaconChainTypes;
@@ -11,7 +9,6 @@ use lighthouse_network::service::api_types::Id;
 use parking_lot::RwLock;
 use std::collections::HashSet;
 use std::sync::Arc;
-use types::data::FixedBlobSidecarList;
 use types::{DataColumnSidecarList, SignedBeaconBlock};
 
 use super::SingleLookupId;
@@ -20,17 +17,16 @@ use super::single_block_lookup::{ComponentRequests, DownloadResult};
 #[derive(Debug, Copy, Clone)]
 pub enum ResponseType {
     Block,
-    Blob,
     CustodyColumn,
 }
 
-/// This trait unifies common single block lookup functionality across blocks and blobs. This
-/// includes making requests, verifying responses, and handling processing results. A
-/// `SingleBlockLookup` includes both a `BlockRequestState` and a `BlobRequestState`, this trait is
-/// implemented for each.
+/// This trait unifies common single block lookup functionality across blocks and data columns.
+/// This includes making requests, verifying responses, and handling processing results. A
+/// `SingleBlockLookup` includes both a `BlockRequestState` and a `CustodyRequestState`, this trait
+/// is implemented for each.
 ///
 /// The use of the `ResponseType` associated type gives us a degree of type
-/// safety when handling a block/blob response ensuring we only mutate the correct corresponding
+/// safety when handling a block/column response ensuring we only mutate the correct corresponding
 /// state.
 pub trait RequestState<T: BeaconChainTypes> {
     /// The type created after validation.
@@ -61,7 +57,7 @@ pub trait RequestState<T: BeaconChainTypes> {
     /// Returns the `ResponseType` associated with this trait implementation. Useful in logging.
     fn response_type() -> ResponseType;
 
-    /// A getter for the `BlockRequestState` or `BlobRequestState` associated with this trait.
+    /// A getter for the `BlockRequestState` or `CustodyRequestState` associated with this trait.
     fn request_state_mut(request: &mut SingleBlockLookup<T>) -> Result<&mut Self, &'static str>;
 
     /// A getter for a reference to the `SingleLookupRequestState` associated with this trait.
@@ -114,54 +110,6 @@ impl<T: BeaconChainTypes> RequestState<T> for BlockRequestState<T::EthSpec> {
     }
 }
 
-impl<T: BeaconChainTypes> RequestState<T> for BlobRequestState<T::EthSpec> {
-    type VerifiedResponseType = FixedBlobSidecarList<T::EthSpec>;
-
-    fn make_request(
-        &self,
-        id: Id,
-        lookup_peers: Arc<RwLock<HashSet<PeerId>>>,
-        expected_blobs: usize,
-        cx: &mut SyncNetworkContext<T>,
-    ) -> Result<LookupRequestResult, LookupRequestError> {
-        cx.blob_lookup_request(id, lookup_peers, self.block_root, expected_blobs)
-            .map_err(LookupRequestError::SendFailedNetwork)
-    }
-
-    fn send_for_processing(
-        id: Id,
-        download_result: DownloadResult<Self::VerifiedResponseType>,
-        cx: &SyncNetworkContext<T>,
-    ) -> Result<(), LookupRequestError> {
-        let DownloadResult {
-            value,
-            block_root,
-            seen_timestamp,
-            ..
-        } = download_result;
-        cx.send_blobs_for_processing(id, block_root, value, seen_timestamp)
-            .map_err(LookupRequestError::SendFailedProcessor)
-    }
-
-    fn response_type() -> ResponseType {
-        ResponseType::Blob
-    }
-    fn request_state_mut(request: &mut SingleBlockLookup<T>) -> Result<&mut Self, &'static str> {
-        match &mut request.component_requests {
-            ComponentRequests::WaitingForBlock => Err("waiting for block"),
-            ComponentRequests::ActiveBlobRequest(request, _) => Ok(request),
-            ComponentRequests::ActiveCustodyRequest { .. } => Err("expecting custody request"),
-            ComponentRequests::NotNeeded { .. } => Err("not needed"),
-        }
-    }
-    fn get_state(&self) -> &SingleLookupRequestState<Self::VerifiedResponseType> {
-        &self.state
-    }
-    fn get_state_mut(&mut self) -> &mut SingleLookupRequestState<Self::VerifiedResponseType> {
-        &mut self.state
-    }
-}
-
 impl<T: BeaconChainTypes> RequestState<T> for CustodyRequestState<T::EthSpec> {
     type VerifiedResponseType = DataColumnSidecarList<T::EthSpec>;
 
@@ -203,7 +151,6 @@ impl<T: BeaconChainTypes> RequestState<T> for CustodyRequestState<T::EthSpec> {
     fn request_state_mut(request: &mut SingleBlockLookup<T>) -> Result<&mut Self, &'static str> {
         match &mut request.component_requests {
             ComponentRequests::WaitingForBlock => Err("waiting for block"),
-            ComponentRequests::ActiveBlobRequest { .. } => Err("expecting blob request"),
             ComponentRequests::ActiveCustodyRequest(request) => Ok(request),
             ComponentRequests::NotNeeded { .. } => Err("not needed"),
         }

beacon_node/network/src/sync/block_sidecar_coupling.rs

@@ -607,11 +607,14 @@ mod tests {
 
         let mut spec = test_spec::<E>();
         spec.deneb_fork_epoch = Some(Epoch::new(0));
+        // Pin to pre-PeerDAS so this exercises the blob (not custody-column) path under any
+        // FORK_NAME.
+        spec.fulu_fork_epoch = None;
         let spec = Arc::new(spec);
         let da_checker = Arc::new(test_da_checker(spec.clone(), NodeCustodyType::Fullnode));
-        // Assert response is finished and RpcBlocks cannot be constructed, because blobs weren't returned.
+        // Blobs are no longer required for availability, so the response succeeds without them.
         let result = info.responses(da_checker, spec).unwrap();
-        assert!(result.is_err())
+        assert!(result.is_ok())
     }
 
     #[test]

beacon_node/network/src/sync/network_context/requests.rs

@@ -9,7 +9,6 @@ use tracing::{Span, debug};
 use types::{Hash256, Slot};
 
 pub use blobs_by_range::BlobsByRangeRequestItems;
-pub use blobs_by_root::{BlobsByRootRequestItems, BlobsByRootSingleBlockRequest};
 pub use blocks_by_range::BlocksByRangeRequestItems;
 pub use blocks_by_root::{BlocksByRootRequestItems, BlocksByRootSingleRequest};
 pub use data_columns_by_range::DataColumnsByRangeRequestItems;
@@ -25,7 +24,6 @@ use crate::metrics;
 use super::{RpcEvent, RpcResponseError, RpcResponseResult};
 
 mod blobs_by_range;
-mod blobs_by_root;
 mod blocks_by_range;
 mod blocks_by_root;
 mod data_columns_by_range;

beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs

@@ -1,73 +0,0 @@
-use lighthouse_network::rpc::methods::BlobsByRootRequest;
-use std::sync::Arc;
-use types::{BlobSidecar, EthSpec, ForkContext, Hash256, data::BlobIdentifier};
-
-use super::{ActiveRequestItems, LookupVerifyError};
-
-#[derive(Debug, Clone)]
-pub struct BlobsByRootSingleBlockRequest {
-    pub block_root: Hash256,
-    pub indices: Vec<u64>,
-}
-
-impl BlobsByRootSingleBlockRequest {
-    pub fn into_request(self, spec: &ForkContext) -> Result<BlobsByRootRequest, String> {
-        BlobsByRootRequest::new(
-            self.indices
-                .into_iter()
-                .map(|index| BlobIdentifier {
-                    block_root: self.block_root,
-                    index,
-                })
-                .collect(),
-            spec,
-        )
-    }
-}
-
-pub struct BlobsByRootRequestItems<E: EthSpec> {
-    request: BlobsByRootSingleBlockRequest,
-    items: Vec<Arc<BlobSidecar<E>>>,
-}
-
-impl<E: EthSpec> BlobsByRootRequestItems<E> {
-    pub fn new(request: BlobsByRootSingleBlockRequest) -> Self {
-        Self {
-            request,
-            items: vec![],
-        }
-    }
-}
-
-impl<E: EthSpec> ActiveRequestItems for BlobsByRootRequestItems<E> {
-    type Item = Arc<BlobSidecar<E>>;
-
-    /// Appends a chunk to this multi-item request. If all expected chunks are received, this
-    /// method returns `Some`, resolving the request before the stream terminator.
-    /// The active request SHOULD be dropped after `add_response` returns an error
-    fn add(&mut self, blob: Self::Item) -> Result<bool, LookupVerifyError> {
-        let block_root = blob.block_root();
-        if self.request.block_root != block_root {
-            return Err(LookupVerifyError::UnrequestedBlockRoot(block_root));
-        }
-
-        if !blob.verify_blob_sidecar_inclusion_proof() {
-            return Err(LookupVerifyError::InvalidInclusionProof);
-        }
-
-        if !self.request.indices.contains(&blob.index) {
-            return Err(LookupVerifyError::UnrequestedIndex(blob.index));
-        }
-        if self.items.iter().any(|b| b.index == blob.index) {
-            return Err(LookupVerifyError::DuplicatedData(blob.slot(), blob.index));
-        }
-
-        self.items.push(blob);
-
-        Ok(self.items.len() >= self.request.indices.len())
-    }
-
-    fn consume(&mut self) -> Vec<Self::Item> {
-        std::mem::take(&mut self.items)
-    }
-}