Recursive snarks #259
Recursive snarks #259
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe pull request removes the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -34,15 +35,29 @@ where | |||
| tokio::spawn(future); | |||
| } | |||
|
|
|||
| #[derive(Deserialize)] | |||
| pub struct VerificationKeys { | |||
There was a problem hiding this comment.
maybe in the wrong place
![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
🔭 Outside diff range comments (2)
crates/da/src/lib.rs (1)
17-17: 💡 Verification agent🧩 Analysis chain
Verify if this sp1_sdk import should be removed.
The PR objective mentions removing the
sp1-sdkdependency, but this line still imports from it. Please check if this is intentional or if it was overlooked.
🏁 Script executed:
#!/bin/bash # Check if sp1_sdk is still used elsewhere in the codebase echo "Checking for remaining sp1_sdk imports..." rg "use.*sp1_sdk" -g "*.rs"Length of output: 362
Action Required: Clarify and Remove sp1_sdk Usage
The PR objective is to remove the
sp1-sdkdependency, yet we still find references in multiple files. In particular, the filecrates/da/src/lib.rs(line 17) imports:use {prism_common::transaction::Transaction, sp1_sdk::SP1ProofWithPublicValues};Additionally, our search shows remaining imports in:
crates/node_types/prover/src/prover/mod.rscrates/zk/sp1-script/src/main.rsPlease confirm whether:
- The usage of
sp1_sdk::SP1ProofWithPublicValueshere (and in the other files) is still required.- If it is not needed, update the code by removing these imports (and any associated usage) to fully align with the PR’s objective.
- If the dependency is still required in some modules, provide a clear rationale for its retention.
crates/node_types/prover/src/prover/mod.rs (1)
360-383: 🛠️ Refactor suggestionAvoid using
panic!for verification failures.
Panicking on proof verification failure can shut down the entire node. Instead, consider returning an error and handling it gracefully or logging and continuing, depending on your reliability requirements.-match client.verify(&epoch.proof, verifying_key) { - Ok(_) => info!("zkSNARK for epoch {} was validated successfully", epoch.height), - Err(err) => panic!( - "failed to validate epoch at height {}: {:?}", - epoch.height, err - ), -} +if let Err(err) = client.verify(&epoch.proof, verifying_key) { + return Err(anyhow!( + "Proof verification failed at epoch {}: {:?}", + epoch.height, + err + )); +} +info!("zkSNARK for epoch {} was validated successfully", epoch.height);
🧹 Nitpick comments (7)
crates/zk/sp1/src/bin/recursive_prover.rs (1)
19-22: Consider improving error reporting.While panicking is appropriate for mandatory verification, providing more detailed error information could help with debugging.
- let result = Groth16Verifier::verify(&proof, &public_values, &vkey_hash, &GROTH16_VK_BYTES); - if result.is_err() { - panic!("recursive verification failed"); - } + if let Err(err) = Groth16Verifier::verify(&proof, &public_values, &vkey_hash, &GROTH16_VK_BYTES) { + panic!("recursive verification failed: {}", err); + }justfile (1)
102-105: Generating verification keys with inline JSON.
Consider checking the exit status of thecargo prove vkeycalls to surface errors early if key generation fails.crates/node_types/lightclient/src/lightclient.rs (2)
38-42: NewVerificationKeysstruct.
Wrapping bothbase_vkandrecursive_vkinto a single struct improves maintainability. If these keys become more complex, consider storing them in a more strongly typed fashion (e.g., hex-decoded binary) to prevent accidental misuse.
286-286: PassingvkeyandGROTH16_VK_BYTESto verifier.
Ensure the usage ofGROTH16_VK_BYTESis documented so future maintainers understand its role alongside thevkey.crates/node_types/prover/src/prover/mod.rs (3)
122-129: Consider grouping related prover fields into their own structures.
Adding separate fields for base and recursive proofs is good for readability, but you may encapsulate them into a dedicated struct (e.g.BaseProverandRecursiveProver) to reduce repetition and simplify code management.-pub struct Prover { - ... - base_prover_client: Arc<RwLock<CpuProver>>, - base_proving_key: SP1ProvingKey, - base_verifying_key: SP1VerifyingKey, - recursive_prover_client: Arc<RwLock<CpuProver>>, - recursive_proving_key: SP1ProvingKey, - recursive_verifying_key: SP1VerifyingKey, - ... +pub struct BaseProverBundle { + pub client: Arc<RwLock<CpuProver>>, + pub proving_key: SP1ProvingKey, + pub verifying_key: SP1VerifyingKey, +} + +pub struct RecursiveProverBundle { + pub client: Arc<RwLock<CpuProver>>, + pub proving_key: SP1ProvingKey, + pub verifying_key: SP1VerifyingKey, +} + +pub struct Prover { + ... + base: BaseProverBundle, + recursive: RecursiveProverBundle, + ... }
149-159: Consider refactoring the repeated prover client instantiation.
You can factor out common builder logic into a helper function that returns either a mock or CPU-basedProverClientbased on the feature gate, avoiding code duplication between base and recursive initialization.
442-519: Code duplication in base vs. recursive proof generation.
While splitting logic between the first epoch and subsequent epochs is correct for base/recursive, large portions of the code are repeated. You might streamline inputs and unify proof generation steps if the major difference is just the previous epoch data for the recursive flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/cli/Cargo.toml(0 hunks)crates/cli/src/main.rs(0 hunks)crates/da/src/celestia/utils.rs(2 hunks)crates/da/src/lib.rs(2 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/events.rs(4 hunks)crates/node_types/lightclient/src/lightclient.rs(7 hunks)crates/node_types/prover/src/prover/mod.rs(5 hunks)crates/node_types/wasm-lightclient/src/worker.rs(0 hunks)crates/tests/Cargo.toml(0 hunks)crates/tests/src/lib.rs(0 hunks)crates/zk/sp1/Cargo.toml(1 hunks)crates/zk/sp1/src/bin/base_prover.rs(1 hunks)crates/zk/sp1/src/bin/recursive_prover.rs(1 hunks)justfile(1 hunks)
💤 Files with no reviewable changes (5)
- crates/cli/Cargo.toml
- crates/tests/Cargo.toml
- crates/tests/src/lib.rs
- crates/cli/src/main.rs
- crates/node_types/wasm-lightclient/src/worker.rs
✅ Files skipped from review due to trivial changes (1)
- crates/zk/sp1/src/bin/base_prover.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (29)
crates/node_types/lightclient/Cargo.toml (1)
24-24: Addedserde_jsondependency to support verification key deserialization.The addition of
serde_jsonaligns with the PR's goal of restructuring verification key handling, likely to support the newload_sp1_verifying_keysfunction mentioned in the AI summary.crates/da/src/celestia/utils.rs (2)
27-27: Updated start height in default configuration.The start height has been increased from 4683905 to 4851608, which may represent a protocol upgrade point or network synchronization requirement.
84-84: Updated start height in Specter network configuration.Good to see consistent updates of the start height value in both places.
crates/da/src/lib.rs (2)
36-36: Added newpublic_valuesfield to FinalizedEpoch struct.This change appears to be part of the verification key restructuring, allowing direct access to public values.
54-54: Added necessary cloning of public_values.Ensures that the public values are properly maintained when creating an epoch instance without a signature.
crates/zk/sp1/src/bin/recursive_prover.rs (2)
7-23: Strong verification model for recursive proofs.The implementation enforces mandatory verification with no option to bypass it, which is appropriate for a cryptographic system. The documentation clearly states the purpose and behavior.
26-33: Clear batch processing with performance tracking.The implementation includes appropriate logging markers for performance tracking and handles commitment of the roots correctly.
crates/zk/sp1/Cargo.toml (2)
13-14: Newsp1-verifierdependency added.
This aligns with the new verification approach. No issues detected.
15-21: New binary targets defined.
Specifyingbase_proverandrecursive_proveras separate binaries is a clear organizational choice. Good job.justfile (3)
90-94: Build steps for SP1 base binary.
No issues found. These steps are clearly outlined and consistent with the updated build process.
95-98: Build steps for SP1 recursive binary.
The separation of the recursive binary build is well-structured; it cleanly mirrors the base binary approach.
99-101: Creation of verifying keys directory.
The directory creation command is straightforward. No further issues.crates/node_types/lightclient/src/events.rs (4)
22-23: New enum variants for recursive verification.
AddingRecursiveVerificationStartedandRecursiveVerificationCompletedclarifies event handling for the new verification flow.
59-64: Updated Display implementation.
Providing user-friendly messages with the verification height is helpful for debugging and logs. Looks good.
76-76:formatted_logfield added toEventInfo.
Embedding the event’s string representation in the event info is an effective way to provide logs alongside structured data.
117-121: Populatingformatted_loginsendmethod.
Generating aformatted_logfor each event ensures consistent logging. Good approach.crates/node_types/lightclient/src/lightclient.rs (9)
6-6: Importingserde::Deserialize.
This import is necessary for reference to#[derive(Deserialize)]. No issues here.
44-50: Embedded JSON & loading function.
Embedding and deserializing at compile time is a neat solution for WASM environments. Verify that the embedded data is secure and kept up-to-date.
59-60: Doc comment for SP1 verifying keys.
Clear documentation helps new contributors understand the purpose ofsp1_vkeys.
82-82: Storingsp1_vkeys.
Including the verifying keys directly in theLightClientis consistent with the struct’s purpose.
113-148: Backward search for the most recent epoch.
Be aware that if no valid epoch is found at or abovestart_height, this loop terminates on the first decrement. Ensure the logic meets the intended coverage of all heights in the search range.
182-182: Check public values length before access.
Preventing out-of-range errors is crucial. This is a solid safety check.
190-190: Extracting commitments from public values.
Logic is straightforward and effectively ensures consistent reading of the commitment bytes.
203-206: Conditional proof verification.
Skipping proof verification undermock_proveraids fast testing and development. Good compliance with the feature flag.
277-282: Selecting the appropriate verification key.
Choosing the base key for height 0 and the recursive key otherwise appears correct for a two-phase system. Confirm that transitions at higher epoch heights, if any, follow the same logic.crates/node_types/prover/src/prover/mod.rs (4)
35-38: The code still referencessp1_sdk, which contradicts the PR summary stating it's removed.
The AI-generated summary claims that the pull request removes thesp1-sdkdependency, yet these lines still import it. Please verify whether the library is intended to remain or if further cleanup is needed.Likely an incorrect or invalid review comment.
40-42:BASE_PRISM_ELFandRECURSIVE_PRISM_ELFpoint to the same ELF file.
If these two constants are meant to reference distinct proofs, ensure that you are using unique files. Otherwise, consider renaming to reflect that they are currently identical.Would you like me to generate a script to confirm that both ELF paths refer to separate files on your system?
160-162: Double-check error handling on.setup()calls.
Ensure thesetup()calls on both prover clients handle potential errors correctly, especially in production builds. Currently, an internal setup error could result in unhandled panics in downstream usage.
168-173: Initialization looks clean and consistent.
Storing both base and recursive keys and clients in theProverstruct is straightforward and consistent. No further suggestions at this time.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/node_types/lightclient/src/lightclient.rs (4)
38-43: Consider derivingSerializeas well.
If you intend to log or storeVerificationKeysin the future, derivingSerializecan help streamline that process.
44-51: Check error handling strategy.
Currently, any deserialization error inadvertently bubbles up. Depending on your environment, consider conforming to a graceful fallback or a more descriptive error message if loading the JSON fails.
59-60: Naming alignment consideration.
You may renamesp1_vkeysto reflect that it stores both base and recursive verifying keys, for example,sp1_verifying_keys.
79-79: Avoid usingexpectin production-critical paths.
A failing JSON load will panic the entire client. Using a recoverable error or graceful fallback is often better in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/node_types/lightclient/src/lightclient.rs(8 hunks)crates/node_types/prover/src/prover/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
crates/node_types/lightclient/src/lightclient.rs (6)
6-6: No issues identified.
82-82: Looks good.
101-101: Initialization flag is well-placed.
184-184: Validation logic is correct.
Checking the public values length ensures safe slicing for commitments.
192-192: Commitment extraction verified.
279-289: Confirm that the included Groth16 VK bytes match the base vs. recursive verification key.
Usingsp1_verifier::GROTH16_VK_BYTESas a universal parameter might be intentional. If separate parameters are ever needed for base and recursive keys, ensure that this approach remains valid.crates/node_types/prover/src/prover/mod.rs (8)
35-38: Inconsistency with AI-generated summary.
The code referencessp1_sdk, yet the summary claimed its removal. Ensure the final intent aligns with the actual usage ofsp1_sdk.Likely an incorrect or invalid review comment.
40-42: Check that both constants refer to the same ELF intentionally.
Currently,BASE_PRISM_ELFandRECURSIVE_PRISM_ELFboth point to the same file. If different binaries are intended, update accordingly.
122-129: New fields look consistent.
149-159: Separate prover clients for base and recursive proofs are well-defined.
161-172: Key setup sequence is clear and correct.
360-389: Base vs. recursive verification logic is well-structured.
442-470: Base proof generation flow looks good.
471-519: Recursive proof generation flow is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/node_types/lightclient/src/lightclient.rs (1)
79-79: 🛠️ Refactor suggestionConsider graceful error handling for missing verification keys.
The client currently panics if verification keys cannot be loaded, which could be disruptive at runtime.
Consider implementing a more graceful approach:
- let sp1_vkeys = load_sp1_verifying_keys().expect("Failed to load SP1 verifying keys"); + let sp1_vkeys = match load_sp1_verifying_keys() { + Ok(keys) => keys, + Err(e) => { + // Log the error + log::error!("Failed to load SP1 verifying keys: {}", e); + // Return a default implementation or propagate the error + panic!("Cannot proceed without verification keys: {}", e); + } + };Alternatively, consider making the constructor return a
Resultto propagate the error upwards.
🧹 Nitpick comments (1)
crates/node_types/lightclient/src/lightclient.rs (1)
44-50: Consider adding error context when deserializing embedded keys.The implementation uses
include_str!to embed the verification keys at compile time, which is appropriate for WASM compatibility. However, the error handling could be improved.Consider enhancing the error context to make debugging easier:
pub fn load_sp1_verifying_keys() -> Result<VerificationKeys> { - let keys: VerificationKeys = serde_json::from_str(EMBEDDED_KEYS_JSON)?; + let keys: VerificationKeys = serde_json::from_str(EMBEDDED_KEYS_JSON) + .map_err(|e| anyhow::anyhow!("Failed to parse verification keys JSON: {}", e))?; Ok(keys) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/node_types/lightclient/src/lightclient.rs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
crates/node_types/lightclient/src/lightclient.rs (8)
6-6: New dependency for JSON deserialization.The addition of the
serde::Deserializeimport supports the newVerificationKeysstruct deserialization from embedded JSON.
38-42: LGTM: Well-structured verification keys model.The
VerificationKeysstruct properly separates base and recursive verification keys, which aligns with the PR's objective of supporting recursive snarks.
60-60: Field updated to support multiple verification keys.The
sp1_vkeysfield now uses the newVerificationKeysstruct instead of a single verification key, which properly supports the distinction between base and recursive proofs.
101-102: Added flag to track initial backward search.The new flag properly tracks whether the initial backward search has been performed, preventing redundant searches.
114-157: Improved backward search algorithm with proper error handling.The code now searches backward from the network height to find the most recent epoch, processes it if found, and includes proper underflow protection to prevent infinite loops.
The backward search implementation correctly:
- Only runs once due to the
performed_initial_searchflag- Includes appropriate event notifications for verification status
- Handles potential underflow when
latest_epoch_heightis 0- Updates the current height after successful processing
This approach is more efficient than starting from the beginning and processing all epochs.
190-190: Simplified check for public values length.The check for public values length is now more straightforward, directly verifying if the length is less than the required 64 bytes.
211-214: Simplified SNARK proof verification call.The verification process now passes only the necessary parameters to the
verify_snark_proofmethod.
285-289: Added logic to select the appropriate verification key based on epoch height.The implementation now correctly selects the appropriate verification key based on the epoch height:
base_vkfor the genesis epoch (height 0)recursive_vkfor all subsequent epochsThis supports the PR objective of differentiating between base and recursive proofs.
sebasti810
force-pushed
the
recursive-snarks
branch
from
61fa549 to
d9b1057
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
justfile (1)
91-98: Leverage parallelizable build steps for efficiency
These lines build the base and recursive binaries sequentially. Consider splitting them into parallel tasks to shorten build times or to allow partial builds if one fails.crates/node_types/lightclient/src/lightclient.rs (1)
114-157: Backward search logic might benefit from refactoring
This block has several nested checks and break conditions. Extracting them into helper functions or clarifying the exit conditions will improve readability and maintainability.crates/node_types/prover/src/prover/mod.rs (1)
442-519: Refactor duplication inprove_epoch
The base and recursive proof branches share similar I/O logic. Consider abstracting out common functionality, such as signature insertion and proof validation, to reduce repetition and potential maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.gitignore(1 hunks)crates/cli/Cargo.toml(0 hunks)crates/cli/src/main.rs(0 hunks)crates/da/src/celestia/utils.rs(2 hunks)crates/da/src/lib.rs(2 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/events.rs(4 hunks)crates/node_types/lightclient/src/lightclient.rs(8 hunks)crates/node_types/prover/src/prover/mod.rs(5 hunks)crates/node_types/wasm-lightclient/src/worker.rs(0 hunks)crates/tests/Cargo.toml(0 hunks)crates/tests/src/lib.rs(0 hunks)crates/zk/sp1/Cargo.toml(1 hunks)crates/zk/sp1/src/bin/base_prover.rs(1 hunks)crates/zk/sp1/src/bin/recursive_prover.rs(1 hunks)justfile(1 hunks)verifying_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (5)
- crates/cli/Cargo.toml
- crates/tests/Cargo.toml
- crates/cli/src/main.rs
- crates/tests/src/lib.rs
- crates/node_types/wasm-lightclient/src/worker.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- .gitignore
- crates/node_types/lightclient/Cargo.toml
- crates/da/src/celestia/utils.rs
- verifying_keys/keys.json
- crates/da/src/lib.rs
- crates/zk/sp1/src/bin/recursive_prover.rs
- crates/zk/sp1/Cargo.toml
- crates/node_types/lightclient/src/events.rs
- crates/zk/sp1/src/bin/base_prover.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (15)
justfile (1)
99-105: Improve error handling for verification key generation
Currently, ifcargo prove vkeyfails or ifgrep '0x'unexpectedly returns nothing, the file might contain invalid JSON. Consider adding checks or conditional logic to ensure valid output before creatingkeys.json.crates/node_types/lightclient/src/lightclient.rs (9)
6-6: SerDe import looks good
Importingserde::Deserializeis necessary for the new JSON deserialization of embedded keys.
38-42: Struct for verification keys is straightforward
Defining a dedicated struct for base and recursive keys clarifies usage.
44-50: Ensure robust handling when loading embedded keys
If the keys file is missing, malformed, or has unexpected structure,serde_json::from_strwill error out. If you'd prefer more graceful failure, consider wrapping in a custom error message or fallback behavior.
59-59: Documentation is informative
The doc comment clarifies the source of these verification keys.
60-60: New fieldsp1_vkeysis well-integrated
Storing both base and recursive keys in a single struct is a clean approach.
79-79: Check whether panicking on key load is desired
Using.expect(...)terminates the process on error. If you need resiliency, consider returning a Result error and handling it gracefully at a higher level.
101-101:performed_initial_searchflag clarifies logic flow
Maintains state on whether the backward search has been attempted.
190-190: Validate the reason for enforcing 64-byte public values
Hardcoding a minimum of 64 bytes may cause edge-case failures if the final epoch proof changes in size. Verify you do not need a more robust check.
285-290: Dynamically selecting verification key is sensible
Using the base key for epoch 0 and the recursive key afterward aligns with the new design.crates/node_types/prover/src/prover/mod.rs (5)
40-42: Base and recursive ELF constants are valid placeholders
Embedding these ELF files ensures consistency between builds and runtime.
120-129: Introducing distinct fields for base and recursive proofs improves clarity
Separating these resources helps avoid confusion over which keys and clients to use in each scenario.
150-159: Separate initialization of base and recursive prover clients
Creating independent clients for each approach is a clear design choice that facilitates modular development.
161-163: Check for setup errors
Ifbase_prover_client.setuporrecursive_prover_client.setupfails or returns invalid keys, the build process might proceed silently. Consider verifying these returns for consistency before continuing.
361-372: Conditional logic for selecting the correct prover
Distinguishing the first epoch from subsequent ones is straightforward and aligns with the base/recursive design.
Summary by CodeRabbit
New Features
Chores / Refactor