Deneb PR feedback updates (#4716)

* move length update outside of if let in LRU cache

* add comment and use hex for G1_POINT_AT_INFINITY

* remove some misleading comments from `ssz_snappy`

* make sure we can't overflow on blobs by range requests with large counts

* downgrade gossip verification internal availability check error

* change blob rpc responses from BlockingFnWithManualSendOnIdle to BlockingFn

* remove unnecessary collect in blobs by range response

* add a comment to blobs by range response start slot logic

* typo persist_data_availabilty_checker -> persist_data_availability_checker

* unify cheap_state_advance_to_obtain_committees

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -648,7 +648,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         Ok(())
     }
 
-    pub fn persist_data_availabilty_checker(&self) -> Result<(), Error> {
+    pub fn persist_data_availability_checker(&self) -> Result<(), Error> {
         let _timer = metrics::start_timer(&metrics::PERSIST_DATA_AVAILABILITY_CHECKER);
         self.data_availability_checker.persist_all()?;
 
@@ -6298,7 +6298,7 @@ impl<T: BeaconChainTypes> Drop for BeaconChain<T> {
         let drop = || -> Result<(), Error> {
             self.persist_head_and_fork_choice()?;
             self.persist_op_pool()?;
-            self.persist_data_availabilty_checker()?;
+            self.persist_data_availability_checker()?;
             self.persist_eth1_cache()
         };
 

beacon_node/beacon_chain/src/blob_verification.rs

@@ -1,12 +1,12 @@
 use derivative::Derivative;
 use slot_clock::SlotClock;
-use state_processing::state_advance::partial_state_advance;
 use std::sync::Arc;
 
 use crate::beacon_chain::{
     BeaconChain, BeaconChainTypes, BLOCK_PROCESSING_CACHE_LOCK_TIMEOUT,
     VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT,
 };
+use crate::block_verification::cheap_state_advance_to_obtain_committees;
 use crate::data_availability_checker::AvailabilityCheckError;
 use crate::kzg_utils::{validate_blob, validate_blobs};
 use crate::BeaconChainError;
@@ -14,11 +14,10 @@ use kzg::Kzg;
 use slog::{debug, warn};
 use ssz_derive::{Decode, Encode};
 use ssz_types::VariableList;
-use std::borrow::Cow;
 use types::blob_sidecar::BlobIdentifier;
 use types::{
-    BeaconState, BeaconStateError, BlobSidecar, BlobSidecarList, ChainSpec, CloneConfig, EthSpec,
+    BeaconStateError, BlobSidecar, BlobSidecarList, CloneConfig, EthSpec, Hash256,
-    Hash256, RelativeEpoch, SignedBlobSidecar, Slot,
+    SignedBlobSidecar, Slot,
 };
 
 /// An error occurred while validating a gossip blob.
@@ -308,12 +307,13 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
                     "block_root" => %block_root,
                     "index" => %blob_index,
                 );
-                let state = cheap_state_advance_to_obtain_committees(
+                let state =
-                    &mut snapshot.beacon_state,
+                    cheap_state_advance_to_obtain_committees::<_, GossipBlobError<T::EthSpec>>(
-                    Some(snapshot.beacon_block_root),
+                        &mut snapshot.beacon_state,
-                    blob_slot,
+                        Some(snapshot.beacon_block_root),
-                    &chain.spec,
+                        blob_slot,
-                )?;
+                        &chain.spec,
+                    )?;
                 (
                     state.get_beacon_proposer_index(blob_slot, &chain.spec)?,
                     state.fork(),
@@ -344,7 +344,7 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
                         parent_block.state_root()
                     ))
                 })?;
-            let state = cheap_state_advance_to_obtain_committees(
+            let state = cheap_state_advance_to_obtain_committees::<_, GossipBlobError<T::EthSpec>>(
                 &mut parent_state,
                 Some(parent_block.state_root()),
                 blob_slot,
@@ -428,56 +428,6 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
     })
 }
 
