Revalidate the txpool across fork boundaries

[flcl42]

Changes

• Revalidate pending transactions when the active release specification changes, including intrinsic-gas checks.
• Stream persistent light-blob revalidation and retain only failed hashes/results, avoiding whole-pool blob-payload retention.
• Keep production selection conservative until that revalidation succeeds, then restore the regular fast path.
• Make revalidation completion retry-safe across failures and rapid fork/reorg transitions, so the pool reports a target state safe only after the current generation completes.
• Preserve the existing public head-validator compatibility wiring while requiring the narrow spec-change validator at every TxPool construction path.

Related to #12599.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

• Focused txpool fork-boundary, retry, persistent-startup, and queued-reorg tests (5 passed).
• Transaction-source selection tests (68 passed).
• Release solution and Network benchmark builds with warnings treated as errors.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @flcl42's task in 9m 3s —— View job

---

Review: Revalidate the txpool across fork boundaries

• Gather context (read diff, trace call paths)
• Review TxPool.RevalidateTransactionsAtSpecChange
• Review TxPoolTxSource fork-boundary filtering
• Review test coverage
• Post inline findings + summary

---

The gap being closed is real and worth fixing: nothing today re-checks spec-dependent tx validity when the head crosses a fork, so txs that were fine under fork N can linger and be selected under fork N+1. The two-sided approach (producer-side check for the boundary block, pool-side purge for everything after) is the right shape, and the regression tests target exactly the right behaviours — in particular GetTransactions_should_not_let_an_invalid_blob_displace_a_valid_blob covers a subtle failure where an invalid blob consumes a block slot.

The concerns below are all about how much work the new paths do at the boundary, plus two pool-hygiene issues.

Findings

#	Severity	Where	Issue
1	High	TxPool.cs:840-842	Revalidate uses the full _validator, whose blob chain ends in MempoolBlobTxValidator → full KZG proof re-verification. Combined with the blob loop it forces a DB read + deserialization of every persisted blob tx (default PersistentBlobStorageSize = 16384, up to 6×128 KB each), thrashes the 256-entry _blobTxCache that engine_getBlobsV* relies on, and does all of it under the _newHeadLock write lock in ProcessNewHeadLoop. It's also redundant — proof validity is spec-independent, and the proof version is already checked on the light tx by HeadTxValidator on every head change (TxPool.cs:749). Use a narrow spec-dependent validator (IntrinsicGasTxValidator.Instance, matching the producer side).
2	Medium	TxPool.cs:846-851	RemoveTransaction drops a single tx and leaves higher-nonce txs from the same sender stranded. The existing eviction path deliberately avoids this (MarkForEviction: evictNextTxs |= tx.SupportsBlobs; — "evict all following txs to prevent nonce gaps between blob tx").
3	Medium	TxPool.cs:849	DeleteFromLongTerm is unconditional. In MarkForEviction it's gated on allowLaterPoolReentrance, set only for InvalidProofVersion — the one recoverable failure. A tx failing the new intrinsic-gas floor can never become valid again, so dropping the memo invites endless re-announce → re-download (up to 1 MiB for blob txs) → re-reject.
4	Medium	TxPoolTxSource.cs:112-120	IsValidBlobForNextBlock resolves the full blob tx from RocksDB before the cheap proof-version check. At a BlobProofVersion-changing fork every blob tx fails that check, and since a false filter result doesn't advance countOfRemainingBlobs, SelectBlobTransactions never hits its early break — so the producer walks the whole blob pool doing full-blob DB reads, on the deadline-bound block-production path. LightTransaction already carries ProofVersion (LightTransaction.cs:31,66-68), so this is free to fix.
5	Medium	TxPoolTxSource.cs:48-59	Gating on isForkBoundary races with the pool purge: OnHeadChange only enqueues to _headBlocksChannel, and UpdateBuckets() runs later on ProcessNewHeadLoop. If payload building for block N+2 starts before that queue drains, the check is off and the pool isn't cleaned yet. The check is nearly free to run unconditionally — CalculateIntrinsicGas memoizes per (tx, spec) (EthereumGasPolicy.cs:566-579) — which would also drop the parentSpec lookup and the fragile ReferenceEquals spec-identity assumption.
6	Low	TxPoolTxSource.cs:145-149	Pipeline now runs twice per blob tx on ordinary blocks (light tx during ordering, full tx in ResolveBlob), three times at a boundary.
7	Low	TxPool.cs:816-840	Revalidate missing private; no log line or metric for what can be a mass eviction on fork day.
8	Low	tests	The 10-slot access-list setup is copy-pasted 4× across 3 files; the 42_400 magic number carries the whole point of the tests and is uncommented.
9	Low	TxPoolTests.Blobs.cs:219	Unrelated whitespace-only change.

