diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 22e50e4185..71a5ff3fce 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -70,7 +70,7 @@ use bls::{PublicKey, PublicKeyBytes}; use educe::Educe; use eth2::types::{BlockGossip, EventKind}; use execution_layer::PayloadStatus; -pub use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; +pub use fork_choice::{AttestationFromBlock, ParentImportedStatus, PayloadVerificationStatus}; use metrics::TryExt; use parking_lot::RwLockReadGuard; use proto_array::Block as ProtoBlock; @@ -882,13 +882,6 @@ impl GossipVerifiedBlock { }); } - // TODO(gloas) The following validation can only be completed once fork choice has been implemented: - // The block's parent execution payload (defined by bid.parent_block_hash) has been seen - // (via gossip or non-gossip sources) (a client MAY queue blocks for processing - // once the parent payload is retrieved). If execution_payload verification of block's execution - // payload parent by an execution node is complete, verify the block's execution payload - // parent (defined by bid.parent_block_hash) passes all validation. - drop(fork_choice_read_lock); // Track the number of skip slots between the block and its parent. @@ -1869,12 +1862,18 @@ fn verify_parent_block_is_known( fork_choice_read_lock: &RwLockReadGuard>, block: Arc>, ) -> Result<(ProtoBlock, Arc>), BlockError> { - if let Some(proto_block) = fork_choice_read_lock.get_block(&block.parent_root()) { - Ok((proto_block, block)) - } else { - Err(BlockError::ParentUnknown { - parent_root: block.parent_root(), - }) + // The block's parent execution payload (defined by bid.parent_block_hash) has been seen + // (via gossip or non-gossip sources) (a client MAY queue blocks for processing + // once the parent payload is retrieved). If execution_payload verification of block's execution + // payload parent by an execution node is complete, verify the block's execution payload + // parent (defined by bid.parent_block_hash) passes all validation. + match fork_choice_read_lock.is_parent_imported(&block) { + ParentImportedStatus::Imported(parent) => Ok((parent, block)), + ParentImportedStatus::UnknownBlock | ParentImportedStatus::UnimportedPayload => { + Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }) + } } } diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 804268a613..1aa9356ab1 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -85,7 +85,7 @@ pub use beacon_fork_choice_store::{ }; pub use block_verification::{ BlockError, ExecutionPayloadError, ExecutionPendingBlock, GossipVerifiedBlock, - IntoExecutionPendingBlock, IntoGossipVerifiedBlock, InvalidSignature, + IntoExecutionPendingBlock, IntoGossipVerifiedBlock, InvalidSignature, ParentImportedStatus, PayloadVerificationOutcome, PayloadVerificationStatus, build_blob_data_column_sidecars, get_block_root, signature_verify_chain_segment, }; diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index 090b7f0ddc..7347fda517 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -7,9 +7,10 @@ use crate::sync::network_context::{ LookupRequestResult, PeerGroup, ReqId, RpcRequestSendError, RpcResponseError, SendErrorProcessor, SyncNetworkContext, }; +use beacon_chain::BeaconChainTypes; use beacon_chain::BlockProcessStatus; +use beacon_chain::ParentImportedStatus; use beacon_chain::block_verification_types::AsBlock; -use beacon_chain::{BeaconChainTypes, ExecutionStatus}; use educe::Educe; use lighthouse_network::service::api_types::Id; use parking_lot::RwLock; @@ -40,60 +41,6 @@ pub struct AwaitingParent { } impl AwaitingParent { - pub fn is_parent_imported(&self, cx: &mut SyncNetworkContext) -> bool { - if self.parent_is_genesis() { - // Zero hash is the parent of the genesis block — not a real block, so no - // parent-known check is needed. Fall through to send the block for processing. - return true; - } - - if let Some(parent_block) = cx - .chain - .canonical_head - .fork_choice_read_lock() - .get_block(&self.parent_root) - { - if parent_block.slot == cx.spec().genesis_slot { - // The genesis block is always imported by definition - return true; - } - - if let Some(gloas_bid_parent_hash) = self.gloas_bid_parent_hash { - // Post-gloas block, check if it's FULL or EMPTY - let parent_hash = match parent_block.execution_status { - ExecutionStatus::Valid(hash) => hash, - ExecutionStatus::Invalid(hash) => hash, - ExecutionStatus::Optimistic(hash) => hash, - ExecutionStatus::Irrelevant(_) => { - if let Some(hash) = parent_block.execution_payload_block_hash { - hash - } else { - // This should never happen! - return false; - } - } - }; - let is_full = gloas_bid_parent_hash == parent_hash; - if is_full { - // Post-gloas block FULL, we need the payload to be imported first - cx.chain - .canonical_head - .fork_choice_read_lock() - .is_payload_received(&self.parent_root) - } else { - // Post-gloas block EMPTY, and block is imported - true - } - } else { - // Pre-gloas block - true - } - } else { - // Parent is unknown - false - } - } - pub fn parent_is_genesis(&self) -> bool { self.parent_root == Hash256::ZERO } @@ -680,6 +627,16 @@ impl SingleBlockLookup { self.awaiting_parent } + /// The parent relationship implied by this lookup's downloaded block: the parent root plus + /// (post-gloas) the parent's committed payload hash taken from this block's bid. `None` until + /// the block has been downloaded. Used to donate this lookup's peers to a FULL parent's + /// payload fetch. + pub fn downloaded_parent(&self) -> Option { + self.block_request + .peek_block() + .map(|block| AwaitingParent::from_block(block)) + } + /// Mark this lookup as no longer awaiting a parent lookup. Components can be sent for /// processing. pub fn resolve_awaiting_parent(&mut self) { @@ -778,15 +735,29 @@ impl SingleBlockLookup { break; } - let awaiting_parent = AwaitingParent::from_block(block); - - if !awaiting_parent.is_parent_imported(cx) { - self.awaiting_parent = Some(awaiting_parent); - return Ok(LookupResult::ParentUnknown { - awaiting_parent, - block_root: self.block_root, - peers: self.all_peers(), - }); + // Check if the parent block is known to fork-choice. If the block is FULL + // expect the payload to be imported too. + match cx + .chain + .canonical_head + .fork_choice_read_lock() + .is_parent_imported(block) + { + // Parent block is imported (and, if this block is FULL, its payload too): + // safe to send this block for processing. + ParentImportedStatus::Imported(_) => {} + // Parent block is unknown, or it's FULL and the parent's payload has not + // been imported yet. Park this lookup until the parent resolves. + ParentImportedStatus::UnknownBlock + | ParentImportedStatus::UnimportedPayload => { + let awaiting_parent = AwaitingParent::from_block(block); + self.awaiting_parent = Some(awaiting_parent); + return Ok(LookupResult::ParentUnknown { + awaiting_parent, + block_root: self.block_root, + peers: self.all_peers(), + }); + } } let block = block.clone(); diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index bb43396473..c8cf7b68e3 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -62,7 +62,7 @@ enum RangeBlockDataRequest { } #[derive(Debug)] -pub(crate) enum CouplingError { +pub enum CouplingError { InternalError(String), /// The peer we requested the columns from was faulty/malicious DataColumnPeerFailure { diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 2de8ce7d81..b2d8ba4b57 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -207,6 +207,13 @@ pub enum InvalidPayloadAttestation { }, } +#[allow(clippy::large_enum_variant)] +pub enum ParentImportedStatus { + Imported(ProtoBlock), + UnknownBlock, + UnimportedPayload, +} + impl From for Error { fn from(e: String) -> Self { Error::ProtoArrayStringError(e) @@ -1548,6 +1555,21 @@ where .map_err(Error::ProtoArrayStringError) } + /// Returns the import status of the parent + pub fn is_parent_imported(&self, block: &SignedBeaconBlock) -> ParentImportedStatus { + if let Some(proto_block) = self.get_block(&block.parent_root()) { + if let Ok(bid) = block.message().body().signed_execution_payload_bid() + && proto_block.is_child_full(bid) + && !self.is_payload_received(&block.parent_root()) + { + return ParentImportedStatus::UnimportedPayload; + } + ParentImportedStatus::Imported(proto_block) + } else { + ParentImportedStatus::UnknownBlock + } + } + /// Returns whether the proposer should extend the execution payload chain of the given block. pub fn should_extend_payload(&self, block_root: &Hash256) -> Result> { let proposer_boost_root = self.fc_store.proposer_boost_root(); diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 159eab0ec0..d2134d0bcb 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -4,9 +4,9 @@ mod metrics; pub use crate::fork_choice::{ AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, - InvalidAttestation, InvalidBlock, InvalidPayloadAttestation, PayloadVerificationStatus, - PersistedForkChoice, PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, - ResetPayloadStatuses, + InvalidAttestation, InvalidBlock, InvalidPayloadAttestation, ParentImportedStatus, + PayloadVerificationStatus, PersistedForkChoice, PersistedForkChoiceV28, PersistedForkChoiceV29, + QueuedAttestation, ResetPayloadStatuses, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::{ diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 96d2302266..b7835da1a1 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -17,7 +17,7 @@ use std::{ }; use types::{ AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, - Slot, + SignedExecutionPayloadBid, Slot, }; pub const DEFAULT_PRUNE_THRESHOLD: usize = 256; @@ -292,6 +292,19 @@ impl Block { } } } + + pub fn is_child_full(&self, child_bid: &SignedExecutionPayloadBid) -> bool { + if let Some(execution_payload_block_hash) = self.execution_payload_block_hash { + execution_payload_block_hash == child_bid.message.parent_block_hash + } else if let Some(execution_block_hash) = self.execution_status.block_hash() { + // Parent is before Gloas, and child is gloas + execution_block_hash == child_bid.message.parent_block_hash + } else { + // TODO(gloas): What to return here? The child is Gloas but parent doesn't have an + // execution hash + false + } + } } /// A Vec-wrapper which will grow to match any request.