feat(l1, l2): make some store getters async #2430

iovoid · 2025-04-09T15:53:57Z

Motivation

Like with #2336 the goal is to avoid blocking the current task.

Description

Makes store getters not related to tries (and thus the EVM) async, and propagates the changes to users of store. They are made async by using spawn_blocking

Many instances of functional code (and_then, map) had to be replaced due to bad async support.

Closes #2424

github-actions · 2025-04-09T15:54:58Z

Lines of code report

Total lines added: 394
Total lines removed: 3
Total lines changed: 397

Detailed view

+--------------------------------------------------------+-------+------+
| File                                                   | Lines | Diff |
+--------------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/blockchain/test_runner.rs          | 147   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/ethrex.rs                            | 82    | +1   |
+--------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                      | 340   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2.rs                                | 250   | +1   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs                 | 510   | +3   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/fork_choice.rs                | 142   | +1   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs                    | 571   | -3   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/smoke_test.rs                 | 235   | +6   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_committer.rs             | 405   | +5   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_watcher.rs               | 247   | +1   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/prover_server.rs            | 472   | +4   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/connection.rs        | 535   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/eth/transactions.rs  | 309   | +3   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync.rs                   | 568   | +1   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync/trie_rebuild.rs      | 244   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/sync_manager.rs           | 129   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/fork_choice.rs     | 390   | +14  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs         | 683   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/account.rs            | 233   | +12  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/block.rs              | 356   | +4   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/fee_market.rs         | 234   | +2   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/filter.rs             | 605   | +3   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/logs.rs               | 177   | +4   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/transaction.rs        | 602   | +17  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/l2/transaction.rs         | 216   | +5   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/types/block_identifier.rs | 140   | +8   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/storage/api.rs                           | 251   | +20  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/storage/store.rs                         | 1248  | +29  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/storage/store_db/in_memory.rs            | 609   | +37  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/storage/store_db/libmdbx.rs              | 1366  | +87  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/storage/store_db/redb.rs                 | 1218  | +114 |
+--------------------------------------------------------+-------+------+

mpaulucci

LGTM

fkrause98 · 2025-04-11T14:48:39Z

crates/networking/rpc/engine/fork_choice.rs

                InvalidForkChoice::NewHeadAlreadyCanonical => {
                    ForkChoiceResponse::from(PayloadStatus::valid_with_hash(
-                        latest_canonical_block_hash(&context.storage).unwrap(),
+                        latest_canonical_block_hash(&context.storage).await.unwrap(),


Since this function returns a result, let's leverage this PR to remove this unwrap

Oppen · 2025-04-11T15:53:15Z

crates/networking/rpc/engine/payload.rs

+        let mut bodies = Vec::new();
+        for hash in self.hashes.iter() {
+            bodies.push(context.storage.get_block_body_by_hash(*hash).await?)
+        }


It might make sense to do a single request to the DB for the bulk.

Implemented

Oppen · 2025-04-11T15:53:52Z

crates/networking/rpc/engine/payload.rs

+        for block_num in self.start..=last {
+            bodies.push(context.storage.get_block_body(block_num).await?)
+        }


Implemented!

Arkenan · 2025-04-11T16:10:01Z

crates/networking/rpc/eth/filter.rs

-        });
+        let latest_block_num = storage.get_latest_block_number().await?;
+        // Box needed to keep the future Sync
+        // https://github.com/rust-lang/rust/issues/128095


It's not very clear to me why we need to use box. There is no mention to it in that github issue.

It doesn't have to be Box (anything that can keep the variable Sync works). The issue explains why we need to keep the variable Sync even though it's dropped before the next await

Arkenan · 2025-04-11T16:15:09Z

crates/storage/store_db/libmdbx.rs

    }