-/// Performs a cheap (time-efficient) state advancement so the committees and proposer shuffling for
-/// `slot` can be obtained from `state`.
-///
-/// The state advancement is "cheap" since it does not generate state roots. As a result, the
-/// returned state might be holistically invalid but the committees/proposers will be correct (since
-/// they do not rely upon state roots).
-///
-/// If the given `state` can already serve the `slot`, the committees will be built on the `state`
-/// and `Cow::Borrowed(state)` will be returned. Otherwise, the state will be cloned, cheaply
-/// advanced and then returned as a `Cow::Owned`. The end result is that the given `state` is never
-/// mutated to be invalid (in fact, it is never changed beyond a simple committee cache build).
-///
-/// Note: This is a copy of the `block_verification::cheap_state_advance_to_obtain_committees` to return
-/// a BlobError error type instead.
-fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
-    state: &'a mut BeaconState<E>,
-    state_root_opt: Option<Hash256>,
-    blob_slot: Slot,
-    spec: &ChainSpec,
-) -> Result<Cow<'a, BeaconState<E>>, GossipBlobError<E>> {
-    let block_epoch = blob_slot.epoch(E::slots_per_epoch());
-
-    if state.current_epoch() == block_epoch {
-        // Build both the current and previous epoch caches, as the previous epoch caches are
-        // useful for verifying attestations in blocks from the current epoch.
-        state.build_committee_cache(RelativeEpoch::Previous, spec)?;
-        state.build_committee_cache(RelativeEpoch::Current, spec)?;
-
-        Ok(Cow::Borrowed(state))
-    } else if state.slot() > blob_slot {
-        Err(GossipBlobError::BlobIsNotLaterThanParent {
-            blob_slot,
-            parent_slot: state.slot(),
-        })
-    } else {
-        let mut state = state.clone_with(CloneConfig::committee_caches_only());
-        let target_slot = block_epoch.start_slot(E::slots_per_epoch());
-
-        // Advance the state into the same epoch as the block. Use the "partial" method since state
-        // roots are not important for proposer/attester shuffling.
-        partial_state_advance(&mut state, state_root_opt, target_slot, spec)
-            .map_err(|e| GossipBlobError::BeaconChainError(BeaconChainError::from(e)))?;
-
-        state.build_committee_cache(RelativeEpoch::Previous, spec)?;
-        state.build_committee_cache(RelativeEpoch::Current, spec)?;
-
-        Ok(Cow::Owned(state))
-    }
-}
-
 /// Wrapper over a `BlobSidecar` for which we have completed kzg verification.
 /// i.e. `verify_blob_kzg_proof(blob, commitment, proof) == true`.
 #[derive(Debug, Derivative, Clone, Encode, Decode)]

beacon_node/beacon_chain/src/block_verification.rs

@@ -90,6 +90,7 @@ use state_processing::{
     StateProcessingStrategy, VerifyBlockRoot,
 };
 use std::borrow::Cow;
+use std::fmt::Debug;
 use std::fs;
 use std::io::Write;
 use std::sync::Arc;
