fix(levm): use post-refund gas for check_gas_limit in sequential execution#6309
Conversation
…ution The check_gas_limit accumulator was using pre-refund gas (report.gas_used), which for Amsterdam+ blocks is higher than post-refund gas. This causes valid tightly-packed blocks to be rejected with "Gas allowance exceeded" when executing sequentially without a BAL (e.g. syncing from a peer that doesn't support eth/71). Use post-refund gas (report.gas_spent) for the limit check while keeping pre-refund gas for block_gas_used (header validation). Pre-Amsterdam both values are identical, so no behavior change.
🤖 Kimi Code ReviewReview SummaryThe PR correctly fixes the gas limit check issue by using post-refund gas ( Issues Found
Security Assessment✅ No security vulnerabilities found - The change correctly implements EIP-7778 gas accounting rules and prevents DoS attacks that could occur from incorrectly rejecting valid blocks. Performance Impact✅ Minimal impact - Only adds one additional accumulator variable per transaction, negligible overhead. Suggested Changes// In both functions, update the comment to be more accurate:
// EIP-7778: Separate gas tracking
// - gas_spent (POST-REFUND) for receipt cumulative_gas_used and gas limit validation
// - gas_used (PRE-REFUND for Amsterdam+) for block accounting / header validationConsider extracting the gas tracking logic into a shared function to reduce code duplication between the two execution paths. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Kimi Code ReviewReview SummaryThe PR correctly implements EIP-7778's gas accounting changes by separating pre-refund and post-refund gas tracking. However, there are some issues that need attention: 1. Potential Bug - Gas Limit Check LogicFile: The gas limit check now uses post-refund gas ( // Post-refund accumulator for check_gas_limit (pre-refund would reject valid
// tightly-packed Amsterdam blocks when executing sequentially without BAL)This comment suggests the change is to prevent rejecting valid blocks, but EIP-7778 explicitly changes the gas limit check to use post-refund values. The comment should be updated to reflect this is the intended EIP-7778 behavior, not a workaround. 2. Code DuplicationThe same gas tracking logic is duplicated in both 3. Missing ValidationThere's no validation that 4. Minor - Comment InconsistencyThe comment on lines 134-135 and 311-312 states: // - gas_spent (POST-REFUND) for receipt cumulative_gas_used and limit checkThis is partially incorrect - RecommendationUpdate the misleading comments to accurately reflect EIP-7778's specification that gas limit checks should use post-refund values. The implementation itself appears correct for the EIP-7778 changes. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Claude Code ReviewI now have a thorough understanding of the change. Here's my review: Review: fix(levm): use post-refund gas for
|
🤖 Claude Code ReviewNow I have a complete picture of the PR. Let me write the review. PR Review:
|
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code ReviewNo blocking bugs found in this diff. The change is internally consistent in both execution paths and aligns with the stated EIP-7778 split ( Non-blocking review points:
Overall: change looks good, with the main gap being explicit tests around the altered gas-limit gate semantics. Automated review by OpenAI Codex · custom prompt |
🤖 Codex Code Review
Overall, the PR direction looks correct and internally consistent with the Automated review by OpenAI Codex · custom prompt |
Greptile SummaryThis PR correctly fixes a critical regression in sequential block execution for Amsterdam+ (EIP-7778) blocks, where valid tightly-packed blocks were incorrectly rejected with "Gas allowance exceeded" because the Key changes:
The implementation is logically sound and symmetric across both sequential execution paths. EF tests ( Confidence Score: 5/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start execute_block / execute_block_pipeline sequential] --> B[Init accumulators\ncumulative_gas_used = 0\nblock_gas_used = 0\ngas_used_for_limit_check = 0]
B --> C{For each tx}
C --> D["check_gas_limit(gas_used_for_limit_check,\n tx.gas_limit(), block.gas_limit)"]
D -- Exceeds limit --> E[Return Err: Gas allowance exceeded]
D -- OK --> F[execute_tx → report]
F --> G["cumulative_gas_used += report.gas_spent (POST-REFUND)\ngas_used_for_limit_check += report.gas_spent (POST-REFUND)\nblock_gas_used += report.gas_used (PRE-REFUND)"]
G --> H[Emit receipt with cumulative_gas_used]
H --> C
C -- Done --> I["BlockExecutionResult { block_gas_used } ← PRE-REFUND\n(used by validate_gas_used for header check)"]
Last reviewed commit: fe44f38 |
Use cumulative_gas_used directly for check_gas_limit since both accumulate the same value (report.gas_spent from zero).
| // - gas_used (PRE-REFUND for Amsterdam+) for block accounting | ||
| // - gas_spent (POST-REFUND) for receipt cumulative_gas_used and limit check | ||
| // - gas_used (PRE-REFUND for Amsterdam+) for block accounting / header validation | ||
| cumulative_gas_used += report.gas_spent; |
There was a problem hiding this comment.
I think it might be a good idea to rename cumulative_gas_used to cumulative_gas_spent to make it even clearer what it tracks
There was a problem hiding this comment.
Clean fix — using post-refund cumulative_gas_used for check_gas_limit is correct per EIP-7778, and the post-mortem in the PR description is excellent. The change is symmetric across both execute_block and execute_block_pipeline, and pre-Amsterdam behavior is preserved since both values are identical.
| for (tx_idx, (tx, tx_sender)) in transactions_with_sender.into_iter().enumerate() { | ||
| check_gas_limit(block_gas_used, tx.gas_limit(), block.header.gas_limit)?; | ||
| check_gas_limit( | ||
| cumulative_gas_used, |
There was a problem hiding this comment.
nit: check_gas_limit's first parameter is still named block_gas_used and the error message says "Block gas used overflow", but it now receives the post-refund cumulative value. Consider renaming the parameter to cumulative_gas (or similar) and adjusting the error message so future debugging sessions aren't misleading.
…ution (lambdaclass#6309) ## Summary - Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator in both sequential execution paths (`execute_block` and `execute_block_pipeline`) - Keep pre-refund gas (`report.gas_used`) for `block_gas_used` in `BlockExecutionResult` (used for header validation via `validate_gas_used`) Pre-Amsterdam, both values are identical so there is no behavior change. ## Post-mortem **Observed error on bal-devnet-2:** ``` Error: Invalid Block: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Caused by: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Location: /ethrex/cmd/ethrex/initializers.rs:629:9 ``` **Root cause:** When a node executes Amsterdam+ blocks sequentially (e.g. full sync, engine API catch-up, or any path without a BAL), `check_gas_limit` accumulates gas to verify each transaction fits within the block gas limit. EIP-7778 introduced separate gas tracking: `gas_used` (pre-refund, higher) for block-level accounting and `gas_spent` (post-refund, lower) for what the user actually pays. The `check_gas_limit` accumulator was using pre-refund gas, which inflates the running total. **Why it failed:** The accumulated pre-refund gas (79.3M) + next tx's gas limit (1M) = 80.3M, exceeding the block gas limit (80M). With post-refund gas the accumulated value is lower (accounting for refunds from SSTORE clears, etc.), keeping the total within the 80M limit. The block is valid — it's at 99.2% capacity and the refund difference is just enough to push the pre-refund accumulator over. **Fix:** Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator, matching the pre-Amsterdam behavior where both values were identical. Pre-refund gas (`report.gas_used`) is still used for `block_gas_used` in the `BlockExecutionResult`, which is validated against `header.gas_used`. **Why EF tests didn't catch this:** The existing EIP-7778 EF test (`test_multi_transaction_gas_accounting`) has `gasUsed == gasLimit` (fully packed block), but each transaction's pre-refund gas is bounded by its own `gas_limit`. The check is `accumulated_gas + next_tx.gas_limit <= block_gas_limit` — since a tx's actual gas can never exceed its own limit, the accumulated pre-refund gas from prior txs never pushes the check over. The bug only manifests with real-world blocks where large refunds (e.g. SSTORE clears) create a significant gap between pre-refund and post-refund gas AND the block is packed tightly enough that the inflated accumulator + next tx's gas limit overflows the block limit. <img width="2816" height="1420" alt="image" src="https://github.com/user-attachments/assets/0908a3d4-a6bd-44de-8667-ef32e2338458" /> ## Test plan - [x] `cargo build` passes - [x] `cargo test` passes - [x] EIP-7778 EF tests pass (`test_multi_transaction_gas_accounting`) - [ ] Manual devnet test on bal-devnet-2
…ution (#6309) ## Summary - Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator in both sequential execution paths (`execute_block` and `execute_block_pipeline`) - Keep pre-refund gas (`report.gas_used`) for `block_gas_used` in `BlockExecutionResult` (used for header validation via `validate_gas_used`) Pre-Amsterdam, both values are identical so there is no behavior change. ## Post-mortem **Observed error on bal-devnet-2:** ``` Error: Invalid Block: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Caused by: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Location: /ethrex/cmd/ethrex/initializers.rs:629:9 ``` **Root cause:** When a node executes Amsterdam+ blocks sequentially (e.g. full sync, engine API catch-up, or any path without a BAL), `check_gas_limit` accumulates gas to verify each transaction fits within the block gas limit. EIP-7778 introduced separate gas tracking: `gas_used` (pre-refund, higher) for block-level accounting and `gas_spent` (post-refund, lower) for what the user actually pays. The `check_gas_limit` accumulator was using pre-refund gas, which inflates the running total. **Why it failed:** The accumulated pre-refund gas (79.3M) + next tx's gas limit (1M) = 80.3M, exceeding the block gas limit (80M). With post-refund gas the accumulated value is lower (accounting for refunds from SSTORE clears, etc.), keeping the total within the 80M limit. The block is valid — it's at 99.2% capacity and the refund difference is just enough to push the pre-refund accumulator over. **Fix:** Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator, matching the pre-Amsterdam behavior where both values were identical. Pre-refund gas (`report.gas_used`) is still used for `block_gas_used` in the `BlockExecutionResult`, which is validated against `header.gas_used`. **Why EF tests didn't catch this:** The existing EIP-7778 EF test (`test_multi_transaction_gas_accounting`) has `gasUsed == gasLimit` (fully packed block), but each transaction's pre-refund gas is bounded by its own `gas_limit`. The check is `accumulated_gas + next_tx.gas_limit <= block_gas_limit` — since a tx's actual gas can never exceed its own limit, the accumulated pre-refund gas from prior txs never pushes the check over. The bug only manifests with real-world blocks where large refunds (e.g. SSTORE clears) create a significant gap between pre-refund and post-refund gas AND the block is packed tightly enough that the inflated accumulator + next tx's gas limit overflows the block limit. <img width="2816" height="1420" alt="image" src="https://github.com/user-attachments/assets/0908a3d4-a6bd-44de-8667-ef32e2338458" /> ## Test plan - [x] `cargo build` passes - [x] `cargo test` passes - [x] EIP-7778 EF tests pass (`test_multi_transaction_gas_accounting`) - [ ] Manual devnet test on bal-devnet-2
…ution (lambdaclass#6309) ## Summary - Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator in both sequential execution paths (`execute_block` and `execute_block_pipeline`) - Keep pre-refund gas (`report.gas_used`) for `block_gas_used` in `BlockExecutionResult` (used for header validation via `validate_gas_used`) Pre-Amsterdam, both values are identical so there is no behavior change. ## Post-mortem **Observed error on bal-devnet-2:** ``` Error: Invalid Block: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Caused by: Invalid transaction: Gas allowance exceeded: Block gas used overflow: used 79340683 + tx limit 1000000 > block limit 80000000 Location: /ethrex/cmd/ethrex/initializers.rs:629:9 ``` **Root cause:** When a node executes Amsterdam+ blocks sequentially (e.g. full sync, engine API catch-up, or any path without a BAL), `check_gas_limit` accumulates gas to verify each transaction fits within the block gas limit. EIP-7778 introduced separate gas tracking: `gas_used` (pre-refund, higher) for block-level accounting and `gas_spent` (post-refund, lower) for what the user actually pays. The `check_gas_limit` accumulator was using pre-refund gas, which inflates the running total. **Why it failed:** The accumulated pre-refund gas (79.3M) + next tx's gas limit (1M) = 80.3M, exceeding the block gas limit (80M). With post-refund gas the accumulated value is lower (accounting for refunds from SSTORE clears, etc.), keeping the total within the 80M limit. The block is valid — it's at 99.2% capacity and the refund difference is just enough to push the pre-refund accumulator over. **Fix:** Use post-refund gas (`report.gas_spent`) for the `check_gas_limit` accumulator, matching the pre-Amsterdam behavior where both values were identical. Pre-refund gas (`report.gas_used`) is still used for `block_gas_used` in the `BlockExecutionResult`, which is validated against `header.gas_used`. **Why EF tests didn't catch this:** The existing EIP-7778 EF test (`test_multi_transaction_gas_accounting`) has `gasUsed == gasLimit` (fully packed block), but each transaction's pre-refund gas is bounded by its own `gas_limit`. The check is `accumulated_gas + next_tx.gas_limit <= block_gas_limit` — since a tx's actual gas can never exceed its own limit, the accumulated pre-refund gas from prior txs never pushes the check over. The bug only manifests with real-world blocks where large refunds (e.g. SSTORE clears) create a significant gap between pre-refund and post-refund gas AND the block is packed tightly enough that the inflated accumulator + next tx's gas limit overflows the block limit. <img width="2816" height="1420" alt="image" src="https://github.com/user-attachments/assets/0908a3d4-a6bd-44de-8667-ef32e2338458" /> ## Test plan - [x] `cargo build` passes - [x] `cargo test` passes - [x] EIP-7778 EF tests pass (`test_multi_transaction_gas_accounting`) - [ ] Manual devnet test on bal-devnet-2
5 participants
Summary
report.gas_spent) for thecheck_gas_limitaccumulator in both sequential execution paths (execute_blockandexecute_block_pipeline)report.gas_used) forblock_gas_usedinBlockExecutionResult(used for header validation viavalidate_gas_used)Pre-Amsterdam, both values are identical so there is no behavior change.
Post-mortem
Observed error on bal-devnet-2:
Root cause: When a node executes Amsterdam+ blocks sequentially (e.g. full sync, engine API catch-up, or any path without a BAL),
check_gas_limitaccumulates gas to verify each transaction fits within the block gas limit. EIP-7778 introduced separate gas tracking:gas_used(pre-refund, higher) for block-level accounting andgas_spent(post-refund, lower) for what the user actually pays. Thecheck_gas_limitaccumulator was using pre-refund gas, which inflates the running total.Why it failed: The accumulated pre-refund gas (79.3M) + next tx's gas limit (1M) = 80.3M, exceeding the block gas limit (80M). With post-refund gas the accumulated value is lower (accounting for refunds from SSTORE clears, etc.), keeping the total within the 80M limit. The block is valid — it's at 99.2% capacity and the refund difference is just enough to push the pre-refund accumulator over.
Fix: Use post-refund gas (
report.gas_spent) for thecheck_gas_limitaccumulator, matching the pre-Amsterdam behavior where both values were identical. Pre-refund gas (report.gas_used) is still used forblock_gas_usedin theBlockExecutionResult, which is validated againstheader.gas_used.Why EF tests didn't catch this: The existing EIP-7778 EF test (
test_multi_transaction_gas_accounting) hasgasUsed == gasLimit(fully packed block), but each transaction's pre-refund gas is bounded by its owngas_limit. The check isaccumulated_gas + next_tx.gas_limit <= block_gas_limit— since a tx's actual gas can never exceed its own limit, the accumulated pre-refund gas from prior txs never pushes the check over. The bug only manifests with real-world blocks where large refunds (e.g. SSTORE clears) create a significant gap between pre-refund and post-refund gas AND the block is packed tightly enough that the inflated accumulator + next tx's gas limit overflows the block limit.Test plan
cargo buildpassescargo testpassestest_multi_transaction_gas_accounting)