[awskii]

--experimental.parallel-commitment and COMMITMENT_PARALLEL now default to on: a node with neither set computes commitment on ParallelPatriciaHashed. Closes #21137.

Flipping the two defaults is not enough on its own. The CLI flag was one-way (if ctx.Bool(...) { cfg.X = true }) and integration's binding merged both defaults with ||, so a true default would have left =false and the env var inert. Resolution is now: the env var seeds statecfg.ExperimentalParallelCommitment, an explicit flag overrides it either way, and statecfg carries the effective value. statecfg.DefaultParallelCommitment is the single literal behind both the env fallback and the flag's advertised default, so the two cannot drift.

The flip also makes checkParaTrieWired reachable for any NewSharedDomains that selects the parallel variant with no DB wired — under ERIGON_ASSERT that path returns an error rather than warning and falling back. All 46 non-test call sites audited, and they fall into three groups, not two: wired (execV3/execV3Serial call EnableParaTrieDB centrally, covering the cmd/integration and execmodule clusters), opted out (WithSequentialCommitment), and never reaching ComputeCommitment at all (execmodule only flushes the BlockOverlay; InitPraguePreDeploys and the vm/runtime helpers compute no root). That last group is safe by unreachability, not by opting out — do not read it as an invariant the guard depends on. One site was genuinely exposed, db/integrity/commitment_integrity.go:1222, fixed here.

node/ethconfig.Config.ExperimentalParallelCommitment is gone. It had one writer and two readers and could only ever disagree with statecfg; statecfg.ExperimentalParallelCommitment is now the single source of truth, which removes the || merge at cmd/utils/flags.go and the one-way set in node/eth/backend.go.

CI arms are unaffected — every commitment_mode matrix leg already sets ERIGON_COMMITMENT_PARALLEL explicitly, so both stay pinned regardless of the default.

Changes

• db/state/statecfg: DefaultParallelCommitment = true behind EnvBool("COMMITMENT_PARALLEL", …); cmd/utils takes the flag's Value from the same const
• cmd/utils: setParallelCommitment replaces the one-way set and honours an explicit =false
• node/ethconfig, node/eth: drop the duplicate ExperimentalParallelCommitment field and the one-way sync it fed
• cmd/integration: withExperimentalCommitment defaults to statecfg; the previous || clobbered an env false once the flag default became true
• db/integrity: the per-block history check passes WithSequentialCommitment(), matching its two siblings at :1140 and :1208
• tests: the default, const-vs-flag drift, and both override directions in each binary
• docs: design doc, the operator flag reference, and both generated llms-full.txt copies

[Copilot]

🔵 Needs a closer look

It changes the default for a consensus-critical state-root/commitment computation path, so it warrants final human review despite the configuration-resolution tests.

Pull request overview

Enables parallel commitment (ParallelPatriciaHashed) by default across Erigon and the integration binary, and fixes the previous configuration-resolution pitfalls so COMMITMENT_PARALLEL can seed the default while an explicit CLI flag can override it in either direction.

Changes:

• Flip defaults to “on” for both COMMITMENT_PARALLEL (via statecfg) and --experimental.parallel-commitment (CLI flag).
• Add explicit-resolution logic so --experimental.parallel-commitment=false and COMMITMENT_PARALLEL=false reliably select the sequential trie.
• Add tests covering default + both override directions for both cmd/utils (urfave/cli) and cmd/integration (cobra).

File summaries

File	Description
docs/design/parallel-patricia-hashed.md	Updates design doc tables to reflect “default on” and how to force sequential mode.
db/state/statecfg/state_schema.go	Changes env default for ExperimentalParallelCommitment to true and updates its comment accordingly.
cmd/utils/flags.go	Sets CLI default to true and introduces setParallelCommitment to honor explicit =false overrides.
cmd/utils/flags_test.go	Adds unit test validating default + override behavior for setParallelCommitment.
cmd/integration/commands/flags.go	Aligns integration’s default to statecfg (env-seeded) rather than OR-ing with the CLI default.
cmd/integration/commands/dump_state_test.go	Adds test ensuring integration default follows statecfg and explicit flags override both ways.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[AskAlexSharov]

Review

The resolution chain in setParallelCommitment + withExperimentalCommitment is right, and both tests cover the default plus both override directions per binary. Four things.

1. "CI arms are unaffected" holds for the commitment_mode matrix, not for the QA workflows — and those run with ERIGON_ASSERT.
checkParaTrieWired is a no-op today because pendingVariant is only set when the parallel variant is selected. Flipping the default arms it on every production NewSharedDomains that passes neither WithParaTrieDB nor WithSequentialCommitment, and under dbg.AssertEnabled it returns an error, not the warning.

Nine workflows set ERIGON_ASSERT: true; eight of them set no ERIGON_COMMITMENT_PARALLEL at all, so they inherit the new default:
qa-sync-from-scratch, qa-sync-from-scratch-full-node, qa-sync-from-scratch-minimal-node, qa-sync-with-externalcl, qa-exec-from-zero, qa-rpc-integration-tests, qa-rpc-integration-tests-remote, qa-rpc-integration-tests-gnosis. (qa-rpc-integration-tests-latest is the only one that pins it.)

