diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 2c2b36c37..49cca4a7a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -75,7 +75,6 @@ use state_processing::{ state_advance::{complete_state_advance, partial_state_advance}, BlockSignatureStrategy, SigVerifiedOp, VerifyBlockRoot, }; -use std::borrow::Cow; use std::cmp::Ordering; use std::collections::HashMap; use std::collections::HashSet; @@ -1430,8 +1429,13 @@ impl BeaconChain { pub fn get_aggregated_attestation( &self, data: &AttestationData, - ) -> Option> { - self.naive_aggregation_pool.read().get(data) + ) -> Result>, Error> { + if let Some(attestation) = self.naive_aggregation_pool.read().get(data) { + self.filter_optimistic_attestation(attestation) + .map(Option::Some) + } else { + Ok(None) + } } /// Returns an aggregated `Attestation`, if any, that has a matching @@ -1442,10 +1446,43 @@ impl BeaconChain { &self, slot: Slot, attestation_data_root: &Hash256, - ) -> Option> { - self.naive_aggregation_pool + ) -> Result>, Error> { + if let Some(attestation) = self + .naive_aggregation_pool .read() .get_by_slot_and_root(slot, attestation_data_root) + { + self.filter_optimistic_attestation(attestation) + .map(Option::Some) + } else { + Ok(None) + } + } + + /// Returns `Ok(attestation)` if the supplied `attestation` references a valid + /// `beacon_block_root`. + fn filter_optimistic_attestation( + &self, + attestation: Attestation, + ) -> Result, Error> { + let beacon_block_root = attestation.data.beacon_block_root; + match self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + { + // The attestation references a block that is not in fork choice, it must be + // pre-finalization. + None => Err(Error::CannotAttestToFinalizedBlock { beacon_block_root }), + // The attestation references a fully valid `beacon_block_root`. + Some(execution_status) if execution_status.is_valid_or_irrelevant() => Ok(attestation), + // The attestation references a block that has not been verified by an EL (i.e. it + // is optimistic or invalid). Don't return the block, return an error instead. + Some(execution_status) => Err(Error::HeadBlockNotFullyVerified { + beacon_block_root, + execution_status, + }), + } } /// Return an aggregated `SyncCommitteeContribution` matching the given `root`. @@ -1481,6 +1518,8 @@ impl BeaconChain { // // In effect, the early attester cache prevents slow database IO from causing missed // head/target votes. + // + // The early attester cache should never contain an optimistically imported block. match self .early_attester_cache .try_attest(request_slot, request_index, &self.spec) @@ -1597,6 +1636,22 @@ impl BeaconChain { } drop(head_timer); + // Only attest to a block if it is fully verified (i.e. not optimistic or invalid). + match self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + { + Some(execution_status) if execution_status.is_valid_or_irrelevant() => (), + Some(execution_status) => { + return Err(Error::HeadBlockNotFullyVerified { + beacon_block_root, + execution_status, + }) + } + None => return Err(Error::HeadMissingFromForkChoice(beacon_block_root)), + }; + /* * Phase 2/2: * @@ -1657,64 +1712,6 @@ impl BeaconChain { }) } - /// Produces an "unaggregated" attestation for the given `slot` and `index` that attests to - /// `beacon_block_root`. The provided `state` should match the `block.state_root` for the - /// `block` identified by `beacon_block_root`. - /// - /// The attestation doesn't _really_ have anything about it that makes it unaggregated per say, - /// however this function is only required in the context of forming an unaggregated - /// attestation. It would be an (undetectable) violation of the protocol to create a - /// `SignedAggregateAndProof` based upon the output of this function. - pub fn produce_unaggregated_attestation_for_block( - &self, - slot: Slot, - index: CommitteeIndex, - beacon_block_root: Hash256, - mut state: Cow>, - state_root: Hash256, - ) -> Result, Error> { - let epoch = slot.epoch(T::EthSpec::slots_per_epoch()); - - if state.slot() > slot { - return Err(Error::CannotAttestToFutureState); - } else if state.current_epoch() < epoch { - let mut_state = state.to_mut(); - // Only perform a "partial" state advance since we do not require the state roots to be - // accurate. - partial_state_advance( - mut_state, - Some(state_root), - epoch.start_slot(T::EthSpec::slots_per_epoch()), - &self.spec, - )?; - mut_state.build_committee_cache(RelativeEpoch::Current, &self.spec)?; - } - - let committee_len = state.get_beacon_committee(slot, index)?.committee.len(); - - let target_slot = epoch.start_slot(T::EthSpec::slots_per_epoch()); - let target_root = if state.slot() <= target_slot { - beacon_block_root - } else { - *state.get_block_root(target_slot)? - }; - - Ok(Attestation { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot, - index, - beacon_block_root, - source: state.current_justified_checkpoint(), - target: Checkpoint { - epoch, - root: target_root, - }, - }, - signature: AggregateSignature::empty(), - }) - } - /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for /// multiple attestations using batch BLS verification. Batch verification can provide /// significant CPU-time savings compared to individual verification. @@ -2678,13 +2675,20 @@ impl BeaconChain { } } - // If the block is recent enough, check to see if it becomes the head block. If so, apply it - // to the early attester cache. This will allow attestations to the block without waiting - // for the block and state to be inserted to the database. + // If the block is recent enough and it was not optimistically imported, check to see if it + // becomes the head block. If so, apply it to the early attester cache. This will allow + // attestations to the block without waiting for the block and state to be inserted to the + // database. // // Only performing this check on recent blocks avoids slowing down sync with lots of calls // to fork choice `get_head`. - if block.slot() + EARLY_ATTESTER_CACHE_HISTORIC_SLOTS >= current_slot { + // + // Optimistically imported blocks are not added to the cache since the cache is only useful + // for a small window of time and the complexity of keeping track of the optimistic status + // is not worth it. + if !payload_verification_status.is_optimistic() + && block.slot() + EARLY_ATTESTER_CACHE_HISTORIC_SLOTS >= current_slot + { let new_head_root = fork_choice .get_head(current_slot, &self.spec) .map_err(BeaconChainError::from)?; @@ -4185,7 +4189,7 @@ impl BeaconChain { let status = match head_block.execution_status { ExecutionStatus::Valid(block_hash) => HeadSafetyStatus::Safe(Some(block_hash)), ExecutionStatus::Invalid(block_hash) => HeadSafetyStatus::Invalid(block_hash), - ExecutionStatus::Unknown(block_hash) => HeadSafetyStatus::Unsafe(block_hash), + ExecutionStatus::Optimistic(block_hash) => HeadSafetyStatus::Unsafe(block_hash), ExecutionStatus::Irrelevant(_) => HeadSafetyStatus::Safe(None), }; diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 63ae41492..d156b92c5 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1165,7 +1165,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> { // If the payload did not validate or invalidate the block, check to see if this block is // valid for optimistic import. - if payload_verification_status == PayloadVerificationStatus::NotVerified { + if payload_verification_status.is_optimistic() { let current_slot = chain .slot_clock .now() diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 79f7346ca..8d2754141 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -9,6 +9,7 @@ use crate::observed_aggregates::Error as ObservedAttestationsError; use crate::observed_attesters::Error as ObservedAttestersError; use crate::observed_block_producers::Error as ObservedBlockProducersError; use execution_layer::PayloadStatus; +use fork_choice::ExecutionStatus; use futures::channel::mpsc::TrySendError; use operation_pool::OpPoolError; use safe_arith::ArithError; @@ -162,6 +163,13 @@ pub enum BeaconChainError { fork_choice: Hash256, }, InvalidSlot(Slot), + HeadBlockNotFullyVerified { + beacon_block_root: Hash256, + execution_status: ExecutionStatus, + }, + CannotAttestToFinalizedBlock { + beacon_block_root: Hash256, + }, } easy_from_to!(SlotProcessingError, BeaconChainError); diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index d95a7a671..47446e559 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -61,7 +61,7 @@ pub fn notify_new_payload( Ok(status) => match status { PayloadStatus::Valid => Ok(PayloadVerificationStatus::Verified), PayloadStatus::Syncing | PayloadStatus::Accepted => { - Ok(PayloadVerificationStatus::NotVerified) + Ok(PayloadVerificationStatus::Optimistic) } PayloadStatus::Invalid { latest_valid_hash, .. @@ -193,7 +193,7 @@ pub fn validate_execution_payload_for_gossip( let is_merge_transition_complete = match parent_block.execution_status { // Optimistically declare that an "unknown" status block has completed the merge. - ExecutionStatus::Valid(_) | ExecutionStatus::Unknown(_) => true, + ExecutionStatus::Valid(_) | ExecutionStatus::Optimistic(_) => true, // It's impossible for an irrelevant block to have completed the merge. It is pre-merge // by definition. ExecutionStatus::Irrelevant(_) => false, diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index 3ae3bf8a3..c96dc1b36 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -172,7 +172,7 @@ pub fn reset_fork_choice_to_finalization, Cold: It // retro-actively determine if they were valid or not. // // This scenario is so rare that it seems OK to double-verify some blocks. - let payload_verification_status = PayloadVerificationStatus::NotVerified; + let payload_verification_status = PayloadVerificationStatus::Optimistic; let (block, _) = block.deconstruct(); fork_choice diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6684aeec8..7263bf051 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -31,7 +31,10 @@ use rayon::prelude::*; use sensitive_url::SensitiveUrl; use slog::Logger; use slot_clock::TestingSlotClock; -use state_processing::{state_advance::complete_state_advance, StateRootStrategy}; +use state_processing::{ + state_advance::{complete_state_advance, partial_state_advance}, + StateRootStrategy, +}; use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::str::FromStr; @@ -42,15 +45,7 @@ use task_executor::ShutdownReason; use tree_hash::TreeHash; use types::sync_selection_proof::SyncSelectionProof; pub use types::test_utils::generate_deterministic_keypairs; -use types::{ - typenum::U4294967296, Address, AggregateSignature, Attestation, AttestationData, - AttesterSlashing, BeaconBlock, BeaconState, BeaconStateHash, ChainSpec, Checkpoint, Deposit, - DepositData, Domain, Epoch, EthSpec, ForkName, Graffiti, Hash256, IndexedAttestation, Keypair, - ProposerSlashing, PublicKeyBytes, SelectionProof, SignatureBytes, SignedAggregateAndProof, - SignedBeaconBlock, SignedBeaconBlockHash, SignedContributionAndProof, SignedRoot, - SignedVoluntaryExit, Slot, SubnetId, SyncCommittee, SyncCommitteeContribution, - SyncCommitteeMessage, VariableList, VoluntaryExit, -}; +use types::{typenum::U4294967296, *}; // 4th September 2019 pub const HARNESS_GENESIS_TIME: u64 = 1_567_552_690; @@ -685,6 +680,67 @@ where (signed_block, pre_state) } + /// Produces an "unaggregated" attestation for the given `slot` and `index` that attests to + /// `beacon_block_root`. The provided `state` should match the `block.state_root` for the + /// `block` identified by `beacon_block_root`. + /// + /// The attestation doesn't _really_ have anything about it that makes it unaggregated per say, + /// however this function is only required in the context of forming an unaggregated + /// attestation. It would be an (undetectable) violation of the protocol to create a + /// `SignedAggregateAndProof` based upon the output of this function. + /// + /// This function will produce attestations to optimistic blocks, which is against the + /// specification but useful during testing. + pub fn produce_unaggregated_attestation_for_block( + &self, + slot: Slot, + index: CommitteeIndex, + beacon_block_root: Hash256, + mut state: Cow>, + state_root: Hash256, + ) -> Result, BeaconChainError> { + let epoch = slot.epoch(E::slots_per_epoch()); + + if state.slot() > slot { + return Err(BeaconChainError::CannotAttestToFutureState); + } else if state.current_epoch() < epoch { + let mut_state = state.to_mut(); + // Only perform a "partial" state advance since we do not require the state roots to be + // accurate. + partial_state_advance( + mut_state, + Some(state_root), + epoch.start_slot(E::slots_per_epoch()), + &self.spec, + )?; + mut_state.build_committee_cache(RelativeEpoch::Current, &self.spec)?; + } + + let committee_len = state.get_beacon_committee(slot, index)?.committee.len(); + + let target_slot = epoch.start_slot(E::slots_per_epoch()); + let target_root = if state.slot() <= target_slot { + beacon_block_root + } else { + *state.get_block_root(target_slot)? + }; + + Ok(Attestation { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot, + index, + beacon_block_root, + source: state.current_justified_checkpoint(), + target: Checkpoint { + epoch, + root: target_root, + }, + }, + signature: AggregateSignature::empty(), + }) + } + /// A list of attestations for each committee for the given slot. /// /// The first layer of the Vec is organised per committee. For example, if the return value is @@ -716,7 +772,6 @@ where return None; } let mut attestation = self - .chain .produce_unaggregated_attestation_for_block( attestation_slot, bc.index, @@ -899,6 +954,7 @@ where let aggregate = self .chain .get_aggregated_attestation(&attestation.data) + .unwrap() .unwrap_or_else(|| { committee_attestations.iter().skip(1).fold( attestation.clone(), diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 84317196b..cce7c344f 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -14,6 +14,7 @@ use proto_array::{Error as ProtoArrayError, ExecutionStatus}; use slot_clock::SlotClock; use std::time::Duration; use task_executor::ShutdownReason; +use tree_hash::TreeHash; use types::*; const VALIDATOR_COUNT: usize = 32; @@ -221,8 +222,8 @@ impl InvalidPayloadRig { let execution_status = self.execution_status(root.into()); match is_valid { - Payload::Syncing => assert!(execution_status.is_not_verified()), - Payload::Valid => assert!(execution_status.is_valid()), + Payload::Syncing => assert!(execution_status.is_optimistic()), + Payload::Valid => assert!(execution_status.is_valid_and_post_bellatrix()), Payload::Invalid { .. } => unreachable!(), } @@ -309,7 +310,7 @@ fn invalid_payload_invalidates_parent() { latest_valid_hash: Some(latest_valid_hash), }); - assert!(rig.execution_status(roots[0]).is_valid()); + assert!(rig.execution_status(roots[0]).is_valid_and_post_bellatrix()); assert!(rig.execution_status(roots[1]).is_invalid()); assert!(rig.execution_status(roots[2]).is_invalid()); @@ -397,9 +398,9 @@ fn pre_finalized_latest_valid_hash() { let slot = Slot::new(i); let root = rig.block_root_at_slot(slot).unwrap(); if slot == 1 { - assert!(rig.execution_status(root).is_valid()); + assert!(rig.execution_status(root).is_valid_and_post_bellatrix()); } else { - assert!(rig.execution_status(root).is_not_verified()); + assert!(rig.execution_status(root).is_optimistic()); } } } @@ -446,7 +447,7 @@ fn latest_valid_hash_will_validate() { } else if slot == 0 { assert!(execution_status.is_irrelevant()) } else { - assert!(execution_status.is_valid()) + assert!(execution_status.is_valid_and_post_bellatrix()) } } } @@ -484,9 +485,9 @@ fn latest_valid_hash_is_junk() { let slot = Slot::new(i); let root = rig.block_root_at_slot(slot).unwrap(); if slot == 1 { - assert!(rig.execution_status(root).is_valid()); + assert!(rig.execution_status(root).is_valid_and_post_bellatrix()); } else { - assert!(rig.execution_status(root).is_not_verified()); + assert!(rig.execution_status(root).is_optimistic()); } } } @@ -556,7 +557,7 @@ fn invalidates_all_descendants() { let execution_status = rig.execution_status(root); if slot <= latest_valid_slot { // Blocks prior to the latest valid hash are valid. - assert!(execution_status.is_valid()); + assert!(execution_status.is_valid_and_post_bellatrix()); } else { // Blocks after the latest valid hash are invalid. assert!(execution_status.is_invalid()); @@ -607,7 +608,7 @@ fn switches_heads() { assert_eq!(rig.head_info().block_root, fork_block_root); // The fork block has not yet been validated. - assert!(rig.execution_status(fork_block_root).is_not_verified()); + assert!(rig.execution_status(fork_block_root).is_optimistic()); for root in blocks { let slot = rig.harness.chain.get_block(&root).unwrap().unwrap().slot(); @@ -620,7 +621,7 @@ fn switches_heads() { let execution_status = rig.execution_status(root); if slot <= latest_valid_slot { // Blocks prior to the latest valid hash are valid. - assert!(execution_status.is_valid()); + assert!(execution_status.is_valid_and_post_bellatrix()); } else { // Blocks after the latest valid hash are invalid. assert!(execution_status.is_invalid()); @@ -691,13 +692,13 @@ fn manually_validate_child() { let parent = rig.import_block(Payload::Syncing); let child = rig.import_block(Payload::Syncing); - assert!(rig.execution_status(parent).is_not_verified()); - assert!(rig.execution_status(child).is_not_verified()); + assert!(rig.execution_status(parent).is_optimistic()); + assert!(rig.execution_status(child).is_optimistic()); rig.validate_manually(child); - assert!(rig.execution_status(parent).is_valid()); - assert!(rig.execution_status(child).is_valid()); + assert!(rig.execution_status(parent).is_valid_and_post_bellatrix()); + assert!(rig.execution_status(child).is_valid_and_post_bellatrix()); } #[test] @@ -709,13 +710,13 @@ fn manually_validate_parent() { let parent = rig.import_block(Payload::Syncing); let child = rig.import_block(Payload::Syncing); - assert!(rig.execution_status(parent).is_not_verified()); - assert!(rig.execution_status(child).is_not_verified()); + assert!(rig.execution_status(parent).is_optimistic()); + assert!(rig.execution_status(child).is_optimistic()); rig.validate_manually(parent); - assert!(rig.execution_status(parent).is_valid()); - assert!(rig.execution_status(child).is_not_verified()); + assert!(rig.execution_status(parent).is_valid_and_post_bellatrix()); + assert!(rig.execution_status(child).is_optimistic()); } #[test] @@ -819,7 +820,7 @@ fn invalid_parent() { block_root, Duration::from_secs(0), &state, - PayloadVerificationStatus::NotVerified, + PayloadVerificationStatus::Optimistic, &rig.harness.chain.spec ), Err(ForkChoiceError::ProtoArrayError(message)) @@ -885,3 +886,107 @@ fn payload_preparation_before_transition_block() { assert_eq!(payload_attributes.suggested_fee_recipient, fee_recipient); assert_eq!(fork_choice_state.head_block_hash, latest_block_hash); } + +#[test] +fn attesting_to_optimistic_head() { + let mut rig = InvalidPayloadRig::new(); + rig.move_to_terminal_block(); + rig.import_block(Payload::Valid); // Import a valid transition block. + + let root = rig.import_block(Payload::Syncing); + + let head = rig.harness.chain.head().unwrap(); + let slot = head.beacon_block.slot(); + assert_eq!( + head.beacon_block_root, root, + "the head should be the latest imported block" + ); + assert!( + rig.execution_status(root).is_optimistic(), + "the head should be optimistic" + ); + + /* + * Define an attestation for use during testing. It doesn't have a valid signature, but that's + * not necessary here. + */ + + let attestation = { + let mut attestation = rig + .harness + .chain + .produce_unaggregated_attestation(Slot::new(0), 0) + .unwrap(); + + attestation.aggregation_bits.set(0, true).unwrap(); + attestation.data.slot = slot; + attestation.data.beacon_block_root = root; + + rig.harness + .chain + .naive_aggregation_pool + .write() + .insert(&attestation) + .unwrap(); + + attestation + }; + + /* + * Define some closures to produce attestations. + */ + + let produce_unaggregated = || rig.harness.chain.produce_unaggregated_attestation(slot, 0); + + let get_aggregated = || { + rig.harness + .chain + .get_aggregated_attestation(&attestation.data) + }; + + let get_aggregated_by_slot_and_root = || { + rig.harness + .chain + .get_aggregated_attestation_by_slot_and_root( + attestation.data.slot, + &attestation.data.tree_hash_root(), + ) + }; + + /* + * Ensure attestation production fails with an optimistic head. + */ + + macro_rules! assert_head_block_not_fully_verified { + ($func: expr) => { + assert!(matches!( + $func, + Err(BeaconChainError::HeadBlockNotFullyVerified { + beacon_block_root, + execution_status + }) + if beacon_block_root == root && matches!(execution_status, ExecutionStatus::Optimistic(_)) + )); + } + } + + assert_head_block_not_fully_verified!(produce_unaggregated()); + assert_head_block_not_fully_verified!(get_aggregated()); + assert_head_block_not_fully_verified!(get_aggregated_by_slot_and_root()); + + /* + * Ensure attestation production succeeds once the head is verified. + * + * This is effectively a control for the previous tests. + */ + + rig.validate_manually(root); + assert!( + rig.execution_status(root).is_valid_and_post_bellatrix(), + "the head should no longer be optimistic" + ); + + produce_unaggregated().unwrap(); + get_aggregated().unwrap(); + get_aggregated_by_slot_and_root().unwrap(); +} diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 1b8bb16c5..7b58ce682 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2138,6 +2138,12 @@ pub fn serve( query.slot, &query.attestation_data_root, ) + .map_err(|e| { + warp_utils::reject::custom_bad_request(format!( + "unable to fetch aggregate: {:?}", + e + )) + })? .map(api_types::GenericResponse::from) .ok_or_else(|| { warp_utils::reject::custom_not_found( diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index d71565fc1..c876a69be 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -122,11 +122,22 @@ pub enum PayloadVerificationStatus { /// An EL has declared the execution payload to be valid. Verified, /// An EL has not yet made a determination about the execution payload. - NotVerified, + Optimistic, /// The block is either pre-merge-fork, or prior to the terminal PoW block. Irrelevant, } +impl PayloadVerificationStatus { + /// Returns `true` if the payload was optimistically imported. + pub fn is_optimistic(&self) -> bool { + match self { + PayloadVerificationStatus::Verified => false, + PayloadVerificationStatus::Optimistic => true, + PayloadVerificationStatus::Irrelevant => false, + } + } +} + /// Calculate how far `slot` lies from the start of its epoch. /// /// ## Specification @@ -668,7 +679,9 @@ where } else { match payload_verification_status { PayloadVerificationStatus::Verified => ExecutionStatus::Valid(block_hash), - PayloadVerificationStatus::NotVerified => ExecutionStatus::Unknown(block_hash), + PayloadVerificationStatus::Optimistic => { + ExecutionStatus::Optimistic(block_hash) + } // It would be a logic error to declare a block irrelevant if it has an // execution payload with a non-zero block hash. PayloadVerificationStatus::Irrelevant => { @@ -944,6 +957,15 @@ where } } + /// Returns an `ExecutionStatus` if the block is known **and** a descendant of the finalized root. + pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { + if self.is_descendant_of_finalized(*block_root) { + self.proto_array.get_block_execution_status(block_root) + } else { + None + } + } + /// Returns the `ProtoBlock` for the justified checkpoint. /// /// ## Notes diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index d4a95994e..157306dd5 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -6,4 +6,4 @@ pub use crate::fork_choice::{ PayloadVerificationStatus, PersistedForkChoice, QueuedAttestation, }; pub use fork_choice_store::ForkChoiceStore; -pub use proto_array::{Block as ProtoBlock, InvalidationOperation}; +pub use proto_array::{Block as ProtoBlock, ExecutionStatus, InvalidationOperation}; diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index f2b51c1fd..2980c019e 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -85,7 +85,7 @@ impl ForkChoiceTestDefinition { self.finalized_checkpoint, junk_shuffling_id.clone(), junk_shuffling_id, - ExecutionStatus::Unknown(ExecutionBlockHash::zero()), + ExecutionStatus::Optimistic(ExecutionBlockHash::zero()), ) .expect("should create fork choice struct"); @@ -189,9 +189,9 @@ impl ForkChoiceTestDefinition { justified_checkpoint, finalized_checkpoint, // All blocks are imported optimistically. - execution_status: ExecutionStatus::Unknown(ExecutionBlockHash::from_root( - root, - )), + execution_status: ExecutionStatus::Optimistic( + ExecutionBlockHash::from_root(root), + ), }; fork_choice.process_block(block).unwrap_or_else(|e| { panic!( diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 5b0a46e95..3f7909553 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -387,7 +387,7 @@ impl ProtoArray { ExecutionStatus::Irrelevant(_) => return Ok(()), // The block has an unknown status, set it to valid since any ancestor of a valid // payload can be considered valid. - ExecutionStatus::Unknown(payload_block_hash) => { + ExecutionStatus::Optimistic(payload_block_hash) => { node.execution_status = ExecutionStatus::Valid(payload_block_hash); if let Some(parent_index) = node.parent { parent_index @@ -458,7 +458,7 @@ impl ProtoArray { match node.execution_status { ExecutionStatus::Valid(hash) | ExecutionStatus::Invalid(hash) - | ExecutionStatus::Unknown(hash) => { + | ExecutionStatus::Optimistic(hash) => { // If we're no longer processing the `head_block_root` and the last valid // ancestor is unknown, exit this loop and proceed to invalidate and // descendants of `head_block_root`/`latest_valid_ancestor_root`. @@ -516,7 +516,7 @@ impl ProtoArray { payload_block_hash: *hash, }) } - ExecutionStatus::Unknown(hash) => { + ExecutionStatus::Optimistic(hash) => { invalidated_indices.insert(index); node.execution_status = ExecutionStatus::Invalid(*hash); @@ -580,7 +580,7 @@ impl ProtoArray { payload_block_hash: *hash, }) } - ExecutionStatus::Unknown(hash) | ExecutionStatus::Invalid(hash) => { + ExecutionStatus::Optimistic(hash) | ExecutionStatus::Invalid(hash) => { node.execution_status = ExecutionStatus::Invalid(*hash) } ExecutionStatus::Irrelevant(_) => { diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 8f3f5ed79..88bf7840c 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1,5 +1,5 @@ use crate::error::Error; -use crate::proto_array::{InvalidationOperation, Iter, ProposerBoost, ProtoArray}; +use crate::proto_array::{InvalidationOperation, Iter, ProposerBoost, ProtoArray, ProtoNode}; use crate::ssz_container::SszContainer; use serde_derive::{Deserialize, Serialize}; use ssz::{Decode, Encode}; @@ -28,7 +28,7 @@ pub enum ExecutionStatus { /// An EL has determined that the payload is invalid. Invalid(ExecutionBlockHash), /// An EL has not yet verified the execution payload. - Unknown(ExecutionBlockHash), + Optimistic(ExecutionBlockHash), /// The block is either prior to the merge fork, or after the merge fork but before the terminal /// PoW block has been found. /// @@ -52,30 +52,48 @@ impl ExecutionStatus { match self { ExecutionStatus::Valid(hash) | ExecutionStatus::Invalid(hash) - | ExecutionStatus::Unknown(hash) => Some(*hash), + | ExecutionStatus::Optimistic(hash) => Some(*hash), ExecutionStatus::Irrelevant(_) => None, } } /// Returns `true` if the block: /// - /// - Has execution enabled + /// - Has a valid payload, OR + /// - Does not have execution enabled. + /// + /// Whenever this function returns `true`, the block is *fully valid*. + pub fn is_valid_or_irrelevant(&self) -> bool { + matches!( + self, + ExecutionStatus::Valid(_) | ExecutionStatus::Irrelevant(_) + ) + } + + /// Returns `true` if the block: + /// + /// - Has execution enabled, AND /// - Has a valid payload - pub fn is_valid(&self) -> bool { + /// + /// This function will return `false` for any block from a slot prior to the Bellatrix fork. + /// This means that some blocks that are perfectly valid will still receive a `false` response. + /// See `Self::is_valid_or_irrelevant` for a function that will always return `true` given any + /// perfectly valid block. + pub fn is_valid_and_post_bellatrix(&self) -> bool { matches!(self, ExecutionStatus::Valid(_)) } /// Returns `true` if the block: /// - /// - Has execution enabled + /// - Has execution enabled, AND /// - Has a payload that has not yet been verified by an EL. - pub fn is_not_verified(&self) -> bool { - matches!(self, ExecutionStatus::Unknown(_)) + pub fn is_optimistic(&self) -> bool { + matches!(self, ExecutionStatus::Optimistic(_)) } /// Returns `true` if the block: /// - /// - Has execution enabled + /// - Has execution enabled, AND /// - Has an invalid payload. pub fn is_invalid(&self) -> bool { matches!(self, ExecutionStatus::Invalid(_)) @@ -294,9 +312,13 @@ impl ProtoArrayForkChoice { self.proto_array.indices.contains_key(block_root) } - pub fn get_block(&self, block_root: &Hash256) -> Option { + fn get_proto_node(&self, block_root: &Hash256) -> Option<&ProtoNode> { let block_index = self.proto_array.indices.get(block_root)?; - let block = self.proto_array.nodes.get(*block_index)?; + self.proto_array.nodes.get(*block_index) + } + + pub fn get_block(&self, block_root: &Hash256) -> Option { + let block = self.get_proto_node(block_root)?; let parent_root = block .parent .and_then(|i| self.proto_array.nodes.get(i)) @@ -325,6 +347,12 @@ impl ProtoArrayForkChoice { } } + /// Returns the `block.execution_status` field, if the block is present. + pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { + let block = self.get_proto_node(block_root)?; + Some(block.execution_status) + } + /// Returns the weight of a given block. pub fn get_weight(&self, block_root: &Hash256) -> Option { let block_index = self.proto_array.indices.get(block_root)?;