Isolate withdrawals-processing Feature (#3854)

Makefile

@@ -89,12 +89,12 @@ build-release-tarballs:
 # Runs the full workspace tests in **release**, without downloading any additional
 # test vectors.
 test-release:
-	cargo test --workspace --release --exclude ef_tests --exclude beacon_chain --exclude slasher
+	cargo test --workspace --features withdrawals-processing --release --exclude ef_tests --exclude beacon_chain --exclude slasher
 
 # Runs the full workspace tests in **debug**, without downloading any additional test
 # vectors.
 test-debug:
-	cargo test --workspace --exclude ef_tests --exclude beacon_chain
+	cargo test --workspace --features withdrawals-processing --exclude ef_tests --exclude beacon_chain
 
 # Runs cargo-fmt (linter).
 cargo-fmt:

beacon_node/Cargo.toml

@@ -17,7 +17,6 @@ withdrawals-processing = [
     "beacon_chain/withdrawals-processing",
     "store/withdrawals-processing",
     "execution_layer/withdrawals-processing",
-    "http_api/withdrawals-processing",
 ]
 
 [dependencies]

beacon_node/beacon_chain/Cargo.toml

@@ -12,9 +12,7 @@ participation_metrics = []  # Exposes validator participation metrics to Prometh
 fork_from_env = [] # Initialise the harness chain spec from the FORK_NAME env variable
 withdrawals-processing = [
     "state_processing/withdrawals-processing",
-    "store/withdrawals-processing",
     "execution_layer/withdrawals-processing",
-    "operation_pool/withdrawals-processing"
 ]
 
 [dev-dependencies]

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -362,7 +362,6 @@ pub struct BeaconChain<T: BeaconChainTypes> {
     pub(crate) observed_attester_slashings:
         Mutex<ObservedOperations<AttesterSlashing<T::EthSpec>, T::EthSpec>>,
     /// Maintains a record of which validators we've seen BLS to execution changes for.
-    #[cfg(feature = "withdrawals-processing")]
     pub(crate) observed_bls_to_execution_changes:
         Mutex<ObservedOperations<SignedBlsToExecutionChange, T::EthSpec>>,
     /// The most recently validated light client finality update received on gossip.
@@ -2232,29 +2231,18 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         bls_to_execution_change: SignedBlsToExecutionChange,
     ) -> Result<ObservationOutcome<SignedBlsToExecutionChange, T::EthSpec>, Error> {
-        #[cfg(feature = "withdrawals-processing")]
+        let current_fork = self.spec.fork_name_at_slot::<T::EthSpec>(self.slot()?);
-        {
+        if let ForkName::Base | ForkName::Altair | ForkName::Merge = current_fork {
-            let current_fork = self.spec.fork_name_at_slot::<T::EthSpec>(self.slot()?);
+            // Disallow BLS to execution changes prior to the Capella fork.
-            if let ForkName::Base | ForkName::Altair | ForkName::Merge = current_fork {
+            return Err(Error::BlsToExecutionChangeBadFork(current_fork));
-                // Disallow BLS to execution changes prior to the Capella fork.
-                return Err(Error::BlsToExecutionChangeBadFork(current_fork));
-            }
-
-            let wall_clock_state = self.wall_clock_state()?;
-
-            Ok(self
-                .observed_bls_to_execution_changes
-                .lock()
-                .verify_and_observe(bls_to_execution_change, &wall_clock_state, &self.spec)?)
         }
 
-        // TODO: remove this whole block once withdrawals-processing is removed
+        let wall_clock_state = self.wall_clock_state()?;
-        #[cfg(not(feature = "withdrawals-processing"))]
+
-        {
+        Ok(self
-            #[allow(clippy::drop_non_drop)]
+            .observed_bls_to_execution_changes
-            drop(bls_to_execution_change);
+            .lock()
-            Ok(ObservationOutcome::AlreadyKnown)
+            .verify_and_observe(bls_to_execution_change, &wall_clock_state, &self.spec)?)
-        }
     }
 
     /// Import a BLS to execution change to the op pool.
@@ -2263,12 +2251,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         bls_to_execution_change: SigVerifiedOp<SignedBlsToExecutionChange, T::EthSpec>,
     ) {
         if self.eth1_chain.is_some() {
-            #[cfg(feature = "withdrawals-processing")]
             self.op_pool
                 .insert_bls_to_execution_change(bls_to_execution_change);
-
-            #[cfg(not(feature = "withdrawals-processing"))]
-            drop(bls_to_execution_change);
         }
     }
 

beacon_node/beacon_chain/src/builder.rs

@@ -798,7 +798,6 @@ where
             observed_voluntary_exits: <_>::default(),
             observed_proposer_slashings: <_>::default(),
             observed_attester_slashings: <_>::default(),
-            #[cfg(feature = "withdrawals-processing")]
             observed_bls_to_execution_changes: <_>::default(),
             latest_seen_finality_update: <_>::default(),
             latest_seen_optimistic_update: <_>::default(),

beacon_node/beacon_chain/src/observed_operations.rs

@@ -6,12 +6,9 @@ use std::collections::HashSet;
 use std::marker::PhantomData;
 use types::{
     AttesterSlashing, BeaconState, ChainSpec, EthSpec, ForkName, ProposerSlashing,
-    SignedVoluntaryExit, Slot,
+    SignedBlsToExecutionChange, SignedVoluntaryExit, Slot,
 };
 
-#[cfg(feature = "withdrawals-processing")]
-use types::SignedBlsToExecutionChange;
-
 /// Number of validator indices to store on the stack in `observed_validators`.
 pub const SMALL_VEC_SIZE: usize = 8;
 
@@ -83,7 +80,6 @@ impl<E: EthSpec> ObservableOperation<E> for AttesterSlashing<E> {
     }
 }
 
