Repository navigation
feat(l1): unify guest programs and publish assets ourselves - #7115
Conversation
|
3c3b6f4af (devnet-7) |
this client (devnet-8) | |
|---|---|---|
COLD_STORAGE_ACCESS |
3000 | 2100 |
ACCOUNT_WRITE |
8000 | 9000 |
CALL_VALUE |
10300 | 11300 |
| access-list storage key | full cold cost | cold − WARM_ACCESS |
Five of the 45 failed even while the client was still on devnet-7, so those are
skew against 3c3b6f4af itself; the other 40 appeared with the devnet-8 move.
The two groups are commented separately in EXTRA_SKIPS because they lift at
different times.
Corroborating that these are spec-base skew and not client bugs: ethrex passes
the five original cases against the tests-zkevm@v0.6.2 versions of the same
tests, and the eth-act witness dashboard has ethrex at 22829/22829 on
eels/consume-engine-witness and 19413/19413 on stateless-validator sp1 for
that bundle. Diffing witness_codes_reset_delegation across the two fixture
sets: pre and genesisBlockHeader are byte-identical and it is the same
transaction hash, yet postState, receipts, stateRoot, receiptTrie and
blockAccessListHash all differ — the receipt's cumulativeGasUsed goes
30816 → 24653. Same input, different output.
The regular vectors/ bundle has no eip8025_optional_proofs tests, so
make test-levm never sees any of this.
The pin cannot move today. eip8025_optional_proofs does not exist on the
current forks/amsterdam default branch, and 3c3b6f4af has diverged from it
(124 ahead, 315 behind), so it was never merged there. devnets/glamsterdam/8
is 315 commits behind 3c3b6f4af: it has the devnet-8 gas schedule but not the
#3278 schema, so vectors filled from it would carry the pre-#3278 69-byte
statelessOutputBytes this client cannot parse. No upstream revision has both.
Removal: bump SPEC_SHA in scripts/gen_stateless_vectors.sh once an
upstream revision carries both eip8025_optional_proofs and the devnet-8 gas
schedule, then delete the 40-entry group. Re-check the original five at the same
time; if they still fail against a matching base they are real execution bugs
and must not stay skipped.
Why this file previously claimed an Amsterdam-wide skip
parse_and_execute used to drop every network >= Fork::Amsterdam fixture from
the stateless run. Every test in the tests-zkevm bundle it read is
network: Amsterdam (23,946 of 23,946), so the stateless suite executed nothing
at all while reporting success. parse_and_execute now fails any stateless
fixture file that runs zero tests without a named skip, so a structural skip
cannot silently empty the suite again.
ZisK guest program hash changes with the unsync_cell gate
Where: crates/common/types/block.rs, transaction.rs.
The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.
The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.
This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rs → unsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.
Release signing key is an unprotected repository secret
Where: .github/workflows/tag_release.yaml.
MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.
This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:
- Move
MINISIGN_SECRET_KEY/MINISIGN_PASSWORDinto a GitHub Environment with required
reviewers, and addenvironment:to the two jobs that sign. - Add a ruleset targeting
refs/tags/v*restricting who may create release tags.
Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
|
Too many files changed for review (103 files, 100 file limit). Bypass the limit by tagging |
🤖 Kimi Code Review
Looking at this substantial PR that consolidates stateless validation (EIP-8025) and adds signed artifact release workflows, here are my findings: Critical Security & Correctness1. Deposit Contract Address Hardcoding (crates/common/types/block_execution_witness.rs:1118-1120)The 2. Schema ID Validation (crates/guest-program/src/l1/input.rs:45-50)The
3. Public Key Validation (crates/guest-program/src/l1/program.rs:186-220)The CI/CD & Infrastructure4. Signing Script Robustness (.github/scripts/sign-stateless-artifacts.sh:39)printf '%s\n' "$MINISIGN_SECRET_KEY" > "$WORK_DIR/minisign.key"Issue: If printf '%s\n' "$MINISIGN_SECRET_KEY" > "$WORK_DIR/minisign.key"Actually, this is correct (uses 5. ERE Server Startup Timeout (.github/workflows/tag_release.yaml:284-306)The 900s timeout for OpenVM STARK keygen is generous, but monitor this: if ere-server exits before health checks begin, the 6. Artifact Pattern Matching (.github/workflows/tag_release.yaml:489)pattern: "*ethrex*"Good fix from Code Quality7. Progressive SSZ Implementation (crates/common/types/stateless_ssz.rs:193-218)The hand-written
The comment references 8. Unsafe Curve Operations (crates/guest-program/stateless-validator/src/crypto/openvm.rs:395-410)unsafe { bn::G1Affine::from_xy(px, py) }Safety: The 9. Feature Flag Cleanup (crates/blockchain/blockchain.rs:55-60)Good cleanup removing
Minor Issues10. Python Script Error Handling (.github/scripts/ere-execute.py:97-100)The protobuf varint decoder raises 11. Hardcoded Constants (crates/common/types/stateless_ssz.rs:371-372)pub const STATELESS_INPUT_SCHEMA_ID: u16 = 0x1501;Consider deriving this from fork metadata rather than magic number, though the comment explains this is Amsterdam (fork 0x15, revision 0x01). Testing Recommendations
SummaryThis is a well-structured consolidation PR. The main risks are:
The removal of RISC0 support (due to c-kzg version incompatibility) is noted and tracked. The addition of OpenVM support follows the established patterns for ZisK and SP1. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings:
No other major correctness or EVM-rule regressions stood out from the diff. I could not run the targeted Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Resolve four conflicts, all in the blockchain EF-test harness and its known-issues entry. main's #7094 added a fork-based `skip_stateless_amsterdam` in `parse_and_execute`, gated on the `Option<BackendType>` parameter, because the stateless run read the devnet-7-filled `vectors_zkevm/` bundle while the client had moved to devnet-8. This branch removes that parameter (there is no prover backend to dispatch on any more) and points the stateless run at the generated `vectors_stateless_3278/` set instead, so the skip has neither a mechanism nor a subject left. `test_runner.rs` and `tests/all.rs` therefore keep this branch's side, and the superseded known-issues section is dropped in favour of the entry describing the five named `EXTRA_SKIPS`. The two additions from main that are unrelated to the stateless harness are kept: the `rpc-compat log-bearing cases excluded` entry from #7142, and the `$(AMSTERDAM_ARTIFACT).part` cleanup from #7140, unioned with this branch's `$(STATELESS_VECTORS_ROOT)` and `.stateless-vector-work` removals.
…s path `BlockHeader::hash` is backed by `unsync_cell::OnceCell` under `all(feature = "zisk", target_arch = "riscv64")` and by `once_cell::sync::OnceCell` otherwise. Moving that gate off the removed `eip-8025` feature made the unsync side live for the first time — both ZisK guests enable `ethrex-common/zisk` — and it has no `take()`, which `into_with_burned_fees` calls, so `cargo build` for either ZisK guest fails with E0599. No PR job catches it: the nostd matrix checks `host`, `ere` and `zkvm-interface` on x86_64, where the cfg is false. Add `take()`; verified by relaxing the gate to the host and building every workspace lib and bin against the unsync cell. Alongside it, four findings from a review of this branch: - `verify_stateless_block` re-ran `validate_block_access_list_hash` on a BAL `execute_blocks` had already validated. The check reads only `timestamp`, `gas_limit` and `block_access_list_hash`, none of which `into_with_burned_fees` changes, so the second call could not disagree — it was a duplicate full BAL traversal plus another RLP-encode-and-keccak, paid in proving cycles. - `ExecutionWitness::from_ssz` called `headers_as_vecs()` twice, copying every witness header out of the SSZ list a second time only to drop the first copy. - The hand-written `HashTreeRoot` impls for `ExecutionPayload` and `ExecutionRequests` listed fields in a literal array, so a field added to either struct would have been silently excluded from the root and from `active_fields` with no compile error. Destructure `Self` without `..` so omission stops the build. - `ef_tests-blockchain` kept its `ethrex-prover` dependency after the last use of `BackendType` went away, and `test-stateless-zkevm` had become a strict subset of `test-stateless` under a name saying otherwise. Drop both, and stop `download-test-vectors` fetching a zkEVM bundle no target reads. The per-PR regeneration of the stateless vectors is tracked separately in #7150.
…sion bump The three `stateless-validator/bin/*` guest workspaces each carry their own committed lockfile, and all three still recorded the ethrex path crates at 23.0.0. Merging main brought in the v24.0.0 workspace version bump, so `cargo metadata --locked` could no longer resolve them and `check-cargo-lock` failed on the first one it reached. Only the version strings move; no dependency is added, removed or upgraded. The locks matter because tag_release builds and signs these guests with `--guest-dir`, so without a resolvable lock the published ELF and VK bytes are not pinned.
Merging main moved this client from the glamsterdam-devnet-7 gas schedule to devnet-8, while the generated conformance vectors are still filled at execution-specs 3c3b6f4af, which carries devnet-7: COLD_STORAGE_ACCESS 3000 vs 2100, ACCOUNT_WRITE 8000 vs 9000, CALL_VALUE 10300 vs 11300, and access-list storage keys at the full cold cost rather than cold minus WARM_ACCESS. 40 fixtures that passed before the merge now fail in add_block_pipeline with GasUsedMismatch or ReceiptsRootMismatch, before any stateless validation runs, which is the same class as the five already skipped. The pin cannot move to fix this. `eip8025_optional_proofs` does not exist on the current `forks/amsterdam` default branch and 3c3b6f4af has diverged from it (124 ahead, 315 behind), so it was never merged there; `devnets/glamsterdam/8` is 315 commits behind 3c3b6f4af, carrying the devnet-8 schedule but not the #3278 schema, so vectors filled from it would have the pre-#3278 69-byte statelessOutputBytes this client cannot parse. No revision has both. So skip them by name, in a group kept separate from the original five because the two lift at different times: the five are skew against 3c3b6f4af itself and predate the devnet-8 move. 60 of the 105 fixtures still execute, so the witness and SSZ paths stay covered and the empty-suite guard in `parse_and_execute` is still meaningful — no file is left running zero tests. Verified locally against freshly generated vectors: 100 files, 0 failures. The skip list is derived from the fixtures' own JSON keys rather than file paths, and checked so that no entry is a substring of a key that must keep running.
Four conflicts plus one semantic break that git could not see. `.github/actions/install-zisk/action.yml`: both sides pin the ZisK Rust toolchain instead of riding `latest`. main's #7010 is the later, dedicated fix and additionally handles ziskup >= v1.0.0-alpha resolving its install dir from XDG_CONFIG_HOME, which this branch's version predates and would regress. The branch's contribution here was purely additive, so main's side is taken whole. `tooling/ef_tests/blockchain/{Cargo.toml,test_runner.rs}`: #7115 removed the `BackendType` dispatch from the EF-test runner, so the branch's `zisk` feature and its `BackendType::ZisK` stub arm — which existed only to keep that match exhaustive and returned "not wired into ef_tests-blockchain" — no longer have anything to gate. Both take main's side, and the now-unreferenced `zisk = ["ethrex-prover/zisk"]` feature is dropped along with the `features = ["zisk"]` that `tooling/zkevm_bench` requested from it. zkevm_bench uses `blocks_and_witness_for_test`, `types::TestUnit` and `fork::Fork`, none of which were ever behind that feature. Resolved hunk-by-hunk rather than by taking main's file: this branch adds `blocks_and_witness_for_test` outside the conflict regions, and zkevm_bench imports it. `apply_fork_choice` grew a fifth `max_reorg_depth` parameter on main. That branch-new call site is the only one the merge left at four arguments; it now passes `None`, matching the three existing call sites in the same file. `tooling/Cargo.lock` and `tooling/zkevm_bench/Cargo.lock` regenerated.
#7115 redefined the L1 `ProgramInput` as `Vec<u8>` — the spec's schema-prefixed `statelessInputBytes` — so `ProgramInput::new(blocks, witness)` and the `.blocks` field this tool relied on no longer exist. Any merge of main into this branch hits it; the zisk clippy job was the only thing building the crate, which is why it was the sole failure. `build_ssz_stateless_input` is the one encoder in the tree that produces those bytes. It sits under `crates/l2/sequencer/native_rollup/` but touches nothing beyond `ethrex-common` and `NativeCrypto`, so `ethrex-l2` is taken as a dependency to reach it. That is a heavy edge for a benchmark tool — the lock goes from 468 to 920 packages — and the alternative would be to move the encoder down into `ethrex-common` next to the types it converts. `SszStatelessInput` carries a single `new_payload_request`, while a cache holds a `Vec<Block>`. Every ethrex-replay cache is one block, so the conversion destructures to exactly one and errors otherwise rather than silently benchmarking the first block of a multi-block cache. Enabling `ethrex-l2` does not disturb the input type: its `l2` feature is inert (`l2 = []`) and it depends on `ethrex-guest-program` with default features, which are `["secp256k1"]`. `ProgramInput` therefore stays `Vec<u8>` rather than flipping to the L2 rkyv struct through feature unification. Tests assert the schema-id prefix and non-empty bytes, the structural facts that survive the type change.
**Motivation** Merges the `release/v25.0.0` branch back into `main` now that [v25.0.0](https://github.com/lambdaclass/ethrex/releases/tag/v25.0.0) is published, per the last step of the [release process](https://github.com/lambdaclass/ethrex/blob/main/docs/developers/release-process.md). The second commit matters on its own: **`main`'s release workflow is currently broken**, and this is what fixes it. Any tag cut from `main` before this merges would build every artifact and then fail to publish a release. **Description** Two commits, no conflicts with `main`: - `chore(l1,l2): bump version to 25.0.0` — the 25 internal version pins across 9 `Cargo.toml` files, the regenerated lockfiles, and both `--builder.extra-data` defaults in `docs/CLI.md`. Includes the four `crates/guest-program/stateless-validator/**/Cargo.lock` files, which are each their own workspace and reach the ethrex crates through path dependencies, so they pin the workspace version and go stale on a bump. `make update-cargo-lock` does not cover them while `check-cargo-lock` does, so they were refreshed by hand — worth closing that asymmetry in a follow-up, since every future bump hits it. - `ci(l1,l2): stop the release asset download from matching docker build records` — `finalize-release` downloaded artifacts with `pattern: "*ethrex*"`, which also matches the build records the docker jobs upload as `<org>~ethrex~<id>.dockerbuild`. Those are not zip archives, so `download-artifact` exhausted its retries trying to extract them and the job died before publishing anything. This is a regression from lambdaclass#7115, which widened the pattern to pick up the new `stateless-validator-ethrex-*` artifacts; the previous anchored pattern never matched the docker records. v25.0.0-rc.1 hit it — all 13 build jobs green, no release created. The dry-run rehearsal could not have caught it either: that job only waits on the guest builds, so it runs before any docker record exists. Fixed with two anchored patterns, `ethrex*` and `stateless-validator-ethrex-*`, verified against the run's real 17 artifacts to select exactly the 12 release assets. rc.2 published all 22 assets correctly. **Validation** v25.0.0 went through the full release checklist: three mainnet canaries paired with Prysm/Teku/Grandine, an eth-docker mainnet node, 13 consecutive successful multisync runs across hoodi/sepolia/mainnet, the SP1 GPU integration suite, and the L2 upgrade test — the last of which ran with a real SP1 GPU prover for the first time (see lambdaclass#7203). **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync.
2 participants
Motivation
Takes ownership of the stateless execution guest program release processes.
Currently our guest program deserializes using legacy logic instead of the standard, so we get rid of the legacy path.
Description
Removes the unused legacy guest program path.
Uses ere-compiler to produce the ELF and VKs, and minisign to sign them.
Closes #7011