Fork choice modifications and cleanup (#3962)

## Issue Addressed NA ## Proposed Changes - Implements https://github.com/ethereum/consensus-specs/pull/3290/ - Bumps `ef-tests` to [v1.3.0-rc.4](https://github.com/ethereum/consensus-spec-tests/releases/tag/v1.3.0-rc.4). The `CountRealizedFull` concept has been removed and the `--count-unrealized-full` and `--count-unrealized` BN flags now do nothing but log a `WARN` when used. ## Database Migration Debt This PR removes the `best_justified_checkpoint` from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the `Option`s around the justified and finalized checkpoints on `ProtoNode` whilst we're at it. Those options were added in #2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to `Some` ever since v2.1.0. There's no reason to keep them as options anymore. I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration [over in my repo](https://github.com/paulhauner/lighthouse/tree/fc-pr-18-migration) so we can pick it up after this PR is merged.
2026-04-17 04:48:21 +00:00 · 2023-03-21 07:34:41 +00:00
parent 3ac5583cf9
commit 1f8c17b530
29 changed files with 217 additions and 414 deletions
--- a/beacon_node/beacon_chain/src/beacon_chain.rs
+++ b/beacon_node/beacon_chain/src/beacon_chain.rs
@@ -73,7 +73,7 @@ use itertools::process_results;
 use itertools::Itertools;
 use operation_pool::{AttestationRef, OperationPool, PersistedOperationPool, ReceivedPreCapella};
 use parking_lot::{Mutex, RwLock};
-use proto_array::{CountUnrealizedFull, DoNotReOrg, ProposerHeadError};
+use proto_array::{DoNotReOrg, ProposerHeadError};
 use safe_arith::SafeArith;
 use slasher::Slasher;
 use slog::{crit, debug, error, info, trace, warn, Logger};
@@ -479,7 +479,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
    pub fn load_fork_choice(
        store: BeaconStore<T>,
        reset_payload_statuses: ResetPayloadStatuses,
-        count_unrealized_full: CountUnrealizedFull,
        spec: &ChainSpec,
        log: &Logger,
    ) -> Result<Option<BeaconForkChoice<T>>, Error> {
@@ -496,7 +495,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
            persisted_fork_choice.fork_choice,
            reset_payload_statuses,
            fc_store,
-            count_unrealized_full,
            spec,
            log,
        )?))
@@ -1900,7 +1898,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                self.slot()?,
                verified.indexed_attestation(),
                AttestationFromBlock::False,
-                &self.spec,
            )
            .map_err(Into::into)
    }
@@ -2868,7 +2865,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                    &state,
                    payload_verification_status,
                    &self.spec,
-                    count_unrealized.and(self.config.count_unrealized.into()),
+                    count_unrealized,
                )
                .map_err(|e| BlockError::BeaconChainError(e.into()))?;
        }
@@ -2987,7 +2984,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                ResetPayloadStatuses::always_reset_conditionally(
                    self.config.always_reset_payload_statuses,
                ),
-                self.config.count_unrealized_full,
                &self.store,
                &self.spec,
                &self.log,
--- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs
+++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs
@@ -20,6 +20,14 @@ use types::{
    Hash256, Slot,
 };

