execution/stagedsync: drop dead finalizeWithIBS + finalizeTx (delta-args)#21211
Merged
Conversation
…rgs) and their tests First incremental cut toward the structural goal of #21138: one finalize function per parallel-exec result, with no IntraBlockState used outside workers. This commit removes two finalize variants that are unreachable from production code: * finalizeWithIBS (~120 LOC) — full IBS reconstruction path that was kept around for the BAL-compat strip→rebase→merge route. Zero callers on main since the BAL fix in #21177 + the parallel-exec stack in #21153 rerouted that flow through finalizeTxSimple. * finalizeTx (delta-args variant, ~250 LOC) — direct-fee-balance path consumed only by TestFinalizeTx_AllScenarios and its hasCoinbaseDelta / adjustForTransferDelta helpers. Production calls finalizeTxSimple via the dispatch in `finalize`; this variant was test-only by 958b2fb. Test cleanup removes: * runFinalizeTx test runner * TestFinalizeTx_SimpleTransfer, TestFinalizeTx_London, TestFinalizeTx_AllScenarios * coinbaseIsRecipientScenario, selfTransferScenario fixture builders * hasCoinbaseDelta, adjustForTransferDelta, buildWriteMap, fmtWriteVal, extractBalanceReads helpers that had no other consumers Live finalizeTxSimple tests (TestFinalizeTxSimple_*) and the surviving TestResolveStorageWrites_*, TestNormalizeWriteSet_*, TestSelfDestructPath_* families are untouched. Updates one stale comment in engine_api_bal_test.go that referenced finalizeWithIBS. Verified: make lint clean; full execution/{stagedsync,state,tests} and rpc/jsonrpc test packages green under EXEC3_PARALLEL=true; BAL family 8/8 parallel; EIP-7708 burn-log family green. No semantic change — dead code removal only.
This was referenced May 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Removes two transaction-finalization implementations (finalizeWithIBS, finalizeTx delta-args path) and the dedicated tests that exercised the delta-args finalize flow, continuing the consolidation toward a single per-tx finalize path (finalizeTxSimple) in the parallel execution pipeline.
Changes:
- Deleted unreachable finalize variants (
finalizeWithIBS,finalizeTx) fromexec3_parallel.go. - Removed the
TestFinalizeTx_*test family and related fixture/helper code that only covered the removed delta-args path. - Updated a stale BAL test comment to reference
finalizeTxSimpleinstead of the removed finalize path.
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 | Drops the dead finalize variants, leaving regular tx finalization routed through finalizeTxSimple. |
| execution/stagedsync/exec3_finalize_test.go | Removes tests and helpers that only exercised the deleted delta-args finalize path. |
| execution/engineapi/engine_api_bal_test.go | Updates a comment to reference the current finalize path (finalizeTxSimple). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
approved these changes
May 18, 2026
yperbasis
left a comment
yperbasis
left a comment
Member
There was a problem hiding this comment.
LGTM — pure dead-code removal, scope is tight; verified locally that the dispatcher at exec3_parallel.go:1449-1466 only routes to finalizeSystemTx / finalizeTxSimple (zero production callers for the removed functions), build/vet/surviving TestFinalizeTxSimple_* and TestEngineApiBAL* green.
Three minor nits (non-blocking):
-
Two stale comment references to the now-deleted
finalizeTxremain outside this diff:execution/state/parallel_fixes_test.go:125— "The bug was that finalizeTx appended writes without Version…"execution/stagedsync/exec3_2cache_test.go:162— "legacy IBS path or direct finalizeTx path"
Could be touched up here (or rolled into PR 2 in the #21138 sequence) so grep-by-symbol stops pointing at vanished functions.
-
Adjacent dead code already on main at
exec3_parallel.go:1459(_, _, _ = coinbaseDelta, coinbaseDeltaIncrease, hasCoinbaseDelta) is now provably unused oncefinalizeWithIBSis gone. Pre-existing, out-of-scope for this PR, but a clean one-line follow-up. -
The deleted
TestFinalizeTx_AllScenarioshadt.Skip("… see #20962")on thecoinbase_is_recipientsubcase. Removing the function implicitly resolves #20962 — worth either noting that in the PR body or closing the issue when this merges.
CI checklist boxes (race-tests / kurtosis / hive) still unchecked — worth waiting on those given #21153 / #21017 history.
… method (#21212) ## Stack **Depends on:** #21211 — first cut of [#21138](#21138 heuristic / IBS-dependency removal sequence (dead-finalize cleanup). Must merge first. ## Summary Pure structural cleanup of `finalizeTxSimple`. Pulls the ~55 lines that construct a minimal `IntraBlockState` to feed `postApplyMessageFunc` (AuRa system calls / EIP-7708 burn-log emission via `LogSelfDestructedAccounts`) into its own method, `runPostApplyMessageOnMinIBS`. **No semantic change.** Behavior identical: same engine post-apply hook, same IBS construction, same control flow position. ## Why This is preparation for the IBS-removal PR that follows in the [#21138](#21138) sequence. The end-state #21138 drives toward is *one `finalizeTx` function, no IBS used in the parallel-exec code path outside workers*. Today the live IBS dependency in `finalizeTxSimple` is entangled with surrounding fee-credit logic that has nothing to do with IBS — pulling them apart here, without changing behavior, makes the IBS-removal PR small enough to review confidently. The extracted method's godoc enumerates the three IBS dependencies it carries today, so the swap point is clearly flagged: 1. `ibs.GetRemovedAccountsWithBalance()` lookup (consumed inside `LogSelfDestructedAccounts`). 2. `ibs.AddLog` → `ibs.GetLogs` log-buffer round-trip (so EIP-7708 burn logs reach the receipt). 3. `ibs.AddBalance` fee-credit bookkeeping (so the SD'd coinbase carries `FeeTipped` at the time `LogSelfDestructedAccounts` inspects it). `finalizeTxSimple` shrinks from 257 → 205 lines; the IBS-using block becomes a single method call. ## Test plan - [x] `make lint` clean - [x] `make test-short` green under `EXEC3_PARALLEL=true` across: `execution/stagedsync`, `execution/state`, `execution/tests`, `execution/engineapi`, `execution/execmodule`, `rpc/jsonrpc` - [x] `TestEIP7708BurnLogWhenCoinbaseSelfDestructs` green - [x] `TestEngineApiBAL*` family green - [x] `TestLegacyBlockchain/ValidBlocks/bcEIP3675` green - [ ] CI: race-tests, kurtosis, hive matrix legs green on both serial and parallel ## What's next in the sequence - **PR 3** (next): drain all three IBS deps from `runPostApplyMessageOnMinIBS`. Add explicit SD-with-balance computation on `ExecutionResult`, change `LogSelfDestructedAccounts` to return `[]*types.Log` instead of using `ibs.AddLog`, and remove the `ibs.AddBalance` bookkeeping (which exists only to inform `LogSelfDestructedAccounts`). `finalizeTxSimple` becomes IBS-free. - Later PRs: replace `normalizeWriteSet` with `filterWritesByVersionMap`; replace `calcState.ApplyWrites` with `VersionedWrites.TouchUpdates`; move EIP-7002/7251 syscalls into the worker pool (separate PR — requires interface change to `Engine.Finalize` / `SysCallContract`). ## Related - #21211 — #21138 PR 1 (dead-finalize cleanup, this PR's direct dependency) - #21138 — heuristic / IBS-dependency removal tracker (the parent) - #21153 — parallel-exec correctness stack (merged 2026-05-15)
# Conflicts: # execution/stagedsync/exec3_finalize_test.go # execution/stagedsync/exec3_parallel.go
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
yperbasis
added a commit
that referenced
this pull request
May 22, 2026
Five conflicts: 1) cmd/rpctest/rpctest/account_range_verify.go — main added defer Close() on the two MDBX handles. Trivial: kept both. 2) db/migrations/migrations.go — main landed #21280 (the legacy E2 tables drop spinoff originally cut from this branch), which adds dropLegacyE2Tables. Moksha has both dropLegacyE2Tables and dropAccountIncarnation registered. Resolution: keep both in the ChainDB sequence. 3) db/kv/kvcache/cache.go — main removed AssertCheckValues entirely (the txpool stub call site went away with #21280's caller cleanup). Moksha had it as a no-op stub. Take main's deletion; the only reference left was a commented-out test line. 4) execution/stagedsync/exec3_finalize_test.go — PR #21211 deleted ~240 lines of test scenarios (TestFinalizeTx_SimpleTransfer, _London, _AllScenarios, coinbaseIsRecipientScenario, selfTransferScenario, hasCoinbaseDelta, adjustForTransferDelta) because the finalizeWithIBS / finalizeTx (delta-args) code paths they exercised were dead. Take main's deletion. 5) execution/state/intra_block_state.go — three blocks: a) Same-block-revival check in CreateAccount. Main (#21319) simplified it to `!account.Empty()` on the version-map-refreshed record. Moksha had a path-by-path LatestTxIndex scan (BalancePath/NoncePath/CodeHashPath > destructTxIndex). Take main's cleaner check — semantically equivalent on the same input (the refreshed `account` reflects exactly those higher- txIndex writes). b) prevInc / IncarnationPath bookkeeping for CreateAccount on main. Entirely incarnation-dependent; moksha has no IncarnationPath and no Account.Incarnation. Take HEAD's empty resolution. c) Synthetic versionRead(IncarnationPath, ...) in CreateAccount on main. Same reason. Take HEAD's empty resolution. Plus two non-conflicting touch-ups dropping IncarnationPath references that the merge silently brought in via main-side changes: - execution/state/versionmap.go MVReadResultNone/MapRead recursive- cross-check whitelist dropped IncarnationPath from the path list. - execution/state/versionedio.go SD short-circuit zero-value-fallback whitelist dropped IncarnationPath from the path list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
First incremental cut toward #21138's structural goal: one finalize function per parallel-exec result, with
IntraBlockStateused nowhere outside workers.This PR removes two finalize variants that are already unreachable from production:
finalizeWithIBS(full IBS reconstruction, BAL-compat path)finalizeTx(delta-args variant, direct fee-balance path)TestFinalizeTx_AllScenarios)Plus the test suite that exclusively exercised the delta-args path (
TestFinalizeTx_*, fixture builderscoinbaseIsRecipientScenario/selfTransferScenario, helpershasCoinbaseDelta/adjustForTransferDelta/buildWriteMap/fmtWriteVal/extractBalanceReads) and one stale comment inengine_api_bal_test.go.Net: -690 lines, +1 line, no semantic change.
Why now
The parallel-exec correctness stack landed in #21153 (merged 2026-05-15). The combined effect of that PR plus #21177 routed all production finalize flows through
finalizeTxSimple— these two functions became unreachable. Removing them shrinksexec3_parallel.gofrom 3640 → 3268 lines, making subsequent IBS-dependency drains easier to review.The next steps in the #21138 sequence:
LogSelfDestructedAccountsconsumesresult.SelfDestructedWithBalanceonly, noibs.GetRemovedAccountsWithBalance()call.AddLog→ return logs) and Pull go-ethereum up to 26aea736 (7 Feb 2019) #3 (AddBalancebookkeeping → already onCollectorWrites);finalizeTxSimplebecomes IBS-free.normalizeWriteSet→filterWritesByVersionMap;calcState.ApplyWrites→VersionedWrites.TouchUpdates; move EIP-7002/7251 syscall execution into the worker pool.End state: one
finalizeTx, no IBS outside workers.Test plan
make lintcleanmake test-short(fullexecution/stagedsync,execution/state,execution/tests,rpc/jsonrpcpackages) green underEXEC3_PARALLEL=trueTestEngineApiBAL*) 8/8 parallelTestEIP7708BurnLogWhenCoinbaseSelfDestructsgreenTestFinalizeTxSimple_*family greenRelated