Strict count unrealized (#3522)

## Issue Addressed

Add a flag that can increase count unrealized strictness, defaults to false

## Proposed Changes

Please list or describe the changes introduced by this PR.

## Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.


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

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -65,6 +65,7 @@ use itertools::process_results;
 use itertools::Itertools;
 use operation_pool::{AttestationRef, OperationPool, PersistedOperationPool};
 use parking_lot::{Mutex, RwLock};
+use proto_array::CountUnrealizedFull;
 use safe_arith::SafeArith;
 use slasher::Slasher;
 use slog::{crit, debug, error, info, trace, warn, Logger};
@@ -433,6 +434,7 @@ 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> {
@@ -449,6 +451,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             persisted_fork_choice.fork_choice,
             reset_payload_statuses,
             fc_store,
+            count_unrealized_full,
             spec,
             log,
         )?))
@@ -2934,6 +2937,7 @@ 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/builder.rs

@@ -248,6 +248,7 @@ where
                 ResetPayloadStatuses::always_reset_conditionally(
                     self.chain_config.always_reset_payload_statuses,
                 ),
+                self.chain_config.count_unrealized_full,
                 &self.spec,
                 log,
             )
@@ -365,6 +366,7 @@ 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))?;
@@ -482,6 +484,7 @@ 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))?;
@@ -658,6 +661,7 @@ where
                 Some(current_slot),
                 &self.spec,
                 self.chain_config.count_unrealized.into(),
+                self.chain_config.count_unrealized_full,
             )?;
         }
 

beacon_node/beacon_chain/src/canonical_head.rs

@@ -44,7 +44,8 @@ use crate::{
 };
 use eth2::types::{EventKind, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead};
 use fork_choice::{
-    ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock, ResetPayloadStatuses,
+    CountUnrealizedFull, ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock,
+    ResetPayloadStatuses,
 };
 use itertools::process_results;
 use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard};
@@ -252,13 +253,19 @@ 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 =
+        let fork_choice = <BeaconChain<T>>::load_fork_choice(
-            <BeaconChain<T>>::load_fork_choice(store.clone(), reset_payload_statuses, spec, log)?
+            store.clone(),
-                .ok_or(Error::MissingPersistedForkChoice)?;
+            reset_payload_statuses,
+            count_unrealized_full,
+            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,3 +1,4 @@
+pub use proto_array::CountUnrealizedFull;
 use serde_derive::{Deserialize, Serialize};
 use types::Checkpoint;
 
@@ -42,6 +43,8 @@ pub struct ChainConfig {
     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,
 }
 
 impl Default for ChainConfig {
@@ -61,6 +64,7 @@ impl Default for ChainConfig {
             count_unrealized: true,
             always_reset_payload_statuses: false,
             paranoid_block_proposal: false,
+            count_unrealized_full: CountUnrealizedFull::default(),
         }
     }
 }

beacon_node/beacon_chain/src/fork_revert.rs

