fix(levm): validate remaining block gas in parallel pipeline#6342
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns the Amsterdam BAL-backed parallel LEVM execution pipeline with the sequential path by enforcing the “remaining block gas” constraint based on cumulative post-refund gas (EIP-7778), preventing blocks from being accepted when a later tx’s gas_limit() exceeds remaining block gas.
Changes:
- Carry
tx.gas_limit()through the parallel execution result tuple. - Add
build_parallel_receipts(...)to sort results, runcheck_gas_limit(...)during receipt construction, and return(receipts, block_gas_used). - Add a regression test covering a later transaction whose
gas_limitexceeds remaining block gas.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR fixes a missing block gas limit validation in the parallel LEVM execution pipeline (Amsterdam+ forks). Previously, Changes:
The logic is correct and symmetric with the sequential path: Confidence Score: 5/5
Sequence DiagramsequenceDiagram
participant EP as execute_block_parallel
participant RY as Rayon Tasks
participant BPR as build_parallel_receipts
participant CGL as check_gas_limit
EP->>RY: spawn N tasks with tx_idx 0 to n
RY->>RY: execute_tx produces ExecutionReport
RY-->>EP: Vec of (tx_idx, tx_type, tx_gas_limit, report)
EP->>BPR: build_parallel_receipts(block_gas_limit, results)
BPR->>BPR: sort results by tx_idx
loop for each tx in order
BPR->>CGL: check_gas_limit(cumulative_gas_used, tx_gas_limit, block_gas_limit)
CGL-->>BPR: Ok or EvmError
BPR->>BPR: cumulative_gas_used += gas_spent
BPR->>BPR: block_gas_used += gas_used
BPR->>BPR: push Receipt
end
BPR-->>EP: receipts and block_gas_used
Last reviewed commit: 8b69e7f |
edg-l
left a comment
There was a problem hiding this comment.
Good catch! Thanks for the PR
The CI does it but I validated the ef-tests still pass with
make -C tooling/ef_tests/blockchain test
I checked if geth does this check, but it seems they don't either, so they are probs missing this validation too.
| let mut block_gas_used = 0_u64; | ||
|
|
||
| for (_, tx_type, tx_gas_limit, report) in results { | ||
| check_gas_limit(cumulative_gas_used, tx_gas_limit, block_gas_limit)?; |
There was a problem hiding this comment.
Bug: this passes cumulative_gas_used (sum of gas_spent, post-refund) to check_gas_limit, but the sequential paths (lines 107 and 215) pass block_gas_used (sum of gas_used, pre-refund). Post-EIP-7778 these differ: gas_spent <= gas_used, so the parallel path becomes more lenient and could accept blocks that the sequential path rejects.
Should be:
check_gas_limit(block_gas_used, tx_gas_limit, block_gas_limit)?;There was a problem hiding this comment.
Sorry, I got this wrong. The sequential paths (lines 122 and 285 on main) actually use cumulative_gas_used (post-refund), not block_gas_used. PR #6309 explicitly fixed this to use post-refund gas because pre-refund gas causes false "Gas allowance exceeded" rejections on tightly-packed Amsterdam blocks with large SSTORE refunds (see the post-mortem there).
Your original code using cumulative_gas_used was correct. Commit 6b7f80 switching to block_gas_used reintroduces the bug that #6309 fixed. Please revert to cumulative_gas_used for the parallel path to match the sequential paths.
| } | ||
|
|
||
| #[test] | ||
| fn test_build_parallel_receipts_rejects_tx_that_exceeds_remaining_block_gas() { |
There was a problem hiding this comment.
nit: the test uses gas_used == gas_spent for all entries, so it doesn't exercise the bug described above. Consider using different values (e.g., gas_used: 70, gas_spent: 60) to verify the check uses the pre-refund value.
There was a problem hiding this comment.
Disregard this too — since the check should use cumulative_gas_used (post-refund), the test values should match that. Though having gas_used != gas_spent in the test is still useful to verify the receipt uses gas_spent for cumulative_gas_used correctly.
There was a problem hiding this comment.
Thanks so much! I have been following the project for a while, so I'm really happy I could contribute!!
ElFantasma
left a comment
There was a problem hiding this comment.
Approved
Sorry again for my mistake, and thanks for your contribution!
|
Hey @sueun-dev, the PR is good to go. At ethrex, we have a commit signature verification policy that states that all commits must be verified (read here). Please, sign the commits of this PR for it to be able to be merged. |
Head branch was pushed to by a user without write access
67660a0 to
6fcaf4c
Compare
|
I pushed a verified signed commit and the PR now consists of a single signed commit ( |
…lass#6342) ## Summary - carry each transaction's `gas_limit()` into the BAL-backed parallel execution result set - enforce the same remaining block gas check used by the sequential LEVM path when building parallel receipts - add a regression test for a later transaction whose `gas_limit` exceeds the remaining block gas Fixes lambdaclass#6341. ## Testing - `cargo test -p ethrex-vm` Co-authored-by: Sueun Cho <[email protected]> Co-authored-by: ElFantasma <[email protected]>
Summary
gas_limit()into the BAL-backed parallel execution result setgas_limitexceeds the remaining block gasFixes #6341.
Testing
cargo test -p ethrex-vm