From dd6c8abb5cf05f3faf83bc3fa1ce0d57712c58c2 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 29 May 2025 16:00:56 -0700 Subject: [PATCH 01/27] Add validator_custody_params to chainspec --- consensus/types/src/chain_spec.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 59472e2edcd..072a55f9b94 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -203,6 +203,8 @@ pub struct ChainSpec { pub data_column_sidecar_subnet_count: u64, pub samples_per_slot: u64, pub custody_requirement: u64, + pub validator_custody_requirement: u64, + pub balance_per_additional_custody_group: u64, /* * Networking @@ -975,6 +977,8 @@ impl ChainSpec { data_column_sidecar_subnet_count: 128, number_of_columns: 128, samples_per_slot: 8, + validator_custody_requirement: 8, + balance_per_additional_custody_group: 32000000000, /* * Network specific @@ -1309,6 +1313,8 @@ impl ChainSpec { data_column_sidecar_subnet_count: 128, number_of_columns: 128, samples_per_slot: 8, + validator_custody_requirement: 8, + balance_per_additional_custody_group: 32000000000, /* * Network specific @@ -1650,6 +1656,12 @@ pub struct Config { #[serde(default = "BlobSchedule::default")] #[serde(skip_serializing_if = "BlobSchedule::is_empty")] blob_schedule: BlobSchedule, + #[serde(default = "default_validator_custody_requirement")] + #[serde(with = "serde_utils::quoted_u64")] + validator_custody_requirement: u64, + #[serde(default = "default_balance_per_additional_custody_group")] + #[serde(with = "serde_utils::quoted_u64")] + balance_per_additional_custody_group: u64, } fn default_bellatrix_fork_version() -> [u8; 4] { @@ -1815,6 +1827,14 @@ const fn default_samples_per_slot() -> u64 { 8 } +const fn default_validator_custody_requirement() -> u64 { + 8 +} + +const fn default_balance_per_additional_custody_group() -> u64 { + 32000000000 +} + fn max_blocks_by_root_request_common(max_request_blocks: u64) -> usize { let max_request_blocks = max_request_blocks as usize; RuntimeVariableList::::from_vec( @@ -2024,6 +2044,8 @@ impl Config { samples_per_slot: spec.samples_per_slot, custody_requirement: spec.custody_requirement, blob_schedule: spec.blob_schedule.clone(), + validator_custody_requirement: spec.validator_custody_requirement, + balance_per_additional_custody_group: spec.balance_per_additional_custody_group, } } @@ -2103,6 +2125,8 @@ impl Config { samples_per_slot, custody_requirement, ref blob_schedule, + validator_custody_requirement, + balance_per_additional_custody_group, } = self; if preset_base != E::spec_name().to_string().as_str() { @@ -2187,6 +2211,8 @@ impl Config { samples_per_slot, custody_requirement, blob_schedule: blob_schedule.clone(), + validator_custody_requirement, + balance_per_additional_custody_group, ..chain_spec.clone() }) From 4b4362104e7b532e65553409ecd8f9ec625a822e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 29 May 2025 16:01:11 -0700 Subject: [PATCH 02/27] First pass --- consensus/types/src/validator_custody.rs | 139 +++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 consensus/types/src/validator_custody.rs diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs new file mode 100644 index 00000000000..aae2e576ecc --- /dev/null +++ b/consensus/types/src/validator_custody.rs @@ -0,0 +1,139 @@ +use parking_lot::{lock_api::RwLock, RwLock}; + +use crate::{ChainSpec, Epoch}; + +// TODO(pawan): think more carefully about this number +pub const EPOCHS_BETWEEN_VALIDATOR_CUSTODY_UPDATES: usize = 10; + +/// Specifies the validator custody requirements for the node based +/// on the number of validators attached to the beacon node. +#[derive(Debug, Eq, PartialOrd, Ord, Clone)] +pub enum ValidatorCustody { + /// There are sufficient attached validators for the node to have to + /// custody all the columns. + /// + /// Currently, we would need a minimum of 113 validators (32 ETH balance) attached to + /// the node to custody all `NUMBER_OF_COLUMNS` columns. + /// + /// NUMBER_OF_COLUMNS - VALIDATOR_CUSTODY_REQUIREMENT - SAMPLES_PER_SLOT + 1 + /// = 128 - 8 - 8 + 1 = 113 + AllColumns, + /// All validator counts < 113. + /// + /// Note: A validator here refers to a 32 eth unit. + NumValidators(usize), + NoValidators, +} + +impl ValidatorCustody { + /// Generate the `ValidatorCustody` object based on a persisted value of the + /// `cgc`. + /// + /// This cgc is the value that we get from the persisted metadata/enr. + pub fn new_from_persisted_cgc(cgc: usize, spec: &ChainSpec) -> Self { + // if cgc >= Self::min_validators_for_full_custody(spec) { + // Self::AllColumns + // } else { + // Self::NumValidators(cgc) + // } + unimplemented!("todo(pawan)") + } + + /// The minimum number of validators that need to be attached for + /// the node to have to custody all columns. + fn min_validators_for_full_custody(spec: &ChainSpec) -> usize { + spec.number_of_columns - spec.validator_custody_requirement - spec.samples_per_slot + 1 + } + + /// Total number of columns to custody based on this validator count + pub fn custody_count(&self, spec: &ChainSpec) -> usize { + match self { + Self::AllColumns => spec.number_of_columns, + Self::NumValidators(count) => { + if count == 0 { + 0 + } else { + std::cmp::min( + spec.validator_custody_requirement + count - 1, + spec.number_of_columns, + ) + } + } + } + } +} + +/// Collects the various components for determining the custody count. +#[derive(Debug, Clone)] +pub struct CustodyCount { + /// Columns to be custodied based on number of validators + /// that is attached to this node. + pub validator_custody: ValidatorCustody, + /// Columns to be custodied based on the cli parameters passed by + /// the user on startup. + pub cli_custody_count: usize, +} + +impl CustodyCount { + /// The total number of columns that need to be custodied for a node with + /// the given params. + pub fn custody_columns_count(&self, spec: &ChainSpec) -> usize { + std::cmp::min( + self.cli_custody_count + self.validator_custody.custody_count(spec), + spec.number_of_columns, + ) + } +} + +#[derive(Debug)] +pub struct CustodyContext { + /// This is the `CustodyCount` object we are using to compute the + /// cgc value that we advertise to our peers in our enr and metadata. + advertised_custody: RwLock, + /// This is the `CustodyCount` object that we use to perform sampling duties + /// at head (while syncing or when receiving gossip). + custody_at_head: RwLock, + /// Updates to the number of validators that is attached to this node + /// over a given time duration. + /// TODO(pawan): make this a constant sized queue. + validator_custody_updates: Vec<(Epoch, usize)>, +} + +impl CustodyContext { + /// Create a new custody default custody context object when no persisted object + /// exists. + pub fn new(cli_custody_count: usize, persisted_cgc: usize, spec: &ChainSpec) -> Self { + let advertised_custody = CustodyCount { + cli_custody_count, + validator_custody: ValidatorCustody::NoValidators, + }; + + // The advertised custody and the custody object are the same when we + // create an entirely new object. + let custody_at_head = advertised_custody.clone(); + Self { + advertised_custody: RwLock::new(advertised_custody), + custody_at_head: RwLock::new(custody_at_head), + validator_custody_updates: vec![], + } + } + + pub fn new_from_persisted_custody_context(bytes: &[u8]) -> Result {} + + /// The custody count that we advertise to our peers in our metadata and + /// enr values. + pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> usize { + self.advertised_custody.read().custody_columns_count(spec) + } + + /// The number of columns that we sample for the data availability check + /// at the head of the chain. + /// + /// Use this function to get the custody count number for blocks received + /// on gossip/rpc/sync. + /// + /// This value is essentially the internal `cgc` of the node. + pub fn custody_column_count(&self, spec: &ChainSpec) -> usize { + self.custody_at_head.read().custody_columns_count(spec) + } +} From 8ff8d1eec0b222c70d0fd7fb1d895fe70a0bb086 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 30 May 2025 17:54:15 -0700 Subject: [PATCH 03/27] Better first version --- consensus/types/src/validator_custody.rs | 209 ++++++++++++----------- 1 file changed, 112 insertions(+), 97 deletions(-) diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs index aae2e576ecc..b48edab8428 100644 --- a/consensus/types/src/validator_custody.rs +++ b/consensus/types/src/validator_custody.rs @@ -1,98 +1,64 @@ -use parking_lot::{lock_api::RwLock, RwLock}; +use std::ops::Deref; + +use parking_lot::RwLock; use crate::{ChainSpec, Epoch}; +use ssz::Decode; +use ssz_derive::{Decode, Encode}; // TODO(pawan): think more carefully about this number pub const EPOCHS_BETWEEN_VALIDATOR_CUSTODY_UPDATES: usize = 10; -/// Specifies the validator custody requirements for the node based -/// on the number of validators attached to the beacon node. -#[derive(Debug, Eq, PartialOrd, Ord, Clone)] -pub enum ValidatorCustody { - /// There are sufficient attached validators for the node to have to - /// custody all the columns. - /// - /// Currently, we would need a minimum of 113 validators (32 ETH balance) attached to - /// the node to custody all `NUMBER_OF_COLUMNS` columns. - /// - /// NUMBER_OF_COLUMNS - VALIDATOR_CUSTODY_REQUIREMENT - SAMPLES_PER_SLOT + 1 - /// = 128 - 8 - 8 + 1 = 113 - AllColumns, - /// All validator counts < 113. - /// - /// Note: A validator here refers to a 32 eth unit. - NumValidators(usize), - NoValidators, +/// Specifies the number of validators attached to this node. +#[derive(Debug, Copy, Encode, Decode, Clone)] +pub struct ValidatorCustodyCount { + count: u64, } -impl ValidatorCustody { - /// Generate the `ValidatorCustody` object based on a persisted value of the - /// `cgc`. - /// - /// This cgc is the value that we get from the persisted metadata/enr. - pub fn new_from_persisted_cgc(cgc: usize, spec: &ChainSpec) -> Self { - // if cgc >= Self::min_validators_for_full_custody(spec) { - // Self::AllColumns - // } else { - // Self::NumValidators(cgc) - // } - unimplemented!("todo(pawan)") - } - - /// The minimum number of validators that need to be attached for - /// the node to have to custody all columns. - fn min_validators_for_full_custody(spec: &ChainSpec) -> usize { - spec.number_of_columns - spec.validator_custody_requirement - spec.samples_per_slot + 1 - } - - /// Total number of columns to custody based on this validator count - pub fn custody_count(&self, spec: &ChainSpec) -> usize { - match self { - Self::AllColumns => spec.number_of_columns, - Self::NumValidators(count) => { - if count == 0 { - 0 - } else { - std::cmp::min( - spec.validator_custody_requirement + count - 1, - spec.number_of_columns, - ) - } - } - } +impl ValidatorCustodyCount { + pub fn new(count: u64) -> Self { + Self { count } } } -/// Collects the various components for determining the custody count. -#[derive(Debug, Clone)] -pub struct CustodyCount { - /// Columns to be custodied based on number of validators - /// that is attached to this node. - pub validator_custody: ValidatorCustody, - /// Columns to be custodied based on the cli parameters passed by - /// the user on startup. - pub cli_custody_count: usize, +impl Deref for ValidatorCustodyCount { + type Target = u64; + fn deref(&self) -> &Self::Target { + &self.count + } } -impl CustodyCount { - /// The total number of columns that need to be custodied for a node with - /// the given params. - pub fn custody_columns_count(&self, spec: &ChainSpec) -> usize { +impl ValidatorCustodyCount { + /// Number of columns/custody groups to custody based on the number of validators + /// attached. + pub fn custody_count(&self, spec: &ChainSpec) -> u64 { std::cmp::min( - self.cli_custody_count + self.validator_custody.custody_count(spec), - spec.number_of_columns, + spec.number_of_columns as u64, + spec.validator_custody_requirement + self.count - 1, ) } } #[derive(Debug)] pub struct CustodyContext { - /// This is the `CustodyCount` object we are using to compute the - /// cgc value that we advertise to our peers in our enr and metadata. - advertised_custody: RwLock, - /// This is the `CustodyCount` object that we use to perform sampling duties - /// at head (while syncing or when receiving gossip). - custody_at_head: RwLock, + /// Columns to be custodied based on number of validators + /// that is attached to this node. + /// + /// This is the number that we use to compute the custody count value that + /// we advertise to our peers in the metadata and enr values. + advertised_validator_custody: RwLock, + /// This is the validator count that we use to compute the number of columns we need to + /// sample at head (while syncing or when receiving gossip). + validator_custody_at_head: RwLock, + /// Is the node run as a supernode based on current cli parameters. + current_is_supernode: bool, + /// The persisted value for `is_supernode` based on the previous run of this node. + /// + /// Note: We require this value because if a user restarts the node with a higher cli custody + /// count value than in the previous run, then we should continue advertising the custody + /// count based on the old value than the new one since we haven't backfilled the required + /// columns. + persisted_is_supernode: bool, /// Updates to the number of validators that is attached to this node /// over a given time duration. /// TODO(pawan): make this a constant sized queue. @@ -102,38 +68,87 @@ pub struct CustodyContext { impl CustodyContext { /// Create a new custody default custody context object when no persisted object /// exists. - pub fn new(cli_custody_count: usize, persisted_cgc: usize, spec: &ChainSpec) -> Self { - let advertised_custody = CustodyCount { - cli_custody_count, - validator_custody: ValidatorCustody::NoValidators, - }; - - // The advertised custody and the custody object are the same when we - // create an entirely new object. - let custody_at_head = advertised_custody.clone(); + /// + /// The `is_supernode` value is based on current cli parameters. + pub fn new(is_supernode: bool) -> Self { Self { - advertised_custody: RwLock::new(advertised_custody), - custody_at_head: RwLock::new(custody_at_head), + advertised_validator_custody: RwLock::new(ValidatorCustodyCount::new(0)), + validator_custody_at_head: RwLock::new(ValidatorCustodyCount::new(0)), + current_is_supernode: is_supernode, + persisted_is_supernode: is_supernode, validator_custody_updates: vec![], } } - pub fn new_from_persisted_custody_context(bytes: &[u8]) -> Result {} + /// Deserialize a `CustodyContext` from SSZ bytes. + pub fn new_from_persisted_custody_context( + bytes: &[u8], + is_supernode: bool, + ) -> Result { + let ssz_context = CustodyContextSsz::from_ssz_bytes(bytes) + .map_err(|e| format!("Failed to decode CustodyContextSsz: {:?}", e))?; + Ok(CustodyContext { + advertised_validator_custody: RwLock::new(ssz_context.advertised_validator_custody), + validator_custody_at_head: RwLock::new(ssz_context.validator_custody_at_head), + current_is_supernode: is_supernode, + persisted_is_supernode: ssz_context.persisted_is_supernode, + validator_custody_updates: ssz_context.validator_custody_updates, + }) + } /// The custody count that we advertise to our peers in our metadata and /// enr values. - pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> usize { - self.advertised_custody.read().custody_columns_count(spec) + pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> u64 { + if self.persisted_is_supernode { + return spec.number_of_columns; + } + let advertised_validator_custody = self.advertised_validator_custody.read().count; + if advertised_validator_custody > 0 { + std::cmp::min( + spec.validator_custody_requirement + advertised_validator_custody - 1 + + spec.custody_requirement, + spec.number_of_columns, + ) + } else { + spec.custody_requirement + } } - /// The number of columns that we sample for the data availability check - /// at the head of the chain. + /// The custody count that we use to custody columns currently. /// - /// Use this function to get the custody count number for blocks received - /// on gossip/rpc/sync. - /// - /// This value is essentially the internal `cgc` of the node. - pub fn custody_column_count(&self, spec: &ChainSpec) -> usize { - self.custody_at_head.read().custody_columns_count(spec) + /// This function should be called when figuring out how many columns we + /// need to custody when receiving blocks over gossip/rpc or during sync. + pub fn head_custody_count(&self, spec: &ChainSpec) -> u64 { + if self.current_is_supernode { + return spec.number_of_columns; + } + let custody_at_head = self.validator_custody_at_head.read().count; + if custody_at_head > 0 { + std::cmp::min( + spec.validator_custody_requirement + custody_at_head - 1 + spec.custody_requirement, + spec.number_of_columns, + ) + } else { + spec.custody_requirement + } + } +} + +#[derive(Debug, Encode, Decode, Clone)] +pub struct CustodyContextSsz { + advertised_validator_custody: ValidatorCustodyCount, + validator_custody_at_head: ValidatorCustodyCount, + persisted_is_supernode: bool, + validator_custody_updates: Vec<(Epoch, usize)>, +} + +impl From for CustodyContextSsz { + fn from(context: CustodyContext) -> Self { + CustodyContextSsz { + advertised_validator_custody: context.advertised_validator_custody.read().clone(), + validator_custody_at_head: context.validator_custody_at_head.read().clone(), + persisted_is_supernode: context.persisted_is_supernode, + validator_custody_updates: context.validator_custody_updates.clone(), + } } } From 04cb05de257586d167f7dd4e4c322d43edf14868 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 2 Jun 2025 11:11:53 -0700 Subject: [PATCH 04/27] plumbing --- beacon_node/beacon_chain/src/builder.rs | 19 +++++++++++---- .../src/data_availability_checker.rs | 11 +++++++-- .../lighthouse_network/src/service/mod.rs | 3 +++ .../lighthouse_network/src/service/utils.rs | 3 ++- .../lighthouse_network/src/types/globals.rs | 24 +++++++++++++++---- beacon_node/network/src/service.rs | 2 ++ consensus/types/src/lib.rs | 2 ++ 7 files changed, 52 insertions(+), 12 deletions(-) diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 2346aca00b5..3425be84693 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -42,8 +42,9 @@ use store::{Error as StoreError, HotColdDB, ItemStore, KeyValueStoreOp}; use task_executor::{ShutdownReason, TaskExecutor}; use tracing::{debug, error, info}; use types::{ - BeaconBlock, BeaconState, BlobSidecarList, ChainSpec, Checkpoint, DataColumnSidecarList, Epoch, - EthSpec, FixedBytesExtended, Hash256, Signature, SignedBeaconBlock, Slot, + BeaconBlock, BeaconState, BlobSidecarList, ChainSpec, Checkpoint, CustodyContext, + DataColumnSidecarList, Epoch, EthSpec, FixedBytesExtended, Hash256, Signature, + SignedBeaconBlock, Slot, }; /// An empty struct used to "witness" all the `BeaconChainTypes` traits. It has no user-facing @@ -926,6 +927,10 @@ where } }; + // Construct a dummy custody context that will be modified with the correct values in the + // network constructor. + let custody_context = Arc::new(CustodyContext::new(self.import_all_data_columns)); + let beacon_chain = BeaconChain { spec: self.spec.clone(), config: self.chain_config, @@ -999,8 +1004,14 @@ where validator_monitor: RwLock::new(validator_monitor), genesis_backfill_slot, data_availability_checker: Arc::new( - DataAvailabilityChecker::new(slot_clock, self.kzg.clone(), store, self.spec) - .map_err(|e| format!("Error initializing DataAvailabilityChecker: {:?}", e))?, + DataAvailabilityChecker::new( + slot_clock, + self.kzg.clone(), + store, + custody_context, + self.spec, + ) + .map_err(|e| format!("Error initializing DataAvailabilityChecker: {:?}", e))?, ), kzg: self.kzg.clone(), rng: Arc::new(Mutex::new(rng)), diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 0fd417389b2..8b6ca8fa0b7 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -17,8 +17,8 @@ use task_executor::TaskExecutor; use tracing::{debug, error, info_span, Instrument}; use types::blob_sidecar::{BlobIdentifier, BlobSidecar, FixedBlobSidecarList}; use types::{ - BlobSidecarList, ChainSpec, DataColumnSidecar, DataColumnSidecarList, Epoch, EthSpec, Hash256, - RuntimeVariableList, SignedBeaconBlock, + BlobSidecarList, ChainSpec, CustodyContext, DataColumnSidecar, DataColumnSidecarList, Epoch, + EthSpec, Hash256, RuntimeVariableList, SignedBeaconBlock, }; mod error; @@ -74,6 +74,7 @@ pub struct DataAvailabilityChecker { availability_cache: Arc>, slot_clock: T::SlotClock, kzg: Arc, + custody_context: Arc, spec: Arc, } @@ -111,6 +112,7 @@ impl DataAvailabilityChecker { slot_clock: T::SlotClock, kzg: Arc, store: BeaconStore, + custody_context: Arc, spec: Arc, ) -> Result { let inner = DataAvailabilityCheckerInner::new(OVERFLOW_LRU_CAPACITY, store, spec.clone())?; @@ -118,10 +120,15 @@ impl DataAvailabilityChecker { availability_cache: Arc::new(inner), slot_clock, kzg, + custody_context, spec, }) } + pub fn custody_context(&self) -> Arc { + self.custody_context.clone() + } + /// Checks if the block root is currenlty in the availability cache awaiting import because /// of missing components. pub fn get_execution_valid_block( diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 23060df9e6a..bafcc37d0ec 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -207,12 +207,15 @@ impl Network { }); let meta_data = utils::load_or_build_metadata(&config.network_dir, custody_group_count); let seq_number = *meta_data.seq_number(); + // todo(pawan): load the persisted custody context here and modify the custody context before passing + // it to the globals let globals = NetworkGlobals::new( enr, meta_data, trusted_peers, config.disable_peer_scoring, config.clone(), + ctx.custody_context.clone(), ctx.chain_spec.clone(), ); let network_globals = Arc::new(globals); diff --git a/beacon_node/lighthouse_network/src/service/utils.rs b/beacon_node/lighthouse_network/src/service/utils.rs index 01929bcb01c..d7f5ddb8b33 100644 --- a/beacon_node/lighthouse_network/src/service/utils.rs +++ b/beacon_node/lighthouse_network/src/service/utils.rs @@ -18,7 +18,7 @@ use std::sync::Arc; use std::time::Duration; use tracing::{debug, warn}; use types::{ - ChainSpec, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, SyncSubnetId, + ChainSpec, CustodyContext, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, SyncSubnetId }; pub const NETWORK_KEY_FILENAME: &str = "key"; @@ -30,6 +30,7 @@ pub struct Context<'a> { pub enr_fork_id: EnrForkId, pub fork_context: Arc, pub chain_spec: Arc, + pub custody_context: Arc, pub libp2p_registry: Option<&'a mut Registry>, } diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index fd99d935890..8b2cc45f54a 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -11,7 +11,7 @@ use tracing::error; use types::data_column_custody_group::{ compute_columns_for_custody_group, compute_subnets_from_custody_group, get_custody_groups, }; -use types::{ChainSpec, ColumnIndex, DataColumnSubnetId, EthSpec}; +use types::{ChainSpec, ColumnIndex, CustodyContext, DataColumnSubnetId, EthSpec}; pub struct NetworkGlobals { /// The current local ENR. @@ -33,8 +33,8 @@ pub struct NetworkGlobals { /// The computed sampling subnets and columns is stored to avoid re-computing. pub sampling_subnets: HashSet, pub sampling_columns: HashSet, - /// Constant custody group count (CGC) set at startup - custody_group_count: u64, + /// Context for the amount of columns the node has to custody. + custody_context: Arc, /// Network-related configuration. Immutable after initialization. pub config: Arc, /// Ethereum chain configuration. Immutable after initialization. @@ -48,6 +48,7 @@ impl NetworkGlobals { trusted_peers: Vec, disable_peer_scoring: bool, config: Arc, + custody_context: Arc, spec: Arc, ) -> Self { let node_id = enr.node_id().raw(); @@ -87,6 +88,8 @@ impl NetworkGlobals { sampling_columns.extend(columns); } + // set the values in custody_context based on the + NetworkGlobals { local_enr: RwLock::new(enr.clone()), peer_id: RwLock::new(enr.peer_id()), @@ -98,7 +101,7 @@ impl NetworkGlobals { backfill_state: RwLock::new(BackFillState::Paused), sampling_subnets, sampling_columns, - custody_group_count, + custody_context, config, spec, } @@ -255,7 +258,18 @@ impl NetworkGlobals { let keypair = libp2p::identity::secp256k1::Keypair::generate(); let enr_key: discv5::enr::CombinedKey = discv5::enr::CombinedKey::from_secp256k1(&keypair); let enr = discv5::enr::Enr::builder().build(&enr_key).unwrap(); - NetworkGlobals::new(enr, metadata, trusted_peers, false, config, spec) + let custody_context = Arc::new(CustodyContext::new( + config.subscribe_all_data_column_subnets, + )); + NetworkGlobals::new( + enr, + metadata, + trusted_peers, + false, + config, + custody_context, + spec, + ) } } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 77204b455da..9ff2f33fb00 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -34,6 +34,7 @@ use task_executor::ShutdownReason; use tokio::sync::mpsc; use tokio::time::Sleep; use tracing::{debug, error, info, info_span, trace, warn, Instrument}; +use types::CustodyContext; use types::{ ChainSpec, EthSpec, ForkContext, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, ValidatorSubscription, @@ -266,6 +267,7 @@ impl NetworkService { enr_fork_id, fork_context: fork_context.clone(), chain_spec: beacon_chain.spec.clone(), + custody_context: beacon_chain.data_availability_checker.custody_context(), libp2p_registry, }; diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index f0555a06d6d..9d0524d028a 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -72,6 +72,7 @@ pub mod signing_data; pub mod sync_committee_subscription; pub mod sync_duty; pub mod validator; +pub mod validator_custody; pub mod validator_subscription; pub mod voluntary_exit; pub mod withdrawal_credentials; @@ -264,6 +265,7 @@ pub use crate::withdrawal::Withdrawal; pub use crate::withdrawal_credentials::WithdrawalCredentials; pub use crate::withdrawal_request::WithdrawalRequest; pub use fixed_bytes::FixedBytesExtended; +pub use validator_custody::CustodyContext; pub type CommitteeIndex = u64; pub type Hash256 = fixed_bytes::Hash256; From b3227d0c5331d07fc70c22f5646b234037d957d8 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 2 Jun 2025 13:56:15 -0700 Subject: [PATCH 05/27] Add validator registration --- beacon_node/http_api/src/lib.rs | 18 ++++ beacon_node/http_api/src/test_utils.rs | 6 +- .../lighthouse_network/src/service/utils.rs | 3 +- .../lighthouse_network/src/types/globals.rs | 4 +- consensus/types/src/validator_custody.rs | 86 ++++++++++++++++++- 5 files changed, 109 insertions(+), 8 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 2eaa33a9648..2d495c23f22 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3780,6 +3780,24 @@ pub fn serve( .validator_monitor .write() .auto_register_local_validator(subscription.validator_index); + // Register the validator with the `CustodyContext` + if let Ok(effective_balance) = chain + .canonical_head + .cached_head() + .snapshot + .beacon_state + .get_effective_balance(subscription.validator_index as usize) + { + chain + .data_availability_checker + .custody_context() + .register_validator::( + subscription.validator_index as usize, + effective_balance, + chain.slot().unwrap(), + &chain.spec, + ); + } api_types::ValidatorSubscription { attestation_committee_index: subscription.committee_index, slot: subscription.slot, diff --git a/beacon_node/http_api/src/test_utils.rs b/beacon_node/http_api/src/test_utils.rs index f78a361dad3..1fbb659ce3a 100644 --- a/beacon_node/http_api/src/test_utils.rs +++ b/beacon_node/http_api/src/test_utils.rs @@ -27,7 +27,7 @@ use std::sync::Arc; use std::time::Duration; use store::MemoryStore; use task_executor::test_utils::TestRuntime; -use types::{ChainSpec, EthSpec}; +use types::{ChainSpec, CustodyContext, EthSpec}; pub const TCP_PORT: u16 = 42; pub const UDP_PORT: u16 = 42; @@ -158,12 +158,16 @@ pub async fn create_api_server_with_config( let enr_key = CombinedKey::generate_secp256k1(); let enr = Enr::builder().build(&enr_key).unwrap(); let network_config = Arc::new(NetworkConfig::default()); + let custody_context = Arc::new(CustodyContext::new( + network_config.subscribe_all_data_column_subnets, + )); let network_globals = Arc::new(NetworkGlobals::new( enr.clone(), meta_data, vec![], false, network_config, + custody_context, chain.spec.clone(), )); diff --git a/beacon_node/lighthouse_network/src/service/utils.rs b/beacon_node/lighthouse_network/src/service/utils.rs index d7f5ddb8b33..56821652996 100644 --- a/beacon_node/lighthouse_network/src/service/utils.rs +++ b/beacon_node/lighthouse_network/src/service/utils.rs @@ -18,7 +18,8 @@ use std::sync::Arc; use std::time::Duration; use tracing::{debug, warn}; use types::{ - ChainSpec, CustodyContext, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, SyncSubnetId + ChainSpec, CustodyContext, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, + SyncSubnetId, }; pub const NETWORK_KEY_FILENAME: &str = "key"; diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index 8b2cc45f54a..c1a709ce7e2 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -125,14 +125,14 @@ impl NetworkGlobals { /// Returns true if this node is configured as a PeerDAS supernode pub fn is_supernode(&self) -> bool { - self.custody_group_count == self.spec.number_of_custody_groups + self.custody_context.current_is_supernode } /// Returns the count of custody columns this node must sample for block import pub fn custody_columns_count(&self) -> u64 { // This only panics if the chain spec contains invalid values self.spec - .sampling_size(self.custody_group_count) + .sampling_size(self.custody_context.head_custody_count(&self.spec)) .expect("should compute node sampling size from valid chain spec") } diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs index b48edab8428..8178af148c2 100644 --- a/consensus/types/src/validator_custody.rs +++ b/consensus/types/src/validator_custody.rs @@ -1,8 +1,8 @@ -use std::ops::Deref; +use std::collections::{BTreeMap, HashMap}; use parking_lot::RwLock; -use crate::{ChainSpec, Epoch}; +use crate::{ChainSpec, Epoch, EthSpec, Slot}; use ssz::Decode; use ssz_derive::{Decode, Encode}; @@ -21,7 +21,7 @@ impl ValidatorCustodyCount { } } -impl Deref for ValidatorCustodyCount { +impl std::ops::Deref for ValidatorCustodyCount { type Target = u64; fn deref(&self) -> &Self::Target { &self.count @@ -39,6 +39,66 @@ impl ValidatorCustodyCount { } } +/// TODO(pawan): this currently just registers increases in validator count. +/// Does not handle decreasing validator counts +#[derive(Default, Debug)] +struct ValidatorRegistrations { + /// Set of all validators that is registered to this node along with its effective balance + /// in increments of `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` + validators: HashMap, + /// Maintains the number of unique validators that hit the subscriptions endpoint each + /// epoch where the validator count changed from the previous epoch. + /// + /// If the count is same between subsequent epochs, then the later epoch values aren't stored + /// to save space. + epoch_validators: BTreeMap, +} + +impl ValidatorRegistrations { + /// Returns the validator count at the latest epoch for the custody requirement. + pub fn latest_validator_count_for_custody(&self) -> Option { + self.epoch_validators.last_key_value().map(|(_, v)| *v) + } + + /// Returns the total validator count based on the effective balance. + /// + /// Note: Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of weight. + pub fn custody_requirement(&self) -> u64 { + self.validators.values().sum() + } + + /// Returns the latest epoch at which the validator count changed. + #[allow(dead_code)] + pub fn latest_epoch(&self) -> Option { + self.epoch_validators.last_key_value().map(|(k, _)| *k) + } + + /// Register a new validator index and updates the list of validators if required. + pub fn register_validator( + &mut self, + validator_index: usize, + effective_balance: u64, + slot: Slot, + spec: &ChainSpec, + ) { + let epoch = slot.epoch(E::slots_per_epoch()); + // This is the "weight" of the validator based on the effective balance + let num_validators_for_effective_balance = + effective_balance / spec.balance_per_additional_custody_group; + self.validators + .insert(validator_index, num_validators_for_effective_balance); + let count_at_epoch = self.custody_requirement(); + // If registering the new validator increased the validator count, then + // add a new entry for the current epoch + if Some(count_at_epoch) != self.latest_validator_count_for_custody() { + self.epoch_validators + .entry(epoch) + .and_modify(|old_count| *old_count = count_at_epoch) + .or_insert(count_at_epoch); + } + } +} + #[derive(Debug)] pub struct CustodyContext { /// Columns to be custodied based on number of validators @@ -51,7 +111,7 @@ pub struct CustodyContext { /// sample at head (while syncing or when receiving gossip). validator_custody_at_head: RwLock, /// Is the node run as a supernode based on current cli parameters. - current_is_supernode: bool, + pub current_is_supernode: bool, /// The persisted value for `is_supernode` based on the previous run of this node. /// /// Note: We require this value because if a user restarts the node with a higher cli custody @@ -62,7 +122,10 @@ pub struct CustodyContext { /// Updates to the number of validators that is attached to this node /// over a given time duration. /// TODO(pawan): make this a constant sized queue. + /// might not need this with epoch_validators validator_custody_updates: Vec<(Epoch, usize)>, + /// Maintains all the validators that this node is connected to currently + validator_registrations: RwLock, } impl CustodyContext { @@ -77,6 +140,7 @@ impl CustodyContext { current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, validator_custody_updates: vec![], + validator_registrations: Default::default(), } } @@ -93,9 +157,23 @@ impl CustodyContext { current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_custody_updates: ssz_context.validator_custody_updates, + validator_registrations: Default::default(), }) } + /// Register a new validator index and updates the list of validators if required. + pub fn register_validator( + &self, + validator_index: usize, + effective_balance: u64, + slot: Slot, + spec: &ChainSpec, + ) { + self.validator_registrations + .write() + .register_validator::(validator_index, effective_balance, slot, spec) + } + /// The custody count that we advertise to our peers in our metadata and /// enr values. pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> u64 { From dfc412f956b15513f513b82d8ad2cf36eac5b875 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 2 Jun 2025 18:08:42 -0700 Subject: [PATCH 06/27] Persist and load context from db --- beacon_node/beacon_chain/src/beacon_chain.rs | 31 +++++++++++ beacon_node/beacon_chain/src/builder.rs | 17 ++++-- beacon_node/beacon_chain/src/lib.rs | 1 + .../beacon_chain/src/persisted_custody.rs | 52 +++++++++++++++++++ beacon_node/network/src/service.rs | 1 - beacon_node/store/src/lib.rs | 3 ++ consensus/types/src/validator_custody.rs | 15 +++--- 7 files changed, 107 insertions(+), 13 deletions(-) create mode 100644 beacon_node/beacon_chain/src/persisted_custody.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 990f4b6099c..6cbe9c3a77a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -58,6 +58,7 @@ use crate::observed_data_sidecars::ObservedDataSidecars; use crate::observed_operations::{ObservationOutcome, ObservedOperations}; use crate::observed_slashable::ObservedSlashable; use crate::persisted_beacon_chain::PersistedBeaconChain; +use crate::persisted_custody::{clear_custody_context, persist_custody_context}; use crate::persisted_fork_choice::PersistedForkChoice; use crate::pre_finalization_cache::PreFinalizationBlockCache; use crate::shuffling_cache::{BlockShufflingIds, ShufflingCache}; @@ -130,6 +131,7 @@ use types::blob_sidecar::FixedBlobSidecarList; use types::data_column_sidecar::ColumnIndex; use types::payload::BlockProductionVersion; use types::*; +use types::{validator_custody::CustodyContextSsz}; pub type ForkChoiceError = fork_choice::Error; @@ -668,6 +670,34 @@ impl BeaconChain { Ok(()) } + /// Persists the custody information to disk. + pub fn persist_custody_context(&self) -> Result<(), Error> { + let custody_context: CustodyContextSsz = self + .data_availability_checker + .custody_context() + .as_ref() + .into(); + debug!(?custody_context, "Persisting custody context to store"); + + if let Err(e) = + clear_custody_context::(self.store.clone()) + { + error!(error = ?e, "Failed to clear old custody context"); + } + + match persist_custody_context::( + self.store.clone(), + custody_context, + ) { + Ok(_) => info!("Saved custody state"), + Err(e) => error!( + error = ?e, + "Failed to persist custody context on drop" + ), + } + Ok(()) + } + /// Returns the slot _right now_ according to `self.slot_clock`. Returns `Err` if the slot is /// unavailable. /// @@ -7187,6 +7217,7 @@ impl BeaconChain { impl Drop for BeaconChain { fn drop(&mut self) { let drop = || -> Result<(), Error> { + self.persist_custody_context()?; self.persist_fork_choice()?; self.persist_op_pool()?; self.persist_eth1_cache() diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 3425be84693..6f54a7ce7c3 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -13,6 +13,7 @@ use crate::light_client_server_cache::LightClientServerCache; use crate::migrate::{BackgroundMigrator, MigratorConfig}; use crate::observed_data_sidecars::ObservedDataSidecars; use crate::persisted_beacon_chain::PersistedBeaconChain; +use crate::persisted_custody::load_custody_context; use crate::shuffling_cache::{BlockShufflingIds, ShufflingCache}; use crate::validator_monitor::{ValidatorMonitor, ValidatorMonitorConfig}; use crate::validator_pubkey_cache::ValidatorPubkeyCache; @@ -927,9 +928,19 @@ where } }; - // Construct a dummy custody context that will be modified with the correct values in the - // network constructor. - let custody_context = Arc::new(CustodyContext::new(self.import_all_data_columns)); + // Load the persisted custody context from the db and initialize + // the context for this run + let custody_context = if let Some(custody) = + load_custody_context::(store.clone()) + { + Arc::new(CustodyContext::new_from_persisted_custody_context( + custody, + self.import_all_data_columns, + )) + } else { + Arc::new(CustodyContext::new(self.import_all_data_columns)) + }; + debug!(?custody_context, "Loading persisted custody context"); let beacon_chain = BeaconChain { spec: self.spec.clone(), diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 5b79312d371..a680b97f32b 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -12,6 +12,7 @@ pub mod bellatrix_readiness; pub mod blob_verification; pub mod block_reward; mod block_times_cache; +pub mod persisted_custody; mod block_verification; pub mod block_verification_types; pub mod builder; diff --git a/beacon_node/beacon_chain/src/persisted_custody.rs b/beacon_node/beacon_chain/src/persisted_custody.rs new file mode 100644 index 00000000000..0f529a6a982 --- /dev/null +++ b/beacon_node/beacon_chain/src/persisted_custody.rs @@ -0,0 +1,52 @@ +use ssz::{Decode, Encode}; +use std::sync::Arc; +use store::{DBColumn, Error as StoreError, HotColdDB, ItemStore, StoreItem}; +use types::{validator_custody::CustodyContextSsz, EthSpec, Hash256}; + +/// 32-byte key for accessing the `CustodyContext`. All zero because `CustodyContext` has its own column. +pub const CUSTODY_DB_KEY: Hash256 = Hash256::ZERO; + +pub struct PersistedCustody(CustodyContextSsz); + +pub fn load_custody_context, Cold: ItemStore>( + store: Arc>, +) -> Option { + let res: Result, _> = + store.get_item::(&CUSTODY_DB_KEY); + // Load context from the store + match res { + Ok(Some(c)) => Some(c.0), + _ => None, + } +} + +/// Attempt to persist the custody context object to `self.store`. +pub fn persist_custody_context, Cold: ItemStore>( + store: Arc>, + custody_context: CustodyContextSsz, +) -> Result<(), store::Error> { + store.put_item(&CUSTODY_DB_KEY, &PersistedCustody(custody_context)) +} + +/// Attempts to clear any custody context entries. +pub fn clear_custody_context, Cold: ItemStore>( + store: Arc>, +) -> Result<(), store::Error> { + store.hot_db.delete::(&CUSTODY_DB_KEY) +} + +impl StoreItem for PersistedCustody { + fn db_column() -> DBColumn { + DBColumn::CustodyContext + } + + fn as_store_bytes(&self) -> Vec { + self.0.as_ssz_bytes() + } + + fn from_store_bytes(bytes: &[u8]) -> Result { + let custody_context = CustodyContextSsz::from_ssz_bytes(bytes)?; + + Ok(PersistedCustody(custody_context)) + } +} diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 9ff2f33fb00..aaf29733534 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -34,7 +34,6 @@ use task_executor::ShutdownReason; use tokio::sync::mpsc; use tokio::time::Sleep; use tracing::{debug, error, info, info_span, trace, warn, Instrument}; -use types::CustodyContext; use types::{ ChainSpec, EthSpec, ForkContext, Slot, SubnetId, SyncCommitteeSubscription, SyncSubnetId, Unsigned, ValidatorSubscription, diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 5b30971fd8e..b9d7968e070 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -339,6 +339,8 @@ pub enum DBColumn { BeaconRandaoMixes, #[strum(serialize = "dht")] DhtEnrs, + #[strum(serialize = "custody")] + CustodyContext, /// DEPRECATED. For Optimistically Imported Merge Transition Blocks #[strum(serialize = "otb")] OptimisticTransitionBlock, @@ -397,6 +399,7 @@ impl DBColumn { | Self::PubkeyCache | Self::BeaconRestorePoint | Self::DhtEnrs + | Self::CustodyContext | Self::OptimisticTransitionBlock => 32, Self::BeaconBlockRoots | Self::BeaconBlockRootsChunked diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs index 8178af148c2..03ca0d5be17 100644 --- a/consensus/types/src/validator_custody.rs +++ b/consensus/types/src/validator_custody.rs @@ -3,7 +3,6 @@ use std::collections::{BTreeMap, HashMap}; use parking_lot::RwLock; use crate::{ChainSpec, Epoch, EthSpec, Slot}; -use ssz::Decode; use ssz_derive::{Decode, Encode}; // TODO(pawan): think more carefully about this number @@ -146,19 +145,17 @@ impl CustodyContext { /// Deserialize a `CustodyContext` from SSZ bytes. pub fn new_from_persisted_custody_context( - bytes: &[u8], + ssz_context: CustodyContextSsz, is_supernode: bool, - ) -> Result { - let ssz_context = CustodyContextSsz::from_ssz_bytes(bytes) - .map_err(|e| format!("Failed to decode CustodyContextSsz: {:?}", e))?; - Ok(CustodyContext { + ) -> Self { + CustodyContext { advertised_validator_custody: RwLock::new(ssz_context.advertised_validator_custody), validator_custody_at_head: RwLock::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_custody_updates: ssz_context.validator_custody_updates, validator_registrations: Default::default(), - }) + } } /// Register a new validator index and updates the list of validators if required. @@ -220,8 +217,8 @@ pub struct CustodyContextSsz { validator_custody_updates: Vec<(Epoch, usize)>, } -impl From for CustodyContextSsz { - fn from(context: CustodyContext) -> Self { +impl From<&CustodyContext> for CustodyContextSsz { + fn from(context: &CustodyContext) -> Self { CustodyContextSsz { advertised_validator_custody: context.advertised_validator_custody.read().clone(), validator_custody_at_head: context.validator_custody_at_head.read().clone(), From d45f9a61817266c310291a90ccdbc8b9dd789448 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 3 Jun 2025 15:21:55 -0700 Subject: [PATCH 07/27] Update the custody at head based on registrations --- beacon_node/http_api/src/lib.rs | 68 ++++++++++++------------ consensus/types/src/validator_custody.rs | 55 +++++++++++-------- 2 files changed, 67 insertions(+), 56 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 2d495c23f22..b38fdb9754a 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3768,44 +3768,44 @@ pub fn serve( .and(task_spawner_filter.clone()) .and(chain_filter.clone()) .then( - |subscriptions: Vec, + |committee_subscriptions: Vec, validator_subscription_tx: Sender, task_spawner: TaskSpawner, chain: Arc>| { task_spawner.blocking_json_task(Priority::P0, move || { - let subscriptions: std::collections::BTreeSet<_> = subscriptions - .iter() - .map(|subscription| { - chain - .validator_monitor - .write() - .auto_register_local_validator(subscription.validator_index); - // Register the validator with the `CustodyContext` - if let Ok(effective_balance) = chain - .canonical_head - .cached_head() - .snapshot - .beacon_state - .get_effective_balance(subscription.validator_index as usize) - { - chain - .data_availability_checker - .custody_context() - .register_validator::( - subscription.validator_index as usize, - effective_balance, - chain.slot().unwrap(), - &chain.spec, - ); - } - 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 mut subscriptions: std::collections::BTreeSet<_> = Default::default(); + let mut registrations = Vec::new(); + for subscription in committee_subscriptions.iter() { + chain + .validator_monitor + .write() + .auto_register_local_validator(subscription.validator_index); + // Register the validator with the `CustodyContext` + if let Ok(effective_balance) = chain + .canonical_head + .cached_head() + .snapshot + .beacon_state + .get_effective_balance(subscription.validator_index as usize) + { + registrations + .push((subscription.validator_index as usize, effective_balance)); + } + subscriptions.insert(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, + }); + } + chain + .data_availability_checker + .custody_context() + .register_validator::( + registrations, + chain.slot().unwrap(), + &chain.spec, + ); let message = ValidatorSubscriptionMessage::AttestationSubscribe { subscriptions }; if let Err(e) = validator_subscription_tx.try_send(message) { diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs index 03ca0d5be17..daa3b5953a5 100644 --- a/consensus/types/src/validator_custody.rs +++ b/consensus/types/src/validator_custody.rs @@ -55,17 +55,12 @@ struct ValidatorRegistrations { impl ValidatorRegistrations { /// Returns the validator count at the latest epoch for the custody requirement. + /// + /// This should be equivalent to the current `validator_custody_at_head`. pub fn latest_validator_count_for_custody(&self) -> Option { self.epoch_validators.last_key_value().map(|(_, v)| *v) } - /// Returns the total validator count based on the effective balance. - /// - /// Note: Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of weight. - pub fn custody_requirement(&self) -> u64 { - self.validators.values().sum() - } - /// Returns the latest epoch at which the validator count changed. #[allow(dead_code)] pub fn latest_epoch(&self) -> Option { @@ -86,7 +81,10 @@ impl ValidatorRegistrations { effective_balance / spec.balance_per_additional_custody_group; self.validators .insert(validator_index, num_validators_for_effective_balance); - let count_at_epoch = self.custody_requirement(); + + // Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of "weight". + let count_at_epoch = self.validators.values().sum(); + // If registering the new validator increased the validator count, then // add a new entry for the current epoch if Some(count_at_epoch) != self.latest_validator_count_for_custody() { @@ -118,11 +116,6 @@ pub struct CustodyContext { /// count based on the old value than the new one since we haven't backfilled the required /// columns. persisted_is_supernode: bool, - /// Updates to the number of validators that is attached to this node - /// over a given time duration. - /// TODO(pawan): make this a constant sized queue. - /// might not need this with epoch_validators - validator_custody_updates: Vec<(Epoch, usize)>, /// Maintains all the validators that this node is connected to currently validator_registrations: RwLock, } @@ -138,7 +131,6 @@ impl CustodyContext { validator_custody_at_head: RwLock::new(ValidatorCustodyCount::new(0)), current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, - validator_custody_updates: vec![], validator_registrations: Default::default(), } } @@ -153,22 +145,43 @@ impl CustodyContext { validator_custody_at_head: RwLock::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, - validator_custody_updates: ssz_context.validator_custody_updates, validator_registrations: Default::default(), } } /// Register a new validator index and updates the list of validators if required. + /// + /// Also modifies the internal structures if the validator custody has changed to + /// update the `custody_column_count`. pub fn register_validator( &self, - validator_index: usize, - effective_balance: u64, + validators_and_balance: Vec<(usize, u64)>, slot: Slot, spec: &ChainSpec, ) { - self.validator_registrations - .write() - .register_validator::(validator_index, effective_balance, slot, spec) + // Only do the registrations once per epoch + if slot % E::slots_per_epoch() != 0 { + return; + } + + let mut registrations = self.validator_registrations.write(); + for (validator_index, effective_balance) in validators_and_balance { + registrations.register_validator::(validator_index, effective_balance, slot, spec); + } + + // Completed registrations, now check if the cgc has changed + let mut validator_custody_at_head = self.validator_custody_at_head.write(); + let Some(new_validator_custody) = registrations.latest_validator_count_for_custody() else { + return; + }; + + // Update the current validator custody value if the validator registration changes the number of + // validators + if new_validator_custody != validator_custody_at_head.count { + *validator_custody_at_head = ValidatorCustodyCount { + count: new_validator_custody, + }; + } } /// The custody count that we advertise to our peers in our metadata and @@ -214,7 +227,6 @@ pub struct CustodyContextSsz { advertised_validator_custody: ValidatorCustodyCount, validator_custody_at_head: ValidatorCustodyCount, persisted_is_supernode: bool, - validator_custody_updates: Vec<(Epoch, usize)>, } impl From<&CustodyContext> for CustodyContextSsz { @@ -223,7 +235,6 @@ impl From<&CustodyContext> for CustodyContextSsz { advertised_validator_custody: context.advertised_validator_custody.read().clone(), validator_custody_at_head: context.validator_custody_at_head.read().clone(), persisted_is_supernode: context.persisted_is_supernode, - validator_custody_updates: context.validator_custody_updates.clone(), } } } From 0e23535d4f3316ce982978d2b4c37e548907e466 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 3 Jun 2025 19:00:07 -0700 Subject: [PATCH 08/27] Fix the custody requirement calculation --- consensus/types/src/validator_custody.rs | 171 +++++++++++++++++++++-- 1 file changed, 159 insertions(+), 12 deletions(-) diff --git a/consensus/types/src/validator_custody.rs b/consensus/types/src/validator_custody.rs index daa3b5953a5..9f8d09cd300 100644 --- a/consensus/types/src/validator_custody.rs +++ b/consensus/types/src/validator_custody.rs @@ -5,9 +5,6 @@ use parking_lot::RwLock; use crate::{ChainSpec, Epoch, EthSpec, Slot}; use ssz_derive::{Decode, Encode}; -// TODO(pawan): think more carefully about this number -pub const EPOCHS_BETWEEN_VALIDATOR_CUSTODY_UPDATES: usize = 10; - /// Specifies the number of validators attached to this node. #[derive(Debug, Copy, Encode, Decode, Clone)] pub struct ValidatorCustodyCount { @@ -93,9 +90,33 @@ impl ValidatorRegistrations { .and_modify(|old_count| *old_count = count_at_epoch) .or_insert(count_at_epoch); } + tracing::debug!( + %epoch, + validator_count = count_at_epoch, + "Registered validators" + ); } } +/// Given the `count` of validators, return the custody requirement based on +/// the spec parameters. +/// +/// Note: a validator here represents a unit of 32 eth effective_balance +/// equivalent to `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. +/// +/// For e.g. a validator with eb 32 eth is 1 unit. +/// a validator with eb 65 eth is 65 // 32 = 2 units. +/// +/// See https://github.com/ethereum/consensus-specs/blob/dev/specs/fulu/validator.md#validator-custody +fn get_validators_custody_requirement(count: u64, spec: &ChainSpec) -> u64 { + std::cmp::min( + std::cmp::max(count, spec.validator_custody_requirement), + spec.number_of_custody_groups, + ) +} + +/// Contains all the information the node requires to calculate the +/// number of columns to be custodied when checking for DA. #[derive(Debug)] pub struct CustodyContext { /// Columns to be custodied based on number of validators @@ -178,6 +199,11 @@ impl CustodyContext { // Update the current validator custody value if the validator registration changes the number of // validators if new_validator_custody != validator_custody_at_head.count { + tracing::debug!( + old_count = validator_custody_at_head.count, + new_count = new_validator_custody, + "Validator count at head updated" + ); *validator_custody_at_head = ValidatorCustodyCount { count: new_validator_custody, }; @@ -192,11 +218,8 @@ impl CustodyContext { } let advertised_validator_custody = self.advertised_validator_custody.read().count; if advertised_validator_custody > 0 { - std::cmp::min( - spec.validator_custody_requirement + advertised_validator_custody - 1 - + spec.custody_requirement, - spec.number_of_columns, - ) + get_validators_custody_requirement(advertised_validator_custody, spec) + + spec.custody_requirement } else { spec.custody_requirement } @@ -212,16 +235,14 @@ impl CustodyContext { } let custody_at_head = self.validator_custody_at_head.read().count; if custody_at_head > 0 { - std::cmp::min( - spec.validator_custody_requirement + custody_at_head - 1 + spec.custody_requirement, - spec.number_of_columns, - ) + get_validators_custody_requirement(custody_at_head, spec) + spec.custody_requirement } else { spec.custody_requirement } } } +/// The custody information that gets persisted across runs. #[derive(Debug, Encode, Decode, Clone)] pub struct CustodyContextSsz { advertised_validator_custody: ValidatorCustodyCount, @@ -238,3 +259,129 @@ impl From<&CustodyContext> for CustodyContextSsz { } } } + +#[cfg(test)] +mod tests { + use crate::MainnetEthSpec; + + use super::*; + + type E = MainnetEthSpec; + + #[test] + fn no_validators() { + // supernode without validators + let custody_context = CustodyContext::new(true); + let spec = E::default_spec(); + assert_eq!( + custody_context.head_custody_count(&spec), + spec.number_of_columns + ); + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.number_of_columns + ); + } + + #[test] + fn fullnode() { + // fullnode without validators + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + assert_eq!( + custody_context.head_custody_count(&spec), + spec.custody_requirement, + "head custody count should be minimum spec custody requirement" + ); + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.custody_requirement, + "advertised custody count should be minimum spec custody requirement" + ); + + // add 1 validator + custody_context.register_validator::(vec![(0, 32_000_000_000)], Slot::new(0), &spec); + + assert_eq!( + custody_context.head_custody_count(&spec), + spec.validator_custody_requirement + spec.custody_requirement, + "head custody count should increase with 1 validator" + ); + + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.custody_requirement, + "advertised custody count should not change" + ); + + // add 7 more validators to reach `validator_custody_requirement` + custody_context.register_validator::( + vec![ + (1, 32_000_000_000), + (2, 32_000_000_000), + (3, 32_000_000_000), + (4, 32_000_000_000), + (5, 32_000_000_000), + (6, 32_000_000_000), + (7, 32_000_000_000), + ], + Slot::new(0), + &spec, + ); + + assert_eq!( + custody_context.head_custody_count(&spec), + spec.validator_custody_requirement + spec.custody_requirement, + "head custody count should should be same as with 1 validator" + ); + + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.custody_requirement, + "advertised custody count should not change" + ); + + // adding 1 more validator should increase the custody count + custody_context.register_validator::(vec![(8, 32_000_000_000)], Slot::new(0), &spec); + assert_eq!( + custody_context.head_custody_count(&spec), + spec.validator_custody_requirement + spec.custody_requirement + 1, + "head custody count should increase by 1" + ); + + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.custody_requirement, + "advertised custody count should not change" + ); + + // update effective balance for some validators. + // validator count should increase by 3 + custody_context.register_validator::( + vec![ + (1, 96_000_000_000), + (2, 65_000_000_000), + (3, 32_000_000_000), + (4, 32_000_000_000), + (5, 32_000_000_000), + (6, 32_000_000_000), + (7, 32_000_000_000), + (8, 32_000_000_000), + ], + Slot::new(0), + &spec, + ); + + assert_eq!( + custody_context.head_custody_count(&spec), + spec.validator_custody_requirement + spec.custody_requirement + 4, + "head custody count should increase by 3" + ); + + assert_eq!( + custody_context.advertised_custody_column_count(&spec), + spec.custody_requirement, + "advertised custody count should not change" + ); + } +} From e8345d86bd84a488edd7c0fe488284bc4f18398d Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 5 Jun 2025 20:06:09 -0700 Subject: [PATCH 09/27] Move validator_custody to beacon_chain; broadcast to network on cgc change --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- beacon_node/beacon_chain/src/builder.rs | 6 +- .../src/data_availability_checker.rs | 6 +- .../overflow_lru_cache.rs | 2 +- beacon_node/beacon_chain/src/lib.rs | 4 +- .../beacon_chain/src/persisted_custody.rs | 3 +- .../beacon_chain}/src/validator_custody.rs | 53 +++++++-- beacon_node/http_api/src/publish_blocks.rs | 36 ++++--- beacon_node/http_api/src/test_utils.rs | 7 +- .../tests/broadcast_validation_tests.rs | 3 +- .../lighthouse_network/src/service/mod.rs | 24 ++++- .../lighthouse_network/src/service/utils.rs | 4 +- .../lighthouse_network/src/types/globals.rs | 101 +++++++++++------- .../lighthouse_network/src/types/topics.rs | 8 +- beacon_node/network/src/metrics.rs | 4 +- .../gossip_methods.rs | 5 +- .../src/network_beacon_processor/mod.rs | 9 +- .../src/network_beacon_processor/tests.rs | 2 +- beacon_node/network/src/service.rs | 34 +++++- .../network/src/sync/network_context.rs | 12 ++- .../network/src/sync/range_sync/chain.rs | 2 +- beacon_node/network/src/sync/tests/lookups.rs | 8 +- consensus/types/src/lib.rs | 2 - 23 files changed, 229 insertions(+), 108 deletions(-) rename {consensus/types => beacon_node/beacon_chain}/src/validator_custody.rs (89%) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6cbe9c3a77a..00f4b7ac585 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -65,6 +65,7 @@ use crate::shuffling_cache::{BlockShufflingIds, ShufflingCache}; use crate::sync_committee_verification::{ Error as SyncCommitteeError, VerifiedSyncCommitteeMessage, VerifiedSyncContribution, }; +use crate::validator_custody::CustodyContextSsz; use crate::validator_monitor::{ get_slot_delay_ms, timestamp_now, ValidatorMonitor, HISTORIC_EPOCHS as VALIDATOR_MONITOR_HISTORIC_EPOCHS, @@ -131,7 +132,6 @@ use types::blob_sidecar::FixedBlobSidecarList; use types::data_column_sidecar::ColumnIndex; use types::payload::BlockProductionVersion; use types::*; -use types::{validator_custody::CustodyContextSsz}; pub type ForkChoiceError = fork_choice::Error; diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 6f54a7ce7c3..ae07584f275 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -18,6 +18,7 @@ use crate::shuffling_cache::{BlockShufflingIds, ShufflingCache}; use crate::validator_monitor::{ValidatorMonitor, ValidatorMonitorConfig}; use crate::validator_pubkey_cache::ValidatorPubkeyCache; use crate::ChainConfig; +use crate::CustodyContext; use crate::{ BeaconChain, BeaconChainTypes, BeaconForkChoiceStore, BeaconSnapshot, Eth1Chain, Eth1ChainBackend, ServerSentEventHandler, @@ -43,9 +44,8 @@ use store::{Error as StoreError, HotColdDB, ItemStore, KeyValueStoreOp}; use task_executor::{ShutdownReason, TaskExecutor}; use tracing::{debug, error, info}; use types::{ - BeaconBlock, BeaconState, BlobSidecarList, ChainSpec, Checkpoint, CustodyContext, - DataColumnSidecarList, Epoch, EthSpec, FixedBytesExtended, Hash256, Signature, - SignedBeaconBlock, Slot, + BeaconBlock, BeaconState, BlobSidecarList, ChainSpec, Checkpoint, DataColumnSidecarList, Epoch, + EthSpec, FixedBytesExtended, Hash256, Signature, SignedBeaconBlock, Slot, }; /// An empty struct used to "witness" all the `BeaconChainTypes` traits. It has no user-facing diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 8b6ca8fa0b7..2a1a59eb233 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -5,7 +5,7 @@ use crate::block_verification_types::{ use crate::data_availability_checker::overflow_lru_cache::{ DataAvailabilityCheckerInner, ReconstructColumnsDecision, }; -use crate::{metrics, BeaconChain, BeaconChainTypes, BeaconStore}; +use crate::{metrics, BeaconChain, BeaconChainTypes, BeaconStore, CustodyContext}; use kzg::Kzg; use slot_clock::SlotClock; use std::fmt; @@ -17,8 +17,8 @@ use task_executor::TaskExecutor; use tracing::{debug, error, info_span, Instrument}; use types::blob_sidecar::{BlobIdentifier, BlobSidecar, FixedBlobSidecarList}; use types::{ - BlobSidecarList, ChainSpec, CustodyContext, DataColumnSidecar, DataColumnSidecarList, Epoch, - EthSpec, Hash256, RuntimeVariableList, SignedBeaconBlock, + BlobSidecarList, ChainSpec, DataColumnSidecar, DataColumnSidecarList, Epoch, EthSpec, Hash256, + RuntimeVariableList, SignedBeaconBlock, }; mod error; diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index 3478c183f34..e69615d7add 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -7,7 +7,7 @@ use crate::block_verification_types::{ }; use crate::data_availability_checker::{Availability, AvailabilityCheckError}; use crate::data_column_verification::KzgVerifiedCustodyDataColumn; -use crate::BeaconChainTypes; +use crate::{BeaconChainTypes, CustodyContext}; use lru::LruCache; use parking_lot::RwLock; use std::cmp::Ordering; diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index a680b97f32b..0eec6dc770f 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -12,7 +12,6 @@ pub mod bellatrix_readiness; pub mod blob_verification; pub mod block_reward; mod block_times_cache; -pub mod persisted_custody; mod block_verification; pub mod block_verification_types; pub mod builder; @@ -49,6 +48,7 @@ pub mod observed_data_sidecars; pub mod observed_operations; mod observed_slashable; mod persisted_beacon_chain; +pub mod persisted_custody; mod persisted_fork_choice; mod pre_finalization_cache; pub mod proposer_prep_service; @@ -60,6 +60,7 @@ pub mod summaries_dag; pub mod sync_committee_rewards; pub mod sync_committee_verification; pub mod test_utils; +pub mod validator_custody; pub mod validator_monitor; pub mod validator_pubkey_cache; @@ -101,3 +102,4 @@ pub use state_processing::per_block_processing::errors::{ }; pub use store; pub use types; +pub use validator_custody::CustodyContext; diff --git a/beacon_node/beacon_chain/src/persisted_custody.rs b/beacon_node/beacon_chain/src/persisted_custody.rs index 0f529a6a982..1f57f259eaa 100644 --- a/beacon_node/beacon_chain/src/persisted_custody.rs +++ b/beacon_node/beacon_chain/src/persisted_custody.rs @@ -1,7 +1,8 @@ +use crate::validator_custody::CustodyContextSsz; use ssz::{Decode, Encode}; use std::sync::Arc; use store::{DBColumn, Error as StoreError, HotColdDB, ItemStore, StoreItem}; -use types::{validator_custody::CustodyContextSsz, EthSpec, Hash256}; +use types::{EthSpec, Hash256}; /// 32-byte key for accessing the `CustodyContext`. All zero because `CustodyContext` has its own column. pub const CUSTODY_DB_KEY: Hash256 = Hash256::ZERO; diff --git a/consensus/types/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs similarity index 89% rename from consensus/types/src/validator_custody.rs rename to beacon_node/beacon_chain/src/validator_custody.rs index 9f8d09cd300..22445d2f410 100644 --- a/consensus/types/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -2,8 +2,9 @@ use std::collections::{BTreeMap, HashMap}; use parking_lot::RwLock; -use crate::{ChainSpec, Epoch, EthSpec, Slot}; use ssz_derive::{Decode, Encode}; +use tokio::sync::broadcast::{channel, Receiver, Sender}; +use types::{ChainSpec, Epoch, EthSpec, Slot}; /// Specifies the number of validators attached to this node. #[derive(Debug, Copy, Encode, Decode, Clone)] @@ -115,7 +116,7 @@ fn get_validators_custody_requirement(count: u64, spec: &ChainSpec) -> u64 { ) } -/// Contains all the information the node requires to calculate the +/// Contains all the information the node requires to calculate the /// number of columns to be custodied when checking for DA. #[derive(Debug)] pub struct CustodyContext { @@ -139,6 +140,9 @@ pub struct CustodyContext { persisted_is_supernode: bool, /// Maintains all the validators that this node is connected to currently validator_registrations: RwLock, + sender: Sender, + // TODO(pawan): don't need to keep this most likely + receiver: Receiver, } impl CustodyContext { @@ -147,12 +151,16 @@ impl CustodyContext { /// /// The `is_supernode` value is based on current cli parameters. pub fn new(is_supernode: bool) -> Self { + let (sender, receiver) = channel(10); + Self { advertised_validator_custody: RwLock::new(ValidatorCustodyCount::new(0)), validator_custody_at_head: RwLock::new(ValidatorCustodyCount::new(0)), current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, validator_registrations: Default::default(), + sender, + receiver, } } @@ -161,15 +169,22 @@ impl CustodyContext { ssz_context: CustodyContextSsz, is_supernode: bool, ) -> Self { + let (sender, receiver) = channel(10); CustodyContext { advertised_validator_custody: RwLock::new(ssz_context.advertised_validator_custody), validator_custody_at_head: RwLock::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_registrations: Default::default(), + sender, + receiver, } } + pub fn subscribe(&self) -> Receiver { + self.sender.subscribe() + } + /// Register a new validator index and updates the list of validators if required. /// /// Also modifies the internal structures if the validator custody has changed to @@ -180,10 +195,10 @@ impl CustodyContext { slot: Slot, spec: &ChainSpec, ) { - // Only do the registrations once per epoch - if slot % E::slots_per_epoch() != 0 { - return; - } + // // Only do the registrations once per epoch + // if slot % E::slots_per_epoch() != 0 { + // return; + // } let mut registrations = self.validator_registrations.write(); for (validator_index, effective_balance) in validators_and_balance { @@ -207,6 +222,14 @@ impl CustodyContext { *validator_custody_at_head = ValidatorCustodyCount { count: new_validator_custody, }; + if let Err(e) = self + .sender + .send(CustodyContextMessage::HeadCustodyCountChanged { + new_custody_count: new_validator_custody, + }) + { + tracing::error!(error=?e, "Failed to send custody context message"); + } } } @@ -242,6 +265,24 @@ impl CustodyContext { } } +// write a service that emits events when internal values change +#[derive(Debug, Clone)] +pub enum CustodyContextMessage { + /// The custody count changed because of a change in the + /// number of validators being managed. + /// + /// This should trigger actions downstream like + /// subscribing/unsubscribing new subnets/ + /// backfilling required columns. + HeadCustodyCountChanged { new_custody_count: u64 }, + /// The advertised custody count has changed. + /// + /// This should trigger downstream actions like setting + /// a new cgc value in the enr and metadata fields and + /// performing any related cleanup actions. + AdvertisedCustodyCountChanged { new_custody_count: u64 }, +} + /// The custody information that gets persisted across runs. #[derive(Debug, Encode, Decode, Clone)] pub struct CustodyContextSsz { diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index 463f585f2c7..86b49f071e5 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -138,8 +138,13 @@ pub async fn publish_block>( spawn_build_data_sidecar_task(chain.clone(), block.clone(), unverified_blobs)?; // Gossip verify the block and blobs/data columns separately. - let gossip_verified_block_result = unverified_block - .into_gossip_verified_block(&chain, network_globals.custody_columns_count() as usize); + let gossip_verified_block_result = unverified_block.into_gossip_verified_block( + &chain, + chain + .data_availability_checker + .custody_context() + .head_custody_count(&chain.spec) as usize, + ); let block_root = block_root.unwrap_or_else(|| { gossip_verified_block_result.as_ref().map_or_else( |_| block.canonical_root(), @@ -224,7 +229,7 @@ pub async fn publish_block>( publish_column_sidecars(network_tx, &gossip_verified_columns, &chain).map_err(|_| { warp_utils::reject::custom_server_error("unable to publish data column sidecars".into()) })?; - let sampling_columns_indices = &network_globals.sampling_columns; + let sampling_columns_indices = &network_globals.sampling_columns(); let sampling_columns = gossip_verified_columns .into_iter() .flatten() @@ -301,17 +306,22 @@ pub async fn publish_block>( slot = %block.slot(), "Block previously seen" ); - let import_result = Box::pin(chain.process_block( - block_root, - RpcBlock::new_without_blobs( - Some(block_root), - block.clone(), - network_globals.custody_columns_count() as usize, + let import_result = Box::pin( + chain.process_block( + block_root, + RpcBlock::new_without_blobs( + Some(block_root), + block.clone(), + chain + .data_availability_checker + .custody_context() + .head_custody_count(&chain.spec) as usize, + ), + NotifyExecutionLayer::Yes, + BlockImportSource::HttpApi, + publish_fn, ), - NotifyExecutionLayer::Yes, - BlockImportSource::HttpApi, - publish_fn, - )) + ) .await; post_block_import_logging_and_response( import_result, diff --git a/beacon_node/http_api/src/test_utils.rs b/beacon_node/http_api/src/test_utils.rs index 1fbb659ce3a..2340f7a7873 100644 --- a/beacon_node/http_api/src/test_utils.rs +++ b/beacon_node/http_api/src/test_utils.rs @@ -27,7 +27,7 @@ use std::sync::Arc; use std::time::Duration; use store::MemoryStore; use task_executor::test_utils::TestRuntime; -use types::{ChainSpec, CustodyContext, EthSpec}; +use types::{ChainSpec, EthSpec}; pub const TCP_PORT: u16 = 42; pub const UDP_PORT: u16 = 42; @@ -158,16 +158,13 @@ pub async fn create_api_server_with_config( let enr_key = CombinedKey::generate_secp256k1(); let enr = Enr::builder().build(&enr_key).unwrap(); let network_config = Arc::new(NetworkConfig::default()); - let custody_context = Arc::new(CustodyContext::new( - network_config.subscribe_all_data_column_subnets, - )); let network_globals = Arc::new(NetworkGlobals::new( enr.clone(), meta_data, vec![], false, network_config, - custody_context, + chain.spec.custody_requirement, chain.spec.clone(), )); diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index cd590580be4..04c47b1cbda 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -1786,6 +1786,5 @@ fn get_custody_columns(tester: &InteractiveTester) -> HashSet { .network_globals .as_ref() .unwrap() - .sampling_columns - .clone() + .sampling_columns() } diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index bafcc37d0ec..6685d42f055 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -177,6 +177,7 @@ impl Network { pub async fn new( executor: task_executor::TaskExecutor, mut ctx: ServiceContext<'_>, + custody_column_count: u64, ) -> Result<(Self, Arc>), String> { let config = ctx.config.clone(); trace!("Libp2p Service starting"); @@ -201,21 +202,20 @@ impl Network { )?; // Construct the metadata + // TODO(pawan): fix these let custody_group_count = ctx.chain_spec.is_peer_das_scheduled().then(|| { ctx.chain_spec .custody_group_count(config.subscribe_all_data_column_subnets) }); let meta_data = utils::load_or_build_metadata(&config.network_dir, custody_group_count); let seq_number = *meta_data.seq_number(); - // todo(pawan): load the persisted custody context here and modify the custody context before passing - // it to the globals let globals = NetworkGlobals::new( enr, meta_data, trusted_peers, config.disable_peer_scoring, config.clone(), - ctx.custody_context.clone(), + custody_column_count, ctx.chain_spec.clone(), ); let network_globals = Arc::new(globals); @@ -888,6 +888,24 @@ impl Network { } } + /// Subscribe to all data columns determined by the cgc. + /// TODO(pawan): unsubscribe if count reduces + #[instrument(parent = None, + level = "trace", + fields(service = "libp2p"), + name = "libp2p", + skip_all + )] + pub fn subscribe_new_data_column_subnets(&mut self, custody_column_count: u64) { + self.network_globals + .update_data_column_subnets(custody_column_count); + + for column in self.network_globals.sampling_subnets() { + let kind = GossipKind::DataColumnSidecar(column); + self.subscribe_kind(kind); + } + } + /// Returns the scoring parameters for a topic if set. #[instrument(parent = None, level = "trace", diff --git a/beacon_node/lighthouse_network/src/service/utils.rs b/beacon_node/lighthouse_network/src/service/utils.rs index 56821652996..01929bcb01c 100644 --- a/beacon_node/lighthouse_network/src/service/utils.rs +++ b/beacon_node/lighthouse_network/src/service/utils.rs @@ -18,8 +18,7 @@ use std::sync::Arc; use std::time::Duration; use tracing::{debug, warn}; use types::{ - ChainSpec, CustodyContext, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, - SyncSubnetId, + ChainSpec, DataColumnSubnetId, EnrForkId, EthSpec, ForkContext, SubnetId, SyncSubnetId, }; pub const NETWORK_KEY_FILENAME: &str = "key"; @@ -31,7 +30,6 @@ pub struct Context<'a> { pub enr_fork_id: EnrForkId, pub fork_context: Arc, pub chain_spec: Arc, - pub custody_context: Arc, pub libp2p_registry: Option<&'a mut Registry>, } diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index c1a709ce7e2..e58aed5e3df 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -11,7 +11,7 @@ use tracing::error; use types::data_column_custody_group::{ compute_columns_for_custody_group, compute_subnets_from_custody_group, get_custody_groups, }; -use types::{ChainSpec, ColumnIndex, CustodyContext, DataColumnSubnetId, EthSpec}; +use types::{ChainSpec, ColumnIndex, DataColumnSubnetId, EthSpec}; pub struct NetworkGlobals { /// The current local ENR. @@ -31,10 +31,8 @@ pub struct NetworkGlobals { /// The current state of the backfill sync. pub backfill_state: RwLock, /// The computed sampling subnets and columns is stored to avoid re-computing. - pub sampling_subnets: HashSet, - pub sampling_columns: HashSet, - /// Context for the amount of columns the node has to custody. - custody_context: Arc, + pub sampling_subnets: RwLock>, + pub sampling_columns: RwLock>, /// Network-related configuration. Immutable after initialization. pub config: Arc, /// Ethereum chain configuration. Immutable after initialization. @@ -48,23 +46,23 @@ impl NetworkGlobals { trusted_peers: Vec, disable_peer_scoring: bool, config: Arc, - custody_context: Arc, + custody_group_count: u64, spec: Arc, ) -> Self { let node_id = enr.node_id().raw(); - let custody_group_count = match local_metadata.custody_group_count() { - Ok(&cgc) if cgc <= spec.number_of_custody_groups => cgc, - _ => { - if spec.is_peer_das_scheduled() { - error!( - info = "falling back to default custody requirement", - "custody_group_count from metadata is either invalid or not set. This is a bug!" - ); - } - spec.custody_requirement - } - }; + // let custody_group_count = match local_metadata.custody_group_count() { + // Ok(&cgc) if cgc <= spec.number_of_custody_groups => cgc, + // _ => { + // if spec.is_peer_das_scheduled() { + // error!( + // info = "falling back to default custody requirement", + // "custody_group_count from metadata is either invalid or not set. This is a bug!" + // ); + // } + // spec.custody_requirement + // } + // }; // The below `expect` calls will panic on start up if the chain spec config values used // are invalid @@ -88,7 +86,12 @@ impl NetworkGlobals { sampling_columns.extend(columns); } - // set the values in custody_context based on the + tracing::debug!( + cgc = custody_group_count, + ?sampling_columns, + ?sampling_subnets, + "Starting node with custody params" + ); NetworkGlobals { local_enr: RwLock::new(enr.clone()), @@ -99,14 +102,40 @@ impl NetworkGlobals { gossipsub_subscriptions: RwLock::new(HashSet::new()), sync_state: RwLock::new(SyncState::Stalled), backfill_state: RwLock::new(BackFillState::Paused), - sampling_subnets, - sampling_columns, - custody_context, + sampling_subnets: RwLock::new(sampling_subnets), + sampling_columns: RwLock::new(sampling_columns), config, spec, } } + /// Update the sampling subnets based on an updated cgc. + pub fn update_data_column_subnets(&self, custody_group_count: u64) { + // The below `expect` calls will panic on start up if the chain spec config values used + // are invalid + let sampling_size = self + .spec + .sampling_size(custody_group_count) + .expect("should compute node sampling size from valid chain spec"); + let custody_groups = + get_custody_groups(self.local_enr().node_id().raw(), sampling_size, &self.spec) + .expect("should compute node custody groups"); + + let mut sampling_subnets = self.sampling_subnets.write(); + for custody_index in &custody_groups { + let subnets = compute_subnets_from_custody_group(*custody_index, &self.spec) + .expect("should compute custody subnets for node"); + sampling_subnets.extend(subnets); + } + + let mut sampling_columns = self.sampling_columns.write(); + for custody_index in &custody_groups { + let columns = compute_columns_for_custody_group(*custody_index, &self.spec) + .expect("should compute custody columns for node"); + sampling_columns.extend(columns); + } + } + /// Returns the local ENR from the underlying Discv5 behaviour that external peers may connect /// to. pub fn local_enr(&self) -> Enr { @@ -123,19 +152,6 @@ impl NetworkGlobals { self.listen_multiaddrs.read().clone() } - /// Returns true if this node is configured as a PeerDAS supernode - pub fn is_supernode(&self) -> bool { - self.custody_context.current_is_supernode - } - - /// Returns the count of custody columns this node must sample for block import - pub fn custody_columns_count(&self) -> u64 { - // This only panics if the chain spec contains invalid values - self.spec - .sampling_size(self.custody_context.head_custody_count(&self.spec)) - .expect("should compute node sampling size from valid chain spec") - } - /// Returns the number of libp2p connected peers. pub fn connected_peers(&self) -> usize { self.peers.read().connected_peer_ids().count() @@ -229,10 +245,18 @@ impl NetworkGlobals { enable_light_client_server: self.config.enable_light_client_server, subscribe_all_subnets: self.config.subscribe_all_subnets, subscribe_all_data_column_subnets: self.config.subscribe_all_data_column_subnets, - sampling_subnets: &self.sampling_subnets, + sampling_subnets: self.sampling_subnets.read().clone(), } } + pub fn sampling_columns(&self) -> HashSet { + self.sampling_columns.read().clone() + } + + pub fn sampling_subnets(&self) -> HashSet { + self.sampling_subnets.read().clone() + } + /// TESTING ONLY. Build a dummy NetworkGlobals instance. pub fn new_test_globals( trusted_peers: Vec, @@ -258,16 +282,13 @@ impl NetworkGlobals { let keypair = libp2p::identity::secp256k1::Keypair::generate(); let enr_key: discv5::enr::CombinedKey = discv5::enr::CombinedKey::from_secp256k1(&keypair); let enr = discv5::enr::Enr::builder().build(&enr_key).unwrap(); - let custody_context = Arc::new(CustodyContext::new( - config.subscribe_all_data_column_subnets, - )); NetworkGlobals::new( enr, metadata, trusted_peers, false, config, - custody_context, + spec.number_of_columns, spec, ) } diff --git a/beacon_node/lighthouse_network/src/types/topics.rs b/beacon_node/lighthouse_network/src/types/topics.rs index 56b97303d3e..7a959ecaac6 100644 --- a/beacon_node/lighthouse_network/src/types/topics.rs +++ b/beacon_node/lighthouse_network/src/types/topics.rs @@ -26,11 +26,11 @@ pub const LIGHT_CLIENT_FINALITY_UPDATE: &str = "light_client_finality_update"; pub const LIGHT_CLIENT_OPTIMISTIC_UPDATE: &str = "light_client_optimistic_update"; #[derive(Debug)] -pub struct TopicConfig<'a> { +pub struct TopicConfig { pub enable_light_client_server: bool, pub subscribe_all_subnets: bool, pub subscribe_all_data_column_subnets: bool, - pub sampling_subnets: &'a HashSet, + pub sampling_subnets: HashSet, } /// Returns all the topics the node should subscribe at `fork_name` @@ -85,7 +85,7 @@ pub fn core_topics_to_subscribe( topics.push(GossipKind::DataColumnSidecar(i.into())); } } else { - for subnet in opts.sampling_subnets { + for subnet in &opts.sampling_subnets { topics.push(GossipKind::DataColumnSidecar(*subnet)); } } @@ -126,7 +126,7 @@ pub fn all_topics_at_fork(fork: ForkName, spec: &ChainSpec) -> Vec(fork, &opts, spec) } diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index b129b548416..05c7dc287b0 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -780,7 +780,7 @@ pub fn update_sync_metrics(network_globals: &Arc>) let all_column_subnets = (0..network_globals.spec.data_column_sidecar_subnet_count).map(DataColumnSubnetId::new); - let custody_column_subnets = network_globals.sampling_subnets.iter(); + let custody_column_subnets = network_globals.sampling_subnets(); // Iterate all subnet values to set to zero the empty entries in peers_per_column_subnet for subnet in all_column_subnets { @@ -794,7 +794,7 @@ pub fn update_sync_metrics(network_globals: &Arc>) // Registering this metric is a duplicate for supernodes but helpful for fullnodes. This way // operators can monitor the health of only the subnets of their interest without complex // Grafana queries. - for subnet in custody_column_subnets { + for subnet in custody_column_subnets.iter() { set_gauge_entry( &PEERS_PER_CUSTODY_COLUMN_SUBNET, &[&format!("{subnet}")], diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 8757ab43830..41e8697532a 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1274,7 +1274,10 @@ impl NetworkBeaconProcessor { .clone() .verify_block_for_gossip( block.clone(), - self.network_globals.custody_columns_count() as usize, + self.chain + .data_availability_checker + .custody_context() + .head_custody_count(&self.chain.spec) as usize, ) .await; diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index f9390a2c7b8..df9b656051b 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -843,7 +843,7 @@ impl NetworkBeaconProcessor { block_root: Hash256, publish_blobs: bool, ) { - let custody_columns = self.network_globals.sampling_columns.clone(); + let custody_columns = self.network_globals.sampling_columns(); let self_cloned = self.clone(); let publish_fn = move |blobs_or_data_column| { if publish_blobs { @@ -930,7 +930,12 @@ impl NetworkBeaconProcessor { publish_columns: bool, ) -> Option { // Only supernodes attempt reconstruction - if !self.network_globals.is_supernode() { + if !self + .chain + .data_availability_checker + .custody_context() + .current_is_supernode + { return None; } diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index cb9c9764044..bdae87bd0bd 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -285,7 +285,7 @@ impl TestRig { ) .unwrap() .into_iter() - .filter(|c| network_globals.sampling_columns.contains(&c.index)) + .filter(|c| network_globals.sampling_columns().contains(&c.index)) .collect::>(); (None, Some(custody_columns)) diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index aaf29733534..61e64b3deae 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -5,11 +5,13 @@ use crate::persisted_dht::{clear_dht, load_dht, persist_dht}; use crate::router::{Router, RouterMessage}; use crate::subnet_service::{SubnetService, SubnetServiceMessage, Subscription}; use crate::NetworkConfig; +use beacon_chain::validator_custody::CustodyContextMessage; use beacon_chain::{BeaconChain, BeaconChainTypes}; use beacon_processor::{work_reprocessing_queue::ReprocessQueueMessage, BeaconProcessorSend}; use futures::channel::mpsc::Sender; use futures::future::OptionFuture; use futures::prelude::*; + use lighthouse_network::rpc::InboundRequestId; use lighthouse_network::rpc::RequestType; use lighthouse_network::service::Network; @@ -195,6 +197,8 @@ pub struct NetworkService { gossipsub_parameter_update: tokio::time::Interval, /// Provides fork specific info. fork_context: Arc, + /// Receiver for custody context messages + custody_context_rx: tokio::sync::broadcast::Receiver, } impl NetworkService { @@ -266,12 +270,19 @@ impl NetworkService { enr_fork_id, fork_context: fork_context.clone(), chain_spec: beacon_chain.spec.clone(), - custody_context: beacon_chain.data_availability_checker.custody_context(), libp2p_registry, }; // launch libp2p service - let (mut libp2p, network_globals) = Network::new(executor.clone(), service_context).await?; + let (mut libp2p, network_globals) = Network::new( + executor.clone(), + service_context, + beacon_chain + .data_availability_checker + .custody_context() + .head_custody_count(&beacon_chain.spec), + ) + .await?; // Repopulate the DHT with stored ENR's if discovery is not disabled. if !config.disable_discovery { @@ -323,6 +334,11 @@ impl NetworkService { validator_subscription_recv, } = network_receivers; + let custody_context_rx = beacon_chain + .data_availability_checker + .custody_context() + .subscribe(); + // create the network service and spawn the task let network_service = NetworkService { beacon_chain, @@ -341,6 +357,7 @@ impl NetworkService { metrics_update, gossipsub_parameter_update, fork_context, + custody_context_rx, }; Ok((network_service, network_globals, network_senders)) @@ -432,6 +449,19 @@ impl NetworkService { _ = self.gossipsub_parameter_update.tick() => self.update_gossipsub_parameters(), + custody_message = Box::pin(self.custody_context_rx.recv()) => { + match custody_message { + Ok(CustodyContextMessage::HeadCustodyCountChanged{new_custody_count}) => { + self.libp2p.subscribe_new_data_column_subnets(new_custody_count); + } + Ok(CustodyContextMessage::AdvertisedCustodyCountChanged{new_custody_count}) => { + } + Err(e) => { + error!("Error receiving custody context message: {:?}", e); + } + } + }, + // handle a message sent to the network Some(msg) = self.network_recv.recv() => self.on_network_msg(msg, &mut shutdown_sender).await, diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 58641f86069..138fd826c1b 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -476,7 +476,7 @@ impl SyncNetworkContext { // Attempt to find all required custody peers before sending any request or creating an ID let columns_by_range_peers_to_request = if matches!(batch_type, ByRangeRequestType::BlocksAndColumns) { - let column_indexes = self.network_globals().sampling_columns.clone(); + let column_indexes = self.network_globals().sampling_columns(); Some(self.select_columns_by_range_peers_to_request( &column_indexes, peers, @@ -534,7 +534,7 @@ impl SyncNetworkContext { ( data_column_requests, self.network_globals() - .sampling_columns + .sampling_columns() .clone() .iter() .copied() @@ -928,8 +928,7 @@ impl SyncNetworkContext { // Include only the blob indexes not yet imported (received through gossip) let custody_indexes_to_fetch = self .network_globals() - .sampling_columns - .clone() + .sampling_columns() .into_iter() .filter(|index| !custody_indexes_imported.contains(index)) .collect::>(); @@ -1490,7 +1489,10 @@ impl SyncNetworkContext { let block = RpcBlock::new_without_blobs( Some(block_root), block, - self.network_globals().custody_columns_count() as usize, + self.chain + .data_availability_checker + .custody_context() + .head_custody_count(&self.chain.spec) as usize, ); debug!(block = ?block_root, id, "Sending block for processing"); diff --git a/beacon_node/network/src/sync/range_sync/chain.rs b/beacon_node/network/src/sync/range_sync/chain.rs index be017344170..6100d322b83 100644 --- a/beacon_node/network/src/sync/range_sync/chain.rs +++ b/beacon_node/network/src/sync/range_sync/chain.rs @@ -1032,7 +1032,7 @@ impl SyncingChain { // Require peers on all sampling column subnets before sending batches let peers_on_all_custody_subnets = network .network_globals() - .sampling_subnets + .sampling_subnets() .iter() .all(|subnet_id| { let peer_count = network diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 84ff1c7e259..5a53bf63180 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1205,12 +1205,8 @@ impl TestRig { payload_verification_status: PayloadVerificationStatus::Verified, is_valid_merge_transition_block: false, }; - let executed_block = AvailabilityPendingExecutedBlock::new( - block, - import_data, - payload_verification_outcome, - self.network_globals.custody_columns_count() as usize, - ); + let executed_block = + AvailabilityPendingExecutedBlock::new(block, import_data, payload_verification_outcome); match self .harness .chain diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 9d0524d028a..f0555a06d6d 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -72,7 +72,6 @@ pub mod signing_data; pub mod sync_committee_subscription; pub mod sync_duty; pub mod validator; -pub mod validator_custody; pub mod validator_subscription; pub mod voluntary_exit; pub mod withdrawal_credentials; @@ -265,7 +264,6 @@ pub use crate::withdrawal::Withdrawal; pub use crate::withdrawal_credentials::WithdrawalCredentials; pub use crate::withdrawal_request::WithdrawalRequest; pub use fixed_bytes::FixedBytesExtended; -pub use validator_custody::CustodyContext; pub type CommitteeIndex = u64; pub type Hash256 = fixed_bytes::Hash256; From 748a65bcf478cb97dc3676d0b6c910571bde45f9 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 6 Jun 2025 16:31:59 -0700 Subject: [PATCH 10/27] Fix a bunch of conditions; change internal api to use atomics --- .../beacon_chain/src/validator_custody.rs | 189 ++++++++++-------- 1 file changed, 104 insertions(+), 85 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 22445d2f410..b973a675569 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -1,4 +1,7 @@ -use std::collections::{BTreeMap, HashMap}; +use std::{ + collections::{BTreeMap, HashMap}, + sync::atomic::{AtomicU64, Ordering}, +}; use parking_lot::RwLock; @@ -6,7 +9,13 @@ use ssz_derive::{Decode, Encode}; use tokio::sync::broadcast::{channel, Receiver, Sender}; use types::{ChainSpec, Epoch, EthSpec, Slot}; -/// Specifies the number of validators attached to this node. +const CHANNEL_CAPACITY: usize = 10; + +/// Specifies the number of validators attached to this node in increments of +/// `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. +/// +/// Note: The `count` here effectively refers to the sum of all effective +/// balances of all attached validators divided by `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. #[derive(Debug, Copy, Encode, Decode, Clone)] pub struct ValidatorCustodyCount { count: u64, @@ -25,44 +34,36 @@ impl std::ops::Deref for ValidatorCustodyCount { } } -impl ValidatorCustodyCount { - /// Number of columns/custody groups to custody based on the number of validators - /// attached. - pub fn custody_count(&self, spec: &ChainSpec) -> u64 { - std::cmp::min( - spec.number_of_columns as u64, - spec.validator_custody_requirement + self.count - 1, - ) - } -} - /// TODO(pawan): this currently just registers increases in validator count. /// Does not handle decreasing validator counts #[derive(Default, Debug)] struct ValidatorRegistrations { /// Set of all validators that is registered to this node along with its effective balance /// in increments of `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` + /// + /// Key is validator index and value is effective_balance // BALANCE_PER_ADDITIONAL_CUSTODY_GROUP. validators: HashMap, - /// Maintains the number of unique validators that hit the subscriptions endpoint each - /// epoch where the validator count changed from the previous epoch. + /// Maintains the validator custody requirement at a given epoch. /// - /// If the count is same between subsequent epochs, then the later epoch values aren't stored - /// to save space. - epoch_validators: BTreeMap, + /// Note: Only stores the epoch value when there's a change in custody requirement. + /// So if epoch 10 and 11 has the same custody requirement, only 10 is stored. + epoch_validator_custody_requirements: BTreeMap, } impl ValidatorRegistrations { - /// Returns the validator count at the latest epoch for the custody requirement. - /// - /// This should be equivalent to the current `validator_custody_at_head`. - pub fn latest_validator_count_for_custody(&self) -> Option { - self.epoch_validators.last_key_value().map(|(_, v)| *v) + /// Returns the validator custody requirement at the latest epoch. + pub fn latest_validator_custody_requirement(&self) -> Option { + self.epoch_validator_custody_requirements + .last_key_value() + .map(|(_, v)| *v) } /// Returns the latest epoch at which the validator count changed. #[allow(dead_code)] pub fn latest_epoch(&self) -> Option { - self.epoch_validators.last_key_value().map(|(k, _)| *k) + self.epoch_validator_custody_requirements + .last_key_value() + .map(|(k, _)| *k) } /// Register a new validator index and updates the list of validators if required. @@ -81,19 +82,22 @@ impl ValidatorRegistrations { .insert(validator_index, num_validators_for_effective_balance); // Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of "weight". - let count_at_epoch = self.validators.values().sum(); + let validator_count_at_epoch = self.validators.values().sum(); + let validator_custody_requirement = + get_validators_custody_requirement(validator_count_at_epoch, spec); - // If registering the new validator increased the validator count, then + // If registering the new validator increased the total validator "units", then // add a new entry for the current epoch - if Some(count_at_epoch) != self.latest_validator_count_for_custody() { - self.epoch_validators + if Some(validator_custody_requirement) != self.latest_validator_custody_requirement() { + self.epoch_validator_custody_requirements .entry(epoch) - .and_modify(|old_count| *old_count = count_at_epoch) - .or_insert(count_at_epoch); + .and_modify(|old_custody| *old_custody = validator_custody_requirement) + .or_insert(validator_custody_requirement); } tracing::debug!( %epoch, - validator_count = count_at_epoch, + validator_count = validator_count_at_epoch, + validator_custody_requirement, "Registered validators" ); } @@ -123,12 +127,12 @@ pub struct CustodyContext { /// Columns to be custodied based on number of validators /// that is attached to this node. /// - /// This is the number that we use to compute the custody count value that + /// This is the number that we use to compute the cgc value that /// we advertise to our peers in the metadata and enr values. - advertised_validator_custody: RwLock, - /// This is the validator count that we use to compute the number of columns we need to - /// sample at head (while syncing or when receiving gossip). - validator_custody_at_head: RwLock, + advertised_validator_custody_count: AtomicU64, + /// This is the validator custody count that we use to compute the number of columns we need to + /// custody at head (while syncing or when receiving gossip). + validator_custody_count_at_head: AtomicU64, /// Is the node run as a supernode based on current cli parameters. pub current_is_supernode: bool, /// The persisted value for `is_supernode` based on the previous run of this node. @@ -151,11 +155,11 @@ impl CustodyContext { /// /// The `is_supernode` value is based on current cli parameters. pub fn new(is_supernode: bool) -> Self { - let (sender, receiver) = channel(10); + let (sender, receiver) = channel(CHANNEL_CAPACITY); Self { - advertised_validator_custody: RwLock::new(ValidatorCustodyCount::new(0)), - validator_custody_at_head: RwLock::new(ValidatorCustodyCount::new(0)), + advertised_validator_custody_count: AtomicU64::new(0), + validator_custody_count_at_head: AtomicU64::new(0), current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, validator_registrations: Default::default(), @@ -169,10 +173,12 @@ impl CustodyContext { ssz_context: CustodyContextSsz, is_supernode: bool, ) -> Self { - let (sender, receiver) = channel(10); + let (sender, receiver) = channel(CHANNEL_CAPACITY); CustodyContext { - advertised_validator_custody: RwLock::new(ssz_context.advertised_validator_custody), - validator_custody_at_head: RwLock::new(ssz_context.validator_custody_at_head), + advertised_validator_custody_count: AtomicU64::new( + ssz_context.advertised_validator_custody, + ), + validator_custody_count_at_head: AtomicU64::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_registrations: Default::default(), @@ -195,40 +201,47 @@ impl CustodyContext { slot: Slot, spec: &ChainSpec, ) { - // // Only do the registrations once per epoch - // if slot % E::slots_per_epoch() != 0 { - // return; - // } - + // Complete all registrations let mut registrations = self.validator_registrations.write(); for (validator_index, effective_balance) in validators_and_balance { registrations.register_validator::(validator_index, effective_balance, slot, spec); } - // Completed registrations, now check if the cgc has changed - let mut validator_custody_at_head = self.validator_custody_at_head.write(); - let Some(new_validator_custody) = registrations.latest_validator_count_for_custody() else { + // Completed registrations, now check if the validator custody requirement has changed + let Some(new_validator_custody) = registrations.latest_validator_custody_requirement() + else { return; }; - // Update the current validator custody value if the validator registration changes the number of - // validators - if new_validator_custody != validator_custody_at_head.count { + let current_cgc = self.head_custody_count(spec); + let validator_custody_count_at_head = + self.validator_custody_count_at_head.load(Ordering::Relaxed); + + if new_validator_custody != validator_custody_count_at_head { tracing::debug!( - old_count = validator_custody_at_head.count, + old_count = validator_custody_count_at_head, new_count = new_validator_custody, "Validator count at head updated" ); - *validator_custody_at_head = ValidatorCustodyCount { - count: new_validator_custody, - }; - if let Err(e) = self - .sender - .send(CustodyContextMessage::HeadCustodyCountChanged { - new_custody_count: new_validator_custody, - }) - { - tracing::error!(error=?e, "Failed to send custody context message"); + self.validator_custody_count_at_head + .store(new_validator_custody, Ordering::Relaxed); + + let updated_cgc = self.head_custody_count(spec); + // Send the message to network only if there are more columns subnets to subscribe to + if updated_cgc > current_cgc { + tracing::debug!( + old_cgc = current_cgc, + updated_cgc, + "Custody group count updated" + ); + if let Err(e) = self + .sender + .send(CustodyContextMessage::HeadCustodyCountChanged { + new_custody_count: updated_cgc, + }) + { + tracing::error!(error=?e, "Failed to send custody context message"); + } } } } @@ -237,15 +250,15 @@ impl CustodyContext { /// enr values. pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> u64 { if self.persisted_is_supernode { - return spec.number_of_columns; - } - let advertised_validator_custody = self.advertised_validator_custody.read().count; - if advertised_validator_custody > 0 { - get_validators_custody_requirement(advertised_validator_custody, spec) - + spec.custody_requirement - } else { - spec.custody_requirement + return spec.number_of_custody_groups; } + let advertised_validator_custody = self + .advertised_validator_custody_count + .load(Ordering::Relaxed); + std::cmp::min( + advertised_validator_custody + spec.custody_requirement, + spec.number_of_custody_groups, + ) } /// The custody count that we use to custody columns currently. @@ -254,14 +267,14 @@ impl CustodyContext { /// need to custody when receiving blocks over gossip/rpc or during sync. pub fn head_custody_count(&self, spec: &ChainSpec) -> u64 { if self.current_is_supernode { - return spec.number_of_columns; - } - let custody_at_head = self.validator_custody_at_head.read().count; - if custody_at_head > 0 { - get_validators_custody_requirement(custody_at_head, spec) + spec.custody_requirement - } else { - spec.custody_requirement + return spec.number_of_custody_groups; } + let validator_custody_count_at_head = + self.validator_custody_count_at_head.load(Ordering::Relaxed); + std::cmp::min( + validator_custody_count_at_head + spec.custody_requirement, + spec.number_of_custody_groups, + ) } } @@ -286,16 +299,22 @@ pub enum CustodyContextMessage { /// The custody information that gets persisted across runs. #[derive(Debug, Encode, Decode, Clone)] pub struct CustodyContextSsz { - advertised_validator_custody: ValidatorCustodyCount, - validator_custody_at_head: ValidatorCustodyCount, + advertised_validator_custody: u64, + validator_custody_at_head: u64, persisted_is_supernode: bool, } impl From<&CustodyContext> for CustodyContextSsz { fn from(context: &CustodyContext) -> Self { CustodyContextSsz { - advertised_validator_custody: context.advertised_validator_custody.read().clone(), - validator_custody_at_head: context.validator_custody_at_head.read().clone(), + advertised_validator_custody: context + .advertised_validator_custody_count + .load(Ordering::Relaxed) + .clone(), + validator_custody_at_head: context + .validator_custody_count_at_head + .load(Ordering::Relaxed) + .clone(), persisted_is_supernode: context.persisted_is_supernode, } } @@ -316,11 +335,11 @@ mod tests { let spec = E::default_spec(); assert_eq!( custody_context.head_custody_count(&spec), - spec.number_of_columns + spec.number_of_custody_groups ); assert_eq!( custody_context.advertised_custody_column_count(&spec), - spec.number_of_columns + spec.number_of_custody_groups ); } From 6ab8bbd83e86a7f317cdd033c128ba26ca2d6ce2 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 6 Jun 2025 18:53:24 -0700 Subject: [PATCH 11/27] Renames and some logic fixes --- .../beacon_chain/src/validator_custody.rs | 84 ++++++++----------- beacon_node/http_api/src/publish_blocks.rs | 4 +- .../lighthouse_network/src/service/mod.rs | 16 ++-- .../lighthouse_network/src/types/globals.rs | 13 --- .../gossip_methods.rs | 2 +- beacon_node/network/src/service.rs | 2 +- .../network/src/sync/network_context.rs | 2 +- consensus/types/src/chain_spec.rs | 8 -- 8 files changed, 50 insertions(+), 81 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index b973a675569..2b1cc95b224 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -11,29 +11,6 @@ use types::{ChainSpec, Epoch, EthSpec, Slot}; const CHANNEL_CAPACITY: usize = 10; -/// Specifies the number of validators attached to this node in increments of -/// `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. -/// -/// Note: The `count` here effectively refers to the sum of all effective -/// balances of all attached validators divided by `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. -#[derive(Debug, Copy, Encode, Decode, Clone)] -pub struct ValidatorCustodyCount { - count: u64, -} - -impl ValidatorCustodyCount { - pub fn new(count: u64) -> Self { - Self { count } - } -} - -impl std::ops::Deref for ValidatorCustodyCount { - type Target = u64; - fn deref(&self) -> &Self::Target { - &self.count - } -} - /// TODO(pawan): this currently just registers increases in validator count. /// Does not handle decreasing validator counts #[derive(Default, Debug)] @@ -213,7 +190,7 @@ impl CustodyContext { return; }; - let current_cgc = self.head_custody_count(spec); + let current_cgc = self.custody_group_count(spec); let validator_custody_count_at_head = self.validator_custody_count_at_head.load(Ordering::Relaxed); @@ -226,7 +203,7 @@ impl CustodyContext { self.validator_custody_count_at_head .store(new_validator_custody, Ordering::Relaxed); - let updated_cgc = self.head_custody_count(spec); + let updated_cgc = self.custody_group_count(spec); // Send the message to network only if there are more columns subnets to subscribe to if updated_cgc > current_cgc { tracing::debug!( @@ -248,33 +225,46 @@ impl CustodyContext { /// The custody count that we advertise to our peers in our metadata and /// enr values. - pub fn advertised_custody_column_count(&self, spec: &ChainSpec) -> u64 { + pub fn advertised_custody_group_count(&self, spec: &ChainSpec) -> u64 { if self.persisted_is_supernode { return spec.number_of_custody_groups; } let advertised_validator_custody = self .advertised_validator_custody_count .load(Ordering::Relaxed); - std::cmp::min( - advertised_validator_custody + spec.custody_requirement, - spec.number_of_custody_groups, - ) + + // If there are no validators, return the minimum custody_requirement + if advertised_validator_custody > 0 { + advertised_validator_custody + } else { + spec.custody_requirement + } } /// The custody count that we use to custody columns currently. /// /// This function should be called when figuring out how many columns we /// need to custody when receiving blocks over gossip/rpc or during sync. - pub fn head_custody_count(&self, spec: &ChainSpec) -> u64 { + pub fn custody_group_count(&self, spec: &ChainSpec) -> u64 { if self.current_is_supernode { return spec.number_of_custody_groups; } let validator_custody_count_at_head = self.validator_custody_count_at_head.load(Ordering::Relaxed); - std::cmp::min( - validator_custody_count_at_head + spec.custody_requirement, - spec.number_of_custody_groups, - ) + + // If there are no validators, return the minimum custody_requirement + if validator_custody_count_at_head > 0 { + validator_custody_count_at_head + } else { + spec.custody_requirement + } + } + + /// Returns the count of custody columns this node must sample for block import. + pub fn sampling_count(&self, spec: &ChainSpec) -> u64 { + // This only panics if the chain spec contains invalid values + spec.sampling_size(self.custody_group_count(spec)) + .expect("should compute node sampling size from valid chain spec") } } @@ -334,11 +324,11 @@ mod tests { let custody_context = CustodyContext::new(true); let spec = E::default_spec(); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.number_of_custody_groups ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.number_of_custody_groups ); } @@ -349,12 +339,12 @@ mod tests { let custody_context = CustodyContext::new(false); let spec = E::default_spec(); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.custody_requirement, "head custody count should be minimum spec custody requirement" ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should be minimum spec custody requirement" ); @@ -363,13 +353,13 @@ mod tests { custody_context.register_validator::(vec![(0, 32_000_000_000)], Slot::new(0), &spec); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.validator_custody_requirement + spec.custody_requirement, "head custody count should increase with 1 validator" ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should not change" ); @@ -390,13 +380,13 @@ mod tests { ); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.validator_custody_requirement + spec.custody_requirement, "head custody count should should be same as with 1 validator" ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should not change" ); @@ -404,13 +394,13 @@ mod tests { // adding 1 more validator should increase the custody count custody_context.register_validator::(vec![(8, 32_000_000_000)], Slot::new(0), &spec); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.validator_custody_requirement + spec.custody_requirement + 1, "head custody count should increase by 1" ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should not change" ); @@ -433,13 +423,13 @@ mod tests { ); assert_eq!( - custody_context.head_custody_count(&spec), + custody_context.custody_group_count(&spec), spec.validator_custody_requirement + spec.custody_requirement + 4, "head custody count should increase by 3" ); assert_eq!( - custody_context.advertised_custody_column_count(&spec), + custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should not change" ); diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index 86b49f071e5..c2c6459828e 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -143,7 +143,7 @@ pub async fn publish_block>( chain .data_availability_checker .custody_context() - .head_custody_count(&chain.spec) as usize, + .sampling_count(&chain.spec) as usize, ); let block_root = block_root.unwrap_or_else(|| { gossip_verified_block_result.as_ref().map_or_else( @@ -315,7 +315,7 @@ pub async fn publish_block>( chain .data_availability_checker .custody_context() - .head_custody_count(&chain.spec) as usize, + .sampling_count(&chain.spec) as usize, ), NotifyExecutionLayer::Yes, BlockImportSource::HttpApi, diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 6685d42f055..a21ad993c6c 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -177,7 +177,7 @@ impl Network { pub async fn new( executor: task_executor::TaskExecutor, mut ctx: ServiceContext<'_>, - custody_column_count: u64, + custody_group_count: u64, ) -> Result<(Self, Arc>), String> { let config = ctx.config.clone(); trace!("Libp2p Service starting"); @@ -202,12 +202,12 @@ impl Network { )?; // Construct the metadata - // TODO(pawan): fix these - let custody_group_count = ctx.chain_spec.is_peer_das_scheduled().then(|| { - ctx.chain_spec - .custody_group_count(config.subscribe_all_data_column_subnets) - }); - let meta_data = utils::load_or_build_metadata(&config.network_dir, custody_group_count); + let custody_group_count_metadata = ctx + .chain_spec + .is_peer_das_scheduled() + .then(|| custody_group_count); + let meta_data = + utils::load_or_build_metadata(&config.network_dir, custody_group_count_metadata); let seq_number = *meta_data.seq_number(); let globals = NetworkGlobals::new( enr, @@ -215,7 +215,7 @@ impl Network { trusted_peers, config.disable_peer_scoring, config.clone(), - custody_column_count, + custody_group_count, ctx.chain_spec.clone(), ); let network_globals = Arc::new(globals); diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index e58aed5e3df..da71137126f 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -51,19 +51,6 @@ impl NetworkGlobals { ) -> Self { let node_id = enr.node_id().raw(); - // let custody_group_count = match local_metadata.custody_group_count() { - // Ok(&cgc) if cgc <= spec.number_of_custody_groups => cgc, - // _ => { - // if spec.is_peer_das_scheduled() { - // error!( - // info = "falling back to default custody requirement", - // "custody_group_count from metadata is either invalid or not set. This is a bug!" - // ); - // } - // spec.custody_requirement - // } - // }; - // The below `expect` calls will panic on start up if the chain spec config values used // are invalid let sampling_size = spec diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 41e8697532a..b65b996d092 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1277,7 +1277,7 @@ impl NetworkBeaconProcessor { self.chain .data_availability_checker .custody_context() - .head_custody_count(&self.chain.spec) as usize, + .sampling_count(&self.chain.spec) as usize, ) .await; diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 61e64b3deae..2549131c11f 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -280,7 +280,7 @@ impl NetworkService { beacon_chain .data_availability_checker .custody_context() - .head_custody_count(&beacon_chain.spec), + .sampling_count(&beacon_chain.spec), ) .await?; diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 138fd826c1b..9f20174b641 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -1492,7 +1492,7 @@ impl SyncNetworkContext { self.chain .data_availability_checker .custody_context() - .head_custody_count(&self.chain.spec) as usize, + .sampling_count(&self.chain.spec) as usize, ); debug!(block = ?block_root, id, "Sending block for processing"); diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 072a55f9b94..b4fd5afe871 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -733,14 +733,6 @@ impl ChainSpec { Ok(std::cmp::max(custody_column_count, self.samples_per_slot)) } - pub fn custody_group_count(&self, is_supernode: bool) -> u64 { - if is_supernode { - self.number_of_custody_groups - } else { - self.custody_requirement - } - } - pub fn all_data_column_sidecar_subnets(&self) -> impl Iterator { (0..self.data_column_sidecar_subnet_count).map(DataColumnSubnetId::new) } From b23a4dcc787728109893b0e09520a8fde15d1c8f Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 6 Jun 2025 19:10:13 -0700 Subject: [PATCH 12/27] Remove unnecessary receiver --- .../src/data_availability_checker/overflow_lru_cache.rs | 2 +- beacon_node/beacon_chain/src/validator_custody.rs | 8 ++------ beacon_node/lighthouse_network/src/types/globals.rs | 1 - beacon_node/network/src/service.rs | 3 ++- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index e69615d7add..3478c183f34 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -7,7 +7,7 @@ use crate::block_verification_types::{ }; use crate::data_availability_checker::{Availability, AvailabilityCheckError}; use crate::data_column_verification::KzgVerifiedCustodyDataColumn; -use crate::{BeaconChainTypes, CustodyContext}; +use crate::BeaconChainTypes; use lru::LruCache; use parking_lot::RwLock; use std::cmp::Ordering; diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 2b1cc95b224..b85c756057f 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -122,8 +122,6 @@ pub struct CustodyContext { /// Maintains all the validators that this node is connected to currently validator_registrations: RwLock, sender: Sender, - // TODO(pawan): don't need to keep this most likely - receiver: Receiver, } impl CustodyContext { @@ -132,7 +130,7 @@ impl CustodyContext { /// /// The `is_supernode` value is based on current cli parameters. pub fn new(is_supernode: bool) -> Self { - let (sender, receiver) = channel(CHANNEL_CAPACITY); + let (sender, _) = channel(CHANNEL_CAPACITY); Self { advertised_validator_custody_count: AtomicU64::new(0), @@ -141,7 +139,6 @@ impl CustodyContext { persisted_is_supernode: is_supernode, validator_registrations: Default::default(), sender, - receiver, } } @@ -150,7 +147,7 @@ impl CustodyContext { ssz_context: CustodyContextSsz, is_supernode: bool, ) -> Self { - let (sender, receiver) = channel(CHANNEL_CAPACITY); + let (sender, _) = channel(CHANNEL_CAPACITY); CustodyContext { advertised_validator_custody_count: AtomicU64::new( ssz_context.advertised_validator_custody, @@ -160,7 +157,6 @@ impl CustodyContext { persisted_is_supernode: ssz_context.persisted_is_supernode, validator_registrations: Default::default(), sender, - receiver, } } diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index da71137126f..9f7ce72f3c0 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -7,7 +7,6 @@ use crate::{Client, Enr, EnrExt, GossipTopic, Multiaddr, NetworkConfig, PeerId}; use parking_lot::RwLock; use std::collections::HashSet; use std::sync::Arc; -use tracing::error; use types::data_column_custody_group::{ compute_columns_for_custody_group, compute_subnets_from_custody_group, get_custody_groups, }; diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 2549131c11f..00d4a261360 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -454,7 +454,8 @@ impl NetworkService { Ok(CustodyContextMessage::HeadCustodyCountChanged{new_custody_count}) => { self.libp2p.subscribe_new_data_column_subnets(new_custody_count); } - Ok(CustodyContextMessage::AdvertisedCustodyCountChanged{new_custody_count}) => { + Ok(CustodyContextMessage::AdvertisedCustodyCountChanged{new_custody_count: _}) => { + unimplemented!("update metadata and enr"); } Err(e) => { error!("Error receiving custody context message: {:?}", e); From 1adcc4a5406911fa8677a51507a787a9c1565e2c Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Mon, 9 Jun 2025 17:46:41 +0200 Subject: [PATCH 13/27] Update validator custody calculation and tests. --- .../beacon_chain/src/validator_custody.rs | 178 +++++++++--------- beacon_node/http_api/src/lib.rs | 2 +- .../lighthouse_network/src/service/mod.rs | 2 +- .../lighthouse_network/src/types/topics.rs | 2 +- 4 files changed, 89 insertions(+), 95 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index b85c756057f..b5d35e82252 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -16,9 +16,8 @@ const CHANNEL_CAPACITY: usize = 10; #[derive(Default, Debug)] struct ValidatorRegistrations { /// Set of all validators that is registered to this node along with its effective balance - /// in increments of `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` /// - /// Key is validator index and value is effective_balance // BALANCE_PER_ADDITIONAL_CUSTODY_GROUP. + /// Key is validator index and value is effective_balance. validators: HashMap, /// Maintains the validator custody requirement at a given epoch. /// @@ -52,14 +51,11 @@ impl ValidatorRegistrations { spec: &ChainSpec, ) { let epoch = slot.epoch(E::slots_per_epoch()); - // This is the "weight" of the validator based on the effective balance - let num_validators_for_effective_balance = - effective_balance / spec.balance_per_additional_custody_group; - self.validators - .insert(validator_index, num_validators_for_effective_balance); + self.validators.insert(validator_index, effective_balance); // Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of "weight". - let validator_count_at_epoch = self.validators.values().sum(); + let validator_count_at_epoch = + self.validators.values().sum::() / spec.balance_per_additional_custody_group; let validator_custody_requirement = get_validators_custody_requirement(validator_count_at_epoch, spec); @@ -168,7 +164,7 @@ impl CustodyContext { /// /// Also modifies the internal structures if the validator custody has changed to /// update the `custody_column_count`. - pub fn register_validator( + pub fn register_validators( &self, validators_and_balance: Vec<(usize, u64)>, slot: Slot, @@ -295,12 +291,10 @@ impl From<&CustodyContext> for CustodyContextSsz { CustodyContextSsz { advertised_validator_custody: context .advertised_validator_custody_count - .load(Ordering::Relaxed) - .clone(), + .load(Ordering::Relaxed), validator_custody_at_head: context .validator_custody_count_at_head - .load(Ordering::Relaxed) - .clone(), + .load(Ordering::Relaxed), persisted_is_supernode: context.persisted_is_supernode, } } @@ -308,15 +302,14 @@ impl From<&CustodyContext> for CustodyContextSsz { #[cfg(test)] mod tests { - use crate::MainnetEthSpec; + use types::MainnetEthSpec; use super::*; type E = MainnetEthSpec; #[test] - fn no_validators() { - // supernode without validators + fn no_validators_supernode_default() { let custody_context = CustodyContext::new(true); let spec = E::default_spec(); assert_eq!( @@ -330,8 +323,7 @@ mod tests { } #[test] - fn fullnode() { - // fullnode without validators + fn no_validators_fullnode_default() { let custody_context = CustodyContext::new(false); let spec = E::default_spec(); assert_eq!( @@ -344,90 +336,92 @@ mod tests { spec.custody_requirement, "advertised custody count should be minimum spec custody requirement" ); + } - // add 1 validator - custody_context.register_validator::(vec![(0, 32_000_000_000)], Slot::new(0), &spec); - - assert_eq!( - custody_context.custody_group_count(&spec), - spec.validator_custody_requirement + spec.custody_requirement, - "head custody count should increase with 1 validator" - ); - - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.custody_requirement, - "advertised custody count should not change" - ); - - // add 7 more validators to reach `validator_custody_requirement` - custody_context.register_validator::( - vec![ - (1, 32_000_000_000), - (2, 32_000_000_000), - (3, 32_000_000_000), - (4, 32_000_000_000), - (5, 32_000_000_000), - (6, 32_000_000_000), - (7, 32_000_000_000), - ], - Slot::new(0), - &spec, - ); + #[test] + fn register_single_validator_should_update_cgc() { + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + let bal_per_additional_group = spec.balance_per_additional_custody_group; + let min_val_custody_requirement = spec.validator_custody_requirement; + let validators_and_expected_cgc = vec![ + ( + vec![(0, 1 * bal_per_additional_group)], + min_val_custody_requirement, + ), + ( + vec![(0, 8 * bal_per_additional_group)], + min_val_custody_requirement, + ), + (vec![(0, 10 * bal_per_additional_group)], 10), + ]; + + // Update validator every epoch and assert updates + for (idx, (validators_and_balance, expected_cgc)) in + validators_and_expected_cgc.into_iter().enumerate() + { + let epoch = Epoch::new(idx as u64); + custody_context.register_validators::( + validators_and_balance, + epoch.start_slot(E::slots_per_epoch()), + &spec, + ); - assert_eq!( - custody_context.custody_group_count(&spec), - spec.validator_custody_requirement + spec.custody_requirement, - "head custody count should should be same as with 1 validator" - ); + assert_eq!(custody_context.custody_group_count(&spec), expected_cgc); + } + } - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.custody_requirement, - "advertised custody count should not change" - ); + #[test] + fn register_multiple_validators_should_update_cgc() { + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + let bal_per_additional_group = spec.balance_per_additional_custody_group; + let min_val_custody_requirement = spec.validator_custody_requirement; + let validators_and_expected_cgc = vec![ + ( + vec![(0, 1 * bal_per_additional_group)], + min_val_custody_requirement, + ), + ( + vec![(1, 7 * bal_per_additional_group)], + min_val_custody_requirement, + ), + (vec![(2, 2 * bal_per_additional_group)], 10), + ]; + + // Update validator every epoch and assert updates + for (idx, (validators_and_balance, expected_cgc)) in + validators_and_expected_cgc.into_iter().enumerate() + { + let epoch = Epoch::new(idx as u64); + custody_context.register_validators::( + validators_and_balance, + epoch.start_slot(E::slots_per_epoch()), + &spec, + ); - // adding 1 more validator should increase the custody count - custody_context.register_validator::(vec![(8, 32_000_000_000)], Slot::new(0), &spec); - assert_eq!( - custody_context.custody_group_count(&spec), - spec.validator_custody_requirement + spec.custody_requirement + 1, - "head custody count should increase by 1" - ); + assert_eq!(custody_context.custody_group_count(&spec), expected_cgc); + } + } + #[test] + fn advertised_custody_group_count_should_not_change_after_registration() { + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + custody_context.register_validators::(vec![(0, 1_024_000_000)], Slot::new(0), &spec); assert_eq!( custody_context.advertised_custody_group_count(&spec), spec.custody_requirement, "advertised custody count should not change" ); + } - // update effective balance for some validators. - // validator count should increase by 3 - custody_context.register_validator::( - vec![ - (1, 96_000_000_000), - (2, 65_000_000_000), - (3, 32_000_000_000), - (4, 32_000_000_000), - (5, 32_000_000_000), - (6, 32_000_000_000), - (7, 32_000_000_000), - (8, 32_000_000_000), - ], - Slot::new(0), - &spec, - ); - - assert_eq!( - custody_context.custody_group_count(&spec), - spec.validator_custody_requirement + spec.custody_requirement + 4, - "head custody count should increase by 3" - ); - - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.custody_requirement, - "advertised custody count should not change" - ); + #[test] + #[ignore] + fn advertised_custody_group_count_should_change_after_update_called() { + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + custody_context.register_validators::(vec![(0, 1_024_000_000)], Slot::new(0), &spec); + unimplemented!(); } } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index b38fdb9754a..cabfa1fc2bd 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3801,7 +3801,7 @@ pub fn serve( chain .data_availability_checker .custody_context() - .register_validator::( + .register_validators::( registrations, chain.slot().unwrap(), &chain.spec, diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index a21ad993c6c..1d1fc66d4cb 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -205,7 +205,7 @@ impl Network { let custody_group_count_metadata = ctx .chain_spec .is_peer_das_scheduled() - .then(|| custody_group_count); + .then_some(custody_group_count); let meta_data = utils::load_or_build_metadata(&config.network_dir, custody_group_count_metadata); let seq_number = *meta_data.seq_number(); diff --git a/beacon_node/lighthouse_network/src/types/topics.rs b/beacon_node/lighthouse_network/src/types/topics.rs index 7a959ecaac6..f62212bc530 100644 --- a/beacon_node/lighthouse_network/src/types/topics.rs +++ b/beacon_node/lighthouse_network/src/types/topics.rs @@ -126,7 +126,7 @@ pub fn all_topics_at_fork(fork: ForkName, spec: &ChainSpec) -> Vec(fork, &opts, spec) } From 8a56ae02d604135cdb839fb43e87a930197bd080 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 10 Jun 2025 10:43:19 +0200 Subject: [PATCH 14/27] Remove advertised cgc from `CustodyContext` as we've decided it's not necessary and the node could just advertise immediately to allow the node to serve whatever it has stored. --- .../beacon_chain/src/validator_custody.rs | 78 +++---------------- 1 file changed, 11 insertions(+), 67 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index b5d35e82252..266632bafb8 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -97,15 +97,13 @@ fn get_validators_custody_requirement(count: u64, spec: &ChainSpec) -> u64 { /// number of columns to be custodied when checking for DA. #[derive(Debug)] pub struct CustodyContext { - /// Columns to be custodied based on number of validators + /// The Number of custody groups required based on the number of validators /// that is attached to this node. /// - /// This is the number that we use to compute the cgc value that - /// we advertise to our peers in the metadata and enr values. - advertised_validator_custody_count: AtomicU64, - /// This is the validator custody count that we use to compute the number of columns we need to - /// custody at head (while syncing or when receiving gossip). - validator_custody_count_at_head: AtomicU64, + /// This is the number that we use to compute the custody group count that + /// we require for data availability check, and we use to advertise to our peers in the metadata + /// and enr values. + validator_custody_count: AtomicU64, /// Is the node run as a supernode based on current cli parameters. pub current_is_supernode: bool, /// The persisted value for `is_supernode` based on the previous run of this node. @@ -129,8 +127,7 @@ impl CustodyContext { let (sender, _) = channel(CHANNEL_CAPACITY); Self { - advertised_validator_custody_count: AtomicU64::new(0), - validator_custody_count_at_head: AtomicU64::new(0), + validator_custody_count: AtomicU64::new(0), current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, validator_registrations: Default::default(), @@ -138,17 +135,13 @@ impl CustodyContext { } } - /// Deserialize a `CustodyContext` from SSZ bytes. pub fn new_from_persisted_custody_context( ssz_context: CustodyContextSsz, is_supernode: bool, ) -> Self { let (sender, _) = channel(CHANNEL_CAPACITY); CustodyContext { - advertised_validator_custody_count: AtomicU64::new( - ssz_context.advertised_validator_custody, - ), - validator_custody_count_at_head: AtomicU64::new(ssz_context.validator_custody_at_head), + validator_custody_count: AtomicU64::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_registrations: Default::default(), @@ -183,8 +176,7 @@ impl CustodyContext { }; let current_cgc = self.custody_group_count(spec); - let validator_custody_count_at_head = - self.validator_custody_count_at_head.load(Ordering::Relaxed); + let validator_custody_count_at_head = self.validator_custody_count.load(Ordering::Relaxed); if new_validator_custody != validator_custody_count_at_head { tracing::debug!( @@ -192,7 +184,7 @@ impl CustodyContext { new_count = new_validator_custody, "Validator count at head updated" ); - self.validator_custody_count_at_head + self.validator_custody_count .store(new_validator_custody, Ordering::Relaxed); let updated_cgc = self.custody_group_count(spec); @@ -215,24 +207,6 @@ impl CustodyContext { } } - /// The custody count that we advertise to our peers in our metadata and - /// enr values. - pub fn advertised_custody_group_count(&self, spec: &ChainSpec) -> u64 { - if self.persisted_is_supernode { - return spec.number_of_custody_groups; - } - let advertised_validator_custody = self - .advertised_validator_custody_count - .load(Ordering::Relaxed); - - // If there are no validators, return the minimum custody_requirement - if advertised_validator_custody > 0 { - advertised_validator_custody - } else { - spec.custody_requirement - } - } - /// The custody count that we use to custody columns currently. /// /// This function should be called when figuring out how many columns we @@ -241,8 +215,7 @@ impl CustodyContext { if self.current_is_supernode { return spec.number_of_custody_groups; } - let validator_custody_count_at_head = - self.validator_custody_count_at_head.load(Ordering::Relaxed); + let validator_custody_count_at_head = self.validator_custody_count.load(Ordering::Relaxed); // If there are no validators, return the minimum custody_requirement if validator_custody_count_at_head > 0 { @@ -281,7 +254,6 @@ pub enum CustodyContextMessage { /// The custody information that gets persisted across runs. #[derive(Debug, Encode, Decode, Clone)] pub struct CustodyContextSsz { - advertised_validator_custody: u64, validator_custody_at_head: u64, persisted_is_supernode: bool, } @@ -289,12 +261,7 @@ pub struct CustodyContextSsz { impl From<&CustodyContext> for CustodyContextSsz { fn from(context: &CustodyContext) -> Self { CustodyContextSsz { - advertised_validator_custody: context - .advertised_validator_custody_count - .load(Ordering::Relaxed), - validator_custody_at_head: context - .validator_custody_count_at_head - .load(Ordering::Relaxed), + validator_custody_at_head: context.validator_custody_count.load(Ordering::Relaxed), persisted_is_supernode: context.persisted_is_supernode, } } @@ -316,10 +283,6 @@ mod tests { custody_context.custody_group_count(&spec), spec.number_of_custody_groups ); - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.number_of_custody_groups - ); } #[test] @@ -331,11 +294,6 @@ mod tests { spec.custody_requirement, "head custody count should be minimum spec custody requirement" ); - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.custody_requirement, - "advertised custody count should be minimum spec custody requirement" - ); } #[test] @@ -409,19 +367,5 @@ mod tests { let custody_context = CustodyContext::new(false); let spec = E::default_spec(); custody_context.register_validators::(vec![(0, 1_024_000_000)], Slot::new(0), &spec); - assert_eq!( - custody_context.advertised_custody_group_count(&spec), - spec.custody_requirement, - "advertised custody count should not change" - ); - } - - #[test] - #[ignore] - fn advertised_custody_group_count_should_change_after_update_called() { - let custody_context = CustodyContext::new(false); - let spec = E::default_spec(); - custody_context.register_validators::(vec![(0, 1_024_000_000)], Slot::new(0), &spec); - unimplemented!(); } } From 4bde6c661c039800a0f92859b52419661d6d6208 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 10 Jun 2025 10:52:54 +0200 Subject: [PATCH 15/27] Update validator custody unit tests. --- .../beacon_chain/src/validator_custody.rs | 79 +++++++++++++------ 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 266632bafb8..d8f38ec9506 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -302,6 +302,7 @@ mod tests { let spec = E::default_spec(); let bal_per_additional_group = spec.balance_per_additional_custody_group; let min_val_custody_requirement = spec.validator_custody_requirement; + // One single node increases its balance over 3 epochs. let validators_and_expected_cgc = vec![ ( vec![(0, 1 * bal_per_additional_group)], @@ -314,19 +315,7 @@ mod tests { (vec![(0, 10 * bal_per_additional_group)], 10), ]; - // Update validator every epoch and assert updates - for (idx, (validators_and_balance, expected_cgc)) in - validators_and_expected_cgc.into_iter().enumerate() - { - let epoch = Epoch::new(idx as u64); - custody_context.register_validators::( - validators_and_balance, - epoch.start_slot(E::slots_per_epoch()), - &spec, - ); - - assert_eq!(custody_context.custody_group_count(&spec), expected_cgc); - } + register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); } #[test] @@ -335,19 +324,70 @@ mod tests { let spec = E::default_spec(); let bal_per_additional_group = spec.balance_per_additional_custody_group; let min_val_custody_requirement = spec.validator_custody_requirement; + // Add 3 validators over 3 epochs. let validators_and_expected_cgc = vec![ ( vec![(0, 1 * bal_per_additional_group)], min_val_custody_requirement, ), ( - vec![(1, 7 * bal_per_additional_group)], + vec![ + (0, 1 * bal_per_additional_group), + (1, 7 * bal_per_additional_group), + ], min_val_custody_requirement, ), - (vec![(2, 2 * bal_per_additional_group)], 10), + ( + vec![ + (0, 1 * bal_per_additional_group), + (1, 7 * bal_per_additional_group), + (2, 2 * bal_per_additional_group), + ], + 10, + ), + ]; + + register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); + } + + #[test] + fn register_validators_should_not_update_cgc_for_supernode() { + let custody_context = CustodyContext::new(true); + let spec = E::default_spec(); + let bal_per_additional_group = spec.balance_per_additional_custody_group; + + // Add 3 validators over 3 epochs. + let validators_and_expected_cgc = vec![ + ( + vec![(0, 1 * bal_per_additional_group)], + spec.number_of_custody_groups, + ), + ( + vec![ + (0, 1 * bal_per_additional_group), + (1, 7 * bal_per_additional_group), + ], + spec.number_of_custody_groups, + ), + ( + vec![ + (0, 1 * bal_per_additional_group), + (1, 7 * bal_per_additional_group), + (2, 2 * bal_per_additional_group), + ], + spec.number_of_custody_groups, + ), ]; - // Update validator every epoch and assert updates + register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); + } + + /// Update validator every epoch and assert cgc against expected values. + fn register_validators_and_assert_cgc( + custody_context: CustodyContext, + validators_and_expected_cgc: Vec<(Vec<(usize, u64)>, u64)>, + spec: &ChainSpec, + ) { for (idx, (validators_and_balance, expected_cgc)) in validators_and_expected_cgc.into_iter().enumerate() { @@ -361,11 +401,4 @@ mod tests { assert_eq!(custody_context.custody_group_count(&spec), expected_cgc); } } - - #[test] - fn advertised_custody_group_count_should_not_change_after_registration() { - let custody_context = CustodyContext::new(false); - let spec = E::default_spec(); - custody_context.register_validators::(vec![(0, 1_024_000_000)], Slot::new(0), &spec); - } } From 8b26088fa8c80a5b93de36e2cb505d086f0fd5ec Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 10 Jun 2025 11:33:21 +0200 Subject: [PATCH 16/27] Avoid passing custody_count all around verification --- beacon_node/beacon_chain/src/beacon_chain.rs | 3 +- .../beacon_chain/src/block_verification.rs | 24 +++------ .../src/block_verification_types.rs | 16 ------ .../src/data_availability_checker.rs | 34 ++++-------- .../overflow_lru_cache.rs | 53 +++++++++++-------- .../state_lru_cache.rs | 8 --- beacon_node/beacon_chain/src/test_utils.rs | 22 ++------ .../beacon_chain/src/validator_custody.rs | 1 - .../tests/payload_invalidation.rs | 11 ++-- beacon_node/beacon_chain/tests/store_tests.rs | 2 - beacon_node/http_api/src/publish_blocks.rs | 31 +++-------- .../gossip_methods.rs | 8 +-- .../src/network_beacon_processor/tests.rs | 7 +-- .../src/sync/block_sidecar_coupling.rs | 12 ++--- .../network/src/sync/network_context.rs | 9 +--- beacon_node/network/src/sync/tests/range.rs | 3 +- testing/ef_tests/src/cases/fork_choice.rs | 2 +- 17 files changed, 73 insertions(+), 173 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 00f4b7ac585..c13502c59f2 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3018,7 +3018,6 @@ impl BeaconChain { pub async fn verify_block_for_gossip( self: &Arc, block: Arc>, - custody_columns_count: usize, ) -> Result, BlockError> { let chain = self.clone(); self.task_executor @@ -3028,7 +3027,7 @@ impl BeaconChain { let slot = block.slot(); let graffiti_string = block.message().body().graffiti().as_utf8_lossy(); - match GossipVerifiedBlock::new(block, &chain, custody_columns_count) { + match GossipVerifiedBlock::new(block, &chain) { Ok(verified) => { let commitments_formatted = verified.block.commitments_formatted(); debug!( diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 26bf8723929..4613b7c55ad 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -685,7 +685,6 @@ pub struct GossipVerifiedBlock { pub block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, - custody_columns_count: usize, } /// A wrapper around a `SignedBeaconBlock` that indicates that all signatures (except the deposit @@ -721,7 +720,6 @@ pub trait IntoGossipVerifiedBlock: Sized { fn into_gossip_verified_block( self, chain: &BeaconChain, - custody_columns_count: usize, ) -> Result, BlockError>; fn inner_block(&self) -> Arc>; } @@ -730,7 +728,6 @@ impl IntoGossipVerifiedBlock for GossipVerifiedBlock fn into_gossip_verified_block( self, _chain: &BeaconChain, - _custody_columns_count: usize, ) -> Result, BlockError> { Ok(self) } @@ -743,9 +740,8 @@ impl IntoGossipVerifiedBlock for Arc, - custody_columns_count: usize, ) -> Result, BlockError> { - GossipVerifiedBlock::new(self, chain, custody_columns_count) + GossipVerifiedBlock::new(self, chain) } fn inner_block(&self) -> Arc> { @@ -821,7 +817,6 @@ impl GossipVerifiedBlock { pub fn new( block: Arc>, chain: &BeaconChain, - custody_columns_count: usize, ) -> Result { // If the block is valid for gossip we don't supply it to the slasher here because // we assume it will be transformed into a fully verified block. We *do* need to supply @@ -831,14 +826,12 @@ impl GossipVerifiedBlock { // The `SignedBeaconBlock` and `SignedBeaconBlockHeader` have the same canonical root, // but it's way quicker to calculate root of the header since the hash of the tree rooted // at `BeaconBlockBody` is already computed in the header. - Self::new_without_slasher_checks(block, &header, chain, custody_columns_count).map_err( - |e| { - process_block_slash_info::<_, BlockError>( - chain, - BlockSlashInfo::from_early_error_block(header, e), - ) - }, - ) + Self::new_without_slasher_checks(block, &header, chain).map_err(|e| { + process_block_slash_info::<_, BlockError>( + chain, + BlockSlashInfo::from_early_error_block(header, e), + ) + }) } /// As for new, but doesn't pass the block to the slasher. @@ -846,7 +839,6 @@ impl GossipVerifiedBlock { block: Arc>, block_header: &SignedBeaconBlockHeader, chain: &BeaconChain, - custody_columns_count: usize, ) -> Result { // Ensure the block is the correct structure for the fork at `block.slot()`. block @@ -1053,7 +1045,6 @@ impl GossipVerifiedBlock { block_root, parent, consensus_context, - custody_columns_count, }) } @@ -1201,7 +1192,6 @@ impl SignatureVerifiedBlock { block: MaybeAvailableBlock::AvailabilityPending { block_root: from.block_root, block, - custody_columns_count: from.custody_columns_count, }, block_root: from.block_root, parent: Some(parent), diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index dab54dc823e..f7002dcee1c 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -31,7 +31,6 @@ use types::{ pub struct RpcBlock { block_root: Hash256, block: RpcBlockInner, - custody_columns_count: usize, } impl Debug for RpcBlock { @@ -45,10 +44,6 @@ impl RpcBlock { self.block_root } - pub fn custody_columns_count(&self) -> usize { - self.custody_columns_count - } - pub fn as_block(&self) -> &SignedBeaconBlock { match &self.block { RpcBlockInner::Block(block) => block, @@ -103,14 +98,12 @@ impl RpcBlock { pub fn new_without_blobs( block_root: Option, block: Arc>, - custody_columns_count: usize, ) -> Self { let block_root = block_root.unwrap_or_else(|| get_block_root(&block)); Self { block_root, block: RpcBlockInner::Block(block), - custody_columns_count, } } @@ -152,8 +145,6 @@ impl RpcBlock { Ok(Self { block_root, block: inner, - // Block is before PeerDAS - custody_columns_count: 0, }) } @@ -161,7 +152,6 @@ impl RpcBlock { block_root: Option, block: Arc>, custody_columns: Vec>, - custody_columns_count: usize, spec: &ChainSpec, ) -> Result { let block_root = block_root.unwrap_or_else(|| get_block_root(&block)); @@ -182,7 +172,6 @@ impl RpcBlock { Ok(Self { block_root, block: inner, - custody_columns_count, }) } @@ -250,12 +239,10 @@ impl ExecutedBlock { MaybeAvailableBlock::AvailabilityPending { block_root: _, block: pending_block, - custody_columns_count, } => Self::AvailabilityPending(AvailabilityPendingExecutedBlock::new( pending_block, import_data, payload_verification_outcome, - custody_columns_count, )), } } @@ -321,7 +308,6 @@ pub struct AvailabilityPendingExecutedBlock { pub block: Arc>, pub import_data: BlockImportData, pub payload_verification_outcome: PayloadVerificationOutcome, - pub custody_columns_count: usize, } impl AvailabilityPendingExecutedBlock { @@ -329,13 +315,11 @@ impl AvailabilityPendingExecutedBlock { block: Arc>, import_data: BlockImportData, payload_verification_outcome: PayloadVerificationOutcome, - custody_columns_count: usize, ) -> Self { Self { block, import_data, payload_verification_outcome, - custody_columns_count, } } diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 2a1a59eb233..91ff5fb644c 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -115,7 +115,12 @@ impl DataAvailabilityChecker { custody_context: Arc, spec: Arc, ) -> Result { - let inner = DataAvailabilityCheckerInner::new(OVERFLOW_LRU_CAPACITY, store, spec.clone())?; + let inner = DataAvailabilityCheckerInner::new( + OVERFLOW_LRU_CAPACITY, + store, + custody_context.clone(), + spec.clone(), + )?; Ok(Self { availability_cache: Arc::new(inner), slot_clock, @@ -304,7 +309,6 @@ impl DataAvailabilityChecker { &self, block: RpcBlock, ) -> Result, AvailabilityCheckError> { - let custody_columns_count = block.custody_columns_count(); let (block_root, block, blobs, data_columns) = block.deconstruct(); if self.blobs_required_for_block(&block) { return if let Some(blob_list) = blobs { @@ -318,11 +322,7 @@ impl DataAvailabilityChecker { spec: self.spec.clone(), })) } else { - Ok(MaybeAvailableBlock::AvailabilityPending { - block_root, - block, - custody_columns_count, - }) + Ok(MaybeAvailableBlock::AvailabilityPending { block_root, block }) }; } if self.data_columns_required_for_block(&block) { @@ -347,11 +347,7 @@ impl DataAvailabilityChecker { spec: self.spec.clone(), })) } else { - Ok(MaybeAvailableBlock::AvailabilityPending { - block_root, - block, - custody_columns_count, - }) + Ok(MaybeAvailableBlock::AvailabilityPending { block_root, block }) }; } @@ -408,7 +404,6 @@ impl DataAvailabilityChecker { } for block in blocks { - let custody_columns_count = block.custody_columns_count(); let (block_root, block, blobs, data_columns) = block.deconstruct(); let maybe_available_block = if self.blobs_required_for_block(&block) { @@ -421,11 +416,7 @@ impl DataAvailabilityChecker { spec: self.spec.clone(), }) } else { - MaybeAvailableBlock::AvailabilityPending { - block_root, - block, - custody_columns_count, - } + MaybeAvailableBlock::AvailabilityPending { block_root, block } } } else if self.data_columns_required_for_block(&block) { if let Some(data_columns) = data_columns { @@ -439,11 +430,7 @@ impl DataAvailabilityChecker { spec: self.spec.clone(), }) } else { - MaybeAvailableBlock::AvailabilityPending { - block_root, - block, - custody_columns_count, - } + MaybeAvailableBlock::AvailabilityPending { block_root, block } } } else { MaybeAvailableBlock::Available(AvailableBlock { @@ -793,7 +780,6 @@ pub enum MaybeAvailableBlock { AvailabilityPending { block_root: Hash256, block: Arc>, - custody_columns_count: usize, }, } diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index 3478c183f34..589ea8a74bf 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -8,6 +8,7 @@ use crate::block_verification_types::{ use crate::data_availability_checker::{Availability, AvailabilityCheckError}; use crate::data_column_verification::KzgVerifiedCustodyDataColumn; use crate::BeaconChainTypes; +use crate::CustodyContext; use lru::LruCache; use parking_lot::RwLock; use std::cmp::Ordering; @@ -158,6 +159,7 @@ impl PendingComponents { pub fn make_available( &mut self, spec: &Arc, + num_expected_columns: u64, recover: R, ) -> Result>, AvailabilityCheckError> where @@ -171,12 +173,11 @@ impl PendingComponents { }; let num_expected_blobs = block.num_blobs_expected(); - + let num_expected_columns = num_expected_columns as usize; let blob_data = if num_expected_blobs == 0 { Some(AvailableBlockData::NoData) } else if spec.is_peer_das_enabled_for_epoch(block.epoch()) { let num_received_columns = self.verified_data_columns.len(); - let num_expected_columns = block.custody_columns_count(); match num_received_columns.cmp(&num_expected_columns) { Ordering::Greater => { // Should never happen @@ -254,7 +255,6 @@ impl PendingComponents { block, import_data, payload_verification_outcome, - custody_columns_count: _, } = recover(block.clone())?; let available_block = AvailableBlock { @@ -311,16 +311,10 @@ impl PendingComponents { pub fn status_str(&self, block_epoch: Epoch, spec: &ChainSpec) -> String { let block_count = if self.executed_block.is_some() { 1 } else { 0 }; if spec.is_peer_das_enabled_for_epoch(block_epoch) { - let custody_columns_count = if let Some(block) = self.get_cached_block() { - &block.custody_columns_count().to_string() - } else { - "?" - }; format!( - "block {} data_columns {}/{}", + "block {} data_columns {}", block_count, self.verified_data_columns.len(), - custody_columns_count, ) } else { let num_expected_blobs = if let Some(block) = self.get_cached_block() { @@ -346,6 +340,7 @@ pub struct DataAvailabilityCheckerInner { /// This cache holds a limited number of states in memory and reconstructs them /// from disk when necessary. This is necessary until we merge tree-states state_cache: StateLRUCache, + custody_context: Arc, spec: Arc, } @@ -362,11 +357,13 @@ impl DataAvailabilityCheckerInner { pub fn new( capacity: NonZeroUsize, beacon_store: BeaconStore, + custody_context: Arc, spec: Arc, ) -> Result { Ok(Self { critical: RwLock::new(LruCache::new(capacity)), state_cache: StateLRUCache::new(beacon_store, spec.clone()), + custody_context, spec, }) } @@ -474,9 +471,11 @@ impl DataAvailabilityCheckerInner { "Component added to data availability checker" ); - if let Some(available_block) = pending_components.make_available(&self.spec, |block| { - self.state_cache.recover_pending_executed_block(block) - })? { + if let Some(available_block) = pending_components.make_available( + &self.spec, + self.custody_context.sampling_count(&self.spec), + |block| self.state_cache.recover_pending_executed_block(block), + )? { // We keep the pending components in the availability cache during block import (#5845). write_lock.put(block_root, pending_components); drop(write_lock); @@ -526,9 +525,11 @@ impl DataAvailabilityCheckerInner { "Component added to data availability checker" ); - if let Some(available_block) = pending_components.make_available(&self.spec, |block| { - self.state_cache.recover_pending_executed_block(block) - })? { + if let Some(available_block) = pending_components.make_available( + &self.spec, + self.custody_context.sampling_count(&self.spec), + |block| self.state_cache.recover_pending_executed_block(block), + )? { // We keep the pending components in the availability cache during block import (#5845). write_lock.put(block_root, pending_components); drop(write_lock); @@ -620,9 +621,11 @@ impl DataAvailabilityCheckerInner { ); // Check if we have all components and entire set is consistent. - if let Some(available_block) = pending_components.make_available(&self.spec, |block| { - self.state_cache.recover_pending_executed_block(block) - })? { + if let Some(available_block) = pending_components.make_available( + &self.spec, + self.custody_context.sampling_count(&self.spec), + |block| self.state_cache.recover_pending_executed_block(block), + )? { // We keep the pending components in the availability cache during block import (#5845). write_lock.put(block_root, pending_components); drop(write_lock); @@ -861,7 +864,6 @@ mod test { block, import_data, payload_verification_outcome, - custody_columns_count: DEFAULT_TEST_CUSTODY_COLUMN_COUNT, }; (availability_pending_block, gossip_verified_blobs) @@ -888,9 +890,15 @@ mod test { let spec = harness.spec.clone(); let test_store = harness.chain.store.clone(); let capacity_non_zero = new_non_zero_usize(capacity); + let custody_context = Arc::new(CustodyContext::new(false)); let cache = Arc::new( - DataAvailabilityCheckerInner::::new(capacity_non_zero, test_store, spec.clone()) - .expect("should create cache"), + DataAvailabilityCheckerInner::::new( + capacity_non_zero, + test_store, + custody_context, + spec.clone(), + ) + .expect("should create cache"), ); (harness, cache, chain_db_path) } @@ -1240,7 +1248,6 @@ mod pending_components_tests { is_valid_merge_transition_block: false, }, // Default custody columns count, doesn't matter here - custody_columns_count: 8, }; (block.into(), blobs, invalid_blobs) } diff --git a/beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs index 5fe674f30c1..f73857f4682 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/state_lru_cache.rs @@ -24,7 +24,6 @@ pub struct DietAvailabilityPendingExecutedBlock { parent_eth1_finalization_data: Eth1FinalizationData, consensus_context: OnDiskConsensusContext, payload_verification_outcome: PayloadVerificationOutcome, - custody_columns_count: usize, } /// just implementing the same methods as `AvailabilityPendingExecutedBlock` @@ -54,10 +53,6 @@ impl DietAvailabilityPendingExecutedBlock { .unwrap_or_default() } - pub fn custody_columns_count(&self) -> usize { - self.custody_columns_count - } - /// Returns the epoch corresponding to `self.slot()`. pub fn epoch(&self) -> Epoch { self.block.slot().epoch(E::slots_per_epoch()) @@ -107,7 +102,6 @@ impl StateLRUCache { executed_block.import_data.consensus_context, ), payload_verification_outcome: executed_block.payload_verification_outcome, - custody_columns_count: executed_block.custody_columns_count, } } @@ -137,7 +131,6 @@ impl StateLRUCache { .into_consensus_context(), }, payload_verification_outcome: diet_executed_block.payload_verification_outcome, - custody_columns_count: diet_executed_block.custody_columns_count, }) } @@ -224,7 +217,6 @@ impl From> value.import_data.consensus_context, ), payload_verification_outcome: value.payload_verification_outcome, - custody_columns_count: value.custody_columns_count, } } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 3ee8c7ae3f9..e4caea384c7 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -2360,7 +2360,7 @@ where .blob_kzg_commitments() .is_ok_and(|c| !c.is_empty()); if !has_blobs { - return RpcBlock::new_without_blobs(Some(block_root), block, 0); + return RpcBlock::new_without_blobs(Some(block_root), block); } // Blobs are stored as data columns from Fulu (PeerDAS) @@ -2370,14 +2370,8 @@ where .into_iter() .map(CustodyDataColumn::from_asserted_custody) .collect::>(); - RpcBlock::new_with_custody_columns( - Some(block_root), - block, - custody_columns, - self.get_sampling_column_count(), - &self.spec, - ) - .unwrap() + RpcBlock::new_with_custody_columns(Some(block_root), block, custody_columns, &self.spec) + .unwrap() } else { let blobs = self.chain.get_blobs(&block_root).unwrap().blobs(); RpcBlock::new(Some(block_root), block, blobs).unwrap() @@ -2403,15 +2397,9 @@ where .take(sampling_column_count) .map(CustodyDataColumn::from_asserted_custody) .collect::>(); - RpcBlock::new_with_custody_columns( - Some(block_root), - block, - columns, - sampling_column_count, - &self.spec, - )? + RpcBlock::new_with_custody_columns(Some(block_root), block, columns, &self.spec)? } else { - RpcBlock::new_without_blobs(Some(block_root), block, 0) + RpcBlock::new_without_blobs(Some(block_root), block) } } else { let blobs = blob_items diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index d8f38ec9506..66b9ea28708 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -163,7 +163,6 @@ impl CustodyContext { slot: Slot, spec: &ChainSpec, ) { - // Complete all registrations let mut registrations = self.validator_registrations.write(); for (validator_index, effective_balance) in validators_and_balance { registrations.register_validator::(validator_index, effective_balance, slot, spec); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 6b9ff9d6edc..e686daa1d88 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -686,8 +686,7 @@ async fn invalidates_all_descendants() { assert_eq!(fork_parent_state.slot(), fork_parent_slot); let ((fork_block, _), _fork_post_state) = rig.harness.make_block(fork_parent_state, fork_slot).await; - let fork_rpc_block = - RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count); + let fork_rpc_block = RpcBlock::new_without_blobs(None, fork_block.clone()); let fork_block_root = rig .harness .chain @@ -789,8 +788,7 @@ async fn switches_heads() { let ((fork_block, _), _fork_post_state) = rig.harness.make_block(fork_parent_state, fork_slot).await; let fork_parent_root = fork_block.parent_root(); - let fork_rpc_block = - RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count); + let fork_rpc_block = RpcBlock::new_without_blobs(None, fork_block.clone()); let fork_block_root = rig .harness .chain @@ -1060,8 +1058,7 @@ async fn invalid_parent() { )); // Ensure the block built atop an invalid payload is invalid for import. - let rpc_block = - RpcBlock::new_without_blobs(None, block.clone(), rig.harness.sampling_column_count); + let rpc_block = RpcBlock::new_without_blobs(None, block.clone()); assert!(matches!( rig.harness.chain.process_block(rpc_block.block_root(), rpc_block, NotifyExecutionLayer::Yes, BlockImportSource::Lookup, || Ok(()), @@ -1386,7 +1383,7 @@ async fn recover_from_invalid_head_by_importing_blocks() { // Import the fork block, it should become the head. let fork_rpc_block = - RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count); + RpcBlock::new_without_blobs(None, fork_block.clone()); rig.harness .chain .process_block( diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 51c7f0c289e..c472a10e60a 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -2647,7 +2647,6 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() { let invalid_fork_rpc_block = RpcBlock::new_without_blobs( None, invalid_fork_block.clone(), - harness.sampling_column_count, ); // Applying the invalid block should fail. let err = harness @@ -2667,7 +2666,6 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() { let valid_fork_rpc_block = RpcBlock::new_without_blobs( None, valid_fork_block.clone(), - harness.sampling_column_count, ); harness .chain diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index c2c6459828e..75979bbb1d7 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -138,13 +138,7 @@ pub async fn publish_block>( spawn_build_data_sidecar_task(chain.clone(), block.clone(), unverified_blobs)?; // Gossip verify the block and blobs/data columns separately. - let gossip_verified_block_result = unverified_block.into_gossip_verified_block( - &chain, - chain - .data_availability_checker - .custody_context() - .sampling_count(&chain.spec) as usize, - ); + let gossip_verified_block_result = unverified_block.into_gossip_verified_block(&chain); let block_root = block_root.unwrap_or_else(|| { gossip_verified_block_result.as_ref().map_or_else( |_| block.canonical_root(), @@ -306,22 +300,13 @@ pub async fn publish_block>( slot = %block.slot(), "Block previously seen" ); - let import_result = Box::pin( - chain.process_block( - block_root, - RpcBlock::new_without_blobs( - Some(block_root), - block.clone(), - chain - .data_availability_checker - .custody_context() - .sampling_count(&chain.spec) as usize, - ), - NotifyExecutionLayer::Yes, - BlockImportSource::HttpApi, - publish_fn, - ), - ) + let import_result = Box::pin(chain.process_block( + block_root, + RpcBlock::new_without_blobs(Some(block_root), block.clone()), + NotifyExecutionLayer::Yes, + BlockImportSource::HttpApi, + publish_fn, + )) .await; post_block_import_logging_and_response( import_result, diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index b65b996d092..87f657f9352 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1272,13 +1272,7 @@ impl NetworkBeaconProcessor { let verification_result = self .chain .clone() - .verify_block_for_gossip( - block.clone(), - self.chain - .data_availability_checker - .custody_context() - .sampling_count(&self.chain.spec) as usize, - ) + .verify_block_for_gossip(block.clone()) .await; if verification_result.is_ok() { diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index bdae87bd0bd..6456b5718ff 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -382,11 +382,7 @@ impl TestRig { self.network_beacon_processor .send_rpc_beacon_block( block_root, - RpcBlock::new_without_blobs( - Some(block_root), - self.next_block.clone(), - self.custody_columns_count(), - ), + RpcBlock::new_without_blobs(Some(block_root), self.next_block.clone()), std::time::Duration::default(), BlockProcessType::SingleBlock { id: 0 }, ) @@ -401,7 +397,6 @@ impl TestRig { RpcBlock::new_without_blobs( Some(block_root), self.next_block.clone(), - self.custody_columns_count(), ), std::time::Duration::default(), BlockProcessType::SingleBlock { id: 1 }, diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 99428b0c805..0418ab45534 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -257,17 +257,11 @@ impl RangeBlockComponentsRequest { )); } - RpcBlock::new_with_custody_columns( - Some(block_root), - block, - custody_columns, - expects_custody_columns.len(), - spec, - ) - .map_err(|e| format!("{e:?}"))? + RpcBlock::new_with_custody_columns(Some(block_root), block, custody_columns, spec) + .map_err(|e| format!("{e:?}"))? } else { // Block has no data, expects zero columns - RpcBlock::new_without_blobs(Some(block_root), block, 0) + RpcBlock::new_without_blobs(Some(block_root), block) }); } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 9f20174b641..f6be39fa4a8 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -1486,14 +1486,7 @@ impl SyncNetworkContext { .beacon_processor_if_enabled() .ok_or(SendErrorProcessor::ProcessorNotAvailable)?; - let block = RpcBlock::new_without_blobs( - Some(block_root), - block, - self.chain - .data_availability_checker - .custody_context() - .sampling_count(&self.chain.spec) as usize, - ); + let block = RpcBlock::new_without_blobs(Some(block_root), block); debug!(block = ?block_root, id, "Sending block for processing"); // Lookup sync event safety: If `beacon_processor.send_rpc_beacon_block` returns Ok() sync diff --git a/beacon_node/network/src/sync/tests/range.rs b/beacon_node/network/src/sync/tests/range.rs index 932f485dd0d..98c50088366 100644 --- a/beacon_node/network/src/sync/tests/range.rs +++ b/beacon_node/network/src/sync/tests/range.rs @@ -454,13 +454,12 @@ fn build_rpc_block( block, columns.clone(), // TODO(das): Assumes CGC = max value. Change if we want to do more complex tests - columns.len(), spec, ) .unwrap() } // Block has no data, expects zero columns - None => RpcBlock::new_without_blobs(None, block, 0), + None => RpcBlock::new_without_blobs(None, block), } } diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index b507383190f..af3b0bce2de 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -520,7 +520,7 @@ impl Tester { let result: Result, _> = self .block_on_dangerous(self.harness.chain.process_block( block_root, - RpcBlock::new_without_blobs(Some(block_root), block.clone(), 0), + RpcBlock::new_without_blobs(Some(block_root), block.clone()), NotifyExecutionLayer::Yes, BlockImportSource::Lookup, || Ok(()), From ec76b5675ab7ec5ef372f2feb0ef874c9c12ad1a Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 10 Jun 2025 14:55:17 +0200 Subject: [PATCH 17/27] Use finalized state for validator custody calculation. Fix build and lint issues. --- .../overflow_lru_cache.rs | 1 - .../beacon_chain/src/validator_custody.rs | 18 +++++++++--------- .../beacon_chain/tests/payload_invalidation.rs | 3 +-- beacon_node/beacon_chain/tests/store_tests.rs | 10 ++-------- beacon_node/http_api/src/lib.rs | 12 +++++------- .../lighthouse_network/src/discovery/mod.rs | 1 + .../lighthouse_network/src/types/globals.rs | 4 ++-- .../lighthouse_network/src/types/topics.rs | 2 +- beacon_node/lighthouse_network/tests/common.rs | 3 ++- .../src/network_beacon_processor/tests.rs | 5 +---- 10 files changed, 24 insertions(+), 35 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index 589ea8a74bf..d26381d3636 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -703,7 +703,6 @@ mod test { use types::{ExecPayload, MinimalEthSpec}; const LOW_VALIDATOR_COUNT: usize = 32; - const DEFAULT_TEST_CUSTODY_COLUMN_COUNT: usize = 8; fn get_store_with_spec( db_path: &TempDir, diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 66b9ea28708..b4b4ebc96e7 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -304,7 +304,7 @@ mod tests { // One single node increases its balance over 3 epochs. let validators_and_expected_cgc = vec![ ( - vec![(0, 1 * bal_per_additional_group)], + vec![(0, bal_per_additional_group)], min_val_custody_requirement, ), ( @@ -326,19 +326,19 @@ mod tests { // Add 3 validators over 3 epochs. let validators_and_expected_cgc = vec![ ( - vec![(0, 1 * bal_per_additional_group)], + vec![(0, bal_per_additional_group)], min_val_custody_requirement, ), ( vec![ - (0, 1 * bal_per_additional_group), + (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), ], min_val_custody_requirement, ), ( vec![ - (0, 1 * bal_per_additional_group), + (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), (2, 2 * bal_per_additional_group), ], @@ -358,19 +358,19 @@ mod tests { // Add 3 validators over 3 epochs. let validators_and_expected_cgc = vec![ ( - vec![(0, 1 * bal_per_additional_group)], + vec![(0, bal_per_additional_group)], spec.number_of_custody_groups, ), ( vec![ - (0, 1 * bal_per_additional_group), + (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), ], spec.number_of_custody_groups, ), ( vec![ - (0, 1 * bal_per_additional_group), + (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), (2, 2 * bal_per_additional_group), ], @@ -394,10 +394,10 @@ mod tests { custody_context.register_validators::( validators_and_balance, epoch.start_slot(E::slots_per_epoch()), - &spec, + spec, ); - assert_eq!(custody_context.custody_group_count(&spec), expected_cgc); + assert_eq!(custody_context.custody_group_count(spec), expected_cgc); } } } diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index e686daa1d88..f9f39598d4d 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1382,8 +1382,7 @@ async fn recover_from_invalid_head_by_importing_blocks() { } = InvalidHeadSetup::new().await; // Import the fork block, it should become the head. - let fork_rpc_block = - RpcBlock::new_without_blobs(None, fork_block.clone()); + let fork_rpc_block = RpcBlock::new_without_blobs(None, fork_block.clone()); rig.harness .chain .process_block( diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index c472a10e60a..d0f161ed569 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -2644,10 +2644,7 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() { assert_eq!(split.block_root, valid_fork_block.parent_root()); assert_ne!(split.state_root, unadvanced_split_state_root); - let invalid_fork_rpc_block = RpcBlock::new_without_blobs( - None, - invalid_fork_block.clone(), - ); + let invalid_fork_rpc_block = RpcBlock::new_without_blobs(None, invalid_fork_block.clone()); // Applying the invalid block should fail. let err = harness .chain @@ -2663,10 +2660,7 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() { assert!(matches!(err, BlockError::WouldRevertFinalizedSlot { .. })); // Applying the valid block should succeed, but it should not become head. - let valid_fork_rpc_block = RpcBlock::new_without_blobs( - None, - valid_fork_block.clone(), - ); + let valid_fork_rpc_block = RpcBlock::new_without_blobs(None, valid_fork_block.clone()); harness .chain .process_block( diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index cabfa1fc2bd..7e8e77cbdad 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -48,8 +48,8 @@ use directory::DEFAULT_ROOT_DIR; use either::Either; use eth2::types::{ self as api_types, BroadcastValidation, ContextDeserialize, EndpointVersion, ForkChoice, - ForkChoiceNode, LightClientUpdatesQuery, PublishBlockRequest, ValidatorBalancesRequestBody, - ValidatorId, ValidatorStatus, ValidatorsRequestBody, + ForkChoiceNode, LightClientUpdatesQuery, PublishBlockRequest, StateId as CoreStateId, + ValidatorBalancesRequestBody, ValidatorId, ValidatorStatus, ValidatorsRequestBody, }; use eth2::{CONSENSUS_VERSION_HEADER, CONTENT_TYPE_HEADER, SSZ_CONTENT_TYPE_HEADER}; use health_metrics::observe::Observe; @@ -3775,17 +3775,15 @@ pub fn serve( task_spawner.blocking_json_task(Priority::P0, move || { let mut subscriptions: std::collections::BTreeSet<_> = Default::default(); let mut registrations = Vec::new(); + let (finalized_beacon_state, _, _) = + StateId(CoreStateId::Finalized).state(&chain)?; for subscription in committee_subscriptions.iter() { chain .validator_monitor .write() .auto_register_local_validator(subscription.validator_index); // Register the validator with the `CustodyContext` - if let Ok(effective_balance) = chain - .canonical_head - .cached_head() - .snapshot - .beacon_state + if let Ok(effective_balance) = finalized_beacon_state .get_effective_balance(subscription.validator_index as usize) { registrations diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index ad54c6b8b1f..d6845421f31 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -1218,6 +1218,7 @@ mod tests { vec![], false, config.clone(), + spec.custody_requirement, spec.clone(), ); let keypair = keypair.into(); diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index 9f7ce72f3c0..3636fae873c 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -304,7 +304,7 @@ mod test { Arc::new(spec), ); assert_eq!( - globals.sampling_subnets.len(), + globals.sampling_subnets.read().len(), subnet_sampling_size as usize ); } @@ -327,7 +327,7 @@ mod test { Arc::new(spec), ); assert_eq!( - globals.sampling_columns.len(), + globals.sampling_columns.read().len(), subnet_sampling_size as usize ); } diff --git a/beacon_node/lighthouse_network/src/types/topics.rs b/beacon_node/lighthouse_network/src/types/topics.rs index f62212bc530..349bfe66a3d 100644 --- a/beacon_node/lighthouse_network/src/types/topics.rs +++ b/beacon_node/lighthouse_network/src/types/topics.rs @@ -521,7 +521,7 @@ mod tests { enable_light_client_server: false, subscribe_all_subnets: false, subscribe_all_data_column_subnets: false, - sampling_subnets, + sampling_subnets: sampling_subnets.clone(), } } diff --git a/beacon_node/lighthouse_network/tests/common.rs b/beacon_node/lighthouse_network/tests/common.rs index d979ef9265a..0dac126909c 100644 --- a/beacon_node/lighthouse_network/tests/common.rs +++ b/beacon_node/lighthouse_network/tests/common.rs @@ -118,6 +118,7 @@ pub async fn build_libp2p_instance( let (signal, exit) = async_channel::bounded(1); let (shutdown_tx, _) = futures::channel::mpsc::channel(1); let executor = task_executor::TaskExecutor::new(rt, exit, shutdown_tx, service_name); + let custody_group_count = chain_spec.custody_requirement; let libp2p_context = lighthouse_network::Context { config, enr_fork_id: EnrForkId::default(), @@ -126,7 +127,7 @@ pub async fn build_libp2p_instance( libp2p_registry: None, }; Libp2pInstance( - LibP2PService::new(executor, libp2p_context) + LibP2PService::new(executor, libp2p_context, custody_group_count) .await .expect("should build libp2p instance") .0, diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 6456b5718ff..0132ec76d18 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -394,10 +394,7 @@ impl TestRig { self.network_beacon_processor .send_rpc_beacon_block( block_root, - RpcBlock::new_without_blobs( - Some(block_root), - self.next_block.clone(), - ), + RpcBlock::new_without_blobs(Some(block_root), self.next_block.clone()), std::time::Duration::default(), BlockProcessType::SingleBlock { id: 1 }, ) From 96cb6fb57a09817b9dc5db23cbb53b68fa99f2b9 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 10 Jun 2025 15:13:33 +0200 Subject: [PATCH 18/27] Only perform validator custody registration if PeerDAS is scheduled. --- beacon_node/beacon_chain/src/beacon_chain.rs | 23 ++---- .../beacon_chain/src/persisted_custody.rs | 7 -- .../beacon_chain/src/validator_custody.rs | 16 ++-- beacon_node/http_api/src/lib.rs | 73 +++++++++++-------- .../lighthouse_network/src/types/globals.rs | 2 +- 5 files changed, 58 insertions(+), 63 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c13502c59f2..b42878d4cd7 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -58,7 +58,7 @@ use crate::observed_data_sidecars::ObservedDataSidecars; use crate::observed_operations::{ObservationOutcome, ObservedOperations}; use crate::observed_slashable::ObservedSlashable; use crate::persisted_beacon_chain::PersistedBeaconChain; -use crate::persisted_custody::{clear_custody_context, persist_custody_context}; +use crate::persisted_custody::persist_custody_context; use crate::persisted_fork_choice::PersistedForkChoice; use crate::pre_finalization_cache::PreFinalizationBlockCache; use crate::shuffling_cache::{BlockShufflingIds, ShufflingCache}; @@ -679,22 +679,11 @@ impl BeaconChain { .into(); debug!(?custody_context, "Persisting custody context to store"); - if let Err(e) = - clear_custody_context::(self.store.clone()) - { - error!(error = ?e, "Failed to clear old custody context"); - } - - match persist_custody_context::( + persist_custody_context::( self.store.clone(), custody_context, - ) { - Ok(_) => info!("Saved custody state"), - Err(e) => error!( - error = ?e, - "Failed to persist custody context on drop" - ), - } + )?; + Ok(()) } @@ -7216,10 +7205,10 @@ impl BeaconChain { impl Drop for BeaconChain { fn drop(&mut self) { let drop = || -> Result<(), Error> { - self.persist_custody_context()?; self.persist_fork_choice()?; self.persist_op_pool()?; - self.persist_eth1_cache() + self.persist_eth1_cache()?; + self.persist_custody_context() }; if let Err(e) = drop() { diff --git a/beacon_node/beacon_chain/src/persisted_custody.rs b/beacon_node/beacon_chain/src/persisted_custody.rs index 1f57f259eaa..6ede473b36d 100644 --- a/beacon_node/beacon_chain/src/persisted_custody.rs +++ b/beacon_node/beacon_chain/src/persisted_custody.rs @@ -29,13 +29,6 @@ pub fn persist_custody_context, Cold: ItemStore store.put_item(&CUSTODY_DB_KEY, &PersistedCustody(custody_context)) } -/// Attempts to clear any custody context entries. -pub fn clear_custody_context, Cold: ItemStore>( - store: Arc>, -) -> Result<(), store::Error> { - store.hot_db.delete::(&CUSTODY_DB_KEY) -} - impl StoreItem for PersistedCustody { fn db_column() -> DBColumn { DBColumn::CustodyContext diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index b4b4ebc96e7..2828ebd1555 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -23,6 +23,8 @@ struct ValidatorRegistrations { /// /// Note: Only stores the epoch value when there's a change in custody requirement. /// So if epoch 10 and 11 has the same custody requirement, only 10 is stored. + /// This map is never pruned, because currently we never decrease custody requirement, so this + /// map size is contained at 128. epoch_validator_custody_requirements: BTreeMap, } @@ -54,10 +56,10 @@ impl ValidatorRegistrations { self.validators.insert(validator_index, effective_balance); // Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of "weight". - let validator_count_at_epoch = + let validator_custody_units = self.validators.values().sum::() / spec.balance_per_additional_custody_group; let validator_custody_requirement = - get_validators_custody_requirement(validator_count_at_epoch, spec); + get_validators_custody_requirement(validator_custody_units, spec); // If registering the new validator increased the total validator "units", then // add a new entry for the current epoch @@ -69,26 +71,26 @@ impl ValidatorRegistrations { } tracing::debug!( %epoch, - validator_count = validator_count_at_epoch, + validator_custody_units, validator_custody_requirement, "Registered validators" ); } } -/// Given the `count` of validators, return the custody requirement based on +/// Given the `validator_custody_units`, return the custody requirement based on /// the spec parameters. /// -/// Note: a validator here represents a unit of 32 eth effective_balance +/// Note: a `validator_custody_units` here represents the number of 32 eth effective_balance /// equivalent to `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP`. /// /// For e.g. a validator with eb 32 eth is 1 unit. /// a validator with eb 65 eth is 65 // 32 = 2 units. /// /// See https://github.com/ethereum/consensus-specs/blob/dev/specs/fulu/validator.md#validator-custody -fn get_validators_custody_requirement(count: u64, spec: &ChainSpec) -> u64 { +fn get_validators_custody_requirement(validator_custody_units: u64, spec: &ChainSpec) -> u64 { std::cmp::min( - std::cmp::max(count, spec.validator_custody_requirement), + std::cmp::max(validator_custody_units, spec.validator_custody_requirement), spec.number_of_custody_groups, ) } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7e8e77cbdad..8de496d5283 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3773,37 +3773,22 @@ pub fn serve( task_spawner: TaskSpawner, chain: Arc>| { task_spawner.blocking_json_task(Priority::P0, move || { - let mut subscriptions: std::collections::BTreeSet<_> = Default::default(); - let mut registrations = Vec::new(); - let (finalized_beacon_state, _, _) = - StateId(CoreStateId::Finalized).state(&chain)?; - for subscription in committee_subscriptions.iter() { - chain - .validator_monitor - .write() - .auto_register_local_validator(subscription.validator_index); - // Register the validator with the `CustodyContext` - if let Ok(effective_balance) = finalized_beacon_state - .get_effective_balance(subscription.validator_index as usize) - { - registrations - .push((subscription.validator_index as usize, effective_balance)); - } - subscriptions.insert(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, - }); - } - chain - .data_availability_checker - .custody_context() - .register_validators::( - registrations, - chain.slot().unwrap(), - &chain.spec, - ); + let subscriptions: std::collections::BTreeSet<_> = committee_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) { @@ -3818,6 +3803,32 @@ pub fn serve( )); } + if chain.spec.is_peer_das_scheduled() { + let (finalized_beacon_state, _, _) = + StateId(CoreStateId::Finalized).state(&chain)?; + let validators_and_balances = committee_subscriptions + .iter() + .filter_map(|subscription| { + if let Ok(effective_balance) = finalized_beacon_state + .get_effective_balance(subscription.validator_index as usize) + { + Some((subscription.validator_index as usize, effective_balance)) + } else { + None + } + }) + .collect::>(); + + chain + .data_availability_checker + .custody_context() + .register_validators::( + validators_and_balances, + chain.slot().unwrap(), + &chain.spec, + ); + } + Ok(()) }) }, diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index 3636fae873c..ca3e389cc91 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -274,7 +274,7 @@ impl NetworkGlobals { trusted_peers, false, config, - spec.number_of_columns, + spec.number_of_custody_groups, spec, ) } From 011def61c465c3375d11d7be2eea9bc85ec1a9d0 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 10:26:47 +0200 Subject: [PATCH 19/27] Remove network dependency from beacon chain: lift the CGC updated network message to the http api. --- .../beacon_chain/src/validator_custody.rs | 56 ++++++------------- beacon_node/http_api/src/lib.rs | 20 +++++-- .../src/network_beacon_processor/tests.rs | 5 +- beacon_node/network/src/service.rs | 37 +++++------- 4 files changed, 49 insertions(+), 69 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 2828ebd1555..8fb9aed0412 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -6,11 +6,8 @@ use std::{ use parking_lot::RwLock; use ssz_derive::{Decode, Encode}; -use tokio::sync::broadcast::{channel, Receiver, Sender}; use types::{ChainSpec, Epoch, EthSpec, Slot}; -const CHANNEL_CAPACITY: usize = 10; - /// TODO(pawan): this currently just registers increases in validator count. /// Does not handle decreasing validator counts #[derive(Default, Debug)] @@ -117,7 +114,6 @@ pub struct CustodyContext { persisted_is_supernode: bool, /// Maintains all the validators that this node is connected to currently validator_registrations: RwLock, - sender: Sender, } impl CustodyContext { @@ -126,14 +122,11 @@ impl CustodyContext { /// /// The `is_supernode` value is based on current cli parameters. pub fn new(is_supernode: bool) -> Self { - let (sender, _) = channel(CHANNEL_CAPACITY); - Self { validator_custody_count: AtomicU64::new(0), current_is_supernode: is_supernode, persisted_is_supernode: is_supernode, validator_registrations: Default::default(), - sender, } } @@ -141,30 +134,26 @@ impl CustodyContext { ssz_context: CustodyContextSsz, is_supernode: bool, ) -> Self { - let (sender, _) = channel(CHANNEL_CAPACITY); CustodyContext { validator_custody_count: AtomicU64::new(ssz_context.validator_custody_at_head), current_is_supernode: is_supernode, persisted_is_supernode: ssz_context.persisted_is_supernode, validator_registrations: Default::default(), - sender, } } - pub fn subscribe(&self) -> Receiver { - self.sender.subscribe() - } - /// Register a new validator index and updates the list of validators if required. /// /// Also modifies the internal structures if the validator custody has changed to /// update the `custody_column_count`. + /// + /// Returns `Some` along with the updated custody group count if it has changed, otherwise returns `None`. pub fn register_validators( &self, validators_and_balance: Vec<(usize, u64)>, slot: Slot, spec: &ChainSpec, - ) { + ) -> Option { let mut registrations = self.validator_registrations.write(); for (validator_index, effective_balance) in validators_and_balance { registrations.register_validator::(validator_index, effective_balance, slot, spec); @@ -173,7 +162,7 @@ impl CustodyContext { // Completed registrations, now check if the validator custody requirement has changed let Some(new_validator_custody) = registrations.latest_validator_custody_requirement() else { - return; + return None; }; let current_cgc = self.custody_group_count(spec); @@ -196,23 +185,21 @@ impl CustodyContext { updated_cgc, "Custody group count updated" ); - if let Err(e) = self - .sender - .send(CustodyContextMessage::HeadCustodyCountChanged { - new_custody_count: updated_cgc, - }) - { - tracing::error!(error=?e, "Failed to send custody context message"); - } + return Some(CustodyCountChanged { + new_custody_group_count: updated_cgc, + sampling_count: self.sampling_count(spec), + }); } } + + None } /// The custody count that we use to custody columns currently. /// /// This function should be called when figuring out how many columns we /// need to custody when receiving blocks over gossip/rpc or during sync. - pub fn custody_group_count(&self, spec: &ChainSpec) -> u64 { + pub(crate) fn custody_group_count(&self, spec: &ChainSpec) -> u64 { if self.current_is_supernode { return spec.number_of_custody_groups; } @@ -234,22 +221,11 @@ impl CustodyContext { } } -// write a service that emits events when internal values change -#[derive(Debug, Clone)] -pub enum CustodyContextMessage { - /// The custody count changed because of a change in the - /// number of validators being managed. - /// - /// This should trigger actions downstream like - /// subscribing/unsubscribing new subnets/ - /// backfilling required columns. - HeadCustodyCountChanged { new_custody_count: u64 }, - /// The advertised custody count has changed. - /// - /// This should trigger downstream actions like setting - /// a new cgc value in the enr and metadata fields and - /// performing any related cleanup actions. - AdvertisedCustodyCountChanged { new_custody_count: u64 }, +/// The custody count changed because of a change in the +/// number of validators being managed. +pub struct CustodyCountChanged { + pub new_custody_group_count: u64, + pub sampling_count: u64, } /// The custody information that gets persisted across runs. diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 8de496d5283..58cbd8cb1cd 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3765,11 +3765,13 @@ pub fn serve( .and(warp::path::end()) .and(warp_utils::json::json()) .and(validator_subscription_tx_filter.clone()) + .and(network_tx_filter.clone()) .and(task_spawner_filter.clone()) .and(chain_filter.clone()) .then( |committee_subscriptions: Vec, validator_subscription_tx: Sender, + network_tx: UnboundedSender>, task_spawner: TaskSpawner, chain: Arc>| { task_spawner.blocking_json_task(Priority::P0, move || { @@ -3819,14 +3821,22 @@ pub fn serve( }) .collect::>(); - chain + if let Some(cgc_change) = chain .data_availability_checker .custody_context() .register_validators::( - validators_and_balances, - chain.slot().unwrap(), - &chain.spec, - ); + validators_and_balances, + chain.slot().unwrap(), + &chain.spec, + ) { + network_tx.send(NetworkMessage::CustodyCountChanged { + new_custody_group_count: cgc_change.new_custody_group_count, + sampling_count: cgc_change.sampling_count, + }).unwrap_or_else(|e| { + debug!(error = %e, "Could not send message to the network service. \ + Likely shutdown") + }); + } } Ok(()) diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 0132ec76d18..7c92e973d05 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -230,6 +230,7 @@ impl TestRig { vec![], false, network_config, + spec.custody_requirement, spec, )); @@ -374,7 +375,9 @@ impl TestRig { pub fn custody_columns_count(&self) -> usize { self.network_beacon_processor .network_globals - .custody_columns_count() as usize + .sampling_columns + .read() + .len() } pub fn enqueue_rpc_block(&self) { diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 00d4a261360..cec7422ec7c 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -5,7 +5,6 @@ use crate::persisted_dht::{clear_dht, load_dht, persist_dht}; use crate::router::{Router, RouterMessage}; use crate::subnet_service::{SubnetService, SubnetServiceMessage, Subscription}; use crate::NetworkConfig; -use beacon_chain::validator_custody::CustodyContextMessage; use beacon_chain::{BeaconChain, BeaconChainTypes}; use beacon_processor::{work_reprocessing_queue::ReprocessQueueMessage, BeaconProcessorSend}; use futures::channel::mpsc::Sender; @@ -107,6 +106,12 @@ pub enum NetworkMessage { ConnectTrustedPeer(Enr), /// Disconnect from a trusted peer and remove it from the `trusted_peers` mapping. DisconnectTrustedPeer(Enr), + /// Custody group count changed due to a change in validators' weight. + /// Subscribe to new subnets and update ENR metadata. + CustodyCountChanged { + new_custody_group_count: u64, + sampling_count: u64, + }, } /// Messages triggered by validators that may trigger a subscription to a subnet. @@ -197,8 +202,6 @@ pub struct NetworkService { gossipsub_parameter_update: tokio::time::Interval, /// Provides fork specific info. fork_context: Arc, - /// Receiver for custody context messages - custody_context_rx: tokio::sync::broadcast::Receiver, } impl NetworkService { @@ -334,11 +337,6 @@ impl NetworkService { validator_subscription_recv, } = network_receivers; - let custody_context_rx = beacon_chain - .data_availability_checker - .custody_context() - .subscribe(); - // create the network service and spawn the task let network_service = NetworkService { beacon_chain, @@ -357,7 +355,6 @@ impl NetworkService { metrics_update, gossipsub_parameter_update, fork_context, - custody_context_rx, }; Ok((network_service, network_globals, network_senders)) @@ -449,20 +446,6 @@ impl NetworkService { _ = self.gossipsub_parameter_update.tick() => self.update_gossipsub_parameters(), - custody_message = Box::pin(self.custody_context_rx.recv()) => { - match custody_message { - Ok(CustodyContextMessage::HeadCustodyCountChanged{new_custody_count}) => { - self.libp2p.subscribe_new_data_column_subnets(new_custody_count); - } - Ok(CustodyContextMessage::AdvertisedCustodyCountChanged{new_custody_count: _}) => { - unimplemented!("update metadata and enr"); - } - Err(e) => { - error!("Error receiving custody context message: {:?}", e); - } - } - }, - // handle a message sent to the network Some(msg) = self.network_recv.recv() => self.on_network_msg(msg, &mut shutdown_sender).await, @@ -777,6 +760,14 @@ impl NetworkService { ); } } + NetworkMessage::CustodyCountChanged { + new_custody_group_count: _, + sampling_count, + } => { + // TODO: update ENR and metadata + self.libp2p + .subscribe_new_data_column_subnets(sampling_count) + } } } From 92a990a8ebce0eb42e13145f1026590d90cfe290 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 14:28:45 +0200 Subject: [PATCH 20/27] Add an epoch params to sampling size, to determine the cgc to apply to the block. --- .../overflow_lru_cache.rs | 6 +- .../beacon_chain/src/validator_custody.rs | 182 ++++++++++++------ beacon_node/network/src/service.rs | 2 +- 3 files changed, 126 insertions(+), 64 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index d26381d3636..b9303bed37c 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -473,7 +473,7 @@ impl DataAvailabilityCheckerInner { if let Some(available_block) = pending_components.make_available( &self.spec, - self.custody_context.sampling_count(&self.spec), + self.custody_context.sampling_size(Some(epoch), &self.spec), |block| self.state_cache.recover_pending_executed_block(block), )? { // We keep the pending components in the availability cache during block import (#5845). @@ -527,7 +527,7 @@ impl DataAvailabilityCheckerInner { if let Some(available_block) = pending_components.make_available( &self.spec, - self.custody_context.sampling_count(&self.spec), + self.custody_context.sampling_size(Some(epoch), &self.spec), |block| self.state_cache.recover_pending_executed_block(block), )? { // We keep the pending components in the availability cache during block import (#5845). @@ -623,7 +623,7 @@ impl DataAvailabilityCheckerInner { // Check if we have all components and entire set is consistent. if let Some(available_block) = pending_components.make_available( &self.spec, - self.custody_context.sampling_count(&self.spec), + self.custody_context.sampling_size(Some(epoch), &self.spec), |block| self.state_cache.recover_pending_executed_block(block), )? { // We keep the pending components in the availability cache during block import (#5845). diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 8fb9aed0412..b6ec7d0e470 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -8,6 +8,11 @@ use parking_lot::RwLock; use ssz_derive::{Decode, Encode}; use types::{ChainSpec, Epoch, EthSpec, Slot}; +/// A delay before making the CGC change effective to the data availability checker. +const CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS: u64 = 30; + +type ValidatorsAndBalances = Vec<(usize, u64)>; + /// TODO(pawan): this currently just registers increases in validator count. /// Does not handle decreasing validator counts #[derive(Default, Debug)] @@ -27,30 +32,31 @@ struct ValidatorRegistrations { impl ValidatorRegistrations { /// Returns the validator custody requirement at the latest epoch. - pub fn latest_validator_custody_requirement(&self) -> Option { + fn latest_validator_custody_requirement(&self) -> Option { self.epoch_validator_custody_requirements .last_key_value() .map(|(_, v)| *v) } - /// Returns the latest epoch at which the validator count changed. - #[allow(dead_code)] - pub fn latest_epoch(&self) -> Option { + /// Lookup the active custody requirement at the given epoch. + fn custody_requirement_at_epoch(&self, epoch: Epoch) -> Option { self.epoch_validator_custody_requirements - .last_key_value() - .map(|(k, _)| *k) + .range(..=epoch) + .last() + .map(|(_, custody_count)| *custody_count) } /// Register a new validator index and updates the list of validators if required. - pub fn register_validator( + /// Returns `Some((effective_epoch, new_cgc))` if the registration results in a CGC update. + pub(crate) fn register_validators( &mut self, - validator_index: usize, - effective_balance: u64, + validators_and_balance: ValidatorsAndBalances, slot: Slot, spec: &ChainSpec, - ) { - let epoch = slot.epoch(E::slots_per_epoch()); - self.validators.insert(validator_index, effective_balance); + ) -> Option<(Epoch, u64)> { + for (validator_index, effective_balance) in validators_and_balance { + self.validators.insert(validator_index, effective_balance); + } // Each `BALANCE_PER_ADDITIONAL_CUSTODY_GROUP` effectively contributes one unit of "weight". let validator_custody_units = @@ -58,20 +64,28 @@ impl ValidatorRegistrations { let validator_custody_requirement = get_validators_custody_requirement(validator_custody_units, spec); + tracing::debug!( + validator_custody_units, + validator_custody_requirement, + "Registered validators" + ); + // If registering the new validator increased the total validator "units", then // add a new entry for the current epoch if Some(validator_custody_requirement) != self.latest_validator_custody_requirement() { + // Apply the change from the next epoch after adding some delay buffer to ensure + // the node has enough time to subscribe to subnets etc. + let effective_delay_slots = + CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS / spec.seconds_per_slot; + let effective_epoch = (slot + effective_delay_slots).epoch(E::slots_per_epoch()) + 1; self.epoch_validator_custody_requirements - .entry(epoch) + .entry(effective_epoch) .and_modify(|old_custody| *old_custody = validator_custody_requirement) .or_insert(validator_custody_requirement); + Some((effective_epoch, validator_custody_requirement)) + } else { + None } - tracing::debug!( - %epoch, - validator_custody_units, - validator_custody_requirement, - "Registered validators" - ); } } @@ -150,17 +164,14 @@ impl CustodyContext { /// Returns `Some` along with the updated custody group count if it has changed, otherwise returns `None`. pub fn register_validators( &self, - validators_and_balance: Vec<(usize, u64)>, + validators_and_balance: ValidatorsAndBalances, slot: Slot, spec: &ChainSpec, ) -> Option { - let mut registrations = self.validator_registrations.write(); - for (validator_index, effective_balance) in validators_and_balance { - registrations.register_validator::(validator_index, effective_balance, slot, spec); - } - - // Completed registrations, now check if the validator custody requirement has changed - let Some(new_validator_custody) = registrations.latest_validator_custody_requirement() + let Some((effective_epoch, new_validator_custody)) = self + .validator_registrations + .write() + .register_validators::(validators_and_balance, slot, spec) else { return None; }; @@ -187,7 +198,7 @@ impl CustodyContext { ); return Some(CustodyCountChanged { new_custody_group_count: updated_cgc, - sampling_count: self.sampling_count(spec), + sampling_count: self.sampling_size(Some(effective_epoch), spec), }); } } @@ -199,7 +210,10 @@ impl CustodyContext { /// /// This function should be called when figuring out how many columns we /// need to custody when receiving blocks over gossip/rpc or during sync. - pub(crate) fn custody_group_count(&self, spec: &ChainSpec) -> u64 { + /// + /// NOTE: this function is intended for internal calculation only and + /// should **NOT** be exposed externally. Use `self.sampling_size_at_epoch` instead. + fn custody_group_count(&self, spec: &ChainSpec) -> u64 { if self.current_is_supernode { return spec.number_of_custody_groups; } @@ -213,10 +227,26 @@ impl CustodyContext { } } - /// Returns the count of custody columns this node must sample for block import. - pub fn sampling_count(&self, spec: &ChainSpec) -> u64 { - // This only panics if the chain spec contains invalid values - spec.sampling_size(self.custody_group_count(spec)) + fn default_custody_group_count(&self, spec: &ChainSpec) -> u64 { + if self.current_is_supernode { + spec.number_of_custody_groups + } else { + spec.custody_requirement + } + } + + /// Returns the count of custody columns this node must sample for a block at `epoch` to import. + /// If an `epoch` is not specified, returns the *current* validator custody requirement. + pub fn sampling_size(&self, epoch_opt: Option, spec: &ChainSpec) -> u64 { + let custody_group_count = if let Some(epoch) = epoch_opt { + self.validator_registrations + .read() + .custody_requirement_at_epoch(epoch) + .unwrap_or_else(|| self.default_custody_group_count(spec)) + } else { + self.custody_group_count(spec) + }; + spec.sampling_size(custody_group_count) .expect("should compute node sampling size from valid chain spec") } } @@ -280,19 +310,21 @@ mod tests { let bal_per_additional_group = spec.balance_per_additional_custody_group; let min_val_custody_requirement = spec.validator_custody_requirement; // One single node increases its balance over 3 epochs. - let validators_and_expected_cgc = vec![ + let validators_and_expected_cgc_change = vec![ ( vec![(0, bal_per_additional_group)], - min_val_custody_requirement, + Some(min_val_custody_requirement), ), - ( - vec![(0, 8 * bal_per_additional_group)], - min_val_custody_requirement, - ), - (vec![(0, 10 * bal_per_additional_group)], 10), + // No CGC change at 8 custody units, as it's the minimum requirement + (vec![(0, 8 * bal_per_additional_group)], None), + (vec![(0, 10 * bal_per_additional_group)], Some(10)), ]; - register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); + register_validators_and_assert_cgc( + custody_context, + validators_and_expected_cgc_change, + &spec, + ); } #[test] @@ -305,14 +337,15 @@ mod tests { let validators_and_expected_cgc = vec![ ( vec![(0, bal_per_additional_group)], - min_val_custody_requirement, + Some(min_val_custody_requirement), ), ( vec![ (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), ], - min_val_custody_requirement, + // No CGC change at 8 custody units, as it's the minimum requirement + None, ), ( vec![ @@ -320,7 +353,7 @@ mod tests { (1, 7 * bal_per_additional_group), (2, 2 * bal_per_additional_group), ], - 10, + Some(10), ), ]; @@ -335,16 +368,13 @@ mod tests { // Add 3 validators over 3 epochs. let validators_and_expected_cgc = vec![ - ( - vec![(0, bal_per_additional_group)], - spec.number_of_custody_groups, - ), + (vec![(0, bal_per_additional_group)], None), ( vec![ (0, bal_per_additional_group), (1, 7 * bal_per_additional_group), ], - spec.number_of_custody_groups, + None, ), ( vec![ @@ -352,30 +382,62 @@ mod tests { (1, 7 * bal_per_additional_group), (2, 2 * bal_per_additional_group), ], - spec.number_of_custody_groups, + None, ), ]; register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); } + #[test] + fn cgc_change_should_be_effective_to_sampling_after_delay() { + let custody_context = CustodyContext::new(false); + let spec = E::default_spec(); + let current_slot = Slot::new(10); + let current_epoch = current_slot.epoch(E::slots_per_epoch()); + let default_sampling_size = custody_context.sampling_size(None, &spec); + let validator_custody_units = 10; + + let _cgc_changed = custody_context.register_validators::( + vec![( + 0, + validator_custody_units * spec.balance_per_additional_custody_group, + )], + current_slot, + &spec, + ); + + // CGC update is not applied for `current_epoch`. + assert_eq!( + custody_context.sampling_size(Some(current_epoch), &spec), + default_sampling_size + ); + // CGC update is applied for the next epoch. + assert_eq!( + custody_context.sampling_size(Some(current_epoch + 1), &spec), + validator_custody_units + ); + } + /// Update validator every epoch and assert cgc against expected values. fn register_validators_and_assert_cgc( custody_context: CustodyContext, - validators_and_expected_cgc: Vec<(Vec<(usize, u64)>, u64)>, + validators_and_expected_cgc_changed: Vec<(ValidatorsAndBalances, Option)>, spec: &ChainSpec, ) { - for (idx, (validators_and_balance, expected_cgc)) in - validators_and_expected_cgc.into_iter().enumerate() + for (idx, (validators_and_balance, expected_cgc_change)) in + validators_and_expected_cgc_changed.into_iter().enumerate() { let epoch = Epoch::new(idx as u64); - custody_context.register_validators::( - validators_and_balance, - epoch.start_slot(E::slots_per_epoch()), - spec, - ); - - assert_eq!(custody_context.custody_group_count(spec), expected_cgc); + let updated_custody_count_opt = custody_context + .register_validators::( + validators_and_balance, + epoch.start_slot(E::slots_per_epoch()), + spec, + ) + .map(|c| c.new_custody_group_count); + + assert_eq!(updated_custody_count_opt, expected_cgc_change); } } } diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index cec7422ec7c..d55ee724b3d 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -283,7 +283,7 @@ impl NetworkService { beacon_chain .data_availability_checker .custody_context() - .sampling_count(&beacon_chain.spec), + .sampling_size(None, &beacon_chain.spec), ) .await?; From 55bcba678a933d157f849b5ea2812740e7ba9513 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 15:34:33 +0200 Subject: [PATCH 21/27] Send CGC updates to network to update ENR, metadata and subscribe to subnets. --- .../beacon_chain/src/validator_custody.rs | 3 +- beacon_node/http_api/src/lib.rs | 4 +- .../lighthouse_network/src/discovery/mod.rs | 10 +++++ .../lighthouse_network/src/service/mod.rs | 38 ++++++++++++++++++- beacon_node/network/src/service.rs | 7 ++-- 5 files changed, 56 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index b6ec7d0e470..2fc21d77a42 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -74,7 +74,8 @@ impl ValidatorRegistrations { // add a new entry for the current epoch if Some(validator_custody_requirement) != self.latest_validator_custody_requirement() { // Apply the change from the next epoch after adding some delay buffer to ensure - // the node has enough time to subscribe to subnets etc. + // the node has enough time to subscribe to subnets etc, and to avoid having + // inconsistent column counts within an epoch. let effective_delay_slots = CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS / spec.seconds_per_slot; let effective_epoch = (slot + effective_delay_slots).epoch(E::slots_per_epoch()) + 1; diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 58cbd8cb1cd..741a5856c9f 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3821,12 +3821,14 @@ pub fn serve( }) .collect::>(); + let current_slot = + chain.slot().map_err(warp_utils::reject::unhandled_error)?; if let Some(cgc_change) = chain .data_availability_checker .custody_context() .register_validators::( validators_and_balances, - chain.slot().unwrap(), + current_slot, &chain.spec, ) { network_tx.send(NetworkMessage::CustodyCountChanged { diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index d6845421f31..d8dac623c78 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -49,6 +49,7 @@ use tracing::{debug, error, info, trace, warn}; use types::{ChainSpec, EnrForkId, EthSpec}; mod subnet_predicate; +use crate::discovery::enr::PEERDAS_CUSTODY_GROUP_COUNT_ENR_KEY; pub use subnet_predicate::subnet_predicate; use types::non_zero_usize::new_non_zero_usize; @@ -476,6 +477,15 @@ impl Discovery { Ok(()) } + pub fn update_enr_cgc(&mut self, custody_group_count: u64) -> Result<(), String> { + self.discv5 + .enr_insert(PEERDAS_CUSTODY_GROUP_COUNT_ENR_KEY, &custody_group_count) + .map_err(|e| format!("{:?}", e))?; + enr::save_enr_to_disk(Path::new(&self.enr_dir), &self.local_enr()); + *self.network_globals.local_enr.write() = self.discv5.local_enr(); + Ok(()) + } + /// Adds/Removes a subnet from the ENR attnets/syncnets Bitfield pub fn update_enr_bitfield(&mut self, subnet: Subnet, value: bool) -> Result<(), String> { let local_enr = self.discv5.local_enr(); diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 1d1fc66d4cb..4a02e12b07d 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -889,7 +889,6 @@ impl Network { } /// Subscribe to all data columns determined by the cgc. - /// TODO(pawan): unsubscribe if count reduces #[instrument(parent = None, level = "trace", fields(service = "libp2p"), @@ -1275,6 +1274,21 @@ impl Network { self.update_metadata_bitfields(); } + /// Updates the cgc value in the ENR. + #[instrument(parent = None, + level = "trace", + fields(service = "libp2p"), + name = "libp2p", + skip_all + )] + pub fn update_enr_cgc(&mut self, new_custody_group_count: u64) { + if let Err(e) = self.discovery_mut().update_enr_cgc(new_custody_group_count) { + crit!(error = e, "Could not update cgc in ENR"); + } + // update the local meta data which informs our peers of the update during PINGS + self.update_metadata_cgc(new_custody_group_count); + } + /// Attempts to discover new peers for a given subnet. The `min_ttl` gives the time at which we /// would like to retain the peers for. #[instrument(parent = None, @@ -1389,6 +1403,28 @@ impl Network { utils::save_metadata_to_disk(&self.network_dir, meta_data); } + #[instrument(parent = None, + level = "trace", + fields(service = "libp2p"), + name = "libp2p", + skip_all + )] + fn update_metadata_cgc(&mut self, custody_group_count: u64) { + let mut meta_data_w = self.network_globals.local_metadata.write(); + + *meta_data_w.seq_number_mut() += 1; + if let Ok(cgc) = meta_data_w.custody_group_count_mut() { + *cgc = custody_group_count; + } + let seq_number = *meta_data_w.seq_number(); + let meta_data = meta_data_w.clone(); + + drop(meta_data_w); + self.eth2_rpc_mut().update_seq_number(seq_number); + // Save the updated metadata to disk + utils::save_metadata_to_disk(&self.network_dir, meta_data); + } + /// Sends a Ping request to the peer. #[instrument(parent = None, level = "trace", diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index d55ee724b3d..73084e5c74c 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -761,12 +761,13 @@ impl NetworkService { } } NetworkMessage::CustodyCountChanged { - new_custody_group_count: _, + new_custody_group_count, sampling_count, } => { - // TODO: update ENR and metadata + // subscribe to `sampling_count` subnets self.libp2p - .subscribe_new_data_column_subnets(sampling_count) + .subscribe_new_data_column_subnets(sampling_count); + self.libp2p.update_enr_cgc(new_custody_group_count); } } } From e29d867b787cbc50ccfe2e988ac7150be08b4c89 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 15:41:19 +0200 Subject: [PATCH 22/27] Lint fixes. --- .../beacon_chain/tests/block_verification.rs | 95 +++++-------------- .../tests/payload_invalidation.rs | 3 +- .../tests/broadcast_validation_tests.rs | 19 ++-- .../src/network_beacon_processor/tests.rs | 8 -- 4 files changed, 35 insertions(+), 90 deletions(-) diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 9225ffd9f41..3ff5f772aa7 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -30,8 +30,6 @@ type E = MainnetEthSpec; const VALIDATOR_COUNT: usize = 24; const CHAIN_SEGMENT_LENGTH: usize = 64 * 5; const BLOCK_INDICES: &[usize] = &[0, 1, 32, 64, 68 + 1, 129, CHAIN_SEGMENT_LENGTH - 1]; -// Default custody group count for tests -const CGC: usize = 8; /// A cached set of keys. static KEYPAIRS: LazyLock> = @@ -144,10 +142,9 @@ fn build_rpc_block( RpcBlock::new(None, block, Some(blobs.clone())).unwrap() } Some(DataSidecars::DataColumns(columns)) => { - RpcBlock::new_with_custody_columns(None, block, columns.clone(), columns.len(), spec) - .unwrap() + RpcBlock::new_with_custody_columns(None, block, columns.clone(), spec).unwrap() } - None => RpcBlock::new_without_blobs(None, block, 0), + None => RpcBlock::new_without_blobs(None, block), } } @@ -370,7 +367,6 @@ async fn chain_segment_non_linear_parent_roots() { blocks[3] = RpcBlock::new_without_blobs( None, Arc::new(SignedBeaconBlock::from_block(block, signature)), - harness.sampling_column_count, ); assert!( @@ -408,7 +404,6 @@ async fn chain_segment_non_linear_slots() { blocks[3] = RpcBlock::new_without_blobs( None, Arc::new(SignedBeaconBlock::from_block(block, signature)), - harness.sampling_column_count, ); assert!( @@ -436,7 +431,6 @@ async fn chain_segment_non_linear_slots() { blocks[3] = RpcBlock::new_without_blobs( None, Arc::new(SignedBeaconBlock::from_block(block, signature)), - harness.sampling_column_count, ); assert!( @@ -578,11 +572,7 @@ async fn invalid_signature_gossip_block() { .into_block_error() .expect("should import all blocks prior to the one being tested"); let signed_block = SignedBeaconBlock::from_block(block, junk_signature()); - let rpc_block = RpcBlock::new_without_blobs( - None, - Arc::new(signed_block), - harness.sampling_column_count, - ); + let rpc_block = RpcBlock::new_without_blobs(None, Arc::new(signed_block)); let process_res = harness .chain .process_block( @@ -1002,7 +992,6 @@ async fn block_gossip_verification() { let (chain_segment, chain_segment_blobs) = get_chain_segment().await; let block_index = CHAIN_SEGMENT_LENGTH - 2; - let cgc = harness.chain.spec.custody_requirement as usize; harness .chain @@ -1016,7 +1005,7 @@ async fn block_gossip_verification() { { let gossip_verified = harness .chain - .verify_block_for_gossip(snapshot.beacon_block.clone(), get_cgc(&blobs_opt)) + .verify_block_for_gossip(snapshot.beacon_block.clone()) .await .expect("should obtain gossip verified block"); @@ -1058,7 +1047,7 @@ async fn block_gossip_verification() { *block.slot_mut() = expected_block_slot; assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature)), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature))).await), BlockError::FutureSlot { present_slot, block_slot, @@ -1092,7 +1081,7 @@ async fn block_gossip_verification() { *block.slot_mut() = expected_finalized_slot; assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature)), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature))).await), BlockError::WouldRevertFinalizedSlot { block_slot, finalized_slot, @@ -1122,10 +1111,10 @@ async fn block_gossip_verification() { unwrap_err( harness .chain - .verify_block_for_gossip( - Arc::new(SignedBeaconBlock::from_block(block, junk_signature())), - cgc - ) + .verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block( + block, + junk_signature() + )),) .await ), BlockError::InvalidSignature(InvalidSignature::ProposerSignature) @@ -1150,7 +1139,7 @@ async fn block_gossip_verification() { *block.parent_root_mut() = parent_root; assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature)), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature))).await), BlockError::ParentUnknown {parent_root: p} if p == parent_root ), @@ -1176,7 +1165,7 @@ async fn block_gossip_verification() { *block.parent_root_mut() = parent_root; assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature)), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature))).await), BlockError::NotFinalizedDescendant { block_parent_root } if block_parent_root == parent_root ), @@ -1213,7 +1202,7 @@ async fn block_gossip_verification() { ); assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(block.clone()), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(block.clone())).await), BlockError::IncorrectBlockProposer { block, local_shuffling, @@ -1225,7 +1214,7 @@ async fn block_gossip_verification() { // Check to ensure that we registered this is a valid block from this proposer. assert!( matches!( - unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(block.clone()), cgc).await), + unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(block.clone())).await), BlockError::DuplicateImportStatusUnknown(_), ), "should register any valid signature against the proposer, even if the block failed later verification" @@ -1233,11 +1222,7 @@ async fn block_gossip_verification() { let block = chain_segment[block_index].beacon_block.clone(); assert!( - harness - .chain - .verify_block_for_gossip(block, cgc) - .await - .is_ok(), + harness.chain.verify_block_for_gossip(block).await.is_ok(), "the valid block should be processed" ); @@ -1255,7 +1240,7 @@ async fn block_gossip_verification() { matches!( harness .chain - .verify_block_for_gossip(block.clone(), cgc) + .verify_block_for_gossip(block.clone()) .await .expect_err("should error when processing known block"), BlockError::DuplicateImportStatusUnknown(_) @@ -1331,17 +1316,8 @@ async fn verify_block_for_gossip_slashing_detection() { let state = harness.get_current_state(); let ((block1, blobs1), _) = harness.make_block(state.clone(), Slot::new(1)).await; let ((block2, _blobs2), _) = harness.make_block(state, Slot::new(1)).await; - let cgc = if block1.fork_name_unchecked().fulu_enabled() { - harness.get_sampling_column_count() - } else { - 0 - }; - let verified_block = harness - .chain - .verify_block_for_gossip(block1, cgc) - .await - .unwrap(); + let verified_block = harness.chain.verify_block_for_gossip(block1).await.unwrap(); if let Some((kzg_proofs, blobs)) = blobs1 { harness @@ -1364,7 +1340,7 @@ async fn verify_block_for_gossip_slashing_detection() { ) .await .unwrap(); - unwrap_err(harness.chain.verify_block_for_gossip(block2, CGC).await); + unwrap_err(harness.chain.verify_block_for_gossip(block2).await); // Slasher should have been handed the two conflicting blocks and crafted a slashing. slasher.process_queued(Epoch::new(0)).unwrap(); @@ -1388,11 +1364,7 @@ async fn verify_block_for_gossip_doppelganger_detection() { .attestations() .map(|att| att.clone_as_attestation()) .collect::>(); - let verified_block = harness - .chain - .verify_block_for_gossip(block, CGC) - .await - .unwrap(); + let verified_block = harness.chain.verify_block_for_gossip(block).await.unwrap(); harness .chain .process_block( @@ -1539,7 +1511,7 @@ async fn add_base_block_to_altair_chain() { assert!(matches!( harness .chain - .verify_block_for_gossip(Arc::new(base_block.clone()), CGC) + .verify_block_for_gossip(Arc::new(base_block.clone())) .await .expect_err("should error when processing base block"), BlockError::InconsistentFork(InconsistentFork { @@ -1549,7 +1521,7 @@ async fn add_base_block_to_altair_chain() { )); // Ensure that it would be impossible to import via `BeaconChain::process_block`. - let base_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(base_block.clone()), 0); + let base_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(base_block.clone())); assert!(matches!( harness .chain @@ -1573,7 +1545,7 @@ async fn add_base_block_to_altair_chain() { harness .chain .process_chain_segment( - vec![RpcBlock::new_without_blobs(None, Arc::new(base_block), 0)], + vec![RpcBlock::new_without_blobs(None, Arc::new(base_block))], NotifyExecutionLayer::Yes, ) .await, @@ -1676,7 +1648,7 @@ async fn add_altair_block_to_base_chain() { assert!(matches!( harness .chain - .verify_block_for_gossip(Arc::new(altair_block.clone()), CGC) + .verify_block_for_gossip(Arc::new(altair_block.clone())) .await .expect_err("should error when processing altair block"), BlockError::InconsistentFork(InconsistentFork { @@ -1686,7 +1658,7 @@ async fn add_altair_block_to_base_chain() { )); // Ensure that it would be impossible to import via `BeaconChain::process_block`. - let altair_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(altair_block.clone()), 0); + let altair_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(altair_block.clone())); assert!(matches!( harness .chain @@ -1710,7 +1682,7 @@ async fn add_altair_block_to_base_chain() { harness .chain .process_chain_segment( - vec![RpcBlock::new_without_blobs(None, Arc::new(altair_block), 0)], + vec![RpcBlock::new_without_blobs(None, Arc::new(altair_block))], NotifyExecutionLayer::Yes ) .await, @@ -1771,11 +1743,7 @@ async fn import_duplicate_block_unrealized_justification() { // Create two verified variants of the block, representing the same block being processed in // parallel. let notify_execution_layer = NotifyExecutionLayer::Yes; - let rpc_block = RpcBlock::new_without_blobs( - Some(block_root), - block.clone(), - harness.sampling_column_count, - ); + let rpc_block = RpcBlock::new_without_blobs(Some(block_root), block.clone()); let verified_block1 = rpc_block .clone() .into_execution_pending_block(block_root, chain, notify_execution_layer) @@ -1846,14 +1814,3 @@ async fn import_execution_pending_block( } } } - -fn get_cgc(blobs_opt: &Option>) -> usize { - if let Some(data_sidecars) = blobs_opt.as_ref() { - match data_sidecars { - DataSidecars::Blobs(_) => 0, - DataSidecars::DataColumns(d) => d.len(), - } - } else { - 0 - } -} diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index f9f39598d4d..05fae7aa70f 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -22,7 +22,6 @@ use task_executor::ShutdownReason; use types::*; const VALIDATOR_COUNT: usize = 32; -const CGC: usize = 8; type E = MainnetEthSpec; @@ -1052,7 +1051,7 @@ async fn invalid_parent() { // Ensure the block built atop an invalid payload is invalid for gossip. assert!(matches!( - rig.harness.chain.clone().verify_block_for_gossip(block.clone(), CGC).await, + rig.harness.chain.clone().verify_block_for_gossip(block.clone()).await, Err(BlockError::ParentExecutionPayloadInvalid { parent_root: invalid_root }) if invalid_root == parent_root )); diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index 04c47b1cbda..843242c22f7 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -39,9 +39,6 @@ type E = MainnetEthSpec; * */ -// Default custody group count for tests -const CGC: usize = 8; - /// This test checks that a block that is **invalid** from a gossip perspective gets rejected when using `broadcast_validation=gossip`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] pub async fn gossip_invalid() { @@ -367,9 +364,9 @@ pub async fn consensus_partial_pass_only_consensus() { ); assert_ne!(block_a.state_root(), block_b.state_root()); - let gossip_block_b = block_b.into_gossip_verified_block(&tester.harness.chain, CGC); + let gossip_block_b = block_b.into_gossip_verified_block(&tester.harness.chain); assert!(gossip_block_b.is_ok()); - let gossip_block_a = block_a.into_gossip_verified_block(&tester.harness.chain, CGC); + let gossip_block_a = block_a.into_gossip_verified_block(&tester.harness.chain); assert!(gossip_block_a.is_err()); /* submit `block_b` which should induce equivocation */ @@ -657,10 +654,10 @@ pub async fn equivocation_consensus_late_equivocation() { ); assert_ne!(block_a.state_root(), block_b.state_root()); - let gossip_block_b = block_b.into_gossip_verified_block(&tester.harness.chain, CGC); + let gossip_block_b = block_b.into_gossip_verified_block(&tester.harness.chain); assert!(gossip_block_b.is_ok()); - let gossip_block_a = block_a.into_gossip_verified_block(&tester.harness.chain, CGC); + let gossip_block_a = block_a.into_gossip_verified_block(&tester.harness.chain); assert!(gossip_block_a.is_err()); let channel = tokio::sync::mpsc::unbounded_channel(); @@ -1294,9 +1291,9 @@ pub async fn blinded_equivocation_consensus_late_equivocation() { ProvenancedBlock::Builder(b, _, _) => b, }; - let gossip_block_b = GossipVerifiedBlock::new(inner_block_b, &tester.harness.chain, CGC); + let gossip_block_b = GossipVerifiedBlock::new(inner_block_b, &tester.harness.chain); assert!(gossip_block_b.is_ok()); - let gossip_block_a = GossipVerifiedBlock::new(inner_block_a, &tester.harness.chain, CGC); + let gossip_block_a = GossipVerifiedBlock::new(inner_block_a, &tester.harness.chain); assert!(gossip_block_a.is_err()); let channel = tokio::sync::mpsc::unbounded_channel(); @@ -1398,7 +1395,7 @@ pub async fn block_seen_on_gossip_without_blobs() { // Simulate the block being seen on gossip. block .clone() - .into_gossip_verified_block(&tester.harness.chain, CGC) + .into_gossip_verified_block(&tester.harness.chain) .unwrap(); // It should not yet be added to fork choice because blobs have not been seen. @@ -1467,7 +1464,7 @@ pub async fn block_seen_on_gossip_with_some_blobs() { // Simulate the block being seen on gossip. block .clone() - .into_gossip_verified_block(&tester.harness.chain, CGC) + .into_gossip_verified_block(&tester.harness.chain) .unwrap(); // Simulate some of the blobs being seen on gossip. diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 7c92e973d05..9e5c96c2cf8 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -372,14 +372,6 @@ impl TestRig { } } - pub fn custody_columns_count(&self) -> usize { - self.network_beacon_processor - .network_globals - .sampling_columns - .read() - .len() - } - pub fn enqueue_rpc_block(&self) { let block_root = self.next_block.canonical_root(); self.network_beacon_processor From f593354a989114d659980896bfcc7468a85cbaf9 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 16:02:14 +0200 Subject: [PATCH 23/27] Cleanups --- .../overflow_lru_cache.rs | 31 ++++++++++++------- .../beacon_chain/src/validator_custody.rs | 2 +- beacon_node/network/src/sync/tests/range.rs | 9 +----- beacon_node/store/src/lib.rs | 2 +- scripts/local_testnet/network_params_das.yaml | 4 +-- .../tests/checkpoint-sync-config-devnet.yaml | 8 ++--- 6 files changed, 29 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index b9303bed37c..36c4f2cdc1e 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -308,13 +308,21 @@ impl PendingComponents { }) } - pub fn status_str(&self, block_epoch: Epoch, spec: &ChainSpec) -> String { + pub fn status_str( + &self, + block_epoch: Epoch, + num_expected_columns: Option, + spec: &ChainSpec, + ) -> String { let block_count = if self.executed_block.is_some() { 1 } else { 0 }; if spec.is_peer_das_enabled_for_epoch(block_epoch) { format!( - "block {} data_columns {}", + "block {} data_columns {}/{}", block_count, self.verified_data_columns.len(), + num_expected_columns + .map(|c| c.to_string()) + .unwrap_or("?".into()) ) } else { let num_expected_blobs = if let Some(block) = self.get_cached_block() { @@ -467,7 +475,7 @@ impl DataAvailabilityCheckerInner { debug!( component = "blobs", ?block_root, - status = pending_components.status_str(epoch, &self.spec), + status = pending_components.status_str(epoch, None, &self.spec), "Component added to data availability checker" ); @@ -518,18 +526,19 @@ impl DataAvailabilityCheckerInner { // Merge in the data columns. pending_components.merge_data_columns(kzg_verified_data_columns)?; + let num_expected_columns = self.custody_context.sampling_size(Some(epoch), &self.spec); debug!( component = "data_columns", ?block_root, - status = pending_components.status_str(epoch, &self.spec), + status = pending_components.status_str(epoch, Some(num_expected_columns), &self.spec), "Component added to data availability checker" ); - if let Some(available_block) = pending_components.make_available( - &self.spec, - self.custody_context.sampling_size(Some(epoch), &self.spec), - |block| self.state_cache.recover_pending_executed_block(block), - )? { + if let Some(available_block) = + pending_components.make_available(&self.spec, num_expected_columns, |block| { + self.state_cache.recover_pending_executed_block(block) + })? + { // We keep the pending components in the availability cache during block import (#5845). write_lock.put(block_root, pending_components); drop(write_lock); @@ -613,10 +622,11 @@ impl DataAvailabilityCheckerInner { // Merge in the block. pending_components.merge_block(diet_executed_block); + let num_expected_columns = self.custody_context.sampling_size(Some(epoch), &self.spec); debug!( component = "block", ?block_root, - status = pending_components.status_str(epoch, &self.spec), + status = pending_components.status_str(epoch, Some(num_expected_columns), &self.spec), "Component added to data availability checker" ); @@ -1246,7 +1256,6 @@ mod pending_components_tests { payload_verification_status: PayloadVerificationStatus::Verified, is_valid_merge_transition_block: false, }, - // Default custody columns count, doesn't matter here }; (block.into(), blobs, invalid_blobs) } diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 2fc21d77a42..5a2fea61bfc 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -13,7 +13,7 @@ const CUSTODY_CHANGE_DA_EFFECTIVE_DELAY_SECONDS: u64 = 30; type ValidatorsAndBalances = Vec<(usize, u64)>; -/// TODO(pawan): this currently just registers increases in validator count. +/// This currently just registers increases in validator count. /// Does not handle decreasing validator counts #[derive(Default, Debug)] struct ValidatorRegistrations { diff --git a/beacon_node/network/src/sync/tests/range.rs b/beacon_node/network/src/sync/tests/range.rs index 98c50088366..c114eca555f 100644 --- a/beacon_node/network/src/sync/tests/range.rs +++ b/beacon_node/network/src/sync/tests/range.rs @@ -449,14 +449,7 @@ fn build_rpc_block( RpcBlock::new(None, block, Some(blobs.clone())).unwrap() } Some(DataSidecars::DataColumns(columns)) => { - RpcBlock::new_with_custody_columns( - None, - block, - columns.clone(), - // TODO(das): Assumes CGC = max value. Change if we want to do more complex tests - spec, - ) - .unwrap() + RpcBlock::new_with_custody_columns(None, block, columns.clone(), spec).unwrap() } // Block has no data, expects zero columns None => RpcBlock::new_without_blobs(None, block), diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index b9d7968e070..eda57b7d8b7 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -339,7 +339,7 @@ pub enum DBColumn { BeaconRandaoMixes, #[strum(serialize = "dht")] DhtEnrs, - #[strum(serialize = "custody")] + #[strum(serialize = "cus")] CustodyContext, /// DEPRECATED. For Optimistically Imported Merge Transition Blocks #[strum(serialize = "otb")] diff --git a/scripts/local_testnet/network_params_das.yaml b/scripts/local_testnet/network_params_das.yaml index b16be34b89c..c896b11330a 100644 --- a/scripts/local_testnet/network_params_das.yaml +++ b/scripts/local_testnet/network_params_das.yaml @@ -2,7 +2,7 @@ participants: - cl_type: lighthouse cl_image: lighthouse:local el_type: geth - el_image: ethpandaops/geth:fusaka-devnet-0 + el_image: ethpandaops/geth:fusaka-devnet-1 cl_extra_params: - --subscribe-all-data-column-subnets - --subscribe-all-subnets @@ -13,7 +13,7 @@ participants: - cl_type: lighthouse cl_image: lighthouse:local el_type: geth - el_image: ethpandaops/geth:fusaka-devnet-0 + el_image: ethpandaops/geth:fusaka-devnet-1 cl_extra_params: # Note: useful for testing range sync (only produce block if the node is in sync to prevent forking) - --sync-tolerance-epochs=0 diff --git a/scripts/tests/checkpoint-sync-config-devnet.yaml b/scripts/tests/checkpoint-sync-config-devnet.yaml index c536d26b3b5..de3020a8847 100644 --- a/scripts/tests/checkpoint-sync-config-devnet.yaml +++ b/scripts/tests/checkpoint-sync-config-devnet.yaml @@ -3,18 +3,18 @@ participants: - cl_type: lighthouse cl_image: lighthouse:local el_type: geth - el_image: ethpandaops/geth:fusaka-devnet-0 + el_image: ethpandaops/geth:fusaka-devnet-1 supernode: true - cl_type: lighthouse cl_image: lighthouse:local el_type: geth - el_image: ethpandaops/geth:fusaka-devnet-0 + el_image: ethpandaops/geth:fusaka-devnet-1 supernode: false checkpoint_sync_enabled: true -checkpoint_sync_url: "https://checkpoint-sync.fusaka-devnet-0.ethpandaops.io" +checkpoint_sync_url: "https://checkpoint-sync.fusaka-devnet-1.ethpandaops.io" global_log_level: debug network_params: - network: fusaka-devnet-0 + network: fusaka-devnet-1 From 3d1a1ba10cc519e51cfce5d6bcefa22b75f8db96 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 17:08:59 +0200 Subject: [PATCH 24/27] Fix incorrect CGC initialisation on startup. --- .../beacon_chain/src/validator_custody.rs | 22 +++++++--------- beacon_node/http_api/src/test_utils.rs | 1 - .../lighthouse_network/src/discovery/mod.rs | 1 - .../lighthouse_network/src/service/mod.rs | 1 - .../lighthouse_network/src/types/globals.rs | 25 +++++++++++-------- .../src/network_beacon_processor/tests.rs | 1 - beacon_node/network/src/service.rs | 2 +- 7 files changed, 25 insertions(+), 28 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 5a2fea61bfc..96d8f51488c 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -177,7 +177,7 @@ impl CustodyContext { return None; }; - let current_cgc = self.custody_group_count(spec); + let current_cgc = self.custody_group_count_at_head(spec); let validator_custody_count_at_head = self.validator_custody_count.load(Ordering::Relaxed); if new_validator_custody != validator_custody_count_at_head { @@ -189,7 +189,7 @@ impl CustodyContext { self.validator_custody_count .store(new_validator_custody, Ordering::Relaxed); - let updated_cgc = self.custody_group_count(spec); + let updated_cgc = self.custody_group_count_at_head(spec); // Send the message to network only if there are more columns subnets to subscribe to if updated_cgc > current_cgc { tracing::debug!( @@ -207,14 +207,10 @@ impl CustodyContext { None } - /// The custody count that we use to custody columns currently. - /// - /// This function should be called when figuring out how many columns we - /// need to custody when receiving blocks over gossip/rpc or during sync. - /// - /// NOTE: this function is intended for internal calculation only and - /// should **NOT** be exposed externally. Use `self.sampling_size_at_epoch` instead. - fn custody_group_count(&self, spec: &ChainSpec) -> u64 { + /// This function is used to determine the custody group count at head ONLY. + /// Do NOT use this directly for data availability check, use `self.sampling_size` instead as + /// CGC can change over epochs. + pub fn custody_group_count_at_head(&self, spec: &ChainSpec) -> u64 { if self.current_is_supernode { return spec.number_of_custody_groups; } @@ -245,7 +241,7 @@ impl CustodyContext { .custody_requirement_at_epoch(epoch) .unwrap_or_else(|| self.default_custody_group_count(spec)) } else { - self.custody_group_count(spec) + self.custody_group_count_at_head(spec) }; spec.sampling_size(custody_group_count) .expect("should compute node sampling size from valid chain spec") @@ -288,7 +284,7 @@ mod tests { let custody_context = CustodyContext::new(true); let spec = E::default_spec(); assert_eq!( - custody_context.custody_group_count(&spec), + custody_context.custody_group_count_at_head(&spec), spec.number_of_custody_groups ); } @@ -298,7 +294,7 @@ mod tests { let custody_context = CustodyContext::new(false); let spec = E::default_spec(); assert_eq!( - custody_context.custody_group_count(&spec), + custody_context.custody_group_count_at_head(&spec), spec.custody_requirement, "head custody count should be minimum spec custody requirement" ); diff --git a/beacon_node/http_api/src/test_utils.rs b/beacon_node/http_api/src/test_utils.rs index 2340f7a7873..f78a361dad3 100644 --- a/beacon_node/http_api/src/test_utils.rs +++ b/beacon_node/http_api/src/test_utils.rs @@ -164,7 +164,6 @@ pub async fn create_api_server_with_config( vec![], false, network_config, - chain.spec.custody_requirement, chain.spec.clone(), )); diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index d8dac623c78..ad4241c5b71 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -1228,7 +1228,6 @@ mod tests { vec![], false, config.clone(), - spec.custody_requirement, spec.clone(), ); let keypair = keypair.into(); diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 4a02e12b07d..5f65a6c6d06 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -215,7 +215,6 @@ impl Network { trusted_peers, config.disable_peer_scoring, config.clone(), - custody_group_count, ctx.chain_spec.clone(), ); let network_globals = Arc::new(globals); diff --git a/beacon_node/lighthouse_network/src/types/globals.rs b/beacon_node/lighthouse_network/src/types/globals.rs index ca3e389cc91..d1ed1c33b07 100644 --- a/beacon_node/lighthouse_network/src/types/globals.rs +++ b/beacon_node/lighthouse_network/src/types/globals.rs @@ -7,6 +7,7 @@ use crate::{Client, Enr, EnrExt, GossipTopic, Multiaddr, NetworkConfig, PeerId}; use parking_lot::RwLock; use std::collections::HashSet; use std::sync::Arc; +use tracing::error; use types::data_column_custody_group::{ compute_columns_for_custody_group, compute_subnets_from_custody_group, get_custody_groups, }; @@ -45,11 +46,23 @@ impl NetworkGlobals { trusted_peers: Vec, disable_peer_scoring: bool, config: Arc, - custody_group_count: u64, spec: Arc, ) -> Self { let node_id = enr.node_id().raw(); + let custody_group_count = match local_metadata.custody_group_count() { + Ok(&cgc) if cgc <= spec.number_of_custody_groups => cgc, + _ => { + if spec.is_peer_das_scheduled() { + error!( + info = "falling back to default custody requirement", + "custody_group_count from metadata is either invalid or not set. This is a bug!" + ); + } + spec.custody_requirement + } + }; + // The below `expect` calls will panic on start up if the chain spec config values used // are invalid let sampling_size = spec @@ -268,15 +281,7 @@ impl NetworkGlobals { let keypair = libp2p::identity::secp256k1::Keypair::generate(); let enr_key: discv5::enr::CombinedKey = discv5::enr::CombinedKey::from_secp256k1(&keypair); let enr = discv5::enr::Enr::builder().build(&enr_key).unwrap(); - NetworkGlobals::new( - enr, - metadata, - trusted_peers, - false, - config, - spec.number_of_custody_groups, - spec, - ) + NetworkGlobals::new(enr, metadata, trusted_peers, false, config, spec) } } diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 9e5c96c2cf8..f6a1069a7f4 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -230,7 +230,6 @@ impl TestRig { vec![], false, network_config, - spec.custody_requirement, spec, )); diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index 73084e5c74c..c9f89ad6686 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -283,7 +283,7 @@ impl NetworkService { beacon_chain .data_availability_checker .custody_context() - .sampling_size(None, &beacon_chain.spec), + .custody_group_count_at_head(&beacon_chain.spec), ) .await?; From 356b01df16fa399207a700a3782f8c1402ed7697 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 17:12:11 +0200 Subject: [PATCH 25/27] Update fulu sync test config --- scripts/tests/genesis-sync-config-fulu.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/scripts/tests/genesis-sync-config-fulu.yaml b/scripts/tests/genesis-sync-config-fulu.yaml index ccdc09c0d3b..25ee90da0da 100644 --- a/scripts/tests/genesis-sync-config-fulu.yaml +++ b/scripts/tests/genesis-sync-config-fulu.yaml @@ -2,14 +2,20 @@ participants: - cl_type: lighthouse cl_image: lighthouse:local + el_type: geth + el_image: ethpandaops/geth:fusaka-devnet-1 count: 2 # nodes without validators, used for testing sync. - cl_type: lighthouse cl_image: lighthouse:local + el_type: geth + el_image: ethpandaops/geth:fusaka-devnet-1 supernode: true validator_count: 0 - cl_type: lighthouse cl_image: lighthouse:local + el_type: geth + el_image: ethpandaops/geth:fusaka-devnet-1 supernode: false validator_count: 0 network_params: From 6404952cf2e7cfed444d84c7c0e1c7da70961cb5 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 18:27:12 +0200 Subject: [PATCH 26/27] Fix incorrect sampling size computation. --- .../beacon_chain/src/validator_custody.rs | 35 +++++++++++-------- scripts/tests/genesis-sync-config-fulu.yaml | 3 +- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/validator_custody.rs b/beacon_node/beacon_chain/src/validator_custody.rs index 96d8f51488c..160333b50e2 100644 --- a/beacon_node/beacon_chain/src/validator_custody.rs +++ b/beacon_node/beacon_chain/src/validator_custody.rs @@ -224,25 +224,20 @@ impl CustodyContext { } } - fn default_custody_group_count(&self, spec: &ChainSpec) -> u64 { - if self.current_is_supernode { - spec.number_of_custody_groups - } else { - spec.custody_requirement - } - } - /// Returns the count of custody columns this node must sample for a block at `epoch` to import. /// If an `epoch` is not specified, returns the *current* validator custody requirement. pub fn sampling_size(&self, epoch_opt: Option, spec: &ChainSpec) -> u64 { - let custody_group_count = if let Some(epoch) = epoch_opt { + let custody_group_count = if self.current_is_supernode { + spec.number_of_custody_groups + } else if let Some(epoch) = epoch_opt { self.validator_registrations .read() .custody_requirement_at_epoch(epoch) - .unwrap_or_else(|| self.default_custody_group_count(spec)) + .unwrap_or(spec.custody_requirement) } else { self.custody_group_count_at_head(spec) }; + spec.sampling_size(custody_group_count) .expect("should compute node sampling size from valid chain spec") } @@ -287,6 +282,10 @@ mod tests { custody_context.custody_group_count_at_head(&spec), spec.number_of_custody_groups ); + assert_eq!( + custody_context.sampling_size(None, &spec), + spec.number_of_custody_groups + ); } #[test] @@ -298,6 +297,10 @@ mod tests { spec.custody_requirement, "head custody count should be minimum spec custody requirement" ); + assert_eq!( + custody_context.sampling_size(None, &spec), + spec.samples_per_slot + ); } #[test] @@ -318,7 +321,7 @@ mod tests { ]; register_validators_and_assert_cgc( - custody_context, + &custody_context, validators_and_expected_cgc_change, &spec, ); @@ -354,7 +357,7 @@ mod tests { ), ]; - register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); + register_validators_and_assert_cgc(&custody_context, validators_and_expected_cgc, &spec); } #[test] @@ -383,7 +386,11 @@ mod tests { ), ]; - register_validators_and_assert_cgc(custody_context, validators_and_expected_cgc, &spec); + register_validators_and_assert_cgc(&custody_context, validators_and_expected_cgc, &spec); + assert_eq!( + custody_context.sampling_size(None, &spec), + spec.number_of_custody_groups + ); } #[test] @@ -418,7 +425,7 @@ mod tests { /// Update validator every epoch and assert cgc against expected values. fn register_validators_and_assert_cgc( - custody_context: CustodyContext, + custody_context: &CustodyContext, validators_and_expected_cgc_changed: Vec<(ValidatorsAndBalances, Option)>, spec: &ChainSpec, ) { diff --git a/scripts/tests/genesis-sync-config-fulu.yaml b/scripts/tests/genesis-sync-config-fulu.yaml index 25ee90da0da..91aa4d1ffd1 100644 --- a/scripts/tests/genesis-sync-config-fulu.yaml +++ b/scripts/tests/genesis-sync-config-fulu.yaml @@ -20,7 +20,8 @@ participants: validator_count: 0 network_params: seconds_per_slot: 6 - fulu_fork_epoch: 0 + electra_fork_epoch: 0 + fulu_fork_epoch: 1 preset: "minimal" additional_services: - tx_fuzz From 8a39d3996353741b7fe3a69b41dc93c31de0903d Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 11 Jun 2025 19:21:03 +0200 Subject: [PATCH 27/27] Fix test harness to use correct sampling size. --- beacon_node/beacon_chain/src/test_utils.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index e4caea384c7..9aee862a04b 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -609,12 +609,6 @@ where let chain = builder.build().expect("should build"); - let sampling_column_count = if self.import_all_data_columns { - chain.spec.number_of_custody_groups as usize - } else { - chain.spec.custody_requirement as usize - }; - BeaconChainHarness { spec: chain.spec.clone(), chain: Arc::new(chain), @@ -625,7 +619,6 @@ where mock_execution_layer: self.mock_execution_layer, mock_builder: None, rng: make_rng(), - sampling_column_count, } } } @@ -682,7 +675,6 @@ pub struct BeaconChainHarness { pub mock_execution_layer: Option>, pub mock_builder: Option>>, - pub sampling_column_count: usize, pub rng: Mutex, } @@ -785,7 +777,10 @@ where } pub fn get_sampling_column_count(&self) -> usize { - self.sampling_column_count + self.chain + .data_availability_checker + .custody_context() + .sampling_size(None, &self.chain.spec) as usize } pub fn slots_per_epoch(&self) -> u64 {