Repository navigation
feat(l2): let the OnChainProposer owner replace the SP1 verifier - #7412
Conversation
|
🤖 Kimi Code ReviewI'll review this PR which adds an Security and Correctness ReviewCritical Issue: Missing Zero-Address Check for
|
| Severity | Issue | Location |
|---|---|---|
| Low | Missing zero-address check edge case in Aligned mode | OnChainProposer.sol (both) |
| Low | Event lacks previous verifier for audit trail | IOnChainProposer.sol (both) |
| Info | Gateway upgrade path not documented | upgrades.md |
The PR is fundamentally correct and safe for merge. The validation logic properly protects against the critical case (losing verifier when proofs are required). Consider addressing the informational items in follow-up.
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
🤖 Claude Code ReviewReview:
|
🤖 Codex Code ReviewFound one concrete issue.
Otherwise, the change looks reasonable:
Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
…) and main, into this branch, so this pull request stacks on them
Lines of code reportTotal lines added: Detailed view |
| } | ||
|
|
||
| /// @inheritdoc IOnChainProposer | ||
| function upgradeSP1Verifier(address new_verifier) public onlyOwner { |
There was a problem hiding this comment.
Worth guarding this against pending batches. Proofs of batches committed under the old version only verify against the old verifier, so swapping while lastVerifiedBatch != lastCommittedBatch strands them. On the regular contract the way out is pause, revertBatch and re-commit; here on the based one there is no revertBatch and no pause, so the only remedy is swapping the old verifier back. The runbook's step 1 says to drain first, but the contract can enforce it:
if (lastVerifiedBatch != lastCommittedBatch)
revert UnverifiedBatchesPending(lastVerifiedBatch, lastCommittedBatch);Same in the regular proposer, with the error declared in both interfaces. It also makes the one-shot upgradeToAndCall(impl, upgradeSP1Verifier(...)) revert when batches are pending, which is the right outcome: that is exactly the state the swap must not happen in.
There was a problem hiding this comment.
Done in 1850f58: both proposers now revert upgradeSP1Verifier with UnverifiedBatchesPending(lastVerified, lastCommitted), declared in both interfaces, so upgradeToAndCall(impl, upgradeSP1Verifier(...)) reverts as a whole in that state too. One trade-off: if the new verifier turns out wrong and batches get committed after the swap, the guard also blocks fixing it with the setter. The regular proposer recovers with pause, revertBatch, the setter and a re-commit; the based one would need an implementation upgrade. That seems the better trade, since a premature swap is the likelier mistake. To narrow the wrong-verifier case, 30648b9 also refuses an address without code, and 2fe1aaa adds Hardhat tests for the setter on both proposers, the guard and the upgradeToAndCall path included.
…anch, keeping the stack current
…to this branch, keeping the stack current
2fe1aaa to
472c4fd
Compare
…ther build sealed. The committer re-sends an already sealed batch as it is, and prover inputs are stored per version, so a batch sealed by the build that ran before an upgrade, or received from the sequencer this node synced from, had no input for the new version: its provers got no work and verification stopped. This happens when an upgrade has to re-commit batches, for example after reverting batches whose proofs the verifier no longer accepts. The input is generated before the checkpoint moves to the batch's end, from the same state the batch starts at.
…w SP1 version can wrap its proofs in a different Groth16 circuit, and the SP1VerifierGroth16 a deployment points at rejects those (WrongVerifierSelector) whatever verification key is registered, while SP1_VERIFIER_ADDRESS could only be set in initialize. upgradeSP1Verifier(address), onlyOwner like upgradeSP1VerificationKey, sets it and emits SP1VerifierUpgraded; it refuses address zero while SP1 proofs are required outside Aligned mode. Both the regular and the based OnChainProposer get it; storage layout is unchanged. An existing deployment can upgrade its OnChainProposer and set the verifier in one Timelock call, upgradeToAndCall(newImplementation, upgradeSP1Verifier(newVerifier)). The upgrades fundamentals explain when the verifier has to be replaced, and that deployments on Succinct's SP1VerifierGateway have nothing to replace.
…ied. Proofs of batches committed before the swap only verify against the verifier they were proven for, so a swap with lastVerifiedBatch below lastCommittedBatch strands them: the regular OnChainProposer can recover with pause, revertBatch and a re-commit, but the based one has neither, so its only way out was swapping the old verifier back. Both proposers now revert upgradeSP1Verifier with UnverifiedBatchesPending(lastVerified, lastCommitted), declared in both interfaces, which also makes a one-shot upgradeToAndCall(implementation, upgradeSP1Verifier(verifier)) revert as a whole in that state. The upgrades fundamentals say the contract enforces the drain. Runtime code grows by 60 bytes to 19,353, and by 62 to 13,775 for the based proposer.
…t code. A mistyped address was accepted and only showed up when the next proof failed to verify, and now that the setter refuses to run while a batch is unverified, batches committed after such a swap can no longer be rescued by swapping again: the regular proposer has to pause and revert them first, and the based one needs a new implementation. Both proposers revert upgradeSP1Verifier with Sp1VerifierHasNoCode for a non-zero address with no code; address zero keeps its existing rules.
…ifier, as the regular one does. It accepted address zero while SP1 proofs were required outside Aligned mode, leaving every verification to fail with InvalidSp1Proof, while upgradeSP1Verifier refuses the same address. The deployer always passes or deploys a verifier in that configuration, so existing deployments are unaffected.
… the Upgradeability workflow runs. Each proposer is deployed behind a UUPS proxy and initialized, and the tests check that the setter replaces the verifier once every batch is verified, refuses while a batch is not, is owner-only, refuses address zero unless in Aligned mode and an address without code, works inside upgradeToAndCall and reverts that whole call while a batch is unverified, and that the initializer refuses a missing SP1 verifier. Committing real batches needs a bridge, blobs and a sequencer, so the tests write the two batch counters at the slots the compiler reports for them.
472c4fd to
a6a69b6
Compare
…ss#7401) **Motivation** The L2 prover and its SP1 batch guest are on SP1 5.0.8, and the newest SP1 is 6.9.0. Succinct froze the SP1 v5.0.0 Groth16 route on its verifier gateway on Oct 9, and Aligned is moving its aggregation mode to SP1 6. 6.9.0 produces the same verification artifacts as 6.8.1, the version Aligned is targeting (circuit version `v6.1.0`). It is also past GHSA-p7v2-jjv3-8p8f, which affects SP1 6.0.0 through 6.8.0. **Description** Prover (`crates/prover`): - `sp1-sdk` moves to `=6.9.0` with its `blocking` API, and `sp1-prover` is no longer a direct dependency. - The backend sets the guest up once with `setup` and keeps the proving key; proving is `prove(&pk, stdin).mode(..).run()`. - SP1 6's blocking API drives its own tokio runtime with `block_on`, which panics on a thread that is already running one. The prover calls the backend from async code, so the SDK calls (setup, execute, prove) now run on a scoped thread outside the runtime. - With `gpu`, it uses the `CudaProver`, which runs SP1 6's local `sp1-gpu-server`. SP1 6 has no remote prover server, so `--sp1-server` / `ETHREX_SP1_SERVER` is now ignored with a warning, and its help text says so. - `sp1-sdk` 6 has no default features, so the network and AWS KMS dependencies no longer come in through the prover. Guest (`crates/guest-program`): - `sp1-zkvm`, `sp1-build` and the build-time `sp1-sdk` move to `=6.9.0`. SP1 6 builds a RISC-V 64-bit guest, and the reproducible build uses the `v6.9.0` image. - The ELF and verifying-key files keep their `riscv32im-succinct-zkvm-*` names, because release assets and scripts refer to them. - The precompile patches move to the `sp1-6.2.0` tags that SP1 6.9.0's own examples use. The `ecdsa` patch goes: the `k256` patch now brings its own. - `kzg-rs` is patched to Succinct's `v0.2.8-sp1-6.2.0` tag. The crates.io release would otherwise pull `sp1-lib` 5's syscall wrappers into an SP1 6 guest. - The lambdaclass `bls12_381` forks are dropped. The guest only used them for `G1Affine`/`G2Affine::new_unchecked` and the exported `Fp`/`Fp2`. The field types are reachable through upstream's `MapToCurve::Field`, as the OpenVM stateless provider already does. Once both coordinates pass the canonical check, `from_uncompressed_unchecked` decodes exactly the same point, because none of its flag bits can be set. `guest-program` now uses `bls12_381` 0.8.0 from crates.io. The SP1 guest patches it to `sp1-patches`' `patch-0.8.0-sp1-6.2.0`, the copy `kzg-rs` already uses, so it builds one `bls12_381` instead of two. The RISC0 guest's lockfile still names the old fork: it cannot be resolved until risc0 publishes a `c-kzg` 2.1.8 or newer tag, which is also why CI skips it. - `sp1-zkvm` 6 has no `embedded` feature, because its embedded allocator is now the default, so the `l2` feature no longer enables it. Contracts and deployment: - The deployer compiles `SP1VerifierGroth16` from sp1-contracts `v6.1.0`, the circuit version SP1 6.9.0 proves against. - An existing deployment's SP1 5 verifier rejects SP1 6 proofs whatever key is registered: they start with a different verifier selector (`0x4388a21c` instead of `0xa4594c59`) and come from a different Groth16 circuit. A deployment with a verifier of its own needs lambdaclass#7412 to replace it; one on Succinct's `SP1VerifierGateway` only registers the new key, since the gateway routes SP1 6 proofs to its `v6.1.0` verifier. Aligned mode: - Aligned mode is unavailable for now, as agreed with the Aligned team, who are releasing SP1 6 support soon. - Their SDK (`agg_mode_sdk`) still uses `sp1-sdk` 5 types and verifies SP1 5 proofs, so it cannot take this guest's proofs or program. - `crates/l2` keeps `sp1-sdk` 5.0.8 for that glue only. A sequencer started with `--aligned` now stops at startup with an error that explains this, and the Aligned deployment guide has a warning. OpenVM L2 backend: - The OpenVM backend of the L2 prover is removed: `--backend openvm` and the `openvm` features of `ethrex-prover` and `ethrex-l2-prover`. - OpenVM 1.5 requires `p3-field =0.4.1` and SP1 6 requires `=0.4.3-succinct`. Both are crates.io releases in the same semver range, so no lockfile can hold both. - The OpenVM guest stays; only the prover backend goes. The docs already list OpenVM's L2 prover as planned. CI and docs: - `install-sp1` installs 6.9.0, and `protoc`, which SP1 6's `sp1-prover-types` build script needs. - The GPU workflow stops a stale `sp1-gpu-server` instead of removing SP1 5's `sp1-gpu` container, which no longer exists. - The GPU workflow enables GPU persistence mode before the test. The SDK waits about one second for the `sp1-gpu-server` it starts to open its socket. Without persistence mode, the runner's driver takes about two seconds to initialize the GPU, so the prover failed at startup. - The GPU workflow runs the prover with a smaller SP1 proving pipeline (`SP1_WORKER_*`, one worker per stage), which `sp1-gpu-server` reads from the prover's environment. With SP1's defaults, the server peaked at 42.4 GB of host memory on the 44 GB runner and was killed. With these settings it peaked at 34.8 GB on the same three Groth16 proofs, up to a 700M-cycle block, measured on the runner. Small proofs took the same time. - The prover docs name 6.9.0 and the GPU server. The SP1 prover guide's troubleshooting section now covers that startup error and the out-of-memory case, replacing SP1 5's Docker GPU error, which can no longer happen. The stateless-validator guests are a separate workspace that follows ere's SP1, and are unaffected. Upgrading an existing deployment to this release, with or without downtime, is documented in lambdaclass#7417, which is stacked on this one and also adds an SP1 version upgrade test. It relies on lambdaclass#7412 for deployments with their own verifier, and on lambdaclass#7411 when the new release commits batches another build sealed. CI does not build or run the SP1 guests, so they were checked separately: - The L1 variant of the guest takes the spec's SSZ stateless input. Built with SP1 6.9.0, it was executed over every stateless input of the `tests-zkevm@v21.0.1` engine fixtures (33,637 blocks) and the 25 `tests-zkevm-benchmark@v21.0.5` workloads (up to 200M gas). Every public output matches the fixture's expected output, including the 1,029 blocks that exercise the EIP-2537 precompiles. - Upgrades from v25.0.0 and v29.0.1 to this release were run on `l2-gpu`, with and without downtime, for a deployment with its own verifier (with lambdaclass#7412) and for one on a local `SP1VerifierGateway` whose `v5.0.0` route was frozen (with lambdaclass#7411). In every case the new release's SP1 6 proofs were verified on L1 and the L2 integration tests passed. - The L2 guest was proven end to end on an NVIDIA RTX 4000 Ada (20 GB) with the `gpu` feature, following the GPU integration workflow: a dev L1, deployment with the v6.1.0 verifier, the sequencer and the SP1 prover. All 29 batches were proven with Groth16 and verified on L1, and the L2 integration tests passed. **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync.
) **Motivation** lambdaclass#7401 moves the L2 prover to SP1 6.9.0, and an existing deployment cannot just switch provers: SP1 6 proofs need the `v6.1.0` `SP1VerifierGroth16`, and what has to change on L1 depends on what the OnChainProposer's `SP1_VERIFIER_ADDRESS` points at. A deployment with its own verifier has to replace it (lambdaclass#7412). One on Succinct's `SP1VerifierGateway` has nothing to replace, but the gateway froze its `v5.0.0` route, so batches committed with SP1 5 and not verified in time have to be reverted and committed again (lambdaclass#7411). The per-release upgrade test only registers the new verification key, which covers neither (lambdaclass#7415). **Description** - The deployment upgrade guide gets a "From v29 to v30" section for SP1 6: - contract steps: deploy the `v6.1.0` verifier and the new OnChainProposer implementation, switch both in one Timelock call, `upgradeToAndCall(newImplementation, upgradeSP1Verifier(newVerifier))`, and register the new verification key; - moving the nodes without downtime (the `--admin.start-at` handover) or with a stop, around the point where every old batch is verified and no new proof has been sent; - with Succinct's gateway: check that the batches to revert published no withdrawals (lambdaclass#7414), `pause`, `revertBatch` and `unpause` through the Timelock, register the key, and either revert the node's rollup store (with the workarounds for lambdaclass#7413) or hand over to the new sequencer; - what changes for the prover host, and that Aligned mode is unavailable. - The upgrades fundamentals explain what changes when the zero-downtime handover moves to a new version: stop the old committer and let its prover verify everything first, register the new key before the handover, and take the old sequencer's enode from its startup log. - A new SP1 version upgrade test runs the three cases on the GPU host from an older release: own verifier, gateway with a stop, and gateway without downtime. It shows how to deploy a gateway on the test L1 and freeze its `v5.0.0` route, and records the results of the last run, from v25.0.0. This is the top of a stack: lambdaclass#7411, then lambdaclass#7412, then lambdaclass#7401, then this one. The diff here is the docs only. Closes lambdaclass#7415 **Checklist** - [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR includes breaking changes to the `Store` requiring a re-sync. --------- Co-authored-by: Javier Chatruc <jrchatruc@gmail.com>
2 participants
Motivation
SP1_VERIFIER_ADDRESScan only be set ininitialize. A new SP1 version can wrap its proofs in a different Groth16 circuit, and anSP1VerifierGroth16only accepts proofs of its own circuit: it checks the proof's first four bytes against itsVERIFIER_HASH(0xa4594c59for SP1 5'sv5.0.0,0x4388a21cfor SP1 6'sv6.1.0) and reverts withWrongVerifierSelectorotherwise, whatever verification key is registered. A deployment that points at a verifier of its own, which is what the deployer creates by default, so cannot move its prover to a new SP1 version. Deployments on Succinct'sSP1VerifierGatewayare not affected: the gateway routes each proof to the verifier of its version.Description
upgradeSP1Verifier(address). It isonlyOwnerlikeupgradeSP1VerificationKey, setsSP1_VERIFIER_ADDRESSand emitsSP1VerifierUpgraded. It refuses address zero while SP1 proofs are required outside Aligned mode, which verifies SP1 proofs through the AlignedProofAggregator instead. It also refuses to run while a committed batch is unverified (UnverifiedBatchesPending(lastVerified, lastCommitted), declared in both interfaces): those batches' proofs only verify against the verifier they were proven for, and the based proposer has norevertBatchto recover them. It refuses a non-zero address without code (Sp1VerifierHasNoCode), which would otherwise only show up when the next proof fails.tooling/hardhat/test/upgrade-sp1-verifier.test.jscovers the setter on both proposers in the Hardhat suite the Upgradeability workflow runs: the swap once every batch is verified, the refusal while one is not, owner-only access, address zero and addresses without code, the one-callupgradeToAndCallpath reverting as a whole while a batch is pending, and the initializer check.upgradeToAndCall(newImplementation, upgradeSP1Verifier(newVerifier)): the upgrade runs the setter with the Timelock as the caller, soonlyOwnerpasses. It reverts as a whole if a batch is still unverified.This was run on a GPU host from v25.0.0 and from v29.0.1 to a build with SP1 6.9.0 (#7401). One
emergencyExecuteofupgradeToAndCall(newImplementation, upgradeSP1Verifier(v6.1.0 verifier))movedSP1_VERIFIER_ADDRESSfrom thev5.0.0verifier to thev6.1.0one, the new release's SP1 6 proofs were verified, and the L2 integration tests passed. Those runs drained every batch before the switch, so the guard added since does not change their path.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.