v1.1.6 Fork Choice changes (#2822)

## Issue Addressed

Resolves: #2741
Includes: #2853 so that we can get ssz static tests passing here on v1.1.6. If we want to merge that first, we can make this diff slightly smaller

## Proposed Changes

- Changes the `justified_epoch` and `finalized_epoch` in the `ProtoArrayNode` each to an `Option<Checkpoint>`. The `Option` is necessary only for the migration, so not ideal. But does allow us to add a default logic to `None` on these fields during the database migration.
- Adds a database migration from a legacy fork choice struct to the new one, search for all necessary block roots in fork choice by iterating through blocks in the db.
- updates related to ethereum/consensus-specs#2727
  -  We will have to update the persisted forkchoice to make sure the justified checkpoint stored is correct according to the updated fork choice logic. This boils down to setting the forkchoice store's justified checkpoint to the justified checkpoint of the block that advanced the finalized checkpoint to the current one. 
  - AFAICT there's no migration steps necessary for the update to allow applying attestations from prior blocks, but would appreciate confirmation on that
- I updated the consensus spec tests to v1.1.6 here, but they will fail until we also implement the proposer score boost updates. I confirmed that the previously failing scenario `new_finalized_slot_is_justified_checkpoint_ancestor` will now pass after the boost updates, but haven't confirmed _all_ tests will pass because I just quickly stubbed out the proposer boost test scenario formatting.
- This PR now also includes proposer boosting ethereum/consensus-specs#2730

## Additional Info
I realized checking justified and finalized roots in fork choice makes it more likely that we trigger this bug: ethereum/consensus-specs#2727

It's possible the combination of justified checkpoint and finalized checkpoint in the forkchoice store is different from in any block in fork choice. So when trying to startup our store's justified checkpoint seems invalid to the rest of fork choice (but it should be valid). When this happens we get an `InvalidBestNode` error and fail to start up. So I'm including that bugfix in this branch.

Todo:

- [x] Fix fork choice tests
- [x] Self review
- [x] Add fix for ethereum/consensus-specs#2727
- [x] Rebase onto Kintusgi 
- [x] Fix `num_active_validators` calculation as @michaelsproul pointed out
- [x] Clean up db migrations

Co-authored-by: realbigsean <seananderson33@gmail.com>

beacon_node/beacon_chain/Cargo.toml

@@ -58,6 +58,7 @@ strum = { version = "0.21.0", features = ["derive"] }
 logging = { path = "../../common/logging" }
 execution_layer = { path = "../execution_layer" }
 sensitive_url = { path = "../../common/sensitive_url" }
+superstruct = "0.3.0"
 
 [[test]]
 name = "beacon_chain_tests"

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -51,7 +51,7 @@ use eth2::types::{
     EventKind, SseBlock, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead, SyncDuty,
 };
 use execution_layer::ExecutionLayer;
-use fork_choice::ForkChoice;
+use fork_choice::{AttestationFromBlock, ForkChoice};
 use futures::channel::mpsc::Sender;
 use itertools::process_results;
 use itertools::Itertools;
@@ -1700,7 +1700,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         self.fork_choice
             .write()
-            .on_attestation(self.slot()?, verified.indexed_attestation())
+            .on_attestation(
+                self.slot()?,
+                verified.indexed_attestation(),
+                AttestationFromBlock::False,
+            )
             .map_err(Into::into)
     }
 
