feat(l1,l2): make write path APIs async #2336
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lines of code reportTotal lines added: Detailed view |
Oppen
added
the
performance
Benchmark Block Execution Results Comparison Against Main
|
Oppen
force-pushed
the
feat/async_apis
branch
2 times, most recently
from
12e2648 to
29500f1
Compare
Oppen
commented
Apr 1, 2025
Comment on lines
221
to
228
| Box::pin(async { | ||
| Self::RemoveDB { | ||
| datadir: opts.datadir.clone(), | ||
| } | ||
| .run(opts) | ||
| .await | ||
| }) | ||
| .await?; |
Contributor
Author
There was a problem hiding this comment.
I believe this was done to propagate the prompt before deletion, so I'll restore it. Initially I had to come to a similar solution but abstracted away (just like the other commit) due to it looking a bit more complex.
Oppen
commented
Apr 1, 2025
| .await | ||
| .map_err(SyncError::JoinHandle) | ||
| }?; | ||
| Self::add_blocks(blockchain, blocks, sync_head_found).await |
Contributor
Author
There was a problem hiding this comment.
We might still need this spawn_blocking in case it hogs the CPU. But if that's the case, I believe the proper place is inside the call.
Oppen
commented
Apr 1, 2025
| )?; | ||
| let sync_head = fork_choice_state.head_block_hash; | ||
| tokio::spawn(async move { | ||
| // If we can't get hold of the syncer, then it means that there is an active sync in process |
Contributor
Author
There was a problem hiding this comment.
I need to restore the comment. It was lost during a period where this task would break the build.
Oppen
commented
Apr 1, 2025
crates/networking/rpc/rpc.rs
Outdated
| .route("/", post(handle_authrpc_request)) | ||
| .route( | ||
| "/", | ||
| post(|ctx, auth, body| async { handle_authrpc_request(ctx, auth, body).await }), |
Contributor
Author
There was a problem hiding this comment.
Maybe this lambda needs a name? This looks just ugly.
Arkenan
approved these changes
Apr 3, 2025
Collaborator
Arkenan
left a comment
Arkenan
left a comment
There was a problem hiding this comment.
LGTM with small comments
crates/blockchain/blockchain.rs
Outdated
|
|
||
| /// Executes a block withing a new vm instance and state | ||
| fn execute_block(&self, block: &Block) -> Result<BlockExecutionResult, ChainError> { | ||
| /// TODO: make the asyncness real |
crates/blockchain/blockchain.rs
Outdated
| } | ||
|
|
||
| pub fn store_block( | ||
| // TODO(PLT): review async |
Collaborator
There was a problem hiding this comment.
What does this mean? Can we start an issue if there's a concrete thing to review?
| let since = Instant::now(); | ||
| // Easiest why to operate on the result of `execute_block` without | ||
| // having to add too much control flow or return early | ||
| // Async doesn't play well with `.and_then` |
Collaborator
There was a problem hiding this comment.
Why doesn't await.and_then work well?
Contributor
Author
There was a problem hiding this comment.
The result of the lambda and_then receives needs to be an Option, but then because that lambda can't be async, I can't await inside it, so the result is a Future.
crates/blockchain/blockchain.rs
Outdated
|
|
||
| /// Add a transaction to the mempool checking that the transaction is valid | ||
| pub fn add_transaction_to_pool(&self, transaction: Transaction) -> Result<H256, MempoolError> { | ||
| pub async fn add_transaction_to_pool( |
Collaborator
There was a problem hiding this comment.
Why is this one async? The mempool is in memory. Or at least it's not async itself.
Contributor
Author
There was a problem hiding this comment.
I think I added that one by mistake during one of the first iterations. The version for blobs also has the same. I'll remove them in a later iteration.
Benchmark for b8e385cClick to view benchmark
|
Oppen
commented
Apr 3, 2025
Benchmark for 6b7d6b9Click to view benchmark
|
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 15, 2025
**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
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
**Motivation** Some of our sync APIs can produce starving when running on Tokio due to taking a long time to reach the next `await`-point. Specifically, writing to the DB tends to take a long time, which blocks other tasks, sometimes the whole runtime due to how the scheduler in Tokio works. Thus, we need a way to inform the runtime we're going to be working for a while, and give it control while we wait for stuff. **Description** Take the mutable APIs for the DB and mark them `async`. Then bubble that up to their users. Then make the functions non-blocking by using `spawn_blocking` to run on the blocking thread, releasing the runtime to handle more work. The DB writing APIs had to change to pass-by-value to satisfy the borrow-checker in the blocking task context. I think I can use proper lifetime bounds with a helper crate, if that's preferred. The values were already being discarded after passing to the DB, so passing by value should not be a problem either way. Special considerations: - For some work performed before benchmarks and EF tests which are inherently synchronous I opted for calling with an ad-hoc runtime instance and `block_on`, as that might reduce the changes needed by localizing the async work. If desired, that can be changed up to making a `tokio::main`. The same is true for some setup functions for tests. - For the DBs I had to separate the Tokio import. This is because they need to compile with L2, which means provers' custom compilers, and those don't support the networking functions in the stdlib, which Tokio with full features (as the workspace dep declares) brings them in. - The InMemoryDB was left untouched other than updating the interfaces, given hashmap access should be quick enough. - I need to comment on [this hack](https://github.com/lambdaclass/ethrex/pull/2336/files#diff-264636d3ee6ee67bd6e136b8c98f74152de6a8e2a07f597cfb5f622d4e0d815aR143-R146): `and_then` can't be used on futures and everything became a mess without that little helper. - I'm unsure about whether or not we also want to cover the read APIs, at least for consistency I would think so, but for now I left them out. closes lambdaclass#2402
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
**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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Some of our sync APIs can produce starving when running on Tokio due to taking a long time to reach the next
await-point.Specifically, writing to the DB tends to take a long time, which blocks other tasks, sometimes the whole runtime due to how the scheduler in Tokio works.
Thus, we need a way to inform the runtime we're going to be working for a while, and give it control while we wait for stuff.
Description
Take the mutable APIs for the DB and mark them
async. Then bubble that up to their users. Then make the functions non-blocking by usingspawn_blockingto run on the blocking thread, releasing the runtime to handle more work.The DB writing APIs had to change to pass-by-value to satisfy the borrow-checker in the blocking task context. I think I can use proper lifetime bounds with a helper crate, if that's preferred. The values were already being discarded after passing to the DB, so passing by value should not be a problem either way.
Special considerations:
block_on, as that might reduce the changes needed by localizing the async work. If desired, that can be changed up to making atokio::main. The same is true for some setup functions for tests.and_thencan't be used on futures and everything became a mess without that little helper.closes #2402