execution/stagedsync: skip the fee credit when the recorded set already carries it - #23132
Open
AskAlexSharov wants to merge 4 commits into
Open
execution/stagedsync: skip the fee credit when the recorded set already carries it#23132AskAlexSharov wants to merge 4 commits into
AskAlexSharov wants to merge 4 commits into
Conversation
…dy carries it The apply loop re-credits every tx once per validation round, and a tx is revalidated whenever an earlier tx's write set moves under it. calcFees rebuilt an identical write set each round and the caller merged it back in, so a credit that never changed cost a WriteSet, four VersionedWrites, an Account, an O(tx writes) MergeInto and a ReleaseMaps every 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. The comparison covers the balance value, version and reason plus the AddressPath account, so a moved base balance or a half-recorded credit still re-credits. Comparing against feeMergeTemp rather than TxOut is what makes this safe: calcFees reads TxOut as the pre-credit balance, so folding into it re-adds the tip every round. BenchmarkCalcFees, 200000x n=6: redundant_recredit 313ns -> 92ns, 10 -> 2 allocs/op, 928B -> 192B/op. The first credit of a tx is unchanged. (cherry picked from commit 53ee047)
Maintain CollectorWrites before the skip so the skipping and emitting paths leave the same state behind for the round. feeEntry.emit is now the only gate on whether an entry is written, the emptied-coinbase predicate is named once instead of spelled out twice, and the methods take a pointer so the entry is not copied per call. The benchmark never returned the emitted set, so the pools it checks maps out of stayed cold and the emit arm was measured against an allocation the apply loop does not pay. Releasing the maps each iteration puts the baseline at 202ns/544B/6 allocs, so the redundant round saves 54%, not 69%. Tests: the changed-balance case now mutates only the balance, the London case uses londonTransferScenario instead of rebuilding an inconsistent one, and a new test drives the re-execution invalidation through VersionedIO rather than hand-placed feeMergeTemp entries. (cherry picked from commit 2b25b1a)
Contributor
There was a problem hiding this comment.
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.calcFeeswith acredited *state.WriteSetinput 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
feeEntryhelper withrecordedInandwriteTo. - Add targeted unit tests and a benchmark covering redundant re-credit skipping, balance-base changes, missing
AddressPath, and theblockExecutor.creditedWritescontract.
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
marked this pull request as ready for review
August 10, 2026 01:55
recordWorkerWrites replaces the two RecordWrites calls in the result path so that installing a new worker output also clears feeMergeTemp[tx]. The skip no longer relies on the recorded-set pointer changing underneath it to notice a re-execution; provenance is dropped where it is lost. Move the CollectorWrites updates back below the no-op check and build each feeEntry only when it is emitted. An unchanged credit leaves CollectorWrites holding the identical values an earlier round put there, so maintaining it on a skipped round is pure cost — and for an EIP-161 emptied coinbase SetAccountBalanceOrDelete allocates a VersionedWrite every call. Cover the two paths that had none: the EIP-161 delete, and recordedIn against writeTo directly, so the pair cannot drift into a permanent skip or a permanent re-credit without a test failing. BenchmarkCalcFees, 200000x n=6: redundant_recredit 94ns -> 87ns against a 206ns first credit. (cherry picked from commit b5e2832)
AskAlexSharov
enabled auto-merge
August 11, 2026 02:42
Collaborator
Author
|
Carries the review follow-ups from #23131 (#23131 (review)): The |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, fourVersionedWrites, anAccount, an O(tx writes)MergeIntoand aReleaseMapseach time.calcFeesnow 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 theAddressPathaccount.Why compare against
feeMergeTempand notTxOut.calcFeesreadsTxOutas the pre-credit balance, so folding into it re-adds the tip every round. That is whatWriteSet.Absorbdid onalex/fee_merge_in_place_37: 715 Wrong-trie-root errors on n5 within a minute, unit suites green throughout.feeMergeTemp[tx]is a separate containercalcFeesnever reads, andfeeMergeTemp[tx] == blockIO.WriteSet(txIndex)stops holding the moment a tx re-executes.feeEntryalso folds together the coinbase and burnt emission, which was the same code written twice.BenchmarkCalcFees, 200000x n=4:Both arms release the emitted set's maps, as
recordFeeMergedoes, so the pools are warm. The skipped round returns before theCollectorWritesupdate and before eitherfeeEntryis built, so an EIP-161 emptied coinbase no longer allocates on it.Same change as #23131 (release/3.6).
mainhas noCollectorWriteson the result, so the twoSetAccountBalanceOrDeletecalls 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". TheAbsorbattempt above is why green unit suites are not sufficient evidence here.