ci, db/state: matrix-test serial vs parallel commitment across the test workflows - #22141
Conversation
|
@mh0lt need to also update test-eest-spec.yml shards |
|
Good catch — done in f86ceab. The EEST spec shards toggled execution via the
Shard names are unchanged, so the ci-gate check names and |
There was a problem hiding this comment.
Pull request overview
This PR repurposes the existing CI “serial vs parallel” matrix axis from execution mode (now parallel by default) to state commitment mode, so CI exercises both serial and parallel commitment paths via a new environment toggle.
Changes:
- Add
ERIGON_COMMITMENT_PARALLELenv plumbing by initializingstatecfg.ExperimentalParallelCommitmentfromdbg.EnvBool("COMMITMENT_PARALLEL", false). - Preserve env-derived commitment defaults through CLI flag registration (notably in the
integrationcommand). - Update CI workflows and EEST tooling to drive serial vs parallel commitment via the new matrix axis and env var.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
db/state/statecfg/state_schema.go |
Initializes ExperimentalParallelCommitment from env (COMMITMENT_PARALLEL via dbg.EnvBool). |
cmd/integration/commands/flags.go |
Uses env-derived defaults when registering experimental commitment flags so env isn’t clobbered. |
tools/run-eest-spec-test.sh |
Switches EEST shard runner to export ERIGON_COMMITMENT_PARALLEL from the shard manifest. |
tools/eest-spec-shards.yml |
Renames shard-manifest key to commitment-parallel to control commitment mode per shard. |
.github/workflows/test-all-erigon.yml |
Renames matrix axis to commitment_mode and drives ERIGON_COMMITMENT_PARALLEL for Go tests. |
.github/workflows/test-all-erigon-race.yml |
Same as above for race-detector test groups with mode-separated build cache keys. |
.github/workflows/test-bench.yml |
Runs benchmarks in both commitment modes via ERIGON_COMMITMENT_PARALLEL. |
.github/workflows/test-hive.yml |
Updates Hive matrix to commitment_mode and injects ERIGON_COMMITMENT_PARALLEL into the Erigon client Dockerfile. |
.github/workflows/test-hive-eest.yml |
Same commitment-mode matrix + Docker ENV injection for Hive EEST runs. |
.github/workflows/test-kurtosis-assertoor.yml |
Updates Kurtosis matrix axis and bakes ERIGON_COMMITMENT_PARALLEL into the runtime image per leg. |
.github/workflows/qa-stage-exec.yml |
Switches QA stage-exec matrix to commitment mode and disambiguates artifacts/datadirs accordingly. |
.github/workflows/qa-txpool-performance-test.yml |
Switches txpool perf matrix to commitment mode and bakes the env var into the runtime image. |
.github/workflows/qa-rpc-integration-tests-latest.yml |
Switches RPC integration tests to commitment-mode matrix and artifacts naming. |
.github/workflows/qa-rpc-performance-tests.yml |
Renames workflow_dispatch input and runtime env handling to ERIGON_COMMITMENT_PARALLEL. |
.github/workflows/qa-rpc-performance-comparison-tests.yml |
Same input/env handling update for the comparison workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@awskii fyi to check the |
|
Addressed the two shard-config comments in |
|
The remaining red legs here (eest blocktests/enginextests parallel, race-tests core-rpc/execution-other parallel) all fail on one class: after an unwind or on a fresh tiny chain, the restored commitment state is a sole-account root with no root branch record, and #22257 fixes this. Verified locally on this branch with it trial-merged: both repro tests pass, and the full So the order is: land #22257, then merge main into this branch again — the parallel legs should go green with no further changes here. |
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. (cherry picked from commit 1436a65)
…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, since the code delete leaves no mem tombstone when prevVal is nil), 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. (cherry picked from commit a8cb055)
The pin's worker tx copied BeginTemporalRo's forkaggs setup, which main removed; the branch+main merge build then failed on the now-undefined forkaggs. Commitment workers read only state domains via aggtx, so the worker tx needs just the pinned file snapshot. (cherry picked from commit bd0dbb0)
…rial warmup The pin was also applied to the warmup factory, which serves serial/streaming commitment's page-cache warmup. Warmup does not compute the root, and pinning it regressed serial commitment to a wrong trie root on mainnet. Restrict the pin to the ParallelPatriciaHashed fold factory (which computes the root and had the torn Accounts/Code read); serial warmup keeps its independent snapshot, so the serial path is behaviorally identical to before the pin change. (cherry picked from commit 995f74c)
…st workflows Repurpose the (now-redundant) serial/parallel exec-mode CI matrix to exercise serial vs parallel commitment instead: the exec_mode axis is renamed commitment_mode, ERIGON_EXEC3_PARALLEL assignments are dropped (exec is parallel in both legs by default), and ERIGON_COMMITMENT_PARALLEL is driven from the axis. state_schema.go reads COMMITMENT_PARALLEL via dbg.EnvBool; the integration BoolVar defaults to the env-derived value. EEST spec/devnet shards select the mode via their -parallel suffix. Squash of the three CI commits from #22141 (4d1d24e, f86ceab, c923faa), reconciled with main's consume-enginex migration (#22344) in qa-stage-exec.yml and test-hive-eest.yml, and rebased onto the extracted parallel-commitment fixes (#22360) so the CI axis no longer carries the code changes it exercises.
f8b1f7f to
b16d560
Compare
…+ add COMMITMENT_PARALLEL toggle (erigontech#22354) ## What Splits the parallel-commitment Go changes out of erigontech#22141 so they land in `main` independently, leaving that PR as a CI-only change. Three commits: 1. **defer parallel trie selection to `EnableParaTrieDB`** — with parallel commitment enabled, 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 panicked with *`ParallelPatriciaHashed.Process requires a TrieContextFactory`*. The context now starts on the sequential trie and upgrades when `EnableParaTrieDB` provides the DB. 2. **pin file generation for parallel-commitment worker reads** — workers each opened a fresh `BeginTemporalRo`, pinning a possibly-newer aggregator visible-file generation than the main commitment tx. A deleted/reincarnated account then read `AccountsDomain=empty` (in-memory overlay) but `CodeDomain=stale` (worker's newer file view), failing the `ERIGON_ASSERT` code-hash consistency check and wedging the node. Adds a files-pin API (`AggregatorRoTx.Pin` → `AggregatorFilesPin`, `kv.TemporalFilesPin`); `ComputeCommitment` pins the main tx's generation and opens worker txns from it. The pin is restricted to the `ParallelPatriciaHashed` fold factory (which computes the root); serial/streaming warmup keeps its independent snapshot, so the serial path is behaviorally identical. 3. **add `COMMITMENT_PARALLEL` env toggle** — `state_schema.go` reads `COMMITMENT_PARALLEL` via `dbg.EnvBool` (mirrors `EXEC3_PARALLEL`). **Defaults false** — behaviour on `main` is unchanged unless the env var / CLI flag turns it on. The integration command's `BoolVar`s now default to the env-derived value instead of clobbering it at registration. ## Why The parallel-commitment CI axis added in erigontech#22141 exposed the two correctness bugs above. They belong in `main` on their own so erigontech#22141 stays a pure CI change; once this merges, erigontech#22141 re-merges `main` and 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). --------- Co-authored-by: awskii <artem.tsskiy@gmail.com> Co-authored-by: awskii <awskii@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.github/workflows/qa-rpc-performance-comparison-tests.yml:210
- This step now drives commitment mode, but still uses the
EXEC_MODEvariable name and messages that refer to a generic "mode". Renaming toCOMMITMENT_MODE(and updating the echo text) will prevent confusion with execution-mode toggles elsewhere in CI.
env:
EXEC_MODE: ${{ github.event.inputs.commitment_mode }}
run: |
set +e # Disable exit on error
echo "Starting Erigon..."
.github/workflows/qa-rpc-performance-comparison-tests.yml:12
- The PR description says all RPC perf workflows were repurposed from exec-mode to commitment-mode, but
qa-rpc-performance-tests.ymlstill uses theexec_modeinput and togglesERIGON_EXEC3_PARALLEL. This leaves that workflow testing a different dimension than the rest and likely misses the intended serial-vs-parallel commitment coverage.
This issue also appears on line 206 of the same file.
commitment_mode:
description: 'Erigon commitment mode ("default" leaves ERIGON_COMMITMENT_PARALLEL unset)'
yperbasis
left a comment
There was a problem hiding this comment.
[P2] Give commitment modes distinct QA result identities
The RPC integration upload at
and the RPC performance upload at reuse result names whose serial/parallel suffix previously meant execution mode. The QA uploader stores no separate mode field, so historical queries and dashboards will combine incompatible configurations under the same series.Please use explicit commitment-serial and commitment-parallel identities for persisted QA results. The same naming pass should distinguish the two commitment legs uploaded by qa-stage-exec and qa-txpool-performance-test, which currently use one identical test name for both rows.
…-matrix # Conflicts: # tools/eest-spec-shards.yml # tools/run-eest-spec-test.sh
|
Pushed 6461c82 (merged current main + review items): @yperbasis [P2] Distinct QA result identities. All four persisted
@taratorio #22822 new shards. Merging main pulled in the new legacy blocktest shards; they came in on the old Copilot. Renamed the comparison workflow's |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
tools/run-eest-spec-test.sh:57
- The comment implies that any shard without a "-parallel" suffix runs with ERIGON_COMMITMENT_PARALLEL=false, but commitment mode is actually driven by the manifest’s
commitment-parallelkey (and some shards opt in without the suffix). This can mislead future shard additions/edits.
# *-parallel any of the above with "-parallel"
# appended runs with
# ERIGON_COMMITMENT_PARALLEL=true. Every
# other shard runs with
# ERIGON_COMMITMENT_PARALLEL=false so
tools/eest-spec-shards.yml:25
- The header says the -sequential/-parallel suffix selects serial vs parallel commitment, but there are shards (for example *-race entries) that set
commitment-parallel: truewithout a -parallel suffix. Clarifying that the manifest key is authoritative would avoid confusion.
# commitment-parallel (optional, default false) — pins ERIGON_COMMITMENT_PARALLEL.
# Execution is parallel in every shard
# (dbg.Exec3Parallel defaults true); the
# -sequential/-parallel suffix selects
# serial vs parallel commitment.
.github/workflows/test-kurtosis-assertoor.yml:343
- The comment says each suite runs twice, but the matrix includes
glamsterdamonly incommitment_mode: parallel. Either add the missing serial entry or update the comment to reflect the exception.
# Each suite runs twice — once with serial commitment
# (ERIGON_COMMITMENT_PARALLEL=false) and once with parallel — so
# divergence is caught on the PR. Execution is parallel in both legs
# (default). Matrix entries spawn separate hosted runners and run
# concurrently — wall-clock unchanged, runner-minutes doubled.
.github/workflows/test-hive-eest.yml:33
- The comment says each shard runs twice, but the matrix includes
glamsterdam-devnetonly incommitment_mode: parallel. Either add the missing serial entry or adjust the comment so it matches the matrix.
# Each shard runs twice — once with serial commitment
# (ERIGON_COMMITMENT_PARALLEL=false) and once with parallel — so
# divergence between the two paths is caught on the PR. Execution is
# parallel in both legs (default). Matrix entries spawn separate runners
# and run concurrently — wall-clock unchanged, runner-minutes doubled.
.github/workflows/qa-rpc-performance-comparison-tests.yml:489
- The Erigon log artifact suffix still uses
-parallel/-serial, which is ambiguous now that the axis is commitment mode (execution is always parallel). Aligning the suffix with the-commitment-*naming used elsewhere would make logs easier to interpret and avoid confusion with historical exec-mode naming.
name: erigon-logs-${{ env.CHAIN }}${{ (github.event.inputs.commitment_mode == 'parallel' && '-parallel') || (github.event.inputs.commitment_mode == 'serial' && '-serial') || '' }}
.github/workflows/qa-rpc-performance-comparison-tests.yml:12
- PR description says the exec_mode matrix axis was repurposed to commitment_mode across 11 test workflows, including
qa-rpc-performance-tests.yml, but that workflow still usesexec_mode/ ERIGON_EXEC3_PARALLEL (see.github/workflows/qa-rpc-performance-tests.yml:14and:225+). Either update that workflow too, or adjust the PR description/scope so coverage expectations match reality.
commitment_mode:
description: 'Erigon commitment mode ("default" leaves ERIGON_COMMITMENT_PARALLEL unset)'
…-matrix # Conflicts: # tools/eest-spec-shards.yml
…-matrix # Conflicts: # .github/workflows/test-hive-eest.yml # tools/eest-spec-shards.yml
…-matrix # Conflicts: # .github/workflows/test-hive-eest.yml # tools/eest-spec-shards.yml
|
Follow-up summary for the last three non-merge implementation commits:
Validation performed locally:
The complete stable, devnet, and legacy EEST fixture corpora were not run locally; CI will exercise those matrix legs. |
…-matrix # Conflicts: # .claude/skills/erigon-implement-eip/SKILL.md # .claude/skills/erigon-test-all/SKILL.md # .github/workflows/test-hive-eest.yml # tools/eest-spec-shards.yml # tools/run-eest-spec-test.sh
|
Merged current Adapted main's #22885 new devnet shards to the serial/parallel commitment convention:
One semantic call to sanity-check: the merge-base had Validated: no conflict markers, all touched YAML parses, |
Summary
Parallel execution (
dbg.Exec3Parallel) is now the default, so the serial/parallel exec-mode CI matrix added in #21017 no longer needs to toggle it — both legs already run parallel exec. This PR repurposes that matrix axis to exercise serial vs parallel commitment instead, so divergence betweenstatecfg.ExperimentalParallelCommitmenttrueandfalseis caught on the PR rather than after a release.Modeled directly on #21017 (the exec-mode matrix PR).
Mechanism
ERIGON_COMMITMENT_PARALLEL.db/state/statecfg/state_schema.gonow initializesExperimentalParallelCommitmentfromdbg.EnvBool("COMMITMENT_PARALLEL", false);envLookupincommon/dbg/dbg_env.goauto-prepends theERIGON_prefix, exactly likeEXEC3_PARALLEL.node/eth/backend.go), so the env value survives when the flag isn't passed;integrationcommand'sBoolVarnow defaults to the env-derived value instead of clobbering it back tofalseat flag registration.exec_modematrix axis is renamedcommitment_mode, theERIGON_EXEC3_PARALLELassignment is dropped (exec is parallel in both legs by default), andERIGON_COMMITMENT_PARALLELis driven from the axis. For the twoworkflow_dispatchperf tests the manualexec_modeinput becomescommitment_mode. DockerENVinjection for the hive/kurtosis/txpool runtime images is updated to the new var. Per-mode Go build-cache lineages and artifact/enclave/testbed names carry over unchanged in shape.Why
ExperimentalParallelCommitmentwas previously reachable only via a CLI flag, so no CI job exercised the parallel-commitment trie path. Reusing the (now redundant) exec-mode axis gives serial-vs-parallel commitment coverage at no extra matrix cost — same runner count, the axis just tests a different dimension.Note on coverage trade-off
This drops the dedicated serial-execution CI leg. That is deliberate: parallel execution is the default and is what ships; the serial-exec matrix leg was guarding a non-default path, whereas serial-vs-parallel commitment is the currently-diverging surface worth gating. Serial execution remains runnable locally via
ERIGON_EXEC3_PARALLEL=false.Workflows touched:
test-all-erigon,test-all-erigon-race,test-bench,test-hive,test-hive-eest,test-kurtosis-assertoor,qa-stage-exec,qa-txpool-performance-test,qa-rpc-integration-tests-latest,qa-rpc-performance-tests,qa-rpc-performance-comparison-tests.Latest main sync
The main sync includes #22895, which split the legacy Constantinople race shard into Byzantium and remaining-fork partitions. Both partitions now have sequential and parallel commitment variants. A structured audit confirms that all 14 logical legacy blocktest shards, race and non-race, have matching mode pairs and differ only in the commitment toggle.
Validation
make lint(two clean runs)make erigon integrationmake test-shortTDD was not used for the main-sync conflict resolution and shard-matrix update because these are mechanical CI configuration changes.