Skip to content

Commit

Permalink
Drop block data from BlockError and BlobError (#5735)
Browse files Browse the repository at this point in the history 
* Drop block data from BlockError and BlobError

* Debug release tests

* Fix release tests

* Revert unnecessary changes to lighthouse_metrics
  • Loading branch information
@dapplion @AgeManning
dapplion authored and AgeManning committed Sep 3, 2024
1 parent 2d86150 commit 86fa0b4
Show file tree
Hide file tree
Showing 15 changed files with 208 additions and 288 deletions.
64 changes: 32 additions & 32 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl TryInto<Hash256> for AvailabilityProcessingStatus {
}

/// The result of a chain segment processing.
pub enum ChainSegmentResult<E: EthSpec> {
pub enum ChainSegmentResult {
/// Processing this chain segment finished successfully.
Successful {
imported_blocks: Vec<(Hash256, Slot)>,
Expand All @@ -215,7 +215,7 @@ pub enum ChainSegmentResult<E: EthSpec> {
/// have been imported.
Failed {
imported_blocks: Vec<(Hash256, Slot)>,
error: BlockError<E>,
error: BlockError,
},
}

Expand Down Expand Up @@ -2159,7 +2159,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self: &Arc<Self>,
blob_sidecar: Arc<BlobSidecar<T::EthSpec>>,
subnet_id: u64,
) -> Result<GossipVerifiedBlob<T>, GossipBlobError<T::EthSpec>> {
) -> Result<GossipVerifiedBlob<T>, GossipBlobError> {
metrics::inc_counter(&metrics::BLOBS_SIDECAR_PROCESSING_REQUESTS);
let _timer = metrics::start_timer(&metrics::BLOBS_SIDECAR_GOSSIP_VERIFICATION_TIMES);
GossipVerifiedBlob::new(blob_sidecar, subnet_id, self).map(|v| {
Expand Down Expand Up @@ -2698,7 +2698,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub fn filter_chain_segment(
self: &Arc<Self>,
chain_segment: Vec<RpcBlock<T::EthSpec>>,
) -> Result<Vec<HashBlockTuple<T::EthSpec>>, ChainSegmentResult<T::EthSpec>> {
) -> Result<Vec<HashBlockTuple<T::EthSpec>>, ChainSegmentResult> {
// This function will never import any blocks.
let imported_blocks = vec![];
let mut filtered_chain_segment = Vec::with_capacity(chain_segment.len());
Expand Down Expand Up @@ -2805,7 +2805,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self: &Arc<Self>,
chain_segment: Vec<RpcBlock<T::EthSpec>>,
notify_execution_layer: NotifyExecutionLayer,
) -> ChainSegmentResult<T::EthSpec> {
) -> ChainSegmentResult {
let mut imported_blocks = vec![];

// Filter uninteresting blocks from the chain segment in a blocking task.
Expand Down Expand Up @@ -2938,7 +2938,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub async fn verify_block_for_gossip(
self: &Arc<Self>,
block: Arc<SignedBeaconBlock<T::EthSpec>>,
) -> Result<GossipVerifiedBlock<T>, BlockError<T::EthSpec>> {
) -> Result<GossipVerifiedBlock<T>, BlockError> {
let chain = self.clone();
self.task_executor
.clone()
Expand Down Expand Up @@ -2986,7 +2986,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub async fn process_gossip_blob(
self: &Arc<Self>,
blob: GossipVerifiedBlob<T>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
let block_root = blob.block_root();

// If this block has already been imported to forkchoice it must have been available, so
Expand Down Expand Up @@ -3026,7 +3026,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
AvailabilityProcessingStatus,
DataColumnsToPublish<T::EthSpec>,
),
BlockError<T::EthSpec>,
BlockError,
> {
let Ok((slot, block_root)) = data_columns
.iter()
Expand Down Expand Up @@ -3062,7 +3062,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
slot: Slot,
block_root: Hash256,
blobs: FixedBlobSidecarList<T::EthSpec>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
// If this block has already been imported to forkchoice it must have been available, so
// we don't need to process its blobs again.
if self
Expand Down Expand Up @@ -3099,7 +3099,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
AvailabilityProcessingStatus,
DataColumnsToPublish<T::EthSpec>,
),
BlockError<T::EthSpec>,
BlockError,
> {
let Ok((slot, block_root)) = custody_columns
.iter()
Expand Down Expand Up @@ -3133,8 +3133,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
fn remove_notified(
&self,
block_root: &Hash256,
r: Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
r: Result<AvailabilityProcessingStatus, BlockError>,
) -> Result<AvailabilityProcessingStatus, BlockError> {
let has_missing_components =
matches!(r, Ok(AvailabilityProcessingStatus::MissingComponents(_, _)));
if !has_missing_components {
Expand All @@ -3148,8 +3148,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
fn remove_notified_custody_columns<P>(
&self,
block_root: &Hash256,
r: Result<(AvailabilityProcessingStatus, P), BlockError<T::EthSpec>>,
) -> Result<(AvailabilityProcessingStatus, P), BlockError<T::EthSpec>> {
r: Result<(AvailabilityProcessingStatus, P), BlockError>,
) -> Result<(AvailabilityProcessingStatus, P), BlockError> {
let has_missing_components = matches!(
r,
Ok((AvailabilityProcessingStatus::MissingComponents(_, _), _))
Expand All @@ -3168,7 +3168,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
unverified_block: B,
block_source: BlockImportSource,
notify_execution_layer: NotifyExecutionLayer,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
self.reqresp_pre_import_cache
.write()
.insert(block_root, unverified_block.block_cloned());
Expand Down Expand Up @@ -3204,8 +3204,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
unverified_block: B,
notify_execution_layer: NotifyExecutionLayer,
block_source: BlockImportSource,
publish_fn: impl FnOnce() -> Result<(), BlockError<T::EthSpec>> + Send + 'static,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
publish_fn: impl FnOnce() -> Result<(), BlockError> + Send + 'static,
) -> Result<AvailabilityProcessingStatus, BlockError> {
// Start the Prometheus timer.
let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES);

Expand Down Expand Up @@ -3326,7 +3326,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub async fn into_executed_block(
self: Arc<Self>,
execution_pending_block: ExecutionPendingBlock<T>,
) -> Result<ExecutedBlock<T::EthSpec>, BlockError<T::EthSpec>> {
) -> Result<ExecutedBlock<T::EthSpec>, BlockError> {
let ExecutionPendingBlock {
block,
import_data,
Expand Down Expand Up @@ -3381,7 +3381,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
async fn check_block_availability_and_import(
self: &Arc<Self>,
block: AvailabilityPendingExecutedBlock<T::EthSpec>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
let slot = block.block.slot();
let availability = self
.data_availability_checker
Expand All @@ -3394,7 +3394,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
async fn check_gossip_blob_availability_and_import(
self: &Arc<Self>,
blob: GossipVerifiedBlob<T>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
let slot = blob.slot();
if let Some(slasher) = self.slasher.as_ref() {
slasher.accept_block_header(blob.signed_block_header());
Expand All @@ -3416,7 +3416,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
AvailabilityProcessingStatus,
DataColumnsToPublish<T::EthSpec>,
),
BlockError<T::EthSpec>,
BlockError,
> {
if let Some(slasher) = self.slasher.as_ref() {
for data_colum in &data_columns {
Expand All @@ -3440,7 +3440,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
slot: Slot,
block_root: Hash256,
blobs: FixedBlobSidecarList<T::EthSpec>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
// Need to scope this to ensure the lock is dropped before calling `process_availability`
// Even an explicit drop is not enough to convince the borrow checker.
{
Expand All @@ -3450,7 +3450,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
.filter_map(|b| b.as_ref().map(|b| b.signed_block_header.clone()))
.unique()
{
if verify_header_signature::<T, BlockError<T::EthSpec>>(self, &header).is_ok() {
if verify_header_signature::<T, BlockError>(self, &header).is_ok() {
slashable_cache
.observe_slashable(
header.message.slot,
Expand Down Expand Up @@ -3484,7 +3484,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
AvailabilityProcessingStatus,
DataColumnsToPublish<T::EthSpec>,
),
BlockError<T::EthSpec>,
BlockError,
> {
// Need to scope this to ensure the lock is dropped before calling `process_availability`
// Even an explicit drop is not enough to convince the borrow checker.
Expand All @@ -3493,7 +3493,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// Assumes all items in custody_columns are for the same block_root
if let Some(column) = custody_columns.first() {
let header = &column.signed_block_header;
if verify_header_signature::<T, BlockError<T::EthSpec>>(self, header).is_ok() {
if verify_header_signature::<T, BlockError>(self, header).is_ok() {
slashable_cache
.observe_slashable(
header.message.slot,
Expand Down Expand Up @@ -3530,7 +3530,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self: &Arc<Self>,
slot: Slot,
availability: Availability<T::EthSpec>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
match availability {
Availability::Available(block) => {
// Block is fully available, import into fork choice
Expand All @@ -3545,7 +3545,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
pub async fn import_available_block(
self: &Arc<Self>,
block: Box<AvailableExecutedBlock<T::EthSpec>>,
) -> Result<AvailabilityProcessingStatus, BlockError<T::EthSpec>> {
) -> Result<AvailabilityProcessingStatus, BlockError> {
let AvailableExecutedBlock {
block,
import_data,
Expand Down Expand Up @@ -3624,7 +3624,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
parent_block: SignedBlindedBeaconBlock<T::EthSpec>,
parent_eth1_finalization_data: Eth1FinalizationData,
mut consensus_context: ConsensusContext<T::EthSpec>,
) -> Result<Hash256, BlockError<T::EthSpec>> {
) -> Result<Hash256, BlockError> {
// ----------------------------- BLOCK NOT YET ATTESTABLE ----------------------------------
// Everything in this initial section is on the hot path between processing the block and
// being able to attest to it. DO NOT add any extra processing in this initial section
Expand Down Expand Up @@ -3934,7 +3934,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
block: BeaconBlockRef<T::EthSpec>,
block_root: Hash256,
state: &BeaconState<T::EthSpec>,
) -> Result<(), BlockError<T::EthSpec>> {
) -> Result<(), BlockError> {
// Only perform the weak subjectivity check if it was configured.
let Some(wss_checkpoint) = self.config.weak_subjectivity_checkpoint else {
return Ok(());
Expand Down Expand Up @@ -4269,7 +4269,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
&self,
block_root: Hash256,
state: &mut BeaconState<T::EthSpec>,
) -> Result<(), BlockError<T::EthSpec>> {
) -> Result<(), BlockError> {
for relative_epoch in [RelativeEpoch::Current, RelativeEpoch::Next] {
let shuffling_id = AttestationShufflingId::new(block_root, state, relative_epoch)?;

Expand Down Expand Up @@ -7042,8 +7042,8 @@ impl From<BeaconStateError> for Error {
}
}

impl<E: EthSpec> ChainSegmentResult<E> {
pub fn into_block_error(self) -> Result<(), BlockError<E>> {
impl ChainSegmentResult {
pub fn into_block_error(self) -> Result<(), BlockError> {
match self {
ChainSegmentResult::Failed { error, .. } => Err(error),
ChainSegmentResult::Successful { .. } => Ok(()),
Expand Down
33 changes: 13 additions & 20 deletions beacon_node/beacon_chain/src/blob_verification.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use types::{

/// An error occurred while validating a gossip blob.
#[derive(Debug)]
pub enum GossipBlobError<E: EthSpec> {
pub enum GossipBlobError {
/// The blob sidecar is from a slot that is later than the current slot (with respect to the
/// gossip clock disparity).
///
Expand Down Expand Up @@ -95,7 +95,7 @@ pub enum GossipBlobError<E: EthSpec> {
/// ## Peer scoring
///
/// We cannot process the blob without validating its parent, the peer isn't necessarily faulty.
BlobParentUnknown(Arc<BlobSidecar<E>>),
BlobParentUnknown { parent_root: Hash256 },

/// Invalid kzg commitment inclusion proof
/// ## Peer scoring
Expand Down Expand Up @@ -145,28 +145,19 @@ pub enum GossipBlobError<E: EthSpec> {
NotFinalizedDescendant { block_parent_root: Hash256 },
}

impl<E: EthSpec> std::fmt::Display for GossipBlobError<E> {
impl std::fmt::Display for GossipBlobError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
GossipBlobError::BlobParentUnknown(blob_sidecar) => {
write!(
f,
"BlobParentUnknown(parent_root:{})",
blob_sidecar.block_parent_root()
)
}
other => write!(f, "{:?}", other),
}
write!(f, "{:?}", self)
}
}

impl<E: EthSpec> From<BeaconChainError> for GossipBlobError<E> {
impl From<BeaconChainError> for GossipBlobError {
fn from(e: BeaconChainError) -> Self {
GossipBlobError::BeaconChainError(e)
}
}

impl<E: EthSpec> From<BeaconStateError> for GossipBlobError<E> {
impl From<BeaconStateError> for GossipBlobError {
fn from(e: BeaconStateError) -> Self {
GossipBlobError::BeaconChainError(BeaconChainError::BeaconStateError(e))
}
Expand All @@ -190,12 +181,12 @@ impl<T: BeaconChainTypes> GossipVerifiedBlob<T> {
blob: Arc<BlobSidecar<T::EthSpec>>,
subnet_id: u64,
chain: &BeaconChain<T>,
) -> Result<Self, GossipBlobError<T::EthSpec>> {
) -> Result<Self, GossipBlobError> {
let header = blob.signed_block_header.clone();
// We only process slashing info if the gossip verification failed
// since we do not process the blob any further in that case.
validate_blob_sidecar_for_gossip(blob, subnet_id, chain).map_err(|e| {
process_block_slash_info::<_, GossipBlobError<T::EthSpec>>(
process_block_slash_info::<_, GossipBlobError>(
chain,
BlockSlashInfo::from_early_error_blob(header, e),
)
Expand Down Expand Up @@ -384,7 +375,7 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
blob_sidecar: Arc<BlobSidecar<T::EthSpec>>,
subnet: u64,
chain: &BeaconChain<T>,
) -> Result<GossipVerifiedBlob<T>, GossipBlobError<T::EthSpec>> {
) -> Result<GossipVerifiedBlob<T>, GossipBlobError> {
let blob_slot = blob_sidecar.slot();
let blob_index = blob_sidecar.index;
let block_parent_root = blob_sidecar.block_parent_root();
Expand Down Expand Up @@ -466,7 +457,9 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
// We have already verified that the blob is past finalization, so we can
// just check fork choice for the block's parent.
let Some(parent_block) = fork_choice.get_block(&block_parent_root) else {
return Err(GossipBlobError::BlobParentUnknown(blob_sidecar));
return Err(GossipBlobError::BlobParentUnknown {
parent_root: block_parent_root,
});
};

// Do not process a blob that does not descend from the finalized root.
Expand Down Expand Up @@ -516,7 +509,7 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
))
})?;

let state = cheap_state_advance_to_obtain_committees::<_, GossipBlobError<T::EthSpec>>(
let state = cheap_state_advance_to_obtain_committees::<_, GossipBlobError>(
&mut parent_state,
Some(parent_state_root),
blob_slot,
Expand Down
Loading

0 comments on commit 86fa0b4

Please sign in to comment.