fix(evm): restore instead of commit for a frame tx CallAndRestore - #12702
fix(evm): restore instead of commit for a frame tx CallAndRestore#12702AnkushinDaniil wants to merge 2 commits into
Conversation
On the frame path Commit ran before Restore, but a commit clears the journals the snapshot indexes into, so the restore reverted nothing. Gas estimation issues repeated CallAndRestore calls against one world state, so the first iteration's committed nonce bump made the next fail the nonce pre-check and eth_estimateGas returned an error instead of an estimate. The frame path holds the whole transaction in the journal until settlement, so the restore alone gives back the entry state; commit now only runs when the caller did not ask for a restore.
|
Claude finished @AnkushinDaniil's task in 9m 1s —— View job Review:
|
| # | Severity | Where | What |
|---|---|---|---|
| 1 | Medium | FrameTxProcessorTests.cs:766 |
eth_estimateGas still returns 0 with no error for a sponsored frame tx. tx.GasLimit is a no-op on the frame path, but CheckFunds/CapByAllowance cap the search by the sender's balance, not the payer's → RightBound = cap = 0, every probe succeeds, BinarySearchEstimate returns Success(0). A regular tx is saved by GasLimitBelowIntrinsicGas; a frame tx has no such guard. Adjacent and pre-existing — fine to acknowledge as out of scope, but the new test (self-paying sender) can't see it. |
| 2 | Low | :772 |
Is.GreaterThan(0ul) also passes if the estimate collapses to the regular-path ~21000 intrinsic bound. Is.GreaterThanOrEqualTo(gasTracer.GasSpent) pins the meaning. |
| 3 | Low | :774 |
Assert.That(balance == 1.Ether, Is.True) reports only Expected: True; use Is.EqualTo(1.Ether) like every other balance assertion in the fixture. |
| 4 | Low | :748-751 |
Member-level doc should be XML doc per coding-style.md; the neighbouring test already uses /// <summary>/<remarks>. |
Nothing to flag on security (RPC-only path, no consensus surface), performance (one branch, cold settlement path), or hot-path allocation. The regression test does drive the real GasEstimator rather than asserting the branch directly, which is the right level.
Not verified
I could not execute dotnet test in this environment (command not permitted), so I did not independently reproduce the stated Nethermind.Evm.Test 5081-passed run or confirm the test fails without the fix. Everything above is from reading the code.
· branch daniil/eip8141-estimate-restore
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| TransactionResult probe = _transactionProcessor.CallAndRestore(tx, gasTracer); | ||
| Assert.That(probe.TransactionExecuted, Is.True, probe.ErrorDescription ?? probe.Error.ToString()); | ||
|
|
||
| GasEstimator estimator = new(_transactionProcessor, _stateProvider, _specProvider, new BlocksConfig()); |
There was a problem hiding this comment.
Medium — the other half of frame-tx estimation is still broken, and this test can't see it (adjacent / pre-existing; fine to acknowledge as out of scope).
A frame tx has no gas_limit field — FrameTxDecoder synthesises tx.GasLimit as the frame-gas sum, and ExecuteFrameTx never reads it (it derives txGasLimit = intrinsicGas + totalFrameGas). So GasEstimator.TryExecute's txClone.GasLimit = gasLimit is a no-op for frame txs: every probe in the binary search succeeds regardless of the limit.
That is harmless while the sender is also the payer, but GasEstimator.CheckFunds / CapByAllowance (and BlockchainBridge.EstimateGas's pre-cap) measure affordability against tx.SenderAddress, whereas a frame tx is funded by the frame-approved payer — the sponsorship case EIP-8141 exists for. With a zero-balance sender:
allowance = available / feeCap = 0→RightBound = cap = 0TryExecute(..., 0)succeeds (limit ignored), so nogas required exceeds allowanceerrorShouldContinueSearchshort-circuits onleftBound + 1 < rightBound, so the loop never runsrightBound == capandTryExecute(0)succeeds →EstimationResult.Success(0)
Net result: eth_estimateGas returns 0 with err == null for a sponsored frame tx. A regular tx is saved here by GasLimitBelowIntrinsicGas → IsGasRelatedFailure; frame txs have no equivalent guard.
This test uses SelfVerifyFrame(), where the sender approves payment for itself, so it stays on the happy path. Either scope the funds/allowance check to the payer for frame txs, or add a TestCase with a sponsor + a zero-balance sender documenting the current behaviour so it isn't mistaken for working.
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(error, Is.Null); | ||
| Assert.That(estimate, Is.GreaterThan(0ul)); |
There was a problem hiding this comment.
Low — Is.GreaterThan(0ul) doesn't pin the estimate's meaning.
Because the gas limit is a no-op on the frame path (see the comment on line 766), the search converges from the optimistic guess down to roughly gasTracer.GasSpent × (1 + errorMargin). > 0 also passes if the estimate silently collapses to the regular-path lower bound (EthereumGasPolicy.CalculateIntrinsicGas returns ~21000 for a frame tx, since tx.Data is empty and the real intrinsic is Eip8141Constants.IntrinsicGasCost + …). Anchoring it to the probe's spent gas makes the assertion say what the estimate is supposed to mean:
| Assert.That(estimate, Is.GreaterThan(0ul)); | |
| Assert.That(estimate, Is.GreaterThanOrEqualTo(gasTracer.GasSpent)); |
There was a problem hiding this comment.
EstimateGasTracer.GasSpent is internal to Nethermind.Blockchain, so this does not compile from Nethermind.Evm.Test. The assertion now anchors on GasCostOf.Transaction, which is above the regular-path lower bound you describe, so the collapse case is excluded without widening the tracer's surface.
| Assert.That(error, Is.Null); | ||
| Assert.That(estimate, Is.GreaterThan(0ul)); | ||
| Assert.That(_stateProvider.GetNonce(Sender), Is.EqualTo(0ul), "the estimation loop committed a nonce bump"); | ||
| Assert.That(_stateProvider.GetBalance(Sender) == 1.Ether, Is.True, "the estimation loop committed a payer charge"); |
There was a problem hiding this comment.
Low — the assertion loses its diagnostic. On failure this reports only Expected: True / But was: False, hiding the actual balance. Every other balance assertion in this fixture (lines 150, 189, 267, 569, …) uses Is.EqualTo:
| Assert.That(_stateProvider.GetBalance(Sender) == 1.Ether, Is.True, "the estimation loop committed a payer charge"); | |
| Assert.That(_stateProvider.GetBalance(Sender), Is.EqualTo(1.Ether), "the estimation loop committed a payer charge"); |
| // eth_estimateGas runs one probe plus a binary search of CallAndRestore calls against a single | ||
| // world state, so a frame transaction's CallAndRestore has to leave nothing behind: a surviving | ||
| // nonce bump makes the next iteration fail the nonce pre-check and the estimate comes back as an | ||
| // error instead of a gas figure. |
There was a problem hiding this comment.
Low (style) — this is member-level documentation, so AGENTS.md / .agents/rules/coding-style.md ask for XML doc rather than a leading // block; Execute_VerifyFrame_IsStatic a few lines above already uses /// <summary> + /// <remarks> in this same fixture.
| // eth_estimateGas runs one probe plus a binary search of CallAndRestore calls against a single | |
| // world state, so a frame transaction's CallAndRestore has to leave nothing behind: a surviving | |
| // nonce bump makes the next iteration fail the nonce pre-check and the estimate comes back as an | |
| // error instead of a gas figure. | |
| /// <summary>A frame transaction's <c>CallAndRestore</c> must leave nothing behind, so a gas-estimation | |
| /// loop can run repeated probes against one world state.</summary> | |
| /// <remarks> | |
| /// <c>eth_estimateGas</c> runs one probe plus a binary search of <c>CallAndRestore</c> calls against a | |
| /// single world state: a surviving nonce bump makes the next iteration fail the nonce pre-check and the | |
| /// estimate comes back as an error instead of a gas figure. | |
| /// </remarks> |
EVM Opcode Benchmark DiffAggregated runs: base=1, pr=1 No significant regressions or improvements detected. |
Use XML doc for the fixture note, compare the balance by value, and pin the estimate above the regular-path lower bound.
3 participants
Raised by @wurdum in review of #12593 and tracked separately as out of scope there.
On the frame path settlement ran
Commitand thenRestore. A commit clears the journals the entry snapshot indexes into, so the restore reverted nothing, which is exactly the combinationCommitAndRestore(eth_call,eth_estimateGas,trace_call) asks for.The visible effect is gas estimation.
GasEstimatorruns a probe plus a binary search ofCallAndRestorecalls against a single world state, so the first iteration's committed nonce bump and payer charge survive into the next one, which then fails the nonce pre-check. Estimation for a frame transaction came back as an error instead of a gas figure. The regular path does not have this because it never commits inside the estimate loop without unwinding afterwards.The frame path keeps the whole transaction in the journal until settlement, so the restore on its own returns the state the caller started with. Commit now runs only when the caller did not ask for a restore.
Regression test:
FrameTxProcessorTests.CallAndRestore_RepeatedForGasEstimation_LeavesNoStateAndEstimatesdrivesGasEstimatorover a frame transaction and asserts a non-zero estimate with no error, plus an unchanged sender nonce and balance afterwards. Without the fix it reports estimate 0, nonce 1 and a charged balance.Tests:
Nethermind.Evm.Test5081 passed / 8 skipped / 0 failed;Nethermind.Blockchain.TestTransactionProcessorTests 116 passed.