execution: leave gas pool and state untouched for rejected transactions#21907
execution: leave gas pool and state untouched for rejected transactions#21907yperbasis wants to merge 27 commits into
Conversation
Align TxnExecutor.Execute with the execution-spec (and geth) ordering: the intrinsic-gas check — regular intrinsic (regular + EIP-8037 state) and the EIP-7623 calldata floor — now runs before preCheck/buyGas and the nonce increment, so an intrinsic-gas rejection leaves sender state untouched. The accept/reject predicate is unchanged. Also report the binding bound (max of regular intrinsic and the floor) in the error instead of only the regular intrinsic component.
Relocate the intrinsic-gas validation (regular + EIP-8037 state, and the EIP-7623 floor) from Execute into preCheck as clause 7, next to the EIP-7825 cap and ahead of buyGas, keeping it before any state mutation. The consensus-rule clause list moves onto preCheck's docstring where the checks now live. No behavior change.
The relocated clause list kept geth's legacy numbering; reorder it to the sequence the checks actually run in, and move the two enforced elsewhere (intrinsic-gas overflow, value transfer) to a trailing note.
preCheck now calls calcIntrinsicGas itself and returns the result for Execute's gas accounting, instead of receiving it as a parameter. This mirrors the execution-spec's validate_transaction, which computes and returns the intrinsic cost as its first step. No behavior change.
…ility Run the intrinsic/floor check between buyGas's affordability check and the balance debit, rather than ahead of buyGas. A tx that fails both checks again reports "insufficient funds" before "intrinsic gas too low" — geth's precedence, which rpc-tests eth_callMany/test_04 pins — while the sender is still not charged for an under-gassed tx: the debit and the nonce increment stay after the intrinsic check.
… doc preCheck computes intrinsic gas now, so the overflow check is again one of its rules; list it as clause 1 and renumber the rest.
…se 7 Restores full 7-clause parity with main: the value-transfer affordability is enforced by the EVM during the call, annotated as such, and ordered last.
…r rule The topmost-call value-transfer affordability is folded into buyGas's balance check (balance >= gas*feeCap + value), not enforced by the EVM, and it runs before the intrinsic check. Order it as clause 6 (the value term of the gas-fee balance check) ahead of intrinsic usage, and drop the inaccurate "checked by the EVM" note.
…Check
Keep the computed intrinsic gas in a TxnExecutor field (alongside the
other per-tx computed values) instead of returning it from preCheck and
threading it into buyGas. preCheck now returns just error with bare
returns and ends with `return st.buyGas(gasBailout)`; buyGas and
calcIntrinsicGas drop their params, with calcIntrinsicGas deriving its
inputs from st.msg. Also tighten the clause-list doc ("main rules") and
the Execute comment (the debit lives in buyGas).
MakePreState applied EIP-161 empty-account clearing when committing the alloc, dropping a genesis-declared empty (balance=nonce=code=0) account from the pre-state — diverging from geth, whose pre-state commit uses deleteEmptyObjects=false. Disable the clearing for the alloc commit (EIP-161 still applies during execution), matching geth and the existing CommitOverrideDirtyAccounts precedent.
…37 note Rename the TxnExecutor field to st.intrinsicGas and the regular+state sum locals to intrinsicGasSum (buyGas, ApplyFrame), and restore the EIP-8037 "intrinsic_gas = regular + state; cover the sum, not each component" comment on the check. No behavior change.
yperbasis
changed the title
…tate" This reverts commit cb6453c.
yperbasis
changed the title
There was a problem hiding this comment.
Pull request overview
This PR changes TxnExecutor’s validation/debit ordering so transactions rejected for intrinsic-gas validation (including the EIP-7623 calldata floor and EIP-8037 regular+state intrinsic sum) do not mutate sender state (nonce/balance), while preserving “insufficient funds” error precedence—aligning behavior with geth/execution-spec for Execute-based paths (e.g., evm t8n, parallel exec, trace_call).
Changes:
- Compute and store intrinsic-gas once during
preCheck, then enforce intrinsic-gas constraints insidebuyGasbefore debiting sender balance / incrementing nonce. - Update gas accounting paths to reuse the stored intrinsic-gas result.
- Add tests pinning (a) no sender-state mutation on intrinsic-gas rejection and (b) insufficient-funds precedence over intrinsic-gas errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| execution/protocol/txn_executor.go | Reorders/centralizes intrinsic-gas computation and moves intrinsic-gas rejection ahead of sender-state mutations in Execute. |
| execution/protocol/txn_executor_test.go | Adds regression tests for sender-state immutability on intrinsic-gas reject and for error-ordering parity with geth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Restore the salient point in preCheck's docstring: the gas fee and block pool gas are charged only after every check passes, so a rejected tx leaves sender state untouched and consumes no pool gas.
ApplyFrame (RIP-7560) now validates intrinsic gas before verifyAuthorities mutates state, and buyGas reserves blob gas from the block pool only after the affordability and intrinsic checks pass — so a rejected tx/frame leaves both sender state and the pool untouched. Adds regression tests for each.
yperbasis
changed the title
The deterministic SetCode/type-4 prerequisites (type-4 before Prague, contract creation with type-4, empty authorization list) were checked in verifyAuthorities — after the gas debit and nonce increment in Execute — so a tx rejected for them mutated sender state. Move them into preCheck (via a shared checkSetCodeAuthorizations helper), before buyGas, so every consensus reject leaves sender state untouched. Adds TestType4Prereq_NoStateMutationOnReject.
…clusion Extend CheckBlockGasInclusion to also check the EIP-4844 blob-gas pool, so preCheck validates blob availability alongside regular/state gas (returning ErrBlobGasLimitReached). Blob-pool exhaustion now surfaces during validation, consistent with regular gas, instead of in buyGas. buyGas's SubBlobGas remains the decrement, mirroring ConsumeRegular. Also align the file-level lifecycle sketch with the actual flow. Behavior is unchanged where blob gas is available.
Execute aliased st.intrinsicGas into intrinsicGasResult; ApplyFrame kept its own local. Use the st.intrinsicGas field directly in both (ApplyFrame stores its calcIntrinsicGas result there), so the field is the single source.
Move checkSetCodeAuthorizations after the fee-cap/blob-price/EIP-7825 checks and before the affordability and intrinsic checks, matching geth's preCheck precedence (nonce, EOA, feeCap, blob-fee, SetCode, balance, intrinsic). Order does not affect consensus; this aligns the rejection precedence with the reference clients.
… gas CheckMaxInitCodeSize ran in Execute after buyGas and SetNonce, so a contract-creation tx with oversized initcode but enough gas to clear the intrinsic check mutated sender nonce/balance before being rejected (the same divergence class as the EIP-7623 floor bug; geth's t8n reverts a rejected tx, Erigon's does not). Move the check into preCheck, before any state mutation. Also enumerate every preCheck consensus check in its docstring.
The docstring re-enumeration renumbered the clauses; update the inline comments to match (balance is clauses 10-11, intrinsic 12, initcode 13).
buyGas wrote evm.BlobFee and Execute read it back within the same call; the value already lives in st.blobGasVal (computed in preCheck). Read blobGasVal directly at the Gnosis/Aura blob-fee burn and remove the now-unused BlobFee field from TxContext, eliminating a mid-execution mutation of EVM state.
yperbasis
changed the title
The affordability check dereferenced MaxFeePerBlobGas() under IsCancun alone; a non-blob tx whose Message returns a nil cap (e.g. the simulated backend's callMsg) would panic. Gate the blob-fee term on BlobGas() > 0, matching the blob fee-cap check; the term is zero for non-blob txs anyway.
yperbasis
changed the title
preCheck's affordability check now skips the blob-fee term when MaxFeePerBlobGas() is nil (possible for non-blob-tx Message impls in NoBaseFee call/trace contexts), treating it as zero — a no-op for real blob txs, which always carry a cap. CheckBlockGasInclusion can now fail on blob gas too, so the parallel-exec wrapper no longer mislabels every failure "block gas used overflow"; the wrapped error already names the specific dimension.
Replace the numbered clause list (and the fragile Clause-N cross-references in the body) with a one-line description at each check site, so the comments live next to the code they describe and can't drift from a separate list.
ApplyFrame writes st.intrinsicGas directly, so the field comment said 'computed by preCheck' alone was misleading.
AskAlexSharov
left a comment
AskAlexSharov
left a comment
There was a problem hiding this comment.
Automated review (high-effort, recall-biased). Overall a clean, well-tested refactor — no hard consensus bug; block validity is unchanged on every path. The notes below are error-variant precedence shifts (the eest/hive engine matrix is documented-sensitive to these — see execution/exec/txtask.go:398-417, PR #21017), two Execute↔ApplyFrame inconsistencies, one latent footgun, and a couple of altitude notes.
|
|
||
| // Check whether the init code size has been exceeded. | ||
| if contractCreation { | ||
| if err := vm.CheckMaxInitCodeSize(uint64(len(st.data)), isEIP3860, rules.IsAmsterdam); err != nil { |
There was a problem hiding this comment.
ApplyFrame vs Execute disagree on EIP-3860-initcode vs intrinsic-gas precedence. Here ApplyFrame checks initcode size before the intrinsic-gas check (line 451). In Execute/preCheck the order is reversed: intrinsic gas (preCheck ~line 399) then initcode (~line 405). For an oversized-initcode creation that also underpays intrinsic gas, ApplyFrame returns ErrMaxInitCodeSizeExceeded while Execute returns ErrIntrinsicGas. geth orders intrinsic-before-initcode (the Execute order), so ApplyFrame is the outlier — and both functions were touched here, so the two paths could be unified.
| if err := CheckBlockGasInclusion(st.gp, regularContribution, stateContribution); err != nil { | ||
| // The block must have enough regular, state, and blob gas left for the tx. | ||
| regularContribution, stateContribution := InclusionContributions(st.msg.Gas(), st.intrinsicGas, rules.IsAmsterdam) | ||
| if err := CheckBlockGasInclusion(st.gp, regularContribution, stateContribution, st.msg.BlobGas()); err != nil { |
There was a problem hiding this comment.
Blob dimension shifts the blob-limit error ahead of the fee-cap/affordability checks. CheckBlockGasInclusion now returns ErrBlobGasLimitReached here, before the EIP-4844 blob-fee-cap check (~line 314), the EIP-7825/EIP-1559 caps, and the affordability check (~line 359). A blob tx that both over-budgets the block's blob pool and fails one of those later checks now reports ErrBlobGasLimitReached, where the old order (blob exhaustion caught later in buyGas's SubBlobGas) reported the fee-cap/funds variant. execution/exec/txtask.go:398-417 documents that the eest engine matrix asserts specific error variants (the GAS_ALLOWANCE_EXCEEDED vs INSUFFICIENT_MAX_FEE_PER_GAS red on PR #21017) — worth running the parallel-exec eest legs before merge.
| return st.buyGas(gasBailout) | ||
| // EIP-7702: reject malformed SetCode transactions. Placed after the fee-cap | ||
| // checks and before the affordability/intrinsic checks, matching geth. | ||
| if err := checkSetCodeAuthorizations(st.msg.Authorizations(), st.msg.To().IsNil(), rules.IsPrague); err != nil { |
There was a problem hiding this comment.
EIP-7702 setcode-prereq precedence change is untested for the combined case. Hoisting this before the affordability (line 359) and intrinsic (line 399) checks now matches geth (geth validates setcode prereqs in preCheck before buyGas) — good — but it flips erigon's own historical precedence: a malformed type-4 tx (pre-Prague / contract-creation / empty auths) whose sender also can't afford gas or is below intrinsic now returns the setcode error instead of ErrInsufficientFunds/ErrIntrinsicGas. TestType4Prereq_NoStateMutationOnReject uses a funded, above-intrinsic sender, so the combined-failure precedence the comment asserts isn't actually pinned by a test.
| func (st *TxnExecutor) verifyAuthorities(auths []types.Authorization, contractCreation bool, chainID string) ([]accounts.Address, uint64, error) { | ||
| var stateIgasRefund uint64 | ||
| verifiedAuthorities := make([]accounts.Address, 0) | ||
| if err := checkSetCodeAuthorizations(auths, contractCreation, st.evm.ChainRules().IsPrague); err != nil { |
There was a problem hiding this comment.
EIP-7702 prerequisites are now validated twice per tx on the Execute path — once via checkSetCodeAuthorizations in preCheck (line 340) and again here inside verifyAuthorities. Harmless today (pure function), but it's redundant per-tx work and two call sites of one rule that can drift; ApplyFrame relies only on this copy. Consider one canonical stateless gate both entry points call once, separated from the state-mutating remainder of verifyAuthorities.
| } | ||
| st.state.SubBalance(st.msg.From(), gasVal, tracing.BalanceDecreaseGasBuy) | ||
| st.state.SubBalance(st.msg.From(), blobGasVal, tracing.BalanceDecreaseGasBuy) | ||
| st.state.SubBalance(st.msg.From(), st.gasVal, tracing.BalanceDecreaseGasBuy) |
There was a problem hiding this comment.
Temporal coupling: buyGas silently depends on preCheck having populated st.gasVal/st.blobGasVal. I confirmed buyGas is only ever called immediately after preCheck today, so no active bug — but the fields default to zero and the ordering contract is unenforced by the type system. A future caller or a reorder of Execute that runs buyGas without preCheck would SubBalance(sender, 0) here — gas bought for free, no panic, no compile error. Prefer preCheck returning a small result struct that buyGas takes as a parameter, so the dependency lives in the signatures.
| return nil, ErrGasUintOverflow | ||
| } | ||
| if st.msg.Gas() < intrinsicGas { | ||
| if st.msg.Gas() < intrinsicGasSum { |
There was a problem hiding this comment.
ApplyFrame omits the EIP-7623 FloorGasCost lower bound that Execute enforces. Execute/preCheck also rejects when Gas() < intrinsicGas.FloorGasCost (~line 399); here only Gas() < intrinsicGasSum is checked. On Prague+, a heavy-calldata tx whose gas sits between the intrinsic sum and the calldata floor is accepted by ApplyFrame but rejected by Execute. Pre-existing, but worth confirming it's intended for the RIP-7560 path while the function is being rewritten.
| // remaining budgets: regular and state in the EIP-8037 reservoirs, and blob in | ||
| // the EIP-4844 blob-gas pool. Callers obtain the regular and state contributions | ||
| // from InclusionContributions; blobGas is the transaction's blob gas. | ||
| func CheckBlockGasInclusion(gp *GasPool, regularGas, stateGas, blobGas uint64) error { |
There was a problem hiding this comment.
CheckBlockGasInclusion now conflates two budget models. The EIP-4844 blob-pool check is folded into a helper named and documented for the EIP-8037 regular/state reservoirs, and blob availability is then checked again in buyGas via SubBlobGas. Consider keeping the reservoir check and the blob-pool check as separate single-responsibility predicates that preCheck composes — the third positional uint64 meaning "blob gas" is easy to mis-pass (e.g. 0 or the wrong dimension).
| // EIP-4844: the max fee per blob gas must cover the blob gas price. | ||
| if st.msg.BlobGas() > 0 && rules.IsCancun { | ||
| blobGasPrice := st.evm.Context.BlobBaseFee | ||
| maxFeePerBlobGas := st.msg.MaxFeePerBlobGas() |
There was a problem hiding this comment.
Asymmetric nil-handling of MaxFeePerBlobGas(). The blob-fee-cap check just below (blobGasPrice.Cmp(maxFeePerBlobGas), ~line 318) dereferences the cap without the maxFeePerBlobGas != nil guard that the affordability check at ~line 372 was deliberately given (uint256.Int.Cmp derefs its arg → panic on nil). Safe for the production *Message type (MaxFeePerBlobGas() returns &m.maxFeePerBlobGas, never nil), but reachable for callMsg/eth_call-style impls with blob hashes and a nil cap in a !NoBaseFee context. Low severity — mirror the line-372 guard here for consistency.
A transaction (or RIP-7560 frame) rejected during validation now leaves sender state and the block gas pools untouched.
TxnExecutorcleanly separates validation from gas buying.preCheckperforms all consensus validation and mutates nothing; only once it passes doesExecutecallbuyGas, which debits the gas/blob fee and reserves blob gas from the block pool. Because every check precedes any mutation, a transaction rejected for any consensus reason — bad nonce, insufficient funds, intrinsic gas or the EIP-7623 calldata floor too low, the EIP-7825 gas-limit cap, the EIP-7702 SetCode prerequisites, oversized contract-creation initcode (EIP-3860), or insufficient block regular/state/blob gas — leaves the sender's nonce and balance unchanged and consumes no pool gas.Error precedence matches geth (e.g.
insufficient fundsbeforeintrinsic gas too low). The RIP-7560ApplyFramepath gets the same guarantee: it validates intrinsic gas beforeverifyAuthoritiesmutates state.This matters for
evm t8nand the parallel-exec /trace_callpaths that shareExecute; in live block validation such a transaction invalidates the whole block on every client. The separateevm t8nempty-account difference is handled in #21915.Closes ethereum-bounty/erigon#16