From 9266c55c34f5fd3019c26bb0931ea6eee8a8abf2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 19 Oct 2021 14:32:24 +1100 Subject: [PATCH 01/24] Add parsing for test format --- testing/ef_tests/src/cases.rs | 2 + testing/ef_tests/src/cases/fork_choice.rs | 70 +++++++++++++++++++++++ testing/ef_tests/src/handler.rs | 42 ++++++++++++++ testing/ef_tests/tests/tests.rs | 12 ++++ 4 files changed, 126 insertions(+) create mode 100644 testing/ef_tests/src/cases/fork_choice.rs diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index b3934911e92..e2904217628 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 00000000000..515317840e6 --- /dev/null +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -0,0 +1,70 @@ +use super::*; +use crate::decode::{ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; +use serde_derive::Deserialize; +use types::{BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, Slot}; + +#[derive(Debug, Clone, Deserialize)] +#[serde(rename_all = "snake_case")] +pub struct Head { + slot: Slot, + root: Hash256, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct Checks { + head: Option, + time: Option, + genesis_time: Option, + justified_checkpoint: Option, + finalized_checkpoint: Option, + best_justified_checkpoint: Option, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(untagged, rename_all = "snake_case")] +pub enum Step { + Tick { tick: u64 }, + ValidBlock { block: String }, + MaybeValidBlock { block: String, valid: bool }, + Attestation { attestation: String }, + Checks(Checks), +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(bound = "E: EthSpec")] +pub struct ForkChoiceTest { + pub description: String, + pub anchor_state: BeaconState, + pub anchor_block: BeaconBlock, + pub steps: Vec, +} + +impl LoadCase for ForkChoiceTest { + fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { + let description = format!("{:?}", path); + let spec = &testing_spec::(fork_name); + let steps = yaml_decode_file(&path.join("steps.yaml"))?; + 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) + })?; + + 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> { + // TODO(paul): run tests + Ok(()) + } +} diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 6430d983183..940be86a51a 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -421,6 +421,48 @@ impl Handler for FinalityHandler { } } +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct ForkChoiceGetHeadHandler(PhantomData); + +impl Handler for ForkChoiceGetHeadHandler { + // Reuse the blocks case runner. + 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() + } +} + +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct ForkChoiceOnBlockHandler(PhantomData); + +impl Handler for ForkChoiceOnBlockHandler { + // Reuse the blocks case runner. + 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() + } +} + #[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 e41da6b8499..25a46185580 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(); From d19c6432c0ab015037fd7ff869909461b5011060 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 19 Oct 2021 14:32:54 +1100 Subject: [PATCH 02/24] Remove fork choice from ignored tests --- testing/ef_tests/check_all_files_accessed.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 5e119a86df1..a7149c1a594 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -38,11 +38,6 @@ # 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/" From a5f936c03199a7593a1172313efcdba4e061ca67 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 19 Oct 2021 14:44:07 +1100 Subject: [PATCH 03/24] Decode blocks and attestations --- testing/ef_tests/src/cases/fork_choice.rs | 47 +++++++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 515317840e6..95968a73792 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -1,7 +1,10 @@ use super::*; -use crate::decode::{ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; +use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; use serde_derive::Deserialize; -use types::{BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, Slot}; +use types::{ + Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, + SignedBeaconBlock, Slot, +}; #[derive(Debug, Clone, Deserialize)] #[serde(rename_all = "snake_case")] @@ -22,11 +25,11 @@ pub struct Checks { #[derive(Debug, Clone, Deserialize)] #[serde(untagged, rename_all = "snake_case")] -pub enum Step { +pub enum Step { Tick { tick: u64 }, - ValidBlock { block: String }, - MaybeValidBlock { block: String, valid: bool }, - Attestation { attestation: String }, + ValidBlock { block: B }, + MaybeValidBlock { block: B, valid: bool }, + Attestation { attestation: A }, Checks(Checks), } @@ -36,14 +39,42 @@ pub struct ForkChoiceTest { pub description: String, pub anchor_state: BeaconState, pub anchor_block: BeaconBlock, - pub steps: Vec, + pub steps: Vec, Attestation>>, } impl LoadCase for ForkChoiceTest { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { let description = format!("{:?}", path); let spec = &testing_spec::(fork_name); - let steps = yaml_decode_file(&path.join("steps.yaml"))?; + let steps: Vec> = yaml_decode_file(&path.join("steps.yaml"))?; + // This somewhat awkward step resolves 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 } => { + let block = ssz_decode_file_with( + &path.join(format!("{}.ssz_snappy", block)), + |bytes| SignedBeaconBlock::from_ssz_bytes(bytes, spec), + )?; + Ok(Step::ValidBlock { block }) + } + Step::MaybeValidBlock { block, valid } => { + let block = ssz_decode_file_with( + &path.join(format!("{}.ssz_snappy", block)), + |bytes| SignedBeaconBlock::from_ssz_bytes(bytes, spec), + )?; + Ok(Step::MaybeValidBlock { block, valid }) + } + Step::Attestation { attestation } => { + let attestation = + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation)))?; + Ok(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) From 6f42d925b3e9124084463ca021128f009354a12d Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 11:01:15 +1100 Subject: [PATCH 04/24] Fix decoding --- testing/ef_tests/src/cases/fork_choice.rs | 39 +++++++++++------------ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 95968a73792..ff1a784cf5b 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -1,36 +1,39 @@ use super::*; use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; +use beacon_chain::test_utils::BeaconChainHarness; use serde_derive::Deserialize; use types::{ - Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, + Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, Signature, SignedBeaconBlock, Slot, }; #[derive(Debug, Clone, Deserialize)] -#[serde(rename_all = "snake_case")] +#[serde(deny_unknown_fields, rename_all = "snake_case")] 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, rename_all = "snake_case")] +#[serde(untagged, deny_unknown_fields)] pub enum Step { Tick { tick: u64 }, ValidBlock { block: B }, MaybeValidBlock { block: B, valid: bool }, Attestation { attestation: A }, - Checks(Checks), + Checks { checks: Checks }, } #[derive(Debug, Clone, Deserialize)] @@ -47,32 +50,28 @@ impl LoadCase for ForkChoiceTest { let description = format!("{:?}", path); let spec = &testing_spec::(fork_name); let steps: Vec> = yaml_decode_file(&path.join("steps.yaml"))?; - // This somewhat awkward step resolves the object names in `steps.yaml` into actual decoded - // block/attestation objects. + // 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 } => { - let block = ssz_decode_file_with( - &path.join(format!("{}.ssz_snappy", block)), - |bytes| SignedBeaconBlock::from_ssz_bytes(bytes, spec), - )?; - Ok(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 } => { - let block = ssz_decode_file_with( - &path.join(format!("{}.ssz_snappy", block)), - |bytes| SignedBeaconBlock::from_ssz_bytes(bytes, spec), - )?; - Ok(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 } => { - let attestation = - ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation)))?; - Ok(Step::Attestation { attestation }) + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))) + .map(|attestation| Step::Attestation { attestation }) } - Step::Checks(checks) => Ok(Step::Checks(checks)), + Step::Checks { checks } => Ok(Step::Checks { checks }), }) .collect::>()?; let anchor_state = ssz_decode_state(&path.join("anchor_state.ssz_snappy"), spec)?; From a0f33eebec9ea42377ee843dfdbae700d2905f04 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 12:12:41 +1100 Subject: [PATCH 05/24] Add harness to ef_tests --- Cargo.lock | 2 + .../src/attestation_verification.rs | 2 +- beacon_node/beacon_chain/src/lib.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 25 ++ consensus/fork_choice/src/fork_choice.rs | 10 + testing/ef_tests/Cargo.toml | 2 + testing/ef_tests/src/cases/fork_choice.rs | 219 +++++++++++++++++- testing/ef_tests/src/error.rs | 4 + 8 files changed, 258 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a51182b6417..53ca914698e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1468,6 +1468,7 @@ dependencies = [ name = "ef_tests" version = "0.2.0" dependencies = [ + "beacon_chain", "bls", "cached_tree_hash", "compare_fields", @@ -1485,6 +1486,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 eb29e8d2f32..c672ff6be6a 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/lib.rs b/beacon_node/beacon_chain/src/lib.rs index fd4cb149568..bbe03e3231a 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 536df8f58ec..0479c10ce0c 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -182,6 +182,31 @@ impl Builder> { self.store = Some(store); self.store_mutator(Box::new(mutator)) } + + pub fn checkpoint_sync_ephemeral_store( + mut self, + weak_subj_state: BeaconState, + weak_subj_block: SignedBeaconBlock, + 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 + .weak_subjectivity_state(weak_subj_state, weak_subj_block, genesis_state) + .expect("should build using weak subj state") + }; + self.store = Some(store); + self.store_mutator(Box::new(mutator)) + } } impl Builder> { diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a943e11e012..45f1e7b7e71 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -797,6 +797,16 @@ where *self.fc_store.finalized_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/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index ee05f40713d..e1668a9b49d 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/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index ff1a784cf5b..b0a790712ce 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -1,13 +1,19 @@ use super::*; use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; -use beacon_chain::test_utils::BeaconChainHarness; +use beacon_chain::{ + attestation_verification::{ + obtain_indexed_attestation_and_committees_per_slot, VerifiedAttestation, + }, + test_utils::{BeaconChainHarness, EphemeralHarnessType}, + BeaconChainTypes, HeadInfo, +}; use serde_derive::Deserialize; use types::{ - Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, Signature, - SignedBeaconBlock, Slot, + Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, + IndexedAttestation, Signature, SignedBeaconBlock, Slot, }; -#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Deserialize)] #[serde(deny_unknown_fields, rename_all = "snake_case")] pub struct Head { slot: Slot, @@ -93,8 +99,209 @@ impl Case for ForkChoiceTest { self.description.clone() } - fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { - // TODO(paul): run tests + fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { + let genesis_time = self.anchor_state.genesis_time(); + let spec = testing_spec::(fork_name); + let genesis_state = { + let mut state = self.anchor_state.clone(); + *state.slot_mut() = spec.genesis_slot; + state + }; + let signed_anchor_block = + SignedBeaconBlock::from_block(self.anchor_block.clone(), Signature::empty()); + let harness = BeaconChainHarness::builder(E::default()) + .spec(spec.clone()) + .keypairs(vec![]) + .checkpoint_sync_ephemeral_store( + self.anchor_state.clone(), + signed_anchor_block, + genesis_state, + ) + .build(); + + let tick_to_slot = |tick: u64| { + 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 / spec.seconds_per_slot; + Ok(spec.genesis_slot + slots_since_genesis) + }; + + let find_head = || -> Result { + harness + .chain + .fork_choice() + .map_err(|e| Error::InternalError(format!("failed to find head with {:?}", e)))?; + harness + .chain + .head_info() + .map_err(|e| Error::InternalError(format!("failed to read head with {:?}", e))) + }; + + for step in &self.steps { + match step { + Step::Tick { tick } => { + let slot = tick_to_slot(*tick)?; + harness.set_current_slot(slot); + harness + .chain + .fork_choice + .write() + .update_time(slot) + .map_err(|e| { + Error::InternalError(format!( + "setting tick to {} failed with {:?}", + tick, e + )) + })?; + } + Step::ValidBlock { block } => { + harness.chain.process_block(block.clone()).map_err(|e| { + Error::InternalError(format!( + "valid block {} failed import with {:?}", + block.canonical_root(), + e + )) + })?; + } + Step::MaybeValidBlock { block, valid } => { + let result = harness.chain.process_block(block.clone()); + if result.is_ok() != *valid { + return Err(Error::DidntFail(format!( + "block with root {} should be invalid", + block.canonical_root(), + ))); + } + // TODO(paul) try apply directly to fork choice? + } + Step::Attestation { attestation } => { + let (indexed_attestation, _) = + obtain_indexed_attestation_and_committees_per_slot( + &harness.chain, + attestation, + ) + .map_err(|e| { + Error::InternalError(format!( + "attestation indexing failed with {:?}", + e + )) + })?; + let verified_attestation: ManuallyVerifiedAttestation> = + ManuallyVerifiedAttestation { + attestation, + indexed_attestation, + }; + + harness + .chain + .apply_attestation_to_fork_choice(&verified_attestation) + .map_err(|e| { + Error::InternalError(format!("attestation import failed with {:?}", e)) + })?; + } + Step::Checks { checks } => { + let Checks { + head, + time, + genesis_time, + justified_checkpoint, + justified_checkpoint_root, + finalized_checkpoint, + best_justified_checkpoint, + } = checks; + + if let Some(expected_head) = head { + let chain_head = find_head().map(|head| Head { + slot: head.slot, + root: head.block_root, + })?; + + check_equal("head", chain_head, *expected_head)?; + } + + if let Some(expected_time) = time { + let slot = harness.chain.slot().map_err(|e| { + Error::InternalError(format!( + "reading current slot failed with {:?}", + e + )) + })?; + let expected_slot = tick_to_slot(*expected_time)?; + check_equal("time", slot, expected_slot)?; + } + + if let Some(expected_genesis_time) = genesis_time { + let genesis_time = harness.chain.slot_clock.genesis_duration().as_secs(); + check_equal("genesis_time", genesis_time, *expected_genesis_time)?; + } + + if let Some(expected_justified_checkpoint) = justified_checkpoint { + let chain_head = find_head()?; + check_equal( + "justified_checkpoint", + chain_head.current_justified_checkpoint, + *expected_justified_checkpoint, + )?; + } + + if let Some(expected_justified_checkpoint_root) = justified_checkpoint_root { + let chain_head = find_head()?; + check_equal( + "justified_checkpoint_root", + chain_head.current_justified_checkpoint.root, + *expected_justified_checkpoint_root, + )?; + } + + if let Some(expected_finalized_checkpoint) = finalized_checkpoint { + let chain_head = find_head()?; + check_equal( + "finalized_checkpoint", + chain_head.finalized_checkpoint, + *expected_finalized_checkpoint, + )?; + } + + if let Some(expected_best_justified_checkpoint) = best_justified_checkpoint { + let best_justified_checkpoint = + harness.chain.fork_choice.read().best_justified_checkpoint(); + check_equal( + "best_justified_checkpoint", + best_justified_checkpoint, + *expected_best_justified_checkpoint, + )?; + } + } + } + } + + Ok(()) + } +} + +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 + ))) + } +} + +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 1d1844558b8..c5795777ada 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(), } } From e13ba5f398e54b868aef45a0f996255ba5429281 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 15:25:47 +1100 Subject: [PATCH 06/24] Patch around 0x00 checkpoints --- consensus/fork_choice/src/fork_choice.rs | 5 ++++ testing/ef_tests/src/cases/fork_choice.rs | 31 ++++++++++++++++++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 45f1e7b7e71..06ca27922ff 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -797,6 +797,11 @@ 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 diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index b0a790712ce..ea76b58825b 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -102,11 +102,23 @@ impl Case for ForkChoiceTest { fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { let genesis_time = self.anchor_state.genesis_time(); let spec = testing_spec::(fork_name); + + assert_eq!(self.anchor_state.slot(), spec.genesis_slot); + let genesis_state = { let mut state = self.anchor_state.clone(); *state.slot_mut() = spec.genesis_slot; state }; + + let genesis_block_root = if self.anchor_block.slot() == spec.genesis_slot { + self.anchor_block.canonical_root() + } else if let Ok(root) = self.anchor_state.get_block_root(spec.genesis_slot) { + *root + } else { + Hash256::zero() + }; + let signed_anchor_block = SignedBeaconBlock::from_block(self.anchor_block.clone(), Signature::empty()); let harness = BeaconChainHarness::builder(E::default()) @@ -236,10 +248,16 @@ impl Case for ForkChoiceTest { } if let Some(expected_justified_checkpoint) = justified_checkpoint { - let chain_head = find_head()?; + let mut current_justified_checkpoint = + find_head()?.current_justified_checkpoint; + + if current_justified_checkpoint == Checkpoint::default() { + current_justified_checkpoint.root = genesis_block_root; + } + check_equal( "justified_checkpoint", - chain_head.current_justified_checkpoint, + current_justified_checkpoint, *expected_justified_checkpoint, )?; } @@ -254,10 +272,15 @@ impl Case for ForkChoiceTest { } if let Some(expected_finalized_checkpoint) = finalized_checkpoint { - let chain_head = find_head()?; + let mut finalized_checkpoint = find_head()?.finalized_checkpoint; + + if finalized_checkpoint == Checkpoint::default() { + finalized_checkpoint.root = genesis_block_root; + } + check_equal( "finalized_checkpoint", - chain_head.finalized_checkpoint, + finalized_checkpoint, *expected_finalized_checkpoint, )?; } From b4cc5ffd85772b22e05ad3e46a5d6fe682908a76 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:08:16 +1100 Subject: [PATCH 07/24] Refactor to use `Tester` --- beacon_node/beacon_chain/src/test_utils.rs | 11 +- testing/ef_tests/src/cases/fork_choice.rs | 388 ++++++++++++--------- 2 files changed, 231 insertions(+), 168 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 0479c10ce0c..b14493d6209 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -183,12 +183,7 @@ impl Builder> { self.store_mutator(Box::new(mutator)) } - pub fn checkpoint_sync_ephemeral_store( - mut self, - weak_subj_state: BeaconState, - weak_subj_block: SignedBeaconBlock, - genesis_state: BeaconState, - ) -> Self { + 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( @@ -201,8 +196,8 @@ impl Builder> { ); let mutator = move |builder: BeaconChainBuilder<_>| { builder - .weak_subjectivity_state(weak_subj_state, weak_subj_block, genesis_state) - .expect("should build using weak subj state") + .genesis_state(genesis_state) + .expect("should build state using recent genesis") }; self.store = Some(store); self.store_mutator(Box::new(mutator)) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index ea76b58825b..5173f7971e6 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -9,8 +9,8 @@ use beacon_chain::{ }; use serde_derive::Deserialize; use types::{ - Attestation, BeaconBlock, BeaconState, Checkpoint, EthSpec, ForkName, Hash256, - IndexedAttestation, Signature, SignedBeaconBlock, Slot, + Attestation, BeaconBlock, BeaconState, Checkpoint, Epoch, EthSpec, ForkName, Hash256, + IndexedAttestation, SignedBeaconBlock, Slot, }; #[derive(Debug, Clone, Copy, PartialEq, Deserialize)] @@ -100,117 +100,16 @@ impl Case for ForkChoiceTest { } fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { - let genesis_time = self.anchor_state.genesis_time(); - let spec = testing_spec::(fork_name); - - assert_eq!(self.anchor_state.slot(), spec.genesis_slot); - - let genesis_state = { - let mut state = self.anchor_state.clone(); - *state.slot_mut() = spec.genesis_slot; - state - }; - - let genesis_block_root = if self.anchor_block.slot() == spec.genesis_slot { - self.anchor_block.canonical_root() - } else if let Ok(root) = self.anchor_state.get_block_root(spec.genesis_slot) { - *root - } else { - Hash256::zero() - }; - - let signed_anchor_block = - SignedBeaconBlock::from_block(self.anchor_block.clone(), Signature::empty()); - let harness = BeaconChainHarness::builder(E::default()) - .spec(spec.clone()) - .keypairs(vec![]) - .checkpoint_sync_ephemeral_store( - self.anchor_state.clone(), - signed_anchor_block, - genesis_state, - ) - .build(); - - let tick_to_slot = |tick: u64| { - 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 / spec.seconds_per_slot; - Ok(spec.genesis_slot + slots_since_genesis) - }; - - let find_head = || -> Result { - harness - .chain - .fork_choice() - .map_err(|e| Error::InternalError(format!("failed to find head with {:?}", e)))?; - harness - .chain - .head_info() - .map_err(|e| Error::InternalError(format!("failed to read head with {:?}", e))) - }; + let tester = Tester::new(self, testing_spec::(fork_name))?; for step in &self.steps { match step { - Step::Tick { tick } => { - let slot = tick_to_slot(*tick)?; - harness.set_current_slot(slot); - harness - .chain - .fork_choice - .write() - .update_time(slot) - .map_err(|e| { - Error::InternalError(format!( - "setting tick to {} failed with {:?}", - tick, e - )) - })?; - } - Step::ValidBlock { block } => { - harness.chain.process_block(block.clone()).map_err(|e| { - Error::InternalError(format!( - "valid block {} failed import with {:?}", - block.canonical_root(), - e - )) - })?; - } + Step::Tick { tick } => tester.set_tick(*tick), + Step::ValidBlock { block } => tester.process_block(block.clone(), true)?, Step::MaybeValidBlock { block, valid } => { - let result = harness.chain.process_block(block.clone()); - if result.is_ok() != *valid { - return Err(Error::DidntFail(format!( - "block with root {} should be invalid", - block.canonical_root(), - ))); - } - // TODO(paul) try apply directly to fork choice? - } - Step::Attestation { attestation } => { - let (indexed_attestation, _) = - obtain_indexed_attestation_and_committees_per_slot( - &harness.chain, - attestation, - ) - .map_err(|e| { - Error::InternalError(format!( - "attestation indexing failed with {:?}", - e - )) - })?; - let verified_attestation: ManuallyVerifiedAttestation> = - ManuallyVerifiedAttestation { - attestation, - indexed_attestation, - }; - - harness - .chain - .apply_attestation_to_fork_choice(&verified_attestation) - .map_err(|e| { - Error::InternalError(format!("attestation import failed with {:?}", e)) - })?; + tester.process_block(block.clone(), *valid)? } + Step::Attestation { attestation } => tester.process_attestation(attestation)?, Step::Checks { checks } => { let Checks { head, @@ -223,76 +122,33 @@ impl Case for ForkChoiceTest { } = checks; if let Some(expected_head) = head { - let chain_head = find_head().map(|head| Head { - slot: head.slot, - root: head.block_root, - })?; - - check_equal("head", chain_head, *expected_head)?; + tester.check_head(*expected_head)?; } if let Some(expected_time) = time { - let slot = harness.chain.slot().map_err(|e| { - Error::InternalError(format!( - "reading current slot failed with {:?}", - e - )) - })?; - let expected_slot = tick_to_slot(*expected_time)?; - check_equal("time", slot, expected_slot)?; + tester.check_time(*expected_time)?; } if let Some(expected_genesis_time) = genesis_time { - let genesis_time = harness.chain.slot_clock.genesis_duration().as_secs(); - check_equal("genesis_time", genesis_time, *expected_genesis_time)?; + tester.check_genesis_time(*expected_genesis_time)?; } if let Some(expected_justified_checkpoint) = justified_checkpoint { - let mut current_justified_checkpoint = - find_head()?.current_justified_checkpoint; - - if current_justified_checkpoint == Checkpoint::default() { - current_justified_checkpoint.root = genesis_block_root; - } - - check_equal( - "justified_checkpoint", - current_justified_checkpoint, - *expected_justified_checkpoint, - )?; + tester.check_justified_checkpoint(*expected_justified_checkpoint)?; } if let Some(expected_justified_checkpoint_root) = justified_checkpoint_root { - let chain_head = find_head()?; - check_equal( - "justified_checkpoint_root", - chain_head.current_justified_checkpoint.root, - *expected_justified_checkpoint_root, - )?; + tester + .check_justified_checkpoint_root(*expected_justified_checkpoint_root)?; } if let Some(expected_finalized_checkpoint) = finalized_checkpoint { - let mut finalized_checkpoint = find_head()?.finalized_checkpoint; - - if finalized_checkpoint == Checkpoint::default() { - finalized_checkpoint.root = genesis_block_root; - } - - check_equal( - "finalized_checkpoint", - finalized_checkpoint, - *expected_finalized_checkpoint, - )?; + tester.check_finalized_checkpoint(*expected_finalized_checkpoint)?; } if let Some(expected_best_justified_checkpoint) = best_justified_checkpoint { - let best_justified_checkpoint = - harness.chain.fork_choice.read().best_justified_checkpoint(); - check_equal( - "best_justified_checkpoint", - best_justified_checkpoint, - *expected_best_justified_checkpoint, - )?; + tester + .check_best_justified_checkpoint(*expected_best_justified_checkpoint)?; } } } @@ -302,6 +158,218 @@ impl Case for ForkChoiceTest { } } +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(); + + 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) { + let slot = self.tick_to_slot(tick).unwrap(); + self.harness.set_current_slot(slot); + 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()); + // TODO(paul) try apply directly to fork choice? + if result.is_ok() != valid { + return Err(Error::DidntFail(format!( + "block with root {} should be invalid", + block.canonical_root(), + ))); + } else { + 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) + } +} + fn check_equal(check: &str, result: T, expected: T) -> Result<(), Error> { if result == expected { Ok(()) From 947c58345d15d0b6d800938bd030b995a21bc9f0 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:12:27 +1100 Subject: [PATCH 08/24] Add comments --- testing/ef_tests/src/cases/fork_choice.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 5173f7971e6..453126d42b5 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -158,6 +158,7 @@ impl Case for ForkChoiceTest { } } +/// A testing rig used to execute a test case. struct Tester { harness: BeaconChainHarness>, spec: ChainSpec, @@ -370,6 +371,7 @@ fn assert_checkpoints_eq(name: &str, genesis_epoch: Epoch, head: Checkpoint, fc: } } +/// Convenience function to create `Error` messages. fn check_equal(check: &str, result: T, expected: T) -> Result<(), Error> { if result == expected { Ok(()) @@ -381,6 +383,12 @@ fn check_equal(check: &str, result: T, expected: T) -> Res } } +/// 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, From 4b2ef55776e6de0b9be9f945b2c833de4b9f29b8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:25:17 +1100 Subject: [PATCH 09/24] Check genesis block root --- beacon_node/beacon_chain/src/test_utils.rs | 1 + testing/ef_tests/src/cases/fork_choice.rs | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index b14493d6209..b0b15501a44 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -183,6 +183,7 @@ impl Builder> { 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"); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 453126d42b5..a543ad3aba1 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -186,6 +186,14 @@ impl Tester { .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 From e041c86e89517092336a3c7511a5dfb0e9b3210a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:28:29 +1100 Subject: [PATCH 10/24] Remove todo --- testing/ef_tests/src/cases/fork_choice.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index a543ad3aba1..25bce86abca 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -239,7 +239,6 @@ impl Tester { pub fn process_block(&self, block: SignedBeaconBlock, valid: bool) -> Result<(), Error> { let result = self.harness.chain.process_block(block.clone()); - // TODO(paul) try apply directly to fork choice? if result.is_ok() != valid { return Err(Error::DidntFail(format!( "block with root {} should be invalid", From ea889302caeb5f0261c79853ed1d68196f89599c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:33:04 +1100 Subject: [PATCH 11/24] Appease clippy --- testing/ef_tests/src/cases/fork_choice.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 25bce86abca..5b3d6d0826a 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -39,7 +39,7 @@ pub enum Step { ValidBlock { block: B }, MaybeValidBlock { block: B, valid: bool }, Attestation { attestation: A }, - Checks { checks: Checks }, + Checks { checks: Box }, } #[derive(Debug, Clone, Deserialize)] @@ -119,7 +119,7 @@ impl Case for ForkChoiceTest { justified_checkpoint_root, finalized_checkpoint, best_justified_checkpoint, - } = checks; + } = checks.as_ref(); if let Some(expected_head) = head { tester.check_head(*expected_head)?; From 1b1fd7843482e48c20d790de05c0b9f092579422 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 17:42:26 +1100 Subject: [PATCH 12/24] Ignore failing tests --- testing/ef_tests/src/cases/fork_choice.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 5b3d6d0826a..b561649f4bd 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -53,7 +53,14 @@ pub struct ForkChoiceTest { impl LoadCase for ForkChoiceTest { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { - let description = format!("{:?}", path); + let description = format!( + "{}", + path.iter() + .last() + .expect("path must be non-empty") + .to_str() + .expect("path must be valid OsStr") + ); 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. @@ -102,6 +109,14 @@ impl Case for ForkChoiceTest { fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { let tester = Tester::new(self, testing_spec::(fork_name))?; + match self.description.as_str() { + "new_justified_is_later_than_store_justified" + | "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), From 4a1b665f0037fb32389edb8a5b0bf4eb78ec4951 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 20 Oct 2021 18:03:40 +1100 Subject: [PATCH 13/24] Appease clippy --- testing/ef_tests/src/cases/fork_choice.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index b561649f4bd..0daed498400 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -53,14 +53,13 @@ pub struct ForkChoiceTest { impl LoadCase for ForkChoiceTest { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { - let description = format!( - "{}", - path.iter() - .last() - .expect("path must be non-empty") - .to_str() - .expect("path must be valid OsStr") - ); + 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. From 7df529cd559745a60c9080432a293675c102cf0e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 21 Oct 2021 10:00:42 +1100 Subject: [PATCH 14/24] Don't run tests with fake crypto --- testing/ef_tests/src/handler.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 940be86a51a..411d005d352 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -440,6 +440,12 @@ impl Handler for ForkChoiceGetHeadHandler { 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)] @@ -461,6 +467,12 @@ impl Handler for ForkChoiceOnBlockHandler { 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)] From 6d18163bd42b0b7b9e63f8e3d1b7b9ffba9c6059 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 21 Oct 2021 11:51:02 +1100 Subject: [PATCH 15/24] Parse meta file, if it exists --- testing/ef_tests/src/cases/fork_choice.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 0daed498400..bd2516bb159 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -14,7 +14,7 @@ use types::{ }; #[derive(Debug, Clone, Copy, PartialEq, Deserialize)] -#[serde(deny_unknown_fields, rename_all = "snake_case")] +#[serde(deny_unknown_fields)] pub struct Head { slot: Slot, root: Hash256, @@ -42,6 +42,12 @@ pub enum Step { Checks { checks: Box }, } +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Meta { + description: String, +} + #[derive(Debug, Clone, Deserialize)] #[serde(bound = "E: EthSpec")] pub struct ForkChoiceTest { @@ -91,6 +97,18 @@ impl LoadCase for ForkChoiceTest { 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, From 6598cd5f62bba853620e14531ce089f91f04d332 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 21 Oct 2021 12:11:12 +1100 Subject: [PATCH 16/24] Remove unnecessary derives --- testing/ef_tests/src/cases/fork_choice.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index bd2516bb159..e0cae044570 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -48,8 +48,7 @@ pub struct Meta { description: String, } -#[derive(Debug, Clone, Deserialize)] -#[serde(bound = "E: EthSpec")] +#[derive(Debug)] pub struct ForkChoiceTest { pub description: String, pub anchor_state: BeaconState, From 2cbaa8ade4990179f603f73d7c1ec3abc8b6868e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 21 Oct 2021 17:16:53 +1100 Subject: [PATCH 17/24] Use safe_slots_to_update_justified from spec --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- beacon_node/beacon_chain/src/fork_revert.rs | 2 +- consensus/fork_choice/src/fork_choice.rs | 15 ++++++--------- consensus/fork_choice/src/lib.rs | 1 - 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 16bbf8a74a6..90e1c84f8fe 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 31678580a0b..8d0545c58c7 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/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 06ca27922ff..d0aa8abc1da 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)?; diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 7b508afd49c..5e9deac3b51 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; From f5a4bba069d852641c9497ed866960b757e5fa26 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 21 Oct 2021 17:18:42 +1100 Subject: [PATCH 18/24] Un-ignore a test --- testing/ef_tests/src/cases/fork_choice.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index e0cae044570..80a65b29f76 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -125,12 +125,8 @@ impl Case for ForkChoiceTest { fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { let tester = Tester::new(self, testing_spec::(fork_name))?; - match self.description.as_str() { - "new_justified_is_later_than_store_justified" - | "new_finalized_slot_is_justified_checkpoint_ancestor" => { - return Err(Error::SkippedKnownFailure) - } - _ => (), + if self.description == "new_finalized_slot_is_justified_checkpoint_ancestor" { + return Err(Error::SkippedKnownFailure); }; for step in &self.steps { From 701b968b0936ada27670aa39acf732d0f2862b42 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 22 Oct 2021 09:27:49 +1100 Subject: [PATCH 19/24] Fix consensus/fork_choice tests --- consensus/fork_choice/tests/tests.rs | 32 ++++++++++++++++++---------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 08b098707f1..8c94a59d2ac 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. From e2337d5c0727f604c2ceb9cac4ee6a1d957b2b44 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 2 Nov 2021 13:34:49 +1100 Subject: [PATCH 20/24] Add comment about failure --- testing/ef_tests/src/cases/fork_choice.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 80a65b29f76..2a200c523a7 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -125,6 +125,11 @@ impl Case for ForkChoiceTest { 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); }; From a715381901a3e9816d0888b2da4f460716679c64 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 2 Nov 2021 14:00:33 +1100 Subject: [PATCH 21/24] Apply invalid blocks directly to on_block --- testing/ef_tests/src/cases/fork_choice.rs | 53 +++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 2a200c523a7..9cfc458ea5e 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -8,6 +8,7 @@ use beacon_chain::{ BeaconChainTypes, HeadInfo, }; use serde_derive::Deserialize; +use state_processing::state_advance::complete_state_advance; use types::{ Attestation, BeaconBlock, BeaconState, Checkpoint, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SignedBeaconBlock, Slot, @@ -271,14 +272,58 @@ impl Tester { 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 {} should be invalid", - block.canonical_root(), + "block with root {} was valid={} whilst test expects valid={}", + block_root, + result.is_ok(), + valid ))); - } else { - Ok(()) } + + // 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> { From 727b8491ea394938ddaa4740deb71bd3b4cc5c33 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 5 Nov 2021 15:47:02 +1100 Subject: [PATCH 22/24] Apply suggestions from code review Co-authored-by: Michael Sproul --- testing/ef_tests/src/handler.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 411d005d352..11bda8f9f35 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -426,7 +426,6 @@ impl Handler for FinalityHandler { pub struct ForkChoiceGetHeadHandler(PhantomData); impl Handler for ForkChoiceGetHeadHandler { - // Reuse the blocks case runner. type Case = cases::ForkChoiceTest; fn config_name() -> &'static str { @@ -453,7 +452,6 @@ impl Handler for ForkChoiceGetHeadHandler { pub struct ForkChoiceOnBlockHandler(PhantomData); impl Handler for ForkChoiceOnBlockHandler { - // Reuse the blocks case runner. type Case = cases::ForkChoiceTest; fn config_name() -> &'static str { From 73c63af553e3a75c5f5f28f0f5cd2d37342d9b52 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 8 Nov 2021 11:34:49 +1100 Subject: [PATCH 23/24] Remove hard-coded harness slot time --- beacon_node/beacon_chain/src/test_utils.rs | 5 ++--- beacon_node/beacon_chain/tests/store_tests.rs | 10 +++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index b0b15501a44..d407d835424 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"; @@ -318,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"); @@ -352,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 8a2412b2fa4..f9af16bbe72 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() From 50d9f5a9b732b881be2df1a340c6e993bee8709c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 8 Nov 2021 11:37:03 +1100 Subject: [PATCH 24/24] Set slot clock in fork choice tests --- testing/ef_tests/src/cases/fork_choice.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 9cfc458ea5e..7d7b21da132 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -9,6 +9,7 @@ use beacon_chain::{ }; 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, @@ -260,8 +261,15 @@ impl Tester { } 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(); - self.harness.set_current_slot(slot); + assert_eq!(slot, self.harness.chain.slot().unwrap()); + self.harness .chain .fork_choice