From 8c7ba308ace7d698805783477edb3f0fa2ad56da Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 28 Oct 2024 20:34:42 +1100 Subject: [PATCH] Reviewers comments --- beacon_node/network/src/service.rs | 4 +--- beacon_node/network/src/service/tests.rs | 2 +- beacon_node/network/src/subnet_service/mod.rs | 17 ++++++----------- .../network/src/subnet_service/tests/mod.rs | 5 ++--- consensus/types/src/subnet_id.rs | 12 +++++++++--- 5 files changed, 19 insertions(+), 21 deletions(-) diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 629633dcb04..3b47d9c8036 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -819,7 +819,7 @@ impl NetworkService { /// 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 NetworkService { let subscriptions = subscriptions.into_iter().map(Subscription::SyncCommittee); self.subnet_service.validator_subscriptions(subscriptions) } - } { - warn!(self.log, "Validator subscription failed"; "error" => e); } } diff --git a/beacon_node/network/src/service/tests.rs b/beacon_node/network/src/service/tests.rs index 39b04af2a7c..c46e46e0fae 100644 --- a/beacon_node/network/src/service/tests.rs +++ b/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; diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 7e09549f1b0..3bd67043f5d 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -138,7 +138,8 @@ impl SubnetService { .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 SubnetService { } } - /// 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 { self.subscriptions.iter() @@ -223,13 +224,10 @@ impl SubnetService { /// /// 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, - ) -> Result<(), String> { + pub fn validator_subscriptions(&mut self, subscriptions: impl Iterator) { // 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 SubnetService { 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 SubnetService { /// request. /// /// If there is sufficient time, queues a peer discovery request for all the required subnets. - /// `subnets_to_discover` takes a (subnet_id, Option), 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 SubnetService { 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, diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 31fc35ad9d6..93b09673701 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/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); } diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index d3724d40c3d..187b070d29f 100644 --- a/consensus/types/src/subnet_id.rs +++ b/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> = 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 {