From 637c8c72e0f33a3e7e747593f2dacce4d2e41c64 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 13 Apr 2022 03:54:42 +0000 Subject: [PATCH] Disallow attesting to optimistic head (#3140) ## Issue Addressed NA ## Proposed Changes Disallow the production of attestations and retrieval of unaggregated attestations when they reference an optimistic head. Add tests to this end. I also moved `BeaconChain::produce_unaggregated_attestation_for_block` to the `BeaconChainHarness`. It was only being used during tests, so it's nice to stop pretending it's production code. I also needed something that could produce attestations to optimistic blocks in order to simulate scenarios where the justified checkpoint is determined invalid (if no one would attest to an optimistic block, we could never justify it and then flip it to invalid). ## Additional Info - ~~Blocked on #3126~~ --- beacon_node/beacon_chain/src/beacon_chain.rs | 140 +++++++++-------- .../beacon_chain/src/block_verification.rs | 2 +- beacon_node/beacon_chain/src/errors.rs | 8 + .../beacon_chain/src/execution_payload.rs | 4 +- beacon_node/beacon_chain/src/fork_revert.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 78 ++++++++-- .../tests/payload_invalidation.rs | 145 +++++++++++++++--- beacon_node/http_api/src/lib.rs | 6 + consensus/fork_choice/src/fork_choice.rs | 26 +++- consensus/fork_choice/src/lib.rs | 2 +- .../src/fork_choice_test_definition.rs | 8 +- consensus/proto_array/src/proto_array.rs | 8 +- .../src/proto_array_fork_choice.rs | 50 ++++-- 13 files changed, 354 insertions(+), 125 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e4d572ddebf..6c674c0f9c0 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; @@ -1432,8 +1431,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 @@ -1444,10 +1448,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`. @@ -1483,6 +1520,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) @@ -1599,6 +1638,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: * @@ -1659,64 +1714,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. @@ -2680,13 +2677,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)?; @@ -4237,7 +4241,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 63ae4149299..d156b92c54c 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 79f7346ca2e..8d275414176 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 d95a7a671ce..47446e55925 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 3ae3bf8a3eb..c96dc1b36e4 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 6684aeec826..7263bf0513e 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 84317196b11..cce7c344f48 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 558ae3374eb..88e5a1c6c93 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2208,6 +2208,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 21e9d8641a8..e73818b4f85 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -125,11 +125,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 @@ -671,7 +682,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 => { @@ -947,6 +960,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 d4a95994e0d..157306dd5f8 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 f2b51c1fd40..2980c019e82 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 5b0a46e952d..3f7909553b2 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 8f3f5ed798b..88bf7840c28 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)?;