From e23726c0a1b02a1afb24f84e799dca881f821cbc Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Tue, 9 Jul 2019 12:36:26 +0200 Subject: [PATCH 01/34] Renamed fork_choice::process_attestation_from_block --- beacon_node/beacon_chain/src/fork_choice.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index c693145ea6..cdda563868 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -112,7 +112,7 @@ impl ForkChoice { // // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md for attestation in &block.body.attestations { - self.process_attestation_from_block(state, attestation)?; + self.process_attestation(state, attestation)?; } self.backend.process_block(block, block_root)?; @@ -120,7 +120,7 @@ impl ForkChoice { Ok(()) } - fn process_attestation_from_block( + pub fn process_attestation( &self, state: &BeaconState, attestation: &Attestation, From adf1d9c533d51c908b8e3b7430ba7e2554a2ef45 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Tue, 9 Jul 2019 12:36:59 +0200 Subject: [PATCH 02/34] Processing attestation in fork choice --- beacon_node/beacon_chain/src/beacon_chain.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 2d82822701..ca4667e00e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -480,6 +480,14 @@ impl BeaconChain { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); + match self.store.exists::(&attestation.data.target_root) { + Ok(true) => { + per_block_processing::validate_attestation_time_independent_only(&*self.state.read(), &attestation, &self.spec)?; + self.fork_choice.process_attestation(&*self.state.read(), &attestation); + }, + _ => {} + }; + let result = self .op_pool .insert_attestation(attestation, &*self.state.read(), &self.spec); From 40b166edcdb5d6102216f8f00397659089027ccc Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Thu, 11 Jul 2019 16:32:01 +0200 Subject: [PATCH 03/34] Retrieving state from store and checking signature --- beacon_node/beacon_chain/src/beacon_chain.rs | 24 ++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ca4667e00e..74d24244eb 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -15,7 +15,7 @@ use state_processing::per_block_processing::errors::{ }; use state_processing::{ per_block_processing, per_block_processing_without_verifying_block_signature, - per_slot_processing, BlockProcessingError, + per_slot_processing, BlockProcessingError, common }; use std::sync::Arc; use store::iter::{BlockIterator, BlockRootsIterator, StateRootsIterator}; @@ -480,14 +480,24 @@ impl BeaconChain { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); - match self.store.exists::(&attestation.data.target_root) { - Ok(true) => { - per_block_processing::validate_attestation_time_independent_only(&*self.state.read(), &attestation, &self.spec)?; - self.fork_choice.process_attestation(&*self.state.read(), &attestation); - }, - _ => {} + // Retrieve the attestation's state from `store` if necessary. + let attestation_state = match attestation.data.beacon_block_root == self.canonical_head.read().beacon_block_root { + true => Some(self.state.read().clone()), + false => match self.store.get::(&attestation.data.beacon_block_root) { + Ok(Some(block)) => match self.store.get::>(&block.state_root) { + Ok(state) => state, + _ => None + }, + _ => None + } }; + if let Some(state) = attestation_state { + let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; + per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; + self.fork_choice.process_attestation(&state, &attestation); + } + let result = self .op_pool .insert_attestation(attestation, &*self.state.read(), &self.spec); From 7cdfa3cc279be167b933b3cb632f83233d35baf8 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Fri, 19 Jul 2019 14:52:01 +0200 Subject: [PATCH 04/34] Looser check on beacon state validity. --- beacon_node/beacon_chain/src/beacon_chain.rs | 42 ++++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f18b49e129..701d900c76 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -493,19 +493,7 @@ impl BeaconChain { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); - // Retrieve the attestation's state from `store` if necessary. - let attestation_state = match attestation.data.beacon_block_root == self.canonical_head.read().beacon_block_root { - true => Some(self.state.read().clone()), - false => match self.store.get::(&attestation.data.beacon_block_root) { - Ok(Some(block)) => match self.store.get::>(&block.state_root) { - Ok(state) => state, - _ => None - }, - _ => None - } - }; - - if let Some(state) = attestation_state { + if let Some(state) = self.get_attestation_state(&attestation) { let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; self.fork_choice.process_attestation(&state, &attestation); @@ -535,6 +523,34 @@ impl BeaconChain { result } + fn get_attestation_state(&self, attestation: &Attestation) -> Option> { + let blocks = BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), self.state.read().slot.clone()); + for (root, slot) in blocks { + if root == attestation.data.target_root + && self.slot_epochs_equal_or_adjacent(slot, self.state.read().slot) { + return Some(self.state.read().clone()); + } + }; + + match self.store.get::(&attestation.data.target_root) { + Ok(Some(block)) => match self.store.get::>(&block.state_root) { + Ok(state) => state, + _ => None + }, + _ => None + } + } + + fn slot_epochs_equal_or_adjacent(&self, slot_a: Slot, slot_b: Slot) -> bool { + let slots_per_epoch = T::EthSpec::slots_per_epoch(); + let epoch_a = slot_a.epoch(slots_per_epoch); + let epoch_b = slot_b.epoch(slots_per_epoch); + + epoch_a == epoch_b + || epoch_a + 1 == epoch_b + || epoch_b + 1 == epoch_a + } + /// Accept some deposit and queue it for inclusion in an appropriate block. pub fn process_deposit( &self, From bef7ca6bfb1bb68d43d58b69e9a658b59c69d014 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Sat, 20 Jul 2019 12:47:59 +0200 Subject: [PATCH 05/34] Cleaned up get_attestation_state --- beacon_node/beacon_chain/src/beacon_chain.rs | 23 ++++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 701d900c76..d02ab31763 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -523,15 +523,24 @@ impl BeaconChain { result } + /// Retrieves the `BeaconState` used to create the attestation. fn get_attestation_state(&self, attestation: &Attestation) -> Option> { + // Current state is used if the attestation targets a historic block and a slot within an + // equal or adjacent epoch. + let slots_per_epoch = T::EthSpec::slots_per_epoch(); + let min_slot = (self.state.read().slot.epoch(slots_per_epoch) - 1).start_slot(slots_per_epoch); let blocks = BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), self.state.read().slot.clone()); for (root, slot) in blocks { - if root == attestation.data.target_root - && self.slot_epochs_equal_or_adjacent(slot, self.state.read().slot) { + if root == attestation.data.target_root { return Some(self.state.read().clone()); } + + if slot == min_slot { + break; + } }; + // A different state is retrieved from the database. match self.store.get::(&attestation.data.target_root) { Ok(Some(block)) => match self.store.get::>(&block.state_root) { Ok(state) => state, @@ -541,16 +550,6 @@ impl BeaconChain { } } - fn slot_epochs_equal_or_adjacent(&self, slot_a: Slot, slot_b: Slot) -> bool { - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let epoch_a = slot_a.epoch(slots_per_epoch); - let epoch_b = slot_b.epoch(slots_per_epoch); - - epoch_a == epoch_b - || epoch_a + 1 == epoch_b - || epoch_b + 1 == epoch_a - } - /// Accept some deposit and queue it for inclusion in an appropriate block. pub fn process_deposit( &self, From 3b8a584c550fc7788806e04e7a7e76a9dfb07796 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Sun, 21 Jul 2019 22:53:39 +0200 Subject: [PATCH 06/34] Expanded fork choice api to provide latest validator message. --- eth2/lmd_ghost/src/lib.rs | 3 +++ eth2/lmd_ghost/src/reduced_tree.rs | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/eth2/lmd_ghost/src/lib.rs b/eth2/lmd_ghost/src/lib.rs index dd413e2eb5..f18b5b81f7 100644 --- a/eth2/lmd_ghost/src/lib.rs +++ b/eth2/lmd_ghost/src/lib.rs @@ -43,4 +43,7 @@ pub trait LmdGhost: Send + Sync { finalized_block: &BeaconBlock, finalized_block_root: Hash256, ) -> Result<()>; + + /// Returns the latest message for a given validator index. + fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)>; } diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index dace2bda6f..f069ae68c5 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -87,6 +87,12 @@ where .update_root(new_block.slot, new_root) .map_err(|e| format!("update_finalized_root failed: {:?}", e)) } + + fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)> { + self.core + .write() + .latest_message(validator_index) + } } struct ReducedTree { @@ -222,6 +228,13 @@ where Ok(head_node.block_hash) } + pub fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)> { + match self.latest_votes.get(validator_index) { + Some(v) => Some((v.hash.clone(), v.slot.clone())), + None => None + } + } + fn find_head_from<'a>(&'a self, start_node: &'a Node) -> Result<&'a Node> { if start_node.does_not_have_children() { Ok(start_node) From b2471eca494369e946fa212610af6b2c8be44802 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Tue, 23 Jul 2019 20:50:18 +0200 Subject: [PATCH 07/34] Checking if the an attestation contains a latest message --- beacon_node/beacon_chain/src/beacon_chain.rs | 19 ++++------------ beacon_node/beacon_chain/src/fork_choice.rs | 24 ++++++++++++++++++++ eth2/lmd_ghost/src/lib.rs | 2 +- eth2/lmd_ghost/src/reduced_tree.rs | 2 +- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d02ab31763..f215465f2f 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -494,9 +494,11 @@ impl BeaconChain { let timer = self.metrics.attestation_processing_times.start_timer(); if let Some(state) = self.get_attestation_state(&attestation) { - let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; - per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; - self.fork_choice.process_attestation(&state, &attestation); + if self.fork_choice.should_process_attestation(&state, &attestation) { + let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; + per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; + self.fork_choice.process_attestation(&state, &attestation); + } } let result = self @@ -509,17 +511,6 @@ impl BeaconChain { self.metrics.attestation_processing_successes.inc(); } - // TODO: process attestation. Please consider: - // - // - Because a block was not added to the op pool does not mean it's invalid (it might - // just be old). - // - The attestation should be rejected if we don't know the block (ideally it should be - // queued, but this may be overkill). - // - The attestation _must_ be validated against it's state before being added to fork - // choice. - // - You can avoid verifying some attestations by first checking if they're a latest - // message. This would involve expanding the `LmdGhost` API. - result } diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index cdda563868..6b69e3e084 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -4,6 +4,7 @@ use state_processing::common::get_attesting_indices_unsorted; use std::sync::Arc; use store::{Error as StoreError, Store}; use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256}; +use state_processing::common; type Result = std::result::Result; @@ -120,6 +121,9 @@ impl ForkChoice { Ok(()) } + /// Process an attestation. + /// + /// Assumes the attestation is valid. pub fn process_attestation( &self, state: &BeaconState, @@ -162,6 +166,26 @@ impl ForkChoice { Ok(()) } + /// Determines whether or not the given attestation contains a latest messages. + pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> bool { + let validator_indices = common::get_attesting_indices_unsorted( + state, + &attestation.data, + &attestation.aggregation_bitfield, + ).unwrap(); + + let target_slot = attestation.data.target_epoch.start_slot(T::EthSpec::slots_per_epoch()); + + validator_indices + .iter() + .find(|&&v| { + match self.backend.latest_message(v) { + Some((_, slot)) => target_slot > slot, + None => true + } + }).is_some() + } + /// Inform the fork choice that the given block (and corresponding root) have been finalized so /// it may prune it's storage. /// diff --git a/eth2/lmd_ghost/src/lib.rs b/eth2/lmd_ghost/src/lib.rs index f18b5b81f7..183d45c9a4 100644 --- a/eth2/lmd_ghost/src/lib.rs +++ b/eth2/lmd_ghost/src/lib.rs @@ -45,5 +45,5 @@ pub trait LmdGhost: Send + Sync { ) -> Result<()>; /// Returns the latest message for a given validator index. - fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)>; + fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)>; } diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index f069ae68c5..0985441dfc 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -88,7 +88,7 @@ where .map_err(|e| format!("update_finalized_root failed: {:?}", e)) } - fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)> { + fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { self.core .write() .latest_message(validator_index) From 51645aa9af00ef2b93226f65c0a64c411a73243d Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Wed, 24 Jul 2019 18:03:48 +0200 Subject: [PATCH 08/34] Correct process_attestation error handling. --- beacon_node/beacon_chain/src/beacon_chain.rs | 10 +++++--- beacon_node/beacon_chain/src/errors.rs | 5 ++++ beacon_node/beacon_chain/src/fork_choice.rs | 11 +++++---- beacon_node/rpc/src/attestation.rs | 26 ++++++++++++++++++-- 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f215465f2f..67d9281277 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -22,6 +22,7 @@ use store::iter::{BestBlockRootsIterator, BlockIterator, BlockRootsIterator, Sta use store::{Error as DBError, Store}; use tree_hash::TreeHash; use types::*; +use crate::BeaconChainError; // Text included in blocks. // Must be 32-bytes or panic. @@ -489,15 +490,15 @@ impl BeaconChain { pub fn process_attestation( &self, attestation: Attestation, - ) -> Result<(), AttestationValidationError> { + ) -> Result<(), Error> { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); if let Some(state) = self.get_attestation_state(&attestation) { - if self.fork_choice.should_process_attestation(&state, &attestation) { + if self.fork_choice.should_process_attestation(&state, &attestation)? { let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; - self.fork_choice.process_attestation(&state, &attestation); + self.fork_choice.process_attestation(&state, &attestation)?; } } @@ -511,7 +512,7 @@ impl BeaconChain { self.metrics.attestation_processing_successes.inc(); } - result + result.map_err(|e| BeaconChainError::AttestationValidationError(e)) } /// Retrieves the `BeaconState` used to create the attestation. @@ -968,3 +969,4 @@ impl From for Error { Error::BeaconStateError(e) } } + diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 0d619d7f2d..4e2170ca84 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -3,6 +3,7 @@ use crate::metrics::Error as MetricsError; use state_processing::BlockProcessingError; use state_processing::SlotProcessingError; use types::*; +use state_processing::per_block_processing::errors::{AttestationValidationError, IndexedAttestationValidationError}; macro_rules! easy_from_to { ($from: ident, $to: ident) => { @@ -31,6 +32,8 @@ pub enum BeaconChainError { MissingBeaconState(Hash256), SlotProcessingError(SlotProcessingError), MetricsError(String), + AttestationValidationError(AttestationValidationError), + IndexedAttestationValidationError(IndexedAttestationValidationError) } easy_from_to!(SlotProcessingError, BeaconChainError); @@ -53,3 +56,5 @@ pub enum BlockProductionError { easy_from_to!(BlockProcessingError, BlockProductionError); easy_from_to!(BeaconStateError, BlockProductionError); easy_from_to!(SlotProcessingError, BlockProductionError); +easy_from_to!(AttestationValidationError, BeaconChainError); +easy_from_to!(IndexedAttestationValidationError, BeaconChainError); diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 6b69e3e084..92b683590a 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -166,24 +166,24 @@ impl ForkChoice { Ok(()) } - /// Determines whether or not the given attestation contains a latest messages. - pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> bool { + /// Determines whether or not the given attestation contains a latest message. + pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> Result { let validator_indices = common::get_attesting_indices_unsorted( state, &attestation.data, &attestation.aggregation_bitfield, - ).unwrap(); + )?; let target_slot = attestation.data.target_epoch.start_slot(T::EthSpec::slots_per_epoch()); - validator_indices + Ok(validator_indices .iter() .find(|&&v| { match self.backend.latest_message(v) { Some((_, slot)) => target_slot > slot, None => true } - }).is_some() + }).is_some()) } /// Inform the fork choice that the given block (and corresponding root) have been finalized so @@ -218,3 +218,4 @@ impl From for Error { Error::BackendError(e) } } + diff --git a/beacon_node/rpc/src/attestation.rs b/beacon_node/rpc/src/attestation.rs index b85d4e9475..48d9eb4693 100644 --- a/beacon_node/rpc/src/attestation.rs +++ b/beacon_node/rpc/src/attestation.rs @@ -1,4 +1,4 @@ -use beacon_chain::{BeaconChain, BeaconChainTypes}; +use beacon_chain::{BeaconChain, BeaconChainTypes, BeaconChainError}; use eth2_libp2p::PubsubMessage; use eth2_libp2p::TopicBuilder; use eth2_libp2p::BEACON_ATTESTATION_TOPIC; @@ -159,7 +159,7 @@ impl AttestationService for AttestationServiceInstance { resp.set_success(true); } - Err(e) => { + Err(BeaconChainError::AttestationValidationError(e)) => { // Attestation was invalid warn!( self.log, @@ -170,6 +170,28 @@ impl AttestationService for AttestationServiceInstance { resp.set_success(false); resp.set_msg(format!("InvalidAttestation: {:?}", e).as_bytes().to_vec()); } + Err(BeaconChainError::IndexedAttestationValidationError(e)) => { + // Indexed attestation was invalid + warn!( + self.log, + "PublishAttestation"; + "type" => "invalid_attestation", + "error" => format!("{:?}", e), + ); + resp.set_success(false); + resp.set_msg(format!("InvalidIndexedAttestation: {:?}", e).as_bytes().to_vec()); + } + Err(e) => { + // Attestation was invalid + warn!( + self.log, + "PublishAttestation"; + "type" => "beacon_chain_error", + "error" => format!("{:?}", e), + ); + resp.set_success(false); + resp.set_msg(format!("There was a beacon chain error: {:?}", e).as_bytes().to_vec()); + } }; let error_log = self.log.clone(); From b49d592eee41fbbd04e88e8e6a08c3be372d1b77 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Wed, 24 Jul 2019 18:06:18 +0200 Subject: [PATCH 09/34] Copy paste error in comment fixed. --- beacon_node/rpc/src/attestation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/rpc/src/attestation.rs b/beacon_node/rpc/src/attestation.rs index 48d9eb4693..1cfa81a04b 100644 --- a/beacon_node/rpc/src/attestation.rs +++ b/beacon_node/rpc/src/attestation.rs @@ -182,7 +182,7 @@ impl AttestationService for AttestationServiceInstance { resp.set_msg(format!("InvalidIndexedAttestation: {:?}", e).as_bytes().to_vec()); } Err(e) => { - // Attestation was invalid + // Some other error warn!( self.log, "PublishAttestation"; From b096e3a6432be6ce6850b211704a858bfcad8a28 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 5 Aug 2019 16:25:21 +1000 Subject: [PATCH 10/34] Tidy ancestor iterators --- beacon_node/beacon_chain/src/beacon_chain.rs | 75 +++++---- beacon_node/beacon_chain/src/fork_choice.rs | 2 +- beacon_node/beacon_chain/src/iter.rs | 48 ++++++ beacon_node/beacon_chain/src/lib.rs | 1 + beacon_node/beacon_chain/src/test_utils.rs | 2 +- beacon_node/network/src/sync/simple_sync.rs | 19 +-- beacon_node/rest_api/src/beacon_node.rs | 2 +- beacon_node/rpc/src/attestation.rs | 6 +- beacon_node/rpc/src/validator.rs | 6 +- beacon_node/store/src/iter.rs | 159 +------------------ eth2/lmd_ghost/tests/test.rs | 4 +- 11 files changed, 125 insertions(+), 199 deletions(-) create mode 100644 beacon_node/beacon_chain/src/iter.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d520f0b5c9..2901289943 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1,6 +1,7 @@ use crate::checkpoint::CheckPoint; use crate::errors::{BeaconChainError as Error, BlockProductionError}; use crate::fork_choice::{Error as ForkChoiceError, ForkChoice}; +use crate::iter::{ReverseBlockRootIterator, ReverseStateRootIterator}; use crate::metrics::Metrics; use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY}; use lmd_ghost::LmdGhost; @@ -19,7 +20,7 @@ use state_processing::{ per_slot_processing, BlockProcessingError, }; use std::sync::Arc; -use store::iter::{BestBlockRootsIterator, BlockIterator, BlockRootsIterator, StateRootsIterator}; +use store::iter::{BlockRootsIterator, StateRootsIterator}; use store::{Error as DBError, Store}; use tree_hash::TreeHash; use types::*; @@ -224,45 +225,53 @@ impl BeaconChain { Ok(headers?) } - /// Iterate in reverse (highest to lowest slot) through all blocks from the block at `slot` - /// through to the genesis block. - /// - /// Returns `None` for headers prior to genesis or when there is an error reading from `Store`. - /// - /// Contains duplicate headers when skip slots are encountered. - pub fn rev_iter_blocks(&self, slot: Slot) -> BlockIterator { - BlockIterator::owned(self.store.clone(), self.state.read().clone(), slot) - } - /// Iterates in reverse (highest to lowest slot) through all block roots from `slot` through to - /// genesis. + /// Iterates through all the `BeaconBlock` roots and slots, first returning + /// `self.head().beacon_block` then all prior blocks until either genesis or if the database + /// fails to return a prior block. /// - /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`. + /// Returns duplicate roots for skip-slots. /// - /// Contains duplicate roots when skip slots are encountered. - pub fn rev_iter_block_roots(&self, slot: Slot) -> BlockRootsIterator { - BlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot) - } - - /// Iterates in reverse (highest to lowest slot) through all block roots from largest - /// `slot <= beacon_state.slot` through to genesis. + /// Iterator returns `(Hash256, Slot)`. /// - /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`. + /// ## Note /// - /// Contains duplicate roots when skip slots are encountered. - pub fn rev_iter_best_block_roots( + /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot + /// returned may be earlier than the wall-clock slot. + pub fn rev_iter_block_roots( &self, slot: Slot, - ) -> BestBlockRootsIterator { - BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot) + ) -> ReverseBlockRootIterator { + let state = &self.head().beacon_state; + let block_root = self.head().beacon_block_root; + let block_slot = state.slot; + + let iter = BlockRootsIterator::owned(self.store.clone(), state.clone(), slot); + + ReverseBlockRootIterator::new((block_root, block_slot), iter) } - /// Iterates in reverse (highest to lowest slot) through all state roots from `slot` through to - /// genesis. + /// Iterates through all the `BeaconState` roots and slots, first returning + /// `self.head().beacon_state` then all prior states until either genesis or if the database + /// fails to return a prior state. /// - /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`. - pub fn rev_iter_state_roots(&self, slot: Slot) -> StateRootsIterator { - StateRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot) + /// Iterator returns `(Hash256, Slot)`. + /// + /// ## Note + /// + /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot + /// returned may be earlier than the wall-clock slot. + pub fn rev_iter_state_roots( + &self, + slot: Slot, + ) -> ReverseStateRootIterator { + let state = &self.head().beacon_state; + let state_root = self.head().beacon_state_root; + let state_slot = state.slot; + + let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot); + + ReverseStateRootIterator::new((state_root, state_slot), iter) } /// Returns the block at the given root, if any. @@ -279,8 +288,10 @@ impl BeaconChain { /// Returns a read-lock guarded `BeaconState` which is the `canonical_head` that has been /// updated to match the current slot clock. - pub fn current_state(&self) -> RwLockReadGuard> { - self.state.read() + pub fn speculative_state(&self) -> Result>, Error> { + // TODO: ensure the state has done a catch-up. + + Ok(self.state.read()) } /// Returns a read-lock guarded `CheckPoint` struct for reading the head (as chosen by the diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index b77979b741..74778be32e 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -52,7 +52,7 @@ impl ForkChoice { // been justified for at least 1 epoch ... If no such descendant exists, // set justified_head to finalized_head. let (start_state, start_block_root, start_block_slot) = { - let state = chain.current_state(); + let state = &chain.head().beacon_state; let (block_root, block_slot) = if state.current_epoch() + 1 > state.current_justified_checkpoint.epoch { diff --git a/beacon_node/beacon_chain/src/iter.rs b/beacon_node/beacon_chain/src/iter.rs new file mode 100644 index 0000000000..f73e88afa8 --- /dev/null +++ b/beacon_node/beacon_chain/src/iter.rs @@ -0,0 +1,48 @@ +use store::iter::{BlockRootsIterator, StateRootsIterator}; +use types::{Hash256, Slot}; + +pub type ReverseBlockRootIterator<'a, E, S> = + ReverseHashAndSlotIterator>; +pub type ReverseStateRootIterator<'a, E, S> = + ReverseHashAndSlotIterator>; + +pub type ReverseHashAndSlotIterator = ReverseChainIterator<(Hash256, Slot), I>; + +/// Provides a wrapper for an iterator that returns a given `T` before it starts returning results of +/// the `Iterator`. +pub struct ReverseChainIterator { + first_value_used: bool, + first_value: T, + iter: I, +} + +impl ReverseChainIterator +where + T: Sized, + I: Iterator + Sized, +{ + pub fn new(first_value: T, iter: I) -> Self { + Self { + first_value_used: false, + first_value, + iter, + } + } +} + +impl Iterator for ReverseChainIterator +where + T: Clone, + I: Iterator, +{ + type Item = T; + + fn next(&mut self) -> Option { + if self.first_value_used { + self.iter.next() + } else { + self.first_value_used = true; + Some(self.first_value.clone()) + } + } +} diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index df1de153a2..c2efcad130 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -2,6 +2,7 @@ mod beacon_chain; mod checkpoint; mod errors; mod fork_choice; +mod iter; mod metrics; mod persisted_beacon_chain; pub mod test_utils; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6242b8a0a6..cdcd8bb21e 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -198,7 +198,7 @@ where fn get_state_at_slot(&self, state_slot: Slot) -> BeaconState { let state_root = self .chain - .rev_iter_state_roots(self.chain.current_state().slot - 1) + .rev_iter_state_roots(self.chain.head().beacon_state.slot - 1) .find(|(_hash, slot)| *slot == state_slot) .map(|(hash, _slot)| hash) .expect("could not find state root"); diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index ac001415cd..215e37e7f3 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -266,7 +266,7 @@ impl SimpleSync { fn root_at_slot(&self, target_slot: Slot) -> Option { self.chain - .rev_iter_best_block_roots(target_slot) + .rev_iter_block_roots(target_slot) .take(1) .find(|(_root, slot)| *slot == target_slot) .map(|(root, _slot)| root) @@ -280,6 +280,8 @@ impl SimpleSync { req: BeaconBlockRootsRequest, network: &mut NetworkContext, ) { + let state = &self.chain.head().beacon_state; + debug!( self.log, "BlockRootsRequest"; @@ -290,8 +292,8 @@ impl SimpleSync { let mut roots: Vec = self .chain - .rev_iter_best_block_roots(req.start_slot + req.count) - .take(req.count as usize) + .rev_iter_block_roots(std::cmp::min(req.start_slot + req.count, state.slot)) + .take_while(|(_root, slot)| req.start_slot <= *slot) .map(|(block_root, slot)| BlockRootSlot { slot, block_root }) .collect(); @@ -302,7 +304,7 @@ impl SimpleSync { "peer" => format!("{:?}", peer_id), "msg" => "Failed to return all requested hashes", "start_slot" => req.start_slot, - "current_slot" => self.chain.current_state().slot, + "current_slot" => self.chain.present_slot(), "requested" => req.count, "returned" => roots.len(), ); @@ -389,6 +391,8 @@ impl SimpleSync { req: BeaconBlockHeadersRequest, network: &mut NetworkContext, ) { + let state = &self.chain.head().beacon_state; + debug!( self.log, "BlockHeadersRequest"; @@ -399,13 +403,10 @@ impl SimpleSync { let count = req.max_headers; // Collect the block roots. - // - // Instead of using `chain.rev_iter_blocks` we collect the roots first. This avoids - // unnecessary block deserialization when `req.skip_slots > 0`. let mut roots: Vec = self .chain - .rev_iter_best_block_roots(req.start_slot + count) - .take(count as usize) + .rev_iter_block_roots(std::cmp::min(req.start_slot + count, state.slot)) + .take_while(|(_root, slot)| req.start_slot <= *slot) .map(|(root, _slot)| root) .collect(); diff --git a/beacon_node/rest_api/src/beacon_node.rs b/beacon_node/rest_api/src/beacon_node.rs index 87d2d3cdcd..bd8d98a53d 100644 --- a/beacon_node/rest_api/src/beacon_node.rs +++ b/beacon_node/rest_api/src/beacon_node.rs @@ -54,7 +54,7 @@ fn get_version(_req: Request) -> APIResult { fn get_genesis_time(req: Request) -> APIResult { let beacon_chain = req.extensions().get::>>().unwrap(); let gen_time = { - let state = beacon_chain.current_state(); + let state = &beacon_chain.head().beacon_state; state.genesis_time }; let body = Body::from( diff --git a/beacon_node/rpc/src/attestation.rs b/beacon_node/rpc/src/attestation.rs index 5ea8368fd8..20425d292d 100644 --- a/beacon_node/rpc/src/attestation.rs +++ b/beacon_node/rpc/src/attestation.rs @@ -40,7 +40,11 @@ impl AttestationService for AttestationServiceInstance { // verify the slot, drop lock on state afterwards { let slot_requested = req.get_slot(); - let state = &self.chain.current_state(); + // TODO: this whole module is legacy and not maintained well. + let state = &self + .chain + .speculative_state() + .expect("This is legacy code and should be removed"); // Start by performing some checks // Check that the AttestationData is for the current slot (otherwise it will not be valid) diff --git a/beacon_node/rpc/src/validator.rs b/beacon_node/rpc/src/validator.rs index b13303e25c..080c828a78 100644 --- a/beacon_node/rpc/src/validator.rs +++ b/beacon_node/rpc/src/validator.rs @@ -29,7 +29,11 @@ impl ValidatorService for ValidatorServiceInstance { trace!(self.log, "RPC request"; "endpoint" => "GetValidatorDuties", "epoch" => req.get_epoch()); let spec = &self.chain.spec; - let state = &self.chain.current_state(); + // TODO: this whole module is legacy and not maintained well. + let state = &self + .chain + .speculative_state() + .expect("This is legacy code and should be removed"); let epoch = Epoch::from(req.get_epoch()); let mut resp = GetDutiesResponse::new(); let resp_validators = resp.mut_active_validators(); diff --git a/beacon_node/store/src/iter.rs b/beacon_node/store/src/iter.rs index 55c525b11f..fc5d806795 100644 --- a/beacon_node/store/src/iter.rs +++ b/beacon_node/store/src/iter.rs @@ -4,20 +4,23 @@ use std::sync::Arc; use types::{BeaconBlock, BeaconState, BeaconStateError, EthSpec, Hash256, Slot}; /// Implemented for types that have ancestors (e.g., blocks, states) that may be iterated over. +/// +/// ## Note +/// +/// It is assumed that all ancestors for this object are stored in the database. If this is not the +/// case, the iterator will start returning `None` prior to genesis. pub trait AncestorIter { /// Returns an iterator over the roots of the ancestors of `self`. fn try_iter_ancestor_roots(&self, store: Arc) -> Option; } -impl<'a, U: Store, E: EthSpec> AncestorIter> - for BeaconBlock -{ +impl<'a, U: Store, E: EthSpec> AncestorIter> for BeaconBlock { /// Iterates across all the prior block roots of `self`, starting at the most recent and ending /// at genesis. - fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { + fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { let state = store.get::>(&self.state_root).ok()??; - Some(BestBlockRootsIterator::owned(store, state, self.slot)) + Some(BlockRootsIterator::owned(store, state, self.slot)) } } @@ -116,11 +119,6 @@ impl<'a, T: EthSpec, U: Store> Iterator for BlockIterator<'a, T, U> { /// exhausted. /// /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`. -/// -/// ## Notes -/// -/// See [`BestBlockRootsIterator`](struct.BestBlockRootsIterator.html), which has different -/// `start_slot` logic. #[derive(Clone)] pub struct BlockRootsIterator<'a, T: EthSpec, U> { store: Arc, @@ -180,104 +178,6 @@ impl<'a, T: EthSpec, U: Store> Iterator for BlockRootsIterator<'a, T, U> { } } -/// Iterates backwards through block roots with `start_slot` highest possible value -/// `<= beacon_state.slot`. -/// -/// The distinction between `BestBlockRootsIterator` and `BlockRootsIterator` is: -/// -/// - `BestBlockRootsIterator` uses best-effort slot. When `start_slot` is greater than the latest available block root -/// on `beacon_state`, returns `Some(root, slot)` where `slot` is the latest available block -/// root. -/// - `BlockRootsIterator` is strict about `start_slot`. When `start_slot` is greater than the latest available block root -/// on `beacon_state`, returns `None`. -/// -/// This is distinct from `BestBlockRootsIterator`. -/// -/// Uses the `block_roots` field of `BeaconState` to as the source of block roots and will -/// perform a lookup on the `Store` for a prior `BeaconState` if `block_roots` has been -/// exhausted. -/// -/// Returns `None` for roots prior to genesis or when there is an error reading from `Store`. -#[derive(Clone)] -pub struct BestBlockRootsIterator<'a, T: EthSpec, U> { - store: Arc, - beacon_state: Cow<'a, BeaconState>, - slot: Slot, -} - -impl<'a, T: EthSpec, U: Store> BestBlockRootsIterator<'a, T, U> { - /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { - let mut slot = start_slot; - if slot >= beacon_state.slot { - // Slot may be too high. - slot = beacon_state.slot; - if beacon_state.get_block_root(slot).is_err() { - slot -= 1; - } - } - - Self { - store, - beacon_state: Cow::Borrowed(beacon_state), - slot: slot + 1, - } - } - - /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { - let mut slot = start_slot; - if slot >= beacon_state.slot { - // Slot may be too high. - slot = beacon_state.slot; - // TODO: Use a function other than `get_block_root` as this will always return `Err()` - // for slot = state.slot. - if beacon_state.get_block_root(slot).is_err() { - slot -= 1; - } - } - - Self { - store, - beacon_state: Cow::Owned(beacon_state), - slot: slot + 1, - } - } -} - -impl<'a, T: EthSpec, U: Store> Iterator for BestBlockRootsIterator<'a, T, U> { - type Item = (Hash256, Slot); - - fn next(&mut self) -> Option { - if self.slot == 0 { - // End of Iterator - return None; - } - - self.slot -= 1; - - match self.beacon_state.get_block_root(self.slot) { - Ok(root) => Some((*root, self.slot)), - Err(BeaconStateError::SlotOutOfBounds) => { - // Read a `BeaconState` from the store that has access to prior historical root. - let beacon_state: BeaconState = { - // Load the earliest state from disk. - let new_state_root = self.beacon_state.get_oldest_state_root().ok()?; - - self.store.get(&new_state_root).ok()? - }?; - - self.beacon_state = Cow::Owned(beacon_state); - - let root = self.beacon_state.get_block_root(self.slot).ok()?; - - Some((*root, self.slot)) - } - _ => None, - } - } -} - #[cfg(test)] mod test { use super::*; @@ -337,49 +237,6 @@ mod test { } } - #[test] - fn best_block_root_iter() { - let store = Arc::new(MemoryStore::open()); - let slots_per_historical_root = MainnetEthSpec::slots_per_historical_root(); - - let mut state_a: BeaconState = get_state(); - let mut state_b: BeaconState = get_state(); - - state_a.slot = Slot::from(slots_per_historical_root); - state_b.slot = Slot::from(slots_per_historical_root * 2); - - let mut hashes = (0..).into_iter().map(|i| Hash256::from(i)); - - for root in &mut state_a.block_roots[..] { - *root = hashes.next().unwrap() - } - for root in &mut state_b.block_roots[..] { - *root = hashes.next().unwrap() - } - - let state_a_root = hashes.next().unwrap(); - state_b.state_roots[0] = state_a_root; - store.put(&state_a_root, &state_a).unwrap(); - - let iter = BestBlockRootsIterator::new(store.clone(), &state_b, state_b.slot); - - assert!( - iter.clone().find(|(_root, slot)| *slot == 0).is_some(), - "iter should contain zero slot" - ); - - let mut collected: Vec<(Hash256, Slot)> = iter.collect(); - collected.reverse(); - - let expected_len = 2 * MainnetEthSpec::slots_per_historical_root(); - - assert_eq!(collected.len(), expected_len); - - for i in 0..expected_len { - assert_eq!(collected[i].0, Hash256::from(i as u64)); - } - } - #[test] fn state_root_iter() { let store = Arc::new(MemoryStore::open()); diff --git a/eth2/lmd_ghost/tests/test.rs b/eth2/lmd_ghost/tests/test.rs index fbe3855605..0ac263638c 100644 --- a/eth2/lmd_ghost/tests/test.rs +++ b/eth2/lmd_ghost/tests/test.rs @@ -10,7 +10,7 @@ use lmd_ghost::{LmdGhost, ThreadSafeReducedTree as BaseThreadSafeReducedTree}; use rand::{prelude::*, rngs::StdRng}; use std::sync::Arc; use store::{ - iter::{AncestorIter, BestBlockRootsIterator}, + iter::{AncestorIter, BlockRootsIterator}, MemoryStore, Store, }; use types::{BeaconBlock, EthSpec, Hash256, MinimalEthSpec, Slot}; @@ -159,7 +159,7 @@ fn get_ancestor_roots( .expect("block should exist") .expect("store should not error"); - as AncestorIter<_, BestBlockRootsIterator>>::try_iter_ancestor_roots( + as AncestorIter<_, BlockRootsIterator>>::try_iter_ancestor_roots( &block, store, ) .expect("should be able to create ancestor iter") From edd99fafb6c42212bda9bcaa8f77d11c15515e23 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Thu, 25 Jul 2019 15:08:18 +0200 Subject: [PATCH 11/34] Getting attestation slot via helper method --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- beacon_node/beacon_chain/src/fork_choice.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 67d9281277..8a9421a1b7 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -10,7 +10,7 @@ use operation_pool::{OperationPool, PersistedOperationPool}; use parking_lot::{RwLock, RwLockReadGuard}; use slot_clock::SlotClock; use state_processing::per_block_processing::errors::{ - AttestationValidationError, AttesterSlashingValidationError, DepositValidationError, + AttesterSlashingValidationError, DepositValidationError, ExitValidationError, ProposerSlashingValidationError, TransferValidationError, }; use state_processing::{ diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 92b683590a..0f98ac9ce9 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -174,13 +174,13 @@ impl ForkChoice { &attestation.aggregation_bitfield, )?; - let target_slot = attestation.data.target_epoch.start_slot(T::EthSpec::slots_per_epoch()); + let block_slot = state.get_attestation_slot(&attestation.data)?; Ok(validator_indices .iter() .find(|&&v| { match self.backend.latest_message(v) { - Some((_, slot)) => target_slot > slot, + Some((_, slot)) => block_slot > slot, None => true } }).is_some()) From 78f39115229e4bf5ace88193303443c50297e613 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Fri, 26 Jul 2019 12:48:17 +0200 Subject: [PATCH 12/34] Refactored attestation creation in test utils --- beacon_node/beacon_chain/src/test_utils.rs | 127 ++++++++++++--------- 1 file changed, 72 insertions(+), 55 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 991d29418e..c43309cbfd 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -8,11 +8,7 @@ use std::sync::Arc; use store::MemoryStore; use store::Store; use tree_hash::{SignedRoot, TreeHash}; -use types::{ - test_utils::TestingBeaconStateBuilder, AggregateSignature, Attestation, - AttestationDataAndCustodyBit, BeaconBlock, BeaconState, Bitfield, ChainSpec, Domain, EthSpec, - Hash256, Keypair, RelativeEpoch, SecretKey, Signature, Slot, -}; +use types::{test_utils::TestingBeaconStateBuilder, AggregateSignature, Attestation, AttestationDataAndCustodyBit, BeaconBlock, BeaconState, Bitfield, ChainSpec, Domain, EthSpec, Hash256, Keypair, RelativeEpoch, SecretKey, Signature, Slot, CrosslinkCommittee}; pub use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY}; @@ -171,7 +167,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_attestations_to_op_pool( + self.add_attestations_to_chain( &attestation_strategy, &new_state, block_root, @@ -256,18 +252,16 @@ where (block, state) } - /// Adds attestations to the `BeaconChain` operations pool to be included in future blocks. + /// Adds attestations to the `BeaconChain` operations pool and fork choice. /// /// The `attestation_strategy` dictates which validators should attest. - fn add_attestations_to_op_pool( + fn add_attestations_to_chain( &self, attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, head_block_slot: Slot, ) { - let spec = &self.spec; - let fork = &state.fork; let attesting_validators: Vec = match attestation_strategy { AttestationStrategy::AllValidators => (0..self.keypairs.len()).collect(), @@ -279,55 +273,18 @@ where .expect("should get committees") .iter() .for_each(|cc| { - let committee_size = cc.committee.len(); - for (i, validator_index) in cc.committee.iter().enumerate() { // Note: searching this array is worst-case `O(n)`. A hashset could be a better // alternative. if attesting_validators.contains(validator_index) { - let data = self - .chain - .produce_attestation_data_for_block( - cc.shard, - head_block_root, - head_block_slot, - state, - ) - .expect("should produce attestation data"); - - let mut aggregation_bitfield = Bitfield::new(); - aggregation_bitfield.set(i, true); - aggregation_bitfield.set(committee_size, false); - - let mut custody_bitfield = Bitfield::new(); - custody_bitfield.set(committee_size, false); - - let signature = { - let message = AttestationDataAndCustodyBit { - data: data.clone(), - custody_bit: false, - } - .tree_hash_root(); - - let domain = - spec.get_domain(data.target_epoch, Domain::Attestation, fork); - - let mut agg_sig = AggregateSignature::new(); - agg_sig.add(&Signature::new( - &message, - domain, - self.get_sk(*validator_index), - )); - - agg_sig - }; - - let attestation = Attestation { - aggregation_bitfield, - data, - custody_bitfield, - signature, - }; + let attestation = self.create_attestation( + *validator_index, + cc, + head_block_root, + head_block_slot, + state, + i + ); self.chain .process_attestation(attestation) @@ -337,6 +294,66 @@ where }); } + /// Creates an attestation for a validator with the given data. + pub fn create_attestation( + &self, + validator_index: usize, + crosslink_committee: &CrosslinkCommittee, + head_block_root: Hash256, + head_block_slot: Slot, + state: &BeaconState, + bitfield_index: usize + ) -> Attestation { + let committee_size = crosslink_committee.committee.len(); + let spec = &self.spec; + let fork = &state.fork; + + let data = self + .chain + .produce_attestation_data_for_block( + crosslink_committee.shard, + head_block_root, + head_block_slot, + state, + ) + .expect("should produce attestation data"); + + let mut aggregation_bitfield = Bitfield::new(); + aggregation_bitfield.set(bitfield_index, true); + aggregation_bitfield.set(committee_size, false); + + let mut custody_bitfield = Bitfield::new(); + custody_bitfield.set(committee_size, false); + + let signature = { + let message = AttestationDataAndCustodyBit { + data: data.clone(), + custody_bit: false, + } + .tree_hash_root(); + + let domain = + spec.get_domain(data.target_epoch, Domain::Attestation, fork); + + let mut agg_sig = AggregateSignature::new(); + agg_sig.add(&Signature::new( + &message, + domain, + self.get_sk(validator_index), + )); + + agg_sig + }; + + Attestation { + aggregation_bitfield, + data, + custody_bitfield, + signature, + } + } + + /// Returns the secret key for the given validator index. fn get_sk(&self, validator_index: usize) -> &SecretKey { &self.keypairs[validator_index].sk From dcac8d56bd163845c0b928abf1ca54a85e179fd2 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Sat, 27 Jul 2019 22:32:11 +0200 Subject: [PATCH 13/34] Revert "Refactored attestation creation in test utils" This reverts commit 4d277fe4239a7194758b18fb5c00dfe0b8231306. --- beacon_node/beacon_chain/src/test_utils.rs | 127 +++++++++------------ 1 file changed, 55 insertions(+), 72 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index c43309cbfd..991d29418e 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -8,7 +8,11 @@ use std::sync::Arc; use store::MemoryStore; use store::Store; use tree_hash::{SignedRoot, TreeHash}; -use types::{test_utils::TestingBeaconStateBuilder, AggregateSignature, Attestation, AttestationDataAndCustodyBit, BeaconBlock, BeaconState, Bitfield, ChainSpec, Domain, EthSpec, Hash256, Keypair, RelativeEpoch, SecretKey, Signature, Slot, CrosslinkCommittee}; +use types::{ + test_utils::TestingBeaconStateBuilder, AggregateSignature, Attestation, + AttestationDataAndCustodyBit, BeaconBlock, BeaconState, Bitfield, ChainSpec, Domain, EthSpec, + Hash256, Keypair, RelativeEpoch, SecretKey, Signature, Slot, +}; pub use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY}; @@ -167,7 +171,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_attestations_to_chain( + self.add_attestations_to_op_pool( &attestation_strategy, &new_state, block_root, @@ -252,16 +256,18 @@ where (block, state) } - /// Adds attestations to the `BeaconChain` operations pool and fork choice. + /// Adds attestations to the `BeaconChain` operations pool to be included in future blocks. /// /// The `attestation_strategy` dictates which validators should attest. - fn add_attestations_to_chain( + fn add_attestations_to_op_pool( &self, attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, head_block_slot: Slot, ) { + let spec = &self.spec; + let fork = &state.fork; let attesting_validators: Vec = match attestation_strategy { AttestationStrategy::AllValidators => (0..self.keypairs.len()).collect(), @@ -273,18 +279,55 @@ where .expect("should get committees") .iter() .for_each(|cc| { + let committee_size = cc.committee.len(); + for (i, validator_index) in cc.committee.iter().enumerate() { // Note: searching this array is worst-case `O(n)`. A hashset could be a better // alternative. if attesting_validators.contains(validator_index) { - let attestation = self.create_attestation( - *validator_index, - cc, - head_block_root, - head_block_slot, - state, - i - ); + let data = self + .chain + .produce_attestation_data_for_block( + cc.shard, + head_block_root, + head_block_slot, + state, + ) + .expect("should produce attestation data"); + + let mut aggregation_bitfield = Bitfield::new(); + aggregation_bitfield.set(i, true); + aggregation_bitfield.set(committee_size, false); + + let mut custody_bitfield = Bitfield::new(); + custody_bitfield.set(committee_size, false); + + let signature = { + let message = AttestationDataAndCustodyBit { + data: data.clone(), + custody_bit: false, + } + .tree_hash_root(); + + let domain = + spec.get_domain(data.target_epoch, Domain::Attestation, fork); + + let mut agg_sig = AggregateSignature::new(); + agg_sig.add(&Signature::new( + &message, + domain, + self.get_sk(*validator_index), + )); + + agg_sig + }; + + let attestation = Attestation { + aggregation_bitfield, + data, + custody_bitfield, + signature, + }; self.chain .process_attestation(attestation) @@ -294,66 +337,6 @@ where }); } - /// Creates an attestation for a validator with the given data. - pub fn create_attestation( - &self, - validator_index: usize, - crosslink_committee: &CrosslinkCommittee, - head_block_root: Hash256, - head_block_slot: Slot, - state: &BeaconState, - bitfield_index: usize - ) -> Attestation { - let committee_size = crosslink_committee.committee.len(); - let spec = &self.spec; - let fork = &state.fork; - - let data = self - .chain - .produce_attestation_data_for_block( - crosslink_committee.shard, - head_block_root, - head_block_slot, - state, - ) - .expect("should produce attestation data"); - - let mut aggregation_bitfield = Bitfield::new(); - aggregation_bitfield.set(bitfield_index, true); - aggregation_bitfield.set(committee_size, false); - - let mut custody_bitfield = Bitfield::new(); - custody_bitfield.set(committee_size, false); - - let signature = { - let message = AttestationDataAndCustodyBit { - data: data.clone(), - custody_bit: false, - } - .tree_hash_root(); - - let domain = - spec.get_domain(data.target_epoch, Domain::Attestation, fork); - - let mut agg_sig = AggregateSignature::new(); - agg_sig.add(&Signature::new( - &message, - domain, - self.get_sk(validator_index), - )); - - agg_sig - }; - - Attestation { - aggregation_bitfield, - data, - custody_bitfield, - signature, - } - } - - /// Returns the secret key for the given validator index. fn get_sk(&self, validator_index: usize) -> &SecretKey { &self.keypairs[validator_index].sk From f4b169ce80b8b5acbc1b32b9ab488acabdb0bc84 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Mon, 29 Jul 2019 22:51:42 +0200 Subject: [PATCH 14/34] Integration tests for free attestation processing --- beacon_node/beacon_chain/src/fork_choice.rs | 7 +- beacon_node/beacon_chain/src/test_utils.rs | 6 +- beacon_node/beacon_chain/tests/tests.rs | 92 ++++++++++++++++++++- 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 0f98ac9ce9..7d1830afec 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -3,7 +3,7 @@ use lmd_ghost::LmdGhost; use state_processing::common::get_attesting_indices_unsorted; use std::sync::Arc; use store::{Error as StoreError, Store}; -use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256}; +use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot}; use state_processing::common; type Result = std::result::Result; @@ -186,6 +186,11 @@ impl ForkChoice { }).is_some()) } + // Returns the latest message for a given validator + pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { + self.backend.latest_message(validator_index) + } + /// Inform the fork choice that the given block (and corresponding root) have been finalized so /// it may prune it's storage. /// diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 991d29418e..9a440b8877 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -171,7 +171,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_attestations_to_op_pool( + self.add_free_attestations( &attestation_strategy, &new_state, block_root, @@ -256,10 +256,10 @@ where (block, state) } - /// Adds attestations to the `BeaconChain` operations pool to be included in future blocks. + /// Adds attestations to the `BeaconChain` operations pool and fork choice. /// /// The `attestation_strategy` dictates which validators should attest. - fn add_attestations_to_op_pool( + fn add_free_attestations( &self, attestation_strategy: &AttestationStrategy, state: &BeaconState, diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 882d9f2355..2f4e5badeb 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -1,4 +1,3 @@ -#![cfg(not(debug_assertions))] use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, CommonTypes, PersistedBeaconChain, @@ -8,7 +7,7 @@ use lmd_ghost::ThreadSafeReducedTree; use rand::Rng; use store::{MemoryStore, Store}; use types::test_utils::{SeedableRng, TestRandom, XorShiftRng}; -use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, Slot}; +use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, Slot, RelativeEpoch}; // Should ideally be divisible by 3. pub const VALIDATOR_COUNT: usize = 24; @@ -265,3 +264,92 @@ fn roundtrip_operation_pool() { assert_eq!(harness.chain.op_pool, restored_op_pool); } + +#[test] +fn free_attestations_added_to_fork_choice_some_none() { + let num_blocks_produced = MinimalEthSpec::slots_per_epoch() / 2; + + let harness = get_harness(VALIDATOR_COUNT); + + harness.extend_chain( + num_blocks_produced as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ); + + let state = &harness.chain.head().beacon_state; + let fork_choice = &harness.chain.fork_choice; + + let validators: Vec = (0..VALIDATOR_COUNT).collect(); + let slots: Vec = validators + .iter() + .map(|&v| + state.get_attestation_duties(v, RelativeEpoch::Current) + .expect("should get attester duties") + .unwrap() + .slot + ).collect(); + let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); + + for (validator, slot) in validator_slots.clone() { + let latest_message = fork_choice.latest_message(*validator); + + if slot <= num_blocks_produced && slot != 0{ + assert_eq!( + latest_message.unwrap().1, slot, + "Latest message slot should be equal to attester duty." + ) + } else { + assert!( + latest_message.is_none(), + "Latest message slot should be None." + ) + } + } +} + +#[test] +fn free_attestations_added_to_fork_choice_all_updated() { + let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 2 - 1; + + let harness = get_harness(VALIDATOR_COUNT); + + harness.extend_chain( + num_blocks_produced as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ); + + let state = &harness.chain.head().beacon_state; + let fork_choice = &harness.chain.fork_choice; + + let validators: Vec = (0..VALIDATOR_COUNT).collect(); + let slots: Vec = validators + .iter() + .map(|&v| + state.get_attestation_duties(v, RelativeEpoch::Current) + .expect("should get attester duties") + .unwrap() + .slot + ).collect(); + let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); + + for (validator, slot) in validator_slots { + let latest_message = fork_choice.latest_message(*validator); + + assert_eq!( + latest_message.unwrap().1, slot, + "Latest message slot should be equal to attester duty." + ); + + if slot != num_blocks_produced { + let block_root = state.get_block_root(slot) + .expect("Should get block root at slot"); + + assert_eq!( + latest_message.unwrap().0, *block_root, + "Latest message block root should be equal to block at slot." + ); + } + } +} \ No newline at end of file From c431bd993e9ed0d7aeffd37574e30a416955ea9c Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Tue, 6 Aug 2019 14:56:13 +0200 Subject: [PATCH 15/34] Implicit conflicts resolved. --- beacon_node/beacon_chain/src/beacon_chain.rs | 10 +++++----- beacon_node/beacon_chain/src/fork_choice.rs | 12 ++++-------- beacon_node/beacon_chain/tests/tests.rs | 1 + 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3e8467a492..49e2cec838 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -520,8 +520,8 @@ impl BeaconChain { if let Some(state) = self.get_attestation_state(&attestation) { if self.fork_choice.should_process_attestation(&state, &attestation)? { - let indexed_attestation = common::convert_to_indexed(&state, &attestation)?; - per_block_processing::verify_indexed_attestation(&state, &indexed_attestation, &self.spec)?; + let indexed_attestation = common::get_indexed_attestation(&state, &attestation)?; + per_block_processing::is_valid_indexed_attestation(&state, &indexed_attestation, &self.spec)?; self.fork_choice.process_attestation(&state, &attestation)?; } } @@ -540,14 +540,14 @@ impl BeaconChain { } /// Retrieves the `BeaconState` used to create the attestation. - fn get_attestation_state(&self, attestation: &Attestation) -> Option> { + fn get_attestation_state(&self, attestation: &Attestation) -> Option> { // Current state is used if the attestation targets a historic block and a slot within an // equal or adjacent epoch. let slots_per_epoch = T::EthSpec::slots_per_epoch(); let min_slot = (self.state.read().slot.epoch(slots_per_epoch) - 1).start_slot(slots_per_epoch); let blocks = BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), self.state.read().slot.clone()); for (root, slot) in blocks { - if root == attestation.data.target_root { + if root == attestation.data.target.root { return Some(self.state.read().clone()); } @@ -557,7 +557,7 @@ impl BeaconChain { }; // A different state is retrieved from the database. - match self.store.get::(&attestation.data.target_root) { + match self.store.get::>(&attestation.data.target.root) { Ok(Some(block)) => match self.store.get::>(&block.state_root) { Ok(state) => state, _ => None diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 29b3664f18..3900575aee 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -4,7 +4,6 @@ use state_processing::common::get_attesting_indices; use std::sync::Arc; use store::{Error as StoreError, Store}; use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot}; -use state_processing::common; type Result = std::result::Result; @@ -172,14 +171,11 @@ impl ForkChoice { } /// Determines whether or not the given attestation contains a latest message. - pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> Result { - let validator_indices = common::get_attesting_indices_unsorted( - state, - &attestation.data, - &attestation.aggregation_bitfield, - )?; + pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> Result { + let validator_indices = + get_attesting_indices(state, &attestation.data, &attestation.aggregation_bits)?; - let block_slot = state.get_attestation_slot(&attestation.data)?; + let block_slot = state.get_attestation_data_slot(&attestation.data)?; Ok(validator_indices .iter() diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 730b8ec67e..cc1a84973e 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -1,3 +1,4 @@ +#![cfg(not(debug_assertions))] use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, CommonTypes, PersistedBeaconChain, From ce73705498f3c39504b2822f67f422da7a3bfdb2 Mon Sep 17 00:00:00 2001 From: Grant Wuerker Date: Tue, 6 Aug 2019 19:17:15 +0200 Subject: [PATCH 16/34] formatting --- beacon_node/beacon_chain/src/beacon_chain.rs | 52 +++++++++++++------- beacon_node/beacon_chain/src/errors.rs | 6 ++- beacon_node/beacon_chain/src/fork_choice.rs | 22 +++++---- beacon_node/beacon_chain/src/test_utils.rs | 7 +-- beacon_node/beacon_chain/tests/tests.rs | 34 ++++++++----- beacon_node/rpc/src/attestation.rs | 14 ++++-- eth2/lmd_ghost/src/reduced_tree.rs | 6 +-- 7 files changed, 85 insertions(+), 56 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 49e2cec838..0becbf2c9c 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3,6 +3,7 @@ use crate::errors::{BeaconChainError as Error, BlockProductionError}; use crate::fork_choice::{Error as ForkChoiceError, ForkChoice}; use crate::metrics::Metrics; use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY}; +use crate::BeaconChainError; use lmd_ghost::LmdGhost; use log::trace; use operation_pool::DepositInsertStatus; @@ -11,19 +12,18 @@ use parking_lot::{RwLock, RwLockReadGuard}; use slog::{error, info, warn, Logger}; use slot_clock::SlotClock; use state_processing::per_block_processing::errors::{ - AttesterSlashingValidationError, DepositValidationError, - ExitValidationError, ProposerSlashingValidationError, TransferValidationError, + AttesterSlashingValidationError, DepositValidationError, ExitValidationError, + ProposerSlashingValidationError, TransferValidationError, }; use state_processing::{ - per_block_processing, per_block_processing_without_verifying_block_signature, - per_slot_processing, BlockProcessingError, common + common, per_block_processing, per_block_processing_without_verifying_block_signature, + per_slot_processing, BlockProcessingError, }; use std::sync::Arc; use store::iter::{BestBlockRootsIterator, BlockIterator, BlockRootsIterator, StateRootsIterator}; use store::{Error as DBError, Store}; use tree_hash::TreeHash; use types::*; -use crate::BeaconChainError; // Text included in blocks. // Must be 32-bytes or panic. @@ -511,17 +511,21 @@ impl BeaconChain { /// /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation /// if possible. - pub fn process_attestation( - &self, - attestation: Attestation, - ) -> Result<(), Error> { + pub fn process_attestation(&self, attestation: Attestation) -> Result<(), Error> { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); if let Some(state) = self.get_attestation_state(&attestation) { - if self.fork_choice.should_process_attestation(&state, &attestation)? { + if self + .fork_choice + .should_process_attestation(&state, &attestation)? + { let indexed_attestation = common::get_indexed_attestation(&state, &attestation)?; - per_block_processing::is_valid_indexed_attestation(&state, &indexed_attestation, &self.spec)?; + per_block_processing::is_valid_indexed_attestation( + &state, + &indexed_attestation, + &self.spec, + )?; self.fork_choice.process_attestation(&state, &attestation)?; } } @@ -540,12 +544,20 @@ impl BeaconChain { } /// Retrieves the `BeaconState` used to create the attestation. - fn get_attestation_state(&self, attestation: &Attestation) -> Option> { + fn get_attestation_state( + &self, + attestation: &Attestation, + ) -> Option> { // Current state is used if the attestation targets a historic block and a slot within an // equal or adjacent epoch. let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let min_slot = (self.state.read().slot.epoch(slots_per_epoch) - 1).start_slot(slots_per_epoch); - let blocks = BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), self.state.read().slot.clone()); + let min_slot = + (self.state.read().slot.epoch(slots_per_epoch) - 1).start_slot(slots_per_epoch); + let blocks = BestBlockRootsIterator::owned( + self.store.clone(), + self.state.read().clone(), + self.state.read().slot.clone(), + ); for (root, slot) in blocks { if root == attestation.data.target.root { return Some(self.state.read().clone()); @@ -554,15 +566,18 @@ impl BeaconChain { if slot == min_slot { break; } - }; + } // A different state is retrieved from the database. - match self.store.get::>(&attestation.data.target.root) { + match self + .store + .get::>(&attestation.data.target.root) + { Ok(Some(block)) => match self.store.get::>(&block.state_root) { Ok(state) => state, - _ => None + _ => None, }, - _ => None + _ => None, } } @@ -1031,4 +1046,3 @@ impl From for Error { Error::BeaconStateError(e) } } - diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 4e2170ca84..266c598ac8 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -1,9 +1,11 @@ use crate::fork_choice::Error as ForkChoiceError; use crate::metrics::Error as MetricsError; +use state_processing::per_block_processing::errors::{ + AttestationValidationError, IndexedAttestationValidationError, +}; use state_processing::BlockProcessingError; use state_processing::SlotProcessingError; use types::*; -use state_processing::per_block_processing::errors::{AttestationValidationError, IndexedAttestationValidationError}; macro_rules! easy_from_to { ($from: ident, $to: ident) => { @@ -33,7 +35,7 @@ pub enum BeaconChainError { SlotProcessingError(SlotProcessingError), MetricsError(String), AttestationValidationError(AttestationValidationError), - IndexedAttestationValidationError(IndexedAttestationValidationError) + IndexedAttestationValidationError(IndexedAttestationValidationError), } easy_from_to!(SlotProcessingError, BeaconChainError); diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 3900575aee..d16a8f9a83 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -3,7 +3,9 @@ use lmd_ghost::LmdGhost; use state_processing::common::get_attesting_indices; use std::sync::Arc; use store::{Error as StoreError, Store}; -use types::{Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot}; +use types::{ + Attestation, BeaconBlock, BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, Slot, +}; type Result = std::result::Result; @@ -171,7 +173,11 @@ impl ForkChoice { } /// Determines whether or not the given attestation contains a latest message. - pub fn should_process_attestation(&self, state: &BeaconState, attestation: &Attestation) -> Result { + pub fn should_process_attestation( + &self, + state: &BeaconState, + attestation: &Attestation, + ) -> Result { let validator_indices = get_attesting_indices(state, &attestation.data, &attestation.aggregation_bits)?; @@ -179,12 +185,11 @@ impl ForkChoice { Ok(validator_indices .iter() - .find(|&&v| { - match self.backend.latest_message(v) { - Some((_, slot)) => block_slot > slot, - None => true - } - }).is_some()) + .find(|&&v| match self.backend.latest_message(v) { + Some((_, slot)) => block_slot > slot, + None => true, + }) + .is_some()) } // Returns the latest message for a given validator @@ -224,4 +229,3 @@ impl From for Error { Error::BackendError(e) } } - diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 8f0d4c8ee4..ab1a31690d 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -178,12 +178,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_free_attestations( - &attestation_strategy, - &new_state, - block_root, - slot, - ); + self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot); } else { panic!("block should be successfully processed: {:?}", outcome); } diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index cc1a84973e..5b8a09fafc 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -8,7 +8,7 @@ use lmd_ghost::ThreadSafeReducedTree; use rand::Rng; use store::{MemoryStore, Store}; use types::test_utils::{SeedableRng, TestRandom, XorShiftRng}; -use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, Slot, RelativeEpoch}; +use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, RelativeEpoch, Slot}; // Should ideally be divisible by 3. pub const VALIDATOR_COUNT: usize = 24; @@ -270,20 +270,23 @@ fn free_attestations_added_to_fork_choice_some_none() { let validators: Vec = (0..VALIDATOR_COUNT).collect(); let slots: Vec = validators .iter() - .map(|&v| - state.get_attestation_duties(v, RelativeEpoch::Current) + .map(|&v| { + state + .get_attestation_duties(v, RelativeEpoch::Current) .expect("should get attester duties") .unwrap() .slot - ).collect(); + }) + .collect(); let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); for (validator, slot) in validator_slots.clone() { let latest_message = fork_choice.latest_message(*validator); - if slot <= num_blocks_produced && slot != 0{ + if slot <= num_blocks_produced && slot != 0 { assert_eq!( - latest_message.unwrap().1, slot, + latest_message.unwrap().1, + slot, "Latest message slot should be equal to attester duty." ) } else { @@ -313,30 +316,35 @@ fn free_attestations_added_to_fork_choice_all_updated() { let validators: Vec = (0..VALIDATOR_COUNT).collect(); let slots: Vec = validators .iter() - .map(|&v| - state.get_attestation_duties(v, RelativeEpoch::Current) + .map(|&v| { + state + .get_attestation_duties(v, RelativeEpoch::Current) .expect("should get attester duties") .unwrap() .slot - ).collect(); + }) + .collect(); let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); for (validator, slot) in validator_slots { let latest_message = fork_choice.latest_message(*validator); assert_eq!( - latest_message.unwrap().1, slot, + latest_message.unwrap().1, + slot, "Latest message slot should be equal to attester duty." ); if slot != num_blocks_produced { - let block_root = state.get_block_root(slot) + let block_root = state + .get_block_root(slot) .expect("Should get block root at slot"); assert_eq!( - latest_message.unwrap().0, *block_root, + latest_message.unwrap().0, + *block_root, "Latest message block root should be equal to block at slot." ); } } -} \ No newline at end of file +} diff --git a/beacon_node/rpc/src/attestation.rs b/beacon_node/rpc/src/attestation.rs index c7b3a5711f..00a6431519 100644 --- a/beacon_node/rpc/src/attestation.rs +++ b/beacon_node/rpc/src/attestation.rs @@ -1,4 +1,4 @@ -use beacon_chain::{BeaconChain, BeaconChainTypes, BeaconChainError}; +use beacon_chain::{BeaconChain, BeaconChainError, BeaconChainTypes}; use eth2_libp2p::PubsubMessage; use eth2_libp2p::TopicBuilder; use eth2_libp2p::BEACON_ATTESTATION_TOPIC; @@ -179,7 +179,11 @@ impl AttestationService for AttestationServiceInstance { "error" => format!("{:?}", e), ); resp.set_success(false); - resp.set_msg(format!("InvalidIndexedAttestation: {:?}", e).as_bytes().to_vec()); + resp.set_msg( + format!("InvalidIndexedAttestation: {:?}", e) + .as_bytes() + .to_vec(), + ); } Err(e) => { // Some other error @@ -190,7 +194,11 @@ impl AttestationService for AttestationServiceInstance { "error" => format!("{:?}", e), ); resp.set_success(false); - resp.set_msg(format!("There was a beacon chain error: {:?}", e).as_bytes().to_vec()); + resp.set_msg( + format!("There was a beacon chain error: {:?}", e) + .as_bytes() + .to_vec(), + ); } }; diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index 0ef78c37e9..5d70748042 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -111,9 +111,7 @@ where } fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { - self.core - .write() - .latest_message(validator_index) + self.core.write().latest_message(validator_index) } } @@ -263,7 +261,7 @@ where pub fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)> { match self.latest_votes.get(validator_index) { Some(v) => Some((v.hash.clone(), v.slot.clone())), - None => None + None => None, } } From 2c3fc318bafcd230ff6b9c3c44519d2f6197018e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 7 Aug 2019 13:20:15 +1000 Subject: [PATCH 17/34] Do first pass on Grants code --- beacon_node/beacon_chain/src/beacon_chain.rs | 143 ++++++++++++++++--- beacon_node/beacon_chain/src/errors.rs | 3 + beacon_node/network/src/sync/simple_sync.rs | 7 +- eth2/types/src/beacon_block.rs | 5 + 4 files changed, 141 insertions(+), 17 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0becbf2c9c..fed48036d9 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -54,6 +54,12 @@ pub enum BlockProcessingOutcome { PerBlockProcessingError(BlockProcessingError), } +#[derive(Debug, PartialEq)] +pub enum AttestationProcessingOutcome { + Processed, + UnknownHeadBlock { beacon_block_root: Hash256 }, +} + pub trait BeaconChainTypes { type Store: store::Store; type SlotClock: slot_clock::SlotClock; @@ -511,28 +517,114 @@ impl BeaconChain { /// /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation /// if possible. - pub fn process_attestation(&self, attestation: Attestation) -> Result<(), Error> { + pub fn process_attestation( + &self, + attestation: Attestation, + ) -> Result { + // From the store, load the attestation's "head block". + // + // An honest validator would have set this block to be the head of the chain (i.e., the + // result of running fork choice). + if let Some(attestation_head_block) = self + .store + .get::>(&attestation.data.beacon_block_root)? + { + // Attempt to process the attestation using the `self.head()` state. + // + // This is purely an effort to avoid loading a `BeaconState` unnecessarily from the DB. + let outcome: Option> = { + // Take a read lock on the head beacon state. + // + // The purpose of this whole `let processed ...` block is to ensure that the read + // lock is dropped if we don't end up using the head beacon state. + let state = &self.head().beacon_state; + + // If it turns out that the attestation was made using the head state, then there + // is no need to load a state from the database to process the attestation. + if state.current_epoch() == attestation_head_block.epoch() + && state + .get_block_root(attestation_head_block.slot) + .map(|root| *root == attestation.data.beacon_block_root) + .unwrap_or_else(|_| false) + { + // The head state is able to be used to validate this attestation. No need to load + // anything from the database. + Some(self.process_attestation_for_state_and_block( + attestation.clone(), + state, + &attestation_head_block, + )) + } else { + None + } + }; + + // TODO: we could try and see if the "speculative state" (e.g., self.state) can support + // this, without needing to load it from the db. + + if let Some(result) = outcome { + result + } else { + // The state required to verify this attestation must be loaded from the database. + let mut state: BeaconState = self + .store + .get(&attestation_head_block.state_root)? + .ok_or_else(|| Error::MissingBeaconState(attestation_head_block.state_root))?; + + // Ensure the state loaded from the database matches the state of the attestation + // head block. + for _ in state.slot.as_u64()..attestation_head_block.slot.as_u64() { + per_slot_processing(&mut state, &self.spec)?; + } + + self.process_attestation_for_state_and_block( + attestation, + &state, + &attestation_head_block, + ) + } + } else { + // Reject any block where we have not processed `attestation.data.beacon_block_root`. + // + // This is likely overly restrictive, we could store the attestation for later + // processing. + warn!( + self.log, + "Dropping attestation for unknown block"; + "block" => format!("{}", attestation.data.beacon_block_root) + ); + Ok(AttestationProcessingOutcome::UnknownHeadBlock { + beacon_block_root: attestation.data.beacon_block_root, + }) + } + } + + fn process_attestation_for_state_and_block( + &self, + attestation: Attestation, + state: &BeaconState, + head_block: &BeaconBlock, + ) -> Result { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); - if let Some(state) = self.get_attestation_state(&attestation) { - if self - .fork_choice - .should_process_attestation(&state, &attestation)? - { - let indexed_attestation = common::get_indexed_attestation(&state, &attestation)?; - per_block_processing::is_valid_indexed_attestation( - &state, - &indexed_attestation, - &self.spec, - )?; - self.fork_choice.process_attestation(&state, &attestation)?; - } + if self + .fork_choice + .should_process_attestation(state, &attestation)? + { + // TODO: check validation. + let indexed_attestation = common::get_indexed_attestation(state, &attestation)?; + per_block_processing::is_valid_indexed_attestation( + state, + &indexed_attestation, + &self.spec, + )?; + self.fork_choice.process_attestation(&state, &attestation)?; } let result = self .op_pool - .insert_attestation(attestation, &*self.state.read(), &self.spec); + .insert_attestation(attestation, state, &self.spec); timer.observe_duration(); @@ -540,14 +632,32 @@ impl BeaconChain { self.metrics.attestation_processing_successes.inc(); } - result.map_err(|e| BeaconChainError::AttestationValidationError(e)) + result + .map(|_| AttestationProcessingOutcome::Processed) + .map_err(|e| Error::AttestationValidationError(e)) } + fn state_can_process_attestation( + state: &BeaconState, + data: &AttestationData, + head_block: &BeaconBlock, + ) -> bool { + (state.current_epoch() - 1 <= data.target.epoch) + && (data.target.epoch <= state.current_epoch() + 1) + && state + .get_block_root(head_block.slot) + .map(|root| *root == data.beacon_block_root) + .unwrap_or_else(|_| false) + } + + /* /// Retrieves the `BeaconState` used to create the attestation. fn get_attestation_state( &self, attestation: &Attestation, ) -> Option> { + let state = &self.head().beacon_state; + // Current state is used if the attestation targets a historic block and a slot within an // equal or adjacent epoch. let slots_per_epoch = T::EthSpec::slots_per_epoch(); @@ -580,6 +690,7 @@ impl BeaconChain { _ => None, } } + */ /// Accept some deposit and queue it for inclusion in an appropriate block. pub fn process_deposit( diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 266c598ac8..0b8fae7bf6 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -34,6 +34,9 @@ pub enum BeaconChainError { MissingBeaconState(Hash256), SlotProcessingError(SlotProcessingError), MetricsError(String), + NoStateForAttestation { + beacon_block_root: Hash256, + }, AttestationValidationError(AttestationValidationError), IndexedAttestationValidationError(IndexedAttestationValidationError), } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index ac001415cd..13e9203dd3 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -630,7 +630,12 @@ impl SimpleSync { _network: &mut NetworkContext, ) { match self.chain.process_attestation(msg) { - Ok(()) => info!(self.log, "ImportedAttestation"; "source" => "gossip"), + Ok(outcome) => info!( + self.log, + "Processed attestation"; + "source" => "gossip", + "outcome" => format!("{:?}", outcome) + ), Err(e) => { warn!(self.log, "InvalidAttestation"; "source" => "gossip", "error" => format!("{:?}", e)) } diff --git a/eth2/types/src/beacon_block.rs b/eth2/types/src/beacon_block.rs index 772ef0c46d..ecf8797992 100644 --- a/eth2/types/src/beacon_block.rs +++ b/eth2/types/src/beacon_block.rs @@ -62,6 +62,11 @@ impl BeaconBlock { } } + /// Returns the epoch corresponding to `self.slot`. + pub fn epoch(&self) -> Epoch { + self.slot.epoch(T::slots_per_epoch()) + } + /// Returns the `signed_root` of the block. /// /// Spec v0.8.1 From fe2402b361b9e7d8bc6f23cb022da875c32c050c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 7 Aug 2019 16:02:30 +1000 Subject: [PATCH 18/34] Add another attestation processing test --- beacon_node/beacon_chain/src/beacon_chain.rs | 9 ++--- beacon_node/beacon_chain/src/lib.rs | 4 ++- beacon_node/beacon_chain/src/test_utils.rs | 30 +++++++++++++++-- beacon_node/beacon_chain/tests/tests.rs | 35 ++++++++++++++++++++ 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c58e619bc1..8d5922850e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -543,7 +543,7 @@ impl BeaconChain { // Attempt to process the attestation using the `self.head()` state. // // This is purely an effort to avoid loading a `BeaconState` unnecessarily from the DB. - let outcome: Option> = { + let optional_outcome: Option> = { // Take a read lock on the head beacon state. // // The purpose of this whole `let processed ...` block is to ensure that the read @@ -553,10 +553,11 @@ impl BeaconChain { // If it turns out that the attestation was made using the head state, then there // is no need to load a state from the database to process the attestation. if state.current_epoch() == attestation_head_block.epoch() - && state + && (state .get_block_root(attestation_head_block.slot) .map(|root| *root == attestation.data.beacon_block_root) .unwrap_or_else(|_| false) + || attestation.data.beacon_block_root == self.head().beacon_block_root) { // The head state is able to be used to validate this attestation. No need to load // anything from the database. @@ -573,8 +574,8 @@ impl BeaconChain { // TODO: we could try and see if the "speculative state" (e.g., self.state) can support // this, without needing to load it from the db. - if let Some(result) = outcome { - result + if let Some(outcome) = optional_outcome { + outcome } else { // The state required to verify this attestation must be loaded from the database. let mut state: BeaconState = self diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index c2efcad130..3188760a42 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -7,7 +7,9 @@ mod metrics; mod persisted_beacon_chain; pub mod test_utils; -pub use self::beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; +pub use self::beacon_chain::{ + AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BlockProcessingOutcome, +}; pub use self::checkpoint::CheckPoint; pub use self::errors::{BeaconChainError, BlockProductionError}; pub use lmd_ghost; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 1049c66ad3..ce6d4d20ce 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -268,6 +268,28 @@ where head_block_root: Hash256, head_block_slot: Slot, ) { + self.get_free_attestations( + attestation_strategy, + state, + head_block_root, + head_block_slot, + ) + .into_iter() + .for_each(|attestation| { + self.chain + .process_attestation(attestation) + .expect("should process attestation"); + }); + } + + /// Generates a `Vec` for some attestation strategy and head_block. + pub fn get_free_attestations( + &self, + attestation_strategy: &AttestationStrategy, + state: &BeaconState, + head_block_root: Hash256, + head_block_slot: Slot, + ) -> Vec> { let spec = &self.spec; let fork = &state.fork; @@ -276,6 +298,8 @@ where AttestationStrategy::SomeValidators(vec) => vec.clone(), }; + let mut vec = vec![]; + state .get_crosslink_committees_at_slot(state.slot) .expect("should get committees") @@ -328,12 +352,12 @@ where signature, }; - self.chain - .process_attestation(attestation) - .expect("should process attestation"); + vec.push(attestation) } } }); + + vec } /// Creates two forks: diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 5b8a09fafc..1f84008496 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -4,6 +4,7 @@ use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, CommonTypes, PersistedBeaconChain, BEACON_CHAIN_DB_KEY, }; +use beacon_chain::AttestationProcessingOutcome; use lmd_ghost::ThreadSafeReducedTree; use rand::Rng; use store::{MemoryStore, Store}; @@ -298,6 +299,40 @@ fn free_attestations_added_to_fork_choice_some_none() { } } +#[test] +fn free_attestations_over_slots() { + let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 5; + + let harness = get_harness(VALIDATOR_COUNT); + + let mut attestations = vec![]; + + for _ in 0..num_blocks_produced { + harness.extend_chain( + 2, + BlockStrategy::OnCanonicalHead, + // Don't produce & include any attestations (we'll collect them later). + AttestationStrategy::SomeValidators(vec![]), + ); + + attestations.append(&mut harness.get_free_attestations( + &AttestationStrategy::AllValidators, + &harness.chain.head().beacon_state, + harness.chain.head().beacon_block_root, + harness.chain.head().beacon_block.slot, + )); + + harness.advance_slot(); + } + + for attestation in attestations { + assert_eq!( + harness.chain.process_attestation(attestation), + Ok(AttestationProcessingOutcome::Processed) + ) + } +} + #[test] fn free_attestations_added_to_fork_choice_all_updated() { let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 2 - 1; From 378fe05c895a19a960c51ec91d1f89084fc561ce Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 7 Aug 2019 16:40:49 +1000 Subject: [PATCH 19/34] Tidy attestation processing --- beacon_node/beacon_chain/Cargo.toml | 1 + beacon_node/beacon_chain/src/beacon_chain.rs | 16 +------------ beacon_node/beacon_chain/src/test_utils.rs | 24 ++++++++++++++++---- beacon_node/beacon_chain/tests/tests.rs | 15 ++++++++---- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml index af6736edec..89260cf51b 100644 --- a/beacon_node/beacon_chain/Cargo.toml +++ b/beacon_node/beacon_chain/Cargo.toml @@ -24,3 +24,4 @@ lmd_ghost = { path = "../../eth2/lmd_ghost" } [dev-dependencies] rand = "0.5.5" +lazy_static = "1.3.0" diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8d5922850e..5fc59ba663 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4,7 +4,6 @@ use crate::fork_choice::{Error as ForkChoiceError, ForkChoice}; use crate::iter::{ReverseBlockRootIterator, ReverseStateRootIterator}; use crate::metrics::Metrics; use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY}; -use crate::BeaconChainError; use lmd_ghost::LmdGhost; use log::trace; use operation_pool::DepositInsertStatus; @@ -615,7 +614,7 @@ impl BeaconChain { &self, attestation: Attestation, state: &BeaconState, - head_block: &BeaconBlock, + _head_block: &BeaconBlock, ) -> Result { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); @@ -649,19 +648,6 @@ impl BeaconChain { .map_err(|e| Error::AttestationValidationError(e)) } - fn state_can_process_attestation( - state: &BeaconState, - data: &AttestationData, - head_block: &BeaconBlock, - ) -> bool { - (state.current_epoch() - 1 <= data.target.epoch) - && (data.target.epoch <= state.current_epoch() + 1) - && state - .get_block_root(head_block.slot) - .map(|root| *root == data.beacon_block_root) - .unwrap_or_else(|_| false) - } - /* /// Retrieves the `BeaconState` used to create the attestation. fn get_attestation_state( diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index ce6d4d20ce..293d3b9b91 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -84,14 +84,30 @@ where { /// Instantiate a new harness with `validator_count` initial validators. pub fn new(validator_count: usize) -> Self { + let state_builder = TestingBeaconStateBuilder::from_default_keypairs_file_if_exists( + validator_count, + &E::default_spec(), + ); + let (genesis_state, keypairs) = state_builder.build(); + + Self::from_state_and_keypairs(genesis_state, keypairs) + } + + /// Instantiate a new harness with an initial validator for each key supplied. + pub fn from_keypairs(keypairs: Vec) -> Self { + let state_builder = TestingBeaconStateBuilder::from_keypairs(keypairs, &E::default_spec()); + let (genesis_state, keypairs) = state_builder.build(); + + Self::from_state_and_keypairs(genesis_state, keypairs) + } + + /// Instantiate a new harness with the given genesis state and a keypair for each of the + /// initial validators in the given state. + pub fn from_state_and_keypairs(genesis_state: BeaconState, keypairs: Vec) -> Self { let spec = E::default_spec(); let store = Arc::new(MemoryStore::open()); - let state_builder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(validator_count, &spec); - let (genesis_state, keypairs) = state_builder.build(); - let mut genesis_block = BeaconBlock::empty(&spec); genesis_block.state_root = Hash256::from_slice(&genesis_state.tree_hash_root()); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 1f84008496..d286aaec0c 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -1,5 +1,8 @@ #![cfg(not(debug_assertions))] +#[macro_use] +extern crate lazy_static; + use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, CommonTypes, PersistedBeaconChain, BEACON_CHAIN_DB_KEY, @@ -9,17 +12,21 @@ use lmd_ghost::ThreadSafeReducedTree; use rand::Rng; use store::{MemoryStore, Store}; use types::test_utils::{SeedableRng, TestRandom, XorShiftRng}; -use types::{Deposit, EthSpec, Hash256, MinimalEthSpec, RelativeEpoch, Slot}; +use types::{Deposit, EthSpec, Hash256, Keypair, MinimalEthSpec, RelativeEpoch, Slot}; // Should ideally be divisible by 3. pub const VALIDATOR_COUNT: usize = 24; +lazy_static! { + /// A cached set of keys. + static ref KEYPAIRS: Vec = types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT); +} + type TestForkChoice = ThreadSafeReducedTree; fn get_harness(validator_count: usize) -> BeaconChainHarness { - let harness = BeaconChainHarness::new(validator_count); + let harness = BeaconChainHarness::from_keypairs(KEYPAIRS[0..validator_count].to_vec()); - // Move past the zero slot. harness.advance_slot(); harness @@ -300,7 +307,7 @@ fn free_attestations_added_to_fork_choice_some_none() { } #[test] -fn free_attestations_over_slots() { +fn attestations_with_increasing_slots() { let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 5; let harness = get_harness(VALIDATOR_COUNT); From 65ce94b2efc5acd97acfb742dd72380626fa210e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 7 Aug 2019 16:54:35 +1000 Subject: [PATCH 20/34] Remove old code fragment --- beacon_node/beacon_chain/src/beacon_chain.rs | 42 -------------------- 1 file changed, 42 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5fc59ba663..60b65c95b4 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -648,48 +648,6 @@ impl BeaconChain { .map_err(|e| Error::AttestationValidationError(e)) } - /* - /// Retrieves the `BeaconState` used to create the attestation. - fn get_attestation_state( - &self, - attestation: &Attestation, - ) -> Option> { - let state = &self.head().beacon_state; - - // Current state is used if the attestation targets a historic block and a slot within an - // equal or adjacent epoch. - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let min_slot = - (self.state.read().slot.epoch(slots_per_epoch) - 1).start_slot(slots_per_epoch); - let blocks = BestBlockRootsIterator::owned( - self.store.clone(), - self.state.read().clone(), - self.state.read().slot.clone(), - ); - for (root, slot) in blocks { - if root == attestation.data.target.root { - return Some(self.state.read().clone()); - } - - if slot == min_slot { - break; - } - } - - // A different state is retrieved from the database. - match self - .store - .get::>(&attestation.data.target.root) - { - Ok(Some(block)) => match self.store.get::>(&block.state_root) { - Ok(state) => state, - _ => None, - }, - _ => None, - } - } - */ - /// Accept some deposit and queue it for inclusion in an appropriate block. pub fn process_deposit( &self, From 9f9af746eaa255d11bca18e17368cbacb3666d22 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 8 Aug 2019 10:29:27 +1000 Subject: [PATCH 21/34] Add non-compiling half finished changes --- .../src/per_block_processing.rs | 14 ++- .../verify_attestation.rs | 113 +++++++----------- 2 files changed, 54 insertions(+), 73 deletions(-) diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index 3c89215550..3acadfde26 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -14,10 +14,7 @@ pub use self::verify_proposer_slashing::verify_proposer_slashing; pub use is_valid_indexed_attestation::{ is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature, }; -pub use verify_attestation::{ - verify_attestation, verify_attestation_time_independent_only, - verify_attestation_without_signature, -}; +pub use verify_attestation::{verify_attestation_for_block, verify_attestation_for_state}; pub use verify_deposit::{ get_existing_validator_index, verify_deposit_merkle_proof, verify_deposit_signature, }; @@ -37,6 +34,12 @@ mod verify_exit; mod verify_proposer_slashing; mod verify_transfer; +#[derive(PartialEq)] +pub enum VerifySignatures { + True, + False, +} + /// Updates the state for a new block, whilst validating that the block is valid. /// /// Returns `Ok(())` if the block is valid and the state was successfully updated. Otherwise @@ -312,7 +315,8 @@ pub fn process_attestations( .par_iter() .enumerate() .try_for_each(|(i, attestation)| { - verify_attestation(state, attestation, spec).map_err(|e| e.into_with_index(i)) + verify_attestation_for_block(state, attestation, spec, VerifySignatures::True) + .map_err(|e| e.into_with_index(i)) })?; // Update the state in series. diff --git a/eth2/state_processing/src/per_block_processing/verify_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_attestation.rs index af25300457..bca6a90854 100644 --- a/eth2/state_processing/src/per_block_processing/verify_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_attestation.rs @@ -1,4 +1,5 @@ use super::errors::{AttestationInvalid as Invalid, AttestationValidationError as Error}; +use super::VerifySignatures; use crate::common::get_indexed_attestation; use crate::per_block_processing::{ is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature, @@ -6,67 +7,23 @@ use crate::per_block_processing::{ use tree_hash::TreeHash; use types::*; -/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the -/// given state. -/// -/// Returns `Ok(())` if the `Attestation` is valid, otherwise indicates the reason for invalidity. -/// -/// Spec v0.8.0 -pub fn verify_attestation( - state: &BeaconState, - attestation: &Attestation, - spec: &ChainSpec, -) -> Result<(), Error> { - verify_attestation_parametric(state, attestation, spec, true, false) -} - -/// Like `verify_attestation` but doesn't run checks which may become true in future states. -pub fn verify_attestation_time_independent_only( - state: &BeaconState, - attestation: &Attestation, - spec: &ChainSpec, -) -> Result<(), Error> { - verify_attestation_parametric(state, attestation, spec, true, true) -} - -/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the -/// given state, without validating the aggregate signature. -/// -/// Returns `Ok(())` if the `Attestation` is valid, otherwise indicates the reason for invalidity. -/// -/// Spec v0.8.0 -pub fn verify_attestation_without_signature( - state: &BeaconState, - attestation: &Attestation, - spec: &ChainSpec, -) -> Result<(), Error> { - verify_attestation_parametric(state, attestation, spec, false, false) -} - /// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the /// given state, optionally validating the aggregate signature. /// -/// /// Spec v0.8.0 -fn verify_attestation_parametric( +pub fn verify_attestation_for_block( state: &BeaconState, attestation: &Attestation, spec: &ChainSpec, - verify_signature: bool, - time_independent_only: bool, + verify_signatures: VerifySignatures, ) -> Result<(), Error> { let data = &attestation.data; - verify!( - data.crosslink.shard < T::ShardCount::to_u64(), - Invalid::BadShard - ); // Check attestation slot. let attestation_slot = state.get_attestation_data_slot(&data)?; verify!( - time_independent_only - || attestation_slot + spec.min_attestation_inclusion_delay <= state.slot, + attestation_slot + spec.min_attestation_inclusion_delay <= state.slot, Invalid::IncludedTooEarly { state: state.slot, delay: spec.min_attestation_inclusion_delay, @@ -81,27 +38,47 @@ fn verify_attestation_parametric( } ); - // Verify the Casper FFG vote and crosslink data. - if !time_independent_only { - let parent_crosslink = verify_casper_ffg_vote(attestation, state)?; + verify_attestation_for_state(state, attestation, spec, verify_signatures) +} - verify!( - data.crosslink.parent_root == Hash256::from_slice(&parent_crosslink.tree_hash_root()), - Invalid::BadParentCrosslinkHash - ); - verify!( - data.crosslink.start_epoch == parent_crosslink.end_epoch, - Invalid::BadParentCrosslinkStartEpoch - ); - verify!( - data.crosslink.end_epoch - == std::cmp::min( - data.target.epoch, - parent_crosslink.end_epoch + spec.max_epochs_per_crosslink - ), - Invalid::BadParentCrosslinkEndEpoch - ); - } +/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that preceeds the given +/// `state`. +/// +/// Returns a descriptive `Err` if the attestation is malformed or does not accurately reflect the +/// prior blocks in `state`. +/// +/// Spec v0.8.0 +pub fn verify_attestation_for_state( + state: &BeaconState, + attestation: &Attestation, + spec: &ChainSpec, + verify_signature: VerifySignatures, +) -> Result<(), Error> { + let data = &attestation.data; + verify!( + data.crosslink.shard < T::ShardCount::to_u64(), + Invalid::BadShard + ); + + // Verify the Casper FFG vote and crosslink data. + let parent_crosslink = verify_casper_ffg_vote(attestation, state)?; + + verify!( + data.crosslink.parent_root == Hash256::from_slice(&parent_crosslink.tree_hash_root()), + Invalid::BadParentCrosslinkHash + ); + verify!( + data.crosslink.start_epoch == parent_crosslink.end_epoch, + Invalid::BadParentCrosslinkStartEpoch + ); + verify!( + data.crosslink.end_epoch + == std::cmp::min( + data.target.epoch, + parent_crosslink.end_epoch + spec.max_epochs_per_crosslink + ), + Invalid::BadParentCrosslinkEndEpoch + ); // Crosslink data root is zero (to be removed in phase 1). verify!( @@ -111,7 +88,7 @@ fn verify_attestation_parametric( // Check signature and bitfields let indexed_attestation = get_indexed_attestation(state, attestation)?; - if verify_signature { + if verify_signature == VerifySignatures::True { is_valid_indexed_attestation(state, &indexed_attestation, spec)?; } else { is_valid_indexed_attestation_without_signature(state, &indexed_attestation, spec)?; From 7c134a7504d2e8ff8b8cdd7d20459f96abde04a9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 8 Aug 2019 16:47:24 +1000 Subject: [PATCH 22/34] Simplify, fix bugs, add tests for chain iters --- beacon_node/beacon_chain/src/beacon_chain.rs | 53 ++++----------- beacon_node/beacon_chain/src/test_utils.rs | 32 +++------ beacon_node/beacon_chain/tests/tests.rs | 68 +++++++++++++++++++- beacon_node/store/src/iter.rs | 30 ++++----- eth2/lmd_ghost/src/reduced_tree.rs | 6 +- 5 files changed, 106 insertions(+), 83 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 60b65c95b4..e8dcd50abf 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -244,15 +244,12 @@ impl BeaconChain { /// /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot /// returned may be earlier than the wall-clock slot. - pub fn rev_iter_block_roots( - &self, - slot: Slot, - ) -> ReverseBlockRootIterator { + pub fn rev_iter_block_roots(&self) -> ReverseBlockRootIterator { let state = &self.head().beacon_state; let block_root = self.head().beacon_block_root; let block_slot = state.slot; - let iter = BlockRootsIterator::owned(self.store.clone(), state.clone(), slot); + let iter = BlockRootsIterator::owned(self.store.clone(), state.clone()); ReverseBlockRootIterator::new((block_root, block_slot), iter) } @@ -267,15 +264,12 @@ impl BeaconChain { /// /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot /// returned may be earlier than the wall-clock slot. - pub fn rev_iter_state_roots( - &self, - slot: Slot, - ) -> ReverseStateRootIterator { + pub fn rev_iter_state_roots(&self) -> ReverseStateRootIterator { let state = &self.head().beacon_state; let state_root = self.head().beacon_state_root; let state_slot = state.slot; - let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot); + let iter = StateRootsIterator::owned(self.store.clone(), state.clone()); ReverseStateRootIterator::new((state_root, state_slot), iter) } @@ -448,9 +442,8 @@ impl BeaconChain { pub fn produce_attestation_data(&self, shard: u64) -> Result { let state = self.state.read(); let head_block_root = self.head().beacon_block_root; - let head_block_slot = self.head().beacon_block.slot; - self.produce_attestation_data_for_block(shard, head_block_root, head_block_slot, &*state) + self.produce_attestation_data_for_block(shard, head_block_root, &*state) } /// Produce an `AttestationData` that attests to the chain denoted by `block_root` and `state`. @@ -461,39 +454,19 @@ impl BeaconChain { &self, shard: u64, head_block_root: Hash256, - head_block_slot: Slot, state: &BeaconState, ) -> Result { // Collect some metrics. self.metrics.attestation_production_requests.inc(); let timer = self.metrics.attestation_production_times.start_timer(); - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let current_epoch_start_slot = state.current_epoch().start_slot(slots_per_epoch); - // The `target_root` is the root of the first block of the current epoch. - // - // The `state` does not know the root of the block for it's current slot (it only knows - // about blocks from prior slots). This creates an edge-case when the state is on the first - // slot of the epoch -- we're unable to obtain the `target_root` because it is not a prior - // root. - // - // This edge case is handled in two ways: - // - // - If the head block is on the same slot as the state, we use it's root. - // - Otherwise, assume the current slot has been skipped and use the block root from the - // prior slot. - // - // For all other cases, we simply read the `target_root` from `state.latest_block_roots`. - let target_root = if state.slot == current_epoch_start_slot { - if head_block_slot == current_epoch_start_slot { - head_block_root - } else { - *state.get_block_root(current_epoch_start_slot - 1)? - } - } else { - *state.get_block_root(current_epoch_start_slot)? - }; + let target_root = self + .rev_iter_block_roots() + .find(|(_root, slot)| *slot % T::EthSpec::slots_per_epoch() == 0) + .map(|(root, _slot)| root) + .ok_or_else(|| Error::UnableToFindTargetRoot(self.head().beacon_state.slot))?; + let target = Checkpoint { epoch: state.current_epoch(), root: target_root, @@ -523,7 +496,7 @@ impl BeaconChain { }) } - /// Accept a new attestation from the network. + /// Accept a new, potentially invalid attestation from the network. /// /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation /// if possible. @@ -614,7 +587,7 @@ impl BeaconChain { &self, attestation: Attestation, state: &BeaconState, - _head_block: &BeaconBlock, + block: &BeaconBlock, ) -> Result { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 293d3b9b91..f2ec5a0fd6 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -194,7 +194,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot); + self.add_free_attestations(&attestation_strategy, &new_state, block_root); } else { panic!("block should be successfully processed: {:?}", outcome); } @@ -209,7 +209,7 @@ where fn get_state_at_slot(&self, state_slot: Slot) -> BeaconState { let state_root = self .chain - .rev_iter_state_roots(self.chain.head().beacon_state.slot - 1) + .rev_iter_state_roots() .find(|(_hash, slot)| *slot == state_slot) .map(|(hash, _slot)| hash) .expect("could not find state root"); @@ -282,20 +282,14 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, - head_block_slot: Slot, ) { - self.get_free_attestations( - attestation_strategy, - state, - head_block_root, - head_block_slot, - ) - .into_iter() - .for_each(|attestation| { - self.chain - .process_attestation(attestation) - .expect("should process attestation"); - }); + self.get_free_attestations(attestation_strategy, state, head_block_root) + .into_iter() + .for_each(|attestation| { + self.chain + .process_attestation(attestation) + .expect("should process attestation"); + }); } /// Generates a `Vec` for some attestation strategy and head_block. @@ -304,7 +298,6 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, - head_block_slot: Slot, ) -> Vec> { let spec = &self.spec; let fork = &state.fork; @@ -329,12 +322,7 @@ where if attesting_validators.contains(validator_index) { let data = self .chain - .produce_attestation_data_for_block( - cc.shard, - head_block_root, - head_block_slot, - state, - ) + .produce_attestation_data_for_block(cc.shard, head_block_root, state) .expect("should produce attestation data"); let mut aggregation_bits = BitList::with_capacity(committee_size).unwrap(); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index d286aaec0c..8dc4ae6ec8 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -32,6 +32,73 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness = harness.chain.rev_iter_block_roots().collect(); + let state_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_state_roots().collect(); + + assert_eq!( + block_roots.len(), + state_roots.len(), + "should be an equal amount of block and state roots" + ); + + assert!( + block_roots.iter().any(|(_root, slot)| *slot == 0), + "should contain genesis block root" + ); + assert!( + state_roots.iter().any(|(_root, slot)| *slot == 0), + "should contain genesis state root" + ); + + assert_eq!( + block_roots.len(), + num_blocks_produced as usize + 1, + "should contain all produced blocks, plus the genesis block" + ); + + block_roots.windows(2).for_each(|x| { + assert_eq!( + x[1].1, + x[0].1 - 1, + "block root slots should be decreasing by one" + ) + }); + state_roots.windows(2).for_each(|x| { + assert_eq!( + x[1].1, + x[0].1 - 1, + "state root slots should be decreasing by one" + ) + }); + + let head = &harness.chain.head(); + + assert_eq!( + *block_roots.first().expect("should have some block roots"), + (head.beacon_block_root, head.beacon_block.slot), + "first block root and slot should be for the head block" + ); + + assert_eq!( + *state_roots.first().expect("should have some state roots"), + (head.beacon_state_root, head.beacon_state.slot), + "first state root and slot should be for the head state" + ); +} + #[test] fn chooses_fork() { let harness = get_harness(VALIDATOR_COUNT); @@ -326,7 +393,6 @@ fn attestations_with_increasing_slots() { &AttestationStrategy::AllValidators, &harness.chain.head().beacon_state, harness.chain.head().beacon_block_root, - harness.chain.head().beacon_block.slot, )); harness.advance_slot(); diff --git a/beacon_node/store/src/iter.rs b/beacon_node/store/src/iter.rs index c4e557b2de..84bf3759fb 100644 --- a/beacon_node/store/src/iter.rs +++ b/beacon_node/store/src/iter.rs @@ -20,7 +20,7 @@ impl<'a, U: Store, E: EthSpec> AncestorIter> for fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { let state = store.get::>(&self.state_root).ok()??; - Some(BlockRootsIterator::owned(store, state, self.slot)) + Some(BlockRootsIterator::owned(store, state)) } } @@ -32,19 +32,19 @@ pub struct StateRootsIterator<'a, T: EthSpec, U> { } impl<'a, T: EthSpec, U: Store> StateRootsIterator<'a, T, U> { - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Borrowed(beacon_state), - slot: start_slot + 1, } } - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Owned(beacon_state), - slot: start_slot + 1, } } } @@ -88,16 +88,16 @@ pub struct BlockIterator<'a, T: EthSpec, U> { impl<'a, T: EthSpec, U: Store> BlockIterator<'a, T, U> { /// Create a new iterator over all blocks in the given `beacon_state` and prior states. - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { - roots: BlockRootsIterator::new(store, beacon_state, start_slot), + roots: BlockRootsIterator::new(store, beacon_state), } } /// Create a new iterator over all blocks in the given `beacon_state` and prior states. - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { - roots: BlockRootsIterator::owned(store, beacon_state, start_slot), + roots: BlockRootsIterator::owned(store, beacon_state), } } } @@ -128,20 +128,20 @@ pub struct BlockRootsIterator<'a, T: EthSpec, U> { impl<'a, T: EthSpec, U: Store> BlockRootsIterator<'a, T, U> { /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Borrowed(beacon_state), - slot: start_slot + 1, } } /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Owned(beacon_state), - slot: start_slot + 1, } } } @@ -218,7 +218,7 @@ mod test { state_b.state_roots[0] = state_a_root; store.put(&state_a_root, &state_a).unwrap(); - let iter = BlockRootsIterator::new(store.clone(), &state_b, state_b.slot - 1); + let iter = BlockRootsIterator::new(store.clone(), &state_b); assert!( iter.clone().find(|(_root, slot)| *slot == 0).is_some(), @@ -267,7 +267,7 @@ mod test { store.put(&state_a_root, &state_a).unwrap(); store.put(&state_b_root, &state_b).unwrap(); - let iter = StateRootsIterator::new(store.clone(), &state_b, state_b.slot - 1); + let iter = StateRootsIterator::new(store.clone(), &state_b); assert!( iter.clone().find(|(_root, slot)| *slot == 0).is_some(), diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index 5d70748042..9668620b79 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -611,11 +611,7 @@ where let block = self.get_block(child)?; let state = self.get_state(block.state_root)?; - Ok(BlockRootsIterator::owned( - self.store.clone(), - state, - block.slot - 1, - )) + Ok(BlockRootsIterator::owned(self.store.clone(), state)) } /// Verify the integrity of `self`. Returns `Ok(())` if the tree has integrity, otherwise returns `Err(description)`. From b1591c3c12d777377524fcd37809ad4508e4e7c9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 8 Aug 2019 16:49:27 +1000 Subject: [PATCH 23/34] Remove attestation processing from op pool --- beacon_node/beacon_chain/src/beacon_chain.rs | 68 +++++++++++++++++-- beacon_node/beacon_chain/src/errors.rs | 1 + beacon_node/beacon_chain/src/fork_choice.rs | 18 ++--- eth2/operation_pool/src/lib.rs | 20 ++++-- .../src/per_block_processing.rs | 6 +- .../verify_attestation.rs | 10 +-- 6 files changed, 92 insertions(+), 31 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e8dcd50abf..8982cdf796 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -11,12 +11,15 @@ use operation_pool::{OperationPool, PersistedOperationPool}; use parking_lot::{RwLock, RwLockReadGuard}; use slog::{error, info, warn, Logger}; use slot_clock::SlotClock; -use state_processing::per_block_processing::errors::{ - AttesterSlashingValidationError, DepositValidationError, ExitValidationError, - ProposerSlashingValidationError, TransferValidationError, +use state_processing::per_block_processing::{ + errors::{ + AttestationValidationError, AttesterSlashingValidationError, DepositValidationError, + ExitValidationError, ProposerSlashingValidationError, TransferValidationError, + }, + verify_attestation_for_state, VerifySignatures, }; use state_processing::{ - common, per_block_processing, per_block_processing_without_verifying_block_signature, + per_block_processing, per_block_processing_without_verifying_block_signature, per_slot_processing, BlockProcessingError, }; use std::sync::Arc; @@ -58,6 +61,7 @@ pub enum BlockProcessingOutcome { pub enum AttestationProcessingOutcome { Processed, UnknownHeadBlock { beacon_block_root: Hash256 }, + Invalid(AttestationValidationError), } pub trait BeaconChainTypes { @@ -543,9 +547,6 @@ impl BeaconChain { } }; - // TODO: we could try and see if the "speculative state" (e.g., self.state) can support - // this, without needing to load it from the db. - if let Some(outcome) = optional_outcome { outcome } else { @@ -583,6 +584,25 @@ impl BeaconChain { } } + /// Verifies the `attestation` against the `state` to which it is attesting. + /// + /// Updates fork choice with any new latest messages, but _does not_ find or update the head. + /// + /// ## Notes + /// + /// The given `state` must fulfil one of the following conditions: + /// + /// - `state` corresponds to the `block.state_root` identified by + /// `attestation.data.beacon_block_root`. (Viz., `attestation` was created using `state`. + /// - `state.slot` is in the same epoch as `block.slot` and + /// `attestation.data.beacon_block_root` is in `state.block_roots`. (Viz., the attestation was + /// attesting to an ancestor of `state` from the same epoch as `state`. + /// + /// Additionally, `attestation.data.beacon_block_root` **must** be available to read in + /// `self.store` _and_ be the root of the given `block`. + /// + /// If the given conditions are not fulfilled, the function may error or provide a false + /// negative (indicating that a given `attestation` is invalid when it is was validly formed). fn process_attestation_for_state_and_block( &self, attestation: Attestation, @@ -592,6 +612,39 @@ impl BeaconChain { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); + let result = if let Err(e) = + verify_attestation_for_state(state, &attestation, &self.spec, VerifySignatures::True) + { + warn!( + self.log, + "Invalid attestation"; + "state_epoch" => state.current_epoch(), + "error" => format!("{:?}", e), + ); + + Ok(AttestationProcessingOutcome::Invalid(e)) + } else { + // Provide the attestation to fork choice, updating the validator latest messages but + // _without_ finding and updating the head. + self.fork_choice + .process_attestation(&state, &attestation, block)?; + + // Provide the valid attestation to op pool, which may choose to retain the + // attestation for inclusion in a future block. + self.op_pool + .insert_attestation(attestation, state, &self.spec)?; + + // Update the metrics. + self.metrics.attestation_processing_successes.inc(); + + Ok(AttestationProcessingOutcome::Processed) + }; + + timer.observe_duration(); + + result + + /* if self .fork_choice .should_process_attestation(state, &attestation)? @@ -619,6 +672,7 @@ impl BeaconChain { result .map(|_| AttestationProcessingOutcome::Processed) .map_err(|e| Error::AttestationValidationError(e)) + */ } /// Accept some deposit and queue it for inclusion in an appropriate block. diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 0b8fae7bf6..7a51fc4258 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -26,6 +26,7 @@ pub enum BeaconChainError { previous_epoch: Epoch, new_epoch: Epoch, }, + UnableToFindTargetRoot(Slot), BeaconStateError(BeaconStateError), DBInconsistent(String), DBError(store::Error), diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 71415d1915..83d6c335f1 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -20,7 +20,6 @@ pub enum Error { pub struct ForkChoice { backend: T::LmdGhost, - store: Arc, /// Used for resolving the `0x00..00` alias back to genesis. /// /// Does not necessarily need to be the _actual_ genesis, it suffices to be the finalized root @@ -39,7 +38,6 @@ impl ForkChoice { genesis_block_root: Hash256, ) -> Self { Self { - store: store.clone(), backend: T::LmdGhost::new(store, genesis_block, genesis_block_root), genesis_block_root, } @@ -119,7 +117,7 @@ impl ForkChoice { // // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md for attestation in &block.body.attestations { - self.process_attestation(state, attestation)?; + self.process_attestation(state, attestation, block)?; } self.backend.process_block(block, block_root)?; @@ -127,13 +125,14 @@ impl ForkChoice { Ok(()) } - /// Process an attestation. + /// Process an attestation which references `block` in `attestation.data.beacon_block_root`. /// /// Assumes the attestation is valid. pub fn process_attestation( &self, state: &BeaconState, attestation: &Attestation, + block: &BeaconBlock, ) -> Result<()> { let block_hash = attestation.data.beacon_block_root; @@ -152,20 +151,13 @@ impl ForkChoice { // to genesis just by being present in the chain. // // Additionally, don't add any block hash to fork choice unless we have imported the block. - if block_hash != Hash256::zero() - && self - .store - .exists::>(&block_hash) - .unwrap_or(false) - { + if block_hash != Hash256::zero() { let validator_indices = get_attesting_indices(state, &attestation.data, &attestation.aggregation_bits)?; - let block_slot = state.get_attestation_data_slot(&attestation.data)?; - for validator_index in validator_indices { self.backend - .process_attestation(validator_index, block_hash, block_slot)?; + .process_attestation(validator_index, block_hash, block.slot)?; } } diff --git a/eth2/operation_pool/src/lib.rs b/eth2/operation_pool/src/lib.rs index 92d5fb1683..ba9ca81c07 100644 --- a/eth2/operation_pool/src/lib.rs +++ b/eth2/operation_pool/src/lib.rs @@ -15,9 +15,10 @@ use state_processing::per_block_processing::errors::{ ExitValidationError, ProposerSlashingValidationError, TransferValidationError, }; use state_processing::per_block_processing::{ - get_slashable_indices_modular, verify_attestation, verify_attestation_time_independent_only, + get_slashable_indices_modular, verify_attestation_for_block_inclusion, verify_attester_slashing, verify_exit, verify_exit_time_independent_only, verify_proposer_slashing, verify_transfer, verify_transfer_time_independent_only, + VerifySignatures, }; use std::collections::{btree_map::Entry, hash_map, BTreeMap, HashMap, HashSet}; use std::marker::PhantomData; @@ -64,15 +65,16 @@ impl OperationPool { } /// Insert an attestation into the pool, aggregating it with existing attestations if possible. + /// + /// ## Note + /// + /// This function assumes the given `attestation` is valid. pub fn insert_attestation( &self, attestation: Attestation, state: &BeaconState, spec: &ChainSpec, ) -> Result<(), AttestationValidationError> { - // Check that attestation signatures are valid. - verify_attestation_time_independent_only(state, &attestation, spec)?; - let id = AttestationId::from_data(&attestation.data, state, spec); // Take a write lock on the attestations map. @@ -128,7 +130,15 @@ impl OperationPool { }) .flat_map(|(_, attestations)| attestations) // That are valid... - .filter(|attestation| verify_attestation(state, attestation, spec).is_ok()) + .filter(|attestation| { + verify_attestation_for_block_inclusion( + state, + attestation, + spec, + VerifySignatures::True, + ) + .is_ok() + }) .map(|att| AttMaxCover::new(att, earliest_attestation_validators(att, state))); maximum_cover(valid_attestations, T::MaxAttestations::to_usize()) diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index 3acadfde26..a64158ac98 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -14,7 +14,9 @@ pub use self::verify_proposer_slashing::verify_proposer_slashing; pub use is_valid_indexed_attestation::{ is_valid_indexed_attestation, is_valid_indexed_attestation_without_signature, }; -pub use verify_attestation::{verify_attestation_for_block, verify_attestation_for_state}; +pub use verify_attestation::{ + verify_attestation_for_block_inclusion, verify_attestation_for_state, +}; pub use verify_deposit::{ get_existing_validator_index, verify_deposit_merkle_proof, verify_deposit_signature, }; @@ -315,7 +317,7 @@ pub fn process_attestations( .par_iter() .enumerate() .try_for_each(|(i, attestation)| { - verify_attestation_for_block(state, attestation, spec, VerifySignatures::True) + verify_attestation_for_block_inclusion(state, attestation, spec, VerifySignatures::True) .map_err(|e| e.into_with_index(i)) })?; diff --git a/eth2/state_processing/src/per_block_processing/verify_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_attestation.rs index bca6a90854..74dbefa232 100644 --- a/eth2/state_processing/src/per_block_processing/verify_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_attestation.rs @@ -7,11 +7,13 @@ use crate::per_block_processing::{ use tree_hash::TreeHash; use types::*; -/// Indicates if an `Attestation` is valid to be included in a block in the current epoch of the -/// given state, optionally validating the aggregate signature. +/// Returns `Ok(())` if the given `attestation` is valid to be included in a block that is applied +/// to `state`. Otherwise, returns a descriptive `Err`. +/// +/// Optionally verifies the aggregate signature, depending on `verify_signatures`. /// /// Spec v0.8.0 -pub fn verify_attestation_for_block( +pub fn verify_attestation_for_block_inclusion( state: &BeaconState, attestation: &Attestation, spec: &ChainSpec, @@ -41,7 +43,7 @@ pub fn verify_attestation_for_block( verify_attestation_for_state(state, attestation, spec, verify_signatures) } -/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that preceeds the given +/// Returns `Ok(())` if `attestation` is a valid attestation to the chain that precedes the given /// `state`. /// /// Returns a descriptive `Err` if the attestation is malformed or does not accurately reflect the From 76bb6710844d4f683d1681ef738efe9e5880b137 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 9 Aug 2019 11:54:35 +1000 Subject: [PATCH 24/34] Fix bug with fork choice, tidy --- beacon_node/beacon_chain/src/beacon_chain.rs | 63 +++++++++---------- beacon_node/beacon_chain/src/fork_choice.rs | 14 ++++- beacon_node/beacon_chain/src/test_utils.rs | 6 +- beacon_node/beacon_chain/tests/tests.rs | 22 ++++--- .../src/per_block_processing/errors.rs | 3 + .../verify_attestation.rs | 11 ++++ 6 files changed, 70 insertions(+), 49 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3d50d701c4..81e5bdd65a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -524,7 +524,13 @@ impl BeaconChain { // If it turns out that the attestation was made using the head state, then there // is no need to load a state from the database to process the attestation. - if state.current_epoch() == attestation_head_block.epoch() + // + // Note: use the epoch of the target because it indicates which epoch the + // attestation was created in. You cannot use the epoch of the head block, because + // the block doesn't necessarily need to be in the same epoch as the attestation + // (e.g., if there are skip slots between the epoch the block was created in and + // the epoch for the attestation). + if state.current_epoch() == attestation.data.target.epoch && (state .get_block_root(attestation_head_block.slot) .map(|root| *root == attestation.data.beacon_block_root) @@ -546,7 +552,11 @@ impl BeaconChain { if let Some(outcome) = optional_outcome { outcome } else { - // The state required to verify this attestation must be loaded from the database. + // Use the `data.beacon_block_root` to load the state from the latest non-skipped + // slot preceding the attestations creation. + // + // This state is guaranteed to be in the same chain as the attestation, but it's + // not guaranteed to be from the same slot or epoch as the attestation. let mut state: BeaconState = self .store .get(&attestation_head_block.state_root)? @@ -554,7 +564,21 @@ impl BeaconChain { // Ensure the state loaded from the database matches the state of the attestation // head block. - for _ in state.slot.as_u64()..attestation_head_block.slot.as_u64() { + // + // The state needs to be advanced from the current slot through to the epoch in + // which the attestation was created in. It would be an error to try and use + // `state.get_attestation_data_slot(..)` because the state matching the + // `data.beacon_block_root` isn't necessarily in a nearby epoch to the attestation + // (e.g., if there were lots of skip slots since the head of the chain and the + // epoch creation epoch). + for _ in state.slot.as_u64() + ..attestation + .data + .target + .epoch + .start_slot(T::EthSpec::slots_per_epoch()) + .as_u64() + { per_slot_processing(&mut state, &self.spec)?; } @@ -639,36 +663,6 @@ impl BeaconChain { timer.observe_duration(); result - - /* - if self - .fork_choice - .should_process_attestation(state, &attestation)? - { - // TODO: check validation. - let indexed_attestation = common::get_indexed_attestation(state, &attestation)?; - per_block_processing::is_valid_indexed_attestation( - state, - &indexed_attestation, - &self.spec, - )?; - self.fork_choice.process_attestation(&state, &attestation)?; - } - - let result = self - .op_pool - .insert_attestation(attestation, state, &self.spec); - - timer.observe_duration(); - - if result.is_ok() { - self.metrics.attestation_processing_successes.inc(); - } - - result - .map(|_| AttestationProcessingOutcome::Processed) - .map_err(|e| Error::AttestationValidationError(e)) - */ } /// Accept some deposit and queue it for inclusion in an appropriate block. @@ -735,7 +729,7 @@ impl BeaconChain { return Ok(BlockProcessingOutcome::GenesisBlock); } - let block_root = block.block_header().canonical_root(); + let block_root = block.canonical_root(); if block_root == self.genesis_block_root { return Ok(BlockProcessingOutcome::GenesisBlock); @@ -781,6 +775,7 @@ impl BeaconChain { per_slot_processing(&mut state, &self.spec)?; } + state.build_committee_cache(RelativeEpoch::Previous, &self.spec)?; state.build_committee_cache(RelativeEpoch::Current, &self.spec)?; // Apply the received block to its parent state (which has been transitioned into this diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 83d6c335f1..6800f61d8c 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -19,6 +19,7 @@ pub enum Error { } pub struct ForkChoice { + store: Arc, backend: T::LmdGhost, /// Used for resolving the `0x00..00` alias back to genesis. /// @@ -38,6 +39,7 @@ impl ForkChoice { genesis_block_root: Hash256, ) -> Self { Self { + store: store.clone(), backend: T::LmdGhost::new(store, genesis_block, genesis_block_root), genesis_block_root, } @@ -117,9 +119,19 @@ impl ForkChoice { // // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md for attestation in &block.body.attestations { - self.process_attestation(state, attestation, block)?; + let block = self + .store + .get::>(&attestation.data.beacon_block_root)? + .ok_or_else(|| Error::MissingBlock(attestation.data.beacon_block_root))?; + + self.process_attestation(state, attestation, &block)?; } + // This does not apply a vote to the block, it just makes fork choice aware of the block so + // it can still be identified as the head even if it doesn't have any votes. + // + // A case where a block without any votes can be the head is where it is the only child of + // a block that has the majority of votes applied to it. self.backend.process_block(block, block_root)?; Ok(()) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index f2ec5a0fd6..6997f52aec 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -349,14 +349,12 @@ where agg_sig }; - let attestation = Attestation { + vec.push(Attestation { aggregation_bits, data, custody_bits, signature, - }; - - vec.push(attestation) + }) } } }); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 8dc4ae6ec8..c22f025639 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -342,27 +342,29 @@ fn free_attestations_added_to_fork_choice_some_none() { let state = &harness.chain.head().beacon_state; let fork_choice = &harness.chain.fork_choice; - let validators: Vec = (0..VALIDATOR_COUNT).collect(); - let slots: Vec = validators - .iter() - .map(|&v| { - state - .get_attestation_duties(v, RelativeEpoch::Current) + let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT) + .into_iter() + .map(|validator_index| { + let slot = state + .get_attestation_duties(validator_index, RelativeEpoch::Current) .expect("should get attester duties") .unwrap() - .slot + .slot; + + (validator_index, slot) }) .collect(); - let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); for (validator, slot) in validator_slots.clone() { - let latest_message = fork_choice.latest_message(*validator); + let latest_message = fork_choice.latest_message(validator); if slot <= num_blocks_produced && slot != 0 { assert_eq!( latest_message.unwrap().1, slot, - "Latest message slot should be equal to attester duty." + "Latest message slot for {} should be equal to slot {}.", + validator, + slot ) } else { assert!( diff --git a/eth2/state_processing/src/per_block_processing/errors.rs b/eth2/state_processing/src/per_block_processing/errors.rs index 65179167c1..436ec96cee 100644 --- a/eth2/state_processing/src/per_block_processing/errors.rs +++ b/eth2/state_processing/src/per_block_processing/errors.rs @@ -136,6 +136,9 @@ pub enum AttestationInvalid { delay: u64, attestation: Slot, }, + /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the + /// future). + AttestsToFutureState { state: Slot, attestation: Slot }, /// Attestation slot is too far in the past to be included in a block. IncludedTooLate { state: Slot, attestation: Slot }, /// Attestation target epoch does not match the current or previous epoch. diff --git a/eth2/state_processing/src/per_block_processing/verify_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_attestation.rs index 74dbefa232..127d251dea 100644 --- a/eth2/state_processing/src/per_block_processing/verify_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_attestation.rs @@ -62,6 +62,17 @@ pub fn verify_attestation_for_state( Invalid::BadShard ); + let attestation_slot = state.get_attestation_data_slot(&data)?; + + // An attestation cannot attest to a state that is later than itself. + verify!( + attestation_slot <= state.slot, + Invalid::AttestsToFutureState { + state: state.slot, + attestation: attestation_slot + } + ); + // Verify the Casper FFG vote and crosslink data. let parent_crosslink = verify_casper_ffg_vote(attestation, state)?; From d191812d4bfbed364f8f9157a708ff516011a026 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 9 Aug 2019 12:23:10 +1000 Subject: [PATCH 25/34] Fix overly restrictive check in fork choice. --- beacon_node/beacon_chain/src/beacon_chain.rs | 33 +++++++++++++++---- .../src/per_block_processing/errors.rs | 3 -- .../verify_attestation.rs | 11 ------- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 81e5bdd65a..7af64924e4 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -60,7 +60,15 @@ pub enum BlockProcessingOutcome { #[derive(Debug, PartialEq)] pub enum AttestationProcessingOutcome { Processed, - UnknownHeadBlock { beacon_block_root: Hash256 }, + UnknownHeadBlock { + beacon_block_root: Hash256, + }, + /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the + /// future). + AttestsToFutureState { + state: Slot, + attestation: Slot, + }, Invalid(AttestationValidationError), } @@ -582,11 +590,24 @@ impl BeaconChain { per_slot_processing(&mut state, &self.spec)?; } - self.process_attestation_for_state_and_block( - attestation, - &state, - &attestation_head_block, - ) + let attestation_slot = state.get_attestation_data_slot(&attestation.data)?; + + // Reject any attestation where the `state` loaded from `data.beacon_block_root` + // has a higher slot than the attestation. + // + // Permitting this would allow for attesters to vote on _future_ slots. + if attestation_slot > state.slot { + Ok(AttestationProcessingOutcome::AttestsToFutureState { + state: state.slot, + attestation: attestation_slot, + }) + } else { + self.process_attestation_for_state_and_block( + attestation, + &state, + &attestation_head_block, + ) + } } } else { // Reject any block where we have not processed `attestation.data.beacon_block_root`. diff --git a/eth2/state_processing/src/per_block_processing/errors.rs b/eth2/state_processing/src/per_block_processing/errors.rs index 436ec96cee..65179167c1 100644 --- a/eth2/state_processing/src/per_block_processing/errors.rs +++ b/eth2/state_processing/src/per_block_processing/errors.rs @@ -136,9 +136,6 @@ pub enum AttestationInvalid { delay: u64, attestation: Slot, }, - /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the - /// future). - AttestsToFutureState { state: Slot, attestation: Slot }, /// Attestation slot is too far in the past to be included in a block. IncludedTooLate { state: Slot, attestation: Slot }, /// Attestation target epoch does not match the current or previous epoch. diff --git a/eth2/state_processing/src/per_block_processing/verify_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_attestation.rs index 127d251dea..74dbefa232 100644 --- a/eth2/state_processing/src/per_block_processing/verify_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_attestation.rs @@ -62,17 +62,6 @@ pub fn verify_attestation_for_state( Invalid::BadShard ); - let attestation_slot = state.get_attestation_data_slot(&data)?; - - // An attestation cannot attest to a state that is later than itself. - verify!( - attestation_slot <= state.slot, - Invalid::AttestsToFutureState { - state: state.slot, - attestation: attestation_slot - } - ); - // Verify the Casper FFG vote and crosslink data. let parent_crosslink = verify_casper_ffg_vote(attestation, state)?; From 67fe21c1c03f550c74d6e0b190f05843770e6fcf Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 9 Aug 2019 12:32:32 +1000 Subject: [PATCH 26/34] Ensure committee cache is build during attn proc --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 7af64924e4..834b04582d 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -590,6 +590,8 @@ impl BeaconChain { per_slot_processing(&mut state, &self.spec)?; } + state.build_committee_cache(RelativeEpoch::Current, &self.spec)?; + let attestation_slot = state.get_attestation_data_slot(&attestation.data)?; // Reject any attestation where the `state` loaded from `data.beacon_block_root` From f4121d9debb4ff235d8d6c74236d00d373be4020 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 9 Aug 2019 12:34:56 +1000 Subject: [PATCH 27/34] Ignore unknown blocks at fork choice --- beacon_node/beacon_chain/src/fork_choice.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 6800f61d8c..640f5223d5 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -119,12 +119,14 @@ impl ForkChoice { // // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md for attestation in &block.body.attestations { - let block = self + // If the `data.beacon_block_root` block is not known to us, simply ignore the latest + // vote. + if let Some(block) = self .store .get::>(&attestation.data.beacon_block_root)? - .ok_or_else(|| Error::MissingBlock(attestation.data.beacon_block_root))?; - - self.process_attestation(state, attestation, &block)?; + { + self.process_attestation(state, attestation, &block)?; + } } // This does not apply a vote to the block, it just makes fork choice aware of the block so From 64a6e1475c567d9dd137033a93f6ab27291a0dd8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 11:31:31 +1000 Subject: [PATCH 28/34] Various minor fixes --- beacon_node/beacon_chain/src/beacon_chain.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 834b04582d..7fefb7690f 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -539,11 +539,11 @@ impl BeaconChain { // (e.g., if there are skip slots between the epoch the block was created in and // the epoch for the attestation). if state.current_epoch() == attestation.data.target.epoch - && (state - .get_block_root(attestation_head_block.slot) - .map(|root| *root == attestation.data.beacon_block_root) - .unwrap_or_else(|_| false) - || attestation.data.beacon_block_root == self.head().beacon_block_root) + && (attestation.data.beacon_block_root == self.head().beacon_block_root + || state + .get_block_root(attestation_head_block.slot) + .map(|root| *root == attestation.data.beacon_block_root) + .unwrap_or_else(|_| false)) { // The head state is able to be used to validate this attestation. No need to load // anything from the database. @@ -558,6 +558,7 @@ impl BeaconChain { }; if let Some(outcome) = optional_outcome { + // Verification was already completed with an in-memory state. Return that result. outcome } else { // Use the `data.beacon_block_root` to load the state from the latest non-skipped @@ -612,13 +613,13 @@ impl BeaconChain { } } } else { - // Reject any block where we have not processed `attestation.data.beacon_block_root`. + // Drop any attestation where we have not processed `attestation.data.beacon_block_root`. // // This is likely overly restrictive, we could store the attestation for later // processing. warn!( self.log, - "Dropping attestation for unknown block"; + "Dropped attestation for unknown block"; "block" => format!("{}", attestation.data.beacon_block_root) ); Ok(AttestationProcessingOutcome::UnknownHeadBlock { From 0d4b58978ccd5423f8026a8436215c14279abff4 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 17:19:27 +1000 Subject: [PATCH 29/34] Make fork choice write lock in to read lock --- eth2/lmd_ghost/src/reduced_tree.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index 9668620b79..822c388f6a 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -111,7 +111,7 @@ where } fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { - self.core.write().latest_message(validator_index) + self.core.read().latest_message(validator_index) } } @@ -258,10 +258,10 @@ where Ok(head_node.block_hash) } - pub fn latest_message(&mut self, validator_index: usize) -> Option<(Hash256, Slot)> { - match self.latest_votes.get(validator_index) { - Some(v) => Some((v.hash.clone(), v.slot.clone())), - None => None, + pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { + match self.latest_votes.get_ref(validator_index) { + Some(Some(v)) => Some((v.hash.clone(), v.slot.clone())), + _ => None, } } @@ -776,6 +776,14 @@ where &self.0[i] } + pub fn get_ref(&self, i: usize) -> Option<&T> { + if i < self.0.len() { + Some(&self.0[i]) + } else { + None + } + } + pub fn insert(&mut self, i: usize, element: T) { self.ensure(i); self.0[i] = element; From 1beab66078e41a7ae46b9c304d1dd478de1716e5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 17:21:54 +1000 Subject: [PATCH 30/34] Remove unused method --- beacon_node/beacon_chain/src/fork_choice.rs | 24 +++------------------ 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 640f5223d5..edd426f296 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -178,27 +178,9 @@ impl ForkChoice { Ok(()) } - /// Determines whether or not the given attestation contains a latest message. - pub fn should_process_attestation( - &self, - state: &BeaconState, - attestation: &Attestation, - ) -> Result { - let validator_indices = - get_attesting_indices(state, &attestation.data, &attestation.aggregation_bits)?; - - let block_slot = state.get_attestation_data_slot(&attestation.data)?; - - Ok(validator_indices - .iter() - .find(|&&v| match self.backend.latest_message(v) { - Some((_, slot)) => block_slot > slot, - None => true, - }) - .is_some()) - } - - // Returns the latest message for a given validator + /// Returns the latest message for a given validator, if any. + /// + /// Returns `(block_root, block_slot)`. pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Slot)> { self.backend.latest_message(validator_index) } From 963fb7bc87901a00a1bfcdb1c3120cdfd6985f14 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 17:36:53 +1000 Subject: [PATCH 31/34] Tidy comments --- beacon_node/beacon_chain/src/beacon_chain.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 7fefb7690f..c92a05a728 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -506,8 +506,16 @@ impl BeaconChain { /// Accept a new, potentially invalid attestation from the network. /// - /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation - /// if possible. + /// If valid, the attestation is added to `self.op_pool` and `self.fork_choice`. + /// + /// Returns an `Ok(AttestationProcessingOutcome)` if the chain was able to make a determination + /// about the `attestation` (wether it was invalid or not). Returns an `Err` if the was an + /// error during this process and no determination was able to be made. + /// + /// ## Notes + /// + /// - Whilst the `attestation` is added to fork choice, the head is not updated. That must be + /// done separately. pub fn process_attestation( &self, attestation: Attestation, @@ -538,6 +546,9 @@ impl BeaconChain { // the block doesn't necessarily need to be in the same epoch as the attestation // (e.g., if there are skip slots between the epoch the block was created in and // the epoch for the attestation). + // + // This check also ensures that the slot for `data.beacon_block_root` is not higher + // than `state.root` by ensuring that the block is in the history of `state`. if state.current_epoch() == attestation.data.target.epoch && (attestation.data.beacon_block_root == self.head().beacon_block_root || state @@ -638,9 +649,8 @@ impl BeaconChain { /// /// - `state` corresponds to the `block.state_root` identified by /// `attestation.data.beacon_block_root`. (Viz., `attestation` was created using `state`. - /// - `state.slot` is in the same epoch as `block.slot` and - /// `attestation.data.beacon_block_root` is in `state.block_roots`. (Viz., the attestation was - /// attesting to an ancestor of `state` from the same epoch as `state`. + /// - `state.slot` is in the same epoch as `data.target.epoch` and + /// `attestation.data.beacon_block_root` is in the history of `state`. /// /// Additionally, `attestation.data.beacon_block_root` **must** be available to read in /// `self.store` _and_ be the root of the given `block`. From 04bef689e33c0f3f4288a96d90bd06f91e0eacea Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 17:47:34 +1000 Subject: [PATCH 32/34] Fix attestation prod. target roots change --- beacon_node/beacon_chain/src/beacon_chain.rs | 34 ++++++++++++++++---- beacon_node/beacon_chain/src/test_utils.rs | 30 +++++++++++------ beacon_node/beacon_chain/tests/tests.rs | 1 + 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c92a05a728..7488a7795e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -450,8 +450,9 @@ impl BeaconChain { pub fn produce_attestation_data(&self, shard: u64) -> Result { let state = self.state.read(); let head_block_root = self.head().beacon_block_root; + let head_block_slot = self.head().beacon_block.slot; - self.produce_attestation_data_for_block(shard, head_block_root, &*state) + self.produce_attestation_data_for_block(shard, head_block_root, head_block_slot, &*state) } /// Produce an `AttestationData` that attests to the chain denoted by `block_root` and `state`. @@ -462,18 +463,39 @@ impl BeaconChain { &self, shard: u64, head_block_root: Hash256, + head_block_slot: Slot, state: &BeaconState, ) -> Result { // Collect some metrics. self.metrics.attestation_production_requests.inc(); let timer = self.metrics.attestation_production_times.start_timer(); + let slots_per_epoch = T::EthSpec::slots_per_epoch(); + let current_epoch_start_slot = state.current_epoch().start_slot(slots_per_epoch); + // The `target_root` is the root of the first block of the current epoch. - let target_root = self - .rev_iter_block_roots() - .find(|(_root, slot)| *slot % T::EthSpec::slots_per_epoch() == 0) - .map(|(root, _slot)| root) - .ok_or_else(|| Error::UnableToFindTargetRoot(self.head().beacon_state.slot))?; + // + // The `state` does not know the root of the block for it's current slot (it only knows + // about blocks from prior slots). This creates an edge-case when the state is on the first + // slot of the epoch -- we're unable to obtain the `target_root` because it is not a prior + // root. + // + // This edge case is handled in two ways: + // + // - If the head block is on the same slot as the state, we use it's root. + // - Otherwise, assume the current slot has been skipped and use the block root from the + // prior slot. + // + // For all other cases, we simply read the `target_root` from `state.latest_block_roots`. + let target_root = if state.slot == current_epoch_start_slot { + if head_block_slot == current_epoch_start_slot { + head_block_root + } else { + *state.get_block_root(current_epoch_start_slot - 1)? + } + } else { + *state.get_block_root(current_epoch_start_slot)? + }; let target = Checkpoint { epoch: state.current_epoch(), diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6997f52aec..298c637dbd 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -194,7 +194,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_free_attestations(&attestation_strategy, &new_state, block_root); + self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot); } else { panic!("block should be successfully processed: {:?}", outcome); } @@ -282,14 +282,20 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, + head_block_slot: Slot, ) { - self.get_free_attestations(attestation_strategy, state, head_block_root) - .into_iter() - .for_each(|attestation| { - self.chain - .process_attestation(attestation) - .expect("should process attestation"); - }); + self.get_free_attestations( + attestation_strategy, + state, + head_block_root, + head_block_slot, + ) + .into_iter() + .for_each(|attestation| { + self.chain + .process_attestation(attestation) + .expect("should process attestation"); + }); } /// Generates a `Vec` for some attestation strategy and head_block. @@ -298,6 +304,7 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, + head_block_slot: Slot, ) -> Vec> { let spec = &self.spec; let fork = &state.fork; @@ -322,7 +329,12 @@ where if attesting_validators.contains(validator_index) { let data = self .chain - .produce_attestation_data_for_block(cc.shard, head_block_root, state) + .produce_attestation_data_for_block( + cc.shard, + head_block_root, + head_block_slot, + state, + ) .expect("should produce attestation data"); let mut aggregation_bits = BitList::with_capacity(committee_size).unwrap(); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index c22f025639..22b667f159 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -395,6 +395,7 @@ fn attestations_with_increasing_slots() { &AttestationStrategy::AllValidators, &harness.chain.head().beacon_state, harness.chain.head().beacon_block_root, + harness.chain.head().beacon_block.slot, )); harness.advance_slot(); From 6c9ebf4b9647932379b64baee681ddf16e4dc144 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sun, 11 Aug 2019 09:15:39 +1000 Subject: [PATCH 33/34] Fix compile error in store iters --- beacon_node/store/src/iter.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon_node/store/src/iter.rs b/beacon_node/store/src/iter.rs index 3d01b7015d..c97241903f 100644 --- a/beacon_node/store/src/iter.rs +++ b/beacon_node/store/src/iter.rs @@ -15,7 +15,7 @@ pub trait AncestorIter { } impl<'a, U: Store, E: EthSpec> AncestorIter> for BeaconBlock { - /// Iterates across all the prior block roots of `self`, starting at the most recent and ending + /// Iterates across all available prior block roots of `self`, starting at the most recent and ending /// at genesis. fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { let state = store.get::>(&self.state_root).ok()??; @@ -25,11 +25,11 @@ impl<'a, U: Store, E: EthSpec> AncestorIter> for } impl<'a, U: Store, E: EthSpec> AncestorIter> for BeaconState { - /// Iterates across all the prior state roots of `self`, starting at the most recent and ending + /// Iterates across all available prior state roots of `self`, starting at the most recent and ending /// at genesis. fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { // The `self.clone()` here is wasteful. - Some(StateRootsIterator::owned(store, self.clone(), self.slot)) + Some(StateRootsIterator::owned(store, self.clone())) } } From 4020d13064fb6e6085e90aad23d2b1a5891f03df Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sun, 11 Aug 2019 09:34:49 +1000 Subject: [PATCH 34/34] Reject any attestation prior to finalization --- beacon_node/beacon_chain/src/beacon_chain.rs | 44 +++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d30b12c988..9ccf595893 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -69,6 +69,11 @@ pub enum AttestationProcessingOutcome { state: Slot, attestation: Slot, }, + /// The slot is finalized, no need to import. + FinalizedSlot { + attestation: Epoch, + finalized: Epoch, + }, Invalid(AttestationValidationError), } @@ -550,6 +555,23 @@ impl BeaconChain { .store .get::>(&attestation.data.beacon_block_root)? { + let finalized_epoch = self.head().beacon_state.finalized_checkpoint.epoch; + + if attestation_head_block.slot + <= finalized_epoch.start_slot(T::EthSpec::slots_per_epoch()) + { + // Ignore any attestation where the slot of `data.beacon_block_root` is equal to or + // prior to the finalized epoch. + // + // For any valid attestation if the `beacon_block_root` is prior to finalization, then + // all other parameters (source, target, etc) must all be prior to finalization and + // therefore no longer interesting. + return Ok(AttestationProcessingOutcome::FinalizedSlot { + attestation: attestation_head_block.epoch(), + finalized: finalized_epoch, + }); + } + // Attempt to process the attestation using the `self.head()` state. // // This is purely an effort to avoid loading a `BeaconState` unnecessarily from the DB. @@ -688,7 +710,27 @@ impl BeaconChain { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); - let result = if let Err(e) = + // Find the highest between: + // + // - The highest valid finalized epoch we've ever seen (i.e., the head). + // - The finalized epoch that this attestation was created against. + let finalized_epoch = std::cmp::max( + self.head().beacon_state.finalized_checkpoint.epoch, + state.finalized_checkpoint.epoch, + ); + + let result = if block.slot <= finalized_epoch.start_slot(T::EthSpec::slots_per_epoch()) { + // Ignore any attestation where the slot of `data.beacon_block_root` is equal to or + // prior to the finalized epoch. + // + // For any valid attestation if the `beacon_block_root` is prior to finalization, then + // all other parameters (source, target, etc) must all be prior to finalization and + // therefore no longer interesting. + Ok(AttestationProcessingOutcome::FinalizedSlot { + attestation: block.slot.epoch(T::EthSpec::slots_per_epoch()), + finalized: finalized_epoch, + }) + } else if let Err(e) = verify_attestation_for_state(state, &attestation, &self.spec, VerifySignatures::True) { warn!(