[r3.6] execution/stagedsync: skip the fee credit when the recorded set already carries it

[AskAlexSharov]

The apply loop re-credits every tx once per validation round — 4-5 at chaintip, ~30 in catch-up. The credit only moves when an earlier tx's writes moved under it, so most rounds rebuilt an identical write set and merged it back in: a WriteSet, four VersionedWrites, an Account, an O(tx writes) MergeInto and a ReleaseMaps each time.

calcFees now takes the set an earlier fee merge produced for the tx and returns nothing when that set already carries the exact credit — balance value, version and reason, plus the AddressPath account.

Why compare against feeMergeTemp and not TxOut. calcFees reads TxOut as the pre-credit balance, so folding into it re-adds the tip every round. That is what WriteSet.Absorb did on alex/fee_merge_in_place_37: 715 Wrong-trie-root errors on n5 within a minute, unit suites green throughout. feeMergeTemp[txIndex] is a separate container calcFees never reads, and it stops matching the recorded write set the moment a tx re-executes.

Each entry also carries the version it credited, so a credit cannot outlive the incarnation it was computed for — which is the same incarnation whose CollectorWrites the skipped round leaves in place. recordFeeMerge owns the merge (MergeInto + RecordWrites + the temp), so the merge product and the tx's recorded write set cannot drift apart, and both it and recordWorkerWrites reclaim the set they displace.

feeEntry also folds together the coinbase and burnt emission, which was the same code written twice.

A nil *feeEntry is the address a fee adjustment does not touch, so a zero value cannot read as "already recorded". recordedIn compares the AddressPath account by Equals rather than struct equality, which would also fold in Root and PrevIncarnation — fields feeAddressAccount never fills.

The deleted arm keys on version and value alone, so a worker's own SELFDESTRUCT at this version reads as the credit. That is harmless, since the entry writes that identical delete, but the comment previously claimed a version fence that is not there. TestFeeEntryDeletedMatchesAnyWriterAtThisVersion pins what the arm actually accepts.

BenchmarkCalcFees, 200000x n=6:

	ns/op	allocs/op	B/op
first_credit	206	6	544
redundant_recredit	87	2	192

Both arms release the emitted set's maps, as recordFeeMerge does, so the pools are warm. The skipped round returns before the CollectorWrites update and before any VersionedWrite or CollectorWrites entry is built, so an EIP-161 emptied coinbase no longer allocates on it.

Forward port. main has no CollectorWrites and a 4-arg calcFees, so this does not cherry-pick. #23132 is the hand-written twin, carrying the same credited parameter and feeEntry split minus the CollectorWrites block and the skip's ordering ahead of it. Nothing else differs between the two.

Draft until an n5 soak with grep -ci "Wrong trie root". The Absorb attempt above is why green unit suites are not sufficient evidence here.

[AskAlexSharov]

Same change on main: #23132.

[Copilot]

Pull request overview

This PR optimizes Erigon’s parallel execution staged sync fee-crediting by avoiding redundant rebuild/merge of the same fee-credit write set across validation rounds when the already-recorded write set is known to carry the exact credit.

Changes:

• Extend execResult.calcFees to accept an optional “already-credited recorded set” and short-circuit when the required coinbase/burnt credits are already present with the expected version/reason and required AddressPath sibling.
• Refactor fee-credit write emission into a shared feeEntry helper (coinbase + burnt) and adjust feeAddressAccount to return a value type.
• Add targeted unit tests + a benchmark to validate/measure redundant re-credit skipping and creditedWrites behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
execution/stagedsync/exec3_parallel.go	Adds creditedWrites plumbing and short-circuit logic in calcFees, plus refactors fee-credit write construction into feeEntry.
execution/stagedsync/exec3_finalize_test.go	Updates existing finalize tests for the new calcFees(..., credited) signature.
execution/stagedsync/exec3_fee_credit_test.go	Adds new tests/benchmark covering redundant re-credit skipping, re-credit triggers, and creditedWrites pointer-identity gating.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[yperbasis]

Non-blocking follow-ups:

