From 9567d591d0efe4bd0357bbcb1c1ae1c3ce0781aa Mon Sep 17 00:00:00 2001 From: samtvlabs Date: Mon, 25 Sep 2023 05:27:39 +0000 Subject: [PATCH] fix: PR comments --- .../src/capella/block_processing.rs | 66 ++++++++++--------- .../src/state_transition/error.rs | 26 +++++--- 2 files changed, 52 insertions(+), 40 deletions(-) diff --git a/ethereum-consensus/src/capella/block_processing.rs b/ethereum-consensus/src/capella/block_processing.rs index 6faea7332..4cba059b3 100644 --- a/ethereum-consensus/src/capella/block_processing.rs +++ b/ethereum-consensus/src/capella/block_processing.rs @@ -4,16 +4,17 @@ use crate::{ get_randao_mix, is_fully_withdrawable_validator, is_partially_withdrawable_validator, process_attestation, process_attester_slashing, process_block_header, process_deposit, process_eth1_data, process_proposer_slashing, process_randao, process_sync_aggregate, - process_voluntary_exit, BeaconBlock, BeaconBlockBody, BeaconState, BlsPublicKey, - DomainType, ExecutionAddress, ExecutionEngine, ExecutionPayload, ExecutionPayloadHeader, + process_voluntary_exit, BeaconBlock, BeaconBlockBody, BeaconState, DomainType, + ExecutionAddress, ExecutionEngine, ExecutionPayload, ExecutionPayloadHeader, NewPayloadRequest, SignedBlsToExecutionChange, Withdrawal, }, + crypto::hash, primitives::{BLS_WITHDRAWAL_PREFIX, ETH1_ADDRESS_WITHDRAWAL_PREFIX}, signing::verify_signed_data, ssz::prelude::*, state_transition::{ - invalid_operation_error, Context, InvalidDeposit, InvalidExecutionPayload, - InvalidOperation, InvalidWithdrawals, Result, + invalid_operation_error, Context, InvalidBlsToExecutionChange, InvalidDeposit, + InvalidExecutionPayload, InvalidOperation, InvalidWithdrawals, Result, }, }; @@ -44,22 +45,34 @@ pub fn process_bls_to_execution_change< signed_address_change: &SignedBlsToExecutionChange, context: &Context, ) -> Result<()> { - let address_change = &signed_address_change.message; + let mut address_change = signed_address_change.message.clone(); let signature = &signed_address_change.signature; if address_change.validator_index >= state.validators.len() { - return Err(invalid_operation_error(InvalidOperation::ValidatorIndex( - address_change.validator_index, - ))); + return Err(invalid_operation_error(InvalidOperation::BlsToExecutionChange( + InvalidBlsToExecutionChange::BadValidatorIndex(address_change.validator_index), + ))) } let withdrawal_credentials_prefix = state.validators[address_change.validator_index].withdrawal_credentials[0]; if withdrawal_credentials_prefix != BLS_WITHDRAWAL_PREFIX { - return Err(invalid_operation_error(InvalidOperation::WithdrawalCredentialsPrefix( - state.validators[address_change.validator_index].withdrawal_credentials[0], - ))); + return Err(invalid_operation_error(InvalidOperation::BlsToExecutionChange( + InvalidBlsToExecutionChange::WithdrawalCredentialsPrefix( + state.validators[address_change.validator_index].withdrawal_credentials[0], + ), + ))) + } + + let public_key = address_change.from_bls_public_key.clone(); + + if state.validators[address_change.validator_index].withdrawal_credentials[1..] != + hash(public_key.as_slice())[1..] + { + return Err(invalid_operation_error(InvalidOperation::BlsToExecutionChange( + InvalidBlsToExecutionChange::WithdrawalCredentialsPublicKey(public_key), + ))) } let domain = compute_domain( @@ -68,24 +81,15 @@ pub fn process_bls_to_execution_change< Some(state.genesis_validators_root), context, )?; - let public_key = &address_change.from_bls_public_key; - let signed_data = - verify_signed_data(&mut address_change.clone(), signature, public_key, domain); - if signed_data.is_err() { - return Err(invalid_operation_error(InvalidOperation::ExecutionChange( - signed_address_change.signature.clone(), - ))); - } + + verify_signed_data(&mut address_change, signature, &public_key, domain)?; let validator = &mut state.validators[address_change.validator_index]; validator.withdrawal_credentials[0] = ETH1_ADDRESS_WITHDRAWAL_PREFIX; - let mut withdrawal_credentials: Vec = validator.withdrawal_credentials.as_ref().to_vec(); - withdrawal_credentials[12..].copy_from_slice(address_change.to_execution_address.as_ref()); - let withdrawal_credentials: &[u8] = &withdrawal_credentials; - let withdrawal_credentials = - ByteVector::<32>::try_from(withdrawal_credentials).expect("Wrong size"); - validator.withdrawal_credentials = withdrawal_credentials; + validator.withdrawal_credentials[1..12].fill(0); + validator.withdrawal_credentials[12..] + .copy_from_slice(address_change.to_execution_address.as_ref()); Ok(()) } @@ -150,7 +154,7 @@ pub fn process_operations< expected: expected_deposit_count, count: body.deposits.len(), }, - ))); + ))) } body.proposer_slashings .iter_mut() @@ -222,7 +226,7 @@ pub fn process_execution_payload< expected: state.latest_execution_payload_header.block_hash.clone(), } .into(), - )); + )) } let current_epoch = get_current_epoch(state, context); @@ -234,7 +238,7 @@ pub fn process_execution_payload< expected: randao_mix.clone(), } .into(), - )); + )) } let timestamp = compute_timestamp_at_slot(state, state.slot, context)?; @@ -245,7 +249,7 @@ pub fn process_execution_payload< expected: timestamp, } .into(), - )); + )) } let new_payload_request = NewPayloadRequest(payload); @@ -316,7 +320,7 @@ pub fn process_withdrawals< provided: execution_payload.withdrawals.to_vec(), expected: expected_withdrawals, }, - ))); + ))) } for withdrawal in &expected_withdrawals { @@ -401,7 +405,7 @@ pub fn get_expected_withdrawals< withdrawal_index += 1; } if withdrawals.len() == context.max_withdrawals_per_payload { - break; + break } validator_index = (validator_index + 1) % state.validators.len(); } diff --git a/ethereum-consensus/src/state_transition/error.rs b/ethereum-consensus/src/state_transition/error.rs index f61ebe081..9b60cf0e9 100644 --- a/ethereum-consensus/src/state_transition/error.rs +++ b/ethereum-consensus/src/state_transition/error.rs @@ -1,5 +1,5 @@ use crate::{ - capella::Withdrawal, + capella::{BlsPublicKey, Withdrawal}, crypto::Error as CryptoError, phase0::{AttestationData, BeaconBlockHeader, Checkpoint}, primitives::{BlsSignature, Bytes32, Epoch, Hash32, Root, Slot, ValidatorIndex}, @@ -85,14 +85,10 @@ pub enum InvalidOperation { SyncAggregate(#[from] InvalidSyncAggregate), #[error("invalid execution payload: {0}")] ExecutionPayload(#[from] InvalidExecutionPayload), - #[error("invalid bls signature for execution chang {0:?}")] - ExecutionChange(BlsSignature), - #[error("validator index is out of bounds {0}")] - ValidatorIndex(usize), - #[error("invalid withdrawal credentials prefix{0}")] - WithdrawalCredentialsPrefix(u8), - #[error("invalid withdrawal credentials public key{0:?}")] - WithdrawalCredentialsPublicKey(&'static [u8]), + #[error("invalid withdrawals: {0}")] + Withdrawal(#[from] InvalidWithdrawals), + #[error("invalid bls signature to execution change: {0}")] + BlsToExecutionChange(#[from] InvalidBlsToExecutionChange), } #[derive(Debug, Error)] @@ -195,6 +191,18 @@ pub enum InvalidWithdrawals { IncorrectWithdrawals { provided: Vec, expected: Vec }, } +#[derive(Debug, Error)] +pub enum InvalidBlsToExecutionChange { + #[error("invalid bls signature for execution change {0:?}")] + ExecutionChange(BlsSignature), + #[error("validator index is out of bounds {0}")] + BadValidatorIndex(usize), + #[error("invalid withdrawal credentials prefix{0}")] + WithdrawalCredentialsPrefix(u8), + #[error("invalid withdrawal credentials public key{0:?}")] + WithdrawalCredentialsPublicKey(BlsPublicKey), +} + #[derive(Debug, Error)] pub enum InvalidSyncAggregate { #[error("invalid sync committee aggregate signature {signature} signing over previous slot block root {root}")]