From 2d01ae60360de4a6de159fd3d5380c5f91b438f7 Mon Sep 17 00:00:00 2001 From: Mark Mackey Date: Wed, 9 Nov 2022 19:28:14 -0600 Subject: [PATCH] Fixed compiling with withdrawals enabled --- beacon_node/beacon_chain/src/beacon_chain.rs | 12 ++++ beacon_node/store/src/partial_beacon_state.rs | 17 ++---- .../process_operations.rs | 8 ++- .../verify_bls_to_execution_change.rs | 57 +++++++++++++++++++ .../state_processing/src/upgrade/capella.rs | 4 +- .../state_processing/src/upgrade/eip4844.rs | 4 +- consensus/types/src/beacon_block_body.rs | 16 ++++++ .../types/src/bls_to_execution_change.rs | 30 ++++++++++ consensus/types/src/signed_beacon_block.rs | 8 +++ .../src/signed_bls_to_execution_change.rs | 26 +++++++++ 10 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs create mode 100644 consensus/types/src/bls_to_execution_change.rs create mode 100644 consensus/types/src/signed_bls_to_execution_change.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index a7d0fe5c6..fd4cf99f1 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -261,6 +261,8 @@ struct PartialBeaconBlock> { voluntary_exits: Vec, sync_aggregate: Option>, prepare_payload_handle: Option>, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes: Vec, } pub type BeaconForkChoice = ForkChoice< @@ -3485,6 +3487,9 @@ impl BeaconChain { let eth1_data = eth1_chain.eth1_data_for_block_production(&state, &self.spec)?; let deposits = eth1_chain.deposits_for_block_inclusion(&state, ð1_data, &self.spec)?; + let bls_to_execution_changes = self + .op_pool + .get_bls_to_execution_changes(&state, &self.spec); // Iterate through the naive aggregation pool and ensure all the attestations from there // are included in the operation pool. @@ -3642,6 +3647,7 @@ impl BeaconChain { voluntary_exits, sync_aggregate, prepare_payload_handle, + bls_to_execution_changes, }) } @@ -3670,6 +3676,8 @@ impl BeaconChain { // this function. We can assume that the handle has already been consumed in order to // produce said `execution_payload`. prepare_payload_handle: _, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, } = partial_beacon_block; let inner_block = match &state { @@ -3751,6 +3759,8 @@ impl BeaconChain { .to_payload() .try_into() .map_err(|_| BlockProductionError::InvalidPayloadFork)?, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes: bls_to_execution_changes.into(), }, }), BeaconState::Eip4844(_) => BeaconBlock::Eip4844(BeaconBlockEip4844 { @@ -3773,6 +3783,8 @@ impl BeaconChain { .to_payload() .try_into() .map_err(|_| BlockProductionError::InvalidPayloadFork)?, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes: bls_to_execution_changes.into(), //FIXME(sean) get blobs blob_kzg_commitments: VariableList::from(kzg_commitments), }, diff --git a/beacon_node/store/src/partial_beacon_state.rs b/beacon_node/store/src/partial_beacon_state.rs index 5cff00529..ad52bc5b8 100644 --- a/beacon_node/store/src/partial_beacon_state.rs +++ b/beacon_node/store/src/partial_beacon_state.rs @@ -107,13 +107,10 @@ where // Withdrawals #[cfg(feature = "withdrawals")] #[superstruct(only(Capella, Eip4844))] - pub withdrawal_queue: VariableList, - #[cfg(feature = "withdrawals")] - #[superstruct(only(Capella, Eip4844))] pub next_withdrawal_index: u64, #[cfg(feature = "withdrawals")] #[superstruct(only(Capella, Eip4844))] - pub next_partial_withdrawal_validator_index: u64, + pub latest_withdrawal_validator_index: u64, } /// Implement the conversion function from BeaconState -> PartialBeaconState. @@ -215,9 +212,8 @@ impl PartialBeaconState { next_sync_committee, inactivity_scores, latest_execution_payload_header, - withdrawal_queue, next_withdrawal_index, - next_partial_withdrawal_validator_index + latest_withdrawal_validator_index ] ), #[cfg(not(feature = "withdrawals"))] @@ -248,9 +244,8 @@ impl PartialBeaconState { next_sync_committee, inactivity_scores, latest_execution_payload_header, - withdrawal_queue, next_withdrawal_index, - next_partial_withdrawal_validator_index + latest_withdrawal_validator_index ] ), #[cfg(not(feature = "withdrawals"))] @@ -467,9 +462,8 @@ impl TryInto> for PartialBeaconState { next_sync_committee, inactivity_scores, latest_execution_payload_header, - withdrawal_queue, next_withdrawal_index, - next_partial_withdrawal_validator_index + latest_withdrawal_validator_index ] ), #[cfg(not(feature = "withdrawals"))] @@ -498,9 +492,8 @@ impl TryInto> for PartialBeaconState { next_sync_committee, inactivity_scores, latest_execution_payload_header, - withdrawal_queue, next_withdrawal_index, - next_partial_withdrawal_validator_index + latest_withdrawal_validator_index ] ), #[cfg(not(feature = "withdrawals"))] diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index a85cbce6e..cc8be937d 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -300,7 +300,9 @@ pub fn process_bls_to_execution_changes<'a, T: EthSpec, Payload: AbstractExecPay | BeaconBlockBodyRef::Altair(_) | BeaconBlockBodyRef::Merge(_) => Ok(()), BeaconBlockBodyRef::Capella(_) | BeaconBlockBodyRef::Eip4844(_) => { - for (i, signed_address_change) in block_body.bls_to_execution_changes()?.enumerate() { + for (i, signed_address_change) in + block_body.bls_to_execution_changes()?.iter().enumerate() + { verify_bls_to_execution_change( state, &signed_address_change, @@ -310,9 +312,9 @@ pub fn process_bls_to_execution_changes<'a, T: EthSpec, Payload: AbstractExecPay .map_err(|e| e.into_with_index(i))?; state - .get_validator_mut(signed_address_change.message.validator_index)? + .get_validator_mut(signed_address_change.message.validator_index as usize)? .change_withdrawal_credentials( - signed_address_change.message.to_execution_address, + &signed_address_change.message.to_execution_address, spec, ); } diff --git a/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs b/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs new file mode 100644 index 000000000..3c1569145 --- /dev/null +++ b/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs @@ -0,0 +1,57 @@ +use super::errors::{BlockOperationError, BlsExecutionChangeInvalid as Invalid}; +use crate::per_block_processing::signature_sets::bls_execution_change_signature_set; +use crate::VerifySignatures; +use eth2_hashing::hash; +use types::*; + +type Result = std::result::Result>; + +fn error(reason: Invalid) -> BlockOperationError { + BlockOperationError::invalid(reason) +} + +/// Indicates if a `BlsToExecutionChange` is valid to be included in a block in the current epoch of the given +/// state. +/// +/// Returns `Ok(())` if the `SignedBlsToExecutionChange` is valid, otherwise indicates the reason for invalidity. +pub fn verify_bls_to_execution_change( + state: &BeaconState, + signed_address_change: &SignedBlsToExecutionChange, + verify_signatures: VerifySignatures, + spec: &ChainSpec, +) -> Result<()> { + let address_change = &signed_address_change.message; + + let validator = state + .validators() + .get(address_change.validator_index as usize) + .ok_or_else(|| error(Invalid::ValidatorUnknown(address_change.validator_index)))?; + + verify!( + validator + .withdrawal_credentials + .as_bytes() + .first() + .map(|byte| *byte == spec.bls_withdrawal_prefix_byte) + .unwrap_or(false), + Invalid::NonBlsWithdrawalCredentials + ); + + let pubkey_hash = hash(address_change.from_bls_pubkey.as_serialized()); + + // FIXME: Should this check be put inside the verify_signatures.is_true() condition? + // I believe that's used for fuzzing so this is a Mehdi question.. + verify!( + validator.withdrawal_credentials.as_bytes()[1..] == pubkey_hash[1..], + Invalid::WithdrawalCredentialsMismatch + ); + + if verify_signatures.is_true() { + verify!( + bls_execution_change_signature_set(state, signed_address_change, spec,)?.verify(), + Invalid::BadSignature + ); + } + + Ok(()) +} diff --git a/consensus/state_processing/src/upgrade/capella.rs b/consensus/state_processing/src/upgrade/capella.rs index e64c83980..e4e8ed853 100644 --- a/consensus/state_processing/src/upgrade/capella.rs +++ b/consensus/state_processing/src/upgrade/capella.rs @@ -58,11 +58,9 @@ pub fn upgrade_to_capella( latest_execution_payload_header: pre.latest_execution_payload_header.upgrade_to_capella(), // Withdrawals #[cfg(feature = "withdrawals")] - withdrawal_queue: VariableList::empty(), - #[cfg(feature = "withdrawals")] next_withdrawal_index: 0, #[cfg(feature = "withdrawals")] - next_partial_withdrawal_validator_index: 0, + latest_withdrawal_validator_index: 0, // Caches total_active_balance: pre.total_active_balance, committee_caches: mem::take(&mut pre.committee_caches), diff --git a/consensus/state_processing/src/upgrade/eip4844.rs b/consensus/state_processing/src/upgrade/eip4844.rs index 78fb16033..8ef3a21b1 100644 --- a/consensus/state_processing/src/upgrade/eip4844.rs +++ b/consensus/state_processing/src/upgrade/eip4844.rs @@ -65,11 +65,9 @@ pub fn upgrade_to_eip4844( latest_execution_payload_header: pre.latest_execution_payload_header.upgrade_to_eip4844(), // Withdrawals #[cfg(feature = "withdrawals")] - withdrawal_queue: mem::take(&mut pre.withdrawal_queue), - #[cfg(feature = "withdrawals")] next_withdrawal_index: pre.next_withdrawal_index, #[cfg(feature = "withdrawals")] - next_partial_withdrawal_validator_index: pre.next_partial_withdrawal_validator_index, + latest_withdrawal_validator_index: 0, // Caches total_active_balance: pre.total_active_balance, committee_caches: mem::take(&mut pre.committee_caches), diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index ce5c12774..c0d7b243e 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -301,6 +301,8 @@ impl From>> voluntary_exits, sync_aggregate, execution_payload: FullPayloadCapella { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, } = body; ( @@ -317,6 +319,8 @@ impl From>> execution_payload: BlindedPayloadCapella { execution_payload_header: From::from(execution_payload.clone()), }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, }, Some(execution_payload), ) @@ -341,6 +345,8 @@ impl From>> voluntary_exits, sync_aggregate, execution_payload: FullPayloadEip4844 { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, blob_kzg_commitments, } = body; @@ -358,6 +364,8 @@ impl From>> execution_payload: BlindedPayloadEip4844 { execution_payload_header: From::from(execution_payload.clone()), }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, blob_kzg_commitments, }, Some(execution_payload), @@ -425,6 +433,8 @@ impl BeaconBlockBodyCapella> { voluntary_exits, sync_aggregate, execution_payload: FullPayloadCapella { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, } = self; BeaconBlockBodyCapella { @@ -440,6 +450,8 @@ impl BeaconBlockBodyCapella> { execution_payload: BlindedPayloadCapella { execution_payload_header: From::from(execution_payload.clone()), }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes: bls_to_execution_changes.clone(), } } } @@ -457,6 +469,8 @@ impl BeaconBlockBodyEip4844> { voluntary_exits, sync_aggregate, execution_payload: FullPayloadEip4844 { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, blob_kzg_commitments, } = self; @@ -473,6 +487,8 @@ impl BeaconBlockBodyEip4844> { execution_payload: BlindedPayloadEip4844 { execution_payload_header: From::from(execution_payload.clone()), }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes: bls_to_execution_changes.clone(), blob_kzg_commitments: blob_kzg_commitments.clone(), } } diff --git a/consensus/types/src/bls_to_execution_change.rs b/consensus/types/src/bls_to_execution_change.rs new file mode 100644 index 000000000..ca8e0ecf7 --- /dev/null +++ b/consensus/types/src/bls_to_execution_change.rs @@ -0,0 +1,30 @@ +use crate::test_utils::TestRandom; +use crate::*; +use bls::PublicKeyBytes; +use serde_derive::{Deserialize, Serialize}; +use ssz_derive::{Decode, Encode}; +use test_random_derive::TestRandom; +use tree_hash_derive::TreeHash; + +/// A deposit to potentially become a beacon chain validator. +/// +/// Spec v0.12.1 +#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] +pub struct BlsToExecutionChange { + #[serde(with = "eth2_serde_utils::quoted_u64")] + pub validator_index: u64, + pub from_bls_pubkey: PublicKeyBytes, + pub to_execution_address: Address, +} + +impl SignedRoot for BlsToExecutionChange {} + +#[cfg(test)] +mod tests { + use super::*; + + ssz_and_tree_hash_tests!(BlsToExecutionChange); +} diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 70cc4c112..2a8398f83 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -341,6 +341,8 @@ impl SignedBeaconBlockCapella> { voluntary_exits, sync_aggregate, execution_payload: BlindedPayloadCapella { .. }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, }, }, signature, @@ -362,6 +364,8 @@ impl SignedBeaconBlockCapella> { voluntary_exits, sync_aggregate, execution_payload: FullPayloadCapella { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, }, }, signature, @@ -393,6 +397,8 @@ impl SignedBeaconBlockEip4844> { voluntary_exits, sync_aggregate, execution_payload: BlindedPayloadEip4844 { .. }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, blob_kzg_commitments, }, }, @@ -415,6 +421,8 @@ impl SignedBeaconBlockEip4844> { voluntary_exits, sync_aggregate, execution_payload: FullPayloadEip4844 { execution_payload }, + #[cfg(feature = "withdrawals")] + bls_to_execution_changes, blob_kzg_commitments, }, }, diff --git a/consensus/types/src/signed_bls_to_execution_change.rs b/consensus/types/src/signed_bls_to_execution_change.rs new file mode 100644 index 000000000..fc636bb82 --- /dev/null +++ b/consensus/types/src/signed_bls_to_execution_change.rs @@ -0,0 +1,26 @@ +use crate::test_utils::TestRandom; +use crate::*; +use bls::Signature; +use serde_derive::{Deserialize, Serialize}; +use ssz_derive::{Decode, Encode}; +use test_random_derive::TestRandom; +use tree_hash_derive::TreeHash; + +/// A deposit to potentially become a beacon chain validator. +/// +/// Spec v0.12.1 +#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] +pub struct SignedBlsToExecutionChange { + pub message: BlsToExecutionChange, + pub signature: Signature, +} + +#[cfg(test)] +mod tests { + use super::*; + + ssz_and_tree_hash_tests!(SignedBlsToExecutionChange); +}