diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e8dcd50ab..8982cdf79 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 0b8fae7bf..7a51fc425 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 71415d191..83d6c335f 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 92d5fb168..ba9ca81c0 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 3acadfde2..a64158ac9 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 bca6a9085..74dbefa23 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