@@ -2443,11 +2447,17 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         {
             let _fork_choice_block_timer =
                 metrics::start_timer(&metrics::FORK_CHOICE_PROCESS_BLOCK_TIMES);
+            let block_delay = self
+                .slot_clock
+                .seconds_from_current_slot_start(self.spec.seconds_per_slot)
+                .ok_or(Error::UnableToComputeTimeAtSlot)?;
+
             fork_choice
                 .on_block(
                     current_slot,
                     &block,
                     block_root,
+                    block_delay,
                     &state,
                     payload_verification_status,
                     &self.spec,
@@ -2472,7 +2482,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             let indexed_attestation = get_indexed_attestation(committee.committee, attestation)
                 .map_err(|e| BlockError::BeaconChainError(e.into()))?;
 
-            match fork_choice.on_attestation(current_slot, &indexed_attestation) {
+            match fork_choice.on_attestation(
+                current_slot,
+                &indexed_attestation,
+                AttestationFromBlock::True,
+            ) {
                 Ok(()) => Ok(()),
                 // Ignore invalid attestations whilst importing attestations from a block. The
                 // block might be very old and therefore the attestations useless to fork choice.
@@ -3009,7 +3023,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
     fn fork_choice_internal(&self) -> Result<(), Error> {
         // Determine the root of the block that is the head of the chain.
-        let beacon_block_root = self.fork_choice.write().get_head(self.slot()?)?;
+        let beacon_block_root = self
+            .fork_choice
+            .write()
+            .get_head(self.slot()?, &self.spec)?;
 
         let current_head = self.head_info()?;
         let old_finalized_checkpoint = current_head.finalized_checkpoint;

beacon_node/beacon_chain/src/beacon_fork_choice_store.rs

@@ -1,15 +1,17 @@
 //! Defines the `BeaconForkChoiceStore` which provides the persistent storage for the `ForkChoice`
 //! struct.
 //!
-//! Additionally, the private `BalancesCache` struct is defined; a cache designed to avoid database
+//! Additionally, the `BalancesCache` struct is defined; a cache designed to avoid database
 //! reads when fork choice requires the validator balances of the justified state.
 
 use crate::{metrics, BeaconSnapshot};
+use derivative::Derivative;
 use fork_choice::ForkChoiceStore;
 use ssz_derive::{Decode, Encode};
 use std::marker::PhantomData;
 use std::sync::Arc;
 use store::{Error as StoreError, HotColdDB, ItemStore};
+use superstruct::superstruct;
 use types::{BeaconBlock, BeaconState, BeaconStateError, Checkpoint, EthSpec, Hash256, Slot};
 
 #[derive(Debug)]
@@ -68,7 +70,7 @@ struct CacheItem {
 ///
 /// It is effectively a mapping of `epoch_boundary_block_root -> state.balances`.
 #[derive(PartialEq, Clone, Default, Debug, Encode, Decode)]
-struct BalancesCache {
+pub struct BalancesCache {
     items: Vec<CacheItem>,
 }
 
@@ -154,35 +156,21 @@ impl BalancesCache {
 
 /// Implements `fork_choice::ForkChoiceStore` in order to provide a persistent backing to the
 /// `fork_choice::ForkChoice` struct.
-#[derive(Debug)]
+#[derive(Debug, Derivative)]
+#[derivative(PartialEq(bound = "E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>"))]
 pub struct BeaconForkChoiceStore<E: EthSpec, Hot: ItemStore<E>, Cold: ItemStore<E>> {
+    #[derivative(PartialEq = "ignore")]
     store: Arc<HotColdDB<E, Hot, Cold>>,
     balances_cache: BalancesCache,
     time: Slot,
     finalized_checkpoint: Checkpoint,
     justified_checkpoint: Checkpoint,
     justified_balances: Vec<u64>,
     best_justified_checkpoint: Checkpoint,
+    proposer_boost_root: Hash256,
     _phantom: PhantomData<E>,
 }
 
-impl<E, Hot, Cold> PartialEq for BeaconForkChoiceStore<E, Hot, Cold>
-where
-    E: EthSpec,
-    Hot: ItemStore<E>,
-    Cold: ItemStore<E>,
-{
-    /// This implementation ignores the `store` and `slot_clock`.
-    fn eq(&self, other: &Self) -> bool {
-        self.balances_cache == other.balances_cache
-            && self.time == other.time
-            && self.finalized_checkpoint == other.finalized_checkpoint
-            && self.justified_checkpoint == other.justified_checkpoint
-            && self.justified_balances == other.justified_balances
-            && self.best_justified_checkpoint == other.best_justified_checkpoint
-    }
-}
-
 impl<E, Hot, Cold> BeaconForkChoiceStore<E, Hot, Cold>
 where
     E: EthSpec,
@@ -225,6 +213,7 @@ where
             justified_balances: anchor_state.balances().clone().into(),
             finalized_checkpoint,
             best_justified_checkpoint: justified_checkpoint,
+            proposer_boost_root: Hash256::zero(),
             _phantom: PhantomData,
         }
     }
@@ -239,6 +228,7 @@ where
             justified_checkpoint: self.justified_checkpoint,
             justified_balances: self.justified_balances.clone(),
             best_justified_checkpoint: self.best_justified_checkpoint,
+            proposer_boost_root: self.proposer_boost_root,
         }
     }
 
@@ -255,6 +245,7 @@ where
             justified_checkpoint: persisted.justified_checkpoint,
             justified_balances: persisted.justified_balances,
             best_justified_checkpoint: persisted.best_justified_checkpoint,
+            proposer_boost_root: persisted.proposer_boost_root,
             _phantom: PhantomData,
         })
     }
@@ -301,6 +292,10 @@ where
         &self.finalized_checkpoint
     }
 
