diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 801ef7696f..7b825be2c3 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -56,6 +56,7 @@ use crate::observed_block_producers::ObservedBlockProducers; use crate::observed_data_sidecars::ObservedDataSidecars; use crate::observed_operations::{ObservationOutcome, ObservedOperations}; use crate::observed_slashable::ObservedSlashable; +use crate::payload_envelope_verification::{ExecutedEnvelope, ExecutionPendingEnvelope}; use crate::persisted_beacon_chain::PersistedBeaconChain; use crate::persisted_custody::persist_custody_context; use crate::persisted_fork_choice::PersistedForkChoice; @@ -3547,6 +3548,33 @@ impl BeaconChain { )) } + /// Accepts a fully-verified payload envelope and awaits on its payload verification handle to + /// get a fully `ExecutedEnvelope`. + /// + /// An error is returned if the verification handle couldn't be awaited. + #[instrument(skip_all, level = "debug")] + pub async fn into_executed_payload_envelope( + self: Arc, + pending_envelope: ExecutionPendingEnvelope, + ) -> Result, BlockError> { + let ExecutionPendingEnvelope { + signed_envelope, + import_data, + payload_verification_handle, + } = pending_envelope; + + let payload_verification_outcome = payload_verification_handle + .await + .map_err(BeaconChainError::TokioJoin)? + .ok_or(BeaconChainError::RuntimeShutdown)??; + + Ok(ExecutedEnvelope::new( + signed_envelope, + import_data, + payload_verification_outcome, + )) + } + /* Import methods */ /// Checks if the block is available, and imports immediately if so, otherwise caches the block diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 631a0180dd..0be058231a 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -7,7 +7,6 @@ //! So, this module contains functions that one might expect to find in other crates, but they live //! here for good reason. -use crate::payload_envelope_verification::EnvelopeError; use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, BlockProductionError, ExecutionPayloadError, diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/gossip_verified_envelope.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/gossip_verified_envelope.rs index 580c6e9234..a7ccc056ac 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/gossip_verified_envelope.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/gossip_verified_envelope.rs @@ -4,7 +4,10 @@ use educe::Educe; use slot_clock::SlotClock; use state_processing::{VerifySignatures, envelope_processing::process_execution_payload_envelope}; use tracing::debug; -use types::{EthSpec, SignedBeaconBlock, SignedExecutionPayloadEnvelope, consts::gloas::BUILDER_INDEX_SELF_BUILD}; +use types::{ + EthSpec, SignedBeaconBlock, SignedExecutionPayloadEnvelope, + consts::gloas::BUILDER_INDEX_SELF_BUILD, +}; use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, NotifyExecutionLayer, @@ -70,7 +73,7 @@ impl GossipVerifiedEnvelope { .signed_execution_payload_bid()? .message; - // check that the envelopes slot isnt from a slot prior + // check that the envelopes slot isnt from a slot prior // to the latest finalized slot. if envelope.slot < latest_finalized_slot { return Err(EnvelopeError::PriorToFinalization { @@ -103,54 +106,58 @@ impl GossipVerifiedEnvelope { }); } - // Get the fork from the proposer cache so we can verify the signature. - // This is currently the most efficient way to implement envelope signature verification - // because the `fork` might depend on advancing the parent state. + // Verify the envelope signature. + // + // For self-build envelopes, we can use the proposer cache for the fork and the + // validator pubkey cache for the proposer's pubkey, avoiding a state load from disk. + // For external builder envelopes, we must load the state to access the builder registry. + let builder_index = envelope.builder_index; let block_slot = envelope.slot; let block_epoch = block_slot.epoch(T::EthSpec::slots_per_epoch()); let proposer_shuffling_decision_block = proto_block.proposer_shuffling_root_for_child_block(block_epoch, &chain.spec); - let mut opt_snapshot = None; - let envelope_ref = signed_envelope.as_ref(); - let proposer = chain.with_proposer_cache::<_, EnvelopeError>( - proposer_shuffling_decision_block, - block_epoch, - |proposers| proposers.get_slot::(block_slot), - || { - debug!( - %beacon_block_root, - block_hash = %envelope_ref.block_hash(), - "Proposer shuffling cache miss for envelope verification" - ); - // The proposer index was *not* cached and we must load the parent in order to - // determine the proposer index. - let snapshot = load_snapshot(envelope_ref, chain)?; - opt_snapshot = Some(Box::new(snapshot.clone())); - Ok((snapshot.state_root, snapshot.pre_state)) - }, - )?; - let fork = proposer.fork; - let builder_index = envelope.builder_index; - let index = if builder_index == BUILDER_INDEX_SELF_BUILD { - block.message().proposer_index() - } else { - builder_index - }; + let (signature_is_valid, opt_snapshot) = if builder_index == BUILDER_INDEX_SELF_BUILD { + // Fast path: self-build envelopes can be verified without loading the state. + let envelope_ref = signed_envelope.as_ref(); + let mut opt_snapshot = None; + let proposer = chain.with_proposer_cache::<_, EnvelopeError>( + proposer_shuffling_decision_block, + block_epoch, + |proposers| proposers.get_slot::(block_slot), + || { + debug!( + %beacon_block_root, + "Proposer shuffling cache miss for envelope verification" + ); + let snapshot = load_snapshot(envelope_ref, chain)?; + opt_snapshot = Some(Box::new(snapshot.clone())); + Ok((snapshot.state_root, snapshot.pre_state)) + }, + )?; + let fork = proposer.fork; - let signature_is_valid = { - // TODO(gloas) the builder pubkey wont be in the validator pubkey cache - // this will currently only work for local block building. let pubkey_cache = chain.validator_pubkey_cache.read(); let pubkey = pubkey_cache - .get(index as usize) - .ok_or_else(|| EnvelopeError::UnknownValidator { builder_index: index })?; - signed_envelope.verify_signature( + .get(block.message().proposer_index() as usize) + .ok_or_else(|| EnvelopeError::UnknownValidator { + builder_index: block.message().proposer_index(), + })?; + let is_valid = signed_envelope.verify_signature( pubkey, &fork, chain.genesis_validators_root, &chain.spec, - ) + ); + (is_valid, opt_snapshot) + } else { + // TODO(gloas) we should probably introduce a builder cache or some type of + // global cache. + // External builder: must load the state to get the builder pubkey. + let snapshot = load_snapshot(signed_envelope.as_ref(), chain)?; + let is_valid = + signed_envelope.verify_signature_with_state(&snapshot.pre_state, &chain.spec)?; + (is_valid, Some(Box::new(snapshot))) }; if !signature_is_valid { @@ -179,20 +186,13 @@ impl IntoExecutionPendingEnvelope for GossipVerifiedEnve let envelope = &signed_envelope.message; let payload = &envelope.payload; - // TODO(gloas) unwrap - let bid = chain - .get_full_block(&envelope.beacon_block_root) - .unwrap() - .unwrap() - .message() - .body() - .signed_execution_payload_bid() - .unwrap() - .message; - // Verify the execution payload is valid - let payload_notifier = - PayloadNotifier::new(chain.clone(), envelope, notify_execution_layer)?; + let payload_notifier = PayloadNotifier::new( + chain.clone(), + signed_envelope.clone(), + self.block.clone(), + notify_execution_layer, + )?; let block_root = envelope.beacon_block_root; let slot = self.block.slot(); diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs index b166e90fa1..7ddc4b5c64 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/mod.rs @@ -17,9 +17,6 @@ //! |--------------- //! | //! ▼ -//! SignatureVerifiedEnvelope -//! | -//! ▼ //! ExecutionPendingEnvelope //! | //! await @@ -41,15 +38,16 @@ use types::{ }; use crate::{ - BeaconChain, BeaconChainError, BeaconChainTypes, NotifyExecutionLayer, - block_verification::PayloadVerificationHandle, + AvailabilityProcessingStatus, BeaconChain, BeaconChainError, BeaconChainTypes, + ExecutionPayloadError, NotifyExecutionLayer, PayloadVerificationOutcome, + block_verification::PayloadVerificationHandle, block_verification_types::BlockImportData, payload_envelope_verification::gossip_verified_envelope::GossipVerifiedEnvelope, }; pub mod execution_pending_envelope; pub mod gossip_verified_envelope; mod payload_notifier; -mod signature_verified_envelope; +mod tests; pub trait IntoExecutionPendingEnvelope: Sized { fn into_execution_pending_envelope( @@ -101,7 +99,67 @@ pub struct EnvelopeProcessingSnapshot { pub beacon_block_root: Hash256, } -#[derive(Debug, Clone)] +/// A payload envelope that has gone through processing checks and execution by an EL client. +/// This envelope hasn't necessarily completed data availability checks. +/// +/// +/// It contains 2 variants: +/// 1. `Available`: This enelope has been executed and also contains all data to consider it +/// fully available. +/// 2. `AvailabilityPending`: This envelope hasn't received all required blobs to consider it +/// fully available. +pub enum ExecutedEnvelope { + Available(AvailableExecutedEnvelope), + // TODO(gloas) implement availability pending + AvailabilityPending(), +} + +impl ExecutedEnvelope { + pub fn new( + envelope: MaybeAvailableEnvelope, + import_data: EnvelopeImportData, + payload_verification_outcome: PayloadVerificationOutcome, + ) -> Self { + match envelope { + MaybeAvailableEnvelope::Available(available_envelope) => { + Self::Available(AvailableExecutedEnvelope::new( + available_envelope, + import_data, + payload_verification_outcome, + )) + } + // TODO(gloas) implement availability pending + MaybeAvailableEnvelope::AvailabilityPending { + block_hash: _, + envelope: _, + } => Self::AvailabilityPending(), + } + } +} + +/// A payload envelope that has completed all payload processing checks including verification +/// by an EL client **and** has all requisite blob data to be imported into fork choice. +pub struct AvailableExecutedEnvelope { + pub envelope: AvailableEnvelope, + pub import_data: EnvelopeImportData, + pub payload_verification_outcome: PayloadVerificationOutcome, +} + +impl AvailableExecutedEnvelope { + pub fn new( + envelope: AvailableEnvelope, + import_data: EnvelopeImportData, + payload_verification_outcome: PayloadVerificationOutcome, + ) -> Self { + Self { + envelope, + import_data, + payload_verification_outcome, + } + } +} + +#[derive(Debug)] pub enum EnvelopeError { /// The envelope's block root is unknown. BlockRootUnknown { @@ -142,6 +200,8 @@ pub enum EnvelopeError { BlockProcessingError(BlockProcessingError), // Some EnvelopeProcessingError EnvelopeProcessingError(EnvelopeProcessingError), + // Error verifying the execution payload + ExecutionPayloadError(ExecutionPayloadError), } impl From for EnvelopeError { @@ -150,6 +210,12 @@ impl From for EnvelopeError { } } +impl From for EnvelopeError { + fn from(e: ExecutionPayloadError) -> Self { + EnvelopeError::ExecutionPayloadError(e) + } +} + impl From for EnvelopeError { fn from(e: BeaconStateError) -> Self { EnvelopeError::BeaconStateError(e) diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/payload_notifier.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/payload_notifier.rs index 6bbdd971a5..b485af0bfa 100644 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/payload_notifier.rs +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/payload_notifier.rs @@ -1,15 +1,10 @@ use std::sync::Arc; -use execution_layer::NewPayloadRequest; +use execution_layer::{NewPayloadRequest, NewPayloadRequestGloas}; use fork_choice::PayloadVerificationStatus; -use state_processing::{ - envelope_processing::partially_verify_payload_envelope, - per_block_processing::is_execution_enabled, -}; +use state_processing::per_block_processing::deneb::kzg_commitment_to_versioned_hash; use tracing::warn; -use types::{ - BeaconState, ExecutionPayloadBid, Hash256, SignedBeaconBlock, SignedExecutionPayloadEnvelope, -}; +use types::{SignedBeaconBlock, SignedExecutionPayloadEnvelope}; use crate::{ BeaconChain, BeaconChainTypes, BlockError, ExecutionPayloadError, NotifyExecutionLayer, @@ -19,33 +14,26 @@ use crate::{ /// Used to await the result of executing payload with a remote EE. pub struct PayloadNotifier { pub chain: Arc>, - pub envelope: Arc>, + envelope: Arc>, + block: Arc>, payload_verification_status: Option, } impl PayloadNotifier { pub fn new( chain: Arc>, - bid: &ExecutionPayloadBid, envelope: Arc>, - state: &BeaconState, + block: Arc>, notify_execution_layer: NotifyExecutionLayer, ) -> Result { let payload_verification_status = { - // Perform the initial stages of payload verification. - // - // We will duplicate these checks again during `per_block_processing`, however these - // checks are cheap and doing them here ensures we have verified them before marking - // the block as optimistically imported. This is particularly relevant in the case - // where we do not send the block to the EL at all. let payload_message = &envelope.message; - partially_verify_payload_envelope(state, &envelope, &chain.spec).unwrap(); // TODO(gloas) unwrap match notify_execution_layer { NotifyExecutionLayer::No if chain.config.optimistic_finalized_sync => { - // Create a NewPayloadRequest (no clones required) and check optimistic sync verifications - let new_payload_request: NewPayloadRequest = - payload_message.try_into()?; + // TODO(gloas) unwrap + let new_payload_request = + Self::build_new_payload_request(&envelope, &block).unwrap(); if let Err(e) = new_payload_request.perform_optimistic_sync_verifications() { warn!( block_number = ?payload_message.payload.block_number, @@ -65,6 +53,7 @@ impl PayloadNotifier { Ok(Self { chain, envelope, + block, payload_verification_status, }) } @@ -73,13 +62,34 @@ impl PayloadNotifier { if let Some(precomputed_status) = self.payload_verification_status { Ok(precomputed_status) } else { - // tODO(gloas) fix zero - notify_new_payload( - &self.chain, - Hash256::ZERO, - self.envelope.message.try_into()?, - ) - .await + let block_root = self.envelope.message.beacon_block_root; + let request = Self::build_new_payload_request(&self.envelope, &self.block)?; + notify_new_payload(&self.chain, block_root, request).await } } + + fn build_new_payload_request<'a>( + envelope: &'a SignedExecutionPayloadEnvelope, + block: &'a SignedBeaconBlock, + ) -> Result, BlockError> { + let bid = &block + .message() + .body() + .signed_execution_payload_bid() + .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))? + .message; + + let versioned_hashes = bid + .blob_kzg_commitments + .iter() + .map(kzg_commitment_to_versioned_hash) + .collect(); + + Ok(NewPayloadRequest::Gloas(NewPayloadRequestGloas { + execution_payload: &envelope.message.payload, + versioned_hashes, + parent_beacon_block_root: block.message().parent_root(), + execution_requests: &envelope.message.execution_requests, + })) + } } diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/signature_verified_envelope.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/signature_verified_envelope.rs deleted file mode 100644 index cd430daf5b..0000000000 --- a/beacon_node/beacon_chain/src/payload_envelope_verification/signature_verified_envelope.rs +++ /dev/null @@ -1,40 +0,0 @@ -use std::sync::Arc; - -use state_processing::ConsensusContext; -use types::{BeaconState, Hash256, SignedExecutionPayloadEnvelope}; - -use crate::{BeaconChain, BeaconChainTypes, payload_envelope_verification::{EnvelopeError, MaybeAvailableEnvelope}}; - - -/// A wrapper around a `SignedExecutionPayloadEnvelope` that indicates that all signatures (except the deposit -/// signatures) have been verified. -pub struct SignatureVerifiedEnvelope { - envelope: SignedExecutionPayloadEnvelope, - block_root: Hash256, - state: Option>, - consensus_context: ConsensusContext, -} - - -impl SignatureVerifiedEnvelope { - pub fn new( - envelope: Arc>, - state: &mut BeaconState, - block_root: Hash256, - chain: &BeaconChain, - ) -> Result { - let is_signature_valid = envelope.verify_signature_with_state(state, &chain.spec)?; - - if !is_signature_valid { - return Err(EnvelopeError::BadSignature) - } - - Self { - envelope, - block_root, - state - } - - todo!() - } -} \ No newline at end of file diff --git a/beacon_node/beacon_chain/src/payload_envelope_verification/tests.rs b/beacon_node/beacon_chain/src/payload_envelope_verification/tests.rs new file mode 100644 index 0000000000..381a5eaa4f --- /dev/null +++ b/beacon_node/beacon_chain/src/payload_envelope_verification/tests.rs @@ -0,0 +1,20 @@ +use std::sync::Arc; + +use crate::{AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, payload_envelope_verification::{ExecutedEnvelope, ExecutionPendingEnvelope}}; + +async fn import_execution_pending_envelope( + chain: Arc>, + execution_pending_envelope: ExecutionPendingEnvelope, +) -> Result { + match chain + .clone() + .into_executed_payload_envelope(execution_pending_envelope) + .await + .unwrap() + { + ExecutedEnvelope::Available(envelope) => todo!(), + ExecutedEnvelope::AvailabilityPending() => { + Err("AvailabilityPending not expected in this test. Block not imported.".to_string()) + } + } +} diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 145d171bbc..ba94296b85 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -5,7 +5,7 @@ use state_processing::per_block_processing::deneb::kzg_commitment_to_versioned_h use superstruct::superstruct; use types::{ BeaconBlockRef, BeaconStateError, EthSpec, ExecutionBlockHash, ExecutionPayload, - ExecutionPayloadEnvelope, ExecutionPayloadRef, Hash256, VersionedHash, + ExecutionPayloadRef, Hash256, VersionedHash, }; use types::{ ExecutionPayloadBellatrix, ExecutionPayloadCapella, ExecutionPayloadDeneb, diff --git a/consensus/state_processing/src/envelope_processing.rs b/consensus/state_processing/src/envelope_processing.rs index 0cdb6caaef..d46728dbbc 100644 --- a/consensus/state_processing/src/envelope_processing.rs +++ b/consensus/state_processing/src/envelope_processing.rs @@ -276,103 +276,3 @@ pub fn process_execution_payload_envelope( Ok(()) } - -/// Performs *partial* verification of the `payload envelope`. -pub fn partially_verify_payload_envelope( - state: &BeaconState, - signed_envelope: &SignedExecutionPayloadEnvelope, - spec: &ChainSpec, -) -> Result<(), EnvelopeProcessingError> { - let envelope = &signed_envelope.message; - let payload = &signed_envelope.message.payload; - - // Verify consistency with the beacon block - let latest_block_header_root = state.latest_block_header().tree_hash_root(); - envelope_verify!( - envelope.beacon_block_root == latest_block_header_root, - EnvelopeProcessingError::LatestBlockHeaderMismatch { - envelope_root: envelope.beacon_block_root, - block_header_root: latest_block_header_root, - } - ); - envelope_verify!( - envelope.slot == state.slot(), - EnvelopeProcessingError::SlotMismatch { - envelope_slot: envelope.slot, - parent_state_slot: state.slot(), - } - ); - - // Verify consistency with the committed bid - let committed_bid = state.latest_execution_payload_bid()?; - envelope_verify!( - envelope.builder_index == committed_bid.builder_index, - EnvelopeProcessingError::BuilderIndexMismatch { - committed_bid: committed_bid.builder_index, - envelope: envelope.builder_index, - } - ); - envelope_verify!( - committed_bid.prev_randao == payload.prev_randao, - EnvelopeProcessingError::PrevRandaoMismatch { - committed_bid: committed_bid.prev_randao, - envelope: payload.prev_randao, - } - ); - - // Verify consistency with expected withdrawals - // NOTE: we don't bother hashing here except in case of error, because we can just compare for - // equality directly. This equality check could be more straight-forward if the types were - // changed to match (currently we are comparing VariableList to List). This could happen - // coincidentally when we adopt ProgressiveList. - envelope_verify!( - payload.withdrawals.len() == state.payload_expected_withdrawals()?.len() - && payload - .withdrawals - .iter() - .eq(state.payload_expected_withdrawals()?.iter()), - EnvelopeProcessingError::WithdrawalsRootMismatch { - state: state.payload_expected_withdrawals()?.tree_hash_root(), - payload: payload.withdrawals.tree_hash_root(), - } - ); - - // Verify the gas limit - envelope_verify!( - committed_bid.gas_limit == payload.gas_limit, - EnvelopeProcessingError::GasLimitMismatch { - committed_bid: committed_bid.gas_limit, - envelope: payload.gas_limit, - } - ); - - // Verify the block hash - envelope_verify!( - committed_bid.block_hash == payload.block_hash, - EnvelopeProcessingError::BlockHashMismatch { - committed_bid: committed_bid.block_hash, - envelope: payload.block_hash, - } - ); - - // Verify consistency of the parent hash with respect to the previous execution payload - envelope_verify!( - payload.parent_hash == *state.latest_block_hash()?, - EnvelopeProcessingError::ParentHashMismatch { - state: *state.latest_block_hash()?, - envelope: payload.parent_hash, - } - ); - - // Verify timestamp - let state_timestamp = compute_timestamp_at_slot(state, state.slot(), spec)?; - envelope_verify!( - payload.timestamp == state_timestamp, - EnvelopeProcessingError::TimestampMismatch { - state: state_timestamp, - envelope: payload.timestamp, - } - ); - - Ok(()) -}