-
Notifications
You must be signed in to change notification settings - Fork 216
fix(l1): defer Amsterdam block gas overflow check to post-execution #6486
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
Changes from all commits
a8c431a
09b87d6
7d80962
7231609
baca2a7
43ea8e7
3ae8c9f
93bc5a0
e714e7f
a56d14f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,6 @@ use ethrex_levm::account::{AccountStatus, LevmAccount}; | |
| use ethrex_levm::call_frame::Stack; | ||
| use ethrex_levm::constants::{ | ||
| POST_OSAKA_GAS_LIMIT_CAP, STACK_LIMIT, SYS_CALL_GAS_LIMIT, TX_BASE_COST, | ||
| TX_MAX_GAS_LIMIT_AMSTERDAM, | ||
| }; | ||
| use ethrex_levm::db::Database; | ||
| use ethrex_levm::db::gen_db::{CacheDB, GeneralizedDatabase}; | ||
|
|
@@ -126,18 +125,13 @@ impl LEVM { | |
| })?; | ||
|
|
||
| for (tx_idx, (tx, tx_sender)) in transactions_with_sender.into_iter().enumerate() { | ||
| // Pre-tx gas limit guard per EIP-8037/EIP-7825: | ||
| // Amsterdam: check min(TX_MAX_GAS_LIMIT, tx.gas) against regular gas only. | ||
| // State gas is NOT checked per-tx; block-end validation enforces | ||
| // max(block_regular, block_state) <= gas_limit. | ||
| // Pre-Amsterdam: check tx.gas against cumulative_gas_used (post-refund sum). | ||
| if is_amsterdam { | ||
| check_gas_limit( | ||
| block_regular_gas_used, | ||
| tx.gas_limit().min(TX_MAX_GAS_LIMIT_AMSTERDAM), | ||
| block.header.gas_limit, | ||
| )?; | ||
| } else { | ||
| // Pre-tx gas limit guard: | ||
| // Pre-Amsterdam: reject tx if cumulative post-refund gas + tx.gas > block limit. | ||
| // Amsterdam+: skip — EIP-8037's 2D gas model means cumulative gas (regular + | ||
| // state) can legally exceed the block gas limit as long as | ||
| // max(sum_regular, sum_state) stays within it. Block-level overflow is | ||
| // detected post-execution. | ||
| if !is_amsterdam { | ||
| check_gas_limit(cumulative_gas_used, tx.gas_limit(), block.header.gas_limit)?; | ||
| } | ||
|
Comment on lines
127
to
136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the per-tx guard removed, the sequential/pipeline paths execute every transaction in the block before discovering a gas overflow. For block production (as opposed to validation), this means a builder that over-fills a block will run all transactions and only learn about the overflow after the fact. A lightweight upper-bound estimate — e.g. checking whether Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 127-136
Comment:
**Deferred check wastes computation during Amsterdam block production**
With the per-tx guard removed, the sequential/pipeline paths execute every transaction in the block before discovering a gas overflow. For block production (as opposed to validation), this means a builder that over-fills a block will run all transactions and only learn about the overflow after the fact. A lightweight upper-bound estimate — e.g. checking whether `block_regular_gas_used + tx.gas_limit()` already exceeds `block.header.gas_limit` before executing — could short-circuit wasted work without breaking the post-refund-accounting semantics. This is a performance concern, not a correctness issue.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
|
@@ -175,6 +169,20 @@ impl LEVM { | |
| report.gas_used, | ||
| report.gas_spent, | ||
| ); | ||
|
|
||
| // DoS protection: early exit if either regular or state gas exceeds the limit. | ||
| // Since block_gas_used = max(regular, state), if either component exceeds | ||
| // the limit, we know the block is invalid and can safely reject without | ||
| // violating EIP-8037 semantics. | ||
| if block_regular_gas_used > block.header.gas_limit | ||
| || block_state_gas_used > block.header.gas_limit | ||
| { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: Block gas used overflow: \ | ||
| block_gas_used {block_gas_used} > block_gas_limit {}", | ||
| block.header.gas_limit | ||
| ))); | ||
| } | ||
| } else { | ||
| block_gas_used = block_gas_used.saturating_add(report.gas_used); | ||
| } | ||
|
|
@@ -189,6 +197,17 @@ impl LEVM { | |
| receipts.push(receipt); | ||
| } | ||
|
|
||
| // EIP-7778 (Amsterdam+): block-level gas overflow check. | ||
| // Per-tx checks are skipped for Amsterdam because block gas is computed | ||
| // from pre-refund values; overflow can only be detected after execution. | ||
| if is_amsterdam && block_gas_used > block.header.gas_limit { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: Block gas used overflow: \ | ||
| block_gas_used {block_gas_used} > block_gas_limit {}", | ||
| block.header.gas_limit | ||
| ))); | ||
| } | ||
|
Comment on lines
+203
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 189-195
Comment:
**Block-level error wrapped in `EvmError::Transaction`**
`EvmError::Transaction` formats as `"Invalid Transaction: …"`, but this error is triggered by the cumulative block-level gas total exceeding the limit — no single transaction is invalid in isolation. `EvmError::Header` (formats as `"Invalid Header: …"`) would be semantically closer to a block invariant violation, or a dedicated `EvmError::Block` variant could be introduced. The mismatch is what drives the acknowledged remaining Hive failure where the test harness sees `BlockException.GAS_USED_OVERFLOW` instead of `TransactionException.GAS_ALLOWANCE_EXCEEDED`. The same pattern applies to the pipeline (line 503) and parallel (line 994) paths.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| // Set BAL index for post-execution phase (requests + withdrawals, uint16) | ||
| // Order must match geth: requests (system calls) BEFORE withdrawals. | ||
| if is_amsterdam { | ||
|
|
@@ -424,18 +443,13 @@ impl LEVM { | |
| let mut tx_since_last_flush = 2; | ||
|
|
||
| for (tx_idx, (tx, tx_sender)) in transactions_with_sender.into_iter().enumerate() { | ||
| // Pre-tx gas limit guard per EIP-8037/EIP-7825: | ||
| // Amsterdam: check min(TX_MAX_GAS_LIMIT, tx.gas) against regular gas only. | ||
| // State gas is NOT checked per-tx; block-end validation enforces | ||
| // max(block_regular, block_state) <= gas_limit. | ||
| // Pre-Amsterdam: check tx.gas against cumulative_gas_used (post-refund sum). | ||
| if is_amsterdam { | ||
| check_gas_limit( | ||
| block_regular_gas_used, | ||
| tx.gas_limit().min(TX_MAX_GAS_LIMIT_AMSTERDAM), | ||
| block.header.gas_limit, | ||
| )?; | ||
| } else { | ||
| // Pre-tx gas limit guard: | ||
| // Pre-Amsterdam: reject tx if cumulative post-refund gas + tx.gas > block limit. | ||
| // Amsterdam+: skip — EIP-8037's 2D gas model means cumulative gas (regular + | ||
| // state) can legally exceed the block gas limit as long as | ||
| // max(sum_regular, sum_state) stays within it. Block-level overflow is | ||
| // detected post-execution. | ||
| if !is_amsterdam { | ||
| check_gas_limit(cumulative_gas_used, tx.gas_limit(), block.header.gas_limit)?; | ||
| } | ||
|
|
||
|
|
@@ -483,6 +497,20 @@ impl LEVM { | |
| if is_amsterdam { | ||
| // Amsterdam+: block gas = max(regular_sum, state_sum) | ||
| block_gas_used = block_regular_gas_used.max(block_state_gas_used); | ||
|
|
||
| // DoS protection: early exit if either regular or state gas exceeds the limit. | ||
| // Since block_gas_used = max(regular, state), if either component exceeds | ||
| // the limit, we know the block is invalid and can safely reject without | ||
| // violating EIP-8037 semantics. | ||
| if block_regular_gas_used > block.header.gas_limit | ||
| || block_state_gas_used > block.header.gas_limit | ||
| { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: Block gas used overflow: \ | ||
| block_gas_used {block_gas_used} > block_gas_limit {}", | ||
| block.header.gas_limit | ||
| ))); | ||
| } | ||
| } else { | ||
| block_gas_used = block_gas_used.saturating_add(report.gas_used); | ||
| } | ||
|
|
@@ -497,6 +525,17 @@ impl LEVM { | |
| receipts.push(receipt); | ||
| } | ||
|
|
||
| // EIP-7778 (Amsterdam+): block-level gas overflow check. | ||
| // Per-tx checks are skipped for Amsterdam because block gas is computed | ||
| // from pre-refund values; overflow can only be detected after execution. | ||
| if is_amsterdam && block_gas_used > block.header.gas_limit { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: Block gas used overflow: \ | ||
| block_gas_used {block_gas_used} > block_gas_limit {}", | ||
| block.header.gas_limit | ||
| ))); | ||
| } | ||
|
|
||
| #[cfg(feature = "perf_opcode_timings")] | ||
| { | ||
| let mut timings = OPCODE_TIMINGS.lock().expect("poison"); | ||
|
|
@@ -972,32 +1011,21 @@ impl LEVM { | |
| // balance in the BAL won't match execution that ran all txs). | ||
| let mut block_regular_gas_used = 0_u64; | ||
| let mut block_state_gas_used = 0_u64; | ||
| for (tx_idx, _, report, _, _, _) in &exec_results { | ||
| // Per-tx check: only regular gas is checked per-tx (EIP-8037/EIP-7825). | ||
| // State gas is validated at block end via max(regular, state) <= gas_limit. | ||
| let tx_gas_limit = txs_with_sender[*tx_idx].0.gas_limit(); | ||
| check_gas_limit( | ||
| block_regular_gas_used, | ||
| tx_gas_limit.min(TX_MAX_GAS_LIMIT_AMSTERDAM), | ||
| header.gas_limit, | ||
| )?; | ||
| for (_, _, report, _, _, _) in &exec_results { | ||
| let tx_state_gas = report.state_gas_used; | ||
| let tx_regular_gas = report.gas_used.saturating_sub(tx_state_gas); | ||
| block_regular_gas_used = block_regular_gas_used.saturating_add(tx_regular_gas); | ||
| block_state_gas_used = block_state_gas_used.saturating_add(tx_state_gas); | ||
| // Post-tx check: needed because all txs are already executed — if the last tx | ||
| // pushes actual gas over the limit, there's no next iteration to catch it | ||
| // like the sequential path does. | ||
| let running_block_gas_after = block_regular_gas_used.max(block_state_gas_used); | ||
| if running_block_gas_after > header.gas_limit { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: \ | ||
| used {running_block_gas_after} > block limit {}", | ||
| header.gas_limit | ||
| ))); | ||
| } | ||
| } | ||
| let block_gas_used = block_regular_gas_used.max(block_state_gas_used); | ||
| // EIP-7778: block-level overflow check using pre-refund gas. | ||
| if block_gas_used > header.gas_limit { | ||
| return Err(EvmError::Transaction(format!( | ||
| "Gas allowance exceeded: Block gas used overflow: \ | ||
| block_gas_used {block_gas_used} > block_gas_limit {}", | ||
| header.gas_limit | ||
| ))); | ||
| } | ||
|
|
||
| // 4. Per-tx BAL validation — now safe to run after gas limit is confirmed OK. | ||
| // Also mark off storage_reads that appear in per-tx execution state. | ||
|
|
||
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.
I think we should still have some sort of check to prevent DoS by blocks using more gas than allowed. In principle in a 10MB block with 100B transactions each with a 16M limit we could end up executing 1.6Tgas of code before realizing the block is actually invalid.