diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index b73b6037f7..69b9ef043c 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3322,9 +3322,7 @@ impl BeaconChain { block_delay, &state, payload_verification_status, - self.config.progressive_balances_mode, &self.spec, - &self.log, ) .map_err(|e| BlockError::BeaconChainError(e.into()))?; } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 59a276cdc6..0da57dd81a 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -759,8 +759,6 @@ where store.clone(), Some(current_slot), &self.spec, - self.chain_config.progressive_balances_mode, - &log, )?; } diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index 79d8f2a441..ed0936e8d7 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -1,7 +1,7 @@ pub use proto_array::{DisallowedReOrgOffsets, ReOrgThreshold}; use serde::{Deserialize, Serialize}; use std::time::Duration; -use types::{Checkpoint, Epoch, ProgressiveBalancesMode}; +use types::{Checkpoint, Epoch}; pub const DEFAULT_RE_ORG_THRESHOLD: ReOrgThreshold = ReOrgThreshold(20); pub const DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION: Epoch = Epoch::new(2); @@ -82,8 +82,6 @@ pub struct ChainConfig { /// /// This is useful for block builders and testing. pub always_prepare_payload: bool, - /// Whether to use `ProgressiveBalancesCache` in unrealized FFG progression calculation. - pub progressive_balances_mode: ProgressiveBalancesMode, /// Number of epochs between each migration of data from the hot database to the freezer. pub epochs_per_migration: u64, /// Size of the promise cache for de-duplicating parallel state requests. @@ -117,7 +115,6 @@ impl Default for ChainConfig { shuffling_cache_size: crate::shuffling_cache::DEFAULT_CACHE_SIZE, genesis_backfill: false, always_prepare_payload: false, - progressive_balances_mode: ProgressiveBalancesMode::Fast, epochs_per_migration: crate::migrate::DEFAULT_EPOCHS_PER_MIGRATION, parallel_state_cache_size: DEFAULT_PARALLEL_STATE_CACHE_SIZE, } diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index 52cfc474b4..d25cecda55 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -10,10 +10,7 @@ use state_processing::{ use std::sync::Arc; use std::time::Duration; use store::{iter::ParentRootBlockIterator, HotColdDB, ItemStore}; -use types::{ - BeaconState, ChainSpec, EthSpec, ForkName, Hash256, ProgressiveBalancesMode, SignedBeaconBlock, - Slot, -}; +use types::{BeaconState, ChainSpec, EthSpec, ForkName, Hash256, SignedBeaconBlock, Slot}; const CORRUPT_DB_MESSAGE: &str = "The database could be corrupt. Check its file permissions or \ consider deleting it by running with the --purge-db flag."; @@ -103,8 +100,6 @@ pub fn reset_fork_choice_to_finalization, Cold: It store: Arc>, current_slot: Option, spec: &ChainSpec, - progressive_balances_mode: ProgressiveBalancesMode, - log: &Logger, ) -> Result, E>, String> { // Fetch finalized block. let finalized_checkpoint = head_state.finalized_checkpoint(); @@ -202,9 +197,7 @@ pub fn reset_fork_choice_to_finalization, Cold: It Duration::from_secs(0), &state, payload_verification_status, - progressive_balances_mode, spec, - log, ) .map_err(|e| format!("Error applying replayed block to fork choice: {:?}", e))?; } diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index edba16e135..30e180c70a 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1077,9 +1077,7 @@ async fn invalid_parent() { Duration::from_secs(0), &state, PayloadVerificationStatus::Optimistic, - rig.harness.chain.config.progressive_balances_mode, &rig.harness.chain.spec, - rig.harness.logger() ), Err(ForkChoiceError::ProtoArrayStringError(message)) if message.contains(&format!( diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 23d366d15a..ea54e06d34 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1,6 +1,5 @@ use clap::{App, Arg, ArgGroup}; use strum::VariantNames; -use types::ProgressiveBalancesMode; pub fn cli_app<'a, 'b>() -> App<'a, 'b> { App::new("beacon_node") @@ -1265,14 +1264,9 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("progressive-balances") .long("progressive-balances") .value_name("MODE") - .help("Control the progressive balances cache mode. The default `fast` mode uses \ - the cache to speed up fork choice. A more conservative `checked` mode \ - compares the cache's results against results without the cache. If \ - there is a mismatch, it falls back to the cache-free result. Using the \ - default `fast` mode is recommended unless advised otherwise by the \ - Lighthouse team.") + .help("Deprecated. This optimisation is now the default and cannot be disabled.") .takes_value(true) - .possible_values(ProgressiveBalancesMode::VARIANTS) + .possible_values(&["fast", "disabled", "checked", "strict"]) ) .arg( Arg::with_name("unsafe-and-dangerous-mode") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 879d1d22b9..3102cb4441 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -860,10 +860,12 @@ pub fn get_config( client_config.network.invalid_block_storage = Some(path); } - if let Some(progressive_balances_mode) = - clap_utils::parse_optional(cli_args, "progressive-balances")? - { - client_config.chain.progressive_balances_mode = progressive_balances_mode; + if cli_args.is_present("progressive-balances") { + warn!( + log, + "Progressive balances mode is deprecated"; + "info" => "please remove --progressive-balances" + ); } if let Some(max_workers) = clap_utils::parse_optional(cli_args, "beacon-processor-max-workers")? diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 93dc4e3c4f..f2684a4d9f 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -12,7 +12,6 @@ use std::cmp::Ordering; use std::collections::BTreeSet; use std::marker::PhantomData; use std::time::Duration; -use types::ProgressiveBalancesMode; use types::{ consts::merge::INTERVALS_PER_SLOT, AbstractExecPayload, AttestationShufflingId, AttesterSlashing, BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, Checkpoint, Epoch, @@ -73,7 +72,6 @@ pub enum Error { }, UnrealizedVoteProcessing(state_processing::EpochProcessingError), ValidatorStatuses(BeaconStateError), - ProgressiveBalancesCacheCheckFailed(String), } impl From for Error { @@ -636,8 +634,6 @@ where /// /// The supplied block **must** pass the `state_transition` function as it will not be run /// here. - #[allow(clippy::too_many_arguments)] - // FIXME(sproul): remove progressive balances mode pub fn on_block>( &mut self, system_time_current_slot: Slot, @@ -646,9 +642,7 @@ where block_delay: Duration, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, - _progressive_balances_mode: ProgressiveBalancesMode, spec: &ChainSpec, - _log: &Logger, ) -> Result<(), Error> { // If this block has already been processed we do not need to reprocess it. // We check this immediately in case re-processing the block mutates some property of the diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 5305c13ae6..3cae5b740e 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -16,8 +16,8 @@ use std::time::Duration; use store::MemoryStore; use types::{ test_utils::generate_deterministic_keypair, BeaconBlockRef, BeaconState, ChainSpec, Checkpoint, - Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, MainnetEthSpec, ProgressiveBalancesMode, - RelativeEpoch, SignedBeaconBlock, Slot, SubnetId, + Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, MainnetEthSpec, RelativeEpoch, + SignedBeaconBlock, Slot, SubnetId, }; pub type E = MainnetEthSpec; @@ -47,8 +47,10 @@ impl fmt::Debug for ForkChoiceTest { impl ForkChoiceTest { /// Creates a new tester. pub fn new() -> Self { + // Run fork choice tests against the latest fork. + let spec = ForkName::latest().make_genesis_spec(ChainSpec::default()); let harness = BeaconChainHarness::builder(MainnetEthSpec) - .default_spec() + .spec(spec) .deterministic_keypairs(VALIDATOR_COUNT) .fresh_ephemeral_store() .build(); @@ -58,29 +60,13 @@ impl ForkChoiceTest { /// Creates a new tester with a custom chain config. pub fn new_with_chain_config(chain_config: ChainConfig) -> Self { - let harness = BeaconChainHarness::builder(MainnetEthSpec) - .default_spec() - .chain_config(chain_config) - .deterministic_keypairs(VALIDATOR_COUNT) - .fresh_ephemeral_store() - .build(); - - Self { harness } - } - - /// Creates a new tester with the specified `ProgressiveBalancesMode` and genesis from latest fork. - fn new_with_progressive_balances_mode(mode: ProgressiveBalancesMode) -> ForkChoiceTest { - // genesis with latest fork (at least altair required to test the cache) + // Run fork choice tests against the latest fork. let spec = ForkName::latest().make_genesis_spec(ChainSpec::default()); let harness = BeaconChainHarness::builder(MainnetEthSpec) .spec(spec) - .chain_config(ChainConfig { - progressive_balances_mode: mode, - ..ChainConfig::default() - }) + .chain_config(chain_config) .deterministic_keypairs(VALIDATOR_COUNT) .fresh_ephemeral_store() - .mock_execution_layer() .build(); Self { harness } @@ -338,9 +324,7 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - self.harness.chain.config.progressive_balances_mode, &self.harness.chain.spec, - self.harness.logger(), ) .unwrap(); self @@ -383,9 +367,7 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, - self.harness.chain.config.progressive_balances_mode, &self.harness.chain.spec, - self.harness.logger(), ) .expect_err("on_block did not return an error"); comparison_func(err); @@ -1348,7 +1330,7 @@ async fn weak_subjectivity_check_epoch_boundary_is_skip_slot_failure() { /// where the slashed validator is a target attester in previous / current epoch. #[tokio::test] async fn progressive_balances_cache_attester_slashing() { - ForkChoiceTest::new_with_progressive_balances_mode(ProgressiveBalancesMode::Strict) + ForkChoiceTest::new() // first two epochs .apply_blocks_while(|_, state| state.finalized_checkpoint().epoch == 0) .await @@ -1379,7 +1361,7 @@ async fn progressive_balances_cache_attester_slashing() { /// where the slashed validator is a target attester in previous / current epoch. #[tokio::test] async fn progressive_balances_cache_proposer_slashing() { - ForkChoiceTest::new_with_progressive_balances_mode(ProgressiveBalancesMode::Strict) + ForkChoiceTest::new() // first two epochs .apply_blocks_while(|_, state| state.finalized_checkpoint().epoch == 0) .await diff --git a/consensus/types/src/beacon_state/progressive_balances_cache.rs b/consensus/types/src/beacon_state/progressive_balances_cache.rs index d0e0010b93..69c76447b4 100644 --- a/consensus/types/src/beacon_state/progressive_balances_cache.rs +++ b/consensus/types/src/beacon_state/progressive_balances_cache.rs @@ -8,8 +8,6 @@ use crate::{ }; use arbitrary::Arbitrary; use safe_arith::SafeArith; -use serde::{Deserialize, Serialize}; -use strum::{Display, EnumString, EnumVariantNames}; /// This cache keeps track of the accumulated target attestation balance for the current & previous /// epochs. The cached values can be utilised by fork choice to calculate unrealized justification @@ -285,33 +283,6 @@ impl ProgressiveBalancesCache { } } -#[derive( - Debug, PartialEq, Eq, Clone, Copy, Deserialize, Serialize, Display, EnumString, EnumVariantNames, -)] -#[strum(serialize_all = "lowercase")] -pub enum ProgressiveBalancesMode { - /// Disable the usage of progressive cache, and use the existing `ParticipationCache` calculation. - Disabled, - /// Enable the usage of progressive cache, with checks against the `ParticipationCache` and falls - /// back to the existing calculation if there is a balance mismatch. - Checked, - /// Enable the usage of progressive cache, with checks against the `ParticipationCache`. BeaconStateErrors - /// if there is a balance mismatch. Used in testing only. - Strict, - /// Enable the usage of progressive cache, with no comparative checks against the - /// `ParticipationCache`. This is fast but an experimental mode, use with caution. - Fast, -} - -impl ProgressiveBalancesMode { - pub fn perform_comparative_checks(&self) -> bool { - match self { - Self::Disabled | Self::Fast => false, - Self::Checked | Self::Strict => true, - } - } -} - /// `ProgressiveBalancesCache` is only enabled from `Altair` as it requires `ParticipationCache`. pub fn is_progressive_balances_enabled(state: &BeaconState) -> bool { match state { diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 3036afb69f..3ef44e9ed2 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -20,10 +20,7 @@ use std::time::Duration; use store::hdiff::HierarchyConfig; use tempfile::TempDir; use types::non_zero_usize::new_non_zero_usize; -use types::{ - Address, Checkpoint, Epoch, ExecutionBlockHash, ForkName, Hash256, MainnetEthSpec, - ProgressiveBalancesMode, -}; +use types::{Address, Checkpoint, Epoch, ExecutionBlockHash, ForkName, Hash256, MainnetEthSpec}; const DEFAULT_ETH1_ENDPOINT: &str = "http://localhost:8545/"; const DUMMY_ENR_TCP_PORT: u16 = 7777; @@ -2497,29 +2494,12 @@ fn invalid_gossip_verified_blocks_path() { }); } -#[test] -fn progressive_balances_default() { - CommandLineTest::new() - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.progressive_balances_mode, - ProgressiveBalancesMode::Fast - ) - }); -} - #[test] fn progressive_balances_checked() { + // Flag is deprecated but supplying it should not crash until we remove it completely. CommandLineTest::new() .flag("progressive-balances", Some("checked")) - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.progressive_balances_mode, - ProgressiveBalancesMode::Checked - ) - }); + .run_with_zero_port(); } #[test] diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 9884a709eb..467d71b859 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -25,7 +25,7 @@ use std::time::Duration; use types::{ Attestation, AttesterSlashing, BeaconBlock, BeaconState, BlobSidecar, BlobsList, Checkpoint, EthSpec, ExecutionBlockHash, ForkName, Hash256, IndexedAttestation, KzgProof, - ProgressiveBalancesMode, ProposerPreparationData, SignedBeaconBlock, Slot, Uint256, + ProposerPreparationData, SignedBeaconBlock, Slot, Uint256, }; #[derive(Default, Debug, PartialEq, Clone, Deserialize, Decode)] @@ -557,9 +557,7 @@ impl Tester { block_delay, &state, PayloadVerificationStatus::Irrelevant, - ProgressiveBalancesMode::Strict, &self.harness.chain.spec, - self.harness.logger(), ); if result.is_ok() {