Isolate withdrawals-processing Feature

ethDreamer · ethDreamer · commit 17204e0764d9 · 2023-01-06T13:27:17.000-06:00
diff --git a/Makefile b/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:
diff --git a/beacon_node/Cargo.toml b/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]
diff --git a/beacon_node/beacon_chain/Cargo.toml b/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]
diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/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 {
-                // 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)?)
+        let current_fork = self.spec.fork_name_at_slot::<T::EthSpec>(self.slot()?);
+        if let ForkName::Base | ForkName::Altair | ForkName::Merge = current_fork {
+            // Disallow BLS to execution changes prior to the Capella fork.
+            return Err(Error::BlsToExecutionChangeBadFork(current_fork));
         }
 
-        // TODO: remove this whole block once withdrawals-processing is removed
-        #[cfg(not(feature = "withdrawals-processing"))]
-        {
-            #[allow(clippy::drop_non_drop)]
-            drop(bls_to_execution_change);
-            Ok(ObservationOutcome::AlreadyKnown)
-        }
+        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)?)
     }
 
     /// 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);
         }
     }
 
diff --git a/beacon_node/beacon_chain/src/builder.rs b/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(),
diff --git a/beacon_node/beacon_chain/src/observed_operations.rs b/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]
diff --git a/beacon_node/execution_layer/Cargo.toml b/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"}
diff --git a/beacon_node/http_api/Cargo.toml b/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"] }
diff --git a/beacon_node/http_api/src/lib.rs b/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,
-                                        PubsubMessage::BlsToExecutionChange(Box::new(
-                                            verified_address_change.as_inner().clone(),
-                                        )),
-                                    )?;
-                                }
-
+                                publish_pubsub_message(
+                                    &network_tx,
+                                    PubsubMessage::BlsToExecutionChange(Box::new(
+                                        verified_address_change.as_inner().clone(),
+                                    )),
+                                )?;
                                 chain.import_bls_to_execution_change(verified_address_change);
                             }
                             Ok(ObservationOutcome::AlreadyKnown) => {
diff --git a/beacon_node/operation_pool/Cargo.toml b/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"
diff --git a/beacon_node/operation_pool/src/lib.rs b/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,
-                verified_change,
-            );
-        }
-        #[cfg(not(feature = "withdrawals-processing"))]
-        {
-            drop(verified_change);
-        }
+        self.bls_to_execution_changes.write().insert(
+            verified_change.as_inner().message.validator_index,
+            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(),
-                |address_change| {
-                    address_change.signature_is_still_valid(&state.fork())
-                        && state
-                            .get_validator(
-                                address_change.as_inner().message.validator_index as usize,
-                            )
-                            .map_or(false, |validator| {
-                                !validator.has_eth1_withdrawal_credential(spec)
-                            })
-                },
-                |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![]
-        }
+        filter_limit_operations(
+            self.bls_to_execution_changes.read().values(),
+            |address_change| {
+                address_change.signature_is_still_valid(&state.fork())
+                    && state
+                        .get_validator(address_change.as_inner().message.validator_index as usize)
+                        .map_or(false, |validator| {
+                            !validator.has_eth1_withdrawal_credential(spec)
+                        })
+            },
+            |address_change| address_change.as_inner().clone(),
+            T::MaxBlsToExecutionChanges::to_usize(),
+        )
     }
 
     /// 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(),
-                |validator_index, validator| {
-                    validator.has_eth1_withdrawal_credential(spec)
-                        && head_block
-                            .message()
-                            .body()
-                            .bls_to_execution_changes()
-                            .map_or(true, |recent_changes| {
-                                !recent_changes
-                                    .iter()
-                                    .any(|c| c.message.validator_index == validator_index)
-                            })
-                },
-                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_validator_hash_map(
+            &mut self.bls_to_execution_changes.write(),
+            |validator_index, validator| {
+                validator.has_eth1_withdrawal_credential(spec)
+                    && head_block
+                        .message()
+                        .body()
+                        .bls_to_execution_changes()
+                        .map_or(true, |recent_changes| {
+                            !recent_changes
+                                .iter()
+                                .any(|c| c.message.validator_index == validator_index)
+                        })
+            },
+            head_state,
+        );
     }
 
     /// 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()
-                .iter()
-                .map(|(_, address_change)| address_change.as_inner().clone())
-                .collect()
-        }
-
-        #[cfg(not(feature = "withdrawals-processing"))]
-        vec![]
+        self.bls_to_execution_changes
+            .read()
+            .iter()
+            .map(|(_, address_change)| address_change.as_inner().clone())
+            .collect()
     }
 }
 
diff --git a/beacon_node/operation_pool/src/persistence.rs b/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(),
diff --git a/common/eth2/Cargo.toml b/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"]
diff --git a/consensus/state_processing/src/per_block_processing.rs b/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(_) => {
diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs
diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs
diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs
diff --git a/testing/ef_tests/src/lib.rs b/testing/ef_tests/src/lib.rs
diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs

Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,6 @@ withdrawals-processing = [`
`17`	`17`	`"beacon_chain/withdrawals-processing",`
`18`	`18`	`"store/withdrawals-processing",`
`19`	`19`	`"execution_layer/withdrawals-processing",`
`20`		`- "http_api/withdrawals-processing",`
`21`	`20`	`]`
`22`	`21`
`23`	`22`	`[dependencies]`
Original file line number	Diff line number	Diff line change
`@@ -12,9 +12,7 @@ participation_metrics = [] # Exposes validator participation metrics to Prometh`
`12`	`12`	`fork_from_env = [] # Initialise the harness chain spec from the FORK_NAME env variable`
`13`	`13`	`withdrawals-processing = [`
`14`	`14`	`"state_processing/withdrawals-processing",`
`15`		`- "store/withdrawals-processing",`
`16`	`15`	`"execution_layer/withdrawals-processing",`
`17`		`- "operation_pool/withdrawals-processing"`
`18`	`16`	`]`
`19`	`17`
`20`	`18`	`[dev-dependencies]`