Enforce Optimistic Sync Conditions & CLI Tests (v2) (#3050)

## Description

This PR adds a single, trivial commit (f5d2b27) atop #2986 to resolve a tests compile error. The original author (@ethDreamer) is AFK so I'm getting this one merged ☺️ 

Please see #2986 for more information about the other, significant changes in this PR.


Co-authored-by: Mark Mackey <[email protected]>
Co-authored-by: ethDreamer <[email protected]>

Author: paulhauner

Cargo.lock

@@ -312,6 +312,8 @@ pub enum ExecutionPayloadError {
     ///
     /// The peer is not necessarily invalid.
     PoWParentMissing(ExecutionBlockHash),
+    /// The execution node is syncing but we fail the conditions for optimistic sync
+    UnverifiedNonOptimisticCandidate,
 }
 
 impl From<execution_layer::Error> for ExecutionPayloadError {
@@ -1128,6 +1130,29 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
         // `randao` may change.
         let payload_verification_status = notify_new_payload(chain, &state, block.message())?;
 
+        // If the payload did not validate or invalidate the block, check to see if this block is
+        // valid for optimistic import.
+        if payload_verification_status == PayloadVerificationStatus::NotVerified {
+            let current_slot = chain
+                .slot_clock
+                .now()
+                .ok_or(BeaconChainError::UnableToReadSlot)?;
+
+            if !chain
+                .fork_choice
+                .read()
+                .is_optimistic_candidate_block(
+                    current_slot,
+                    block.slot(),
+                    &block.parent_root(),
+                    &chain.spec,
+                )
+                .map_err(BeaconChainError::from)?
+            {
+                return Err(ExecutionPayloadError::UnverifiedNonOptimisticCandidate.into());
+            }
+        }
+
         // If the block is sufficiently recent, notify the validator monitor.
         if let Some(slot) = chain.slot_clock.now() {
             let epoch = slot.epoch(T::EthSpec::slots_per_epoch());

beacon_node/beacon_chain/src/block_verification.rs

@@ -141,13 +141,25 @@ pub fn validate_merge_block<T: BeaconChainTypes>(
         }
         .into()),
         None => {
-            debug!(
-                chain.log,
-                "Optimistically accepting terminal block";
-                "block_hash" => ?execution_payload.parent_hash,
-                "msg" => "the terminal block/parent was unavailable"
-            );
-            Ok(())
+            let current_slot = chain
+                .slot_clock
+                .now()
+                .ok_or(BeaconChainError::UnableToReadSlot)?;
+            // Check the optimistic sync conditions. Note that because this is the merge block,
+            // the justified checkpoint can't have execution enabled so we only need to check the
+            // current slot is at least SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the block
+            // https://github.com/ethereum/consensus-specs/blob/v1.1.9/sync/optimistic.md#when-to-optimistically-import-blocks
+            if block.slot() + chain.spec.safe_slots_to_import_optimistically <= current_slot {
+                debug!(
+                    chain.log,
+                    "Optimistically accepting terminal block";
+                    "block_hash" => ?execution_payload.parent_hash,
+                    "msg" => "the terminal block/parent was unavailable"
+                );
+                Ok(())
+            } else {
+                Err(ExecutionPayloadError::UnverifiedNonOptimisticCandidate.into())
+            }
         }
     }
 }

beacon_node/beacon_chain/src/execution_payload.rs

@@ -772,6 +772,7 @@ impl<T: BeaconChainTypes> Worker<T> {
             }
             // TODO(merge): reconsider peer scoring for this event.
             Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)))
