refactor(l1): move mempool from Store to Blockchain #2089

LeanSerra · 2025-02-25T21:10:12Z

Motivation

The mempool is currently stored in the Store struct, it makes more sense to have it in the Blockchain struct

Description

Move all the functions related to mempool to a new struct called Mempool that implements all the functions related to the mempool.
To add new transactions to the mempool we need to do some validations, for this we need access to the store so some mempool functions are exposed by the blockchain struct
- Blockchain::add_transaction_to_pool,
- Blockchain::add_blob_transaction_to_pool
- Blockchain::remove_transaction_from_pool

Closes #2072

github-actions · 2025-02-25T21:28:51Z

| File                                                                           | Lines | Diff |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/ethrex.rs                           | 433   | +3   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/blockchain.rs                | 370   | +90  |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/mempool.rs                   | 571   | +66  |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/payload.rs                   | 552   | -17  |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/l2/proposer/l1_watcher.rs               | 245   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/l2/proposer/mod.rs                      | 103   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/discv4/server.rs         | 680   | +3   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/network.rs               | 173   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/rlpx/connection.rs       | 505   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/rlpx/eth/transactions.rs | 306   | +9   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/rlpx/handshake.rs        | 424   | +2   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/engine/fork_choice.rs    | 359   | +2   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/eth/account.rs           | 221   | -1   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/eth/filter.rs            | 584   | +12  |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/eth/gas_price.rs         | 133   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/eth/max_priority_fee.rs  | 118   | +4   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/rpc.rs                   | 635   | +13  |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/utils.rs                 | 292   | +3   |
+--------------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/storage/store.rs                        | 1116  | -153 |
+--------------------------------------------------------------------------------+-------+------+

Total lines added: +227
Total lines removed: 171
Total lines changed: 398

cmd/ethrex/ethrex.rs

mpaulucci · 2025-02-26T11:49:47Z

crates/blockchain/mempool.rs

    Ok(hash)
 }

 /// Add a transaction to the mempool
