feat: remove groth16 and mock_prover features, replaced by SP1_PROVER env var #304
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
## Walkthrough
This change removes the `groth16` and `mock_prover` feature flags from multiple Cargo.toml files and build scripts, unifying the prover implementation to always use `EnvProver` instead of conditionally using `CpuProver` or mock provers. All conditional logic and code paths related to these feature flags are eliminated, and the initialization of prover clients is simplified to use environment-based configuration. The build and test scripts are also updated to no longer reference these features. Additionally, a new `recursive_proofs` boolean config flag is introduced to control recursive proof usage at runtime, set via environment variable `SP1_PROVER`.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------|
| crates/cli/Cargo.toml<br>crates/node_types/lightclient/Cargo.toml<br>crates/node_types/prover/Cargo.toml<br>crates/tests/Cargo.toml | Removed `groth16` and `mock_prover` feature flags from `[features]` sections and dependency specifications. |
| crates/node_types/prover/src/prover/mod.rs | Replaced all usage of `CpuProver` with `EnvProver`; removed conditional compilation for mock/cpu/groth16; simplified prover logic; added `recursive_proofs` config flag; updated epoch management and proof verification logic. |
| justfile | Removed `--features groth16` and `--features "mock_prover"` from build, test, and coverage recipes. |
| .github/workflows/ci.yml | Removed `--features mock_prover` from cargo test commands; added `SP1_PROVER=mock` environment variable instead. |
| CLAUDE.md | Updated test command example to use `SP1_PROVER=mock` environment variable instead of `--features "mock_prover"`. |
| Cargo.toml | Updated zk-related dependencies to version 4.2.0; removed `mock_prover` from workspace features. |
| crates/errors/src/lib.rs | Added `ParsingError(String)` variant to `DatabaseError` enum. |
| crates/node_types/lightclient/src/lightclient.rs | Removed conditional compilation around `verify_snark_proof`; proof verification now always runs. |
| crates/node_types/prover/src/prover/tests.rs | Updated tests to use `get_latest_epoch_height()` instead of `get_epoch()` and adjusted expected epoch values accordingly. |
| crates/storage/Cargo.toml | Added `prism-da` as a workspace dependency. |
| crates/storage/src/database.rs | Replaced simple epoch get/set with detailed epoch management using `FinalizedEpoch`; added methods for latest epoch retrieval. |
| crates/storage/src/inmemory.rs | Changed epoch storage from single `u64` to vector of `FinalizedEpoch`; updated methods accordingly with validation. |
| crates/storage/src/redis.rs | Replaced simple epoch get/set with structured `FinalizedEpoch` storage keyed by height; added sequential insertion validation. |
| crates/storage/src/rocksdb.rs | Replaced simple epoch get/set with `FinalizedEpoch` keyed by height; added atomic batch writes and validation for epoch storage. |
| verification_keys/keys.json | Replaced existing base and recursive verification keys with new hexadecimal values. |
| crates/cli/src/main.rs | Added `recursive_proofs` boolean config parameter to prover config, set via `SP1_PROVER` environment variable to disable recursive proofs in mock mode. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Config
participant Prover
participant EnvProver
Config->>Prover: Initialize (recursive_proofs flag)
Prover->>EnvProver: ProverClient::from_env()
Prover->>EnvProver: prove_with_base_prover()/prove_with_recursive_prover()
EnvProver-->>Prover: Proof result
Prover-->>Config: Proof outputAssessment against linked issues
Possibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/node_types/prover/src/prover/mod.rs (1)
527-527: Update outdated commentThe comment still mentions mock prover which is no longer part of the implementation after this PR.
- // we use the base prover for the first epoch and always for mock prover because recursive verification is not really supported at the moment + // we use the base prover for the first epoch and the recursive prover for all subsequent epochs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/cli/Cargo.toml(1 hunks)crates/node_types/lightclient/Cargo.toml(2 hunks)crates/node_types/prover/Cargo.toml(0 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/tests/Cargo.toml(1 hunks)justfile(3 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/prover/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
crates/cli/Cargo.toml (1)
13-16: Successful removal of groth16 and mock_prover featuresThe removal of the groth16 and mock_prover features from the [features] section is consistent with the PR objective of replacing these features with the SP1_PROVER environment variable.
crates/node_types/lightclient/Cargo.toml (2)
9-12: Successful removal of groth16 and mock_prover featuresThe removal of the groth16 and mock_prover features from the [features] section aligns with the PR objective of replacing these features with the SP1_PROVER environment variable.
24-24: Minor formatting improvementAdded a space inside the braces for better code style consistency.
justfile (3)
90-91: Successful removal of groth16 feature flag from build commandRemoving the
--features groth16flag from the build command aligns with the PR objective of replacing feature flags with the SP1_PROVER environment variable.
111-112: Successfully replaced mock_prover feature with SP1_PROVER env varThe unit-test recipe now uses
SP1_PROVER=mockinstead of the--features "mock_prover"flag, which aligns with the PR objective.
121-122: Successfully replaced mock_prover feature with SP1_PROVER env var in coverage commandThe coverage command now uses
SP1_PROVER=mockinstead of--features "mock_prover", but retains an empty--featuresflag. This appears to be a successful migration to the environment variable approach.crates/tests/Cargo.toml (2)
20-20: Successfully removed mock_prover feature from prism-prover dependencyThe mock_prover feature has been successfully removed from the prism-prover dependency, which aligns with the PR objective.
22-22:✅ Verification successful
Verify if mock_prover feature should still be used for prism-lightclient
While the PR objective is to remove the groth16 and mock_prover features and replace them with the SP1_PROVER environment variable, the prism-lightclient dependency still includes the mock_prover feature. Please verify if this is intentional or if it was overlooked.
🏁 Script executed:
#!/bin/bash # Check if mock_prover feature is defined in prism-lightclient grep -r "mock_prover" --include="*.rs" --include="*.toml" ./crates/node_types/lightclient/Length of output: 516
Mock_prover feature is still required in tests
Thelightclientcrate’ssrc/lightclient.rsuses#[cfg(feature = "mock_prover")](and its inverse) to gate proof‐verification logic. Enablingmock_proverincrates/tests/Cargo.tomlis therefore intentional so the tests compile against the code’s current conditional branches.crates/node_types/prover/src/prover/mod.rs (6)
36-37: Clean import list replacementThe import list has been updated to remove specialized provers, keeping only the
EnvProverand necessary related imports. This aligns with the PR objective of removing thegroth16andmock_proverfeature flags.
123-123: Appropriate type replacement from CpuProver to EnvProverBoth the base and recursive prover clients have been updated to use
EnvProverinstead of likelyCpuProverbefore. This unified approach allows prover configuration to be controlled through environment variables rather than compile-time features.Also applies to: 127-127
151-152: Simplified prover client initializationThe prover client initialization has been simplified to use
ProverClient::from_env()for both base and recursive provers, removing any conditional logic that was likely present before. This is a cleaner approach that relies on runtime configuration.
442-442: Updated return types for prove_with functionsThe return types for both
prove_with_base_proverandprove_with_recursive_proverfunctions have been correctly updated to return read guards forEnvProverinstead of the previous type.Also applies to: 473-473
528-532: Simplified prover selection logicThe logic for selecting between base and recursive prover has been greatly simplified to only check the epoch height, removing conditional branches that were likely based on feature flags. This makes the code more readable and maintainable.
451-451:✅ Verification successful
Unconditional use of Groth16 proving
The method now always invokes Groth16 proving without conditional compilation, which is consistent with the removal of feature flags. Ensure this behavior works correctly under all expected environment configurations.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to removed features in the codebase echo "Checking for remaining references to removed features..." rg -i "groth16" --type rust rg -i "mock_prover" --type rustLength of output: 4908
Unconditional Groth16 proving confirmed
Allgroth16feature flags have been removed and both prover methods now invoke Groth16 unconditionally across the codebase (verified viarg -i groth16). Please ensure your full test suite—including any environment-specific tests—passes to validate this behavior under all expected configurations.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
crates/errors/src/lib.rs (1)
23-25: 🛠️ Refactor suggestionDuplicate “ParsingError” variants can confuse error-handling paths
GeneralError::ParsingErrorand the newly-addedDatabaseError::ParsingErrorboth convey the same failure mode. Having two variants with almost identical semantics:
- forces callers to choose “the right parsing error”,
- complicates pattern-matching (
matcharms are duplicated),- makes conversion logic (
From<…>) harder to maintain.Unless you intentionally need to distinguish where the parsing failed (general code vs. DB back-end), prefer re-using the existing
GeneralError::ParsingErrorthrough the existingDatabaseError::GeneralError(…)wrapper.- #[error("parsing error: {0}")] - ParsingError(String), + // Remove this variant and rely on the existing + // DatabaseError::GeneralError(GeneralError::ParsingError(_)) + // mapping.If keeping both variants is required, add clear doc-comments that describe the difference and audit all
matchblocks to ensure both cases are handled.Also applies to: 63-64
🧹 Nitpick comments (6)
.github/workflows/ci.yml (2)
22-22: Use GitHub Actionsenvmapping forSP1_PROVER
To improve readability and follow GH Actions best practices, declareSP1_PROVERin anenvblock instead of inline:- name: Run non-integration tests - run: SP1_PROVER=mock cargo test --lib --release -- --skip test_light_client_prover_talking + env: + SP1_PROVER: mock + run: cargo test --lib --release -- --skip test_light_client_prover_talking
92-92: Use a dedicatedenvsection forSP1_PROVER
Similarly, for the integration-test step, extract the environment variable into its ownenvsection for consistency:- name: Run integration tests - run: SP1_PROVER=mock cargo test -p prism-tests --lib --release + env: + SP1_PROVER: mock + run: cargo test -p prism-tests --lib --releasecrates/storage/src/inmemory.rs (1)
128-135:get_latest_epoch_heightpanics on empty DB – consider returningNotFoundwithout special-casingGood to see an explicit error, but downstream callers frequently unwrap; providing
Option<u64>(or a bespokeDatabaseError::NotFoundError) would clarify intent and remove the magic-1logic.crates/node_types/prover/src/prover/tests.rs (2)
158-167: Avoid doubleunwrap()and improve readabilityThe current predicate calls
unwrap()right afteris_ok(), consuming theResultand risking a panic if code is later refactored. A pattern-match is safer and clearer:- let epoch = prover2.clone().db.get_latest_epoch_height(); - if epoch.is_ok() && epoch.unwrap() == 3 { + if matches!(prover2.clone().db.get_latest_epoch_height(), Ok(3)) {
200-206: Same minor simplification applies here- let epoch = prover2.clone().db.get_latest_epoch_height().unwrap(); - assert_eq!(epoch, 3); + assert_eq!( + prover2.clone().db.get_latest_epoch_height().expect("no epoch present"), + 3 + );crates/storage/src/rocksdb.rs (1)
111-117: Avoid lossyu64 → usizecasts when comparing heights
usizeis 32-bit on some targets; casting a largeu64epoch height can truncate the value and pass the sequential-height check even though the numbers differ.
Keep everything inu64:-if latest as usize + 1 != epoch.height as usize { +if latest + 1 != epoch.height {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml(2 hunks)CLAUDE.md(2 hunks)Cargo.toml(1 hunks)crates/errors/src/lib.rs(1 hunks)crates/node_types/lightclient/src/lightclient.rs(0 hunks)crates/node_types/prover/src/prover/mod.rs(16 hunks)crates/node_types/prover/src/prover/tests.rs(2 hunks)crates/storage/Cargo.toml(1 hunks)crates/storage/src/database.rs(2 hunks)crates/storage/src/inmemory.rs(5 hunks)crates/storage/src/redis.rs(3 hunks)crates/storage/src/rocksdb.rs(2 hunks)crates/tests/Cargo.toml(1 hunks)justfile(4 hunks)verification_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (1)
- crates/node_types/lightclient/src/lightclient.rs
✅ Files skipped from review due to trivial changes (4)
- crates/storage/Cargo.toml
- verification_keys/keys.json
- CLAUDE.md
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- justfile
- crates/tests/Cargo.toml
🧰 Additional context used
🧬 Code Graph Analysis (2)
crates/storage/src/inmemory.rs (3)
crates/storage/src/redis.rs (5)
new(48-72)get_epoch(199-211)add_epoch(213-250)get_latest_epoch_height(252-256)get_latest_epoch(258-261)crates/storage/src/rocksdb.rs (6)
new(31-35)new(45-53)get_epoch(91-104)add_epoch(106-144)get_latest_epoch_height(146-155)get_latest_epoch(157-160)crates/storage/src/database.rs (4)
get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)
crates/storage/src/rocksdb.rs (4)
crates/storage/src/inmemory.rs (4)
get_epoch(106-112)add_epoch(114-126)get_latest_epoch_height(128-134)get_latest_epoch(136-139)crates/storage/src/database.rs (4)
get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)crates/storage/src/redis.rs (4)
get_epoch(199-211)add_epoch(213-250)get_latest_epoch_height(252-256)get_latest_epoch(258-261)crates/serde/src/binary.rs (2)
decode_from_bytes(24-24)decode_from_bytes(32-34)
🔇 Additional comments (5)
crates/storage/src/database.rs (1)
21-26:✅ Verification successful
Trait additions look good, but scan the codebase for stale
set_epochcallsThe old
set_epoch()API has been removed in favour ofadd_epoch()and richer getters. To avoid build breaks, ensure no crates still invoke the obsolete method.If the search returns anything, migrate those sites to
add_epoch().
🏁 Script executed:
#!/bin/bash # grep for lingering `set_epoch(` calls rg -n $'\\.set_epoch\\('Length of output: 23
🏁 Script executed:
#!/bin/bash # Search for any lingering `set_epoch(` calls anywhere in the codebase rg -n 'set_epoch\s*\('Length of output: 24
No stale
set_epoch()calls found
I ranrg -n 'set_epoch\s*\('across the repo and confirmed there are no remaining usages of the old API. The removal ofset_epoch()won’t introduce any build breaks.crates/storage/src/inmemory.rs (2)
154-155:flush_databaseclears epochs but leavessync_heightuntouchedAfter a full flush,
sync_heightlikely no longer reflects reality. Reset it to0(or1if height is one-based) to keep internal state consistent.
106-111:⚠️ Potential issueIndexing with
usizemay overflow for large epoch heights
epochs.get(*height as usize)and the subsequent cast implicitly assume everyu64height fits intousize. On 32-bit targets, heights ≥ 2³² will panic.- match epochs.get(*height as usize) { + if *height > usize::MAX as u64 { + return Err(DatabaseError::ReadError(format!( + "epoch height {} exceeds addressable memory", height + )) + .into()); + } + match epochs.get(*height as usize) {Similar
as usizecasts appear inadd_epoch; consider replacing them with an explicitexpected_heightcomputed inu64and compared without casting.Likely an incorrect or invalid review comment.
crates/storage/src/rocksdb.rs (1)
107-124: Don’t silently swallow database errors when checking the latest epoch height
get_latest_epoch_height().ok()converts any error – including I/O or data-corruption errors – intoNone, treating them the same as “no height stored yet”.
That makes genuine corruption or permission issues indistinguishable from the clean bootstrap case and can cause hard-to-diagnose bugs later on.-let latest_height = self.get_latest_epoch_height().ok(); +let latest_height = match self.get_latest_epoch_height() { + Ok(h) => Some(h), + Err(e) => { + // Only ignore the “not found” bootstrap case. + if e.downcast_ref::<DatabaseError>() + .is_some_and(|de| matches!(de, DatabaseError::NotFoundError(_))) + { + None + } else { + return Err(e); + } + } +};[ suggest_essential_refactor ]
crates/node_types/prover/src/prover/mod.rs (1)
143-149: Verify whethersaved_epoch = latest + 1is correct – potential off-by-one
KeyDirectoryTree::load(db, saved_epoch)is fedlatest_epoch_height + 1.
If the tree’s version equals the last finalized epoch height (rather than “next height to be produced”), this will load a non-existent version and may corrupt state.Please double-check the expected semantics of
KeyDirectoryTree::load.
If the tree is versioned by epoch height, you likely want:-let saved_epoch = match db.get_latest_epoch_height() { - Ok(height) => height, +let saved_epoch = match db.get_latest_epoch_height() { + Ok(height) => height,
distractedm1nd
force-pushed
the
no-zk-feature
branch
from
cbe4ffa to
3a68b64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/storage/src/rocksdb.rs (1)
1-312: 🛠️ Refactor suggestionMissing tests for new epoch-related methods
While the code implements the Database trait methods correctly, there are no tests for the new epoch-related methods (get_epoch, add_epoch, get_latest_epoch_height, get_latest_epoch).
Consider adding tests similar to the existing ones for other methods. This is especially important since these methods handle critical data and have validation logic that should be verified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.yml(2 hunks)CLAUDE.md(2 hunks)Cargo.toml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(5 hunks)crates/errors/src/lib.rs(1 hunks)crates/node_types/lightclient/Cargo.toml(2 hunks)crates/node_types/lightclient/src/lightclient.rs(0 hunks)crates/node_types/prover/Cargo.toml(0 hunks)crates/node_types/prover/src/prover/mod.rs(17 hunks)crates/node_types/prover/src/prover/tests.rs(2 hunks)crates/storage/Cargo.toml(1 hunks)crates/storage/src/database.rs(2 hunks)crates/storage/src/inmemory.rs(5 hunks)crates/storage/src/redis.rs(3 hunks)crates/storage/src/rocksdb.rs(2 hunks)crates/tests/Cargo.toml(1 hunks)justfile(4 hunks)verification_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (2)
- crates/node_types/prover/Cargo.toml
- crates/node_types/lightclient/src/lightclient.rs
✅ Files skipped from review due to trivial changes (1)
- justfile
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/storage/Cargo.toml
- crates/tests/Cargo.toml
- crates/node_types/prover/src/prover/tests.rs
- Cargo.toml
- verification_keys/keys.json
- crates/node_types/lightclient/Cargo.toml
- CLAUDE.md
- crates/cli/Cargo.toml
- crates/errors/src/lib.rs
- crates/cli/src/main.rs
- .github/workflows/ci.yml
- crates/storage/src/database.rs
- crates/storage/src/redis.rs
- crates/storage/src/inmemory.rs
- crates/node_types/prover/src/prover/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/storage/src/rocksdb.rs (4)
crates/storage/src/database.rs (4)
get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)crates/storage/src/inmemory.rs (4)
get_epoch(106-112)add_epoch(114-126)get_latest_epoch_height(128-134)get_latest_epoch(136-139)crates/storage/src/redis.rs (4)
get_epoch(199-211)add_epoch(213-250)get_latest_epoch_height(252-256)get_latest_epoch(258-261)crates/serde/src/binary.rs (2)
decode_from_bytes(24-24)decode_from_bytes(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (6)
crates/storage/src/rocksdb.rs (6)
4-4: Import addition and organization looks goodThe addition of
anyhow::{Result, anyhow}matches its usage throughout the file for error handling and creating detailed error messages.
15-15: Import refinement for RocksDB types looks goodExplicitly importing the RocksDB types (
DB,DBWithThreadMode,MultiThreaded,Options) improves code clarity.
21-21: Added constant for epoch storageThe new key prefix constant follows the established naming convention for other key prefixes in the file.
91-104: Solid implementation of get_epoch methodThe method correctly retrieves and deserializes a FinalizedEpoch by its height, with appropriate error handling for both missing records and deserialization failures. This implementation aligns well with the similar implementations in inmemory.rs and redis.rs.
146-155: Appropriate implementation of get_latest_epoch_heightThe method properly retrieves the latest epoch height from the database with appropriate error handling.
157-160: Clean implementation of get_latest_epochThis method efficiently combines get_latest_epoch_height and get_epoch to retrieve the most recent epoch.
…dulated by SP1_PROVER
distractedm1nd
force-pushed
the
no-zk-feature
branch
from
bc22172 to
3994302
Compare
closes #194
Now just uses SP1_PROVER everywhere. The features no longer exist
Summary by CodeRabbit
Refactor
Bug Fixes
Chores