@@ -1,6 +1,7 @@
 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::{
@@ -100,6 +101,7 @@ 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();
@@ -152,6 +154,7 @@ 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

@@ -50,7 +50,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;
+pub use self::chain_config::{ChainConfig, CountUnrealizedFull};
 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/migration_schema_v7.rs

@@ -6,7 +6,7 @@ use crate::schema_change::types::{ProtoNodeV6, SszContainerV10, SszContainerV6,
 use crate::types::{ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, Slot};
 use crate::{BeaconForkChoiceStore, BeaconSnapshot};
 use fork_choice::ForkChoice;
-use proto_array::{core::ProtoNode, core::SszContainer, ProtoArrayForkChoice};
+use proto_array::{core::ProtoNode, core::SszContainer, CountUnrealizedFull, ProtoArrayForkChoice};
 use ssz::four_byte_option_impl;
 use ssz::{Decode, Encode};
 use std::collections::{HashMap, HashSet};
@@ -52,6 +52,8 @@ pub(crate) fn update_with_reinitialized_fork_choice<T: BeaconChainTypes>(
         // Don't provide the current slot here, just use what's in the store. We don't need to know
         // the head here, plus it's nice to avoid mutating fork choice during this process.
         None,
+        // This config will get overwritten on startup.
+        CountUnrealizedFull::default(),
         spec,
     )
     .map_err(|e| format!("{:?}", e))?;
@@ -88,7 +90,9 @@ pub(crate) fn update_fork_choice<T: BeaconChainTypes>(
         ssz_container_v6.into_ssz_container_v7(justified_checkpoint, finalized_checkpoint);
     let ssz_container_v10: SszContainerV10 = ssz_container_v7.into();
     let ssz_container: SszContainer = ssz_container_v10.into();
-    let mut fork_choice: ProtoArrayForkChoice = ssz_container.into();
+    // `CountUnrealizedFull::default()` represents the count-unrealized-full config which will be overwritten on startup.
+    let mut fork_choice: ProtoArrayForkChoice =
+        (ssz_container, CountUnrealizedFull::default()).into();
 
     update_checkpoints::<T>(finalized_checkpoint.root, &nodes_v6, &mut fork_choice, db)
         .map_err(StoreError::SchemaMigrationError)?;

beacon_node/src/cli.rs

@@ -786,6 +786,14 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .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`.")
+                .takes_value(true)
+                .default_value("false")
+        )
         .arg(
             Arg::with_name("reset-payload-statuses")
                 .long("reset-payload-statuses")

beacon_node/src/config.rs

@@ -643,6 +643,8 @@ pub fn get_config<E: EthSpec>(
 
     client_config.chain.count_unrealized =
         clap_utils::parse_required(cli_args, "count-unrealized")?;
+    client_config.chain.count_unrealized_full =
+        clap_utils::parse_required::<bool>(cli_args, "count-unrealized-full")?.into();
 
     client_config.chain.always_reset_payload_statuses =
         cli_args.is_present("reset-payload-statuses");

consensus/fork_choice/src/fork_choice.rs

@@ -1,5 +1,7 @@
 use crate::{ForkChoiceStore, InvalidationOperation};
-use proto_array::{Block as ProtoBlock, ExecutionStatus, ProtoArrayForkChoice};
+use proto_array::{
+    Block as ProtoBlock, CountUnrealizedFull, ExecutionStatus, ProtoArrayForkChoice,
+};
 use slog::{crit, debug, warn, Logger};
 use ssz_derive::{Decode, Encode};
 use state_processing::{
@@ -374,6 +376,7 @@ where
         anchor_block: &SignedBeaconBlock<E>,
         anchor_state: &BeaconState<E>,
         current_slot: Option<Slot>,
+        count_unrealized_full_config: CountUnrealizedFull,
         spec: &ChainSpec,
     ) -> Result<Self, Error<T::Error>> {
         // Sanity check: the anchor must lie on an epoch boundary.
@@ -420,6 +423,7 @@ where
             current_epoch_shuffling_id,
             next_epoch_shuffling_id,
             execution_status,
+            count_unrealized_full_config,
         )?;
 
         let mut fork_choice = Self {
@@ -1451,11 +1455,13 @@ where
     pub fn proto_array_from_persisted(
         persisted: &PersistedForkChoice,
         reset_payload_statuses: ResetPayloadStatuses,
+        count_unrealized_full: CountUnrealizedFull,
         spec: &ChainSpec,
         log: &Logger,
     ) -> Result<ProtoArrayForkChoice, Error<T::Error>> {
-        let mut proto_array = ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes)
+        let mut proto_array =
-            .map_err(Error::InvalidProtoArrayBytes)?;
+            ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes, count_unrealized_full)
+                .map_err(Error::InvalidProtoArrayBytes)?;
         let contains_invalid_payloads = proto_array.contains_invalid_payloads();
 
         debug!(
@@ -1486,7 +1492,7 @@ where
                 "error" => e,
                 "info" => "please report this error",
             );
-            ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes)
+            ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes, count_unrealized_full)
                 .map_err(Error::InvalidProtoArrayBytes)
         } else {
             debug!(
@@ -1503,11 +1509,17 @@ where
         persisted: PersistedForkChoice,
         reset_payload_statuses: ResetPayloadStatuses,
         fc_store: T,
+        count_unrealized_full: CountUnrealizedFull,
         spec: &ChainSpec,
         log: &Logger,
     ) -> Result<Self, Error<T::Error>> {
-        let proto_array =
+        let proto_array = Self::proto_array_from_persisted(
-            Self::proto_array_from_persisted(&persisted, reset_payload_statuses, spec, log)?;
+            &persisted,
+            reset_payload_statuses,
+            count_unrealized_full,
+            spec,
+            log,
+        )?;
 
         let current_slot = fc_store.get_current_slot();
 

consensus/fork_choice/src/lib.rs

@@ -7,4 +7,6 @@ pub use crate::fork_choice::{
     PersistedForkChoice, QueuedAttestation, ResetPayloadStatuses,
 };
 pub use fork_choice_store::ForkChoiceStore;
-pub use proto_array::{Block as ProtoBlock, ExecutionStatus, InvalidationOperation};
+pub use proto_array::{
+    Block as ProtoBlock, CountUnrealizedFull, ExecutionStatus, InvalidationOperation,
+};

consensus/proto_array/src/fork_choice_test_definition.rs

@@ -3,6 +3,7 @@ mod ffg_updates;
 mod no_votes;
 mod votes;
 
+use crate::proto_array::CountUnrealizedFull;
 use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkChoice};
 use crate::InvalidationOperation;
 use serde_derive::{Deserialize, Serialize};
@@ -87,6 +88,7 @@ impl ForkChoiceTestDefinition {
             junk_shuffling_id.clone(),
             junk_shuffling_id,
             ExecutionStatus::Optimistic(ExecutionBlockHash::zero()),
+            CountUnrealizedFull::default(),
         )
         .expect("should create fork choice struct");
         let equivocating_indices = BTreeSet::new();
@@ -296,8 +298,8 @@ fn get_checkpoint(i: u64) -> Checkpoint {
 
 fn check_bytes_round_trip(original: &ProtoArrayForkChoice) {
     let bytes = original.as_bytes();
-    let decoded =
+    let decoded = ProtoArrayForkChoice::from_bytes(&bytes, CountUnrealizedFull::default())
-        ProtoArrayForkChoice::from_bytes(&bytes).expect("fork choice should decode from bytes");
+        .expect("fork choice should decode from bytes");
     assert!(
         *original == decoded,
         "fork choice should encode and decode without change"

consensus/proto_array/src/lib.rs

@@ -4,7 +4,7 @@ mod proto_array;
 mod proto_array_fork_choice;
 mod ssz_container;
 
-pub use crate::proto_array::InvalidationOperation;
+pub use crate::proto_array::{CountUnrealizedFull, InvalidationOperation};
 pub use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkChoice};
 pub use error::Error;
 

consensus/proto_array/src/proto_array.rs

@@ -118,6 +118,24 @@ impl Default for ProposerBoost {
     }
 }
 
+/// Indicate whether we should strictly count unrealized justification/finalization votes.
+#[derive(Default, PartialEq, Eq, Debug, Serialize, Deserialize, Copy, Clone)]
+pub enum CountUnrealizedFull {
+    True,
+    #[default]
+    False,
+}
+
+impl From<bool> for CountUnrealizedFull {
+    fn from(b: bool) -> Self {
+        if b {
+            CountUnrealizedFull::True
+        } else {
+            CountUnrealizedFull::False
+        }
+    }
+}
+
 #[derive(PartialEq, Debug, Serialize, Deserialize, Clone)]
 pub struct ProtoArray {
     /// Do not attempt to prune the tree unless it has at least this many nodes. Small prunes
@@ -128,6 +146,7 @@ pub struct ProtoArray {
     pub nodes: Vec<ProtoNode>,
     pub indices: HashMap<Hash256, usize>,
     pub previous_proposer_boost: ProposerBoost,
+    pub count_unrealized_full: CountUnrealizedFull,
 }
 
 impl ProtoArray {
@@ -878,12 +897,14 @@ impl ProtoArray {
             return false;
         }
 
+        let genesis_epoch = Epoch::new(0);
+
         let checkpoint_match_predicate =
             |node_justified_checkpoint: Checkpoint, node_finalized_checkpoint: Checkpoint| {
                 let correct_justified = node_justified_checkpoint == self.justified_checkpoint
-                    || self.justified_checkpoint.epoch == Epoch::new(0);
+                    || self.justified_checkpoint.epoch == genesis_epoch;
                 let correct_finalized = node_finalized_checkpoint == self.finalized_checkpoint
-                    || self.finalized_checkpoint.epoch == Epoch::new(0);
+                    || self.finalized_checkpoint.epoch == genesis_epoch;
                 correct_justified && correct_finalized
             };
 
@@ -898,13 +919,26 @@ impl ProtoArray {
             node.justified_checkpoint,
             node.finalized_checkpoint,
         ) {
-            if node.slot.epoch(E::slots_per_epoch()) < current_slot.epoch(E::slots_per_epoch()) {
+            let current_epoch = current_slot.epoch(E::slots_per_epoch());
-                checkpoint_match_predicate(
+
-                    unrealized_justified_checkpoint,
+            // If previous epoch is justified, pull up all tips to at least the previous epoch
-                    unrealized_finalized_checkpoint,
+            if CountUnrealizedFull::True == self.count_unrealized_full
-                )
+                && (current_epoch > genesis_epoch
+                    && self.justified_checkpoint.epoch + 1 == current_epoch)
+            {
+                unrealized_justified_checkpoint.epoch + 1 >= current_epoch
+            // If previous epoch is not justified, pull up only tips from past epochs up to the current epoch
             } else {
-                checkpoint_match_predicate(justified_checkpoint, finalized_checkpoint)
+                // If block is from a previous epoch, filter using unrealized justification & finalization information
+                if node.slot.epoch(E::slots_per_epoch()) < current_epoch {
+                    checkpoint_match_predicate(
+                        unrealized_justified_checkpoint,
+                        unrealized_finalized_checkpoint,
+                    )
+                // If block is from the current epoch, filter using the head state's justification & finalization information
+                } else {
+                    checkpoint_match_predicate(justified_checkpoint, finalized_checkpoint)
+                }
             }
         } else if let (Some(justified_checkpoint), Some(finalized_checkpoint)) =
             (node.justified_checkpoint, node.finalized_checkpoint)

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -1,4 +1,5 @@
 use crate::error::Error;
+use crate::proto_array::CountUnrealizedFull;
 use crate::proto_array::{
     calculate_proposer_boost, InvalidationOperation, Iter, ProposerBoost, ProtoArray, ProtoNode,
 };
@@ -186,6 +187,7 @@ impl ProtoArrayForkChoice {
         current_epoch_shuffling_id: AttestationShufflingId,
         next_epoch_shuffling_id: AttestationShufflingId,
         execution_status: ExecutionStatus,
+        count_unrealized_full: CountUnrealizedFull,
     ) -> Result<Self, String> {
         let mut proto_array = ProtoArray {
             prune_threshold: DEFAULT_PRUNE_THRESHOLD,
@@ -194,6 +196,7 @@ impl ProtoArrayForkChoice {
             nodes: Vec::with_capacity(1),
             indices: HashMap::with_capacity(1),
             previous_proposer_boost: ProposerBoost::default(),
+            count_unrealized_full,
         };
 
         let block = Block {
@@ -531,8 +534,12 @@ impl ProtoArrayForkChoice {
         SszContainer::from(self).as_ssz_bytes()
     }
 
-    pub fn from_bytes(bytes: &[u8]) -> Result<Self, String> {
+    pub fn from_bytes(
+        bytes: &[u8],
+        count_unrealized_full: CountUnrealizedFull,
+    ) -> Result<Self, String> {
         SszContainer::from_ssz_bytes(bytes)
+            .map(|container| (container, count_unrealized_full))
             .map(Into::into)
             .map_err(|e| format!("Failed to decode ProtoArrayForkChoice: {:?}", e))
     }
@@ -692,6 +699,7 @@ mod test_compute_deltas {
             junk_shuffling_id.clone(),
             junk_shuffling_id.clone(),
             execution_status,
+            CountUnrealizedFull::default(),
         )
         .unwrap();
 

consensus/proto_array/src/ssz_container.rs

@@ -1,6 +1,6 @@
 use crate::proto_array::ProposerBoost;
 use crate::{
-    proto_array::{ProtoArray, ProtoNode},
+    proto_array::{CountUnrealizedFull, ProtoArray, ProtoNode},
     proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker},
 };
 use ssz::{four_byte_option_impl, Encode};
@@ -41,8 +41,8 @@ impl From<&ProtoArrayForkChoice> for SszContainer {
     }
 }
 
-impl From<SszContainer> for ProtoArrayForkChoice {
+impl From<(SszContainer, CountUnrealizedFull)> for ProtoArrayForkChoice {
-    fn from(from: SszContainer) -> Self {
+    fn from((from, count_unrealized_full): (SszContainer, CountUnrealizedFull)) -> Self {
         let proto_array = ProtoArray {
             prune_threshold: from.prune_threshold,
             justified_checkpoint: from.justified_checkpoint,
@@ -50,6 +50,7 @@ impl From<SszContainer> for ProtoArrayForkChoice {
             nodes: from.nodes,
             indices: from.indices.into_iter().collect::<HashMap<_, _>>(),
             previous_proposer_boost: from.previous_proposer_boost,
+            count_unrealized_full,
         };
 
         Self {

lighthouse/tests/beacon_node.rs

@@ -1,4 +1,4 @@
-use beacon_node::ClientConfig as Config;
+use beacon_node::{beacon_chain::CountUnrealizedFull, ClientConfig as Config};
 
 use crate::exec::{CommandLineTestExec, CompletedTest};
 use eth1::Eth1Endpoint;
@@ -178,6 +178,45 @@ fn count_unrealized_true() {
         .with_config(|config| assert!(config.chain.count_unrealized));
 }
 
+#[test]
+fn count_unrealized_full_no_arg() {
+    CommandLineTest::new()
+        .flag("count-unrealized-full", None)
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(
+                config.chain.count_unrealized_full,
+                CountUnrealizedFull::False
+            )
+        });
+}
+
+#[test]
+fn count_unrealized_full_false() {
+    CommandLineTest::new()
+        .flag("count-unrealized-full", Some("false"))
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(
+                config.chain.count_unrealized_full,
+                CountUnrealizedFull::False
+            )
+        });
+}
+
+#[test]
+fn count_unrealized_full_true() {
+    CommandLineTest::new()
+        .flag("count-unrealized-full", Some("true"))
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(
+                config.chain.count_unrealized_full,
+                CountUnrealizedFull::True
+            )
+        });
+}
+
 #[test]
 fn reset_payload_statuses_default() {
     CommandLineTest::new()