Skip to content

Drop block data from BlockError and BlobError #5735

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.

Sign up for GitHub

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

Merged
mergify merged 4 commits into from
Sep 3, 2024
Merged

Drop block data from BlockError and BlobError #5735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45c97b5
Drop block data from BlockError and BlobError
dapplion May 7, 2024
cc1fb12
Debug release tests
dapplion Aug 28, 2024
0ad4c30
Fix release tests
dapplion Sep 2, 2024
bacd93d
Revert unnecessary changes to lighthouse_metrics
michaelsproul Sep 3, 2024
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
64 changes: 32 additions & 32 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Open in desktop
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 @@ -3135,8 +3135,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 @@ -3150,8 +3150,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 @@ -3170,7 +3170,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 @@ -3206,8 +3206,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 @@ -3328,7 +3328,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 @@ -3383,7 +3383,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 @@ -3396,7 +3396,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 @@ -3418,7 +3418,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 @@ -3442,7 +3442,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 @@ -3452,7 +3452,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 @@ -3486,7 +3486,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 @@ -3495,7 +3495,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 @@ -3532,7 +3532,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 @@ -3547,7 +3547,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 @@ -3626,7 +3626,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 @@ -3936,7 +3936,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 @@ -4271,7 +4271,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 @@ -7044,8 +7044,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
Open in desktop
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 {
Comment thread
dapplion marked this conversation as resolved.
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)

@dapplion dapplion Sep 2, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Kept this implementation of Display as some release tests assert the full error description. I'm not sure of which consumers expect the full error details in Display formatting. If we switch to strum::Display we may lose information in some error logs throughout the codebase. Thoughts?

}
}

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