From b68d08c84724f9df6be7ac9a1cf6a23db8f07b65 Mon Sep 17 00:00:00 2001 From: Mac L Date: Wed, 1 May 2024 18:12:38 +1000 Subject: [PATCH] Start replacing boilerplate --- beacon_node/beacon_chain/src/beacon_chain.rs | 66 +++++++++++-------- .../execution_layer/src/engine_api/http.rs | 32 +++++---- .../src/test_utils/mock_builder.rs | 15 +++-- beacon_node/http_api/src/lib.rs | 12 ++-- consensus/types/src/chain_spec.rs | 5 +- consensus/types/src/feature_name.rs | 2 +- consensus/types/src/fork_name.rs | 63 +++++++++++++++++- consensus/types/src/voluntary_exit.rs | 15 ++--- validator_client/src/validator_store.rs | 29 ++++---- 9 files changed, 155 insertions(+), 84 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8eeb75fd7d..7e73d9150b 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -733,6 +733,20 @@ impl BeaconChain { .map(|slot| slot.epoch(T::EthSpec::slots_per_epoch())) } + /// Returns the latest fork for the current slot. + pub fn current_fork(&self) -> Result { + Ok(self.spec.fork_name_at_slot::(self.slot()?)) + } + + /// Checks if a feature is enabled on the current fork. + pub fn is_feature_enabled(&self, feature: FeatureName) -> bool { + if let Ok(current_fork) = self.current_fork() { + current_fork.is_feature_enabled(feature) + } else { + false + } + } + /// Iterates across all `(block_root, slot)` pairs from `start_slot` /// to the head of the chain (inclusive). /// @@ -2523,7 +2537,7 @@ impl BeaconChain { bls_to_execution_change: SignedBlsToExecutionChange, ) -> Result, Error> { // Ignore BLS to execution changes on gossip prior to Capella. - if !self.current_slot_is_post_capella()? { + if !self.is_feature_enabled(FeatureName::Capella) { return Err(Error::BlsToExecutionPriorToCapella); } self.verify_bls_to_execution_change_for_http_api(bls_to_execution_change) @@ -2536,16 +2550,6 @@ impl BeaconChain { }) } - /// Check if the current slot is greater than or equal to the Capella fork epoch. - pub fn current_slot_is_post_capella(&self) -> Result { - let current_fork = self.spec.fork_name_at_slot::(self.slot()?); - if let ForkName::Base | ForkName::Altair | ForkName::Bellatrix = current_fork { - Ok(false) - } else { - Ok(true) - } - } - /// Import a BLS to execution change to the op pool. /// /// Return `true` if the change was added to the pool. @@ -5552,27 +5556,31 @@ impl BeaconChain { payload_attributes } else { let prepare_slot_fork = self.spec.fork_name_at_slot::(prepare_slot); - let withdrawals = match prepare_slot_fork { - ForkName::Base | ForkName::Altair | ForkName::Bellatrix => None, - ForkName::Capella | ForkName::Deneb | ForkName::Electra => { - let chain = self.clone(); - self.spawn_blocking_handle( - move || { - chain.get_expected_withdrawals(&forkchoice_update_params, prepare_slot) - }, - "prepare_beacon_proposer_withdrawals", - ) - .await? - .map(Some)? - } + let withdrawals = if prepare_slot_fork.is_feature_enabled(FeatureName::Capella) { + let chain = self.clone(); + self.spawn_blocking_handle( + move || chain.get_expected_withdrawals(&forkchoice_update_params, prepare_slot), + "prepare_beacon_proposer_withdrawals", + ) + .await? + .map(Some)? + } else { + None }; - let parent_beacon_block_root = match prepare_slot_fork { - ForkName::Base | ForkName::Altair | ForkName::Bellatrix | ForkName::Capella => None, - ForkName::Deneb | ForkName::Electra => { + let parent_beacon_block_root = + if prepare_slot_fork.is_feature_enabled(FeatureName::Deneb) { Some(pre_payload_attributes.parent_beacon_block_root) - } - }; + } else { + None + }; + + //let parent_beacon_block_root = match prepare_slot_fork { + // ForkName::Base | ForkName::Altair | ForkName::Bellatrix | ForkName::Capella => None, + // ForkName::Deneb | ForkName::Electra => { + // Some(pre_payload_attributes.parent_beacon_block_root) + // } + //}; let payload_attributes = PayloadAttributes::new( self.slot_clock diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 93705a1692..76d98b2d63 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -1217,27 +1217,25 @@ impl HttpJsonRpc { payload_id: PayloadId, ) -> Result, Error> { let engine_capabilities = self.get_engine_capabilities(None).await?; - match fork_name { - ForkName::Bellatrix | ForkName::Capella => { - if engine_capabilities.get_payload_v2 { - self.get_payload_v2(fork_name, payload_id).await - } else if engine_capabilities.new_payload_v1 { - self.get_payload_v1(payload_id).await - } else { - Err(Error::RequiredMethodUnsupported("engine_getPayload")) - } + if fork_name.is_feature_enabled(FeatureName::Deneb) { + if engine_capabilities.get_payload_v3 { + self.get_payload_v3(fork_name, payload_id).await + } else { + Err(Error::RequiredMethodUnsupported("engine_getPayloadV3")) } - ForkName::Deneb | ForkName::Electra => { - if engine_capabilities.get_payload_v3 { - self.get_payload_v3(fork_name, payload_id).await - } else { - Err(Error::RequiredMethodUnsupported("engine_getPayloadV3")) - } + } else if fork_name.is_feature_enabled(FeatureName::Bellatrix) { + if engine_capabilities.get_payload_v2 { + self.get_payload_v2(fork_name, payload_id).await + } else if engine_capabilities.new_payload_v1 { + self.get_payload_v1(payload_id).await + } else { + Err(Error::RequiredMethodUnsupported("engine_getPayload")) } - ForkName::Base | ForkName::Altair => Err(Error::UnsupportedForkVariant(format!( + } else { + Err(Error::UnsupportedForkVariant(format!( "called get_payload with {}", fork_name - ))), + ))) } } diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index c9ae1e60cd..a57563e24f 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -20,9 +20,9 @@ use types::builder_bid::{ }; use types::{ Address, BeaconState, ChainSpec, EthSpec, ExecPayload, ExecutionPayload, - ExecutionPayloadHeaderRefMut, ForkName, ForkVersionedResponse, Hash256, PublicKeyBytes, - Signature, SignedBlindedBeaconBlock, SignedRoot, SignedValidatorRegistrationData, Slot, - Uint256, + ExecutionPayloadHeaderRefMut, FeatureName, ForkName, ForkVersionedResponse, Hash256, + PublicKeyBytes, Signature, SignedBlindedBeaconBlock, SignedRoot, + SignedValidatorRegistrationData, Slot, Uint256, }; use types::{ExecutionBlockHash, SecretKey}; use warp::{Filter, Rejection}; @@ -479,16 +479,17 @@ pub fn serve( let prev_randao = head_state .get_randao_mix(head_state.current_epoch()) .map_err(|_| reject("couldn't get prev randao"))?; - let expected_withdrawals = match fork { - ForkName::Base | ForkName::Altair | ForkName::Bellatrix => None, - ForkName::Capella | ForkName::Deneb | ForkName::Electra => Some( + let expected_withdrawals = if fork.is_feature_enabled(FeatureName::Capella) { + Some( builder .beacon_client .get_expected_withdrawals(&StateId::Head) .await .unwrap() .data, - ), + ) + } else { + None }; let payload_attributes = match fork { diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 024e268e2a..9bf4cf3c0a 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -79,11 +79,11 @@ use tokio_stream::{ }; use types::{ fork_versioned_response::EmptyMetadata, Attestation, AttestationData, AttestationShufflingId, - AttesterSlashing, BeaconStateError, CommitteeCache, ConfigAndPreset, Epoch, EthSpec, ForkName, - ForkVersionedResponse, Hash256, ProposerPreparationData, ProposerSlashing, RelativeEpoch, - SignedAggregateAndProof, SignedBlindedBeaconBlock, SignedBlsToExecutionChange, - SignedContributionAndProof, SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, - SyncCommitteeMessage, SyncContributionData, + AttesterSlashing, BeaconStateError, CommitteeCache, ConfigAndPreset, Epoch, EthSpec, + FeatureName, ForkName, ForkVersionedResponse, Hash256, ProposerPreparationData, + ProposerSlashing, RelativeEpoch, SignedAggregateAndProof, SignedBlindedBeaconBlock, + SignedBlsToExecutionChange, SignedContributionAndProof, SignedValidatorRegistrationData, + SignedVoluntaryExit, Slot, SyncCommitteeMessage, SyncContributionData, }; use validator::pubkey_to_validator_index; use version::{ @@ -2046,7 +2046,7 @@ pub fn serve( .to_execution_address; // New to P2P *and* op pool, gossip immediately if post-Capella. - let received_pre_capella = if chain.current_slot_is_post_capella().unwrap_or(false) { + let received_pre_capella = if chain.is_feature_enabled(FeatureName::Capella) { ReceivedPreCapella::No } else { ReceivedPreCapella::Yes diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 8d40196ed6..4bc7f2d8d6 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -337,9 +337,10 @@ impl ChainSpec { } pub fn inactivity_penalty_quotient_for_fork(&self, fork_name: ForkName) -> u64 { - if fork_name >= ForkName::Bellatrix { + // TODO(superstruct_features) Is this a better pattern? + if fork_name.is_feature_enabled(FeatureName::Bellatrix) { self.inactivity_penalty_quotient_bellatrix - } else if fork_name >= ForkName::Altair { + } else if fork_name.is_feature_enabled(FeatureName::Altair) { self.inactivity_penalty_quotient_altair } else { self.inactivity_penalty_quotient diff --git a/consensus/types/src/feature_name.rs b/consensus/types/src/feature_name.rs index a11df6ee84..6ceabff3d6 100644 --- a/consensus/types/src/feature_name.rs +++ b/consensus/types/src/feature_name.rs @@ -3,7 +3,7 @@ // // For now, older Forks have a single "super-feature" which contains all features associated with // that Fork. It may be worth splitting these up at a later time. -#[derive(PartialEq)] +#[derive(PartialEq, Clone, Copy, Debug)] pub enum FeatureName { // Altair. Altair, diff --git a/consensus/types/src/fork_name.rs b/consensus/types/src/fork_name.rs index 4e67099dd4..4b073f1a49 100644 --- a/consensus/types/src/fork_name.rs +++ b/consensus/types/src/fork_name.rs @@ -1,5 +1,5 @@ use crate::fork_order::FORK_ORDER; -use crate::{ChainSpec, Epoch}; +use crate::{ChainSpec, Epoch, FeatureName}; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use std::cmp::{Ord, Ordering}; @@ -49,6 +49,20 @@ impl ForkName { FORK_ORDER[FORK_ORDER.len() - 1].0 } + pub fn list_all_enabled_features(self) -> Vec { + let mut res = vec![]; + for (fork, features) in FORK_ORDER { + if *fork <= self { + res.extend(features.iter()); + } + } + res + } + + pub fn is_feature_enabled(self, feature: FeatureName) -> bool { + self.list_all_enabled_features().contains(&feature) + } + /// Set the activation slots in the given `ChainSpec` so that the fork named by `self` /// is the only fork in effect from genesis. pub fn make_genesis_spec(&self, mut spec: ChainSpec) -> ChainSpec { @@ -296,4 +310,51 @@ mod test { assert!(prev_fork < fork); } } + + #[test] + fn check_fork_name_enabled_features() { + let base = ForkName::Base; + let altair = ForkName::Altair; + let bellatrix = ForkName::Bellatrix; + let capella = ForkName::Capella; + let deneb = ForkName::Deneb; + let electra = ForkName::Electra; + + assert_eq!(base.list_all_enabled_features(), vec![]); + assert_eq!( + altair.list_all_enabled_features(), + vec![FeatureName::Altair] + ); + assert_eq!( + bellatrix.list_all_enabled_features(), + vec![FeatureName::Altair, FeatureName::Bellatrix] + ); + assert_eq!( + capella.list_all_enabled_features(), + vec![ + FeatureName::Altair, + FeatureName::Bellatrix, + FeatureName::Capella + ] + ); + assert_eq!( + deneb.list_all_enabled_features(), + vec![ + FeatureName::Altair, + FeatureName::Bellatrix, + FeatureName::Capella, + FeatureName::Deneb + ] + ); + assert_eq!( + electra.list_all_enabled_features(), + vec![ + FeatureName::Altair, + FeatureName::Bellatrix, + FeatureName::Capella, + FeatureName::Deneb, + FeatureName::Electra + ] + ); + } } diff --git a/consensus/types/src/voluntary_exit.rs b/consensus/types/src/voluntary_exit.rs index 4c7c16757e..fe80b3602a 100644 --- a/consensus/types/src/voluntary_exit.rs +++ b/consensus/types/src/voluntary_exit.rs @@ -1,6 +1,6 @@ use crate::{ - test_utils::TestRandom, ChainSpec, Domain, Epoch, ForkName, Hash256, SecretKey, SignedRoot, - SignedVoluntaryExit, + test_utils::TestRandom, ChainSpec, Domain, Epoch, FeatureName, ForkName, Hash256, SecretKey, + SignedRoot, SignedVoluntaryExit, }; use serde::{Deserialize, Serialize}; @@ -41,12 +41,11 @@ impl VoluntaryExit { spec: &ChainSpec, ) -> SignedVoluntaryExit { let fork_name = spec.fork_name_at_epoch(self.epoch); - let fork_version = match fork_name { - ForkName::Base | ForkName::Altair | ForkName::Bellatrix | ForkName::Capella => { - spec.fork_version_for_name(fork_name) - } - // EIP-7044 - ForkName::Deneb | ForkName::Electra => spec.fork_version_for_name(ForkName::Capella), + // EIP-7044 + let fork_version = if fork_name.is_feature_enabled(FeatureName::Deneb) { + spec.fork_version_for_name(ForkName::Capella) + } else { + spec.fork_version_for_name(fork_name) }; let domain = spec.compute_domain(Domain::VoluntaryExit, fork_version, genesis_validators_root); diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index f3bdc2c0f6..168a172c1f 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -19,7 +19,7 @@ use task_executor::TaskExecutor; use types::{ attestation::Error as AttestationError, graffiti::GraffitiString, AbstractExecPayload, Address, AggregateAndProof, Attestation, BeaconBlock, BlindedPayload, ChainSpec, ContributionAndProof, - Domain, Epoch, EthSpec, Fork, ForkName, Graffiti, Hash256, PublicKeyBytes, SelectionProof, + Domain, Epoch, EthSpec, FeatureName, Fork, Graffiti, Hash256, PublicKeyBytes, SelectionProof, Signature, SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof, SignedRoot, SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, SyncAggregatorSelectionData, SyncCommitteeContribution, SyncCommitteeMessage, SyncSelectionProof, SyncSubnetId, @@ -359,17 +359,13 @@ impl ValidatorStore { fn signing_context(&self, domain: Domain, signing_epoch: Epoch) -> SigningContext { if domain == Domain::VoluntaryExit { - match self.spec.fork_name_at_epoch(signing_epoch) { - ForkName::Base | ForkName::Altair | ForkName::Bellatrix | ForkName::Capella => { - SigningContext { - domain, - epoch: signing_epoch, - fork: self.fork(signing_epoch), - genesis_validators_root: self.genesis_validators_root, - } - } - // EIP-7044 - ForkName::Deneb | ForkName::Electra => SigningContext { + // EIP-7044 + if self + .spec + .fork_name_at_epoch(signing_epoch) + .is_feature_enabled(FeatureName::Deneb) + { + SigningContext { domain, epoch: signing_epoch, fork: Fork { @@ -378,7 +374,14 @@ impl ValidatorStore { epoch: signing_epoch, }, genesis_validators_root: self.genesis_validators_root, - }, + } + } else { + SigningContext { + domain, + epoch: signing_epoch, + fork: self.fork(signing_epoch), + genesis_validators_root: self.genesis_validators_root, + } } } else { SigningContext {