diff --git a/Cargo.lock b/Cargo.lock index 5e3b5881eb7..31e775ec200 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3225,7 +3225,6 @@ dependencies = [ "ethereum_ssz_derive", "proto_array", "slog", - "slot_clock", "state_processing", "store", "tokio", diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 20a93e31e8d..e8b0d0a45cf 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4230,7 +4230,8 @@ impl BeaconChain { head_slot: Slot, canonical_head: Hash256, ) -> Option> { - let re_org_threshold = self.config.re_org_threshold?; + let re_org_head_threshold = self.config.re_org_head_threshold?; + let re_org_parent_threshold = self.config.re_org_parent_threshold?; if self.spec.proposer_score_boost.is_none() { warn!( @@ -4287,7 +4288,8 @@ impl BeaconChain { .get_proposer_head( slot, canonical_head, - re_org_threshold, + re_org_head_threshold, + re_org_parent_threshold, &self.config.re_org_disallowed_offsets, self.config.re_org_max_epochs_since_finalization, ) @@ -4349,7 +4351,7 @@ impl BeaconChain { "weak_head" => ?canonical_head, "parent" => ?re_org_parent_block, "head_weight" => proposer_head.head_node.weight, - "threshold_weight" => proposer_head.re_org_weight_threshold + "threshold_weight" => proposer_head.re_org_head_weight_threshold ); Some(pre_state) @@ -4569,9 +4571,14 @@ impl BeaconChain { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_OVERRIDE_FCU_TIMES); // Never override if proposer re-orgs are disabled. - let re_org_threshold = self + let re_org_head_threshold = self .config - .re_org_threshold + .re_org_head_threshold + .ok_or(DoNotReOrg::ReOrgsDisabled)?; + + let re_org_parent_threshold = self + .config + .re_org_parent_threshold .ok_or(DoNotReOrg::ReOrgsDisabled)?; let head_block_root = canonical_forkchoice_params.head_root; @@ -4582,7 +4589,8 @@ impl BeaconChain { .fork_choice_read_lock() .get_preliminary_proposer_head( head_block_root, - re_org_threshold, + re_org_head_threshold, + re_org_parent_threshold, &self.config.re_org_disallowed_offsets, self.config.re_org_max_epochs_since_finalization, ) @@ -4652,18 +4660,20 @@ impl BeaconChain { // If the current slot is already equal to the proposal slot (or we are in the tail end of // the prior slot), then check the actual weight of the head against the re-org threshold. let head_weak = if fork_choice_slot == re_org_block_slot { - info.head_node.weight < info.re_org_weight_threshold + info.head_node.weight < info.re_org_head_weight_threshold } else { true }; if !head_weak { return Err(DoNotReOrg::HeadNotWeak { head_weight: info.head_node.weight, - re_org_weight_threshold: info.re_org_weight_threshold, + re_org_head_weight_threshold: info.re_org_head_weight_threshold, } .into()); } + // TODO(is_parent_strong) do we need an is_parent_strong check here + // Check that the head block arrived late and is vulnerable to a re-org. This check is only // a heuristic compared to the proper weight check in `get_state_for_re_org`, the reason // being that we may have only *just* received the block and not yet processed any diff --git a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs index 4b68121077f..2a42b49b422 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -156,7 +156,6 @@ pub struct BeaconForkChoiceStore, Cold: ItemStore< unrealized_finalized_checkpoint: Checkpoint, proposer_boost_root: Hash256, equivocating_indices: BTreeSet, - block_timeliness: bool, _phantom: PhantomData, } @@ -206,7 +205,6 @@ where unrealized_finalized_checkpoint: finalized_checkpoint, proposer_boost_root: Hash256::zero(), equivocating_indices: BTreeSet::new(), - block_timeliness: true, _phantom: PhantomData, }) } @@ -224,7 +222,6 @@ where unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, proposer_boost_root: self.proposer_boost_root, equivocating_indices: self.equivocating_indices.clone(), - block_timeliness: self.block_timeliness, } } @@ -246,7 +243,6 @@ where unrealized_finalized_checkpoint: persisted.unrealized_finalized_checkpoint, proposer_boost_root: persisted.proposer_boost_root, equivocating_indices: persisted.equivocating_indices, - block_timeliness: persisted.block_timeliness, _phantom: PhantomData, }) } @@ -364,14 +360,6 @@ where fn extend_equivocating_indices(&mut self, indices: impl IntoIterator) { self.equivocating_indices.extend(indices); } - - fn block_timeliness(&self) -> bool { - self.block_timeliness - } - - fn set_block_timeliness(&mut self, is_timely: bool) { - self.block_timeliness = is_timely; - } } pub type PersistedForkChoiceStore = PersistedForkChoiceStoreV17; @@ -399,7 +387,6 @@ pub struct PersistedForkChoiceStore { pub proposer_boost_root: Hash256, #[superstruct(only(V11, V17))] pub equivocating_indices: BTreeSet, - pub block_timeliness: bool, } impl Into for PersistedForkChoiceStoreV11 { @@ -414,7 +401,6 @@ impl Into for PersistedForkChoiceStoreV11 { unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, proposer_boost_root: self.proposer_boost_root, equivocating_indices: self.equivocating_indices, - block_timeliness: self.block_timeliness, } } } @@ -432,7 +418,6 @@ impl Into for PersistedForkChoiceStore { unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, proposer_boost_root: self.proposer_boost_root, equivocating_indices: self.equivocating_indices, - block_timeliness: self.block_timeliness, } } } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index dd4b612f60b..8ca75cf532d 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -172,8 +172,8 @@ where } /// Sets the proposer re-org threshold. - pub fn proposer_re_org_threshold(mut self, threshold: Option) -> Self { - self.chain_config.re_org_threshold = threshold; + pub fn proposer_re_org_head_threshold(mut self, threshold: Option) -> Self { + self.chain_config.re_org_head_threshold = threshold; self } diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index 36481b4dcd0..0583448d77e 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -3,7 +3,8 @@ use serde::{Deserialize, Serialize}; use std::time::Duration; use types::{Checkpoint, Epoch, ProgressiveBalancesMode}; -pub const DEFAULT_RE_ORG_THRESHOLD: ReOrgThreshold = ReOrgThreshold(20); +pub const DEFAULT_RE_ORG_HEAD_THRESHOLD: ReOrgThreshold = ReOrgThreshold(20); +pub const DEFAULT_RE_ORG_PARENT_THRESHOLD: ReOrgThreshold = ReOrgThreshold(160); pub const DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION: Epoch = Epoch::new(2); /// Default to 1/12th of the slot, which is 1 second on mainnet. pub const DEFAULT_RE_ORG_CUTOFF_DENOMINATOR: u32 = 12; @@ -31,8 +32,10 @@ pub struct ChainConfig { pub enable_lock_timeouts: bool, /// The max size of a message that can be sent over the network. pub max_network_size: usize, - /// Maximum percentage of committee weight at which to attempt re-orging the canonical head. - pub re_org_threshold: Option, + /// Maximum percentage of the head committee weight at which to attempt re-orging the canonical head. + pub re_org_head_threshold: Option, + /// Minimum percentage of the parent committee weight at which to attempt re-orging the canonical head. + pub re_org_parent_threshold: Option, /// Maximum number of epochs since finalization for attempting a proposer re-org. pub re_org_max_epochs_since_finalization: Epoch, /// Maximum delay after the start of the slot at which to propose a reorging block. @@ -97,7 +100,8 @@ impl Default for ChainConfig { reconstruct_historic_states: false, enable_lock_timeouts: true, max_network_size: 10 * 1_048_576, // 10M - re_org_threshold: Some(DEFAULT_RE_ORG_THRESHOLD), + re_org_head_threshold: Some(DEFAULT_RE_ORG_HEAD_THRESHOLD), + re_org_parent_threshold: Some(DEFAULT_RE_ORG_PARENT_THRESHOLD), re_org_max_epochs_since_finalization: DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, re_org_cutoff_millis: None, re_org_disallowed_offsets: DisallowedReOrgOffsets::default(), diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index d63d04fcec5..529dc852e98 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -419,7 +419,7 @@ pub async fn proposer_boost_re_org_test( None, Some(Box::new(move |builder| { builder - .proposer_re_org_threshold(Some(ReOrgThreshold(re_org_threshold))) + .proposer_re_org_head_threshold(Some(ReOrgThreshold(re_org_threshold))) .proposer_re_org_max_epochs_since_finalization(Epoch::new( max_epochs_since_finalization, )) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index ba8430aceae..20d1f9e10b5 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -1,6 +1,6 @@ use beacon_chain::chain_config::{ DisallowedReOrgOffsets, ReOrgThreshold, DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR, - DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_RE_ORG_THRESHOLD, + DEFAULT_RE_ORG_HEAD_THRESHOLD, DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, }; use beacon_chain::TrustedSetup; use clap::ArgMatches; @@ -747,12 +747,13 @@ pub fn get_config( } if cli_args.is_present("disable-proposer-reorgs") { - client_config.chain.re_org_threshold = None; + client_config.chain.re_org_head_threshold = None; + client_config.chain.re_org_parent_threshold = None; } else { - client_config.chain.re_org_threshold = Some( + client_config.chain.re_org_head_threshold = Some( clap_utils::parse_optional(cli_args, "proposer-reorg-threshold")? .map(ReOrgThreshold) - .unwrap_or(DEFAULT_RE_ORG_THRESHOLD), + .unwrap_or(DEFAULT_RE_ORG_HEAD_THRESHOLD), ); client_config.chain.re_org_max_epochs_since_finalization = clap_utils::parse_optional(cli_args, "proposer-reorg-epochs-since-finalization")? @@ -760,6 +761,8 @@ pub fn get_config( client_config.chain.re_org_cutoff_millis = clap_utils::parse_optional(cli_args, "proposer-reorg-cutoff")?; + // TODO(is_parent_strong) do we want re_org_parent_threshold settable? + if let Some(disallowed_offsets_str) = clap_utils::parse_optional::(cli_args, "proposer-reorg-disallowed-offsets")? { diff --git a/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml b/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml index b5faed86c37..2df40798c11 100644 --- a/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/sepolia/config.yaml @@ -73,8 +73,6 @@ PROPOSER_SCORE_BOOST: 40 REORG_HEAD_WEIGHT_THRESHOLD: 20 # 160% REORG_PARENT_WEIGHT_THRESHOLD: 160 -# `2` epochs -REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2 # Deposit contract # --------------------------------------------------------------- diff --git a/consensus/fork_choice/Cargo.toml b/consensus/fork_choice/Cargo.toml index 8e735a44632..7a06d7352b7 100644 --- a/consensus/fork_choice/Cargo.toml +++ b/consensus/fork_choice/Cargo.toml @@ -12,7 +12,6 @@ state_processing = { workspace = true } proto_array = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } -slot_clock = { workspace = true } slog = { workspace = true } [dev-dependencies] diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 88574b5e6f3..85fae4493ac 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1,17 +1,15 @@ use crate::{ForkChoiceStore, InvalidationOperation}; use per_epoch_processing::altair::participation_cache::Error as ParticipationCacheError; use proto_array::{ - calculate_committee_fraction, Block as ProtoBlock, DisallowedReOrgOffsets, ExecutionStatus, - ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, + Block as ProtoBlock, DisallowedReOrgOffsets, ExecutionStatus, ProposerHeadError, + ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, }; use slog::{crit, debug, error, warn, Logger}; -use slot_clock::{SlotClock, SystemTimeSlotClock}; use ssz_derive::{Decode, Encode}; use state_processing::per_epoch_processing::altair::ParticipationCache; use state_processing::per_epoch_processing::{ weigh_justification_and_finalization, JustificationAndFinalizationState, }; -use state_processing::per_slot_processing; use state_processing::{ per_block_processing::errors::AttesterSlashingValidationError, per_epoch_processing, }; @@ -245,16 +243,6 @@ fn compute_start_slot_at_epoch(epoch: Epoch) -> Slot { epoch.start_slot(E::slots_per_epoch()) } -/// Return the epoch number at `slot` -/// -/// ## Specification -/// Equivalent to: -/// -/// TODO -fn compute_epoch_at_slot(slot: Slot) -> Epoch { - return Epoch::new(slot.as_u64() / E::slots_per_epoch()); -} - /// Used for queuing attestations from the current slot. Only contains the minimum necessary /// information about the attestation. #[derive(Clone, PartialEq, Encode, Decode)] @@ -544,7 +532,8 @@ where &self, current_slot: Slot, canonical_head: Hash256, - re_org_threshold: ReOrgThreshold, + re_org_head_threshold: ReOrgThreshold, + re_org_parent_threshold: ReOrgThreshold, disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result>> { @@ -576,7 +565,8 @@ where current_slot, canonical_head, self.fc_store.justified_balances(), - re_org_threshold, + re_org_head_threshold, + re_org_parent_threshold, disallowed_offsets, max_epochs_since_finalization, ) @@ -586,7 +576,8 @@ where pub fn get_preliminary_proposer_head( &self, canonical_head: Hash256, - re_org_threshold: ReOrgThreshold, + re_org_head_threshold: ReOrgThreshold, + re_org_parent_threshold: ReOrgThreshold, disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result>> { @@ -596,7 +587,8 @@ where current_slot, canonical_head, self.fc_store.justified_balances(), - re_org_threshold, + re_org_head_threshold, + re_org_parent_threshold, disallowed_offsets, max_epochs_since_finalization, ) @@ -732,15 +724,12 @@ where })); } - // Add block timeliness to the store + // Add proposer score boost if the block is timely. let is_before_attesting_interval = block_delay < Duration::from_secs(spec.seconds_per_slot / INTERVALS_PER_SLOT); - let is_timely = current_slot == block.slot() && is_before_attesting_interval; - self.fc_store.set_block_timeliness(is_timely); - // Add proposer score boost if the block is timely and not conflicting with an existing block let is_first_block = self.fc_store.proposer_boost_root().is_zero(); - if is_timely && is_first_block { + if current_slot == block.slot() && is_before_attesting_interval && is_first_block { self.fc_store.set_proposer_boost_root(block_root); } @@ -1572,150 +1561,6 @@ where queued_attestations: self.queued_attestations().to_vec(), } } - - fn should_override_forkchoice_update( - &self, - head_root: Hash256, - parent_state: &mut BeaconState, - spec: &ChainSpec, - ) -> Result> { - // TODO unwrap - // TODO maybe use proto_aray - let head_block = self.get_block(&head_root).unwrap(); - let parent_root = head_block.parent_root.unwrap(); - - let parent_block = self - .proto_array - .get_block(&parent_root) - .ok_or_else(|| Error::InvalidBlock(InvalidBlock::UnknownParent(parent_root)))?; - - let current_slot = self.fc_store.get_current_slot(); - let proposal_slot = head_block.slot + Slot::new(1); - - // Only re-org the `head_block` block if it arrived later than the attestation deadline. - let head_late = self.is_head_late(); - - // Shuffling stable. - let shuffling_stable = self.is_shuffling_stable(proposal_slot); - - // FFG information of the new `head_block` will be competitive with the current head. - let ffg_competitive = self.is_ffg_competitive(&head_block, &parent_block); - - // Do not re-org if the chain is not finalizing with acceptable frequency. - let finalization_ok = self.is_finalization_ok(proposal_slot, spec); - - // Only suppress the fork choice update if we are confident that we will propose the next block. - // let parent_state_advanced = chain.get_state(parent_block.state_root); - self.process_slots(parent_state, proposal_slot, spec)?; - let proposer_index = parent_state.get_beacon_proposer_index(parent_block.slot, spec)?; - let proposing_reorg_slot = self.latest_message(proposer_index).is_some(); - - // Single slot re-org. - let parent_slot_ok = parent_block.slot.as_u64() + 1 == head_block.slot.as_u64(); - let proposing_on_time = self.is_proposing_on_time(parent_state, spec); - - // Note that this condition is different from `get_proposer_head` - let current_time_ok = - head_block.slot == current_slot || (proposal_slot == current_slot && proposing_on_time); - let single_slot_reorg = parent_slot_ok && current_time_ok; - - // Check the head weight only if the attestations from the head slot have already been applied. - // Implementations may want to do this in different ways, e.g. by advancing - // `store.time` early, or by counting queued attestations during the head block's slot. - let head_weak; - let parent_strong; - - if current_slot > head_block.slot { - head_weak = self.is_head_weak(&head_root, spec); - parent_strong = self.is_parent_strong(&parent_root, spec); - } else { - head_weak = true; - parent_strong = true; - } - let should_override_forkchoice_update = vec![ - head_late, - shuffling_stable, - ffg_competitive, - finalization_ok, - proposing_reorg_slot, - single_slot_reorg, - head_weak, - parent_strong, - ] - .iter() - .all(|&f| f == true); - Ok((should_override_forkchoice_update)) - } - - // TODO - fn is_head_late(&self) -> bool { - false - } - - fn is_head_weak(&self, head_root: &Hash256, spec: &ChainSpec) -> bool { - let reorg_threshold = calculate_committee_fraction::( - self.fc_store.justified_balances(), - spec.proposer_score_boost.unwrap(), - ) - .unwrap(); - let head_weight = self.proto_array.get_weight(head_root).unwrap(); - head_weight < reorg_threshold - } - - fn is_parent_strong(&self, parent_root: &Hash256, spec: &ChainSpec) -> bool { - let parent_threshold = calculate_committee_fraction::( - self.fc_store.justified_balances(), - spec.reorg_parent_weight_threshold.unwrap(), - ) - .unwrap(); - let parent_weight = self.proto_array.get_weight(parent_root).unwrap(); - return parent_weight > parent_threshold; - } - - fn is_shuffling_stable(&self, slot: Slot) -> bool { - return slot % E::slots_per_epoch() != 0; - } - - fn is_ffg_competitive(&self, head_block: &ProtoBlock, parent_block: &ProtoBlock) -> bool { - head_block.unrealized_justified_checkpoint == parent_block.unrealized_justified_checkpoint - } - - fn is_finalization_ok(&self, slot: Slot, spec: &ChainSpec) -> bool { - let epoch_since_finalization = - compute_epoch_at_slot::(slot) - self.fc_store.finalized_checkpoint().epoch; - - // TODO unwrap - epoch_since_finalization <= spec.reorg_max_epochs_since_finalization.unwrap() - } - - fn process_slots( - &self, - state: &mut BeaconState, - slot: Slot, - spec: &ChainSpec, - ) -> Result<(), Error> { - if state.slot() < slot { - return Err(Error::BeaconStateError(BeaconStateError::SlotOutOfBounds)); - } - - while state.slot() < slot { - per_slot_processing(state, state.get_state_root(slot).ok().copied(), spec).unwrap(); - } - - Ok(()) - } - - fn is_proposing_on_time(&self, state: &BeaconState, spec: &ChainSpec) -> bool { - let slot_clock = SystemTimeSlotClock::new( - spec.genesis_slot, - Duration::from_secs(state.genesis_time()), - Duration::from_secs(spec.seconds_per_slot), - ); - - let proposer_reorg_cutoff = spec.seconds_per_slot / INTERVALS_PER_SLOT / 2; - - slot_clock.duration_to_next_slot().unwrap().as_secs() <= proposer_reorg_cutoff - } } /// Process justification and finalization using progressive cache. Also performs a comparative diff --git a/consensus/fork_choice/src/fork_choice_store.rs b/consensus/fork_choice/src/fork_choice_store.rs index 570b2df5ccb..320f10141d9 100644 --- a/consensus/fork_choice/src/fork_choice_store.rs +++ b/consensus/fork_choice/src/fork_choice_store.rs @@ -79,8 +79,4 @@ pub trait ForkChoiceStore: Sized { /// Adds to the set of equivocating indices. fn extend_equivocating_indices(&mut self, indices: impl IntoIterator); - - fn block_timeliness(&self) -> bool; - - fn set_block_timeliness(&mut self, timeliness: bool); } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 1c41b1855b7..150c6e771e2 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -195,8 +195,10 @@ pub struct ProposerHeadInfo { /// Information about the parent of the current head, which should be selected as the parent /// for a new proposal *if* a re-org is decided on. pub parent_node: ProtoNode, - /// The computed fraction of the active committee balance below which we can re-org. - pub re_org_weight_threshold: u64, + /// The computed fraction of the active head committee balance below which we can re-org. + pub re_org_head_weight_threshold: u64, + /// The computed fraction of the active parent committee balance above which we can re-org. + pub re_org_parent_weight_threshold: u64, /// The current slot from fork choice's point of view, may lead the wall-clock slot by upto /// 500ms. pub current_slot: Slot, @@ -259,7 +261,11 @@ pub enum DoNotReOrg { }, HeadNotWeak { head_weight: u64, - re_org_weight_threshold: u64, + re_org_head_weight_threshold: u64, + }, + ParentNotStrong { + parent_weight: u64, + re_org_parent_weight_threshold: u64, }, HeadNotLate, NotProposing, @@ -288,9 +294,21 @@ impl std::fmt::Display for DoNotReOrg { ), Self::HeadNotWeak { head_weight, - re_org_weight_threshold, + re_org_head_weight_threshold, } => { - write!(f, "head not weak ({head_weight}/{re_org_weight_threshold})") + write!( + f, + "head not weak ({head_weight}/{re_org_head_weight_threshold})" + ) + } + Self::ParentNotStrong { + parent_weight, + re_org_parent_weight_threshold, + } => { + write!( + f, + "parent not weak ({parent_weight}/{re_org_parent_weight_threshold})" + ) } Self::HeadNotLate => { write!(f, "head arrived on time") @@ -486,12 +504,14 @@ impl ProtoArrayForkChoice { /// Get the block to propose on during `current_slot`. /// /// This function returns a *definitive* result which should be acted on. + #[allow(clippy::too_many_arguments)] pub fn get_proposer_head( &self, current_slot: Slot, canonical_head: Hash256, justified_balances: &JustifiedBalances, - re_org_threshold: ReOrgThreshold, + re_org_head_threshold: ReOrgThreshold, + re_org_parent_threshold: ReOrgThreshold, disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result> { @@ -499,7 +519,8 @@ impl ProtoArrayForkChoice { current_slot, canonical_head, justified_balances, - re_org_threshold, + re_org_head_threshold, + re_org_parent_threshold, disallowed_offsets, max_epochs_since_finalization, )?; @@ -510,14 +531,26 @@ impl ProtoArrayForkChoice { return Err(DoNotReOrg::HeadDistance.into()); } - // Only re-org if the head's weight is less than the configured committee fraction. + // Only re-org if the head's weight is less than the heads configured committee fraction. let head_weight = info.head_node.weight; - let re_org_weight_threshold = info.re_org_weight_threshold; - let weak_head = head_weight < re_org_weight_threshold; + let re_org_head_weight_threshold = info.re_org_head_weight_threshold; + let weak_head = head_weight < re_org_head_weight_threshold; if !weak_head { return Err(DoNotReOrg::HeadNotWeak { head_weight, - re_org_weight_threshold, + re_org_head_weight_threshold, + } + .into()); + } + + // Only re-org if the parent's weight is greater than the parents configured committee fraction. + let parent_weight = info.parent_node.weight; + let re_org_parent_weight_threshold = info.re_org_parent_weight_threshold; + let parent_strong = parent_weight > re_org_parent_weight_threshold; + if !parent_strong { + return Err(DoNotReOrg::ParentNotStrong { + parent_weight, + re_org_parent_weight_threshold, } .into()); } @@ -529,12 +562,14 @@ impl ProtoArrayForkChoice { /// Get information about the block to propose on during `current_slot`. /// /// This function returns a *partial* result which must be processed further. + #[allow(clippy::too_many_arguments)] pub fn get_proposer_head_info( &self, current_slot: Slot, canonical_head: Hash256, justified_balances: &JustifiedBalances, - re_org_threshold: ReOrgThreshold, + re_org_head_threshold: ReOrgThreshold, + re_org_parent_threshold: ReOrgThreshold, disallowed_offsets: &DisallowedReOrgOffsets, max_epochs_since_finalization: Epoch, ) -> Result> { @@ -595,15 +630,20 @@ impl ProtoArrayForkChoice { return Err(DoNotReOrg::JustificationAndFinalizationNotCompetitive.into()); } - // Compute re-org weight threshold. - let re_org_weight_threshold = - calculate_committee_fraction::(justified_balances, re_org_threshold.0) + // Compute re-org weight thresholds for head and parent. + let re_org_head_weight_threshold = + calculate_committee_fraction::(justified_balances, re_org_head_threshold.0) + .ok_or(Error::ReOrgThresholdOverflow)?; + + let re_org_parent_weight_threshold = + calculate_committee_fraction::(justified_balances, re_org_parent_threshold.0) .ok_or(Error::ReOrgThresholdOverflow)?; Ok(ProposerHeadInfo { head_node, parent_node, - re_org_weight_threshold, + re_org_head_weight_threshold, + re_org_parent_weight_threshold, current_slot, }) } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 5fde98de142..fc04363189e 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -116,7 +116,6 @@ pub struct ChainSpec { pub proposer_score_boost: Option, pub reorg_head_weight_threshold: Option, pub reorg_parent_weight_threshold: Option, - pub reorg_max_epochs_since_finalization: Option, /* * Eth1 @@ -627,7 +626,6 @@ impl ChainSpec { proposer_score_boost: Some(40), reorg_head_weight_threshold: Some(20), reorg_parent_weight_threshold: Some(160), - reorg_max_epochs_since_finalization: Some(2), /* * Eth1 @@ -891,7 +889,6 @@ impl ChainSpec { proposer_score_boost: Some(40), reorg_head_weight_threshold: Some(20), reorg_parent_weight_threshold: Some(160), - reorg_max_epochs_since_finalization: Some(2), /* * Eth1 diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index b61ce5922b3..87e87912b1d 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2,8 +2,8 @@ use beacon_node::ClientConfig as Config; use crate::exec::{CommandLineTestExec, CompletedTest}; use beacon_node::beacon_chain::chain_config::{ - DisallowedReOrgOffsets, DEFAULT_RE_ORG_CUTOFF_DENOMINATOR, - DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_RE_ORG_THRESHOLD, + DisallowedReOrgOffsets, DEFAULT_RE_ORG_CUTOFF_DENOMINATOR, DEFAULT_RE_ORG_HEAD_THRESHOLD, + DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, }; use beacon_processor::BeaconProcessorConfig; use eth1::Eth1Endpoint; @@ -2204,8 +2204,8 @@ fn enable_proposer_re_orgs_default() { .run_with_zero_port() .with_config(|config| { assert_eq!( - config.chain.re_org_threshold, - Some(DEFAULT_RE_ORG_THRESHOLD) + config.chain.re_org_head_threshold, + Some(DEFAULT_RE_ORG_HEAD_THRESHOLD) ); assert_eq!( config.chain.re_org_max_epochs_since_finalization, @@ -2218,20 +2218,22 @@ fn enable_proposer_re_orgs_default() { }); } +// TODO(is_parent_strong) #[test] fn disable_proposer_re_orgs() { CommandLineTest::new() .flag("disable-proposer-reorgs", None) .run_with_zero_port() - .with_config(|config| assert_eq!(config.chain.re_org_threshold, None)); + .with_config(|config| assert_eq!(config.chain.re_org_head_threshold, None)); } +// TODO(is_parent_strong) #[test] -fn proposer_re_org_threshold() { +fn proposer_re_org_head_threshold() { CommandLineTest::new() .flag("proposer-reorg-threshold", Some("90")) .run_with_zero_port() - .with_config(|config| assert_eq!(config.chain.re_org_threshold.unwrap().0, 90)); + .with_config(|config| assert_eq!(config.chain.re_org_head_threshold.unwrap().0, 90)); } #[test] diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 9884a709eb9..9faadf65e93 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -4,7 +4,8 @@ use ::fork_choice::{PayloadVerificationStatus, ProposerHeadError}; use beacon_chain::beacon_proposer_cache::compute_proposer_duties_from_head; use beacon_chain::blob_verification::GossipBlobError; use beacon_chain::chain_config::{ - DisallowedReOrgOffsets, DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_RE_ORG_THRESHOLD, + DisallowedReOrgOffsets, DEFAULT_RE_ORG_HEAD_THRESHOLD, + DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_RE_ORG_PARENT_THRESHOLD, }; use beacon_chain::slot_clock::SlotClock; use beacon_chain::{ @@ -748,7 +749,8 @@ impl Tester { let proposer_head_result = fc.get_proposer_head( slot, canonical_head, - DEFAULT_RE_ORG_THRESHOLD, + DEFAULT_RE_ORG_HEAD_THRESHOLD, + DEFAULT_RE_ORG_PARENT_THRESHOLD, &DisallowedReOrgOffsets::default(), DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, );