diff --git a/beacon_node/beacon_chain/src/data_column_verification.rs b/beacon_node/beacon_chain/src/data_column_verification.rs index bc7778cc635..608e003a228 100644 --- a/beacon_node/beacon_chain/src/data_column_verification.rs +++ b/beacon_node/beacon_chain/src/data_column_verification.rs @@ -215,6 +215,40 @@ impl GossipVerifiedDataColumn ) } + /// Create a `GossipVerifiedDataColumn` from `DataColumnSidecar` for block production ONLY. + /// When publishing a block constructed locally, the EL will have already verified the cell proofs. + /// When publishing a block constructed externally, there will be no columns here. + pub fn new_for_block_publishing( + column_sidecar: Arc>, + chain: &BeaconChain, + ) -> Result { + verify_data_column_sidecar(&column_sidecar)?; + + // Check if the data column is already in the DA checker cache. This happens when data columns + // are made available through the `engine_getBlobs` method. If it exists in the cache, we know + // it has already passed the gossip checks, even though this particular instance hasn't been + // seen / published on the gossip network yet (passed the `verify_is_unknown_sidecar` check above). + // In this case, we should accept it for gossip propagation. + verify_is_unknown_sidecar(chain, &column_sidecar)?; + + if chain + .data_availability_checker + .is_data_column_cached(&column_sidecar.block_root(), &column_sidecar) + { + // Observe this data column so we don't process it again. + if O::observe() { + observe_gossip_data_column(&column_sidecar, chain)?; + } + return Err(GossipDataColumnError::PriorKnownUnpublished); + } + + Ok(Self { + block_root: column_sidecar.block_root(), + data_column: KzgVerifiedDataColumn::from_execution_verified(column_sidecar), + _phantom: Default::default(), + }) + } + /// Create a `GossipVerifiedDataColumn` from `DataColumnSidecar` for testing ONLY. pub fn __new_for_testing(column_sidecar: Arc>) -> Self { Self { @@ -447,12 +481,12 @@ pub fn validate_data_column_sidecar_for_gossip( Ok(()) } -// Verify that this is the first column sidecar received for the tuple: -// (block_header.slot, block_header.proposer_index, column_sidecar.index) -fn verify_is_first_sidecar( +/// Verify that `column_sidecar` is not yet known, i.e. this is the first time `column_sidecar` has been received for the tuple: +/// `(block_header.slot, block_header.proposer_index, column_sidecar.index)` +fn verify_is_unknown_sidecar( chain: &BeaconChain, - data_column: &DataColumnSidecar, + column_sidecar: &DataColumnSidecar, ) -> Result<(), GossipDataColumnError> { if chain .observed_column_sidecars .read() - .proposer_is_known(data_column) + .proposer_is_known(column_sidecar) .map_err(|e| GossipDataColumnError::BeaconChainError(Box::new(e.into())))? { return Err(GossipDataColumnError::PriorKnown { - proposer: data_column.block_proposer_index(), - slot: data_column.slot(), - index: data_column.index, + proposer: column_sidecar.block_proposer_index(), + slot: column_sidecar.slot(), + index: column_sidecar.index, }); } Ok(()) diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index f797e3f3001..b6411167d92 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -3,7 +3,7 @@ use std::future::Future; use beacon_chain::blob_verification::{GossipBlobError, GossipVerifiedBlob}; use beacon_chain::block_verification_types::{AsBlock, RpcBlock}; -use beacon_chain::data_column_verification::{GossipDataColumnError, GossipVerifiedDataColumn}; +use beacon_chain::data_column_verification::GossipVerifiedDataColumn; use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now}; use beacon_chain::{ AvailabilityProcessingStatus, BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, @@ -216,7 +216,7 @@ pub async fn publish_block>( } } - if gossip_verified_columns.iter().map(Option::is_some).count() > 0 { + if !gossip_verified_columns.is_empty() { if let Some(data_column_publishing_delay) = data_column_publishing_delay_for_testing { // Subtract block publishing delay if it is also used. // Note: if `data_column_publishing_delay` is less than `block_publishing_delay`, it @@ -240,7 +240,6 @@ pub async fn publish_block>( let sampling_columns_indices = chain.sampling_columns_for_epoch(epoch); let sampling_columns = gossip_verified_columns .into_iter() - .flatten() .filter(|data_column| sampling_columns_indices.contains(&data_column.index())) .collect::>(); @@ -348,7 +347,7 @@ pub async fn publish_block>( type BuildDataSidecarTaskResult = Result< ( Vec>>, - Vec>>, + Vec>, ), Rejection, >; @@ -382,7 +381,7 @@ fn spawn_build_data_sidecar_task( } else { // Post PeerDAS: construct data columns. let gossip_verified_data_columns = - build_gossip_verified_data_columns(&chain, &block, blobs, kzg_proofs)?; + build_data_columns(&chain, &block, blobs, kzg_proofs)?; Ok((vec![], gossip_verified_data_columns)) } }, @@ -397,12 +396,16 @@ fn spawn_build_data_sidecar_task( }) } -fn build_gossip_verified_data_columns( +/// Build data columns as wrapped `GossipVerifiedDataColumn`s. +/// There is no need to actually perform gossip verification on columns that a block producer +/// is publishing. In the locally constructed case, cell proof verification happens in the EL. +/// In the externally constructed case, there wont be any columns here. +fn build_data_columns( chain: &BeaconChain, block: &SignedBeaconBlock>, blobs: BlobsList, kzg_cell_proofs: KzgProofs, -) -> Result>>, Rejection> { +) -> Result>, Rejection> { let slot = block.slot(); let data_column_sidecars = build_blob_data_column_sidecars(chain, block, blobs, kzg_cell_proofs).map_err(|e| { @@ -414,49 +417,12 @@ fn build_gossip_verified_data_columns( warp_utils::reject::custom_bad_request(format!("{e:?}")) })?; - let slot = block.slot(); let gossip_verified_data_columns = data_column_sidecars .into_iter() - .map(|data_column_sidecar| { - let column_index = data_column_sidecar.index; - let subnet = DataColumnSubnetId::from_column_index(column_index, &chain.spec); - let gossip_verified_column = - GossipVerifiedDataColumn::new(data_column_sidecar, subnet, chain); - - match gossip_verified_column { - Ok(blob) => Ok(Some(blob)), - Err(GossipDataColumnError::PriorKnown { proposer, .. }) => { - // Log the error but do not abort publication, we may need to publish the block - // or some of the other data columns if the block & data columns are only - // partially published by the other publisher. - debug!( - column_index, - %slot, - proposer, - "Data column for publication already known" - ); - Ok(None) - } - Err(GossipDataColumnError::PriorKnownUnpublished) => { - debug!( - column_index, - %slot, - "Data column for publication already known via the EL" - ); - Ok(None) - } - Err(e) => { - error!( - column_index, - %slot, - error = ?e, - "Data column for publication is gossip-invalid" - ); - Err(warp_utils::reject::custom_bad_request(format!("{e:?}"))) - } - } + .filter_map(|data_column_sidecar| { + GossipVerifiedDataColumn::new_for_block_publishing(data_column_sidecar, chain).ok() }) - .collect::, Rejection>>()?; + .collect::>(); Ok(gossip_verified_data_columns) } @@ -533,13 +499,12 @@ fn publish_blob_sidecars( fn publish_column_sidecars( sender_clone: &UnboundedSender>, - data_column_sidecars: &[Option>], + data_column_sidecars: &[GossipVerifiedDataColumn], chain: &BeaconChain, ) -> Result<(), BlockError> { let malicious_withhold_count = chain.config.malicious_withhold_count; let mut data_column_sidecars = data_column_sidecars .iter() - .flatten() .map(|d| d.clone_data_column()) .collect::>(); if malicious_withhold_count > 0 { diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index d9ddbf98920..7f02c2c0fdc 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -85,13 +85,18 @@ pub async fn gossip_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the - // block. let pre_finalized_block_root = Hash256::zero(); - assert_server_message_error( - error_response, - format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}"), - ); + let expected_error_msg = if tester.harness.spec.is_fulu_scheduled() { + format!( + "BAD_REQUEST: NotFinalizedDescendant {{ block_parent_root: {pre_finalized_block_root:?} }}" + ) + } else { + // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the + // block. + format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}") + }; + + assert_server_message_error(error_response, expected_error_msg); } /// This test checks that a block that is valid from a gossip perspective is accepted when using `broadcast_validation=gossip`. @@ -276,13 +281,19 @@ pub async fn consensus_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the - // block. + let pre_finalized_block_root = Hash256::zero(); - assert_server_message_error( - error_response, - format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}"), - ); + let expected_error_msg = if tester.harness.spec.is_fulu_scheduled() { + format!( + "BAD_REQUEST: NotFinalizedDescendant {{ block_parent_root: {pre_finalized_block_root:?} }}" + ) + } else { + // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the + // block. + format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}") + }; + + assert_server_message_error(error_response, expected_error_msg); } /// This test checks that a block that is only valid from a gossip perspective is rejected when using `broadcast_validation=consensus`. @@ -507,13 +518,19 @@ pub async fn equivocation_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the - // block. + let pre_finalized_block_root = Hash256::zero(); - assert_server_message_error( - error_response, - format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}"), - ); + let expected_error_msg = if tester.harness.spec.is_fulu_scheduled() { + format!( + "BAD_REQUEST: NotFinalizedDescendant {{ block_parent_root: {pre_finalized_block_root:?} }}" + ) + } else { + // Since Deneb, the invalidity of the blobs will be detected prior to the invalidity of the + // block. + format!("BAD_REQUEST: ParentUnknown {{ parent_root: {pre_finalized_block_root:?} }}") + }; + + assert_server_message_error(error_response, expected_error_msg); } /// This test checks that a block that is valid from both a gossip and consensus perspective is rejected when using `broadcast_validation=consensus_and_equivocation`.