• Make fee-credit provenance explicit on re-execution: clear feeMergeTemp[tx] when RecordWrites installs a new TxOut, or document pointer identity as a VersionedIO contract.
• Put CollectorWrites updates and unused feeEntry/account construction behind the confirmed no-op check where possible; the EIP-161 delete path still allocates on skipped rounds.
• Add coverage through nextResult, including re-execution and EIP-161 deletion, and directly pin recordedIn/writeTo agreement instead of relying only on the hand-mirrored feeCreditRound fold.

[AskAlexSharov]

All three done.

Provenance on re-execution. recordWorkerWrites now wraps the two RecordWrites calls in the result path and clears feeMergeTemp[tx] with them. The skip no longer infers "this was re-executed" from the recorded-set pointer changing underneath it — the credit is dropped where it is lost.

Work behind the no-op check. Both CollectorWrites updates moved back below the check, and each feeEntry is built only when it is emitted. You are right that a skipped round left nothing to do: an unchanged credit means an earlier round already wrote the identical CollectorWrites values, and the one path that rewrites them later (post-finalize) also re-records the write set, which disables the skip anyway. The EIP-161 delete was the concrete cost — SetAccountBalanceOrDelete's emptyRemoval branch allocates a VersionedWrite on every call. That was my over-correction to an earlier review comment; reverted.

Coverage. Two new tests. TestCalcFees_SkipsRedundantReCreditOnEmptyRemoval covers the EIP-161 delete, which had none. TestFeeEntryWriteToIsRecordedIn pins recordedIn against writeTo directly across all three entry shapes, so the pair cannot drift into a permanent skip or a permanent re-credit silently. TestCreditedWritesAfterReExecution drives the invalidation through a real VersionedIO rather than hand-placed map entries.

The full nextResult-level harness I left out — it is apply-loop test infrastructure rather than coverage for this change, and the n5 soak is the real gate for that layer.

BenchmarkCalcFees after the above: redundant round 87ns / 2 allocs / 192B against a 206ns / 6 / 544B first credit.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[yperbasis]

No correctness bug found; the requests below are lifecycle, hardening, and test items, by severity.

Medium

