diff --git a/Cargo.lock b/Cargo.lock index b11c5ce4e1..77e3fc8227 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1275,6 +1275,7 @@ dependencies = [ name = "ef_tests" version = "0.2.0" dependencies = [ + "beacon_chain", "bls", "cached_tree_hash", "compare_fields", @@ -1292,6 +1293,7 @@ dependencies = [ "serde_yaml", "snap", "state_processing", + "store", "swap_or_not_shuffle", "tree_hash", "tree_hash_derive", diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index eb29e8d2f3..c672ff6be6 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1203,7 +1203,7 @@ type CommitteesPerSlot = u64; /// Returns the `indexed_attestation` and committee count per slot for the `attestation` using the /// public keys cached in the `chain`. -fn obtain_indexed_attestation_and_committees_per_slot( +pub fn obtain_indexed_attestation_and_committees_per_slot( chain: &BeaconChain, attestation: &Attestation, ) -> Result<(IndexedAttestation, CommitteesPerSlot), Error> { diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1ae4f42a2b..5f8b70bf44 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2407,7 +2407,7 @@ impl BeaconChain { let _fork_choice_block_timer = metrics::start_timer(&metrics::FORK_CHOICE_PROCESS_BLOCK_TIMES); fork_choice - .on_block(current_slot, &block, block_root, &state) + .on_block(current_slot, &block, block_root, &state, &self.spec) .map_err(|e| BlockError::BeaconChainError(e.into()))?; } diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index 31678580a0..8d0545c58c 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -166,7 +166,7 @@ pub fn reset_fork_choice_to_finalization, Cold: It let (block, _) = block.deconstruct(); fork_choice - .on_block(block.slot(), &block, block.canonical_root(), &state) + .on_block(block.slot(), &block, block.canonical_root(), &state, spec) .map_err(|e| format!("Error applying replayed block to fork choice: {:?}", e))?; } diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 8e70e72ba0..2cd636f23b 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -36,7 +36,7 @@ mod validator_pubkey_cache; pub use self::beacon_chain::{ AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult, - ForkChoiceError, StateSkipConfig, WhenSlotSkipped, MAXIMUM_GOSSIP_CLOCK_DISPARITY, + ForkChoiceError, HeadInfo, StateSkipConfig, WhenSlotSkipped, MAXIMUM_GOSSIP_CLOCK_DISPARITY, }; pub use self::beacon_snapshot::BeaconSnapshot; pub use self::chain_config::ChainConfig; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 536df8f58e..d407d83542 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -46,8 +46,6 @@ use types::{ // 4th September 2019 pub const HARNESS_GENESIS_TIME: u64 = 1_567_552_690; -// This parameter is required by a builder but not used because we use the `TestingSlotClock`. -pub const HARNESS_SLOT_TIME: Duration = Duration::from_secs(1); // Environment variable to read if `fork_from_env` feature is enabled. const FORK_NAME_ENV_VAR: &str = "FORK_NAME"; @@ -182,6 +180,27 @@ impl Builder> { self.store = Some(store); self.store_mutator(Box::new(mutator)) } + + /// Create a new ephemeral store that uses the specified `genesis_state`. + pub fn genesis_state_ephemeral_store(mut self, genesis_state: BeaconState) -> Self { + let spec = self.spec.as_ref().expect("cannot build without spec"); + + let store = Arc::new( + HotColdDB::open_ephemeral( + self.store_config.clone().unwrap_or_default(), + spec.clone(), + self.log.clone(), + ) + .unwrap(), + ); + let mutator = move |builder: BeaconChainBuilder<_>| { + builder + .genesis_state(genesis_state) + .expect("should build state using recent genesis") + }; + self.store = Some(store); + self.store_mutator(Box::new(mutator)) + } } impl Builder> { @@ -297,6 +316,7 @@ where let log = test_logger(); let spec = self.spec.expect("cannot build without spec"); + let seconds_per_slot = spec.seconds_per_slot; let validator_keypairs = self .validator_keypairs .expect("cannot build without validator keypairs"); @@ -331,7 +351,7 @@ where // Initialize the slot clock only if it hasn't already been initialized. builder = if builder.get_slot_clock().is_none() { builder - .testing_slot_clock(HARNESS_SLOT_TIME) + .testing_slot_clock(Duration::from_secs(seconds_per_slot)) .expect("should configure testing slot clock") } else { builder diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 8a2412b2fa..f9af16bbe7 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -4,7 +4,6 @@ use beacon_chain::attestation_verification::Error as AttnError; use beacon_chain::builder::BeaconChainBuilder; use beacon_chain::test_utils::{ test_spec, AttestationStrategy, BeaconChainHarness, BlockStrategy, DiskHarnessType, - HARNESS_SLOT_TIME, }; use beacon_chain::{ historical_blocks::HistoricalBlockError, migrate::MigratorConfig, BeaconChain, @@ -19,6 +18,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::convert::TryInto; use std::sync::Arc; +use std::time::Duration; use store::{ iter::{BlockRootsIterator, StateRootsIterator}, HotColdDB, LevelDB, StoreConfig, @@ -1812,6 +1812,8 @@ fn weak_subjectivity_sync() { let log = test_logger(); let temp2 = tempdir().unwrap(); let store = get_store(&temp2); + let spec = test_spec::(); + let seconds_per_slot = spec.seconds_per_slot; // Initialise a new beacon chain from the finalized checkpoint let beacon_chain = BeaconChainBuilder::new(MinimalEthSpec) @@ -1823,7 +1825,7 @@ fn weak_subjectivity_sync() { .store_migrator_config(MigratorConfig::default().blocking()) .dummy_eth1_backend() .expect("should build dummy backend") - .testing_slot_clock(HARNESS_SLOT_TIME) + .testing_slot_clock(Duration::from_secs(seconds_per_slot)) .expect("should configure testing slot clock") .shutdown_sender(shutdown_tx) .chain_config(ChainConfig::default()) @@ -2055,6 +2057,8 @@ fn revert_minority_fork_on_resume() { let mut spec2 = MinimalEthSpec::default_spec(); spec2.altair_fork_epoch = Some(fork_epoch); + let seconds_per_slot = spec1.seconds_per_slot; + let all_validators = (0..validator_count).collect::>(); // Chain with no fork epoch configured. @@ -2160,7 +2164,7 @@ fn revert_minority_fork_on_resume() { builder = builder .resume_from_db() .unwrap() - .testing_slot_clock(HARNESS_SLOT_TIME) + .testing_slot_clock(Duration::from_secs(seconds_per_slot)) .unwrap(); builder .get_slot_clock() diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a943e11e01..d0aa8abc1d 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -3,18 +3,13 @@ use std::marker::PhantomData; use proto_array::{Block as ProtoBlock, ProtoArrayForkChoice}; use ssz_derive::{Decode, Encode}; use types::{ - AttestationShufflingId, BeaconBlock, BeaconState, BeaconStateError, Checkpoint, Epoch, EthSpec, - Hash256, IndexedAttestation, RelativeEpoch, SignedBeaconBlock, Slot, + AttestationShufflingId, BeaconBlock, BeaconState, BeaconStateError, ChainSpec, Checkpoint, + Epoch, EthSpec, Hash256, IndexedAttestation, RelativeEpoch, SignedBeaconBlock, Slot, }; use crate::ForkChoiceStore; use std::cmp::Ordering; -/// Defined here: -/// -/// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#configuration -pub const SAFE_SLOTS_TO_UPDATE_JUSTIFIED: u64 = 8; - #[derive(Debug)] pub enum Error { InvalidAttestation(InvalidAttestation), @@ -379,13 +374,14 @@ where &mut self, current_slot: Slot, state: &BeaconState, + spec: &ChainSpec, ) -> Result> { self.update_time(current_slot)?; let new_justified_checkpoint = &state.current_justified_checkpoint(); if compute_slots_since_epoch_start::(self.fc_store.get_current_slot()) - < SAFE_SLOTS_TO_UPDATE_JUSTIFIED + < spec.safe_slots_to_update_justified { return Ok(true); } @@ -442,6 +438,7 @@ where block: &BeaconBlock, block_root: Hash256, state: &BeaconState, + spec: &ChainSpec, ) -> Result<(), Error> { let current_slot = self.update_time(current_slot)?; @@ -500,7 +497,7 @@ where self.fc_store .set_best_justified_checkpoint(state.current_justified_checkpoint()); } - if self.should_update_justified_checkpoint(current_slot, state)? { + if self.should_update_justified_checkpoint(current_slot, state, spec)? { self.fc_store .set_justified_checkpoint(state.current_justified_checkpoint()) .map_err(Error::UnableToSetJustifiedCheckpoint)?; @@ -797,6 +794,21 @@ where *self.fc_store.finalized_checkpoint() } + /// Return the justified checkpoint. + pub fn justified_checkpoint(&self) -> Checkpoint { + *self.fc_store.justified_checkpoint() + } + + /// Return the best justified checkpoint. + /// + /// ## Warning + /// + /// This is distinct to the "justified checkpoint" or the "current justified checkpoint". This + /// "best justified checkpoint" value should only be used internally or for testing. + pub fn best_justified_checkpoint(&self) -> Checkpoint { + *self.fc_store.best_justified_checkpoint() + } + /// Returns the latest message for a given validator, if any. /// /// Returns `(block_root, block_slot)`. diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 7b508afd49..5e9deac3b5 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -3,7 +3,6 @@ mod fork_choice_store; pub use crate::fork_choice::{ Error, ForkChoice, InvalidAttestation, InvalidBlock, PersistedForkChoice, QueuedAttestation, - SAFE_SLOTS_TO_UPDATE_JUSTIFIED, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::Block as ProtoBlock; diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 068b6387f0..2c0d498e19 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -10,14 +10,12 @@ use beacon_chain::{ BeaconChain, BeaconChainError, BeaconForkChoiceStore, ChainConfig, ForkChoiceError, StateSkipConfig, WhenSlotSkipped, }; -use fork_choice::{ - ForkChoiceStore, InvalidAttestation, InvalidBlock, QueuedAttestation, - SAFE_SLOTS_TO_UPDATE_JUSTIFIED, -}; +use fork_choice::{ForkChoiceStore, InvalidAttestation, InvalidBlock, QueuedAttestation}; use store::MemoryStore; use types::{ test_utils::generate_deterministic_keypair, BeaconBlock, BeaconBlockRef, BeaconState, - Checkpoint, Epoch, EthSpec, Hash256, IndexedAttestation, MainnetEthSpec, Slot, SubnetId, + ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, IndexedAttestation, MainnetEthSpec, Slot, + SubnetId, }; pub type E = MainnetEthSpec; @@ -232,7 +230,7 @@ impl ForkChoiceTest { /// Moves to the next slot that is *outside* the `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` range. pub fn move_outside_safe_to_update(self) -> Self { - while is_safe_to_update(self.harness.chain.slot().unwrap()) { + while is_safe_to_update(self.harness.chain.slot().unwrap(), &self.harness.chain.spec) { self.harness.advance_slot() } self @@ -240,7 +238,7 @@ impl ForkChoiceTest { /// Moves to the next slot that is *inside* the `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` range. pub fn move_inside_safe_to_update(self) -> Self { - while !is_safe_to_update(self.harness.chain.slot().unwrap()) { + while !is_safe_to_update(self.harness.chain.slot().unwrap(), &self.harness.chain.spec) { self.harness.advance_slot() } self @@ -270,7 +268,13 @@ impl ForkChoiceTest { .chain .fork_choice .write() - .on_block(current_slot, &block, block.canonical_root(), &state) + .on_block( + current_slot, + &block, + block.canonical_root(), + &state, + &self.harness.chain.spec, + ) .unwrap(); self } @@ -305,7 +309,13 @@ impl ForkChoiceTest { .chain .fork_choice .write() - .on_block(current_slot, &block, block.canonical_root(), &state) + .on_block( + current_slot, + &block, + block.canonical_root(), + &state, + &self.harness.chain.spec, + ) .err() .expect("on_block did not return an error"); comparison_func(err); @@ -458,8 +468,8 @@ impl ForkChoiceTest { } } -fn is_safe_to_update(slot: Slot) -> bool { - slot % E::slots_per_epoch() < SAFE_SLOTS_TO_UPDATE_JUSTIFIED +fn is_safe_to_update(slot: Slot, spec: &ChainSpec) -> bool { + slot % E::slots_per_epoch() < spec.safe_slots_to_update_justified } /// - The new justified checkpoint descends from the current. diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index ee05f40713..e1668a9b49 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -32,3 +32,5 @@ swap_or_not_shuffle = { path = "../../consensus/swap_or_not_shuffle" } types = { path = "../../consensus/types" } snap = "1.0.1" fs2 = "0.4.3" +beacon_chain = { path = "../../beacon_node/beacon_chain" } +store = { path = "../../beacon_node/store" } diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 5e119a86df..a7149c1a59 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -38,11 +38,6 @@ excluded_paths = [ # LightClientSnapshot "tests/minimal/altair/ssz_static/LightClientSnapshot", "tests/mainnet/altair/ssz_static/LightClientSnapshot", - # Fork choice - "tests/mainnet/phase0/fork_choice", - "tests/minimal/phase0/fork_choice", - "tests/mainnet/altair/fork_choice", - "tests/minimal/altair/fork_choice", # Merkle-proof tests for light clients "tests/mainnet/altair/merkle/single_proof/pyspec_tests/", "tests/minimal/altair/merkle/single_proof/pyspec_tests/" diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index b3934911e9..e290421762 100644 --- a/testing/ef_tests/src/cases.rs +++ b/testing/ef_tests/src/cases.rs @@ -14,6 +14,7 @@ mod bls_verify_msg; mod common; mod epoch_processing; mod fork; +mod fork_choice; mod genesis_initialization; mod genesis_validity; mod operations; @@ -35,6 +36,7 @@ pub use bls_verify_msg::*; pub use common::SszStaticType; pub use epoch_processing::*; pub use fork::ForkTest; +pub use fork_choice::*; pub use genesis_initialization::*; pub use genesis_validity::*; pub use operations::*; diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs new file mode 100644 index 0000000000..7d7b21da13 --- /dev/null +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -0,0 +1,498 @@ +use super::*; +use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; +use beacon_chain::{ + attestation_verification::{ + obtain_indexed_attestation_and_committees_per_slot, VerifiedAttestation, + }, + test_utils::{BeaconChainHarness, EphemeralHarnessType}, + BeaconChainTypes, HeadInfo, +}; +use serde_derive::Deserialize; +use state_processing::state_advance::complete_state_advance; +use std::time::Duration; +use types::{ + Attestation, BeaconBlock, BeaconState, Checkpoint, Epoch, EthSpec, ForkName, Hash256, + IndexedAttestation, SignedBeaconBlock, Slot, +}; + +#[derive(Debug, Clone, Copy, PartialEq, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Head { + slot: Slot, + root: Hash256, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Checks { + head: Option, + time: Option, + genesis_time: Option, + justified_checkpoint: Option, + justified_checkpoint_root: Option, + finalized_checkpoint: Option, + best_justified_checkpoint: Option, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(untagged, deny_unknown_fields)] +pub enum Step { + Tick { tick: u64 }, + ValidBlock { block: B }, + MaybeValidBlock { block: B, valid: bool }, + Attestation { attestation: A }, + Checks { checks: Box }, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Meta { + description: String, +} + +#[derive(Debug)] +pub struct ForkChoiceTest { + pub description: String, + pub anchor_state: BeaconState, + pub anchor_block: BeaconBlock, + pub steps: Vec, Attestation>>, +} + +impl LoadCase for ForkChoiceTest { + fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { + let description = path + .iter() + .last() + .expect("path must be non-empty") + .to_str() + .expect("path must be valid OsStr") + .to_string(); + let spec = &testing_spec::(fork_name); + let steps: Vec> = yaml_decode_file(&path.join("steps.yaml"))?; + // Resolve the object names in `steps.yaml` into actual decoded block/attestation objects. + let steps = steps + .into_iter() + .map(|step| match step { + Step::Tick { tick } => Ok(Step::Tick { tick }), + Step::ValidBlock { block } => { + ssz_decode_file_with(&path.join(format!("{}.ssz_snappy", block)), |bytes| { + SignedBeaconBlock::from_ssz_bytes(bytes, spec) + }) + .map(|block| Step::ValidBlock { block }) + } + Step::MaybeValidBlock { block, valid } => { + ssz_decode_file_with(&path.join(format!("{}.ssz_snappy", block)), |bytes| { + SignedBeaconBlock::from_ssz_bytes(bytes, spec) + }) + .map(|block| Step::MaybeValidBlock { block, valid }) + } + Step::Attestation { attestation } => { + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))) + .map(|attestation| Step::Attestation { attestation }) + } + Step::Checks { checks } => Ok(Step::Checks { checks }), + }) + .collect::>()?; + let anchor_state = ssz_decode_state(&path.join("anchor_state.ssz_snappy"), spec)?; + let anchor_block = ssz_decode_file_with(&path.join("anchor_block.ssz_snappy"), |bytes| { + BeaconBlock::from_ssz_bytes(bytes, spec) + })?; + + // None of the tests have a `meta.yaml` file, except the altair/genesis tests. For those + // tests, the meta file has an irrelevant comment as the `description` field. If the meta + // file is present, we parse it for two reasons: + // + // - To satisfy the `check_all_files_accessed.py` check. + // - To ensure that the `meta.yaml` only contains a description field and nothing else that + // might be useful. + let meta_path = path.join("meta.yaml"); + if meta_path.exists() { + let _meta: Meta = yaml_decode_file(&meta_path)?; + } + + Ok(Self { + description, + anchor_state, + anchor_block, + steps, + }) + } +} + +impl Case for ForkChoiceTest { + fn description(&self) -> String { + self.description.clone() + } + + fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { + let tester = Tester::new(self, testing_spec::(fork_name))?; + + // The reason for this failure is documented here: + // + // https://github.com/sigp/lighthouse/issues/2741 + // + // We should eventually solve the above issue and remove this `SkippedKnownFailure`. + if self.description == "new_finalized_slot_is_justified_checkpoint_ancestor" { + return Err(Error::SkippedKnownFailure); + }; + + for step in &self.steps { + match step { + Step::Tick { tick } => tester.set_tick(*tick), + Step::ValidBlock { block } => tester.process_block(block.clone(), true)?, + Step::MaybeValidBlock { block, valid } => { + tester.process_block(block.clone(), *valid)? + } + Step::Attestation { attestation } => tester.process_attestation(attestation)?, + Step::Checks { checks } => { + let Checks { + head, + time, + genesis_time, + justified_checkpoint, + justified_checkpoint_root, + finalized_checkpoint, + best_justified_checkpoint, + } = checks.as_ref(); + + if let Some(expected_head) = head { + tester.check_head(*expected_head)?; + } + + if let Some(expected_time) = time { + tester.check_time(*expected_time)?; + } + + if let Some(expected_genesis_time) = genesis_time { + tester.check_genesis_time(*expected_genesis_time)?; + } + + if let Some(expected_justified_checkpoint) = justified_checkpoint { + tester.check_justified_checkpoint(*expected_justified_checkpoint)?; + } + + if let Some(expected_justified_checkpoint_root) = justified_checkpoint_root { + tester + .check_justified_checkpoint_root(*expected_justified_checkpoint_root)?; + } + + if let Some(expected_finalized_checkpoint) = finalized_checkpoint { + tester.check_finalized_checkpoint(*expected_finalized_checkpoint)?; + } + + if let Some(expected_best_justified_checkpoint) = best_justified_checkpoint { + tester + .check_best_justified_checkpoint(*expected_best_justified_checkpoint)?; + } + } + } + } + + Ok(()) + } +} + +/// A testing rig used to execute a test case. +struct Tester { + harness: BeaconChainHarness>, + spec: ChainSpec, +} + +impl Tester { + pub fn new(case: &ForkChoiceTest, spec: ChainSpec) -> Result { + let genesis_time = case.anchor_state.genesis_time(); + + if case.anchor_state.slot() != spec.genesis_slot { + // I would hope that future fork-choice tests would start from a non-genesis anchors, + // however at the time of writing, none do. I think it would be quite easy to do + // non-genesis anchors via a weak-subjectivity/checkpoint start. + // + // Whilst those tests don't exist, we'll avoid adding checkpoint start complexity to the + // `BeaconChainHarness` and create a hard failure so we can deal with it then. + return Err(Error::FailedToParseTest( + "anchor state is not a genesis state".into(), + )); + } + + let harness = BeaconChainHarness::builder(E::default()) + .spec(spec.clone()) + .keypairs(vec![]) + .genesis_state_ephemeral_store(case.anchor_state.clone()) + .build(); + + if harness.chain.genesis_block_root != case.anchor_block.canonical_root() { + // This check will need to be removed if/when the fork-choice tests use a non-genesis + // anchor state. + return Err(Error::FailedToParseTest( + "anchor block differs from locally-generated genesis block".into(), + )); + } + + assert_eq!( + harness.chain.slot_clock.genesis_duration().as_secs(), + genesis_time + ); + + Ok(Self { harness, spec }) + } + + fn tick_to_slot(&self, tick: u64) -> Result { + let genesis_time = self.harness.chain.slot_clock.genesis_duration().as_secs(); + let since_genesis = tick + .checked_sub(genesis_time) + .ok_or_else(|| Error::FailedToParseTest("tick is prior to genesis".into()))?; + let slots_since_genesis = since_genesis / self.spec.seconds_per_slot; + Ok(self.spec.genesis_slot + slots_since_genesis) + } + + fn find_head(&self) -> Result { + self.harness + .chain + .fork_choice() + .map_err(|e| Error::InternalError(format!("failed to find head with {:?}", e)))?; + self.harness + .chain + .head_info() + .map_err(|e| Error::InternalError(format!("failed to read head with {:?}", e))) + } + + fn genesis_epoch(&self) -> Epoch { + self.spec.genesis_slot.epoch(E::slots_per_epoch()) + } + + pub fn set_tick(&self, tick: u64) { + self.harness + .chain + .slot_clock + .set_current_time(Duration::from_secs(tick)); + + // Compute the slot time manually to ensure the slot clock is correct. + let slot = self.tick_to_slot(tick).unwrap(); + assert_eq!(slot, self.harness.chain.slot().unwrap()); + + self.harness + .chain + .fork_choice + .write() + .update_time(slot) + .unwrap(); + } + + pub fn process_block(&self, block: SignedBeaconBlock, valid: bool) -> Result<(), Error> { + let result = self.harness.chain.process_block(block.clone()); + let block_root = block.canonical_root(); + if result.is_ok() != valid { + return Err(Error::DidntFail(format!( + "block with root {} was valid={} whilst test expects valid={}", + block_root, + result.is_ok(), + valid + ))); + } + + // Apply invalid blocks directly against the fork choice `on_block` function. This ensures + // that the block is being rejected by `on_block`, not just some upstream block processing + // function. + if !valid { + // A missing parent block whilst `valid == false` means the test should pass. + if let Some(parent_block) = self.harness.chain.get_block(&block.parent_root()).unwrap() + { + let parent_state_root = parent_block.state_root(); + let mut state = self + .harness + .chain + .get_state(&parent_state_root, Some(parent_block.slot())) + .unwrap() + .unwrap(); + + complete_state_advance( + &mut state, + Some(parent_state_root), + block.slot(), + &self.harness.chain.spec, + ) + .unwrap(); + + let (block, _) = block.deconstruct(); + let result = self.harness.chain.fork_choice.write().on_block( + self.harness.chain.slot().unwrap(), + &block, + block_root, + &state, + &self.harness.chain.spec, + ); + + if result.is_ok() { + return Err(Error::DidntFail(format!( + "block with root {} should fail on_block", + block_root, + ))); + } + } + } + + Ok(()) + } + + pub fn process_attestation(&self, attestation: &Attestation) -> Result<(), Error> { + let (indexed_attestation, _) = + obtain_indexed_attestation_and_committees_per_slot(&self.harness.chain, attestation) + .map_err(|e| { + Error::InternalError(format!("attestation indexing failed with {:?}", e)) + })?; + let verified_attestation: ManuallyVerifiedAttestation> = + ManuallyVerifiedAttestation { + attestation, + indexed_attestation, + }; + + self.harness + .chain + .apply_attestation_to_fork_choice(&verified_attestation) + .map_err(|e| Error::InternalError(format!("attestation import failed with {:?}", e))) + } + + pub fn check_head(&self, expected_head: Head) -> Result<(), Error> { + let chain_head = self.find_head().map(|head| Head { + slot: head.slot, + root: head.block_root, + })?; + + check_equal("head", chain_head, expected_head) + } + + pub fn check_time(&self, expected_time: u64) -> Result<(), Error> { + let slot = self.harness.chain.slot().map_err(|e| { + Error::InternalError(format!("reading current slot failed with {:?}", e)) + })?; + let expected_slot = self.tick_to_slot(expected_time)?; + check_equal("time", slot, expected_slot) + } + + pub fn check_genesis_time(&self, expected_genesis_time: u64) -> Result<(), Error> { + let genesis_time = self.harness.chain.slot_clock.genesis_duration().as_secs(); + check_equal("genesis_time", genesis_time, expected_genesis_time) + } + + pub fn check_justified_checkpoint(&self, expected_checkpoint: Checkpoint) -> Result<(), Error> { + let head_checkpoint = self.find_head()?.current_justified_checkpoint; + let fc_checkpoint = self.harness.chain.fork_choice.read().justified_checkpoint(); + + assert_checkpoints_eq( + "justified_checkpoint", + self.genesis_epoch(), + head_checkpoint, + fc_checkpoint, + ); + + check_equal("justified_checkpoint", fc_checkpoint, expected_checkpoint) + } + + pub fn check_justified_checkpoint_root( + &self, + expected_checkpoint_root: Hash256, + ) -> Result<(), Error> { + let head_checkpoint = self.find_head()?.current_justified_checkpoint; + let fc_checkpoint = self.harness.chain.fork_choice.read().justified_checkpoint(); + + assert_checkpoints_eq( + "justified_checkpoint_root", + self.genesis_epoch(), + head_checkpoint, + fc_checkpoint, + ); + + check_equal( + "justified_checkpoint_root", + fc_checkpoint.root, + expected_checkpoint_root, + ) + } + + pub fn check_finalized_checkpoint(&self, expected_checkpoint: Checkpoint) -> Result<(), Error> { + let head_checkpoint = self.find_head()?.finalized_checkpoint; + let fc_checkpoint = self.harness.chain.fork_choice.read().finalized_checkpoint(); + + assert_checkpoints_eq( + "finalized_checkpoint", + self.genesis_epoch(), + head_checkpoint, + fc_checkpoint, + ); + + check_equal("finalized_checkpoint", fc_checkpoint, expected_checkpoint) + } + + pub fn check_best_justified_checkpoint( + &self, + expected_checkpoint: Checkpoint, + ) -> Result<(), Error> { + let best_justified_checkpoint = self + .harness + .chain + .fork_choice + .read() + .best_justified_checkpoint(); + check_equal( + "best_justified_checkpoint", + best_justified_checkpoint, + expected_checkpoint, + ) + } +} + +/// Checks that the `head` checkpoint from the beacon chain head matches the `fc` checkpoint gleaned +/// directly from fork choice. +/// +/// This function is necessary due to a quirk documented in this issue: +/// +/// https://github.com/ethereum/consensus-specs/issues/2566 +fn assert_checkpoints_eq(name: &str, genesis_epoch: Epoch, head: Checkpoint, fc: Checkpoint) { + if fc.epoch == genesis_epoch { + assert_eq!( + head, + Checkpoint { + epoch: genesis_epoch, + root: Hash256::zero() + }, + "{} (genesis)", + name + ) + } else { + assert_eq!(head, fc, "{} (non-genesis)", name) + } +} + +/// Convenience function to create `Error` messages. +fn check_equal(check: &str, result: T, expected: T) -> Result<(), Error> { + if result == expected { + Ok(()) + } else { + Err(Error::NotEqual(format!( + "{} check failed: Got {:?} | Expected {:?}", + check, result, expected + ))) + } +} + +/// An attestation that is not verified in the `BeaconChain` sense, but verified-enough for these +/// tests. +/// +/// The `BeaconChain` verification is not appropriate since these tests use `Attestation`s with +/// multiple participating validators. Therefore, they are neither aggregated or unaggregated +/// attestations. +pub struct ManuallyVerifiedAttestation<'a, T: BeaconChainTypes> { + #[allow(dead_code)] + attestation: &'a Attestation, + indexed_attestation: IndexedAttestation, +} + +impl<'a, T: BeaconChainTypes> VerifiedAttestation for ManuallyVerifiedAttestation<'a, T> { + fn attestation(&self) -> &Attestation { + self.attestation + } + + fn indexed_attestation(&self) -> &IndexedAttestation { + &self.indexed_attestation + } +} diff --git a/testing/ef_tests/src/error.rs b/testing/ef_tests/src/error.rs index 1d1844558b..c5795777ad 100644 --- a/testing/ef_tests/src/error.rs +++ b/testing/ef_tests/src/error.rs @@ -12,6 +12,8 @@ pub enum Error { SkippedBls, /// Skipped the test because it's known to fail. SkippedKnownFailure, + /// The test failed due to some internal error preventing the test from running. + InternalError(String), } impl Error { @@ -23,6 +25,7 @@ impl Error { Error::InvalidBLSInput(_) => "InvalidBLSInput", Error::SkippedBls => "SkippedBls", Error::SkippedKnownFailure => "SkippedKnownFailure", + Error::InternalError(_) => "InternalError", } } @@ -32,6 +35,7 @@ impl Error { Error::DidntFail(m) => m.as_str(), Error::FailedToParseTest(m) => m.as_str(), Error::InvalidBLSInput(m) => m.as_str(), + Error::InternalError(m) => m.as_str(), _ => self.name(), } } diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 6430d98318..11bda8f9f3 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -421,6 +421,58 @@ impl Handler for FinalityHandler { } } +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct ForkChoiceGetHeadHandler(PhantomData); + +impl Handler for ForkChoiceGetHeadHandler { + type Case = cases::ForkChoiceTest; + + fn config_name() -> &'static str { + E::name() + } + + fn runner_name() -> &'static str { + "fork_choice" + } + + fn handler_name(&self) -> String { + "get_head".into() + } + + fn is_enabled_for_fork(&self, _fork_name: ForkName) -> bool { + // These tests check block validity (which may include signatures) and there is no need to + // run them with fake crypto. + cfg!(not(feature = "fake_crypto")) + } +} + +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct ForkChoiceOnBlockHandler(PhantomData); + +impl Handler for ForkChoiceOnBlockHandler { + type Case = cases::ForkChoiceTest; + + fn config_name() -> &'static str { + E::name() + } + + fn runner_name() -> &'static str { + "fork_choice" + } + + fn handler_name(&self) -> String { + "on_block".into() + } + + fn is_enabled_for_fork(&self, _fork_name: ForkName) -> bool { + // These tests check block validity (which may include signatures) and there is no need to + // run them with fake crypto. + cfg!(not(feature = "fake_crypto")) + } +} + #[derive(Derivative)] #[derivative(Default(bound = ""))] pub struct GenesisValidityHandler(PhantomData); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index e41da6b849..25a4618558 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -386,6 +386,18 @@ fn finality() { FinalityHandler::::default().run(); } +#[test] +fn fork_choice_get_head() { + ForkChoiceGetHeadHandler::::default().run(); + ForkChoiceGetHeadHandler::::default().run(); +} + +#[test] +fn fork_choice_on_block() { + ForkChoiceOnBlockHandler::::default().run(); + ForkChoiceOnBlockHandler::::default().run(); +} + #[test] fn genesis_initialization() { GenesisInitializationHandler::::default().run();