feat(test-specs): Allow automatic transaction gas-limit#2969
Conversation
|
i looked at the changes for |
Co-authored-by: Leo Lara <leolara@users.noreply.github.com>
danceratopz
left a comment
There was a problem hiding this comment.
Hey @marioevz, this is a great addition. Here's a review of the framework changes (only covered the testing framework so far; with partial review of tests for context: tests/shanghai/, tests/amsterdam/eip7928_*). Few comments below, each links to a proposed fix; the full set, including a new unit test module for the dynamic gas-limit machinery is in marioevz#6.
|
@LouisTsai-Csie could you check if |
Co-authored-by: Leo Lara <leolara@users.noreply.github.com>
Co-authored-by: Leo Lara <leolara@users.noreply.github.com>
Co-authored-by: Leo Lara <leolara@users.noreply.github.com>
Co-authored-by: danceratopz <danceratopz@gmail.com>
Co-authored-by: danceratopz <danceratopz@gmail.com>
…enabled Co-authored-by: danceratopz <danceratopz@gmail.com>
|
@leolara @danceratopz pushed the fixes to all of your comments, thank you for the reviews! |
Thanks @marioevz, I think you missed the unit tests added in marioevz@ebbe563 |
kclowes
left a comment
There was a problem hiding this comment.
Berlin-Frontier look good to me. This is such a nice change!
|
@marioevz I think there are places where we are setting Transaction(gas_limit=None) but setting it to a variable that can be |
|
|
I think there are no unit tests for with_gas_limit, _calculate_implicit_gas_limit, calculate_max_gas_limit and state_gas_reservoir. Perhaps we don't need it because they are called from other tests. |
Co-authored-by: Leo Lara <leolara@users.noreply.github.com>
Nice catch, uploaded a new commit a2e13fb. Just as a side note, I think this specific tests need an update because they are no longer really accurate (they hard-code assumed gas costs for the system contracts, which no longer hold). |
|
Once @spencer-tb signs off we can proceed to merge. |
🗒️ Description
Summary
While reviewing #2901 I noticed that the bulk of its test changes were simply bumping the
gas_limitof transactions in tests that don't actually care about the exact gas limit — they only care that the transaction executes in full without running out of gas.This PR makes the transaction
gas_limitoptional. When a test omits it, the filling/execution tooling calculates it automatically based on fork properties.Review Plan
Tests
Testing Framework
Tooling changes
Transaction.gas_limitnow defaults toNoneinstead ofHexNumber(21_000).Transaction.set_gas_limit()resolves an unset limit in place, and signing now raisesValueError("gas_limit must be set to sign a transaction")if the limit is still unset by the time the transaction is signed.This lead to many typing issues that have now been resolved, but it can be argued that a better choice is to leave
HexNumberas the type forTransaction.gas_limitand rely instead onmodel_fields_setto determine whether the gas limit has been set by the tester.BlockchainTestandStateTestFillersFor State tests (
StateTest), an unsetgas_limitis filled with the fork'stransaction_gas_limit_cap(), falling back to the environmentgas_limitwhen there is no cap.For Blockchain tests (
BlockchainTest), the block's remaining gas (env.gas_limitminus the gas already claimed by transactions with explicit limits) is split evenly across the transactions that leavegas_limitunset, clamped to the fork's transaction gas-limit cap. It raises a clear "test correctness" error if no gas remains.Execute Tests
The same logic is implemented for the execute path (
calculate_max_transaction_gas_limitinexecution/base.py, applied intransaction_post.pyandblob_transaction.py), so implicit gas limits work when running against a live client, not just during filling.It can be argued that there's a better path to resolve the transaction gas limit in the case of execute, such as:
eth_estimateGasand use the value returned, with the minor caveat that the contracts that the transaction requires have to be already on chain before calling this RPC method.Both methods require more work and could be implemented in a future PR.
State gas reservoir (EIP-8037)
Rather than special-casing EIP-8037 (
fork.is_eip_enabled(8037)), this PR introduces a fork-level concept:Fork.state_gas_reservoir_enabled()(alongside the existingtransaction_gas_limit_cap()), defaulting to False and turned on by EIP-8037, currently in Amsterdam.Transaction.state_gas_reservoirfield. When a fork enables the reservoir, the transaction gas-limit cap is treated as removed andset_gas_limit()accounts for the requested reservoir (transaction_gas_limit_cap + state_gas_reservoir) when sizing the implicit limit.tx.state_gas_reservoir = state_gas_reservoirinstead oftx.gas_limit = gas_limit_cap + state_gas_reservoir).Test cleanup
The implicit gas limit was applied across the test suite, removing explicit
gas_limitassignments from transactions wherever the precise limit was irrelevant to what the test exercises. This touches ~509 test files (net ~7,200 lines removed) spanning Frontier through Amsterdam and many individual EIPs (1153, 4844, 5656, 6780, 7516, 7708, 7843, 7928, 8024, Blake2, etc.).Tests that should rely on the implicit gas limit are those that only need the transaction to execute fully without running out of gas. Tests that intentionally probe gas-limit behavior continue to set it explicitly.
Incidental fixes surfaced while porting tests
pre.fund_eoadefault bumped from10**21to10**27to fix deposit tests that were under-funded.🔗 Related Issues or PRs
Closes #2248:
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture