chore(l1): remove/justify clones from common #4769

SDartayet · 2025-10-03T21:36:26Z

Motivation

Cleaning up our code and improving it's performance. This PR is part of a broader campaign to remove clones from our code.

Description

This PR removes most of the .clone() calls from the types and utils of the common crate. Those clones that couldn't be removed are justified with an // ok-clone comment

Part of #4668

github-actions · 2025-10-03T21:39:49Z

Lines of code report

Total lines added: 56
Total lines removed: 56
Total lines changed: 112

Detailed view

+---------------------------------------------------------+-------+------+
| File                                                    | Lines | Diff |
+---------------------------------------------------------+-------+------+
| ethrex/crates/common/serde_utils.rs                     | 575   | -36  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/common/types/blobs_bundle.rs              | 369   | +1   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/common/types/transaction.rs               | 2821  | -16  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/eth/eth69/receipts.rs | 52    | +9   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs          | 695   | +8   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs                   | 459   | -2   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/environment.rs                | 92    | -1   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/hooks/default_hook.rs         | 349   | +2   |
+---------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state/runner/levm_runner.rs     | 478   | +23  |
+---------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state/runner/mod.rs             | 278   | +2   |
+---------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state/runner/revm_runner.rs     | 681   | +10  |
+---------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state_v2/src/modules/error.rs   | 10    | +1   |
+---------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state_v2/src/modules/runner.rs  | 220   | -1   |
+---------------------------------------------------------+-------+------+

github-actions · 2025-10-03T22:05:19Z

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Detailed Results

Benchmark Results: BubbleSort

Command	Mean [s]	Min [s]	Max [s]	Relative
`main_revm_BubbleSort`	4.468 ± 0.018	4.443	4.501	1.03 ± 0.01
`main_levm_BubbleSort`	4.491 ± 0.068	4.360	4.556	1.03 ± 0.02
`pr_revm_BubbleSort`	4.600 ± 0.088	4.451	4.729	1.06 ± 0.02
`pr_levm_BubbleSort`	4.346 ± 0.019	4.319	4.373	1.00

Benchmark Results: ERC20Approval

Command	Mean [s]	Min [s]	Max [s]	Relative
`main_revm_ERC20Approval`	1.457 ± 0.028	1.429	1.506	1.00
`main_levm_ERC20Approval`	1.521 ± 0.015	1.512	1.560	1.04 ± 0.02
`pr_revm_ERC20Approval`	1.502 ± 0.018	1.482	1.534	1.03 ± 0.02
`pr_levm_ERC20Approval`	1.583 ± 0.036	1.532	1.636	1.09 ± 0.03

Benchmark Results: ERC20Mint

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_ERC20Mint`	184.8 ± 2.5	182.1	189.6	1.01 ± 0.04
`main_levm_ERC20Mint`	192.3 ± 4.8	185.6	199.2	1.05 ± 0.05
`pr_revm_ERC20Mint`	182.5 ± 6.5	174.8	193.1	1.00
`pr_levm_ERC20Mint`	199.4 ± 2.1	193.9	201.5	1.09 ± 0.04

Benchmark Results: ERC20Transfer

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_ERC20Transfer`	345.6 ± 7.5	333.5	356.7	1.00 ± 0.03
`main_levm_ERC20Transfer`	374.5 ± 6.7	363.5	383.9	1.09 ± 0.03
`pr_revm_ERC20Transfer`	344.3 ± 6.6	334.9	355.3	1.00
`pr_levm_ERC20Transfer`	378.4 ± 10.4	361.4	391.9	1.10 ± 0.04

Benchmark Results: Factorial

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_Factorial`	225.4 ± 2.9	221.9	230.7	1.01 ± 0.02
`main_levm_Factorial`	273.1 ± 28.9	261.7	355.4	1.22 ± 0.13
`pr_revm_Factorial`	223.4 ± 1.7	221.7	226.3	1.00
`pr_levm_Factorial`	264.1 ± 1.9	260.9	266.3	1.18 ± 0.01

Benchmark Results: FactorialRecursive

Command	Mean [s]	Min [s]	Max [s]	Relative
`main_revm_FactorialRecursive`	1.572 ± 0.030	1.511	1.610	1.00
`main_levm_FactorialRecursive`	8.208 ± 0.188	7.947	8.502	5.22 ± 0.16
`pr_revm_FactorialRecursive`	1.609 ± 0.048	1.543	1.701	1.02 ± 0.04
`pr_levm_FactorialRecursive`	8.428 ± 0.071	8.320	8.511	5.36 ± 0.11

Benchmark Results: Fibonacci

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_Fibonacci`	205.0 ± 6.7	199.4	221.4	1.01 ± 0.04
`main_levm_Fibonacci`	249.6 ± 2.3	246.8	254.9	1.23 ± 0.02
`pr_revm_Fibonacci`	202.2 ± 3.5	199.4	210.3	1.00
`pr_levm_Fibonacci`	254.1 ± 6.7	249.6	272.2	1.26 ± 0.04

