From aa853b9a52356893992439b12034aa377f071000 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 6 Jul 2022 19:37:24 +1000 Subject: [PATCH] Remove clones from attestation import --- .../src/attestation_verification.rs | 9 ++++++- beacon_node/beacon_chain/src/beacon_chain.rs | 24 +++++++------------ beacon_node/beacon_chain/src/test_utils.rs | 2 +- beacon_node/http_api/src/lib.rs | 13 +++++----- .../beacon_processor/worker/gossip_methods.rs | 15 +++++++++++- 5 files changed, 39 insertions(+), 24 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 63af6ab9e1..b60ce7efe5 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -318,10 +318,17 @@ impl<'a, T: BeaconChainTypes> Clone for IndexedUnaggregatedAttestation<'a, T> { /// A helper trait implemented on wrapper types that can be progressed to a state where they can be /// verified for application to fork choice. -pub trait VerifiedAttestation { +pub trait VerifiedAttestation: Sized { fn attestation(&self) -> &Attestation; fn indexed_attestation(&self) -> &IndexedAttestation; + + // Inefficient default implementation. This is overridden for gossip verified attestations. + fn into_attestation_and_indices(self) -> (Attestation, Vec) { + let attestation = self.attestation().clone(); + let attesting_indices = self.indexed_attestation().attesting_indices.clone().into(); + (attestation, attesting_indices) + } } impl<'a, T: BeaconChainTypes> VerifiedAttestation for VerifiedAggregatedAttestation<'a, T> { diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 65640092fc..7ea48a2fa4 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1856,28 +1856,22 @@ impl BeaconChain { /// Accepts a `VerifiedAttestation` and attempts to apply it to `self.op_pool`. /// /// The op pool is used by local block producers to pack blocks with operations. - pub fn add_to_block_inclusion_pool( + pub fn add_to_block_inclusion_pool( &self, - verified_attestation: &impl VerifiedAttestation, - ) -> Result<(), AttestationError> { + verified_attestation: A, + ) -> Result<(), AttestationError> + where + A: VerifiedAttestation, + { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_APPLY_TO_OP_POOL); // If there's no eth1 chain then it's impossible to produce blocks and therefore // useless to put things in the op pool. if self.eth1_chain.is_some() { - let fork = self.canonical_head.cached_head().head_fork(); - - // TODO: address these clones. - let attesting_indices = verified_attestation - .indexed_attestation() - .attesting_indices - .clone() - .into(); + let (attestation, attesting_indices) = + verified_attestation.into_attestation_and_indices(); self.op_pool - .insert_attestation( - verified_attestation.attestation().clone(), - attesting_indices, - ) + .insert_attestation(attestation, attesting_indices) .map_err(Error::from)?; } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 62765c2222..579bd3194b 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1411,7 +1411,7 @@ where self.chain .apply_attestation_to_fork_choice(&verified) .unwrap(); - self.chain.add_to_block_inclusion_pool(&verified).unwrap(); + self.chain.add_to_block_inclusion_pool(verified).unwrap(); } } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 606dfb64dc..c0994510d3 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2271,12 +2271,13 @@ pub fn serve( ); failures.push(api_types::Failure::new(index, format!("Fork choice: {:?}", e))); } - if let Err(e) = chain.add_to_block_inclusion_pool(&verified_aggregate) { - warn!(log, - "Could not add verified aggregate attestation to the inclusion pool"; - "error" => format!("{:?}", e), - "request_index" => index, - ); + if let Err(e) = chain.add_to_block_inclusion_pool(verified_aggregate) { + warn!( + log, + "Could not add verified aggregate attestation to the inclusion pool"; + "error" => ?e, + "request_index" => index, + ); failures.push(api_types::Failure::new(index, format!("Op pool: {:?}", e))); } } diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 56f38c7f22..3586050e16 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -54,6 +54,12 @@ impl VerifiedAttestation for VerifiedUnaggregate { fn indexed_attestation(&self) -> &IndexedAttestation { &self.indexed_attestation } + + fn into_attestation_and_indices(self) -> (Attestation, Vec) { + let attestation = *self.attestation; + let attesting_indices = self.indexed_attestation.attesting_indices.into(); + (attestation, attesting_indices) + } } /// An attestation that failed validation by the `BeaconChain`. @@ -81,6 +87,13 @@ impl VerifiedAttestation for VerifiedAggregate { fn indexed_attestation(&self) -> &IndexedAttestation { &self.indexed_attestation } + + /// Efficient clone-free implementation that moves out of the `Box`. + fn into_attestation_and_indices(self) -> (Attestation, Vec) { + let attestation = self.signed_aggregate.message.aggregate; + let attesting_indices = self.indexed_attestation.attesting_indices.into(); + (attestation, attesting_indices) + } } /// An attestation that failed validation by the `BeaconChain`. @@ -595,7 +608,7 @@ impl Worker { } } - if let Err(e) = self.chain.add_to_block_inclusion_pool(&verified_aggregate) { + if let Err(e) = self.chain.add_to_block_inclusion_pool(verified_aggregate) { debug!( self.log, "Attestation invalid for op pool";