fix(test-types,test-specs,test-execute): Harden implicit gas-limit resolution#6
Open
danceratopz wants to merge 6 commits into
Open
Conversation
…g gas `calculate_max_transaction_gas_limit()` returned 0 both when no transaction required an implicit gas limit (benign) and when explicit transaction gas limits already consumed the full environment gas limit (a test correctness error). The execute formats (`TransactionPost`, `BlobTransaction`) call it unconditionally from `prepare_transactions()` and never inspect the return value, so in the over-subscribed case the ambiguous 0 flowed into `set_gas_limit(max_gas_limit=0)`: transactions were signed with `gas_limit=0` and sent to the client, failing late with a confusing client-side rejection instead of a clear test definition error. Split the sentinel into its two meanings: return 0 only when there is nothing to do, and raise a "test correctness" error when transactions with implicit gas limits are left without remaining gas. This puts the check inside the calculation itself so every present and future caller gets it, rather than relying on a call-site convention.
Mirror the equivalent change made to `BaseExecute` in `execution/base.py`: `BlockchainTest.calculate_max_transaction_gas_limit()` now raises the "test correctness" error directly when transactions with implicit gas limits are left without remaining gas, instead of returning an ambiguous 0 that callers must remember to check. The caller-side check in `generate_block_data()` is now dead code and is removed: the function can no longer return 0 there, since it is only invoked when at least one transaction has an unset gas limit. No behavior change on the fill path: the same condition raises the same kind of error, one frame earlier.
…s-limit calculation `calculate_max_transaction_gas_limit()` existed as two verbatim copies on either side of the spec/execute split: one on `BlockchainTest` for the fill path and one on `BaseExecute` for the execute formats. The function encodes the policy for sizing implicit transaction gas limits (even split of remaining environment gas, capping, state gas reservoir handling), and both copies must agree for filled and executed tests to behave the same: the recently added no-remaining-gas check already had to be applied twice. Hoist it to a single module-level function in `test_types/transaction_types.py`, next to `Transaction.set_gas_limit()`, so that all implicit gas-limit policy lives in one place. Neither class used `self` or `cls` (both copies were static), so a free function is the natural shape. The parameter is narrowed from `env: Environment` to `env_gas_limit: int`: the function only ever read `env.gas_limit`, and taking the full `Environment` would have required `transaction_types` to depend on `block_types` for a single field.
…ported fork
`Transaction.state_gas_reservoir` is only meaningful on forks where
`fork.state_gas_reservoir_enabled()` is true (currently Amsterdam via
EIP-8037). Previously, a positive reservoir on any other fork was
silently dropped by `set_gas_limit()`: clamped to the gas limit cap on
Osaka, ignored entirely pre-Osaka. A test parametrized across a fork
range could therefore appear to cover reservoir behavior while
actually filling plain capped transactions, with no signal to the
author.
Raise a "test correctness" error when a transaction requests a
positive reservoir on a fork that cannot honor it. The guard runs
before the unset-gas-limit check so that transactions with explicit
gas limits are also caught, and it deliberately uses
`state_gas_reservoir > 0` rather than `model_fields_set` so the
explicit `state_gas_reservoir=0` idiom (pin the gas limit to exactly
the cap, a no-op before EIP-8037) keeps working across fork ranges.
No current test trips the guard: all positive usages are in
`tests/amsterdam/eip8037_*` behind `valid_at("EIP8037")` markers, and
the fork transition test only attaches reservoir transactions to
post-transition blocks.
…r semantics
The check that a state gas reservoir request fits within the available
gas was a bare `assert` with no message. The condition is reachable
through normal test authoring: on the blockchain fill path,
`max_gas_limit` is the per-transaction even split of the block's
remaining gas, so a block crowded with enough implicit-gas-limit
transactions pushes the share below `transaction_gas_limit_cap +
state_gas_reservoir` and the fill died with an opaque
`AssertionError` pointing into framework internals. Raise a "test
correctness" error instead, reporting the requested reservoir, the
required gas limit, and the gas actually available.
The adjacent assert that a reservoir-enabled fork carries a gas limit
cap kept as an `assert` (it guards an internal invariant that test
authors cannot violate through the `Transaction` API), but its message
had a typo ("set calculate") and presumed the source of the
inconsistency. State the violated precondition neutrally (both
argument names and values) followed by the protocol rationale: the
reservoir is defined as gas above the cap, as EIP-8037 builds on
EIP-7825.
Also document the three-state semantics of `state_gas_reservoir`,
which previously lived only in the implementation: unset keeps the
full implicit gas limit, an explicit 0 pins the gas limit to exactly
the cap, and a positive value pins it to the cap plus the requested
reservoir. The field `description` and the expanded `set_gas_limit`
docstring now both spell this out.
`Transaction.set_gas_limit()` and `calculate_max_transaction_gas_limit()` size every transaction in the suite that omits an explicit gas limit, but had no direct unit coverage: existing framework tests were only updated to pass explicit limits. Lock in the established behavior: - Base resolution: unset limits resolve to the maximum, clamped to the fork's transaction gas limit cap; explicit limits (including the explicit `gas_limit=None` unset idiom) are never modified; the resolution is sticky across repeated calls; signing without a gas limit raises. - State gas reservoir (EIP-8037) three-state semantics: unset keeps the full uncapped maximum, an explicit 0 pins the gas limit to exactly the cap (also valid on forks without a reservoir), and a positive value pins it to the cap plus the requested reservoir. - Test correctness errors: a reservoir exceeding the available gas, a positive reservoir on a fork without the state gas reservoir (for both implicit and explicit gas limits), and explicit limits leaving implicit transactions no remaining environment gas. - Even split: remaining environment gas is divided across implicit transactions after deducting explicit limits, clamped to the cap on Osaka and uncapped on Amsterdam where the reservoir removes it; all benign cases (no implicit transactions, empty list) return 0 without raising.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🗒️ Description
Hardens the implicit gas-limit machinery from ethereum#2969: clear "test correctness" errors instead of silent
gas_limit=0transactions, droppedstate_gas_reservoirrequests, and bare asserts; deduplicatescalculate_max_transaction_gas_limit(); adds unit tests. Each commit message explains its change in detail.🔗 Related Issues or PRs
Suggestions for ethereum#2969.
A reservoir-adjacent animal