diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9900535b2c7..d30109728f5 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -70,7 +70,7 @@ use crate::validator_monitor::{ }; use crate::validator_pubkey_cache::ValidatorPubkeyCache; use crate::{ - kzg_utils, metrics, AvailabilityPendingExecutedBlock, BeaconChainError, BeaconForkChoiceStore, + metrics, AvailabilityPendingExecutedBlock, BeaconChainError, BeaconForkChoiceStore, BeaconSnapshot, CachedHead, }; use eth2::types::{ @@ -5748,8 +5748,6 @@ impl BeaconChain { let (mut block, _) = block.deconstruct(); *block.state_root_mut() = state_root; - let blobs_verification_timer = - metrics::start_timer(&metrics::BLOCK_PRODUCTION_BLOBS_VERIFICATION_TIMES); let blob_items = match maybe_blobs_and_proofs { Some((blobs, proofs)) => { let expected_kzg_commitments = @@ -5768,37 +5766,11 @@ impl BeaconChain { ))); } - let kzg_proofs = Vec::from(proofs); - - let kzg = self.kzg.as_ref(); - if self - .spec - .is_peer_das_enabled_for_epoch(slot.epoch(T::EthSpec::slots_per_epoch())) - { - kzg_utils::validate_blobs_and_cell_proofs::( - kzg, - blobs.iter().collect(), - &kzg_proofs, - expected_kzg_commitments, - ) - .map_err(BlockProductionError::KzgError)?; - } else { - kzg_utils::validate_blobs::( - kzg, - expected_kzg_commitments, - blobs.iter().collect(), - &kzg_proofs, - ) - .map_err(BlockProductionError::KzgError)?; - } - - Some((kzg_proofs.into(), blobs)) + Some((proofs, blobs)) } None => None, }; - drop(blobs_verification_timer); - metrics::inc_counter(&metrics::BLOCK_PRODUCTION_SUCCESSES); trace!( diff --git a/beacon_node/beacon_chain/src/data_column_verification.rs b/beacon_node/beacon_chain/src/data_column_verification.rs index 3009522bf60..e079b5ab78c 100644 --- a/beacon_node/beacon_chain/src/data_column_verification.rs +++ b/beacon_node/beacon_chain/src/data_column_verification.rs @@ -266,6 +266,12 @@ impl KzgVerifiedDataColumn { verify_kzg_for_data_column(data_column, kzg) } + /// Mark a data column as KZG verified. Caller must ONLY use this on columns constructed + /// from EL blobs. + pub fn from_execution_verified(data_column: Arc>) -> Self { + Self { data: data_column } + } + /// Create a `KzgVerifiedDataColumn` from `DataColumnSidecar` for testing ONLY. pub(crate) fn __new_for_testing(data_column: Arc>) -> Self { Self { data: data_column } diff --git a/beacon_node/beacon_chain/src/fetch_blobs/fetch_blobs_beacon_adapter.rs b/beacon_node/beacon_chain/src/fetch_blobs/fetch_blobs_beacon_adapter.rs index 4a7a5aeea21..fe8af5b70ea 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs/fetch_blobs_beacon_adapter.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs/fetch_blobs_beacon_adapter.rs @@ -1,17 +1,16 @@ use crate::blob_verification::{GossipBlobError, GossipVerifiedBlob}; -use crate::data_column_verification::KzgVerifiedDataColumn; use crate::fetch_blobs::{EngineGetBlobsOutput, FetchEngineBlobError}; use crate::observed_block_producers::ProposalKey; use crate::observed_data_sidecars::DoNotObserve; use crate::{AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes}; use execution_layer::json_structures::{BlobAndProofV1, BlobAndProofV2}; -use kzg::{Error as KzgError, Kzg}; +use kzg::Kzg; #[cfg(test)] use mockall::automock; use std::collections::HashSet; use std::sync::Arc; use task_executor::TaskExecutor; -use types::{BlobSidecar, ChainSpec, ColumnIndex, DataColumnSidecar, Hash256, Slot}; +use types::{BlobSidecar, ChainSpec, ColumnIndex, Hash256, Slot}; /// An adapter to the `BeaconChain` functionalities to remove `BeaconChain` from direct dependency to enable testing fetch blobs logic. pub(crate) struct FetchBlobsBeaconAdapter { @@ -77,14 +76,7 @@ impl FetchBlobsBeaconAdapter { GossipVerifiedBlob::::new(blob.clone(), blob.index, &self.chain) } - pub(crate) fn verify_data_columns_kzg( - &self, - data_columns: Vec>>, - ) -> Result>, KzgError> { - KzgVerifiedDataColumn::from_batch(data_columns, &self.chain.kzg) - } - - pub(crate) fn known_for_proposal( + pub(crate) fn data_column_known_for_proposal( &self, proposal_key: ProposalKey, ) -> Option> { diff --git a/beacon_node/beacon_chain/src/fetch_blobs/mod.rs b/beacon_node/beacon_chain/src/fetch_blobs/mod.rs index e02405ddbad..bf4409fbb9c 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs/mod.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs/mod.rs @@ -14,7 +14,7 @@ mod tests; use crate::blob_verification::{GossipBlobError, GossipVerifiedBlob}; use crate::block_verification_types::AsBlock; -use crate::data_column_verification::KzgVerifiedCustodyDataColumn; +use crate::data_column_verification::{KzgVerifiedCustodyDataColumn, KzgVerifiedDataColumn}; #[cfg_attr(test, double)] use crate::fetch_blobs::fetch_blobs_beacon_adapter::FetchBlobsBeaconAdapter; use crate::kzg_utils::blobs_to_data_column_sidecars; @@ -311,6 +311,9 @@ async fn fetch_and_process_blobs_v2( return Ok(None); } + // Up until this point we have not observed the data columns in the gossip cache, which allows + // them to arrive independently while this function is running. In publish_fn we will observe + // them and then publish any columns that had not already been observed. publish_fn(EngineGetBlobsOutput::CustodyColumns( custody_columns_to_import.clone(), )); @@ -358,17 +361,24 @@ async fn compute_custody_columns_to_import( // `DataAvailabilityChecker` requires a strict match on custody columns count to // consider a block available. let mut custody_columns = data_columns_result - .map(|mut data_columns| { - data_columns.retain(|col| custody_columns_indices.contains(&col.index)); + .map(|data_columns| { data_columns + .into_iter() + .filter(|col| custody_columns_indices.contains(&col.index)) + .map(|col| { + KzgVerifiedCustodyDataColumn::from_asserted_custody( + KzgVerifiedDataColumn::from_execution_verified(col), + ) + }) + .collect::>() }) .map_err(FetchEngineBlobError::DataColumnSidecarError)?; // Only consider columns that are not already observed on gossip. - if let Some(observed_columns) = chain_adapter_cloned.known_for_proposal( + if let Some(observed_columns) = chain_adapter_cloned.data_column_known_for_proposal( ProposalKey::new(block.message().proposer_index(), block.slot()), ) { - custody_columns.retain(|col| !observed_columns.contains(&col.index)); + custody_columns.retain(|col| !observed_columns.contains(&col.index())); if custody_columns.is_empty() { return Ok(vec![]); } @@ -378,26 +388,13 @@ async fn compute_custody_columns_to_import( if let Some(known_columns) = chain_adapter_cloned.cached_data_column_indexes(&block_root) { - custody_columns.retain(|col| !known_columns.contains(&col.index)); + custody_columns.retain(|col| !known_columns.contains(&col.index())); if custody_columns.is_empty() { return Ok(vec![]); } } - // KZG verify data columns before publishing. This prevents blobs with invalid - // KZG proofs from the EL making it into the data availability checker. We do not - // immediately add these blobs to the observed blobs/columns cache because we want - // to allow blobs/columns to arrive on gossip and be accepted (and propagated) while - // we are waiting to publish. Just before publishing we will observe the blobs/columns - // and only proceed with publishing if they are not yet seen. - let verified = chain_adapter_cloned - .verify_data_columns_kzg(custody_columns) - .map_err(FetchEngineBlobError::KzgError)?; - - Ok(verified - .into_iter() - .map(KzgVerifiedCustodyDataColumn::from_asserted_custody) - .collect()) + Ok(custody_columns) }, "compute_custody_columns_to_import", ) diff --git a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs index 3178020c758..9cb597c6df9 100644 --- a/beacon_node/beacon_chain/src/fetch_blobs/tests.rs +++ b/beacon_node/beacon_chain/src/fetch_blobs/tests.rs @@ -1,4 +1,3 @@ -use crate::data_column_verification::KzgVerifiedDataColumn; use crate::fetch_blobs::fetch_blobs_beacon_adapter::MockFetchBlobsBeaconAdapter; use crate::fetch_blobs::{ fetch_and_process_engine_blobs_inner, EngineGetBlobsOutput, FetchEngineBlobError, @@ -156,7 +155,7 @@ mod get_blobs_v2 { mock_fork_choice_contains_block(&mut mock_adapter, vec![]); // All data columns already seen on gossip mock_adapter - .expect_known_for_proposal() + .expect_data_column_known_for_proposal() .returning(|_| Some(hashset![0, 1, 2])); // No blobs should be processed mock_adapter.expect_process_engine_blobs().times(0); @@ -192,17 +191,12 @@ mod get_blobs_v2 { // All blobs returned, fork choice doesn't contain block mock_get_blobs_v2_response(&mut mock_adapter, Some(blobs_and_proofs)); mock_fork_choice_contains_block(&mut mock_adapter, vec![]); - mock_adapter.expect_known_for_proposal().returning(|_| None); mock_adapter - .expect_cached_data_column_indexes() + .expect_data_column_known_for_proposal() .returning(|_| None); mock_adapter - .expect_verify_data_columns_kzg() - .returning(|c| { - Ok(c.into_iter() - .map(KzgVerifiedDataColumn::__new_for_testing) - .collect()) - }); + .expect_cached_data_column_indexes() + .returning(|_| None); mock_process_engine_blobs_result( &mut mock_adapter, Ok(AvailabilityProcessingStatus::Imported(block_root)), diff --git a/beacon_node/beacon_chain/src/metrics.rs b/beacon_node/beacon_chain/src/metrics.rs index 5ca764821f2..7696d386fe7 100644 --- a/beacon_node/beacon_chain/src/metrics.rs +++ b/beacon_node/beacon_chain/src/metrics.rs @@ -1824,15 +1824,6 @@ pub static KZG_VERIFICATION_DATA_COLUMN_BATCH_TIMES: LazyLock> ) }); -pub static BLOCK_PRODUCTION_BLOBS_VERIFICATION_TIMES: LazyLock> = LazyLock::new( - || { - try_create_histogram( - "beacon_block_production_blobs_verification_seconds", - "Time taken to verify blobs against commitments and creating BlobSidecar objects in block production" - ) - }, -); - /* * Data Availability cache metrics */