diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index f74af9fb0..79a452185 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/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( .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( // 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( state.validator_registry.push(validator); state.balances.push(deposit.data.amount); } - - state.deposit_index += 1; } Ok(()) diff --git a/eth2/state_processing/src/per_block_processing/errors.rs b/eth2/state_processing/src/per_block_processing/errors.rs index b6a1fd629..3d308153e 100644 --- a/eth2/state_processing/src/per_block_processing/errors.rs +++ b/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, 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 121aa5bc2..6c3109764 100644 --- a/eth2/state_processing/src/per_block_processing/verify_deposit.rs +++ b/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( +pub fn verify_deposit_signature( state: &BeaconState, 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( /// Verify that a deposit is included in the state's eth1 deposit root. /// /// Spec v0.6.1 -fn verify_deposit_merkle_proof( +pub fn verify_deposit_merkle_proof( state: &BeaconState, 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(()) }