From 2c42e450b5f71abe35afc67adef82b318f30351d Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 13 May 2024 10:10:40 +0300 Subject: [PATCH 01/10] Initial temp commit --- .../src/subnet_service/attestation_subnets.rs | 565 +++++++++--------- consensus/types/src/chain_spec.rs | 57 +- consensus/types/src/subnet_id.rs | 94 +-- 3 files changed, 301 insertions(+), 415 deletions(-) diff --git a/beacon_node/network/src/subnet_service/attestation_subnets.rs b/beacon_node/network/src/subnet_service/attestation_subnets.rs index ab9ffb95a6c..f6d8b355f58 100644 --- a/beacon_node/network/src/subnet_service/attestation_subnets.rs +++ b/beacon_node/network/src/subnet_service/attestation_subnets.rs @@ -8,7 +8,6 @@ use std::collections::{HashMap, VecDeque}; use std::pin::Pin; use std::sync::Arc; use std::task::{Context, Poll}; -use std::time::Duration; use beacon_chain::{BeaconChain, BeaconChainTypes}; use delay_map::{HashMapDelay, HashSetDelay}; @@ -16,7 +15,10 @@ use futures::prelude::*; use lighthouse_network::{discv5::enr::NodeId, NetworkConfig, Subnet, SubnetDiscovery}; use slog::{debug, error, info, o, trace, warn}; use slot_clock::SlotClock; -use types::{Attestation, EthSpec, Slot, SubnetId, ValidatorSubscription}; +use types::{ + Attestation, EthSpec, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, + ValidatorSubscription, +}; use crate::metrics; @@ -29,27 +31,26 @@ pub(crate) const MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD: u64 = 2; /// Currently a whole slot ahead. const ADVANCE_SUBSCRIBE_SLOT_FRACTION: u32 = 1; -#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] -pub(crate) enum SubscriptionKind { - /// Long lived subscriptions. - /// - /// These have a longer duration and are advertised in our ENR. - LongLived, - /// Short lived subscriptions. - /// - /// Subscribing to these subnets has a short duration and we don't advertise it in our ENR. - ShortLived, -} - /// A particular subnet at a given slot. #[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] pub struct ExactSubnet { /// The `SubnetId` associated with this subnet. - pub subnet_id: SubnetId, - /// The `Slot` associated with this subnet. + pub subnet_id: Subnet, + /// For Attestations, this slot represents the start time at which we need to subscribe to the + /// slot. For SyncCommittee subnet id's this represents the end slot at which we no longer need + /// to subscribe to the subnet. + // NOTE: There was different logic between the two subscriptions and having a different + // interpretation of this variable seemed like the best way to group the logic, even though it + // may be counter-intuitive (apologies to future readers). pub slot: Slot, } +/// The enum used to group all kinds of validator subscriptions +pub enum Subscription { + Attestation(ValidatorSubscription), + SyncCommittee(SyncCommitteeSubscription), +} + pub struct AttestationService { /// Queued events to return to the driving service. events: VecDeque, @@ -61,17 +62,18 @@ pub struct AttestationService { /// /// Once they expire, we unsubscribe from these. /// We subscribe to subnets when we are an aggregator for an exact subnet. - short_lived_subscriptions: HashMapDelay, + // NOTE: When setup the defalut timeout is set for sync committee subscriptions. + subscriptions: HashMapDelay, - /// Subnets we are currently subscribed to as long lived subscriptions. - /// - /// We advertise these in our ENR. When these expire, the subnet is removed from our ENR. - /// These are required of all beacon nodes. The exact number is determined by the chain - /// specification. - long_lived_subscriptions: HashSet, + /// A list of permanent subnets that this node is subscribed to. + // TODO: Shift this to a dynamic bitfield + permanent_attestation_subscriptions: HashSet, + + /// A collection of timeouts for when to unsubscirbe from a sync committee subnet. + // sync_unsubscriptions: HashSetDelay, - /// Short lived subscriptions that need to be executed in the future. - scheduled_short_lived_subscriptions: HashSetDelay, + /// Subscriptions that need to be executed in the future. + scheduled_subscriptions: HashSetDelay, /// A collection timeouts to track the existence of aggregate validator subscriptions at an /// `ExactSubnet`. @@ -86,12 +88,6 @@ pub struct AttestationService { /// We are always subscribed to all subnets. subscribe_all_subnets: bool, - /// Our Discv5 node_id. - node_id: NodeId, - - /// Future used to manage subscribing and unsubscribing from long lived subnets. - next_long_lived_subscription_event: Pin>, - /// Whether this node is a block proposer-only node. proposer_only: bool, @@ -115,39 +111,50 @@ impl AttestationService { if config.subscribe_all_subnets { slog::info!(log, "Subscribing to all subnets"); + } + + // Build the list of known permanent subscriptions, so that we know not to subscribe or + // discover them. + let mut permanent_attestation_subscriptions = HashSet::default(); + if config.subscribe_all_subnets { + // We are subscribed to all subnets, set all the bits to true. + for index in 0..::SubnetBitfieldLength::to_u64() { + permanent_attestation_subscriptions.insert(SubnetId::from(index)); + } } else { - slog::info!(log, "Deterministic long lived subnets enabled"; "subnets_per_node" => beacon_chain.spec.subnets_per_node, "subscription_duration_in_epochs" => beacon_chain.spec.epochs_per_subnet_subscription); + // Not subscribed to all subnets, so just calculate the required subnets from the + for subnet_id in SubnetId::compute_attestation_subnets::( + node_id.raw().into(), + &beacon_chain.spec, + ) { + permanent_attestation_subscriptions.insert(subnet_id); + } } + // Set up the sync committee subscriptions + let spec = &beacon_chain.spec; + let epoch_duration_secs = + beacon_chain.slot_clock.slot_duration().as_secs() * T::EthSpec::slots_per_epoch(); + let default_sync_committee_duration = Duration::from_secs( + epoch_duration_secs.saturating_mul(spec.epochs_per_sync_committee_period.as_u64()), + ); + let track_validators = !config.import_all_attestations; let aggregate_validators_on_subnet = track_validators.then(|| HashSetDelay::new(slot_duration)); - let mut service = AttestationService { + AttestationService { events: VecDeque::with_capacity(10), beacon_chain, - short_lived_subscriptions: HashMapDelay::new(slot_duration), - long_lived_subscriptions: HashSet::default(), - scheduled_short_lived_subscriptions: HashSetDelay::default(), + subscriptions: HashMapDelay::new(default_sync_committee_duration), + permanent_attestation_subscriptions, + scheduled_subscriptions: HashSetDelay::default(), aggregate_validators_on_subnet, waker: None, discovery_disabled: config.disable_discovery, subscribe_all_subnets: config.subscribe_all_subnets, - node_id, - next_long_lived_subscription_event: { - // Set a dummy sleep. Calculating the current subnet subscriptions will update this - // value with a smarter timing - Box::pin(tokio::time::sleep(Duration::from_secs(1))) - }, proposer_only: config.proposer_only, log, - }; - - // If we are not subscribed to all subnets, handle the deterministic set of subnets - if !config.subscribe_all_subnets { - service.recompute_long_lived_subnets(); } - - service } /// Return count of all currently subscribed subnets (long-lived **and** short-lived). @@ -156,36 +163,32 @@ impl AttestationService { if self.subscribe_all_subnets { self.beacon_chain.spec.attestation_subnet_count as usize } else { - let count = self - .short_lived_subscriptions - .keys() - .chain(self.long_lived_subscriptions.iter()) - .collect::>() - .len(); + let count = self.subscriptions.keys().collect::>().len(); count } } - /// Returns whether we are subscribed to a subnet for testing purposes. + /// Return count of all currently subscribed sync committee subnets. #[cfg(test)] - pub(crate) fn is_subscribed( - &self, - subnet_id: &SubnetId, - subscription_kind: SubscriptionKind, - ) -> bool { - match subscription_kind { - SubscriptionKind::LongLived => self.long_lived_subscriptions.contains(subnet_id), - SubscriptionKind::ShortLived => self.short_lived_subscriptions.contains_key(subnet_id), + pub fn sync_committee_subscription_count(&self) -> usize { + use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; + if self.subscribe_all_subnets { + SYNC_COMMITTEE_SUBNET_COUNT as usize + } else { + self.subscriptions.len() } } + /// Returns whether we are subscribed to a subnet for testing purposes. #[cfg(test)] - pub(crate) fn long_lived_subscriptions(&self) -> &HashSet { - &self.long_lived_subscriptions + pub(crate) fn is_subscribed(&self, subnet_id: &SubnetId) -> bool { + self.subscriptions + .contains_key(&Subnet::Attestation(subnet_id)) } /// Processes a list of validator subscriptions. /// + /// This is fundamentally called form the HTTP API when a validator requests duties from us /// This will: /// - Register new validators as being known. /// - Search for peers for required subnets. @@ -196,7 +199,7 @@ impl AttestationService { /// safely dropped. pub fn validator_subscriptions( &mut self, - subscriptions: impl Iterator, + subscriptions: impl Iterator, ) -> Result<(), String> { // If the node is in a proposer-only state, we ignore all subnet subscriptions. if self.proposer_only { @@ -204,64 +207,107 @@ impl AttestationService { } // Maps each subnet_id subscription to it's highest slot - let mut subnets_to_discover: HashMap = HashMap::new(); + let mut subnets_to_discover: HashMap = HashMap::new(); // Registers the validator with the attestation service. - for subscription in subscriptions { - metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_REQUESTS); - - trace!(self.log, - "Validator subscription"; - "subscription" => ?subscription, - ); - - // Compute the subnet that is associated with this subscription - let subnet_id = match SubnetId::compute_subnet::( - subscription.slot, - subscription.attestation_committee_index, - subscription.committee_count_at_slot, - &self.beacon_chain.spec, - ) { - Ok(subnet_id) => subnet_id, - Err(e) => { - warn!(self.log, - "Failed to compute subnet id for validator subscription"; - "error" => ?e, - ); - continue; - } - }; - // Ensure each subnet_id inserted into the map has the highest slot as it's value. - // Higher slot corresponds to higher min_ttl in the `SubnetDiscovery` entry. - if let Some(slot) = subnets_to_discover.get(&subnet_id) { - if subscription.slot > *slot { - subnets_to_discover.insert(subnet_id, subscription.slot); + for general_subscription in subscriptions { + match general_subscription { + Subscription::Attestation(subscription) => { + metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_REQUESTS); + + // Compute the subnet that is associated with this subscription + let subnet_id = match SubnetId::compute_subnet::( + subscription.slot, + subscription.attestation_committee_index, + subscription.committee_count_at_slot, + &self.beacon_chain.spec, + ) { + Ok(subnet_id) => Subnet::Attestation(subnet_id), + Err(e) => { + warn!(self.log, + "Failed to compute subnet id for validator subscription"; + "error" => ?e, + ); + continue; + } + }; + + // Ensure each subnet_id inserted into the map has the highest slot as it's value. + // Higher slot corresponds to higher min_ttl in the `SubnetDiscovery` entry. + if let Some(slot) = subnets_to_discover.get(&subnet_id) { + if subscription.slot > *slot { + subnets_to_discover.insert(subnet_id, subscription.slot); + } + } else if !self.discovery_disabled { + subnets_to_discover.insert(subnet_id, subscription.slot); + } + + let exact_subnet = ExactSubnet { + subnet_id, + slot: subscription.slot, + }; + + // Determine if the validator is an aggregator. If so, we subscribe to the subnet and + // if successful add the validator to a mapping of known aggregators for that exact + // subnet. + + if subscription.is_aggregator { + metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_AGGREGATOR_REQUESTS); + if let Err(e) = self.subscribe_to_subnet(exact_subnet) { + warn!(self.log, + "Subscription to subnet error"; + "error" => e, + ); + } + } } - } else if !self.discovery_disabled { - subnets_to_discover.insert(subnet_id, subscription.slot); - } + Subscription::SyncCommittee(subscription) => { + metrics::inc_counter(&metrics::SYNC_COMMITTEE_SUBSCRIPTION_REQUESTS); + // NOTE: We assume all subscriptions have been verified before reaching this service - let exact_subnet = ExactSubnet { - subnet_id, - slot: subscription.slot, - }; - - // Determine if the validator is an aggregator. If so, we subscribe to the subnet and - // if successful add the validator to a mapping of known aggregators for that exact - // subnet. - - if subscription.is_aggregator { - metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_AGGREGATOR_REQUESTS); - if let Err(e) = self.subscribe_to_short_lived_subnet(exact_subnet) { - warn!(self.log, - "Subscription to subnet error"; - "error" => e, - ); - } else { + // Registers the validator with the subnet service. trace!(self.log, - "Subscribed to subnet for aggregator duties"; - "exact_subnet" => ?exact_subnet, + "Sync committee subscription"; + "subscription" => ?subscription, ); + + let subnet_ids = + match SyncSubnetId::compute_subnets_for_sync_committee::( + &subscription.sync_committee_indices, + ) { + Ok(subnet_ids) => subnet_ids, + Err(e) => { + warn!(self.log, + "Failed to compute subnet id for sync committee subscription"; + "error" => ?e, + "validator_index" => subscription.validator_index + ); + continue; + } + }; + + for subnet_id in subnet_ids { + let exact_subnet = ExactSubnet { + subnet_id: Subnet::SyncCommittee(subnet_id), + slot: subscription + .until_epoch + .start_slot(T::EthSpec::slots_per_epoch()), + }; + subnets_to_discover.push(exact_subnet.clone()); + if let Err(e) = self.subscribe_to_sync_subnet(exact_subnet.subnet_id) { + warn!(self.log, + "Subscription to sync subnet error"; + "error" => e, + "validator_index" => subscription.validator_index, + ); + } else { + trace!(self.log, + "Subscribed to subnet for sync committee duties"; + "exact_subnet" => ?exact_subnet, + "validator_index" => subscription.validator_index + ); + } + } } } } @@ -269,110 +315,13 @@ impl AttestationService { // If the discovery mechanism isn't disabled, attempt to set up a peer discovery for the // required subnets. if !self.discovery_disabled { - if let Err(e) = self.discover_peers_request( - subnets_to_discover - .into_iter() - .map(|(subnet_id, slot)| ExactSubnet { subnet_id, slot }), - ) { + if let Err(e) = self.discover_peers_request(subnets_to_discover.into_iter()) { warn!(self.log, "Discovery lookup request error"; "error" => e); }; } - Ok(()) } - fn recompute_long_lived_subnets(&mut self) { - // Ensure the next computation is scheduled even if assigning subnets fails. - let next_subscription_event = self - .recompute_long_lived_subnets_inner() - .unwrap_or_else(|_| self.beacon_chain.slot_clock.slot_duration()); - - debug!(self.log, "Recomputing deterministic long lived subnets"); - self.next_long_lived_subscription_event = - Box::pin(tokio::time::sleep(next_subscription_event)); - - if let Some(waker) = self.waker.as_ref() { - waker.wake_by_ref(); - } - } - - /// Gets the long lived subnets the node should be subscribed to during the current epoch and - /// the remaining duration for which they remain valid. - fn recompute_long_lived_subnets_inner(&mut self) -> Result { - let current_epoch = self.beacon_chain.epoch().map_err(|e| { - if !self - .beacon_chain - .slot_clock - .is_prior_to_genesis() - .unwrap_or(false) - { - error!(self.log, "Failed to get the current epoch from clock"; "err" => ?e) - } - })?; - - let (subnets, next_subscription_epoch) = SubnetId::compute_subnets_for_epoch::( - self.node_id.raw().into(), - current_epoch, - &self.beacon_chain.spec, - ) - .map_err(|e| error!(self.log, "Could not compute subnets for current epoch"; "err" => e))?; - - let next_subscription_slot = - next_subscription_epoch.start_slot(T::EthSpec::slots_per_epoch()); - let next_subscription_event = self - .beacon_chain - .slot_clock - .duration_to_slot(next_subscription_slot) - .ok_or_else(|| { - error!( - self.log, - "Failed to compute duration to next to long lived subscription event" - ) - })?; - - self.update_long_lived_subnets(subnets.collect()); - - Ok(next_subscription_event) - } - - /// Updates the long lived subnets. - /// - /// New subnets are registered as subscribed, removed subnets as unsubscribed and the Enr - /// updated accordingly. - fn update_long_lived_subnets(&mut self, mut subnets: HashSet) { - info!(self.log, "Subscribing to long-lived subnets"; "subnets" => ?subnets.iter().collect::>()); - for subnet in &subnets { - // Add the events for those subnets that are new as long lived subscriptions. - if !self.long_lived_subscriptions.contains(subnet) { - // Check if this subnet is new and send the subscription event if needed. - if !self.short_lived_subscriptions.contains_key(subnet) { - debug!(self.log, "Subscribing to subnet"; - "subnet" => ?subnet, - "subscription_kind" => ?SubscriptionKind::LongLived, - ); - self.queue_event(SubnetServiceMessage::Subscribe(Subnet::Attestation( - *subnet, - ))); - } - self.queue_event(SubnetServiceMessage::EnrAdd(Subnet::Attestation(*subnet))); - if !self.discovery_disabled { - self.queue_event(SubnetServiceMessage::DiscoverPeers(vec![SubnetDiscovery { - subnet: Subnet::Attestation(*subnet), - min_ttl: None, - }])) - } - } - } - - // Update the long_lived_subnets set and check for subnets that are being removed - std::mem::swap(&mut self.long_lived_subscriptions, &mut subnets); - for subnet in subnets { - if !self.long_lived_subscriptions.contains(&subnet) { - self.handle_removed_subnet(subnet, SubscriptionKind::LongLived); - } - } - } - /// Checks if we have subscribed aggregate validators for the subnet. If not, checks the gossip /// verification, re-propagates and returns false. pub fn should_process_attestation( @@ -409,9 +358,12 @@ impl AttestationService { /// 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, - exact_subnets: impl Iterator, + subnets_to_discover: impl Iterator, ) -> Result<(), &'static str> { let current_slot = self .beacon_chain @@ -419,11 +371,14 @@ impl AttestationService { .now() .ok_or("Could not get the current slot")?; - let discovery_subnets: Vec = exact_subnets - .filter_map(|exact_subnet| { + let discovery_subnets: Vec = subnets_to_discover + .filter_map(|(subnet, relevant_slot)| { + // We generate discovery requests for all subnets (even one's we are permenantly + // subscribed to) in order to ensure our peer counts are satisfactory to perform the + // necessary duties. + // Check if there is enough time to perform a discovery lookup. - if exact_subnet.slot - >= current_slot.saturating_add(MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD) + if relevant_slot >= current_slot.saturating_add(MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD) { // Send out an event to start looking for peers. // Require the peer for an additional slot to ensure we keep the peer for the @@ -431,18 +386,15 @@ impl AttestationService { let min_ttl = self .beacon_chain .slot_clock - .duration_to_slot(exact_subnet.slot + 1) + .duration_to_slot(relevant_slot + 1) .map(|duration| std::time::Instant::now() + duration); - Some(SubnetDiscovery { - subnet: Subnet::Attestation(exact_subnet.subnet_id), - min_ttl, - }) + Some(SubnetDiscovery { subnet, min_ttl }) } else { // We may want to check the global PeerInfo to see estimated timeouts for each // peer before they can be removed. warn!(self.log, "Not enough time for a discovery search"; - "subnet_id" => ?exact_subnet + "subnet_id" => ?subnet, ); None } @@ -456,10 +408,19 @@ impl AttestationService { } // Subscribes to the subnet if it should be done immediately, or schedules it if required. - fn subscribe_to_short_lived_subnet( + fn subscribe_to_subnet( &mut self, ExactSubnet { subnet_id, slot }: ExactSubnet, ) -> Result<(), &'static str> { + // If the subnet is one of our permanent subnets, we do not need to subscribe. + if self.subscribe_all_subnets + || self + .permanent_attestation_subscriptions + .contains(&subnet_id) + { + return Ok(()); + } + let slot_duration = self.beacon_chain.slot_clock.slot_duration(); // Calculate how long before we need to subscribe to the subnet. @@ -485,17 +446,79 @@ impl AttestationService { // immediately. if time_to_subscription_start.is_zero() { // This is a current or past slot, we subscribe immediately. - self.subscribe_to_short_lived_subnet_immediately(subnet_id, slot + 1)?; + self.subscribe_to_subnet_immediately(subnet_id, slot + 1)?; } else { // This is a future slot, schedule subscribing. trace!(self.log, "Scheduling subnet subscription"; "subnet" => ?subnet_id, "time_to_subscription_start" => ?time_to_subscription_start); - self.scheduled_short_lived_subscriptions + self.scheduled_subscriptions .insert_at(ExactSubnet { subnet_id, slot }, time_to_subscription_start); } Ok(()) } + /// Adds a subscription event and an associated unsubscription event if required. + fn subscribe_to_sync_subnet(&mut self, exact_subnet: ExactSubnet) -> Result<(), &'static str> { + // Return if we have subscribed to all subnets + if self.subscribe_all_subnets { + return Ok(()); + } + + // Return if we already have a subscription for the subnet and its closer or + if let Some(until_slot) = self.subscriptions.get(exact_subnet.subnet_id) { + if until_slot >= *exact_subnet.slot { + return Ok(()); + } + } + + // Initialise timing variables + let current_slot = self + .beacon_chain + .slot_clock + .now() + .ok_or("Could not get the current slot")?; + + // Calculate the duration to the unsubscription event. + let expected_end_subscription_duration = if current_slot >= exact_subnet.slot { + warn!( + self.log, + "Sync committee subscription is past expiration"; + "current_slot" => current_slot, + "exact_subnet" => ?exact_subnet, + ); + return Ok(()); + } else { + let slot_duration = self.beacon_chain.slot_clock.slot_duration(); + + // the duration until we no longer need this subscription. We assume a single slot is + // sufficient. + self.beacon_chain + .slot_clock + .duration_to_slot(exact_subnet.slot) + .ok_or("Unable to determine duration to unsubscription slot")? + + slot_duration + }; + + if !self + .subscriptions + .insert_at(exact_subnet.subnet_id, expected_end_subscription_duration) + { + // We are not currently subscribed and have no waiting subscription, create one + debug!(self.log, "Subscribing to subnet"; "subnet" => *exact_subnet.subnet_id, "until_epoch" => ?exact_subnet.slot); + self.events + .push_back(SubnetServiceMessage::Subscribe(Subnet::SyncCommittee( + exact_subnet.subnet_id, + ))); + + // add the subnet to the ENR bitfield + self.events + .push_back(SubnetServiceMessage::EnrAdd(Subnet::SyncCommittee( + exact_subnet.subnet_id, + ))); + } + Ok(()) + } + /* A collection of functions that handle the various timeouts */ /// Registers a subnet as subscribed. @@ -505,7 +528,7 @@ impl AttestationService { /// out the appropriate events. /// /// On determinist long lived subnets, this is only used for short lived subscriptions. - fn subscribe_to_short_lived_subnet_immediately( + fn subscribe_to_subnet_immediately( &mut self, subnet_id: SubnetId, end_slot: Slot, @@ -526,18 +549,11 @@ impl AttestationService { return Err("Time when subscription would end has already passed."); } - let subscription_kind = SubscriptionKind::ShortLived; - // We need to check and add a subscription for the right kind, regardless of the presence // of the subnet as a subscription of the other kind. This is mainly since long lived // subscriptions can be removed at any time when a validator goes offline. - let (subscriptions, already_subscribed_as_other_kind) = ( - &mut self.short_lived_subscriptions, - self.long_lived_subscriptions.contains(&subnet_id), - ); - - match subscriptions.get(&subnet_id) { + match self.subscriptions.get(&subnet_id) { Some(current_end_slot) => { // We are already subscribed. Check if we need to extend the subscription. if &end_slot > current_end_slot { @@ -545,27 +561,25 @@ impl AttestationService { "subnet" => ?subnet_id, "prev_end_slot" => current_end_slot, "new_end_slot" => end_slot, - "subscription_kind" => ?subscription_kind, ); - subscriptions.insert_at(subnet_id, end_slot, time_to_subscription_end); + self.subscriptions + .insert_at(subnet_id, end_slot, time_to_subscription_end); } } None => { // This is a new subscription. Add with the corresponding timeout and send the // notification. - subscriptions.insert_at(subnet_id, end_slot, time_to_subscription_end); + self.subscriptions + .insert_at(subnet_id, end_slot, time_to_subscription_end); // Inform of the subscription. - if !already_subscribed_as_other_kind { - debug!(self.log, "Subscribing to subnet"; - "subnet" => ?subnet_id, - "end_slot" => end_slot, - "subscription_kind" => ?subscription_kind, - ); - self.queue_event(SubnetServiceMessage::Subscribe(Subnet::Attestation( - subnet_id, - ))); - } + debug!(self.log, "Subscribing to subnet"; + "subnet" => ?subnet_id, + "end_slot" => end_slot, + ); + self.queue_event(SubnetServiceMessage::Subscribe(Subnet::Attestation( + subnet_id, + ))); } } @@ -575,26 +589,14 @@ impl AttestationService { // Unsubscribes from a subnet that was removed if it does not continue to exist as a // subscription of the other kind. For long lived subscriptions, it also removes the // advertisement from our ENR. - fn handle_removed_subnet(&mut self, subnet_id: SubnetId, subscription_kind: SubscriptionKind) { - let exists_in_other_subscriptions = match subscription_kind { - SubscriptionKind::LongLived => self.short_lived_subscriptions.contains_key(&subnet_id), - SubscriptionKind::ShortLived => self.long_lived_subscriptions.contains(&subnet_id), - }; - - if !exists_in_other_subscriptions { + fn handle_removed_subnet(&mut self, subnet_id: SubnetId) { + if !self.subscriptions.contains_key(&subnet_id) { // Subscription no longer exists as short lived or long lived. - debug!(self.log, "Unsubscribing from subnet"; "subnet" => ?subnet_id, "subscription_kind" => ?subscription_kind); + debug!(self.log, "Unsubscribing from subnet"; "subnet" => ?subnet_id); self.queue_event(SubnetServiceMessage::Unsubscribe(Subnet::Attestation( subnet_id, ))); } - - if subscription_kind == SubscriptionKind::LongLived { - // Remove from our ENR even if we remain subscribed in other way. - self.queue_event(SubnetServiceMessage::EnrRemove(Subnet::Attestation( - subnet_id, - ))); - } } } @@ -616,28 +618,11 @@ impl Stream for AttestationService { return Poll::Ready(Some(event)); } - // If we aren't subscribed to all subnets, handle the deterministic long-lived subnets - if !self.subscribe_all_subnets { - match self.next_long_lived_subscription_event.as_mut().poll(cx) { - Poll::Ready(_) => { - self.recompute_long_lived_subnets(); - // We re-wake the task as there could be other subscriptions to process - self.waker - .as_ref() - .expect("Waker has been set") - .wake_by_ref(); - } - Poll::Pending => {} - } - } - // Process scheduled subscriptions that might be ready, since those can extend a soon to // expire subscription. - match self.scheduled_short_lived_subscriptions.poll_next_unpin(cx) { + match self.scheduled_subscriptions.poll_next_unpin(cx) { Poll::Ready(Some(Ok(ExactSubnet { subnet_id, slot }))) => { - if let Err(e) = - self.subscribe_to_short_lived_subnet_immediately(subnet_id, slot + 1) - { + if let Err(e) = self.subscribe_to_subnet_immediately(subnet_id, slot + 1) { debug!(self.log, "Failed to subscribe to short lived subnet"; "subnet" => ?subnet_id, "err" => e); } self.waker @@ -651,10 +636,10 @@ impl Stream for AttestationService { Poll::Ready(None) | Poll::Pending => {} } - // Finally process any expired subscriptions. - match self.short_lived_subscriptions.poll_next_unpin(cx) { + // Process any expired subscriptions. + match self.subscriptions.poll_next_unpin(cx) { Poll::Ready(Some(Ok((subnet_id, _end_slot)))) => { - self.handle_removed_subnet(subnet_id, SubscriptionKind::ShortLived); + self.handle_removed_subnet(subnet_id); // We re-wake the task as there could be other subscriptions to process self.waker .as_ref() diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index b0346a14ef8..bdd02014c5d 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -198,7 +198,6 @@ pub struct ChainSpec { pub target_aggregators_per_committee: u64, pub gossip_max_size: u64, pub max_request_blocks: u64, - pub epochs_per_subnet_subscription: u64, pub min_epochs_for_block_requests: u64, pub max_chunk_size: u64, pub ttfb_timeout: u64, @@ -209,9 +208,7 @@ pub struct ChainSpec { pub message_domain_valid_snappy: [u8; 4], pub subnets_per_node: u8, pub attestation_subnet_count: u64, - pub attestation_subnet_extra_bits: u8, pub attestation_subnet_prefix_bits: u8, - pub attestation_subnet_shuffling_prefix_bits: u8, /* * Networking Deneb @@ -761,7 +758,6 @@ impl ChainSpec { subnets_per_node: 2, maximum_gossip_clock_disparity_millis: default_maximum_gossip_clock_disparity_millis(), target_aggregators_per_committee: 16, - epochs_per_subnet_subscription: default_epochs_per_subnet_subscription(), gossip_max_size: default_gossip_max_size(), min_epochs_for_block_requests: default_min_epochs_for_block_requests(), max_chunk_size: default_max_chunk_size(), @@ -769,10 +765,7 @@ impl ChainSpec { resp_timeout: default_resp_timeout(), message_domain_invalid_snappy: default_message_domain_invalid_snappy(), message_domain_valid_snappy: default_message_domain_valid_snappy(), - attestation_subnet_extra_bits: default_attestation_subnet_extra_bits(), attestation_subnet_prefix_bits: default_attestation_subnet_prefix_bits(), - attestation_subnet_shuffling_prefix_bits: - default_attestation_subnet_shuffling_prefix_bits(), max_request_blocks: default_max_request_blocks(), /* @@ -1063,7 +1056,6 @@ impl ChainSpec { subnets_per_node: 4, // Make this larger than usual to avoid network damage maximum_gossip_clock_disparity_millis: default_maximum_gossip_clock_disparity_millis(), target_aggregators_per_committee: 16, - epochs_per_subnet_subscription: default_epochs_per_subnet_subscription(), gossip_max_size: default_gossip_max_size(), min_epochs_for_block_requests: 33024, max_chunk_size: default_max_chunk_size(), @@ -1071,11 +1063,8 @@ impl ChainSpec { resp_timeout: default_resp_timeout(), message_domain_invalid_snappy: default_message_domain_invalid_snappy(), message_domain_valid_snappy: default_message_domain_valid_snappy(), - attestation_subnet_extra_bits: default_attestation_subnet_extra_bits(), - attestation_subnet_prefix_bits: default_attestation_subnet_prefix_bits(), - attestation_subnet_shuffling_prefix_bits: - default_attestation_subnet_shuffling_prefix_bits(), max_request_blocks: default_max_request_blocks(), + attestation_subnet_prefix_bits: default_attestation_subnet_prefix_bits(), /* * Networking Deneb Specific @@ -1227,9 +1216,6 @@ pub struct Config { #[serde(default = "default_max_request_blocks")] #[serde(with = "serde_utils::quoted_u64")] max_request_blocks: u64, - #[serde(default = "default_epochs_per_subnet_subscription")] - #[serde(with = "serde_utils::quoted_u64")] - epochs_per_subnet_subscription: u64, #[serde(default = "default_min_epochs_for_block_requests")] #[serde(with = "serde_utils::quoted_u64")] min_epochs_for_block_requests: u64, @@ -1254,15 +1240,9 @@ pub struct Config { #[serde(default = "default_message_domain_valid_snappy")] #[serde(with = "serde_utils::bytes_4_hex")] message_domain_valid_snappy: [u8; 4], - #[serde(default = "default_attestation_subnet_extra_bits")] - #[serde(with = "serde_utils::quoted_u8")] - attestation_subnet_extra_bits: u8, #[serde(default = "default_attestation_subnet_prefix_bits")] #[serde(with = "serde_utils::quoted_u8")] attestation_subnet_prefix_bits: u8, - #[serde(default = "default_attestation_subnet_shuffling_prefix_bits")] - #[serde(with = "serde_utils::quoted_u8")] - attestation_subnet_shuffling_prefix_bits: u8, #[serde(default = "default_max_request_blocks_deneb")] #[serde(with = "serde_utils::quoted_u64")] max_request_blocks_deneb: u64, @@ -1332,6 +1312,10 @@ fn default_subnets_per_node() -> u8 { 2u8 } +fn default_attestation_subnet_prefix_bits() -> u8 { + 6 +} + const fn default_max_per_epoch_activation_churn_limit() -> u64 { 8 } @@ -1364,18 +1348,6 @@ const fn default_message_domain_valid_snappy() -> [u8; 4] { [1, 0, 0, 0] } -const fn default_attestation_subnet_extra_bits() -> u8 { - 0 -} - -const fn default_attestation_subnet_prefix_bits() -> u8 { - 6 -} - -const fn default_attestation_subnet_shuffling_prefix_bits() -> u8 { - 3 -} - const fn default_max_request_blocks() -> u64 { 1024 } @@ -1404,10 +1376,6 @@ const fn default_max_per_epoch_activation_exit_churn_limit() -> u64 { 256_000_000_000 } -const fn default_epochs_per_subnet_subscription() -> u64 { - 256 -} - const fn default_attestation_propagation_slot_range() -> u64 { 32 } @@ -1548,6 +1516,7 @@ impl Config { shard_committee_period: spec.shard_committee_period, eth1_follow_distance: spec.eth1_follow_distance, subnets_per_node: spec.subnets_per_node, + attestation_subnet_prefix_bits: spec.attestation_subnet_prefix_bits, inactivity_score_bias: spec.inactivity_score_bias, inactivity_score_recovery_rate: spec.inactivity_score_recovery_rate, @@ -1564,7 +1533,6 @@ impl Config { gossip_max_size: spec.gossip_max_size, max_request_blocks: spec.max_request_blocks, - epochs_per_subnet_subscription: spec.epochs_per_subnet_subscription, min_epochs_for_block_requests: spec.min_epochs_for_block_requests, max_chunk_size: spec.max_chunk_size, ttfb_timeout: spec.ttfb_timeout, @@ -1573,9 +1541,6 @@ impl Config { maximum_gossip_clock_disparity_millis: spec.maximum_gossip_clock_disparity_millis, message_domain_invalid_snappy: spec.message_domain_invalid_snappy, message_domain_valid_snappy: spec.message_domain_valid_snappy, - attestation_subnet_extra_bits: spec.attestation_subnet_extra_bits, - attestation_subnet_prefix_bits: spec.attestation_subnet_prefix_bits, - attestation_subnet_shuffling_prefix_bits: spec.attestation_subnet_shuffling_prefix_bits, max_request_blocks_deneb: spec.max_request_blocks_deneb, max_request_blob_sidecars: spec.max_request_blob_sidecars, min_epochs_for_blob_sidecars_requests: spec.min_epochs_for_blob_sidecars_requests, @@ -1623,6 +1588,7 @@ impl Config { shard_committee_period, eth1_follow_distance, subnets_per_node, + attestation_subnet_prefix_bits, inactivity_score_bias, inactivity_score_recovery_rate, ejection_balance, @@ -1640,11 +1606,7 @@ impl Config { resp_timeout, message_domain_invalid_snappy, message_domain_valid_snappy, - attestation_subnet_extra_bits, - attestation_subnet_prefix_bits, - attestation_subnet_shuffling_prefix_bits, max_request_blocks, - epochs_per_subnet_subscription, attestation_propagation_slot_range, maximum_gossip_clock_disparity_millis, max_request_blocks_deneb, @@ -1702,11 +1664,8 @@ impl Config { resp_timeout, message_domain_invalid_snappy, message_domain_valid_snappy, - attestation_subnet_extra_bits, attestation_subnet_prefix_bits, - attestation_subnet_shuffling_prefix_bits, max_request_blocks, - epochs_per_subnet_subscription, attestation_propagation_slot_range, maximum_gossip_clock_disparity_millis, max_request_blocks_deneb, @@ -1991,9 +1950,7 @@ mod yaml_tests { check_default!(resp_timeout); check_default!(message_domain_invalid_snappy); check_default!(message_domain_valid_snappy); - check_default!(attestation_subnet_extra_bits); check_default!(attestation_subnet_prefix_bits); - check_default!(attestation_subnet_shuffling_prefix_bits); assert_eq!(chain_spec.bellatrix_fork_epoch, None); } diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 9b6a2e6a192..02644033a8e 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -1,9 +1,8 @@ //! Identifies each shard by an integer identifier. -use crate::{AttestationData, ChainSpec, CommitteeIndex, Epoch, EthSpec, Slot}; +use crate::{AttestationData, ChainSpec, CommitteeIndex, EthSpec, Slot}; use safe_arith::{ArithError, SafeArith}; use serde::{Deserialize, Serialize}; use std::ops::{Deref, DerefMut}; -use swap_or_not_shuffle::compute_shuffled_index; const MAX_SUBNET_ID: usize = 64; @@ -74,46 +73,15 @@ impl SubnetId { /// 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. - #[allow(clippy::arithmetic_side_effects)] - pub fn compute_subnets_for_epoch( + pub fn compute_attestation_subnets( node_id: ethereum_types::U256, - epoch: Epoch, spec: &ChainSpec, - ) -> Result<(impl Iterator, Epoch), &'static str> { - // simplify variable naming - let subscription_duration = spec.epochs_per_subnet_subscription; + ) -> impl Iterator { + // The bits of the node-id we are using to define the subnets. let prefix_bits = spec.attestation_subnet_prefix_bits as u64; - let shuffling_prefix_bits = spec.attestation_subnet_shuffling_prefix_bits as u64; // calculate the prefixes used to compute the subnet and shuffling let node_id_prefix = (node_id >> (256 - prefix_bits)).as_u64(); - let shuffling_prefix = (node_id >> (256 - (prefix_bits + shuffling_prefix_bits))).as_u64(); - - // number of groups the shuffling creates - let shuffling_groups = 1 << shuffling_prefix_bits; - // shuffling group for this node - let shuffling_bits = shuffling_prefix % shuffling_groups; - let epoch_transition = (node_id_prefix - + (shuffling_bits * (subscription_duration >> shuffling_prefix_bits))) - % subscription_duration; - - // Calculate at which epoch this node needs to re-evaluate - let valid_until_epoch = epoch.as_u64() - + subscription_duration - .saturating_sub((epoch.as_u64() + epoch_transition) % subscription_duration); - - let subscription_event_idx = (epoch.as_u64() + epoch_transition) / subscription_duration; - let permutation_seed = - ethereum_hashing::hash(&int_to_bytes::int_to_bytes8(subscription_event_idx)); - - let num_subnets = 1 << spec.attestation_subnet_prefix_bits; - let permutated_prefix = compute_shuffled_index( - node_id_prefix as usize, - num_subnets, - &permutation_seed, - spec.shuffle_round_count, - ) - .ok_or("Unable to shuffle")? as u64; // Get the constants we need to avoid holding a reference to the spec let &ChainSpec { @@ -123,9 +91,9 @@ impl SubnetId { } = spec; let subnet_set_generator = (0..subnets_per_node).map(move |idx| { - SubnetId::new((permutated_prefix + idx as u64) % attestation_subnet_count) + SubnetId::new((node_id_prefix + idx as u64) % attestation_subnet_count) }); - Ok((subnet_set_generator, valid_until_epoch.into())) + subnet_set_generator } } @@ -173,7 +141,7 @@ mod tests { /// A set of tests compared to the python specification #[test] - fn compute_subnets_for_epoch_unit_test() { + fn compute_attestation_subnets_test() { // Randomized variables used generated with the python specification let node_ids = [ "0", @@ -182,59 +150,35 @@ mod tests { "27726842142488109545414954493849224833670205008410190955613662332153332462900", "39755236029158558527862903296867805548949739810920318269566095185775868999998", "31899136003441886988955119620035330314647133604576220223892254902004850516297", - "58579998103852084482416614330746509727562027284701078483890722833654510444626", - "28248042035542126088870192155378394518950310811868093527036637864276176517397", - "60930578857433095740782970114409273483106482059893286066493409689627770333527", - "103822458477361691467064888613019442068586830412598673713899771287914656699997", ] .map(|v| ethereum_types::U256::from_dec_str(v).unwrap()); - let epochs = [ - 54321u64, 1017090249, 1827566880, 846255942, 766597383, 1204990115, 1616209495, - 1774367616, 1484598751, 3525502229, - ] - .map(Epoch::from); + let expected_subnets = [ + vec![0, 1], + vec![49u64, 50u64], + vec![10, 11], + vec![15, 16], + vec![21, 22], + vec![17, 18], + ]; // Test mainnet let spec = ChainSpec::mainnet(); - // Calculated by hand - let expected_valid_time = [ - 54528u64, 1017090255, 1827567030, 846256049, 766597387, 1204990287, 1616209536, - 1774367857, 1484598847, 3525502311, - ]; - - // Calculated from pyspec - let expected_subnets = [ - vec![4u64, 5u64], - vec![31, 32], - vec![39, 40], - vec![38, 39], - vec![53, 54], - vec![57, 58], - vec![48, 49], - vec![1, 2], - vec![34, 35], - vec![37, 38], - ]; - for x in 0..node_ids.len() { println!("Test: {}", x); println!( - "NodeId: {}\n Epoch: {}\n, expected_update_time: {}\n, expected_subnets: {:?}", - node_ids[x], epochs[x], expected_valid_time[x], expected_subnets[x] + "NodeId: {}\nExpected_subnets: {:?}", + node_ids[x], expected_subnets[x] ); - let (computed_subnets, valid_time) = SubnetId::compute_subnets_for_epoch::< - crate::MainnetEthSpec, - >(node_ids[x], epochs[x], &spec) - .unwrap(); + let computed_subnets = + SubnetId::compute_attestation_subnets::(node_ids[x], &spec); assert_eq!( expected_subnets[x], computed_subnets.map(SubnetId::into).collect::>() ); - assert_eq!(Epoch::from(expected_valid_time[x]), valid_time); } } } From bd5a75388079b5f3848bc4189b0a89baa878e8ca Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 16 Jul 2024 16:51:22 +1000 Subject: [PATCH 02/10] First draft without tests --- Cargo.lock | 25 +- Cargo.toml | 4 +- beacon_node/network/src/service.rs | 90 +-- .../src/subnet_service/attestation_subnets.rs | 672 ----------------- beacon_node/network/src/subnet_service/mod.rs | 678 +++++++++++++++++- .../src/subnet_service/sync_subnets.rs | 359 ---------- .../network/src/subnet_service/tests/mod.rs | 16 +- consensus/types/src/subnet_id.rs | 7 +- 8 files changed, 716 insertions(+), 1135 deletions(-) delete mode 100644 beacon_node/network/src/subnet_service/attestation_subnets.rs delete mode 100644 beacon_node/network/src/subnet_service/sync_subnets.rs diff --git a/Cargo.lock b/Cargo.lock index 6bbb79edcb6..3a7096760de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1861,6 +1861,17 @@ dependencies = [ "tokio-util", ] +[[package]] +name = "delay_map" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df941644b671f05f59433e481ba0d31ac10e3667de725236a4c0d587c496fba1" +dependencies = [ + "futures", + "tokio", + "tokio-util", +] + [[package]] name = "deposit_contract" version = "0.2.0" @@ -2080,7 +2091,7 @@ dependencies = [ "aes 0.7.5", "aes-gcm 0.9.2", "arrayvec", - "delay_map", + "delay_map 0.3.0", "enr", "fnv", "futures", @@ -4947,7 +4958,7 @@ version = "0.2.0" dependencies = [ "async-channel", "bytes", - "delay_map", + "delay_map 0.4.0", "directory", "dirs", "discv5", @@ -5508,7 +5519,7 @@ dependencies = [ "async-channel", "beacon_chain", "beacon_processor", - "delay_map", + "delay_map 0.4.0", "derivative", "error-chain", "eth2", @@ -8409,9 +8420,9 @@ checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" [[package]] name = "tokio" -version = "1.37.0" +version = "1.38.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1adbebffeca75fcfd058afa480fb6c0b81e165a0323f9c9d39c9697e37c46787" +checksum = "ba4f4a02a7a80d6f274636f0aa95c7e383b912d41fe721a31f29e29698585a4a" dependencies = [ "backtrace", "bytes", @@ -8437,9 +8448,9 @@ dependencies = [ [[package]] name = "tokio-macros" -version = "2.2.0" +version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b8a1e28f2deaa14e508979454cb3a223b10b938b45af148bc0986de36f1923b" +checksum = "5f5ae998a069d4b5aba8ee9dad856af7d520c3699e6159b185c2acd48155d39a" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index c09a3af7ce4..f6239365527 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -108,7 +108,7 @@ clap = { version = "4.5.4", features = ["cargo", "wrap_help"] } c-kzg = { version = "1", default-features = false } compare_fields_derive = { path = "common/compare_fields_derive" } criterion = "0.5" -delay_map = "0.3" +delay_map = "0.4" derivative = "2" dirs = "3" either = "1.9" @@ -154,7 +154,7 @@ serde_json = "1" serde_repr = "0.1" serde_yaml = "0.9" sha2 = "0.9" -slog = { version = "2", features = ["max_level_trace", "release_max_level_trace", "nested-values"] } +slog = { version = "2", features = ["max_level_debug", "release_max_level_debug", "nested-values"] } slog-async = "2" slog-term = "2" sloggers = { version = "2", features = ["json"] } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index e215f25387b..c659cb52b43 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -3,12 +3,9 @@ use crate::nat; use crate::network_beacon_processor::InvalidBlockStorage; use crate::persisted_dht::{clear_dht, load_dht, persist_dht}; use crate::router::{Router, RouterMessage}; -use crate::subnet_service::SyncCommitteeService; +use crate::subnet_service::{SubnetService, SubnetServiceMessage, Subscription}; +use crate::NetworkConfig; use crate::{error, metrics}; -use crate::{ - subnet_service::{AttestationService, SubnetServiceMessage}, - NetworkConfig, -}; use beacon_chain::{BeaconChain, BeaconChainTypes}; use beacon_processor::{work_reprocessing_queue::ReprocessQueueMessage, BeaconProcessorSend}; use futures::channel::mpsc::Sender; @@ -170,9 +167,7 @@ pub struct NetworkService { /// The underlying libp2p service that drives all the network interactions. libp2p: Network, /// An attestation and subnet manager service. - attestation_service: AttestationService, - /// A sync committeee subnet manager service. - sync_committee_service: SyncCommitteeService, + subnet_service: SubnetService, /// The receiver channel for lighthouse to communicate with the network service. network_recv: mpsc::UnboundedReceiver>, /// The receiver channel for lighthouse to send validator subscription requests. @@ -319,16 +314,13 @@ impl NetworkService { network_log.clone(), )?; - // attestation subnet service - let attestation_service = AttestationService::new( + // attestation and sync committee subnet service + let subnet_service = SubnetService::new( beacon_chain.clone(), network_globals.local_enr().node_id(), config, &network_log, ); - // sync committee subnet service - let sync_committee_service = - SyncCommitteeService::new(beacon_chain.clone(), config, &network_log); // create a timer for updating network metrics let metrics_update = tokio::time::interval(Duration::from_secs(METRIC_UPDATE_INTERVAL)); @@ -346,8 +338,7 @@ impl NetworkService { let network_service = NetworkService { beacon_chain, libp2p, - attestation_service, - sync_committee_service, + subnet_service, network_recv, validator_subscription_recv, router_send, @@ -461,11 +452,8 @@ impl NetworkService { // handle a message from a validator requesting a subscription to a subnet Some(msg) = self.validator_subscription_recv.recv() => self.on_validator_subscription_msg(msg).await, - // process any attestation service events - Some(msg) = self.attestation_service.next() => self.on_attestation_service_msg(msg), - - // process any sync committee service events - Some(msg) = self.sync_committee_service.next() => self.on_sync_committee_service_message(msg), + // process any subnet service events + Some(msg) = self.subnet_service.next() => self.on_subnet_service_msg(msg), event = self.libp2p.next_event() => self.on_libp2p_event(event, &mut shutdown_sender).await, @@ -553,13 +541,14 @@ impl NetworkService { match message { // attestation information gets processed in the attestation service PubsubMessage::Attestation(ref subnet_and_attestation) => { - let subnet = subnet_and_attestation.0; + let subnet_id = subnet_and_attestation.0; let attestation = &subnet_and_attestation.1; // checks if we have an aggregator for the slot. If so, we should process // the attestation, else we just just propagate the Attestation. - let should_process = self - .attestation_service - .should_process_attestation(subnet, attestation); + let should_process = self.subnet_service.should_process_attestation( + Subnet::Attestation(subnet_id), + attestation, + ); self.send_to_router(RouterMessage::PubsubMessage( id, source, @@ -788,23 +777,17 @@ impl NetworkService { /// Handle a message sent to the network service. async fn on_validator_subscription_msg(&mut self, msg: ValidatorSubscriptionMessage) { - match msg { + if let Err(e) = match msg { ValidatorSubscriptionMessage::AttestationSubscribe { subscriptions } => { - if let Err(e) = self - .attestation_service - .validator_subscriptions(subscriptions.into_iter()) - { - warn!(self.log, "Attestation validator subscription failed"; "error" => e); - } + let subscriptions = subscriptions.into_iter().map(Subscription::Attestation); + self.subnet_service.validator_subscriptions(subscriptions) } ValidatorSubscriptionMessage::SyncCommitteeSubscribe { subscriptions } => { - if let Err(e) = self - .sync_committee_service - .validator_subscriptions(subscriptions) - { - warn!(self.log, "Sync committee calidator subscription failed"; "error" => e); - } + let subscriptions = subscriptions.into_iter().map(Subscription::SyncCommittee); + self.subnet_service.validator_subscriptions(subscriptions) } + } { + warn!(self.log, "Validator subscription failed"; "error" => e); } } @@ -839,35 +822,7 @@ impl NetworkService { } } - fn on_attestation_service_msg(&mut self, msg: SubnetServiceMessage) { - match msg { - SubnetServiceMessage::Subscribe(subnet) => { - for fork_digest in self.required_gossip_fork_digests() { - let topic = - GossipTopic::new(subnet.into(), GossipEncoding::default(), fork_digest); - self.libp2p.subscribe(topic); - } - } - SubnetServiceMessage::Unsubscribe(subnet) => { - for fork_digest in self.required_gossip_fork_digests() { - let topic = - GossipTopic::new(subnet.into(), GossipEncoding::default(), fork_digest); - self.libp2p.unsubscribe(topic); - } - } - SubnetServiceMessage::EnrAdd(subnet) => { - self.libp2p.update_enr_subnet(subnet, true); - } - SubnetServiceMessage::EnrRemove(subnet) => { - self.libp2p.update_enr_subnet(subnet, false); - } - SubnetServiceMessage::DiscoverPeers(subnets_to_discover) => { - self.libp2p.discover_subnet_peers(subnets_to_discover); - } - } - } - - fn on_sync_committee_service_message(&mut self, msg: SubnetServiceMessage) { + fn on_subnet_service_msg(&mut self, msg: SubnetServiceMessage) { match msg { SubnetServiceMessage::Subscribe(subnet) => { for fork_digest in self.required_gossip_fork_digests() { @@ -886,9 +841,6 @@ impl NetworkService { SubnetServiceMessage::EnrAdd(subnet) => { self.libp2p.update_enr_subnet(subnet, true); } - SubnetServiceMessage::EnrRemove(subnet) => { - self.libp2p.update_enr_subnet(subnet, false); - } SubnetServiceMessage::DiscoverPeers(subnets_to_discover) => { self.libp2p.discover_subnet_peers(subnets_to_discover); } diff --git a/beacon_node/network/src/subnet_service/attestation_subnets.rs b/beacon_node/network/src/subnet_service/attestation_subnets.rs deleted file mode 100644 index a06b3a8d51b..00000000000 --- a/beacon_node/network/src/subnet_service/attestation_subnets.rs +++ /dev/null @@ -1,672 +0,0 @@ -//! This service keeps track of which shard subnet the beacon node should be subscribed to at any -//! given time. It schedules subscriptions to shard subnets, requests peer discoveries and -//! determines whether attestations should be aggregated and/or passed to the beacon node. - -use super::SubnetServiceMessage; -use std::collections::HashSet; -use std::collections::{HashMap, VecDeque}; -use std::pin::Pin; -use std::sync::Arc; -use std::task::{Context, Poll}; - -use beacon_chain::{BeaconChain, BeaconChainTypes}; -use delay_map::{HashMapDelay, HashSetDelay}; -use futures::prelude::*; -use lighthouse_network::{discv5::enr::NodeId, NetworkConfig, Subnet, SubnetDiscovery}; -use slog::{debug, error, info, o, trace, warn}; -use slot_clock::SlotClock; -use types::{ - Attestation, EthSpec, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, - ValidatorSubscription, -}; - -use crate::metrics; - -/// The minimum number of slots ahead that we attempt to discover peers for a subscription. If the -/// slot is less than this number, skip the peer discovery process. -/// Subnet discovery query takes at most 30 secs, 2 slots take 24s. -pub(crate) const MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD: u64 = 2; -/// The fraction of a slot that we subscribe to a subnet before the required slot. -/// -/// Currently a whole slot ahead. -const ADVANCE_SUBSCRIBE_SLOT_FRACTION: u32 = 1; - -/// The number of slots after an aggregator duty where we remove the entry from -/// `aggregate_validators_on_subnet` delay map. -const UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY: u32 = 2; - -/// A particular subnet at a given slot. -#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] -pub struct ExactSubnet { - /// The `SubnetId` associated with this subnet. - pub subnet_id: Subnet, - /// For Attestations, this slot represents the start time at which we need to subscribe to the - /// slot. For SyncCommittee subnet id's this represents the end slot at which we no longer need - /// to subscribe to the subnet. - // NOTE: There was different logic between the two subscriptions and having a different - // interpretation of this variable seemed like the best way to group the logic, even though it - // may be counter-intuitive (apologies to future readers). - pub slot: Slot, -} - -/// The enum used to group all kinds of validator subscriptions -pub enum Subscription { - Attestation(ValidatorSubscription), - SyncCommittee(SyncCommitteeSubscription), -} - -pub struct AttestationService { - /// Queued events to return to the driving service. - events: VecDeque, - - /// A reference to the beacon chain to process received attestations. - pub(crate) beacon_chain: Arc>, - - /// Subnets we are currently subscribed to as short lived subscriptions. - /// - /// Once they expire, we unsubscribe from these. - /// We subscribe to subnets when we are an aggregator for an exact subnet. - // NOTE: When setup the defalut timeout is set for sync committee subscriptions. - subscriptions: HashMapDelay, - - /// A list of permanent subnets that this node is subscribed to. - // TODO: Shift this to a dynamic bitfield - permanent_attestation_subscriptions: HashSet, - - /// A collection of timeouts for when to unsubscirbe from a sync committee subnet. - // sync_unsubscriptions: HashSetDelay, - - /// Subscriptions that need to be executed in the future. - scheduled_subscriptions: HashSetDelay, - - /// A collection timeouts to track the existence of aggregate validator subscriptions at an - /// `ExactSubnet`. - aggregate_validators_on_subnet: Option>, - - /// The waker for the current thread. - waker: Option, - - /// The discovery mechanism of lighthouse is disabled. - discovery_disabled: bool, - - /// We are always subscribed to all subnets. - subscribe_all_subnets: bool, - - /// Whether this node is a block proposer-only node. - proposer_only: bool, - - /// The logger for the attestation service. - log: slog::Logger, -} - -impl AttestationService { - /* Public functions */ - - /// Establish the service based on the passed configuration. - pub fn new( - beacon_chain: Arc>, - node_id: NodeId, - config: &NetworkConfig, - log: &slog::Logger, - ) -> Self { - let log = log.new(o!("service" => "attestation_service")); - - let slot_duration = beacon_chain.slot_clock.slot_duration(); - - if config.subscribe_all_subnets { - slog::info!(log, "Subscribing to all subnets"); - } - - // Build the list of known permanent subscriptions, so that we know not to subscribe or - // discover them. - let mut permanent_attestation_subscriptions = HashSet::default(); - if config.subscribe_all_subnets { - // We are subscribed to all subnets, set all the bits to true. - for index in 0..::SubnetBitfieldLength::to_u64() { - permanent_attestation_subscriptions.insert(SubnetId::from(index)); - } - } else { - // Not subscribed to all subnets, so just calculate the required subnets from the - for subnet_id in SubnetId::compute_attestation_subnets::( - node_id.raw().into(), - &beacon_chain.spec, - ) { - permanent_attestation_subscriptions.insert(subnet_id); - } - } - - // Set up the sync committee subscriptions - let spec = &beacon_chain.spec; - let epoch_duration_secs = - beacon_chain.slot_clock.slot_duration().as_secs() * T::EthSpec::slots_per_epoch(); - let default_sync_committee_duration = Duration::from_secs( - epoch_duration_secs.saturating_mul(spec.epochs_per_sync_committee_period.as_u64()), - ); - - let track_validators = !config.import_all_attestations; - let aggregate_validators_on_subnet = - track_validators.then(|| HashSetDelay::new(slot_duration)); - AttestationService { - events: VecDeque::with_capacity(10), - beacon_chain, - subscriptions: HashMapDelay::new(default_sync_committee_duration), - permanent_attestation_subscriptions, - scheduled_subscriptions: HashSetDelay::default(), - aggregate_validators_on_subnet, - waker: None, - discovery_disabled: config.disable_discovery, - subscribe_all_subnets: config.subscribe_all_subnets, - proposer_only: config.proposer_only, - log, - } - } - - /// Return count of all currently subscribed subnets (long-lived **and** short-lived). - #[cfg(test)] - pub fn subscription_count(&self) -> usize { - if self.subscribe_all_subnets { - self.beacon_chain.spec.attestation_subnet_count as usize - } else { - let count = self.subscriptions.keys().collect::>().len(); - count - } - } - - /// Return count of all currently subscribed sync committee subnets. - #[cfg(test)] - pub fn sync_committee_subscription_count(&self) -> usize { - use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; - if self.subscribe_all_subnets { - SYNC_COMMITTEE_SUBNET_COUNT as usize - } else { - self.subscriptions.len() - } - } - - /// Returns whether we are subscribed to a subnet for testing purposes. - #[cfg(test)] - pub(crate) fn is_subscribed(&self, subnet_id: &SubnetId) -> bool { - self.subscriptions - .contains_key(&Subnet::Attestation(subnet_id)) - } - - /// Processes a list of validator subscriptions. - /// - /// This is fundamentally called form the HTTP API when a validator requests duties from us - /// This will: - /// - Register new validators as being known. - /// - Search for peers for required subnets. - /// - Request subscriptions for subnets on specific slots when required. - /// - Build the timeouts for each of these events. - /// - /// 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> { - // If the node is in a proposer-only state, we ignore all subnet subscriptions. - if self.proposer_only { - return Ok(()); - } - - // Maps each subnet_id subscription to it's highest slot - let mut subnets_to_discover: HashMap = HashMap::new(); - - // Registers the validator with the attestation service. - for general_subscription in subscriptions { - match general_subscription { - Subscription::Attestation(subscription) => { - metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_REQUESTS); - - // Compute the subnet that is associated with this subscription - let subnet_id = match SubnetId::compute_subnet::( - subscription.slot, - subscription.attestation_committee_index, - subscription.committee_count_at_slot, - &self.beacon_chain.spec, - ) { - Ok(subnet_id) => Subnet::Attestation(subnet_id), - Err(e) => { - warn!(self.log, - "Failed to compute subnet id for validator subscription"; - "error" => ?e, - ); - continue; - } - }; - - // Ensure each subnet_id inserted into the map has the highest slot as it's value. - // Higher slot corresponds to higher min_ttl in the `SubnetDiscovery` entry. - if let Some(slot) = subnets_to_discover.get(&subnet_id) { - if subscription.slot > *slot { - subnets_to_discover.insert(subnet_id, subscription.slot); - } - } else if !self.discovery_disabled { - subnets_to_discover.insert(subnet_id, subscription.slot); - } - - let exact_subnet = ExactSubnet { - subnet_id, - slot: subscription.slot, - }; - - // Determine if the validator is an aggregator. If so, we subscribe to the subnet and - // if successful add the validator to a mapping of known aggregators for that exact - // subnet. - - if subscription.is_aggregator { - metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_AGGREGATOR_REQUESTS); - if let Err(e) = self.subscribe_to_subnet(exact_subnet) { - warn!(self.log, - "Subscription to subnet error"; - "error" => e, - ); - } - } - } - Subscription::SyncCommittee(subscription) => { - metrics::inc_counter(&metrics::SYNC_COMMITTEE_SUBSCRIPTION_REQUESTS); - // NOTE: We assume all subscriptions have been verified before reaching this service - - // Registers the validator with the subnet service. - trace!(self.log, - "Sync committee subscription"; - "subscription" => ?subscription, - ); - - let subnet_ids = - match SyncSubnetId::compute_subnets_for_sync_committee::( - &subscription.sync_committee_indices, - ) { - Ok(subnet_ids) => subnet_ids, - Err(e) => { - warn!(self.log, - "Failed to compute subnet id for sync committee subscription"; - "error" => ?e, - "validator_index" => subscription.validator_index - ); - continue; - } - }; - - for subnet_id in subnet_ids { - let exact_subnet = ExactSubnet { - subnet_id: Subnet::SyncCommittee(subnet_id), - slot: subscription - .until_epoch - .start_slot(T::EthSpec::slots_per_epoch()), - }; - subnets_to_discover.push(exact_subnet.clone()); - if let Err(e) = self.subscribe_to_sync_subnet(exact_subnet.subnet_id) { - warn!(self.log, - "Subscription to sync subnet error"; - "error" => e, - "validator_index" => subscription.validator_index, - ); - } else { - trace!(self.log, - "Subscribed to subnet for sync committee duties"; - "exact_subnet" => ?exact_subnet, - "validator_index" => subscription.validator_index - ); - } - } - } - } - } - - // If the discovery mechanism isn't disabled, attempt to set up a peer discovery for the - // required subnets. - if !self.discovery_disabled { - if let Err(e) = self.discover_peers_request(subnets_to_discover.into_iter()) { - 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 - /// verification, re-propagates and returns false. - pub fn should_process_attestation( - &self, - subnet: SubnetId, - attestation: &Attestation, - ) -> bool { - // Proposer-only mode does not need to process attestations - if self.proposer_only { - return false; - } - self.aggregate_validators_on_subnet - .as_ref() - .map(|tracked_vals| { - tracked_vals.contains_key(&ExactSubnet { - subnet_id: subnet, - slot: attestation.data().slot, - }) - }) - .unwrap_or(true) - } - - /* Internal private functions */ - - /// Adds an event to the event queue and notifies that this service is ready to be polled - /// again. - fn queue_event(&mut self, ev: SubnetServiceMessage) { - self.events.push_back(ev); - if let Some(waker) = &self.waker { - waker.wake_by_ref() - } - } - /// Checks if there are currently queued discovery requests and the time required to make the - /// 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, - subnets_to_discover: impl Iterator, - ) -> Result<(), &'static str> { - let current_slot = self - .beacon_chain - .slot_clock - .now() - .ok_or("Could not get the current slot")?; - - let discovery_subnets: Vec = subnets_to_discover - .filter_map(|(subnet, relevant_slot)| { - // We generate discovery requests for all subnets (even one's we are permenantly - // subscribed to) in order to ensure our peer counts are satisfactory to perform the - // necessary duties. - - // Check if there is enough time to perform a discovery lookup. - if relevant_slot >= current_slot.saturating_add(MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD) - { - // Send out an event to start looking for peers. - // Require the peer for an additional slot to ensure we keep the peer for the - // duration of the subscription. - let min_ttl = self - .beacon_chain - .slot_clock - .duration_to_slot(relevant_slot + 1) - .map(|duration| std::time::Instant::now() + duration); - Some(SubnetDiscovery { subnet, min_ttl }) - } else { - // We may want to check the global PeerInfo to see estimated timeouts for each - // peer before they can be removed. - warn!(self.log, - "Not enough time for a discovery search"; - "subnet_id" => ?subnet, - ); - None - } - }) - .collect(); - - if !discovery_subnets.is_empty() { - self.queue_event(SubnetServiceMessage::DiscoverPeers(discovery_subnets)); - } - Ok(()) - } - - // Subscribes to the subnet if it should be done immediately, or schedules it if required. - fn subscribe_to_subnet( - &mut self, - ExactSubnet { subnet_id, slot }: ExactSubnet, - ) -> Result<(), &'static str> { - // If the subnet is one of our permanent subnets, we do not need to subscribe. - if self.subscribe_all_subnets - || self - .permanent_attestation_subscriptions - .contains(&subnet_id) - { - return Ok(()); - } - - let slot_duration = self.beacon_chain.slot_clock.slot_duration(); - - // The short time we schedule the subscription before it's actually required. This - // ensures we are subscribed on time, and allows consecutive subscriptions to the same - // subnet to overlap, reducing subnet churn. - let advance_subscription_duration = slot_duration / ADVANCE_SUBSCRIBE_SLOT_FRACTION; - // The time to the required slot. - let time_to_subscription_slot = self - .beacon_chain - .slot_clock - .duration_to_slot(slot) - .unwrap_or_default(); // If this is a past slot we will just get a 0 duration. - - // Calculate how long before we need to subscribe to the subnet. - let time_to_subscription_start = - time_to_subscription_slot.saturating_sub(advance_subscription_duration); - - // The time after a duty slot where we no longer need it in the `aggregate_validators_on_subnet` - // delay map. - let time_to_unsubscribe = - time_to_subscription_slot + UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY * slot_duration; - if let Some(tracked_vals) = self.aggregate_validators_on_subnet.as_mut() { - tracked_vals.insert_at(ExactSubnet { subnet_id, slot }, time_to_unsubscribe); - } - - // If the subscription should be done in the future, schedule it. Otherwise subscribe - // immediately. - if time_to_subscription_start.is_zero() { - // This is a current or past slot, we subscribe immediately. - self.subscribe_to_subnet_immediately(subnet_id, slot + 1)?; - } else { - // This is a future slot, schedule subscribing. - trace!(self.log, "Scheduling subnet subscription"; "subnet" => ?subnet_id, "time_to_subscription_start" => ?time_to_subscription_start); - self.scheduled_subscriptions - .insert_at(ExactSubnet { subnet_id, slot }, time_to_subscription_start); - } - - Ok(()) - } - - /// Adds a subscription event and an associated unsubscription event if required. - fn subscribe_to_sync_subnet(&mut self, exact_subnet: ExactSubnet) -> Result<(), &'static str> { - // Return if we have subscribed to all subnets - if self.subscribe_all_subnets { - return Ok(()); - } - - // Return if we already have a subscription for the subnet and its closer or - if let Some(until_slot) = self.subscriptions.get(exact_subnet.subnet_id) { - if until_slot >= *exact_subnet.slot { - return Ok(()); - } - } - - // Initialise timing variables - let current_slot = self - .beacon_chain - .slot_clock - .now() - .ok_or("Could not get the current slot")?; - - // Calculate the duration to the unsubscription event. - let expected_end_subscription_duration = if current_slot >= exact_subnet.slot { - warn!( - self.log, - "Sync committee subscription is past expiration"; - "current_slot" => current_slot, - "exact_subnet" => ?exact_subnet, - ); - return Ok(()); - } else { - let slot_duration = self.beacon_chain.slot_clock.slot_duration(); - - // the duration until we no longer need this subscription. We assume a single slot is - // sufficient. - self.beacon_chain - .slot_clock - .duration_to_slot(exact_subnet.slot) - .ok_or("Unable to determine duration to unsubscription slot")? - + slot_duration - }; - - if !self - .subscriptions - .insert_at(exact_subnet.subnet_id, expected_end_subscription_duration) - { - // We are not currently subscribed and have no waiting subscription, create one - debug!(self.log, "Subscribing to subnet"; "subnet" => *exact_subnet.subnet_id, "until_epoch" => ?exact_subnet.slot); - self.events - .push_back(SubnetServiceMessage::Subscribe(Subnet::SyncCommittee( - exact_subnet.subnet_id, - ))); - - // add the subnet to the ENR bitfield - self.events - .push_back(SubnetServiceMessage::EnrAdd(Subnet::SyncCommittee( - exact_subnet.subnet_id, - ))); - } - Ok(()) - } - - /* A collection of functions that handle the various timeouts */ - - /// Registers a subnet as subscribed. - /// - /// Checks that the time in which the subscription would end is not in the past. If we are - /// already subscribed, extends the timeout if necessary. If this is a new subscription, we send - /// out the appropriate events. - /// - /// On determinist long lived subnets, this is only used for short lived subscriptions. - fn subscribe_to_subnet_immediately( - &mut self, - subnet_id: SubnetId, - end_slot: Slot, - ) -> Result<(), &'static str> { - if self.subscribe_all_subnets { - // Case not handled by this service. - return Ok(()); - } - - let time_to_subscription_end = self - .beacon_chain - .slot_clock - .duration_to_slot(end_slot) - .unwrap_or_default(); - - // First check this is worth doing. - if time_to_subscription_end.is_zero() { - return Err("Time when subscription would end has already passed."); - } - - // We need to check and add a subscription for the right kind, regardless of the presence - // of the subnet as a subscription of the other kind. This is mainly since long lived - // subscriptions can be removed at any time when a validator goes offline. - - match self.subscriptions.get(&subnet_id) { - Some(current_end_slot) => { - // We are already subscribed. Check if we need to extend the subscription. - if &end_slot > current_end_slot { - trace!(self.log, "Extending subscription to subnet"; - "subnet" => ?subnet_id, - "prev_end_slot" => current_end_slot, - "new_end_slot" => end_slot, - ); - self.subscriptions - .insert_at(subnet_id, end_slot, time_to_subscription_end); - } - } - None => { - // This is a new subscription. Add with the corresponding timeout and send the - // notification. - self.subscriptions - .insert_at(subnet_id, end_slot, time_to_subscription_end); - - // Inform of the subscription. - debug!(self.log, "Subscribing to subnet"; - "subnet" => ?subnet_id, - "end_slot" => end_slot, - ); - self.queue_event(SubnetServiceMessage::Subscribe(Subnet::Attestation( - subnet_id, - ))); - } - } - - Ok(()) - } - - // Unsubscribes from a subnet that was removed if it does not continue to exist as a - // subscription of the other kind. For long lived subscriptions, it also removes the - // advertisement from our ENR. - fn handle_removed_subnet(&mut self, subnet_id: SubnetId) { - if !self.subscriptions.contains_key(&subnet_id) { - // Subscription no longer exists as short lived or long lived. - debug!(self.log, "Unsubscribing from subnet"; "subnet" => ?subnet_id); - self.queue_event(SubnetServiceMessage::Unsubscribe(Subnet::Attestation( - subnet_id, - ))); - } - } -} - -impl Stream for AttestationService { - type Item = SubnetServiceMessage; - - fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - // Update the waker if needed. - if let Some(waker) = &self.waker { - if waker.will_wake(cx.waker()) { - self.waker = Some(cx.waker().clone()); - } - } else { - self.waker = Some(cx.waker().clone()); - } - - // Send out any generated events. - if let Some(event) = self.events.pop_front() { - return Poll::Ready(Some(event)); - } - - // Process scheduled subscriptions that might be ready, since those can extend a soon to - // expire subscription. - match self.scheduled_subscriptions.poll_next_unpin(cx) { - Poll::Ready(Some(Ok(ExactSubnet { subnet_id, slot }))) => { - if let Err(e) = self.subscribe_to_subnet_immediately(subnet_id, slot + 1) { - debug!(self.log, "Failed to subscribe to short lived subnet"; "subnet" => ?subnet_id, "err" => e); - } - self.waker - .as_ref() - .expect("Waker has been set") - .wake_by_ref(); - } - Poll::Ready(Some(Err(e))) => { - error!(self.log, "Failed to check for scheduled subnet subscriptions"; "error"=> e); - } - Poll::Ready(None) | Poll::Pending => {} - } - - // Process any expired subscriptions. - match self.subscriptions.poll_next_unpin(cx) { - Poll::Ready(Some(Ok((subnet_id, _end_slot)))) => { - self.handle_removed_subnet(subnet_id); - // We re-wake the task as there could be other subscriptions to process - self.waker - .as_ref() - .expect("Waker has been set") - .wake_by_ref(); - } - Poll::Ready(Some(Err(e))) => { - error!(self.log, "Failed to check for subnet unsubscription times"; "error"=> e); - } - Poll::Ready(None) | Poll::Pending => {} - } - - // Poll to remove entries on expiration, no need to act on expiration events. - if let Some(tracked_vals) = self.aggregate_validators_on_subnet.as_mut() { - if let Poll::Ready(Some(Err(e))) = tracked_vals.poll_next_unpin(cx) { - error!(self.log, "Failed to check for aggregate validator on subnet expirations"; "error"=> e); - } - } - - Poll::Pending - } -} diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 6450fc72eee..65605b02c12 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -1,10 +1,25 @@ -pub mod attestation_subnets; -pub mod sync_subnets; +//! This service keeps track of which shard subnet the beacon node should be subscribed to at any +//! given time. It schedules subscriptions to shard subnets, requests peer discoveries and +//! determines whether attestations should be aggregated and/or passed to the beacon node. -use lighthouse_network::{Subnet, SubnetDiscovery}; +use std::collections::HashSet; +use std::collections::{HashMap, VecDeque}; +use std::pin::Pin; +use std::sync::Arc; +use std::task::{Context, Poll}; +use std::time::Duration; +use tokio::time::Instant; -pub use attestation_subnets::AttestationService; -pub use sync_subnets::SyncCommitteeService; +use beacon_chain::{BeaconChain, BeaconChainTypes}; +use delay_map::HashSetDelay; +use futures::prelude::*; +use lighthouse_network::{discv5::enr::NodeId, NetworkConfig, Subnet, SubnetDiscovery}; +use slog::{debug, error, o, warn}; +use slot_clock::SlotClock; +use types::{ + Attestation, EthSpec, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, + ValidatorSubscription, +}; #[cfg(test)] mod tests; @@ -17,12 +32,660 @@ pub enum SubnetServiceMessage { Unsubscribe(Subnet), /// Add the `SubnetId` to the ENR bitfield. EnrAdd(Subnet), - /// Remove the `SubnetId` from the ENR bitfield. - EnrRemove(Subnet), /// Discover peers for a list of `SubnetDiscovery`. DiscoverPeers(Vec), } +use crate::metrics; + +/// The minimum number of slots ahead that we attempt to discover peers for a subscription. If the +/// slot is less than this number, skip the peer discovery process. +/// Subnet discovery query takes at most 30 secs, 2 slots take 24s. +pub(crate) const MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD: u64 = 2; +/// The fraction of a slot that we subscribe to a subnet before the required slot. +/// +/// Currently a whole slot ahead. +const ADVANCE_SUBSCRIBE_SLOT_FRACTION: u32 = 1; + +/// The number of slots after an aggregator duty where we remove the entry from +/// `aggregate_validators_on_subnet` delay map. +const UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY: u32 = 2; + +/// A particular subnet at a given slot. +#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] +pub struct ExactSubnet { + /// The `SubnetId` associated with this subnet. + pub subnet: Subnet, + /// For Attestations, this slot represents the start time at which we need to subscribe to the + /// slot. For SyncCommittee subnet id's this represents the end slot at which we no longer need + /// to subscribe to the subnet. + // NOTE: There was different logic between the two subscriptions and having a different + // interpretation of this variable seemed like the best way to group the logic, even though it + // may be counter-intuitive (apologies to future readers). + pub slot: Slot, +} + +/// The enum used to group all kinds of validator subscriptions +pub enum Subscription { + Attestation(ValidatorSubscription), + SyncCommittee(SyncCommitteeSubscription), +} + +pub struct SubnetService { + /// Queued events to return to the driving service. + events: VecDeque, + + /// A reference to the beacon chain to process received attestations. + pub(crate) beacon_chain: Arc>, + + /// Subnets we are currently subscribed to as short lived subscriptions. + /// + /// Once they expire, we unsubscribe from these. + /// We subscribe to subnets when we are an aggregator for an exact subnet. + // NOTE: When setup the default timeout is set for sync committee subscriptions. + subscriptions: HashSetDelay, + + /// Subscriptions that need to be executed in the future. + scheduled_subscriptions: HashSetDelay, + + /// A list of permanent subnets that this node is subscribed to. + // TODO: Shift this to a dynamic bitfield + permanent_attestation_subscriptions: HashSet, + + /// A collection timeouts to track the existence of aggregate validator subscriptions at an + /// `ExactSubnet`. + aggregate_validators_on_subnet: Option>, + + /// The waker for the current thread. + waker: Option, + + /// The discovery mechanism of lighthouse is disabled. + discovery_disabled: bool, + + /// We are always subscribed to all subnets. + subscribe_all_subnets: bool, + + /// Whether this node is a block proposer-only node. + proposer_only: bool, + + /// The logger for the attestation service. + log: slog::Logger, +} + +impl SubnetService { + /* Public functions */ + + /// Establish the service based on the passed configuration. + pub fn new( + beacon_chain: Arc>, + node_id: NodeId, + config: &NetworkConfig, + log: &slog::Logger, + ) -> Self { + let log = log.new(o!("service" => "attestation_service")); + + let slot_duration = beacon_chain.slot_clock.slot_duration(); + + if config.subscribe_all_subnets { + slog::info!(log, "Subscribing to all subnets"); + } + + // Build the list of known permanent subscriptions, so that we know not to subscribe or + // discover them. + let mut permanent_attestation_subscriptions = HashSet::default(); + if config.subscribe_all_subnets { + // We are subscribed to all subnets, set all the bits to true. + for index in 0..::SubnetBitfieldLength::to_u64() { + permanent_attestation_subscriptions + .insert(Subnet::Attestation(SubnetId::from(index))); + } + } else { + // Not subscribed to all subnets, so just calculate the required subnets from the + for subnet_id in SubnetId::compute_attestation_subnets::( + node_id.raw().into(), + &beacon_chain.spec, + ) { + permanent_attestation_subscriptions.insert(Subnet::Attestation(subnet_id)); + } + } + + // Set up the sync committee subscriptions + let spec = &beacon_chain.spec; + let epoch_duration_secs = + beacon_chain.slot_clock.slot_duration().as_secs() * T::EthSpec::slots_per_epoch(); + let default_sync_committee_duration = Duration::from_secs( + epoch_duration_secs.saturating_mul(spec.epochs_per_sync_committee_period.as_u64()), + ); + + let track_validators = !config.import_all_attestations; + let aggregate_validators_on_subnet = + track_validators.then(|| HashSetDelay::new(slot_duration)); + + let mut events = VecDeque::with_capacity(10); + + // Queue discovery queries for the permanent attestation subnets + events.push_back(SubnetServiceMessage::DiscoverPeers( + permanent_attestation_subscriptions + .iter() + .cloned() + .map(|subnet| SubnetDiscovery { + subnet, + min_ttl: None, + }) + .collect(), + )); + + // Pre-populate the events with permanent subscriptions + for subnet in permanent_attestation_subscriptions.iter() { + events.push_back(SubnetServiceMessage::Subscribe(*subnet)); + events.push_back(SubnetServiceMessage::EnrAdd(*subnet)); + } + + SubnetService { + events, + beacon_chain, + subscriptions: HashSetDelay::new(default_sync_committee_duration), + permanent_attestation_subscriptions, + scheduled_subscriptions: HashSetDelay::default(), + aggregate_validators_on_subnet, + waker: None, + discovery_disabled: config.disable_discovery, + subscribe_all_subnets: config.subscribe_all_subnets, + proposer_only: config.proposer_only, + log, + } + } + + /// Return count of all currently subscribed subnets (long-lived **and** short-lived). + #[cfg(test)] + pub fn subscription_count(&self) -> usize { + if self.subscribe_all_subnets { + self.beacon_chain.spec.attestation_subnet_count as usize + } else { + self.subscriptions + .iter() + .filter(|subnet| matches!(subnet, Subnet::Attestation(_))) + .collect::>() + .len() + } + } + + /// Return count of all currently subscribed sync committee subnets. + #[cfg(test)] + pub fn sync_committee_subscription_count(&self) -> usize { + use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; + if self.subscribe_all_subnets { + SYNC_COMMITTEE_SUBNET_COUNT as usize + } else { + self.subscriptions + .iter() + .filter(|subnet| matches!(subnet, Subnet::SyncCommittee(_))) + .collect::>() + .len() + } + } + + /// Returns whether we are subscribed to a subnet for testing purposes. + #[cfg(test)] + pub(crate) fn is_subscribed(&self, subnet: &Subnet) -> bool { + self.subscriptions.contains_key(subnet) + } + + /// Processes a list of validator subscriptions. + /// + /// This is fundamentally called form the HTTP API when a validator requests duties from us + /// This will: + /// - Register new validators as being known. + /// - Search for peers for required subnets. + /// - Request subscriptions for subnets on specific slots when required. + /// - Build the timeouts for each of these events. + /// + /// 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> { + // If the node is in a proposer-only state, we ignore all subnet subscriptions. + if self.proposer_only { + return Ok(()); + } + + // Maps each subnet subscription to it's highest slot + let mut subnets_to_discover: HashMap = HashMap::new(); + + // Registers the validator with the attestation service. + for general_subscription in subscriptions { + match general_subscription { + Subscription::Attestation(subscription) => { + metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_REQUESTS); + + // Compute the subnet that is associated with this subscription + let subnet = match SubnetId::compute_subnet::( + subscription.slot, + subscription.attestation_committee_index, + subscription.committee_count_at_slot, + &self.beacon_chain.spec, + ) { + Ok(subnet_id) => Subnet::Attestation(subnet_id), + Err(e) => { + warn!(self.log, + "Failed to compute subnet id for validator subscription"; + "error" => ?e, + ); + continue; + } + }; + + // Ensure each subnet_id inserted into the map has the highest slot as it's value. + // Higher slot corresponds to higher min_ttl in the `SubnetDiscovery` entry. + if let Some(slot) = subnets_to_discover.get(&subnet) { + if subscription.slot > *slot { + subnets_to_discover.insert(subnet, subscription.slot); + } + } else if !self.discovery_disabled { + subnets_to_discover.insert(subnet, subscription.slot); + } + + let exact_subnet = ExactSubnet { + subnet, + slot: subscription.slot, + }; + + // Determine if the validator is an aggregator. If so, we subscribe to the subnet and + // if successful add the validator to a mapping of known aggregators for that exact + // subnet. + + if subscription.is_aggregator { + metrics::inc_counter(&metrics::SUBNET_SUBSCRIPTION_AGGREGATOR_REQUESTS); + if let Err(e) = self.subscribe_to_subnet(exact_subnet) { + warn!(self.log, + "Subscription to subnet error"; + "error" => e, + ); + } + } + } + Subscription::SyncCommittee(subscription) => { + metrics::inc_counter(&metrics::SYNC_COMMITTEE_SUBSCRIPTION_REQUESTS); + // NOTE: We assume all subscriptions have been verified before reaching this service + + // Registers the validator with the subnet service. + let subnet_ids = + match SyncSubnetId::compute_subnets_for_sync_committee::( + &subscription.sync_committee_indices, + ) { + Ok(subnet_ids) => subnet_ids, + Err(e) => { + warn!(self.log, + "Failed to compute subnet id for sync committee subscription"; + "error" => ?e, + "validator_index" => subscription.validator_index + ); + continue; + } + }; + + let spec = &self.beacon_chain.spec; + let sync_committee_duration_in_slots = spec + .epochs_per_sync_committee_period + .as_u64() + .saturating_mul(T::EthSpec::slots_per_epoch()); + + for subnet_id in subnet_ids { + let subnet = Subnet::SyncCommittee(subnet_id); + let slot_when_required = subscription + .until_epoch + .start_slot(T::EthSpec::slots_per_epoch()); + subnets_to_discover.insert(subnet.clone(), slot_when_required); + + let Some(duration_to_unsubscribe) = + self.beacon_chain.slot_clock.duration_to_slot( + slot_when_required + sync_committee_duration_in_slots, + ) + else { + warn!(self.log, "Subscription to sync subnet error"; "error" => "Unable to determine duration to unsubscription slot", "validator_index" => subscription.validator_index); + continue; + }; + + if duration_to_unsubscribe == Duration::from_secs(0) { + let current_slot = self + .beacon_chain + .slot_clock + .now() + .unwrap_or(Slot::from(0u64)); + warn!( + self.log, + "Sync committee subscription is past expiration"; + "subnet" => ?subnet, + "current_slot" => ?current_slot, + "unsubscribe_slot" => ?slot_when_required + sync_committee_duration_in_slots, + + ); + } + + self.subscribe_to_sync_subnet(subnet, duration_to_unsubscribe); + } + } + } + } + + // If the discovery mechanism isn't disabled, attempt to set up a peer discovery for the + // required subnets. + if !self.discovery_disabled { + if let Err(e) = self.discover_peers_request(subnets_to_discover.into_iter()) { + 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 + /// verification, re-propagates and returns false. + pub fn should_process_attestation( + &self, + subnet: Subnet, + attestation: &Attestation, + ) -> bool { + // Proposer-only mode does not need to process attestations + if self.proposer_only { + return false; + } + self.aggregate_validators_on_subnet + .as_ref() + .map(|tracked_vals| { + tracked_vals.contains_key(&ExactSubnet { + subnet, + slot: attestation.data().slot, + }) + }) + .unwrap_or(true) + } + + /* Internal private functions */ + + /// Adds an event to the event queue and notifies that this service is ready to be polled + /// again. + fn queue_event(&mut self, ev: SubnetServiceMessage) { + self.events.push_back(ev); + if let Some(waker) = &self.waker { + waker.wake_by_ref() + } + } + /// Checks if there are currently queued discovery requests and the time required to make the + /// 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, + subnets_to_discover: impl Iterator, + ) -> Result<(), &'static str> { + let current_slot = self + .beacon_chain + .slot_clock + .now() + .ok_or("Could not get the current slot")?; + + let discovery_subnets: Vec = subnets_to_discover + .filter_map(|(subnet, relevant_slot)| { + // We generate discovery requests for all subnets (even one's we are permenantly + // subscribed to) in order to ensure our peer counts are satisfactory to perform the + // necessary duties. + + // Check if there is enough time to perform a discovery lookup. + if relevant_slot >= current_slot.saturating_add(MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD) + { + // Send out an event to start looking for peers. + // Require the peer for an additional slot to ensure we keep the peer for the + // duration of the subscription. + let min_ttl = self + .beacon_chain + .slot_clock + .duration_to_slot(relevant_slot + 1) + .map(|duration| std::time::Instant::now() + duration); + Some(SubnetDiscovery { subnet, min_ttl }) + } else { + // We may want to check the global PeerInfo to see estimated timeouts for each + // peer before they can be removed. + warn!(self.log, + "Not enough time for a discovery search"; + "subnet_id" => ?subnet, + ); + None + } + }) + .collect(); + + if !discovery_subnets.is_empty() { + self.queue_event(SubnetServiceMessage::DiscoverPeers(discovery_subnets)); + } + Ok(()) + } + + // Subscribes to the subnet if it should be done immediately, or schedules it if required. + fn subscribe_to_subnet( + &mut self, + ExactSubnet { subnet, slot }: ExactSubnet, + ) -> Result<(), &'static str> { + // If the subnet is one of our permanent subnets, we do not need to subscribe. + if self.subscribe_all_subnets || self.permanent_attestation_subscriptions.contains(&subnet) + { + return Ok(()); + } + + let slot_duration = self.beacon_chain.slot_clock.slot_duration(); + + // The short time we schedule the subscription before it's actually required. This + // ensures we are subscribed on time, and allows consecutive subscriptions to the same + // subnet to overlap, reducing subnet churn. + let advance_subscription_duration = slot_duration / ADVANCE_SUBSCRIBE_SLOT_FRACTION; + // The time to the required slot. + let time_to_subscription_slot = self + .beacon_chain + .slot_clock + .duration_to_slot(slot) + .unwrap_or_default(); // If this is a past slot we will just get a 0 duration. + + // Calculate how long before we need to subscribe to the subnet. + let time_to_subscription_start = + time_to_subscription_slot.saturating_sub(advance_subscription_duration); + + // The time after a duty slot where we no longer need it in the `aggregate_validators_on_subnet` + // delay map. + let time_to_unsubscribe = + time_to_subscription_slot + UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY * slot_duration; + if let Some(tracked_vals) = self.aggregate_validators_on_subnet.as_mut() { + tracked_vals.insert_at(ExactSubnet { subnet, slot }, time_to_unsubscribe); + } + + // If the subscription should be done in the future, schedule it. Otherwise subscribe + // immediately. + if time_to_subscription_start.is_zero() { + // This is a current or past slot, we subscribe immediately. + self.subscribe_to_subnet_immediately(subnet, slot + 1)?; + } else { + // This is a future slot, schedule subscribing. + self.scheduled_subscriptions + .insert_at(subnet, time_to_subscription_start); + } + + Ok(()) + } + + /// Adds a subscription event and an associated unsubscription event if required. + fn subscribe_to_sync_subnet(&mut self, subnet: Subnet, duration_to_unsubscribe: Duration) { + // Return if we have subscribed to all subnets + if self.subscribe_all_subnets { + return; + } + + // Return if we already have a subscription for the subnet and its closer or + if let Some(current_instant_to_unsubscribe) = self.subscriptions.deadline(&subnet) { + // The extra 500ms in the comparison accounts of the inaccuracy of the underlying + // DelayQueue inside the delaymap struct. + let current_duration_to_unsubscribe = (current_instant_to_unsubscribe + + Duration::from_millis(500)) + .checked_duration_since(Instant::now()) + .unwrap_or(Duration::from_secs(0)); + + if duration_to_unsubscribe > current_duration_to_unsubscribe { + self.subscriptions + .update_timeout(&subnet, duration_to_unsubscribe); + } + } else { + // We have not subscribed before, so subscribe + self.subscriptions + .insert_at(subnet, duration_to_unsubscribe); + // We are not currently subscribed and have no waiting subscription, create one + debug!(self.log, "Subscribing to subnet"; "subnet" => ?subnet, "until" => ?duration_to_unsubscribe); + self.events + .push_back(SubnetServiceMessage::Subscribe(subnet)); + + // add the subnet to the ENR bitfield + self.events.push_back(SubnetServiceMessage::EnrAdd(subnet)); + } + } + + /* A collection of functions that handle the various timeouts */ + + /// Registers a subnet as subscribed. + /// + /// Checks that the time in which the subscription would end is not in the past. If we are + /// already subscribed, extends the timeout if necessary. If this is a new subscription, we send + /// out the appropriate events. + fn subscribe_to_subnet_immediately( + &mut self, + subnet: Subnet, + end_slot: Slot, + ) -> Result<(), &'static str> { + if self.subscribe_all_subnets { + // Case not handled by this service. + return Ok(()); + } + + let time_to_subscription_end = self + .beacon_chain + .slot_clock + .duration_to_slot(end_slot) + .unwrap_or_default(); + + // First check this is worth doing. + if time_to_subscription_end.is_zero() { + return Err("Time when subscription would end has already passed."); + } + + // We need to check and add a subscription for the right kind, regardless of the presence + // of the subnet as a subscription of the other kind. This is mainly since long lived + // subscriptions can be removed at any time when a validator goes offline. + + match self.subscriptions.deadline(&subnet) { + Some(current_end_slot_time) => { + // We are already subscribed. Check if we need to extend the subscription. + if current_end_slot_time + .checked_duration_since(Instant::now()) + .unwrap_or(Duration::from_secs(0)) + < time_to_subscription_end + { + self.subscriptions + .update_timeout(&subnet, time_to_subscription_end); + } + } + None => { + // This is a new subscription. Add with the corresponding timeout and send the + // notification. + self.subscriptions + .insert_at(subnet, time_to_subscription_end); + + // Inform of the subscription. + debug!(self.log, "Subscribing to subnet"; + "subnet" => ?subnet, + "end_slot" => end_slot, + ); + self.queue_event(SubnetServiceMessage::Subscribe(subnet)); + } + } + Ok(()) + } + + // Unsubscribes from a subnet that was removed if it does not continue to exist as a + // subscription of the other kind. For long lived subscriptions, it also removes the + // advertisement from our ENR. + fn handle_removed_subnet(&mut self, subnet: Subnet) { + if !self.subscriptions.contains_key(&subnet) { + // Subscription no longer exists as short lived or long lived. + debug!(self.log, "Unsubscribing from subnet"; "subnet" => ?subnet); + self.queue_event(SubnetServiceMessage::Unsubscribe(subnet)); + } + } +} + +impl Stream for SubnetService { + type Item = SubnetServiceMessage; + + fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + // Update the waker if needed. + if let Some(waker) = &self.waker { + if waker.will_wake(cx.waker()) { + self.waker = Some(cx.waker().clone()); + } + } else { + self.waker = Some(cx.waker().clone()); + } + + // Send out any generated events. + if let Some(event) = self.events.pop_front() { + return Poll::Ready(Some(event)); + } + + // Process scheduled subscriptions that might be ready, since those can extend a soon to + // expire subscription. + match self.scheduled_subscriptions.poll_next_unpin(cx) { + Poll::Ready(Some(Ok(subnet))) => { + let current_slot = self.beacon_chain.slot_clock.now().unwrap_or_default(); + if let Err(e) = self.subscribe_to_subnet_immediately(subnet, current_slot + 1) { + debug!(self.log, "Failed to subscribe to short lived subnet"; "subnet" => ?subnet, "err" => e); + } + self.waker + .as_ref() + .expect("Waker has been set") + .wake_by_ref(); + } + Poll::Ready(Some(Err(e))) => { + error!(self.log, "Failed to check for scheduled subnet subscriptions"; "error"=> e); + } + Poll::Ready(None) | Poll::Pending => {} + } + + // Process any expired subscriptions. + match self.subscriptions.poll_next_unpin(cx) { + Poll::Ready(Some(Ok(subnet))) => { + self.handle_removed_subnet(subnet); + // We re-wake the task as there could be other subscriptions to process + self.waker + .as_ref() + .expect("Waker has been set") + .wake_by_ref(); + } + Poll::Ready(Some(Err(e))) => { + error!(self.log, "Failed to check for subnet unsubscription times"; "error"=> e); + } + Poll::Ready(None) | Poll::Pending => {} + } + + // Poll to remove entries on expiration, no need to act on expiration events. + if let Some(tracked_vals) = self.aggregate_validators_on_subnet.as_mut() { + if let Poll::Ready(Some(Err(e))) = tracked_vals.poll_next_unpin(cx) { + error!(self.log, "Failed to check for aggregate validator on subnet expirations"; "error"=> e); + } + } + + Poll::Pending + } +} + /// Note: This `PartialEq` impl is for use only in tests. /// The `DiscoverPeers` comparison is good enough for testing only. #[cfg(test)] @@ -32,7 +695,6 @@ impl PartialEq for SubnetServiceMessage { (SubnetServiceMessage::Subscribe(a), SubnetServiceMessage::Subscribe(b)) => a == b, (SubnetServiceMessage::Unsubscribe(a), SubnetServiceMessage::Unsubscribe(b)) => a == b, (SubnetServiceMessage::EnrAdd(a), SubnetServiceMessage::EnrAdd(b)) => a == b, - (SubnetServiceMessage::EnrRemove(a), SubnetServiceMessage::EnrRemove(b)) => a == b, (SubnetServiceMessage::DiscoverPeers(a), SubnetServiceMessage::DiscoverPeers(b)) => { if a.len() != b.len() { return false; diff --git a/beacon_node/network/src/subnet_service/sync_subnets.rs b/beacon_node/network/src/subnet_service/sync_subnets.rs deleted file mode 100644 index eda7ce8efbd..00000000000 --- a/beacon_node/network/src/subnet_service/sync_subnets.rs +++ /dev/null @@ -1,359 +0,0 @@ -//! This service keeps track of which sync committee subnet the beacon node should be subscribed to at any -//! given time. It schedules subscriptions to sync committee subnets and requests peer discoveries. - -use std::collections::{hash_map::Entry, HashMap, VecDeque}; -use std::pin::Pin; -use std::sync::Arc; -use std::task::{Context, Poll}; -use std::time::Duration; - -use futures::prelude::*; -use slog::{debug, error, o, trace, warn}; - -use super::SubnetServiceMessage; -use beacon_chain::{BeaconChain, BeaconChainTypes}; -use delay_map::HashSetDelay; -use lighthouse_network::{NetworkConfig, Subnet, SubnetDiscovery}; -use slot_clock::SlotClock; -use types::{Epoch, EthSpec, SyncCommitteeSubscription, SyncSubnetId}; - -use crate::metrics; - -/// The minimum number of slots ahead that we attempt to discover peers for a subscription. If the -/// slot is less than this number, skip the peer discovery process. -/// Subnet discovery query takes at most 30 secs, 2 slots take 24s. -const MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD: u64 = 2; - -/// A particular subnet at a given slot. -#[derive(PartialEq, Eq, Hash, Clone, Debug)] -pub struct ExactSubnet { - /// The `SyncSubnetId` associated with this subnet. - pub subnet_id: SyncSubnetId, - /// The epoch until which we need to stay subscribed to the subnet. - pub until_epoch: Epoch, -} -pub struct SyncCommitteeService { - /// Queued events to return to the driving service. - events: VecDeque, - - /// A reference to the beacon chain to process received attestations. - pub(crate) beacon_chain: Arc>, - - /// The collection of all currently subscribed subnets. - subscriptions: HashMap, - - /// A collection of timeouts for when to unsubscribe from a subnet. - unsubscriptions: HashSetDelay, - - /// The waker for the current thread. - waker: Option, - - /// The discovery mechanism of lighthouse is disabled. - discovery_disabled: bool, - - /// We are always subscribed to all subnets. - subscribe_all_subnets: bool, - - /// Whether this node is a block proposer-only node. - proposer_only: bool, - - /// The logger for the attestation service. - log: slog::Logger, -} - -impl SyncCommitteeService { - /* Public functions */ - - pub fn new( - beacon_chain: Arc>, - config: &NetworkConfig, - log: &slog::Logger, - ) -> Self { - let log = log.new(o!("service" => "sync_committee_service")); - - let spec = &beacon_chain.spec; - let epoch_duration_secs = - beacon_chain.slot_clock.slot_duration().as_secs() * T::EthSpec::slots_per_epoch(); - let default_timeout = - epoch_duration_secs.saturating_mul(spec.epochs_per_sync_committee_period.as_u64()); - - SyncCommitteeService { - events: VecDeque::with_capacity(10), - beacon_chain, - subscriptions: HashMap::new(), - unsubscriptions: HashSetDelay::new(Duration::from_secs(default_timeout)), - waker: None, - subscribe_all_subnets: config.subscribe_all_subnets, - discovery_disabled: config.disable_discovery, - proposer_only: config.proposer_only, - log, - } - } - - /// Return count of all currently subscribed subnets. - #[cfg(test)] - pub fn subscription_count(&self) -> usize { - use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; - if self.subscribe_all_subnets { - SYNC_COMMITTEE_SUBNET_COUNT as usize - } else { - self.subscriptions.len() - } - } - - /// Processes a list of sync committee subscriptions. - /// - /// This will: - /// - Search for peers for required subnets. - /// - Request subscriptions required subnets. - /// - Build the timeouts for each of these events. - /// - /// This returns a result simply for the ergonomics of using ?. The result can be - /// safely dropped. - pub fn validator_subscriptions( - &mut self, - subscriptions: Vec, - ) -> Result<(), String> { - // A proposer-only node does not subscribe to any sync-committees - if self.proposer_only { - return Ok(()); - } - - let mut subnets_to_discover = Vec::new(); - for subscription in subscriptions { - metrics::inc_counter(&metrics::SYNC_COMMITTEE_SUBSCRIPTION_REQUESTS); - //NOTE: We assume all subscriptions have been verified before reaching this service - - // Registers the validator with the subnet service. - // This will subscribe to long-lived random subnets if required. - trace!(self.log, - "Sync committee subscription"; - "subscription" => ?subscription, - ); - - let subnet_ids = match SyncSubnetId::compute_subnets_for_sync_committee::( - &subscription.sync_committee_indices, - ) { - Ok(subnet_ids) => subnet_ids, - Err(e) => { - warn!(self.log, - "Failed to compute subnet id for sync committee subscription"; - "error" => ?e, - "validator_index" => subscription.validator_index - ); - continue; - } - }; - - for subnet_id in subnet_ids { - let exact_subnet = ExactSubnet { - subnet_id, - until_epoch: subscription.until_epoch, - }; - subnets_to_discover.push(exact_subnet.clone()); - if let Err(e) = self.subscribe_to_subnet(exact_subnet.clone()) { - warn!(self.log, - "Subscription to sync subnet error"; - "error" => e, - "validator_index" => subscription.validator_index, - ); - } else { - trace!(self.log, - "Subscribed to subnet for sync committee duties"; - "exact_subnet" => ?exact_subnet, - "validator_index" => subscription.validator_index - ); - } - } - } - // If the discovery mechanism isn't disabled, attempt to set up a peer discovery for the - // required subnets. - if !self.discovery_disabled { - if let Err(e) = self.discover_peers_request(subnets_to_discover.iter()) { - warn!(self.log, "Discovery lookup request error"; "error" => e); - }; - } - - // pre-emptively wake the thread to check for new events - if let Some(waker) = &self.waker { - waker.wake_by_ref(); - } - Ok(()) - } - - /* Internal private functions */ - - /// Checks if there are currently queued discovery requests and the time required to make the - /// request. - /// - /// If there is sufficient time, queues a peer discovery request for all the required subnets. - fn discover_peers_request<'a>( - &mut self, - exact_subnets: impl Iterator, - ) -> Result<(), &'static str> { - let current_slot = self - .beacon_chain - .slot_clock - .now() - .ok_or("Could not get the current slot")?; - - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - - let discovery_subnets: Vec = exact_subnets - .filter_map(|exact_subnet| { - let until_slot = exact_subnet.until_epoch.end_slot(slots_per_epoch); - // check if there is enough time to perform a discovery lookup - if until_slot >= current_slot.saturating_add(MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD) { - // if the slot is more than epoch away, add an event to start looking for peers - // add one slot to ensure we keep the peer for the subscription slot - let min_ttl = self - .beacon_chain - .slot_clock - .duration_to_slot(until_slot + 1) - .map(|duration| std::time::Instant::now() + duration); - Some(SubnetDiscovery { - subnet: Subnet::SyncCommittee(exact_subnet.subnet_id), - min_ttl, - }) - } else { - // We may want to check the global PeerInfo to see estimated timeouts for each - // peer before they can be removed. - warn!(self.log, - "Not enough time for a discovery search"; - "subnet_id" => ?exact_subnet - ); - None - } - }) - .collect(); - - if !discovery_subnets.is_empty() { - self.events - .push_back(SubnetServiceMessage::DiscoverPeers(discovery_subnets)); - } - Ok(()) - } - - /// Adds a subscription event and an associated unsubscription event if required. - fn subscribe_to_subnet(&mut self, exact_subnet: ExactSubnet) -> Result<(), &'static str> { - // Return if we have subscribed to all subnets - if self.subscribe_all_subnets { - return Ok(()); - } - - // Return if we already have a subscription for exact_subnet - if self.subscriptions.get(&exact_subnet.subnet_id) == Some(&exact_subnet.until_epoch) { - return Ok(()); - } - - // Return if we already have subscription set to expire later than the current request. - if let Some(until_epoch) = self.subscriptions.get(&exact_subnet.subnet_id) { - if *until_epoch >= exact_subnet.until_epoch { - return Ok(()); - } - } - - // initialise timing variables - let current_slot = self - .beacon_chain - .slot_clock - .now() - .ok_or("Could not get the current slot")?; - - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let until_slot = exact_subnet.until_epoch.end_slot(slots_per_epoch); - // Calculate the duration to the unsubscription event. - let expected_end_subscription_duration = if current_slot >= until_slot { - warn!( - self.log, - "Sync committee subscription is past expiration"; - "current_slot" => current_slot, - "exact_subnet" => ?exact_subnet, - ); - return Ok(()); - } else { - let slot_duration = self.beacon_chain.slot_clock.slot_duration(); - - // the duration until we no longer need this subscription. We assume a single slot is - // sufficient. - self.beacon_chain - .slot_clock - .duration_to_slot(until_slot) - .ok_or("Unable to determine duration to unsubscription slot")? - + slot_duration - }; - - if let Entry::Vacant(e) = self.subscriptions.entry(exact_subnet.subnet_id) { - // We are not currently subscribed and have no waiting subscription, create one - debug!(self.log, "Subscribing to subnet"; "subnet" => *exact_subnet.subnet_id, "until_epoch" => ?exact_subnet.until_epoch); - e.insert(exact_subnet.until_epoch); - self.events - .push_back(SubnetServiceMessage::Subscribe(Subnet::SyncCommittee( - exact_subnet.subnet_id, - ))); - - // add the subnet to the ENR bitfield - self.events - .push_back(SubnetServiceMessage::EnrAdd(Subnet::SyncCommittee( - exact_subnet.subnet_id, - ))); - - // add an unsubscription event to remove ourselves from the subnet once completed - self.unsubscriptions - .insert_at(exact_subnet.subnet_id, expected_end_subscription_duration); - } else { - // We are already subscribed, extend the unsubscription duration - self.unsubscriptions - .update_timeout(&exact_subnet.subnet_id, expected_end_subscription_duration); - } - - Ok(()) - } - - /// A queued unsubscription is ready. - fn handle_unsubscriptions(&mut self, subnet_id: SyncSubnetId) { - debug!(self.log, "Unsubscribing from subnet"; "subnet" => *subnet_id); - - self.subscriptions.remove(&subnet_id); - self.events - .push_back(SubnetServiceMessage::Unsubscribe(Subnet::SyncCommittee( - subnet_id, - ))); - - self.events - .push_back(SubnetServiceMessage::EnrRemove(Subnet::SyncCommittee( - subnet_id, - ))); - } -} - -impl Stream for SyncCommitteeService { - type Item = SubnetServiceMessage; - - fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - // update the waker if needed - if let Some(waker) = &self.waker { - if waker.will_wake(cx.waker()) { - self.waker = Some(cx.waker().clone()); - } - } else { - self.waker = Some(cx.waker().clone()); - } - - // process any un-subscription events - match self.unsubscriptions.poll_next_unpin(cx) { - Poll::Ready(Some(Ok(exact_subnet))) => self.handle_unsubscriptions(exact_subnet), - Poll::Ready(Some(Err(e))) => { - error!(self.log, "Failed to check for subnet unsubscription times"; "error"=> e); - } - Poll::Ready(None) | Poll::Pending => {} - } - - // process any generated events - if let Some(event) = self.events.pop_front() { - return Poll::Ready(Some(event)); - } - - Poll::Pending - } -} diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 74f3f59df3c..f07fdb964d6 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -4,7 +4,6 @@ use beacon_chain::{ eth1_chain::CachingEth1Backend, BeaconChain, }; -use futures::prelude::*; use genesis::{generate_deterministic_keypairs, interop_genesis_state, DEFAULT_ETH1_BLOCK_HASH}; use lazy_static::lazy_static; use lighthouse_network::NetworkConfig; @@ -114,15 +113,13 @@ lazy_static! { static ref CHAIN: TestBeaconChain = TestBeaconChain::new_with_system_clock(); } -fn get_attestation_service( - log_level: Option, -) -> AttestationService { +fn get_attestation_service(log_level: Option) -> SubnetService { let log = get_logger(log_level); let config = NetworkConfig::default(); let beacon_chain = CHAIN.chain.clone(); - AttestationService::new( + SubnetService::new( beacon_chain, lighthouse_network::discv5::enr::NodeId::random(), &config, @@ -130,15 +127,6 @@ fn get_attestation_service( ) } -fn get_sync_committee_service() -> SyncCommitteeService { - let log = get_logger(None); - let config = NetworkConfig::default(); - - let beacon_chain = CHAIN.chain.clone(); - - SyncCommitteeService::new(beacon_chain, &config, &log) -} - // gets a number of events from the subscription service, or returns none if it times out after a number // of slots async fn get_events + Unpin>( diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 31492265ed0..099ec9c5cf5 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -76,6 +76,7 @@ impl SubnetId { /// 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. + #[allow(clippy::arithmetic_side_effects)] pub fn compute_attestation_subnets( node_id: ethereum_types::U256, spec: &ChainSpec, @@ -93,10 +94,8 @@ impl SubnetId { .. } = spec; - let subnet_set_generator = (0..subnets_per_node).map(move |idx| { - SubnetId::new((node_id_prefix + idx as u64) % attestation_subnet_count) - }); - subnet_set_generator + (0..subnets_per_node) + .map(move |idx| SubnetId::new((node_id_prefix + idx as u64) % attestation_subnet_count)) } } From 6bd0f75f3f8b428f29b2df02ac67cb27a65e4ceb Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 22 Jul 2024 16:15:49 +1000 Subject: [PATCH 03/10] Update tests for new version --- beacon_node/network/src/subnet_service/mod.rs | 65 ++- .../network/src/subnet_service/tests/mod.rs | 376 ++++++++---------- 2 files changed, 182 insertions(+), 259 deletions(-) diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 65605b02c12..80ca4239440 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -66,6 +66,7 @@ pub struct ExactSubnet { } /// The enum used to group all kinds of validator subscriptions +#[derive(Debug, Clone, PartialEq)] pub enum Subscription { Attestation(ValidatorSubscription), SyncCommittee(SyncCommitteeSubscription), @@ -198,31 +199,13 @@ impl SubnetService { /// Return count of all currently subscribed subnets (long-lived **and** short-lived). #[cfg(test)] - pub fn subscription_count(&self) -> usize { - if self.subscribe_all_subnets { - self.beacon_chain.spec.attestation_subnet_count as usize - } else { - self.subscriptions - .iter() - .filter(|subnet| matches!(subnet, Subnet::Attestation(_))) - .collect::>() - .len() - } + pub fn subscriptions(&self) -> impl Iterator { + self.subscriptions.iter() } - /// Return count of all currently subscribed sync committee subnets. #[cfg(test)] - pub fn sync_committee_subscription_count(&self) -> usize { - use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; - if self.subscribe_all_subnets { - SYNC_COMMITTEE_SUBNET_COUNT as usize - } else { - self.subscriptions - .iter() - .filter(|subnet| matches!(subnet, Subnet::SyncCommittee(_))) - .collect::>() - .len() - } + pub fn permanent_subscriptions(&self) -> impl Iterator { + self.permanent_attestation_subscriptions.iter() } /// Returns whether we are subscribed to a subnet for testing purposes. @@ -326,23 +309,17 @@ impl SubnetService { } }; - let spec = &self.beacon_chain.spec; - let sync_committee_duration_in_slots = spec - .epochs_per_sync_committee_period - .as_u64() - .saturating_mul(T::EthSpec::slots_per_epoch()); - for subnet_id in subnet_ids { let subnet = Subnet::SyncCommittee(subnet_id); - let slot_when_required = subscription + let slot_required_until = subscription .until_epoch .start_slot(T::EthSpec::slots_per_epoch()); - subnets_to_discover.insert(subnet.clone(), slot_when_required); + subnets_to_discover.insert(subnet, slot_required_until); - let Some(duration_to_unsubscribe) = - self.beacon_chain.slot_clock.duration_to_slot( - slot_when_required + sync_committee_duration_in_slots, - ) + let Some(duration_to_unsubscribe) = self + .beacon_chain + .slot_clock + .duration_to_slot(slot_required_until) else { warn!(self.log, "Subscription to sync subnet error"; "error" => "Unable to determine duration to unsubscription slot", "validator_index" => subscription.validator_index); continue; @@ -359,12 +336,15 @@ impl SubnetService { "Sync committee subscription is past expiration"; "subnet" => ?subnet, "current_slot" => ?current_slot, - "unsubscribe_slot" => ?slot_when_required + sync_committee_duration_in_slots, - - ); + "unsubscribe_slot" => ?slot_required_until, ); + continue; } - self.subscribe_to_sync_subnet(subnet, duration_to_unsubscribe); + self.subscribe_to_sync_subnet( + subnet, + duration_to_unsubscribe, + slot_required_until, + ); } } } @@ -516,7 +496,12 @@ impl SubnetService { } /// Adds a subscription event and an associated unsubscription event if required. - fn subscribe_to_sync_subnet(&mut self, subnet: Subnet, duration_to_unsubscribe: Duration) { + fn subscribe_to_sync_subnet( + &mut self, + subnet: Subnet, + duration_to_unsubscribe: Duration, + slot_required_until: Slot, + ) { // Return if we have subscribed to all subnets if self.subscribe_all_subnets { return; @@ -540,7 +525,7 @@ impl SubnetService { self.subscriptions .insert_at(subnet, duration_to_unsubscribe); // We are not currently subscribed and have no waiting subscription, create one - debug!(self.log, "Subscribing to subnet"; "subnet" => ?subnet, "until" => ?duration_to_unsubscribe); + debug!(self.log, "Subscribing to subnet"; "subnet" => ?subnet, "until" => ?slot_required_until); self.events .push_back(SubnetServiceMessage::Subscribe(subnet)); diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index f07fdb964d6..673e7da4000 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -20,6 +20,10 @@ use types::{ SyncCommitteeSubscription, SyncSubnetId, ValidatorSubscription, }; +// Set to enable/disable logging +const TEST_LOG_LEVEL: Option = Some(slog::Level::Debug); +//const TEST_LOG_LEVEL: Option = None; + const SLOT_DURATION_MILLIS: u64 = 400; type TestBeaconChainType = Witness< @@ -113,8 +117,8 @@ lazy_static! { static ref CHAIN: TestBeaconChain = TestBeaconChain::new_with_system_clock(); } -fn get_attestation_service(log_level: Option) -> SubnetService { - let log = get_logger(log_level); +fn get_subnet_service() -> SubnetService { + let log = get_logger(TEST_LOG_LEVEL); let config = NetworkConfig::default(); let beacon_chain = CHAIN.chain.clone(); @@ -160,10 +164,10 @@ async fn get_events + Unpin>( events } -mod attestation_service { +mod test { #[cfg(not(windows))] - use crate::subnet_service::attestation_subnets::MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD; + use crate::subnet_service::MIN_PEER_DISCOVERY_SLOT_LOOK_AHEAD; use super::*; @@ -172,13 +176,13 @@ mod attestation_service { slot: Slot, committee_count_at_slot: u64, is_aggregator: bool, - ) -> ValidatorSubscription { - ValidatorSubscription { + ) -> Subscription { + Subscription::Attestation(ValidatorSubscription { attestation_committee_index, slot, committee_count_at_slot, is_aggregator, - } + }) } fn get_subscriptions( @@ -186,7 +190,7 @@ mod attestation_service { slot: Slot, committee_count_at_slot: u64, is_aggregator: bool, - ) -> Vec { + ) -> Vec { (0..validator_count) .map(|validator_index| { get_subscription( @@ -204,71 +208,79 @@ mod attestation_service { // subscription config let committee_index = 1; // Keep a low subscription slot so that there are no additional subnet discovery events. - let subscription_slot = 0; - let committee_count = 1; let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; // create the attestation service and subscriptions - let mut attestation_service = get_attestation_service(None); - let current_slot = attestation_service + let mut subnet_service = get_subnet_service(); + let _events = get_events(&mut subnet_service, None, 1).await; + + let current_slot = subnet_service .beacon_chain .slot_clock .now() .expect("Could not get current slot"); + // Generate a subnet that isn't in our permanent subnet collection + let subscription_slot = current_slot + 1; + let mut committee_count = 1; + let mut subnet = Subnet::Attestation( + SubnetId::compute_subnet::( + current_slot, + committee_index, + committee_count, + &subnet_service.beacon_chain.spec, + ) + .unwrap(), + ); + while subnet_service + .permanent_subscriptions() + .any(|x| *x == subnet) + { + committee_count += 1; + subnet = Subnet::Attestation( + SubnetId::compute_subnet::( + subscription_slot, + committee_index, + committee_count, + &subnet_service.beacon_chain.spec, + ) + .unwrap(), + ); + } + let subscriptions = vec![get_subscription( committee_index, - current_slot + Slot::new(subscription_slot), + current_slot, committee_count, true, )]; // submit the subscriptions - attestation_service + subnet_service .validator_subscriptions(subscriptions.into_iter()) .unwrap(); // not enough time for peer discovery, just subscribe, unsubscribe - let subnet_id = SubnetId::compute_subnet::( - current_slot + Slot::new(subscription_slot), - committee_index, - committee_count, - &attestation_service.beacon_chain.spec, - ) - .unwrap(); let expected = [ - SubnetServiceMessage::Subscribe(Subnet::Attestation(subnet_id)), - SubnetServiceMessage::Unsubscribe(Subnet::Attestation(subnet_id)), + SubnetServiceMessage::Subscribe(subnet), + SubnetServiceMessage::Unsubscribe(subnet), ]; // Wait for 1 slot duration to get the unsubscription event let events = get_events( - &mut attestation_service, - Some(subnets_per_node * 3 + 2), - (MainnetEthSpec::slots_per_epoch() * 3) as u32, + &mut subnet_service, + Some(2), + (MainnetEthSpec::slots_per_epoch()) as u32, ) .await; - matches::assert_matches!( - events[..6], - [ - SubnetServiceMessage::Subscribe(_any1), - SubnetServiceMessage::EnrAdd(_any3), - SubnetServiceMessage::DiscoverPeers(_), - SubnetServiceMessage::Subscribe(_), - SubnetServiceMessage::EnrAdd(_), - SubnetServiceMessage::DiscoverPeers(_), - ] - ); + assert_eq!(events, expected); - // If the long lived and short lived subnets are the same, there should be no more events - // as we don't resubscribe already subscribed subnets. - if !attestation_service - .is_subscribed(&subnet_id, attestation_subnets::SubscriptionKind::LongLived) - { - assert_eq!(expected[..], events[subnets_per_node * 3..]); - } - // Should be subscribed to only subnets_per_node long lived subnet after unsubscription. - assert_eq!(attestation_service.subscription_count(), subnets_per_node); + // Should be subscribed to only subnets_per_node permananet subnet after unsubscription. + assert_eq!( + subnet_service.permanent_subscriptions().count(), + subnets_per_node + ); + assert_eq!(subnet_service.subscriptions().count(), 0); } /// Test to verify that we are not unsubscribing to a subnet before a required subscription. @@ -277,7 +289,6 @@ mod attestation_service { async fn test_same_subnet_unsubscription() { // subscription config let committee_count = 1; - let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; // Makes 2 validator subscriptions to the same subnet but at different slots. // There should be just 1 unsubscription event for the later slot subscription (subscription_slot2). @@ -287,8 +298,9 @@ mod attestation_service { let com2 = 0; // create the attestation service and subscriptions - let mut attestation_service = get_attestation_service(None); - let current_slot = attestation_service + let mut subnet_service = get_subnet_service(); + let _events = get_events(&mut subnet_service, None, 0).await; + let current_slot = subnet_service .beacon_chain .slot_clock .now() @@ -312,7 +324,7 @@ mod attestation_service { current_slot + Slot::new(subscription_slot1), com1, committee_count, - &attestation_service.beacon_chain.spec, + &subnet_service.beacon_chain.spec, ) .unwrap(); @@ -320,7 +332,7 @@ mod attestation_service { current_slot + Slot::new(subscription_slot2), com2, committee_count, - &attestation_service.beacon_chain.spec, + &subnet_service.beacon_chain.spec, ) .unwrap(); @@ -329,110 +341,83 @@ mod attestation_service { assert_eq!(subnet_id1, subnet_id2); // submit the subscriptions - attestation_service + subnet_service .validator_subscriptions(vec![sub1, sub2].into_iter()) .unwrap(); // Unsubscription event should happen at slot 2 (since subnet id's are the same, unsubscription event should be at higher slot + 1) - // Get all events for 1 slot duration (unsubscription event should happen after 2 slot durations). - let events = get_events(&mut attestation_service, None, 1).await; - matches::assert_matches!( - events[..3], - [ - SubnetServiceMessage::Subscribe(_any1), - SubnetServiceMessage::EnrAdd(_any3), - SubnetServiceMessage::DiscoverPeers(_), - ] - ); - let expected = SubnetServiceMessage::Subscribe(Subnet::Attestation(subnet_id1)); - // Should be still subscribed to 2 long lived and up to 1 short lived subnet if both are - // different. - if !attestation_service.is_subscribed( - &subnet_id1, - attestation_subnets::SubscriptionKind::LongLived, - ) { - // The index is 3*subnets_per_node (because we subscribe + discover + enr per long lived - // subnet) + 1 - let index = 3 * subnets_per_node; - assert_eq!(expected, events[index]); - assert_eq!( - attestation_service.subscription_count(), - subnets_per_node + 1 - ); + if subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) { + // If we are permanently subscribed to this subnet, we won't see a subscribe message + let _ = get_events(&mut subnet_service, None, 1).await; } else { - assert!(attestation_service.subscription_count() == subnets_per_node); + let subscription = get_events(&mut subnet_service, None, 1).await; + assert_eq!(subscription, [expected]); } // Get event for 1 more slot duration, we should get the unsubscribe event now. - let unsubscribe_event = get_events(&mut attestation_service, None, 1).await; + let unsubscribe_event = get_events(&mut subnet_service, None, 1).await; // If the long lived and short lived subnets are different, we should get an unsubscription // event. - if !attestation_service.is_subscribed( - &subnet_id1, - attestation_subnets::SubscriptionKind::LongLived, - ) { - assert_eq!( - [SubnetServiceMessage::Unsubscribe(Subnet::Attestation( - subnet_id1 - ))], - unsubscribe_event[..] - ); + let expected = SubnetServiceMessage::Unsubscribe(Subnet::Attestation(subnet_id1)); + if !subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) { + assert_eq!([expected], unsubscribe_event[..]); } // Should be subscribed 2 long lived subnet after unsubscription. - assert_eq!(attestation_service.subscription_count(), subnets_per_node); + assert_eq!(subnet_service.subscriptions().count(), 0); } #[tokio::test] async fn subscribe_all_subnets() { let attestation_subnet_count = MainnetEthSpec::default_spec().attestation_subnet_count; let subscription_slot = 3; - let subscription_count = attestation_subnet_count; + let subscriptions_count = attestation_subnet_count; let committee_count = 1; let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; // create the attestation service and subscriptions - let mut attestation_service = get_attestation_service(None); - let current_slot = attestation_service + let mut subnet_service = get_subnet_service(); + let current_slot = subnet_service .beacon_chain .slot_clock .now() .expect("Could not get current slot"); let subscriptions = get_subscriptions( - subscription_count, + subscriptions_count, current_slot + subscription_slot, committee_count, true, ); // submit the subscriptions - attestation_service + subnet_service .validator_subscriptions(subscriptions.into_iter()) .unwrap(); - let events = get_events(&mut attestation_service, Some(131), 10).await; + let events = get_events(&mut subnet_service, Some(130), 10).await; let mut discover_peer_count = 0; let mut enr_add_count = 0; - let mut unexpected_msg_count = 0; let mut unsubscribe_event_count = 0; + let mut subscription_event_count = 0; for event in &events { match event { SubnetServiceMessage::DiscoverPeers(_) => discover_peer_count += 1, - SubnetServiceMessage::Subscribe(_any_subnet) => {} + SubnetServiceMessage::Subscribe(_any_subnet) => subscription_event_count += 1, SubnetServiceMessage::EnrAdd(_any_subnet) => enr_add_count += 1, SubnetServiceMessage::Unsubscribe(_) => unsubscribe_event_count += 1, - _ => unexpected_msg_count += 1, } } - // There should be a Subscribe Event, and Enr Add event and a DiscoverPeers event for each - // long-lived subnet initially. The next event should be a bulk discovery event. - let bulk_discovery_index = 3 * subnets_per_node; + // There should be a Subscribe Event, an Enr Add event for each + // permanent subnet initially. There is a single discovery event for the permanent + // subnets. + // The next event should be a bulk discovery event. + let bulk_discovery_index = subnets_per_node * 2 + 1; // The bulk discovery request length should be equal to validator_count let bulk_discovery_event = &events[bulk_discovery_index]; if let SubnetServiceMessage::DiscoverPeers(d) = bulk_discovery_event { @@ -443,14 +428,13 @@ mod attestation_service { // 64 `DiscoverPeer` requests of length 1 corresponding to deterministic subnets // and 1 `DiscoverPeer` request corresponding to bulk subnet discovery. - assert_eq!(discover_peer_count, subnets_per_node + 1); - assert_eq!(attestation_service.subscription_count(), subnets_per_node); + assert_eq!(discover_peer_count, 1 + 1); + assert_eq!(subscription_event_count, attestation_subnet_count); assert_eq!(enr_add_count, subnets_per_node); assert_eq!( unsubscribe_event_count, attestation_subnet_count - subnets_per_node as u64 ); - assert_eq!(unexpected_msg_count, 0); // test completed successfully } @@ -461,30 +445,30 @@ mod attestation_service { let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; // the 65th subscription should result in no more messages than the previous scenario - let subscription_count = attestation_subnet_count + 1; + let subscriptions_count = attestation_subnet_count + 1; let committee_count = 1; // create the attestation service and subscriptions - let mut attestation_service = get_attestation_service(None); - let current_slot = attestation_service + let mut subnet_service = get_subnet_service(); + let current_slot = subnet_service .beacon_chain .slot_clock .now() .expect("Could not get current slot"); let subscriptions = get_subscriptions( - subscription_count, + subscriptions_count, current_slot + subscription_slot, committee_count, true, ); // submit the subscriptions - attestation_service + subnet_service .validator_subscriptions(subscriptions.into_iter()) .unwrap(); - let events = get_events(&mut attestation_service, None, 3).await; + let events = get_events(&mut subnet_service, None, 3).await; let mut discover_peer_count = 0; let mut enr_add_count = 0; let mut unexpected_msg_count = 0; @@ -494,7 +478,10 @@ mod attestation_service { SubnetServiceMessage::DiscoverPeers(_) => discover_peer_count += 1, SubnetServiceMessage::Subscribe(_any_subnet) => {} SubnetServiceMessage::EnrAdd(_any_subnet) => enr_add_count += 1, - _ => unexpected_msg_count += 1, + _ => { + unexpected_msg_count += 1; + println!("{:?}", event); + } } } @@ -508,8 +495,8 @@ mod attestation_service { // subnets_per_node `DiscoverPeer` requests of length 1 corresponding to long-lived subnets // and 1 `DiscoverPeer` request corresponding to the bulk subnet discovery. - assert_eq!(discover_peer_count, subnets_per_node + 1); - assert_eq!(attestation_service.subscription_count(), subnets_per_node); + assert_eq!(discover_peer_count, 1 + 1); // Generates a single discovery for permanent + // subscriptions and 1 for the subscription assert_eq!(enr_add_count, subnets_per_node); assert_eq!(unexpected_msg_count, 0); } @@ -519,7 +506,6 @@ mod attestation_service { async fn test_subscribe_same_subnet_several_slots_apart() { // subscription config let committee_count = 1; - let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; // Makes 2 validator subscriptions to the same subnet but at different slots. // There should be just 1 unsubscription event for the later slot subscription (subscription_slot2). @@ -529,8 +515,11 @@ mod attestation_service { let com2 = 0; // create the attestation service and subscriptions - let mut attestation_service = get_attestation_service(None); - let current_slot = attestation_service + let mut subnet_service = get_subnet_service(); + // Remove permanent events + let _events = get_events(&mut subnet_service, None, 0).await; + + let current_slot = subnet_service .beacon_chain .slot_clock .now() @@ -554,7 +543,7 @@ mod attestation_service { current_slot + Slot::new(subscription_slot1), com1, committee_count, - &attestation_service.beacon_chain.spec, + &subnet_service.beacon_chain.spec, ) .unwrap(); @@ -562,7 +551,7 @@ mod attestation_service { current_slot + Slot::new(subscription_slot2), com2, committee_count, - &attestation_service.beacon_chain.spec, + &subnet_service.beacon_chain.spec, ) .unwrap(); @@ -571,39 +560,28 @@ mod attestation_service { assert_eq!(subnet_id1, subnet_id2); // submit the subscriptions - attestation_service + subnet_service .validator_subscriptions(vec![sub1, sub2].into_iter()) .unwrap(); // Unsubscription event should happen at the end of the slot. - let events = get_events(&mut attestation_service, None, 1).await; - matches::assert_matches!( - events[..3], - [ - SubnetServiceMessage::Subscribe(_any1), - SubnetServiceMessage::EnrAdd(_any3), - SubnetServiceMessage::DiscoverPeers(_), - ] - ); + let events = get_events(&mut subnet_service, None, 1).await; let expected_subscription = SubnetServiceMessage::Subscribe(Subnet::Attestation(subnet_id1)); let expected_unsubscription = SubnetServiceMessage::Unsubscribe(Subnet::Attestation(subnet_id1)); - if !attestation_service.is_subscribed( - &subnet_id1, - attestation_subnets::SubscriptionKind::LongLived, - ) { - assert_eq!(expected_subscription, events[subnets_per_node * 3]); - assert_eq!(expected_unsubscription, events[subnets_per_node * 3 + 2]); + if !subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) { + assert_eq!(expected_subscription, events[0]); + assert_eq!(expected_unsubscription, events[2]); } - assert_eq!(attestation_service.subscription_count(), 2); + assert_eq!(subnet_service.subscriptions().count(), 0); println!("{events:?}"); let subscription_slot = current_slot + subscription_slot2 - 1; // one less do to the // advance subscription time - let wait_slots = attestation_service + let wait_slots = subnet_service .beacon_chain .slot_clock .duration_to_slot(subscription_slot) @@ -611,70 +589,20 @@ mod attestation_service { .as_millis() as u64 / SLOT_DURATION_MILLIS; - let no_events = dbg!(get_events(&mut attestation_service, None, wait_slots as u32).await); + let no_events = dbg!(get_events(&mut subnet_service, None, wait_slots as u32).await); assert_eq!(no_events, []); - let second_subscribe_event = get_events(&mut attestation_service, None, 2).await; - // If the long lived and short lived subnets are different, we should get an unsubscription event. - if !attestation_service.is_subscribed( - &subnet_id1, - attestation_subnets::SubscriptionKind::LongLived, - ) { + let second_subscribe_event = get_events(&mut subnet_service, None, 2).await; + // If the permanent and short lived subnets are different, we should get an unsubscription event. + if !subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) { assert_eq!( - [SubnetServiceMessage::Subscribe(Subnet::Attestation( - subnet_id1 - ))], + [expected_subscription, expected_unsubscription], second_subscribe_event[..] ); } } - #[tokio::test] - async fn test_update_deterministic_long_lived_subnets() { - let mut attestation_service = get_attestation_service(None); - let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; - - let current_slot = attestation_service - .beacon_chain - .slot_clock - .now() - .expect("Could not get current slot"); - - let subscriptions = get_subscriptions(20, current_slot, 30, false); - - // submit the subscriptions - attestation_service - .validator_subscriptions(subscriptions.into_iter()) - .unwrap(); - - // There should only be the same subscriptions as there are in the specification, - // regardless of subscriptions - assert_eq!( - attestation_service.long_lived_subscriptions().len(), - subnets_per_node - ); - - let events = get_events(&mut attestation_service, None, 4).await; - - // Check that we attempt to subscribe and register ENRs - matches::assert_matches!( - events[..6], - [ - SubnetServiceMessage::Subscribe(_), - SubnetServiceMessage::EnrAdd(_), - SubnetServiceMessage::DiscoverPeers(_), - SubnetServiceMessage::Subscribe(_), - SubnetServiceMessage::EnrAdd(_), - SubnetServiceMessage::DiscoverPeers(_), - ] - ); - } -} - -mod sync_committee_service { - use super::*; - #[tokio::test] async fn subscribe_and_unsubscribe() { // subscription config @@ -683,19 +611,23 @@ mod sync_committee_service { let sync_committee_indices = vec![1]; // create the attestation service and subscriptions - let mut sync_committee_service = get_sync_committee_service(); + let mut subnet_service = get_subnet_service(); + let _events = get_events(&mut subnet_service, None, 0).await; - let subscriptions = vec![SyncCommitteeSubscription { - validator_index, - sync_committee_indices: sync_committee_indices.clone(), - until_epoch, - }]; + let subscriptions = + std::iter::once(Subscription::SyncCommittee(SyncCommitteeSubscription { + validator_index, + sync_committee_indices: sync_committee_indices.clone(), + until_epoch, + })); // submit the subscriptions - sync_committee_service + subnet_service .validator_subscriptions(subscriptions) .unwrap(); + // Remove permanent subscription events + let subnet_ids = SyncSubnetId::compute_subnets_for_sync_committee::( &sync_committee_indices, ) @@ -704,8 +636,8 @@ mod sync_committee_service { // Note: the unsubscription event takes 2 epochs (8 * 2 * 0.4 secs = 3.2 secs) let events = get_events( - &mut sync_committee_service, - Some(5), + &mut subnet_service, + Some(4), (MainnetEthSpec::slots_per_epoch() * 3) as u32, // Have some buffer time before getting 5 events ) .await; @@ -721,12 +653,11 @@ mod sync_committee_service { [ SubnetServiceMessage::DiscoverPeers(_), SubnetServiceMessage::Unsubscribe(_), - SubnetServiceMessage::EnrRemove(_), ] ); // Should be unsubscribed at the end. - assert_eq!(sync_committee_service.subscription_count(), 0); + assert_eq!(subnet_service.subscriptions().count(), 0); } #[tokio::test] @@ -737,21 +668,24 @@ mod sync_committee_service { let sync_committee_indices = vec![1]; // create the attestation service and subscriptions - let mut sync_committee_service = get_sync_committee_service(); + let mut subnet_service = get_subnet_service(); + // Get the initial events from permanent subnet subscriptions + let _events = get_events(&mut subnet_service, None, 1).await; - let subscriptions = vec![SyncCommitteeSubscription { - validator_index, - sync_committee_indices: sync_committee_indices.clone(), - until_epoch, - }]; + let subscriptions = + std::iter::once(Subscription::SyncCommittee(SyncCommitteeSubscription { + validator_index, + sync_committee_indices: sync_committee_indices.clone(), + until_epoch, + })); // submit the subscriptions - sync_committee_service + subnet_service .validator_subscriptions(subscriptions) .unwrap(); // Get all immediate events (won't include unsubscriptions) - let events = get_events(&mut sync_committee_service, None, 1).await; + let events = get_events(&mut subnet_service, None, 1).await; matches::assert_matches!( events[..], [ @@ -765,28 +699,32 @@ mod sync_committee_service { // Event 1 is a duplicate of an existing subscription // Event 2 is the same subscription with lower `until_epoch` than the existing subscription let subscriptions = vec![ - SyncCommitteeSubscription { + Subscription::SyncCommittee(SyncCommitteeSubscription { validator_index, sync_committee_indices: sync_committee_indices.clone(), until_epoch, - }, - SyncCommitteeSubscription { + }), + Subscription::SyncCommittee(SyncCommitteeSubscription { validator_index, sync_committee_indices: sync_committee_indices.clone(), until_epoch: until_epoch - 1, - }, + }), ]; // submit the subscriptions - sync_committee_service - .validator_subscriptions(subscriptions) + subnet_service + .validator_subscriptions(subscriptions.into_iter()) .unwrap(); // Get all immediate events (won't include unsubscriptions) - let events = get_events(&mut sync_committee_service, None, 1).await; + let events = get_events(&mut subnet_service, None, 1).await; matches::assert_matches!(events[..], [SubnetServiceMessage::DiscoverPeers(_),]); // Should be unsubscribed at the end. - assert_eq!(sync_committee_service.subscription_count(), 1); + let sync_committee_subscriptions = subnet_service + .subscriptions() + .filter(|s| matches!(s, Subnet::SyncCommittee(_))) + .count(); + assert_eq!(sync_committee_subscriptions, 1); } } From 1a89fb4664dbe284d812b4209ed5f13f87f200db Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 9 Sep 2024 12:27:19 +1000 Subject: [PATCH 04/10] Correct comments and reviewers comments --- beacon_node/network/src/service.rs | 2 +- beacon_node/network/src/subnet_service/mod.rs | 50 +++++++++---------- .../network/src/subnet_service/tests/mod.rs | 3 +- consensus/types/src/subnet_id.rs | 2 +- 4 files changed, 27 insertions(+), 30 deletions(-) diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index c659cb52b43..4fcda345669 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -166,7 +166,7 @@ pub struct NetworkService { beacon_chain: Arc>, /// The underlying libp2p service that drives all the network interactions. libp2p: Network, - /// An attestation and subnet manager service. + /// An attestation and sync committee subnet manager service. subnet_service: SubnetService, /// The receiver channel for lighthouse to communicate with the network service. network_recv: mpsc::UnboundedReceiver>, diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 80ca4239440..050f848c3cf 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -17,7 +17,7 @@ use lighthouse_network::{discv5::enr::NodeId, NetworkConfig, Subnet, SubnetDisco use slog::{debug, error, o, warn}; use slot_clock::SlotClock; use types::{ - Attestation, EthSpec, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, + Attestation, EthSpec, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, ValidatorSubscription, }; @@ -51,17 +51,14 @@ const ADVANCE_SUBSCRIBE_SLOT_FRACTION: u32 = 1; /// `aggregate_validators_on_subnet` delay map. const UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY: u32 = 2; -/// A particular subnet at a given slot. +/// A particular subnet at a given slot. This is used for Attestation subnets and not for sync +/// committee subnets because the logic for handling subscriptions between these types is different. #[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)] pub struct ExactSubnet { /// The `SubnetId` associated with this subnet. pub subnet: Subnet, /// For Attestations, this slot represents the start time at which we need to subscribe to the - /// slot. For SyncCommittee subnet id's this represents the end slot at which we no longer need - /// to subscribe to the subnet. - // NOTE: There was different logic between the two subscriptions and having a different - // interpretation of this variable seemed like the best way to group the logic, even though it - // may be counter-intuitive (apologies to future readers). + /// slot. pub slot: Slot, } @@ -123,7 +120,7 @@ impl SubnetService { config: &NetworkConfig, log: &slog::Logger, ) -> Self { - let log = log.new(o!("service" => "attestation_service")); + let log = log.new(o!("service" => "subnet_service")); let slot_duration = beacon_chain.slot_clock.slot_duration(); @@ -136,16 +133,15 @@ impl SubnetService { let mut permanent_attestation_subscriptions = HashSet::default(); if config.subscribe_all_subnets { // We are subscribed to all subnets, set all the bits to true. - for index in 0..::SubnetBitfieldLength::to_u64() { + for index in 0..beacon_chain.spec.attestation_subnet_count { permanent_attestation_subscriptions .insert(Subnet::Attestation(SubnetId::from(index))); } } else { // Not subscribed to all subnets, so just calculate the required subnets from the - for subnet_id in SubnetId::compute_attestation_subnets::( - node_id.raw().into(), - &beacon_chain.spec, - ) { + for subnet_id in + SubnetId::compute_attestation_subnets(node_id.raw().into(), &beacon_chain.spec) + { permanent_attestation_subscriptions.insert(Subnet::Attestation(subnet_id)); } } @@ -165,16 +161,18 @@ impl SubnetService { let mut events = VecDeque::with_capacity(10); // Queue discovery queries for the permanent attestation subnets - events.push_back(SubnetServiceMessage::DiscoverPeers( - permanent_attestation_subscriptions - .iter() - .cloned() - .map(|subnet| SubnetDiscovery { - subnet, - min_ttl: None, - }) - .collect(), - )); + if !config.disable_discovery { + events.push_back(SubnetServiceMessage::DiscoverPeers( + permanent_attestation_subscriptions + .iter() + .cloned() + .map(|subnet| SubnetDiscovery { + subnet, + min_ttl: None, + }) + .collect(), + )); + } // Pre-populate the events with permanent subscriptions for subnet in permanent_attestation_subscriptions.iter() { @@ -507,7 +505,7 @@ impl SubnetService { return; } - // Return if we already have a subscription for the subnet and its closer or + // Update the unsubscription duration if we already have a subscription for the subnet if let Some(current_instant_to_unsubscribe) = self.subscriptions.deadline(&subnet) { // The extra 500ms in the comparison accounts of the inaccuracy of the underlying // DelayQueue inside the delaymap struct. @@ -595,9 +593,7 @@ impl SubnetService { Ok(()) } - // Unsubscribes from a subnet that was removed if it does not continue to exist as a - // subscription of the other kind. For long lived subscriptions, it also removes the - // advertisement from our ENR. + // Unsubscribes from a subnet that was removed. fn handle_removed_subnet(&mut self, subnet: Subnet) { if !self.subscriptions.contains_key(&subnet) { // Subscription no longer exists as short lived or long lived. diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 673e7da4000..e6161c6a44e 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -7,6 +7,7 @@ use beacon_chain::{ use genesis::{generate_deterministic_keypairs, interop_genesis_state, DEFAULT_ETH1_BLOCK_HASH}; use lazy_static::lazy_static; use lighthouse_network::NetworkConfig; +use logging::test_logger; use slog::{o, Drain, Logger}; use sloggers::{null::NullLoggerBuilder, Build}; use slot_clock::{SlotClock, SystemTimeSlotClock}; @@ -118,7 +119,7 @@ lazy_static! { } fn get_subnet_service() -> SubnetService { - let log = get_logger(TEST_LOG_LEVEL); + let log = test_logger(); let config = NetworkConfig::default(); let beacon_chain = CHAIN.chain.clone(); diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 099ec9c5cf5..7fd5b83e349 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -77,7 +77,7 @@ impl SubnetId { /// 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. #[allow(clippy::arithmetic_side_effects)] - pub fn compute_attestation_subnets( + pub fn compute_attestation_subnets( node_id: ethereum_types::U256, spec: &ChainSpec, ) -> impl Iterator { From 03b404990ac9026c0d9e59349181017ac45edd74 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 9 Sep 2024 12:39:10 +1000 Subject: [PATCH 05/10] Fix errors --- beacon_node/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 899ebdcaac8..e18c0b9c8b5 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -158,7 +158,7 @@ pub struct NetworkService { /// A reference to the underlying beacon chain. beacon_chain: Arc>, /// The underlying libp2p service that drives all the network interactions. - libp2p: Network, + libp2p: Network, /// An attestation and sync committee subnet manager service. subnet_service: SubnetService, /// The receiver channel for lighthouse to communicate with the network service. From 1b6833f5770f3769cb4bcdac20a89a77974f3646 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 9 Sep 2024 12:42:23 +1000 Subject: [PATCH 06/10] Missed a comment, corrected it --- beacon_node/network/src/subnet_service/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 050f848c3cf..6b1a3fe371d 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -560,10 +560,9 @@ impl SubnetService { return Err("Time when subscription would end has already passed."); } - // We need to check and add a subscription for the right kind, regardless of the presence - // of the subnet as a subscription of the other kind. This is mainly since long lived - // subscriptions can be removed at any time when a validator goes offline. - + // Check if we already have this subscription. If we do, optionally update the timeout of + // when we need the subscription, otherwise leave as is. + // If this is a new subscription simply add it to our mapping and subscribe. match self.subscriptions.deadline(&subnet) { Some(current_end_slot_time) => { // We are already subscribed. Check if we need to extend the subscription. From 0a20547e3d9c893fc99befdd9b68f95d26743871 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 19 Sep 2024 11:38:21 +1000 Subject: [PATCH 07/10] Fix lints --- beacon_node/network/src/subnet_service/mod.rs | 2 +- beacon_node/network/src/subnet_service/tests/mod.rs | 4 ++-- consensus/types/src/subnet_id.rs | 10 ++-------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/beacon_node/network/src/subnet_service/mod.rs b/beacon_node/network/src/subnet_service/mod.rs index 6b1a3fe371d..7e09549f1b0 100644 --- a/beacon_node/network/src/subnet_service/mod.rs +++ b/beacon_node/network/src/subnet_service/mod.rs @@ -140,7 +140,7 @@ impl SubnetService { } else { // Not subscribed to all subnets, so just calculate the required subnets from the for subnet_id in - SubnetId::compute_attestation_subnets(node_id.raw().into(), &beacon_chain.spec) + SubnetId::compute_attestation_subnets(node_id.raw(), &beacon_chain.spec) { permanent_attestation_subscriptions.insert(Subnet::Attestation(subnet_id)); } diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 752f8d1c450..1e4d7b08381 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -22,7 +22,7 @@ use types::{ // Set to enable/disable logging const TEST_LOG_LEVEL: Option = Some(slog::Level::Debug); -//const TEST_LOG_LEVEL: Option = None; +// const TEST_LOG_LEVEL: Option = None; const SLOT_DURATION_MILLIS: u64 = 400; @@ -45,7 +45,7 @@ impl TestBeaconChain { let keypairs = generate_deterministic_keypairs(1); - let log = get_logger(None); + let log = get_logger(TEST_LOG_LEVEL); let store = HotColdDB::open_ephemeral(StoreConfig::default(), spec.clone(), log.clone()).unwrap(); diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 7c6327c3295..d3724d40c3d 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -172,17 +172,11 @@ mod tests { for x in 0..node_ids.len() { println!("Test: {}", x); println!( -<<<<<<< HEAD - "NodeId: {}\nExpected_subnets: {:?}", + "NodeId: {:?}\nExpected_subnets: {:?}", node_ids[x], expected_subnets[x] -======= - "NodeId: {:?}\n Epoch: {}\n, expected_update_time: {}\n, expected_subnets: {:?}", - node_ids[x], epochs[x], expected_valid_time[x], expected_subnets[x] ->>>>>>> network/unstable ); - let computed_subnets = - SubnetId::compute_attestation_subnets::(node_ids[x], &spec); + let computed_subnets = SubnetId::compute_attestation_subnets(node_ids[x], &spec); assert_eq!( expected_subnets[x], From 6ca4a025909a55461584936b2beebabfeb690e80 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 19 Sep 2024 12:06:06 +1000 Subject: [PATCH 08/10] Fix tests --- beacon_node/network/src/service/tests.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/beacon_node/network/src/service/tests.rs b/beacon_node/network/src/service/tests.rs index fec5f3f83f7..c5fd638dda6 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; @@ -167,21 +167,18 @@ mod tests { // Subscribe to the topics. runtime.block_on(async { while network_globals.gossipsub_subscriptions.read().len() < 2 { - if let Some(msg) = network_service.attestation_service.next().await { - network_service.on_attestation_service_msg(msg); + if let Some(msg) = network_service.subnet_service.next().await { + network_service.on_subnet_service_msg(msg); } } }); // Make sure the service is subscribed to the topics. let (old_topic1, old_topic2) = { - let mut subnets = SubnetId::compute_subnets_for_epoch::( + let mut subnets = SubnetId::compute_attestation_subnets( network_globals.local_enr().node_id().raw(), - beacon_chain.epoch().unwrap(), &spec, ) - .unwrap() - .0 .collect::>(); assert_eq!(2, subnets.len()); From 8c7ba308ace7d698805783477edb3f0fa2ad56da Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 28 Oct 2024 20:34:42 +1100 Subject: [PATCH 09/10] 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 { From 51cbf1c71f7944d1d4d814a9a8b050d699f42675 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 25 Nov 2024 15:56:54 +1100 Subject: [PATCH 10/10] Prevent clash with pin of rust_eth_kzg --- Cargo.lock | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 062ea0fc7b2..d7ce7b9f6c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1558,9 +1558,9 @@ dependencies = [ [[package]] name = "crate_crypto_internal_eth_kzg_bls12_381" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "319cb241b1ad37f8c376b2ebcd7233f53e033a50de6f0a9cf38e6cc545554de4" +checksum = "a23be5253f1bd7fd411721a58712308c4747d0a41d040bbf8ebb78d52909a480" dependencies = [ "blst", "blstrs", @@ -1572,9 +1572,9 @@ dependencies = [ [[package]] name = "crate_crypto_internal_eth_kzg_erasure_codes" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "abedcc3eb4d04655c53704a66880945f5a39c6d96b337f0ab4a086dda795c954" +checksum = "f2067ce20ef380ff33a93ce0af62bea22d35531b7f3586224d8d5176ec6cf578" dependencies = [ "crate_crypto_internal_eth_kzg_bls12_381", "crate_crypto_internal_eth_kzg_polynomial", @@ -1582,24 +1582,24 @@ dependencies = [ [[package]] name = "crate_crypto_internal_eth_kzg_maybe_rayon" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3db089718921ca28d137ed8b1a7406f10e6bfbc61794a2339bcd9c99d5ddefc6" +checksum = "558f50324ff016e5fe93113c78a72776d790d52f244ae9602a8013a67a189b66" [[package]] name = "crate_crypto_internal_eth_kzg_polynomial" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "750e6dbe493d337d98502975137f055a52767021bcdb69aa8926ed8ee28c7980" +checksum = "9e051c4f5aa5696bd7c504930485436ec62bf14f30a4c2d78504f3f8ec6a3daf" dependencies = [ "crate_crypto_internal_eth_kzg_bls12_381", ] [[package]] name = "crate_crypto_kzg_multi_open_fk20" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c3337262f48c7fee2999cf23bb9cb693299211671fe46b593ac81fcb218de68b" +checksum = "66ed6bf8993d9f3b361da4ed38f067503e08c0b948af0d6f4bb941dd647c0f2c" dependencies = [ "crate_crypto_internal_eth_kzg_bls12_381", "crate_crypto_internal_eth_kzg_maybe_rayon", @@ -7360,9 +7360,9 @@ dependencies = [ [[package]] name = "rust_eth_kzg" -version = "0.5.2" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1407734b92b14e4b9a4d2925e2f9d2c7413f3bc16866cbb1261c9c5e5bf0f04f" +checksum = "3291fd0d9c629a56537d74bbc1e7bcaf5be610f2f7b55af85c4fea843c6aeca3" dependencies = [ "crate_crypto_internal_eth_kzg_bls12_381", "crate_crypto_internal_eth_kzg_erasure_codes",