fix(l2): limit block to blob size#2292
Merged
Merged
Conversation
Lines of code reportTotal lines added: Detailed view |
ManuelBilbao
requested changes
Mar 21, 2025
avilagaston9
commented
Mar 21, 2025
| Execution(Box<revm::db::CacheDB<ExecutionDB>>), | ||
| } | ||
|
|
||
| impl Clone for EvmState { |
Contributor
Author
There was a problem hiding this comment.
This was needed to restore the previous EVM state after executing a transaction whose resulting state diff doesn't fit in a blob.
Contributor
There was a problem hiding this comment.
That merits a code comment I believe. My first instinct in a few months after seeing this would be to try and convert it into a derive, breaking the logic. Can't be the only one.
Also, where this differs from the derive(Clone) should also be commented. I guess it's the cache clone at the end?
Contributor
Author
There was a problem hiding this comment.
The problem was that revm::db::State doesn't implement the Clone trait. I added a comment here: dc9d5d8!
mpaulucci
reviewed
Mar 24, 2025
| context.account_updates = account_updates; | ||
| Ok(()) | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
the idea is not to pollute l1 with a lot of l2 code, can't this go inside l2 crate?
Contributor
Author
There was a problem hiding this comment.
Moved tol2/sequencer/block_producer/payload_builder.rs!
Contributor
Author
Added in 3169adb! |
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
|
added 2 commits
April 3, 2025 16:15
jrchatruc
approved these changes
Apr 4, 2025
ManuelBilbao
approved these changes
Apr 16, 2025
Comment on lines
+115
to
+120
| if !blob_txs.is_empty() && context.blobs_bundle.blobs.len() >= max_blob_number_per_block { | ||
| debug!("No more blob gas to run blob transactions"); | ||
| blob_txs.clear(); | ||
| } | ||
| // Fetch the next transactions | ||
| let (head_tx, is_blob) = match (plain_txs.peek(), blob_txs.peek()) { |
Contributor
There was a problem hiding this comment.
I think we won't support blob txs in the L2, so maybe this is unnecessary
Collaborator
There was a problem hiding this comment.
Yeah, we almost certainly won't support them, but I would tackle this in a separate PR where we remove all blob txs support on the L2
Comment on lines
+230
to
+238
| async fn check_state_diff_size( | ||
| acc_withdrawals_size: &mut usize, | ||
| acc_deposits_size: &mut usize, | ||
| acc_state_diff_size: &mut usize, | ||
| tx: Transaction, | ||
| receipt: &Receipt, | ||
| context: &mut PayloadBuildContext, | ||
| accounts_info_cache: &mut HashMap<Address, Option<AccountInfo>>, | ||
| ) -> Result<bool, BlockProducerError> { |
Contributor
There was a problem hiding this comment.
I'd add a doc comment saying what changes it'll do to the mut variables. Also maybe consider rename the function to something like update_state_diff_size
jrchatruc
approved these changes
Apr 16, 2025
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
…lass#2357) **Motivation** Noticed on lambdaclass#2292 that the majority of the time in `LEVM::get_state_transitions()` was spent on calls to `get_account_info()`. While looking for areas to improve, I found that we were calling `get_account_info()` three times instead of reusing the value returned in the first call. **Description** Removes the repeated calls to `get_account_info`. Testing locally shows a `2x` speed improvement in `payload_builder::build_payload()` implemented in lambdaclass#2292. Closes None
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
**Motivation**
With our current implementation, if a block state diff exceeds the blob
size, we are unable to commit that block.
**Description**
This PR provides an initial solution by calculating the state diff size
after including each transaction in the block. If the size exceeds the
blob limit, the previous state is restored and transactions continue to
be added from the remaining senders in the mempool.
**Observations**
- `blockchain::build_payload` was "replicated" in
`block_producer/payload_builder.rs` with two key differences:
- It doesn't call `blockchain::apply_system_operations`.
- It uses a custom L2 implementation of `fill_transactions` which adds
the logic described above.
- Some functions in `blockchain` are now public to allow access from
`payload_builder`.
- `PayloadBuildContext` now contains am owned `Block` instead of a
mutable reference of it, as we need to be able to clone the
`PayloadBuildContext` to restore a previous state.
- `PayloadBuildContext` is cloned before each transaction execution,
which may impact performance.
- After each transaction, `vm.get_state_transitions()` is called to
compute the state diff size.
- Since `REVM::vm.get_state_transitions()` mutates the
`PayloadBuildContext`, we need to clone it to avoid unintended changes.
- An `accounts_info_cache` is used to prevent calling `get_account_info`
on every tx execution.
> [!WARNING]
> - **REVM**: Payload building is **10x slower** due to frequent
`clone()` calls.
> - **LEVM**: Payload building is **100x slower** because
`LEVM::get_state_transitions` internally calls `get_account_info`.
>
> *These issues will be addressed in future PRs.*
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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
With our current implementation, if a block state diff exceeds the blob size, we are unable to commit that block.
Description
This PR provides an initial solution by calculating the state diff size after including each transaction in the block. If the size exceeds the blob limit, the previous state is restored and transactions continue to be added from the remaining senders in the mempool.
Observations
blockchain::build_payloadwas "replicated" inblock_producer/payload_builder.rswith two key differences:blockchain::apply_system_operations.fill_transactionswhich adds the logic described above.blockchainare now public to allow access frompayload_builder.PayloadBuildContextnow contains am ownedBlockinstead of a mutable reference of it, as we need to be able to clone thePayloadBuildContextto restore a previous state.PayloadBuildContextis cloned before each transaction execution, which may impact performance.vm.get_state_transitions()is called to compute the state diff size.REVM::vm.get_state_transitions()mutates thePayloadBuildContext, we need to clone it to avoid unintended changes.accounts_info_cacheis used to prevent callingget_account_infoon every tx execution.Warning
clone()calls.LEVM::get_state_transitionsinternally callsget_account_info.These issues will be addressed in future PRs.