From ceeab02e3a6a4f518a89392f76c7574fcd17d6df Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 14 Jan 2022 07:20:54 +0000 Subject: [PATCH] Lazy hashing for SignedBeaconBlock in sync (#2916) ## Proposed Changes Allocate less memory in sync by hashing the `SignedBeaconBlock`s in a batch directly, rather than going via SSZ bytes. Credit to @paulhauner for finding this source of temporary allocations. --- Cargo.lock | 1 + .../network/src/sync/range_sync/batch.rs | 3 +-- consensus/ssz_types/Cargo.toml | 1 + consensus/ssz_types/src/bitfield.rs | 4 +++- consensus/ssz_types/src/fixed_vector.rs | 4 +++- consensus/ssz_types/src/variable_list.rs | 4 +++- consensus/types/src/attestation.rs | 6 +++++- consensus/types/src/beacon_block.rs | 9 ++++++--- consensus/types/src/beacon_block_body.rs | 9 ++++++--- consensus/types/src/deposit.rs | 4 +++- consensus/types/src/deposit_data.rs | 4 +++- consensus/types/src/execution_payload.rs | 4 +++- consensus/types/src/graffiti.rs | 2 +- consensus/types/src/signed_beacon_block.rs | 9 ++++++--- .../types/src/signed_beacon_block_header.rs | 18 +----------------- consensus/types/src/signed_voluntary_exit.rs | 4 +++- consensus/types/src/sync_aggregate.rs | 6 +++++- consensus/types/src/voluntary_exit.rs | 4 +++- crypto/bls/src/generic_aggregate_signature.rs | 13 +++++++++++++ crypto/bls/src/generic_signature.rs | 8 ++++++++ crypto/bls/src/generic_signature_bytes.rs | 7 +++++++ crypto/bls/src/impls/fake_crypto.rs | 8 ++++++++ 22 files changed, 93 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bc53b134f..bef8b437b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1701,6 +1701,7 @@ name = "eth2_ssz_types" version = "0.2.2" dependencies = [ "arbitrary", + "derivative", "eth2_serde_utils", "eth2_ssz", "serde", diff --git a/beacon_node/network/src/sync/range_sync/batch.rs b/beacon_node/network/src/sync/range_sync/batch.rs index 70e27b5a0..e0b15cb49 100644 --- a/beacon_node/network/src/sync/range_sync/batch.rs +++ b/beacon_node/network/src/sync/range_sync/batch.rs @@ -1,7 +1,6 @@ use crate::sync::RequestId; use lighthouse_network::rpc::methods::BlocksByRangeRequest; use lighthouse_network::PeerId; -use ssz::Encode; use std::collections::HashSet; use std::hash::{Hash, Hasher}; use std::ops::Sub; @@ -390,7 +389,7 @@ impl Attempt { #[allow(clippy::ptr_arg)] fn new(peer_id: PeerId, blocks: &Vec>) -> Self { let mut hasher = std::collections::hash_map::DefaultHasher::new(); - blocks.as_ssz_bytes().hash(&mut hasher); + blocks.hash(&mut hasher); let hash = hasher.finish(); Attempt { peer_id, hash } } diff --git a/consensus/ssz_types/Cargo.toml b/consensus/ssz_types/Cargo.toml index 4d4b073f4..b71de4ccd 100644 --- a/consensus/ssz_types/Cargo.toml +++ b/consensus/ssz_types/Cargo.toml @@ -17,6 +17,7 @@ eth2_serde_utils = "0.1.1" eth2_ssz = "0.4.1" typenum = "1.12.0" arbitrary = { version = "1.0", features = ["derive"], optional = true } +derivative = "2.1.1" [dev-dependencies] serde_json = "1.0.58" diff --git a/consensus/ssz_types/src/bitfield.rs b/consensus/ssz_types/src/bitfield.rs index afecd8ce7..dfad3aedc 100644 --- a/consensus/ssz_types/src/bitfield.rs +++ b/consensus/ssz_types/src/bitfield.rs @@ -1,6 +1,7 @@ use crate::tree_hash::bitfield_bytes_tree_hash_root; use crate::Error; use core::marker::PhantomData; +use derivative::Derivative; use eth2_serde_utils::hex::{encode as hex_encode, PrefixedHexVisitor}; use serde::de::{Deserialize, Deserializer}; use serde::ser::{Serialize, Serializer}; @@ -87,7 +88,8 @@ pub type BitVector = Bitfield>; /// The internal representation of the bitfield is the same as that required by SSZ. The lowest /// byte (by `Vec` index) stores the lowest bit-indices and the right-most bit stores the lowest /// bit-index. E.g., `vec![0b0000_0001, 0b0000_0010]` has bits `0, 9` set. -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = ""))] pub struct Bitfield { bytes: Vec, len: usize, diff --git a/consensus/ssz_types/src/fixed_vector.rs b/consensus/ssz_types/src/fixed_vector.rs index 8b8d660fb..ca5d40f14 100644 --- a/consensus/ssz_types/src/fixed_vector.rs +++ b/consensus/ssz_types/src/fixed_vector.rs @@ -1,5 +1,6 @@ use crate::tree_hash::vec_tree_hash_root; use crate::Error; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use std::marker::PhantomData; use std::ops::{Deref, DerefMut, Index, IndexMut}; @@ -44,7 +45,8 @@ pub use typenum; /// let long: FixedVector<_, typenum::U5> = FixedVector::from(base); /// assert_eq!(&long[..], &[1, 2, 3, 4, 0]); /// ``` -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: std::hash::Hash"))] #[serde(transparent)] pub struct FixedVector { vec: Vec, diff --git a/consensus/ssz_types/src/variable_list.rs b/consensus/ssz_types/src/variable_list.rs index 242a55b2c..1414d12c8 100644 --- a/consensus/ssz_types/src/variable_list.rs +++ b/consensus/ssz_types/src/variable_list.rs @@ -1,5 +1,6 @@ use crate::tree_hash::vec_tree_hash_root; use crate::Error; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use std::marker::PhantomData; use std::ops::{Deref, DerefMut, Index, IndexMut}; @@ -46,7 +47,8 @@ pub use typenum; /// // Push a value to if it _does_ exceed the maximum. /// assert!(long.push(6).is_err()); /// ``` -#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Derivative)] +#[derivative(PartialEq, Eq, Hash(bound = "T: std::hash::Hash"))] #[serde(transparent)] pub struct VariableList { vec: Vec, diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 66d9e78a8..1c9ec3bc4 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,3 +1,4 @@ +use derivative::Derivative; use safe_arith::ArithError; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -23,7 +24,10 @@ pub enum Error { /// /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, Derivative, +)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] #[serde(bound = "T: EthSpec")] pub struct Attestation { pub aggregation_bits: BitList, diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index a83be72a0..e524f0c12 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -5,6 +5,7 @@ use crate::beacon_block_body::{ use crate::test_utils::TestRandom; use crate::*; use bls::Signature; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz::{Decode, DecodeError}; use ssz_derive::{Decode, Encode}; @@ -19,15 +20,16 @@ use tree_hash_derive::TreeHash; variant_attributes( derive( Debug, - PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, - TestRandom + TestRandom, + Derivative, ), + derivative(PartialEq, Hash(bound = "T: EthSpec")), serde(bound = "T: EthSpec", deny_unknown_fields), cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary)), ), @@ -36,7 +38,8 @@ use tree_hash_derive::TreeHash; tree_hash(enum_behaviour = "transparent") ) )] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, TreeHash)] +#[derive(Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] #[serde(untagged)] #[serde(bound = "T: EthSpec")] #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index d3d005462..c4df4f277 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -1,5 +1,6 @@ use crate::test_utils::TestRandom; use crate::*; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; @@ -15,22 +16,24 @@ use tree_hash_derive::TreeHash; variant_attributes( derive( Debug, - PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, - TestRandom + TestRandom, + Derivative, ), + derivative(PartialEq, Hash(bound = "T: EthSpec")), serde(bound = "T: EthSpec", deny_unknown_fields), cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary)) ), cast_error(ty = "Error", expr = "Error::IncorrectStateVariant"), partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") )] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Derivative)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] #[serde(untagged)] #[serde(bound = "T: EthSpec")] #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] diff --git a/consensus/types/src/deposit.rs b/consensus/types/src/deposit.rs index 4b201360a..a347cf675 100644 --- a/consensus/types/src/deposit.rs +++ b/consensus/types/src/deposit.rs @@ -12,7 +12,9 @@ pub const DEPOSIT_TREE_DEPTH: usize = 32; /// /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] pub struct Deposit { pub proof: FixedVector, pub data: DepositData, diff --git a/consensus/types/src/deposit_data.rs b/consensus/types/src/deposit_data.rs index d984f168f..6c5444e11 100644 --- a/consensus/types/src/deposit_data.rs +++ b/consensus/types/src/deposit_data.rs @@ -11,7 +11,9 @@ use tree_hash_derive::TreeHash; /// /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] pub struct DepositData { pub pubkey: PublicKeyBytes, pub withdrawal_credentials: Hash256, diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 1b29fb34f..2fb253f12 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -1,4 +1,5 @@ use crate::{test_utils::TestRandom, *}; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; @@ -9,8 +10,9 @@ pub type Transaction = VariableList; #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive( - Default, Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, + Default, Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, Derivative, )] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] #[serde(bound = "T: EthSpec")] pub struct ExecutionPayload { pub parent_hash: Hash256, diff --git a/consensus/types/src/graffiti.rs b/consensus/types/src/graffiti.rs index cecd6c201..f5f74b601 100644 --- a/consensus/types/src/graffiti.rs +++ b/consensus/types/src/graffiti.rs @@ -12,7 +12,7 @@ use tree_hash::TreeHash; pub const GRAFFITI_BYTES_LEN: usize = 32; /// The 32-byte `graffiti` field on a beacon block. -#[derive(Default, Debug, PartialEq, Clone, Copy, Serialize, Deserialize)] +#[derive(Default, Debug, PartialEq, Hash, Clone, Copy, Serialize, Deserialize)] #[serde(transparent)] #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] pub struct Graffiti(#[serde(with = "serde_graffiti")] pub [u8; GRAFFITI_BYTES_LEN]); diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 383805f97..8d7df0cb0 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -1,5 +1,6 @@ use crate::*; use bls::Signature; +use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use std::fmt; @@ -41,19 +42,21 @@ impl From for Hash256 { variant_attributes( derive( Debug, - PartialEq, Clone, Serialize, Deserialize, Encode, Decode, - TreeHash + TreeHash, + Derivative, ), + derivative(PartialEq, Hash(bound = "E: EthSpec")), cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary)), serde(bound = "E: EthSpec") ) )] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, TreeHash)] +#[derive(Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative)] +#[derivative(PartialEq, Hash(bound = "E: EthSpec"))] #[serde(untagged)] #[serde(bound = "E: EthSpec")] #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] diff --git a/consensus/types/src/signed_beacon_block_header.rs b/consensus/types/src/signed_beacon_block_header.rs index df7888ec2..dc786beb6 100644 --- a/consensus/types/src/signed_beacon_block_header.rs +++ b/consensus/types/src/signed_beacon_block_header.rs @@ -2,11 +2,8 @@ use crate::{ test_utils::TestRandom, BeaconBlockHeader, ChainSpec, Domain, EthSpec, Fork, Hash256, PublicKey, Signature, SignedRoot, }; -use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; -use ssz::Encode; use ssz_derive::{Decode, Encode}; -use std::hash::{Hash, Hasher}; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -15,26 +12,13 @@ use tree_hash_derive::TreeHash; /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive( - Derivative, Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, + Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, )] -#[derivative(PartialEq, Eq)] pub struct SignedBeaconBlockHeader { pub message: BeaconBlockHeader, pub signature: Signature, } -/// Implementation of non-crypto-secure `Hash`, for use with `HashMap` and `HashSet`. -/// -/// Guarantees `header1 == header2 -> hash(header1) == hash(header2)`. -/// -/// Used in the slasher. -impl Hash for SignedBeaconBlockHeader { - fn hash(&self, state: &mut H) { - self.message.hash(state); - self.signature.as_ssz_bytes().hash(state); - } -} - impl SignedBeaconBlockHeader { /// Verify that this block header was signed by `pubkey`. pub fn verify_signature( diff --git a/consensus/types/src/signed_voluntary_exit.rs b/consensus/types/src/signed_voluntary_exit.rs index 49a9b5345..69f0e6e2c 100644 --- a/consensus/types/src/signed_voluntary_exit.rs +++ b/consensus/types/src/signed_voluntary_exit.rs @@ -10,7 +10,9 @@ use tree_hash_derive::TreeHash; /// /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] pub struct SignedVoluntaryExit { pub message: VoluntaryExit, pub signature: Signature, diff --git a/consensus/types/src/sync_aggregate.rs b/consensus/types/src/sync_aggregate.rs index 781c67374..2292b0211 100644 --- a/consensus/types/src/sync_aggregate.rs +++ b/consensus/types/src/sync_aggregate.rs @@ -1,6 +1,7 @@ use crate::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; use crate::test_utils::TestRandom; use crate::{AggregateSignature, BitVector, EthSpec, SyncCommitteeContribution}; +use derivative::Derivative; use safe_arith::{ArithError, SafeArith}; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -20,7 +21,10 @@ impl From for Error { } #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, Derivative, +)] +#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] #[serde(bound = "T: EthSpec")] pub struct SyncAggregate { pub sync_committee_bits: BitVector, diff --git a/consensus/types/src/voluntary_exit.rs b/consensus/types/src/voluntary_exit.rs index 66d2f0094..cc10632d0 100644 --- a/consensus/types/src/voluntary_exit.rs +++ b/consensus/types/src/voluntary_exit.rs @@ -12,7 +12,9 @@ use tree_hash_derive::TreeHash; /// /// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] -#[derive(Debug, PartialEq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom)] +#[derive( + Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, +)] pub struct VoluntaryExit { /// Earliest epoch when voluntary exit can be processed. pub epoch: Epoch, diff --git a/crypto/bls/src/generic_aggregate_signature.rs b/crypto/bls/src/generic_aggregate_signature.rs index 2001de042..fdb59626f 100644 --- a/crypto/bls/src/generic_aggregate_signature.rs +++ b/crypto/bls/src/generic_aggregate_signature.rs @@ -9,6 +9,7 @@ use serde::de::{Deserialize, Deserializer}; use serde::ser::{Serialize, Serializer}; use ssz::{Decode, Encode}; use std::fmt; +use std::hash::{Hash, Hasher}; use std::marker::PhantomData; use tree_hash::TreeHash; @@ -264,6 +265,18 @@ where impl_tree_hash!(SIGNATURE_BYTES_LEN); } +/// Hashes the `self.serialize()` bytes. +#[allow(clippy::derive_hash_xor_eq)] +impl Hash for GenericAggregateSignature +where + Sig: TSignature, + AggSig: TAggregateSignature, +{ + fn hash(&self, state: &mut H) { + self.serialize().hash(state); + } +} + impl fmt::Display for GenericAggregateSignature where Sig: TSignature, diff --git a/crypto/bls/src/generic_signature.rs b/crypto/bls/src/generic_signature.rs index f3aeeb559..10ef75fc6 100644 --- a/crypto/bls/src/generic_signature.rs +++ b/crypto/bls/src/generic_signature.rs @@ -7,6 +7,7 @@ use serde::de::{Deserialize, Deserializer}; use serde::ser::{Serialize, Serializer}; use ssz::{Decode, Encode}; use std::fmt; +use std::hash::{Hash, Hasher}; use std::marker::PhantomData; use tree_hash::TreeHash; @@ -145,6 +146,13 @@ impl> TreeHash for GenericSignature> Hash for GenericSignature { + fn hash(&self, state: &mut H) { + self.serialize().hash(state); + } +} + impl> fmt::Display for GenericSignature { impl_display!(); } diff --git a/crypto/bls/src/generic_signature_bytes.rs b/crypto/bls/src/generic_signature_bytes.rs index b5c028497..aa33c90d0 100644 --- a/crypto/bls/src/generic_signature_bytes.rs +++ b/crypto/bls/src/generic_signature_bytes.rs @@ -9,6 +9,7 @@ use serde::ser::{Serialize, Serializer}; use ssz::{Decode, Encode}; use std::convert::TryInto; use std::fmt; +use std::hash::{Hash, Hasher}; use std::marker::PhantomData; use tree_hash::TreeHash; @@ -84,6 +85,12 @@ impl PartialEq for GenericSignatureBytes { } } +impl Hash for GenericSignatureBytes { + fn hash(&self, hasher: &mut H) { + self.bytes.hash(hasher); + } +} + /// Serializes the `GenericSignature` in compressed form, storing the bytes in the newly created `Self`. impl From> for GenericSignatureBytes where diff --git a/crypto/bls/src/impls/fake_crypto.rs b/crypto/bls/src/impls/fake_crypto.rs index 35582df38..f2d8b79b9 100644 --- a/crypto/bls/src/impls/fake_crypto.rs +++ b/crypto/bls/src/impls/fake_crypto.rs @@ -113,6 +113,14 @@ impl PartialEq for Signature { } } +impl Eq for Signature {} + +impl std::hash::Hash for Signature { + fn hash(&self, hasher: &mut H) { + self.0.hash(hasher); + } +} + #[derive(Clone)] pub struct AggregateSignature([u8; SIGNATURE_BYTES_LEN]);