From aaa9f762911bca01f11359b32a01fca0a8b9c8a8 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 29 Sep 2025 20:14:13 -0300 Subject: [PATCH 01/20] Added error to pass hive tests and improved error messages --- crates/vm/levm/src/errors.rs | 4 +++- crates/vm/levm/src/hooks/default_hook.rs | 9 ++++++--- tooling/ef_tests/state/deserialize.rs | 3 +++ tooling/ef_tests/state/runner/levm_runner.rs | 3 +++ tooling/ef_tests/state/types.rs | 1 + tooling/ef_tests/state_v2/src/modules/deserialize.rs | 3 +++ tooling/ef_tests/state_v2/src/modules/result_check.rs | 3 +++ tooling/ef_tests/state_v2/src/modules/types.rs | 1 + 8 files changed, 23 insertions(+), 4 deletions(-) diff --git a/crates/vm/levm/src/errors.rs b/crates/vm/levm/src/errors.rs index dd71b78d30..568de40407 100644 --- a/crates/vm/levm/src/errors.rs +++ b/crates/vm/levm/src/errors.rs @@ -96,8 +96,10 @@ pub enum TxValidationError { priority_fee: U256, max_fee_per_gas: U256, }, - #[error("Intrinsic gas too low")] + #[error("Transaction gas limit lower than the minimum gas cost to execute the transaction")] IntrinsicGasTooLow, + #[error("Transaction gas limit lower than the gas cost floor for calldata tokens")] + IntrinsicGasBelowFloorGasCost, #[error( "Gas allowance exceeded. Block gas limit: {block_gas_limit}, transaction gas limit: {tx_gas_limit}" )] diff --git a/crates/vm/levm/src/hooks/default_hook.rs b/crates/vm/levm/src/hooks/default_hook.rs index 22ce5f7d16..cfaf2d7a5c 100644 --- a/crates/vm/levm/src/hooks/default_hook.rs +++ b/crates/vm/levm/src/hooks/default_hook.rs @@ -248,6 +248,10 @@ pub fn validate_min_gas_limit(vm: &mut VM<'_>) -> Result<(), VMError> { let calldata = vm.current_call_frame.calldata.clone(); let intrinsic_gas: u64 = vm.get_intrinsic_gas()?; + if vm.current_call_frame.gas_limit < intrinsic_gas { + return Err(TxValidationError::IntrinsicGasTooLow.into()); + } + // calldata_cost = tokens_in_calldata * 4 let calldata_cost: u64 = gas_cost::tx_calldata(&calldata)?; @@ -261,9 +265,8 @@ pub fn validate_min_gas_limit(vm: &mut VM<'_>) -> Result<(), VMError> { .checked_add(TX_BASE_COST) .ok_or(InternalError::Overflow)?; - let min_gas_limit = max(intrinsic_gas, floor_cost_by_tokens); - if vm.current_call_frame.gas_limit < min_gas_limit { - return Err(TxValidationError::IntrinsicGasTooLow.into()); + if vm.current_call_frame.gas_limit < floor_cost_by_tokens { + return Err(TxValidationError::IntrinsicGasBelowFloorGasCost.into()); } Ok(()) diff --git a/tooling/ef_tests/state/deserialize.rs b/tooling/ef_tests/state/deserialize.rs index 391969072d..f4772d7fc9 100644 --- a/tooling/ef_tests/state/deserialize.rs +++ b/tooling/ef_tests/state/deserialize.rs @@ -41,6 +41,9 @@ where "TransactionException.INTRINSIC_GAS_TOO_LOW" => { TransactionExpectedException::IntrinsicGasTooLow } + "TransactionException.INTRINSIC_BELOW_FLOOR_GAS_COST" => { + TransactionExpectedException::IntrinsicGasBelowFloorGasCost + } "TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => { TransactionExpectedException::InsufficientAccountFunds } diff --git a/tooling/ef_tests/state/runner/levm_runner.rs b/tooling/ef_tests/state/runner/levm_runner.rs index 5a51b91cfc..bc2ed6a0d6 100644 --- a/tooling/ef_tests/state/runner/levm_runner.rs +++ b/tooling/ef_tests/state/runner/levm_runner.rs @@ -309,6 +309,9 @@ fn exception_is_expected( ( TransactionExpectedException::IntrinsicGasTooLow, VMError::TxValidation(TxValidationError::IntrinsicGasTooLow) + ) | ( + TransactionExpectedException::IntrinsicGasBelowFloorGasCost, + VMError::TxValidation(TxValidationError::IntrinsicGasBelowFloorGasCost) ) | ( TransactionExpectedException::InsufficientAccountFunds, VMError::TxValidation(TxValidationError::InsufficientAccountFunds) diff --git a/tooling/ef_tests/state/types.rs b/tooling/ef_tests/state/types.rs index 41408aa3d0..cd7307ef72 100644 --- a/tooling/ef_tests/state/types.rs +++ b/tooling/ef_tests/state/types.rs @@ -156,6 +156,7 @@ pub enum TransactionExpectedException { Type3TxInvalidBlobVersionedHash, Type4TxContractCreation, IntrinsicGasTooLow, + IntrinsicGasBelowFloorGasCost, InsufficientAccountFunds, SenderNotEoa, PriorityGreaterThanMaxFeePerGas, diff --git a/tooling/ef_tests/state_v2/src/modules/deserialize.rs b/tooling/ef_tests/state_v2/src/modules/deserialize.rs index a00d7bbd65..d8d6145757 100644 --- a/tooling/ef_tests/state_v2/src/modules/deserialize.rs +++ b/tooling/ef_tests/state_v2/src/modules/deserialize.rs @@ -38,6 +38,9 @@ where "TransactionException.INTRINSIC_GAS_TOO_LOW" => { TransactionExpectedException::IntrinsicGasTooLow } + "TransactionException.INTRINSIC_GAS_BELOW_FLOOR_GAS_COST" => { + TransactionExpectedException::IntrinsicGasBelowFloorGasCost + } "TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => { TransactionExpectedException::InsufficientAccountFunds } diff --git a/tooling/ef_tests/state_v2/src/modules/result_check.rs b/tooling/ef_tests/state_v2/src/modules/result_check.rs index 6cf7a2bcd4..be32ef27de 100644 --- a/tooling/ef_tests/state_v2/src/modules/result_check.rs +++ b/tooling/ef_tests/state_v2/src/modules/result_check.rs @@ -160,6 +160,9 @@ fn exception_matches_expected( ( TransactionExpectedException::IntrinsicGasTooLow, VMError::TxValidation(TxValidationError::IntrinsicGasTooLow) + ) | ( + TransactionExpectedException::IntrinsicGasBelowFloorGasCost, + VMError::TxValidation(TxValidationError::IntrinsicGasBelowFloorGasCost) ) | ( TransactionExpectedException::InsufficientAccountFunds, VMError::TxValidation(TxValidationError::InsufficientAccountFunds) diff --git a/tooling/ef_tests/state_v2/src/modules/types.rs b/tooling/ef_tests/state_v2/src/modules/types.rs index 2c868a3b29..7799e10023 100644 --- a/tooling/ef_tests/state_v2/src/modules/types.rs +++ b/tooling/ef_tests/state_v2/src/modules/types.rs @@ -447,6 +447,7 @@ pub enum TransactionExpectedException { Type3TxInvalidBlobVersionedHash, Type4TxContractCreation, IntrinsicGasTooLow, + IntrinsicGasBelowFloorGasCost, InsufficientAccountFunds, SenderNotEoa, PriorityGreaterThanMaxFeePerGas, From ab8ab3786c3917f98a0c9dad80b5d9b0a288a5fb Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 29 Sep 2025 20:24:48 -0300 Subject: [PATCH 02/20] Removed unused import --- crates/vm/levm/src/hooks/default_hook.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/vm/levm/src/hooks/default_hook.rs b/crates/vm/levm/src/hooks/default_hook.rs index cfaf2d7a5c..4ed68e1c28 100644 --- a/crates/vm/levm/src/hooks/default_hook.rs +++ b/crates/vm/levm/src/hooks/default_hook.rs @@ -11,8 +11,6 @@ use crate::{ use bytes::Bytes; use ethrex_common::{Address, U256, types::Fork}; -use std::cmp::max; - pub const MAX_REFUND_QUOTIENT: u64 = 5; pub struct DefaultHook; From 1aa1d80aaca6e56612a2bc3229641ce4635112ed Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 30 Sep 2025 10:44:27 -0300 Subject: [PATCH 03/20] Updating message in blockchain tests --- tooling/ef_tests/blockchain/deserialize.rs | 2 +- tooling/ef_tests/blockchain/test_runner.rs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tooling/ef_tests/blockchain/deserialize.rs b/tooling/ef_tests/blockchain/deserialize.rs index 396dcdcb25..166adb4e40 100644 --- a/tooling/ef_tests/blockchain/deserialize.rs +++ b/tooling/ef_tests/blockchain/deserialize.rs @@ -40,7 +40,7 @@ where ) } "TransactionException.INTRINSIC_GAS_TOO_LOW" => { - BlockChainExpectedException::TxtException("Intrinsic gas too low".to_string()) + BlockChainExpectedException::TxtException("Transaction gas limit lower than the minimum gas cost to execute the transaction".to_string()) } "TransactionException.INSUFFICIENT_ACCOUNT_FUNDS" => { BlockChainExpectedException::TxtException( diff --git a/tooling/ef_tests/blockchain/test_runner.rs b/tooling/ef_tests/blockchain/test_runner.rs index 1fbc2753e2..3b6116ddb0 100644 --- a/tooling/ef_tests/blockchain/test_runner.rs +++ b/tooling/ef_tests/blockchain/test_runner.rs @@ -242,9 +242,11 @@ fn match_alternative_revm_exception_msg(expected_msg: &String, msg: &str) -> boo SENDER_NOT_EOA_REGEX ) | ( "call gas cost exceeds the gas limit", - "Intrinsic gas too low" - ) | ("gas floor exceeds the gas limit", "Intrinsic gas too low") - | ("empty blobs", "Type 3 transaction without blobs") + "Transaction gas limit lower than the minimum gas cost to execute the transaction" + ) | ( + "gas floor exceeds the gas limit", + "Transaction gas limit lower than the minimum gas cost to execute the transaction" + ) | ("empty blobs", "Type 3 transaction without blobs") | ( "blob versioned hashes not supported", "Type 3 transactions are not supported before the Cancun fork" From ebed22abc6e83c23ffd8db100d8acc354a5a3123 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Wed, 1 Oct 2025 16:02:47 -0300 Subject: [PATCH 04/20] Added error for case where system contract code is empty --- crates/vm/backends/levm/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 158878d7f0..0dd95b6a15 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -381,6 +381,13 @@ pub fn generic_system_contract_levm( ..Default::default() }; + let account_code_length = db.get_account_code(contract_address)?.len(); + if account_code_length == 0 && contract_address != HISTORY_STORAGE_ADDRESS.address { + return Err(EvmError::SystemContractCallFailed(format!( + "System contract: {contract_address} has no code after deployment" + ))); + }; + let tx = &Transaction::EIP1559Transaction(EIP1559Transaction { to: TxKind::Call(contract_address), value: U256::zero(), From 2080e2f071f6d9ae5dde60bfac4b53fcecf6cb7c Mon Sep 17 00:00:00 2001 From: SDartayet Date: Wed, 1 Oct 2025 17:56:50 -0300 Subject: [PATCH 05/20] Fixed failing test --- crates/vm/backends/levm/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 0dd95b6a15..36cdbee21c 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -381,8 +381,10 @@ pub fn generic_system_contract_levm( ..Default::default() }; - let account_code_length = db.get_account_code(contract_address)?.len(); - if account_code_length == 0 && contract_address != HISTORY_STORAGE_ADDRESS.address { + if !(contract_address == HISTORY_STORAGE_ADDRESS.address + || contract_address == BEACON_ROOTS_ADDRESS.address) + && db.get_account_code(contract_address)?.len() == 0 + { return Err(EvmError::SystemContractCallFailed(format!( "System contract: {contract_address} has no code after deployment" ))); From b9fe38fd5475ebc8138962b0e16974a91f096cd0 Mon Sep 17 00:00:00 2001 From: SDartayet <44068466+SDartayet@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:39:17 -0300 Subject: [PATCH 06/20] fix lint issues --- crates/vm/backends/levm/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 36cdbee21c..0e8af14437 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -383,7 +383,7 @@ pub fn generic_system_contract_levm( if !(contract_address == HISTORY_STORAGE_ADDRESS.address || contract_address == BEACON_ROOTS_ADDRESS.address) - && db.get_account_code(contract_address)?.len() == 0 + && db.get_account_code(contract_address)?.len().is_empty() { return Err(EvmError::SystemContractCallFailed(format!( "System contract: {contract_address} has no code after deployment" From 2749d4d13d99d59e7bf32f9a419b162abe70ca7d Mon Sep 17 00:00:00 2001 From: SDartayet <44068466+SDartayet@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:43:54 -0300 Subject: [PATCH 07/20] fixing previous commit --- crates/vm/backends/levm/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 0e8af14437..57bd0ba7d2 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -383,7 +383,7 @@ pub fn generic_system_contract_levm( if !(contract_address == HISTORY_STORAGE_ADDRESS.address || contract_address == BEACON_ROOTS_ADDRESS.address) - && db.get_account_code(contract_address)?.len().is_empty() + && db.get_account_code(contract_address)?.is_empty() { return Err(EvmError::SystemContractCallFailed(format!( "System contract: {contract_address} has no code after deployment" From 84ba906122912f38b694029ea2fbc638f901906a Mon Sep 17 00:00:00 2001 From: SDartayet Date: Wed, 8 Oct 2025 14:54:37 -0300 Subject: [PATCH 08/20] Addressing review comments --- crates/vm/backends/levm/mod.rs | 9 ++++++--- crates/vm/system_contracts.rs | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 57bd0ba7d2..175bb7c337 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -4,7 +4,7 @@ mod tracing; use super::BlockExecutionResult; use crate::system_contracts::{ BEACON_ROOTS_ADDRESS, CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS, HISTORY_STORAGE_ADDRESS, - SYSTEM_ADDRESS, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS, + SYSTEM_ADDRESS, SYSTEM_CONTRACTS_WITH_CODE, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS, }; use crate::{EvmError, ExecutionResult}; use bytes::Bytes; @@ -381,8 +381,11 @@ pub fn generic_system_contract_levm( ..Default::default() }; - if !(contract_address == HISTORY_STORAGE_ADDRESS.address - || contract_address == BEACON_ROOTS_ADDRESS.address) + // This check is not necessary in practice, since contract deployment has succesfully happened in all relevant testnets and mainnet + // However, it's necessary to pass some of the Hive tests related to system contract deployment, which is why we have it + if SYSTEM_CONTRACTS_WITH_CODE + .iter() + .any(|contract| contract.address == contract_address) && db.get_account_code(contract_address)?.is_empty() { return Err(EvmError::SystemContractCallFailed(format!( diff --git a/crates/vm/system_contracts.rs b/crates/vm/system_contracts.rs index d7be861a31..f7270a7221 100644 --- a/crates/vm/system_contracts.rs +++ b/crates/vm/system_contracts.rs @@ -69,3 +69,9 @@ pub fn system_contracts_for_fork(fork: Fork) -> impl Iterator Date: Thu, 9 Oct 2025 13:15:20 -0300 Subject: [PATCH 09/20] =?UTF-8?q?=C3=85ddressing=20review=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/vm/backends/levm/mod.rs | 4 ++-- crates/vm/system_contracts.rs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/vm/backends/levm/mod.rs b/crates/vm/backends/levm/mod.rs index 175bb7c337..1fc0911370 100644 --- a/crates/vm/backends/levm/mod.rs +++ b/crates/vm/backends/levm/mod.rs @@ -4,7 +4,7 @@ mod tracing; use super::BlockExecutionResult; use crate::system_contracts::{ BEACON_ROOTS_ADDRESS, CONSOLIDATION_REQUEST_PREDEPLOY_ADDRESS, HISTORY_STORAGE_ADDRESS, - SYSTEM_ADDRESS, SYSTEM_CONTRACTS_WITH_CODE, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS, + PRAGUE_SYSTEM_CONTRACTS, SYSTEM_ADDRESS, WITHDRAWAL_REQUEST_PREDEPLOY_ADDRESS, }; use crate::{EvmError, ExecutionResult}; use bytes::Bytes; @@ -383,7 +383,7 @@ pub fn generic_system_contract_levm( // This check is not necessary in practice, since contract deployment has succesfully happened in all relevant testnets and mainnet // However, it's necessary to pass some of the Hive tests related to system contract deployment, which is why we have it - if SYSTEM_CONTRACTS_WITH_CODE + if PRAGUE_SYSTEM_CONTRACTS .iter() .any(|contract| contract.address == contract_address) && db.get_account_code(contract_address)?.is_empty() diff --git a/crates/vm/system_contracts.rs b/crates/vm/system_contracts.rs index f7270a7221..411d04298a 100644 --- a/crates/vm/system_contracts.rs +++ b/crates/vm/system_contracts.rs @@ -70,8 +70,7 @@ pub fn system_contracts_for_fork(fork: Fork) -> impl Iterator Date: Mon, 13 Oct 2025 12:00:47 -0300 Subject: [PATCH 10/20] Justified clones --- crates/blockchain/blockchain.rs | 12 ++--- crates/blockchain/metrics/l2/metrics.rs | 1 + crates/blockchain/metrics/metrics_blocks.rs | 1 + .../metrics/metrics_transactions.rs | 5 +- crates/blockchain/metrics/profiling.rs | 2 +- crates/blockchain/payload.rs | 11 ++-- crates/blockchain/smoke_test.rs | 51 +++++++++---------- crates/blockchain/tracing.rs | 4 +- 8 files changed, 42 insertions(+), 45 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index 28b7e6a3ba..5ba2fc9ab6 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -53,7 +53,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, @@ -148,7 +148,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)?; @@ -217,11 +217,11 @@ 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::L2 => Evm::new_from_db_for_l2(logger.clone()), + BlockchainType::L1 => Evm::new_from_db_for_l1(logger.clone()), // ok-clone: increase arc reference count + BlockchainType::L2 => Evm::new_from_db_for_l2(logger.clone()), // ok-clone: increase arc reference count }; // Re-execute block with logger @@ -536,7 +536,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, ); diff --git a/crates/blockchain/metrics/l2/metrics.rs b/crates/blockchain/metrics/l2/metrics.rs index 3c20d33d49..4c5759fccb 100644 --- a/crates/blockchain/metrics/l2/metrics.rs +++ b/crates/blockchain/metrics/l2/metrics.rs @@ -240,6 +240,7 @@ impl Metrics { pub fn gather_metrics(&self) -> Result { 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())) diff --git a/crates/blockchain/metrics/metrics_blocks.rs b/crates/blockchain/metrics/metrics_blocks.rs index 2dead6b192..bfb2c34d6d 100644 --- a/crates/blockchain/metrics/metrics_blocks.rs +++ b/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())) diff --git a/crates/blockchain/metrics/metrics_transactions.rs b/crates/blockchain/metrics/metrics_transactions.rs index a71a051ae3..a0c2e90bdd 100644 --- a/crates/blockchain/metrics/metrics_transactions.rs +++ b/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 { 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())) diff --git a/crates/blockchain/metrics/profiling.rs b/crates/blockchain/metrics/profiling.rs index cc45a0d6b0..636b673a2c 100644 --- a/crates/blockchain/metrics/profiling.rs +++ b/crates/blockchain/metrics/profiling.rs @@ -40,7 +40,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 } } } diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index bf9ae54ae5..0690f6a145 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -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 => Evm::new_for_l2(vm_db)?, @@ -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(), }) @@ -335,7 +335,7 @@ impl Blockchain { /// 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, 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 payload_build_task = tokio::task::spawn(async move { @@ -365,7 +365,7 @@ impl Blockchain { cancel_token: CancellationToken, ) -> Result { 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 let mut res = self_clone.build_payload(payload.clone()).await?; @@ -389,8 +389,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)?; diff --git a/crates/blockchain/smoke_test.rs b/crates/blockchain/smoke_test.rs index 5b3732849d..648ffc88b0 100644 --- a/crates/blockchain/smoke_test.rs +++ b/crates/blockchain/smoke_test.rs @@ -25,12 +25,12 @@ mod blockchain_integration_test { let genesis_hash = genesis_header.hash(); // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count // Add first block. We'll make it canonical. let block_1a = new_block(&store, &genesis_header).await; let hash_1a = block_1a.hash(); - blockchain.add_block(block_1a.clone()).await.unwrap(); + blockchain.add_block(block_1a.clone()).await.unwrap(); // ok-clone: clone used in test store .forkchoice_update(None, 1, hash_1a, None, None) .await @@ -44,7 +44,7 @@ mod blockchain_integration_test { let block_1b = new_block(&store, &genesis_header).await; let hash_1b = block_1b.hash(); blockchain - .add_block(block_1b.clone()) + .add_block(block_1b.clone()) // ok-clone: clone used in test .await .expect("Could not add block 1b."); let retrieved_1b = store.get_block_header_by_hash(hash_1b).unwrap().unwrap(); @@ -56,7 +56,7 @@ mod blockchain_integration_test { let block_2 = new_block(&store, &block_1b.header).await; let hash_2 = block_2.hash(); blockchain - .add_block(block_2.clone()) + .add_block(block_2) .await .expect("Could not add block 2."); let retrieved_2 = store.get_block_header_by_hash(hash_2).unwrap(); @@ -65,14 +65,9 @@ mod blockchain_integration_test { assert!(store.get_canonical_block_hash(2).await.unwrap().is_none()); // Receive block 2 as new head. - apply_fork_choice( - &store, - block_2.hash(), - genesis_header.hash(), - genesis_header.hash(), - ) - .await - .unwrap(); + apply_fork_choice(&store, hash_2, genesis_header.hash(), genesis_header.hash()) + .await + .unwrap(); // Check that canonical blocks changed to the new branch. assert!(is_canonical(&store, 0, genesis_hash).await.unwrap()); @@ -87,12 +82,12 @@ mod blockchain_integration_test { let genesis_header = store.get_block_header(0).unwrap().unwrap(); // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count // Build a single valid block. let block_1 = new_block(&store, &genesis_header).await; let hash_1 = block_1.hash(); - blockchain.add_block(block_1.clone()).await.unwrap(); + blockchain.add_block(block_1.clone()).await.unwrap(); // ok-clone: clone used in test apply_fork_choice(&store, hash_1, H256::zero(), H256::zero()) .await .unwrap(); @@ -101,7 +96,7 @@ mod blockchain_integration_test { let mut block_2 = new_block(&store, &block_1.header).await; block_2.header.parent_hash = H256::random(); let hash_2 = block_2.hash(); - let result = blockchain.add_block(block_2.clone()).await; + let result = blockchain.add_block(block_2).await; assert!(matches!(result, Err(ChainError::ParentNotFound))); // block 2 should now be pending. @@ -122,12 +117,12 @@ mod blockchain_integration_test { let genesis_hash = genesis_header.hash(); // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count // Add first block. Not canonical. let block_1a = new_block(&store, &genesis_header).await; let hash_1a = block_1a.hash(); - blockchain.add_block(block_1a.clone()).await.unwrap(); + blockchain.add_block(block_1a).await.unwrap(); let retrieved_1a = store.get_block_header_by_hash(hash_1a).unwrap().unwrap(); assert!(!is_canonical(&store, 1, hash_1a).await.unwrap()); @@ -136,7 +131,7 @@ mod blockchain_integration_test { let block_1b = new_block(&store, &genesis_header).await; let hash_1b = block_1b.hash(); blockchain - .add_block(block_1b.clone()) + .add_block(block_1b.clone()) // ok-clone: clone used in test .await .expect("Could not add block 1b."); apply_fork_choice(&store, hash_1b, genesis_hash, genesis_hash) @@ -153,7 +148,7 @@ mod blockchain_integration_test { let block_2 = new_block(&store, &block_1b.header).await; let hash_2 = block_2.hash(); blockchain - .add_block(block_2.clone()) + .add_block(block_2) .await .expect("Could not add block 2."); apply_fork_choice(&store, hash_2, genesis_hash, genesis_hash) @@ -172,7 +167,7 @@ mod blockchain_integration_test { // Receive block 1a as new head. apply_fork_choice( &store, - block_1a.hash(), + hash_1a, genesis_header.hash(), genesis_header.hash(), ) @@ -194,13 +189,13 @@ mod blockchain_integration_test { let genesis_hash = genesis_header.hash(); // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count // Add block at height 1. let block_1 = new_block(&store, &genesis_header).await; let hash_1 = block_1.hash(); blockchain - .add_block(block_1.clone()) + .add_block(block_1.clone()) // ok-clone: clone used in test .await .expect("Could not add block 1b."); @@ -208,7 +203,7 @@ mod blockchain_integration_test { let block_2 = new_block(&store, &block_1.header).await; let hash_2 = block_2.hash(); blockchain - .add_block(block_2.clone()) + .add_block(block_2) .await .expect("Could not add block 2."); @@ -247,12 +242,12 @@ mod blockchain_integration_test { let genesis_hash = genesis_header.hash(); // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count // Add block at height 1. let block_1 = new_block(&store, &genesis_header).await; blockchain - .add_block(block_1.clone()) + .add_block(block_1.clone()) // ok-clone: clone used in test .await .expect("Could not add block 1b."); @@ -260,7 +255,7 @@ mod blockchain_integration_test { let block_2 = new_block(&store, &block_1.header).await; let hash_2 = block_2.hash(); blockchain - .add_block(block_2.clone()) + .add_block(block_2) .await .expect("Could not add block 2."); @@ -280,7 +275,7 @@ mod blockchain_integration_test { let block_1b = new_block(&store, &genesis_header).await; let hash_b = block_1b.hash(); blockchain - .add_block(block_1b.clone()) + .add_block(block_1b) .await .expect("Could not add block b."); @@ -310,7 +305,7 @@ mod blockchain_integration_test { }; // Create blockchain - let blockchain = Blockchain::default_with_store(store.clone().clone()); + let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count let block = create_payload(&args, store, Bytes::new()).unwrap(); let result = blockchain.build_payload(block).await.unwrap(); diff --git a/crates/blockchain/tracing.rs b/crates/blockchain/tracing.rs index b10e94533d..75c7c088fe 100644 --- a/crates/blockchain/tracing.rs +++ b/crates/blockchain/tracing.rs @@ -67,7 +67,7 @@ impl Blockchain { let block = Arc::new(block); let mut call_traces = vec![]; for index in 0..block.body.transactions.len() { - // We are cloning the `Arc`s here, not the structs themselves + // ok-clone: We are cloning the `Arc`s here, not the structs themselves let block = block.clone(); let vm = vm.clone(); let tx_hash = block.as_ref().body.transactions[index].hash(); @@ -103,7 +103,7 @@ impl Blockchain { .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 parent_hash, block_hash_cache, ); From 65f12a75390fa0b9db5e415e802a58d2a654db1e Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 13 Oct 2025 12:23:57 -0300 Subject: [PATCH 11/20] Removed clones --- crates/blockchain/blockchain.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index 5ba2fc9ab6..a9ed754101 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -185,13 +185,12 @@ impl Blockchain { &self, blocks: &[Block], ) -> Result { - 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 @@ -241,13 +240,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() { @@ -523,7 +519,7 @@ impl Blockchain { ) -> Result<(), (ChainError, Option)> { 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)); }; @@ -555,7 +551,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 { @@ -566,7 +562,7 @@ 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 From eedd31f2961ae7812f7efebe6e49278f5365ac71 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 13 Oct 2025 12:40:46 -0300 Subject: [PATCH 12/20] Removed clone from get payload --- crates/blockchain/blockchain.rs | 2 +- crates/blockchain/payload.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index a9ed754101..a84a0bc9fe 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -411,7 +411,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()) diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 0690f6a145..18890307ac 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -280,7 +280,7 @@ impl PayloadBuildContext { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Default)] pub struct PayloadBuildResult { pub blobs_bundle: BlobsBundle, pub block_value: U256, @@ -316,7 +316,7 @@ impl From for PayloadBuildResult { impl Blockchain { /// Attempts to fetch a payload given it's id. If the payload is still being built, it will be finished. /// Fails if there is no payload or active payload build task for the given id. - pub async fn get_payload(&self, payload_id: u64) -> Result { + pub async fn get_payload(&mut self, payload_id: u64) -> Result { let mut payloads = self.payloads.lock().await; // Find the given payload and finish the active build process if needed let idx = payloads @@ -326,8 +326,8 @@ impl Blockchain { let finished_payload = (payload_id, payloads.remove(idx).1.to_payload().await?); payloads.insert(idx, finished_payload); // Return the held payload - match &payloads[idx].1 { - PayloadOrTask::Payload(payload) => Ok(*payload.clone()), + match &mut payloads[idx].1 { + PayloadOrTask::Payload(payload) => Ok(std::mem::take(payload)), _ => unreachable!("we already converted the payload into a finished version"), } } @@ -337,7 +337,7 @@ impl Blockchain { pub async fn initiate_payload_build(self: Arc, payload: Block, payload_id: u64) { 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) From 6597ab5a066feddb15b9e1bab7b5f5552de769c1 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 13 Oct 2025 15:41:42 -0300 Subject: [PATCH 13/20] Removed/justified more clones --- crates/blockchain/mempool.rs | 6 +++--- crates/blockchain/payload.rs | 4 ++-- crates/blockchain/smoke_test.rs | 2 +- crates/l2/sequencer/block_producer.rs | 2 +- crates/networking/rpc/engine/fork_choice.rs | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/blockchain/mempool.rs b/crates/blockchain/mempool.rs index cc5055f2ec..131d5a1244 100644 --- a/crates/blockchain/mempool.rs +++ b/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::>(); @@ -222,7 +222,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()); @@ -244,7 +244,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 } } diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 18890307ac..217ec7b2b1 100644 --- a/crates/blockchain/payload.rs +++ b/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 { @@ -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 diff --git a/crates/blockchain/smoke_test.rs b/crates/blockchain/smoke_test.rs index 648ffc88b0..565d7ecf3c 100644 --- a/crates/blockchain/smoke_test.rs +++ b/crates/blockchain/smoke_test.rs @@ -307,7 +307,7 @@ mod blockchain_integration_test { // Create blockchain let blockchain = Blockchain::default_with_store(store.clone()); // ok-clone: store struct fields are all arcs, so this just increases their reference count - let block = create_payload(&args, store, Bytes::new()).unwrap(); + let block = create_payload(args, store, Bytes::new()).unwrap(); let result = blockchain.build_payload(block).await.unwrap(); result.payload } diff --git a/crates/l2/sequencer/block_producer.rs b/crates/l2/sequencer/block_producer.rs index 2a7be56c72..6b2bb2b764 100644 --- a/crates/l2/sequencer/block_producer.rs +++ b/crates/l2/sequencer/block_producer.rs @@ -151,7 +151,7 @@ impl BlockProducer { elasticity_multiplier: self.elasticity_multiplier, gas_ceil: self.block_gas_limit, }; - let payload = create_payload(&args, &self.store, Bytes::new())?; + let payload = create_payload(args, &self.store, Bytes::new())?; // Blockchain builds the payload from mempool txs and executes them let payload_build_result = build_payload( diff --git a/crates/networking/rpc/engine/fork_choice.rs b/crates/networking/rpc/engine/fork_choice.rs index adfbcdd935..b29e4ffb18 100644 --- a/crates/networking/rpc/engine/fork_choice.rs +++ b/crates/networking/rpc/engine/fork_choice.rs @@ -379,7 +379,7 @@ async fn build_payload( let payload_id = args .id() .map_err(|error| RpcErr::Internal(error.to_string()))?; - let payload = match create_payload(&args, &context.storage, context.node_data.extra_data) { + let payload = match create_payload(args, &context.storage, context.node_data.extra_data) { Ok(payload) => payload, Err(ChainError::EvmError(error)) => return Err(error.into()), // Parent block is guaranteed to be present at this point, From 1bd9e29248924d40d0bc18cd31fbdc0f6ddd0da5 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 13 Oct 2025 15:59:22 -0300 Subject: [PATCH 14/20] Removed more clones --- cmd/ethrex/bench/build_block_benchmark.rs | 2 +- crates/blockchain/fork_choice.rs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cmd/ethrex/bench/build_block_benchmark.rs b/cmd/ethrex/bench/build_block_benchmark.rs index 63897dc2f7..698c347e81 100644 --- a/cmd/ethrex/bench/build_block_benchmark.rs +++ b/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()) } diff --git a/crates/blockchain/fork_choice.rs b/crates/blockchain/fork_choice.rs index 2e5bbc03c6..e81de62821 100644 --- a/crates/blockchain/fork_choice.rs +++ b/crates/blockchain/fork_choice.rs @@ -157,11 +157,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) { @@ -176,7 +175,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) From 9b1ed00e8dbcd0a3ee4f5cd1c51538945bc02daa Mon Sep 17 00:00:00 2001 From: SDartayet Date: Mon, 13 Oct 2025 16:38:16 -0300 Subject: [PATCH 15/20] Undid previous change --- crates/blockchain/payload.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 217ec7b2b1..d9d9105519 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -316,7 +316,7 @@ impl From for PayloadBuildResult { impl Blockchain { /// Attempts to fetch a payload given it's id. If the payload is still being built, it will be finished. /// Fails if there is no payload or active payload build task for the given id. - pub async fn get_payload(&mut self, payload_id: u64) -> Result { + pub async fn get_payload(&self, payload_id: u64) -> Result { let mut payloads = self.payloads.lock().await; // Find the given payload and finish the active build process if needed let idx = payloads @@ -326,8 +326,8 @@ impl Blockchain { let finished_payload = (payload_id, payloads.remove(idx).1.to_payload().await?); payloads.insert(idx, finished_payload); // Return the held payload - match &mut payloads[idx].1 { - PayloadOrTask::Payload(payload) => Ok(std::mem::take(payload)), + match &payloads[idx].1 { + 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"), } } From af22cb9f8f0bb2336fc25c807fbe3919b0e28470 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 14 Oct 2025 11:11:05 -0300 Subject: [PATCH 16/20] Addressed lints, justified clone --- crates/blockchain/blockchain.rs | 2 +- crates/blockchain/mempool.rs | 2 +- crates/l2/sequencer/block_producer/payload_builder.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index a84a0bc9fe..51febc3871 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -566,7 +566,7 @@ impl Blockchain { }; 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, diff --git a/crates/blockchain/mempool.rs b/crates/blockchain/mempool.rs index 131d5a1244..cc6c5fd7f3 100644 --- a/crates/blockchain/mempool.rs +++ b/crates/blockchain/mempool.rs @@ -851,7 +851,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(); diff --git a/crates/l2/sequencer/block_producer/payload_builder.rs b/crates/l2/sequencer/block_producer/payload_builder.rs index a0e9d6b20d..3f78677f41 100644 --- a/crates/l2/sequencer/block_producer/payload_builder.rs +++ b/crates/l2/sequencer/block_producer/payload_builder.rs @@ -45,7 +45,7 @@ pub async fn build_payload( let gas_limit = payload.header.gas_limit; debug!("Building payload"); - let mut context = PayloadBuildContext::new(payload, store, blockchain.options.r#type.clone())?; + let mut context = PayloadBuildContext::new(payload, store, blockchain.options.r#type)?; fill_transactions( blockchain.clone(), From bc613efc0cd02d2ed8734bd070b3a0707e877882 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 14 Oct 2025 11:33:51 -0300 Subject: [PATCH 17/20] removed more clones --- crates/blockchain/payload.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index d9d9105519..245c0404a1 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -368,12 +368,11 @@ impl Blockchain { 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 - let mut res = self_clone.build_payload(payload.clone()).await?; + let mut res = self_clone.build_payload(payload).await?; while start.elapsed() < SECONDS_PER_SLOT && !cancel_token.is_cancelled() { - let payload = payload.clone(); // Cancel the current build process and return the previous payload if it is requested earlier if let Some(current_res) = cancel_token - .run_until_cancelled(self_clone.build_payload(payload)) + .run_until_cancelled(self_clone.build_payload(std::mem::take(&mut res.payload))) .await { res = current_res?; @@ -395,6 +394,7 @@ impl Blockchain { self.apply_system_operations(&mut context)?; } self.apply_withdrawals(&mut context)?; + context.payload.body.transactions.clear(); self.fill_transactions(&mut context)?; self.extract_requests(&mut context)?; self.finalize_payload(&mut context).await?; From 736fdd14c1bfb56d1b55a43aef73d2d521c7591d Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 14 Oct 2025 14:52:12 -0300 Subject: [PATCH 18/20] Added todo-clone --- crates/blockchain/payload.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 245c0404a1..46df180c7e 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -368,11 +368,13 @@ impl Blockchain { 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 - let mut res = self_clone.build_payload(payload).await?; + // 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(); // Cancel the current build process and return the previous payload if it is requested earlier if let Some(current_res) = cancel_token - .run_until_cancelled(self_clone.build_payload(std::mem::take(&mut res.payload))) + .run_until_cancelled(self_clone.build_payload(payload)) .await { res = current_res?; @@ -394,7 +396,6 @@ impl Blockchain { self.apply_system_operations(&mut context)?; } self.apply_withdrawals(&mut context)?; - context.payload.body.transactions.clear(); self.fill_transactions(&mut context)?; self.extract_requests(&mut context)?; self.finalize_payload(&mut context).await?; From 9e1da8140e7487f7562fce5121e30688054489c8 Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 14 Oct 2025 14:59:47 -0300 Subject: [PATCH 19/20] Justified clone --- crates/blockchain/blockchain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/blockchain/blockchain.rs b/crates/blockchain/blockchain.rs index 3dac0ce091..387d1b2373 100644 --- a/crates/blockchain/blockchain.rs +++ b/crates/blockchain/blockchain.rs @@ -139,7 +139,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); }; From 703af21510b92e4c3eba9edce0fccc30a0fc65fb Mon Sep 17 00:00:00 2001 From: SDartayet Date: Tue, 14 Oct 2025 15:40:30 -0300 Subject: [PATCH 20/20] added todo-cloe comments --- crates/blockchain/payload.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/blockchain/payload.rs b/crates/blockchain/payload.rs index 46df180c7e..7acc2449f0 100644 --- a/crates/blockchain/payload.rs +++ b/crates/blockchain/payload.rs @@ -707,6 +707,8 @@ impl std::ops::Deref for HeadTransaction { impl From 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 } }