+            | Err(e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::UnverifiedNonOptimisticCandidate))
             | Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection)) => {
                 debug!(self.log, "Could not verify block for gossip, ignoring the block";
                             "error" => %e);

beacon_node/network/src/beacon_processor/worker/gossip_methods.rs

@@ -23,4 +23,5 @@ hex = "0.4.2"
 serde = "1.0.116"
 serde_derive = "1.0.116"
 serde_json = "1.0.66"
+serde_yaml = "0.8.13"
 eth2_network_config = { path = "../common/eth2_network_config" }

boot_node/Cargo.toml

@@ -3,8 +3,6 @@ use clap::ArgMatches;
 use slog::{o, Drain, Level, Logger};
 
 use eth2_network_config::Eth2NetworkConfig;
-use std::fs::File;
-use std::path::PathBuf;
 mod cli;
 pub mod config;
 mod server;
@@ -86,15 +84,13 @@ fn main<T: EthSpec>(
     // parse the CLI args into a useable config
     let config: BootNodeConfig<T> = BootNodeConfig::new(bn_matches, eth2_network_config)?;
 
-    // Dump config if `dump-config` flag is set
-    let dump_config = clap_utils::parse_optional::<PathBuf>(lh_matches, "dump-config")?;
-    if let Some(dump_path) = dump_config {
-        let config_sz = BootNodeConfigSerialization::from_config_ref(&config);
-        let mut file = File::create(dump_path)
-            .map_err(|e| format!("Failed to create dumped config: {:?}", e))?;
-        serde_json::to_writer(&mut file, &config_sz)
-            .map_err(|e| format!("Error serializing config: {:?}", e))?;
-    }
+    // Dump configs if `dump-config` or `dump-chain-config` flags are set
+    let config_sz = BootNodeConfigSerialization::from_config_ref(&config);
+    clap_utils::check_dump_configs::<_, T>(
+        lh_matches,
+        &config_sz,
+        &eth2_network_config.chain_spec::<T>()?,
+    )?;
 
     // Run the boot node
     if !lh_matches.is_present("immediate-shutdown") {

boot_node/src/lib.rs

@@ -13,3 +13,7 @@ dirs = "3.0.1"
 eth2_network_config = { path = "../eth2_network_config" }
 eth2_ssz = "0.4.1"
 ethereum-types = "0.12.1"
+serde = "1.0.116"
+serde_json = "1.0.59"
+serde_yaml = "0.8.13"
+types = { path = "../../consensus/types"}

common/clap_utils/Cargo.toml

@@ -6,6 +6,7 @@ use ethereum_types::U256 as Uint256;
 use ssz::Decode;
 use std::path::PathBuf;
 use std::str::FromStr;
+use types::{ChainSpec, Config, EthSpec};
 
 pub mod flags;
 
@@ -52,6 +53,12 @@ pub fn get_eth2_network_config(cli_args: &ArgMatches) -> Result<Eth2NetworkConfi
             .terminal_block_hash_activation_epoch = epoch;
     }
 
+    if let Some(slots) = parse_optional(cli_args, "safe-slots-to-import-optimistically")? {
+        eth2_network_config
+            .config
+            .safe_slots_to_import_optimistically = slots;
+    }
+
     Ok(eth2_network_config)
 }
 
@@ -157,3 +164,29 @@ pub fn parse_ssz_optional<T: Decode>(
         })
         .transpose()
 }
+
+/// Writes configs to file if `dump-config` or `dump-chain-config` flags are set
+pub fn check_dump_configs<S, E>(
+    matches: &ArgMatches,
+    config: S,
+    spec: &ChainSpec,
+) -> Result<(), String>
+where
+    S: serde::Serialize,
+    E: EthSpec,
+{
+    if let Some(dump_path) = parse_optional::<PathBuf>(matches, "dump-config")? {
+        let mut file = std::fs::File::create(dump_path)
+            .map_err(|e| format!("Failed to open file for writing config: {:?}", e))?;
+        serde_json::to_writer(&mut file, &config)
+            .map_err(|e| format!("Error serializing config: {:?}", e))?;
+    }
+    if let Some(dump_path) = parse_optional::<PathBuf>(matches, "dump-chain-config")? {
+        let chain_config = Config::from_chain_spec::<E>(spec);
+        let mut file = std::fs::File::create(dump_path)
+            .map_err(|e| format!("Failed to open file for writing chain config: {:?}", e))?;
+        serde_yaml::to_writer(&mut file, &chain_config)
+            .map_err(|e| format!("Error serializing config: {:?}", e))?;
+    }
+    Ok(())
+}

common/clap_utils/src/lib.rs

@@ -10,7 +10,7 @@ pub enum SensitiveError {
 }
 
 // Wrapper around Url which provides a custom `Display` implementation to protect user secrets.
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct SensitiveUrl {
     pub full: Url,
     pub redacted: String,

common/sensitive_url/src/lib.rs

@@ -928,6 +928,54 @@ where
             .is_descendant(self.fc_store.finalized_checkpoint().root, block_root)
     }
 
