Forward sync columns by root

beacon_node/beacon_chain/src/block_verification.rs

@@ -418,7 +418,7 @@ pub enum ExecutionPayloadError {
 }
 
 impl ExecutionPayloadError {
-    pub fn penalize_peer(&self) -> bool {
+    pub fn penalize_gossip_peer(&self) -> bool {
         // This match statement should never have a default case so that we are
         // always forced to consider here whether or not to penalize a peer when
         // we add a new error condition.
@@ -447,6 +447,39 @@ impl ExecutionPayloadError {
             ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false,
         }
     }
+
+    pub fn penalize_sync_peer(&self) -> bool {
+        // This match statement should never have a default case so that we are
+        // always forced to consider here whether or not to penalize a peer when
+        // we add a new error condition.
+        match self {
+            // The peer has nothing to do with this error, do not penalize them.
+            ExecutionPayloadError::NoExecutionConnection => false,
+            // The peer has nothing to do with this error, do not penalize them.
+            ExecutionPayloadError::RequestFailed(_) => false,
+            // For the sync case, we do not want a peer to keep sending us blocks that our
+            // execution engine considers invalid.
+            //
+            // Also, we ask peers for blocks over sync/rpc only when they indicate
+            // that they have fully validated a given block (using their status message).
+            //
+            // Hence, we should penalize for this error in the sync case.
+            ExecutionPayloadError::RejectedByExecutionEngine { .. } => true,
+            // There is no reason for an honest peer to propagate a block with an invalid
+            // payload time stamp.
+            ExecutionPayloadError::InvalidPayloadTimestamp { .. } => true,
+            // We do not want to receive these blocks over rpc even though the gossip
+            // case is still allowed.
+            ExecutionPayloadError::InvalidTerminalPoWBlock { .. } => true,
+            // We should penalize RPC blocks, since even an optimistic node shouldn't
+            // verify this block.
+            ExecutionPayloadError::InvalidActivationEpoch { .. } => true,
+            // As per `Self::InvalidActivationEpoch`.
+            ExecutionPayloadError::InvalidTerminalBlockHash { .. } => true,
+            // Do not penalize the peer since it's not their fault that *we're* optimistic.
+            ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false,
+        }
+    }
 }
 
 impl From<execution_layer::Error> for ExecutionPayloadError {

beacon_node/lighthouse_network/src/peer_manager/peerdb.rs

@@ -326,6 +326,36 @@ impl<E: EthSpec> PeerDB<E> {
             .map(|(peer_id, _)| peer_id)
     }
 
+    /// Returns an iterator of all good gossipsub peers that are supposed to be custodying
+    /// the given subnet id and have the epoch according to their status messages.
+    pub fn good_custody_subnet_peer_range_sync(
+        &self,
+        subnet: DataColumnSubnetId,
+        epoch: Epoch,
+    ) -> impl Iterator<Item = &PeerId> {
+        self.peers
+            .iter()
+            .filter(move |(_, info)| {
+                // The custody_subnets hashset can be populated via enr or metadata
+                let is_custody_subnet_peer = info.is_assigned_to_custody_subnet(&subnet);
+
+                info.is_connected()
+                    && is_custody_subnet_peer
+                    && match info.sync_status() {
+                        SyncStatus::Synced { info } => {
+                            info.has_slot(epoch.end_slot(E::slots_per_epoch()))
+                        }
+                        SyncStatus::Advanced { info } => {
+                            info.has_slot(epoch.end_slot(E::slots_per_epoch()))
+                        }
+                        SyncStatus::IrrelevantPeer
+                        | SyncStatus::Behind { .. }
+                        | SyncStatus::Unknown => false,
+                    }
+            })
+            .map(|(peer_id, _)| peer_id)
+    }
+
     /// Checks if there is at least one good peer for each specified custody subnet for the given epoch.
     /// A "good" peer is one that is both connected and synced (or advanced) for the specified epoch.
     pub fn has_good_custody_range_sync_peer(

beacon_node/lighthouse_network/src/rpc/methods.rs

@@ -535,6 +535,18 @@ impl<E: EthSpec> DataColumnsByRootRequest<E> {
         Ok(Self { data_column_ids })
     }
 
+    pub fn from_single_block(block_root: Hash256, indices: Vec<u64>) -> Result<Self, &'static str> {
+        let columns = VariableList::new(indices)
+            .map_err(|_| "Number of indices exceeds total number of columns")?;
+        DataColumnsByRootRequest::new(
+            vec![DataColumnsByRootIdentifier {
+                block_root,
+                columns,
+            }],
+            1,
+        )
+    }
+
     pub fn max_requested(&self) -> usize {
         self.data_column_ids.iter().map(|id| id.columns.len()).sum()
     }

beacon_node/lighthouse_network/src/service/api_types.rs

@@ -9,6 +9,12 @@ use types::{
 
 pub type Id = u32;
 
+#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
+pub enum RangeRequestType {
+    ForwardSync,
+    BackfillSync,
+}
+
 #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
 pub struct SingleLookupReqId {
     pub lookup_id: Id,
@@ -38,6 +44,7 @@ pub enum SyncRequestId {
 pub struct DataColumnsByRootRequestId {
     pub id: Id,
     pub requester: DataColumnsByRootRequester,
+    pub peer: PeerId,
 }
 
 #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
@@ -46,6 +53,20 @@ pub struct BlocksByRangeRequestId {
     pub id: Id,
     /// The Id of the overall By Range request for block components.
     pub parent_request_id: ComponentsByRangeRequestId,
+    /// The peer that we made this request to
+    pub peer_id: PeerId,
+}
+
+impl BlocksByRangeRequestId {
+    pub fn batch_id(&self) -> Epoch {
+        match self.parent_request_id.requester {
+            RangeRequestId::BackfillSync { batch_id } => batch_id,
+            RangeRequestId::RangeSync {
+                chain_id: _,
+                batch_id,
+            } => batch_id,
+        }
+    }
 }
 
 #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
@@ -86,12 +107,31 @@ pub enum RangeRequestId {
     RangeSync { chain_id: Id, batch_id: Epoch },
     BackfillSync { batch_id: Epoch },
 }
+impl RangeRequestId {
+    pub fn batch_id(&self) -> Epoch {
+        match &self {
+            RangeRequestId::BackfillSync { batch_id } => *batch_id,
+            RangeRequestId::RangeSync {
+                chain_id: _,
+                batch_id,
+            } => *batch_id,
+        }
+    }
+
+    pub fn batch_type(&self) -> RangeRequestType {
+        match &self {
+            RangeRequestId::BackfillSync { .. } => RangeRequestType::BackfillSync,
+            RangeRequestId::RangeSync { .. } => RangeRequestType::ForwardSync,
+        }
+    }
+}
 
 // TODO(das) refactor in a separate PR. We might be able to remove this and replace
 // [`DataColumnsByRootRequestId`] with a [`SingleLookupReqId`].
 #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
 pub enum DataColumnsByRootRequester {
     Custody(CustodyId),
+    RangeSync { parent: ComponentsByRangeRequestId },
 }
 
 #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
@@ -222,6 +262,7 @@ impl Display for DataColumnsByRootRequester {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {
             Self::Custody(id) => write!(f, "Custody/{id}"),
+            Self::RangeSync { parent } => write!(f, "Range/{parent}"),
         }
     }
 }
@@ -255,6 +296,7 @@ mod tests {
                     lookup_id: 101,
                 }),
             }),
+            peer: PeerId::random(),
         };
         assert_eq!(format!("{id}"), "123/Custody/121/Lookup/101");
     }

