Remove progressive balances mode (#5224)

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3369,9 +3369,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                     block_delay,
                     &state,
                     payload_verification_status,
-                    self.config.progressive_balances_mode,
                     &self.spec,
-                    &self.log,
                 )
                 .map_err(|e| BlockError::BeaconChainError(e.into()))?;
         }

beacon_node/beacon_chain/src/builder.rs

@@ -751,8 +751,6 @@ where
                 store.clone(),
                 Some(current_slot),
                 &self.spec,
-                self.chain_config.progressive_balances_mode,
-                &log,
             )?;
         }
 

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);
@@ -79,8 +79,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,
     /// When set to true Light client server computes and caches state proofs for serving updates
@@ -114,7 +112,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,
             enable_light_client_server: false,
         }

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<E: EthSpec, Hot: ItemStore<E>, Cold: It
     store: Arc<HotColdDB<E, Hot, Cold>>,
     current_slot: Option<Slot>,
     spec: &ChainSpec,
-    progressive_balances_mode: ProgressiveBalancesMode,
-    log: &Logger,
 ) -> Result<ForkChoice<BeaconForkChoiceStore<E, Hot, Cold>, E>, String> {
     // Fetch finalized block.
     let finalized_checkpoint = head_state.finalized_checkpoint();
@@ -202,9 +197,7 @@ pub fn reset_fork_choice_to_finalization<E: EthSpec, Hot: ItemStore<E>, 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))?;
     }

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!(

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")
@@ -1208,14 +1207,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("beacon-processor-max-workers")

beacon_node/src/config.rs

@@ -833,10 +833,12 @@ pub fn get_config<E: EthSpec>(
         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")?

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<T> {
     },
     UnrealizedVoteProcessing(state_processing::EpochProcessingError),
     ValidatorStatuses(BeaconStateError),
-    ProgressiveBalancesCacheCheckFailed(String),
 }
 
 impl<T> From<InvalidAttestation> for Error<T> {
@@ -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<Payload: AbstractExecPayload<E>>(
         &mut self,
         system_time_current_slot: Slot,
@@ -646,9 +642,7 @@ where
         block_delay: Duration,
         state: &BeaconState<E>,
         payload_verification_status: PayloadVerificationStatus,
-        _progressive_balances_mode: ProgressiveBalancesMode,
         spec: &ChainSpec,
-        _log: &Logger,
     ) -> Result<(), Error<T::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

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

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<E: EthSpec>(state: &BeaconState<E>) -> bool {
     match state {

lighthouse/tests/beacon_node.rs

@@ -19,10 +19,7 @@ use std::string::ToString;
 use std::time::Duration;
 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;
@@ -2462,29 +2459,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]

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<E: EthSpec> Tester<E> {
                         block_delay,
                         &state,
                         PayloadVerificationStatus::Irrelevant,
-                        ProgressiveBalancesMode::Strict,
                         &self.harness.chain.spec,
-                        self.harness.logger(),
                     );
 
                 if result.is_ok() {