Fork choice modifications and cleanup (#3962)

## Issue Addressed

NA

## Proposed Changes

- Implements ethereum/consensus-specs#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.

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,

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;
     }

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

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,
             )?;
         }
 

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

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.

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))?;

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;

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,

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])
+}

beacon_node/beacon_chain/tests/tests.rs

@@ -500,7 +500,7 @@ async fn unaggregated_attestations_added_to_fork_choice_some_none() {
     // Move forward a slot so all queued attestations can be processed.
     harness.advance_slot();
     fork_choice
-        .update_time(harness.chain.slot().unwrap(), &harness.chain.spec)
+        .update_time(harness.chain.slot().unwrap())
         .unwrap();
 
     let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT)
@@ -614,7 +614,7 @@ async fn unaggregated_attestations_added_to_fork_choice_all_updated() {
     // Move forward a slot so all queued attestations can be processed.
     harness.advance_slot();
     fork_choice
-        .update_time(harness.chain.slot().unwrap(), &harness.chain.spec)
+        .update_time(harness.chain.slot().unwrap())
         .unwrap();
 
     let validators: Vec<usize> = (0..VALIDATOR_COUNT).collect();

beacon_node/src/cli.rs

@@ -966,16 +966,15 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
             Arg::with_name("count-unrealized")
                 .long("count-unrealized")
                 .hidden(true)
-                .help("Enables an alternative, potentially more performant FFG \
-                       vote tracking method.")
+                .help("This flag is deprecated and has no effect.")
                 .takes_value(true)
                 .default_value("true")
         )
         .arg(
             Arg::with_name("count-unrealized-full")
                 .long("count-unrealized-full")
                 .hidden(true)
-                .help("Stricter version of `count-unrealized`.")
+                .help("This flag is deprecated and has no effect.")
                 .takes_value(true)
                 .default_value("false")
         )