diff --git a/eth2/operation_pool/src/lib.rs b/eth2/operation_pool/src/lib.rs index c625cdc832..ecf4e41e7b 100644 --- a/eth2/operation_pool/src/lib.rs +++ b/eth2/operation_pool/src/lib.rs @@ -3,8 +3,8 @@ use itertools::Itertools; use ssz::ssz_encode; use state_processing::per_block_processing::errors::ProposerSlashingValidationError; use state_processing::per_block_processing::{ - validate_attestation, verify_deposit_merkle_proof, verify_exit, verify_proposer_slashing, - verify_transfer, verify_transfer_partial, + validate_attestation, verify_deposit, verify_exit, verify_exit_time_independent_only, + verify_proposer_slashing, verify_transfer, verify_transfer_partial, }; use std::collections::{btree_map::Entry, hash_map, BTreeMap, HashMap, HashSet}; use types::chain_spec::Domain; @@ -179,19 +179,27 @@ impl OperationPool { /// Add a deposit to the pool. /// /// No two distinct deposits should be added with the same index. - pub fn insert_deposit(&mut self, deposit: Deposit) -> DepositInsertStatus { + pub fn insert_deposit( + &mut self, + deposit: Deposit, + state: &BeaconState, + spec: &ChainSpec, + ) -> Result { use DepositInsertStatus::*; match self.deposits.entry(deposit.index) { Entry::Vacant(entry) => { + // FIXME: error prop + verify_deposit(state, &deposit, VERIFY_DEPOSIT_PROOFS, spec).map_err(|_| ())?; entry.insert(deposit); - Fresh + Ok(Fresh) } Entry::Occupied(mut entry) => { if entry.get() == &deposit { - Duplicate + Ok(Duplicate) } else { - Replaced(Box::new(entry.insert(deposit))) + verify_deposit(state, &deposit, VERIFY_DEPOSIT_PROOFS, spec).map_err(|_| ())?; + Ok(Replaced(Box::new(entry.insert(deposit)))) } } } @@ -204,14 +212,7 @@ impl OperationPool { let start_idx = state.deposit_index; (start_idx..start_idx + spec.max_deposits) .map(|idx| self.deposits.get(&idx)) - .take_while(|deposit| { - // NOTE: we don't use verify_deposit, because it requires the - // deposit's index to match the state's, and we would like to return - // a batch with increasing indices - deposit.map_or(false, |deposit| { - !VERIFY_DEPOSIT_PROOFS || verify_deposit_merkle_proof(state, deposit, spec) - }) - }) + .take_while(Option::is_some) .flatten() .cloned() .collect() @@ -287,7 +288,7 @@ impl OperationPool { state: &BeaconState, spec: &ChainSpec, ) -> Result<(), ()> { - verify_exit(state, &exit, spec, false).map_err(|_| ())?; + verify_exit_time_independent_only(state, &exit, spec).map_err(|_| ())?; self.voluntary_exits.insert(exit.validator_index, exit); Ok(()) } @@ -297,7 +298,7 @@ impl OperationPool { pub fn get_voluntary_exits(&self, state: &BeaconState, spec: &ChainSpec) -> Vec { filter_limit_operations( self.voluntary_exits.values(), - |exit| verify_exit(state, exit, spec, true).is_ok(), + |exit| verify_exit(state, exit, spec).is_ok(), spec.max_voluntary_exits, ) } @@ -398,53 +399,51 @@ mod tests { use super::DepositInsertStatus::*; use super::*; use types::test_utils::{SeedableRng, TestRandom, XorShiftRng}; + use types::*; #[test] fn insert_deposit() { - let mut rng = XorShiftRng::from_seed([42; 16]); + let rng = &mut XorShiftRng::from_seed([42; 16]); + let (ref spec, ref state) = test_state(rng); let mut op_pool = OperationPool::new(); - let deposit1 = Deposit::random_for_test(&mut rng); - let mut deposit2 = Deposit::random_for_test(&mut rng); + let deposit1 = make_deposit(rng, state, spec); + let mut deposit2 = make_deposit(rng, state, spec); deposit2.index = deposit1.index; - assert_eq!(op_pool.insert_deposit(deposit1.clone()), Fresh); - assert_eq!(op_pool.insert_deposit(deposit1.clone()), Duplicate); assert_eq!( - op_pool.insert_deposit(deposit2), - Replaced(Box::new(deposit1)) + op_pool.insert_deposit(deposit1.clone(), state, spec), + Ok(Fresh) + ); + assert_eq!( + op_pool.insert_deposit(deposit1.clone(), state, spec), + Ok(Duplicate) + ); + assert_eq!( + op_pool.insert_deposit(deposit2, state, spec), + Ok(Replaced(Box::new(deposit1))) ); - } - - // Create `count` dummy deposits with sequential deposit IDs beginning from `start`. - fn dummy_deposits(rng: &mut XorShiftRng, start: u64, count: u64) -> Vec { - let proto_deposit = Deposit::random_for_test(rng); - (start..start + count) - .map(|index| { - let mut deposit = proto_deposit.clone(); - deposit.index = index; - deposit - }) - .collect() } #[test] fn get_deposits_max() { - let mut rng = XorShiftRng::from_seed([42; 16]); + let rng = &mut XorShiftRng::from_seed([42; 16]); + let (spec, mut state) = test_state(rng); let mut op_pool = OperationPool::new(); - let spec = ChainSpec::foundation(); let start = 10000; let max_deposits = spec.max_deposits; let extra = 5; let offset = 1; assert!(offset <= extra); - let deposits = dummy_deposits(&mut rng, start, max_deposits + extra); + let deposits = dummy_deposits(rng, &state, &spec, start, max_deposits + extra); for deposit in &deposits { - assert_eq!(op_pool.insert_deposit(deposit.clone()), Fresh); + assert_eq!( + op_pool.insert_deposit(deposit.clone(), &state, &spec), + Ok(Fresh) + ); } - let mut state = BeaconState::random_for_test(&mut rng); state.deposit_index = start + offset; let deposits_for_block = op_pool.get_deposits(&state, &spec); @@ -458,18 +457,20 @@ mod tests { #[test] fn prune_deposits() { let rng = &mut XorShiftRng::from_seed([42; 16]); + let (spec, state) = test_state(rng); let mut op_pool = OperationPool::new(); let start1 = 100; - let count = 100; + // test is super slow in debug mode if this parameter is too high + let count = 5; let gap = 25; let start2 = start1 + count + gap; - let deposits1 = dummy_deposits(rng, start1, count); - let deposits2 = dummy_deposits(rng, start2, count); + let deposits1 = dummy_deposits(rng, &state, &spec, start1, count); + let deposits2 = dummy_deposits(rng, &state, &spec, start2, count); for d in deposits1.into_iter().chain(deposits2) { - op_pool.insert_deposit(d); + assert!(op_pool.insert_deposit(d, &state, &spec).is_ok()); } assert_eq!(op_pool.num_deposits(), 2 * count as usize); @@ -504,5 +505,50 @@ mod tests { assert_eq!(op_pool.num_deposits(), 0); } + // Create a random deposit (with a valid proof of posession) + fn make_deposit(rng: &mut XorShiftRng, state: &BeaconState, spec: &ChainSpec) -> Deposit { + let keypair = Keypair::random(); + let mut deposit = Deposit::random_for_test(rng); + let mut deposit_input = DepositInput { + pubkey: keypair.pk.clone(), + withdrawal_credentials: Hash256::zero(), + proof_of_possession: Signature::empty_signature(), + }; + deposit_input.proof_of_possession = deposit_input.create_proof_of_possession( + &keypair.sk, + state.slot.epoch(spec.slots_per_epoch), + &state.fork, + spec, + ); + deposit.deposit_data.deposit_input = deposit_input; + deposit + } + + // Create `count` dummy deposits with sequential deposit IDs beginning from `start`. + fn dummy_deposits( + rng: &mut XorShiftRng, + state: &BeaconState, + spec: &ChainSpec, + start: u64, + count: u64, + ) -> Vec { + let proto_deposit = make_deposit(rng, state, spec); + (start..start + count) + .map(|index| { + let mut deposit = proto_deposit.clone(); + deposit.index = index; + deposit + }) + .collect() + } + + fn test_state(rng: &mut XorShiftRng) -> (ChainSpec, BeaconState) { + let spec = ChainSpec::foundation(); + let mut state = BeaconState::random_for_test(rng); + state.fork = Fork::genesis(&spec); + + (spec, state) + } + // TODO: more tests } diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index e0e3595529..617da00d4d 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -11,9 +11,8 @@ pub use self::verify_proposer_slashing::verify_proposer_slashing; pub use validate_attestation::{validate_attestation, validate_attestation_without_signature}; pub use verify_deposit::{ get_existing_validator_index, verify_deposit, verify_deposit_index, - verify_deposit_merkle_proof, }; -pub use verify_exit::verify_exit; +pub use verify_exit::{verify_exit, verify_exit_time_independent_only}; pub use verify_slashable_attestation::verify_slashable_attestation; pub use verify_transfer::{execute_transfer, verify_transfer, verify_transfer_partial}; @@ -429,7 +428,7 @@ pub fn process_exits( .par_iter() .enumerate() .try_for_each(|(i, exit)| { - verify_exit(&state, exit, spec, true).map_err(|e| e.into_with_index(i)) + verify_exit(&state, exit, spec).map_err(|e| e.into_with_index(i)) })?; // Update the state in series. diff --git a/eth2/state_processing/src/per_block_processing/verify_deposit.rs b/eth2/state_processing/src/per_block_processing/verify_deposit.rs index 1b974d972a..a3a0f5734c 100644 --- a/eth2/state_processing/src/per_block_processing/verify_deposit.rs +++ b/eth2/state_processing/src/per_block_processing/verify_deposit.rs @@ -89,11 +89,7 @@ pub fn get_existing_validator_index( /// Verify that a deposit is included in the state's eth1 deposit root. /// /// Spec v0.5.0 -pub fn verify_deposit_merkle_proof( - state: &BeaconState, - deposit: &Deposit, - spec: &ChainSpec, -) -> bool { +fn verify_deposit_merkle_proof(state: &BeaconState, deposit: &Deposit, spec: &ChainSpec) -> bool { let leaf = hash(&get_serialized_deposit_data(deposit)); verify_merkle_proof( Hash256::from_slice(&leaf), diff --git a/eth2/state_processing/src/per_block_processing/verify_exit.rs b/eth2/state_processing/src/per_block_processing/verify_exit.rs index 14dad3442c..a3b6943950 100644 --- a/eth2/state_processing/src/per_block_processing/verify_exit.rs +++ b/eth2/state_processing/src/per_block_processing/verify_exit.rs @@ -7,17 +7,30 @@ use types::*; /// /// Returns `Ok(())` if the `Exit` is valid, otherwise indicates the reason for invalidity. /// -/// The `check_future_epoch` argument determines whether the exit's epoch should be checked -/// against the state's current epoch to ensure it doesn't occur in the future. -/// It should ordinarily be set to true, except for operations stored for -/// some time (such as in the OperationPool). -/// /// Spec v0.5.0 pub fn verify_exit( state: &BeaconState, exit: &VoluntaryExit, spec: &ChainSpec, - check_future_epoch: bool, +) -> Result<(), Error> { + verify_exit_parametric(state, exit, spec, false) +} + +/// Like `verify_exit` but doesn't run checks which may become true in future states. +pub fn verify_exit_time_independent_only( + state: &BeaconState, + exit: &VoluntaryExit, + spec: &ChainSpec, +) -> Result<(), Error> { + verify_exit_parametric(state, exit, spec, true) +} + +/// Parametric version of `verify_exit` that skips some checks if `time_independent_only` is true. +fn verify_exit_parametric( + state: &BeaconState, + exit: &VoluntaryExit, + spec: &ChainSpec, + time_independent_only: bool, ) -> Result<(), Error> { let validator = state .validator_registry @@ -38,7 +51,7 @@ pub fn verify_exit( // Exits must specify an epoch when they become valid; they are not valid before then. verify!( - !check_future_epoch || state.current_epoch(spec) >= exit.epoch, + time_independent_only || state.current_epoch(spec) >= exit.epoch, Invalid::FutureEpoch { state: state.current_epoch(spec), exit: exit.epoch diff --git a/eth2/types/src/attestation.rs b/eth2/types/src/attestation.rs index 6c572c8528..043711015a 100644 --- a/eth2/types/src/attestation.rs +++ b/eth2/types/src/attestation.rs @@ -31,7 +31,9 @@ pub struct Attestation { impl Attestation { /// Are the aggregation bitfields of these attestations disjoint? pub fn signers_disjoint_from(&self, other: &Attestation) -> bool { - self.aggregation_bitfield.intersection(&other.aggregation_bitfield).is_zero() + self.aggregation_bitfield + .intersection(&other.aggregation_bitfield) + .is_zero() } /// Aggregate another Attestation into this one. @@ -41,7 +43,8 @@ impl Attestation { debug_assert_eq!(self.data, other.data); debug_assert!(self.signers_disjoint_from(other)); - self.aggregation_bitfield.union_inplace(&other.aggregation_bitfield); + self.aggregation_bitfield + .union_inplace(&other.aggregation_bitfield); self.custody_bitfield.union_inplace(&other.custody_bitfield); // FIXME: signature aggregation once our BLS library wraps it }