-
Notifications
You must be signed in to change notification settings - Fork 131
fix(l1): stop block building preemptively #4999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes unnecessary async/await patterns from the block building process to prevent blocking calls that were causing missed slots in high-throughput local networks. The key change is converting synchronous storage operations that were incorrectly marked as async back to synchronous functions, and wrapping the actual CPU-intensive block building in spawn_blocking to prevent blocking the async runtime.
Key Changes:
- Removed
asyncfromStore::apply_account_updates_batchand related storage methods that perform synchronous operations - Modified
Blockchain::build_payloadandfinalize_payloadto be synchronous - Wrapped block building calls in
spawn_blockingwithin the payload building loop to run on blocking thread pool
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/storage/store.rs | Removed async from apply_account_updates_batch and apply_account_updates_from_trie_batch methods |
| crates/blockchain/payload.rs | Converted build_payload and finalize_payload to sync, added spawn_blocking wrapper in build loop |
| crates/blockchain/blockchain.rs | Removed await calls for apply_account_updates_batch |
| crates/l2/sequencer/block_producer.rs | Removed await call for apply_account_updates_batch |
| crates/l2/sequencer/block_producer/payload_builder.rs | Removed await call for finalize_payload |
| crates/blockchain/smoke_test.rs | Removed await call for build_payload in test |
| tooling/ef_tests/state_v2/src/modules/result_check.rs | Removed async from post_state_root function |
| cmd/ethrex/l2/command.rs | Removed await call for apply_account_updates_from_trie_batch |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Lines of code reportTotal lines added: Detailed view |
| /// Applies account updates based on the block's latest storage state | ||
| /// and returns the new state root after the updates have been applied. | ||
| #[instrument(level = "trace", name = "Trie update", skip_all)] | ||
| pub async fn apply_account_updates_batch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im quite confused about this change (probably due to lack of knowledge). Intuitively I would expect ALL store functions to be async. And also not have to put spawn_blocking in places, which are code smell imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the record, I also think that Store functions should be async. However, I prefer functions to be marked as async only when they are actually asynchronous. This one didn't call any async functions, so the keyword only misleads. What's more, I suspect the compiler actually removes the keyword internally, and I'm upset Clippy doesn't mark cases like this 😠
I agree that a spawn_blocking is a smell when intermingled with async code, but in this case, it's justified: building a block is mostly a synchronous task. To avoid using spawn_blocking, we'd need to rework the EVM itself to be async, or keep a dedicated threadpool for EVMs (essentially making our own spawn_blocking).
Motivation
We were seeing some missed slots in local networks with high throughput (900Mgas limit). The cause was a blocking call being passed to
CancellationToken::run_until_cancelled.Description
This PR changes the block building process to run on a blocking task with
spawn_blocking, and also removes theasynctag to some non-async methods fromStoreandBlockchain.Closes #4992