Operation pool: refactor verify_deposit/exit

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,
+    validate_attestation, verify_deposit, verify_exit, verify_exit_time_independent_only,
-    verify_transfer, verify_transfer_partial,
+    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<DepositInsertStatus, ()> {
         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| {
+            .take_while(Option::is_some)
-                // 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)
-                })
-            })
             .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<VoluntaryExit> {
         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 deposit1 = make_deposit(rng, state, spec);
-        let mut deposit2 = Deposit::random_for_test(&mut rng);
+        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),
+            op_pool.insert_deposit(deposit1.clone(), state, spec),
-            Replaced(Box::new(deposit1))
+            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<Deposit> {
-        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 deposits1 = dummy_deposits(rng, &state, &spec, start1, count);
-        let deposits2 = dummy_deposits(rng, start2, 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<Deposit> {
+        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
 }

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.

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(
+fn verify_deposit_merkle_proof(state: &BeaconState, deposit: &Deposit, spec: &ChainSpec) -> bool {
-    state: &BeaconState,
-    deposit: &Deposit,
-    spec: &ChainSpec,
-) -> bool {
     let leaf = hash(&get_serialized_deposit_data(deposit));
     verify_merkle_proof(
         Hash256::from_slice(&leaf),

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

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
     }