Fix bugs in deposit processing

eth2/state_processing/src/per_block_processing.rs

@@ -10,7 +10,10 @@ pub use validate_attestation::{
     validate_attestation, validate_attestation_time_independent_only,
     validate_attestation_without_signature,
 };
-pub use verify_deposit::{get_existing_validator_index, verify_deposit, verify_deposit_index};
+pub use verify_deposit::{
+    get_existing_validator_index, verify_deposit_index, verify_deposit_merkle_proof,
+    verify_deposit_signature,
+};
 pub use verify_exit::{verify_exit, verify_exit_time_independent_only};
 pub use verify_indexed_attestation::{
     verify_indexed_attestation, verify_indexed_attestation_without_signature,
@@ -28,9 +31,6 @@ mod verify_indexed_attestation;
 mod verify_proposer_slashing;
 mod verify_transfer;
 
-// Set to `true` to check the merkle proof that a deposit is in the eth1 deposit root.
-const VERIFY_DEPOSIT_MERKLE_PROOFS: bool = true;
-
 /// 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
@@ -371,14 +371,15 @@ pub fn process_deposits<T: EthSpec>(
         .par_iter()
         .enumerate()
         .try_for_each(|(i, deposit)| {
-            verify_deposit(state, deposit, VERIFY_DEPOSIT_MERKLE_PROOFS, spec)
-                .map_err(|e| e.into_with_index(i))
+            verify_deposit_merkle_proof(state, deposit, spec).map_err(|e| e.into_with_index(i))
         })?;
 
     // Check `state.deposit_index` and update the state in series.
     for (i, deposit) in deposits.iter().enumerate() {
         verify_deposit_index(state, deposit).map_err(|e| e.into_with_index(i))?;
 
+        state.deposit_index += 1;
+
         // Ensure the state's pubkey cache is fully up-to-date, it will be used to check to see if the
         // depositing validator already exists in the registry.
         state.update_pubkey_cache()?;
@@ -396,6 +397,12 @@ pub fn process_deposits<T: EthSpec>(
             // Update the existing validator balance.
             safe_add_assign!(state.balances[index as usize], amount);
         } else {
+            // The signature should be checked for new validators. Return early for a bad
+            // signature.
+            if verify_deposit_signature(state, deposit, spec).is_err() {
+                return Ok(());
+            }
+
             // Create a new validator.
             let validator = Validator {
                 pubkey: deposit.data.pubkey.clone(),
@@ -413,8 +420,6 @@ pub fn process_deposits<T: EthSpec>(
             state.validator_registry.push(validator);
             state.balances.push(deposit.data.amount);
         }
-
-        state.deposit_index += 1;
     }
 
     Ok(())

eth2/state_processing/src/per_block_processing/errors.rs

@@ -321,8 +321,8 @@ pub enum DepositValidationError {
 pub enum DepositInvalid {
     /// The deposit index does not match the state index.
     BadIndex { state: u64, deposit: u64 },
-    /// The proof-of-possession does not match the given pubkey.
-    BadProofOfPossession,
+    /// The signature (proof-of-possession) does not match the given pubkey.
+    BadSignature,
     /// The withdrawal credentials for the depositing validator did not match the withdrawal
     /// credentials of an existing validator with the same public key.
     BadWithdrawalCredentials,

eth2/state_processing/src/per_block_processing/verify_deposit.rs

@@ -1,44 +1,23 @@
 use super::errors::{DepositInvalid as Invalid, DepositValidationError as Error};
-use hashing::hash;
 use merkle_proof::verify_merkle_proof;
-use ssz::ssz_encode;
-use ssz_derive::Encode;
 use tree_hash::{SignedRoot, TreeHash};
 use types::*;
 
-/// Indicates if a `Deposit` is valid to be included in a block in the current epoch of the given
-/// state.
-///
-/// Returns `Ok(())` if the `Deposit` is valid, otherwise indicates the reason for invalidity.
-///
-/// This function _does not_ check `state.deposit_index` so this function may be run in parallel.
-/// See the `verify_deposit_index` function for this.
-///
-/// Note: this function is incomplete.
+/// Verify `Deposit.pubkey` signed `Deposit.signature`.
 ///
 /// Spec v0.6.1
-pub fn verify_deposit<T: EthSpec>(
+pub fn verify_deposit_signature<T: EthSpec>(
     state: &BeaconState<T>,
     deposit: &Deposit,
-    verify_merkle_branch: bool,
     spec: &ChainSpec,
 ) -> Result<(), Error> {
-    if verify_merkle_branch {
-        verify!(
-            verify_deposit_merkle_proof(state, deposit, spec),
-            Invalid::BadMerkleProof
-        );
-    }
-
-    // NOTE: proof of possession should only be verified when the validator
-    // is not already part of the registry
     verify!(
         deposit.data.signature.verify(
             &deposit.data.signed_root(),
             spec.get_domain(state.current_epoch(), Domain::Deposit, &state.fork),
             &deposit.data.pubkey,
         ),
-        Invalid::BadProofOfPossession
+        Invalid::BadSignature
     );
 
     Ok(())
@@ -91,17 +70,23 @@ pub fn get_existing_validator_index<T: EthSpec>(
 /// Verify that a deposit is included in the state's eth1 deposit root.
 ///
 /// Spec v0.6.1
-fn verify_deposit_merkle_proof<T: EthSpec>(
+pub fn verify_deposit_merkle_proof<T: EthSpec>(
     state: &BeaconState<T>,
     deposit: &Deposit,
     spec: &ChainSpec,
-) -> bool {
+) -> Result<(), Error> {
     let leaf = deposit.data.tree_hash_root();
-    verify_merkle_proof(
-        Hash256::from_slice(&leaf),
-        &deposit.proof[..],
-        spec.deposit_contract_tree_depth as usize,
-        deposit.index as usize,
-        state.latest_eth1_data.deposit_root,
-    )
+
+    verify!(
+        verify_merkle_proof(
+            Hash256::from_slice(&leaf),
+            &deposit.proof[..],
+            spec.deposit_contract_tree_depth as usize,
+            deposit.index as usize,
+            state.latest_eth1_data.deposit_root,
+        ),
+        Invalid::BadMerkleProof
+    );
+
+    Ok(())
 }