Skip to content

Remove KZG verification from local block production and blobs fetched from the EL #7713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 2 additions & 30 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -5748,8 +5748,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
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 =
Expand All @@ -5768,37 +5766,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
)));
}

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::<T::EthSpec>(
kzg,
blobs.iter().collect(),
&kzg_proofs,
expected_kzg_commitments,
)
.map_err(BlockProductionError::KzgError)?;
} else {
kzg_utils::validate_blobs::<T::EthSpec>(
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!(
Expand Down
6 changes: 6 additions & 0 deletions beacon_node/beacon_chain/src/data_column_verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,12 @@ impl<E: EthSpec> KzgVerifiedDataColumn<E> {
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<DataColumnSidecar<E>>) -> Self {
Self { data: data_column }
}

/// Create a `KzgVerifiedDataColumn` from `DataColumnSidecar` for testing ONLY.
pub(crate) fn __new_for_testing(data_column: Arc<DataColumnSidecar<E>>) -> Self {
Self { data: data_column }
Expand Down
Original file line number Diff line number Diff line change
@@ -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<T: BeaconChainTypes> {
Expand Down Expand Up @@ -77,14 +76,7 @@ impl<T: BeaconChainTypes> FetchBlobsBeaconAdapter<T> {
GossipVerifiedBlob::<T, DoNotObserve>::new(blob.clone(), blob.index, &self.chain)
}

pub(crate) fn verify_data_columns_kzg(
&self,
data_columns: Vec<Arc<DataColumnSidecar<T::EthSpec>>>,
) -> Result<Vec<KzgVerifiedDataColumn<T::EthSpec>>, 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<HashSet<ColumnIndex>> {
Expand Down
34 changes: 14 additions & 20 deletions beacon_node/beacon_chain/src/fetch_blobs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -358,17 +358,24 @@ async fn compute_custody_columns_to_import<T: BeaconChainTypes>(
// `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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't removed the gossip verification in fetch_blobs_v1 above which does kzg verification. Do we want to remove that as well?

)
})
.collect::<Vec<_>>()
})
.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![]);
}
Expand All @@ -378,26 +385,13 @@ async fn compute_custody_columns_to_import<T: BeaconChainTypes>(
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",
)
Expand Down
14 changes: 4 additions & 10 deletions beacon_node/beacon_chain/src/fetch_blobs/tests.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)),
Expand Down
9 changes: 0 additions & 9 deletions beacon_node/beacon_chain/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1824,15 +1824,6 @@ pub static KZG_VERIFICATION_DATA_COLUMN_BATCH_TIMES: LazyLock<Result<Histogram>>
)
});

pub static BLOCK_PRODUCTION_BLOBS_VERIFICATION_TIMES: LazyLock<Result<Histogram>> = 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
*/
Expand Down