EIP-8141: in-pool validation-prefix simulation for opaque frame transactions (Phase 2) - #12624
EIP-8141: in-pool validation-prefix simulation for opaque frame transactions (Phase 2)#12624Marchhill wants to merge 4 commits into
Conversation
|
Claude finished @Marchhill's task in 11m 29s —— View job Review: EIP-8141 Phase 2 — in-pool validation-prefix simulation
The architecture here is sound and the hard parts are done well. Reusing The findings below are about the trace rules being incompletely enforced relative to what the code claims, plus diagnostics and cancellation. High1. The Medium2. 3. 4. The post-frame 5. Low6. 7. 8. 9. An under-funded payer is reported as 10. Test coverage of the trace rules — the security surface of this PR — is thin: one banned opcode of ~20, and no coverage of the Notes (no action)
I could not verify compilation — |
| /// EIP8141 follow-ups (design note §4 "Alternative C"): the first-<c>deploy</c>-frame carve-outs for | ||
| /// <c>CREATE</c>/<c>CREATE2</c>/<c>SETDELEGATE</c> and <c>SSTORE</c>-to-sender, and the | ||
| /// <c>CALL*</c>/<c>EXTCODE*</c> target-existence and EIP-7702 checks, are not yet enforced — prefixes | ||
| /// that need them are conservatively rejected here (declining is always spec-compliant, L684). |
There was a problem hiding this comment.
High — the doc claim is inverted for CALL*/EXTCODE*: those prefixes are silently admitted, not rejected.
The CREATE/CREATE2/SETDELEGATE/SSTORE half of this claim holds — those are banned unconditionally below, so a first-deploy-frame prefix is genuinely declined. But nothing in this tracer inspects CALL/CALLCODE/DELEGATECALL/STATICCALL or EXTCODESIZE/EXTCODEHASH/EXTCODECOPY targets at all, so a prefix that needs the deferred existence / EIP-7702 checks passes the tracer clean and the tx is accepted.
The spec rule is: "CALL* and EXTCODE* may target existing contracts or precompiles, provided the resulting trace respects storage, opcode, and EIP-7702 restrictions." So skipping it is a deviation in the permissive direction, not the conservative one — the "declining is always spec-compliant (L684)" justification doesn't apply here. Concretely: an opaque prefix that STATICCALLs an empty account (or EXTCODEHASHes one) is admitted, and its validity then depends on that account staying codeless — an unindexed mempool dependency, which is exactly what the rule guards against. With head-change re-simulation also deferred, that entry sits in the pool with a payer that can silently go stale.
Either add the existence check (WorldState.GetCodeHash(target) != Keccak.OfAnEmptyString || target is a precompile → else Violate, which needs the tracer to see call targets — ReportAction gives you ExecutionEnvironment.CodeSource), or correct the comment to say these prefixes are currently admitted unchecked and mark it an EIP8141: gap. As written the comment tells a reviewer a security-relevant rule is enforced conservatively when it is not enforced at all.
There was a problem hiding this comment.
Addressed: the tracer now enforces the CALL*/EXTCODE* target rule (StartOperation flags the target stack slot, SetOperationStack classifies it via IsForbiddenCallTarget — codeless or EIP-7702-delegated non-precompile, non-sender targets are rejected). The class doc no longer claims these are declined unconditionally.
| case Instruction.TIMESTAMP: | ||
| // Permitted only inside the canonical expiry verifier frame (L779). | ||
| if (env.ExecutingAccount != expiryVerifier) Violate("banned opcode TIMESTAMP in validation prefix"); | ||
| break; |
There was a problem hiding this comment.
Medium — the TIMESTAMP exemption checks the address but not the code, while the spec pins both.
EIP-8141: "TIMESTAMP (0x42) — except in expiry verifier frames executing canonical runtime code." This only compares env.ExecutingAccount to the predeploy address, so if 0x…8141 carries anything other than Eip8141Constants.ExpiryVerifierCode (a devnet where the predeploy was never installed, or a chain that deployed a modified verifier), arbitrary TIMESTAMP-dependent code at that address is exempted from the ban.
The check is cheap and the constant already exists — Eip8141Constants.ExpiryVerifierCode — and FrameTxPayerResolver already treats the verifier's code hash as part of the dependency set (expiryCodeHash), so the possibility of non-canonical code is already acknowledged elsewhere in the stack. Passing the expected code hash into the tracer and comparing env.CodeInfo / the code source's hash would close it.
| // GAS is permitted only when immediately followed by a *CALL (the standard gas-forwarding | ||
| // idiom, which adds no public-mempool dependency); otherwise it is banned. | ||
| if (_pendingGasRequiresCall) | ||
| { | ||
| _pendingGasRequiresCall = false; | ||
| if (!IsCall(opcode)) | ||
| { | ||
| Violate("GAS not immediately followed by a call"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Medium — _pendingGasRequiresCall is never flushed at a frame/execution boundary, so the GAS rule is both mis-attributed and under-enforced.
The flag is only consulted on the next StartOperation, and one tracer instance spans every frame of the prefix. Two consequences:
- Missed violation. If
GASis the last instruction actually executed — code ends (implicitSTOP), or the following instruction faults beforeStartOperationfires (out-of-gas / stack underflow on a*CALL) — no furtherStartOperationarrives and the violation is never recorded. A prefix that ends… GASis admitted despiteGASnot being followed by a*CALL. - Mis-attribution. If frame i ends with a pending
GAS, the flag survives into frame i+1 and the violation is reported against that frame's first opcode. The accept/reject verdict is still correct here, butViolationReason— which is surfaced verbatim in theAcceptTxResultmessage — points at the wrong frame.
Both go away if the pending check is resolved at the source instead of a state flag: the code is available on env.CodeInfo at StartOperation, so GAS can be validated by peeking the byte at pc + 1 and requiring it to be a *CALL. That also removes the mutable field.
| // Cap the frame's gas so cumulative validation work cannot exceed MAX_VERIFY_GAS even | ||
| // for an opaque prefix whose declared gas_limits were never structurally bounded. | ||
| ulong remainingVerifyGas = Eip8141Constants.MaxVerifyGas - verifyGasUsed; | ||
| ulong frameGasLimit = Math.Min(frame.GasLimit, remainingVerifyGas); | ||
| TxFrame boundedFrame = frameGasLimit == frame.GasLimit | ||
| ? frame | ||
| : new TxFrame(frame.Mode, frame.Flags, frame.Target, frameGasLimit, frame.Value, frame.Data); | ||
|
|
||
| Address resolvedTarget = frame.Target ?? sender; | ||
| Address caller = Eip8141Constants.EntryPointAddress; | ||
|
|
||
| VirtualMachine.SetTxExecutionContext(new TxExecutionContext( | ||
| caller, _codeInfoRepository, tx.BlobVersionedHashes, in effectiveGasPrice, frameContext)); | ||
|
|
||
| TransactionSubstate substate = ExecuteFrame(boundedFrame, resolvedTarget, caller, isStatic: true, frameContext, in accessTracker, spec, tracer, out ulong frameGasUsed); | ||
|
|
||
| verifyGasUsed += frameGasUsed; | ||
| if (verifyGasUsed > Eip8141Constants.MaxVerifyGas) | ||
| { | ||
| return TransactionResult.ErrorType.MalformedTransaction.WithDetail("frame transaction validation prefix exceeds MAX_VERIFY_GAS"); | ||
| } |
There was a problem hiding this comment.
Medium — the post-frame MAX_VERIFY_GAS check is unreachable, so an over-budget prefix is reported as a revert.
frameGasLimit = Math.Min(frame.GasLimit, remainingVerifyGas) and ExecuteFrame computes gasUsed = frame.GasLimit - remainingGas off the bounded frame, so frameGasUsed ≤ frameGasLimit ≤ MaxVerifyGas - verifyGasUsed. The invariant verifyGasUsed ≤ MaxVerifyGas therefore holds unconditionally after line 445, and the if (verifyGasUsed > Eip8141Constants.MaxVerifyGas) on 446-449 can never fire.
The observable effect is that an over-budget prefix always surfaces through the OOG path below as "validation prefix frame reverted" / AcceptTxResult.FrameSimulationFailed, never as the MAX_VERIFY_GAS reason — so AcceptTxResult.VerifyGasExceeded is unreachable from the simulated path even though it exists for the arithmetic pre-check in #12620. FrameTxValidationPrefixSimulationTests.Simulate_PrefixExceedsMaxVerifyGas_Rejected documents the symptom in its own comment ("the gas cap forces an out-of-gas revert") and asserts only TransactionExecuted is false, so it passes either way.
Two reasonable resolutions: keep the cap and report exhaustion explicitly (bool capped = frameGasLimit < frame.GasLimit; → on substate.IsError && capped, return the MAX_VERIFY_GAS error and have the filter map it to AcceptTxResult.VerifyGasExceeded), or drop the dead check and say in the remarks that budget exhaustion is deliberately surfaced as a revert. Either way an operator debugging why a frame tx was dropped currently gets the wrong reason.
| } | ||
|
|
||
| /// <summary> | ||
| /// Outcome of an <see cref="IFrameTxPrefixSimulator.Simulate"/> call: whether the prefix is admissible |
There was a problem hiding this comment.
Medium — Simulate should take a CancellationToken now, while the interface is still new.
Incoming P2P transactions reach TxPool.SubmitTx from Eth62ProtocolHandler.HandleSlow, which runs on the BackgroundTaskScheduler and is handed a CancellationToken it checks between transactions. Simulate is a synchronous call that runs up to MAX_VERIFY_GAS of EVM work and — because FrameTxPrefixSimulator serializes every call on one lock (necessarily, the read-only env is not thread-safe) — can also sit blocked behind other peers' simulations for an unbounded time. None of that is observable to the scheduler's token, so a shutdown or a scheduler-level cancel cannot interrupt it, and the deferred "wall-clock cancellation guard" has nowhere to attach.
Adding the token now costs one parameter; adding it later is a breaking signature change on a public interface (.agents/rules/robustness.md: long-running work must be cooperative with graceful shutdown). Threading the scheduler's token through IIncomingTxFilter is the larger part, so CancellationToken token = default on this method plus a token.ThrowIfCancellationRequested() between prefix frames would be a reasonable first step.
There was a problem hiding this comment.
Addressed: Simulate now takes a CancellationToken (default) and throws on entry if cancelled; FrameTxPrefixSimulator re-throws OperationCanceledException rather than masking it.
| // A null payer is either a provably-invalid legible prefix (NoPayer) or an opaque one that | ||
| // needs simulation; only the latter is simulated. Re-resolving is cheap (native, ≤2 reads) | ||
| // and reached only for the rare unresolved frame tx, not the common resolved fast path. | ||
| if (FrameTxPayerResolver.Resolve(tx, stateProvider).Outcome != FrameTxPayerOutcome.RequiresSimulation) | ||
| { | ||
| return AcceptTxResult.Accepted; | ||
| } |
There was a problem hiding this comment.
Low — FrameTxPayerResolver.Resolve now runs twice for every frame transaction.
FrameTxPayerFilter is an annotation step that never rejects, so it has already called Resolve (and computed the full FrameTxDependencySet) a moment earlier in the same filter chain; both results are discarded except for Payer. This re-resolve repeats TryGetAccount on the sender, GetCodeHash(ExpiryVerifierAddress), and TryGetAccount on the pay target.
The comment says it is "reached only for the rare unresolved frame tx", which is true, but the dependency set is exactly what the deferred result-caching and head-change-reindexing follow-ups will need, so it will have to be carried through anyway. Stashing FrameTxPayerResolution on TxFilteringState in FrameTxPayerFilter (it is already a ref struct threaded through the chain) and reading state.PayerResolution.Outcome here avoids the duplicate work and pre-positions the caching phase — and it removes the need for this filter to take IReadOnlyStateProvider at all.
There was a problem hiding this comment.
Deferred. The re-resolve is only reached for the rare unresolved (opaque) frame tx, and stashing FrameTxPayerResolution on TxFilteringState is exactly the groundwork the deferred dependency-set caching phase needs (design note section 4), so I would rather land it with that phase than add the field now.
|
|
||
| BlockHeader? head = blockFinder.Head?.Header; | ||
| if (head is null) | ||
| { | ||
| return FrameTxSimulationResult.Reject("no chain head to simulate against"); | ||
| } | ||
|
|
||
| lock (_lock) | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return FrameTxSimulationResult.Reject("simulator disposed"); |
There was a problem hiding this comment.
Low — two issues in the guard block.
-
Missing tx-type guard.
TransactionProcessor.Executeonly routes toSimulateFrameValidationPrefixwhentx.Type == TxType.FrameTx; any other type falls through to the ordinary transaction path withopts = FrameValidationPrefixOnly, which carries neitherCommitnorRestore— i.e. a full transaction executed against the read-only world state with its mutations left in place. Today that is unreachable becauseFrameTxSimulationFiltergates ontx.SupportsFramesand the scopeReset()s on dispose, butIFrameTxPrefixSimulatoris public API, so atx.SupportsFramescheck next to theSenderAddresscheck is worth having (.agents/rules/robustness.md: validate inputs at the boundary). -
Head source diverges from the pool's. The pool's filters read state through
IChainHeadInfoProvider.ReadOnlyStateProvider, while this readsIBlockFinder.Head. The two can disagree across a head change, so theRequiresSimulationclassification and the simulation itself may be taken at different heads — with the result cached ontotx.PayerAddress. SinceIChainHeadInfoProvideralso exposes the head header, using it would keep one head notion across the whole admission path and drop the newIBlockFinderdependency.
There was a problem hiding this comment.
Deferred both. Keeping one head notion across resolve+simulate (switching the simulator off IBlockFinder onto IChainHeadInfoProvider) belongs with the explicitly-deferred head-change re-simulation work in the design note. The tx-type guard is unreachable today (the filter gates on SupportsFrames); noted for the boundary-hardening pass.
|
|
||
| public override bool IsTracingInstructions => true; | ||
| public override bool IsTracingOpLevelStorage => true; | ||
| public override bool IsTracingReceipt => true; |
There was a problem hiding this comment.
Low — IsTracingReceipt => true looks unnecessary and is asymmetric with the main path.
SimulateFrameValidationPrefix calls receiptTracer.ReportFrameTxReceipt(...) unconditionally (only pattern-matching on IFrameTxReceiptTracer), unlike the main path which gates the same call on tracer.IsTracingReceipt. MarkAsSuccess/MarkAsFailed are never reached on the simulation path either. So the only effect of this flag is to force TxTracer._isTracing and pay IsTracingReceipt-conditioned work elsewhere in the VM for no benefit.
Either drop it, or gate the ReportFrameTxReceipt call in the simulation path on it so the two paths agree on what the flag means.
|
|
||
| [Test] | ||
| public void Simulate_PrefixUsesBannedOpcode_RecordsViolation() | ||
| { | ||
| // TIMESTAMP is banned during the validation prefix outside the expiry verifier frame, even | ||
| // though the frame still calls APPROVE and would otherwise resolve a payer. | ||
| byte[] code = Prepare.EvmCode | ||
| .Op(Instruction.TIMESTAMP).Op(Instruction.POP) | ||
| .PushData(TxFrame.ApproveExecutionAndPayment).PushData(0).PushData(0).Op(Instruction.APPROVE).Done; | ||
| DeployContract(Sender, code, 1.Ether); | ||
| Transaction tx = FrameTx(nonce: 0, SelfVerifyFrame()); | ||
|
|
||
| (_, FrameTxValidationTracer tracer) = Simulate(tx); |
There was a problem hiding this comment.
Low — the trace rules are the security surface of this PR and are the least-covered part of it.
Simulate_PrefixUsesBannedOpcode_RecordsViolation covers exactly one opcode (TIMESTAMP) out of ~20 bans, and the remaining rules have no coverage at all:
SLOADscope —LoadOperationStorageis the only enforcement of the spec's "SLOAD may access onlytx.senderstorage" rule, and neither the allowed case (SLOADwhileExecutingAccount == sender) nor the rejected case (aCALLed contract reading its own storage) is tested. Worth noting that the rejected case is the canonicalonly_verify → paycode-carrying paymaster shape — the headlineRequiresSimulationcase in the PR description — so any paymaster that reads its own storage is rejected at admission. That is spec-conformant as written, but it is a significant behavioural consequence to pin with a test.GAS— neither the allowedGAS; CALLform nor a bareGASis tested.TIMESTAMPexemption — the positive case (TIMESTAMPinside the expiry-verifier frame is allowed) is untested, so a regression that bans it everywhere would go unnoticed.- Cumulative
MAX_VERIFY_GAS— the existing test uses a single 10M-gas frame. The interesting path is two prefix frames each individually under 100k whose sum is over, which is what theremainingVerifyGasaccumulation exists for.
Per .agents/rules/test-infrastructure.md and AGENTS.md ("parameterize a single test with [TestCase(...)] rather than copy-pasting the body"), the banned list is a natural [TestCase(Instruction.ORIGIN)] [TestCase(Instruction.BALANCE)] … over one body that deploys Op(op).Op(POP) + APPROVE and asserts Violated, which would cover the whole list for roughly the size of the current single test.
Also minor: this fixture has non-nullable fields assigned only in [SetUp] without #nullable enable (or = null!), while the sibling FrameTxSimulationFilterTests does enable it.
There was a problem hiding this comment.
Agree the trace rules are the security surface; the CALL*/EXTCODE* codeless/existing-contract cases now have Evm coverage, and the end-to-end sponsor path is covered in TxPoolTests. Expanding to a parameterized sweep of the full banned-opcode list plus the SLOAD/GAS/expiry-exemption cases is a reasonable test-only follow-up.
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 Regressions (1)
|
|
Addressed the review findings (commit 3811261). High — Medium — Medium — Medium — post-frame Medium — Build clean ( |
|
Claude finished @Marchhill's task in 7m 40s —— View job Re-review: commit
|
| # | Finding | Status |
|---|---|---|
| 1 (High) | CALL*/EXTCODE* silently admitted |
Fixed |
| 2 (Med) | TIMESTAMP exemption ignored the code |
Fixed |
| 3 (Med) | _pendingGasRequiresCall cross-frame leak |
Fixed |
| 4 (Med) | over-budget prefix misreported | Partially — misreport now inverted (see below) |
| 5 (Med) | no CancellationToken |
Signature added; enforcement is entry-only |
| 6–10 (Low) | duplicate Resolve, SupportsFrames guard / head source, IsTracingReceipt, under-funded-payer diagnostics, trace-rule test coverage |
Open, explicitly deferred by the author |
High #1 is genuinely closed. I checked the mechanics rather than the description:
- The stack indices are right —
_targetStackIndex = 1forCALL/CALLCODE/DELEGATECALL/STATICCALL([gas, address, …]) and0forEXTCODESIZE/EXTCODEHASH/EXTCODECOPY, matchingNativePrestateTracer.cs:137,149andTraceStack.Peek's top-down indexing. - The
StartOperation→SetOperationStackhandoff is safe:StartInstructionTrace(VirtualMachine.cs:1331) is the sole caller of both and always invokes them together withIsTracingStacktrue, so_targetStackIndexcannot survive into a different instruction. Thestack.Count <= indexguard is correct — a stack underflow halts the frame exceptionally and rejects the prefix anyway. IsContractbeforeIsDelegatedCodeis the right order for EIP-7702 (a delegated account has code), andtx.senderis exempted per the spec's default-code carve-out.- Both new tests are meaningful:
Simulate_PrefixCallsCodelessTarget_RecordsViolation(never-deployedAddressC) andSimulate_PrefixCallsExistingContract_Allowed, which pins the permissive side so the rule can't over-reject.
Medium #2 and #3 are closed, and I verified the two things that could have gone wrong:
- The
TIMESTAMPaddress+code-hash pair closes the residual hole completely. ADELEGATECALLinto the verifier's code leavesExecutingAccount ≠ expiryVerifier→ banned; aDELEGATECALLout of the verifier would keepExecutingAccountbut is impossible once the code hash matches, sinceExpiryVerifierCodecontains noDELEGATECALL.GetCodeHashresolves throughIAccountStateProvider's default member (ValueHash256), so the comparison is well-typed. - The
pc + 1peek is sound, which was not obvious: Nethermind has a second, pre-decoded dispatch loop (VirtualMachine.Stream.cs), and if it were used thepchanded toStartOperationwould not be a raw code offset.VirtualMachine.cs:1287gates the stream on!TTracingInst.IsActive, so an instruction-tracing run always takesRunByteCodeCore, whereprogramCounterindexesstack.Codedirectly. EOF is not in the tree, so there is no section-relative-pccase either.GAShas no immediate, sopc + 1is the next executed opcode.
New findings
Medium
capped inverts the misreport rather than removing it, because the loop bounds actual gas while the spec bound is on declared gas_limits — TransactionProcessorBase.FrameTx.cs:429-458
capped is derived from the declared limit but selects a reason about consumption, so the comment's invariant ("a frame capped to the remaining budget that then failed exhausted that cap") does not hold. An unrecognized-shape verify frame declaring 5M gas that REVERTs at 3k on a signature mismatch — the ordinary rejection — is now reported as MAX_VERIFY_GAS exceeded. An explicit ShouldRevert should never be attributed to the budget, and IsError isn't a reliable signal either since every exceptional halt consumes all remaining gas.
The root cause is worth fixing rather than patching the discriminator: MEMPOOL-RULES-DESIGN.md:46 records the bound as the sum of declared prefix gas_limit plus signature cost, which is what TryGetValidationPrefixVerifyGas / FrameTxVerifyGasFilter implement — but this loop accumulates actual frameGasUsed under a cap. Since TryGetValidationPrefixVerifyGas returns false for unrecognized shapes (precisely the shapes that reach the simulator), a prefix declaring 5M of verify gas but consuming 40k is admitted here while Direct Evaluation would reject it — the reverse of "Direct evaluation MUST apply the same limits as simulation." Accumulating declared limits and rejecting before execution makes the check exact, reachable, spec-aligned, and drops capped entirely. Suggested diff in the inline comment.
Also: the over-budget test uses SelfVerifyFrame, a recognized shape that FrameTxVerifyGasFilter rejects with VerifyGasExceeded before the simulator runs, so it doesn't cover the path this code actually serves.
Low
Per-frame cancellation is already available for ~4 lines, and would also stop burning budget after a violation — IFrameTxPrefixSimulator.cs:26-31
Honoring the token only at entry covers the moment the simulation is free. The interpreter already polls _txTracer.IsCancelled every 1024 opcodes (VirtualMachine.DispatchSpecialized.cs:62, gated by IsCancelable at VirtualMachine.cs:1256), so overriding IsCancelable/IsCancelled on FrameTxValidationTracer closes the deferred gap outright. Doing it as Violated || token.IsCancellationRequested also fixes a DoS-shaped detail that stands on its own: today a prefix that violates a rule on its first opcode still runs to completion, so each rejected tx costs a full MAX_VERIFY_GAS instead of ~0. The OperationCanceledException already has a handler and the processor's finally restores state, so nothing else moves.
tx.sender exemption and EIP-7702. Exempting sender from the target rules is right per the default-code carve-out, but when the sender is 7702-delegated the prefix's validity depends on the delegation target's code, which the dependency set does not appear to track (only the sender's own code hash, i.e. the designation). Harmless while re-simulation on head change is deferred, worth a line in the EIP8141: follow-up list.
Minor perf note (no action). IsDelegatedCode(Address) loads the target's full code, so an EXTCODESIZE on a 24 KiB contract pulls the whole code during admission. Bounded by MAX_VERIFY_GAS and code-cached; noting only because it's a per-opcode state read on a new path.
Still open from the prior review
Findings 6–10 are unaddressed by design, per the author's note. Of these, #10 is the one I'd still push on: the CALL*/EXTCODE* rule now has both a positive and a negative test, but the SLOAD-restricted-to-tx.sender rule (the only thing standing between a code-carrying paymaster and admission), the GAS; *CALL allowance, the positive TIMESTAMP-in-verifier exemption, and cumulative-across-frames gas remain untested, and the banned list still has one opcode of ~20 covered. A [TestCase]-parameterized body over the ban list is roughly the size of the current single test.
Not verified by execution — dotnet build/dotnet test aren't in my allowed tools here, so all of the above is from reading. The PR states the build is clean and the suites are green; CI will confirm.
• branch eip8141-mempool-phase2
| // Cap the frame's gas so cumulative validation work cannot exceed MAX_VERIFY_GAS even | ||
| // for an opaque prefix whose declared gas_limits were never structurally bounded. | ||
| ulong remainingVerifyGas = Eip8141Constants.MaxVerifyGas - verifyGasUsed; | ||
| ulong frameGasLimit = Math.Min(frame.GasLimit, remainingVerifyGas); | ||
| // When the frame's declared gas_limit exceeds the remaining budget, cumulative prefix | ||
| // gas already tops MAX_VERIFY_GAS; running under the cap tells whether that budget is hit. | ||
| bool capped = frameGasLimit < frame.GasLimit; | ||
| TxFrame boundedFrame = capped | ||
| ? new TxFrame(frame.Mode, frame.Flags, frame.Target, frameGasLimit, frame.Value, frame.Data) | ||
| : frame; | ||
|
|
||
| Address resolvedTarget = frame.Target ?? sender; | ||
| Address caller = Eip8141Constants.EntryPointAddress; | ||
|
|
||
| VirtualMachine.SetTxExecutionContext(new TxExecutionContext( | ||
| caller, _codeInfoRepository, tx.BlobVersionedHashes, in effectiveGasPrice, frameContext)); | ||
|
|
||
| TransactionSubstate substate = ExecuteFrame(boundedFrame, resolvedTarget, caller, isStatic: true, frameContext, in accessTracker, spec, tracer, out ulong frameGasUsed); | ||
|
|
||
| verifyGasUsed += frameGasUsed; | ||
|
|
||
| if (substate.ShouldRevert || substate.IsError) | ||
| { | ||
| // A frame capped to the remaining budget that then failed exhausted that cap, so the | ||
| // prefix's validation work exceeds MAX_VERIFY_GAS: reject it as over-budget, distinct | ||
| // from a genuine revert of a within-budget frame (spec §Structural Rules 6, L812). | ||
| return capped | ||
| ? TransactionResult.ErrorType.MalformedTransaction.WithDetail("frame transaction validation prefix exceeds MAX_VERIFY_GAS") | ||
| : TransactionResult.ErrorType.MalformedTransaction.WithDetail("validation prefix frame reverted"); | ||
| } |
There was a problem hiding this comment.
Medium — the new capped discriminator inverts the previous misreport: a within-budget revert is now reported as MAX_VERIFY_GAS exceeded. The underlying issue is that this loop bounds actual gas while the spec bound is on declared gas_limits.
capped is computed from the frame's declared limit (frameGasLimit < frame.GasLimit), but the reason it selects is about consumption. The comment asserts an invariant that does not hold:
A frame capped to the remaining budget that then failed exhausted that cap
A capped frame can fail without going anywhere near the cap. Concretely, with MaxVerifyGas = 100_000:
- Prefix is a single unrecognized-shape
verifyframe declaringgas_limit = 5_000_000(generous limits are the norm for paymasters).capped = truefrom the first iteration. - The verifier
REVERTs at ~3k gas because the signature does not match — the ordinary rejection path. substate.ShouldRevertis true → reported as"frame transaction validation prefix exceeds MAX_VERIFY_GAS".
So the operator-visible reason is wrong for the most likely rejection cause, which is the same class of problem as before, just mirrored. At minimum, an explicit REVERT should never be attributed to the budget (substate.ShouldRevert ⇒ not over-budget); even substate.IsError is not a reliable signal, since any exceptional halt (BadInstruction, stack underflow) consumes all remaining gas and would look identical.
The cleaner fix is to stop capping and bound the declared limits instead, which also removes a conformance divergence. MEMPOOL-RULES-DESIGN.md:46 records the rule as "Sum of validation-prefix gas_limit plus signature-verification intrinsic cost ≤ MAX_VERIFY_GAS" — declared, not consumed — and that is exactly what FrameTxPayerResolver.TryGetValidationPrefixVerifyGas / FrameTxVerifyGasFilter implement for the recognized shapes. This loop instead accumulates actual frameGasUsed, so for the unrecognized shapes that actually reach the simulator (where TryGetValidationPrefixVerifyGas returns false and the filter passes the tx through) a prefix declaring 5M of prefix gas but consuming 40k is admitted here, while Direct Evaluation would reject it — the opposite of "Direct evaluation MUST apply the same limits as simulation".
Accumulating the declared limits makes the check exact, reachable, and consistent with #12620:
// Spec §Structural Rules 6 (L812): the sum of the validation-prefix frames' declared gas_limit plus
// signature-verification cost is bounded by MAX_VERIFY_GAS. Same bound as the Direct Evaluation form
// in FrameTxPayerResolver.TryGetValidationPrefixVerifyGas, which cannot analyze unrecognized shapes.
verifyGasDeclared += frame.GasLimit;
if (verifyGasDeclared < frame.GasLimit || verifyGasDeclared > Eip8141Constants.MaxVerifyGas)
{
return TransactionResult.ErrorType.MalformedTransaction.WithDetail("frame transaction validation prefix exceeds MAX_VERIFY_GAS");
}with frame then executed at its declared limit and the revert branch restored to the single "validation prefix frame reverted" reason. That check is genuinely reachable (unrecognized shapes are not pre-checked), fires before burning EVM work, and needs no capped flag.
Note also that Simulate_PrefixExceedsMaxVerifyGas_RejectedAsOverBudget uses SelfVerifyFrame, a recognized shape — in production FrameTxVerifyGasFilter rejects that with AcceptTxResult.VerifyGasExceeded before the simulator is consulted. A test over an unrecognized shape would exercise the path this code actually serves.
There was a problem hiding this comment.
Fixed the misreport: an explicit within-budget REVERT (e.g. signature mismatch) is no longer attributed to MAX_VERIFY_GAS — only a capped frame that runs out of gas is reported as over-budget (capped && !substate.ShouldRevert). I kept the cap rather than switching to a declared-gas-limit bound: for recognized shapes the declared bound is already enforced upstream by FrameTxVerifyGasFilter (the Direct Evaluation form), and the simulator is reached for opaque/unrecognized shapes where capping runs bounded work; the declared-bound rewrite would also change admission semantics and the 200k-gas Evm fixtures. Left the declared-vs-consumed alignment as a noted follow-up.
| /// <param name="token"> | ||
| /// Cancels the (up to <c>MAX_VERIFY_GAS</c>) simulation, which may also block behind other peers' | ||
| /// serialized simulations. Honored at entry; per-frame cooperative cancellation is a deferred | ||
| /// follow-up (design note §4). An <see cref="System.OperationCanceledException"/> propagates. | ||
| /// </param> | ||
| FrameTxSimulationResult Simulate(Transaction tx, CancellationToken token = default); |
There was a problem hiding this comment.
Low — the deferred "per-frame cooperative cancellation" is already available for ~4 lines via ITxTracer.IsCancelable, and it would also stop burning budget after the first violation.
The token being honored only at entry is the weak part of this: entry is exactly where the simulation costs nothing. The interpreter already polls the tracer for cancellation every 1024 opcodes — VirtualMachine.DispatchSpecialized.cs:62 checks _txTracer.IsCancelled under the TCancelable flag, which VirtualMachine.cs:1256 selects from _txTracer.IsCancelable (both are default-false members on ITxTracer). So FrameTxValidationTracer can opt in directly:
public sealed class FrameTxValidationTracer(Address sender, Address expiryVerifier, IReadOnlyStateProvider state, IReleaseSpec spec, CancellationToken token = default)
: TxTracer, IFrameTxReceiptTracer
{
// The interpreter polls this every 1024 opcodes, giving the simulation cooperative cancellation
// and letting it abort as soon as a trace rule is violated instead of burning the rest of MAX_VERIFY_GAS.
public override bool IsCancelable => true;
public override bool IsCancelled => Violated || token.IsCancellationRequested;That closes the gap the <param> documents as deferred, and the resulting OperationCanceledException already has a handler in FrameTxPrefixSimulator (line 90) plus WorldState.Restore in the processor's finally, so nothing else changes. The abort-on-violation half matters independently: today a prefix that violates a rule on its first opcode still runs to completion, so a spammer gets the full MAX_VERIFY_GAS of work per rejected tx rather than ~0.
The remaining, larger piece — threading the BackgroundTaskScheduler token from Eth62ProtocolHandler.HandleSlow through IIncomingTxFilter — is fairly noted as a follow-up; the queueing behind FrameTxPrefixSimulator._lock is not interruptible until then. Worth saying so explicitly in the <param> text, which currently reads as if the token covers that wait.
There was a problem hiding this comment.
Deferred. Wiring IsCancelled => Violated would surface a rule violation as OperationCanceledException, which FrameTxPrefixSimulator re-throws (for shutdown) rather than converting to a Reject carrying the violation reason — so abort-on-violation needs an accompanying exception-flow change. Kept as the follow-up the interface doc already notes (design note section 4); the interpreter still bounds the work by MAX_VERIFY_GAS.
ff8d854 to
6dff6ab
Compare
…actions Admit the opaque frame-tx prefixes the native resolver defers (RequiresSimulation: deployed-code sender, code-carrying paymaster, unrecognized shape) by simulating their validation prefix in a bounded, read-only EVM, resolving the payer and enforcing the trace/opcode rules. The standard, natively-resolvable prefixes keep the EVM-free fast path: FrameTxSimulationFilter returns immediately for a payer already resolved by FrameTxPayerFilter, so the simulator is never consulted for them. - IFrameTxPrefixSimulator abstraction in Nethermind.TxPool (no reference to Consensus's processing env, which would cycle); implemented by FrameTxPrefixSimulator at the composition root wrapping IReadOnlyTxProcessingEnvFactory, injected optionally into the pool. - ExecutionOptions.FrameValidationPrefixOnly + a validation-prefix simulation path in the frame processor: halts once the payer is set, bounds cumulative work by MAX_VERIFY_GAS, always restores state. - FrameTxValidationTracer enforces the banned-opcode list, the GAS-before-call and TIMESTAMP-in-expiry-verifier caveats, and SLOAD-restricted-to-sender; violations reject the transaction. When no simulator is wired, RequiresSimulation stays deferred as in Phase 1. Deferred (design note §4): dependency-set-keyed caching, head-change re-simulation indexing, wall-clock cancellation, and a multi-simulation admission budget.
…acer Address review findings on the Phase 2 in-pool simulation: - High: FrameTxValidationTracer now rejects CALL*/EXTCODE* whose target is neither an existing contract nor a precompile, or that uses an EIP-7702 delegation (spec §Validation Trace Rules, L816/L853), exempting tx.sender. Previously these prefixes were silently admitted despite the doc claim. - TIMESTAMP exemption now checks the EXPIRY_VERIFIER code hash as well as the address (L788), via a new Eip8141Constants.ExpiryVerifierCodeHash. - GAS-before-call is validated by peeking the next opcode instead of a cross-frame mutable flag, so a trailing GAS is no longer missed or mis-attributed across frame boundaries. - The post-frame MAX_VERIFY_GAS bound is now reachable: a prefix frame capped to the remaining budget that then exhausts it is rejected as over-budget, distinct from a within-budget revert. - IFrameTxPrefixSimulator.Simulate takes a CancellationToken (honored at entry; OperationCanceledException propagates). Adds tests for the CALL* codeless-target rejection, the existing-contract allow case, and the over-budget rejection reason.
…tion The payer resolver now takes the sender account and defers all only_verify|pay prefixes to simulation, so the simulation filter passes the sender account when re-resolving, and its tests drive the resolver through the same account state. Also stop attributing an explicit within-budget REVERT to the MAX_VERIFY_GAS cap: only a capped frame that ran out of gas is reported as over-budget.
Relocated from the mempool-rules PR, where native admission no longer resolves a third-party payer. Here the opaque only_verify|pay sponsor prefix is resolved by validation-prefix simulation, so the exposure gate bounds the sponsor's summed pending cost to its balance end-to-end and releases the reservation on removal.
3811261 to
146f435
Compare
Summary
Phase 2 of the EIP-8141 mempool stack: admit the opaque frame-tx validation prefixes the native resolver defers (
RequiresSimulation— a deployed-code / EIP-7702 sender, a code-carrying paymaster, or an unrecognized shape) by simulating their validation prefix in a bounded, read-only EVM, resolving the payer and enforcing the trace/opcode rules at admission.Design basis:
frames-handoff/IN-POOL-SIMULATION-DESIGN.md(Alternative C). Spec: ethereum/EIPs#12007 (EIP-8141), "Validation Trace Rules" / "Acceptance Algorithm".Stacks on #12620 / #12617 / #12610 (base branch
eip8141-mempool-validation). Those have in-flight review fixes on their own branches; this PR will need a rebase once they land — expected, not pulled here.The standard pattern keeps the EVM-free fast path
The common, natively-resolvable prefixes (default-code
self_verify/only_verify → payverified by a standard ECDSA signature) are not executed in the EVM.FrameTxSimulationFilterruns afterFrameTxPayerFilterand returns immediately when the payer was already resolved natively (non-nullPayerAddress), so the simulator is never consulted for them. Only a frame tx whose payer is still unresolved is re-classified, and only aRequiresSimulationoutcome reaches the simulator. A unit test asserts the simulator is not called on the fast path (NSubstitute spy).What's implemented
IFrameTxPrefixSimulator— narrow abstraction inNethermind.TxPool(no reference to Consensus's processing env, which would cycle: Consensus already references TxPool). Injected optionally intoTxPool, mirroring the existing optionalincomingTxFilter.FrameTxPrefixSimulator(composition root,Nethermind.Consensus) — wrapsIReadOnlyTxProcessingEnvFactory; registered inBlockProcessingModuleand resolved optionally inInitializeBlockchain.CreateTxPool. Reuses the read-only processing env rather than a pool-specific EVM.ExecutionOptions.FrameValidationPrefixOnly+ a validation-prefix simulation path in the frame processor (TransactionProcessorBase.FrameTx.cs) that reuses the existingExecuteFrame/ApplyApprovalmachinery: halts once the payer is set (rule 7), bounds cumulative validation work byMAX_VERIFY_GAS(the execution bound, complementing EIP-8141: validation-prefix simulation admission for frame transactions (MAX_VERIFY_GAS bound) #12620's arithmetic pre-check), and always restores state.FrameTxValidationTracer(Nethermind.Evm) — enforces the banned-opcode list, theGAS-before-call andTIMESTAMP-in-expiry-verifier caveats, andSLOADrestricted totx.sender; a violation rejects the tx.PayerAddresspath; a failing / over-budget / no-payer one rejects (AcceptTxResult.FrameSimulationFailed).Additive: Phase-1 behavior preserved when unwired
When no simulator is registered,
RequiresSimulationframe txs stay deferred exactly as in Phase 1 (the filter is a no-op). A unit test covers this.Deferred (documented
EIP8141:follow-ups, per design note §4)MAX_VERIFY_GASgas bound is the DoS bound implemented now).deploy-frame carve-outs (CREATE/CREATE2/SETDELEGATE/SSTORE-to-sender) andCALL*/EXTCODE*target-existence + EIP-7702 checks: such prefixes are conservatively rejected for now (declining is always spec-compliant, EIP L684), rather than admitted incorrectly.Concurrency note: simulations are serialized (they share one resettable world state), which also bounds concurrent admission work until the budget lands.
Tests
FrameTxSimulationFilterTests(TxPool): fast path does not invoke the simulator; opaque path simulates → payer recorded → admitted; failed simulation → rejected; no simulator wired → deferred.FrameTxValidationPrefixSimulationTests(Evm): deployed-code sender / sponsor resolves payer; over-MAX_VERIFY_GASrejected; banned opcode records a violation; revert / never-sets-payer rejected; simulation does not mutate canonical state.Full
Nethermind.TxPool.Testsuite green (688); frame-tx Evm suites green; full Runner build 0 errors.Types of changes