From b87dc81ad665a85c6668c8137112af5eb174183f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 4 Apr 2022 16:04:17 +1000 Subject: [PATCH 01/13] Disallow attn production for optimistic heads --- beacon_node/beacon_chain/src/beacon_chain.rs | 88 +++++++++++-- beacon_node/beacon_chain/src/errors.rs | 3 + .../tests/payload_invalidation.rs | 117 ++++++++++++++++++ consensus/fork_choice/src/fork_choice.rs | 20 +++ .../src/proto_array_fork_choice.rs | 16 ++- 5 files changed, 234 insertions(+), 10 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6599bb69fa6..06f31050ae9 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1431,7 +1431,26 @@ impl BeaconChain { &self, data: &AttestationData, ) -> Option> { - self.naive_aggregation_pool.read().get(data) + let attestation = self.naive_aggregation_pool.read().get(data)?; + + // Only return an aggregate for a block if it is fully verified (i.e. not optimistic). + // + // If the head *is* optimistic, return an error without producing an aggregate. + // + // Since we read the execution status from fork choice, we will not return an aggregate if + // it attests to a finalized block. Aggregates that attest to pre-finalized blocks are not + // useful to the network and should very rarely occur. + let beacon_block_root = attestation.data.beacon_block_root; + if self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + .map_or(true, |execution_status| execution_status.is_not_verified()) + { + None + } else { + Some(attestation) + } } /// Returns an aggregated `Attestation`, if any, that has a matching @@ -1443,9 +1462,29 @@ impl BeaconChain { slot: Slot, attestation_data_root: &Hash256, ) -> Option> { - self.naive_aggregation_pool + let attestation = self + .naive_aggregation_pool .read() - .get_by_slot_and_root(slot, attestation_data_root) + .get_by_slot_and_root(slot, attestation_data_root)?; + + // Only return an aggregate for a block if it is fully verified (i.e. not optimistic). + // + // If the head *is* optimistic, return an error without producing an aggregate. + // + // Since we read the execution status from fork choice, we will not return an aggregate if + // it attests to a finalized block. Aggregates that attest to pre-finalized blocks are not + // useful to the network and should very rarely occur. + let beacon_block_root = attestation.data.beacon_block_root; + if self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + .map_or(true, |execution_status| execution_status.is_not_verified()) + { + None + } else { + Some(attestation) + } } /// Return an aggregated `SyncCommitteeContribution` matching the given `root`. @@ -1481,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) @@ -1597,6 +1638,19 @@ impl BeaconChain { } drop(head_timer); + // Only attest to a block if it is fully verified (i.e. not optimistic). + // + // If the head *is* optimistic, return an error without producing an attestation. + if self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + .ok_or(Error::HeadMissingFromForkChoice(beacon_block_root))? + .is_not_verified() + { + return Err(Error::CannotAttestToOptimisticHead { beacon_block_root }); + } + /* * Phase 2/2: * @@ -1673,6 +1727,19 @@ impl BeaconChain { mut state: Cow>, state_root: Hash256, ) -> Result, Error> { + // Only attest to a block if it is fully verified (i.e. not optimistic). + // + // If the head *is* optimistic, return an error without producing an attestation. + if self + .fork_choice + .read() + .get_block_execution_status(&beacon_block_root) + .ok_or(Error::HeadMissingFromForkChoice(beacon_block_root))? + .is_not_verified() + { + return Err(Error::CannotAttestToOptimisticHead { beacon_block_root }); + } + let epoch = slot.epoch(T::EthSpec::slots_per_epoch()); if state.slot() > slot { @@ -2678,13 +2745,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)?; diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 79f7346ca2e..345bda0da60 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -162,6 +162,9 @@ pub enum BeaconChainError { fork_choice: Hash256, }, InvalidSlot(Slot), + CannotAttestToOptimisticHead { + beacon_block_root: Hash256, + }, } easy_from_to!(SlotProcessingError, BeaconChainError); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index e1c082a880e..940dc2eb2a9 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -11,8 +11,10 @@ use execution_layer::{ use fork_choice::{Error as ForkChoiceError, InvalidationOperation, PayloadVerificationStatus}; use proto_array::{Error as ProtoArrayError, ExecutionStatus}; use slot_clock::SlotClock; +use std::borrow::Cow; use std::time::Duration; use task_executor::ShutdownReason; +use tree_hash::TreeHash; use types::*; const VALIDATOR_COUNT: usize = 32; @@ -810,3 +812,118 @@ fn invalid_parent() { )) )); } + +#[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_not_verified(), + "the head should be optimistic" + ); + + /* + * Define some closures to produce attestations. + */ + + let produce_unaggregated = || rig.harness.chain.produce_unaggregated_attestation(slot, 0); + + let produce_unaggregated_for_block = || { + rig.harness + .chain + .produce_unaggregated_attestation_for_block( + slot, + 0, + root, + Cow::Owned(head.beacon_state.clone()), + head.beacon_state_root(), + ) + }; + + 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 + }; + + 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. + */ + + assert!(matches!( + produce_unaggregated(), + Err(BeaconChainError::CannotAttestToOptimisticHead { + beacon_block_root + }) + if beacon_block_root == root + )); + + assert!(matches!( + produce_unaggregated_for_block(), + Err(BeaconChainError::CannotAttestToOptimisticHead { + beacon_block_root + }) + if beacon_block_root == root + )); + + assert_eq!(get_aggregated(), None); + + assert_eq!(get_aggregated_by_slot_and_root(), None); + + /* + * 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(), + "the head should no longer be optimistic" + ); + + produce_unaggregated().unwrap(); + produce_unaggregated_for_block().unwrap(); + get_aggregated().unwrap(); + get_aggregated_by_slot_and_root().unwrap(); +} diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index d71565fc197..68e68c1fb13 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -127,6 +127,17 @@ pub enum PayloadVerificationStatus { Irrelevant, } +impl PayloadVerificationStatus { + /// Returns `true` if the payload was optimistically imported. + pub fn is_optimistic(&self) -> bool { + match self { + PayloadVerificationStatus::Verified => false, + PayloadVerificationStatus::NotVerified => true, + PayloadVerificationStatus::Irrelevant => false, + } + } +} + /// Calculate how far `slot` lies from the start of its epoch. /// /// ## Specification @@ -944,6 +955,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/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 8f3f5ed798b..96982353356 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}; @@ -294,9 +294,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 +329,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)?; From 60777d291d24cd89329dbedf5d5d75f8ffb65f21 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 5 Apr 2022 16:07:06 +1000 Subject: [PATCH 02/13] Move function into test harness --- beacon_node/beacon_chain/src/beacon_chain.rs | 72 ----------------- beacon_node/beacon_chain/src/test_utils.rs | 77 ++++++++++++++++--- .../tests/payload_invalidation.rs | 21 ----- 3 files changed, 66 insertions(+), 104 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 06f31050ae9..e2724c7ee29 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; @@ -1711,77 +1710,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> { - // Only attest to a block if it is fully verified (i.e. not optimistic). - // - // If the head *is* optimistic, return an error without producing an attestation. - if self - .fork_choice - .read() - .get_block_execution_status(&beacon_block_root) - .ok_or(Error::HeadMissingFromForkChoice(beacon_block_root))? - .is_not_verified() - { - return Err(Error::CannotAttestToOptimisticHead { beacon_block_root }); - } - - 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. diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6684aeec826..cb9e2071e81 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, diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 940dc2eb2a9..b4c13070cea 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -838,18 +838,6 @@ fn attesting_to_optimistic_head() { let produce_unaggregated = || rig.harness.chain.produce_unaggregated_attestation(slot, 0); - let produce_unaggregated_for_block = || { - rig.harness - .chain - .produce_unaggregated_attestation_for_block( - slot, - 0, - root, - Cow::Owned(head.beacon_state.clone()), - head.beacon_state_root(), - ) - }; - let attestation = { let mut attestation = rig .harness @@ -898,14 +886,6 @@ fn attesting_to_optimistic_head() { if beacon_block_root == root )); - assert!(matches!( - produce_unaggregated_for_block(), - Err(BeaconChainError::CannotAttestToOptimisticHead { - beacon_block_root - }) - if beacon_block_root == root - )); - assert_eq!(get_aggregated(), None); assert_eq!(get_aggregated_by_slot_and_root(), None); @@ -923,7 +903,6 @@ fn attesting_to_optimistic_head() { ); produce_unaggregated().unwrap(); - produce_unaggregated_for_block().unwrap(); get_aggregated().unwrap(); get_aggregated_by_slot_and_root().unwrap(); } From 0c714e9a206a69e0bf5b63d28d230e941a549ec9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 6 Apr 2022 09:18:48 +1000 Subject: [PATCH 03/13] Tidy --- .../beacon_chain/tests/payload_invalidation.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index b4c13070cea..763c8b0b391 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -833,11 +833,10 @@ fn attesting_to_optimistic_head() { ); /* - * Define some closures to produce attestations. + * Define an attestation for use during testing. It doesn't have a valid signature, but that's + * not necessary here. */ - let produce_unaggregated = || rig.harness.chain.produce_unaggregated_attestation(slot, 0); - let attestation = { let mut attestation = rig .harness @@ -859,6 +858,12 @@ fn attesting_to_optimistic_head() { attestation }; + /* + * Define some closures to produce attestations. + */ + + let produce_unaggregated = || rig.harness.chain.produce_unaggregated_attestation(slot, 0); + let get_aggregated = || { rig.harness .chain From 46b5f26a6e3fe3ef421a8f273bf4dcc107a9af78 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 6 Apr 2022 16:27:25 +1000 Subject: [PATCH 04/13] Remove unused import --- beacon_node/beacon_chain/tests/payload_invalidation.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 763c8b0b391..51592a8e544 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -11,7 +11,6 @@ use execution_layer::{ use fork_choice::{Error as ForkChoiceError, InvalidationOperation, PayloadVerificationStatus}; use proto_array::{Error as ProtoArrayError, ExecutionStatus}; use slot_clock::SlotClock; -use std::borrow::Cow; use std::time::Duration; use task_executor::ShutdownReason; use tree_hash::TreeHash; From bc8f220f72653b82ea6d86bb186242542e181878 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 7 Apr 2022 15:50:26 +1000 Subject: [PATCH 05/13] Return `Result` from agg fns, rename --- beacon_node/beacon_chain/src/beacon_chain.rs | 86 +++++++++---------- beacon_node/beacon_chain/src/errors.rs | 7 +- .../beacon_chain/src/execution_payload.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 1 + .../tests/payload_invalidation.rs | 67 ++++++++------- beacon_node/http_api/src/lib.rs | 5 ++ consensus/fork_choice/src/fork_choice.rs | 4 +- 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 | 36 ++++++-- 11 files changed, 131 insertions(+), 95 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e2724c7ee29..22df778244f 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1429,26 +1429,12 @@ impl BeaconChain { pub fn get_aggregated_attestation( &self, data: &AttestationData, - ) -> Option> { - let attestation = self.naive_aggregation_pool.read().get(data)?; - - // Only return an aggregate for a block if it is fully verified (i.e. not optimistic). - // - // If the head *is* optimistic, return an error without producing an aggregate. - // - // Since we read the execution status from fork choice, we will not return an aggregate if - // it attests to a finalized block. Aggregates that attest to pre-finalized blocks are not - // useful to the network and should very rarely occur. - let beacon_block_root = attestation.data.beacon_block_root; - if self - .fork_choice - .read() - .get_block_execution_status(&beacon_block_root) - .map_or(true, |execution_status| execution_status.is_not_verified()) - { - None + ) -> Result>, Error> { + if let Some(attestation) = self.naive_aggregation_pool.read().get(data) { + self.filter_optimistic_attestation(attestation) + .map(Option::Some) } else { - Some(attestation) + Ok(None) } } @@ -1460,29 +1446,40 @@ impl BeaconChain { &self, slot: Slot, attestation_data_root: &Hash256, - ) -> Option> { - let attestation = self + ) -> Result>, Error> { + if let Some(attestation) = self .naive_aggregation_pool .read() - .get_by_slot_and_root(slot, attestation_data_root)?; + .get_by_slot_and_root(slot, attestation_data_root) + { + self.filter_optimistic_attestation(attestation) + .map(Option::Some) + } else { + Ok(None) + } + } - // Only return an aggregate for a block if it is fully verified (i.e. not optimistic). - // - // If the head *is* optimistic, return an error without producing an aggregate. - // - // Since we read the execution status from fork choice, we will not return an aggregate if - // it attests to a finalized block. Aggregates that attest to pre-finalized blocks are not - // useful to the network and should very rarely occur. + fn filter_optimistic_attestation( + &self, + attestation: Attestation, + ) -> Result, Error> { let beacon_block_root = attestation.data.beacon_block_root; - if self + match self .fork_choice .read() .get_block_execution_status(&beacon_block_root) - .map_or(true, |execution_status| execution_status.is_not_verified()) { - None - } else { - Some(attestation) + // 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, + }), } } @@ -1637,18 +1634,21 @@ impl BeaconChain { } drop(head_timer); - // Only attest to a block if it is fully verified (i.e. not optimistic). - // - // If the head *is* optimistic, return an error without producing an attestation. - if self + // 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) - .ok_or(Error::HeadMissingFromForkChoice(beacon_block_root))? - .is_not_verified() { - return Err(Error::CannotAttestToOptimisticHead { 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: @@ -4124,7 +4124,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/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 345bda0da60..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,7 +163,11 @@ pub enum BeaconChainError { fork_choice: Hash256, }, InvalidSlot(Slot), - CannotAttestToOptimisticHead { + HeadBlockNotFullyVerified { + beacon_block_root: Hash256, + execution_status: ExecutionStatus, + }, + CannotAttestToFinalizedBlock { beacon_block_root: Hash256, }, } diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index d95a7a671ce..e035d6693d2 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -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/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index cb9e2071e81..7263bf0513e 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -954,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 51592a8e544..334ba89b14b 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -200,8 +200,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!(), } @@ -288,7 +288,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()); @@ -376,9 +376,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()); } } } @@ -425,7 +425,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()) } } } @@ -463,9 +463,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()); } } } @@ -535,7 +535,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()); @@ -586,7 +586,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(); @@ -599,7 +599,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()); @@ -670,13 +670,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] @@ -688,13 +688,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] @@ -827,7 +827,7 @@ fn attesting_to_optimistic_head() { "the head should be the latest imported block" ); assert!( - rig.execution_status(root).is_not_verified(), + rig.execution_status(root).is_optimistic(), "the head should be optimistic" ); @@ -882,17 +882,22 @@ fn attesting_to_optimistic_head() { * Ensure attestation production fails with an optimistic head. */ - assert!(matches!( - produce_unaggregated(), - Err(BeaconChainError::CannotAttestToOptimisticHead { - beacon_block_root - }) - if beacon_block_root == root - )); - - assert_eq!(get_aggregated(), None); + 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_eq!(get_aggregated_by_slot_and_root(), None); + 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. @@ -902,7 +907,7 @@ fn attesting_to_optimistic_head() { rig.validate_manually(root); assert!( - rig.execution_status(root).is_valid(), + rig.execution_status(root).is_valid_and_post_bellatrix(), "the head should no longer be optimistic" ); diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 1b8bb16c5a8..6b88035d2d8 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2138,6 +2138,11 @@ pub fn serve( query.slot, &query.attestation_data_root, ) + .map_err(|_e| { + warp_utils::reject::custom_bad_request( + "unable to fetch aggregate".to_string(), + ) + })? .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 68e68c1fb13..c03db90c84f 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -679,7 +679,9 @@ where } else { match payload_verification_status { PayloadVerificationStatus::Verified => ExecutionStatus::Valid(block_hash), - PayloadVerificationStatus::NotVerified => ExecutionStatus::Unknown(block_hash), + PayloadVerificationStatus::NotVerified => { + 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 => { 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 96982353356..75b36884309 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -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 - pub fn is_valid(&self) -> bool { + /// - 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 + /// - Hash a valid payload + /// + /// 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(_)) From 55e3a68b9488999792033292d31d19a7856e95bf Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 7 Apr 2022 17:07:02 +1000 Subject: [PATCH 06/13] Add comment --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 22df778244f..ea949c37d99 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1459,6 +1459,8 @@ impl BeaconChain { } } + /// Returns `Ok(attestation)` if the supplied `attestation` references a valid + /// `beacon_block_root`. fn filter_optimistic_attestation( &self, attestation: Attestation, From fea76f9f61e37ac6413ba53f2046fed21ca65658 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 12:12:31 +1000 Subject: [PATCH 07/13] Fix typo --- consensus/proto_array/src/proto_array_fork_choice.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 75b36884309..88bf7840c28 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -73,7 +73,7 @@ impl ExecutionStatus { /// Returns `true` if the block: /// /// - Has execution enabled, AND - /// - Hash a valid payload + /// - Has a valid payload /// /// 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. From f9ddb305edb1394c04f90de5f8099a5dde5808b8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 12:17:28 +1000 Subject: [PATCH 08/13] Rename `NotVerified` to `Optimistic` --- beacon_node/beacon_chain/src/block_verification.rs | 2 +- beacon_node/beacon_chain/src/execution_payload.rs | 2 +- beacon_node/beacon_chain/src/fork_revert.rs | 2 +- beacon_node/beacon_chain/tests/payload_invalidation.rs | 2 +- consensus/fork_choice/src/fork_choice.rs | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 63ae4149299..adabf6219b5 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 == PayloadVerificationStatus::Optimistic { let current_slot = chain .slot_clock .now() diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index e035d6693d2..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, .. 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/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 334ba89b14b..ef0f97c40a0 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -798,7 +798,7 @@ fn invalid_parent() { block_root, Duration::from_secs(0), &state, - PayloadVerificationStatus::NotVerified, + PayloadVerificationStatus::Optimistic, &rig.harness.chain.spec ), Err(ForkChoiceError::ProtoArrayError(message)) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index c03db90c84f..c876a69be45 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -122,7 +122,7 @@ 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, } @@ -132,7 +132,7 @@ impl PayloadVerificationStatus { pub fn is_optimistic(&self) -> bool { match self { PayloadVerificationStatus::Verified => false, - PayloadVerificationStatus::NotVerified => true, + PayloadVerificationStatus::Optimistic => true, PayloadVerificationStatus::Irrelevant => false, } } @@ -679,7 +679,7 @@ where } else { match payload_verification_status { PayloadVerificationStatus::Verified => ExecutionStatus::Valid(block_hash), - PayloadVerificationStatus::NotVerified => { + PayloadVerificationStatus::Optimistic => { ExecutionStatus::Optimistic(block_hash) } // It would be a logic error to declare a block irrelevant if it has an From 4c7fedf11dacbb4e728174eca0002fcf829a133f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 12:25:41 +1000 Subject: [PATCH 09/13] Ensure error is not swallowed --- beacon_node/http_api/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 6b88035d2d8..7b58ce68247 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2138,10 +2138,11 @@ pub fn serve( query.slot, &query.attestation_data_root, ) - .map_err(|_e| { - warp_utils::reject::custom_bad_request( - "unable to fetch aggregate".to_string(), - ) + .map_err(|e| { + warp_utils::reject::custom_bad_request(format!( + "unable to fetch aggregate: {:?}", + e + )) })? .map(api_types::GenericResponse::from) .ok_or_else(|| { From f592aa38fdf29f549b8eb79b78a48fcbeeb2154e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 12:26:37 +1000 Subject: [PATCH 10/13] Use `is_optimistic` fn --- beacon_node/beacon_chain/src/block_verification.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index adabf6219b5..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::Optimistic { + if payload_verification_status.is_optimistic() { let current_slot = chain .slot_clock .now() From e11187c08ff226d5055404bf82c849e999fa11c4 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 14:45:19 +1000 Subject: [PATCH 11/13] Remove HeadSafetyStatus --- beacon_node/beacon_chain/src/beacon_chain.rs | 34 ++------------------ beacon_node/beacon_chain/src/lib.rs | 5 +-- beacon_node/client/src/notifier.rs | 13 ++++---- 3 files changed, 12 insertions(+), 40 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9b572550a91..8ea7aeaf4ef 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -230,25 +230,6 @@ pub trait BeaconChainTypes: Send + Sync + 'static { type EthSpec: types::EthSpec; } -/// Indicates the EL payload verification status of the head beacon block. -#[derive(Debug, PartialEq)] -pub enum HeadSafetyStatus { - /// The head block has either been verified by an EL or is does not require EL verification - /// (e.g., it is pre-merge or pre-terminal-block). - /// - /// If the block is post-terminal-block, `Some(execution_payload.block_hash)` is included with - /// the variant. - Safe(Option), - /// The head block execution payload has not yet been verified by an EL. - /// - /// The `execution_payload.block_hash` of the head block is returned. - Unsafe(ExecutionBlockHash), - /// The head block execution payload was deemed to be invalid by an EL. - /// - /// The `execution_payload.block_hash` of the head block is returned. - Invalid(ExecutionBlockHash), -} - pub type BeaconForkChoice = ForkChoice< BeaconForkChoiceStore< ::EthSpec, @@ -4163,24 +4144,15 @@ impl BeaconChain { } } - /// Returns the status of the current head block, regarding the validity of the execution - /// payload. - pub fn head_safety_status(&self) -> Result { + /// Returns the execution status of the head block. + pub fn head_execution_status(&self) -> Result { let head = self.head_info()?; let head_block = self .fork_choice .read() .get_block(&head.block_root) .ok_or(BeaconChainError::HeadMissingFromForkChoice(head.block_root))?; - - 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::Optimistic(block_hash) => HeadSafetyStatus::Unsafe(block_hash), - ExecutionStatus::Irrelevant(_) => HeadSafetyStatus::Safe(None), - }; - - Ok(status) + Ok(head_block.execution_status) } /// This function takes a configured weak subjectivity `Checkpoint` and the latest finalized `Checkpoint`. diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 65908547fff..c57cc37966e 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -41,8 +41,8 @@ mod validator_pubkey_cache; pub use self::beacon_chain::{ AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult, - ForkChoiceError, HeadInfo, HeadSafetyStatus, ProduceBlockVerification, StateSkipConfig, - WhenSlotSkipped, INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY, + ForkChoiceError, HeadInfo, ProduceBlockVerification, StateSkipConfig, WhenSlotSkipped, + INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY, }; pub use self::beacon_snapshot::BeaconSnapshot; pub use self::chain_config::ChainConfig; @@ -53,6 +53,7 @@ pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceSto pub use block_verification::{BlockError, ExecutionPayloadError, GossipVerifiedBlock}; pub use eth1_chain::{Eth1Chain, Eth1ChainBackend}; pub use events::ServerSentEventHandler; +pub use fork_choice::ExecutionStatus; pub use metrics::scrape_for_metrics; pub use parking_lot; pub use slot_clock; diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index 22c3bfcb3a8..96eb7c8bcb2 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -1,5 +1,5 @@ use crate::metrics; -use beacon_chain::{BeaconChain, BeaconChainTypes, HeadSafetyStatus}; +use beacon_chain::{BeaconChain, BeaconChainTypes, ExecutionStatus}; use lighthouse_network::{types::SyncState, NetworkGlobals}; use parking_lot::Mutex; use slog::{crit, debug, error, info, warn, Logger}; @@ -264,11 +264,9 @@ pub fn spawn_notifier( head_root.to_string() }; - let block_hash = match beacon_chain.head_safety_status() { - Ok(HeadSafetyStatus::Safe(hash_opt)) => hash_opt - .map(|hash| format!("{} (verified)", hash)) - .unwrap_or_else(|| "n/a".to_string()), - Ok(HeadSafetyStatus::Unsafe(block_hash)) => { + let block_hash = match beacon_chain.head_execution_status() { + Ok(ExecutionStatus::Valid(block_hash)) => format!("{} (verified)", block_hash), + Ok(ExecutionStatus::Optimistic(block_hash)) => { warn!( log, "Head execution payload is unverified"; @@ -276,7 +274,7 @@ pub fn spawn_notifier( ); format!("{} (unverified)", block_hash) } - Ok(HeadSafetyStatus::Invalid(block_hash)) => { + Ok(ExecutionStatus::Invalid(block_hash)) => { crit!( log, "Head execution payload is invalid"; @@ -285,6 +283,7 @@ pub fn spawn_notifier( ); format!("{} (invalid)", block_hash) } + Ok(ExecutionStatus::Irrelevant(_)) => "n/a".to_string(), Err(e) => { error!( log, From a26342ed19cbf974f863c33d17b6c2ea5ce13b28 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 14:59:19 +1000 Subject: [PATCH 12/13] Update HTTP API --- beacon_node/http_api/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7b58ce68247..55fbcf53ada 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -23,7 +23,7 @@ use beacon_chain::{ observed_operations::ObservationOutcome, validator_monitor::{get_block_delay_ms, timestamp_now}, AttestationError as AttnError, BeaconChain, BeaconChainError, BeaconChainTypes, - HeadSafetyStatus, ProduceBlockVerification, WhenSlotSkipped, + ExecutionStatus, ProduceBlockVerification, WhenSlotSkipped, }; use block_id::BlockId; use eth2::types::{self as api_types, EndpointVersion, ValidatorId}; @@ -394,24 +394,24 @@ pub fn serve( // Create a `warp` filter that rejects requests unless the head has been verified by the // execution layer. - let only_with_safe_head = warp::any() + let not_while_optimistic_filter = warp::any() .and(chain_filter.clone()) .and_then(move |chain: Arc>| async move { - let status = chain.head_safety_status().map_err(|e| { + let status = chain.head_execution_status().map_err(|e| { warp_utils::reject::custom_server_error(format!( - "failed to read head safety status: {:?}", + "failed to read head execution status: {:?}", e )) })?; match status { - HeadSafetyStatus::Safe(_) => Ok(()), - HeadSafetyStatus::Unsafe(hash) => { + ExecutionStatus::Valid(_) | ExecutionStatus::Irrelevant(_) => Ok(()), + ExecutionStatus::Optimistic(hash) => { Err(warp_utils::reject::custom_server_error(format!( "optimistic head hash {:?} has not been verified by the execution layer", hash ))) } - HeadSafetyStatus::Invalid(hash) => { + ExecutionStatus::Invalid(hash) => { Err(warp_utils::reject::custom_server_error(format!( "the head block has an invalid payload {:?}, this may be unrecoverable", hash @@ -2095,7 +2095,7 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(only_with_safe_head.clone()) + .and(not_while_optimistic_filter.clone()) .and(chain_filter.clone()) .and_then( |query: api_types::ValidatorAttestationDataQuery, chain: Arc>| { @@ -2128,7 +2128,7 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(only_with_safe_head.clone()) + .and(not_while_optimistic_filter.clone()) .and(chain_filter.clone()) .and_then( |query: api_types::ValidatorAggregateAttestationQuery, chain: Arc>| { @@ -2205,7 +2205,7 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(only_with_safe_head) + .and(not_while_optimistic_filter) .and(chain_filter.clone()) .and_then( |sync_committee_data: SyncContributionData, chain: Arc>| { From 7100234fbf61aa59fc5c68cd5781cfb8ab8b7ef2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 8 Apr 2022 15:01:05 +1000 Subject: [PATCH 13/13] Remove filter from HTTP API --- beacon_node/http_api/src/lib.rs | 34 +-------------------------------- 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 55fbcf53ada..7fa81ddb641 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -23,7 +23,7 @@ use beacon_chain::{ observed_operations::ObservationOutcome, validator_monitor::{get_block_delay_ms, timestamp_now}, AttestationError as AttnError, BeaconChain, BeaconChainError, BeaconChainTypes, - ExecutionStatus, ProduceBlockVerification, WhenSlotSkipped, + ProduceBlockVerification, WhenSlotSkipped, }; use block_id::BlockId; use eth2::types::{self as api_types, EndpointVersion, ValidatorId}; @@ -392,35 +392,6 @@ pub fn serve( ) .untuple_one(); - // Create a `warp` filter that rejects requests unless the head has been verified by the - // execution layer. - let not_while_optimistic_filter = warp::any() - .and(chain_filter.clone()) - .and_then(move |chain: Arc>| async move { - let status = chain.head_execution_status().map_err(|e| { - warp_utils::reject::custom_server_error(format!( - "failed to read head execution status: {:?}", - e - )) - })?; - match status { - ExecutionStatus::Valid(_) | ExecutionStatus::Irrelevant(_) => Ok(()), - ExecutionStatus::Optimistic(hash) => { - Err(warp_utils::reject::custom_server_error(format!( - "optimistic head hash {:?} has not been verified by the execution layer", - hash - ))) - } - ExecutionStatus::Invalid(hash) => { - Err(warp_utils::reject::custom_server_error(format!( - "the head block has an invalid payload {:?}, this may be unrecoverable", - hash - ))) - } - } - }) - .untuple_one(); - // Create a `warp` filter that provides access to the logger. let inner_ctx = ctx.clone(); let log_filter = warp::any().map(move || inner_ctx.log.clone()); @@ -2095,7 +2066,6 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(not_while_optimistic_filter.clone()) .and(chain_filter.clone()) .and_then( |query: api_types::ValidatorAttestationDataQuery, chain: Arc>| { @@ -2128,7 +2098,6 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(not_while_optimistic_filter.clone()) .and(chain_filter.clone()) .and_then( |query: api_types::ValidatorAggregateAttestationQuery, chain: Arc>| { @@ -2205,7 +2174,6 @@ pub fn serve( .and(warp::path::end()) .and(warp::query::()) .and(not_while_syncing_filter.clone()) - .and(not_while_optimistic_filter) .and(chain_filter.clone()) .and_then( |sync_committee_data: SyncContributionData, chain: Arc>| {