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[tx] is a separate container calcFees never reads, and feeMergeTemp[tx] == blockIO.WriteSet(txIndex) stops holding the moment a tx re-executes.

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

BenchmarkCalcFees, 200000x n=4:

	ns/op	allocs/op	B/op
first_credit	208	6	528
redundant_recredit	92	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 either feeEntry is built, so an EIP-161 emptied coinbase no longer allocates on it.

Same change as #23131 (release/3.6). main has no CollectorWrites on the result, so the two SetAccountBalanceOrDelete calls that sit alongside the emission there are absent here, and the test harness drops the matching setup line. Everything else is identical.

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.

[Copilot]

Pull request overview

This PR optimizes the staged sync “apply loop” fee-credit path by avoiding redundant per-validation-round fee re-credits when the already-recorded write set for a transaction already contains the exact same coinbase/burnt-account credit (including value, version, reason, and the required AddressPath sibling write).

Changes:

• Extend execResult.calcFees with a credited *state.WriteSet input and short-circuit to no-op when the fee credit is already present in that recorded set.
• Refactor the duplicated coinbase/burnt fee-write emission logic into a shared feeEntry helper with recordedIn and writeTo.
• Add targeted unit tests and a benchmark covering redundant re-credit skipping, balance-base changes, missing AddressPath, and the blockExecutor.creditedWrites contract.

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 the “already credited” fast-path, introduces feeEntry, and wires creditedWrites into the validation-loop fee-credit call site.
execution/stagedsync/exec3_finalize_test.go	Updates existing finalize tests for the new calcFees(..., credited) signature.
execution/stagedsync/exec3_fee_credit_test.go	New tests + benchmark validating the skip behavior and the creditedWrites pointer-identity rule used to avoid stale-credit reuse on re-exec.

---

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

[AskAlexSharov]

Carries the review follow-ups from #23131 (#23131 (review)): recordWorkerWrites clearing feeMergeTemp on re-execution, lazy feeEntry construction, and the EIP-161 delete + recordedIn/writeTo round-trip tests.

The CollectorWrites half of that feedback does not apply here — main has no such field, so there was never anything above the check to move.

[Copilot]

Pull request overview

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

Suppressed comments (1)

execution/stagedsync/exec3_parallel.go:2086

• This type also represents the burnt-fee adjustment, so describing every entry as part of a tip credit is inaccurate. Use a fee-neutral description to keep the abstraction's contract clear.

// feeEntry is one address's share of a tip credit: the post-credit account,
// whose Balance is also the BalancePath value, or a delete when EIP-161 removes
// the emptied account instead.

[yperbasis]

Combined review — my pass plus the Codex finding folded in. No correctness bug found: every skip mismatch fails toward a full re-credit. Remaining items by severity.

High

• recordWorkerWrites (exec3_parallel.go:2500) deletes the displaced fee-merge product from feeMergeTemp without queueMapRelease, so its maps go to GC on every re-execution of an already-credited tx (same as the Codex point). Not a regression — base leaked the same way — but the new helper is the natural owner of the release. Commit d85b484 on the 3.6 twin (alex/calcfees_skip_recredit_36) already implements the model: release in recordWorkerWrites, version-pinned feeMergeTemp, recordFeeMerge owning the merge+record, and TestRecordWorkerWritesDropsCreditedTemp asserting the release. Please port it here; it also closes the first medium item and the first low item.

Medium

• The finalize-path RecordWrites (exec3_parallel.go:2957) bypasses the feeMergeTemp protocol. Latent today because finalizeTx returns nil writes for regular txs, but if that changes, a stale temp silently disables the skip and its maps become unreclaimable. The version pin in the ported commit closes this.
• feeEntry's zero value reads as "already recorded" (exec3_parallel.go:2092): a future construction site that forgets emit: true makes recordedIn vacuously true and writeTo a no-op, so the credit is dropped with no error and no failing test. Nil *feeEntry = absent would make that state unrepresentable.
• recordedIn compares *aw.Val == e.acc (exec3_parallel.go:2109), stricter than Account.Equals because it also includes Root and PrevIncarnation. If feeAddressAccount ever fills those asymmetrically, the skip silently stops firing — an invisible perf regression. Use an Equals-based compare, or add a comment plus a symmetry test row.
• Test gap (exec3_fee_credit_test.go:153): the deleted arm of recordedIn accepts any same-version SelfDestruct write, more than the docstring claims; the fences that make this benign (worker writes stamped with TxNum=0) are not pinned by any test. Add a foreign-SD rejection row or soften the docstring.

