erigontech / erigontech/erigon
execution/stagedsync: follow-ups for fee-merge write-set reclaim (#23099 / #23106)
- Dominant language
- Go
- Stars
- 3.6k
- Forks
- 1.5k
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 455
Description
Follow-ups from a review of #23106 (main pick of #23099, which is merged to release/3.6). That change moves the release of superseded fee-merge write sets off the hot loop into a background releaser (`recordFeeMerge` / `queueMapRelease` / `mapReleases` in `execution/stagedsync/exec3_parallel.go`).
The review confirmed the core of the change is safe: the superseded temp is unreachable once queued, queueing happens only on the exec loop, there is no double-release path, and the touched tests pass under `-race`. The items below are robustness, efficiency and convention follow-ups, roughly in priority order. Scope is main; the pool-level item was explicitly out of scope for 3.6.
### Test shield (one-line fix)
- [ ] In `exec3_fee_merge_temp_test.go`, the round-1 negative assertion (txOut must survive the fee merge) is not shielded by `be.awaitMapReleases()`. If a future bug wrongly queues a live set for release, the release waits in the channel while the test reads `Count()`, so the test would still pass most of the time — exactly the regression class it exists to catch. Add `be.awaitMapReleases()` after the first `recordFeeMerge`; `Wait` on a zero counter returns immediately.
### Memory retention and lifecycle
- [ ] Each queued `mapRelease` carries `&be.mapReleasing`. This interior pointer keeps the whole `blockExecutor` (results, tasks, blockIO, caches) reachable until the global releaser drains the entry, heaviest under memory pressure — and the WaitGroup has no production waiter, only the tests call `awaitMapReleases`. A separately allocated WaitGroup or a done-channel would pin only itself.
- [ ] The releaser is a process-global goroutine over a never-closed 4096-slot channel: no Stop/drain hook (unfriendly to goleak), and one serial consumer shared by every `parallelExecutor` in the process, so one storage-heavy release delays all instances. A per-executor releaser managed like the commitment calculator would give lifecycle, isolation and a natural drain point — or see the simplification below.
### Efficiency
- [ ] When the channel is full, the fallback runs the O(entries) map clear inline on the exec loop — exactly when release traffic peaks (revalidation storms). Skipping the release and letting GC take the set (`Done()` only) is correctness-neutral and keeps the hot path flat; the stale-temp path after re-execution already GC-drops without releasing.
- [ ] The dominant release still runs synchronously on the serial apply loop: `ReleaseOutputMaps` walks every tx's write set for every block, and the root cost (`vwMapPool` clear-on-put with no size cap) sits in the state package where a fix would help all callers. A size cap for pooled maps or a cheaper clear may shrink or remove the need for the queue.
### Robustness parity (future-conditional — verified not reachable today)
- [ ] The queued path runs `ReleaseMaps` on a bare goroutine without recover. A recoverable panic there kills the process, while the same panic inline is contained by the exec loop's deferred recover. A recover-and-log wrapper in the consumer restores parity between the two branches.
- [ ] `WriteSet` has no released-tripwire: a future use-after-supersede reader would hit a nondeterministic `concurrent map read and map write` throw instead of the deterministic empty reads it got before. The sibling `ReleaseOutputMaps` path guards this class with `outputsReleased` + `assertOutputsLive` under `dbg.AssertEnabled`; the fee-merge temps deserve the same guard.
### Coverage gap (pre-existing, from #22883)
- [ ] The finalize-merge site also supersedes the just-recorded fee-merge temp (`existingWrites` replaced by the merged set) but is not routed through `recordFeeMerge`, so those temps are never pooled and the `feeMergeTemp` entry goes stale. The ownership argument is identical to the covered site.
### Simplification (may resolve several items above at once)
- [ ] `recordFeeMerge` runs only on the exec loop, so a plain per-executor pending slice, drained at the existing block-end release point next to `ReleaseOutputMaps`, gives the same offload with zero new concurrency primitives — and makes the tests synchronous again, which also resolves the retention, lifecycle and test-shield items. Whether releasing pays at all versus a plain GC drop is still being measured (`alex/exec_dbg_37` gates it behind a `RELEASE_FEE_MERGE_MAPS` switch for A/B); if GC-drop wins, the whole mechanism can go away.
### Conventions
- [ ] The new comments say the release is kept off "the apply loop", but `recordFeeMerge` runs on the exec loop (`processResults`); the apply loop is the `applyResults` consumer. Fix the wording — the PR body itself has it right.
- [ ] The rationale comment for `queueMapRelease` is attached to `type mapRelease` and starts with a different symbol's name ("ReleaseMaps clears every map…"). Move it onto `queueMapRelease`; give the type a one-liner starting with its name, or none.
- [ ] The channel-full inline fallback and the WaitGroup handshake have no test — nothing in the tests ever fills `mapReleases`. A small test that fills the channel and asserts the inline release + `Done` would close the gap.
cc @AskAlexSharov
Contributor guide
Assessment
This issue has not been assessed yet.