From f93dfd0c28ed484133b99b7af2cb55aa85413c16 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 7 Mar 2022 17:33:59 +1100 Subject: [PATCH] Arc-ify immutable Validator fields --- .../beacon_chain/src/validator_monitor.rs | 4 +- .../src/validator_pubkey_cache.rs | 2 +- beacon_node/http_api/src/lib.rs | 6 +- .../process_operations.rs | 7 +- .../per_block_processing/signature_sets.rs | 2 +- consensus/types/src/beacon_state.rs | 6 +- consensus/types/src/lib.rs | 3 +- consensus/types/src/tree_hash_impls.rs | 166 ------------------ consensus/types/src/validator.rs | 72 +++++++- 9 files changed, 80 insertions(+), 188 deletions(-) delete mode 100644 consensus/types/src/tree_hash_impls.rs diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index 6292409d7f..f168ec258a 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -297,10 +297,10 @@ impl ValidatorMonitor { .skip(self.indices.len()) .for_each(|(i, validator)| { let i = i as u64; - if let Some(validator) = self.validators.get_mut(&validator.pubkey) { + if let Some(validator) = self.validators.get_mut(validator.pubkey()) { validator.set_index(i) } - self.indices.insert(i, validator.pubkey); + self.indices.insert(i, *validator.pubkey()); }); // Update metrics for individual validators. diff --git a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs index 518310eba8..da55e73e82 100644 --- a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs +++ b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs @@ -116,7 +116,7 @@ impl ValidatorPubkeyCache { .validators() .iter_from(self.pubkeys.len()) .unwrap() // FIXME(sproul) - .map(|v| v.pubkey), + .map(|v| *v.pubkey()), ) } else { Ok(()) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index dcc6528a9b..3a3f408a39 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -530,7 +530,7 @@ pub fn serve( query.id.as_ref().map_or(true, |ids| { ids.iter().any(|id| match id { ValidatorId::PublicKey(pubkey) => { - &validator.pubkey == pubkey + validator.pubkey() == pubkey } ValidatorId::Index(param_index) => { *param_index == *index as u64 @@ -578,7 +578,7 @@ pub fn serve( query.id.as_ref().map_or(true, |ids| { ids.iter().any(|id| match id { ValidatorId::PublicKey(pubkey) => { - &validator.pubkey == pubkey + validator.pubkey() == pubkey } ValidatorId::Index(param_index) => { *param_index == *index as u64 @@ -635,7 +635,7 @@ pub fn serve( .map_state(&chain, |state| { let index_opt = match &validator_id { ValidatorId::PublicKey(pubkey) => { - state.validators().iter().position(|v| v.pubkey == *pubkey) + state.validators().iter().position(|v| v.pubkey() == pubkey) } ValidatorId::Index(index) => Some(*index as usize), }; 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 fefdb84a06..f830e62541 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -6,6 +6,7 @@ use crate::common::{ use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::VerifySignatures; use safe_arith::SafeArith; +use std::sync::Arc; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; pub fn process_operations<'a, T: EthSpec>( @@ -340,8 +341,10 @@ pub fn process_deposit( // Create a new validator. let validator = Validator { - pubkey: deposit.data.pubkey, - withdrawal_credentials: deposit.data.withdrawal_credentials, + immutable: Arc::new(ValidatorImmutable { + pubkey: deposit.data.pubkey, + withdrawal_credentials: deposit.data.withdrawal_credentials, + }), activation_eligibility_epoch: spec.far_future_epoch, activation_epoch: spec.far_future_epoch, exit_epoch: spec.far_future_epoch, diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 5a89bd6867..326bc76803 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -63,7 +63,7 @@ where .validators() .get(validator_index) .and_then(|v| { - let pk: Option = v.pubkey.decompress().ok(); + let pk: Option = v.pubkey().decompress().ok(); pk }) .map(Cow::Owned) diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 3f240c29b8..10ea3983cb 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -865,7 +865,7 @@ impl BeaconState { .map(|&index| { self.validators() .get(index) - .map(|v| v.pubkey) + .map(|v| *v.pubkey()) .ok_or(Error::UnknownValidator(index)) }) .collect::, _>>()?; @@ -896,7 +896,7 @@ impl BeaconState { validator_indices .iter() .map(|&validator_index| { - let pubkey = self.get_validator(validator_index as usize)?.pubkey; + let pubkey = *self.get_validator(validator_index as usize)?.pubkey(); Ok(SyncDuty::from_sync_committee( validator_index, @@ -1555,7 +1555,7 @@ impl BeaconState { for (i, validator) in self.validators().iter_from(start_index)?.enumerate() { let index = start_index.safe_add(i)?; - let success = pubkey_cache.insert(validator.pubkey, index); + let success = pubkey_cache.insert(*validator.pubkey(), index); if !success { return Err(Error::PubkeyCacheInconsistent); } diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 7e3ca39ccb..ac3d7b78e8 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -80,7 +80,6 @@ pub mod sync_committee_contribution; pub mod sync_committee_message; pub mod sync_selection_proof; pub mod sync_subnet_id; -mod tree_hash_impls; pub mod slot_data; #[cfg(feature = "sqlite")] @@ -154,7 +153,7 @@ pub use crate::sync_committee_subscription::SyncCommitteeSubscription; pub use crate::sync_duty::SyncDuty; pub use crate::sync_selection_proof::SyncSelectionProof; pub use crate::sync_subnet_id::SyncSubnetId; -pub use crate::validator::Validator; +pub use crate::validator::{Validator, ValidatorImmutable}; pub use crate::validator_subscription::ValidatorSubscription; pub use crate::voluntary_exit::VoluntaryExit; diff --git a/consensus/types/src/tree_hash_impls.rs b/consensus/types/src/tree_hash_impls.rs deleted file mode 100644 index ec23927d30..0000000000 --- a/consensus/types/src/tree_hash_impls.rs +++ /dev/null @@ -1,166 +0,0 @@ -//! This module contains custom implementations of `CachedTreeHash` for ETH2-specific types. -//! -//! It makes some assumptions about the layouts and update patterns of other structs in this -//! crate, and should be updated carefully whenever those structs are changed. -use crate::{Epoch, Hash256, PublicKeyBytes, Validator}; -use cached_tree_hash::{int_log, CacheArena, CachedTreeHash, Error, TreeHashCache}; -use int_to_bytes::int_to_fixed_bytes32; -use tree_hash::merkle_root; - -/// Number of struct fields on `Validator`. -const NUM_VALIDATOR_FIELDS: usize = 8; - -impl CachedTreeHash for Validator { - fn new_tree_hash_cache(&self, arena: &mut CacheArena) -> TreeHashCache { - TreeHashCache::new(arena, int_log(NUM_VALIDATOR_FIELDS), NUM_VALIDATOR_FIELDS) - } - - /// Efficiently tree hash a `Validator`, assuming it was updated by a valid state transition. - /// - /// Specifically, we assume that the `pubkey` and `withdrawal_credentials` fields are constant. - fn recalculate_tree_hash_root( - &self, - arena: &mut CacheArena, - cache: &mut TreeHashCache, - ) -> Result { - // Otherwise just check the fields which might have changed. - let dirty_indices = cache - .leaves() - .iter_mut(arena)? - .enumerate() - .flat_map(|(i, leaf)| { - // Fields pubkey and withdrawal_credentials are constant - if (i == 0 || i == 1) && cache.initialized { - None - } else if process_field_by_index(self, i, leaf, !cache.initialized) { - Some(i) - } else { - None - } - }) - .collect(); - - cache.update_merkle_root(arena, dirty_indices) - } -} - -fn process_field_by_index( - v: &Validator, - field_idx: usize, - leaf: &mut Hash256, - force_update: bool, -) -> bool { - match field_idx { - 0 => process_pubkey_bytes_field(&v.pubkey, leaf, force_update), - 1 => process_slice_field(v.withdrawal_credentials.as_bytes(), leaf, force_update), - 2 => process_u64_field(v.effective_balance, leaf, force_update), - 3 => process_bool_field(v.slashed, leaf, force_update), - 4 => process_epoch_field(v.activation_eligibility_epoch, leaf, force_update), - 5 => process_epoch_field(v.activation_epoch, leaf, force_update), - 6 => process_epoch_field(v.exit_epoch, leaf, force_update), - 7 => process_epoch_field(v.withdrawable_epoch, leaf, force_update), - _ => panic!( - "Validator type only has {} fields, {} out of bounds", - NUM_VALIDATOR_FIELDS, field_idx - ), - } -} - -fn process_pubkey_bytes_field( - val: &PublicKeyBytes, - leaf: &mut Hash256, - force_update: bool, -) -> bool { - let new_tree_hash = merkle_root(val.as_serialized(), 0); - process_slice_field(new_tree_hash.as_bytes(), leaf, force_update) -} - -fn process_slice_field(new_tree_hash: &[u8], leaf: &mut Hash256, force_update: bool) -> bool { - if force_update || leaf.as_bytes() != new_tree_hash { - leaf.assign_from_slice(new_tree_hash); - true - } else { - false - } -} - -fn process_u64_field(val: u64, leaf: &mut Hash256, force_update: bool) -> bool { - let new_tree_hash = int_to_fixed_bytes32(val); - process_slice_field(&new_tree_hash[..], leaf, force_update) -} - -fn process_epoch_field(val: Epoch, leaf: &mut Hash256, force_update: bool) -> bool { - process_u64_field(val.as_u64(), leaf, force_update) -} - -fn process_bool_field(val: bool, leaf: &mut Hash256, force_update: bool) -> bool { - process_u64_field(val as u64, leaf, force_update) -} - -#[cfg(test)] -mod test { - use super::*; - use crate::test_utils::TestRandom; - use crate::Epoch; - use rand::SeedableRng; - use rand_xorshift::XorShiftRng; - use tree_hash::TreeHash; - - fn test_validator_tree_hash(v: &Validator) { - let arena = &mut CacheArena::default(); - - let mut cache = v.new_tree_hash_cache(arena); - // With a fresh cache - assert_eq!( - &v.tree_hash_root()[..], - v.recalculate_tree_hash_root(arena, &mut cache) - .unwrap() - .as_bytes(), - "{:?}", - v - ); - // With a completely up-to-date cache - assert_eq!( - &v.tree_hash_root()[..], - v.recalculate_tree_hash_root(arena, &mut cache) - .unwrap() - .as_bytes(), - "{:?}", - v - ); - } - - #[test] - fn default_validator() { - test_validator_tree_hash(&Validator::default()); - } - - #[test] - fn zeroed_validator() { - let v = Validator { - activation_eligibility_epoch: Epoch::from(0u64), - activation_epoch: Epoch::from(0u64), - ..Default::default() - }; - test_validator_tree_hash(&v); - } - - #[test] - fn random_validators() { - let mut rng = XorShiftRng::from_seed([0xf1; 16]); - let num_validators = 1000; - (0..num_validators) - .map(|_| Validator::random_for_test(&mut rng)) - .for_each(|v| test_validator_tree_hash(&v)); - } - - #[test] - #[allow(clippy::assertions_on_constants)] - pub fn smallvec_size_check() { - // If this test fails we need to go and reassess the length of the `SmallVec` in - // `cached_tree_hash::TreeHashCache`. If the size of the `SmallVec` is too slow we're going - // to start doing heap allocations for each validator, this will fragment memory and slow - // us down. - assert!(NUM_VALIDATOR_FIELDS <= 8,); - } -} diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 21a6b39b6d..88cb58b9c4 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -3,17 +3,17 @@ use crate::{ }; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; +use std::sync::Arc; use test_random_derive::TestRandom; -use tree_hash_derive::TreeHash; +use tree_hash::TreeHash; + +const NUM_FIELDS: usize = 8; /// Information about a `BeaconChain` validator. -/// -/// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TestRandom, TreeHash)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TestRandom)] pub struct Validator { - pub pubkey: PublicKeyBytes, - pub withdrawal_credentials: Hash256, + pub immutable: Arc, #[serde(with = "eth2_serde_utils::quoted_u64")] pub effective_balance: u64, pub slashed: bool, @@ -23,7 +23,23 @@ pub struct Validator { pub withdrawable_epoch: Epoch, } +/// The immutable fields of a validator, behind an `Arc` to enable sharing. +#[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TestRandom)] +pub struct ValidatorImmutable { + pub pubkey: PublicKeyBytes, + pub withdrawal_credentials: Hash256, +} + impl Validator { + pub fn pubkey(&self) -> &PublicKeyBytes { + &self.immutable.pubkey + } + + pub fn withdrawal_credentials(&self) -> Hash256 { + self.immutable.withdrawal_credentials + } + /// Returns `true` if the validator is considered active at some epoch. pub fn is_active_at(&self, epoch: Epoch) -> bool { self.activation_epoch <= epoch && epoch < self.exit_epoch @@ -65,14 +81,35 @@ impl Validator { // Has not yet been activated && self.activation_epoch == spec.far_future_epoch } + + fn tree_hash_root_internal(&self) -> Result { + let mut hasher = tree_hash::MerkleHasher::with_leaves(NUM_FIELDS); + + hasher.write(self.pubkey().tree_hash_root().as_bytes())?; + hasher.write(self.withdrawal_credentials().tree_hash_root().as_bytes())?; + hasher.write(self.effective_balance.tree_hash_root().as_bytes())?; + hasher.write(self.slashed.tree_hash_root().as_bytes())?; + hasher.write( + self.activation_eligibility_epoch + .tree_hash_root() + .as_bytes(), + )?; + hasher.write(self.activation_epoch.tree_hash_root().as_bytes())?; + hasher.write(self.exit_epoch.tree_hash_root().as_bytes())?; + hasher.write(self.withdrawable_epoch.tree_hash_root().as_bytes())?; + + hasher.finish() + } } impl Default for Validator { /// Yields a "default" `Validator`. Primarily used for testing. fn default() -> Self { Self { - pubkey: PublicKeyBytes::empty(), - withdrawal_credentials: Hash256::default(), + immutable: Arc::new(ValidatorImmutable { + pubkey: PublicKeyBytes::empty(), + withdrawal_credentials: Hash256::default(), + }), activation_eligibility_epoch: Epoch::from(std::u64::MAX), activation_epoch: Epoch::from(std::u64::MAX), exit_epoch: Epoch::from(std::u64::MAX), @@ -83,6 +120,25 @@ impl Default for Validator { } } +impl TreeHash for Validator { + fn tree_hash_type() -> tree_hash::TreeHashType { + tree_hash::TreeHashType::Container + } + + fn tree_hash_packed_encoding(&self) -> tree_hash::PackedEncoding { + unreachable!("Struct should never be packed.") + } + + fn tree_hash_packing_factor() -> usize { + unreachable!("Struct should never be packed.") + } + + fn tree_hash_root(&self) -> Hash256 { + self.tree_hash_root_internal() + .expect("Validator tree_hash_root should not fail") + } +} + #[cfg(test)] mod tests { use super::*;