-#[cfg(feature = "withdrawals-processing")]
 impl<E: EthSpec> ObservableOperation<E> for SignedBlsToExecutionChange {
     fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> {
         smallvec![self.message.validator_index]

beacon_node/execution_layer/Cargo.toml

@@ -5,7 +5,7 @@ edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 [features]
-withdrawals-processing = ["state_processing/withdrawals-processing", "eth2/withdrawals-processing"]
+withdrawals-processing = ["state_processing/withdrawals-processing"]
 
 [dependencies]
 types = { path = "../../consensus/types"}

beacon_node/http_api/Cargo.toml

@@ -5,9 +5,6 @@ authors = ["Paul Hauner <paul@paulhauner.com>"]
 edition = "2021"
 autotests = false # using a single test binary compiles faster
 
-[features]
-withdrawals-processing = []
-
 [dependencies]
 warp = { version = "0.3.2", features = ["tls"] }
 serde = { version = "1.0.116", features = ["derive"] }

beacon_node/http_api/src/lib.rs

@@ -1687,16 +1687,12 @@ pub fn serve<T: BeaconChainTypes>(
 
                         match chain.verify_bls_to_execution_change_for_gossip(address_change) {
                             Ok(ObservationOutcome::New(verified_address_change)) => {
-                                #[cfg(feature = "withdrawals-processing")]
+                                publish_pubsub_message(
-                                {
+                                    &network_tx,
-                                    publish_pubsub_message(
+                                    PubsubMessage::BlsToExecutionChange(Box::new(
-                                        &network_tx,
+                                        verified_address_change.as_inner().clone(),
-                                        PubsubMessage::BlsToExecutionChange(Box::new(
+                                    )),
-                                            verified_address_change.as_inner().clone(),
+                                )?;
-                                        )),
-                                    )?;
-                                }
-
                                 chain.import_bls_to_execution_change(verified_address_change);
                             }
                             Ok(ObservationOutcome::AlreadyKnown) => {

beacon_node/operation_pool/Cargo.toml

@@ -4,9 +4,6 @@ version = "0.2.0"
 authors = ["Michael Sproul <michael@sigmaprime.io>"]
 edition = "2021"
 
-[features]
-withdrawals-processing = []
-
 [dependencies]
 derivative = "2.1.1"
 itertools = "0.10.0"

beacon_node/operation_pool/src/lib.rs

@@ -51,7 +51,6 @@ pub struct OperationPool<T: EthSpec + Default> {
     /// Map from exiting validator to their exit data.
     voluntary_exits: RwLock<HashMap<u64, SigVerifiedOp<SignedVoluntaryExit, T>>>,
     /// Map from credential changing validator to their execution change data.
-    #[cfg(feature = "withdrawals-processing")]
     bls_to_execution_changes: RwLock<HashMap<u64, SigVerifiedOp<SignedBlsToExecutionChange, T>>>,
     /// Reward cache for accelerating attestation packing.
     reward_cache: RwLock<RewardCache>,
@@ -518,17 +517,10 @@ impl<T: EthSpec> OperationPool<T> {
         &self,
         verified_change: SigVerifiedOp<SignedBlsToExecutionChange, T>,
     ) {
-        #[cfg(feature = "withdrawals-processing")]
+        self.bls_to_execution_changes.write().insert(
-        {
+            verified_change.as_inner().message.validator_index,
-            self.bls_to_execution_changes.write().insert(
+            verified_change,
-                verified_change.as_inner().message.validator_index,
+        );
-                verified_change,
-            );
-        }
-        #[cfg(not(feature = "withdrawals-processing"))]
-        {
-            drop(verified_change);
-        }
     }
 
     /// Get a list of execution changes for inclusion in a block.
@@ -539,32 +531,19 @@ impl<T: EthSpec> OperationPool<T> {
         state: &BeaconState<T>,
         spec: &ChainSpec,
     ) -> Vec<SignedBlsToExecutionChange> {
-        #[cfg(feature = "withdrawals-processing")]
+        filter_limit_operations(
-        {
+            self.bls_to_execution_changes.read().values(),
-            filter_limit_operations(
+            |address_change| {
-                self.bls_to_execution_changes.read().values(),
+                address_change.signature_is_still_valid(&state.fork())
-                |address_change| {
+                    && state
-                    address_change.signature_is_still_valid(&state.fork())
+                        .get_validator(address_change.as_inner().message.validator_index as usize)
-                        && state
+                        .map_or(false, |validator| {
-                            .get_validator(
+                            !validator.has_eth1_withdrawal_credential(spec)
-                                address_change.as_inner().message.validator_index as usize,
+                        })
-                            )
+            },
-                            .map_or(false, |validator| {
+            |address_change| address_change.as_inner().clone(),
-                                !validator.has_eth1_withdrawal_credential(spec)
+            T::MaxBlsToExecutionChanges::to_usize(),
-                            })
+        )
-                },
-                |address_change| address_change.as_inner().clone(),
-                T::MaxBlsToExecutionChanges::to_usize(),
-            )
-        }
-
-        // TODO: remove this whole block once withdrwals-processing is removed
-        #[cfg(not(feature = "withdrawals-processing"))]
-        {
-            #[allow(clippy::drop_copy)]
-            drop((state, spec));
-            vec![]
-        }
     }
 
     /// Prune BLS to execution changes that have been applied to the state more than 1 block ago.
@@ -579,32 +558,22 @@ impl<T: EthSpec> OperationPool<T> {
         head_state: &BeaconState<T>,
         spec: &ChainSpec,
     ) {
-        #[cfg(feature = "withdrawals-processing")]
+        prune_validator_hash_map(
-        {
+            &mut self.bls_to_execution_changes.write(),
-            prune_validator_hash_map(
+            |validator_index, validator| {
-                &mut self.bls_to_execution_changes.write(),
+                validator.has_eth1_withdrawal_credential(spec)
-                |validator_index, validator| {
+                    && head_block
-                    validator.has_eth1_withdrawal_credential(spec)
+                        .message()
-                        && head_block
+                        .body()
-                            .message()
+                        .bls_to_execution_changes()
-                            .body()
+                        .map_or(true, |recent_changes| {
-                            .bls_to_execution_changes()
+                            !recent_changes
-                            .map_or(true, |recent_changes| {
+                                .iter()
-                                !recent_changes
+                                .any(|c| c.message.validator_index == validator_index)
-                                    .iter()
+                        })
-                                    .any(|c| c.message.validator_index == validator_index)
+            },
-                            })
+            head_state,
-                },
+        );
-                head_state,
-            );
-        }
-
-        // TODO: remove this whole block once withdrwals-processing is removed
-        #[cfg(not(feature = "withdrawals-processing"))]
-        {
-            #[allow(clippy::drop_copy)]
-            drop((head_block, head_state, spec));
-        }
     }
 
     /// Prune all types of transactions given the latest head state and head fork.
@@ -691,17 +660,11 @@ impl<T: EthSpec> OperationPool<T> {
     ///
     /// This method may return objects that are invalid for block inclusion.
     pub fn get_all_bls_to_execution_changes(&self) -> Vec<SignedBlsToExecutionChange> {
-        #[cfg(feature = "withdrawals-processing")]
+        self.bls_to_execution_changes
-        {
+            .read()
-            self.bls_to_execution_changes
+            .iter()
-                .read()
+            .map(|(_, address_change)| address_change.as_inner().clone())
-                .iter()
+            .collect()
-                .map(|(_, address_change)| address_change.as_inner().clone())
-                .collect()
-        }
-
-        #[cfg(not(feature = "withdrawals-processing"))]
-        vec![]
     }
 }
 

beacon_node/operation_pool/src/persistence.rs

@@ -143,7 +143,6 @@ impl<T: EthSpec> PersistedOperationPool<T> {
             proposer_slashings,
             voluntary_exits,
             // FIXME(capella): implement schema migration for address changes in op pool
-            #[cfg(feature = "withdrawals-processing")]
             bls_to_execution_changes: Default::default(),
             reward_cache: Default::default(),
             _phantom: Default::default(),

common/eth2/Cargo.toml

@@ -35,4 +35,3 @@ procinfo = { version = "0.4.2", optional = true }
 [features]
 default = ["lighthouse"]
 lighthouse = ["proto_array", "psutil", "procinfo", "store", "slashing_protection"]
-withdrawals-processing = ["store/withdrawals-processing"]

consensus/state_processing/src/per_block_processing.rs

@@ -19,7 +19,6 @@ pub use process_operations::process_operations;
 pub use verify_attestation::{
     verify_attestation_for_block_inclusion, verify_attestation_for_state,
 };
-#[cfg(feature = "withdrawals-processing")]
 pub use verify_bls_to_execution_change::verify_bls_to_execution_change;
 pub use verify_deposit::{
     get_existing_validator_index, verify_deposit_merkle_proof, verify_deposit_signature,
@@ -36,13 +35,11 @@ pub mod signature_sets;
 pub mod tests;
 mod verify_attestation;
 mod verify_attester_slashing;
-#[cfg(feature = "withdrawals-processing")]
 mod verify_bls_to_execution_change;
 mod verify_deposit;
 mod verify_exit;
 mod verify_proposer_slashing;
 
-#[cfg(feature = "withdrawals-processing")]
 use crate::common::decrease_balance;
 
 #[cfg(feature = "arbitrary-fuzz")]
@@ -165,7 +162,6 @@ pub fn per_block_processing<T: EthSpec, Payload: AbstractExecPayload<T>>(
     // previous block.
     if is_execution_enabled(state, block.body()) {
         let payload = block.body().execution_payload()?;
-        #[cfg(feature = "withdrawals-processing")]
         process_withdrawals::<T, Payload>(state, payload, spec)?;
         process_execution_payload::<T, Payload>(state, payload, spec)?;
     }
@@ -524,12 +520,14 @@ pub fn get_expected_withdrawals<T: EthSpec>(
 }
 
 /// Apply withdrawals to the state.
-#[cfg(feature = "withdrawals-processing")]
 pub fn process_withdrawals<'payload, T: EthSpec, Payload: AbstractExecPayload<T>>(
     state: &mut BeaconState<T>,
     payload: Payload::Ref<'payload>,
     spec: &ChainSpec,
 ) -> Result<(), BlockProcessingError> {
+    if cfg!(not(feature = "withdrawals-processing")) {
+        return Ok(());
+    }
     match state {
         BeaconState::Merge(_) => Ok(()),
         BeaconState::Capella(_) | BeaconState::Eip4844(_) => {

consensus/state_processing/src/per_block_processing/process_operations.rs

@@ -34,7 +34,6 @@ pub fn process_operations<'a, T: EthSpec, Payload: AbstractExecPayload<T>>(
     process_deposits(state, block_body.deposits(), spec)?;
     process_exits(state, block_body.voluntary_exits(), verify_signatures, spec)?;
 
-    #[cfg(feature = "withdrawals-processing")]
     if let Ok(bls_to_execution_changes) = block_body.bls_to_execution_changes() {
         process_bls_to_execution_changes(state, bls_to_execution_changes, verify_signatures, spec)?;
     }
@@ -295,13 +294,15 @@ pub fn process_exits<T: EthSpec>(
 ///
 /// Returns `Ok(())` if the validation and state updates completed successfully. Otherwise returns
 /// an `Err` describing the invalid object or cause of failure.
-#[cfg(feature = "withdrawals-processing")]
 pub fn process_bls_to_execution_changes<T: EthSpec>(
     state: &mut BeaconState<T>,
     bls_to_execution_changes: &[SignedBlsToExecutionChange],
     verify_signatures: VerifySignatures,
     spec: &ChainSpec,
 ) -> Result<(), BlockProcessingError> {
+    if cfg!(not(feature = "withdrawals-processing")) {
+        return Ok(());
+    }
     for (i, signed_address_change) in bls_to_execution_changes.iter().enumerate() {
         verify_bls_to_execution_change(state, signed_address_change, verify_signatures, spec)
             .map_err(|e| e.into_with_index(i))?;

consensus/state_processing/src/verify_operation.rs

@@ -1,8 +1,10 @@
 use crate::per_block_processing::{
     errors::{
-        AttesterSlashingValidationError, ExitValidationError, ProposerSlashingValidationError,
+        AttesterSlashingValidationError, BlsExecutionChangeValidationError, ExitValidationError,
+        ProposerSlashingValidationError,
     },
-    verify_attester_slashing, verify_exit, verify_proposer_slashing,
+    verify_attester_slashing, verify_bls_to_execution_change, verify_exit,
+    verify_proposer_slashing,
 };
 use crate::VerifySignatures;
 use derivative::Derivative;
@@ -12,15 +14,7 @@ use ssz_derive::{Decode, Encode};
 use std::marker::PhantomData;
 use types::{
     AttesterSlashing, BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing,
-    SignedVoluntaryExit,
+    SignedBlsToExecutionChange, SignedVoluntaryExit,
-};
-
-#[cfg(feature = "withdrawals-processing")]
-use {
-    crate::per_block_processing::{
-        errors::BlsExecutionChangeValidationError, verify_bls_to_execution_change,
-    },
-    types::SignedBlsToExecutionChange,
 };
 
 const MAX_FORKS_VERIFIED_AGAINST: usize = 2;
@@ -202,7 +196,6 @@ impl<E: EthSpec> VerifyOperation<E> for ProposerSlashing {
     }
 }
 
-#[cfg(feature = "withdrawals-processing")]
 impl<E: EthSpec> VerifyOperation<E> for SignedBlsToExecutionChange {
     type Error = BlsExecutionChangeValidationError;
 

testing/ef_tests/src/cases/operations.rs

@@ -4,30 +4,24 @@ use crate::case_result::compare_beacon_state_results_without_caches;
 use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file};
 use crate::testing_spec;
 use serde_derive::Deserialize;
-#[cfg(feature = "withdrawals-processing")]
-use state_processing::per_block_processing::process_operations::{
-    process_bls_to_execution_changes, process_bls_to_execution_changes,
-};
 use state_processing::{
     per_block_processing::{
         errors::BlockProcessingError,
         process_block_header, process_execution_payload,
         process_operations::{
-            altair, base, process_attester_slashings, process_deposits, process_exits,
+            altair, base, process_attester_slashings, process_bls_to_execution_changes,
-            process_proposer_slashings,
+            process_deposits, process_exits, process_proposer_slashings,
         },
-        process_sync_aggregate, VerifyBlockRoot, VerifySignatures,
+        process_sync_aggregate, process_withdrawals, VerifyBlockRoot, VerifySignatures,
     },
     ConsensusContext,
 };
 use std::fmt::Debug;
 use std::path::Path;
-#[cfg(feature = "withdrawals-processing")]
-use types::SignedBlsToExecutionChange;
 use types::{
     Attestation, AttesterSlashing, BeaconBlock, BeaconState, BlindedPayload, ChainSpec, Deposit,
-    EthSpec, ExecutionPayload, ForkName, FullPayload, ProposerSlashing, SignedVoluntaryExit,
+    EthSpec, ExecutionPayload, ForkName, FullPayload, ProposerSlashing, SignedBlsToExecutionChange,
-    SyncAggregate,
+    SignedVoluntaryExit, SyncAggregate,
 };
 
 #[derive(Debug, Clone, Default, Deserialize)]
@@ -42,7 +36,6 @@ struct ExecutionMetadata {
 }
 
 /// Newtype for testing withdrawals.
-#[cfg(feature = "withdrawals-processing")]
 #[derive(Debug, Clone, Deserialize)]
 pub struct WithdrawalsPayload<T: EthSpec> {
     payload: FullPayload<T>,
@@ -341,7 +334,6 @@ impl<E: EthSpec> Operation<E> for BlindedPayload<E> {
     }
 }
 
-#[cfg(feature = "withdrawals-processing")]
 impl<E: EthSpec> Operation<E> for WithdrawalsPayload<E> {
     fn handler_name() -> String {
         "withdrawals".into()
@@ -374,7 +366,6 @@ impl<E: EthSpec> Operation<E> for WithdrawalsPayload<E> {
     }
 }
 
-#[cfg(feature = "withdrawals-processing")]
 impl<E: EthSpec> Operation<E> for SignedBlsToExecutionChange {
     fn handler_name() -> String {
         "bls_to_execution_change".into()

testing/ef_tests/src/lib.rs

@@ -1,5 +1,4 @@
 pub use case_result::CaseResult;
-#[cfg(feature = "withdrawals-processing")]
 pub use cases::WithdrawalsPayload;
 pub use cases::{
     Case, EffectiveBalanceUpdates, Eth1DataReset, HistoricalRootsUpdate, InactivityUpdates,

testing/ef_tests/tests/tests.rs

@@ -82,14 +82,12 @@ fn operations_execution_payload_blinded() {
     OperationsHandler::<MainnetEthSpec, BlindedPayload<_>>::default().run();
 }
 
-#[cfg(feature = "withdrawals-processing")]
 #[test]
 fn operations_withdrawals() {
     OperationsHandler::<MinimalEthSpec, WithdrawalsPayload<_>>::default().run();
     OperationsHandler::<MainnetEthSpec, WithdrawalsPayload<_>>::default().run();
 }
 
-#[cfg(feature = "withdrawals-processing")]
 #[test]
 fn operations_bls_to_execution_change() {
     OperationsHandler::<MinimalEthSpec, SignedBlsToExecutionChange>::default().run();