+    /// Returns `Ok(false)` if a block is not viable to be imported optimistically.
+    ///
+    /// ## Notes
+    ///
+    /// Equivalent to the function with the same name in the optimistic sync specs:
+    ///
+    /// https://github.com/ethereum/consensus-specs/blob/dev/sync/optimistic.md#helpers
+    pub fn is_optimistic_candidate_block(
+        &self,
+        current_slot: Slot,
+        block_slot: Slot,
+        block_parent_root: &Hash256,
+        spec: &ChainSpec,
+    ) -> Result<bool, Error<T::Error>> {
+        // If the block is sufficiently old, import it.
+        if block_slot + spec.safe_slots_to_import_optimistically <= current_slot {
+            return Ok(true);
+        }
+
+        // If the justified block has execution enabled, then optimistically import any block.
+        if self
+            .get_justified_block()?
+            .execution_status
+            .is_execution_enabled()
+        {
+            return Ok(true);
+        }
+
+        // If the block has an ancestor with a verified parent, import this block.
+        //
+        // TODO: This condition is not yet merged into the spec. See:
+        //
+        // https://github.com/ethereum/consensus-specs/pull/2841
+        //
+        // ## Note
+        //
+        // If `block_parent_root` is unknown this iter will always return `None`.
+        if self
+            .proto_array
+            .iter_nodes(block_parent_root)
+            .any(|node| node.execution_status.is_valid())
+        {
+            return Ok(true);
+        }
+
+        Ok(false)
+    }
+
     /// Return the current finalized checkpoint.
     pub fn finalized_checkpoint(&self) -> Checkpoint {
         *self.fc_store.finalized_checkpoint()

consensus/fork_choice/src/fork_choice.rs

@@ -1,5 +1,5 @@
 use crate::error::Error;
-use crate::proto_array::{ProposerBoost, ProtoArray};
+use crate::proto_array::{Iter, ProposerBoost, ProtoArray};
 use crate::ssz_container::SszContainer;
 use serde_derive::{Deserialize, Serialize};
 use ssz::{Decode, Encode};
@@ -40,6 +40,10 @@ pub enum ExecutionStatus {
 }
 
 impl ExecutionStatus {
+    pub fn is_execution_enabled(&self) -> bool {
+        !matches!(self, ExecutionStatus::Irrelevant(_))
+    }
+
     pub fn irrelevant() -> Self {
         ExecutionStatus::Irrelevant(false)
     }
@@ -341,6 +345,11 @@ impl ProtoArrayForkChoice {
         }
     }
 
+    /// See `ProtoArray::iter_nodes`
+    pub fn iter_nodes<'a>(&'a self, block_root: &Hash256) -> Iter<'a> {
+        self.proto_array.iter_nodes(block_root)
+    }
+
     pub fn as_bytes(&self) -> Vec<u8> {
         SszContainer::from(self).as_ssz_bytes()
     }

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -146,6 +146,7 @@ pub struct ChainSpec {
     pub terminal_total_difficulty: Uint256,
     pub terminal_block_hash: ExecutionBlockHash,
     pub terminal_block_hash_activation_epoch: Epoch,
+    pub safe_slots_to_import_optimistically: u64,
 
     /*
      * Networking
@@ -551,6 +552,7 @@ impl ChainSpec {
                 .expect("addition does not overflow"),
             terminal_block_hash: ExecutionBlockHash::zero(),
             terminal_block_hash_activation_epoch: Epoch::new(u64::MAX),
+            safe_slots_to_import_optimistically: 128u64,
 
             /*
              * Network specific
@@ -748,6 +750,7 @@ impl ChainSpec {
                 .expect("addition does not overflow"),
             terminal_block_hash: ExecutionBlockHash::zero(),
             terminal_block_hash_activation_epoch: Epoch::new(u64::MAX),
+            safe_slots_to_import_optimistically: 128u64,
 
             /*
              * Network specific
@@ -791,6 +794,9 @@ pub struct Config {
     // TODO(merge): remove this default
     #[serde(default = "default_terminal_block_hash_activation_epoch")]
     pub terminal_block_hash_activation_epoch: Epoch,
+    // TODO(merge): remove this default
+    #[serde(default = "default_safe_slots_to_import_optimistically")]
+    pub safe_slots_to_import_optimistically: u64,
 
     #[serde(with = "eth2_serde_utils::quoted_u64")]
     min_genesis_active_validator_count: u64,
@@ -878,6 +884,10 @@ fn default_terminal_block_hash_activation_epoch() -> Epoch {
     Epoch::new(u64::MAX)
 }
 
+fn default_safe_slots_to_import_optimistically() -> u64 {
+    128u64
+}
+
 impl Default for Config {
     fn default() -> Self {
         let chain_spec = MainnetEthSpec::default_spec();
@@ -935,6 +945,7 @@ impl Config {
             terminal_total_difficulty: spec.terminal_total_difficulty,
             terminal_block_hash: spec.terminal_block_hash,
             terminal_block_hash_activation_epoch: spec.terminal_block_hash_activation_epoch,
+            safe_slots_to_import_optimistically: spec.safe_slots_to_import_optimistically,
 
             min_genesis_active_validator_count: spec.min_genesis_active_validator_count,
             min_genesis_time: spec.min_genesis_time,
@@ -985,6 +996,7 @@ impl Config {
             terminal_total_difficulty,
             terminal_block_hash,
             terminal_block_hash_activation_epoch,
+            safe_slots_to_import_optimistically,
             min_genesis_active_validator_count,
             min_genesis_time,
             genesis_fork_version,
@@ -1040,6 +1052,7 @@ impl Config {
             terminal_total_difficulty,
             terminal_block_hash,
             terminal_block_hash_activation_epoch,
+            safe_slots_to_import_optimistically,
             ..chain_spec.clone()
         })
     }
@@ -1227,6 +1240,7 @@ mod yaml_tests {
         #TERMINAL_TOTAL_DIFFICULTY: 115792089237316195423570985008687907853269984665640564039457584007913129638911
         #TERMINAL_BLOCK_HASH: 0x0000000000000000000000000000000000000000000000000000000000000001
         #TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH: 18446744073709551614
+        #SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY: 2
         MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 16384
         MIN_GENESIS_TIME: 1606824000
         GENESIS_FORK_VERSION: 0x00000000
@@ -1266,6 +1280,10 @@ mod yaml_tests {
             chain_spec.terminal_block_hash_activation_epoch,
             default_terminal_block_hash_activation_epoch()
         );
+        assert_eq!(
+            chain_spec.safe_slots_to_import_optimistically,
+            default_safe_slots_to_import_optimistically()
+        );
 
         assert_eq!(
             chain_spec.bellatrix_fork_epoch,