From 75320ff8bc075f209d2e3bf0a3400f3c2d97a0ca Mon Sep 17 00:00:00 2001 From: realbigsean Date: Sun, 22 Jan 2023 05:54:25 +0100 Subject: [PATCH] cleanup --- beacon_node/beacon_chain/src/beacon_chain.rs | 9 ++++----- .../beacon_chain/src/blob_verification.rs | 20 +++++++++++-------- .../beacon_chain/src/block_verification.rs | 12 +++++++---- beacon_node/http_api/src/publish_blocks.rs | 16 +++++++++++---- .../state_processing/src/consensus_context.rs | 12 +++++++++++ .../src/per_block_processing.rs | 5 +---- .../per_block_processing/eip4844/eip4844.rs | 8 ++++++-- consensus/types/src/signed_beacon_block.rs | 2 ++ 8 files changed, 57 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 89d9ffcce9..c39a2b26ce 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4591,11 +4591,10 @@ impl BeaconChain { //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::(&kzg, &blobs) .map_err(|e| BlockProductionError::KzgError(e))?; diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 7576bcf781..dc909b89a5 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/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 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( block_root: Hash256, chain: &BeaconChain, ) -> Result<(), BlobError> { - // Validate commitments agains transactions in the block. if verify_kzg_commitments_against_transactions::(transactions, kzg_commitments) .is_err() { @@ -148,7 +151,7 @@ fn verify_data_availability( /// 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 { @@ -252,9 +255,10 @@ impl IntoAvailableBlock for BlockWrapper { } } -/// 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(AvailableBlockInner); @@ -262,7 +266,7 @@ pub struct AvailableBlock(AvailableBlockInner); /// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. #[derive(Clone, Debug, Derivative)] #[derivative(PartialEq, Hash(bound = "E: EthSpec"))] -pub enum AvailableBlockInner { +enum AvailableBlockInner { Block(Arc>), BlockAndBlob(SignedBeaconBlockAndBlobsSidecar), } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 1c278def0a..8b1d6b847b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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( //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 GossipVerifiedBlock { // 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 SignatureVerifiedBlock { 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)?; diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index ec779d8a35..a9a0bc9c6b 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -60,10 +60,18 @@ pub async fn publish_block( 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( diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index 47243a56f2..4c8966f92c 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -18,6 +18,8 @@ pub struct ConsensusContext { /// Cache of indexed attestations constructed during block processing. indexed_attestations: HashMap<(AttestationData, BitList), IndexedAttestation>, + /// Whether `verify_kzg_commitments_against_transactions` has successfully passed. + kzg_commitments_consistent: bool, } #[derive(Debug, PartialEq, Clone)] @@ -40,6 +42,7 @@ impl ConsensusContext { proposer_index: None, current_block_root: None, indexed_attestations: HashMap::new(), + kzg_commitments_consistent: false, } } @@ -155,4 +158,13 @@ impl ConsensusContext { 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 + } } diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 01e7614ace..c61662090e 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -180,10 +180,7 @@ pub fn per_block_processing>( )?; } - 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(()) } diff --git a/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs b/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs index b1336eb112..be4afc57d3 100644 --- a/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs +++ b/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>( block_body: BeaconBlockBodyRef, + ctxt: &mut ConsensusContext, ) -> 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::(transactions, kzg_commitments)? { return Err(BlockProcessingError::BlobVersionHashMismatch); } diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 5f853ce23b..0df386b945 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -37,7 +37,9 @@ impl From 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, }