Low

• The temp-identity predicate exists twice: creditedWrites (exec3_parallel.go:2509) and recordFeeMerge (line 2521). The ported commit dissolves it.
• feeEntry.acc and .reason are dead in the deleted state (exec3_parallel.go:2056); populate them only in the credit arm.
• When the coinbase is the burnt contract address, the skip can never fire: one recorded write cannot carry both tracing reasons (exec3_parallel.go:2073). Perf-only and pathological; a short comment is enough.
• newFeeCreditRound (exec3_fee_credit_test.go:30) re-copies the runFinalizeTx scaffold, and recorded/credited alias the same pointer after the first round (line 61). The ported commit's test consolidation is a good moment to fix both.

Follow-up outside this PR: with the skip keeping the set identity stable, the remaining per-round cost is the unconditional FlushVersionedWrites of an unchanged set (exec3_parallel.go:2869); a (set pointer, flag) memo could remove it.

[AskAlexSharov]

All items addressed on current head. Package suite green; BenchmarkCalcFees unchanged — 195 ns/op, 6 allocs for the first credit against 87 ns/op, 2 allocs for the skipped round, so the skip still fires.

High

Ported. recordWorkerWrites takes the version instead of the index, queueMapReleases the displaced product before dropping it, and recordFeeMerge now owns the MergeInto and the RecordWrites — so the merge product and the tx's recorded set cannot drift apart, and the "a temp is never an execResult's TxOut" rule is local to the one function that populates the map.

I drove the leak red before porting: with the old recordWorkerWrites, tip.Count() after a re-execution is 1, not 0. TestRecordWorkerWritesDropsCreditedTemp is that assertion.

Medium

1. Finalize-path RecordWrites — closed by the version pin, as you said. feeMergeTemp now stores feeMerge{writes, version} and creditedWrites rejects anything whose version is not the caller's, so a temp that outlives its recorded set cannot be handed back. TestCreditedWritesPinsVersion covers the re-executed incarnation, another tx's product, and a nil set.

2. feeEntry's zero value — took your suggestion: absent is nil *feeEntry, emit is gone. recordedIn and writeTo are nil-receiver methods, so a construction site that skips an entry gets the absent behaviour by not building it, and there is no longer a field to forget. TestFeeEntryNilIsAbsent pins both halves.

3. recordedIn stricter than Equals — switched to aw.Val.Equals(&e.acc). Equals compares exactly the four fields feeAddressAccount fills; Root and PrevIncarnation are set by neither side, so folding them in could only ever turn the skip off silently.

4. Test gap on the deleted arm — added TestFeeEntryDeletedRejectsForeignSelfDestruct, which pins the fence rather than softening the docstring: a SelfDestruct write left unstamped (the worker's own) is rejected, and so is a same-version write whose Val is false. The docstring now says the version stamp is what makes the arm safe.

Low

1. Duplicated temp-identity predicate — gone; recordFeeMerge is the only place that compares against feeMergeTemp.

2. Dead acc/reason in the deleted state — the coinbase entry fills them only in the credit arm now.

3. Coinbase == burnt address — comment added at the skip.

4. newFeeCreditRound — run returns a copy of the credit, so it stays observable after MergeInto folds the recorded set into it in place. The scaffold itself stays: unlike the 3.6 twin, buildExecResult on main does not populate TxIn/TxOut, so those two lines are doing real work. For the "not only the hand-mirrored fold" half I added TestCalcFeesRoundThroughBlockExecutor, which runs the round the way the apply loop does — creditedWrites in, recordFeeMerge out — against the real feeMergeTemp bookkeeping. TestCreditedWrites and TestCreditedWritesAfterReExecution are superseded by the new pair and removed.

The follow-up on the unconditional FlushVersionedWrites of an unchanged set is worth its own PR — the set identity is stable now, so the (set pointer, flag) memo you describe has something to key on. Not in this one.