perf(l1): disable balance check for prewarming to avoid early reverts#6259
perf(l1): disable balance check for prewarming to avoid early reverts#6259
Conversation
github-actions
bot
added
L1
🤖 Kimi Code ReviewSecurity & Correctness Review1. Critical Security Issue: Balance Check Bypass in ProductionFile: The
Recommendation: Add a compile-time or runtime assertion ensuring #[cfg(not(feature = "prewarm"))]
if disable_balance_check {
return Err(EvmError::InvalidConfiguration(
"Balance checks cannot be disabled in production".to_string()
));
}2. Missing Gas Accounting ValidationFile: While balance checks are skipped, gas limit validation is not. This creates an inconsistency where:
Recommendation: Either:
3. State Consistency RiskFile: The
Recommendation: Document this limitation clearly and ensure prewarm caches are invalidated before real execution. 4. Code Clarity IssueFile: The parameter name Self::execute_tx(..., false) // What does false mean?
Self::execute_tx(..., true) // What does true mean?Recommendation: Use an enum or named struct: enum ExecutionMode {
Normal,
Prewarm,
}
Self::execute_tx(..., ExecutionMode::Normal)5. Missing TestsNo test files were modified to verify:
Recommendation: Add tests in
Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewFindings
If this is strictly prewarm-only and state is fully discarded, the change looks fine. No other correctness, security, gas, or EVM behavior issues spotted in this diff. Possible next steps:
Automated review by OpenAI Codex · custom prompt |
Greptile SummaryThis PR adds a
Issue found:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/environment.rs | Added disable_balance_check flag to Environment struct with clear documentation |
| crates/vm/levm/src/hooks/default_hook.rs | Added early returns in validate_sender_balance and deduct_caller when flag is set, but L2 hook's deduct_caller_fee_token not updated |
| crates/vm/backends/levm/mod.rs | Correctly passes disable_balance_check: true only from prewarmer path (warm_block), false elsewhere |
Last reviewed commit: 48a115e
Lines of code reportTotal lines added: Detailed view |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn validate_sender_balance(vm: &mut VM<'_>, sender_balance: U256) -> Result<(), VMError> { | ||
| if vm.env.disable_balance_check { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The L2 hook's deduct_caller_fee_token function (called from L2 transaction validation) doesn't check disable_balance_check, which means L2 transactions with fee tokens will still revert early during prewarming. Consider applying the same pattern to l2_hook::deduct_caller_fee_token at crates/vm/levm/src/hooks/l2_hook.rs:557
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/hooks/default_hook.rs
Line: 490-493
Comment:
The L2 hook's `deduct_caller_fee_token` function (called from L2 transaction validation) doesn't check `disable_balance_check`, which means L2 transactions with fee tokens will still revert early during prewarming. Consider applying the same pattern to `l2_hook::deduct_caller_fee_token` at crates/vm/levm/src/hooks/l2_hook.rs:557
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have enough context to write a thorough review. Review:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a performance optimization for the L1 LEVM prewarmer by introducing a disable_balance_check flag that allows the prewarmer to skip early balance validation and deduction checks. This enables the prewarmer to reach deeper into transaction execution paths, warming more storage slots even for transactions that would normally revert due to insufficient balance.
Changes:
- Added
disable_balance_checkboolean field to theEnvironmentstruct with documentation - Modified
validate_sender_balanceanddeduct_callerhooks to skip checks when the flag is enabled - Threaded the new parameter through
execute_tx_in_block, passingfalsefor normal execution andtrueonly for the prewarmer path
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/vm/levm/src/environment.rs | Added disable_balance_check field to Environment struct with documentation explaining its use in the prewarmer |
| crates/vm/levm/src/hooks/default_hook.rs | Added early return checks in validate_sender_balance and deduct_caller when disable_balance_check is true |
| crates/vm/backends/levm/mod.rs | Added disable_balance_check parameter to execute_tx_in_block, initialized the field in environment constructors, and passed appropriate values (false for normal execution, true for prewarmer) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let env = Environment { | ||
| origin: system_address, | ||
| // EIPs 2935, 4788, 7002 and 7251 dictate that the system calls have a gas limit of 30 million and they do not use intrinsic gas. | ||
| // So we add the base cost that will be taken in the execution. | ||
| gas_limit: SYS_CALL_GAS_LIMIT + TX_BASE_COST, | ||
| block_number: block_header.number.into(), | ||
| coinbase: block_header.coinbase, | ||
| timestamp: block_header.timestamp.into(), | ||
| prev_randao: Some(block_header.prev_randao), | ||
| base_fee_per_gas: U256::zero(), | ||
| gas_price: U256::zero(), | ||
| block_excess_blob_gas: block_header.excess_blob_gas.map(U256::from), | ||
| block_blob_gas_used: block_header.blob_gas_used.map(U256::from), | ||
| block_gas_limit: i64::MAX as u64, // System calls, have no constraint on the block's gas limit. | ||
| config, | ||
| ..Default::default() | ||
| }; |
There was a problem hiding this comment.
The Environment construction in the system contract call function is missing the explicit initialization of the disable_balance_check field. While it will default to false through the Default trait, for consistency and clarity with other Environment constructions in this file (lines 461-489 and 988-1015), this field should be explicitly set to false.
| /// When true, skip balance deduction in `deduct_caller`. Used by the prewarmer | ||
| /// to avoid early reverts on insufficient balance so that warming touches more storage. |
There was a problem hiding this comment.
The comment states that skipping balance checks is used by the prewarmer to "avoid early reverts on insufficient balance", but the check being skipped in validate_sender_balance is critical for transaction validity. While this is acceptable for the prewarmer (since it's only warming the cache and not actually executing transactions), the comment should clarify that the prewarmer results are discarded and not used for actual transaction execution. Consider adding a warning comment that this flag MUST NEVER be set to true for actual transaction execution, only for cache warming.
| /// When true, skip balance deduction in `deduct_caller`. Used by the prewarmer | |
| /// to avoid early reverts on insufficient balance so that warming touches more storage. | |
| /// When true, skip balance deduction in `deduct_caller`. | |
| /// | |
| /// This flag is intended **only** for the prewarmer to avoid early reverts on | |
| /// insufficient balance so that cache warming can touch more storage/state. | |
| /// | |
| /// Any execution performed with this flag set to `true` must have its results | |
| /// discarded and MUST NOT be used for actual transaction execution, validation, | |
| /// or consensus-critical state transitions. This flag MUST NEVER be set to | |
| /// `true` in normal transaction processing; it is for cache warming only. |
Benchmark Results ComparisonBenchmark Results: MstoreBench
Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Benchmark Block Execution Results Comparison Against Main
|
When warm_block() runs speculative execution for warming, deduct_caller() can fail on insufficient balance causing the tx to revert before touching the storage slots it would have accessed. Add disable_balance_check flag to Environment and skip balance deduction/validation in warming mode so that warming covers more storage slots. Inspired by reth commit 95ed3771.
The new field was added to the Environment struct but three callsites that construct it with explicit fields (no ..Default::default()) were not updated: ef_tests state runner, ef_tests state_v2 runner, and eip7708 tests.
ilitteri
force-pushed
the
perf/disable-balance-check-warming
branch
from
3533dc3 to
7f3d47e
Compare
| &mut group_db, | ||
| vm_type, | ||
| stack_pool, | ||
| true, |
There was a problem hiding this comment.
nit: bare boolean at the call site (true / false) doesn't convey intent. Reads a bit better with a /* disable_balance_check */ comment or a constant, e.g.
const WARMING_MODE: bool = true;Same applies to the false at the normal execution call site (line 238).
…l path After merging main (which added the disable_balance_check param in #6259), the BAL parallel execution call site was missing the argument. Normal execution uses false (balance checks enabled).
Motivation
During block execution, the prewarmer thread runs transactions speculatively to warm storage slots and account state into the cache. However, when a sender has insufficient balance, the prewarmer reverts early in
validate_sender_balance/deduct_caller, missing the opportunity to warm the storage slots that the transaction would have touched deeper in execution.Description
disable_balance_checkflag to the LEVMEnvironmentstructvalidate_sender_balanceanddeduct_callerwhen the flag is setdisable_balance_check: trueonly from the prewarmer path,falsefor normal execution