Sites that construct without either option and do not EnableParaTrieDB at the construction site: execmodule/forkchoice.go:414,613, set_head.go:106, inserters.go:81, exec_module.go:577, executor.go:234,277, cmd/integration/commands/{stages,state_stages,state_domains,commitment}.go, vm/runtime/runtime.go:134,198, blockgen/chain_makers.go:366, engineapi/testing_api.go:100. Several are wired later inside exec3.go:193,313; I could not confirm all of them are. Worth one ERIGON_ASSERT=true QA leg on this branch before merging, or an audit of that list — this is the part of the flip that is not covered by the matrix legs.

2. node/eth/backend.go:302 is the same one-way set this PR just removed from SetEthConfig.

if config.ExperimentalParallelCommitment {
    statecfg.ExperimentalParallelCommitment = true
}

With the default true, an embedder that builds an ethconfig.Config directly and sets ExperimentalParallelCommitment = false — devnet, execmoduletester, any in-process node — leaves statecfg at true and silently runs the parallel trie. That is the last hop of the resolution chain the description lays out, and it still clobbers =false. Assign unconditionally.

3. cmd/utils/flags.go:2043 still merges with ||.

if cfg.ExperimentalParallelCommitment || statecfg.ExperimentalParallelCommitment {

It runs after setParallelCommitment, which makes the two equal by construction, so the || is dead — and it is the same pattern removed from withExperimentalCommitment in this PR. Drop one side.

4. Nit. The flag is on by default and its usage no longer says EXPERIMENTAL, but the name is still --experimental.parallel-commitment and it is still filed under the experimental block in default_flags.go. Keeping the name is the right call for compatibility — worth one line in the docs page saying so, since the stability row now reads "Default on".

[Copilot]

🔵 Needs a closer look

It changes the default state-root/commitment computation path, which is correctness-critical and warrants final human review despite the added tests.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Lite

[AskAlexSharov]

Review

The checkParaTrieWired audit lands, and DefaultParallelCommitment behind both the env fallback and the flag's Value closes the drift. Verified while re-reading: envLookup("ERIGON_COMMITMENT_PARALLEL") hits the prefixed branch first, so the rename works and the double-prefix fallback is never reachable; blockgen's ComputeCommitment at chain_makers.go:605 belongs to GenerateChain's wired domains (:429/:434), not the unwired InitPraguePreDeploys.

1. The env rename is a silent break that reverses the operator's intent, and it is bundled with the flip.
COMMITMENT_PARALLEL=false worked before — envLookup tries the bare name first. It now does nothing, and the thing it used to disable is now the default. So the one setting whose whole purpose is "do not use the parallel trie" silently selects the parallel trie. .claude/skills/benchmarkoor/SKILL.md:399 still documents COMMITMENT_PARALLEL=true, which is the in-repo proof that the old spelling is in use.

Either keep the unprefixed name accepted for a release, or fail fast at startup when COMMITMENT_PARALLEL is set and ERIGON_COMMITMENT_PARALLEL is not. Failing that, it belongs in its own PR: a default flip and a knob rename land two independent behaviour changes at once, and this is the combination where they interact badly.

2. node/eth/backend.go:302 is still the one-way set this PR removes from SetEthConfig.

if config.ExperimentalParallelCommitment {
    statecfg.ExperimentalParallelCommitment = true
}

With the default true, an embedder that builds an ethconfig.Config directly and sets ExperimentalParallelCommitment = false — devnet, execmoduletester, any in-process node — leaves statecfg at true and silently runs the parallel trie. It is the last hop of the resolution chain the description lays out, and it still clobbers =false. Assign unconditionally.

3. cmd/utils/flags.go:2048 still merges with ||.

if cfg.ExperimentalParallelCommitment || statecfg.ExperimentalParallelCommitment {

It runs after setParallelCommitment, which makes the two equal by construction, so the || is dead — and it is the same pattern this PR removes from withExperimentalCommitment. Drop one side.

4. The audit claim in the description is stronger than the code.
"every other site opts out explicitly, except db/integrity/commitment_integrity.go:1222" — but execmodule/forkchoice.go:414,613, set_head.go:106, inserters.go:81, exec_module.go:577, executor.go:234,277, vm/runtime/runtime.go:134,198 and blockgen/chain_makers.go:366 construct with no option and never call EnableParaTrieDB. They are safe because they never reach computeCommitment — execmodule only flushes the BlockOverlay, and InitPraguePreDeploys and the runtime helpers compute no root — not because they opt out. Worth restating that way, so the next reader does not treat "every site opts out explicitly" as an invariant the guard depends on.

5. Nit. # COMMITMENT_PARALLEL is deliberately not accepted. is copy-pasted into six workflow files, and those six edits are comment-only — the env name was already prefixed. The rationale already lives at state_schema.go:203; one home is enough.

[awskii]

2 and 3 — done, but not the way point 2 asks. Assigning unconditionally would have introduced a bug: findReferences on the field gives 7 references — the declaration, one writer (setParallelCommitment), two readers (flags.go:2045, backend.go:302), three in tests. Nothing else in the repo ever sets it, engine_api_tester.go:357 included. So statecfg.X = config.X at backend.go:302 would force statecfg to false for every caller that builds a config and leaves the field at its zero value — the engine-API tester today, devnet the moment it stops going through SetEthConfig.

The field had one writer and two readers and could only ever disagree with statecfg, so it is deleted instead: node/ethconfig.Config.ExperimentalParallelCommitment, the one-way set at backend.go:302, and the dead || at flags.go:2045 all go together. statecfg is the only source of truth, and the last hop you describe no longer exists rather than being fixed.

4 — corrected, thank you. The description now says three groups, not two: wired centrally by execV3/execV3Serial, opted out via WithSequentialCommitment, and never reaching ComputeCommitment at all. It says explicitly that the third group is safe by unreachability and must not be read as an invariant the guard depends on.

5 — already gone, deleted outright in 68425a82e29 rather than shortened; that landed a few minutes before your review.

1 — open, and the one I am not deciding unilaterally. The break is real: COMMITMENT_PARALLEL=false used to mean sequential and now silently means parallel, which is the worst direction for it to fail. Of your three options, keeping the bare name accepted and splitting the PR both contradict the explicit ask this change came from (one name for one knob), so the only one that satisfies both is failing fast at startup when COMMITMENT_PARALLEL is set and ERIGON_COMMITMENT_PARALLEL is not. That refuses to start a node carrying a stale env var, which is a deliberate outward-facing behaviour change, so I want it confirmed before I add it. Holding here.

[awskii]

1 — you were right, and the rename is reverted in 4be432c5b47.

statecfg is back to dbg.EnvBool("COMMITMENT_PARALLEL", DefaultParallelCommitment), so envLookup accepts both spellings again and COMMITMENT_PARALLEL=false selects the sequential trie exactly as it did before. ERIGON_COMMITMENT_PARALLEL=false keeps working through the prefix branch, so the CI matrix legs are untouched either way. The env-name test went with it, and the docs are back to the unprefixed spelling that .claude/skills/benchmarkoor/SKILL.md:399 documents.

That leaves this PR doing one thing: flipping the default and making the flag resolve in both directions. The rename was a second behaviour change riding along, and the direction it failed in — the one setting whose purpose is "not the parallel trie" silently selecting it — is not one to ship on the same commit as the flip.

[Copilot]

🔵 Needs a closer look

Canonical environment-variable precedence and related documentation must be corrected before approval.

Review details

Suppressed comments (5)

db/state/statecfg/state_schema.go:201

• Passing the unprefixed name makes envLookup check COMMITMENT_PARALLEL before ERIGON_COMMITMENT_PARALLEL. If both exist, a stale unprefixed value wins, contrary to the intended prefixed-only setting. Pass the canonical name directly so the legacy alias cannot override it.

var ExperimentalParallelCommitment = dbg.EnvBool("COMMITMENT_PARALLEL", DefaultParallelCommitment)

docs/design/parallel-patricia-hashed.md:271

• The design table should use the canonical ERIGON_COMMITMENT_PARALLEL name; the unprefixed spelling emits a warning and can conflict with the setting used by CI and tooling.

| `--experimental.parallel-commitment` | on | selects `VariantParallelHexPatricia` (`execctx.PickTrieVariant`); `=false`, or `COMMITMENT_PARALLEL=false`, selects `VariantHexPatriciaTrie` |

docs/site/docs/fundamentals/configuring-erigon.mdx:510

• This newly recommends the non-canonical environment variable, which emits an envLookup warning and conflicts with the ERIGON_COMMITMENT_PARALLEL name used by workflows and tooling. Document the canonical name instead.

* `--experimental.parallel-commitment` *(New in v3.6)*: Compute the commitment trie with the fully parallel `ParallelPatriciaHashed` implementation. Pass `--experimental.parallel-commitment=false`, or set `COMMITMENT_PARALLEL=false`, to fall back to the sequential `HexPatriciaHashed` trie.

docs/site/static/llms-full.txt:2715

• Keep this generated reference aligned with the canonical ERIGON_COMMITMENT_PARALLEL setting; the unprefixed form emits a warning.

* `--experimental.parallel-commitment` *(New in v3.6)*: Compute the commitment trie with the fully parallel `ParallelPatriciaHashed` implementation. Pass `--experimental.parallel-commitment=false`, or set `COMMITMENT_PARALLEL=false`, to fall back to the sequential `HexPatriciaHashed` trie.

llms-full.txt:2715

• Keep this generated reference aligned with the canonical ERIGON_COMMITMENT_PARALLEL setting; the unprefixed form emits a warning.

* `--experimental.parallel-commitment` *(New in v3.6)*: Compute the commitment trie with the fully parallel `ParallelPatriciaHashed` implementation. Pass `--experimental.parallel-commitment=false`, or set `COMMITMENT_PARALLEL=false`, to fall back to the sequential `HexPatriciaHashed` trie.

• Files reviewed: 18/18 changed files
• Comments generated: 0 new
• Review effort level: Balanced