beacon_node/network/src/metrics.rs

@@ -484,6 +484,12 @@ pub static SYNC_ACTIVE_NETWORK_REQUESTS: LazyLock<Result<IntGaugeVec>> = LazyLoc
         &["type"],
     )
 });
+pub static SYNC_PENDING_ROOT_RANGE_REQUESTS: LazyLock<Result<IntGauge>> = LazyLock::new(|| {
+    try_create_int_gauge(
+        "sync_pending_root_range_requests",
+        "Current count of pending columns by root requests waiting for peers",
+    )
+});
 pub static SYNC_UNKNOWN_NETWORK_REQUESTS: LazyLock<Result<IntCounterVec>> = LazyLock::new(|| {
     try_create_int_counter_vec(
         "sync_unknwon_network_request",

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -1330,7 +1330,9 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
                 return None;
             }
-            Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
+            Err(ref e @ BlockError::ExecutionPayloadError(ref epe))
+                if !epe.penalize_gossip_peer() =>
+            {
                 debug!(error = %e, "Could not verify block for gossip. Ignoring the block");
                 self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
                 return None;
@@ -1562,7 +1564,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                     "Block with unknown parent attempted to be processed"
                 );
             }
-            Err(e @ BlockError::ExecutionPayloadError(epe)) if !epe.penalize_peer() => {
+            Err(e @ BlockError::ExecutionPayloadError(epe)) if !epe.penalize_gossip_peer() => {
                 debug!(
                     error = %e,
                     "Failed to verify execution payload"