Reviewers comments

beacon_node/network/src/service.rs

@@ -819,7 +819,7 @@ impl<T: BeaconChainTypes> NetworkService<T> {
 
     /// Handle a message sent to the network service.
     async fn on_validator_subscription_msg(&mut self, msg: ValidatorSubscriptionMessage) {
-        if let Err(e) = match msg {
+        match msg {
             ValidatorSubscriptionMessage::AttestationSubscribe { subscriptions } => {
                 let subscriptions = subscriptions.into_iter().map(Subscription::Attestation);
                 self.subnet_service.validator_subscriptions(subscriptions)
@@ -828,8 +828,6 @@ impl<T: BeaconChainTypes> NetworkService<T> {
                 let subscriptions = subscriptions.into_iter().map(Subscription::SyncCommittee);
                 self.subnet_service.validator_subscriptions(subscriptions)
             }
-        } {
-            warn!(self.log, "Validator subscription failed"; "error" => e);
         }
     }
 

beacon_node/network/src/service/tests.rs

@@ -1,4 +1,4 @@
-// #[cfg(not(debug_assertions))]
+#[cfg(not(debug_assertions))]
 #[cfg(test)]
 mod tests {
     use crate::persisted_dht::load_dht;

beacon_node/network/src/subnet_service/mod.rs

@@ -138,7 +138,8 @@ impl<T: BeaconChainTypes> SubnetService<T> {
                     .insert(Subnet::Attestation(SubnetId::from(index)));
             }
         } else {
-            // Not subscribed to all subnets, so just calculate the required subnets from the
+            // Not subscribed to all subnets, so just calculate the required subnets from the node
+            // id.
             for subnet_id in
                 SubnetId::compute_attestation_subnets(node_id.raw(), &beacon_chain.spec)
             {
@@ -195,7 +196,7 @@ impl<T: BeaconChainTypes> SubnetService<T> {
         }
     }
 
-    /// Return count of all currently subscribed subnets (long-lived **and** short-lived).
+    /// Return count of all currently subscribed short-lived subnets.
     #[cfg(test)]
     pub fn subscriptions(&self) -> impl Iterator<Item = &Subnet> {
         self.subscriptions.iter()
@@ -223,13 +224,10 @@ impl<T: BeaconChainTypes> SubnetService<T> {
     ///
     /// This returns a result simply for the ergonomics of using ?. The result can be
     /// safely dropped.
-    pub fn validator_subscriptions(
-        &mut self,
-        subscriptions: impl Iterator<Item = Subscription>,
-    ) -> Result<(), String> {
+    pub fn validator_subscriptions(&mut self, subscriptions: impl Iterator<Item = Subscription>) {
         // If the node is in a proposer-only state, we ignore all subnet subscriptions.
         if self.proposer_only {
-            return Ok(());
+            return;
         }
 
         // Maps each subnet subscription to it's highest slot
@@ -355,7 +353,6 @@ impl<T: BeaconChainTypes> SubnetService<T> {
                 warn!(self.log, "Discovery lookup request error"; "error" => e);
             };
         }
-        Ok(())
     }
 
     /// Checks if we have subscribed aggregate validators for the subnet. If not, checks the gossip
@@ -394,8 +391,6 @@ impl<T: BeaconChainTypes> SubnetService<T> {
     /// request.
     ///
     /// If there is sufficient time, queues a peer discovery request for all the required subnets.
-    /// `subnets_to_discover` takes a (subnet_id, Option<Slot>), where if the slot is not set, we
-    /// send a discovery request immediately.
     // NOTE: Sending early subscriptions results in early searching for peers on subnets.
     fn discover_peers_request(
         &mut self,
@@ -493,7 +488,7 @@ impl<T: BeaconChainTypes> SubnetService<T> {
         Ok(())
     }
 
-    /// Adds a subscription event and an associated unsubscription event if required.
+    /// Adds a subscription event to the sync subnet.
     fn subscribe_to_sync_subnet(
         &mut self,
         subnet: Subnet,

beacon_node/network/src/subnet_service/tests/mod.rs

@@ -208,7 +208,6 @@ mod test {
     async fn subscribe_current_slot_wait_for_unsubscribe() {
         // subscription config
         let committee_index = 1;
-        // Keep a low subscription slot so that there are no additional subnet discovery events.
         let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize;
 
         // create the attestation service and subscriptions
@@ -298,7 +297,7 @@ mod test {
         let com1 = 1;
         let com2 = 0;
 
-        // create the attestation service and subscriptions
+        // create the subnet service and subscriptions
         let mut subnet_service = get_subnet_service();
         let _events = get_events(&mut subnet_service, None, 0).await;
         let current_slot = subnet_service
@@ -367,7 +366,7 @@ mod test {
             assert_eq!([expected], unsubscribe_event[..]);
         }
 
-        // Should be subscribed 2 long lived subnet after unsubscription.
+        // Should  no longer be subscribed to any short lived subnets after unsubscription.
         assert_eq!(subnet_service.subscriptions().count(), 0);
     }
 

consensus/types/src/subnet_id.rs

@@ -8,6 +8,10 @@ use std::sync::LazyLock;
 
 const MAX_SUBNET_ID: usize = 64;
 
+/// The number of bits in a Discovery `NodeId`. This is used for binary operations on the node-id
+/// data.
+const NODE_ID_BITS: u64 = 256;
+
 static SUBNET_ID_TO_STRING: LazyLock<Vec<String>> = LazyLock::new(|| {
     let mut v = Vec::with_capacity(MAX_SUBNET_ID);
 
@@ -73,8 +77,8 @@ impl SubnetId {
             .into())
     }
 
-    /// Computes the set of subnets the node should be subscribed to during the current epoch,
-    /// along with the first epoch in which these subscriptions are no longer valid.
+    /// Computes the set of subnets the node should be subscribed to. We subscribe to these subnets
+    /// for the duration of the node's runtime.
     #[allow(clippy::arithmetic_side_effects)]
     pub fn compute_attestation_subnets(
         raw_node_id: [u8; 32],
@@ -85,7 +89,9 @@ impl SubnetId {
 
         let node_id = U256::from_be_slice(&raw_node_id);
         // calculate the prefixes used to compute the subnet and shuffling
-        let node_id_prefix = (node_id >> (256 - prefix_bits)).as_le_slice().get_u64_le();
+        let node_id_prefix = (node_id >> (NODE_ID_BITS - prefix_bits))
+            .as_le_slice()
+            .get_u64_le();
 
         // Get the constants we need to avoid holding a reference to the spec
         let &ChainSpec {