@@ -575,7 +576,7 @@ pub fn signature_verify_chain_segment<T: BeaconChainTypes>(
         .map(|(_, block)| block.slot())
         .unwrap_or_else(|| slot);
 
-    let state = cheap_state_advance_to_obtain_committees(
+    let state = cheap_state_advance_to_obtain_committees::<_, BlockError<T::EthSpec>>(
         &mut parent.pre_state,
         parent.beacon_state_root,
         highest_slot,
@@ -887,7 +888,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             );
 
             // The state produced is only valid for determining proposer/attester shuffling indices.
-            let state = cheap_state_advance_to_obtain_committees(
+            let state = cheap_state_advance_to_obtain_committees::<_, BlockError<T::EthSpec>>(
                 &mut parent.pre_state,
                 parent.beacon_state_root,
                 block.slot(),
@@ -1017,7 +1018,7 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
 
         let (mut parent, block) = load_parent(block_root, block, chain)?;
 
-        let state = cheap_state_advance_to_obtain_committees(
+        let state = cheap_state_advance_to_obtain_committees::<_, BlockError<T::EthSpec>>(
             &mut parent.pre_state,
             parent.beacon_state_root,
             block.slot(),
@@ -1067,7 +1068,7 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
             load_parent(from.block_root, from.block, chain)?
         };
 
-        let state = cheap_state_advance_to_obtain_committees(
+        let state = cheap_state_advance_to_obtain_committees::<_, BlockError<T::EthSpec>>(
             &mut parent.pre_state,
             parent.beacon_state_root,
             block.slot(),
@@ -1900,6 +1901,30 @@ fn load_parent<T: BeaconChainTypes, B: AsBlock<T::EthSpec>>(
     result
 }
 
+/// This trait is used to unify `BlockError` and `BlobError` so
+/// `cheap_state_advance_to_obtain_committees` can be re-used in gossip blob validation.
+pub trait CheapStateAdvanceError: From<BeaconStateError> + From<BeaconChainError> + Debug {
+    fn not_later_than_parent_error(block_slot: Slot, state_slot: Slot) -> Self;
+}
+
+impl<E: EthSpec> CheapStateAdvanceError for BlockError<E> {
+    fn not_later_than_parent_error(block_slot: Slot, parent_slot: Slot) -> Self {
+        BlockError::BlockIsNotLaterThanParent {
+            block_slot,
+            parent_slot,
+        }
+    }
+}
+
+impl<E: EthSpec> CheapStateAdvanceError for GossipBlobError<E> {
+    fn not_later_than_parent_error(blob_slot: Slot, parent_slot: Slot) -> Self {
+        GossipBlobError::BlobIsNotLaterThanParent {
+            blob_slot,
+            parent_slot,
+        }
+    }
+}
+
 /// Performs a cheap (time-efficient) state advancement so the committees and proposer shuffling for
 /// `slot` can be obtained from `state`.
 ///
@@ -1911,12 +1936,12 @@ fn load_parent<T: BeaconChainTypes, B: AsBlock<T::EthSpec>>(
 /// and `Cow::Borrowed(state)` will be returned. Otherwise, the state will be cloned, cheaply
 /// advanced and then returned as a `Cow::Owned`. The end result is that the given `state` is never
 /// mutated to be invalid (in fact, it is never changed beyond a simple committee cache build).
-fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
+pub fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec, Err: CheapStateAdvanceError>(
     state: &'a mut BeaconState<E>,
     state_root_opt: Option<Hash256>,
     block_slot: Slot,
     spec: &ChainSpec,
-) -> Result<Cow<'a, BeaconState<E>>, BlockError<E>> {
+) -> Result<Cow<'a, BeaconState<E>>, Err> {
     let block_epoch = block_slot.epoch(E::slots_per_epoch());
 
     if state.current_epoch() == block_epoch {
@@ -1927,10 +1952,7 @@ fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
 
         Ok(Cow::Borrowed(state))
     } else if state.slot() > block_slot {
-        Err(BlockError::BlockIsNotLaterThanParent {
+        Err(Err::not_later_than_parent_error(block_slot, state.slot()))
-            block_slot,
-            parent_slot: state.slot(),
-        })
     } else {
         let mut state = state.clone_with(CloneConfig::committee_caches_only());
         let target_slot = block_epoch.start_slot(E::slots_per_epoch());
@@ -1938,7 +1960,7 @@ fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
         // Advance the state into the same epoch as the block. Use the "partial" method since state
         // roots are not important for proposer/attester shuffling.
         partial_state_advance(&mut state, state_root_opt, target_slot, spec)
-            .map_err(|e| BlockError::BeaconChainError(BeaconChainError::from(e)))?;
+            .map_err(BeaconChainError::from)?;
 
         state.build_committee_cache(RelativeEpoch::Previous, spec)?;
         state.build_committee_cache(RelativeEpoch::Current, spec)?;

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

@@ -691,9 +691,9 @@ impl<T: BeaconChainTypes> OverflowLRUCache<T> {
                     // it is still LRU entry -> delete it from memory & record that it's on disk
                     write_lock.in_memory.pop_entry(&lru_root);
                     write_lock.store_keys.insert(lru_root);
-                    stored = write_lock.in_memory.len();
                 }
             }
+            stored = write_lock.in_memory.len();
             drop(write_lock);
         }
 

beacon_node/beacon_processor/src/lib.rs

@@ -618,8 +618,8 @@ pub enum Work<E: EthSpec> {
     Status(BlockingFn),
     BlocksByRangeRequest(BlockingFnWithManualSendOnIdle),
     BlocksByRootsRequest(BlockingFnWithManualSendOnIdle),
-    BlobsByRangeRequest(BlockingFnWithManualSendOnIdle),
+    BlobsByRangeRequest(BlockingFn),
-    BlobsByRootsRequest(BlockingFnWithManualSendOnIdle),
+    BlobsByRootsRequest(BlockingFn),
     GossipBlsToExecutionChange(BlockingFn),
     LightClientBootstrapRequest(BlockingFn),
     ApiRequestP0(BlockingOrAsync),
@@ -1461,10 +1461,10 @@ impl<E: EthSpec> BeaconProcessor<E> {
                 .spawn_async(async move {
                     work.await;
                 }),
-            Work::BlobsByRangeRequest(work)
+            Work::BlobsByRangeRequest(process_fn) | Work::BlobsByRootsRequest(process_fn) => {
-            | Work::BlobsByRootsRequest(work)
+                task_spawner.spawn_blocking(process_fn)
-            | Work::BlocksByRangeRequest(work)
+            }
-            | Work::BlocksByRootsRequest(work) => {
+            Work::BlocksByRangeRequest(work) | Work::BlocksByRootsRequest(work) => {
                 task_spawner.spawn_blocking_with_manual_send_idle(work)
             }
             Work::ChainSegmentBackfill(process_fn) => task_spawner.spawn_async(process_fn),

beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs

@@ -402,20 +402,15 @@ fn context_bytes<T: EthSpec>(
                     // NOTE: If you are adding another fork type here, be sure to modify the
                     //       `fork_context.to_context_bytes()` function to support it as well!
                     SignedBeaconBlock::Deneb { .. } => {
-                        // Deneb context being `None` implies that "merge never happened".
                         fork_context.to_context_bytes(ForkName::Deneb)
                     }
                     SignedBeaconBlock::Capella { .. } => {
-                        // Capella context being `None` implies that "merge never happened".
                         fork_context.to_context_bytes(ForkName::Capella)
                     }
                     SignedBeaconBlock::Merge { .. } => {
-                        // Merge context being `None` implies that "merge never happened".
                         fork_context.to_context_bytes(ForkName::Merge)
                     }
                     SignedBeaconBlock::Altair { .. } => {
-                        // Altair context being `None` implies that "altair never happened".
-                        // This code should be unreachable if altair is disabled since only Version::V1 would be valid in that case.
                         fork_context.to_context_bytes(ForkName::Altair)
                     }
                     SignedBeaconBlock::Base { .. } => Some(fork_context.genesis_context_bytes()),

beacon_node/lighthouse_network/src/rpc/methods.rs

@@ -297,6 +297,12 @@ pub struct BlobsByRangeRequest {
     pub count: u64,
 }
 
+impl BlobsByRangeRequest {
+    pub fn max_blobs_requested<E: EthSpec>(&self) -> u64 {
+        self.count.saturating_mul(E::max_blobs_per_block() as u64)
+    }
+}
+
 /// Request a number of beacon block roots from a peer.
 #[superstruct(
     variants(V1, V2),

beacon_node/lighthouse_network/src/rpc/outbound.rs

@@ -99,7 +99,7 @@ impl<TSpec: EthSpec> OutboundRequest<TSpec> {
             OutboundRequest::Goodbye(_) => 0,
             OutboundRequest::BlocksByRange(req) => *req.count(),
             OutboundRequest::BlocksByRoot(req) => req.block_roots().len() as u64,
-            OutboundRequest::BlobsByRange(req) => req.count * TSpec::max_blobs_per_block() as u64,
+            OutboundRequest::BlobsByRange(req) => req.max_blobs_requested::<TSpec>(),
             OutboundRequest::BlobsByRoot(req) => req.blob_ids.len() as u64,
             OutboundRequest::Ping(_) => 1,
             OutboundRequest::MetaData(_) => 1,

beacon_node/lighthouse_network/src/rpc/protocol.rs

@@ -574,7 +574,7 @@ impl<TSpec: EthSpec> InboundRequest<TSpec> {
             InboundRequest::Goodbye(_) => 0,
             InboundRequest::BlocksByRange(req) => *req.count(),
             InboundRequest::BlocksByRoot(req) => req.block_roots().len() as u64,
-            InboundRequest::BlobsByRange(req) => req.count * TSpec::max_blobs_per_block() as u64,
+            InboundRequest::BlobsByRange(req) => req.max_blobs_requested::<TSpec>(),
             InboundRequest::BlobsByRoot(req) => req.blob_ids.len() as u64,
             InboundRequest::Ping(_) => 1,
             InboundRequest::MetaData(_) => 1,

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -1186,7 +1186,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                     | AvailabilityCheckError::MissingBlobs
                     | AvailabilityCheckError::StoreError(_)
                     | AvailabilityCheckError::DecodeError(_) => {
-                        crit!(
+                        warn!(
                             self.log,
                             "Internal availability check error";
                             "error" => ?err,

beacon_node/network/src/network_beacon_processor/mod.rs

@@ -562,9 +562,8 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         request: BlobsByRangeRequest,
     ) -> Result<(), Error<T::EthSpec>> {
         let processor = self.clone();
-        let process_fn = move |send_idle_on_drop| {
+        let process_fn =
-            processor.handle_blobs_by_range_request(send_idle_on_drop, peer_id, request_id, request)
+            move || processor.handle_blobs_by_range_request(peer_id, request_id, request);
-        };
 
         self.try_send(BeaconWorkEvent {
             drop_during_sync: false,
@@ -580,9 +579,8 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         request: BlobsByRootRequest,
     ) -> Result<(), Error<T::EthSpec>> {
         let processor = self.clone();
-        let process_fn = move |send_idle_on_drop| {
+        let process_fn =
-            processor.handle_blobs_by_root_request(send_idle_on_drop, peer_id, request_id, request)
+            move || processor.handle_blobs_by_root_request(peer_id, request_id, request);
-        };
 
         self.try_send(BeaconWorkEvent {
             drop_during_sync: false,

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

@@ -220,7 +220,6 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
     /// Handle a `BlobsByRoot` request from the peer.
     pub fn handle_blobs_by_root_request(
         self: Arc<Self>,
-        send_on_drop: SendOnDrop,
         peer_id: PeerId,
         request_id: PeerRequestId,
         request: BlobsByRootRequest,
@@ -286,7 +285,6 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         if send_response {
             self.send_response(peer_id, Response::BlobsByRoot(None), request_id);
         }
-        drop(send_on_drop);
     }
 
     /// Handle a `BlocksByRoot` request from the peer.
@@ -607,7 +605,6 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
     /// Handle a `BlobsByRange` request from the peer.
     pub fn handle_blobs_by_range_request(
         self: Arc<Self>,
-        send_on_drop: SendOnDrop,
         peer_id: PeerId,
         request_id: PeerRequestId,
         req: BlobsByRangeRequest,
@@ -619,7 +616,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         );
 
         // Should not send more than max request blocks
-        if req.count * T::EthSpec::max_blobs_per_block() as u64 > MAX_REQUEST_BLOB_SIDECARS {
+        if req.max_blobs_requested::<T::EthSpec>() > MAX_REQUEST_BLOB_SIDECARS {
             return self.send_error_response(
                 peer_id,
                 RPCResponseErrorCode::InvalidRequest,
@@ -711,13 +708,16 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 }
             };
 
-        // Pick out the required blocks, ignoring skip-slots.
+        // Use `WhenSlotSkipped::Prev` to get the most recent block root prior to
+        // `request_start_slot` in order to check whether the `request_start_slot` is a skip.
         let mut last_block_root = req.start_slot.checked_sub(1).and_then(|prev_slot| {
             self.chain
                 .block_root_at_slot(Slot::new(prev_slot), WhenSlotSkipped::Prev)
                 .ok()
                 .flatten()
         });
+
+        // Pick out the required blocks, ignoring skip-slots.
         let maybe_block_roots = process_results(forwards_block_root_iter, |iter| {
             iter.take_while(|(_, slot)| slot.as_u64() < req.start_slot.saturating_add(req.count))
                 // map skip slots to None
@@ -745,7 +745,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         };
 
         // remove all skip slots
-        let block_roots = block_roots.into_iter().flatten().collect::<Vec<_>>();
+        let block_roots = block_roots.into_iter().flatten();
 
         let mut blobs_sent = 0;
         let mut send_response = true;
@@ -806,7 +806,5 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 id: request_id,
             });
         }
-
-        drop(send_on_drop);
     }
 }

crypto/kzg/src/kzg_proof.rs

@@ -24,9 +24,10 @@ impl From<KzgProof> for c_kzg_min::Bytes48 {
 }
 
 impl KzgProof {
+    /// Creates a valid proof using `G1_POINT_AT_INFINITY`.
     pub fn empty() -> Self {
         let mut bytes = [0; BYTES_PER_PROOF];
-        bytes[0] = 192;
+        bytes[0] = 0xc0;
         Self(bytes)
     }
 }