debug_executionWitness: legacy + canonical conformance#21629
Merged
Conversation
… queries (Task 8) Full corpus run: 15425 PASS / 7107 FAIL subtests across 356/2871 fixture files. Suite is red by design; failures recorded for human triage, not muted. Runner fix found during the run: invalid-block fixtures (expectException) carry an executionWitness but no canonical blocknumber, so debug_executionWitness can't be queried for them. Capture expectException per block and skip the witness query for rejected blocks (RunWithTester already asserts rejection). This removed 972 false "no parseable block number" failures. Dominant remaining signal: debug_executionWitness returns a strictly smaller witness (state/codes/headers) than EEST's canonical stateless witness across nearly every feature - one-directional shortfall, likely a single root cause in the witness builder. Plus EIP-7702 stateless re-exec rejecting delegated EOA senders. Recorded under Task 8 results for triage.
…ing, add witness parser unit test - tools/test-groups: document the greedy-partition ordering invariant (subset groups must precede broader ones or resolve empty) - execution/tests/testutil: add fixture-independent unit test for the WitnessBlockTest parser accessors (NumBlocks, ExpectedWitnessForBlock, BlockNumberForBlock, BlockExpectsException), covering them when the downloaded corpus is absent - test-all-erigon-race.yml: trim over-verbose cache comment per comment policy
…ness # Conflicts: # execution/tests/testutil/block_test_util.go
main (#21532) changed ExecutionWitnessResult.Headers to []hexutil.Bytes (RLP-encoded). Update rpcHeaderHashes to RLP-decode like expectedHeaderHashes instead of reading a map["hash"] field.
zkevm@v0.4.0 charges EIP-8037 AUTH_BASE on 7702 clears of an undelegated authority; current spec and tests-bal@v7.2.0 refund it. SkipLoad the 21 affected for_amsterdam subtests until the corpus is bumped. Tracking: #21563.
…fixtures The witness_validation_* fixtures store deliberately mutated executionWitnesses (stateless-verifier negative tests), so producer comparison always diverges - erigon's produced witness is the canonical one. SkipLoad the 22 affected files until a stateless-verify consumer mode exists. Tracking: #21566.
…rom execution witness debug_executionWitness computes the post-state commitment by replaying a block's updates in key-sorted order. Deleting a key that reduces a branch to a single child collapses it, and the collapse tracer touches the surviving sibling into the witness. When a later insert in the same block refills that branch, its final form has >=2 children and the sibling is not part of the canonical witness, so the witness carried one extra node. Carry the collapsing branch's prefix through CollapseTracer. After the post-state commitment, read each candidate branch from the in-memory commitment domain and drop the sibling when the branch ended with >=2 children (transient collapse); keep siblings whose branch genuinely collapsed.
… filter - defer SetCollapseTracer(nil) so the tracer is cleared on the error and root-mismatch paths, not only on success - clear collapseTracer in resetForReuse() so a pooled trie cannot retain a stale closure - preallocate siblingPaths - reword the filter comment: it restores canonical witness membership, it does not reproduce the spec's insert-before-delete replay order
…ent-collapse-filter
Add ERIGON_WITNESS_MODE (default legacy) selecting the debug_executionWitness output format; canonical preserves the minimized format. Legacy codes emit every bytecode loaded during execution (accessed plus in-block-created) and a single empty bytecode when an empty-code account is loaded.
…, to be reverted)
rpc/jsonrpc: update TestExecutionWitness to expect populated Keys (20B address / 32B slot preimages), replacing the stale Geth-compat nil assertion now that legacy mode emits keys in both modes. execution/tests: pin the zkevm witness corpus to canonical mode via t.Setenv so CI compares against the minimized canonical fixtures regardless of the legacy deployment default.
taratorio
reviewed
Jun 5, 2026
taratorio
reviewed
Jun 5, 2026
taratorio
requested changes
Jun 5, 2026
Per PR #21629 review (the #21092 pattern): the ~22k-test zkevm execution-witness conformance suite must not run under `go test` / `make test-all`. Replace it with a `zkevmtest` cmd/evm subcommand executed as parallel `eest-spec-tests` shards (incl. a -race variant). - cmd/evm/zkevmrunner.go: new `zkevmtest` subcommand — imports each EEST zkevm blockchain_tests fixture, queries debug_executionWitness (canonical) per block, multiset-compares state/codes/headers. Expected-fail patterns for the known #21563/#21566 divergences are ported via TestMatcher so the shard budget stays 0. - testutil: add BlockTest.RunWithTesterCLI (nil tb, caller-owned tester) and TestMatcher.CheckFailureForName for CLI runners; RunCLI now shares the path. - shards: add zkevm-witness + zkevm-witness-race-all to eest-spec-shards.yml, route them in run-eest-spec-test.sh, cache the eest_zkevm tarball in test-eest-spec.yml, and add Makefile targets provisioning test-fixtures-zkevm. - remove the go-test suite (execution/tests/eest_zkevm_witness), its execution-eest-zkevm test-group, the zkevm steps in test-all-erigon-race.yml, and the GOTEST_PACKAGES filter (back to ./...).
taratorio
reviewed
Jun 5, 2026
taratorio
approved these changes
Jun 5, 2026
…-race) Address PR #21629 review: rename zkevm-witness-race-all -> zkevm-witness-race (the -all suffix was redundant) and drop the separate non-race zkevm-witness shard. The race shard already runs the full witness comparison over the whole corpus, so the non-race shard added no coverage — only faster feedback — and the original suite was race-only. Race-shard detection now keys on "-race" (matching the bare suffix) instead of "-race-".
Member
|
from claude review Review: PR #21629 —
|
taratorio
approved these changes
Jun 5, 2026
- WitnessKeys: dedup via map[common.Address]/map[common.Hash] + bytes.Clone instead of round-tripping every key through string(); 20B address and 32B slot keys can't collide, so the typed sets reproduce the same dedup with fewer allocations (behavior unchanged; TestCollectAccessedState_*). - TestResolveWitnessMode: drop the stale "canonical env" wording — mode is the request param only (the env was removed in this PR). - witness-legacy-mode-spec.md: note the EIP-7928 system address is also kept when accessed by a user transaction, not only on a real state change.
…ad loop From the PR review: WITNESS_STRICT_VERIFY was checked via os.Getenv(...) != "" on every account/storage read during stateless verification, so WITNESS_STRICT_VERIFY=false would enable strict mode. Parse it once with dbg.EnvBool into witnessStateless.strictVerify (production behavior with the env unset is unchanged). Also sharpen the empty-code comment to note that only pre-state empty-code loads emit the empty entry.
Member
Author
|
Thanks — went through the claude-review findings:
|
This was
linked to
issues
Jun 5, 2026
Member
Author
|
Ran a continuous legacy-mode The divergences are benign minimization differences — erigon's witness self-verifies in every one. Breakdown:
Root cause is a created-flag asymmetry. The IBS sets Caveat for anyone using No correctness fix needed. If exact key-set parity with reth-legacy is wanted, the one safe change is to also minimize 7702-EOA empty zero-reads (mirror the CREATE2 path) — removes the keys-over noise, keeps self-verify valid. Can open a follow-up PR if useful. |
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jun 6, 2026
…h#21640) ## Problem `tools/run-eest-spec-test.sh` invokes the cmd/evm runner with `|| true` and decides shard pass/fail from the `--jsonout` report alone. That design is correct for the non-race shards (the runners always exit 0 and report via JSON; crashes surface as missing/truncated JSON, which the script catches via the `total == 0` check and `set -euo pipefail` on jq). It is wrong for the `*-race-*` shards: the Go race runtime reports detected races to **stderr** and overrides the process exit code to **66** (even through `os.Exit(0)`), while the run completes normally and the JSON stays fully valid with every test passing. The `|| true` discards that exit code, so a race shard whose binary detected a data race still went green — the `WARNING: DATA RACE` report only visible in the job log. The race detector's whole value is catching races that *don't* happen to corrupt results, so this silently weakened the guarantee of every race shard: ``` blocktests-stable-race-{pre-cancun,cancun,prague,osaka}-{sequential,parallel} blocktests-devnet-race-amsterdam ``` (and `zkevm-witness-race-all` once erigontech#21629 lands). Pre-existing since the shards were introduced in erigontech#21092; surfaced while reviewing erigontech#21629, which doubles the corpus running under the race detector. ## Fix Capture the runner's exit code and fail the shard explicitly on 66 with a pointer to the race report above it in the log. All other exit-code tolerance is unchanged. ## Verification (local repro) Built a minimal `-race` Go binary containing a genuine data race that emits valid passing JSON (`[{"name":"t1","pass":true}]`) and calls `os.Exit(0)`, then ran it through the real script as `EVM_BIN` against `blocktests-stable-race-cancun-sequential`: - **Before** (unmodified script): binary prints `Found 1 data race(s)` and exits 66 → script prints `ran 1 tests, 0 failed` and exits **0** (green despite the race). - **After**: same run exits **1** with `ERROR: data race detected (race runtime exit code 66); see WARNING: DATA RACE in the log above`. Regression matrix with stub binaries through the patched script, confirming only the race case changes behavior: | Scenario | Before | After | |---|---|---| | race detected (exit 66, valid passing JSON) | exit 0 | **exit 1** | | clean pass (exit 0) | exit 0 | exit 0 | | non-zero exit ≠ 66 with valid JSON | exit 0 (tolerated by design) | exit 0 | | test failures over budget | exit 1 | exit 1 | `shellcheck --severity=warning` (the lint-workflow invocation) is clean on the modified script; `make lint` clean. The race shards running on this PR exercise the patched path end-to-end in CI (race-free run → `rc=0` → behavior identical to before). ## Note on TDD This is a shell-script CI guard with no test harness; the Red → Green cycle above (reproduce the masked exit code through the unmodified script, then flip it with the fix) stands in for a checked-in failing test. ## End-to-end verification with a real race in erigon code Repeated the verification with the full production pipeline instead of stub binaries: planted a deliberate data race in `IntraBlockState.CommitBlock` (two unsynchronized goroutines incrementing a package var, executed once per imported block — never committed), built the real `evm.race` via `make evm.race`, and ran a single real EEST fixture (`cancun/eip1153_tstore/test_basic_tload_after_store.json`, one `fork_Cancun` test) through the `blocktests-stable-race-cancun-sequential` shard: - **old script** (origin/main): race report correctly attributes `intra_block_state.go` in `CommitBlock.func1`, binary prints `Found 1 data race(s)` and exits 66 → script prints `ran 1 tests, 0 failed`, **exit 0** — green despite the race. - **this PR's script**: same binary, same fixture → **exit 1**, last line `ERROR: data race detected (race runtime exit code 66); see WARNING: DATA RACE in the log above`. - **control**: race reverted, `evm.race` rebuilt clean, same fixture through this PR's script → 0 race warnings, `ran 1 tests, 0 failed`, **exit 0** — no false positive.
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.
Adds the zkEVM (canonical) conformance runner for
debug_executionWitness, the legacy-mode field population, the optionalmoderequest param, and a format spec.Changes
zkevmtestrunner — acmd/evmsubcommand (mirroringstaterunner/blockrunner/enginexrunner) that checksdebug_executionWitness(canonical mode) against theethereum/execution-spec-testszkevm@v0.4.0 corpus (~22.5k tests). Runs as thezkevm-witness-raceshard in theeest-spec-testsworkflow, not undergo test. Known divergences (zkevm@v0.4.0 fixtures: stale EIP-8037 AUTH_BASE gas on EIP-7702 undelegated clears #21563, zkevm@v0.4.0 eip8025_optional_proofs witness_validation_* are stateless-verifier negative tests, not producer outputs #21566) are absorbed in the runner so the shard budget stays 0.keys/codes—keysholds the 20B address / 32B storage-slot preimages;codesholds pre-state bytecode incl. EIP-7702 designators and the empty-code entry.moderequest param (legacy/canonical).docs/plans/witness-legacy-mode-spec.md, re-verified against code.#21487
Carries #21487's suite plus the fixes that make it pass; #21487 is redundant once CI is green here. The open question from #21487 — whether a fixture-gated suite belongs in the required race-gate — is resolved: it's now a
zkevmtestrunner shard in theeest-spec-testsworkflow (the #21092 pattern), not ago testpackage.Mainnet legacy soak vs reth-legacy
Rolling soak near tip (block ~
25.25M, ~3.7k blocks checked):verifyWitnessStateless.{keys, codes, state}.keys, 86codes, 62state) — a few extra nodes per block, still re-executing to the correct root. 3 blocks show minorkeys/codesunder-inclusion, none affectingverifyWitnessStateless.