db/state, execution/commitment: fix parallel-commitment worker reads + add COMMITMENT_PARALLEL toggle - #22354
Conversation
With COMMITMENT_PARALLEL set globally, every SharedDomains selected the parallel trie, but only DB-backed consumers (exec, builder, squeeze, backtester) wire the per-worker TrieContextFactory it needs — integrity checks, RPC-created domains, and test harnesses failed with 'ParallelPatriciaHashed.Process requires a TrieContextFactory'. The context now starts on the sequential trie and upgrades to the selected parallel/streaming variant when EnableParaTrieDB provides the DB. SeekCommitment may restore state before the DB is wired, so the upgrade adopts the already-restored trie as the parallel template instead of re-encoding (SetState re-reads a sole-account root through the not-yet-installed context). Touching before the upgrade panics: keys collected on the sequential buffer would be dropped.
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness issues in the parallel commitment pipeline by (1) deferring selection of parallel/streaming tries until a DB-backed TrieContextFactory is available, (2) pinning the aggregator visible-file generation so parallel worker reads stay generation-consistent with the main commitment transaction, and (3) adding an env-driven toggle (COMMITMENT_PARALLEL, default off) for enabling parallel commitment without changing default behavior on main.
Changes:
- Add a “pending variant” mechanism so contexts start on the sequential trie and upgrade to parallel/streaming only when
EnableParaTrieDBwires the DB. - Introduce a refcounted file-generation pin (
AggregatorRoTx.Pin→AggregatorFilesPin,kv.TemporalFilesPin) and use it to open parallel worker temporal txns on the same snapshot as the main commitment tx. - Initialize
statecfg.ExperimentalParallelCommitmentfromdbg.EnvBool("COMMITMENT_PARALLEL", false)and ensure CLI flag defaults don’t overwrite the env-derived value.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| execution/commitment/parallel_patricia_hashed.go | Adds AdoptRootTrie to carry restored state across sequential→parallel trie upgrade. |
| execution/commitment/commitmentdb/commitment_context.go | Defers parallel/streaming trie selection until DB is available; pins file-generation for worker reads; adds worker-tx helper. |
| db/state/statecfg/state_schema.go | Reads COMMITMENT_PARALLEL env var to initialize ExperimentalParallelCommitment (default false). |
| db/state/aggregator.go | Adds AggregatorFilesPin and pin-based BeginFilesRo spawning to keep reads on a consistent visible generation. |
| db/state/aggregator_visible_from_test.go | Adds a test asserting pinned worker reads stay on the source visible generation. |
| db/kv/temporal/kv_temporal.go | Implements Pin() kv.TemporalFilesPin for temporal txns, enabling pinned worker BeginTemporalRo. |
| db/kv/membatchwithdb/memory_mutation.go | Forwards optional pinning capability through overlay tx wrappers. |
| db/kv/kv_interface.go | Introduces the TemporalFilesPin interface (optional capability via type assertion). |
| cmd/integration/commands/flags.go | Uses env-derived defaults when registering experimental commitment flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Pin forwards the files-pin capability to the underlying tx so that parallel | ||
| // commitment workers reading through an overlay view still pin the same file | ||
| // generation as the main read — domain (Accounts/Code/Storage) reads fall | ||
| // through the overlay to the underlying tx, so pinning it keeps worker reads | ||
| // consistent. Returns nil when the underlying tx can't pin files, letting the | ||
| // caller fall back to an independent snapshot. | ||
| func (m *MemoryMutation) Pin() kv.TemporalFilesPin { | ||
| if p, ok := m.db.(interface{ Pin() kv.TemporalFilesPin }); ok { | ||
| return p.Pin() | ||
| } | ||
| return nil | ||
| } |
…el-commitment worker reads Parallel-commitment workers each opened a fresh BeginTemporalRo, pinning whatever aggregator visible-file generation was current — possibly newer than the main commitment tx. A deleted/reincarnated account then read AccountsDomain=empty (from the in-memory overlay) but CodeDomain=stale (from the worker's newer file view), failing the ERIGON_ASSERT code-hash consistency check and wedging the node. Serial commitment reads through one tx and stayed consistent, so this only surfaced once CI began exercising the parallel-commitment axis. Add a files-pin API (AggregatorRoTx.Pin -> AggregatorFilesPin, kv.TemporalFilesPin): ComputeCommitment pins the main tx's file generation and opens worker read txns from it, so all concurrent workers observe the snapshot the in-memory overlay was built against. Forwarded through the block-overlay wrapper for the builder path; falls back (with a warning) for backends that can't pin files. Worker txns read only state domains via aggtx, so they need just the pinned file snapshot (no forkaggs). The pin is restricted to the ParallelPatriciaHashed fold factory (which computes the root and had the torn Accounts/Code read); serial/streaming warmup keeps its independent snapshot, so the serial path is behaviorally identical to before the pin.
state_schema.go reads COMMITMENT_PARALLEL via dbg.EnvBool (envLookup auto-prepends ERIGON_), mirroring EXEC3_PARALLEL, so parallel commitment can be toggled without a code change. Defaults false — behaviour is unchanged unless the env var (or the existing CLI flag) turns it on. The integration command's BoolVars now default to the env-derived value instead of clobbering it to the flag default at registration.
6db3f5a to
80033cc
Compare
…omments MemoryMutation.Pin read m.db without the RLock its documented mu contract requires (UpdateTxn writes m.db under Lock) — a latent race. Take the read lock. Also correct comments the review surfaced: the warmup default branch is serial-only (streaming is a *ParallelPatriciaHashed and takes the pinned branch above), the RootTrie doc had drifted onto AdoptRootTrie leaving RootTrie undocumented, and the pin test named a non-existent BeginFilesRoFrom.
| // AdoptRootTrie replaces the template with a trie that already carries state | ||
| // (e.g. restored before the variant upgrade); the previous trie must not be used after. | ||
| func (p *ParallelPatriciaHashed) AdoptRootTrie(root *HexPatriciaHashed) { | ||
| p.template = root | ||
| } |
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
Reviewed at 5f51209. Ran the new files-pin test plus ERIGON_COMMITMENT_PARALLEL=true runs of commitmentdb/execctx at the PR head — all pass. Findings, none blocking:
-
Merge-main follow-up: #22353 (landed after this branched) added
tx.blocktx = db.beginBlockFilesRo()toDB.BeginTemporalRo.temporalFilesPin.BeginTemporalRohand-duplicates the tx construction and will merge cleanly without it — harmless whileblockFilesis dormant, but pin-created worker txns will silently lack the block-files view once it's enabled. When merging main, mirror the assignment (or factor a shared tx constructor so future fields can't drift). -
Latent lifetime hazard: the factory stored via
trie.SetTrieContextFactorycapturesworkerPin, which the deferredClose()releases whenComputeCommitmentreturns; any later invocation of the stored factory nil-derefs inAggregatorFilesPin.BeginFilesRo(p.v.refcnt.Add(1)on nil). Unreachable today — the factory only runs insideProcess, andStreamingCommitter.StartSchedulerhas no production callers — but streaming's background eager folds will hit it. Cheap hardening:defer trie.SetTrieContextFactory(nil)inComputeCommitment(turns future misuse into the existing loud "requires a TrieContextFactory" error), or nil-guard the closed pin with a clear panic message. -
Test gap: the deferred-selection fix has a dedicated test for the wired path (
TestSharedDomains_ParallelFlag_RootEquivalence) but none for the unwired consumer contract that actually broke (parallel flag on, noEnableParaTrieDB→ stays sequential, commitment succeeds). I verified it manually at the PR head; a small test next to the equivalence test would pin it. -
Minor: the anonymous
interface{ Pin() kv.TemporalFilesPin }is spelled out twice (memory_mutation.goandfilesPinnerincommitment_context.go) — a named optional-capability interface inkvnext toTemporalFilesPinwould keep the contract in one place. Alsoaggregator_visible_from_test.gowould read better asaggregator_files_pin_test.goto match its content.
|
my version didnt passed ci though #22360 |
The base branch regenerates the {0,1} step while src still holds those files
open. POSIX allows rename-over-open-file so Linux and macOS pass, but Windows
denies it and the test fails in both matrix legs:
renaming: rename v1.0-accounts.0-1.kv.<n>.tmp v1.0-accounts.0-1.kv:
Access is denied.
Only add the {1,2} step: publishing a newer generation is all the test needs,
and the pin assertions are unchanged.
This is the same fix main already carries via #22354 and belongs to the base
branch (#22141), not here; it is ported so this PR's own CI is reviewable.
What
Splits the parallel-commitment Go changes out of #22141 so they land in
mainindependently, leaving that PR as a CI-only change. Three commits:
defer parallel trie selection to
EnableParaTrieDB— with parallelcommitment enabled, every
SharedDomainsselected the parallel trie, but onlyDB-backed consumers (exec, builder, squeeze, backtester) wire the per-worker
TrieContextFactoryit needs; integrity checks, RPC-created domains, and testharnesses panicked with
ParallelPatriciaHashed.Process requires a TrieContextFactory. The context now starts on the sequential trie and upgradeswhen
EnableParaTrieDBprovides the DB.pin file generation for parallel-commitment worker reads — workers each
opened a fresh
BeginTemporalRo, pinning a possibly-newer aggregator visible-filegeneration than the main commitment tx. A deleted/reincarnated account then read
AccountsDomain=empty(in-memory overlay) butCodeDomain=stale(worker's newerfile view), failing the
ERIGON_ASSERTcode-hash consistency check and wedging thenode. Adds a files-pin API (
AggregatorRoTx.Pin→AggregatorFilesPin,kv.TemporalFilesPin);ComputeCommitmentpins the main tx's generation and opensworker txns from it. The pin is restricted to the
ParallelPatriciaHashedfoldfactory (which computes the root); serial/streaming warmup keeps its independent
snapshot, so the serial path is behaviorally identical.
add
COMMITMENT_PARALLELenv toggle —state_schema.goreadsCOMMITMENT_PARALLELviadbg.EnvBool(mirrorsEXEC3_PARALLEL). Defaultsfalse — behaviour on
mainis unchanged unless the env var / CLI flag turns iton. The integration command's
BoolVars now default to the env-derived valueinstead of clobbering it at registration.
Why
The parallel-commitment CI axis added in #22141 exposed the two correctness bugs
above. They belong in
mainon their own so #22141 stays a pure CI change; once thismerges, #22141 re-merges
mainand the Go drops out of its diff.Testing
make erigon integration— builds.make lint— 0 issues.go test ./db/state/ -run 'TestAggregator|VisibleFrom|FilesPin|Pin'— pass (incl.the new
aggregator_visible_from_test.go).ERIGON_COMMITMENT_PARALLEL=true go test ./execution/commitment/... ./execution/execmodule/... ./db/state/— pass (exercises the DB-backed parallel-commitment path that previously panicked/wedged).