diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8be199d719..0c37e70021 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - AsBlock, AvailabilityPendingBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, + AsBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, IntoAvailableBlock, }; use crate::block_times_cache::BlockTimesCache; @@ -442,8 +442,7 @@ pub struct BeaconChain { /// Provides monitoring of a set of explicitly defined validators. pub validator_monitor: RwLock>, pub blob_cache: BlobCache, - pub blob_cache: BlobCache, - pub kzg: Option>, + pub kzg: Option>, } type BeaconBlockAndState = (BeaconBlock, BeaconState); @@ -1084,34 +1083,8 @@ impl BeaconChain { pub fn get_blobs( &self, block_root: &Hash256, - data_availability_boundary: Epoch, - ) -> Result>, Error> { - match self.store.get_blobs(block_root)? { - Some(blobs) => Ok(Some(blobs)), - None => { - // Check for the corresponding block to understand whether we *should* have blobs. - self.get_blinded_block(block_root)? - .map(|block| { - // If there are no KZG commitments in the block, we know the sidecar should - // be empty. - let expected_kzg_commitments = - match block.message().body().blob_kzg_commitments() { - Ok(kzg_commitments) => kzg_commitments, - Err(_) => return Err(Error::NoKzgCommitmentsFieldOnBlock), - }; - if expected_kzg_commitments.is_empty() { - Ok(BlobsSidecar::empty_from_parts(*block_root, block.slot())) - } else if data_availability_boundary <= block.epoch() { - // We should have blobs for all blocks younger than the boundary. - Err(Error::BlobsUnavailable) - } else { - // We shouldn't have blobs for blocks older than the boundary. - Err(Error::BlobsOlderThanDataAvailabilityBoundary(block.epoch())) - } - }) - .transpose() - } - } + ) -> Result>, Error> { + self.store.get_blobs(block_root) } pub fn get_blinded_block( diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 50c58391e9..04b5c1a4bb 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -12,8 +12,8 @@ use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; use types::blob_sidecar::BlobSidecar; use types::{ - BeaconBlockRef, BeaconStateError, BlobsSidecar, EthSpec, Hash256, KzgCommitment, - SignedBeaconBlock, SignedBeaconBlockAndBlobsSidecar, SignedBeaconBlockHeader, + BeaconBlockRef, BeaconStateError, EthSpec, Hash256, KzgCommitment, + SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, }; use types::{Epoch, ExecPayload}; @@ -289,23 +289,6 @@ pub enum DataAvailabilityCheckRequired { No, } -impl BlockWrapper { - fn into_availablilty_pending_block( - self, - block_root: Hash256, - chain: &BeaconChain, - ) -> Result, BlobError> { - match self { - BlockWrapper::Block(block) => { - AvailabilityPendingBlock::new(block, block_root, da_check_required) - } - BlockWrapper::BlockAndBlobs(block, blobs_sidecar) => { - AvailabilityPendingBlock::new_with_blobs(block, blobs_sidecar, da_check_required) - } - } - } -} - pub trait IntoAvailableBlock { fn into_available_block( self, @@ -314,39 +297,32 @@ pub trait IntoAvailableBlock { ) -> Result, BlobError>; } -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. An -/// `AvailableBlock` has passed any required data availability checks and should be used in -/// consensus. -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: BeaconChainTypes"))] -pub struct AvailabilityPendingBlock { - block: Arc>, - data_availability_handle: DataAvailabilityHandle, +impl IntoAvailableBlock for BlockWrapper { + fn into_available_block(self, block_root: Hash256, chain: &BeaconChain) -> Result, BlobError> { + todo!() + } } -/// Used to await the result of data availability check. -type DataAvailabilityHandle = JoinHandle>, BlobError>>; - #[derive(Clone, Debug, Derivative)] #[derivative(PartialEq, Hash(bound = "T: BeaconChainTypes"))] -pub struct AvailableBlock { - block: Arc>, - blobs: Blobs, +pub struct AvailableBlock { + block: Arc>, + blobs: Blobs, } -impl AvailableBlock { +impl AvailableBlock { pub fn blobs(&self) -> Option>> { match &self.blobs { Blobs::NotRequired | Blobs::None => None, - Blobs::Available(block_sidecar) => Some(block_sidecar.clone()), + Blobs::Available(blob_sidecar) => Some(blob_sidecar.clone()), } } pub fn deconstruct( self, ) -> ( - Arc>, - Option>>, + Arc>, + Option>>, ) { match self.blobs { Blobs::NotRequired | Blobs::None => (self.block, None), @@ -365,119 +341,6 @@ pub enum Blobs { None, } -//TODO(sean) add block root to the availability pending block? -impl AvailabilityPendingBlock { - pub fn new( - beacon_block: Arc>, - block_root: Hash256, - chain: &BeaconChain, - ) -> Result { - let data_availability_boundary = chain.data_availability_boundary(); - let da_check_required = - data_availability_boundary.map_or(DataAvailabilityCheckRequired::No, |boundary| { - if chain.epoch()? >= boundary { - DataAvailabilityCheckRequired::Yes - } else { - DataAvailabilityCheckRequired::No - } - }); - - match beacon_block.as_ref() { - // No data availability check required prior to Eip4844. - SignedBeaconBlock::Base(_) - | SignedBeaconBlock::Altair(_) - | SignedBeaconBlock::Capella(_) - | SignedBeaconBlock::Merge(_) => Ok(AvailabilityPendingBlock { - block: beacon_block, - data_availability_handle: async { Ok(Some(Blobs::NotRequired)) }, - }), - SignedBeaconBlock::Eip4844(_) => { - match da_check_required { - DataAvailabilityCheckRequired::Yes => { - // Attempt to reconstruct empty blobs here. - let blobs_sidecar = beacon_block - .reconstruct_empty_blobs(Some(block_root)) - .map(Arc::new)?; - Ok(AvailableBlock(AvailableBlockInner::BlockAndBlob( - SignedBeaconBlockAndBlobsSidecar { - beacon_block, - blobs_sidecar, - }, - ))) - } - DataAvailabilityCheckRequired::No => AvailabilityPendingBlock { - block: beacon_block, - data_availability_handle: async { Ok(Some(Blobs::NotRequired)) }, - }, - } - } - } - } - - /// This function is private because an `AvailableBlock` should be - /// constructed via the `into_available_block` method. - //TODO(sean) do we want this to optionally cricumvent the beacon cache? - fn new_with_blobs( - beacon_block: Arc>, - blobs_sidecar: Arc>, - chain: &BeaconChain, - ) -> Result { - let data_availability_boundary = chain.data_availability_boundary(); - let da_check_required = - data_availability_boundary.map_or(DataAvailabilityCheckRequired::No, |boundary| { - if chain.epoch()? >= boundary { - DataAvailabilityCheckRequired::Yes - } else { - DataAvailabilityCheckRequired::No - } - }); - match beacon_block.as_ref() { - // This method shouldn't be called with a pre-Eip4844 block. - SignedBeaconBlock::Base(_) - | SignedBeaconBlock::Altair(_) - | SignedBeaconBlock::Capella(_) - | SignedBeaconBlock::Merge(_) => Err(BlobError::InconsistentFork), - SignedBeaconBlock::Eip4844(_) => { - match da_check_required { - DataAvailabilityCheckRequired::Yes => Ok(AvailableBlock { - block: beacon_block, - blobs: Blobs::Available(blobs_sidecar), - }), - DataAvailabilityCheckRequired::No => { - // Blobs were not verified so we drop them, we'll instead just pass around - // an available `Eip4844` block without blobs. - Ok(AvailableBlock { - block: beacon_block, - blobs: Blobs::NotRequired, - }) - } - } - } - } - } -} - -pub trait IntoBlockWrapper: AsBlock { - fn into_block_wrapper(self) -> BlockWrapper; -} - -impl IntoBlockWrapper for BlockWrapper { - fn into_block_wrapper(self) -> BlockWrapper { - self - } -} - -impl IntoBlockWrapper for AvailabilityPendingBlock { - fn into_block_wrapper(self) -> BlockWrapper { - let (block, blobs) = self.deconstruct(); - if let Some(blobs) = blobs { - BlockWrapper::BlockAndBlobs(block, blobs) - } else { - BlockWrapper::Block(block) - } - } -} - pub trait AsBlock { fn slot(&self) -> Slot; fn epoch(&self) -> Epoch; @@ -542,7 +405,7 @@ impl AsBlock for BlockWrapper { fn canonical_root(&self) -> Hash256 { match &self { BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlob(block, _) => block.canonical_root(), + BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), } } } @@ -599,82 +462,42 @@ impl AsBlock for &BlockWrapper { fn canonical_root(&self) -> Hash256 { match &self { BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlob(block, _) => block.canonical_root(), + BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), } } } -impl AsBlock for AvailabilityPendingBlock { - fn slot(&self) -> Slot { - match &self.0 { - AvailableBlockInner::Block(block) => block.slot(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.slot() - } - } - } - fn epoch(&self) -> Epoch { - match &self.0 { - AvailableBlockInner::Block(block) => block.epoch(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.epoch() - } - } - } - fn parent_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.parent_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.parent_root() - } - } - } - fn state_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.state_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.state_root() - } - } - } - fn signed_block_header(&self) -> SignedBeaconBlockHeader { - match &self.0 { - AvailableBlockInner::Block(block) => block.signed_block_header(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.signed_block_header() - } - } - } - fn message(&self) -> BeaconBlockRef { - match &self.0 { - AvailableBlockInner::Block(block) => block.message(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.message() - } - } - } - fn as_block(&self) -> &SignedBeaconBlock { - match &self.0 { - AvailableBlockInner::Block(block) => block, - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - &block_sidecar_pair.beacon_block - } - } - } - fn block_cloned(&self) -> Arc> { - match &self.0 { - AvailableBlockInner::Block(block) => block.clone(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.clone() - } - } - } - fn canonical_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.canonical_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.canonical_root() - } +/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. This makes no +/// claims about data availability and should not be used in consensus. This struct is useful in +/// networking when we want to send blocks around without consensus checks. +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = "E: EthSpec"))] +pub enum BlockWrapper { + Block(Arc>), + BlockAndBlobs(Arc>, Arc>), +} + +impl BlockWrapper { + pub fn new( + block: Arc>, + blobs_sidecar: Option>>, + ) -> Self { + if let Some(blobs_sidecar) = blobs_sidecar { + BlockWrapper::BlockAndBlobs(block, blobs_sidecar) + } else { + BlockWrapper::Block(block) } } } + +impl From> for BlockWrapper { + fn from(block: SignedBeaconBlock) -> Self { + BlockWrapper::Block(Arc::new(block)) + } +} + +impl From>> for BlockWrapper { + fn from(block: Arc>) -> Self { + BlockWrapper::Block(block) + } +} diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 1e420f8a4b..00603727ad 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -49,7 +49,7 @@ #![allow(clippy::result_large_err)] use crate::blob_verification::{ - validate_blob_for_gossip, AsBlock, AvailabilityPendingBlock, AvailableBlock, BlobError, + AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, IntoBlockWrapper, }; use crate::eth1_finalization_cache::Eth1FinalizationData; @@ -627,7 +627,7 @@ pub fn signature_verify_chain_segment( #[derive(Derivative)] #[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct GossipVerifiedBlock { - pub block: AvailabilityPendingBlock, + pub block: Arc>, pub block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -636,7 +636,7 @@ pub struct GossipVerifiedBlock { /// A wrapper around a `SignedBeaconBlock` that indicates that all signatures (except the deposit /// signatures) have been verified. pub struct SignatureVerifiedBlock { - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -689,7 +689,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockError> { + ) -> Result, BlockError> { self.into_execution_pending_block_slashable(block_root, chain, notify_execution_layer) .map(|execution_pending| { // Supply valid block to slasher. @@ -707,7 +707,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>>; + ) -> Result, BlockSlashInfo>>; fn block(&self) -> &SignedBeaconBlock; } @@ -930,8 +930,6 @@ impl GossipVerifiedBlock { // Validate the block's execution_payload (if any). validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; - let available_block = validate_blob_for_gossip(block, block_root, chain)?; - // Having checked the proposer index and the block root we can cache them. let consensus_context = ConsensusContext::new(available_block.slot()) .set_current_block_root(block_root) @@ -969,7 +967,7 @@ impl IntoExecutionPendingBlock for GossipVerifiedBlock &SignedBeaconBlock { - self.block.as_block() + self.block.as_ref() } } @@ -979,7 +977,7 @@ impl SignatureVerifiedBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn new( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, chain: &BeaconChain, ) -> Result> { @@ -1029,7 +1027,7 @@ impl SignatureVerifiedBlock { /// As for `new` above but producing `BlockSlashInfo`. pub fn check_slashable( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, chain: &BeaconChain, ) -> Result>> { @@ -1121,7 +1119,7 @@ impl IntoExecutionPendingBlock for SignatureVerifiedBloc } fn block(&self) -> &SignedBeaconBlock { - self.block.as_block() + self.block.as_ref() } } @@ -1152,28 +1150,6 @@ impl IntoExecutionPendingBlock for Arc IntoExecutionPendingBlock for AvailabilityPendingBlock { - /// Verifies the `SignedBeaconBlock` by first transforming it into a `SignatureVerifiedBlock` - /// and then using that implementation of `IntoExecutionPendingBlock` to complete verification. - fn into_execution_pending_block_slashable( - self, - block_root: Hash256, - chain: &Arc>, - notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { - // Perform an early check to prevent wasting time on irrelevant blocks. - let block_root = check_block_relevancy(self.as_block(), block_root, chain) - .map_err(|e| BlockSlashInfo::SignatureNotChecked(self.signed_block_header(), e))?; - - SignatureVerifiedBlock::check_slashable(self, block_root, chain)? - .into_execution_pending_block_slashable(block_root, chain, notify_execution_layer) - } - - fn block(&self) -> &SignedBeaconBlock { - self.as_block() - } -} - impl ExecutionPendingBlock { /// Instantiates `Self`, a wrapper that indicates that the given `block` is fully valid. See /// the struct-level documentation for more information. @@ -1183,7 +1159,7 @@ impl ExecutionPendingBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn from_signature_verified_components( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, parent: PreProcessingSnapshot, mut consensus_context: ConsensusContext, diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 2df1a4f05b..a0785f98e8 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -1,4 +1,4 @@ -use crate::blob_verification::{AvailabilityPendingBlock, AvailableBlock}; +use crate::blob_verification::{AvailableBlock}; use crate::{ attester_cache::{CommitteeLengths, Error}, metrics, @@ -21,7 +21,7 @@ pub struct CacheItem { * Values used to make the block available. */ block: Arc>, - blobs: Option>>, + blobs: Option>>, proto_block: ProtoBlock, } @@ -160,7 +160,7 @@ impl EarlyAttesterCache { } /// Returns the blobs, if `block_root` matches the cached item. - pub fn get_blobs(&self, block_root: Hash256) -> Option>> { + pub fn get_blobs(&self, block_root: Hash256) -> Option>> { self.item .read() .as_ref() diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index fa615a9c2a..a932fc2022 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,7 +1,7 @@ -use crate::blob_verification::{verify_data_availability, AvailabilityPendingBlock}; +use crate::blob_verification::{verify_data_availability}; use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; use crate::kzg_utils::validate_blob; -use crate::{BeaconChainError, BlockError}; +use crate::{BeaconChainError, BeaconChainTypes, BlockError}; use eth2::reqwest::header::Entry; use kzg::{Kzg, KzgCommitment}; use parking_lot::{Mutex, RwLock}; @@ -11,27 +11,29 @@ use std::future::Future; use std::sync::Arc; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; use types::{EthSpec, Hash256}; +use kzg::Error as KzgError; pub enum BlobCacheError { DuplicateBlob(Hash256), + Kzg(KzgError), } /// This cache contains /// - blobs that have been gossip verified /// - commitments for blocks that have been gossip verified, but the commitments themselves /// have not been verified against blobs /// - blocks that have been fully verified and only require a data availability check -pub struct GossipBlobCache { - rpc_blob_cache: RwLock>>>, +pub struct GossipBlobCache { + rpc_blob_cache: RwLock>>>, gossip_blob_cache: Mutex>>, kzg: Kzg, } -struct GossipBlobCacheInner { - verified_blobs: Vec>>, +struct GossipBlobCacheInner { + verified_blobs: Vec>>, executed_block: Option>, } -impl GossipBlobCache { +impl GossipBlobCache { pub fn new(kzg: Kzg) -> Self { Self { rpc_blob_cache: RwLock::new(HashMap::new()), @@ -45,14 +47,14 @@ impl GossipBlobCache { /// cached, verify the block and import it. /// /// This should only accept gossip verified blobs, so we should not have to worry about dupes. - pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { + pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { // TODO(remove clones) - let verified = validate_blob( + let verified = validate_blob::( &self.kzg, blob.blob.clone(), blob.kzg_commitment.clone(), blob.kzg_proof, - )?; + ).map_err(|e|BlobCacheError::Kzg(e))?; if verified { let mut blob_cache = self.gossip_blob_cache.lock(); @@ -85,22 +87,29 @@ impl GossipBlobCache { Ok(()) } - pub fn put_block(&self, block: ExecutedBlock) -> () { + pub fn put_block(&self, executed_block: ExecutedBlock) -> Result<(), BlobCacheError> { let mut guard = self.gossip_blob_cache.lock(); guard - .entry(block.block_root) + .entry(executed_block.block_root) .and_modify(|cache| { - if cache.verified_blobs == block.block.message_eip4844().blob_kzg_commitments() { - // send to reprocessing queue ? - } else if let Some(dup) = cache.executed_block.insert(block) { - // return error + if let Ok(block) = executed_block.block.message_eip4844() { + let verified_commitments_vec: Vec<_> = cache.verified_blobs.iter().map(|blob|blob.kzg_commitment).collect(); + let verified_commitments = VariableList::from(verified_commitments_vec); + if verified_commitments == block.body.blob_kzg_commitments { + // send to reprocessing queue ? + } else { + let _ = cache.executed_block.insert(executed_block); + // log that we cached + } } else { - // log that we cached it + // log error } }) .or_insert(GossipBlobCacheInner { verified_blobs: vec![], - executed_block: Some(block), + executed_block: Some(executed_block), }); + + Ok(()) } } diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index ad6509cb0f..502e453aad 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -11,7 +11,7 @@ use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; /// Container of the data that identifies an individual blob. -#[derive(Encode, Decode, Clone, Debug, PartialEq)] +#[derive(Encode, Decode, Clone, Debug, PartialEq, Eq, Hash)] pub struct BlobIdentifier { pub block_root: Hash256, pub index: u64,