Benchmark Results: FibonacciRecursive

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_FibonacciRecursive`	864.5 ± 22.9	824.5	906.1	1.00
`main_levm_FibonacciRecursive`	1027.5 ± 20.7	1001.2	1053.6	1.19 ± 0.04
`pr_revm_FibonacciRecursive`	894.6 ± 8.2	883.2	909.3	1.03 ± 0.03
`pr_levm_FibonacciRecursive`	1016.1 ± 24.7	987.9	1059.2	1.18 ± 0.04

Benchmark Results: ManyHashes

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_ManyHashes`	12.5 ± 0.0	12.4	12.5	1.03 ± 0.03
`main_levm_ManyHashes`	13.3 ± 0.4	12.8	13.6	1.10 ± 0.04
`pr_revm_ManyHashes`	12.1 ± 0.4	11.7	12.5	1.00
`pr_levm_ManyHashes`	13.7 ± 0.1	13.5	13.9	1.14 ± 0.04

Benchmark Results: MstoreBench

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_MstoreBench`	247.8 ± 6.8	239.7	259.5	1.03 ± 0.03
`main_levm_MstoreBench`	715.6 ± 15.1	699.6	735.7	2.96 ± 0.06
`pr_revm_MstoreBench`	241.6 ± 1.1	240.3	243.9	1.00
`pr_levm_MstoreBench`	706.0 ± 8.5	698.4	724.4	2.92 ± 0.04

Benchmark Results: Push

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_Push`	290.2 ± 8.5	275.4	299.1	1.03 ± 0.04
`main_levm_Push`	786.7 ± 14.2	773.6	816.4	2.79 ± 0.08
`pr_revm_Push`	282.2 ± 6.3	273.5	291.1	1.00
`pr_levm_Push`	774.2 ± 5.3	767.6	782.5	2.74 ± 0.06

Benchmark Results: SstoreBench_no_opt

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`main_revm_SstoreBench_no_opt`	208.4 ± 5.4	203.0	221.5	2.43 ± 0.07
`main_levm_SstoreBench_no_opt`	85.8 ± 1.0	84.4	87.4	1.00
`pr_revm_SstoreBench_no_opt`	208.8 ± 5.6	203.1	220.4	2.43 ± 0.07
`pr_levm_SstoreBench_no_opt`	90.7 ± 1.0	89.1	91.9	1.06 ± 0.02

crates/common/types/fork_id.rs

fmoletta · 2025-10-06T21:45:23Z

crates/networking/p2p/rlpx/eth/eth68/receipts.rs

    }

