cleanup

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -4591,11 +4591,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         //FIXME(sean)
         // - add a new timer for processing here
         if let Some(blobs) = blobs_opt {
-            let kzg = if let Some(kzg) = &self.kzg {
-                kzg
-            } else {
-                return Err(BlockProductionError::TrustedSetupNotInitialized);
-            };
+            let kzg = self
+                .kzg
+                .as_ref()
+                .ok_or(BlockProductionError::TrustedSetupNotInitialized)?;
             let kzg_aggregated_proof =
                 kzg_utils::compute_aggregate_kzg_proof::<T::EthSpec>(&kzg, &blobs)
                     .map_err(|e| BlockProductionError::KzgError(e))?;

beacon_node/beacon_chain/src/blob_verification.rs

@@ -60,12 +60,16 @@ pub enum BlobError {
     BeaconChainError(BeaconChainError),
     /// No blobs for the specified block where we would expect blobs.
     UnavailableBlobs,
+    /// Blobs provided for a pre-Eip4844 fork.
     InconsistentFork,
 }
 
 impl From<BlobReconstructionError> for BlobError {
-    fn from(_: BlobReconstructionError) -> Self {
-        BlobError::UnavailableBlobs
+    fn from(e: BlobReconstructionError) -> Self {
+        match e {
+            BlobReconstructionError::UnavailableBlobs => BlobError::UnavailableBlobs,
+            BlobReconstructionError::InconsistentFork => BlobError::InconsistentFork,
+        }
     }
 }
 
@@ -119,7 +123,6 @@ fn verify_data_availability<T: BeaconChainTypes>(
     block_root: Hash256,
     chain: &BeaconChain<T>,
 ) -> Result<(), BlobError> {
-    // Validate commitments agains transactions in the block.
     if verify_kzg_commitments_against_transactions::<T::EthSpec>(transactions, kzg_commitments)
         .is_err()
     {
@@ -148,7 +151,7 @@ fn verify_data_availability<T: BeaconChainTypes>(
 
 /// 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.
+/// networking when we want to send blocks around without consensus checks.
 #[derive(Clone, Debug, Derivative)]
 #[derivative(PartialEq, Hash(bound = "E: EthSpec"))]
 pub enum BlockWrapper<E: EthSpec> {
@@ -252,9 +255,10 @@ impl<T: BeaconChainTypes> IntoAvailableBlock<T> for BlockWrapper<T::EthSpec> {
     }
 }
 
-/// 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.
+/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`].  An
+/// `AvailableBlock` has passed any required data availability checks and should be used in
+/// consensus. This newtype wraps `AvailableBlockInner` to ensure data availability checks
+/// cannot be circumvented on construction.
 #[derive(Clone, Debug, Derivative)]
 #[derivative(PartialEq, Hash(bound = "E: EthSpec"))]
 pub struct AvailableBlock<E: EthSpec>(AvailableBlockInner<E>);
@@ -262,7 +266,7 @@ pub struct AvailableBlock<E: EthSpec>(AvailableBlockInner<E>);
 /// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`].
 #[derive(Clone, Debug, Derivative)]
 #[derivative(PartialEq, Hash(bound = "E: EthSpec"))]
-pub enum AvailableBlockInner<E: EthSpec> {
+enum AvailableBlockInner<E: EthSpec> {
     Block(Arc<SignedBeaconBlock<E>>),
     BlockAndBlob(SignedBeaconBlockAndBlobsSidecar<E>),
 }

beacon_node/beacon_chain/src/block_verification.rs

@@ -69,7 +69,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;
 use safe_arith::ArithError;
 use slog::{debug, error, warn, Logger};
 use slot_clock::SlotClock;
@@ -601,6 +601,7 @@ pub fn signature_verify_chain_segment<T: BeaconChainTypes>(
 
         //FIXME(sean) batch kzg verification
         let available_block = block.clone().into_available_block(*block_root, chain)?;
+        consensus_context = consensus_context.set_kzg_commitments_consistent(true);
 
         // Save the block and its consensus context. The context will have had its proposer index
         // and attesting indices filled in, which can be used to accelerate later block processing.
@@ -927,7 +928,8 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
         // 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)
-            .set_proposer_index(available_block.as_block().message().proposer_index());
+            .set_proposer_index(available_block.as_block().message().proposer_index())
+            .set_kzg_commitments_consistent(true);
 
         Ok(Self {
             block: available_block,
@@ -999,8 +1001,10 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
 
         let mut signature_verifier = get_signature_verifier(&state, &pubkey_cache, &chain.spec);
 
-        let mut consensus_context =
-            ConsensusContext::new(block.slot()).set_current_block_root(block_root);
+        let mut consensus_context = ConsensusContext::new(block.slot())
+            .set_current_block_root(block_root)
+            // An `AvailabileBlock is passed in here, so we know this check has been run.`
+            .set_kzg_commitments_consistent(true);
 
         signature_verifier.include_all_signatures(block.as_block(), &mut consensus_context)?;
 

beacon_node/http_api/src/publish_blocks.rs

@@ -60,10 +60,18 @@ pub async fn publish_block<T: BeaconChainTypes>(
     let delay = get_block_delay_ms(seen_timestamp, block.message(), &chain.slot_clock);
     metrics::observe_duration(&metrics::HTTP_API_BLOCK_BROADCAST_DELAY_TIMES, delay);
 
-    //FIXME(sean) handle errors
-    let available_block = wrapped_block
-        .into_available_block(block_root, &chain)
-        .unwrap();
+    let available_block = match wrapped_block.into_available_block(block_root, &chain) {
+        Ok(available_block) => available_block,
+        Err(e) => {
+            let msg = format!("{:?}", e);
+            error!(
+                log,
+                "Invalid block provided to HTTP API";
+                "reason" => &msg
+            );
+            return Err(warp_utils::reject::broadcast_without_import(msg));
+        }
+    };
 
     match chain
         .process_block(

consensus/state_processing/src/consensus_context.rs

@@ -18,6 +18,8 @@ pub struct ConsensusContext<T: EthSpec> {
     /// Cache of indexed attestations constructed during block processing.
     indexed_attestations:
         HashMap<(AttestationData, BitList<T::MaxValidatorsPerCommittee>), IndexedAttestation<T>>,
+    /// Whether `verify_kzg_commitments_against_transactions` has successfully passed.
+    kzg_commitments_consistent: bool,
 }
 
 #[derive(Debug, PartialEq, Clone)]
@@ -40,6 +42,7 @@ impl<T: EthSpec> ConsensusContext<T> {
             proposer_index: None,
             current_block_root: None,
             indexed_attestations: HashMap::new(),
+            kzg_commitments_consistent: false,
         }
     }
 
@@ -155,4 +158,13 @@ impl<T: EthSpec> ConsensusContext<T> {
     pub fn num_cached_indexed_attestations(&self) -> usize {
         self.indexed_attestations.len()
     }
+
+    pub fn set_kzg_commitments_consistent(mut self, kzg_commitments_consistent: bool) -> Self {
+        self.kzg_commitments_consistent = kzg_commitments_consistent;
+        self
+    }
+
+    pub fn kzg_commitments_consistent(&self) -> bool {
+        self.kzg_commitments_consistent
+    }
 }

consensus/state_processing/src/per_block_processing.rs

@@ -180,10 +180,7 @@ pub fn per_block_processing<T: EthSpec, Payload: AbstractExecPayload<T>>(
         )?;
     }
 
-    process_blob_kzg_commitments(block.body())?;
-
-    //FIXME(sean) add `validate_blobs_sidecar` (is_data_available) and only run it if the consensus
-    // context tells us it wasnt already run
+    process_blob_kzg_commitments(block.body(), ctxt)?;
 
     Ok(())
 }

consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs

@@ -1,4 +1,4 @@
-use crate::BlockProcessingError;
+use crate::{BlockProcessingError, ConsensusContext};
 use eth2_hashing::hash_fixed;
 use itertools::{EitherOrBoth, Itertools};
 use safe_arith::SafeArith;
@@ -11,13 +11,17 @@ use types::{
 
 pub fn process_blob_kzg_commitments<T: EthSpec, Payload: AbstractExecPayload<T>>(
     block_body: BeaconBlockBodyRef<T, Payload>,
+    ctxt: &mut ConsensusContext<T>,
 ) -> Result<(), BlockProcessingError> {
+    // Return early if this check has already been run.
+    if ctxt.kzg_commitments_consistent() {
+        return Ok(());
+    }
     if let (Ok(payload), Ok(kzg_commitments)) = (
         block_body.execution_payload(),
         block_body.blob_kzg_commitments(),
     ) {
         if let Some(transactions) = payload.transactions() {
-            //FIXME(sean) only run if this wasn't run in gossip (use consensus context)
             if !verify_kzg_commitments_against_transactions::<T>(transactions, kzg_commitments)? {
                 return Err(BlockProcessingError::BlobVersionHashMismatch);
             }

consensus/types/src/signed_beacon_block.rs

@@ -37,7 +37,9 @@ impl From<SignedBeaconBlockHash> for Hash256 {
 
 #[derive(Debug)]
 pub enum BlobReconstructionError {
+    /// No blobs for the specified block where we would expect blobs.
     UnavailableBlobs,
+    /// Blobs provided for a pre-Eip4844 fork.
     InconsistentFork,
 }