diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index caa73401e2..635e8d3ba6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -7,6 +7,7 @@ use crate::attester_cache::{AttesterCache, AttesterCacheKey}; 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::{AvailableBlock, BlockWrapper, IntoAvailableBlock}; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ check_block_is_finalized_descendant, check_block_relevancy, get_block_root, @@ -107,7 +108,6 @@ use tree_hash::TreeHash; use types::beacon_state::CloneConfig; use types::consts::eip4844::MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS; use types::consts::merge::INTERVALS_PER_SLOT; -use types::signed_block_and_blobs::BlockWrapper; use types::*; pub type ForkChoiceError = fork_choice::Error; @@ -2381,6 +2381,9 @@ impl BeaconChain { let block_root = get_block_root(block.block()); + //FIXME(sean) + let available_block = block.into_available_block(block_root); + if let Some((child_parent_root, child_slot)) = children.get(i) { // If this block has a child in this chain segment, ensure that its parent root matches // the root of this block. @@ -2780,7 +2783,7 @@ impl BeaconChain { #[allow(clippy::too_many_arguments)] fn import_block( &self, - signed_block: BlockWrapper, + signed_block: AvailableBlock, block_root: Hash256, mut state: BeaconState, confirmed_state_roots: Vec, @@ -2940,7 +2943,7 @@ impl BeaconChain { // If the write fails, revert fork choice to the version from disk, else we can // end up with blocks in fork choice that are missing from disk. // See https://github.com/sigp/lighthouse/issues/2028 - let (signed_block, blobs) = signed_block.deconstruct(Some(block_root)); + let (signed_block, blobs) = signed_block.deconstruct(); let block = signed_block.message(); let mut ops: Vec<_> = confirmed_state_roots .into_iter() @@ -2949,7 +2952,7 @@ impl BeaconChain { ops.push(StoreOp::PutBlock(block_root, signed_block.clone())); ops.push(StoreOp::PutState(block.state_root(), &state)); - if let Some(blobs) = blobs? { + if let Some(blobs) = blobs { if blobs.blobs.len() > 0 { //FIXME(sean) using this for debugging for now info!(self.log, "Writing blobs to store"; "block_root" => ?block_root); @@ -4569,10 +4572,7 @@ impl BeaconChain { }; // Use a context without block root or proposer index so that both are checked. - let mut ctxt = ConsensusContext::new(block.slot()) - //FIXME(sean) This is a hack beacuse `valdiate blobs sidecar requires the block root` - // which we won't have until after the state root is calculated. - .set_blobs_sidecar_validated(true); + let mut ctxt = ConsensusContext::new(block.slot()); per_block_processing( &mut state, diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 0725bd865c..e8c1a86707 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -1,10 +1,14 @@ +use derivative::Derivative; +use slasher::test_utils::block; use slot_clock::SlotClock; +use std::sync::Arc; use crate::beacon_chain::{BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY}; -use crate::{kzg_utils, BeaconChainError}; +use crate::BlockError::BlobValidation; +use crate::{kzg_utils, BeaconChainError, BlockError}; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; use types::signed_beacon_block::BlobReconstructionError; -use types::{BeaconStateError, BlobsSidecar, Hash256, KzgCommitment, Slot, Transactions}; +use types::{BeaconStateError, BlobsSidecar, Epoch, EthSpec, Hash256, KzgCommitment, SignedBeaconBlock, SignedBeaconBlockAndBlobsSidecar, Slot, Transactions}; #[derive(Debug)] pub enum BlobError { @@ -29,29 +33,14 @@ pub enum BlobError { block_slot: Slot, }, - /// The blob sidecar contains an incorrectly formatted `BLSFieldElement` > `BLS_MODULUS`. - /// - /// - /// ## Peer scoring - /// - /// The peer has sent an invalid message. - BlobOutOfRange { - blob_index: usize, - }, - /// The blob sidecar contains a KZGCommitment that is not a valid G1 point on /// the bls curve. /// /// ## Peer scoring /// /// The peer has sent an invalid message. + //FIXME(sean3) InvalidKZGCommitment, - /// The proposal signature in invalid. - /// - /// ## Peer scoring - /// - /// The signature on the blob sidecar invalid and the peer is faulty. - ProposalSignatureInvalid, /// No kzg ccommitment associated with blob sidecar. KzgCommitmentMissing, @@ -68,17 +57,6 @@ pub enum BlobError { KzgError(kzg::Error), - /// A blob sidecar for this proposer and slot has already been observed. - /// - /// ## Peer scoring - /// - /// The `proposer` has already proposed a sidecar at this slot. The existing sidecar may or may not - /// be equal to the given sidecar. - RepeatSidecar { - proposer: u64, - slot: Slot, - }, - /// There was an error whilst processing the sync contribution. It is not known if it is valid or invalid. /// /// ## Peer scoring @@ -87,12 +65,13 @@ pub enum BlobError { /// sync committee message is valid. BeaconChainError(BeaconChainError), /// No blobs for the specified block where we would expect blobs. - MissingBlobs, + UnavailableBlobs, + InconsistentFork, } impl From for BlobError { fn from(_: BlobReconstructionError) -> Self { - BlobError::MissingBlobs + BlobError::UnavailableBlobs } } @@ -109,6 +88,36 @@ impl From for BlobError { } pub fn validate_blob_for_gossip( + block_wrapper: BlockWrapper, + block_root: Hash256, + chain: &BeaconChain, +) -> Result, BlobError> { + if let BlockWrapper::BlockAndBlob(block, blobs_sidecar) = block_wrapper { + let blob_slot = blobs_sidecar.beacon_block_slot; + // Do not gossip or process blobs from future or past slots. + let latest_permissible_slot = chain + .slot_clock + .now_with_future_tolerance(MAXIMUM_GOSSIP_CLOCK_DISPARITY) + .ok_or(BeaconChainError::UnableToReadSlot)?; + if blob_slot > latest_permissible_slot { + return Err(BlobError::FutureSlot { + message_slot: latest_permissible_slot, + latest_permissible_slot: blob_slot, + }); + } + + if blob_slot != block.slot() { + return Err(BlobError::SlotMismatch { + blob_slot, + block_slot, + }); + } + } + + block_wrapper.into_available_block(block_root, chain) +} + +fn verify_data_availability( blob_sidecar: &BlobsSidecar, kzg_commitments: &[KzgCommitment], transactions: &Transactions, @@ -116,26 +125,6 @@ pub fn validate_blob_for_gossip( block_root: Hash256, chain: &BeaconChain, ) -> Result<(), BlobError> { - let blob_slot = blob_sidecar.beacon_block_slot; - // Do not gossip or process blobs from future or past slots. - let latest_permissible_slot = chain - .slot_clock - .now_with_future_tolerance(MAXIMUM_GOSSIP_CLOCK_DISPARITY) - .ok_or(BeaconChainError::UnableToReadSlot)?; - if blob_slot > latest_permissible_slot { - return Err(BlobError::FutureSlot { - message_slot: latest_permissible_slot, - latest_permissible_slot: blob_slot, - }); - } - - if blob_slot != block_slot { - return Err(BlobError::SlotMismatch { - blob_slot, - block_slot, - }); - } - // Validate commitments agains transactions in the block. if verify_kzg_commitments_against_transactions::(transactions, kzg_commitments) .is_err() @@ -162,3 +151,244 @@ pub fn validate_blob_for_gossip( } Ok(()) } + +/// 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 adding consensus logic. +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] +pub enum BlockWrapper { + Block(Arc>), + BlockAndBlob(Arc>, Arc>), +} + +impl BlockWrapper { + pub fn new( + block: Arc>, + blobs_sidecar: Option>>, + ) -> Self { + if let Some(blobs_sidecar) = blobs_sidecar { + BlockWrapper::BlockAndBlob(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) + } +} + +pub trait IntoAvailableBlock { + fn into_available_block( + self, + block_root: Hash256, + chain: &BeaconChain, + ) -> Result, BlobError>; +} + +#[derive(Copy, Clone)] +pub enum DataAvailabilityCheckRequired { + Yes, + No +} + +impl IntoAvailableBlock for BlockWrapper { + fn into_available_block( + self, + block_root: Hash256, + chain: &BeaconChain, + ) -> Result, BlobError> { + let data_availability_boundary = chain.data_availability_boundary(); + let da_check_required = data_availability_boundary.map_or(DataAvailabilityCheckRequired::No, |boundary|{ + if self.epoch() >= boundary { + DataAvailabilityCheckRequired::Yes + } else { + DataAvailabilityCheckRequired::No + } + }); + match self { + BlockWrapper::Block(block) => AvailableBlock::new(block, block_root, da_check_required), + BlockWrapper::BlockAndBlob(block, blobs_sidecar) => { + if matches!(da_check_required, DataAvailabilityCheckRequired::Yes) { + let kzg_commitments = block + .message() + .body() + .blob_kzg_commitments() + .map_err(|_| BlobError::KzgCommitmentMissing)?; + let transactions = block + .message() + .body() + .execution_payload_eip4844() + .map(|payload| payload.transactions()) + .map_err(|_| BlobError::TransactionsMissing)? + .ok_or(BlobError::TransactionsMissing)?; + verify_data_availability( + &blobs_sidecar, + kzg_commitments, + transactions, + block.slot(), + block_root, + chain, + )?; + } + + AvailableBlock::new_with_blobs(block, blobs_sidecar, da_check_required) + } + } + } +} + +/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. This newtype +/// wraps the `BlockWrapperInner` to ensure blobs cannot be accessed via an enum match. This would +/// circumvent empty blob reconstruction when accessing blobs. +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] +pub struct AvailableBlock(AvailableBlockInner); + +/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] +pub enum AvailableBlockInner { + Block(Arc>), + BlockAndBlob(SignedBeaconBlockAndBlobsSidecar), +} + +impl AvailableBlock { + pub fn new( + beacon_block: Arc>, + block_root: Hash256, + da_check_required: DataAvailabilityCheckRequired, + ) -> Result { + match beacon_block.as_ref() { + // No data availability check required prior to Eip4844. + SignedBeaconBlock::Base(_) + | SignedBeaconBlock::Altair(_) + | SignedBeaconBlock::Capella(_) + | SignedBeaconBlock::Merge(_) => { + Ok(AvailableBlock(AvailableBlockInner::Block(beacon_block))) + } + 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)?; + return Ok(AvailableBlock(AvailableBlockInner::BlockAndBlob( + SignedBeaconBlockAndBlobsSidecar { + beacon_block, + blobs_sidecar, + }, + ))) + } + DataAvailabilityCheckRequired::No => { + Ok(AvailableBlock(AvailableBlockInner::Block(beacon_block))) + } + } + } + } + } + + /// This function is private because an `AvailableBlock` should be + /// constructed via the `into_available_block` method. + fn new_with_blobs( + beacon_block: Arc>, + blobs_sidecar: Arc>, + da_check_required: DataAvailabilityCheckRequired + ) -> Result { + 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(AvailableBlockInner::BlockAndBlob( + SignedBeaconBlockAndBlobsSidecar { + beacon_block, + 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(AvailableBlockInner::Block(beacon_block))) + } + } + + }, + } + } + + pub fn slot(&self) -> Slot { + match &self.0 { + AvailableBlockInner::Block(block) => block.slot(), + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + block_sidecar_pair.beacon_block.slot() + } + } + } + pub fn block(&self) -> &SignedBeaconBlock { + match &self.0 { + AvailableBlockInner::Block(block) => &block, + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + &block_sidecar_pair.beacon_block + } + } + } + pub fn block_cloned(&self) -> Arc> { + match &self.0 { + AvailableBlockInner::Block(block) => block.clone(), + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + block_sidecar_pair.beacon_block.clone() + } + } + } + + pub fn blobs(&self) -> Option>> { + match &self.0 { + AvailableBlockInner::Block(_) => None, + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + Some(block_sidecar_pair.blobs_sidecar.clone()) + } + } + } + + pub fn message(&self) -> crate::BeaconBlockRef { + match &self.0 { + AvailableBlockInner::Block(block) => block.message(), + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + block_sidecar_pair.beacon_block.message() + } + } + } + + pub fn parent_root(&self) -> Hash256 { + self.block().parent_root() + } + + pub fn deconstruct(self) -> (Arc>, Option>>) { + match self.0 { + AvailableBlockInner::Block(block) => (block, None), + AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { + let SignedBeaconBlockAndBlobsSidecar { + beacon_block, + blobs_sidecar, + } = block_sidecar_pair; + (beacon_block, Some(blobs_sidecar)) + } + } + } +} diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 2b759e4ad9..7837106001 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -31,6 +31,9 @@ //! |--------------- //! | //! ▼ +//! AvailableBLock +//! | +//! ▼ //! SignatureVerifiedBlock //! | //! ▼ @@ -42,7 +45,7 @@ //! END //! //! ``` -use crate::blob_verification::{validate_blob_for_gossip, BlobError}; +use crate::blob_verification::{validate_blob_for_gossip, AvailableBlock, BlobError, BlockWrapper}; use crate::eth1_finalization_cache::Eth1FinalizationData; use crate::execution_payload::{ is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block, @@ -65,7 +68,7 @@ use eth2::types::EventKind; use execution_layer::PayloadStatus; use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; use parking_lot::RwLockReadGuard; -use proto_array::Block as ProtoBlock; +use proto_array::{Block as ProtoBlock, Block}; use safe_arith::ArithError; use slog::{debug, error, warn, Logger}; use slot_clock::SlotClock; @@ -88,7 +91,6 @@ use store::{Error as DBError, HotStateSummary, KeyValueStore, StoreOp}; use task_executor::JoinHandle; use tree_hash::TreeHash; use types::signed_beacon_block::BlobReconstructionError; -use types::signed_block_and_blobs::BlockWrapper; use types::ExecPayload; use types::{ BeaconBlockRef, BeaconState, BeaconStateError, BlindedPayload, ChainSpec, CloneConfig, Epoch, @@ -619,7 +621,7 @@ pub fn signature_verify_chain_segment( #[derive(Derivative)] #[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct GossipVerifiedBlock { - pub block: BlockWrapper, + pub block: AvailableBlock, pub block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -628,7 +630,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: BlockWrapper, + block: AvailableBlock, block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -651,7 +653,7 @@ type PayloadVerificationHandle = /// due to finality or some other event. A `ExecutionPendingBlock` should be imported into the /// `BeaconChain` immediately after it is instantiated. pub struct ExecutionPendingBlock { - pub block: BlockWrapper, + pub block: AvailableBlock, pub block_root: Hash256, pub state: BeaconState, pub parent_block: SignedBeaconBlock>, @@ -912,36 +914,12 @@ impl GossipVerifiedBlock { // Validate the block's execution_payload (if any). validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; - if let Some(blobs_sidecar) = block.blobs(Some(block_root))? { - let kzg_commitments = block - .message() - .body() - .blob_kzg_commitments() - .map_err(|_| BlockError::BlobValidation(BlobError::KzgCommitmentMissing))?; - let transactions = block - .message() - .body() - .execution_payload_eip4844() - .map(|payload| payload.transactions()) - .map_err(|_| BlockError::BlobValidation(BlobError::TransactionsMissing))? - .ok_or(BlockError::BlobValidation(BlobError::TransactionsMissing))?; - validate_blob_for_gossip( - &blobs_sidecar, - kzg_commitments, - transactions, - block.slot(), - block_root, - chain, - ) - .map_err(BlobValidation)?; - } + let available_block = validate_blob_for_gossip(block)?; // Having checked the proposer index and the block root we can cache them. let consensus_context = ConsensusContext::new(block.slot()) .set_current_block_root(block_root) - .set_proposer_index(block.message().proposer_index()) - .set_blobs_sidecar_validated(true) // Validated in `validate_blob_for_gossip` - .set_blobs_verified_vs_txs(true); + .set_proposer_index(block.message().proposer_index()); Ok(Self { block, @@ -984,7 +962,7 @@ impl SignatureVerifiedBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn new( - block: BlockWrapper, + block: AvailableBlock, block_root: Hash256, chain: &BeaconChain, ) -> Result> { @@ -1032,7 +1010,7 @@ impl SignatureVerifiedBlock { /// As for `new` above but producing `BlockSlashInfo`. pub fn check_slashable( - block: BlockWrapper, + block: AvailableBlock, block_root: Hash256, chain: &BeaconChain, ) -> Result>> { @@ -1049,7 +1027,7 @@ impl SignatureVerifiedBlock { let (mut parent, block) = if let Some(parent) = from.parent { (parent, from.block) } else { - load_parent(from.block_root, from.block, chain)? + load_parent(from.block_root, from.block.block(), chain)? }; let state = cheap_state_advance_to_obtain_committees( @@ -1108,7 +1086,7 @@ impl IntoExecutionPendingBlock for SignatureVerifiedBloc let (parent, block) = if let Some(parent) = self.parent { (parent, self.block) } else { - load_parent(self.block_root, self.block, chain) + load_parent(self.block_root, self.block.block(), chain) .map_err(|e| BlockSlashInfo::SignatureValid(header.clone(), e))? }; @@ -1150,7 +1128,7 @@ impl IntoExecutionPendingBlock for Arc IntoExecutionPendingBlock for BlockWrapper { +impl IntoExecutionPendingBlock for AvailableBlock { /// 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( @@ -1182,7 +1160,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: BlockWrapper, + block: AvailableBlock, block_root: Hash256, parent: PreProcessingSnapshot, mut consensus_context: ConsensusContext, @@ -1559,58 +1537,6 @@ impl ExecutionPendingBlock { } drop(fork_choice); - /* - * Verify kzg proofs and kzg commitments against transactions if required - */ - //FIXME(sean) should this be prior to applying attestions to fork choice above? done in parallel? - if let Some(data_availability_boundary) = chain.data_availability_boundary() { - if block_slot.epoch(T::EthSpec::slots_per_epoch()) >= data_availability_boundary { - let sidecar = block - .blobs(Some(block_root))? - .ok_or(BlockError::BlobValidation(BlobError::MissingBlobs))?; - let kzg = chain.kzg.as_ref().ok_or(BlockError::BlobValidation( - BlobError::TrustedSetupNotInitialized, - ))?; - let transactions = block - .message() - .body() - .execution_payload_eip4844() - .map(|payload| payload.transactions()) - .map_err(|_| BlockError::BlobValidation(BlobError::TransactionsMissing))? - .ok_or(BlockError::BlobValidation(BlobError::TransactionsMissing))?; - let kzg_commitments = block - .message() - .body() - .blob_kzg_commitments() - .map_err(|_| BlockError::BlobValidation(BlobError::KzgCommitmentMissing))?; - if !consensus_context.blobs_sidecar_validated() { - if !kzg_utils::validate_blobs_sidecar( - &kzg, - block.slot(), - block_root, - kzg_commitments, - &sidecar, - ) - .map_err(|e| BlockError::BlobValidation(BlobError::KzgError(e)))? - { - return Err(BlockError::BlobValidation(BlobError::InvalidKzgProof)); - } - } - if !consensus_context.blobs_verified_vs_txs() - && verify_kzg_commitments_against_transactions::( - transactions, - kzg_commitments, - ) - //FIXME(sean) we should maybe just map this error so we have more info about the mismatch - .is_err() - { - return Err(BlockError::BlobValidation( - BlobError::TransactionCommitmentMismatch, - )); - } - } - } - Ok(Self { block, block_root, @@ -1816,7 +1742,7 @@ fn load_parent( block_root: Hash256, block: BlockWrapper, chain: &BeaconChain, -) -> Result<(PreProcessingSnapshot, BlockWrapper), BlockError> { +) -> Result<(PreProcessingSnapshot, &SignedBeaconBlock), BlockError> { let spec = &chain.spec; // Reject any block if its parent is not known to fork choice. diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 1216d5d4d8..69c3f519a4 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -5,7 +5,7 @@ use crate::{ use parking_lot::RwLock; use proto_array::Block as ProtoBlock; use std::sync::Arc; -use store::signed_block_and_blobs::BlockWrapper; +use store::signed_block_and_blobs::AvailableBlock; use types::*; pub struct CacheItem { @@ -51,7 +51,7 @@ impl EarlyAttesterCache { pub fn add_head_block( &self, beacon_block_root: Hash256, - block: BlockWrapper, + block: AvailableBlock, proto_block: ProtoBlock, state: &BeaconState, spec: &ChainSpec, @@ -69,7 +69,7 @@ impl EarlyAttesterCache { }, }; - let (block, blobs) = block.deconstruct(Some(beacon_block_root)); + let (block, blobs) = block.deconstruct(); let item = CacheItem { epoch, committee_lengths, @@ -77,7 +77,7 @@ impl EarlyAttesterCache { source, target, block, - blobs: blobs.map_err(|_| Error::MissingBlobs)?, + blobs, proto_block, }; diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index db9da1ede3..3a233b957d 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -9,7 +9,7 @@ use slot_clock::SlotClock; use std::sync::Arc; use tokio::sync::mpsc::UnboundedSender; use tree_hash::TreeHash; -use types::signed_block_and_blobs::BlockWrapper; +use types::signed_block_and_blobs::AvailableBlock; use types::{ AbstractExecPayload, BlindedPayload, EthSpec, ExecPayload, ExecutionBlockHash, FullPayload, Hash256, SignedBeaconBlock, SignedBeaconBlockAndBlobsSidecar, @@ -32,7 +32,7 @@ pub async fn publish_block( // Send the block, regardless of whether or not it is valid. The API // specification is very clear that this is the desired behaviour. - let wrapped_block: BlockWrapper = + let wrapped_block: AvailableBlock = if matches!(block.as_ref(), &SignedBeaconBlock::Eip4844(_)) { if let Some(sidecar) = chain.blob_cache.pop(&block_root) { let block_and_blobs = SignedBeaconBlockAndBlobsSidecar { diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 46ac5bd0fb..a7fd2c8337 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -1,6 +1,7 @@ use std::{collections::VecDeque, sync::Arc}; -use types::{signed_block_and_blobs::BlockWrapper, BlobsSidecar, EthSpec, SignedBeaconBlock}; +use types::signed_block_and_blobs::BlockWrapper; +use types::{signed_block_and_blobs::AvailableBlock, BlobsSidecar, EthSpec, SignedBeaconBlock}; #[derive(Debug, Default)] pub struct BlocksAndBlobsRequestInfo { @@ -46,21 +47,20 @@ impl BlocksAndBlobsRequestInfo { .map(|sidecar| sidecar.beacon_block_slot == beacon_block.slot()) .unwrap_or(false) { - let blobs_sidecar = - accumulated_sidecars.pop_front().ok_or("missing sidecar")?; - Ok(BlockWrapper::new_with_blobs(beacon_block, blobs_sidecar)) + let blobs_sidecar = accumulated_sidecars.pop_front(); + BlockWrapper::new(beacon_block, blobs_sidecar) } else { - Ok(beacon_block.into()) + BlockWrapper::new(beacon_block, None) } }) - .collect::, _>>(); + .collect::>(); // if accumulated sidecars is not empty, throw an error. if !accumulated_sidecars.is_empty() { return Err("Received more sidecars than blocks"); } - pairs + Ok(pairs) } pub fn is_finished(&self) -> bool { diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index 23dd989f98..47243a56f2 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -18,10 +18,6 @@ pub struct ConsensusContext { /// Cache of indexed attestations constructed during block processing. indexed_attestations: HashMap<(AttestationData, BitList), IndexedAttestation>, - /// Whether `validate_blobs_sidecar` has successfully passed. - blobs_sidecar_validated: bool, - /// Whether `verify_kzg_commitments_against_transactions` has successfully passed. - blobs_verified_vs_txs: bool, } #[derive(Debug, PartialEq, Clone)] @@ -44,8 +40,6 @@ impl ConsensusContext { proposer_index: None, current_block_root: None, indexed_attestations: HashMap::new(), - blobs_sidecar_validated: false, - blobs_verified_vs_txs: false, } } @@ -161,22 +155,4 @@ impl ConsensusContext { pub fn num_cached_indexed_attestations(&self) -> usize { self.indexed_attestations.len() } - - pub fn set_blobs_sidecar_validated(mut self, blobs_sidecar_validated: bool) -> Self { - self.blobs_sidecar_validated = blobs_sidecar_validated; - self - } - - pub fn set_blobs_verified_vs_txs(mut self, blobs_verified_vs_txs: bool) -> Self { - self.blobs_verified_vs_txs = blobs_verified_vs_txs; - self - } - - pub fn blobs_sidecar_validated(&self) -> bool { - self.blobs_sidecar_validated - } - - pub fn blobs_verified_vs_txs(&self) -> bool { - self.blobs_verified_vs_txs - } } diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index f57169c72d..5f853ce23b 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -35,8 +35,10 @@ impl From for Hash256 { } } +#[derive(Debug)] pub enum BlobReconstructionError { - BlobsMissing, + UnavailableBlobs, + InconsistentFork, } /// A `BeaconBlock` and a signature from its proposer. @@ -244,25 +246,24 @@ impl> SignedBeaconBlock /// Reconstructs an empty `BlobsSidecar`, using the given block root if provided, else calculates it. /// If this block has kzg commitments, an error will be returned. If this block is from prior to the - /// Eip4844 fork, `None` will be returned. + /// Eip4844 fork, this will error. pub fn reconstruct_empty_blobs( &self, block_root_opt: Option, - ) -> Result>, BlobReconstructionError> { - self.message() + ) -> Result, BlobReconstructionError> { + let kzg_commitments = self + .message() .body() .blob_kzg_commitments() - .map(|kzg_commitments| { - if kzg_commitments.len() > 0 { - Err(BlobReconstructionError::BlobsMissing) - } else { - Ok(Some(BlobsSidecar::empty_from_parts( - block_root_opt.unwrap_or(self.canonical_root()), - self.slot(), - ))) - } - }) - .unwrap_or(Ok(None)) + .map_err(|_| BlobReconstructionError::InconsistentFork)?; + if kzg_commitments.is_empty() { + Ok(BlobsSidecar::empty_from_parts( + block_root_opt.unwrap_or(self.canonical_root()), + self.slot(), + )) + } else { + Err(BlobReconstructionError::UnavailableBlobs) + } } } diff --git a/consensus/types/src/signed_block_and_blobs.rs b/consensus/types/src/signed_block_and_blobs.rs index c589fbcfeb..60f6aac71a 100644 --- a/consensus/types/src/signed_block_and_blobs.rs +++ b/consensus/types/src/signed_block_and_blobs.rs @@ -1,4 +1,3 @@ -use crate::signed_beacon_block::BlobReconstructionError; use crate::{BlobsSidecar, EthSpec, Hash256, SignedBeaconBlock, SignedBeaconBlockEip4844, Slot}; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; @@ -33,128 +32,3 @@ impl SignedBeaconBlockAndBlobsSidecar { }) } } - -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. This newtype -/// wraps the `BlockWrapperInner` to ensure blobs cannot be accessed via an enum match. This would -/// circumvent empty blob reconstruction when accessing blobs. -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] -pub struct BlockWrapper(BlockWrapperInner); - -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] -pub enum BlockWrapperInner { - Block(Arc>), - BlockAndBlob(SignedBeaconBlockAndBlobsSidecar), -} - -impl BlockWrapper { - pub fn new(block: Arc>) -> Self { - Self(BlockWrapperInner::Block(block)) - } - - pub fn new_with_blobs( - beacon_block: Arc>, - blobs_sidecar: Arc>, - ) -> Self { - Self(BlockWrapperInner::BlockAndBlob( - SignedBeaconBlockAndBlobsSidecar { - beacon_block, - blobs_sidecar, - }, - )) - } - - pub fn slot(&self) -> Slot { - match &self.0 { - BlockWrapperInner::Block(block) => block.slot(), - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.slot() - } - } - } - pub fn block(&self) -> &SignedBeaconBlock { - match &self.0 { - BlockWrapperInner::Block(block) => &block, - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => &block_sidecar_pair.beacon_block, - } - } - pub fn block_cloned(&self) -> Arc> { - match &self.0 { - BlockWrapperInner::Block(block) => block.clone(), - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.clone() - } - } - } - - pub fn blobs( - &self, - block_root: Option, - ) -> Result>>, BlobReconstructionError> { - match &self.0 { - BlockWrapperInner::Block(block) => block - .reconstruct_empty_blobs(block_root) - .map(|blob_opt| blob_opt.map(Arc::new)), - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => { - Ok(Some(block_sidecar_pair.blobs_sidecar.clone())) - } - } - } - - pub fn message(&self) -> crate::BeaconBlockRef { - match &self.0 { - BlockWrapperInner::Block(block) => block.message(), - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.message() - } - } - } - - pub fn parent_root(&self) -> Hash256 { - self.block().parent_root() - } - - pub fn deconstruct( - self, - block_root: Option, - ) -> ( - Arc>, - Result>>, BlobReconstructionError>, - ) { - match self.0 { - BlockWrapperInner::Block(block) => { - let blobs = block - .reconstruct_empty_blobs(block_root) - .map(|blob_opt| blob_opt.map(Arc::new)); - (block, blobs) - } - BlockWrapperInner::BlockAndBlob(block_sidecar_pair) => { - let SignedBeaconBlockAndBlobsSidecar { - beacon_block, - blobs_sidecar, - } = block_sidecar_pair; - (beacon_block, Ok(Some(blobs_sidecar))) - } - } - } -} - -impl From> for BlockWrapper { - fn from(block: SignedBeaconBlock) -> Self { - BlockWrapper(BlockWrapperInner::Block(Arc::new(block))) - } -} - -impl From>> for BlockWrapper { - fn from(block: Arc>) -> Self { - BlockWrapper(BlockWrapperInner::Block(block)) - } -} - -impl From> for BlockWrapper { - fn from(block: SignedBeaconBlockAndBlobsSidecar) -> Self { - BlockWrapper(BlockWrapperInner::BlockAndBlob(block)) - } -}