• exec3_parallel.go:2480-2482: recordWorkerWrites displaces the credited merged set from blockIO, then deletes its last owner from feeMergeTemp without queueMapRelease, so each re-execution of an already-credited tx sends the merged set's pooled maps to GC. recordFeeMerge releases in the identical supersede case. Fix: if temp := be.feeMergeTemp[tx]; temp != nil { be.queueMapRelease(temp) } before the delete. This is safe: the temp is never an execResult's TxOut (the merge site is gated on !tipWrites.IsEmpty()), and the caller finishes iterating prevWrites before the call.
• exec3_parallel.go:2035-2037: the skip is sound only because of three unstated guards — the fee merge runs only for Err == nil results, every results[tx] replacement bumps the incarnation (so a stale credit fails recordedIn's Version pin), and CollectorWrites has no consumer for regular txs on this branch. None of the three is asserted or documented; relaxing any one re-opens the silent-divergence class the PR body describes for the Absorb attempt. Consider keeping the credited-set provenance on execResult instead of the block-level feeMergeTemp, so replacing results[tx] invalidates the credit by construction — or at least assert the guards.
• exec3_fee_credit_test.go:251: TestCreditedWritesAfterReExecution simulates re-execution with bare blockIO.RecordWrites, bypassing recordWorkerWrites — removing the delete(be.feeMergeTemp, tx) leaves the package suite green (mutation-tested). Add a white-box test: recordFeeMerge then recordWorkerWrites, then assert feeMergeTemp[tx] is gone and creditedWrites(tx, oldMerged) returns nil. It would also pin the release requested above.

Low

• exec3_parallel.go:2770-2779: the anchor invariant (feeMergeTemp[tx] == blockIO.WriteSet(txIndex) iff the set carries the credit) holds only while RecordWrites + recordFeeMerge stay adjacent and receive the same pointer pair, and creditedWrites re-implements the same identity predicate. recordFeeMerge has one production caller: give it txVersion and let it do the RecordWrites itself, symmetric with recordWorkerWrites.
• exec3_parallel.go:2039-2048: the CollectorWrites guards re-check coinbaseEntry.emit / burntEntry.emit although emitCoinbase / emitBurnt gated the entries' construction just above, and the guard bodies use only the raw locals. if emitCoinbase / if emitBurnt keeps the predicate spelled once (the emit field stays load-bearing for recordedIn/writeTo).
• exec3_fee_credit_test.go:53-63: run returns tip after r.recorded.MergeInto(tip) has folded the recorded set into it, so the documented "credit calcFees produced" is really the merged union. Benign while the scenario's TxOut has no coinbase/burnt entries; a sender==coinbase variant would silently build fixtures from a worker write. Merge a clone, or fix the doc comment.
• exec3_fee_credit_test.go:30: newFeeCreditRound is the package's 5th near-copy of the scenario→fixture wiring (cf. runFinalizeTx); a shared helper on testFinalizeScenario would keep the two test families' fixtures identical.

Nit

• No main twin: main has no CollectorWrites and a 4-arg calcFees, so a straight cherry-pick won't apply. Worth a note in the PR body on the forward-port plan.
• PR body: "before either feeEntry is built" — both entries are built before the skip check (recordedIn needs e.acc). The allocation claim still holds (they are stack values); suggest "before any VersionedWrite or CollectorWrites entry is built".

[AskAlexSharov]

All eight items addressed on current head. Package suite green; BenchmarkCalcFees unchanged (206 / 87 ns/op).

Medium

1. Missing release in recordWorkerWrites — fixed, queueMapRelease before the delete. TestRecordWorkerWritesDropsCreditedTemp was red on exactly that assertion before the fix (the credit-drop half already passed). The safety argument is now local rather than call-site reasoning: recordFeeMerge returns early on an empty tip, so the recorded product is always the set calcFees allocated and never an execResult's TxOut.

2. Unstated guards — took invalidation-by-construction, but on the temp rather than on execResult: each entry now carries the version it credited, and creditedWrites rejects anything else. Mutation-checked — dropping temp.version == txVersion turns TestCreditedWritesPinsVersion red.

   Not moved onto execResult, because the credit's owner is the recorded-write-set slot in blockIO, which is block-level: the abort path replaces results[tx] while leaving the recorded set in place, so per-result provenance would strand the reclaim precisely where the release has to happen — after the caller finishes iterating prevWrites, one function past the assignment. The version pin buys the same property (a fresh taskVersion per dispatch ⇒ new incarnation ⇒ credit rejected) without mis-modelling the lifetime.

   Guard by guard: Err == nil is spelled at the call site; the incarnation bump and the CollectorWrites coherence are now enforced, since a surviving credit and the CollectorWrites entries an earlier round left behind are the same incarnation by construction.

3. White-box test — added, plus TestCalcFeesRoundThroughBlockExecutor, which drives the round through creditedWrites / recordFeeMerge instead of the fixture's stand-in. The bare-RecordWrites simulation is gone.

Low

1. recordFeeMerge push-down — done, and it took the MergeInto and the non-empty check with it; the caller now hands it (txVersion, recorded, tip) and nothing else. TestRecordFeeMergeSkipsEmptyTip pins the footgun that move closes: an empty tip must not mark the worker's TxOut as carrying a credit.
2. emit re-check — now if emitCoinbase / if emitBurnt.
3. run returning the union — returns a copy taken before the merge.
4. Fixture near-copies — the TxIn / TxOut / CollectorWrites copy moved into buildExecResult, so all five sites share it. Left the versionMap/reader wiring per-test: three of the five differ on purpose (one shares a versionMap across txs, one pre-seeds an abandoned incarnation, one stamps a per-iteration version).

Nit — forward-port note and the "before any VersionedWrite or CollectorWrites entry is built" wording are in the body. Also folded the fee-credit tests into exec3_finalize_test.go (next to the fixtures) and exec3_fee_merge_temp_test.go (next to the other blockExecutor white-box tests), so the PR no longer adds a test file.

Still draft pending the n5 soak.