From 25feedfde348b530c4fa2348cc71a06b746898ed Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 29 Aug 2024 16:11:19 -0700 Subject: [PATCH 01/31] First pass --- beacon_node/beacon_chain/src/beacon_chain.rs | 5 +- .../beacon_chain/src/blob_verification.rs | 10 ++- .../beacon_chain/src/block_verification.rs | 4 +- .../src/block_verification_types.rs | 17 ------ .../src/data_availability_checker.rs | 7 +-- .../overflow_lru_cache.rs | 47 ++++++++------ beacon_node/beacon_chain/src/kzg_utils.rs | 7 ++- .../src/observed_data_sidecars.rs | 14 +++-- beacon_node/beacon_chain/src/test_utils.rs | 10 +-- beacon_node/client/src/builder.rs | 3 +- .../test_utils/execution_block_generator.rs | 3 +- beacon_node/http_api/src/block_id.rs | 3 +- beacon_node/http_api/src/publish_blocks.rs | 8 ++- .../lighthouse_network/src/rpc/methods.rs | 14 ++++- .../lighthouse_network/src/rpc/protocol.rs | 6 +- .../src/sync/block_sidecar_coupling.rs | 16 +++-- beacon_node/network/src/sync/manager.rs | 2 + .../network/src/sync/network_context.rs | 34 +++++++++-- beacon_node/store/src/hot_cold_store.rs | 18 +++++- .../store/src/impls/execution_payload.rs | 3 +- .../src/per_block_processing.rs | 6 +- consensus/types/src/beacon_block_body.rs | 2 - consensus/types/src/blob_sidecar.rs | 33 +++++----- consensus/types/src/chain_spec.rs | 26 ++++++++ consensus/types/src/data_column_sidecar.rs | 14 ++--- consensus/types/src/eth_spec.rs | 12 +--- consensus/types/src/lib.rs | 4 +- consensus/types/src/preset.rs | 3 - consensus/types/src/runtime_var_list.rs | 61 +++++++++++++++++++ 29 files changed, 262 insertions(+), 130 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9d744003360..e1cf40f65f6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1249,7 +1249,10 @@ impl BeaconChain { pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { match self.store.get_blobs(block_root)? { Some(blobs) => Ok(blobs), - None => Ok(BlobSidecarList::default()), + None => Ok(BlobSidecarList::empty( + // TODO(pawan): fix this + self.spec.max_blobs_per_block(Epoch::new(0)) as usize, + )), } } diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 99fc5d9d0c0..4a39a9d2531 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -17,7 +17,8 @@ use std::time::Duration; use tree_hash::TreeHash; use types::blob_sidecar::BlobIdentifier; use types::{ - BeaconStateError, BlobSidecar, Epoch, EthSpec, Hash256, SignedBeaconBlockHeader, Slot, + BeaconStateError, BlobSidecar, Epoch, EthSpec, Hash256, RuntimeVariableList, + SignedBeaconBlockHeader, Slot, }; /// An error occurred while validating a gossip blob. @@ -172,10 +173,7 @@ impl From for GossipBlobError { } } -pub type GossipVerifiedBlobList = VariableList< - GossipVerifiedBlob, - <::EthSpec as EthSpec>::MaxBlobsPerBlock, ->; +pub type GossipVerifiedBlobList = RuntimeVariableList>; /// A wrapper around a `BlobSidecar` that indicates it has been approved for re-gossiping on /// the p2p network. @@ -399,7 +397,7 @@ pub fn validate_blob_sidecar_for_gossip( // since we only subscribe to `MaxBlobsPerBlock` subnets over gossip network. // We include this check only for completeness. // Getting this error would imply something very wrong with our networking decoding logic. - if blob_index >= T::EthSpec::max_blobs_per_block() as u64 { + if blob_index >= chain.spec.max_blobs_per_block(blob_epoch) { return Err(GossipBlobError::InvalidSubnet { expected: subnet, received: blob_index, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index d9662d59f9e..6582096befb 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -779,7 +779,9 @@ fn build_gossip_verified_blobs( GossipVerifiedBlob::new(Arc::new(blob), i as u64, chain)?; gossip_verified_blobs.push(gossip_verified_blob); } - let gossip_verified_blobs = VariableList::from(gossip_verified_blobs); + let max_len = chain.spec.max_blobs_per_block(block.epoch()) as usize; + let gossip_verified_blobs = + RuntimeVariableList::from_vec(gossip_verified_blobs, max_len); Ok::<_, BlockContentsError>(gossip_verified_blobs) }) .transpose() diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index b271f0a2f98..67643318f2c 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -181,23 +181,6 @@ impl RpcBlock { }) } - pub fn new_from_fixed( - block_root: Hash256, - block: Arc>, - blobs: FixedBlobSidecarList, - ) -> Result { - let filtered = blobs - .into_iter() - .filter_map(|b| b.clone()) - .collect::>(); - let blobs = if filtered.is_empty() { - None - } else { - Some(VariableList::from(filtered)) - }; - Self::new(Some(block_root), block, blobs) - } - #[allow(clippy::type_complexity)] pub fn deconstruct( self, diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 470cee713fa..cef7c528f6a 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -200,7 +200,7 @@ impl DataAvailabilityChecker { .ok_or(AvailabilityCheckError::SlotClockError)?; let verified_blobs = - KzgVerifiedBlobList::new(Vec::from(blobs).into_iter().flatten(), kzg, seen_timestamp) + KzgVerifiedBlobList::new(blobs.into_vec().into_iter().flatten(), kzg, seen_timestamp) .map_err(AvailabilityCheckError::Kzg)?; self.availability_cache @@ -384,14 +384,13 @@ impl DataAvailabilityChecker { blocks: Vec>, ) -> Result>, AvailabilityCheckError> { let mut results = Vec::with_capacity(blocks.len()); - let all_blobs: BlobSidecarList = blocks + let all_blobs = blocks .iter() .filter(|block| self.blobs_required_for_block(block.as_block())) // this clone is cheap as it's cloning an Arc .filter_map(|block| block.blobs().cloned()) .flatten() - .collect::>() - .into(); + .collect::>(); // verify kzg for all blobs at once if !all_blobs.is_empty() { 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 4863982b552..9c33051ae02 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 @@ -16,9 +16,10 @@ use std::collections::HashSet; use std::num::NonZeroUsize; use std::sync::Arc; use types::blob_sidecar::BlobIdentifier; +use types::runtime_var_list::RuntimeFixedList; use types::{ BlobSidecar, ChainSpec, ColumnIndex, DataColumnIdentifier, DataColumnSidecar, - DataColumnSidecarList, Epoch, EthSpec, Hash256, SignedBeaconBlock, + DataColumnSidecarList, Epoch, EthSpec, Hash256, RuntimeVariableList, SignedBeaconBlock, }; pub type DataColumnsToPublish = Option>; @@ -32,7 +33,7 @@ pub type DataColumnsToPublish = Option>; #[derive(Clone)] pub struct PendingComponents { pub block_root: Hash256, - pub verified_blobs: FixedVector>, E::MaxBlobsPerBlock>, + pub verified_blobs: RuntimeFixedList>>, pub verified_data_columns: Vec>, pub executed_block: Option>, pub reconstruction_started: bool, @@ -50,9 +51,7 @@ impl PendingComponents { } /// Returns an immutable reference to the fixed vector of cached blobs. - pub fn get_cached_blobs( - &self, - ) -> &FixedVector>, E::MaxBlobsPerBlock> { + pub fn get_cached_blobs(&self) -> &RuntimeFixedList>> { &self.verified_blobs } @@ -73,9 +72,7 @@ impl PendingComponents { } /// Returns a mutable reference to the fixed vector of cached blobs. - pub fn get_cached_blobs_mut( - &mut self, - ) -> &mut FixedVector>, E::MaxBlobsPerBlock> { + pub fn get_cached_blobs_mut(&mut self) -> &mut RuntimeFixedList>> { &mut self.verified_blobs } @@ -147,10 +144,7 @@ impl PendingComponents { /// Blobs are only inserted if: /// 1. The blob entry at the index is empty and no block exists. /// 2. The block exists and its commitment matches the blob's commitment. - pub fn merge_blobs( - &mut self, - blobs: FixedVector>, E::MaxBlobsPerBlock>, - ) { + pub fn merge_blobs(&mut self, blobs: RuntimeFixedList>>) { for (index, blob) in blobs.iter().cloned().enumerate() { let Some(blob) = blob else { continue }; self.merge_single_blob(index, blob); @@ -194,7 +188,7 @@ impl PendingComponents { /// Blobs that don't match the new block's commitments are evicted. pub fn merge_block(&mut self, block: DietAvailabilityPendingExecutedBlock) { self.insert_block(block); - let reinsert = std::mem::take(self.get_cached_blobs_mut()); + let reinsert = self.get_cached_blobs_mut().take(); self.merge_blobs(reinsert); } @@ -223,10 +217,11 @@ impl PendingComponents { } /// Returns an empty `PendingComponents` object with the given block root. - pub fn empty(block_root: Hash256) -> Self { + pub fn empty(block_root: Hash256, max_len: usize) -> Self { Self { block_root, - verified_blobs: FixedVector::default(), + // TODO(pawan): just make this a vec potentially + verified_blobs: RuntimeFixedList::new(vec![None; max_len]), verified_data_columns: vec![], executed_block: None, reconstruction_started: false, @@ -280,7 +275,12 @@ impl PendingComponents { else { return Err(AvailabilityCheckError::Unexpected); }; - (Some(VariableList::new(verified_blobs)?), None) + let max_len = + spec.max_blobs_per_block(diet_executed_block.as_block().epoch()) as usize; + ( + Some(RuntimeVariableList::new(verified_blobs, max_len)?), + None, + ) } BlockImportRequirement::CustodyColumns(_) => { let verified_data_columns = verified_data_columns @@ -477,7 +477,8 @@ impl DataAvailabilityCheckerInner { epoch: Epoch, kzg_verified_blobs: I, ) -> Result, AvailabilityCheckError> { - let mut fixed_blobs = FixedVector::default(); + let mut fixed_blobs = + RuntimeFixedList::new(vec![None; self.spec.max_blobs_per_block(epoch) as usize]); for blob in kzg_verified_blobs { if let Some(blob_opt) = fixed_blobs.get_mut(blob.blob_index() as usize) { @@ -491,7 +492,9 @@ impl DataAvailabilityCheckerInner { let mut pending_components = write_lock .pop_entry(&block_root) .map(|(_, v)| v) - .unwrap_or_else(|| PendingComponents::empty(block_root)); + .unwrap_or_else(|| { + PendingComponents::empty(block_root, self.spec.max_blobs_per_block(epoch) as usize) + }); // Merge in the blobs. pending_components.merge_blobs(fixed_blobs); @@ -527,7 +530,9 @@ impl DataAvailabilityCheckerInner { let mut pending_components = write_lock .pop_entry(&block_root) .map(|(_, v)| v) - .unwrap_or_else(|| PendingComponents::empty(block_root)); + .unwrap_or_else(|| { + PendingComponents::empty(block_root, self.spec.max_blobs_per_block(epoch) as usize) + }); // Merge in the data columns. pending_components.merge_data_columns(kzg_verified_data_columns)?; @@ -614,7 +619,9 @@ impl DataAvailabilityCheckerInner { let mut pending_components = write_lock .pop_entry(&block_root) .map(|(_, v)| v) - .unwrap_or_else(|| PendingComponents::empty(block_root)); + .unwrap_or_else(|| { + PendingComponents::empty(block_root, self.spec.max_blobs_per_block(epoch) as usize) + }); // Merge in the block. pending_components.merge_block(diet_executed_block); diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index 55c1ee9e980..db0847073ad 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -188,9 +188,10 @@ fn build_data_column_sidecars( spec: &ChainSpec, ) -> Result, String> { let number_of_columns = spec.number_of_columns; - let mut columns = vec![Vec::with_capacity(E::max_blobs_per_block()); number_of_columns]; - let mut column_kzg_proofs = - vec![Vec::with_capacity(E::max_blobs_per_block()); number_of_columns]; + let max_blobs_per_block = + spec.max_blobs_per_block(signed_block_header.message.slot.epoch(E::slots_per_epoch())) as usize; + let mut columns = vec![Vec::with_capacity(max_blobs_per_block); number_of_columns]; + let mut column_kzg_proofs = vec![Vec::with_capacity(max_blobs_per_block); number_of_columns]; for (blob_cells, blob_cell_proofs) in blob_cells_and_proofs_vec { // we iterate over each column, and we construct the column from "top to bottom", diff --git a/beacon_node/beacon_chain/src/observed_data_sidecars.rs b/beacon_node/beacon_chain/src/observed_data_sidecars.rs index 601241dd8ad..7cc5f2e92a1 100644 --- a/beacon_node/beacon_chain/src/observed_data_sidecars.rs +++ b/beacon_node/beacon_chain/src/observed_data_sidecars.rs @@ -23,7 +23,7 @@ pub trait ObservableDataSidecar { fn slot(&self) -> Slot; fn block_proposer_index(&self) -> u64; fn index(&self) -> u64; - fn max_num_of_items(spec: &ChainSpec) -> usize; + fn max_num_of_items(spec: &ChainSpec, slot: Slot) -> usize; } impl ObservableDataSidecar for BlobSidecar { @@ -39,8 +39,8 @@ impl ObservableDataSidecar for BlobSidecar { self.index } - fn max_num_of_items(_spec: &ChainSpec) -> usize { - E::max_blobs_per_block() + fn max_num_of_items(spec: &ChainSpec, slot: Slot) -> usize { + spec.max_blobs_per_block(slot.epoch(E::slots_per_epoch())) as usize } } @@ -57,7 +57,7 @@ impl ObservableDataSidecar for DataColumnSidecar { self.index } - fn max_num_of_items(spec: &ChainSpec) -> usize { + fn max_num_of_items(spec: &ChainSpec, _slot: Slot) -> usize { spec.number_of_columns } } @@ -102,7 +102,9 @@ impl ObservedDataSidecars { slot: data_sidecar.slot(), proposer: data_sidecar.block_proposer_index(), }) - .or_insert_with(|| HashSet::with_capacity(T::max_num_of_items(&self.spec))); + .or_insert_with(|| { + HashSet::with_capacity(T::max_num_of_items(&self.spec, data_sidecar.slot())) + }); let did_not_exist = data_indices.insert(data_sidecar.index()); Ok(!did_not_exist) @@ -122,7 +124,7 @@ impl ObservedDataSidecars { } fn sanitize_data_sidecar(&self, data_sidecar: &T) -> Result<(), Error> { - if data_sidecar.index() >= T::max_num_of_items(&self.spec) as u64 { + if data_sidecar.index() >= T::max_num_of_items(&self.spec, data_sidecar.slot()) as u64 { return Err(Error::InvalidDataIndex(data_sidecar.index())); } let finalized_slot = self.finalized_slot; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index b28d221da7e..cd2e355cafe 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1991,7 +1991,7 @@ where let (block, blob_items) = block_contents; let sidecars = blob_items - .map(|(proofs, blobs)| BlobSidecar::build_sidecars(blobs, &block, proofs)) + .map(|(proofs, blobs)| BlobSidecar::build_sidecars(blobs, &block, proofs, &self.spec)) .transpose() .unwrap(); let block_hash: SignedBeaconBlockHash = self @@ -2017,7 +2017,7 @@ where let (block, blob_items) = block_contents; let sidecars = blob_items - .map(|(proofs, blobs)| BlobSidecar::build_sidecars(blobs, &block, proofs)) + .map(|(proofs, blobs)| BlobSidecar::build_sidecars(blobs, &block, proofs, &self.spec)) .transpose() .unwrap(); let block_root = block.canonical_root(); @@ -2636,7 +2636,8 @@ pub fn generate_rand_block_and_blobs( // Get either zero blobs or a random number of blobs between 1 and Max Blobs. let payload: &mut FullPayloadDeneb = &mut message.body.execution_payload; let num_blobs = match num_blobs { - NumBlobs::Random => rng.gen_range(1..=E::max_blobs_per_block()), + // TODO(pawan): thread the chainspec value here + NumBlobs::Random => rng.gen_range(1..=6), NumBlobs::Number(n) => n, NumBlobs::None => 0, }; @@ -2656,7 +2657,8 @@ pub fn generate_rand_block_and_blobs( // Get either zero blobs or a random number of blobs between 1 and Max Blobs. let payload: &mut FullPayloadElectra = &mut message.body.execution_payload; let num_blobs = match num_blobs { - NumBlobs::Random => rng.gen_range(1..=E::max_blobs_per_block()), + // TODO(pawan): thread the chainspec value here + NumBlobs::Random => rng.gen_range(1..=6), NumBlobs::Number(n) => n, NumBlobs::None => 0, }; diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index d299eebec8e..aa50ede84a9 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -352,10 +352,11 @@ where let anchor_block = SignedBeaconBlock::from_ssz_bytes(&anchor_block_bytes, &spec) .map_err(|e| format!("Unable to parse weak subj block SSZ: {:?}", e))?; let anchor_blobs = if anchor_block.message().body().has_blobs() { + let max_blobs_len = spec.max_blobs_per_block(anchor_block.epoch()) as usize; let anchor_blobs_bytes = anchor_blobs_bytes .ok_or("Blobs for checkpoint must be provided using --checkpoint-blobs")?; Some( - BlobSidecarList::from_ssz_bytes(&anchor_blobs_bytes) + BlobSidecarList::from_ssz_bytes(&anchor_blobs_bytes, max_blobs_len) .map_err(|e| format!("Unable to parse weak subj blobs SSZ: {e:?}"))?, ) } else { diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index 216c3b7844f..a18ac711aa7 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -673,7 +673,8 @@ impl ExecutionBlockGenerator { ForkName::Deneb | ForkName::Electra => { // get random number between 0 and Max Blobs let mut rng = self.rng.lock(); - let num_blobs = rng.gen::() % (E::max_blobs_per_block() + 1); + // TODO(pawan): thread the chainspec value here somehow + let num_blobs = rng.gen::() % 6; let (bundle, transactions) = generate_blobs(num_blobs)?; for tx in Vec::from(transactions) { execution_payload diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index f35df2f5e86..e3ed15ebc22 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -279,13 +279,14 @@ impl BlockId { .get_blobs(&root) .map_err(warp_utils::reject::beacon_chain_error)?; + let max_len = blob_sidecar_list.max_len(); let blob_sidecar_list_filtered = match indices.indices { Some(vec) => { let list = blob_sidecar_list .into_iter() .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) .collect(); - BlobSidecarList::new(list) + BlobSidecarList::new(list, max_len) .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? } None => blob_sidecar_list, diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index bbdfc31d430..9f1d17677e9 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -22,7 +22,8 @@ use tree_hash::TreeHash; use types::{ AbstractExecPayload, BeaconBlockRef, BlobSidecarList, BlockImportSource, DataColumnSidecarList, DataColumnSubnetId, EthSpec, ExecPayload, ExecutionBlockHash, ForkName, FullPayload, - FullPayloadBellatrix, Hash256, SignedBeaconBlock, SignedBlindedBeaconBlock, VariableList, + FullPayloadBellatrix, Hash256, RuntimeVariableList, SignedBeaconBlock, + SignedBlindedBeaconBlock, VariableList, }; use warp::http::StatusCode; use warp::{reply::Response, Rejection, Reply}; @@ -198,7 +199,10 @@ pub async fn publish_block>(); - VariableList::from(blobs) + RuntimeVariableList::from_vec( + blobs, + chain.spec.max_blobs_per_block(block.epoch()) as usize, + ) }); let data_cols_opt = gossip_verified_data_columns .as_ref() diff --git a/beacon_node/lighthouse_network/src/rpc/methods.rs b/beacon_node/lighthouse_network/src/rpc/methods.rs index a96b9d1b166..0ff469aafbb 100644 --- a/beacon_node/lighthouse_network/src/rpc/methods.rs +++ b/beacon_node/lighthouse_network/src/rpc/methods.rs @@ -24,6 +24,13 @@ use types::{ pub type MaxErrorLen = U256; pub const MAX_ERROR_LEN: u64 = 256; +/// The max number of blobs we expect in the configs to set for compile time params. +/// Note: This value is an estimate that we should use only for rate limiting, +/// bounds checking and other non-consensus critical operations. +/// +/// For exact value, we should always check the chainspec. +pub const MAX_BLOBS_PER_BLOCK_CEILING: u64 = 16; + /// Wrapper over SSZ List to represent error message in rpc responses. #[derive(Debug, Clone)] pub struct ErrorType(pub VariableList); @@ -326,8 +333,13 @@ pub struct BlobsByRangeRequest { } impl BlobsByRangeRequest { + /// This function provides an upper bound on number of blobs expected in + /// a certain slot range. + /// + /// Note: **must not** use for anything consensus critical, only for + /// bounds checking and rate limiting. pub fn max_blobs_requested(&self) -> u64 { - self.count.saturating_mul(E::max_blobs_per_block() as u64) + self.count.saturating_mul(MAX_BLOBS_PER_BLOCK_CEILING as u64) } } diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index f4bdf6450b8..1b560f1d1e6 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -92,7 +92,7 @@ pub static SIGNED_BEACON_BLOCK_DENEB_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_CAPELLA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_deneb_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional offsets for the `ExecutionPayload` - + (::ssz_fixed_len() * ::max_blobs_per_block()) + + (::ssz_fixed_len() * MAX_BLOBS_PER_BLOCK_CEILING as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field. // @@ -100,7 +100,7 @@ pub static SIGNED_BEACON_BLOCK_ELECTRA_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_ELECTRA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_electra_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional ssz offset for the `ExecutionPayload` field - + (::ssz_fixed_len() * ::max_blobs_per_block()) + + (::ssz_fixed_len() * MAX_BLOBS_PER_BLOCK_CEILING as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field. @@ -636,7 +636,7 @@ pub fn rpc_blob_limits() -> RpcLimits { pub fn rpc_data_column_limits() -> RpcLimits { RpcLimits::new( DataColumnSidecar::::empty().as_ssz_bytes().len(), - DataColumnSidecar::::max_size(), + DataColumnSidecar::::max_size(MAX_BLOBS_PER_BLOCK_CEILING as usize), ) } diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 966ce55fabe..d642a2b4dce 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -8,7 +8,8 @@ use std::{ sync::Arc, }; use types::{ - BlobSidecar, ChainSpec, ColumnIndex, DataColumnSidecar, EthSpec, Hash256, SignedBeaconBlock, + BlobSidecar, ChainSpec, ColumnIndex, DataColumnSidecar, EthSpec, Hash256, RuntimeVariableList, + SignedBeaconBlock, }; #[derive(Debug)] @@ -31,6 +32,7 @@ pub struct RangeBlockComponentsRequest { num_custody_column_requests: Option, /// The peers the request was made to. pub(crate) peer_ids: Vec, + max_blobs_per_block: usize, } impl RangeBlockComponentsRequest { @@ -39,6 +41,7 @@ impl RangeBlockComponentsRequest { expects_custody_columns: Option>, num_custody_column_requests: Option, peer_ids: Vec, + max_blobs_per_block: usize, ) -> Self { Self { blocks: <_>::default(), @@ -51,6 +54,7 @@ impl RangeBlockComponentsRequest { expects_custody_columns, num_custody_column_requests, peer_ids, + max_blobs_per_block, } } @@ -100,7 +104,7 @@ impl RangeBlockComponentsRequest { let mut responses = Vec::with_capacity(blocks.len()); let mut blob_iter = blobs.into_iter().peekable(); for block in blocks.into_iter() { - let mut blob_list = Vec::with_capacity(E::max_blobs_per_block()); + let mut blob_list = Vec::with_capacity(self.max_blobs_per_block); while { let pair_next_blob = blob_iter .peek() @@ -111,7 +115,7 @@ impl RangeBlockComponentsRequest { blob_list.push(blob_iter.next().ok_or("Missing next blob".to_string())?); } - let mut blobs_buffer = vec![None; E::max_blobs_per_block()]; + let mut blobs_buffer = vec![None; self.max_blobs_per_block]; for blob in blob_list { let blob_index = blob.index as usize; let Some(blob_opt) = blobs_buffer.get_mut(blob_index) else { @@ -123,7 +127,11 @@ impl RangeBlockComponentsRequest { *blob_opt = Some(blob); } } - let blobs = VariableList::from(blobs_buffer.into_iter().flatten().collect::>()); + let blobs = RuntimeVariableList::new( + blobs_buffer.into_iter().flatten().collect::>(), + self.max_blobs_per_block, + ) + .map_err(|_| "Blobs returned exceeds max length".to_string())?; responses.push(RpcBlock::new(None, block, Some(blobs)).map_err(|e| format!("{e:?}"))?) } diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index d6ce14adb16..866268b6fd3 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -1120,6 +1120,7 @@ impl SyncManager { .network .range_block_and_blob_response(id, block_or_blob) { + let epoch = resp.sender_id.batch_id(); match resp.responses { Ok(blocks) => { match resp.sender_id { @@ -1163,6 +1164,7 @@ impl SyncManager { resp.expects_custody_columns, None, vec![], + self.chain.spec.max_blobs_per_block(epoch) as usize, ), ); // inform range that the request needs to be treated as failed diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 3da6f92cfed..8d954628eb1 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -36,8 +36,8 @@ use std::time::Duration; use tokio::sync::mpsc; use types::blob_sidecar::FixedBlobSidecarList; use types::{ - BlobSidecar, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, EthSpec, Hash256, - SignedBeaconBlock, Slot, + chain_spec, BlobSidecar, ChainSpec, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, + EthSpec, Hash256, SignedBeaconBlock, Slot, }; pub mod custody; @@ -61,6 +61,15 @@ pub enum RangeRequestId { }, } +impl RangeRequestId { + pub fn batch_id(&self) -> BatchId { + match self { + RangeRequestId::RangeSync { batch_id, .. } => *batch_id, + RangeRequestId::BackfillSync { batch_id, .. } => *batch_id, + } + } +} + #[derive(Debug)] pub enum RpcEvent { StreamTermination, @@ -422,11 +431,14 @@ impl SyncNetworkContext { (None, None) }; + // TODO(pawan): this would break if a batch contains multiple epochs + let max_blobs_len = self.chain.spec.max_blobs_per_block(epoch); let info = RangeBlockComponentsRequest::new( expected_blobs, expects_custody_columns, num_of_custody_column_req, requested_peers, + max_blobs_len as usize, ); self.range_block_components_requests .insert(id, (sender_id, info)); @@ -977,9 +989,16 @@ impl SyncNetworkContext { RpcEvent::Response(blob, seen_timestamp) => { let request = request.get_mut(); match request.add_response(blob) { - Ok(Some(blobs)) => to_fixed_blob_sidecar_list(blobs) - .map(|blobs| (blobs, seen_timestamp)) - .map_err(|e| (e.into(), request.resolve())), + Ok(Some(blobs)) => { + let max_len = if let Some(blob) = blobs.first() { + self.chain.spec.max_blobs_per_block(blob.epoch()) as usize + } else { + 6 + }; + to_fixed_blob_sidecar_list(blobs, max_len) + .map(|blobs| (blobs, seen_timestamp)) + .map_err(|e| (e.into(), request.resolve())) + } Ok(None) => return None, Err(e) => Err((e.into(), request.resolve())), } @@ -1218,8 +1237,11 @@ impl SyncNetworkContext { fn to_fixed_blob_sidecar_list( blobs: Vec>>, + max_len: usize, ) -> Result, LookupVerifyError> { - let mut fixed_list = FixedBlobSidecarList::default(); + // TODO(pawan): have a method on fixed runtime vector that just initializes a raw vec with max_len = None + // to signify an empty fixed runtime vector + let mut fixed_list = FixedBlobSidecarList::new(vec![None; max_len]); for blob in blobs.into_iter() { let index = blob.index as usize; *fixed_list diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index fecd8e37442..0addb619377 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -1669,7 +1669,23 @@ impl, Cold: ItemStore> HotColdDB .get_bytes(DBColumn::BeaconBlob.into(), block_root.as_bytes())? { Some(ref blobs_bytes) => { - let blobs = BlobSidecarList::from_ssz_bytes(blobs_bytes)?; + // We insert a VariableList of BlobSidecars into the db, but retrieve + // a plain vec since we don't know the length limit of the list without + // knowing the slot. + // The encoding of a VariableList is same as a regular vec. + let blobs = BlobSidecarVec::from_ssz_bytes(blobs_bytes)?; + let max_blobs_per_block = blobs + .first() + .map(|blob| { + self.spec + .max_blobs_per_block(blob.slot().epoch(E::slots_per_epoch())) + }) + // This is the case where we have no blobs for the slot, doesn't matter what value we keep for max here + // TODO(pawan): double check that this is the case + // we could also potentially deal with just vecs in the db since we only add length validated sidecar + // lists to the db + .unwrap_or(6); + let blobs = BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize); self.block_cache .lock() .put_blobs(*block_root, blobs.clone()); diff --git a/beacon_node/store/src/impls/execution_payload.rs b/beacon_node/store/src/impls/execution_payload.rs index 14fc10ad6de..3f7c4172bc9 100644 --- a/beacon_node/store/src/impls/execution_payload.rs +++ b/beacon_node/store/src/impls/execution_payload.rs @@ -1,7 +1,7 @@ use crate::{DBColumn, Error, StoreItem}; use ssz::{Decode, Encode}; use types::{ - BlobSidecarList, EthSpec, ExecutionPayload, ExecutionPayloadBellatrix, ExecutionPayloadCapella, + EthSpec, ExecutionPayload, ExecutionPayloadBellatrix, ExecutionPayloadCapella, ExecutionPayloadDeneb, ExecutionPayloadElectra, }; @@ -26,7 +26,6 @@ impl_store_item!(ExecutionPayloadBellatrix); impl_store_item!(ExecutionPayloadCapella); impl_store_item!(ExecutionPayloadDeneb); impl_store_item!(ExecutionPayloadElectra); -impl_store_item!(BlobSidecarList); /// This fork-agnostic implementation should be only used for writing. /// diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index e7655b453a8..7b2ca900556 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -391,10 +391,12 @@ pub fn partially_verify_execution_payload = VariableList::MaxBlobCommitmentsPerBlock>; -pub type KzgCommitmentOpts = - FixedVector, ::MaxBlobsPerBlock>; /// The number of leaves (including padding) on the `BeaconBlockBody` Merkle tree. /// diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 0f7dbb2673c..c2981e14cc2 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -1,10 +1,13 @@ use crate::test_utils::TestRandom; -use crate::ForkName; use crate::{ beacon_block_body::BLOB_KZG_COMMITMENTS_INDEX, BeaconBlockHeader, BeaconStateError, Blob, Epoch, EthSpec, FixedVector, Hash256, SignedBeaconBlockHeader, Slot, VariableList, }; -use crate::{ForkVersionDeserialize, KzgProofs, SignedBeaconBlock}; +use crate::{ + runtime_var_list::RuntimeFixedList, ForkVersionDeserialize, KzgProofs, RuntimeVariableList, + SignedBeaconBlock, +}; +use crate::{ChainSpec, ForkName}; use bls::Signature; use derivative::Derivative; use kzg::{Blob as KzgBlob, Kzg, KzgCommitment, KzgProof, BYTES_PER_BLOB, BYTES_PER_FIELD_ELEMENT}; @@ -30,19 +33,6 @@ pub struct BlobIdentifier { pub index: u64, } -impl BlobIdentifier { - pub fn get_all_blob_ids(block_root: Hash256) -> Vec { - let mut blob_ids = Vec::with_capacity(E::max_blobs_per_block()); - for i in 0..E::max_blobs_per_block() { - blob_ids.push(BlobIdentifier { - block_root, - index: i as u64, - }); - } - blob_ids - } -} - impl PartialOrd for BlobIdentifier { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -260,19 +250,24 @@ impl BlobSidecar { blobs: BlobsList, block: &SignedBeaconBlock, kzg_proofs: KzgProofs, + spec: &ChainSpec, ) -> Result, BlobSidecarError> { let mut blob_sidecars = vec![]; for (i, (kzg_proof, blob)) in kzg_proofs.iter().zip(blobs).enumerate() { let blob_sidecar = BlobSidecar::new(i, blob, block, *kzg_proof)?; blob_sidecars.push(Arc::new(blob_sidecar)); } - Ok(VariableList::from(blob_sidecars)) + Ok(RuntimeVariableList::from_vec( + blob_sidecars, + spec.max_blobs_per_block(block.epoch()) as usize, + )) } } -pub type BlobSidecarList = VariableList>, ::MaxBlobsPerBlock>; -pub type FixedBlobSidecarList = - FixedVector>>, ::MaxBlobsPerBlock>; +pub type BlobSidecarList = RuntimeVariableList>>; +/// Alias for a non length-constrained list of `BlobSidecar`s. +pub type BlobSidecarVec = Vec>>; +pub type FixedBlobSidecarList = RuntimeFixedList>>>; pub type BlobsList = VariableList, ::MaxBlobCommitmentsPerBlock>; impl ForkVersionDeserialize for BlobSidecarList { diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 10b00d5ba1d..5af0f349de3 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -230,6 +230,7 @@ pub struct ChainSpec { pub max_request_data_column_sidecars: u64, pub min_epochs_for_blob_sidecars_requests: u64, pub blob_sidecar_subnet_count: u64, + max_blobs_per_block: u64, /* * Networking Derived @@ -607,6 +608,16 @@ impl ChainSpec { } } + /// Returns the deneb preset value if peerdas epoch hasn't hit. + /// Otherwise, returns the value obtained from the config.yaml. + pub fn max_blobs_per_block(&self, epoch: Epoch) -> u64 { + if self.is_peer_das_enabled_for_epoch(epoch) { + self.max_blobs_per_block + } else { + default_max_blobs_per_block() + } + } + pub fn data_columns_per_subnet(&self) -> usize { self.number_of_columns .safe_div(self.data_column_sidecar_subnet_count as usize) @@ -843,6 +854,7 @@ impl ChainSpec { max_request_data_column_sidecars: default_max_request_data_column_sidecars(), min_epochs_for_blob_sidecars_requests: default_min_epochs_for_blob_sidecars_requests(), blob_sidecar_subnet_count: default_blob_sidecar_subnet_count(), + max_blobs_per_block: default_max_blobs_per_block(), /* * Derived Deneb Specific @@ -1164,6 +1176,8 @@ impl ChainSpec { max_request_data_column_sidecars: default_max_request_data_column_sidecars(), min_epochs_for_blob_sidecars_requests: 16384, blob_sidecar_subnet_count: default_blob_sidecar_subnet_count(), + // TODO(pawan): check if gnosis preset values match + max_blobs_per_block: default_max_blobs_per_block(), /* * Derived Deneb Specific @@ -1364,6 +1378,9 @@ pub struct Config { #[serde(default = "default_blob_sidecar_subnet_count")] #[serde(with = "serde_utils::quoted_u64")] blob_sidecar_subnet_count: u64, + #[serde(default = "default_max_blobs_per_block")] + #[serde(with = "serde_utils::quoted_u64")] + max_blobs_per_block: u64, #[serde(default = "default_min_per_epoch_churn_limit_electra")] #[serde(with = "serde_utils::quoted_u64")] @@ -1499,6 +1516,12 @@ const fn default_blob_sidecar_subnet_count() -> u64 { 6 } +/// Its important to keep this consistent with the deneb preset value for +/// `MAX_BLOBS_PER_BLOCK` else we might run into consensus issues. +const fn default_max_blobs_per_block() -> u64 { + 6 +} + const fn default_min_per_epoch_churn_limit_electra() -> u64 { 128_000_000_000 } @@ -1718,6 +1741,7 @@ impl Config { max_request_data_column_sidecars: spec.max_request_data_column_sidecars, min_epochs_for_blob_sidecars_requests: spec.min_epochs_for_blob_sidecars_requests, blob_sidecar_subnet_count: spec.blob_sidecar_subnet_count, + max_blobs_per_block: spec.max_blobs_per_block, min_per_epoch_churn_limit_electra: spec.min_per_epoch_churn_limit_electra, max_per_epoch_activation_exit_churn_limit: spec @@ -1795,6 +1819,7 @@ impl Config { max_request_data_column_sidecars, min_epochs_for_blob_sidecars_requests, blob_sidecar_subnet_count, + max_blobs_per_block, min_per_epoch_churn_limit_electra, max_per_epoch_activation_exit_churn_limit, @@ -1863,6 +1888,7 @@ impl Config { max_request_data_column_sidecars, min_epochs_for_blob_sidecars_requests, blob_sidecar_subnet_count, + max_blobs_per_block, min_per_epoch_churn_limit_electra, max_per_epoch_activation_exit_churn_limit, diff --git a/consensus/types/src/data_column_sidecar.rs b/consensus/types/src/data_column_sidecar.rs index 90c05aea1f7..ea0fb2b14a7 100644 --- a/consensus/types/src/data_column_sidecar.rs +++ b/consensus/types/src/data_column_sidecar.rs @@ -1,7 +1,7 @@ use crate::beacon_block_body::{KzgCommitments, BLOB_KZG_COMMITMENTS_INDEX}; use crate::test_utils::TestRandom; -use crate::BeaconStateError; -use crate::{BeaconBlockHeader, EthSpec, Hash256, KzgProofs, SignedBeaconBlockHeader, Slot}; +use crate::{BeaconBlockHeader, Epoch, EthSpec, Hash256, KzgProofs, SignedBeaconBlockHeader, Slot}; +use crate::{BeaconStateError, ChainSpec}; use bls::Signature; use derivative::Derivative; use kzg::Error as KzgError; @@ -110,18 +110,16 @@ impl DataColumnSidecar { .len() } - pub fn max_size() -> usize { + pub fn max_size(max_blobs_per_block: usize) -> usize { Self { index: 0, - column: VariableList::new(vec![Cell::::default(); E::MaxBlobsPerBlock::to_usize()]) - .unwrap(), + column: VariableList::new(vec![Cell::::default(); max_blobs_per_block]).unwrap(), kzg_commitments: VariableList::new(vec![ KzgCommitment::empty_for_testing(); - E::MaxBlobsPerBlock::to_usize() + max_blobs_per_block ]) .unwrap(), - kzg_proofs: VariableList::new(vec![KzgProof::empty(); E::MaxBlobsPerBlock::to_usize()]) - .unwrap(), + kzg_proofs: VariableList::new(vec![KzgProof::empty(); max_blobs_per_block]).unwrap(), signed_block_header: SignedBeaconBlockHeader { message: BeaconBlockHeader::empty(), signature: Signature::empty(), diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 09ef8e3c1a7..3d496d214eb 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -4,8 +4,7 @@ use safe_arith::SafeArith; use serde::{Deserialize, Serialize}; use ssz_types::typenum::{ bit::B0, UInt, U0, U1, U1024, U1048576, U1073741824, U1099511627776, U128, U131072, U134217728, - U16, U16777216, U2, U2048, U256, U262144, U32, U4, U4096, U512, U6, U625, U64, U65536, U8, - U8192, + U16, U16777216, U2, U2048, U256, U262144, U32, U4, U4096, U512, U625, U64, U65536, U8, U8192, }; use ssz_types::typenum::{U17, U9}; use std::fmt::{self, Debug}; @@ -109,7 +108,6 @@ pub trait EthSpec: /* * New in Deneb */ - type MaxBlobsPerBlock: Unsigned + Clone + Sync + Send + Debug + PartialEq + Unpin; type MaxBlobCommitmentsPerBlock: Unsigned + Clone + Sync + Send + Debug + PartialEq + Unpin; type FieldElementsPerBlob: Unsigned + Clone + Sync + Send + Debug + PartialEq; type BytesPerFieldElement: Unsigned + Clone + Sync + Send + Debug + PartialEq; @@ -280,11 +278,6 @@ pub trait EthSpec: Self::MaxWithdrawalsPerPayload::to_usize() } - /// Returns the `MAX_BLOBS_PER_BLOCK` constant for this specification. - fn max_blobs_per_block() -> usize { - Self::MaxBlobsPerBlock::to_usize() - } - /// Returns the `MAX_BLOB_COMMITMENTS_PER_BLOCK` constant for this specification. fn max_blob_commitments_per_block() -> usize { Self::MaxBlobCommitmentsPerBlock::to_usize() @@ -415,7 +408,6 @@ impl EthSpec for MainnetEthSpec { type GasLimitDenominator = U1024; type MinGasLimit = U5000; type MaxExtraDataBytes = U32; - type MaxBlobsPerBlock = U6; type MaxBlobCommitmentsPerBlock = U4096; type BytesPerFieldElement = U32; type FieldElementsPerBlob = U4096; @@ -498,7 +490,6 @@ impl EthSpec for MinimalEthSpec { MinGasLimit, MaxExtraDataBytes, MaxBlsToExecutionChanges, - MaxBlobsPerBlock, BytesPerFieldElement, PendingBalanceDepositsLimit, MaxConsolidationRequestsPerPayload, @@ -551,7 +542,6 @@ impl EthSpec for GnosisEthSpec { type SlotsPerEth1VotingPeriod = U1024; // 64 epochs * 16 slots per epoch type MaxBlsToExecutionChanges = U16; type MaxWithdrawalsPerPayload = U8; - type MaxBlobsPerBlock = U6; type MaxBlobCommitmentsPerBlock = U4096; type FieldElementsPerBlob = U4096; type BytesPerFieldElement = U32; diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 68d48ec7c8b..4db50c30e13 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -138,7 +138,9 @@ pub use crate::beacon_block_body::{ pub use crate::beacon_block_header::BeaconBlockHeader; pub use crate::beacon_committee::{BeaconCommittee, OwnedBeaconCommittee}; pub use crate::beacon_state::{Error as BeaconStateError, *}; -pub use crate::blob_sidecar::{BlobIdentifier, BlobSidecar, BlobSidecarList, BlobsList}; +pub use crate::blob_sidecar::{ + BlobIdentifier, BlobSidecar, BlobSidecarList, BlobSidecarVec, BlobsList, +}; pub use crate::bls_to_execution_change::BlsToExecutionChange; pub use crate::chain_spec::{ChainSpec, Config, Domain}; pub use crate::checkpoint::Checkpoint; diff --git a/consensus/types/src/preset.rs b/consensus/types/src/preset.rs index 2c576ed332c..0d067d3fe95 100644 --- a/consensus/types/src/preset.rs +++ b/consensus/types/src/preset.rs @@ -208,8 +208,6 @@ impl CapellaPreset { #[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] #[serde(rename_all = "UPPERCASE")] pub struct DenebPreset { - #[serde(with = "serde_utils::quoted_u64")] - pub max_blobs_per_block: u64, #[serde(with = "serde_utils::quoted_u64")] pub max_blob_commitments_per_block: u64, #[serde(with = "serde_utils::quoted_u64")] @@ -219,7 +217,6 @@ pub struct DenebPreset { impl DenebPreset { pub fn from_chain_spec(_spec: &ChainSpec) -> Self { Self { - max_blobs_per_block: E::max_blobs_per_block() as u64, max_blob_commitments_per_block: E::max_blob_commitments_per_block() as u64, field_elements_per_blob: E::field_elements_per_blob() as u64, } diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index af4ee87c158..4c6d6e2d927 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -214,6 +214,67 @@ where } } +#[derive(Clone, Debug)] +pub struct RuntimeFixedList { + vec: Vec, + len: usize, +} + +impl RuntimeFixedList { + // TODO(pawan): no need to take len + pub fn new(vec: Vec) -> Self { + let len = vec.len(); + Self { vec, len } + } + + pub fn to_vec(&self) -> Vec { + self.vec.clone() + } + + pub fn as_slice(&self) -> &[T] { + self.vec.as_slice() + } + + pub fn len(&self) -> usize { + self.len + } + + pub fn into_vec(self) -> Vec { + self.vec + } + + pub fn take(&mut self) -> Self { + let new = std::mem::take(&mut self.vec); + Self { + vec: new, + len: self.len, + } + } +} + +impl std::ops::Deref for RuntimeFixedList { + type Target = [T]; + + fn deref(&self) -> &[T] { + &self.vec[..] + } +} + +impl std::ops::DerefMut for RuntimeFixedList { + fn deref_mut(&mut self) -> &mut [T] { + &mut self.vec[..] + } +} + +impl<'a, T> IntoIterator for &'a RuntimeFixedList { + type Item = &'a T; + type IntoIter = std::slice::Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.vec.iter() + } +} + #[cfg(test)] mod test { use super::*; From 4dc6e6515ecf75cefa4de840edc7b57e76a8fc9e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 30 Aug 2024 15:53:18 -0700 Subject: [PATCH 02/31] Add restrictions to RuntimeVariableList api --- consensus/types/src/blob_sidecar.rs | 1 - consensus/types/src/data_column_sidecar.rs | 5 +- consensus/types/src/runtime_var_list.rs | 73 +++++++++++++++++----- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index c2981e14cc2..108c807b475 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -266,7 +266,6 @@ impl BlobSidecar { pub type BlobSidecarList = RuntimeVariableList>>; /// Alias for a non length-constrained list of `BlobSidecar`s. -pub type BlobSidecarVec = Vec>>; pub type FixedBlobSidecarList = RuntimeFixedList>>>; pub type BlobsList = VariableList, ::MaxBlobCommitmentsPerBlock>; diff --git a/consensus/types/src/data_column_sidecar.rs b/consensus/types/src/data_column_sidecar.rs index ea0fb2b14a7..34bdc33886a 100644 --- a/consensus/types/src/data_column_sidecar.rs +++ b/consensus/types/src/data_column_sidecar.rs @@ -1,7 +1,7 @@ use crate::beacon_block_body::{KzgCommitments, BLOB_KZG_COMMITMENTS_INDEX}; use crate::test_utils::TestRandom; -use crate::{BeaconBlockHeader, Epoch, EthSpec, Hash256, KzgProofs, SignedBeaconBlockHeader, Slot}; -use crate::{BeaconStateError, ChainSpec}; +use crate::BeaconStateError; +use crate::{BeaconBlockHeader, EthSpec, Hash256, KzgProofs, SignedBeaconBlockHeader, Slot}; use bls::Signature; use derivative::Derivative; use kzg::Error as KzgError; @@ -11,7 +11,6 @@ use safe_arith::ArithError; use serde::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; -use ssz_types::typenum::Unsigned; use ssz_types::Error as SszError; use ssz_types::{FixedVector, VariableList}; use std::hash::Hash; diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 4c6d6e2d927..aa43e3fe759 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -2,7 +2,7 @@ use derivative::Derivative; use serde::{Deserialize, Serialize}; use ssz::Decode; use ssz_types::Error; -use std::ops::{Deref, DerefMut, Index, IndexMut}; +use std::ops::{Deref, Index, IndexMut}; use std::slice::SliceIndex; /// Emulates a SSZ `List`. @@ -41,8 +41,10 @@ use std::slice::SliceIndex; #[serde(transparent)] pub struct RuntimeVariableList { vec: Vec, + /// A `None` here indicates an uninitialized `Self`. + /// No mutating operation will be allowed until `max_len` is Some #[serde(skip)] - max_len: usize, + max_len: Option, } impl RuntimeVariableList { @@ -50,7 +52,10 @@ impl RuntimeVariableList { /// `Err(OutOfBounds { .. })`. pub fn new(vec: Vec, max_len: usize) -> Result { if vec.len() <= max_len { - Ok(Self { vec, max_len }) + Ok(Self { + vec, + max_len: Some(max_len), + }) } else { Err(Error::OutOfBounds { i: vec.len(), @@ -62,14 +67,17 @@ impl RuntimeVariableList { pub fn from_vec(mut vec: Vec, max_len: usize) -> Self { vec.truncate(max_len); - Self { vec, max_len } + Self { + vec, + max_len: Some(max_len), + } } /// Create an empty list. pub fn empty(max_len: usize) -> Self { Self { vec: vec![], - max_len, + max_len: Some(max_len), } } @@ -77,6 +85,28 @@ impl RuntimeVariableList { self.vec.as_slice() } + pub fn as_mut_slice(&mut self) -> Option<&mut [T]> { + if self.max_len.is_none() { + return None; + }; + Some(self.vec.as_mut_slice()) + } + + /// Returns an instance of `Self` with max_len = None. + /// + /// No mutating operation can be performed on an uninitialized instance + /// without first setting max_len. + pub fn empty_uninitialized() -> Self { + Self { + vec: vec![], + max_len: None, + } + } + + pub fn set_max_len(&mut self, max_len: usize) { + self.max_len = Some(max_len); + } + /// Returns the number of values presently in `self`. pub fn len(&self) -> usize { self.vec.len() @@ -88,7 +118,9 @@ impl RuntimeVariableList { } /// Returns the type-level maximum length. - pub fn max_len(&self) -> usize { + /// + /// Returns `None` if self is uninitialized with a max_len. + pub fn max_len(&self) -> Option { self.max_len } @@ -96,13 +128,17 @@ impl RuntimeVariableList { /// /// Returns `Err(())` when appending `value` would exceed the maximum length. pub fn push(&mut self, value: T) -> Result<(), Error> { - if self.vec.len() < self.max_len { + let Some(max_len) = self.max_len else { + // TODO(pawan): set a better error + return Err(Error::MissingLengthInformation); + }; + if self.vec.len() < max_len { self.vec.push(value); Ok(()) } else { Err(Error::OutOfBounds { i: self.vec.len().saturating_add(1), - len: self.max_len, + len: max_len, }) } } @@ -135,7 +171,10 @@ impl RuntimeVariableList { } else { ssz::decode_list_of_variable_length_items(bytes, Some(max_len))? }; - Ok(Self { vec, max_len }) + Ok(Self { + vec, + max_len: Some(max_len), + }) } } @@ -169,12 +208,6 @@ impl Deref for RuntimeVariableList { } } -impl DerefMut for RuntimeVariableList { - fn deref_mut(&mut self) -> &mut [T] { - &mut self.vec[..] - } -} - impl<'a, T> IntoIterator for &'a RuntimeVariableList { type Item = &'a T; type IntoIter = std::slice::Iter<'a, T>; @@ -221,7 +254,6 @@ pub struct RuntimeFixedList { } impl RuntimeFixedList { - // TODO(pawan): no need to take len pub fn new(vec: Vec) -> Self { let len = vec.len(); Self { vec, len } @@ -280,6 +312,7 @@ mod test { use super::*; use ssz::*; use std::fmt::Debug; + use tree_hash::TreeHash; #[test] fn new() { @@ -358,4 +391,12 @@ mod test { round_trip::(RuntimeVariableList::from_vec(vec![42; 8], 8)); round_trip::(RuntimeVariableList::from_vec(vec![0; 8], 8)); } + + #[test] + fn test_empty_list_encoding() { + use ssz_types::{typenum::U16, VariableList}; + + let a: RuntimeVariableList = RuntimeVariableList::from_vec(vec![], 16); + dbg!(a.tree_hash_root()); + } } From a9cb329a221a809f7dd818984753826f91c2e26b Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 30 Aug 2024 15:54:00 -0700 Subject: [PATCH 03/31] Use empty_uninitialized and fix warnings --- .../beacon_chain/src/blob_verification.rs | 1 - .../beacon_chain/src/block_verification.rs | 1 - .../src/block_verification_types.rs | 3 +-- .../overflow_lru_cache.rs | 1 - beacon_node/client/src/builder.rs | 1 - beacon_node/http_api/src/block_id.rs | 14 +++++++++---- beacon_node/http_api/src/publish_blocks.rs | 2 +- .../src/sync/block_sidecar_coupling.rs | 1 - .../network/src/sync/network_context.rs | 4 ++-- beacon_node/store/src/hot_cold_store.rs | 21 ++++++++----------- consensus/types/src/lib.rs | 4 +--- 11 files changed, 24 insertions(+), 29 deletions(-) diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 4a39a9d2531..ceeb32427e6 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -12,7 +12,6 @@ use crate::{metrics, BeaconChainError}; use kzg::{Error as KzgError, Kzg, KzgCommitment}; use slog::debug; use ssz_derive::{Decode, Encode}; -use ssz_types::VariableList; use std::time::Duration; use tree_hash::TreeHash; use types::blob_sidecar::BlobIdentifier; diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 6582096befb..ef3e3363364 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -81,7 +81,6 @@ use slog::{debug, error, warn, Logger}; use slot_clock::SlotClock; use ssz::Encode; use ssz_derive::{Decode, Encode}; -use ssz_types::VariableList; use state_processing::per_block_processing::{errors::IntoWithIndex, is_merge_transition_block}; use state_processing::{ block_signature_verifier::{BlockSignatureVerifier, Error as BlockSignatureVerifierError}, diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index 67643318f2c..00442ff9668 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -8,11 +8,10 @@ use crate::data_column_verification::{ use crate::eth1_finalization_cache::Eth1FinalizationData; use crate::{get_block_root, GossipVerifiedBlock, PayloadVerificationOutcome}; use derivative::Derivative; -use ssz_types::VariableList; use state_processing::ConsensusContext; use std::fmt::{Debug, Formatter}; use std::sync::Arc; -use types::blob_sidecar::{self, BlobIdentifier, FixedBlobSidecarList}; +use types::blob_sidecar::{self, BlobIdentifier}; use types::data_column_sidecar::{self}; use types::{ BeaconBlockRef, BeaconState, BlindedPayload, BlobSidecarList, ChainSpec, Epoch, EthSpec, 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 9c33051ae02..2ff64dac97d 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 @@ -11,7 +11,6 @@ use crate::BeaconChainTypes; use kzg::Kzg; use lru::LruCache; use parking_lot::RwLock; -use ssz_types::{FixedVector, VariableList}; use std::collections::HashSet; use std::num::NonZeroUsize; use std::sync::Arc; diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index aa50ede84a9..0d1aba8b4d1 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -37,7 +37,6 @@ use network::{NetworkConfig, NetworkSenders, NetworkService}; use slasher::Slasher; use slasher_service::SlasherService; use slog::{debug, info, warn, Logger}; -use ssz::Decode; use std::net::TcpListener; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index e3ed15ebc22..63d00edbff6 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -279,15 +279,21 @@ impl BlockId { .get_blobs(&root) .map_err(warp_utils::reject::beacon_chain_error)?; - let max_len = blob_sidecar_list.max_len(); let blob_sidecar_list_filtered = match indices.indices { Some(vec) => { - let list = blob_sidecar_list + let list: Vec<_> = blob_sidecar_list .into_iter() .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) .collect(); - BlobSidecarList::new(list, max_len) - .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? + if let Some(max_len) = list + .first() + .map(|sidecar| chain.spec.max_blobs_per_block(sidecar.epoch())) + { + BlobSidecarList::new(list, max_len as usize) + .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? + } else { + BlobSidecarList::empty_uninitialized() + } } None => blob_sidecar_list, }; diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index 9f1d17677e9..a53bb7340df 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -23,7 +23,7 @@ use types::{ AbstractExecPayload, BeaconBlockRef, BlobSidecarList, BlockImportSource, DataColumnSidecarList, DataColumnSubnetId, EthSpec, ExecPayload, ExecutionBlockHash, ForkName, FullPayload, FullPayloadBellatrix, Hash256, RuntimeVariableList, SignedBeaconBlock, - SignedBlindedBeaconBlock, VariableList, + SignedBlindedBeaconBlock, }; use warp::http::StatusCode; use warp::{reply::Response, Rejection, Reply}; diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index d642a2b4dce..f241a52c9c8 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -2,7 +2,6 @@ use beacon_chain::{ block_verification_types::RpcBlock, data_column_verification::CustodyDataColumn, get_block_root, }; use lighthouse_network::PeerId; -use ssz_types::VariableList; use std::{ collections::{HashMap, VecDeque}, sync::Arc, diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 8d954628eb1..bcfc9a7da22 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -36,8 +36,8 @@ use std::time::Duration; use tokio::sync::mpsc; use types::blob_sidecar::FixedBlobSidecarList; use types::{ - chain_spec, BlobSidecar, ChainSpec, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, - EthSpec, Hash256, SignedBeaconBlock, Slot, + BlobSidecar, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, EthSpec, + Hash256, SignedBeaconBlock, Slot, }; pub mod custody; diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 0addb619377..13f672d2144 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -1673,19 +1673,16 @@ impl, Cold: ItemStore> HotColdDB // a plain vec since we don't know the length limit of the list without // knowing the slot. // The encoding of a VariableList is same as a regular vec. - let blobs = BlobSidecarVec::from_ssz_bytes(blobs_bytes)?; - let max_blobs_per_block = blobs + let blobs: Vec>> = Vec::<_>::from_ssz_bytes(blobs_bytes)?; + let blobs = if let Some(max_blobs_per_block) = blobs .first() - .map(|blob| { - self.spec - .max_blobs_per_block(blob.slot().epoch(E::slots_per_epoch())) - }) - // This is the case where we have no blobs for the slot, doesn't matter what value we keep for max here - // TODO(pawan): double check that this is the case - // we could also potentially deal with just vecs in the db since we only add length validated sidecar - // lists to the db - .unwrap_or(6); - let blobs = BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize); + .map(|blob| self.spec.max_blobs_per_block(blob.epoch())) + { + BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize) + } else { + // This always implies that there were no blobs for this block_root + BlobSidecarList::empty_uninitialized() + }; self.block_cache .lock() .put_blobs(*block_root, blobs.clone()); diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 4db50c30e13..68d48ec7c8b 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -138,9 +138,7 @@ pub use crate::beacon_block_body::{ pub use crate::beacon_block_header::BeaconBlockHeader; pub use crate::beacon_committee::{BeaconCommittee, OwnedBeaconCommittee}; pub use crate::beacon_state::{Error as BeaconStateError, *}; -pub use crate::blob_sidecar::{ - BlobIdentifier, BlobSidecar, BlobSidecarList, BlobSidecarVec, BlobsList, -}; +pub use crate::blob_sidecar::{BlobIdentifier, BlobSidecar, BlobSidecarList, BlobsList}; pub use crate::bls_to_execution_change::BlsToExecutionChange; pub use crate::chain_spec::{ChainSpec, Config, Domain}; pub use crate::checkpoint::Checkpoint; From 60100fc6be72792ff33913d7e5a53434c792aacf Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 30 Aug 2024 16:04:11 -0700 Subject: [PATCH 04/31] Fix some todos --- beacon_node/beacon_chain/src/beacon_chain.rs | 5 +---- beacon_node/network/src/sync/network_context.rs | 2 -- consensus/types/src/chain_spec.rs | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e1cf40f65f6..2785eac0a44 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1249,10 +1249,7 @@ impl BeaconChain { pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { match self.store.get_blobs(block_root)? { Some(blobs) => Ok(blobs), - None => Ok(BlobSidecarList::empty( - // TODO(pawan): fix this - self.spec.max_blobs_per_block(Epoch::new(0)) as usize, - )), + None => Ok(BlobSidecarList::empty_uninitialized()), } } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index bcfc9a7da22..84e968a5951 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -1239,8 +1239,6 @@ fn to_fixed_blob_sidecar_list( blobs: Vec>>, max_len: usize, ) -> Result, LookupVerifyError> { - // TODO(pawan): have a method on fixed runtime vector that just initializes a raw vec with max_len = None - // to signify an empty fixed runtime vector let mut fixed_list = FixedBlobSidecarList::new(vec![None; max_len]); for blob in blobs.into_iter() { let index = blob.index as usize; diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 5af0f349de3..2c28c3d31d7 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -1176,7 +1176,6 @@ impl ChainSpec { max_request_data_column_sidecars: default_max_request_data_column_sidecars(), min_epochs_for_blob_sidecars_requests: 16384, blob_sidecar_subnet_count: default_blob_sidecar_subnet_count(), - // TODO(pawan): check if gnosis preset values match max_blobs_per_block: default_max_blobs_per_block(), /* From e71020e3e613910e0315f558ead661b490a0ff20 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 3 Sep 2024 17:16:10 -0700 Subject: [PATCH 05/31] Fix take impl on RuntimeFixedList --- consensus/types/src/runtime_var_list.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 2bca5df2d9e..b24be185da8 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -253,7 +253,7 @@ pub struct RuntimeFixedList { len: usize, } -impl RuntimeFixedList { +impl RuntimeFixedList { pub fn new(vec: Vec) -> Self { let len = vec.len(); Self { vec, len } @@ -277,6 +277,7 @@ impl RuntimeFixedList { pub fn take(&mut self) -> Self { let new = std::mem::take(&mut self.vec); + *self = Self::new(vec![T::default(); self.len]); Self { vec: new, len: self.len, From 52bb581e071d5f474d519366e860a4b3a0b52f78 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 3 Sep 2024 18:38:19 -0700 Subject: [PATCH 06/31] cleanup --- beacon_node/beacon_chain/src/kzg_utils.rs | 5 ++-- .../lighthouse_network/src/rpc/methods.rs | 8 +++---- .../network/src/sync/network_context.rs | 4 ++-- consensus/types/src/blob_sidecar.rs | 2 +- consensus/types/src/runtime_var_list.rs | 23 ++++++++++++++++--- 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index db0847073ad..64e960f0f3a 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -188,8 +188,9 @@ fn build_data_column_sidecars( spec: &ChainSpec, ) -> Result, String> { let number_of_columns = spec.number_of_columns; - let max_blobs_per_block = - spec.max_blobs_per_block(signed_block_header.message.slot.epoch(E::slots_per_epoch())) as usize; + let max_blobs_per_block = spec + .max_blobs_per_block(signed_block_header.message.slot.epoch(E::slots_per_epoch())) + as usize; let mut columns = vec![Vec::with_capacity(max_blobs_per_block); number_of_columns]; let mut column_kzg_proofs = vec![Vec::with_capacity(max_blobs_per_block); number_of_columns]; diff --git a/beacon_node/lighthouse_network/src/rpc/methods.rs b/beacon_node/lighthouse_network/src/rpc/methods.rs index 0ff469aafbb..8fe57ae23aa 100644 --- a/beacon_node/lighthouse_network/src/rpc/methods.rs +++ b/beacon_node/lighthouse_network/src/rpc/methods.rs @@ -27,7 +27,7 @@ pub const MAX_ERROR_LEN: u64 = 256; /// The max number of blobs we expect in the configs to set for compile time params. /// Note: This value is an estimate that we should use only for rate limiting, /// bounds checking and other non-consensus critical operations. -/// +/// /// For exact value, we should always check the chainspec. pub const MAX_BLOBS_PER_BLOCK_CEILING: u64 = 16; @@ -335,11 +335,11 @@ pub struct BlobsByRangeRequest { impl BlobsByRangeRequest { /// This function provides an upper bound on number of blobs expected in /// a certain slot range. - /// + /// /// Note: **must not** use for anything consensus critical, only for - /// bounds checking and rate limiting. + /// bounds checking and rate limiting. pub fn max_blobs_requested(&self) -> u64 { - self.count.saturating_mul(MAX_BLOBS_PER_BLOCK_CEILING as u64) + self.count.saturating_mul(MAX_BLOBS_PER_BLOCK_CEILING) } } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 4b255fee4d4..a489ed70575 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -35,8 +35,8 @@ use std::time::Duration; use tokio::sync::mpsc; use types::blob_sidecar::FixedBlobSidecarList; use types::{ - BlobSidecar, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, EthSpec, - Hash256, SignedBeaconBlock, Slot, + BlobSidecar, ColumnIndex, DataColumnSidecar, DataColumnSidecarList, EthSpec, Hash256, + SignedBeaconBlock, Slot, }; pub mod custody; diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 108c807b475..9acbd8d95c2 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -265,7 +265,7 @@ impl BlobSidecar { } pub type BlobSidecarList = RuntimeVariableList>>; -/// Alias for a non length-constrained list of `BlobSidecar`s. +/// Alias for a non length-constrained list of `BlobSidecar`s. pub type FixedBlobSidecarList = RuntimeFixedList>>>; pub type BlobsList = VariableList, ::MaxBlobCommitmentsPerBlock>; diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index b24be185da8..8d03535f34d 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -10,6 +10,11 @@ use std::slice::SliceIndex; /// An ordered, heap-allocated, variable-length, homogeneous collection of `T`, with no more than /// `max_len` values. /// +/// In cases where the `max_length` of the container is unknown at time of initialization, we provide +/// a `Self::empty_uninitialized` constructor that initializes a runtime list without setting the max_len. +/// +/// To ensure there are no inconsistent states, we do not allow any mutating operation if `max_len` is not set. +/// /// ## Example /// /// ``` @@ -35,6 +40,16 @@ use std::slice::SliceIndex; /// /// // Push a value to if it _does_ exceed the maximum. /// assert!(long.push(6).is_err()); +/// +/// let mut uninit = RuntimeVariableList::empty_unitialized(); +/// assert!(uninit.push(5).is_err()); +/// +/// // Set max_len to allow mutation. +/// uninit.set_max_len(5usize); +/// +/// uninit.push(5).unwrap(); +/// assert_eq!(&uninit[..], &[5]); +/// /// ``` #[derive(Debug, Clone, Serialize, Deserialize, Derivative)] #[derivative(PartialEq, Eq, Hash(bound = "T: std::hash::Hash"))] @@ -73,7 +88,7 @@ impl RuntimeVariableList { } } - /// Create an empty list. + /// Create an empty list with the given `max_len`. pub fn empty(max_len: usize) -> Self { Self { vec: vec![], @@ -95,7 +110,7 @@ impl RuntimeVariableList { /// Returns an instance of `Self` with max_len = None. /// /// No mutating operation can be performed on an uninitialized instance - /// without first setting max_len. + /// without first setting `max_len`. pub fn empty_uninitialized() -> Self { Self { vec: vec![], @@ -129,7 +144,7 @@ impl RuntimeVariableList { /// Returns `Err(())` when appending `value` would exceed the maximum length. pub fn push(&mut self, value: T) -> Result<(), Error> { let Some(max_len) = self.max_len else { - // TODO(pawan): set a better error + // TODO(pawan): set a better error? return Err(Error::MissingLengthInformation); }; if self.vec.len() < max_len { @@ -247,6 +262,7 @@ where } } +/// Emulates a SSZ `Vector`. #[derive(Clone, Debug)] pub struct RuntimeFixedList { vec: Vec, @@ -267,6 +283,7 @@ impl RuntimeFixedList { self.vec.as_slice() } + #[allow(clippy::len_without_is_empty)] pub fn len(&self) -> usize { self.len } From d37733b846ce58e318e976d6503ca394b4901141 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 4 Sep 2024 12:47:36 -0700 Subject: [PATCH 07/31] Fix test compilations --- .../overflow_lru_cache.rs | 73 ++++++++++--------- .../src/observed_data_sidecars.rs | 6 +- beacon_node/beacon_chain/tests/events.rs | 2 +- .../network/src/sync/block_lookups/tests.rs | 13 +++- .../src/sync/block_sidecar_coupling.rs | 42 +++++++---- consensus/types/src/runtime_var_list.rs | 18 ++--- 6 files changed, 86 insertions(+), 68 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 ad027a45dd0..ebbb1c5a6fa 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 @@ -837,7 +837,8 @@ mod test { info!(log, "done printing kzg commitments"); let gossip_verified_blobs = if let Some((kzg_proofs, blobs)) = maybe_blobs { - let sidecars = BlobSidecar::build_sidecars(blobs, &block, kzg_proofs).unwrap(); + let sidecars = + BlobSidecar::build_sidecars(blobs, &block, kzg_proofs, &chain.spec).unwrap(); Vec::from(sidecars) .into_iter() .map(|sidecar| { @@ -1152,7 +1153,7 @@ mod pending_components_tests { use super::*; use crate::block_verification_types::BlockImportData; use crate::eth1_finalization_cache::Eth1FinalizationData; - use crate::test_utils::{generate_rand_block_and_blobs, NumBlobs}; + use crate::test_utils::{generate_rand_block_and_blobs, test_spec, NumBlobs}; use crate::PayloadVerificationOutcome; use fork_choice::PayloadVerificationStatus; use kzg::KzgCommitment; @@ -1168,15 +1169,19 @@ mod pending_components_tests { type Setup = ( SignedBeaconBlock, - FixedVector>>, ::MaxBlobsPerBlock>, - FixedVector>>, ::MaxBlobsPerBlock>, + RuntimeFixedList>>>, + RuntimeFixedList>>>, + usize, ); pub fn pre_setup() -> Setup { let mut rng = StdRng::seed_from_u64(0xDEADBEEF0BAD5EEDu64); + let spec = test_spec::(); let (block, blobs_vec) = generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Random, &mut rng); - let mut blobs: FixedVector<_, ::MaxBlobsPerBlock> = FixedVector::default(); + let max_len = spec.max_blobs_per_block(block.epoch()) as usize; + let mut blobs: RuntimeFixedList>>> = + RuntimeFixedList::default(max_len); for blob in blobs_vec { if let Some(b) = blobs.get_mut(blob.index as usize) { @@ -1184,10 +1189,8 @@ mod pending_components_tests { } } - let mut invalid_blobs: FixedVector< - Option>>, - ::MaxBlobsPerBlock, - > = FixedVector::default(); + let mut invalid_blobs: RuntimeFixedList>>> = + RuntimeFixedList::default(max_len); for (index, blob) in blobs.iter().enumerate() { if let Some(invalid_blob) = blob { let mut blob_copy = invalid_blob.as_ref().clone(); @@ -1196,21 +1199,21 @@ mod pending_components_tests { } } - (block, blobs, invalid_blobs) + (block, blobs, invalid_blobs, max_len) } type PendingComponentsSetup = ( DietAvailabilityPendingExecutedBlock, - FixedVector>, ::MaxBlobsPerBlock>, - FixedVector>, ::MaxBlobsPerBlock>, + RuntimeFixedList>>, + RuntimeFixedList>>, ); pub fn setup_pending_components( block: SignedBeaconBlock, - valid_blobs: FixedVector>>, ::MaxBlobsPerBlock>, - invalid_blobs: FixedVector>>, ::MaxBlobsPerBlock>, + valid_blobs: RuntimeFixedList>>>, + invalid_blobs: RuntimeFixedList>>>, ) -> PendingComponentsSetup { - let blobs = FixedVector::from( + let blobs = RuntimeFixedList::new( valid_blobs .iter() .map(|blob_opt| { @@ -1220,7 +1223,7 @@ mod pending_components_tests { }) .collect::>(), ); - let invalid_blobs = FixedVector::from( + let invalid_blobs = RuntimeFixedList::new( invalid_blobs .iter() .map(|blob_opt| { @@ -1252,10 +1255,10 @@ mod pending_components_tests { (block.into(), blobs, invalid_blobs) } - pub fn assert_cache_consistent(cache: PendingComponents) { + pub fn assert_cache_consistent(cache: PendingComponents, max_len: usize) { if let Some(cached_block) = cache.get_cached_block() { let cached_block_commitments = cached_block.get_commitments(); - for index in 0..E::max_blobs_per_block() { + for index in 0..max_len { let block_commitment = cached_block_commitments.get(index).copied(); let blob_commitment_opt = cache.get_cached_blobs().get(index).unwrap(); let blob_commitment = blob_commitment_opt.as_ref().map(|b| *b.get_commitment()); @@ -1274,40 +1277,40 @@ mod pending_components_tests { #[test] fn valid_block_invalid_blobs_valid_blobs() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_block(block_commitments); cache.merge_blobs(random_blobs); cache.merge_blobs(blobs); - assert_cache_consistent(cache); + assert_cache_consistent(cache, max_len); } #[test] fn invalid_blobs_block_valid_blobs() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_blobs(random_blobs); cache.merge_block(block_commitments); cache.merge_blobs(blobs); - assert_cache_consistent(cache); + assert_cache_consistent(cache, max_len); } #[test] fn invalid_blobs_valid_blobs_block() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_blobs(random_blobs); cache.merge_blobs(blobs); cache.merge_block(block_commitments); @@ -1317,46 +1320,46 @@ mod pending_components_tests { #[test] fn block_valid_blobs_invalid_blobs() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_block(block_commitments); cache.merge_blobs(blobs); cache.merge_blobs(random_blobs); - assert_cache_consistent(cache); + assert_cache_consistent(cache, max_len); } #[test] fn valid_blobs_block_invalid_blobs() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_blobs(blobs); cache.merge_block(block_commitments); cache.merge_blobs(random_blobs); - assert_cache_consistent(cache); + assert_cache_consistent(cache, max_len); } #[test] fn valid_blobs_invalid_blobs_block() { - let (block_commitments, blobs, random_blobs) = pre_setup(); + let (block_commitments, blobs, random_blobs, max_len) = pre_setup(); let (block_commitments, blobs, random_blobs) = setup_pending_components(block_commitments, blobs, random_blobs); let block_root = Hash256::zero(); - let mut cache = >::empty(block_root); + let mut cache = >::empty(block_root, max_len); cache.merge_blobs(blobs); cache.merge_blobs(random_blobs); cache.merge_block(block_commitments); - assert_cache_consistent(cache); + assert_cache_consistent(cache, max_len); } } diff --git a/beacon_node/beacon_chain/src/observed_data_sidecars.rs b/beacon_node/beacon_chain/src/observed_data_sidecars.rs index 7cc5f2e92a1..318f9f252d5 100644 --- a/beacon_node/beacon_chain/src/observed_data_sidecars.rs +++ b/beacon_node/beacon_chain/src/observed_data_sidecars.rs @@ -155,7 +155,7 @@ mod tests { use crate::test_utils::test_spec; use bls::Hash256; use std::sync::Arc; - use types::MainnetEthSpec; + use types::{Epoch, MainnetEthSpec}; type E = MainnetEthSpec; @@ -309,7 +309,7 @@ mod tests { #[test] fn simple_observations() { let spec = test_spec::(); - let mut cache = ObservedDataSidecars::>::new(spec); + let mut cache = ObservedDataSidecars::>::new(spec.clone()); // Slot 0, index 0 let proposer_index_a = 420; @@ -465,7 +465,7 @@ mod tests { ); // Try adding an out of bounds index - let invalid_index = E::max_blobs_per_block() as u64; + let invalid_index = spec.max_blobs_per_block(Epoch::new(0)); let sidecar_d = get_blob_sidecar(0, proposer_index_a, invalid_index); assert_eq!( cache.observe_sidecar(&sidecar_d), diff --git a/beacon_node/beacon_chain/tests/events.rs b/beacon_node/beacon_chain/tests/events.rs index d54543e4f6f..463453f0aec 100644 --- a/beacon_node/beacon_chain/tests/events.rs +++ b/beacon_node/beacon_chain/tests/events.rs @@ -69,7 +69,7 @@ async fn blob_sidecar_event_on_process_rpc_blobs() { index: 1, ..BlobSidecar::random_valid(&mut rng, kzg).unwrap() }); - let blobs = FixedBlobSidecarList::from(vec![Some(blob_1.clone()), Some(blob_2.clone())]); + let blobs = FixedBlobSidecarList::new(vec![Some(blob_1.clone()), Some(blob_2.clone())]); let expected_sse_blobs = vec![ SseBlobSidecar::from_blob_sidecar(blob_1.as_ref()), SseBlobSidecar::from_blob_sidecar(blob_2.as_ref()), diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 6d852b2572d..afd2fe0c861 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -39,7 +39,7 @@ use types::{ test_utils::{SeedableRng, XorShiftRng}, BlobSidecar, ForkName, MinimalEthSpec as E, SignedBeaconBlock, Slot, }; -use types::{BeaconState, BeaconStateBase}; +use types::{BeaconState, BeaconStateBase, ChainSpec}; use types::{DataColumnSidecar, Epoch}; type T = Witness, E, MemoryStore, MemoryStore>; @@ -86,6 +86,7 @@ struct TestRig { /// `rng` for generating test blocks and blobs. rng: XorShiftRng, fork_name: ForkName, + spec: ChainSpec, log: Logger, } @@ -153,6 +154,8 @@ impl TestRig { .network_globals .set_sync_state(SyncState::Synced); + let spec = chain.spec.clone(); + let rng = XorShiftRng::from_seed([42; 16]); TestRig { beacon_processor_rx, @@ -174,6 +177,7 @@ impl TestRig { harness, fork_name, log, + spec, } } @@ -2048,8 +2052,8 @@ mod deneb_only { use beacon_chain::{ block_verification_types::RpcBlock, data_availability_checker::AvailabilityCheckError, }; - use ssz_types::VariableList; use std::collections::VecDeque; + use types::RuntimeVariableList; struct DenebTester { rig: TestRig, @@ -2407,12 +2411,15 @@ mod deneb_only { fn parent_block_unknown_parent(mut self) -> Self { self.rig.log("parent_block_unknown_parent"); let block = self.unknown_parent_block.take().unwrap(); + let max_len = self.rig.spec.max_blobs_per_block(block.epoch()) as usize; // Now this block is the one we expect requests from self.block = block.clone(); let block = RpcBlock::new( Some(block.canonical_root()), block, - self.unknown_parent_blobs.take().map(VariableList::from), + self.unknown_parent_blobs + .take() + .map(|vec| RuntimeVariableList::from_vec(vec, max_len)), ) .unwrap(); self.rig.parent_block_processed( diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index f241a52c9c8..0d0a5b80815 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -252,12 +252,15 @@ mod tests { #[test] fn no_blobs_into_responses() { + let spec = test_spec::(); let peer_id = PeerId::random(); - let mut info = RangeBlockComponentsRequest::::new(false, None, None, vec![peer_id]); let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng).0) .collect::>(); + let max_len = spec.max_blobs_per_block(blocks.first().unwrap().epoch()) as usize; + let mut info = + RangeBlockComponentsRequest::::new(false, None, None, vec![peer_id], max_len); // Send blocks and complete terminate response for block in blocks { @@ -272,8 +275,8 @@ mod tests { #[test] fn empty_blobs_into_responses() { + let spec = test_spec::(); let peer_id = PeerId::random(); - let mut info = RangeBlockComponentsRequest::::new(true, None, None, vec![peer_id]); let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| { @@ -281,6 +284,9 @@ mod tests { generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Number(3), &mut rng).0 }) .collect::>(); + let max_len = spec.max_blobs_per_block(blocks.first().unwrap().epoch()) as usize; + let mut info = + RangeBlockComponentsRequest::::new(true, None, None, vec![peer_id], max_len); // Send blocks and complete terminate response for block in blocks { @@ -301,12 +307,7 @@ mod tests { fn rpc_block_with_custody_columns() { let spec = test_spec::(); let expects_custody_columns = vec![1, 2, 3, 4]; - let mut info = RangeBlockComponentsRequest::::new( - false, - Some(expects_custody_columns.clone()), - Some(expects_custody_columns.len()), - vec![PeerId::random()], - ); + let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| { @@ -318,7 +319,14 @@ mod tests { ) }) .collect::>(); - + let max_len = spec.max_blobs_per_block(blocks.first().unwrap().0.epoch()) as usize; + let mut info = RangeBlockComponentsRequest::::new( + false, + Some(expects_custody_columns.clone()), + Some(expects_custody_columns.len()), + vec![PeerId::random()], + max_len, + ); // Send blocks and complete terminate response for block in &blocks { info.add_block_response(Some(block.0.clone().into())); @@ -362,12 +370,7 @@ mod tests { let spec = test_spec::(); let expects_custody_columns = vec![1, 2, 3, 4]; let num_of_data_column_requests = 2; - let mut info = RangeBlockComponentsRequest::::new( - false, - Some(expects_custody_columns.clone()), - Some(num_of_data_column_requests), - vec![PeerId::random()], - ); + let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) .map(|_| { @@ -379,7 +382,14 @@ mod tests { ) }) .collect::>(); - + let max_len = spec.max_blobs_per_block(blocks.first().unwrap().0.epoch()) as usize; + let mut info = RangeBlockComponentsRequest::::new( + false, + Some(expects_custody_columns.clone()), + Some(num_of_data_column_requests), + vec![PeerId::random()], + max_len, + ); // Send blocks and complete terminate response for block in &blocks { info.add_block_response(Some(block.0.clone().into())); diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 8d03535f34d..e93f07981ec 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -292,6 +292,13 @@ impl RuntimeFixedList { self.vec } + pub fn default(max_len: usize) -> Self { + Self { + vec: vec![T::default(); max_len], + len: max_len, + } + } + pub fn take(&mut self) -> Self { let new = std::mem::take(&mut self.vec); *self = Self::new(vec![T::default(); self.len]); @@ -339,7 +346,6 @@ mod test { use super::*; use ssz::*; use std::fmt::Debug; - use tree_hash::TreeHash; #[test] fn new() { @@ -404,7 +410,7 @@ mod test { } fn round_trip(item: RuntimeVariableList) { - let max_len = item.max_len(); + let max_len = item.max_len().unwrap(); let encoded = &item.as_ssz_bytes(); assert_eq!(item.ssz_bytes_len(), encoded.len()); assert_eq!( @@ -418,12 +424,4 @@ mod test { round_trip::(RuntimeVariableList::from_vec(vec![42; 8], 8)); round_trip::(RuntimeVariableList::from_vec(vec![0; 8], 8)); } - - #[test] - fn test_empty_list_encoding() { - use ssz_types::{typenum::U16, VariableList}; - - let a: RuntimeVariableList = RuntimeVariableList::from_vec(vec![], 16); - dbg!(a.tree_hash_root()); - } } From 12c6ef118a1a6d910c48d9d4b23004f3609264c7 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 4 Sep 2024 16:16:36 -0700 Subject: [PATCH 08/31] Fix some more tests --- beacon_node/beacon_chain/tests/block_verification.rs | 6 +++--- .../network/src/network_beacon_processor/tests.rs | 11 ++++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index faa4d74a182..174d1a68e5f 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -206,7 +206,7 @@ fn update_blob_signed_header( signed_block: &SignedBeaconBlock, blobs: &mut BlobSidecarList, ) { - for old_blob_sidecar in blobs.iter_mut() { + for old_blob_sidecar in blobs.as_mut_slice().unwrap() { let new_blob = Arc::new(BlobSidecar:: { index: old_blob_sidecar.index, blob: old_blob_sidecar.blob.clone(), @@ -1214,7 +1214,7 @@ async fn verify_block_for_gossip_slashing_detection() { let slasher = Arc::new( Slasher::open( SlasherConfig::new(slasher_dir.path().into()), - spec, + spec.clone(), test_logger(), ) .unwrap(), @@ -1238,7 +1238,7 @@ async fn verify_block_for_gossip_slashing_detection() { if let Some((kzg_proofs, blobs)) = blobs1 { let sidecars = - BlobSidecar::build_sidecars(blobs, verified_block.block(), kzg_proofs).unwrap(); + BlobSidecar::build_sidecars(blobs, verified_block.block(), kzg_proofs, &spec).unwrap(); for sidecar in sidecars { let blob_index = sidecar.index; let verified_blob = harness diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 40c69a0baa5..9207f6a2dd6 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -256,7 +256,7 @@ impl TestRig { assert!(beacon_processor.is_ok()); let block = next_block_tuple.0; let blob_sidecars = if let Some((kzg_proofs, blobs)) = next_block_tuple.1 { - Some(BlobSidecar::build_sidecars(blobs, &block, kzg_proofs).unwrap()) + Some(BlobSidecar::build_sidecars(blobs, &block, kzg_proofs, &chain.spec).unwrap()) } else { None }; @@ -341,7 +341,7 @@ impl TestRig { } pub fn enqueue_single_lookup_rpc_blobs(&self) { if let Some(blobs) = self.next_blobs.clone() { - let blobs = FixedBlobSidecarList::from(blobs.into_iter().map(Some).collect::>()); + let blobs = FixedBlobSidecarList::new(blobs.into_iter().map(Some).collect::>()); self.network_beacon_processor .send_rpc_blobs( self.next_block.canonical_root(), @@ -1100,7 +1100,12 @@ async fn test_blobs_by_range() { .block_root_at_slot(Slot::new(slot), WhenSlotSkipped::None) .unwrap(); blob_count += root - .map(|root| rig.chain.get_blobs(&root).unwrap_or_default().len()) + .map(|root| { + rig.chain + .get_blobs(&root) + .map(|list| list.len()) + .unwrap_or(0) + }) .unwrap_or(0); } let mut actual_count = 0; From 2fcb2935ec7ef4cd18bbdd8aedb7de61fac69e61 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 6 Sep 2024 18:28:31 -0700 Subject: [PATCH 09/31] Fix test from unstable --- .../src/data_availability_checker/overflow_lru_cache.rs | 2 ++ 1 file changed, 2 insertions(+) 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 ebbb1c5a6fa..1735e782573 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 @@ -972,6 +972,8 @@ mod test { assert_eq!(cache.critical.read().len(), 1); } } + // remove the blob to simulate successful import + cache.remove_pending_components(root); assert!( cache.critical.read().is_empty(), "cache should be empty now that all components available" From de01f923c7452355c87f50c0e8031ca94fa00d36 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 14:06:57 +1100 Subject: [PATCH 10/31] Remove footgun function --- consensus/types/src/runtime_var_list.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index b1365b34c1b..2a4b185632c 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -118,10 +118,6 @@ impl RuntimeVariableList { } } - pub fn set_max_len(&mut self, max_len: usize) { - self.max_len = Some(max_len); - } - /// Returns the number of values presently in `self`. pub fn len(&self) -> usize { self.vec.len() From 1095d60a40be20dd3c229b759fc3c228b51e51e3 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 14:38:40 +1100 Subject: [PATCH 11/31] Minor simplifications --- beacon_node/http_api/src/block_id.rs | 12 +++--------- beacon_node/store/src/hot_cold_store.rs | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index d29c89fa7c0..6db05c6b80e 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -305,15 +305,9 @@ impl BlockId { .into_iter() .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) .collect(); - if let Some(max_len) = list - .first() - .map(|sidecar| chain.spec.max_blobs_per_block(sidecar.epoch())) - { - BlobSidecarList::new(list, max_len as usize) - .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? - } else { - BlobSidecarList::empty_uninitialized() - } + let max_len = chain.spec.max_blobs_per_block(block.epoch()); + BlobSidecarList::new(list, max_len as usize) + .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? } None => blob_sidecar_list, }; diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 44dbdc8cdcc..b313051b410 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -2060,7 +2060,7 @@ impl, Cold: ItemStore> HotColdDB // We insert a VariableList of BlobSidecars into the db, but retrieve // a plain vec since we don't know the length limit of the list without // knowing the slot. - // The encoding of a VariableList is same as a regular vec. + // The encoding of a VariableList is the same as a regular vec. let blobs: Vec>> = Vec::<_>::from_ssz_bytes(blobs_bytes)?; let blobs = if let Some(max_blobs_per_block) = blobs .first() From 2e86585b478c012f6e3483989c87e38161227674 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 15:04:15 +1100 Subject: [PATCH 12/31] Move from preset to config --- .../built_in_network_configs/chiado/config.yaml | 4 +++- .../built_in_network_configs/gnosis/config.yaml | 4 +++- .../built_in_network_configs/holesky/config.yaml | 4 +++- .../built_in_network_configs/mainnet/config.yaml | 4 +++- .../built_in_network_configs/sepolia/config.yaml | 4 +++- consensus/types/presets/gnosis/deneb.yaml | 2 -- consensus/types/presets/mainnet/deneb.yaml | 2 -- consensus/types/presets/minimal/deneb.yaml | 2 -- 8 files changed, 15 insertions(+), 11 deletions(-) diff --git a/common/eth2_network_config/built_in_network_configs/chiado/config.yaml b/common/eth2_network_config/built_in_network_configs/chiado/config.yaml index 1eca01bbeef..2fc3c7e945a 100644 --- a/common/eth2_network_config/built_in_network_configs/chiado/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/chiado/config.yaml @@ -136,9 +136,11 @@ MAX_REQUEST_BLOB_SIDECARS: 768 MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: 16384 # `6` BLOB_SIDECAR_SUBNET_COUNT: 6 +# `uint64(6)` +MAX_BLOBS_PER_BLOCK: 6 # DAS CUSTODY_REQUIREMENT: 4 DATA_COLUMN_SIDECAR_SUBNET_COUNT: 128 NUMBER_OF_COLUMNS: 128 -SAMPLES_PER_SLOT: 8 \ No newline at end of file +SAMPLES_PER_SLOT: 8 diff --git a/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml b/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml index 500555a2694..b8fe4d51857 100644 --- a/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml @@ -119,9 +119,11 @@ MAX_REQUEST_BLOB_SIDECARS: 768 MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: 16384 # `6` BLOB_SIDECAR_SUBNET_COUNT: 6 +# `uint64(6)` +MAX_BLOBS_PER_BLOCK: 6 # DAS CUSTODY_REQUIREMENT: 4 DATA_COLUMN_SIDECAR_SUBNET_COUNT: 128 NUMBER_OF_COLUMNS: 128 -SAMPLES_PER_SLOT: 8 \ No newline at end of file +SAMPLES_PER_SLOT: 8 diff --git a/common/eth2_network_config/built_in_network_configs/holesky/config.yaml b/common/eth2_network_config/built_in_network_configs/holesky/config.yaml index d67d77d3bea..1ba0f03641a 100644 --- a/common/eth2_network_config/built_in_network_configs/holesky/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/holesky/config.yaml @@ -123,9 +123,11 @@ MAX_REQUEST_BLOB_SIDECARS: 768 MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: 4096 # `6` BLOB_SIDECAR_SUBNET_COUNT: 6 +# `uint64(6)` +MAX_BLOBS_PER_BLOCK: 6 # DAS CUSTODY_REQUIREMENT: 4 DATA_COLUMN_SIDECAR_SUBNET_COUNT: 128 NUMBER_OF_COLUMNS: 128 -SAMPLES_PER_SLOT: 8 \ No newline at end of file +SAMPLES_PER_SLOT: 8 diff --git a/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml b/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml index 18591fecdcd..ea3469e3f1d 100644 --- a/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml @@ -145,9 +145,11 @@ MAX_REQUEST_BLOB_SIDECARS: 768 MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: 4096 # `6` BLOB_SIDECAR_SUBNET_COUNT: 6 +# `uint64(6)` +MAX_BLOBS_PER_BLOCK: 6 # DAS CUSTODY_REQUIREMENT: 4 DATA_COLUMN_SIDECAR_SUBNET_COUNT: 128 NUMBER_OF_COLUMNS: 128 -SAMPLES_PER_SLOT: 8 \ No newline at end of file +SAMPLES_PER_SLOT: 8 diff --git a/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml b/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml index b08a6180bf0..38185188976 100644 --- a/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml @@ -119,9 +119,11 @@ MAX_REQUEST_BLOB_SIDECARS: 768 MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: 4096 # `6` BLOB_SIDECAR_SUBNET_COUNT: 6 +# `uint64(6)` +MAX_BLOBS_PER_BLOCK: 6 # DAS CUSTODY_REQUIREMENT: 4 DATA_COLUMN_SIDECAR_SUBNET_COUNT: 128 NUMBER_OF_COLUMNS: 128 -SAMPLES_PER_SLOT: 8 \ No newline at end of file +SAMPLES_PER_SLOT: 8 diff --git a/consensus/types/presets/gnosis/deneb.yaml b/consensus/types/presets/gnosis/deneb.yaml index d2d7d0abed3..9a46a6dafe5 100644 --- a/consensus/types/presets/gnosis/deneb.yaml +++ b/consensus/types/presets/gnosis/deneb.yaml @@ -8,7 +8,5 @@ FIELD_ELEMENTS_PER_BLOB: 4096 # `uint64(2**12)` (= 4096) MAX_BLOB_COMMITMENTS_PER_BLOCK: 4096 -# `uint64(6)` -MAX_BLOBS_PER_BLOCK: 6 # `floorlog2(BLOB_KZG_COMMITMENTS_GINDEX) + 1 + ceillog2(MAX_BLOB_COMMITMENTS_PER_BLOCK)` = 4 + 1 + 12 = 17 KZG_COMMITMENT_INCLUSION_PROOF_DEPTH: 17 diff --git a/consensus/types/presets/mainnet/deneb.yaml b/consensus/types/presets/mainnet/deneb.yaml index 0f56b8bdfac..f426d3ae1a9 100644 --- a/consensus/types/presets/mainnet/deneb.yaml +++ b/consensus/types/presets/mainnet/deneb.yaml @@ -6,7 +6,5 @@ FIELD_ELEMENTS_PER_BLOB: 4096 # `uint64(2**12)` (= 4096) MAX_BLOB_COMMITMENTS_PER_BLOCK: 4096 -# `uint64(6)` -MAX_BLOBS_PER_BLOCK: 6 # `floorlog2(get_generalized_index(BeaconBlockBody, 'blob_kzg_commitments')) + 1 + ceillog2(MAX_BLOB_COMMITMENTS_PER_BLOCK)` = 4 + 1 + 12 = 17 KZG_COMMITMENT_INCLUSION_PROOF_DEPTH: 17 diff --git a/consensus/types/presets/minimal/deneb.yaml b/consensus/types/presets/minimal/deneb.yaml index be2b9fadfa5..b1bbc4ee541 100644 --- a/consensus/types/presets/minimal/deneb.yaml +++ b/consensus/types/presets/minimal/deneb.yaml @@ -6,7 +6,5 @@ FIELD_ELEMENTS_PER_BLOB: 4096 # [customized] MAX_BLOB_COMMITMENTS_PER_BLOCK: 16 -# `uint64(6)` -MAX_BLOBS_PER_BLOCK: 6 # [customized] `floorlog2(BLOB_KZG_COMMITMENTS_GINDEX) + 1 + ceillog2(MAX_BLOB_COMMITMENTS_PER_BLOCK)` = 4 + 1 + 4 = 9 KZG_COMMITMENT_INCLUSION_PROOF_DEPTH: 9 From 32483d385b66f252d50cee5b524e2924157bdcd4 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 15:04:32 +1100 Subject: [PATCH 13/31] Fix typo --- beacon_node/http_api/tests/broadcast_validation_tests.rs | 3 ++- consensus/types/src/runtime_var_list.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index e1ecf2d4fc3..99ed3ef724f 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -1460,7 +1460,8 @@ pub async fn block_seen_on_gossip_with_some_blobs() { let blobs = blobs.expect("should have some blobs"); assert!( blobs.0.len() >= 2, - "need at least 2 blobs for partial reveal" + "need at least 2 blobs for partial reveal, got: {}", + blobs.0.len() ); let partial_kzg_proofs = vec![*blobs.0.first().unwrap()]; diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 2a4b185632c..60c75047261 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -41,7 +41,7 @@ use std::slice::SliceIndex; /// // Push a value to if it _does_ exceed the maximum. /// assert!(long.push(6).is_err()); /// -/// let mut uninit = RuntimeVariableList::empty_unitialized(); +/// let mut uninit = RuntimeVariableList::empty_uninitialized(); /// assert!(uninit.push(5).is_err()); /// /// // Set max_len to allow mutation. From 88bedf09bc647de66bd1ff944bbc8fb13e2b7590 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 15:04:37 +1100 Subject: [PATCH 14/31] Revert "Remove footgun function" This reverts commit de01f923c7452355c87f50c0e8031ca94fa00d36. --- consensus/types/src/runtime_var_list.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 60c75047261..54551aa46f8 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -118,6 +118,10 @@ impl RuntimeVariableList { } } + pub fn set_max_len(&mut self, max_len: usize) { + self.max_len = Some(max_len); + } + /// Returns the number of values presently in `self`. pub fn len(&self) -> usize { self.vec.len() From 063b79c16abd3f6df47b85efcf3858177bc933b9 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 15:32:16 +1100 Subject: [PATCH 15/31] Try fixing tests --- beacon_node/beacon_chain/tests/block_verification.rs | 4 ++++ .../src/test_utils/execution_block_generator.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 078a5d848cb..415aed5831d 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -206,6 +206,10 @@ fn update_blob_signed_header( signed_block: &SignedBeaconBlock, blobs: &mut BlobSidecarList, ) { + // Required to prevent as_mut_slice being called on an uninitialized list. Yuck. + if blobs.is_empty() { + return; + } for old_blob_sidecar in blobs.as_mut_slice().unwrap() { let new_blob = Arc::new(BlobSidecar:: { index: old_blob_sidecar.index, diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index f5e3e560ecc..a190b3fb753 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -672,7 +672,7 @@ impl ExecutionBlockGenerator { // get random number between 0 and Max Blobs let mut rng = self.rng.lock(); // TODO(pawan): thread the chainspec value here somehow - let num_blobs = rng.gen::() % 6; + let num_blobs = rng.gen::() % (6 + 1); let (bundle, transactions) = generate_blobs(num_blobs)?; for tx in Vec::from(transactions) { execution_payload From e4bfe71cd1f0a2784d0bd57f85b2f5d8cf503ac1 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 17:05:30 +1100 Subject: [PATCH 16/31] Thread through ChainSpec --- .../overflow_lru_cache.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 19 +++++++++---------- .../execution_layer/src/engine_api/http.rs | 3 ++- .../test_utils/execution_block_generator.rs | 13 +++++++++++-- .../src/test_utils/mock_execution_layer.rs | 9 +++++---- .../execution_layer/src/test_utils/mod.rs | 9 +++++++-- .../src/sync/block_sidecar_coupling.rs | 13 +++++++++++-- beacon_node/network/src/sync/tests/lookups.rs | 6 ++++-- consensus/types/src/chain_spec.rs | 15 ++++++++------- lcli/src/mock_el.rs | 5 +++-- testing/node_test_rig/src/lib.rs | 9 ++++++++- 11 files changed, 69 insertions(+), 34 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 d5351a5dc2a..4cb425b74a9 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 @@ -1178,7 +1178,7 @@ mod pending_components_tests { let mut rng = StdRng::seed_from_u64(0xDEADBEEF0BAD5EEDu64); let spec = test_spec::(); let (block, blobs_vec) = - generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Random, &mut rng); + generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Random, &mut rng, &spec); let max_len = spec.max_blobs_per_block(block.epoch()) as usize; let mut blobs: RuntimeFixedList>>> = RuntimeFixedList::default(max_len); diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index a792f5f404c..a722669c2d8 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -511,7 +511,7 @@ where pub fn mock_execution_layer_with_config(mut self) -> Self { let mock = mock_execution_layer_from_parts::( - self.spec.as_ref().expect("cannot build without spec"), + self.spec.clone().expect("cannot build without spec"), self.runtime.task_executor.clone(), ); self.execution_layer = Some(mock.el.clone()); @@ -611,7 +611,7 @@ where } pub fn mock_execution_layer_from_parts( - spec: &ChainSpec, + spec: Arc, task_executor: TaskExecutor, ) -> MockExecutionLayer { let shanghai_time = spec.capella_fork_epoch.map(|epoch| { @@ -624,7 +624,7 @@ pub fn mock_execution_layer_from_parts( HARNESS_GENESIS_TIME + spec.seconds_per_slot * E::slots_per_epoch() * epoch.as_u64() }); - let kzg = get_kzg(spec); + let kzg = get_kzg(&spec); MockExecutionLayer::new( task_executor, @@ -633,7 +633,7 @@ pub fn mock_execution_layer_from_parts( cancun_time, prague_time, Some(JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap()), - spec.clone(), + spec, Some(kzg), ) } @@ -2816,11 +2816,12 @@ pub fn generate_rand_block_and_blobs( fork_name: ForkName, num_blobs: NumBlobs, rng: &mut impl Rng, + spec: &ChainSpec, ) -> (SignedBeaconBlock>, Vec>) { let inner = map_fork_name!(fork_name, BeaconBlock, <_>::random_for_test(rng)); let mut block = SignedBeaconBlock::from_block(inner, types::Signature::random_for_test(rng)); - + let max_blobs = spec.max_blobs_per_block(block.epoch()) as usize; let mut blob_sidecars = vec![]; let bundle = match block { @@ -2830,8 +2831,7 @@ pub fn generate_rand_block_and_blobs( // Get either zero blobs or a random number of blobs between 1 and Max Blobs. let payload: &mut FullPayloadDeneb = &mut message.body.execution_payload; let num_blobs = match num_blobs { - // TODO(pawan): thread the chainspec value here - NumBlobs::Random => rng.gen_range(1..=6), + NumBlobs::Random => rng.gen_range(1..=max_blobs), NumBlobs::Number(n) => n, NumBlobs::None => 0, }; @@ -2851,8 +2851,7 @@ pub fn generate_rand_block_and_blobs( // Get either zero blobs or a random number of blobs between 1 and Max Blobs. let payload: &mut FullPayloadElectra = &mut message.body.execution_payload; let num_blobs = match num_blobs { - // TODO(pawan): thread the chainspec value here - NumBlobs::Random => rng.gen_range(1..=6), + NumBlobs::Random => rng.gen_range(1..=max_blobs), NumBlobs::Number(n) => n, NumBlobs::None => 0, }; @@ -2906,7 +2905,7 @@ pub fn generate_rand_block_and_data_columns( DataColumnSidecarList, ) { let kzg = get_kzg(spec); - let (block, blobs) = generate_rand_block_and_blobs(fork_name, num_blobs, rng); + let (block, blobs) = generate_rand_block_and_blobs(fork_name, num_blobs, rng, spec); let blob_refs = blobs.iter().map(|b| &b.blob).collect::>(); let data_columns = blobs_to_data_column_sidecars(&blob_refs, &block, &kzg, spec).unwrap(); diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 33dc60d0378..830eb9d5d90 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -1321,7 +1321,8 @@ mod test { impl Tester { pub fn new(with_auth: bool) -> Self { - let server = MockServer::unit_testing(); + let spec = Arc::new(MainnetEthSpec::default_spec()); + let server = MockServer::unit_testing(spec); let rpc_url = SensitiveUrl::parse(&server.url()).unwrap(); let echo_url = SensitiveUrl::parse(&format!("{}/echo", server.url())).unwrap(); diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index a190b3fb753..7dd39d58ad8 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -153,6 +153,7 @@ pub struct ExecutionBlockGenerator { pub blobs_bundles: HashMap>, pub kzg: Option>, rng: Arc>, + spec: Arc, } fn make_rng() -> Arc> { @@ -162,6 +163,7 @@ fn make_rng() -> Arc> { } impl ExecutionBlockGenerator { + #[allow(clippy::too_many_arguments)] pub fn new( terminal_total_difficulty: Uint256, terminal_block_number: u64, @@ -169,6 +171,7 @@ impl ExecutionBlockGenerator { shanghai_time: Option, cancun_time: Option, prague_time: Option, + spec: Arc, kzg: Option>, ) -> Self { let mut gen = Self { @@ -188,6 +191,7 @@ impl ExecutionBlockGenerator { blobs_bundles: <_>::default(), kzg, rng: make_rng(), + spec, }; gen.insert_pow_block(0).unwrap(); @@ -671,8 +675,11 @@ impl ExecutionBlockGenerator { if execution_payload.fork_name().deneb_enabled() { // get random number between 0 and Max Blobs let mut rng = self.rng.lock(); - // TODO(pawan): thread the chainspec value here somehow - let num_blobs = rng.gen::() % (6 + 1); + let max_blobs = self + .spec + .max_blobs_per_block_by_fork(execution_payload.fork_name()) + as usize; + let num_blobs = rng.gen::() % (max_blobs + 1); let (bundle, transactions) = generate_blobs(num_blobs)?; for tx in Vec::from(transactions) { execution_payload @@ -875,6 +882,7 @@ mod test { const TERMINAL_DIFFICULTY: u64 = 10; const TERMINAL_BLOCK: u64 = 10; const DIFFICULTY_INCREMENT: u64 = 1; + let spec = Arc::new(MainnetEthSpec::default_spec()); let mut generator: ExecutionBlockGenerator = ExecutionBlockGenerator::new( Uint256::from(TERMINAL_DIFFICULTY), @@ -883,6 +891,7 @@ mod test { None, None, None, + spec, None, ); diff --git a/beacon_node/execution_layer/src/test_utils/mock_execution_layer.rs b/beacon_node/execution_layer/src/test_utils/mock_execution_layer.rs index 48372a39be1..f472d21482a 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_execution_layer.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_execution_layer.rs @@ -13,7 +13,7 @@ pub struct MockExecutionLayer { pub server: MockServer, pub el: ExecutionLayer, pub executor: TaskExecutor, - pub spec: ChainSpec, + pub spec: Arc, } impl MockExecutionLayer { @@ -29,7 +29,7 @@ impl MockExecutionLayer { None, None, Some(JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap()), - spec, + Arc::new(spec), None, ) } @@ -42,7 +42,7 @@ impl MockExecutionLayer { cancun_time: Option, prague_time: Option, jwt_key: Option, - spec: ChainSpec, + spec: Arc, kzg: Option>, ) -> Self { let handle = executor.handle().unwrap(); @@ -57,6 +57,7 @@ impl MockExecutionLayer { shanghai_time, cancun_time, prague_time, + spec.clone(), kzg, ); @@ -320,7 +321,7 @@ impl MockExecutionLayer { pub async fn with_terminal_block<'a, U, V>(self, func: U) -> Self where - U: Fn(ChainSpec, ExecutionLayer, Option) -> V, + U: Fn(Arc, ExecutionLayer, Option) -> V, V: Future, { let terminal_block_number = self diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index faf6d4ef0b6..726874ddb06 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -21,7 +21,7 @@ use std::marker::PhantomData; use std::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; use std::sync::{Arc, LazyLock}; use tokio::{runtime, sync::oneshot}; -use types::{EthSpec, ExecutionBlockHash, Uint256}; +use types::{ChainSpec, EthSpec, ExecutionBlockHash, Uint256}; use warp::{http::StatusCode, Filter, Rejection}; use crate::EngineCapabilities; @@ -107,7 +107,7 @@ pub struct MockServer { } impl MockServer { - pub fn unit_testing() -> Self { + pub fn unit_testing(chain_spec: Arc) -> Self { Self::new( &runtime::Handle::current(), JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap(), @@ -117,6 +117,7 @@ impl MockServer { None, // FIXME(capella): should this be the default? None, // FIXME(deneb): should this be the default? None, // FIXME(electra): should this be the default? + chain_spec, None, ) } @@ -124,6 +125,7 @@ impl MockServer { pub fn new_with_config( handle: &runtime::Handle, config: MockExecutionConfig, + spec: Arc, kzg: Option>, ) -> Self { let MockExecutionConfig { @@ -145,6 +147,7 @@ impl MockServer { shanghai_time, cancun_time, prague_time, + spec, kzg, ); @@ -208,6 +211,7 @@ impl MockServer { shanghai_time: Option, cancun_time: Option, prague_time: Option, + spec: Arc, kzg: Option>, ) -> Self { Self::new_with_config( @@ -222,6 +226,7 @@ impl MockServer { cancun_time, prague_time, }, + spec, kzg, ) } diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 0d0a5b80815..7a234eaef04 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -256,7 +256,10 @@ mod tests { let peer_id = PeerId::random(); let mut rng = XorShiftRng::from_seed([42; 16]); let blocks = (0..4) - .map(|_| generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng).0) + .map(|_| { + generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng, &spec) + .0 + }) .collect::>(); let max_len = spec.max_blobs_per_block(blocks.first().unwrap().epoch()) as usize; let mut info = @@ -281,7 +284,13 @@ mod tests { let blocks = (0..4) .map(|_| { // Always generate some blobs. - generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Number(3), &mut rng).0 + generate_rand_block_and_blobs::( + ForkName::Deneb, + NumBlobs::Number(3), + &mut rng, + &spec, + ) + .0 }) .collect::>(); let max_len = spec.max_blobs_per_block(blocks.first().unwrap().epoch()) as usize; diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 8c88ddb62de..ee96db0f636 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -216,7 +216,7 @@ impl TestRig { ) -> (SignedBeaconBlock, Vec>) { let fork_name = self.fork_name; let rng = &mut self.rng; - generate_rand_block_and_blobs::(fork_name, num_blobs, rng) + generate_rand_block_and_blobs::(fork_name, num_blobs, rng, &self.spec) } fn rand_block_and_data_columns( @@ -1331,8 +1331,10 @@ impl TestRig { #[test] fn stable_rng() { + let spec = types::MainnetEthSpec::default_spec(); let mut rng = XorShiftRng::from_seed([42; 16]); - let (block, _) = generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng); + let (block, _) = + generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng, &spec); assert_eq!( block.canonical_root(), Hash256::from_slice( diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index c1d87354a8e..05d5142853a 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -606,14 +606,15 @@ impl ChainSpec { } } - /// Returns the deneb preset value if peerdas epoch hasn't hit. - /// Otherwise, returns the value obtained from the config.yaml. + /// Return the value of `MAX_BLOBS_PER_BLOCK` appropriate for the fork at `epoch`. pub fn max_blobs_per_block(&self, epoch: Epoch) -> u64 { - if self.is_peer_das_enabled_for_epoch(epoch) { - self.max_blobs_per_block - } else { - default_max_blobs_per_block() - } + self.max_blobs_per_block_by_fork(self.fork_name_at_epoch(epoch)) + } + + /// Return the value of `MAX_BLOBS_PER_BLOCK` appropriate for `fork`. + pub fn max_blobs_per_block_by_fork(&self, _fork_name: ForkName) -> u64 { + // TODO(electra): add Electra blobs per block change here + self.max_blobs_per_block } pub fn data_columns_per_subnet(&self) -> usize { diff --git a/lcli/src/mock_el.rs b/lcli/src/mock_el.rs index 8d3220b1df8..aad7c89ddad 100644 --- a/lcli/src/mock_el.rs +++ b/lcli/src/mock_el.rs @@ -9,6 +9,7 @@ use execution_layer::{ }; use std::net::Ipv4Addr; use std::path::PathBuf; +use std::sync::Arc; use types::*; pub fn run(mut env: Environment, matches: &ArgMatches) -> Result<(), String> { @@ -21,7 +22,7 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< let prague_time = parse_optional(matches, "prague-time")?; let handle = env.core_context().executor.handle().unwrap(); - let spec = &E::default_spec(); + let spec = Arc::new(E::default_spec()); let jwt_key = JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap(); std::fs::write(jwt_path, hex::encode(DEFAULT_JWT_SECRET)).unwrap(); @@ -39,7 +40,7 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< prague_time, }; let kzg = None; - let server: MockServer = MockServer::new_with_config(&handle, config, kzg); + let server: MockServer = MockServer::new_with_config(&handle, config, spec, kzg); if all_payloads_valid { eprintln!( diff --git a/testing/node_test_rig/src/lib.rs b/testing/node_test_rig/src/lib.rs index ac01c84b9d0..6e632ccf549 100644 --- a/testing/node_test_rig/src/lib.rs +++ b/testing/node_test_rig/src/lib.rs @@ -7,6 +7,7 @@ use environment::RuntimeContext; use eth2::{reqwest::ClientBuilder, BeaconNodeHttpClient, Timeouts}; use sensitive_url::SensitiveUrl; use std::path::PathBuf; +use std::sync::Arc; use std::time::Duration; use std::time::{SystemTime, UNIX_EPOCH}; use tempfile::{Builder as TempBuilder, TempDir}; @@ -248,8 +249,14 @@ impl LocalExecutionNode { if let Err(e) = std::fs::write(jwt_file_path, config.jwt_key.hex_string()) { panic!("Failed to write jwt file {}", e); } + let spec = Arc::new(E::default_spec()); Self { - server: MockServer::new_with_config(&context.executor.handle().unwrap(), config, None), + server: MockServer::new_with_config( + &context.executor.handle().unwrap(), + config, + spec, + None, + ), datadir, } } From f66e179a40c3917eee39a93534ecf75480172699 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 17:17:17 +1100 Subject: [PATCH 17/31] Fix release tests --- beacon_node/beacon_chain/tests/store_tests.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index e1258ccdea7..d32bbdfeaa4 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -2341,8 +2341,10 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { let kzg = get_kzg(&spec); - let mock = - mock_execution_layer_from_parts(&harness.spec, harness.runtime.task_executor.clone()); + let mock = mock_execution_layer_from_parts( + harness.spec.clone(), + harness.runtime.task_executor.clone(), + ); // Initialise a new beacon chain from the finalized checkpoint. // The slot clock must be set to a time ahead of the checkpoint state. From 440e85419940d4daba406d910e7908dd1fe78668 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 17:24:50 +1100 Subject: [PATCH 18/31] Move RuntimeFixedVector into module and rename --- .../overflow_lru_cache.rs | 40 +++++----- consensus/types/src/blob_sidecar.rs | 9 +-- consensus/types/src/lib.rs | 2 + consensus/types/src/runtime_fixed_vector.rs | 78 ++++++++++++++++++ consensus/types/src/runtime_var_list.rs | 79 ------------------- 5 files changed, 102 insertions(+), 106 deletions(-) create mode 100644 consensus/types/src/runtime_fixed_vector.rs 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 4cb425b74a9..6245d199350 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 @@ -13,10 +13,9 @@ use slog::{debug, Logger}; use std::num::NonZeroUsize; use std::sync::Arc; use types::blob_sidecar::BlobIdentifier; -use types::runtime_var_list::RuntimeFixedList; use types::{ BlobSidecar, ChainSpec, ColumnIndex, DataColumnIdentifier, DataColumnSidecar, Epoch, EthSpec, - Hash256, RuntimeVariableList, SignedBeaconBlock, + Hash256, RuntimeFixedVector, RuntimeVariableList, SignedBeaconBlock, }; /// This represents the components of a partially available block @@ -28,7 +27,7 @@ use types::{ #[derive(Clone)] pub struct PendingComponents { pub block_root: Hash256, - pub verified_blobs: RuntimeFixedList>>, + pub verified_blobs: RuntimeFixedVector>>, pub verified_data_columns: Vec>, pub executed_block: Option>, pub reconstruction_started: bool, @@ -41,7 +40,7 @@ impl PendingComponents { } /// Returns an immutable reference to the fixed vector of cached blobs. - pub fn get_cached_blobs(&self) -> &RuntimeFixedList>> { + pub fn get_cached_blobs(&self) -> &RuntimeFixedVector>> { &self.verified_blobs } @@ -62,7 +61,7 @@ impl PendingComponents { } /// Returns a mutable reference to the fixed vector of cached blobs. - pub fn get_cached_blobs_mut(&mut self) -> &mut RuntimeFixedList>> { + pub fn get_cached_blobs_mut(&mut self) -> &mut RuntimeFixedVector>> { &mut self.verified_blobs } @@ -134,7 +133,7 @@ impl PendingComponents { /// Blobs are only inserted if: /// 1. The blob entry at the index is empty and no block exists. /// 2. The block exists and its commitment matches the blob's commitment. - pub fn merge_blobs(&mut self, blobs: RuntimeFixedList>>) { + pub fn merge_blobs(&mut self, blobs: RuntimeFixedVector>>) { for (index, blob) in blobs.iter().cloned().enumerate() { let Some(blob) = blob else { continue }; self.merge_single_blob(index, blob); @@ -236,8 +235,7 @@ impl PendingComponents { pub fn empty(block_root: Hash256, max_len: usize) -> Self { Self { block_root, - // TODO(pawan): just make this a vec potentially - verified_blobs: RuntimeFixedList::new(vec![None; max_len]), + verified_blobs: RuntimeFixedVector::new(vec![None; max_len]), verified_data_columns: vec![], executed_block: None, reconstruction_started: false, @@ -466,7 +464,7 @@ impl DataAvailabilityCheckerInner { }; let mut fixed_blobs = - RuntimeFixedList::new(vec![None; self.spec.max_blobs_per_block(epoch) as usize]); + RuntimeFixedVector::new(vec![None; self.spec.max_blobs_per_block(epoch) as usize]); for blob in kzg_verified_blobs { if let Some(blob_opt) = fixed_blobs.get_mut(blob.blob_index() as usize) { @@ -1169,8 +1167,8 @@ mod pending_components_tests { type Setup = ( SignedBeaconBlock, - RuntimeFixedList>>>, - RuntimeFixedList>>>, + RuntimeFixedVector>>>, + RuntimeFixedVector>>>, usize, ); @@ -1180,8 +1178,8 @@ mod pending_components_tests { let (block, blobs_vec) = generate_rand_block_and_blobs::(ForkName::Deneb, NumBlobs::Random, &mut rng, &spec); let max_len = spec.max_blobs_per_block(block.epoch()) as usize; - let mut blobs: RuntimeFixedList>>> = - RuntimeFixedList::default(max_len); + let mut blobs: RuntimeFixedVector>>> = + RuntimeFixedVector::default(max_len); for blob in blobs_vec { if let Some(b) = blobs.get_mut(blob.index as usize) { @@ -1189,8 +1187,8 @@ mod pending_components_tests { } } - let mut invalid_blobs: RuntimeFixedList>>> = - RuntimeFixedList::default(max_len); + let mut invalid_blobs: RuntimeFixedVector>>> = + RuntimeFixedVector::default(max_len); for (index, blob) in blobs.iter().enumerate() { if let Some(invalid_blob) = blob { let mut blob_copy = invalid_blob.as_ref().clone(); @@ -1204,16 +1202,16 @@ mod pending_components_tests { type PendingComponentsSetup = ( DietAvailabilityPendingExecutedBlock, - RuntimeFixedList>>, - RuntimeFixedList>>, + RuntimeFixedVector>>, + RuntimeFixedVector>>, ); pub fn setup_pending_components( block: SignedBeaconBlock, - valid_blobs: RuntimeFixedList>>>, - invalid_blobs: RuntimeFixedList>>>, + valid_blobs: RuntimeFixedVector>>>, + invalid_blobs: RuntimeFixedVector>>>, ) -> PendingComponentsSetup { - let blobs = RuntimeFixedList::new( + let blobs = RuntimeFixedVector::new( valid_blobs .iter() .map(|blob_opt| { @@ -1223,7 +1221,7 @@ mod pending_components_tests { }) .collect::>(), ); - let invalid_blobs = RuntimeFixedList::new( + let invalid_blobs = RuntimeFixedVector::new( invalid_blobs .iter() .map(|blob_opt| { diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 67ca5cbd56e..e241fd4b84e 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -1,11 +1,8 @@ use crate::test_utils::TestRandom; use crate::{ beacon_block_body::BLOB_KZG_COMMITMENTS_INDEX, BeaconBlockHeader, BeaconStateError, Blob, - Epoch, EthSpec, FixedVector, Hash256, SignedBeaconBlockHeader, Slot, VariableList, -}; -use crate::{ - runtime_var_list::RuntimeFixedList, ForkVersionDeserialize, KzgProofs, RuntimeVariableList, - SignedBeaconBlock, + Epoch, EthSpec, FixedVector, ForkVersionDeserialize, Hash256, KzgProofs, RuntimeFixedVector, + RuntimeVariableList, SignedBeaconBlock, SignedBeaconBlockHeader, Slot, VariableList, }; use crate::{ChainSpec, ForkName}; use bls::Signature; @@ -297,7 +294,7 @@ impl BlobSidecar { pub type BlobSidecarList = RuntimeVariableList>>; /// Alias for a non length-constrained list of `BlobSidecar`s. -pub type FixedBlobSidecarList = RuntimeFixedList>>>; +pub type FixedBlobSidecarList = RuntimeFixedVector>>>; pub type BlobsList = VariableList, ::MaxBlobCommitmentsPerBlock>; impl ForkVersionDeserialize for BlobSidecarList { diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index dd304c6296c..728266f19c3 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -108,6 +108,7 @@ pub mod data_column_sidecar; pub mod data_column_subnet_id; pub mod light_client_header; pub mod non_zero_usize; +pub mod runtime_fixed_vector; pub mod runtime_var_list; pub use crate::activation_queue::ActivationQueue; @@ -219,6 +220,7 @@ pub use crate::preset::{ pub use crate::proposer_preparation_data::ProposerPreparationData; pub use crate::proposer_slashing::ProposerSlashing; pub use crate::relative_epoch::{Error as RelativeEpochError, RelativeEpoch}; +pub use crate::runtime_fixed_vector::RuntimeFixedVector; pub use crate::runtime_var_list::RuntimeVariableList; pub use crate::selection_proof::SelectionProof; pub use crate::shuffling_id::AttestationShufflingId; diff --git a/consensus/types/src/runtime_fixed_vector.rs b/consensus/types/src/runtime_fixed_vector.rs new file mode 100644 index 00000000000..f169df83bf5 --- /dev/null +++ b/consensus/types/src/runtime_fixed_vector.rs @@ -0,0 +1,78 @@ +/// Emulates a SSZ `Vector`. +#[derive(Clone, Debug)] +pub struct RuntimeFixedVector { + vec: Vec, + len: usize, +} + +impl RuntimeFixedVector { + pub fn new(vec: Vec) -> Self { + let len = vec.len(); + Self { vec, len } + } + + pub fn to_vec(&self) -> Vec { + self.vec.clone() + } + + pub fn as_slice(&self) -> &[T] { + self.vec.as_slice() + } + + #[allow(clippy::len_without_is_empty)] + pub fn len(&self) -> usize { + self.len + } + + pub fn into_vec(self) -> Vec { + self.vec + } + + pub fn default(max_len: usize) -> Self { + Self { + vec: vec![T::default(); max_len], + len: max_len, + } + } + + pub fn take(&mut self) -> Self { + let new = std::mem::take(&mut self.vec); + *self = Self::new(vec![T::default(); self.len]); + Self { + vec: new, + len: self.len, + } + } +} + +impl std::ops::Deref for RuntimeFixedVector { + type Target = [T]; + + fn deref(&self) -> &[T] { + &self.vec[..] + } +} + +impl std::ops::DerefMut for RuntimeFixedVector { + fn deref_mut(&mut self) -> &mut [T] { + &mut self.vec[..] + } +} + +impl IntoIterator for RuntimeFixedVector { + type Item = T; + type IntoIter = std::vec::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.vec.into_iter() + } +} + +impl<'a, T> IntoIterator for &'a RuntimeFixedVector { + type Item = &'a T; + type IntoIter = std::slice::Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.vec.iter() + } +} diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 54551aa46f8..db13da01f65 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -262,85 +262,6 @@ where } } -/// Emulates a SSZ `Vector`. -#[derive(Clone, Debug)] -pub struct RuntimeFixedList { - vec: Vec, - len: usize, -} - -impl RuntimeFixedList { - pub fn new(vec: Vec) -> Self { - let len = vec.len(); - Self { vec, len } - } - - pub fn to_vec(&self) -> Vec { - self.vec.clone() - } - - pub fn as_slice(&self) -> &[T] { - self.vec.as_slice() - } - - #[allow(clippy::len_without_is_empty)] - pub fn len(&self) -> usize { - self.len - } - - pub fn into_vec(self) -> Vec { - self.vec - } - - pub fn default(max_len: usize) -> Self { - Self { - vec: vec![T::default(); max_len], - len: max_len, - } - } - - pub fn take(&mut self) -> Self { - let new = std::mem::take(&mut self.vec); - *self = Self::new(vec![T::default(); self.len]); - Self { - vec: new, - len: self.len, - } - } -} - -impl std::ops::Deref for RuntimeFixedList { - type Target = [T]; - - fn deref(&self) -> &[T] { - &self.vec[..] - } -} - -impl std::ops::DerefMut for RuntimeFixedList { - fn deref_mut(&mut self) -> &mut [T] { - &mut self.vec[..] - } -} - -impl IntoIterator for RuntimeFixedList { - type Item = T; - type IntoIter = std::vec::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - self.vec.into_iter() - } -} - -impl<'a, T> IntoIterator for &'a RuntimeFixedList { - type Item = &'a T; - type IntoIter = std::slice::Iter<'a, T>; - - fn into_iter(self) -> Self::IntoIter { - self.vec.iter() - } -} - #[cfg(test)] mod test { use super::*; From 04b3743ec1e0b650269dd8e58b540c02430d1c0d Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 6 Jan 2025 17:36:58 +1100 Subject: [PATCH 19/31] Add test --- beacon_node/lighthouse_network/src/rpc/methods.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/beacon_node/lighthouse_network/src/rpc/methods.rs b/beacon_node/lighthouse_network/src/rpc/methods.rs index a27637a1bbc..e35af6fb40a 100644 --- a/beacon_node/lighthouse_network/src/rpc/methods.rs +++ b/beacon_node/lighthouse_network/src/rpc/methods.rs @@ -863,3 +863,16 @@ impl slog::KV for StatusMessage { slog::Result::Ok(()) } } + +#[cfg(test)] +mod test { + use super::*; + use types::{ForkName, MainnetEthSpec}; + + #[test] + fn max_blobs_per_block_ceiling() { + let spec = MainnetEthSpec::default_spec(); + let latest_fork = ForkName::latest(); + assert!(spec.max_blobs_per_block_by_fork(latest_fork) <= MAX_BLOBS_PER_BLOCK_CEILING); + } +} From 0cd263fb0edba3b7bf0449a02c3b2e820968e5ad Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 6 Jan 2025 17:11:27 -0800 Subject: [PATCH 20/31] Remove empty RuntimeVarList awefullness --- beacon_node/beacon_chain/src/beacon_chain.rs | 16 ++-- beacon_node/beacon_chain/src/builder.rs | 2 +- .../beacon_chain/src/historical_blocks.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 8 +- .../beacon_chain/tests/block_verification.rs | 6 +- beacon_node/http_api/src/block_id.rs | 27 ++++-- beacon_node/store/src/hot_cold_store.rs | 92 ++++++++++++++++--- beacon_node/store/src/lib.rs | 4 +- consensus/types/src/runtime_var_list.rs | 63 +++---------- 9 files changed, 125 insertions(+), 95 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 7155547d01b..35c0629451c 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -115,6 +115,7 @@ use std::io::prelude::*; use std::marker::PhantomData; use std::sync::Arc; use std::time::Duration; +use store::hot_cold_store::BlobsSidecarListFromRoot; use store::iter::{BlockRootsIterator, ParentRootBlockIterator, StateRootsIterator}; use store::{ DatabaseBlock, Error as DBError, HotColdDB, KeyValueStore, KeyValueStoreOp, StoreItem, StoreOp, @@ -1147,9 +1148,10 @@ impl BeaconChain { pub fn get_blobs_checking_early_attester_cache( &self, block_root: &Hash256, - ) -> Result, Error> { + ) -> Result, Error> { self.early_attester_cache .get_blobs(*block_root) + .map(Into::into) .map_or_else(|| self.get_blobs(block_root), Ok) } @@ -1240,11 +1242,11 @@ impl BeaconChain { /// /// ## Errors /// May return a database error. - pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { - match self.store.get_blobs(block_root)? { - Some(blobs) => Ok(blobs), - None => Ok(BlobSidecarList::empty_uninitialized()), - } + pub fn get_blobs( + &self, + block_root: &Hash256, + ) -> Result, Error> { + self.store.get_blobs(block_root).map_err(Error::from) } /// Returns the data columns at the given root, if any. @@ -3948,7 +3950,7 @@ impl BeaconChain { "block_root" => %block_root, "count" => blobs.len(), ); - ops.push(StoreOp::PutBlobs(block_root, blobs)); + ops.push(StoreOp::PutBlobs(block_root, blobs.into())); } } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 9d99ff9d8e0..78c4048d151 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -563,7 +563,7 @@ where .map_err(|e| format!("Failed to store weak subjectivity block: {e:?}"))?; if let Some(blobs) = weak_subj_blobs { store - .put_blobs(&weak_subj_block_root, blobs) + .put_blobs(&weak_subj_block_root, blobs.into()) .map_err(|e| format!("Failed to store weak subjectivity blobs: {e:?}"))?; } diff --git a/beacon_node/beacon_chain/src/historical_blocks.rs b/beacon_node/beacon_chain/src/historical_blocks.rs index ddae54f464b..50201e70d2f 100644 --- a/beacon_node/beacon_chain/src/historical_blocks.rs +++ b/beacon_node/beacon_chain/src/historical_blocks.rs @@ -141,7 +141,7 @@ impl BeaconChain { if let Some(blobs) = maybe_blobs { new_oldest_blob_slot = Some(block.slot()); self.store - .blobs_as_kv_store_ops(&block_root, blobs, &mut blob_batch); + .blobs_as_kv_store_ops(&block_root, blobs.into(), &mut blob_batch); } // Store the data columns too if let Some(data_columns) = maybe_data_columns { diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index a722669c2d8..aab51cbe60b 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -742,15 +742,15 @@ where pub fn get_head_block(&self) -> RpcBlock { let block = self.chain.head_beacon_block(); let block_root = block.canonical_root(); - let blobs = self.chain.get_blobs(&block_root).unwrap(); - RpcBlock::new(Some(block_root), block, Some(blobs)).unwrap() + let blobs = self.chain.get_blobs(&block_root).unwrap().blobs(); + RpcBlock::new(Some(block_root), block, blobs).unwrap() } pub fn get_full_block(&self, block_root: &Hash256) -> RpcBlock { let block = self.chain.get_blinded_block(block_root).unwrap().unwrap(); let full_block = self.chain.store.make_full_block(block_root, block).unwrap(); - let blobs = self.chain.get_blobs(block_root).unwrap(); - RpcBlock::new(Some(*block_root), Arc::new(full_block), Some(blobs)).unwrap() + let blobs = self.chain.get_blobs(block_root).unwrap().blobs(); + RpcBlock::new(Some(*block_root), Arc::new(full_block), blobs).unwrap() } pub fn get_all_validators(&self) -> Vec { diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 415aed5831d..deb435f0ece 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -206,11 +206,7 @@ fn update_blob_signed_header( signed_block: &SignedBeaconBlock, blobs: &mut BlobSidecarList, ) { - // Required to prevent as_mut_slice being called on an uninitialized list. Yuck. - if blobs.is_empty() { - return; - } - for old_blob_sidecar in blobs.as_mut_slice().unwrap() { + for old_blob_sidecar in blobs.as_mut_slice() { let new_blob = Arc::new(BlobSidecar:: { index: old_blob_sidecar.index, blob: old_blob_sidecar.blob.clone(), diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index 6db05c6b80e..eb718f3f25b 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -284,29 +284,36 @@ impl BlockId { ) })?; + let max_len = chain.spec.max_blobs_per_block(block.epoch()) as usize; // Return the `BlobSidecarList` identified by `self`. let blob_sidecar_list = if !blob_kzg_commitments.is_empty() { - chain + match chain .store .get_blobs(&root) .map_err(|e| warp_utils::reject::beacon_chain_error(e.into()))? - .ok_or_else(|| { - warp_utils::reject::custom_not_found(format!( - "no blobs stored for block {root}" - )) - })? + .blobs() + { + Some(list) => list, + None => { + return Err(warp_utils::reject::custom_not_found(format!( + "no root found for block {root}" + ))) + } + } } else { - BlobSidecarList::empty_uninitialized() + BlobSidecarList::new(vec![], max_len) + .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? }; let blob_sidecar_list_filtered = match indices.indices { Some(vec) => { let list: Vec<_> = blob_sidecar_list - .into_iter() + .iter() .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) + .cloned() .collect(); - let max_len = chain.spec.max_blobs_per_block(block.epoch()); - BlobSidecarList::new(list, max_len as usize) + + BlobSidecarList::new(list, max_len) .map_err(|e| warp_utils::reject::custom_server_error(format!("{:?}", e)))? } None => blob_sidecar_list, diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index b313051b410..22a928d7535 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -43,6 +43,63 @@ use types::data_column_sidecar::{ColumnIndex, DataColumnSidecar, DataColumnSidec use types::*; use zstd::{Decoder, Encoder}; +#[derive(Debug, Clone)] +pub enum BlobsSidecarListFromRoot { + /// Valid root that exists in the DB, but has no blobs associated with it. + NoBlobs, + /// Contains > 1 blob for the requested root. + Blobs(BlobSidecarList), + /// No root exists in the db or cache for the requested root. + NoRoot, +} + +impl From> for BlobsSidecarListFromRoot { + fn from(value: BlobSidecarList) -> Self { + Self::Blobs(value) + } +} + +impl Encode for BlobsSidecarListFromRoot { + fn as_ssz_bytes(&self) -> Vec { + match self { + Self::NoBlobs | Self::NoRoot => vec![], + Self::Blobs(list) => list.as_ssz_bytes(), + } + } + fn is_ssz_fixed_len() -> bool { + false + } + + fn ssz_append(&self, buf: &mut Vec) { + match self { + Self::NoBlobs | Self::NoRoot => {} + Self::Blobs(blobs) => blobs.ssz_append(buf), + } + } + + fn ssz_bytes_len(&self) -> usize { + match self { + Self::NoBlobs | Self::NoRoot => 0, + Self::Blobs(blobs) => blobs.ssz_bytes_len(), + } + } +} + +impl BlobsSidecarListFromRoot { + pub fn blobs(self) -> Option> { + match self { + Self::NoBlobs | Self::NoRoot => None, + Self::Blobs(blobs) => Some(blobs), + } + } + pub fn iter(&self) -> impl Iterator>> { + match self { + Self::NoBlobs | Self::NoRoot => [].iter(), + Self::Blobs(list) => list.iter(), + } + } +} + /// On-disk database that stores finalized states efficiently. /// /// Stores vector fields like the `block_roots` and `state_roots` separately, and only stores @@ -92,7 +149,7 @@ pub struct HotColdDB, Cold: ItemStore> { #[derive(Debug)] struct BlockCache { block_cache: LruCache>, - blob_cache: LruCache>, + blob_cache: LruCache>, data_column_cache: LruCache>>>, } @@ -107,7 +164,7 @@ impl BlockCache { pub fn put_block(&mut self, block_root: Hash256, block: SignedBeaconBlock) { self.block_cache.put(block_root, block); } - pub fn put_blobs(&mut self, block_root: Hash256, blobs: BlobSidecarList) { + pub fn put_blobs(&mut self, block_root: Hash256, blobs: BlobsSidecarListFromRoot) { self.blob_cache.put(block_root, blobs); } pub fn put_data_column(&mut self, block_root: Hash256, data_column: Arc>) { @@ -118,7 +175,10 @@ impl BlockCache { pub fn get_block<'a>(&'a mut self, block_root: &Hash256) -> Option<&'a SignedBeaconBlock> { self.block_cache.get(block_root) } - pub fn get_blobs<'a>(&'a mut self, block_root: &Hash256) -> Option<&'a BlobSidecarList> { + pub fn get_blobs<'a>( + &'a mut self, + block_root: &Hash256, + ) -> Option<&'a BlobsSidecarListFromRoot> { self.blob_cache.get(block_root) } pub fn get_data_column<'a>( @@ -856,7 +916,11 @@ impl, Cold: ItemStore> HotColdDB .key_delete(DBColumn::BeaconBlob.into(), block_root.as_slice()) } - pub fn put_blobs(&self, block_root: &Hash256, blobs: BlobSidecarList) -> Result<(), Error> { + pub fn put_blobs( + &self, + block_root: &Hash256, + blobs: BlobsSidecarListFromRoot, + ) -> Result<(), Error> { self.blobs_db.put_bytes( DBColumn::BeaconBlob.into(), block_root.as_slice(), @@ -869,7 +933,7 @@ impl, Cold: ItemStore> HotColdDB pub fn blobs_as_kv_store_ops( &self, key: &Hash256, - blobs: BlobSidecarList, + blobs: BlobsSidecarListFromRoot, ops: &mut Vec, ) { let db_key = get_key_for_col(DBColumn::BeaconBlob.into(), key.as_slice()); @@ -1280,7 +1344,7 @@ impl, Cold: ItemStore> HotColdDB StoreOp::PutBlobs(_, _) | StoreOp::PutDataColumns(_, _) => true, StoreOp::DeleteBlobs(block_root) => { match self.get_blobs(block_root) { - Ok(Some(blob_sidecar_list)) => { + Ok(blob_sidecar_list) => { blobs_to_delete.push((*block_root, blob_sidecar_list)); } Err(e) => { @@ -1290,7 +1354,6 @@ impl, Cold: ItemStore> HotColdDB "error" => ?e ); } - _ => (), } true } @@ -2045,11 +2108,11 @@ impl, Cold: ItemStore> HotColdDB } /// Fetch blobs for a given block from the store. - pub fn get_blobs(&self, block_root: &Hash256) -> Result>, Error> { + pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { // Check the cache. if let Some(blobs) = self.block_cache.lock().get_blobs(block_root) { metrics::inc_counter(&metrics::BEACON_BLOBS_CACHE_HIT_COUNT); - return Ok(Some(blobs.clone())); + return Ok(blobs.clone()); } match self @@ -2066,17 +2129,20 @@ impl, Cold: ItemStore> HotColdDB .first() .map(|blob| self.spec.max_blobs_per_block(blob.epoch())) { - BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize) + BlobsSidecarListFromRoot::Blobs(BlobSidecarList::from_vec( + blobs, + max_blobs_per_block as usize, + )) } else { // This always implies that there were no blobs for this block_root - BlobSidecarList::empty_uninitialized() + BlobsSidecarListFromRoot::NoBlobs }; self.block_cache .lock() .put_blobs(*block_root, blobs.clone()); - Ok(Some(blobs)) + Ok(blobs) } - None => Ok(None), + None => Ok(BlobsSidecarListFromRoot::NoRoot), } } diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 09ae9a32dd0..6b08f8ff1d8 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -30,7 +30,7 @@ pub mod iter; pub use self::config::StoreConfig; pub use self::consensus_context::OnDiskConsensusContext; -pub use self::hot_cold_store::{HotColdDB, HotStateSummary, Split}; +pub use self::hot_cold_store::{BlobsSidecarListFromRoot, HotColdDB, HotStateSummary, Split}; pub use self::leveldb_store::LevelDB; pub use self::memory_store::MemoryStore; pub use crate::metadata::BlobInfo; @@ -230,7 +230,7 @@ pub trait ItemStore: KeyValueStore + Sync + Send + Sized + 'stati pub enum StoreOp<'a, E: EthSpec> { PutBlock(Hash256, Arc>), PutState(Hash256, &'a BeaconState), - PutBlobs(Hash256, BlobSidecarList), + PutBlobs(Hash256, BlobsSidecarListFromRoot), PutDataColumns(Hash256, DataColumnSidecarList), PutStateSummary(Hash256, HotStateSummary), PutStateTemporaryFlag(Hash256), diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index db13da01f65..8025b54c008 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -10,9 +10,6 @@ use std::slice::SliceIndex; /// An ordered, heap-allocated, variable-length, homogeneous collection of `T`, with no more than /// `max_len` values. /// -/// In cases where the `max_length` of the container is unknown at time of initialization, we provide -/// a `Self::empty_uninitialized` constructor that initializes a runtime list without setting the max_len. -/// /// To ensure there are no inconsistent states, we do not allow any mutating operation if `max_len` is not set. /// /// ## Example @@ -41,11 +38,6 @@ use std::slice::SliceIndex; /// // Push a value to if it _does_ exceed the maximum. /// assert!(long.push(6).is_err()); /// -/// let mut uninit = RuntimeVariableList::empty_uninitialized(); -/// assert!(uninit.push(5).is_err()); -/// -/// // Set max_len to allow mutation. -/// uninit.set_max_len(5usize); /// /// uninit.push(5).unwrap(); /// assert_eq!(&uninit[..], &[5]); @@ -56,10 +48,8 @@ use std::slice::SliceIndex; #[serde(transparent)] pub struct RuntimeVariableList { vec: Vec, - /// A `None` here indicates an uninitialized `Self`. - /// No mutating operation will be allowed until `max_len` is Some #[serde(skip)] - max_len: Option, + max_len: usize, } impl RuntimeVariableList { @@ -67,10 +57,7 @@ impl RuntimeVariableList { /// `Err(OutOfBounds { .. })`. pub fn new(vec: Vec, max_len: usize) -> Result { if vec.len() <= max_len { - Ok(Self { - vec, - max_len: Some(max_len), - }) + Ok(Self { vec, max_len }) } else { Err(Error::OutOfBounds { i: vec.len(), @@ -82,17 +69,14 @@ impl RuntimeVariableList { pub fn from_vec(mut vec: Vec, max_len: usize) -> Self { vec.truncate(max_len); - Self { - vec, - max_len: Some(max_len), - } + Self { vec, max_len } } /// Create an empty list with the given `max_len`. pub fn empty(max_len: usize) -> Self { Self { vec: vec![], - max_len: Some(max_len), + max_len, } } @@ -100,26 +84,8 @@ impl RuntimeVariableList { self.vec.as_slice() } - pub fn as_mut_slice(&mut self) -> Option<&mut [T]> { - if self.max_len.is_none() { - return None; - }; - Some(self.vec.as_mut_slice()) - } - - /// Returns an instance of `Self` with max_len = None. - /// - /// No mutating operation can be performed on an uninitialized instance - /// without first setting `max_len`. - pub fn empty_uninitialized() -> Self { - Self { - vec: vec![], - max_len: None, - } - } - - pub fn set_max_len(&mut self, max_len: usize) { - self.max_len = Some(max_len); + pub fn as_mut_slice(&mut self) -> &mut [T] { + self.vec.as_mut_slice() } /// Returns the number of values presently in `self`. @@ -135,7 +101,7 @@ impl RuntimeVariableList { /// Returns the type-level maximum length. /// /// Returns `None` if self is uninitialized with a max_len. - pub fn max_len(&self) -> Option { + pub fn max_len(&self) -> usize { self.max_len } @@ -143,17 +109,13 @@ impl RuntimeVariableList { /// /// Returns `Err(())` when appending `value` would exceed the maximum length. pub fn push(&mut self, value: T) -> Result<(), Error> { - let Some(max_len) = self.max_len else { - // TODO(pawan): set a better error? - return Err(Error::MissingLengthInformation); - }; - if self.vec.len() < max_len { + if self.vec.len() < self.max_len { self.vec.push(value); Ok(()) } else { Err(Error::OutOfBounds { i: self.vec.len().saturating_add(1), - len: max_len, + len: self.max_len, }) } } @@ -186,10 +148,7 @@ impl RuntimeVariableList { } else { ssz::decode_list_of_variable_length_items(bytes, Some(max_len))? }; - Ok(Self { - vec, - max_len: Some(max_len), - }) + Ok(Self { vec, max_len }) } } @@ -331,7 +290,7 @@ mod test { } fn round_trip(item: RuntimeVariableList) { - let max_len = item.max_len().unwrap(); + let max_len = item.max_len(); let encoded = &item.as_ssz_bytes(); assert_eq!(item.ssz_bytes_len(), encoded.len()); assert_eq!( From 7c215f8181740a200c4c809d4ab2b0386d0a2d11 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 6 Jan 2025 18:27:34 -0800 Subject: [PATCH 21/31] Fix tests --- .../tests/attestation_production.rs | 9 ++++---- .../beacon_chain/tests/block_verification.rs | 12 ++++++---- beacon_node/beacon_chain/tests/store_tests.rs | 23 +++++++++++++------ beacon_node/store/src/hot_cold_store.rs | 7 ++++++ 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/beacon_node/beacon_chain/tests/attestation_production.rs b/beacon_node/beacon_chain/tests/attestation_production.rs index 87fefe71146..60001159938 100644 --- a/beacon_node/beacon_chain/tests/attestation_production.rs +++ b/beacon_node/beacon_chain/tests/attestation_production.rs @@ -155,7 +155,7 @@ async fn produces_attestations() { .store .make_full_block(&block_root, blinded_block) .unwrap(); - let blobs = chain.get_blobs(&block_root).unwrap(); + let blobs = chain.get_blobs(&block_root).unwrap().blobs(); let epoch_boundary_slot = state .current_epoch() @@ -223,7 +223,7 @@ async fn produces_attestations() { assert_eq!(data.target.root, target_root, "bad target root"); let rpc_block = - RpcBlock::::new(None, Arc::new(block.clone()), Some(blobs.clone())) + RpcBlock::::new(None, Arc::new(block.clone()), blobs.clone()) .unwrap(); let beacon_chain::data_availability_checker::MaybeAvailableBlock::Available( available_block, @@ -299,10 +299,11 @@ async fn early_attester_cache_old_request() { let head_blobs = harness .chain .get_blobs(&head.beacon_block_root) - .expect("should get blobs"); + .expect("should get blobs") + .blobs(); let rpc_block = - RpcBlock::::new(None, head.beacon_block.clone(), Some(head_blobs)).unwrap(); + RpcBlock::::new(None, head.beacon_block.clone(), head_blobs).unwrap(); let beacon_chain::data_availability_checker::MaybeAvailableBlock::Available(available_block) = harness .chain diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index deb435f0ece..b276da2b45a 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -65,12 +65,13 @@ async fn get_chain_segment() -> (Vec>, Vec, checkpoint_slot: Slot) { .get_full_block(&wss_block_root) .unwrap() .unwrap(); - let wss_blobs_opt = harness.chain.store.get_blobs(&wss_block_root).unwrap(); + let wss_blobs_opt = harness + .chain + .store + .get_blobs(&wss_block_root) + .unwrap() + .blobs(); let wss_state = full_store .get_state(&wss_state_root, Some(checkpoint_slot)) .unwrap() @@ -2389,7 +2394,11 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { .await .unwrap() .unwrap(); - let store_wss_blobs_opt = beacon_chain.store.get_blobs(&wss_block_root).unwrap(); + let store_wss_blobs_opt = beacon_chain + .store + .get_blobs(&wss_block_root) + .unwrap() + .blobs(); assert_eq!(store_wss_block, wss_block); assert_eq!(store_wss_blobs_opt, wss_blobs_opt); @@ -2408,7 +2417,7 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { .await .unwrap() .unwrap(); - let blobs = harness.chain.get_blobs(&block_root).expect("blobs"); + let blobs = harness.chain.get_blobs(&block_root).expect("blobs").blobs(); let slot = full_block.slot(); let state_root = full_block.state_root(); @@ -2416,7 +2425,7 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { beacon_chain .process_block( full_block.canonical_root(), - RpcBlock::new(Some(block_root), Arc::new(full_block), Some(blobs)).unwrap(), + RpcBlock::new(Some(block_root), Arc::new(full_block), blobs).unwrap(), NotifyExecutionLayer::Yes, BlockImportSource::Lookup, || Ok(()), @@ -2470,13 +2479,13 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { .await .expect("should get block") .expect("should get block"); - let blobs = harness.chain.get_blobs(&block_root).expect("blobs"); + let blobs = harness.chain.get_blobs(&block_root).expect("blobs").blobs(); if let MaybeAvailableBlock::Available(block) = harness .chain .data_availability_checker .verify_kzg_for_rpc_block( - RpcBlock::new(Some(block_root), Arc::new(full_block), Some(blobs)).unwrap(), + RpcBlock::new(Some(block_root), Arc::new(full_block), blobs).unwrap(), ) .expect("should verify kzg") { @@ -3352,7 +3361,7 @@ fn check_blob_existence( .unwrap() .map(Result::unwrap) { - if let Some(blobs) = harness.chain.store.get_blobs(&block_root).unwrap() { + if let Some(blobs) = harness.chain.store.get_blobs(&block_root).unwrap().blobs() { assert!(should_exist, "blobs at slot {slot} exist but should not"); blobs_seen += blobs.len(); } else { diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 22a928d7535..7e13612b183 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -92,6 +92,13 @@ impl BlobsSidecarListFromRoot { Self::Blobs(blobs) => Some(blobs), } } + #[allow(clippy::len_without_is_empty)] + pub fn len(&self) -> usize { + match self { + Self::NoBlobs | Self::NoRoot => 0, + Self::Blobs(blobs) => blobs.len(), + } + } pub fn iter(&self) -> impl Iterator>> { match self { Self::NoBlobs | Self::NoRoot => [].iter(), From 26c409c3a556e131d820fadad1a24d5782ae9797 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 7 Jan 2025 17:19:37 +1100 Subject: [PATCH 22/31] Simplify BlobSidecarListFromRoot --- beacon_node/beacon_chain/src/beacon_chain.rs | 10 +- beacon_node/beacon_chain/src/builder.rs | 2 +- .../beacon_chain/src/historical_blocks.rs | 2 +- .../store/src/blob_sidecar_list_from_root.rs | 42 +++++++ beacon_node/store/src/hot_cold_store.rs | 114 ++++-------------- beacon_node/store/src/lib.rs | 6 +- 6 files changed, 74 insertions(+), 102 deletions(-) create mode 100644 beacon_node/store/src/blob_sidecar_list_from_root.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 35c0629451c..c3506617ed7 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -115,10 +115,10 @@ use std::io::prelude::*; use std::marker::PhantomData; use std::sync::Arc; use std::time::Duration; -use store::hot_cold_store::BlobsSidecarListFromRoot; use store::iter::{BlockRootsIterator, ParentRootBlockIterator, StateRootsIterator}; use store::{ - DatabaseBlock, Error as DBError, HotColdDB, KeyValueStore, KeyValueStoreOp, StoreItem, StoreOp, + BlobSidecarListFromRoot, DatabaseBlock, Error as DBError, HotColdDB, KeyValueStore, + KeyValueStoreOp, StoreItem, StoreOp, }; use task_executor::{ShutdownReason, TaskExecutor}; use tokio::sync::mpsc::Receiver; @@ -1148,7 +1148,7 @@ impl BeaconChain { pub fn get_blobs_checking_early_attester_cache( &self, block_root: &Hash256, - ) -> Result, Error> { + ) -> Result, Error> { self.early_attester_cache .get_blobs(*block_root) .map(Into::into) @@ -1245,7 +1245,7 @@ impl BeaconChain { pub fn get_blobs( &self, block_root: &Hash256, - ) -> Result, Error> { + ) -> Result, Error> { self.store.get_blobs(block_root).map_err(Error::from) } @@ -3950,7 +3950,7 @@ impl BeaconChain { "block_root" => %block_root, "count" => blobs.len(), ); - ops.push(StoreOp::PutBlobs(block_root, blobs.into())); + ops.push(StoreOp::PutBlobs(block_root, blobs)); } } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 78c4048d151..9d99ff9d8e0 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -563,7 +563,7 @@ where .map_err(|e| format!("Failed to store weak subjectivity block: {e:?}"))?; if let Some(blobs) = weak_subj_blobs { store - .put_blobs(&weak_subj_block_root, blobs.into()) + .put_blobs(&weak_subj_block_root, blobs) .map_err(|e| format!("Failed to store weak subjectivity blobs: {e:?}"))?; } diff --git a/beacon_node/beacon_chain/src/historical_blocks.rs b/beacon_node/beacon_chain/src/historical_blocks.rs index 50201e70d2f..ddae54f464b 100644 --- a/beacon_node/beacon_chain/src/historical_blocks.rs +++ b/beacon_node/beacon_chain/src/historical_blocks.rs @@ -141,7 +141,7 @@ impl BeaconChain { if let Some(blobs) = maybe_blobs { new_oldest_blob_slot = Some(block.slot()); self.store - .blobs_as_kv_store_ops(&block_root, blobs.into(), &mut blob_batch); + .blobs_as_kv_store_ops(&block_root, blobs, &mut blob_batch); } // Store the data columns too if let Some(data_columns) = maybe_data_columns { diff --git a/beacon_node/store/src/blob_sidecar_list_from_root.rs b/beacon_node/store/src/blob_sidecar_list_from_root.rs new file mode 100644 index 00000000000..de63eaa76ce --- /dev/null +++ b/beacon_node/store/src/blob_sidecar_list_from_root.rs @@ -0,0 +1,42 @@ +use std::sync::Arc; +use types::{BlobSidecar, BlobSidecarList, EthSpec}; + +#[derive(Debug, Clone)] +pub enum BlobSidecarListFromRoot { + /// Valid root that exists in the DB, but has no blobs associated with it. + NoBlobs, + /// Contains > 1 blob for the requested root. + Blobs(BlobSidecarList), + /// No root exists in the db or cache for the requested root. + NoRoot, +} + +impl From> for BlobSidecarListFromRoot { + fn from(value: BlobSidecarList) -> Self { + Self::Blobs(value) + } +} + +impl BlobSidecarListFromRoot { + pub fn blobs(self) -> Option> { + match self { + Self::NoBlobs | Self::NoRoot => None, + Self::Blobs(blobs) => Some(blobs), + } + } + + #[allow(clippy::len_without_is_empty)] + pub fn len(&self) -> usize { + match self { + Self::NoBlobs | Self::NoRoot => 0, + Self::Blobs(blobs) => blobs.len(), + } + } + + pub fn iter(&self) -> impl Iterator>> { + match self { + Self::NoBlobs | Self::NoRoot => [].iter(), + Self::Blobs(list) => list.iter(), + } + } +} diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 7e13612b183..1dc5711d7a2 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -14,8 +14,8 @@ use crate::metadata::{ }; use crate::state_cache::{PutStateOutcome, StateCache}; use crate::{ - get_data_column_key, get_key_for_col, DBColumn, DatabaseBlock, Error, ItemStore, - KeyValueStoreOp, StoreItem, StoreOp, + get_data_column_key, get_key_for_col, BlobSidecarListFromRoot, DBColumn, DatabaseBlock, Error, + ItemStore, KeyValueStoreOp, StoreItem, StoreOp, }; use crate::{metrics, parse_data_column_key}; use itertools::{process_results, Itertools}; @@ -43,70 +43,6 @@ use types::data_column_sidecar::{ColumnIndex, DataColumnSidecar, DataColumnSidec use types::*; use zstd::{Decoder, Encoder}; -#[derive(Debug, Clone)] -pub enum BlobsSidecarListFromRoot { - /// Valid root that exists in the DB, but has no blobs associated with it. - NoBlobs, - /// Contains > 1 blob for the requested root. - Blobs(BlobSidecarList), - /// No root exists in the db or cache for the requested root. - NoRoot, -} - -impl From> for BlobsSidecarListFromRoot { - fn from(value: BlobSidecarList) -> Self { - Self::Blobs(value) - } -} - -impl Encode for BlobsSidecarListFromRoot { - fn as_ssz_bytes(&self) -> Vec { - match self { - Self::NoBlobs | Self::NoRoot => vec![], - Self::Blobs(list) => list.as_ssz_bytes(), - } - } - fn is_ssz_fixed_len() -> bool { - false - } - - fn ssz_append(&self, buf: &mut Vec) { - match self { - Self::NoBlobs | Self::NoRoot => {} - Self::Blobs(blobs) => blobs.ssz_append(buf), - } - } - - fn ssz_bytes_len(&self) -> usize { - match self { - Self::NoBlobs | Self::NoRoot => 0, - Self::Blobs(blobs) => blobs.ssz_bytes_len(), - } - } -} - -impl BlobsSidecarListFromRoot { - pub fn blobs(self) -> Option> { - match self { - Self::NoBlobs | Self::NoRoot => None, - Self::Blobs(blobs) => Some(blobs), - } - } - #[allow(clippy::len_without_is_empty)] - pub fn len(&self) -> usize { - match self { - Self::NoBlobs | Self::NoRoot => 0, - Self::Blobs(blobs) => blobs.len(), - } - } - pub fn iter(&self) -> impl Iterator>> { - match self { - Self::NoBlobs | Self::NoRoot => [].iter(), - Self::Blobs(list) => list.iter(), - } - } -} - /// On-disk database that stores finalized states efficiently. /// /// Stores vector fields like the `block_roots` and `state_roots` separately, and only stores @@ -156,7 +92,7 @@ pub struct HotColdDB, Cold: ItemStore> { #[derive(Debug)] struct BlockCache { block_cache: LruCache>, - blob_cache: LruCache>, + blob_cache: LruCache>, data_column_cache: LruCache>>>, } @@ -171,7 +107,7 @@ impl BlockCache { pub fn put_block(&mut self, block_root: Hash256, block: SignedBeaconBlock) { self.block_cache.put(block_root, block); } - pub fn put_blobs(&mut self, block_root: Hash256, blobs: BlobsSidecarListFromRoot) { + pub fn put_blobs(&mut self, block_root: Hash256, blobs: BlobSidecarList) { self.blob_cache.put(block_root, blobs); } pub fn put_data_column(&mut self, block_root: Hash256, data_column: Arc>) { @@ -182,10 +118,7 @@ impl BlockCache { pub fn get_block<'a>(&'a mut self, block_root: &Hash256) -> Option<&'a SignedBeaconBlock> { self.block_cache.get(block_root) } - pub fn get_blobs<'a>( - &'a mut self, - block_root: &Hash256, - ) -> Option<&'a BlobsSidecarListFromRoot> { + pub fn get_blobs<'a>(&'a mut self, block_root: &Hash256) -> Option<&'a BlobSidecarList> { self.blob_cache.get(block_root) } pub fn get_data_column<'a>( @@ -923,11 +856,7 @@ impl, Cold: ItemStore> HotColdDB .key_delete(DBColumn::BeaconBlob.into(), block_root.as_slice()) } - pub fn put_blobs( - &self, - block_root: &Hash256, - blobs: BlobsSidecarListFromRoot, - ) -> Result<(), Error> { + pub fn put_blobs(&self, block_root: &Hash256, blobs: BlobSidecarList) -> Result<(), Error> { self.blobs_db.put_bytes( DBColumn::BeaconBlob.into(), block_root.as_slice(), @@ -940,7 +869,7 @@ impl, Cold: ItemStore> HotColdDB pub fn blobs_as_kv_store_ops( &self, key: &Hash256, - blobs: BlobsSidecarListFromRoot, + blobs: BlobSidecarList, ops: &mut Vec, ) { let db_key = get_key_for_col(DBColumn::BeaconBlob.into(), key.as_slice()); @@ -1351,9 +1280,10 @@ impl, Cold: ItemStore> HotColdDB StoreOp::PutBlobs(_, _) | StoreOp::PutDataColumns(_, _) => true, StoreOp::DeleteBlobs(block_root) => { match self.get_blobs(block_root) { - Ok(blob_sidecar_list) => { + Ok(BlobSidecarListFromRoot::Blobs(blob_sidecar_list)) => { blobs_to_delete.push((*block_root, blob_sidecar_list)); } + Ok(BlobSidecarListFromRoot::NoBlobs | BlobSidecarListFromRoot::NoRoot) => {} Err(e) => { error!( self.log, "Error getting blobs"; @@ -2115,11 +2045,11 @@ impl, Cold: ItemStore> HotColdDB } /// Fetch blobs for a given block from the store. - pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { + pub fn get_blobs(&self, block_root: &Hash256) -> Result, Error> { // Check the cache. if let Some(blobs) = self.block_cache.lock().get_blobs(block_root) { metrics::inc_counter(&metrics::BEACON_BLOBS_CACHE_HIT_COUNT); - return Ok(blobs.clone()); + return Ok(blobs.clone().into()); } match self @@ -2132,24 +2062,22 @@ impl, Cold: ItemStore> HotColdDB // knowing the slot. // The encoding of a VariableList is the same as a regular vec. let blobs: Vec>> = Vec::<_>::from_ssz_bytes(blobs_bytes)?; - let blobs = if let Some(max_blobs_per_block) = blobs + if let Some(max_blobs_per_block) = blobs .first() .map(|blob| self.spec.max_blobs_per_block(blob.epoch())) { - BlobsSidecarListFromRoot::Blobs(BlobSidecarList::from_vec( - blobs, - max_blobs_per_block as usize, - )) + let blobs = BlobSidecarList::from_vec(blobs, max_blobs_per_block as usize); + self.block_cache + .lock() + .put_blobs(*block_root, blobs.clone()); + + Ok(BlobSidecarListFromRoot::Blobs(blobs)) } else { // This always implies that there were no blobs for this block_root - BlobsSidecarListFromRoot::NoBlobs - }; - self.block_cache - .lock() - .put_blobs(*block_root, blobs.clone()); - Ok(blobs) + Ok(BlobSidecarListFromRoot::NoBlobs) + } } - None => Ok(BlobsSidecarListFromRoot::NoRoot), + None => Ok(BlobSidecarListFromRoot::NoRoot), } } diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 6b08f8ff1d8..1458fa846c6 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -7,6 +7,7 @@ //! //! Provides a simple API for storing/retrieving all types that sometimes needs type-hints. See //! tests for implementation examples. +pub mod blob_sidecar_list_from_root; pub mod chunked_iter; pub mod chunked_vector; pub mod config; @@ -28,9 +29,10 @@ pub mod state_cache; pub mod iter; +pub use self::blob_sidecar_list_from_root::BlobSidecarListFromRoot; pub use self::config::StoreConfig; pub use self::consensus_context::OnDiskConsensusContext; -pub use self::hot_cold_store::{BlobsSidecarListFromRoot, HotColdDB, HotStateSummary, Split}; +pub use self::hot_cold_store::{HotColdDB, HotStateSummary, Split}; pub use self::leveldb_store::LevelDB; pub use self::memory_store::MemoryStore; pub use crate::metadata::BlobInfo; @@ -230,7 +232,7 @@ pub trait ItemStore: KeyValueStore + Sync + Send + Sized + 'stati pub enum StoreOp<'a, E: EthSpec> { PutBlock(Hash256, Arc>), PutState(Hash256, &'a BeaconState), - PutBlobs(Hash256, BlobsSidecarListFromRoot), + PutBlobs(Hash256, BlobSidecarList), PutDataColumns(Hash256, DataColumnSidecarList), PutStateSummary(Hash256, HotStateSummary), PutStateTemporaryFlag(Hash256), From d4e152c260bf8a38f2dbb782931a79e744c6841a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 9 Jan 2025 16:32:03 +1100 Subject: [PATCH 23/31] Bump quota to account for new target (6) --- beacon_node/lighthouse_network/src/rpc/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/config.rs b/beacon_node/lighthouse_network/src/rpc/config.rs index 7b3a59eac7e..75d49e9cb5f 100644 --- a/beacon_node/lighthouse_network/src/rpc/config.rs +++ b/beacon_node/lighthouse_network/src/rpc/config.rs @@ -110,8 +110,8 @@ impl RateLimiterConfig { pub const DEFAULT_BLOCKS_BY_RANGE_QUOTA: Quota = Quota::n_every(128, 10); pub const DEFAULT_BLOCKS_BY_ROOT_QUOTA: Quota = Quota::n_every(128, 10); // `DEFAULT_BLOCKS_BY_RANGE_QUOTA` * (target + 1) to account for high usage - pub const DEFAULT_BLOBS_BY_RANGE_QUOTA: Quota = Quota::n_every(512, 10); - pub const DEFAULT_BLOBS_BY_ROOT_QUOTA: Quota = Quota::n_every(512, 10); + pub const DEFAULT_BLOBS_BY_RANGE_QUOTA: Quota = Quota::n_every(896, 10); + pub const DEFAULT_BLOBS_BY_ROOT_QUOTA: Quota = Quota::n_every(896, 10); // 320 blocks worth of columns for regular node, or 40 blocks for supernode. // Range sync load balances when requesting blocks, and each batch is 32 blocks. pub const DEFAULT_DATA_COLUMNS_BY_RANGE_QUOTA: Quota = Quota::n_every(5120, 10); From a73ecb59429ca9dadaf09a791e0bb7ed247e66ad Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 9 Jan 2025 16:59:31 +1100 Subject: [PATCH 24/31] Remove clone --- beacon_node/http_api/src/block_id.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index be5d38f9d49..0b00958f26a 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -320,9 +320,8 @@ impl BlockId { let blob_sidecar_list_filtered = match indices { Some(vec) => { let list: Vec<_> = blob_sidecar_list - .iter() + .into_iter() .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) - .cloned() .collect(); BlobSidecarList::new(list, max_blobs_per_block) From f13bdfc70842c45ff2575265aaacd9629c281705 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 9 Jan 2025 15:11:53 -0800 Subject: [PATCH 25/31] Fix issue from review --- .../network/src/sync/network_context.rs | 20 ++++++++++++------- .../src/sync/network_context/requests.rs | 1 + consensus/types/src/runtime_fixed_vector.rs | 5 ++++- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index ed413cb4597..e1b2b974ec4 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -963,14 +963,20 @@ impl SyncNetworkContext { let response = self.blobs_by_root_requests.on_response(id, rpc_event); let response = response.map(|res| { res.and_then(|(blobs, seen_timestamp)| { - let max_len = if let Some(blob) = blobs.first() { - self.chain.spec.max_blobs_per_block(blob.epoch()) as usize + if let Some(max_len) = blobs + .first() + .map(|blob| self.chain.spec.max_blobs_per_block(blob.epoch()) as usize) + { + match to_fixed_blob_sidecar_list(blobs, max_len) { + Ok(blobs) => Ok((blobs, seen_timestamp)), + Err(e) => Err(e.into()), + } } else { - 6 - }; - match to_fixed_blob_sidecar_list(blobs, max_len) { - Ok(blobs) => Ok((blobs, seen_timestamp)), - Err(e) => Err(e.into()), + Err(RpcResponseError::VerifyError( + LookupVerifyError::InternalError( + "Requested blobs for a block that has no blobs".to_string(), + ), + )) } }) }); diff --git a/beacon_node/network/src/sync/network_context/requests.rs b/beacon_node/network/src/sync/network_context/requests.rs index b9214bafcd7..4a5a16459d3 100644 --- a/beacon_node/network/src/sync/network_context/requests.rs +++ b/beacon_node/network/src/sync/network_context/requests.rs @@ -28,6 +28,7 @@ pub enum LookupVerifyError { UnrequestedIndex(u64), InvalidInclusionProof, DuplicateData, + InternalError(String), } /// Collection of active requests of a single ReqResp method, i.e. `blocks_by_root` diff --git a/consensus/types/src/runtime_fixed_vector.rs b/consensus/types/src/runtime_fixed_vector.rs index f169df83bf5..2b08b7bf702 100644 --- a/consensus/types/src/runtime_fixed_vector.rs +++ b/consensus/types/src/runtime_fixed_vector.rs @@ -1,4 +1,7 @@ -/// Emulates a SSZ `Vector`. +//! Emulates a fixed size array but with the length set at runtime. +//! +//! The length of the list cannot be changed once it is set. + #[derive(Clone, Debug)] pub struct RuntimeFixedVector { vec: Vec, From 70917f7b409112ce8dbeb5d32fe9885096fd623b Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 9 Jan 2025 17:52:06 -0800 Subject: [PATCH 26/31] Try to remove ugliness --- .../lighthouse_network/src/rpc/codec.rs | 20 ++++++++++-- .../lighthouse_network/src/rpc/handler.rs | 6 ++-- .../lighthouse_network/src/rpc/methods.rs | 31 +++---------------- beacon_node/lighthouse_network/src/rpc/mod.rs | 5 +-- .../lighthouse_network/src/rpc/protocol.rs | 20 +++++++----- .../src/rpc/rate_limiter.rs | 24 +++++++++----- .../src/rpc/self_limiter.rs | 20 +++++++++--- .../network_beacon_processor/rpc_methods.rs | 8 ----- 8 files changed, 71 insertions(+), 63 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 5d86936d41d..9fa1a3adcca 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -186,6 +186,7 @@ impl Decoder for SSZSnappyInboundCodec { handle_rpc_request( self.protocol.versioned_protocol, &decoded_buffer, + self.fork_context.current_fork(), &self.fork_context.spec, ) } @@ -552,6 +553,7 @@ fn handle_length( fn handle_rpc_request( versioned_protocol: SupportedProtocol, decoded_buffer: &[u8], + current_fork: ForkName, spec: &ChainSpec, ) -> Result>, RPCError> { match versioned_protocol { @@ -583,9 +585,21 @@ fn handle_rpc_request( )?, }), ))), - SupportedProtocol::BlobsByRangeV1 => Ok(Some(RequestType::BlobsByRange( - BlobsByRangeRequest::from_ssz_bytes(decoded_buffer)?, - ))), + SupportedProtocol::BlobsByRangeV1 => { + let req = BlobsByRangeRequest::from_ssz_bytes(decoded_buffer)?; + // TODO(pawan): change this to max_blobs_per_rpc_request in the alpha10 PR + let max_blobs = spec.max_blobs_per_block_by_fork(current_fork); + if req.count > max_blobs { + return Err(RPCError::ErrorResponse( + RpcErrorResponse::InvalidRequest, + format!( + "requested exceeded limit. allowed: {}, requested: {}", + max_blobs, req.count + ), + )); + } + Ok(Some(RequestType::BlobsByRange(req))) + } SupportedProtocol::BlobsByRootV1 => { Ok(Some(RequestType::BlobsByRoot(BlobsByRootRequest { blob_ids: RuntimeVariableList::from_ssz_bytes( diff --git a/beacon_node/lighthouse_network/src/rpc/handler.rs b/beacon_node/lighthouse_network/src/rpc/handler.rs index 0a0a6ca754f..3a008df023d 100644 --- a/beacon_node/lighthouse_network/src/rpc/handler.rs +++ b/beacon_node/lighthouse_network/src/rpc/handler.rs @@ -855,7 +855,8 @@ where } let (req, substream) = substream; - let max_responses = req.max_responses(); + let max_responses = + req.max_responses(self.fork_context.current_fork(), &self.fork_context.spec); // store requests that expect responses if max_responses > 0 { @@ -924,7 +925,8 @@ where } // add the stream to substreams if we expect a response, otherwise drop the stream. - let max_responses = request.max_responses(); + let max_responses = + request.max_responses(self.fork_context.current_fork(), &self.fork_context.spec); if max_responses > 0 { let max_remaining_chunks = if request.expect_exactly_one_response() { // Currently enforced only for multiple responses diff --git a/beacon_node/lighthouse_network/src/rpc/methods.rs b/beacon_node/lighthouse_network/src/rpc/methods.rs index e35af6fb40a..500188beefb 100644 --- a/beacon_node/lighthouse_network/src/rpc/methods.rs +++ b/beacon_node/lighthouse_network/src/rpc/methods.rs @@ -15,6 +15,7 @@ use strum::IntoStaticStr; use superstruct::superstruct; use types::blob_sidecar::BlobIdentifier; use types::light_client_update::MAX_REQUEST_LIGHT_CLIENT_UPDATES; +use types::ForkName; use types::{ blob_sidecar::BlobSidecar, ChainSpec, ColumnIndex, DataColumnIdentifier, DataColumnSidecar, Epoch, EthSpec, Hash256, LightClientBootstrap, LightClientFinalityUpdate, @@ -25,13 +26,6 @@ use types::{ pub type MaxErrorLen = U256; pub const MAX_ERROR_LEN: u64 = 256; -/// The max number of blobs we expect in the configs to set for compile time params. -/// Note: This value is an estimate that we should use only for rate limiting, -/// bounds checking and other non-consensus critical operations. -/// -/// For exact value, we should always check the chainspec. -pub const MAX_BLOBS_PER_BLOCK_CEILING: u64 = 16; - /// Wrapper over SSZ List to represent error message in rpc responses. #[derive(Debug, Clone)] pub struct ErrorType(pub VariableList); @@ -334,13 +328,9 @@ pub struct BlobsByRangeRequest { } impl BlobsByRangeRequest { - /// This function provides an upper bound on number of blobs expected in - /// a certain slot range. - /// - /// Note: **must not** use for anything consensus critical, only for - /// bounds checking and rate limiting. - pub fn max_blobs_requested(&self) -> u64 { - self.count.saturating_mul(MAX_BLOBS_PER_BLOCK_CEILING) + pub fn max_blobs_requested(&self, current_fork: ForkName, spec: &ChainSpec) -> u64 { + let max_blobs_per_block = spec.max_blobs_per_block_by_fork(current_fork); + self.count.saturating_mul(max_blobs_per_block) } } @@ -863,16 +853,3 @@ impl slog::KV for StatusMessage { slog::Result::Ok(()) } } - -#[cfg(test)] -mod test { - use super::*; - use types::{ForkName, MainnetEthSpec}; - - #[test] - fn max_blobs_per_block_ceiling() { - let spec = MainnetEthSpec::default_spec(); - let latest_fork = ForkName::latest(); - assert!(spec.max_blobs_per_block_by_fork(latest_fork) <= MAX_BLOBS_PER_BLOCK_CEILING); - } -} diff --git a/beacon_node/lighthouse_network/src/rpc/mod.rs b/beacon_node/lighthouse_network/src/rpc/mod.rs index 7d091da7660..03f1395b8b5 100644 --- a/beacon_node/lighthouse_network/src/rpc/mod.rs +++ b/beacon_node/lighthouse_network/src/rpc/mod.rs @@ -181,12 +181,13 @@ impl RPC { let inbound_limiter = inbound_rate_limiter_config.map(|config| { debug!(log, "Using inbound rate limiting params"; "config" => ?config); - RateLimiter::new_with_config(config.0) + RateLimiter::new_with_config(config.0, fork_context.clone()) .expect("Inbound limiter configuration parameters are valid") }); let self_limiter = outbound_rate_limiter_config.map(|config| { - SelfRateLimiter::new(config, log.clone()).expect("Configuration parameters are valid") + SelfRateLimiter::new(config, fork_context.clone(), log.clone()) + .expect("Configuration parameters are valid") }); RPC { diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index e4e8232e18f..a7c4524a2e1 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -93,7 +93,7 @@ pub static SIGNED_BEACON_BLOCK_DENEB_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_CAPELLA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_deneb_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional offsets for the `ExecutionPayload` - + (::ssz_fixed_len() * MAX_BLOBS_PER_BLOCK_CEILING as usize) + + (::ssz_fixed_len() * MainnetEthSpec::default_spec().max_blobs_per_block_by_fork(ForkName::Deneb) as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field. // @@ -101,7 +101,7 @@ pub static SIGNED_BEACON_BLOCK_ELECTRA_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_ELECTRA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_electra_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional ssz offset for the `ExecutionPayload` field - + (::ssz_fixed_len() * MAX_BLOBS_PER_BLOCK_CEILING as usize) + + (::ssz_fixed_len() * MainnetEthSpec::default_spec().max_blobs_per_block_by_fork(ForkName::Electra) as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field. @@ -603,8 +603,10 @@ impl ProtocolId { Protocol::BlocksByRoot => rpc_block_limits_by_fork(fork_context.current_fork()), Protocol::BlobsByRange => rpc_blob_limits::(), Protocol::BlobsByRoot => rpc_blob_limits::(), - Protocol::DataColumnsByRoot => rpc_data_column_limits::(), - Protocol::DataColumnsByRange => rpc_data_column_limits::(), + Protocol::DataColumnsByRoot => rpc_data_column_limits::(fork_context.current_fork()), + Protocol::DataColumnsByRange => { + rpc_data_column_limits::(fork_context.current_fork()) + } Protocol::Ping => RpcLimits::new( ::ssz_fixed_len(), ::ssz_fixed_len(), @@ -684,10 +686,12 @@ pub fn rpc_blob_limits() -> RpcLimits { } } -pub fn rpc_data_column_limits() -> RpcLimits { +pub fn rpc_data_column_limits(fork_name: ForkName) -> RpcLimits { RpcLimits::new( DataColumnSidecar::::empty().as_ssz_bytes().len(), - DataColumnSidecar::::max_size(MAX_BLOBS_PER_BLOCK_CEILING as usize), + DataColumnSidecar::::max_size( + E::default_spec().max_blobs_per_block_by_fork(fork_name) as usize + ), ) } @@ -786,13 +790,13 @@ impl RequestType { /* These functions are used in the handler for stream management */ /// Maximum number of responses expected for this request. - pub fn max_responses(&self) -> u64 { + pub fn max_responses(&self, current_fork: ForkName, spec: &ChainSpec) -> u64 { match self { RequestType::Status(_) => 1, RequestType::Goodbye(_) => 0, RequestType::BlocksByRange(req) => *req.count(), RequestType::BlocksByRoot(req) => req.block_roots().len() as u64, - RequestType::BlobsByRange(req) => req.max_blobs_requested::(), + RequestType::BlobsByRange(req) => req.max_blobs_requested(current_fork, spec), RequestType::BlobsByRoot(req) => req.blob_ids.len() as u64, RequestType::DataColumnsByRoot(req) => req.data_column_ids.len() as u64, RequestType::DataColumnsByRange(req) => req.max_requested::(), diff --git a/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs b/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs index ecbacc8c112..f9e66504aa8 100644 --- a/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs @@ -6,10 +6,11 @@ use serde::{Deserialize, Serialize}; use std::future::Future; use std::hash::Hash; use std::pin::Pin; +use std::sync::Arc; use std::task::{Context, Poll}; use std::time::{Duration, Instant}; use tokio::time::Interval; -use types::EthSpec; +use types::{ChainSpec, EthSpec, ForkName, ForkContext}; /// Nanoseconds since a given time. // Maintained as u64 to reduce footprint @@ -109,6 +110,7 @@ pub struct RPCRateLimiter { lc_finality_update_rl: Limiter, /// LightClientUpdatesByRange rate limiter. lc_updates_by_range_rl: Limiter, + fork_context: Arc, } /// Error type for non conformant requests @@ -176,7 +178,7 @@ impl RPCRateLimiterBuilder { self } - pub fn build(self) -> Result { + pub fn build(self, fork_context: Arc) -> Result { // get our quotas let ping_quota = self.ping_quota.ok_or("Ping quota not specified")?; let metadata_quota = self.metadata_quota.ok_or("MetaData quota not specified")?; @@ -253,13 +255,14 @@ impl RPCRateLimiterBuilder { lc_finality_update_rl, lc_updates_by_range_rl, init_time: Instant::now(), + fork_context, }) } } pub trait RateLimiterItem { fn protocol(&self) -> Protocol; - fn max_responses(&self) -> u64; + fn max_responses(&self, current_fork: ForkName, spec: &ChainSpec) -> u64; } impl RateLimiterItem for super::RequestType { @@ -267,13 +270,16 @@ impl RateLimiterItem for super::RequestType { self.versioned_protocol().protocol() } - fn max_responses(&self) -> u64 { - self.max_responses() + fn max_responses(&self, current_fork: ForkName, spec: &ChainSpec) -> u64 { + self.max_responses(current_fork, spec) } } impl RPCRateLimiter { - pub fn new_with_config(config: RateLimiterConfig) -> Result { + pub fn new_with_config( + config: RateLimiterConfig, + fork_context: Arc, + ) -> Result { // Destructure to make sure every configuration value is used. let RateLimiterConfig { ping_quota, @@ -316,7 +322,7 @@ impl RPCRateLimiter { Protocol::LightClientUpdatesByRange, light_client_updates_by_range_quota, ) - .build() + .build(fork_context) } /// Get a builder instance. @@ -330,7 +336,9 @@ impl RPCRateLimiter { request: &Item, ) -> Result<(), RateLimitedErr> { let time_since_start = self.init_time.elapsed(); - let tokens = request.max_responses().max(1); + let tokens = request + .max_responses(self.fork_context.current_fork(), &self.fork_context.spec) + .max(1); let check = |limiter: &mut Limiter| limiter.allows(time_since_start, peer_id, tokens); diff --git a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs index e968ad11e3d..e0c8593f29f 100644 --- a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs @@ -1,5 +1,6 @@ use std::{ collections::{hash_map::Entry, HashMap, VecDeque}, + sync::Arc, task::{Context, Poll}, time::Duration, }; @@ -9,7 +10,7 @@ use libp2p::{swarm::NotifyHandler, PeerId}; use slog::{crit, debug, Logger}; use smallvec::SmallVec; use tokio_util::time::DelayQueue; -use types::EthSpec; +use types::{EthSpec, ForkContext}; use super::{ config::OutboundRateLimiterConfig, @@ -50,9 +51,13 @@ pub enum Error { impl SelfRateLimiter { /// Creates a new [`SelfRateLimiter`] based on configration values. - pub fn new(config: OutboundRateLimiterConfig, log: Logger) -> Result { + pub fn new( + config: OutboundRateLimiterConfig, + fork_context: Arc, + log: Logger, + ) -> Result { debug!(log, "Using self rate limiting params"; "config" => ?config); - let limiter = RateLimiter::new_with_config(config.0)?; + let limiter = RateLimiter::new_with_config(config.0, fork_context)?; Ok(SelfRateLimiter { delayed_requests: Default::default(), @@ -215,7 +220,7 @@ mod tests { use crate::service::api_types::{AppRequestId, RequestId, SyncRequestId}; use libp2p::PeerId; use std::time::Duration; - use types::MainnetEthSpec; + use types::{EthSpec, ForkContext, Hash256, MainnetEthSpec, Slot}; /// Test that `next_peer_request_ready` correctly maintains the queue. #[tokio::test] @@ -225,8 +230,13 @@ mod tests { ping_quota: Quota::n_every(1, 2), ..Default::default() }); + let fork_context = std::sync::Arc::new(ForkContext::new::( + Slot::new(0), + Hash256::ZERO, + &MainnetEthSpec::default_spec(), + )); let mut limiter: SelfRateLimiter = - SelfRateLimiter::new(config, log).unwrap(); + SelfRateLimiter::new(config, fork_context, log).unwrap(); let peer_id = PeerId::random(); for i in 1..=5u32 { diff --git a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs index c4944078fef..b4f19f668d7 100644 --- a/beacon_node/network/src/network_beacon_processor/rpc_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/rpc_methods.rs @@ -890,14 +890,6 @@ impl NetworkBeaconProcessor { "start_slot" => req.start_slot, ); - // Should not send more than max request blocks - if req.max_blobs_requested::() > self.chain.spec.max_request_blob_sidecars { - return Err(( - RpcErrorResponse::InvalidRequest, - "Request exceeded `MAX_REQUEST_BLOBS_SIDECARS`", - )); - } - let request_start_slot = Slot::from(req.start_slot); let data_availability_boundary_slot = match self.chain.data_availability_boundary() { From 56602c1a120c9a8c9d0902cb281f0ef526299cea Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 9 Jan 2025 19:57:03 -0800 Subject: [PATCH 27/31] Fix max value --- beacon_node/lighthouse_network/src/rpc/codec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 9fa1a3adcca..efa2195e4d2 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -553,7 +553,7 @@ fn handle_length( fn handle_rpc_request( versioned_protocol: SupportedProtocol, decoded_buffer: &[u8], - current_fork: ForkName, + _current_fork: ForkName, spec: &ChainSpec, ) -> Result>, RPCError> { match versioned_protocol { @@ -588,7 +588,7 @@ fn handle_rpc_request( SupportedProtocol::BlobsByRangeV1 => { let req = BlobsByRangeRequest::from_ssz_bytes(decoded_buffer)?; // TODO(pawan): change this to max_blobs_per_rpc_request in the alpha10 PR - let max_blobs = spec.max_blobs_per_block_by_fork(current_fork); + let max_blobs = spec.max_request_blob_sidecars; if req.count > max_blobs { return Err(RPCError::ErrorResponse( RpcErrorResponse::InvalidRequest, From e6ba98b8cbc714c5b37b7121f49d2aec77787d2a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 Jan 2025 16:11:06 +1100 Subject: [PATCH 28/31] Fix doctest --- consensus/types/src/runtime_var_list.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/consensus/types/src/runtime_var_list.rs b/consensus/types/src/runtime_var_list.rs index 8025b54c008..857073b3b84 100644 --- a/consensus/types/src/runtime_var_list.rs +++ b/consensus/types/src/runtime_var_list.rs @@ -38,10 +38,6 @@ use std::slice::SliceIndex; /// // Push a value to if it _does_ exceed the maximum. /// assert!(long.push(6).is_err()); /// -/// -/// uninit.push(5).unwrap(); -/// assert_eq!(&uninit[..], &[5]); -/// /// ``` #[derive(Debug, Clone, Serialize, Deserialize, Derivative)] #[derivative(PartialEq, Eq, Hash(bound = "T: std::hash::Hash"))] From dbad9d8d516482642a3a7cb09c8c62ff9437d3c0 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 Jan 2025 16:11:18 +1100 Subject: [PATCH 29/31] Fix formatting --- beacon_node/lighthouse_network/src/rpc/rate_limiter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs b/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs index f9e66504aa8..b9e82a5f1ee 100644 --- a/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use std::task::{Context, Poll}; use std::time::{Duration, Instant}; use tokio::time::Interval; -use types::{ChainSpec, EthSpec, ForkName, ForkContext}; +use types::{ChainSpec, EthSpec, ForkContext, ForkName}; /// Nanoseconds since a given time. // Maintained as u64 to reduce footprint From 97d8dabe9d6017f4aeb9d4dfa0d25bf4bde0d7fb Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 Jan 2025 16:28:29 +1100 Subject: [PATCH 30/31] Fix max check --- beacon_node/lighthouse_network/src/rpc/codec.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index efa2195e4d2..d8645478a1e 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -553,7 +553,7 @@ fn handle_length( fn handle_rpc_request( versioned_protocol: SupportedProtocol, decoded_buffer: &[u8], - _current_fork: ForkName, + current_fork: ForkName, spec: &ChainSpec, ) -> Result>, RPCError> { match versioned_protocol { @@ -587,14 +587,16 @@ fn handle_rpc_request( ))), SupportedProtocol::BlobsByRangeV1 => { let req = BlobsByRangeRequest::from_ssz_bytes(decoded_buffer)?; + let max_requested_blobs = req + .count + .saturating_mul(spec.max_blobs_per_block_by_fork(current_fork)); // TODO(pawan): change this to max_blobs_per_rpc_request in the alpha10 PR - let max_blobs = spec.max_request_blob_sidecars; - if req.count > max_blobs { + if max_requested_blobs > spec.max_request_blob_sidecars { return Err(RPCError::ErrorResponse( RpcErrorResponse::InvalidRequest, format!( "requested exceeded limit. allowed: {}, requested: {}", - max_blobs, req.count + spec.max_request_blob_sidecars, max_requested_blobs ), )); } From a435b3bb41fcc9e45e07864027a50c271c532d95 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 Jan 2025 16:41:17 +1100 Subject: [PATCH 31/31] Delete hardcoded max_blobs_per_block in RPC limits --- beacon_node/lighthouse_network/src/rpc/protocol.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index a7c4524a2e1..cdb71a1d76c 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -77,6 +77,10 @@ pub static SIGNED_BEACON_BLOCK_ELECTRA_MAX_WITHOUT_PAYLOAD: LazyLock = La /// We calculate the value from its fields instead of constructing the block and checking the length. /// Note: This is only the theoretical upper bound. We further bound the max size we receive over the network /// with `max_chunk_size`. +/// +/// FIXME: Given that these limits are useless we should probably delete them. See: +/// +/// https://github.com/sigp/lighthouse/issues/6790 pub static SIGNED_BEACON_BLOCK_BELLATRIX_MAX: LazyLock = LazyLock::new(|| // Size of a full altair block *SIGNED_BEACON_BLOCK_ALTAIR_MAX @@ -93,7 +97,6 @@ pub static SIGNED_BEACON_BLOCK_DENEB_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_CAPELLA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_deneb_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional offsets for the `ExecutionPayload` - + (::ssz_fixed_len() * MainnetEthSpec::default_spec().max_blobs_per_block_by_fork(ForkName::Deneb) as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field. // @@ -101,7 +104,6 @@ pub static SIGNED_BEACON_BLOCK_ELECTRA_MAX: LazyLock = LazyLock::new(|| { *SIGNED_BEACON_BLOCK_ELECTRA_MAX_WITHOUT_PAYLOAD + types::ExecutionPayload::::max_execution_payload_electra_size() // adding max size of execution payload (~16gb) + ssz::BYTES_PER_LENGTH_OFFSET // Adding the additional ssz offset for the `ExecutionPayload` field - + (::ssz_fixed_len() * MainnetEthSpec::default_spec().max_blobs_per_block_by_fork(ForkName::Electra) as usize) + ssz::BYTES_PER_LENGTH_OFFSET }); // Length offset for the blob commitments field.