-    fn get_receipts_for_block(&self, block_hash: &BlockHash) -> Result<Vec<Receipt>, StoreError> {
+    async fn get_receipts_for_block(


Why is this async if it doesn't go through the async api?

Arkenan · 2025-04-11T16:15:44Z

crates/storage/store_db/libmdbx.rs

    }

-    fn read_account_snapshot(&self, start: H256) -> Result<Vec<(H256, AccountState)>, StoreError> {
+    async fn read_account_snapshot(


Same here, how is this async?

Arkenan · 2025-04-11T16:17:56Z

crates/storage/store_db/redb.rs

    }

-    fn get_finalized_block_number(&self) -> Result<Option<BlockNumber>, StoreError> {
+    async fn get_finalized_block_number(&self) -> Result<Option<BlockNumber>, StoreError> {


Is await missing here?

It wasn't missing but the function was sync. Made it async.

Arkenan · 2025-04-11T16:18:05Z

crates/storage/store_db/redb.rs

    }

-    fn get_safe_block_number(&self) -> Result<Option<BlockNumber>, StoreError> {
+    async fn get_safe_block_number(&self) -> Result<Option<BlockNumber>, StoreError> {


Is await missing here?

It wasn't missing but the function was sync. Made it async.

github-actions · 2025-04-11T20:41:11Z

Benchmark Block Execution Results Comparison Against Main

Command	Mean [s]	Min [s]	Max [s]	Relative
`base`	182.445 ± 0.932	180.999	184.686	1.00
`head`	183.186 ± 1.597	180.792	185.537	1.00 ± 0.01

Arkenan

LGTM

**Motivation** Like with lambdaclass#2336 the goal is to avoid blocking the current task. **Description** Makes store getters not related to tries (and thus the EVM) async, and propagates the changes to users of store. They are made async by using `spawn_blocking ` Many instances of functional code (`and_then`, `map`) had to be replaced due to bad async support. Closes lambdaclass#2424

…5408) **Motivation** When we added RPC metrics for success/error rates in #5335, we found that we were returning "Internal error" in the `engine_getPayloadBodiesByRange` method. This was related to changes done in #2430, which turned missing payloads into a DB error. The spec says we should return `null`s for any missing payloads, so this deviates from the spec. **Description** This PR fixes the spec deviation by changing the batch-GET methods from the Store API to return `None` on missing values instead of failing. Closes #5403 --------- Co-authored-by: Rodrigo Oliveri <[email protected]>

feat(l1,l2): make some store getters async

2b7ac79

iovoid requested a review from a team as a code owner April 9, 2025 15:53

iovoid added 10 commits April 9, 2025 14:48

fix: also add async to redb backend

45c3ee3

fix: also add async to l2 rpc

da38207

fix: also add async to cmd, re-fmt

5e82c74

Merge branch 'main' into store/async-getters

474442f

fix: delete debugging file

0515ea7

Merge branch 'main' into store/async-getters

dc46c00

Merge branch 'main' into store/async-getters

881df55

fix: integrate with sync_manager changes

6df74cd

fix: forgot await in l2 cmd

a589aa9

fix: fmt

062dc2c

Arkenan added the performance Block execution throughput and performance in general label Apr 11, 2025

iovoid added 2 commits April 11, 2025 12:17

update changelog

f152e56

Merge branch 'main' into store/async-getters

cb30616

mpaulucci approved these changes Apr 11, 2025

View reviewed changes

fkrause98 requested changes Apr 11, 2025

View reviewed changes

Oppen reviewed Apr 11, 2025

View reviewed changes

Arkenan reviewed Apr 11, 2025

View reviewed changes

iovoid added 7 commits April 11, 2025 13:45

fix: remove unneeded unwrap

5f03857

feat: use bulk read operations

bda37f8

remove unneeded async

0076b6a

feat: make bulk reads async

eba61ab

Merge branch 'main' into store/async-getters

9121595

add redb async read

9fe2084

fix: perf ci arguments

142d8c9

fkrause98 approved these changes Apr 15, 2025

View reviewed changes

Arkenan approved these changes Apr 15, 2025

View reviewed changes

iovoid added this pull request to the merge queue Apr 15, 2025

Merged via the queue into main with commit 5b5c66c Apr 15, 2025
31 checks passed

iovoid deleted the store/async-getters branch April 15, 2025 15:23

MegaRedHand mentioned this pull request Nov 22, 2025

fix(l1): handle missing payloads in engine_getPayloadBodiesByRange #5408

Merged

feat(l1, l2): make some store getters async #2430

feat(l1, l2): make some store getters async #2430

Uh oh!

Conversation

iovoid commented Apr 9, 2025

Uh oh!

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

Uh oh!

Lines of code report

Uh oh!

mpaulucci left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 11, 2025

Benchmark Block Execution Results Comparison Against Main

Uh oh!

Arkenan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Apr 9, 2025 •

edited

Loading