-pub fn add_transaction(transaction: Transaction, store: &Store) -> Result<H256, MempoolError> {
+pub fn add_transaction(


what is the difference between this object and add_transaction_to_pool ?

seems like this should be a method of blockchain struct.

The function add_transaction was calling add_transaction_to_pool at the end, but add_transaction also included transaction validations, for these validations you need to access the store, so I decided to leave a minimal interface with the mempool from the blockchain struct:

add_transaction_to_pool

add_blob_transaction_to_pool

remove_transaction_from_pool

validate_transaction -> this one needs access to the storage

Then the mempool struct implements functions to lock the mempool Mutex then insert the transaction/blob without doing any validity checks leaving a comment indicating this.

crates/l2/proposer/l1_watcher.rs

mpaulucci · 2025-02-26T15:09:40Z

cmd/ethrex/ethrex.rs

@@ -261,7 +261,7 @@ async fn main() {
        peer_table.clone(),
        sync_mode,
        cancel_token.clone(),
-        blockchain,
+        blockchain.clone(),


Can't we pass a blockchain reference instead of cloning? I would disable clone for a blockchain, since it's a singleton.

That would mean adding an Arc and then cloning that Arc right? What we are doing right now is technically the same as the only field we are really cloning is the EVM both the store and mempool are internally arcs so when we clone them we are only cloning the references. I will still change it to Arc if thats what makes more sense.

ok, leave it as is then.

This will be tackled in a separate PR, moving blockchain to an Arc

…functions, remove mempool_remove_transactions as it was only used once and the behaviour was incorrect

…ity checks, mempool don't

…nHashes

mpaulucci · 2025-02-27T16:22:53Z

crates/blockchain/blockchain.rs

+
+    */
+
+    pub fn validate_transaction(


Can be tackled in a follow up, but it seems this should call a function inside mempool

mpaulucci · 2025-02-27T16:52:46Z

crates/networking/p2p/rlpx/handshake.rs

@@ -57,7 +57,7 @@ where
        node,
        stream,
        codec,
-        context.storage,


let's pass both storage and blockchain since these are different entities and shouldn't be coupled together.

@chaja This should be for all places where the store was replaced by blockchain? Specially on the initial layers like rpc start on cmd/ethrex/ethrex.rs right? The idea is to distinguish the Blockchain having a reference to the store vs being the actual owner of the mempool at an arq level?

I'm a bit confused on when would be better to use the store separated from the blockchain struct and when to use the store reference inside the blockchain struct, as far as I understand most of this PR leaned to use the store ref inside blockchain replacing the previous isolated ones.

My comment is about being more future proof. A store might be divided into different stores, like p2p store, state store, history store, etc.. Then relying in the store from blockchain wont be the solution inside every rpc/p2p call.

crates/networking/rpc/utils.rs

crates/networking/rpc/rpc.rs

crates/networking/rpc/engine/exchange_transition_config.rs

crates/networking/rpc/eth/account.rs

mpaulucci

When you remove the remaining context.blockchain.storage from both rpc and p2p

rodrigo-o · 2025-02-27T19:08:03Z

When you remove the remaining context.blockchain.storage from both rpc and p2p

We can create an issue to make the storage in blockchain private following this PR to avoid this kind of ambiguity in the future.

LeanSerra · 2025-02-27T19:24:11Z

When you remove the remaining context.blockchain.storage from both rpc and p2p

We can create an issue to make the storage in blockchain private following this PR to avoid this kind of ambiguity in the future.

Left an issue here #2105

rodrigo-o

Great work, it's almost there, i have a couple of comments in line with previous request from @chaja, still reviewing the mempool and blockchain file, will finish ASAP

crates/networking/p2p/rlpx/eth/transactions.rs

rodrigo-o · 2025-02-27T19:55:16Z

crates/networking/p2p/rlpx/eth/transactions.rs

            return Ok(None);
        };
        let result = match tx {
            Transaction::LegacyTransaction(itx) => P2PTransaction::LegacyTransaction(itx),
            Transaction::EIP2930Transaction(itx) => P2PTransaction::EIP2930Transaction(itx),
            Transaction::EIP1559Transaction(itx) => P2PTransaction::EIP1559Transaction(itx),
            Transaction::EIP4844Transaction(itx) => {
-                let Some(bundle) = store.get_blobs_bundle_from_pool(*hash)? else {
+                let Some(bundle) = mempool.get_blobs_bundle(*hash)? else {


In the future we'll need a consistent interface through blockchain/mempool, right now in the following changes we useblockchain.add_blob_transaction_to_pool or blockchain.add_transaction_to_pool instead of using internal mempool functions as done here. We might want to go one way or the other in a future PR.

I agree, the only reason why I left some functions that access the mempool as members of Blockchain is because to add transactions we perform some validations that use the Store. That was done before me knowing that the Store and Blockchain should be completely separate entities.
Knowing that I think they should be removed from blockchain and be functions of mempool that receive a store like add_transaction_to_pool(self, tx, store)

Yeah, I think both ways have their pros and cons but clearly something we could tackle in another PR, so we could go ahead with it for now and discuss the next step later!

rodrigo-o

Great work @LeanSerra 🚀. Left some minor las comment (all pointing to the same thing)

crates/networking/rpc/rpc.rs

LeanSerra added 2 commits February 25, 2025 18:01

add hex-literal to blockchain only for a test could be removed

878d920

move mempool functions from Store to new struct Mempool

7b5fbc8

LeanSerra added tech debt Refactors, cleanups, etc L1 Ethereum client labels Feb 25, 2025

LeanSerra self-assigned this Feb 25, 2025

LeanSerra changed the title ~~refactor(l1,l2): move mempool from Store to Blockchain~~ refactor(l1): move mempool from Store to Blockchain Feb 25, 2025

Merge branch 'main' into l1/move_mempool_from_store

c4fa7db

mpaulucci reviewed Feb 26, 2025

View reviewed changes

cmd/ethrex/ethrex.rs Outdated Show resolved Hide resolved

mpaulucci reviewed Feb 26, 2025

View reviewed changes

cmd/ethrex/ethrex.rs Outdated Show resolved Hide resolved

mpaulucci reviewed Feb 26, 2025

View reviewed changes

crates/l2/proposer/l1_watcher.rs Outdated Show resolved Hide resolved

LeanSerra added 3 commits February 26, 2025 09:39

fix missing argument for l2

08a4a49

remove arc from mempool

c05fc05

pass blockchain as argument when using both store and mempool

52271d6

mpaulucci reviewed Feb 26, 2025

View reviewed changes

LeanSerra added 7 commits February 26, 2025 15:50

move basic mempool operations to blockchain struct, remove duplicate …

906f68d

…functions, remove mempool_remove_transactions as it was only used once and the behaviour was incorrect

Merge branch 'main' into l1/move_mempool_from_store

3df7183

remove hex-literal dependecy, change test to use hex::decode

065e90f

move filter transaction functions to mempool struct

99f43c4

add comment indicating that blockchain add_tx functions perform valid…

f02ad2d

…ity checks, mempool don't

move get_nonce function to mempool

e375c39

fix using wrong filter function when broadcasting NewPooledTransactio…

d240452

…nHashes

jrchatruc marked this pull request as ready for review February 26, 2025 20:44

jrchatruc requested a review from a team as a code owner February 26, 2025 20:44

mpaulucci reviewed Feb 27, 2025

View reviewed changes

crates/networking/rpc/utils.rs Show resolved Hide resolved

mpaulucci reviewed Feb 27, 2025

View reviewed changes

crates/networking/rpc/rpc.rs Show resolved Hide resolved

pass both blockchain and store when needed for l2 stuff

35db6fe

LeanSerra added 3 commits February 27, 2025 14:39

pass both blockchain and store when needed for connection.rs

fea985e

pass both blockchain and store when needed

8e61405

add missing parameter for ethrex_p2p::start_network in dev mode

43d02f1

mpaulucci reviewed Feb 27, 2025

View reviewed changes

crates/networking/rpc/engine/exchange_transition_config.rs Outdated Show resolved Hide resolved

mpaulucci reviewed Feb 27, 2025

View reviewed changes

crates/networking/rpc/eth/account.rs Outdated Show resolved Hide resolved

mpaulucci approved these changes Feb 27, 2025

View reviewed changes

LeanSerra added 3 commits February 27, 2025 16:04

change .blockchain.storage to .storage

539af5c

change .blockchain.storage to .storage

7377508

change .blockchain.storage to .storage

308fab7

change .blockchain.storage to .storage

c3e7bdf

Merge branch 'main' into l1/move_mempool_from_store

482e796

rodrigo-o reviewed Feb 27, 2025

View reviewed changes

LeanSerra and others added 3 commits February 27, 2025 17:26

pass blockchain as argument instead of mempool

f6c0715

fix function call arguments

36fa3aa

Merge branch 'main' into l1/move_mempool_from_store

6fd0f21

rodrigo-o approved these changes Feb 27, 2025

View reviewed changes

crates/networking/rpc/rpc.rs Outdated Show resolved Hide resolved

crates/networking/rpc/rpc.rs Outdated Show resolved Hide resolved

crates/networking/rpc/rpc.rs Outdated Show resolved Hide resolved

crates/networking/rpc/rpc.rs Outdated Show resolved Hide resolved

change blockchain.storage to storage for tests

a19887a

LeanSerra added this pull request to the merge queue Feb 27, 2025

Merged via the queue into main with commit 1c62317 Feb 27, 2025
24 checks passed

LeanSerra deleted the l1/move_mempool_from_store branch February 27, 2025 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(l1): move mempool from Store to Blockchain #2089

refactor(l1): move mempool from Store to Blockchain #2089

LeanSerra commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

mpaulucci Feb 26, 2025

mpaulucci Feb 26, 2025

LeanSerra Feb 26, 2025

mpaulucci Feb 26, 2025

LeanSerra Feb 26, 2025

mpaulucci Feb 26, 2025

mpaulucci Feb 27, 2025

mpaulucci Feb 27, 2025

mpaulucci Feb 27, 2025

rodrigo-o Feb 27, 2025

mpaulucci Feb 27, 2025

mpaulucci left a comment

rodrigo-o commented Feb 27, 2025

LeanSerra commented Feb 27, 2025

rodrigo-o left a comment

rodrigo-o Feb 27, 2025

LeanSerra Feb 27, 2025

rodrigo-o Feb 27, 2025

rodrigo-o left a comment

refactor(l1): move mempool from Store to Blockchain #2089

refactor(l1): move mempool from Store to Blockchain #2089

Conversation

LeanSerra commented Feb 25, 2025 • edited Loading

github-actions bot commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpaulucci left a comment

Choose a reason for hiding this comment

rodrigo-o commented Feb 27, 2025

LeanSerra commented Feb 27, 2025

rodrigo-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigo-o left a comment

Choose a reason for hiding this comment

LeanSerra commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading