chore(l1): remove blockchain clones

cmd/ethrex/bench/build_block_benchmark.rs

@@ -151,7 +151,7 @@ async fn create_payload_block(genesis_block: &Block, store: &Store) -> (Block, u
         gas_ceil: DEFAULT_BUILDER_GAS_CEIL,
     };
     let id = payload_args.id();
-    let block = create_payload(&payload_args, store, Bytes::new()).unwrap();
+    let block = create_payload(payload_args, store, Bytes::new()).unwrap();
     (block, id.unwrap())
 }
 

crates/blockchain/blockchain.rs

@@ -54,7 +54,7 @@ const MAX_MEMPOOL_SIZE_DEFAULT: usize = 10_000;
 //TODO: Implement a struct Chain or BlockChain to encapsulate
 //functionality and canonical chain state and config
 
-#[derive(Debug, Clone, Default)]
+#[derive(Debug, Copy, Clone, Default)]
 pub enum BlockchainType {
     #[default]
     L1,
@@ -140,7 +140,7 @@ impl Blockchain {
         // Validate if it can be the new head and find the parent
         let Ok(parent_header) = find_parent_header(&block.header, &self.storage) else {
             // If the parent is not present, we store it as pending.
-            self.storage.add_pending_block(block.clone()).await?;
+            self.storage.add_pending_block(block.clone()).await?; // ok-clone: storage consumes block, so we need a copy
             return Err(ChainError::ParentNotFound);
         };
 
@@ -149,7 +149,7 @@ impl Blockchain {
         // Validate the block pre-execution
         validate_block(block, &parent_header, &chain_config, ELASTICITY_MULTIPLIER)?;
 
-        let vm_db = StoreVmDatabase::new(self.storage.clone(), block.header.parent_hash);
+        let vm_db = StoreVmDatabase::new(self.storage.clone(), block.header.parent_hash); // ok-clone: store struct fields are all arcs, so this just increases their reference count
         let mut vm = self.new_evm(vm_db)?;
 
         let execution_result = vm.execute_block(block)?;
@@ -186,13 +186,12 @@ impl Blockchain {
         &self,
         blocks: &[Block],
     ) -> Result<ExecutionWitness, ChainError> {
-        let first_block_header = blocks
+        let first_block_header = &blocks
             .first()
             .ok_or(ChainError::WitnessGeneration(
                 "Empty block batch".to_string(),
             ))?
-            .header
-            .clone();
+            .header;
 
         // Get state at previous block
         let trie = self
@@ -218,13 +217,13 @@ impl Blockchain {
         for block in blocks {
             let parent_hash = block.header.parent_hash;
             let vm_db: DynVmDatabase =
-                Box::new(StoreVmDatabase::new(self.storage.clone(), parent_hash));
+                Box::new(StoreVmDatabase::new(self.storage.clone(), parent_hash)); // ok-clone: store struct fields are all arcs, so this just increases their reference count
             let logger = Arc::new(DatabaseLogger::new(Arc::new(Mutex::new(Box::new(vm_db)))));
             let mut vm = match self.options.r#type {
-                BlockchainType::L1 => Evm::new_from_db_for_l1(logger.clone()),
+                BlockchainType::L1 => Evm::new_from_db_for_l1(logger.clone()), // ok-clone: increase arc reference count
                 BlockchainType::L2(fee_config) => {
                     Evm::new_from_db_for_l2(logger.clone(), fee_config)
-                }
+                } // ok-clone: increase arc reference count
             };
 
             // Re-execute block with logger
@@ -244,13 +243,10 @@ impl Blockchain {
             }
 
             // Get the used block hashes from the logger
-            let logger_block_hashes = logger
-                .block_hashes_accessed
-                .lock()
-                .map_err(|_e| {
+            let logger_block_hashes =
+                std::mem::take(&mut *logger.block_hashes_accessed.lock().map_err(|_e| {
                     ChainError::WitnessGeneration("Failed to get block hashes".to_string())
-                })?
-                .clone();
+                })?);
             block_hashes.extend(logger_block_hashes);
             // Access all the accounts needed for withdrawals
             if let Some(withdrawals) = block.body.withdrawals.as_ref() {
@@ -416,7 +412,7 @@ impl Blockchain {
         };
 
         self.storage
-            .clone()
+            .clone() // ok-clone: store struct fields are all arcs, so this just increases their reference count
             .store_block_updates(update_batch)
             .await
             .map_err(|e| e.into())
@@ -524,7 +520,7 @@ impl Blockchain {
     ) -> Result<(), (ChainError, Option<BatchBlockProcessingFailure>)> {
         let mut last_valid_hash = H256::default();
 
-        let Some(first_block_header) = blocks.first().map(|e| e.header.clone()) else {
+        let Some(first_block_header) = blocks.first().map(|e| &e.header) else {
             return Err((ChainError::Custom("First block not found".into()), None));
         };
 
@@ -537,7 +533,7 @@ impl Blockchain {
         let block_hash_cache = blocks.iter().map(|b| (b.header.number, b.hash())).collect();
 
         let vm_db = StoreVmDatabase::new_with_block_hash_cache(
-            self.storage.clone(),
+            self.storage.clone(), // ok-clone: store struct fields are all arcs, so this just increases their reference count
             first_block_header.parent_hash,
             block_hash_cache,
         );
@@ -556,7 +552,7 @@ impl Blockchain {
             }
             // for the first block, we need to query the store
             let parent_header = if i == 0 {
-                find_parent_header(&block.header, &self.storage).map_err(|err| {
+                &find_parent_header(&block.header, &self.storage).map_err(|err| {
                     (
                         err,
                         Some(BatchBlockProcessingFailure {
@@ -567,11 +563,11 @@ impl Blockchain {
                 })?
             } else {
                 // for the subsequent ones, the parent is the previous block
-                blocks[i - 1].header.clone()
+                &blocks[i - 1].header
             };
 
             let BlockExecutionResult { receipts, .. } = self
-                .execute_block_from_state(&parent_header, block, &chain_config, &mut vm)
+                .execute_block_from_state(parent_header, block, &chain_config, &mut vm)
                 .map_err(|err| {
                     (
                         err,

crates/blockchain/fork_choice.rs

@@ -155,11 +155,10 @@ async fn find_link_with_canonical_chain(
     }
 
     let genesis_number = store.get_earliest_block_number().await?;
-    let mut header = block_header.clone();
+    let mut parent_hash = block_header.parent_hash;
 
     while block_number > genesis_number {
         block_number -= 1;
-        let parent_hash = header.parent_hash;
 
         // Check that the parent exists.
         let parent_header = match store.get_block_header_by_hash(parent_hash) {
@@ -174,7 +173,7 @@ async fn find_link_with_canonical_chain(
             branch.push((block_number, parent_hash));
         }
 
-        header = parent_header;
+        parent_hash = parent_header.parent_hash;
     }
 
     Ok(None)

crates/blockchain/mempool.rs

@@ -135,7 +135,7 @@ impl Mempool {
                 if !inner.broadcast_pool.contains(hash) {
                     None
                 } else {
-                    Some(tx.clone())
+                    Some(tx.clone()) // ok-clone: address and sender fields of MempoolTransaction are cheap to clone, and transaction is an Arc
                 }
             })
             .collect::<Vec<_>>();
@@ -223,7 +223,7 @@ impl Mempool {
             txs_by_sender
                 .entry(tx.sender())
                 .or_insert_with(|| Vec::with_capacity(128))
-                .push(tx.clone())
+                .push(tx.clone()) // ok-clone: address and sender fields of MempoolTransaction are cheap to clone, and transaction is an Arc
         }
 
         txs_by_sender.iter_mut().for_each(|(_, txs)| txs.sort());
@@ -245,7 +245,7 @@ impl Mempool {
                 txs_by_sender
                     .entry(tx.sender())
                     .or_insert_with(|| Vec::with_capacity(128))
-                    .push(tx.clone())
+                    .push(tx.clone()) // ok-clone: address and sender fields of MempoolTransaction are cheap to clone, and transaction is an Arc
             }
         }
 
@@ -852,7 +852,7 @@ mod tests {
         let filter =
             |tx: &Transaction| -> bool { matches!(tx, Transaction::EIP4844Transaction(_)) };
         mempool
-            .add_transaction(blob_tx_hash, blob_tx.clone())
+            .add_transaction(blob_tx_hash, blob_tx.clone()) // ok-clone: clone used in test
             .unwrap();
         mempool.add_transaction(plain_tx_hash, plain_tx).unwrap();
         let txs = mempool.filter_transactions_with_filter_fn(&filter).unwrap();

crates/blockchain/metrics/l2/metrics.rs

@@ -240,6 +240,7 @@ impl Metrics {
     pub fn gather_metrics(&self) -> Result<String, MetricsError> {
         let r = Registry::new();
 
+        // ok-clone: prometheus counter structs are effectively an arc, and we want multiple references to them
         r.register(Box::new(self.status_tracker.clone()))
             .map_err(|e| MetricsError::PrometheusErr(e.to_string()))?;
         r.register(Box::new(self.l1_gas_price.clone()))

crates/blockchain/metrics/metrics_blocks.rs

@@ -69,6 +69,7 @@ impl MetricsBlocks {
 
         let r = Registry::new();
 
+        // ok-clone: prometheus counter structs are effectively an arc, and we want multiple references to them
         r.register(Box::new(self.gas_limit.clone()))
             .map_err(|e| MetricsError::PrometheusErr(e.to_string()))?;
         r.register(Box::new(self.block_number.clone()))

crates/blockchain/metrics/metrics_transactions.rs

@@ -68,7 +68,7 @@ impl MetricsTx {
     }
 
     pub fn inc_tx_with_type(&self, tx_type: MetricsTxType) {
-        let txs = self.transactions_tracker.clone();
+        let txs = self.transactions_tracker.clone(); // ok-clone: prometheus counter structs are effectively an arc, and we want multiple references to them
 
         let txs_builder = match txs.get_metric_with_label_values(&[tx_type.to_str()]) {
             Ok(builder) => builder,
@@ -82,7 +82,7 @@ impl MetricsTx {
     }
 
     pub fn inc_tx_errors(&self, tx_error: &str) {
-        let tx_errors = self.transaction_errors_count.clone();
+        let tx_errors = self.transaction_errors_count.clone(); // ok-clone: prometheus counter structs are effectively an arc, and we want multiple references to them
 
         let tx_errors_builder = match tx_errors.get_metric_with_label_values(&[tx_error]) {
             Ok(builder) => builder,
@@ -120,6 +120,7 @@ impl MetricsTx {
     pub fn gather_metrics(&self) -> Result<String, MetricsError> {
         let r = Registry::new();
 
+        // ok-clone: prometheus counter structs are effectively an arc, and we want multiple references to them
         r.register(Box::new(self.transactions_total.clone()))
             .map_err(|e| MetricsError::PrometheusErr(e.to_string()))?;
         r.register(Box::new(self.transactions_tracker.clone()))

crates/blockchain/metrics/profiling.rs

@@ -41,7 +41,7 @@ where
                 .with_label_values(&[name])
                 .start_timer();
             let mut timers = self.function_timers.lock().unwrap();
-            timers.insert(id.clone(), timer);
+            timers.insert(id.clone(), timer); // ok-clone: necessary due to function interface (which is determined by the crate); plus it's effectively a wrapped u64
         }
     }
 

crates/blockchain/payload.rs

@@ -118,7 +118,7 @@ impl BuildPayloadArgs {
 /// Creates a new payload based on the payload arguments
 // Basic payload block building, can and should be improved
 pub fn create_payload(
-    args: &BuildPayloadArgs,
+    args: BuildPayloadArgs,
     storage: &Store,
     extra_data: Bytes,
 ) -> Result<Block, ChainError> {
@@ -174,7 +174,7 @@ pub fn create_payload(
     let body = BlockBody {
         transactions: Vec::new(),
         ommers: Vec::new(),
-        withdrawals: args.withdrawals.clone(),
+        withdrawals: args.withdrawals,
     };
 
     // Delay applying withdrawals until the payload is requested and built
@@ -233,7 +233,7 @@ impl PayloadBuildContext {
                 .unwrap_or_default(),
         );
 
-        let vm_db = StoreVmDatabase::new(storage.clone(), payload.header.parent_hash);
+        let vm_db = StoreVmDatabase::new(storage.clone(), payload.header.parent_hash); // ok-clone: store struct fields are all arcs, so this just increases their reference count
         let vm = match blockchain_type {
             BlockchainType::L1 => Evm::new_for_l1(vm_db),
             BlockchainType::L2(fee_config) => Evm::new_for_l2(vm_db, fee_config)?,
@@ -249,7 +249,7 @@ impl PayloadBuildContext {
             base_fee_per_blob_gas: U256::from(base_fee_per_blob_gas),
             payload,
             blobs_bundle: BlobsBundle::default(),
-            store: storage.clone(),
+            store: storage.clone(), // ok-clone: store struct fields are all arcs, so this just increases their reference count
             vm,
             account_updates: Vec::new(),
         })
@@ -280,7 +280,7 @@ impl PayloadBuildContext {
     }
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Default)]
 pub struct PayloadBuildResult {
     pub blobs_bundle: BlobsBundle,
     pub block_value: U256,
@@ -327,17 +327,17 @@ impl Blockchain {
         payloads.insert(idx, finished_payload);
         // Return the held payload
         match &payloads[idx].1 {
-            PayloadOrTask::Payload(payload) => Ok(*payload.clone()),
+            PayloadOrTask::Payload(payload) => Ok(*payload.clone()), // todo-clone: might be possible to remove, but it's tricky given mutable access to self isn't normally possible (since Blockchain is usually behind Arc)
             _ => unreachable!("we already converted the payload into a finished version"),
         }
     }
 
     /// Starts a payload build process. The built payload can be retrieved by calling `get_payload`.
     /// The build process will run for the full block building timeslot or until `get_payload` is called
     pub async fn initiate_payload_build(self: Arc<Blockchain>, payload: Block, payload_id: u64) {
-        let self_clone = self.clone();
+        let self_clone = self.clone(); // ok-clone: increase arc reference count
         let cancel_token = CancellationToken::new();
-        let cancel_token_clone = cancel_token.clone();
+        let cancel_token_clone = cancel_token.clone(); // ok-clone: increase arc reference count
         let payload_build_task = tokio::task::spawn(async move {
             self_clone
                 .build_payload_loop(payload, cancel_token_clone)
@@ -365,9 +365,10 @@ impl Blockchain {
         cancel_token: CancellationToken,
     ) -> Result<PayloadBuildResult, ChainError> {
         let start = Instant::now();
-        let self_clone = self.clone();
+        let self_clone = self.clone(); // ok-clone: increase arc reference count
         const SECONDS_PER_SLOT: Duration = Duration::from_secs(12);
         // Attempt to rebuild the payload as many times within the given timeframe to maximize fee revenue
+        // todo-clone: the clones here should be able to be removed if instead of repeatedly building the payload, we kept filling it with transactions until time ran out or payload was requested
         let mut res = self_clone.build_payload(payload.clone()).await?;
         while start.elapsed() < SECONDS_PER_SLOT && !cancel_token.is_cancelled() {
             let payload = payload.clone();
@@ -389,8 +390,7 @@ impl Blockchain {
 
         debug!("Building payload");
         let base_fee = payload.header.base_fee_per_gas.unwrap_or_default();
-        let mut context =
-            PayloadBuildContext::new(payload, &self.storage, self.options.r#type.clone())?;
+        let mut context = PayloadBuildContext::new(payload, &self.storage, self.options.r#type)?;
 
         if let BlockchainType::L1 = self.options.r#type {
             self.apply_system_operations(&mut context)?;
@@ -707,6 +707,8 @@ impl std::ops::Deref for HeadTransaction {
 impl From<HeadTransaction> for Transaction {
     fn from(val: HeadTransaction) -> Self {
         val.tx.transaction().clone()
+        // todo-clone: we could probably remove this clone (although much of the code needs an owned copy anyways)
+        // but it'd require a refactor of how other functions access transactions in the mempool
     }
 }