Smaller notes (not filed inline)

• The constructor call at TxPool.cs:153 sits inside if (_blobTransactions.Count > 0), so when the persistent blob pool is empty at startup _lastRevalidatedSpec stays null and the first head change triggers a full "spec change" pass even though no fork occurred. Harmless (pool is empty then), but the coupling of the guard to the blob count is easy to misread — moving the call outside the if would make the intent clearer.
• If _blobTransactions.TryGetValue fails for a light tx (blob data missing from storage after a crash), the light entry is silently left in the pool forever. Worth a trace log at least.
• I verified blockGasLimit: 0 is safe here — the parameter is unused inside EthereumGasPolicy.Calculate, and it matches what TxValidator.IsWellFormed(tx, spec) already does for mempool validation. No memo poisoning.
• ReferenceEquals on specs holds for SpecProviderBase and ForkScheduleSpecProvider (both return cached per-fork instances), so the identity comparison is sound today — it's just an implicit contract on spec providers that isn't documented anywhere.

Not verified

I could not build or run the test suite in this environment (build commands weren't permitted), so findings are from static analysis of the call paths. The author reports TxPoolSourceTests passing (74 tests).

Recommendation: address #1 (the KZG/DB storm is the one with production impact at a mainnet fork) and #2–#5, or reply with rationale where you disagree.
· branch fix/tx-pool-reevaluation

[claude]

Claude finished @flcl42's task in 9m 26s —— View job

---

Review: Revalidate the txpool across fork boundaries

• Gather context (diff since previous review)
• Re-check the 9 findings from the previous pass
• Review the newly-added code (Guard empty transaction pool revalidation)
• Post inline findings + summary

---

Good progress since the last round — five of the previous findings are genuinely fixed, and the fixes are the right ones:

Previous	Status
#1 full _validator → KZG storm	✅ narrow IntrinsicGasTxValidator injected via a keyed registration
#2 nonce gaps	✅ evictFollowingTransactions + a test that asserts the successor is dropped
#3 unconditional DeleteFromLongTerm	✅ gated on InvalidProofVersion (but see M1 below)
#4 blob resolved before the cheap proof-version check	✅ GetProofVersion() checked first
#5 isForkBoundary gating raced the pool purge	✅ check is now unconditional
#7 no observability	✅ Info log + Metrics.PendingTransactionsEvicted
#9 stray whitespace	✅ reverted
#8 duplicated test setup	➖ partly — BuildUnderGassedAccessList extracted, still one copy per assembly

The new findings below are all in the code that was added to fix those, and two of them are more serious than anything in the first round.

Findings

#	Severity	Where	Issue
C1	Critical	TxPool.cs:871-886	LoadBlobTransactionsForRevalidation retains the full blob payload of the entire blob pool in a Dictionary at once — default PersistentBlobStorageSize is 16384 and its own config comment says "theoretical max - 13GB". And because _lastRevalidatedSpec starts null, this fires on the first head change after every restart, not just at forks: 16k RocksDB reads, 16k decodes, 16k _blobTxCache.Set into a 256-entry LRU, all holding _newHeadLock. Fix is two lines of restructuring: seed _lastRevalidatedSpec in the ctor, and stream the pass keeping only failing hashes.
H1	High	TxPoolTxSource.cs:58-59	tx.IntrinsicGasMemo is null || skips the new check for any memo-less tx — which is every full blob tx decoded from RocksDB (LightTransaction doesn't carry the memo; the 256-entry cache is empty after a restart). So the blob half of the producer-side fix doesn't engage in production. The two new tests only pass because they call IntrinsicGasTxValidator.Instance.IsWellFormed(tx, Amsterdam.Instance) first, purely to populate the memo — worth deleting those lines to see the tests go red.
H2	High	TxPoolTxSource.cs:111-122	IsValidBlobForNextBlock is now the ordering filter, so every candidate OrderCore pops is fully resolved from RocksDB — including the ones SelectBlobTransactions rejects a moment later on the cheap light-tx checks (txBlobCount > maxBlobs, feePerBlobGas > MaxFeePerBlobGas), and a filter rejection doesn't advance countOfRemainingBlobs so the maxBlobsToConsider break doesn't bound the walk. On a blob-fee spike that's hundreds-to-thousands of 128 KB-per-blob reads per engine_getPayload, where master did ≤ 9.
M1	Medium	TxPool.cs:956-975	The shouldBeDumped branch never runs _headTxValidator, so a proof-version mismatch surfaces as InvalidTransactionForm (excluded from the dictionary at line 877) → DeleteFromLongTerm skipped → hash blacklisted forever. A blob tx's hash doesn't cover the wrapper, so re-broadcasting with regenerated proofs is rejected as already-known — permanently. This is exactly what the existing InvalidProofVersion carve-out prevents on the other path.
L1	Low	TxPool.cs:829-862	Early return leaves _lastRevalidatedSpec un-advanced (so a later non-fork head change does a full pass); the "after the protocol change" log also fires on the first post-restart pass; _blobTransactionsToRevalidate / _revalidatedTransactions are implicit parameters whose single-threadedness isn't documented.
L2	Low	TxPool.cs:117 + 4 wiring sites	The validator is wired twice (explicit ctor arg and keyed DI registration), and ?? validator falls back to the full KZG-verifying TxValidator — degrading silently into the very cost the parameter exists to avoid. Prefer the keyed registration alone plus ArgumentNullException.ThrowIfNull.
L3	Low	tests	No test asserts revalidation doesn't run on an ordinary head change, which is the guard for C1/H2's cost. Given how much work a spec-change pass now does, that's the one worth pinning.

Verified while reviewing (not findings)

• ReferenceEquals(_lastRevalidatedSpec, headSpec) is sound: ChainHeadSpecProvider.GetCurrentHeadSpec caches per head number and delegates to reference-stable GetSpec implementations (SpecProviderBase, ForkScheduleSpecProvider, SingleReleaseSpecProvider). Worth knowing that a spec provider returning a fresh instance per call would now trigger a full blob-pool load per block, so C1's cost makes this implicit contract load-bearing.
• No double-counting on Metrics.PendingTransactionsEvicted — line 683 is the capacity-eviction path only.
• _revalidatedTransactions++ is race-free: UpdateBuckets has a single caller (ProcessNewHeadLoop, line 319) and SortedPool.UpdatePool iterates buckets serially under McsLock.
• The pre-materialised dictionary is necessary to avoid re-entering the pool lock from inside UpdatePool — the fix in C1 keeps that property, it just stops retaining the payloads.
• should_evict_transactions_that_become_under_gassed_after_fork correctly covers the nonce-gap case from the previous round (both txs go).

Not verified

Build and test commands were not permitted in this environment, so all of the above is static analysis. The one claim I'd most like executed is H1: delete the two IntrinsicGasTxValidator.Instance.IsWellFormed(…) pre-warm calls from TxPoolSourceTests and confirm GetTransactions_should_not_let_an_invalid_blob_displace_a_valid_blob fails. If it still passes, my reading of the memo lifetime is wrong and H1 collapses.

Recommendation: C1 and H1 should land before merge — C1 is an OOM/latency regression that triggers on every restart rather than only at forks, and H1 means the blob path of the fix is currently inert. H2 and M1 are contained but real.
· branch fix/tx-pool-reevaluation

[claude]

Claude finished @flcl42's task in 8m 53s —— View job

---

Review: Revalidate the txpool across fork boundaries

• Gather context (diff since last review round)
• Re-check prior findings (C1, H1, H2, M1, L1–L3)
• Review the new design in the latest 5 commits
• Post inline findings + summary

---

The design has changed substantially since the last round and mostly for the better. The producer no longer guesses at the next block's spec — it receives the actual targetBlock header — and the pool now publishes whether it has finished revalidating for that spec, so the producer's expensive path engages exactly when the pool can't vouch for itself. That's a cleaner contract than the previous isForkBoundary/IntrinsicGasMemo heuristics, and it closes both H1 and H2.

Previous round	Status
C1 whole blob pool materialised	❌ still present, and now worse — the proof-version pre-filter is gone and it also runs from the constructor
H1 IntrinsicGasMemo is null short-circuit	✅ gone — the check is unconditional under validateForkSensitiveState
H2 every ordered blob candidate resolved from RocksDB	✅ fixed — resolution only happens on the fork path, and consideredBlobCount now bounds that walk at 5 * maxBlobs
M1 recoverable failure permanently blacklisted	✅ fixed — allowLaterPoolReentrance covers InvalidProofVersion and InvalidTransactionForm
L1 orchestration / seeding	⚠️ partly — seeded in the ctor and via AddCore, but see M1 below on the failure path
L2 duplicate validator wiring	⚠️ partly — the silent ?? validator fallback is now a hard ArgumentNullException, but the dual wiring remains
L3 no test that revalidation is skipped on ordinary head changes	✅ should_run_spec_change_validation_only_at_fork_boundary

Findings

#	Severity	Where	Issue
C1	Critical	TxPool.cs:880-896	LoadBlobTransactionsForRevalidation resolves and retains the full blob payload of every tx in the persistent pool (defaults: StorageWithReorgs, PersistentBlobStorageSize = 16384, config comment "theoretical max - 13GB"). Unlike the previous round it no longer pre-filters on proof version, and it now also runs from the constructor on every restart with a non-empty blob DB — so this is no longer a fork-only cost. GB-scale peak RSS held across both UpdatePool calls under _newHeadLock, plus 16k RocksDB reads and a full flush of the 256-entry _blobTxCache that engine_getBlobsV* depends on. A streaming pass retaining only failing hashes keeps the lock-reentrancy property at one-blob-tx peak memory.
M1	Medium	TxPool.cs:842-861	_lastRevalidatedSpec advances before the pass, _completedRevalidatedSpec only after. ProcessNewHeadLoop swallows exceptions, so one throwing pass permanently means (a) the pool is never re-purged for this fork and (b) EnsureSafeForkState returns false for every payload build until the next fork — the producer pays full blob resolution on every engine_getPayload forever. Not a consensus issue (the producer filter still catches the txs), but silent and unbounded.
M2	Medium	ITxPool.cs:57, ITxSource.cs:12	ITxPool gains a required member, ITxSource.GetTransactions changes signature, and FilteredTxSource<T>'s public ctor loses a parameter — all breaking for out-of-tree implementers, but Breaking change is unticked in the description. Those checkboxes drive the auto-labelling described in AGENTS.md. Also EnsureSafeForkState is a predicate named like a command; the caller has to write !…EnsureSafeForkState(…).
L1	Low	MergePlugin.cs:169, TestRpcBlockchain.cs:184, IApiWithBlockchain.cs:35, NethermindApi.cs:84, NextBlockSpecHelper.cs	Dropping the headTxValidator ctor parameter and NextBlockSpecHelper leaves five unreferenced wiring sites. The MergePlugin one actively misleads — it reads as if the merge pool still head-validates every block.
L2	Low	BlockProcessingModule.cs:45	Validator wired twice (keyed registration and four explicit ctor args). The keyed registration alone is what AGENTS.md prefers.
L3	Low	TxPool.cs:972-987	_revalidatedTransactions++ in the shouldBeDumped branch counts txs dropped for insufficient balance/nonce, inflating Metrics.PendingTransactionsEvicted and the fork-day log line that someone will read to judge blast radius. A light blob tx whose stored payload has vanished is also removed with only a Trace.
L4	Low	TxPoolTxSource.cs:181-186	The break condition moved from countOfRemainingBlobs > maxBlobsToConsider to total consideredBlobCount, which changes the non-fork walk too (single-blob txs taken via the fast path now count toward the budget). Bounded — the difference is at most maxBlobs - 1 against a 5 * maxBlobs budget — but it's an unflagged behaviour change on a path this PR isn't otherwise about.
L5	Low	TxValidator.cs:139	IntrinsicGasTxValidator now returns InvalidTransactionForm for LightTransaction. Defensible as a guard, but it puts pool-specific knowledge into a general consensus validator, and the error code doubles as the "allow re-entrance" signal at TxPool.cs:983. A short <remarks> on why a light tx can never be intrinsically validated would help.

Verified while reviewing (not findings)

• No regression from dropping _headTxValidator from the pool. All three HeadTxValidator members (MaxBlobCountBlobTxValidator, GasLimitCapTxValidator, MempoolBlobTxProofVersionValidator) depend only on IReleaseSpec, so evaluating them at spec change rather than every head change is equivalent, and SpecChangeTxValidator is a strict superset. HeadTxValidator also remains in the production filter pipeline via TxFilterPipelineBuilder.WithHeadTxFilter, so the fork-boundary block is still covered for gas-limit cap and proof version even when the pool pass hasn't run.
• GetSpec(targetBlock) vs the old NextBlockSpecHelper: identical whenever payloadAttributes supplies the timestamp (all PoS paths). For Clique/AuRa the timestamp source changes from parent.Timestamp + SecondsPerSlot to the header's own Math.Max(parent.Timestamp + 1, now) — which is the more correct value and matches what PrepareBlockHeader already used for BaseFeePerGas.
• ChainHeadSpecProvider cache key widening from Number to ForkActivation is required by this PR (a timestamp fork at the same block number would otherwise return a stale spec) and preserves the reference-type atomic-publication invariant the existing comment calls out.
• Field-as-implicit-parameter safety: _blobTransactionsToRevalidate / _revalidatedTransactions are only touched under the _newHeadLock write lock (ProcessNewHeadLoop) or in the ctor before HeadChanged is subscribed; SortedPool.UpdatePool iterates buckets serially. The comment at :850 records this.
• AddCore's Interlocked.CompareExchange(…, null) seeding is race-free against UpdateBuckets (read lock vs. write lock) and its claim is sound — when both fields are null the pool is empty, so "revalidated for this spec" is vacuously true.
• Empty-pool-crosses-fork leaves EnsureSafeForkState false until the next head change with a non-empty pool; conservative and self-healing, and pinned by should_not_report_fork_state_safe_after_empty_pool_crosses_fork.
• The three new TxPoolSourceTests are the right shape — in particular should_skip_full_fork_validation_when_pool_is_safe_for_target_block asserts the pipeline is not re-run on the resolved blob, which is what pins H2 shut.

Not verified

Build and dotnet test were not permitted in this environment, so everything above is static analysis. The author reports TxPoolSourceTests green (74 tests).

Recommendation: C1 is the one that should block — it is an OOM/latency regression under default config that now fires on every restart, and it has survived two review rounds while growing in scope. M1 and M2 are cheap to address. Everything else is polish.
· branch fix/tx-pool-reevaluation

[LukaszRozmej]

Review of the txpool fork-boundary revalidation.

The core _validatedSpec design holds up: I traced SubmitTx/AddCore (read lock) against UpdateBuckets (write lock), the snapshot-then-flag ordering in GetPendingForProduction, the reorg re-add path and the throw-mid-walk retry path. The mark is only ever cleared outside the write lock, UpdatePool visits every bucket, buckets dumped early are fully removed, and TxValidator's per-type composites are a superset of SpecChangeTxValidator on Ethereum, so a tx admitted under spec S really is S-validated. The ChainHeadSpecProvider switch to ForkActivation equality is behaviour-preserving (ISpecProvider.GetSpec(header) is exactly GetSpec(new ForkActivation(Number, Timestamp))), every spec provider in the repo returns cached instances so ReferenceEquals is sound, and the new IntrinsicGasTxValidator LightTransaction guard doesn't break any existing IsWellFormed caller.

Seven comments inline: one significant startup/fork-boundary cost, one blob-selection behaviour change that can cost the fork block its blobs, and five smaller correctness/consistency items.