From 84a902a5890e0354aa464d73ed1c3ef669d2ad10 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 7 Mar 2024 18:02:27 +0530 Subject: [PATCH] Reduce load on validator subscription channels (#5311) * Fix tests * Merge branch 'unstable' into unclog-channels * Avoid reallocations * Reduce subscription load on beacon node --- beacon_node/http_api/src/lib.rs | 56 +++++++++---------- beacon_node/network/src/service.rs | 5 +- .../src/subnet_service/attestation_subnets.rs | 5 +- .../network/src/subnet_service/tests/mod.rs | 23 ++------ consensus/types/src/validator_subscription.rs | 4 +- 5 files changed, 39 insertions(+), 54 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index b39450d735..5a8d5cae07 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3448,34 +3448,34 @@ pub fn serve( chain: Arc>, log: Logger| { task_spawner.blocking_json_task(Priority::P0, move || { - for subscription in &subscriptions { - chain - .validator_monitor - .write() - .auto_register_local_validator(subscription.validator_index); - - let validator_subscription = api_types::ValidatorSubscription { - validator_index: subscription.validator_index, - attestation_committee_index: subscription.committee_index, - slot: subscription.slot, - committee_count_at_slot: subscription.committees_at_slot, - is_aggregator: subscription.is_aggregator, - }; - - let message = ValidatorSubscriptionMessage::AttestationSubscribe { - subscriptions: vec![validator_subscription], - }; - if let Err(e) = validator_subscription_tx.try_send(message) { - warn!( - log, - "Unable to process committee subscriptions"; - "info" => "the host may be overloaded or resource-constrained", - "error" => ?e, - ); - return Err(warp_utils::reject::custom_server_error( - "unable to queue subscription, host may be overloaded or shutting down".to_string(), - )); - } + let subscriptions: std::collections::BTreeSet<_> = subscriptions + .iter() + .map(|subscription| { + chain + .validator_monitor + .write() + .auto_register_local_validator(subscription.validator_index); + api_types::ValidatorSubscription { + attestation_committee_index: subscription.committee_index, + slot: subscription.slot, + committee_count_at_slot: subscription.committees_at_slot, + is_aggregator: subscription.is_aggregator, + } + }) + .collect(); + let message = + ValidatorSubscriptionMessage::AttestationSubscribe { subscriptions }; + if let Err(e) = validator_subscription_tx.try_send(message) { + warn!( + log, + "Unable to process committee subscriptions"; + "info" => "the host may be overloaded or resource-constrained", + "error" => ?e, + ); + return Err(warp_utils::reject::custom_server_error( + "unable to queue subscription, host may be overloaded or shutting down" + .to_string(), + )); } Ok(()) diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 0905f4a07b..18284bd236 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -27,6 +27,7 @@ use lighthouse_network::{ MessageId, NetworkEvent, NetworkGlobals, PeerId, }; use slog::{crit, debug, error, info, o, trace, warn}; +use std::collections::BTreeSet; use std::{collections::HashSet, pin::Pin, sync::Arc, time::Duration}; use store::HotColdDB; use strum::IntoStaticStr; @@ -119,7 +120,7 @@ pub enum NetworkMessage { pub enum ValidatorSubscriptionMessage { /// Subscribes a list of validators to specific slots for attestation duties. AttestationSubscribe { - subscriptions: Vec, + subscriptions: BTreeSet, }, SyncCommitteeSubscribe { subscriptions: Vec, @@ -783,7 +784,7 @@ impl NetworkService { ValidatorSubscriptionMessage::AttestationSubscribe { subscriptions } => { if let Err(e) = self .attestation_service - .validator_subscriptions(subscriptions) + .validator_subscriptions(subscriptions.into_iter()) { warn!(self.log, "Attestation validator subscription failed"; "error" => e); } diff --git a/beacon_node/network/src/subnet_service/attestation_subnets.rs b/beacon_node/network/src/subnet_service/attestation_subnets.rs index 1cae6299e1..ab9ffb95a6 100644 --- a/beacon_node/network/src/subnet_service/attestation_subnets.rs +++ b/beacon_node/network/src/subnet_service/attestation_subnets.rs @@ -196,7 +196,7 @@ impl AttestationService { /// safely dropped. pub fn validator_subscriptions( &mut self, - subscriptions: Vec, + subscriptions: impl Iterator, ) -> Result<(), String> { // If the node is in a proposer-only state, we ignore all subnet subscriptions. if self.proposer_only { @@ -227,7 +227,6 @@ impl AttestationService { warn!(self.log, "Failed to compute subnet id for validator subscription"; "error" => ?e, - "validator_index" => subscription.validator_index ); continue; } @@ -257,13 +256,11 @@ impl AttestationService { warn!(self.log, "Subscription to subnet error"; "error" => e, - "validator_index" => subscription.validator_index, ); } else { trace!(self.log, "Subscribed to subnet for aggregator duties"; "exact_subnet" => ?exact_subnet, - "validator_index" => subscription.validator_index ); } } diff --git a/beacon_node/network/src/subnet_service/tests/mod.rs b/beacon_node/network/src/subnet_service/tests/mod.rs index 658c851ba2..74f3f59df3 100644 --- a/beacon_node/network/src/subnet_service/tests/mod.rs +++ b/beacon_node/network/src/subnet_service/tests/mod.rs @@ -180,14 +180,12 @@ mod attestation_service { use super::*; fn get_subscription( - validator_index: u64, attestation_committee_index: CommitteeIndex, slot: Slot, committee_count_at_slot: u64, is_aggregator: bool, ) -> ValidatorSubscription { ValidatorSubscription { - validator_index, attestation_committee_index, slot, committee_count_at_slot, @@ -204,7 +202,6 @@ mod attestation_service { (0..validator_count) .map(|validator_index| { get_subscription( - validator_index, validator_index, slot, committee_count_at_slot, @@ -217,7 +214,6 @@ mod attestation_service { #[tokio::test] async fn subscribe_current_slot_wait_for_unsubscribe() { // subscription config - let validator_index = 1; let committee_index = 1; // Keep a low subscription slot so that there are no additional subnet discovery events. let subscription_slot = 0; @@ -233,7 +229,6 @@ mod attestation_service { .expect("Could not get current slot"); let subscriptions = vec![get_subscription( - validator_index, committee_index, current_slot + Slot::new(subscription_slot), committee_count, @@ -242,7 +237,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(subscriptions) + .validator_subscriptions(subscriptions.into_iter()) .unwrap(); // not enough time for peer discovery, just subscribe, unsubscribe @@ -293,7 +288,6 @@ mod attestation_service { #[tokio::test] async fn test_same_subnet_unsubscription() { // subscription config - let validator_index = 1; let committee_count = 1; let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; @@ -313,7 +307,6 @@ mod attestation_service { .expect("Could not get current slot"); let sub1 = get_subscription( - validator_index, com1, current_slot + Slot::new(subscription_slot1), committee_count, @@ -321,7 +314,6 @@ mod attestation_service { ); let sub2 = get_subscription( - validator_index, com2, current_slot + Slot::new(subscription_slot2), committee_count, @@ -350,7 +342,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(vec![sub1, sub2]) + .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) @@ -431,7 +423,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(subscriptions) + .validator_subscriptions(subscriptions.into_iter()) .unwrap(); let events = get_events(&mut attestation_service, Some(131), 10).await; @@ -501,7 +493,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(subscriptions) + .validator_subscriptions(subscriptions.into_iter()) .unwrap(); let events = get_events(&mut attestation_service, None, 3).await; @@ -538,7 +530,6 @@ mod attestation_service { #[tokio::test] async fn test_subscribe_same_subnet_several_slots_apart() { // subscription config - let validator_index = 1; let committee_count = 1; let subnets_per_node = MainnetEthSpec::default_spec().subnets_per_node as usize; @@ -558,7 +549,6 @@ mod attestation_service { .expect("Could not get current slot"); let sub1 = get_subscription( - validator_index, com1, current_slot + Slot::new(subscription_slot1), committee_count, @@ -566,7 +556,6 @@ mod attestation_service { ); let sub2 = get_subscription( - validator_index, com2, current_slot + Slot::new(subscription_slot2), committee_count, @@ -595,7 +584,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(vec![sub1, sub2]) + .validator_subscriptions(vec![sub1, sub2].into_iter()) .unwrap(); // Unsubscription event should happen at the end of the slot. @@ -668,7 +657,7 @@ mod attestation_service { // submit the subscriptions attestation_service - .validator_subscriptions(subscriptions) + .validator_subscriptions(subscriptions.into_iter()) .unwrap(); // There should only be the same subscriptions as there are in the specification, diff --git a/consensus/types/src/validator_subscription.rs b/consensus/types/src/validator_subscription.rs index fd48660c52..62932638ec 100644 --- a/consensus/types/src/validator_subscription.rs +++ b/consensus/types/src/validator_subscription.rs @@ -4,10 +4,8 @@ use ssz_derive::{Decode, Encode}; /// A validator subscription, created when a validator subscribes to a slot to perform optional aggregation /// duties. -#[derive(PartialEq, Debug, Serialize, Deserialize, Clone, Encode, Decode)] +#[derive(PartialEq, Debug, Serialize, Deserialize, Clone, Encode, Decode, Eq, PartialOrd, Ord)] pub struct ValidatorSubscription { - /// The validators index. - pub validator_index: u64, /// The index of the committee within `slot` of which the validator is a member. Used by the /// beacon node to quickly evaluate the associated `SubnetId`. pub attestation_committee_index: CommitteeIndex,