-    pub fn get_receipts(&self) -> Vec<Vec<Receipt>> {
+    pub fn get_receipts(&mut self) -> Vec<Vec<Receipt>> {


Maybe we should change the name to take_receipts? as to reflect the new behaviour

I would also suggest changing it into an into_receipts method that consumes the Receipts message as we don't seem to use the struct afterwards

Looking at this I also noticed that there is an overlooked clone at file peer_handler.rs just a few lines below where this method is called (Line 708) that could be removed similarly

Sure. Fixed here!

Co-authored-by: fmoletta <[email protected]>

fmoletta · 2025-10-06T21:58:54Z

crates/vm/levm/src/opcode_handlers/block.rs

+        let Some(blob_hashes) = self.tx.blob_versioned_hashes() else {
+            self.current_call_frame.stack.push_zero()?;
+            return Ok(OpcodeResult::Continue);
+        };

        let index = match u256_to_usize(index) {
            Ok(index) if index < blob_hashes.len() => index,


Maybe we could use unwrap_or_default(&vec![]) here and let the logic below handle the case of no blobs as to not repeat the same logic (early zero return) twice

That sadly wouldn't work since the vec![] got dropped (and we need a reference). I did this, let me know what you think

fmoletta · 2025-10-06T22:01:32Z

tooling/ef_tests/state/runner/levm_runner.rs

+    let tx = if authorization_list.is_some() {
+        Transaction::EIP7702Transaction(EIP7702Transaction {
            to: match test_tx.to {
                TxKind::Call(to) => to,
                TxKind::Create => return Err(EFTestRunnerError::EIP7702ShouldNotBeCreateType),
            },
            value: test_tx.value,
            data: test_tx.data.clone(),
-            access_list,
-            authorization_list: list,
+            access_list: access_list,
+            authorization_list: authorization_list.unwrap(),


Why was this change needed?

Ah, that was because I removed the blob hashes field from the environment (since it required cloning the blob hashes from the transaction). The way the runner worked was that for EIP4844 transactions, it encoded them as EIP1599 txs and then injected the blob parameters via the environment. Because this would cause tests to fail now the environment didn't have the blob hashes, I had to add some extra logic to build the test transaction as an EIP4844 one when appropriate

SDartayet added 2 commits October 3, 2025 18:34

Justified test clones

b12afd8

Removed unnecessary blobs clone, unused functions

20c0adc

github-actions bot assigned SDartayet Oct 3, 2025

SDartayet added 3 commits October 3, 2025 18:44

Removed clone when returning tx to field

f192c94

fixed issues with previous commit, removed clone when parsing

85aa0a7

Removed clone in from implementations

29daf3f

SDartayet and others added 6 commits October 6, 2025 10:18

Fix compilation errors

79d6b3b

Removed clone of blob hashes

49902dc

Fixing errors

13dcb52

Fixing errors and lints

b98469a

Update blobs_bundle.rs

cca95d8

Improving readability in tests

417a798

SDartayet changed the title ~~Remove clones from common~~ chore(l1): remove clones from common Oct 6, 2025

github-actions bot added the L1 Ethereum client label Oct 6, 2025

github-project-automation bot added this to ethrex_l1 Oct 6, 2025

SDartayet changed the title ~~chore(l1): remove clones from common~~ chore(l1): remove/justify clones from common Oct 6, 2025

SDartayet and others added 6 commits October 6, 2025 12:40

Fixing errors and lints

bcaa21c

Merge branch 'main' into remove-clones-from-common

0c3af9b

Changed back tx types, derived copy for TxKind

fffc5c9

Merge branch 'main' into remove-clones-from-common

3eb00f4

Fixed runner so tests wouldn't fail after blob hashes refactor

f3a04b8

Fixing lnts

7b0b9b3

SDartayet marked this pull request as ready for review October 6, 2025 21:24

SDartayet requested a review from a team as a code owner October 6, 2025 21:24

ethrex-project-sync bot moved this to In Review in ethrex_l1 Oct 6, 2025

fmoletta reviewed Oct 6, 2025

View reviewed changes

crates/common/types/fork_id.rs Outdated Show resolved Hide resolved

fmoletta reviewed Oct 6, 2025

View reviewed changes

Fix typo

6a82ac7

Co-authored-by: fmoletta <[email protected]>

fmoletta reviewed Oct 6, 2025

View reviewed changes

SDartayet added 4 commits October 6, 2025 19:03

Addressing review comments

c12a7ac

Fixing lints

e947343

Fixing review comments

5e2084e

Fixing lints

7c8e055

chore(l1): remove/justify clones from common #4769

Are you sure you want to change the base?

chore(l1): remove/justify clones from common #4769

Uh oh!

Conversation

SDartayet commented Oct 3, 2025 • edited by fmoletta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lines of code report

Uh oh!

github-actions bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark Results Comparison

Benchmark Results: BubbleSort

Benchmark Results: ERC20Approval

Benchmark Results: ERC20Mint

Benchmark Results: ERC20Transfer

Benchmark Results: Factorial

Benchmark Results: FactorialRecursive

Benchmark Results: Fibonacci

Benchmark Results: FibonacciRecursive

Benchmark Results: ManyHashes

Benchmark Results: MstoreBench

Benchmark Results: Push

Benchmark Results: SstoreBench_no_opt

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SDartayet commented Oct 3, 2025 •

edited by fmoletta

Loading

github-actions bot commented Oct 3, 2025 •

edited

Loading

github-actions bot commented Oct 3, 2025 •

edited

Loading