From 96de05158a763929ef4f00357e484270730a8401 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 4 Mar 2024 00:35:59 +0200 Subject: [PATCH 1/9] initial fork choice additions --- .../src/beacon_fork_choice_store.rs | 15 +++ .../gnosis/config.yaml | 6 ++ .../holesky/config.yaml | 6 ++ .../prater/config.yaml | 6 ++ .../sepolia/config.yaml | 6 ++ consensus/fork_choice/src/fork_choice.rs | 93 ++++++++++++++++++- .../fork_choice/src/fork_choice_store.rs | 4 + .../src/proto_array_fork_choice.rs | 2 +- consensus/types/src/chain_spec.rs | 9 ++ 9 files changed, 143 insertions(+), 4 deletions(-) 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 2a42b49b422..4b68121077f 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -156,6 +156,7 @@ pub struct BeaconForkChoiceStore, Cold: ItemStore< unrealized_finalized_checkpoint: Checkpoint, proposer_boost_root: Hash256, equivocating_indices: BTreeSet, + block_timeliness: bool, _phantom: PhantomData, } @@ -205,6 +206,7 @@ where unrealized_finalized_checkpoint: finalized_checkpoint, proposer_boost_root: Hash256::zero(), equivocating_indices: BTreeSet::new(), + block_timeliness: true, _phantom: PhantomData, }) } @@ -222,6 +224,7 @@ 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, } } @@ -243,6 +246,7 @@ 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, }) } @@ -360,6 +364,14 @@ 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; @@ -387,6 +399,7 @@ pub struct PersistedForkChoiceStore { pub proposer_boost_root: Hash256, #[superstruct(only(V11, V17))] pub equivocating_indices: BTreeSet, + pub block_timeliness: bool, } impl Into for PersistedForkChoiceStoreV11 { @@ -401,6 +414,7 @@ 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, } } } @@ -418,6 +432,7 @@ 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/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml b/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml index 2311d6db0f9..d8963651956 100644 --- a/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/gnosis/config.yaml @@ -84,6 +84,12 @@ CHURN_LIMIT_QUOTIENT: 4096 # --------------------------------------------------------------- # 40% PROPOSER_SCORE_BOOST: 40 +# 20% +REORG_HEAD_WEIGHT_THRESHOLD: 20 +# 160% +REORG_PARENT_WEIGHT_THRESHOLD: 160 +# `2` epochs +REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2 # Deposit contract # --------------------------------------------------------------- diff --git a/common/eth2_network_config/built_in_network_configs/holesky/config.yaml b/common/eth2_network_config/built_in_network_configs/holesky/config.yaml index 1104791ed5b..310135207ed 100644 --- a/common/eth2_network_config/built_in_network_configs/holesky/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/holesky/config.yaml @@ -70,6 +70,12 @@ MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 8 # --------------------------------------------------------------- # 40% PROPOSER_SCORE_BOOST: 40 +# 20% +REORG_HEAD_WEIGHT_THRESHOLD: 20 +# 160% +REORG_PARENT_WEIGHT_THRESHOLD: 160 +# `2` epochs +REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2 # Deposit contract # --------------------------------------------------------------- diff --git a/common/eth2_network_config/built_in_network_configs/prater/config.yaml b/common/eth2_network_config/built_in_network_configs/prater/config.yaml index ac94b638666..f474b172c51 100644 --- a/common/eth2_network_config/built_in_network_configs/prater/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/prater/config.yaml @@ -78,6 +78,12 @@ MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 8 # --------------------------------------------------------------- # 40% PROPOSER_SCORE_BOOST: 40 +# 20% +REORG_HEAD_WEIGHT_THRESHOLD: 20 +# 160% +REORG_PARENT_WEIGHT_THRESHOLD: 160 +# `2` epochs +REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2 # Deposit contract # --------------------------------------------------------------- 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 89b51ba7686..b5faed86c37 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 @@ -69,6 +69,12 @@ MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 8 # --------------------------------------------------------------- # 40% PROPOSER_SCORE_BOOST: 40 +# 20% +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/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 865a5affbb9..03704fa85fe 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -10,6 +10,7 @@ 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, }; @@ -243,6 +244,16 @@ 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)] @@ -720,11 +731,15 @@ where })); } - // Add proposer score boost if the block is timely. + // Add block timeliness to the store let is_before_attesting_interval = - block_delay < Duration::from_secs(spec.seconds_per_slot / INTERVALS_PER_SLOT); + 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 current_slot == block.slot() && is_before_attesting_interval && is_first_block { + if is_timely && is_first_block { self.fc_store.set_proposer_boost_root(block_root); } @@ -1556,6 +1571,78 @@ where queued_attestations: self.queued_attestations().to_vec(), } } + + fn should_override_forkchoice_update(&self, head_root: Hash256, parent_state: &BeaconState, spec: &ChainSpec) -> Result<(), Error> { + // 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); + let proposer_index = parent_state.get_beacon_proposer_index(parent_block.slot, spec); + + // TODO + // process_slots(parent_state_advanced, proposal_slot) + + Ok(()) + + } + + fn is_head_late(&self) { + todo!() + } + + 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(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 process_slot(state: &BeaconState) { + + } } /// 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 320f10141d9..570b2df5ccb 100644 --- a/consensus/fork_choice/src/fork_choice_store.rs +++ b/consensus/fork_choice/src/fork_choice_store.rs @@ -79,4 +79,8 @@ 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..f249d0434ac 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -774,7 +774,7 @@ 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) + Some(block.execution_statugs) } /// Returns the weight of a given block. diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 88f989d2a5a..5fde98de142 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -114,6 +114,9 @@ pub struct ChainSpec { */ pub safe_slots_to_update_justified: u64, 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 @@ -622,6 +625,9 @@ impl ChainSpec { */ safe_slots_to_update_justified: 8, 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 @@ -883,6 +889,9 @@ impl ChainSpec { */ safe_slots_to_update_justified: 8, 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 From eafb7a6f63e4549b096895d92c98b066e1417d49 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 4 Mar 2024 22:40:22 +0200 Subject: [PATCH 2/9] add helper fns --- Cargo.lock | 1 + consensus/fork_choice/Cargo.toml | 1 + consensus/fork_choice/src/fork_choice.rs | 113 ++++++++++++++---- .../src/proto_array_fork_choice.rs | 2 +- 4 files changed, 96 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 31e775ec200..5e3b5881eb7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3225,6 +3225,7 @@ dependencies = [ "ethereum_ssz_derive", "proto_array", "slog", + "slot_clock", "state_processing", "store", "tokio", diff --git a/consensus/fork_choice/Cargo.toml b/consensus/fork_choice/Cargo.toml index 7a06d7352b7..8e735a44632 100644 --- a/consensus/fork_choice/Cargo.toml +++ b/consensus/fork_choice/Cargo.toml @@ -12,6 +12,7 @@ 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 03704fa85fe..88574b5e6f3 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1,10 +1,11 @@ use crate::{ForkChoiceStore, InvalidationOperation}; use per_epoch_processing::altair::participation_cache::Error as ParticipationCacheError; use proto_array::{ - Block as ProtoBlock, DisallowedReOrgOffsets, ExecutionStatus, ProposerHeadError, - ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, + calculate_committee_fraction, 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::{ @@ -733,10 +734,10 @@ where // Add block timeliness to the store let is_before_attesting_interval = - block_delay < Duration::from_secs(spec.seconds_per_slot / INTERVALS_PER_SLOT); + 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 { @@ -1572,7 +1573,12 @@ where } } - fn should_override_forkchoice_update(&self, head_root: Hash256, parent_state: &BeaconState, spec: &ChainSpec) -> Result<(), Error> { + 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(); @@ -1594,23 +1600,76 @@ where // 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); - let proposer_index = parent_state.get_beacon_proposer_index(parent_block.slot, spec); - - // TODO - // process_slots(parent_state_advanced, proposal_slot) - - Ok(()) - + 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_head_late(&self) { - todo!() + 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 { @@ -1622,15 +1681,21 @@ where } 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; - + 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(state: &mut BeaconState, slot: Slot, spec: &ChainSpec) -> Result<(), Error> { + fn process_slots( + &self, + state: &mut BeaconState, + slot: Slot, + spec: &ChainSpec, + ) -> Result<(), Error> { if state.slot() < slot { - return Err(Error::BeaconStateError(BeaconStateError::SlotOutOfBounds)) + return Err(Error::BeaconStateError(BeaconStateError::SlotOutOfBounds)); } while state.slot() < slot { @@ -1640,8 +1705,16 @@ where Ok(()) } - fn process_slot(state: &BeaconState) { + 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 } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index f249d0434ac..1c41b1855b7 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -774,7 +774,7 @@ 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_statugs) + Some(block.execution_status) } /// Returns the weight of a given block. From a27063e9b65f74e51dc001715919f721ec339fde Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 12 Mar 2024 07:32:25 +0200 Subject: [PATCH 3/9] add is_parent_strong --- Cargo.lock | 1 - beacon_node/beacon_chain/src/beacon_chain.rs | 26 ++- .../src/beacon_fork_choice_store.rs | 15 -- beacon_node/beacon_chain/src/builder.rs | 4 +- beacon_node/beacon_chain/src/chain_config.rs | 12 +- .../http_api/tests/interactive_tests.rs | 2 +- beacon_node/src/config.rs | 11 +- .../sepolia/config.yaml | 2 - consensus/fork_choice/Cargo.toml | 1 - consensus/fork_choice/src/fork_choice.rs | 179 ++---------------- .../fork_choice/src/fork_choice_store.rs | 4 - .../src/proto_array_fork_choice.rs | 72 +++++-- consensus/types/src/chain_spec.rs | 3 - lighthouse/tests/beacon_node.rs | 16 +- testing/ef_tests/src/cases/fork_choice.rs | 6 +- 15 files changed, 117 insertions(+), 237 deletions(-) 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, ); From c2a65afdf793a256883cab11f4274a54fa8aeca2 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sun, 17 Mar 2024 12:23:24 +0200 Subject: [PATCH 4/9] disabling proposer reorg should set parent_threshold to u64 max --- beacon_node/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 20d1f9e10b5..90710799621 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -748,7 +748,7 @@ pub fn get_config( if cli_args.is_present("disable-proposer-reorgs") { client_config.chain.re_org_head_threshold = None; - client_config.chain.re_org_parent_threshold = None; + client_config.chain.re_org_parent_threshold = Some(ReOrgThreshold(u64::MAX)); } else { client_config.chain.re_org_head_threshold = Some( clap_utils::parse_optional(cli_args, "proposer-reorg-threshold")? From 2e48a59a16f7aef0976d617ec13832e2ca0afa0a Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 18 Mar 2024 12:55:34 +0200 Subject: [PATCH 5/9] add new flag, is_parent_strong check in override fcu params --- beacon_node/beacon_chain/src/beacon_chain.rs | 21 ++++++++++++++------ beacon_node/src/cli.rs | 10 +++++++++- beacon_node/src/config.rs | 7 +++++++ lighthouse/tests/beacon_node.rs | 17 ++++++++++++++-- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c4a0bc902cc..02ec100c08f 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4677,11 +4677,15 @@ 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_head_weight_threshold + // the prior slot), then check the actual weight of the head against the head re-org threshold + // and the actual weight of the parent against the parent re-org threshold. + let (head_weak, parent_strong) = if fork_choice_slot == re_org_block_slot { + ( + info.head_node.weight < info.re_org_head_weight_threshold, + info.parent_node.weight > info.re_org_parent_weight_threshold, + ) } else { - true + (true, true) }; if !head_weak { return Err(DoNotReOrg::HeadNotWeak { @@ -4690,8 +4694,13 @@ impl BeaconChain { } .into()); } - - // TODO(is_parent_strong) do we need an is_parent_strong check here + if !parent_strong { + return Err(DoNotReOrg::ParentNotStrong { + parent_weight: info.parent_node.weight, + re_org_parent_weight_threshold: info.re_org_parent_weight_threshold, + } + .into()); + } // 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 diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 1a8e0194f6e..e3f08c76e25 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1041,10 +1041,18 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("proposer-reorg-threshold") .long("proposer-reorg-threshold") .value_name("PERCENT") - .help("Percentage of vote weight below which to attempt a proposer reorg. \ + .help("Percentage of head vote weight below which to attempt a proposer reorg. \ Default: 20%") .conflicts_with("disable-proposer-reorgs") ) + .arg( + Arg::with_name("proposer-reorg-parent-threshold") + .long("proposer-reorg-parent-threshold") + .value_name("PERCENT") + .help("Percentage of parent vote weight above which to attempt a proposer reorg. \ + Default: 160%") + .conflicts_with("disable-proposer-reorgs") + ) .arg( Arg::with_name("proposer-reorg-epochs-since-finalization") .long("proposer-reorg-epochs-since-finalization") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 90710799621..d11f9f6a939 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -1,6 +1,7 @@ use beacon_chain::chain_config::{ DisallowedReOrgOffsets, ReOrgThreshold, DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR, DEFAULT_RE_ORG_HEAD_THRESHOLD, DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, + DEFAULT_RE_ORG_PARENT_THRESHOLD, }; use beacon_chain::TrustedSetup; use clap::ArgMatches; @@ -761,6 +762,12 @@ pub fn get_config( client_config.chain.re_org_cutoff_millis = clap_utils::parse_optional(cli_args, "proposer-reorg-cutoff")?; + client_config.chain.re_org_parent_threshold = Some( + clap_utils::parse_optional(cli_args, "proposer-reorg-parent-threshold")? + .map(ReOrgThreshold) + .unwrap_or(DEFAULT_RE_ORG_PARENT_THRESHOLD), + ); + // TODO(is_parent_strong) do we want re_org_parent_threshold settable? if let Some(disallowed_offsets_str) = diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 6892599f307..c8585e61812 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2223,10 +2223,23 @@ 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_head_threshold, None)); + .with_config(|config| { + assert_eq!(config.chain.re_org_head_threshold, None); + assert_eq!( + config.chain.re_org_parent_threshold.unwrap().0, + u64::MAX + ) + }); +} + +#[test] +fn proposer_re_org_parent_threshold() { + CommandLineTest::new() + .flag("proposer-reorg-parent-threshold", Some("90")) + .run_with_zero_port() + .with_config(|config| assert_eq!(config.chain.re_org_parent_threshold.unwrap().0, 90)); } -// TODO(is_parent_strong) #[test] fn proposer_re_org_head_threshold() { CommandLineTest::new() From 837d0584bce82f834e7bb48315cdbd06608999b1 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 27 Oct 2023 11:09:36 +1100 Subject: [PATCH 6/9] cherry-pick changes --- Cargo.lock | 1 + testing/ef_tests/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 306bdaa7237..1623bbd9ad4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2293,6 +2293,7 @@ dependencies = [ "hex", "kzg", "logging", + "proto_array", "rayon", "serde", "serde_json", diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index f3d00fa035c..96976305fbe 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -27,6 +27,7 @@ eth2_network_config = { workspace = true } ethereum_serde_utils = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } +proto_array = { workspace = true } tree_hash = { workspace = true } tree_hash_derive = { workspace = true } cached_tree_hash = { workspace = true } From d829f49610dbebc006e72b2d8444c003b9781158 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 29 Mar 2024 13:43:09 +0300 Subject: [PATCH 7/9] cleanup --- Cargo.lock | 1 - beacon_node/src/config.rs | 2 -- consensus/proto_array/src/proto_array_fork_choice.rs | 2 +- testing/ef_tests/Cargo.toml | 1 - 4 files changed, 1 insertion(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c712f18d96..d798e9b8f2a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2132,7 +2132,6 @@ dependencies = [ "hex", "kzg", "logging", - "proto_array", "rayon", "serde", "serde_json", diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 6c0b5768bde..c7126db10ce 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -770,8 +770,6 @@ pub fn get_config( .unwrap_or(DEFAULT_RE_ORG_PARENT_THRESHOLD), ); - // 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/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 150c6e771e2..1d7774c355a 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -307,7 +307,7 @@ impl std::fmt::Display for DoNotReOrg { } => { write!( f, - "parent not weak ({parent_weight}/{re_org_parent_weight_threshold})" + "parent not strong ({parent_weight}/{re_org_parent_weight_threshold})" ) } Self::HeadNotLate => { diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index 96976305fbe..f3d00fa035c 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -27,7 +27,6 @@ eth2_network_config = { workspace = true } ethereum_serde_utils = { workspace = true } ethereum_ssz = { workspace = true } ethereum_ssz_derive = { workspace = true } -proto_array = { workspace = true } tree_hash = { workspace = true } tree_hash_derive = { workspace = true } cached_tree_hash = { workspace = true } From ea69ec9a5f94a741ccce93a99998e391e0b5395c Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 29 Mar 2024 13:51:32 +0300 Subject: [PATCH 8/9] fmt --- lighthouse/tests/beacon_node.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index c8585e61812..f54d6285665 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2225,10 +2225,7 @@ fn disable_proposer_re_orgs() { .run_with_zero_port() .with_config(|config| { assert_eq!(config.chain.re_org_head_threshold, None); - assert_eq!( - config.chain.re_org_parent_threshold.unwrap().0, - u64::MAX - ) + assert_eq!(config.chain.re_org_parent_threshold.unwrap().0, u64::MAX) }); } From 2a83bdbafaf8561997d8f3cbc21943bfd233a177 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 4 Apr 2024 18:41:53 +1100 Subject: [PATCH 9/9] Minor review tweaks --- beacon_node/src/config.rs | 2 +- book/src/help_bn.md | 5 ++++- lighthouse/tests/beacon_node.rs | 3 +-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index c7126db10ce..effaf294a45 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -751,7 +751,7 @@ pub fn get_config( if cli_args.is_present("disable-proposer-reorgs") { client_config.chain.re_org_head_threshold = None; - client_config.chain.re_org_parent_threshold = Some(ReOrgThreshold(u64::MAX)); + client_config.chain.re_org_parent_threshold = None; } else { client_config.chain.re_org_head_threshold = Some( clap_utils::parse_optional(cli_args, "proposer-reorg-threshold")? diff --git a/book/src/help_bn.md b/book/src/help_bn.md index c0505988ce1..a8265380ee8 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -385,8 +385,11 @@ OPTIONS: --proposer-reorg-epochs-since-finalization Maximum number of epochs since finalization at which proposer reorgs are allowed. Default: 2 + --proposer-reorg-parent-threshold + Percentage of parent vote weight above which to attempt a proposer reorg. Default: 160% + --proposer-reorg-threshold - Percentage of vote weight below which to attempt a proposer reorg. Default: 20% + Percentage of head vote weight below which to attempt a proposer reorg. Default: 20% --prune-blobs Prune blobs from Lighthouse's database when they are older than the data data availability boundary relative diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index f54d6285665..aea8a2fdf57 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -2217,7 +2217,6 @@ fn enable_proposer_re_orgs_default() { }); } -// TODO(is_parent_strong) #[test] fn disable_proposer_re_orgs() { CommandLineTest::new() @@ -2225,7 +2224,7 @@ fn disable_proposer_re_orgs() { .run_with_zero_port() .with_config(|config| { assert_eq!(config.chain.re_org_head_threshold, None); - assert_eq!(config.chain.re_org_parent_threshold.unwrap().0, u64::MAX) + assert_eq!(config.chain.re_org_parent_threshold, None) }); }