EIP-8141: revalidate dependency-affected frame transactions on a new head - #12778
EIP-8141: revalidate dependency-affected frame transactions on a new head#12778Marchhill wants to merge 51 commits into
Conversation
|
Claude finished @Marchhill's task in 5m 29s —— View job Review: EIP-8141 dependency revalidation
The design is right and the layering is clean — That said, I found one issue I'd call blocking and several that should be resolved before merge. The headline one is that the new eviction path leaks payer reservations, and its effects compound with two of the others. Findings
Critical: the reservation leak (finding 1)Worth expanding here because it's the merge-blocker.
The fix is small — move the TestingThe three new tests are well-chosen for the paths they cover (definite rejection evicts, indeterminate retains, unrelated change list doesn't consult the simulator — the
Smaller notes
VerdictNot mergeable as-is: finding 1 is a remotely-triggerable permanent DoS on any paymaster, and findings 2/3 mean the feature both misses the reorg case it claims to cover and reintroduces the head-processing stall it sets out to avoid. Findings 1, 2 and 5 are each a few lines. Findings 3, 4, 6 and 7 are design calls that need either a fix or an explicit rationale in the code. I have not built or run the test suite here (review only, no changes pushed). |
6152ef9 to
6214489
Compare
…lidation # Conflicts: # src/Nethermind/Nethermind.TxPool/IFrameTxPrefixSimulator.cs
…lidation # Conflicts: # src/Nethermind/Nethermind.Consensus/Processing/FrameTxPrefixSimulator.cs
|
The eight table findings all had inline threads and are resolved there. Responding here to the "Smaller notes", which did not:
Separately, a summary-only note from #12624 applies here and is now fixed: an EIP-7702-delegated sender runs the delegate's code, so the delegate is a dependency of the prefix; the sender's own code hash only pins the designation. |
|
@claude review |
|
Claude finished @Marchhill's task in 7m 25s —— View job Review: EIP-8141 dependency revalidation (round 2)
Round-1 findings — all resolved
The "smaller notes" replies are all reasonable, including keeping New findings
A — blob pool. B — the new delegate dependency. This is finding 5's twin, opened up by the head commit. C — hash cache. TestingThe five new tests cover their paths well, and the reservation-lifecycle and reorg cases requested last round are both there. Remaining gaps map onto the findings: no blob-pool case (A), no delegation case at all (B), no "resubmittable after the condition clears" case (C). I could not build or run the suite in this environment ( VerdictMuch improved — the round-1 blockers are genuinely fixed, and #4 and #7 were fixed by removing the risky construct rather than patching it. Nothing here is a merge blocker on its own: A and B are silent coverage holes in the new feature rather than regressions, and C is a behavioural sharp edge inherited from the expiry path. All three are small changes, so I'd resolve them before merge rather than as follow-ups. |
|
@claude review |
|
Claude finished @Marchhill's task in 10m 20s —— View job Review: EIP-8141 dependency revalidation (round 4)
Round-3 findings — all resolved
I also re-walked the round-1/2 fixes against the current text, since the re-index wrapper and the blob-pool fallthrough both sit on the reservation path: all four I checked the load-bearing assumption behind the whole index and it holds: a storage-only write to a dependency account does surface in New findings
1 — the only one I'd resolve before merge. 2 is not a correctness problem, just a mismatch between what the code does (free exactly one slot per pool per head — I also chased two things that turned out not to be findings, recorded so they don't get re-raised: mutating TestingSeven new tests, and the cases requested across rounds 1–3 are all present and well-shaped — reservation lifecycle, resubmittability under the same sender, reorg, the two-step delegation case, and the
VerdictThis is in good shape. Across four rounds the fixes have consistently been the structural kind rather than patches — the lock-free index replaced outright, · branch |
…ut a sort ShedNearlyExpiredFrameTransactions read IsFull before UpdateBuckets dropped what the new head invalidated, so it could evict a live frame transaction for a slot that was about to free itself. One removal clears IsFull, so the pass sheds at most one transaction per pool per head; the sort is replaced by a linear min-scan and the docs now say so. Also names block context as the second axis the account index cannot see.
Keeps this branch's Indeterminate remark: the revalidation pass added here is the production reader whose absence the upstream wording described.
Revert-checked: with the shed call moved back before UpdateBuckets the near-expiry frame transaction is evicted for a slot the stale-nonce drop was about to free, and the test fails.
|
@claude review |
|
Claude finished @Marchhill's task in 6m 54s —— View job Review: EIP-8141 dependency revalidation (round 5)
Round-4 findings — all resolved
I re-walked the earlier rounds' load-bearing fixes against the current text, since the shed move touches the head pipeline they sit in: all four I also re-checked the round-4 dismissal that mutating New findings
1 — the only one I'd resolve before merge, and it is round-2's finding C on the sibling path. The comment says "a deadline only ever gets closer, so unlike a revalidation eviction this cannot reverse" — but the transaction shed here has not expired. The predeploy reverts only once 2 is the pool's no-nonce-gap policy meeting a pass that gets to choose its victim: pick the transaction at the sender's current nonce and the next head's For the record, two things I chased that are not findings: the revalidation sweep passes TestingEleven new tests now, and every case requested in rounds 1–4 is present: reservation lifecycle, resubmittability under the same sender, reorg, the two-step delegation case, the
VerdictThe feature itself reads as correct to me now — five rounds in, the fixes have consistently removed the risky construct rather than patched it (lock-free index → plain |
Capacity pressure, not expiry, decides the shed, so the transaction is still includable and must be able to re-enter once the pressure passes.
Carry the tri-state simulation outcome through revalidation. A node fault now reports Undecided rather than a rejection, and since it is also indeterminate the pass leaves the transaction pending exactly as it already does for an exhausted budget, instead of evicting over the node's own malfunction.
…lidation The only conflict is the remaining-gaps note. This branch's list is the further along one — simulation, dependency-set revalidation and the shared max_cost helper are all closed here — so it stands, with the restored-blob-record gap carried over: LightTxDecoder still does not persist the payer, so a record reloaded from disk holds no exposure reservation.
Changes
Closes the biggest correctness hole in the frame-tx mempool stack: a frame transaction was validated once, at submit, and never rechecked.
FrameTxDependencySetwas constructed by the payer resolver and never consumed — its doc comment promised "a later layer". This is that layer.FrameTxDependencyIndex— maps the chain-head accounts a pending prefix depends on (sender, resolved payer, and the expiry verifier when an expiry frame is present) back to the transaction. Maintained from the pool'sInserted/Removedevents, so it can never outlive pool membership.Head-change revalidation — the new block's changed-account list is intersected with the index before it is disposed, and only that subset is re-resolved after included and expired transactions have left. A head whose change list does not describe everything that moved — a reorg, or a non-sequential block — falls back to every indexed prefix; that is the same completeness test the account cache uses, computed once so the two cannot drift. Revalidating the whole pool per head would be its own denial-of-service vector, which is the point of the index.
Invalid-against-head eviction — a prefix that no longer resolves a payer, or whose payer can no longer cover the pool's exposure to it, is evicted immediately. That is the first tier of the spec's eviction order: such transactions never compete for pool space at all.
Reservation follows the payer — a revalidation that resolves a different payer releases the old reservation and takes a new one against the new payer's balance, evicting if it does not fit.
FrameTxSimulationResult.Indeterminate— a simulation rejected by a resource bound (busy, per-head budget spent, timed out) says nothing about validity. Admission still declines, but revalidation leaves the transaction pending; otherwise an exhausted budget would turn into a mass eviction.Near-expiry shedding — when the pool is full at a head boundary, the pending frame transactions whose deadline is within roughly two slots are shed, nearest deadline first and lowest effective priority fee first among equals. That is the spec's second and third eviction tiers applied where they carry real information: a transaction about to expire is worth little, so it yields its slot rather than displacing a live one through the pool's fee-ordered capacity eviction.
The
EIP8141-GAPlist inNotSupportedTxFilterloses the items now implemented and keeps canonical-paymaster reservation, the failed-APPROVEreplay bound, and a deadline-ordered pool index.Scope
CALL*/EXTCODE*; those are not yet dependencies, so a code change at one does not trigger revalidation. MarkedEIP8141-GAP.IComparer<Transaction>over all transaction types. A frame-only expiry tiebreak that returns 0 for mixed pairs is not transitive and would corrupt the sorted pool; a total order would rank every expiring frame transaction below every other transaction, which is a policy change well beyond the spec. Doing it properly needs a deadline-ordered index insideTxDistinctSortedPooland an eviction-preference hook onSortedPool, so this PR ships the bounded form instead: expired transactions are dropped proactively on each head, and the near-expiry shed pass covers the ordering that matters under pressure. What remains unordered is the deadline ranking among frame transactions with real life left.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
TxPoolTestscovers a sponsored frame transaction evicted once its prefix stops validating against the new head, one retained when the simulation failed on a resource bound, one left untouched when the block's changed accounts miss its dependency set (asserting the simulator is not consulted), a reorg revalidating a transaction its change list never mentions, and a sponsor able to fund again after an eviction released its reservation.Nethermind.TxPool.Test726 passed / 1 skipped;Nethermind.Evm.Testframe suites 147 passed.dotnet format whitespaceclean.Documentation
Requires documentation update
Requires explanation in Release Notes