cmd/utils/app: reorg-to-genesis regression test, import shutdown & port fixes - #22058
Merged
yperbasis merged 26 commits intoJul 2, 2026
Merged
Conversation
…mitment crash
Importing a chain that reorgs back to genesis under the default parallel
executor crashes the commitment calculator ("empty branch data read during
unfold"), leaving the canonical head on the lighter chain. The test drives the
real `erigon init` + `erigon import` flow because the in-process block-test
harness backs chaindata with in-memory MDBX and commits genesis in the same
execution flow, both of which mask this on-disk defect (TestLegacyBlockchain
runs the same fixture and passes).
Red under the default parallel executor, green with EXEC3_PARALLEL=false.
Reproduces #22056.
…mmitment crash
On the first execution batch the genesis (block 0) commitment is computed while
the next block's changeset accumulator is already installed, so genesis's branch
creations were recorded in that later block's changeset. Unwinding the block back
to genesis then reversed them, deleting the genesis commitment and leaving the
parallel commitment trie reading empty branch data for the root ("empty branch
data read during unfold"). Reorgs that unwind to genesis aborted, stranding the
head on the lighter chain (Hive consensus UncleFromSideChain_Cancun and the other
reorg/uncle tests that fork at genesis).
Detach the changeset accumulator while writing genesis (block 0) commitment — in
both the immediate committer path (computeWithBlockAccumulator) and the deferred
FlushPendingUpdates path — so genesis writes go into no changeset. Block 0 is
never unwound below and needs none; this matches the serial executor, whose
block-1 changeset restores the genesis commitment on unwind.
This makes TestImportReorgUnwindToGenesisParallel pass under the default parallel
executor.
Fixes #22056.
yperbasis
changed the title
yperbasis
marked this pull request as ready for review
June 26, 2026 13:23
yperbasis
requested review from
AskAlexSharov,
mh0lt and
sudeepdino008
as code owners
June 26, 2026 13:23
yperbasis
marked this pull request as draft
June 26, 2026 13:26
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a parallel-execution reorg-to-genesis crash in the commitment calculator by ensuring genesis (block 0) commitment writes are not captured into a later block’s changeset accumulator (which would then be reversed on unwind), and adds an on-disk regression test that reproduces the Hive fixture scenario.
Changes:
- Detach the changeset accumulator during block-0 commitment computation to prevent genesis writes from being recorded in another block’s changeset.
- Detach the accumulator during deferred branch flushes when the pending update is for block 0.
- Add an integration-style test that runs
erigon init+erigon importon a fixture that reorgs back to genesis under the parallel executor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| execution/tests/import_reorg_test.go | Adds an on-disk erigon init/import regression test for unwind-to-genesis reorg behavior under parallel exec. |
| execution/stagedsync/committer.go | Detaches the changeset accumulator while computing genesis commitment when cs == nil and BlockNum == 0. |
| db/state/execctx/domain_shared.go | Detaches the changeset accumulator when flushing deferred commitment updates for BlockNum == 0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The import step deliberately tolerates a non-zero exit (the fixture has an intentionally-invalid block), but discarding the error also swallowed a genuine "couldn't exec" failure, which the head assertion would only surface as a confusing "reached block 0". Keep tolerating the expected exit-code error; fail loudly on anything else.
The -race execution-tests CI job runs with t.TempDir() on a 2 GB tmpfs RAM disk (ERIGON_EXECUTION_TESTS_TMPDIR) and provides no prebuilt binary, so erigonBinaryForTest fell back to `go build ./cmd/erigon` into that RAM disk. The linker output plus go build's work dir overflow it, failing with "no space left on device". Build into build/bin/erigon (real disk) and point GOTMPDIR under build/ too, so nothing build-related lands on the RAM disk.
yperbasis
changed the title
The test runs under whichever executor the CI matrix selects (both serial and parallel), so the name shouldn't claim it's parallel-specific. The parallel shard still guards the parallel-exec defect this PR fixes.
…lper The block-0 changeset-accumulator detach was duplicated in computeWithBlockAccumulator and flushPendingUpdates; consolidate into SharedDomains.DetachAccumulatorForGenesisLocked. Behavior-preserving.
…mport-reorg test Derive the genesis config from testforks.Forks["Cancun"] instead of a hardcoded literal so it can't drift from the in-process fixture tests. lastImportedHead now returns the final canonical head (the last head-update logged) rather than the highest-numbered one.
Build erigon in test-all-erigon.yml (Linux shards) and export ERIGON_BIN so the import-reorg test consumes a prebuilt binary instead of building it inside the timed `make test-all` step.
yperbasis
marked this pull request as draft
July 2, 2026 10:43
… the DB importChain never Start()s the stack, so the deferred stack.Close() skips lifecycle Stop and chaindata/poolDB stayed open until process exit. Disable the txpool and stop the backend explicitly. The reorg regression test can then keep its datadir in t.TempDir and assert the head by reopening chaindata instead of scraping the log (which also drops the regex coupling to the head-updated log line).
… helpers SwapAccumulatorLocked left Get/SetChangesetAccumulatorLocked without external callers. Unexport them and point the LockChangesetAccumulator doc at the swap helpers so new locked-window code reaches for those instead of hand-rolling the get/set/restore dance.
yperbasis
changed the title
yperbasis
marked this pull request as ready for review
July 2, 2026 11:32
defer ethereum.Stop() was registered after ethereum.Init, so an Init failure (e.g. a malformed --ethstats URL) returned with the backend never stopped — leaking the open chaindata exactly like the pre-fix success path. Register the defer right after eth.New succeeds; every field Stop touches is either set by eth.New or nil-guarded, and Init's bgComponentsEg goroutines are only spawned after its error returns. TestImportClosesChaindataOnInitError pins the behavior: it fails Init via a malformed --ethstats URL and asserts chaindata can be reopened in-process afterwards (red before the move with mdbx_env_open EAGAIN). The fixture loading moves into loadImportFixtureCase, shared by both tests.
A one-shot import needs no listeners, yet plain 'erigon import' bound JSON-RPC :8545, private-api gRPC :9090, authrpc :8551 and p2p TCP+UDP :30303 — and failed outright when 9090 or 30303 were taken (e.g. an erigon node running on the same box). Extend the forced-flag map with http=false, private.api.addr= and authrpc.port=0 (authrpc has no disable flag; 0 binds an ephemeral port) and set nodeCfg.DisableSentry, which has no CLI flag. Both tests shed the per-caller compensating flags, including the misleading --networkid 1337 (1337 silently resolves the registered 'test' chain; the stored genesis drives the import either way). Also skip the ~2s KZG trusted-setup warmup: the deferred Stop waits it out at exit, dominating short imports (the hive eest/consume-rlp pattern); kzg.Ctx() lazy-inits if a block actually needs the trusted setup. TestImportReorgUnwindToGenesis drops from ~1.7s to ~0.2s, and both import tests now pass with :30303 and :9090 deliberately occupied. In TestImportReorgUnwindToGenesis, check the canonical genesis hash before the import error text so chain-config drift hits its dedicated diagnostic, and hint at the legacy-tests submodule when the fixture read fails.
yperbasis
changed the title
yperbasis
enabled auto-merge
July 2, 2026 14:03
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 4, 2026
…erigontech#22203) Fixes the data race that failed the `race-tests / tests-linux (other, serial)` job on erigontech#22163's CI run ([failing job](https://github.com/erigontech/erigon/actions/runs/28648204895/job/84959805713)): `TestImportClosesChaindataOnInitError` flagged `Aggregator.Close`'s `wg.Wait` racing `MergeLoop`'s `wg.Add`. ## Root cause `MergeLoop`, `BuildFilesInBackground` (on both `Aggregator` and `ForkableAgg`) and `BuildFiles2` register on the aggregator's lifecycle WaitGroup from whatever goroutine calls them. For external callers nothing orders that `Add` against `Close`'s `Wait`. An `Add` from a zero counter concurrent with `Wait` is `sync.WaitGroup` reuse (undefined behavior, flagged by `-race`), and semantically the unregistered goroutine can keep running while `Close` tears down the dirty files. The CI failure hit the `MergeLoop` door (the background-maintenance goroutine `eth.New` spawns), but the same race is reachable through the sibling doors, so this PR guards all of them: - `Aggregator.BuildFilesInBackground` — live on a stock node: the fire-and-forget FCU background-prune goroutine (`FcuBackgroundPrune` defaults to true) ends in `CollateAndPrune → BuildFilesInBackground`, and its adaptive budget can reach 2/3 slot ≈ 8s on mainnet while `Ethereum.Stop` waits on the exec semaphore for at most 5s (`WaitIdle`) before proceeding to `chainDB.Close → Aggregator.Close → wg.Wait`. Same shape for `ProcessFrozenBlocks`' commit cycle, which calls `BuildFilesInBackground` right after a ctx-oblivious MDBX commit. - `Aggregator.BuildFiles2` and `ForkableAgg.BuildFilesInBackground` — same unguarded caller-side `Add`; currently only reachable from tooling/tests, guarded all the same. erigontech#21528 fixed this class for the *nested* spawn sites by making the already-registered parent goroutine `Add` before spawning; that pattern can't cover the entry points themselves — the registration has to be lifecycle-aware. The window became CI-visible when erigontech#22058 added a test that starts the import node, fails init, and immediately stops it. ## Fix A small `closingWaitGroup` (a `sync.WaitGroup` with a mutex-guarded close latch), shared by `Aggregator` and `ForkableAgg`: - `TryAdd` registers unless the latch is set. Every external entry point (`MergeLoop`, `BuildFilesInBackground`, `BuildFiles2`) refuses once closing and returns its usual "nothing to do" result, like the existing `dbg.NoMerge()` / `buildingFiles`-CAS early-outs (the CAS is unwound and `fin` closed on refusal). - `Close` latches via `BeginClose` before cancelling the context and waiting. The mutex gives the happens-before edge: every `Add` is either strictly ordered before `Close`'s `Wait`, or refused. - `BeginClose` doubles as the Close-idempotency latch, replacing the previously unsynchronized `ctxCancel == nil` check / `ctxCancel = nil` write — two concurrent `Close` calls used to race on `ctxCancel` and could invoke a nil func; `Close` is now safe to call concurrently with itself. - Registration goes through a single path — `TryAdd`. The fire-and-forget merge spawns use it too (refused once closing: the merge is skipped and `fin` closed, rather than an unconditional `Add`), and the `buildFiles` errgroup children no longer touch the lifecycle `wg` at all (see *Why dropping the `wg.Add` in `buildFiles` is safe* below). ## TDD Red first, one test per door plus concurrent-Close, each reproducing the exact race signature under `-race` before the fix: - `TestAggregatorCloseVsConcurrentBuildFilesInBackground` — `wg.Wait` (`aggregator.go:638`) vs `wg.Add` (`aggregator.go:2171`) - `TestAggregatorCloseVsConcurrentBuildFiles2` — `wg.Wait` vs `wg.Add` (`aggregator.go:1167`) - `TestAggregatorConcurrentClose` — data races on `ctxCancel` (reads at `aggregator.go:627`/`633` vs the nil-write at `634`) - `TestForkableAggCloseVsConcurrentBuildFilesInBackground` — `forkable_agg.go:462` vs the `BuildFilesInBackground` `Add`, plus the secondary merge-internals vs `closeDirtyFiles` race - `TestForkableAggConcurrentClose` — `ctxCancel` races escalating to an actual nil-func-call panic Green after the fix. Verified additionally: - all Close-related tests (the five above plus `TestAggregatorCloseVsConcurrentMergeLoop`, `TestForkableAggCloseVsConcurrentMergeLoop`, both erigontech#21528 regression tests, and `TestAggregatorCloseReleasesBranchCache`) pass under `-race` with `-count=2`, zero race reports - `TestAggregatorCloseVsConcurrentMergeLoop` also got cheaper: it burned ~42s under `-race` (~97% idle in `WaitForFiles`' 3-second poll ticker whenever a merge attempt overlapped `Close`); with 4 iterations and `t.Parallel` it runs in 6–9s and the whole Close suite finishes in ~19s under `-race -count=2` - `go test -short ./db/state/... ./db/kv/temporal/...` passes - `make lint` clean (two consecutive runs), `make erigon integration` builds - `TestImportClosesChaindataOnInitError` (the original CI failure) passes locally without `-race`; it cannot run under `-race` on darwin/arm64 at all (`fatal error: too many address space collisions for -race mode` at startup — a Go runtime limitation with this test's MDBX mappings, unrelated to this change), so the Linux `race-tests` job on this PR is the definitive check for it ## Why dropping the `wg.Add` in `buildFiles` is safe `buildFiles` (and forkable `buildFile`) build their per-domain / per-II files on an `errgroup.Group` and block on `g.Wait()` before returning. Two facts make a *separate* lifecycle-`wg` registration of those children redundant: 1. `buildFiles` is only ever called **synchronously** from the background goroutine that already registered on the lifecycle `wg` via `TryAdd` (the `buildFilesInBackground` / `BuildFiles2` goroutine). 2. That goroutine cannot reach its own `defer wg.Done()` until `buildFiles` returns — i.e. until `g.Wait()` has joined every child. So `Close`'s `wg.Wait()` already blocks on those children **transitively**, through the still-held count of the entry goroutine. Registering them on the lifecycle `wg` as well was pure double-counting — it changed the counter value but not the set of goroutines `Close` waits for. Removing it keeps `Close`'s guarantee intact while getting rid of an `Add` whose safety depended on the caller's context (which Go code can't observe — a function doesn't know whether it runs inline or in a goroutine). --------- Co-authored-by: Alexey Sharov <AskAlexSharov@gmail.com>
taratorio
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Jul 13, 2026
…behind SwapChangesetAccumulatorLocked (erigontech#22160) Split out of erigontech#22058 so the reorg-to-genesis regression test and this refactor can land independently. ## Accumulator-swap cleanup The swap/restore pattern around commitment writes is consolidated behind `SharedDomains.SwapChangesetAccumulatorLocked(acc)` (returns a restore func), with `DetachChangesetAccumulatorLocked` as its nil case. The previously hand-rolled swap/restore sequences in `flushPendingUpdates`, `flushPendingUpdatesWithoutChangeset`, `computeWithBlockAccumulator`, and `computeIsolated` now use the helpers, collapsing the duplicated restores on error/success paths. `flushPendingUpdatesWithoutChangeset` scopes its locked window in a closure so restore and unlock are defer-based, while the error publish stays outside the lock (the send can block on the apply loop, which contends on `changesetMu`). The consolidation leaves `Get`/`SetChangesetAccumulatorLocked` with no callers outside `SwapChangesetAccumulatorLocked` itself, so they are unexported (with the public `Set`/`GetChangesetAccumulator` delegating to them), and the `LockChangesetAccumulator` / `GetChangesetAccumulator` docs point locked-window callers at the swap helpers 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.
Summary
Under the parallel executor, importing a chain that reorgs back to genesis used to crash the commitment calculator (
empty branch data read during unfold) and leave the canonical head stuck on the lighter chain: genesis (block 0) commitment branch writes were recorded into the next block's changeset, so unwinding back to genesis reversed them and deleted the genesis commitment (#22056).The crash itself is fixed on main since #22092:
ownsChangesetroutes genesis and pre-window blocks throughcomputeIsolated, which computes and flushes deferred branch updates under a nil changeset accumulator, so their writes can no longer land in a later block's changeset. Earlier revisions of this PR carried a genesis-specific fix (DetachAccumulatorForGenesisLocked); #22092's more general routing made it redundant and it has been dropped. What remains:Regression tests
TestImportReorgUnwindToGenesis(cmd/utils/app) runs the realinit+importcommands in-process and asserts the canonical head advances to the heavier side chain (block 4). It reproduces the crash on pre-#22092 code and is green on current main. It drives the actual command path rather than the in-process block-test harness, which commits genesis through a different path that doesn't hit the bug. Skipped in-shortmode.The test asserts directly on the datadir: after import it reopens chaindata read-only and checks the canonical genesis hash (guarding chain-config drift, checked first so drift gets its dedicated diagnostic) plus the head block hash/number, all under
t.TempDir. Both commands run with--log.dir.disableso the process-wide root logger doesn't keep a log file under the datadir open past the commands (Windows can't delete open files).TestImportClosesChaindataOnInitErrorpins the shutdown fix on its error path: it makesethereum.Initfail via a malformed--ethstatsURL and asserts chaindata can be reopened in-process afterwards — red before the defer moved up (mdbx_env_open: resource temporarily unavailable), green after.Import command shutdown
Asserting on the DB is possible because
importnow shuts down cleanly instead of holding chaindata open until process exit:importChainneverStart()s the node stack, so the deferredstack.Close()skips lifecycle Stop andEthereum.Stop()never ran — leaking the open chaindata, the txpool DB, the authrpc listener, and background goroutines for the remainder of the process. The command now force-disables the txpool (a one-shot import doesn't need it, andStopwouldn't close its DB) and stops the backend explicitly viadefer ethereum.Stop(), registered right aftereth.Newsucceeds soInitfailures are covered too.Note that
Stopwaits out the aggregator's in-flight background file builds/merges (WaitForFiles), so an import large enough to cross a step boundary now finishes those builds before exiting instead of abandoning them at process exit.Import command isolation & per-run cost
A plain
erigon importused to bind the default listeners while importing — JSON-RPC :8545, private-api gRPC 127.0.0.1:9090, authrpc :8551, p2p TCP+UDP :30303 — and failed outright when 9090 or 30303 were taken (e.g. an erigon node running on the same box). A one-shot import needs none of them: the forced-flag map now also disables HTTP and the private API and moves authrpc to an ephemeral port (it has no disable flag), andnodeCfg.DisableSentry(no CLI flag) turns p2p off entirely, which also let both tests drop their per-caller isolation flags including the misleading--networkid 1337.The KZG trusted-setup warmup is skipped too (
WarmupKzgCtxOnInit = false): the new deferredStopwaits for it, which added up to ~2s to short imports — the hive eest/consume-rlp per-test pattern whose startup cost the--nat=noneforcing already targets.kzg.Ctx()lazy-inits if a block actually hits the point-evaluation precompile.TestImportReorgUnwindToGenesisdrops from ~1.7s to ~0.2s with these changes, and both import tests pass with :30303 and :9090 deliberately occupied.Fixes #22056.