+/// Ensure this justified checkpoint has an epoch of 0 so that it is never
+/// greater than the justified checkpoint and enshrined as the actual justified
+/// checkpoint.
+const JUNK_BEST_JUSTIFIED_CHECKPOINT: Checkpoint = Checkpoint {
+    epoch: Epoch::new(0),
+    root: Hash256::repeat_byte(0),
+};
+
 #[derive(Debug)]
 pub enum Error {
    UnableToReadSlot,
@@ -144,7 +152,6 @@ pub struct BeaconForkChoiceStore<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<
    finalized_checkpoint: Checkpoint,
    justified_checkpoint: Checkpoint,
    justified_balances: JustifiedBalances,
-    best_justified_checkpoint: Checkpoint,
    unrealized_justified_checkpoint: Checkpoint,
    unrealized_finalized_checkpoint: Checkpoint,
    proposer_boost_root: Hash256,
@@ -194,7 +201,6 @@ where
            justified_checkpoint,
            justified_balances,
            finalized_checkpoint,
-            best_justified_checkpoint: justified_checkpoint,
            unrealized_justified_checkpoint: justified_checkpoint,
            unrealized_finalized_checkpoint: finalized_checkpoint,
            proposer_boost_root: Hash256::zero(),
@@ -212,7 +218,7 @@ where
            finalized_checkpoint: self.finalized_checkpoint,
            justified_checkpoint: self.justified_checkpoint,
            justified_balances: self.justified_balances.effective_balances.clone(),
-            best_justified_checkpoint: self.best_justified_checkpoint,
+            best_justified_checkpoint: JUNK_BEST_JUSTIFIED_CHECKPOINT,
            unrealized_justified_checkpoint: self.unrealized_justified_checkpoint,
            unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint,
            proposer_boost_root: self.proposer_boost_root,
@@ -234,7 +240,6 @@ where
            finalized_checkpoint: persisted.finalized_checkpoint,
            justified_checkpoint: persisted.justified_checkpoint,
            justified_balances,
-            best_justified_checkpoint: persisted.best_justified_checkpoint,
            unrealized_justified_checkpoint: persisted.unrealized_justified_checkpoint,
            unrealized_finalized_checkpoint: persisted.unrealized_finalized_checkpoint,
            proposer_boost_root: persisted.proposer_boost_root,
@@ -277,10 +282,6 @@ where
        &self.justified_balances
    }

-    fn best_justified_checkpoint(&self) -> &Checkpoint {
-        &self.best_justified_checkpoint
-    }
-
    fn finalized_checkpoint(&self) -> &Checkpoint {
        &self.finalized_checkpoint
    }
@@ -333,10 +334,6 @@ where
        Ok(())
    }

-    fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint) {
-        self.best_justified_checkpoint = checkpoint
-    }
-
    fn set_unrealized_justified_checkpoint(&mut self, checkpoint: Checkpoint) {
        self.unrealized_justified_checkpoint = checkpoint;
    }
--- a/beacon_node/beacon_chain/src/block_verification.rs
+++ b/beacon_node/beacon_chain/src/block_verification.rs
@@ -1468,7 +1468,6 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
                current_slot,
                indexed_attestation,
                AttestationFromBlock::True,
-                &chain.spec,
            ) {
                Ok(()) => Ok(()),
                // Ignore invalid attestations whilst importing attestations from a block. The
--- a/beacon_node/beacon_chain/src/builder.rs
+++ b/beacon_node/beacon_chain/src/builder.rs
@@ -18,7 +18,7 @@ use crate::{
 };
 use eth1::Config as Eth1Config;
 use execution_layer::ExecutionLayer;
-use fork_choice::{ForkChoice, ResetPayloadStatuses};
+use fork_choice::{CountUnrealized, ForkChoice, ResetPayloadStatuses};
 use futures::channel::mpsc::Sender;
 use operation_pool::{OperationPool, PersistedOperationPool};
 use parking_lot::RwLock;
@@ -265,7 +265,6 @@ where
                ResetPayloadStatuses::always_reset_conditionally(
                    self.chain_config.always_reset_payload_statuses,
                ),
-                self.chain_config.count_unrealized_full,
                &self.spec,
                log,
            )
@@ -384,7 +383,6 @@ where
            &genesis.beacon_block,
            &genesis.beacon_state,
            current_slot,
-            self.chain_config.count_unrealized_full,
            &self.spec,
        )
        .map_err(|e| format!("Unable to initialize ForkChoice: {:?}", e))?;
@@ -503,7 +501,6 @@ where
            &snapshot.beacon_block,
            &snapshot.beacon_state,
            current_slot,
-            self.chain_config.count_unrealized_full,
            &self.spec,
        )
        .map_err(|e| format!("Unable to initialize ForkChoice: {:?}", e))?;
@@ -681,8 +678,7 @@ where
                store.clone(),
                Some(current_slot),
                &self.spec,
-                self.chain_config.count_unrealized.into(),
-                self.chain_config.count_unrealized_full,
+                CountUnrealized::True,
            )?;
        }

--- a/beacon_node/beacon_chain/src/canonical_head.rs
+++ b/beacon_node/beacon_chain/src/canonical_head.rs
@@ -45,8 +45,7 @@ use crate::{
 };
 use eth2::types::{EventKind, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead};
 use fork_choice::{
-    CountUnrealizedFull, ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock,
-    ResetPayloadStatuses,
+    ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock, ResetPayloadStatuses,
 };
 use itertools::process_results;
 use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard};
