From ecfa9e755521b8a8eb49fff45429d9fcce88af54 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 21 Mar 2023 18:11:51 +0530 Subject: [PATCH] Add a wrapper to allow construction of only valid `AvailableBlock`s --- beacon_node/beacon_chain/src/beacon_chain.rs | 16 +- .../beacon_chain/src/blob_verification.rs | 201 +++++++++++------- .../beacon_chain/src/block_verification.rs | 26 ++- .../beacon_chain/src/gossip_blob_cache.rs | 18 +- beacon_node/http_api/src/publish_blocks.rs | 11 +- 5 files changed, 177 insertions(+), 95 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 542c797d1f..535d483ca0 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2714,7 +2714,9 @@ impl BeaconChain { |chain| { chain .data_availability_checker - .check_block_availability(executed_block) + .check_block_availability(executed_block, |epoch| { + chain.block_needs_da_check(epoch) + }) }, count_unrealized, ) @@ -2846,7 +2848,7 @@ impl BeaconChain { } }; - let slot = available_block.block.slot(); + let slot = available_block.slot(); // import let chain = self.clone(); @@ -2925,7 +2927,7 @@ impl BeaconChain { // ----------------------------------------------------------------------------------------- let current_slot = self.slot()?; let current_epoch = current_slot.epoch(T::EthSpec::slots_per_epoch()); - let block = signed_block.block.message(); + let block = signed_block.message(); let post_exec_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_POST_EXEC_PROCESSING); // Check against weak subjectivity checkpoint. @@ -2969,7 +2971,7 @@ impl BeaconChain { )?; // TODO(pawan): fix this atrocity let signed_block = signed_block.into_available_block().unwrap(); - let block = signed_block.block.message(); + let block = signed_block.message(); // Register the new block with the fork choice service. { @@ -6169,6 +6171,12 @@ impl BeaconChain { }) } + /// Returns true if the given epoch lies within the da boundary and false otherwise. + pub fn block_needs_da_check(&self, block_epoch: Epoch) -> bool { + self.data_availability_boundary() + .map_or(false, |da_epoch| block_epoch >= da_epoch) + } + /// The epoch that is a data availability boundary, or the latest finalized epoch. /// `None` if the `Eip4844` fork is disabled. pub fn finalized_data_availability_boundary(&self) -> Option { diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 64f66365a8..ed43f13010 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -322,15 +322,78 @@ impl IntoAvailableBlock for BlockWrapper { } #[derive(Clone, Debug, PartialEq, Derivative)] -#[derivative(Hash(bound = "T: EthSpec"))] -pub struct AvailableBlock { - pub block: Arc>, - pub blobs: VerifiedBlobs, +#[derivative(Hash(bound = "E: EthSpec"))] +pub struct AvailableBlock(AvailableBlockInner); + +#[derive(Clone, Debug, PartialEq, Derivative)] +#[derivative(Hash(bound = "E: EthSpec"))] +struct AvailableBlockInner { + block: Arc>, + blobs: VerifiedBlobs, } -impl AvailableBlock { - pub fn blobs(&self) -> Option> { - match &self.blobs { +impl AvailableBlock { + pub fn new( + block: Arc>, + blobs: Vec>>, + da_check: impl FnOnce(Epoch) -> bool, + ) -> Result { + if let Ok(block_kzg_commitments) = block.message().body().blob_kzg_commitments() { + if blobs.is_empty() && block_kzg_commitments.is_empty() { + return Ok(Self(AvailableBlockInner { + block, + blobs: VerifiedBlobs::EmptyBlobs, + })); + } + + if blobs.is_empty() { + if da_check(block.epoch()) { + return Ok(Self(AvailableBlockInner { + block, + blobs: VerifiedBlobs::NotRequired, + })); + } else { + return Err("Block within DA boundary but no blobs provided".to_string()); + } + } + + if blobs.len() != block_kzg_commitments.len() { + return Err(format!( + "Block commitments and blobs length must be the same. + Block commitments len: {}, blobs length: {}", + block_kzg_commitments.len(), + blobs.len() + )); + } + + for (block_commitment, blob) in block_kzg_commitments.iter().zip(blobs.iter()) { + if *block_commitment != blob.kzg_commitment { + return Err(format!( + "Invalid input. Blob and block commitment mismatch at index {}", + blob.index + )); + } + } + Ok(Self(AvailableBlockInner { + block, + blobs: VerifiedBlobs::Available(blobs.into()), + })) + } + // This is a pre 4844 block + else { + Ok(Self(AvailableBlockInner { + block, + blobs: VerifiedBlobs::PreEip4844, + })) + } + } + + pub fn block(&self) -> &SignedBeaconBlock { + &self.0.block + } + + pub fn blobs(&self) -> Option> { + match &self.0.blobs { VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { None } @@ -338,12 +401,12 @@ impl AvailableBlock { } } - pub fn deconstruct(self) -> (Arc>, Option>) { - match self.blobs { + pub fn deconstruct(self) -> (Arc>, Option>) { + match self.0.blobs { VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { - (self.block, None) + (self.0.block, None) } - VerifiedBlobs::Available(blobs) => (self.block, Some(blobs)), + VerifiedBlobs::Available(blobs) => (self.0.block, Some(blobs)), } } } @@ -394,117 +457,113 @@ impl BlockWrapper { } } +impl AsBlock for AvailableBlock { + fn slot(&self) -> Slot { + self.0.block.slot() + } + + fn epoch(&self) -> Epoch { + self.0.block.epoch() + } + + fn parent_root(&self) -> Hash256 { + self.0.block.parent_root() + } + + fn state_root(&self) -> Hash256 { + self.0.block.state_root() + } + + fn signed_block_header(&self) -> SignedBeaconBlockHeader { + self.0.block.signed_block_header() + } + + fn message(&self) -> BeaconBlockRef { + self.0.block.message() + } + + fn as_block(&self) -> &SignedBeaconBlock { + &self.0.block + } + + fn block_cloned(&self) -> Arc> { + self.0.block.clone() + } + + fn canonical_root(&self) -> Hash256 { + self.0.block.canonical_root() + } +} + impl AsBlock for BlockWrapper { fn slot(&self) -> Slot { - match self { - BlockWrapper::Available(block) => block.block.slot(), - BlockWrapper::AvailabilityPending(block) => block.slot(), - } + self.as_block().slot() } fn epoch(&self) -> Epoch { - match self { - BlockWrapper::Available(block) => block.block.epoch(), - BlockWrapper::AvailabilityPending(block) => block.epoch(), - } + self.as_block().epoch() } fn parent_root(&self) -> Hash256 { - match self { - BlockWrapper::Available(block) => block.block.parent_root(), - BlockWrapper::AvailabilityPending(block) => block.parent_root(), - } + self.as_block().parent_root() } fn state_root(&self) -> Hash256 { - match self { - BlockWrapper::Available(block) => block.block.state_root(), - BlockWrapper::AvailabilityPending(block) => block.state_root(), - } + self.as_block().state_root() } fn signed_block_header(&self) -> SignedBeaconBlockHeader { - match &self { - BlockWrapper::Available(block) => block.block.signed_block_header(), - BlockWrapper::AvailabilityPending(block) => block.signed_block_header(), - } + self.as_block().signed_block_header() } fn message(&self) -> BeaconBlockRef { - match &self { - BlockWrapper::Available(block) => block.block.message(), - BlockWrapper::AvailabilityPending(block) => block.message(), - } + self.as_block().message() } fn as_block(&self) -> &SignedBeaconBlock { match &self { - BlockWrapper::Available(block) => &block.block, + BlockWrapper::Available(block) => &block.0.block, BlockWrapper::AvailabilityPending(block) => block, } } fn block_cloned(&self) -> Arc> { match &self { - BlockWrapper::Available(block) => block.block.clone(), + BlockWrapper::Available(block) => block.0.block.clone(), BlockWrapper::AvailabilityPending(block) => block.clone(), } } fn canonical_root(&self) -> Hash256 { - match &self { - BlockWrapper::Available(block) => block.block.canonical_root(), - BlockWrapper::AvailabilityPending(block) => block.canonical_root(), - } + self.as_block().canonical_root() } } impl AsBlock for &BlockWrapper { fn slot(&self) -> Slot { - match self { - BlockWrapper::Available(block) => block.block.slot(), - BlockWrapper::AvailabilityPending(block) => block.slot(), - } + self.as_block().slot() } fn epoch(&self) -> Epoch { - match self { - BlockWrapper::Available(block) => block.block.epoch(), - BlockWrapper::AvailabilityPending(block) => block.epoch(), - } + self.as_block().epoch() } fn parent_root(&self) -> Hash256 { - match self { - BlockWrapper::Available(block) => block.block.parent_root(), - BlockWrapper::AvailabilityPending(block) => block.parent_root(), - } + self.as_block().parent_root() } fn state_root(&self) -> Hash256 { - match self { - BlockWrapper::Available(block) => block.block.state_root(), - BlockWrapper::AvailabilityPending(block) => block.state_root(), - } + self.as_block().state_root() } fn signed_block_header(&self) -> SignedBeaconBlockHeader { - match &self { - BlockWrapper::Available(block) => block.block.signed_block_header(), - BlockWrapper::AvailabilityPending(block) => block.signed_block_header(), - } + self.as_block().signed_block_header() } fn message(&self) -> BeaconBlockRef { - match &self { - BlockWrapper::Available(block) => block.block.message(), - BlockWrapper::AvailabilityPending(block) => block.message(), - } + self.as_block().message() } fn as_block(&self) -> &SignedBeaconBlock { match &self { - BlockWrapper::Available(block) => &block.block, + BlockWrapper::Available(block) => &block.0.block, BlockWrapper::AvailabilityPending(block) => block, } } fn block_cloned(&self) -> Arc> { match &self { - BlockWrapper::Available(block) => block.block.clone(), + BlockWrapper::Available(block) => block.0.block.clone(), BlockWrapper::AvailabilityPending(block) => block.clone(), } } fn canonical_root(&self) -> Hash256 { - match &self { - BlockWrapper::Available(block) => block.block.canonical_root(), - BlockWrapper::AvailabilityPending(block) => block.canonical_root(), - } + self.as_block().canonical_root() } } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 91ee0dae65..9af753d8e9 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -93,9 +93,9 @@ use task_executor::JoinHandle; use tree_hash::TreeHash; use types::ExecPayload; use types::{ - BeaconBlockRef, BeaconState, BeaconStateError, BlindedPayload, ChainSpec, CloneConfig, Epoch, - EthSpec, ExecutionBlockHash, Hash256, InconsistentFork, PublicKey, PublicKeyBytes, - RelativeEpoch, SignedBeaconBlock, SignedBeaconBlockHeader, Slot, + BeaconBlockRef, BeaconState, BeaconStateError, BlindedPayload, BlobSidecar, ChainSpec, + CloneConfig, Epoch, EthSpec, ExecutionBlockHash, Hash256, InconsistentFork, PublicKey, + PublicKeyBytes, RelativeEpoch, SignedBeaconBlock, SignedBeaconBlockHeader, Slot, }; pub const POS_PANDA_BANNER: &str = r#" @@ -1171,16 +1171,30 @@ impl IntoExecutionPendingBlock for BlockWrapper block .into_execution_pending_block_slashable(block_root, chain, notify_execution_layer), - BlockWrapper::Available(AvailableBlock { block, blobs }) => { + BlockWrapper::Available(available_block) => { + let (block, blobs) = available_block.deconstruct(); let mut execution_pending_block = block.into_execution_pending_block_slashable( block_root, chain, notify_execution_layer, )?; let block = execution_pending_block.block.block_cloned(); + + let blobs: Vec>> = match blobs { + Some(blob_list) => blob_list.into(), + None => vec![], + }; let available_execution_pending_block = - BlockWrapper::Available(AvailableBlock { block, blobs }); - execution_pending_block.block = available_execution_pending_block; + AvailableBlock::new(block, blobs, |epoch| chain.block_needs_da_check(epoch)) + .map_err(|e| { + BlockSlashInfo::SignatureValid( + execution_pending_block.block.signed_block_header(), + BlockError::AvailabilityCheck( + AvailabilityCheckError::InvalidBlockOrBlob(e), + ), + ) + })?; + execution_pending_block.block = available_execution_pending_block.into(); Ok(execution_pending_block) } } diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index be3a42613e..673fd6c9d4 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,21 +1,22 @@ -use crate::blob_verification::{AsBlock, AvailableBlock, BlockWrapper, VerifiedBlobs}; +use crate::blob_verification::{AsBlock, AvailableBlock, BlockWrapper}; use crate::block_verification::ExecutedBlock; use crate::kzg_utils::validate_blob; use kzg::Error as KzgError; use kzg::Kzg; use parking_lot::{Mutex, RwLock}; -use ssz_types::{Error, VariableList}; +use ssz_types::Error; use std::collections::hash_map::Entry; use std::collections::HashMap; use std::sync::Arc; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; -use types::{EthSpec, Hash256}; +use types::{Epoch, EthSpec, Hash256}; #[derive(Debug)] pub enum AvailabilityCheckError { DuplicateBlob(Hash256), Kzg(KzgError), SszTypes(ssz_types::Error), + InvalidBlockOrBlob(String), } impl From for AvailabilityCheckError { @@ -131,6 +132,7 @@ impl DataAvailabilityChecker { pub fn check_block_availability( &self, executed_block: ExecutedBlock, + da_check_fn: impl FnOnce(Epoch) -> bool, ) -> Result, AvailabilityCheckError> { let block_clone = executed_block.block.clone(); @@ -166,12 +168,10 @@ impl DataAvailabilityChecker { payload_verification_outcome, } = executed_block; - let available_block = BlockWrapper::Available(AvailableBlock { - block, - blobs: VerifiedBlobs::Available(VariableList::new( - removed.verified_blobs, - )?), - }); + let available_block = + AvailableBlock::new(block, removed.verified_blobs, da_check_fn) + .map_err(AvailabilityCheckError::InvalidBlockOrBlob)? + .into(); let available_executed = ExecutedBlock { block: available_block, diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index 88bfe8d580..a302194241 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -1,4 +1,5 @@ use crate::metrics; +use beacon_chain::blob_verification::AsBlock; use beacon_chain::blob_verification::BlockWrapper; use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now}; use beacon_chain::{AvailabilityProcessingStatus, NotifyExecutionLayer}; @@ -110,14 +111,14 @@ pub async fn publish_block( "Valid block from HTTP API"; "block_delay" => ?delay, "root" => format!("{}", root), - "proposer_index" => available_block.block.message().proposer_index(), - "slot" => available_block.block.slot(), + "proposer_index" => available_block.message().proposer_index(), + "slot" => available_block.slot(), ); // Notify the validator monitor. chain.validator_monitor.read().register_api_block( seen_timestamp, - available_block.block.message(), + available_block.message(), root, &chain.slot_clock, ); @@ -133,7 +134,7 @@ pub async fn publish_block( late_block_logging( &chain, seen_timestamp, - available_block.block.message(), + available_block.message(), root, "local", &log, @@ -165,7 +166,7 @@ pub async fn publish_block( log, "Block from HTTP API already known"; "block" => ?block_root, - "slot" => available_block.block.slot(), + "slot" => available_block.slot(), ); Ok(()) }