From 4ce3e40f5d9451f8d59f466be02c1709c942b3d8 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 20 Jun 2022 13:39:01 -0400 Subject: [PATCH] - pass thrugh spec argument - parse discard_equivocations test --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 + beacon_node/beacon_chain/tests/tests.rs | 4 +- consensus/fork_choice/src/fork_choice.rs | 23 ++-- consensus/fork_choice/tests/tests.rs | 2 +- consensus/proto_array/src/proto_array.rs | 114 ++++++++++-------- .../src/proto_array_fork_choice.rs | 2 +- testing/ef_tests/src/cases/fork_choice.rs | 24 +++- testing/ef_tests/src/handler.rs | 4 - 8 files changed, 100 insertions(+), 75 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 72711810171..56a308880b8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1919,6 +1919,7 @@ impl BeaconChain { self.slot()?, verified.indexed_attestation(), AttestationFromBlock::False, + &self.spec, ) .map_err(Into::into) } @@ -2701,6 +2702,7 @@ impl BeaconChain { current_slot, &indexed_attestation, AttestationFromBlock::True, + &self.spec, ) { Ok(()) => Ok(()), // Ignore invalid attestations whilst importing attestations from a block. The diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 7b17937a210..b079baf2ff0 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -457,7 +457,7 @@ fn unaggregated_attestations_added_to_fork_choice_some_none() { // Move forward a slot so all queued attestations can be processed. harness.advance_slot(); fork_choice - .update_time(harness.chain.slot().unwrap()) + .update_time(harness.chain.slot().unwrap(), &MinimalEthSpec) .unwrap(); let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT) @@ -566,7 +566,7 @@ fn unaggregated_attestations_added_to_fork_choice_all_updated() { // Move forward a slot so all queued attestations can be processed. harness.advance_slot(); fork_choice - .update_time(harness.chain.slot().unwrap()) + .update_time(harness.chain.slot().unwrap(), MinimalEthSpec) .unwrap(); let validators: Vec = (0..VALIDATOR_COUNT).collect(); diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 218dc2c63a8..3a7f5af25fc 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -406,7 +406,7 @@ where current_slot: Slot, spec: &ChainSpec, ) -> Result> { - self.update_time(current_slot)?; + self.update_time(current_slot, spec)?; let store = &mut self.fc_store; @@ -451,8 +451,7 @@ where current_slot: Slot, spec: &ChainSpec, ) -> Result> { - //TODO(sean) update_time -> on_tick -> update_checkpoints -> should_update_justified_checkpoint -> update_time - self.update_time(current_slot)?; + self.update_time(current_slot, spec)?; if compute_slots_since_epoch_start::(self.fc_store.get_current_slot()) < spec.safe_slots_to_update_justified @@ -537,7 +536,7 @@ where payload_verification_status: PayloadVerificationStatus, spec: &ChainSpec, ) -> Result<(), Error> { - let current_slot = self.update_time(current_slot)?; + let current_slot = self.update_time(current_slot, spec)?; // Parent block must be known. if !self.proto_array.contains_block(&block.parent_root()) { @@ -901,9 +900,10 @@ where current_slot: Slot, attestation: &IndexedAttestation, is_from_block: AttestationFromBlock, + spec: &ChainSpec, ) -> Result<(), Error> { // Ensure the store is up-to-date. - self.update_time(current_slot)?; + self.update_time(current_slot, spec)?; // Ignore any attestations to the zero hash. // @@ -948,17 +948,16 @@ where /// Call `on_tick` for all slots between `fc_store.get_current_slot()` and the provided /// `current_slot`. Returns the value of `self.fc_store.get_current_slot`. - pub fn update_time(&mut self, current_slot: Slot) -> Result> { + pub fn update_time( + &mut self, + current_slot: Slot, + spec: &ChainSpec, + ) -> Result> { while self.fc_store.get_current_slot() < current_slot { let previous_slot = self.fc_store.get_current_slot(); // Note: we are relying upon `on_tick` to update `fc_store.time` to ensure we don't // get stuck in a loop. - //TODO(sean) fix chain spec - dbg!(&previous_slot); - if (previous_slot) % MainnetEthSpec::slots_per_epoch() == 0 { - dbg!("hitting epoch boundary"); - } - self.on_tick(previous_slot + 1, &ChainSpec::mainnet())? + self.on_tick(previous_slot + 1, spec)? } // Process any attestations that might now be eligible. diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 3f8a2ac6b6b..a495f9584c6 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -152,7 +152,7 @@ impl ForkChoiceTest { .chain .fork_choice .write() - .update_time(self.harness.chain.slot().unwrap()) + .update_time(self.harness.chain.slot().unwrap(), &self.harness.spec) .unwrap(); func(self.harness.chain.fork_choice.read().queued_attestations()); self diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 8e6ca78d8cc..016e45f3cc3 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -5,7 +5,10 @@ use ssz::four_byte_option_impl; use ssz::Encode; use ssz_derive::{Decode, Encode}; use std::collections::{HashMap, HashSet}; -use types::{AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, MainnetEthSpec, Slot}; +use types::{ + AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, + MainnetEthSpec, Slot, +}; // Define a "legacy" implementation of `Option` which uses four bytes for encoding the union // selector. @@ -281,7 +284,11 @@ impl ProtoArray { // If the node has a parent, try to update its best-child and best-descendant. if let Some(parent_index) = node.parent { - self.maybe_update_best_child_and_descendant(parent_index, node_index, current_slot)?; + self.maybe_update_best_child_and_descendant( + parent_index, + node_index, + current_slot, + )?; } } @@ -291,7 +298,7 @@ impl ProtoArray { /// Register a block with the fork choice. /// /// It is only sane to supply a `None` parent for the genesis block. - pub fn on_block(&mut self, block: Block, current_slot:Slot) -> Result<(), Error> { + pub fn on_block(&mut self, block: Block, current_slot: Slot) -> Result<(), Error> { // If the block is already known, simply ignore it. if self.indices.contains_key(&block.root) { return Ok(()); @@ -610,7 +617,11 @@ impl ProtoArray { /// been called without a subsequent `Self::apply_score_changes` call. This is because /// `on_new_block` does not attempt to walk backwards through the tree and update the /// best-child/best-descendant links. - pub fn find_head(&self, justified_root: &Hash256, current_slot: Slot) -> Result { + pub fn find_head( + &self, + justified_root: &Hash256, + current_slot: Slot, + ) -> Result { let justified_index = self .indices .get(justified_root) @@ -743,7 +754,7 @@ impl ProtoArray { &mut self, parent_index: usize, child_index: usize, - current_slot: Slot + current_slot: Slot, ) -> Result<(), Error> { let child = self .nodes @@ -755,7 +766,7 @@ impl ProtoArray { .get(parent_index) .ok_or(Error::InvalidNodeIndex(parent_index))?; - let child_leads_to_viable_head = self.node_leads_to_viable_head(child,current_slot)?; + let child_leads_to_viable_head = self.node_leads_to_viable_head(child, current_slot)?; // These three variables are aliases to the three options that we may set the // `parent.best_child` and `parent.best_descendant` to. @@ -768,54 +779,54 @@ impl ProtoArray { ); let no_change = (parent.best_child, parent.best_descendant); - let (new_best_child, new_best_descendant) = if let Some(best_child_index) = - parent.best_child - { - if best_child_index == child_index && !child_leads_to_viable_head { - // If the child is already the best-child of the parent but it's not viable for - // the head, remove it. - change_to_none - } else if best_child_index == child_index { - // If the child is the best-child already, set it again to ensure that the - // best-descendant of the parent is updated. - change_to_child - } else { - let best_child = self - .nodes - .get(best_child_index) - .ok_or(Error::InvalidBestDescendant(best_child_index))?; + let (new_best_child, new_best_descendant) = + if let Some(best_child_index) = parent.best_child { + if best_child_index == child_index && !child_leads_to_viable_head { + // If the child is already the best-child of the parent but it's not viable for + // the head, remove it. + change_to_none + } else if best_child_index == child_index { + // If the child is the best-child already, set it again to ensure that the + // best-descendant of the parent is updated. + change_to_child + } else { + let best_child = self + .nodes + .get(best_child_index) + .ok_or(Error::InvalidBestDescendant(best_child_index))?; - let best_child_leads_to_viable_head = self.node_leads_to_viable_head(best_child, current_slot)?; + let best_child_leads_to_viable_head = + self.node_leads_to_viable_head(best_child, current_slot)?; - if child_leads_to_viable_head && !best_child_leads_to_viable_head { - // The child leads to a viable head, but the current best-child doesn't. - change_to_child - } else if !child_leads_to_viable_head && best_child_leads_to_viable_head { - // The best child leads to a viable head, but the child doesn't. - no_change - } else if child.weight == best_child.weight { - // Tie-breaker of equal weights by root. - if child.root >= best_child.root { + if child_leads_to_viable_head && !best_child_leads_to_viable_head { + // The child leads to a viable head, but the current best-child doesn't. change_to_child - } else { + } else if !child_leads_to_viable_head && best_child_leads_to_viable_head { + // The best child leads to a viable head, but the child doesn't. no_change - } - } else { - // Choose the winner by weight. - if child.weight >= best_child.weight { - change_to_child + } else if child.weight == best_child.weight { + // Tie-breaker of equal weights by root. + if child.root >= best_child.root { + change_to_child + } else { + no_change + } } else { - no_change + // Choose the winner by weight. + if child.weight >= best_child.weight { + change_to_child + } else { + no_change + } } } - } - } else if child_leads_to_viable_head { - // There is no current best-child and the child is viable. - change_to_child - } else { - // There is no current best-child but the child is not viable. - no_change - }; + } else if child_leads_to_viable_head { + // There is no current best-child and the child is viable. + change_to_child + } else { + // There is no current best-child but the child is not viable. + no_change + }; let parent = self .nodes @@ -830,7 +841,11 @@ impl ProtoArray { /// Indicates if the node itself is viable for the head, or if it's best descendant is viable /// for the head. - fn node_leads_to_viable_head(&self, node: &ProtoNode, current_slot: Slot) -> Result { + fn node_leads_to_viable_head( + &self, + node: &ProtoNode, + current_slot: Slot, + ) -> Result { let best_descendant_is_viable_for_head = if let Some(best_descendant_index) = node.best_descendant { let best_descendant = self @@ -877,7 +892,8 @@ impl ProtoArray { node.justified_checkpoint, node.finalized_checkpoint, ) { - if node.slot.epoch(MainnetEthSpec::slots_per_epoch()) < current_slot.epoch(MainnetEthSpec::slots_per_epoch()) + if node.slot.epoch(MainnetEthSpec::slots_per_epoch()) + < current_slot.epoch(MainnetEthSpec::slots_per_epoch()) { checkpoint_match_predicate( unrealized_justified_checkpoint, diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 1122d427720..b60da4aacf2 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -246,7 +246,7 @@ impl ProtoArrayForkChoice { Ok(()) } - pub fn process_block(&mut self, block: Block, current_slot:Slot) -> Result<(), String> { + pub fn process_block(&mut self, block: Block, current_slot: Slot) -> Result<(), String> { if block.parent_root.is_none() { return Err("Missing parent root".to_string()); } diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 2c54482a90d..982735648fc 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -14,8 +14,8 @@ use ssz_derive::Decode; use state_processing::state_advance::complete_state_advance; use std::time::Duration; use types::{ - Attestation, BeaconBlock, BeaconState, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, - ForkName, Hash256, IndexedAttestation, SignedBeaconBlock, Slot, Uint256, + Attestation, AttesterSlashing, BeaconBlock, BeaconState, Checkpoint, Epoch, EthSpec, + ExecutionBlockHash, ForkName, Hash256, IndexedAttestation, SignedBeaconBlock, Slot, Uint256, }; #[derive(Default, Debug, PartialEq, Clone, Deserialize, Decode)] @@ -50,12 +50,13 @@ pub struct Checks { #[derive(Debug, Clone, Deserialize)] #[serde(untagged, deny_unknown_fields)] -pub enum Step { +pub enum Step { Tick { tick: u64 }, ValidBlock { block: B }, MaybeValidBlock { block: B, valid: bool }, Attestation { attestation: A }, PowBlock { pow_block: P }, + AttesterSlashing { attester_slashing: S }, Checks { checks: Box }, } @@ -71,7 +72,7 @@ pub struct ForkChoiceTest { pub description: String, pub anchor_state: BeaconState, pub anchor_block: BeaconBlock, - pub steps: Vec, Attestation, PowBlock>>, + pub steps: Vec, Attestation, PowBlock, AttesterSlashing>>, } /// Spec for fork choice tests, with proposer boosting enabled. @@ -92,7 +93,8 @@ impl LoadCase for ForkChoiceTest { .expect("path must be valid OsStr") .to_string(); let spec = &fork_choice_spec::(fork_name); - let steps: Vec> = yaml_decode_file(&path.join("steps.yaml"))?; + 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() @@ -118,6 +120,10 @@ impl LoadCase for ForkChoiceTest { ssz_decode_file(&path.join(format!("{}.ssz_snappy", pow_block))) .map(|pow_block| Step::PowBlock { pow_block }) } + Step::AttesterSlashing { attester_slashing } => { + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attester_slashing))) + .map(|attester_slashing| Step::AttesterSlashing { attester_slashing }) + } Step::Checks { checks } => Ok(Step::Checks { checks }), }) .collect::>()?; @@ -159,6 +165,8 @@ impl Case for ForkChoiceTest { // This test is skipped until we can do retrospective confirmations of the terminal // block after an optimistic sync. if self.description == "block_lookup_failed" + //TODO(sean): enable once we implement equivocation logic (https://github.com/ethereum/consensus-specs/pull/2845) + || self.description == "discard_equivocations" { return Err(Error::SkippedKnownFailure); }; @@ -172,6 +180,10 @@ impl Case for ForkChoiceTest { } Step::Attestation { attestation } => tester.process_attestation(attestation)?, Step::PowBlock { pow_block } => tester.process_pow_block(pow_block), + //TODO(sean): enable once we implement equivocation logic (https://github.com/ethereum/consensus-specs/pull/2845) + Step::AttesterSlashing { + attester_slashing: _, + } => (), Step::Checks { checks } => { let Checks { head, @@ -328,7 +340,7 @@ impl Tester { .chain .fork_choice .write() - .update_time(slot) + .update_time(slot, &self.harness.spec) .unwrap(); } diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 67b9ab2adc9..be6c495aaed 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -468,10 +468,6 @@ impl Handler for ForkChoiceHandler { return false; } - if ForkName::Merge != fork_name { - return false; - } - // 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"))