@@ -285,19 +284,13 @@ impl<T: BeaconChainTypes> CanonicalHead<T> {
        // defensive programming.
        mut fork_choice_write_lock: RwLockWriteGuard<BeaconForkChoice<T>>,
        reset_payload_statuses: ResetPayloadStatuses,
-        count_unrealized_full: CountUnrealizedFull,
        store: &BeaconStore<T>,
        spec: &ChainSpec,
        log: &Logger,
    ) -> Result<(), Error> {
-        let fork_choice = <BeaconChain<T>>::load_fork_choice(
-            store.clone(),
-            reset_payload_statuses,
-            count_unrealized_full,
-            spec,
-            log,
-        )?
-        .ok_or(Error::MissingPersistedForkChoice)?;
+        let fork_choice =
+            <BeaconChain<T>>::load_fork_choice(store.clone(), reset_payload_statuses, spec, log)?
+                .ok_or(Error::MissingPersistedForkChoice)?;
        let fork_choice_view = fork_choice.cached_fork_choice_view();
        let beacon_block_root = fork_choice_view.head_block_root;
        let beacon_block = store
--- a/beacon_node/beacon_chain/src/chain_config.rs
+++ b/beacon_node/beacon_chain/src/chain_config.rs
@@ -1,4 +1,4 @@
-pub use proto_array::{CountUnrealizedFull, ReOrgThreshold};
+pub use proto_array::ReOrgThreshold;
 use serde_derive::{Deserialize, Serialize};
 use std::time::Duration;
 use types::{Checkpoint, Epoch};
@@ -48,16 +48,11 @@ pub struct ChainConfig {
    pub builder_fallback_epochs_since_finalization: usize,
    /// Whether any chain health checks should be considered when deciding whether to use the builder API.
    pub builder_fallback_disable_checks: bool,
-    /// When set to `true`, weigh the "unrealized" FFG progression when choosing a head in fork
-    /// choice.
-    pub count_unrealized: bool,
    /// When set to `true`, forget any valid/invalid/optimistic statuses in fork choice during start
    /// up.
    pub always_reset_payload_statuses: bool,
    /// Whether to apply paranoid checks to blocks proposed by this beacon node.
    pub paranoid_block_proposal: bool,
-    /// Whether to strictly count unrealized justified votes.
-    pub count_unrealized_full: CountUnrealizedFull,
    /// Optionally set timeout for calls to checkpoint sync endpoint.
    pub checkpoint_sync_url_timeout: u64,
    /// The offset before the start of a proposal slot at which payload attributes should be sent.
@@ -91,10 +86,8 @@ impl Default for ChainConfig {
            builder_fallback_skips_per_epoch: 8,
            builder_fallback_epochs_since_finalization: 3,
            builder_fallback_disable_checks: false,
-            count_unrealized: true,
            always_reset_payload_statuses: false,
            paranoid_block_proposal: false,
-            count_unrealized_full: CountUnrealizedFull::default(),
            checkpoint_sync_url_timeout: 60,
            prepare_payload_lookahead: Duration::from_secs(4),
            // This value isn't actually read except in tests.
--- a/beacon_node/beacon_chain/src/fork_revert.rs
+++ b/beacon_node/beacon_chain/src/fork_revert.rs
@@ -1,7 +1,6 @@
 use crate::{BeaconForkChoiceStore, BeaconSnapshot};
 use fork_choice::{CountUnrealized, ForkChoice, PayloadVerificationStatus};
 use itertools::process_results;
-use proto_array::CountUnrealizedFull;
 use slog::{info, warn, Logger};
 use state_processing::state_advance::complete_state_advance;
 use state_processing::{
@@ -102,7 +101,6 @@ pub fn reset_fork_choice_to_finalization<E: EthSpec, Hot: ItemStore<E>, Cold: It
    current_slot: Option<Slot>,
    spec: &ChainSpec,
    count_unrealized_config: CountUnrealized,
-    count_unrealized_full_config: CountUnrealizedFull,
 ) -> Result<ForkChoice<BeaconForkChoiceStore<E, Hot, Cold>, E>, String> {
    // Fetch finalized block.
    let finalized_checkpoint = head_state.finalized_checkpoint();
@@ -156,7 +154,6 @@ pub fn reset_fork_choice_to_finalization<E: EthSpec, Hot: ItemStore<E>, Cold: It
        &finalized_snapshot.beacon_block,
        &finalized_snapshot.beacon_state,
        current_slot,
-        count_unrealized_full_config,
        spec,
    )
    .map_err(|e| format!("Unable to reset fork choice for revert: {:?}", e))?;
--- a/beacon_node/beacon_chain/src/lib.rs
+++ b/beacon_node/beacon_chain/src/lib.rs
@@ -57,7 +57,7 @@ pub use self::beacon_chain::{
    INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY,
 };
 pub use self::beacon_snapshot::BeaconSnapshot;
-pub use self::chain_config::{ChainConfig, CountUnrealizedFull};
+pub use self::chain_config::ChainConfig;
 pub use self::errors::{BeaconChainError, BlockProductionError};
 pub use self::historical_blocks::HistoricalBlockError;
 pub use attestation_verification::Error as AttestationError;
--- a/beacon_node/beacon_chain/src/schema_change.rs
+++ b/beacon_node/beacon_chain/src/schema_change.rs
@@ -3,6 +3,7 @@ mod migration_schema_v12;
 mod migration_schema_v13;
 mod migration_schema_v14;
 mod migration_schema_v15;
+mod migration_schema_v16;

 use crate::beacon_chain::{BeaconChainTypes, ETH1_CACHE_DB_KEY};
 use crate::eth1_chain::SszEth1;
@@ -132,6 +133,14 @@ pub fn migrate_schema<T: BeaconChainTypes>(
            let ops = migration_schema_v15::downgrade_from_v15::<T>(db.clone(), log)?;
            db.store_schema_version_atomically(to, ops)
        }
+        (SchemaVersion(15), SchemaVersion(16)) => {
+            let ops = migration_schema_v16::upgrade_to_v16::<T>(db.clone(), log)?;
+            db.store_schema_version_atomically(to, ops)
+        }
+        (SchemaVersion(16), SchemaVersion(15)) => {
+            let ops = migration_schema_v16::downgrade_from_v16::<T>(db.clone(), log)?;
+            db.store_schema_version_atomically(to, ops)
+        }
        // Anything else is an error.
        (_, _) => Err(HotColdDBError::UnsupportedSchemaVersion {
            target_version: to,
--- a/beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs
+++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs
@@ -0,0 +1,46 @@
+use crate::beacon_chain::{BeaconChainTypes, FORK_CHOICE_DB_KEY};
+use crate::persisted_fork_choice::PersistedForkChoiceV11;
+use slog::{debug, Logger};
+use std::sync::Arc;
+use store::{Error, HotColdDB, KeyValueStoreOp, StoreItem};
+
+pub fn upgrade_to_v16<T: BeaconChainTypes>(
+    db: Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>,
+    log: Logger,
+) -> Result<Vec<KeyValueStoreOp>, Error> {
+    drop_balances_cache::<T>(db, log)
+}
+
+pub fn downgrade_from_v16<T: BeaconChainTypes>(
+    db: Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>,
+    log: Logger,
+) -> Result<Vec<KeyValueStoreOp>, Error> {
+    drop_balances_cache::<T>(db, log)
+}
+
+/// Drop the balances cache from the fork choice store.
+///
+/// There aren't any type-level changes in this schema migration, however the
+/// way that we compute the `JustifiedBalances` has changed due to:
+/// https://github.com/sigp/lighthouse/pull/3962
+pub fn drop_balances_cache<T: BeaconChainTypes>(
+    db: Arc<HotColdDB<T::EthSpec, T::HotStore, T::ColdStore>>,
+    log: Logger,
+) -> Result<Vec<KeyValueStoreOp>, Error> {
+    let mut persisted_fork_choice = db
+        .get_item::<PersistedForkChoiceV11>(&FORK_CHOICE_DB_KEY)?
+        .ok_or_else(|| Error::SchemaMigrationError("fork choice missing from database".into()))?;
+
+    debug!(
+        log,
+        "Dropping fork choice balances cache";
+        "item_count" => persisted_fork_choice.fork_choice_store.balances_cache.items.len()
+    );
+
+    // Drop all items in the balances cache.
+    persisted_fork_choice.fork_choice_store.balances_cache = <_>::default();
+
+    let kv_op = persisted_fork_choice.as_kv_store_op(FORK_CHOICE_DB_KEY);
+
+    Ok(vec![kv_op])
+}