Remove clones from attestation import

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<T: BeaconChainTypes> {
+pub trait VerifiedAttestation<T: BeaconChainTypes>: Sized {
     fn attestation(&self) -> &Attestation<T::EthSpec>;
 
     fn indexed_attestation(&self) -> &IndexedAttestation<T::EthSpec>;
+
+    // Inefficient default implementation. This is overridden for gossip verified attestations.
+    fn into_attestation_and_indices(self) -> (Attestation<T::EthSpec>, Vec<u64>) {
+        let attestation = self.attestation().clone();
+        let attesting_indices = self.indexed_attestation().attesting_indices.clone().into();
+        (attestation, attesting_indices)
+    }
 }
 
 impl<'a, T: BeaconChainTypes> VerifiedAttestation<T> for VerifiedAggregatedAttestation<'a, T> {

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1856,28 +1856,22 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     /// 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<A>(
         &self,
-        verified_attestation: &impl VerifiedAttestation<T>,
+        verified_attestation: A,
-    ) -> Result<(), AttestationError> {
+    ) -> Result<(), AttestationError>
+    where
+        A: VerifiedAttestation<T>,
+    {
         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();
+            let (attestation, attesting_indices) =
-
+                verified_attestation.into_attestation_and_indices();
-            // TODO: address these clones.
-            let attesting_indices = verified_attestation
-                .indexed_attestation()
-                .attesting_indices
-                .clone()
-                .into();
             self.op_pool
-                .insert_attestation(
+                .insert_attestation(attestation, attesting_indices)
-                    verified_attestation.attestation().clone(),
-                    attesting_indices,
-                )
                 .map_err(Error::from)?;
         }
 

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();
         }
     }
 

beacon_node/http_api/src/lib.rs

@@ -2271,12 +2271,13 @@ pub fn serve<T: BeaconChainTypes>(
                                 );
                             failures.push(api_types::Failure::new(index, format!("Fork choice: {:?}", e)));
                         }
-                        if let Err(e) = chain.add_to_block_inclusion_pool(&verified_aggregate) {
+                        if let Err(e) = chain.add_to_block_inclusion_pool(verified_aggregate) {
-                            warn!(log,
+                            warn!(
-                                    "Could not add verified aggregate attestation to the inclusion pool";
+                                log,
-                                    "error" => format!("{:?}", e),
+                                "Could not add verified aggregate attestation to the inclusion pool";
-                                    "request_index" => index,
+                                "error" => ?e,
-                                );
+                                "request_index" => index,
+                            );
                             failures.push(api_types::Failure::new(index, format!("Op pool: {:?}", e)));
                         }
                     }

beacon_node/network/src/beacon_processor/worker/gossip_methods.rs

@@ -54,6 +54,12 @@ impl<T: BeaconChainTypes> VerifiedAttestation<T> for VerifiedUnaggregate<T> {
     fn indexed_attestation(&self) -> &IndexedAttestation<T::EthSpec> {
         &self.indexed_attestation
     }
+
+    fn into_attestation_and_indices(self) -> (Attestation<T::EthSpec>, Vec<u64>) {
+        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<T: BeaconChainTypes> VerifiedAttestation<T> for VerifiedAggregate<T> {
     fn indexed_attestation(&self) -> &IndexedAttestation<T::EthSpec> {
         &self.indexed_attestation
     }
+
+    /// Efficient clone-free implementation that moves out of the `Box`.
+    fn into_attestation_and_indices(self) -> (Attestation<T::EthSpec>, Vec<u64>) {
+        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<T: BeaconChainTypes> Worker<T> {
                     }
                 }
 
-                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";