+    fn proposer_boost_root(&self) -> Hash256 {
+        self.proposer_boost_root
+    }
+
     fn set_finalized_checkpoint(&mut self, checkpoint: Checkpoint) {
         self.finalized_checkpoint = checkpoint
     }
@@ -336,15 +331,23 @@ where
     fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint) {
         self.best_justified_checkpoint = checkpoint
     }
+
+    fn set_proposer_boost_root(&mut self, proposer_boost_root: Hash256) {
+        self.proposer_boost_root = proposer_boost_root;
+    }
 }
 
 /// A container which allows persisting the `BeaconForkChoiceStore` to the on-disk database.
-#[derive(Encode, Decode)]
+#[superstruct(variants(V1, V7), variant_attributes(derive(Encode, Decode)), no_enum)]
 pub struct PersistedForkChoiceStore {
-    balances_cache: BalancesCache,
-    time: Slot,
-    finalized_checkpoint: Checkpoint,
-    justified_checkpoint: Checkpoint,
-    justified_balances: Vec<u64>,
-    best_justified_checkpoint: Checkpoint,
+    pub balances_cache: BalancesCache,
+    pub time: Slot,
+    pub finalized_checkpoint: Checkpoint,
+    pub justified_checkpoint: Checkpoint,
+    pub justified_balances: Vec<u64>,
+    pub best_justified_checkpoint: Checkpoint,
+    #[superstruct(only(V7))]
+    pub proposer_boost_root: Hash256,
 }
+
+pub type PersistedForkChoiceStore = PersistedForkChoiceStoreV7;

beacon_node/beacon_chain/src/builder.rs

@@ -591,7 +591,7 @@ where
         };
 
         let initial_head_block_root = fork_choice
-            .get_head(current_slot)
+            .get_head(current_slot, &self.spec)
             .map_err(|e| format!("Unable to get fork choice head: {:?}", e))?;
 
         // Try to decode the head block according to the current fork, if that fails, try

beacon_node/beacon_chain/src/fork_revert.rs

@@ -5,6 +5,7 @@ use slog::{info, warn, Logger};
 use state_processing::state_advance::complete_state_advance;
 use state_processing::{per_block_processing, per_block_processing::BlockSignatureStrategy};
 use std::sync::Arc;
+use std::time::Duration;
 use store::{iter::ParentRootBlockIterator, HotColdDB, ItemStore};
 use types::{BeaconState, ChainSpec, EthSpec, ForkName, Hash256, SignedBeaconBlock, Slot};
 
@@ -176,6 +177,8 @@ pub fn reset_fork_choice_to_finalization<E: EthSpec, Hot: ItemStore<E>, Cold: It
                 block.slot(),
                 &block,
                 block.canonical_root(),
+                // Reward proposer boost. We are reinforcing the canonical chain.
+                Duration::from_secs(0),
                 &state,
                 payload_verification_status,
                 spec,