feat: removing sp1-sdk dependency for lightclient #193
Conversation
WalkthroughThis pull request focuses on updating the dependency management for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/node_types/lightclient/src/lightclient.rs (1)
87-87: Avoid unnecessary conversion with.to_vec()when slicingCalling
.to_vec()onpublic_valuescreates an unnecessary allocation. Sincepublic_valuesis already a slice, you can slice it directly to improve performance.Apply this diff to optimize the slicing:
let mut slice = [0u8; 32]; - slice.copy_from_slice(&public_values.to_vec()[32..64]); + slice.copy_from_slice(&public_values.as_slice()[32..64]); let proof_current_commitment: Digest = Digest::from(slice);crates/cli/src/main.rs (1)
51-56: Consider extracting mock setup to a separate functionThe LightClient initialization with mock setup could be moved to a separate function for better maintainability and testing.
+fn setup_mock_light_client( + da: impl DataAvailabilityLayer, + celestia_config: CelestiaConfig, + prover_vk: Option<Ed25519PublicKey>, +) -> std::io::Result<LightClient> { + let client = ProverClient::mock(); + let (_, vk) = client.setup(PRISM_ELF).map_err(|e| + std::io::Error::new(std::io::ErrorKind::Other, format!("Failed to setup ProverClient: {}", e)) + )?; + Ok(LightClient::new( + da, + celestia_config, + prover_vk, + vk.bytes32(), + )) +}
📜 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 (7)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(2 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/lightclient.rs(4 hunks)crates/tests/Cargo.toml(1 hunks)crates/tests/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: unused dependencies
🔇 Additional comments (9)
crates/node_types/lightclient/src/lightclient.rs (1)
Line range hint
15-29: Integration ofvkey_bytesfield is correctly implementedThe addition of the
vkey_bytesfield to theLightClientstruct and its inclusion in the constructor parameters are properly handled. This change streamlines the verification key management within the light client.crates/tests/src/lib.rs (4)
17-17: Addingsp1_sdkimports forProverClientusageThe addition of
sp1_sdk::{HashableKey, ProverClient}is necessary for creating a mock prover client and aligns with the updates in the test setup.
23-24: DefiningPRISM_ELFconstant for ELF bytes inclusionIntroducing the
PRISM_ELFconstant allows the test to access the ELF bytes required for the prover setup, ensuring the test environment is correctly configured.
39-41: Initializing mockProverClientand generating verification keyThe use of
ProverClient::mock()and obtaining the verification key withsetup(PRISM_ELF)is appropriate for test purposes, facilitating the interaction between the prover and the light client in the test environment.
69-74: UpdatingLightClient::newto includevk.bytes32()Passing
vk.bytes32()to theLightClient::newfunction aligns with the updated constructor and ensures the light client has the necessary verification key bytes for proof verification.crates/node_types/lightclient/Cargo.toml (1)
24-24: Addingsp1-verifierdependencyIncluding
sp1-verifierin the dependencies is essential for the light client's proof verification functionality using theGroth16Verifier. This addition is correctly specified under[dependencies].crates/tests/Cargo.toml (1)
24-25: Verify the necessity of sp1-sdk dependencyThe addition of
sp1-sdkdependency seems to contradict the PR objective of "removing sp1-sdk dependency for lightclient". Please clarify if this dependency is still required for testing purposes.Run this script to check sp1-sdk usage in tests:
✅ Verification successful
sp1-sdk dependency is justified for tests
The
sp1-sdkdependency is used in tests for itsHashableKeyandProverClienttraits, which is appropriate for testing purposes. This test-only dependency doesn't contradict the PR's objective of removing sp1-sdk from the main lightclient implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check sp1-sdk usage in test files rg -l "sp1_sdk" "crates/tests"Length of output: 52
Script:
#!/bin/bash # Description: Check how sp1-sdk is used in lib.rs rg "sp1_sdk" "crates/tests/src/lib.rs" -A 5 -B 5Length of output: 337
crates/cli/Cargo.toml (1)
31-31: Verify sp1-sdk dependency requirementThe addition of
sp1-sdkdependency here also seems to contradict the PR objective. Please clarify if this is a temporary requirement during the transition phase.Run this script to check sp1-sdk usage in CLI:
✅ Verification successful
Dependency verified as transitional requirement
The
sp1-sdkdependency is being used for mock testing purposes throughProverClient::mock(). This appears to be a necessary transitional dependency while the codebase moves towards using prism components. Consider tracking this dependency for future cleanup once the transition is complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check sp1-sdk usage in CLI code rg -l "sp1_sdk" "crates/cli/src"Length of output: 53
Script:
#!/bin/bash # Check actual sp1-sdk usage in main.rs rg "sp1_sdk" "crates/cli/src/main.rs" -B 2 -A 2Length of output: 170
Script:
#!/bin/bash # Check usage of HashableKey and ProverClient in main.rs rg "HashableKey|ProverClient" "crates/cli/src/main.rs" -B 2 -A 2Length of output: 307
Cargo.toml (1)
87-89: LGTM! Version updates are consistentThe version updates for sp1-related dependencies are consistent. The addition of sp1-verifier with disabled default features suggests a more focused approach to verification functionality.
Run this script to check for any version conflicts:
✅ Verification successful
✅ Dependencies are consistently versioned across workspace
All sp1-related dependencies are properly inherited from the workspace root, which defines consistent version 3.2.1 for sp1-zkvm, sp1-sdk, and sp1-verifier. No version conflicts detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version conflicts in sp1-related dependencies cargo tree | grep -E "sp1-(zkvm|sdk|verifier)"Length of output: 144
Script:
#!/bin/bash # Find all Cargo.toml files and search for sp1-related dependencies fd Cargo.toml --type f --exec sh -c 'echo "=== {} ==="; cat {} | grep -A 1 -B 1 "sp1-"'Length of output: 1640
distractedm1nd
force-pushed
the
fix-lc-verification
branch
from
a618994 to
71fe9e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/node_types/lightclient/src/lightclient.rs (3)
17-18: Enhance documentation for sp1_vkey fieldThe current documentation could be more detailed. Consider adding information about the format, source, and usage of the verification key.
Add more context to the documentation:
- /// The verification key for the SP1 program, generated by `vk.bytes32()`. + /// The verification key for the SP1 program in hex format, used for verifying zkSNARK proofs. + /// This key is generated by `vk.bytes32()` during the program setup phase and must match + /// the proving key used by the prover.
87-89: Improve error message for length validationThe panic message could be more descriptive about the expected format.
- if public_values.as_slice().len() < 64 { - panic!("public_values length is less than 64 bytes in epoch {}", finalized_epoch.height); + if public_values.len() < 64 { + panic!( + "Invalid public values format in epoch {}: expected at least 64 bytes for commitments (got {} bytes)", + finalized_epoch.height, + public_values.len() + );
95-95: Avoid unnecessary allocation with to_vec()Using
to_vec()creates an unnecessary allocation whenas_slice()would suffice.- slice.copy_from_slice(&public_values.to_vec()[32..64]); + slice.copy_from_slice(&public_values.as_slice()[32..64]);
📜 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 (7)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(2 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/lightclient.rs(4 hunks)crates/tests/Cargo.toml(1 hunks)crates/tests/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cli/Cargo.toml
- crates/node_types/lightclient/Cargo.toml
- Cargo.toml
- crates/tests/Cargo.toml
- crates/tests/src/lib.rs
- crates/cli/src/main.rs
🧰 Additional context used
📓 Learnings (1)
crates/node_types/lightclient/src/lightclient.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: unit-test
🔇 Additional comments (2)
crates/node_types/lightclient/src/lightclient.rs (2)
28-36: LGTM! Constructor changes align with the new verification approachThe constructor properly initializes the LightClient with the new sp1_vkey parameter.
72-78: LGTM! Appropriate error handling for signature verificationThe panic on signature verification failure is intentional and correct as it prevents the client from proceeding with potentially compromised data.
distractedm1nd
force-pushed
the
fix-lc-verification
branch
from
66ebbb6 to
b779a31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/node_types/lightclient/src/lightclient.rs (3)
17-18: Enhance documentation for sp1_vkey fieldConsider adding more details about the verification key's format, generation process, and its role in proof verification.
- /// The verification key for the SP1 program, generated by `vk.bytes32()`. + /// The verification key for the SP1 program, generated by `vk.bytes32()`. + /// This key is used to verify zkSNARK proofs submitted by the prover. + /// Format: Base64-encoded string of 32 bytes.
87-89: Enhance panic message for public values length checkThe panic message should include the actual length for better debugging.
- panic!("public_values length is less than 64 bytes in epoch {}", finalized_epoch.height); + panic!( + "public_values length {} is less than required 64 bytes in epoch {}", + public_values.as_slice().len(), + finalized_epoch.height + );
95-95: Remove unnecessary allocation in slice operationUsing
to_vec()creates an unnecessary allocation. Useas_slice()instead.- slice.copy_from_slice(&public_values.to_vec()[32..64]); + slice.copy_from_slice(&public_values.as_slice()[32..64]);crates/tests/src/lib.rs (1)
39-41: Consider adding error handling in test setupWhile this is test code, adding error handling would make the tests more robust and provide better failure messages.
- let prover_client = ProverClient::mock(); - let (_, vk) = prover_client.setup(PRISM_ELF); + let prover_client = ProverClient::mock(); + let (_, vk) = prover_client.setup(PRISM_ELF) + .expect("Failed to setup mock ProverClient");
📜 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 (7)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(4 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/lightclient.rs(4 hunks)crates/tests/Cargo.toml(1 hunks)crates/tests/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/node_types/lightclient/Cargo.toml
- crates/cli/Cargo.toml
- Cargo.toml
- crates/tests/Cargo.toml
🧰 Additional context used
📓 Learnings (1)
crates/node_types/lightclient/src/lightclient.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
🔇 Additional comments (7)
crates/node_types/lightclient/src/lightclient.rs (3)
Line range hint
25-37: LGTM: Constructor properly initialized with new sp1_vkey fieldThe constructor correctly handles the new verification key parameter.
71-78: LGTM: Appropriate security measure for signature verificationThe panic on invalid signatures is a correct security measure to prevent processing of potentially compromised data.
114-119:⚠️ Potential issueFix undefined field reference in SNARK verification
The code references
self.vkey_byteswhich doesn't exist. It should useself.sp1_vkey.match Groth16Verifier::verify( &finalized_epoch.proof.bytes(), public_values.as_slice(), - &self.vkey_bytes, + &self.sp1_vkey, &sp1_verifier::GROTH16_VK_BYTES, ) {Likely invalid or redundant comment.
crates/cli/src/main.rs (2)
51-53: Add error handling for ProverClient setupThe setup of ProverClient lacks error handling. Consider wrapping it in a Result to handle potential failures.
- let client = ProverClient::mock(); - let (_, vk) = client.setup(PRISM_ELF); + let client = ProverClient::mock(); + let (_, vk) = client.setup(PRISM_ELF).map_err(|e| + std::io::Error::new(std::io::ErrorKind::Other, format!("Failed to setup ProverClient: {}", e)) + )?;
54-59: LGTM: Correct instantiation of LightClient with verification keyThe LightClient is properly instantiated with all required parameters including the new verification key.
crates/tests/src/lib.rs (2)
17-17: LGTM: Proper import and constant declarationThe ProverClient import and PRISM_ELF constant are correctly defined.
Also applies to: 23-23
74-79: LGTM: Correct test setup for LightClientThe LightClient is properly instantiated in the test with all required parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/cli/src/main.rs (1)
55-60: Add documentation for the verification key parameter.The purpose of the
vk.bytes32()parameter should be documented for clarity.Arc::new(LightClient::new( da, celestia_config, Some(prover_vk), + // The verification key for the SP1 program, generated from the mock prover vk.bytes32(), ))crates/node_types/lightclient/src/lightclient.rs (2)
17-18: Enhance documentation for sp1_vkey field.The documentation could be more detailed about the source and format of the verification key.
- /// The verification key for the SP1 program, generated by `vk.bytes32()`. + /// The verification key for the SP1 program in bytes32 format. + /// This key is generated during prover setup and is used to verify zkSNARKs + /// in the Groth16 verification scheme. pub sp1_vkey: String,
87-89: Improve panic message for public_values length check.The panic message could be more informative about the expected length.
if public_values.as_slice().len() < 64 { - panic!("public_values length is less than 64 bytes in epoch {}", finalized_epoch.height); + panic!( + "Invalid public_values length in epoch {}: expected >= 64 bytes, got {} bytes", + finalized_epoch.height, + public_values.as_slice().len() + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/cli/src/main.rs(4 hunks)crates/node_types/lightclient/src/lightclient.rs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
crates/node_types/lightclient/src/lightclient.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
🔇 Additional comments (3)
crates/cli/src/main.rs (1)
52-54:⚠️ Potential issueAdd error handling and feature gate for mock prover setup.
The mock prover setup lacks error handling and should be feature-gated.
+ #[cfg(feature = "mock_prover")] let client = ProverClient::mock(); - let (_, vk) = client.setup(PRISM_ELF); + let (_, vk) = client.setup(PRISM_ELF) + .map_err(|e| Error::new(ErrorKind::Other, format!("Failed to setup mock prover: {}", e)))?;Likely invalid or redundant comment.
crates/node_types/lightclient/src/lightclient.rs (2)
111-113: LGTM! Feature gate for mock_prover is correctly implemented.The mock_prover feature gate is properly configured, addressing one of the PR objectives.
114-127: LGTM! SNARK verification is correctly implemented.The SNARK verification is properly implemented with appropriate panic on failure, which is the correct behavior as per the retrieved learning about handling verification failures.
distractedm1nd
force-pushed
the
fix-lc-verification
branch
from
00c6409 to
4b8b510
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/node_types/lightclient/src/lightclient.rs (2)
87-89: Enhance panic message for better debugging.While the length check is good, the panic message could be more descriptive about the actual length received.
- panic!("public_values length is less than 64 bytes in epoch {}", finalized_epoch.height); + panic!( + "public_values length {} is less than required 64 bytes in epoch {}", + public_values.as_slice().len(), + finalized_epoch.height + );
95-95: Avoid unnecessary allocation withto_vec().Using
to_vec()creates an unnecessary allocation. Useas_slice()instead, as the data is already available as a slice.- slice.copy_from_slice(&public_values.to_vec()[32..64]); + slice.copy_from_slice(&public_values.as_slice()[32..64]);
📜 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 (7)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(4 hunks)crates/node_types/lightclient/Cargo.toml(1 hunks)crates/node_types/lightclient/src/lightclient.rs(4 hunks)crates/tests/Cargo.toml(1 hunks)crates/tests/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cli/Cargo.toml
- crates/node_types/lightclient/Cargo.toml
- crates/cli/src/main.rs
- crates/tests/src/lib.rs
- Cargo.toml
- crates/tests/Cargo.toml
🧰 Additional context used
📓 Learnings (1)
crates/node_types/lightclient/src/lightclient.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#193
File: crates/node_types/lightclient/src/lightclient.rs:68-74
Timestamp: 2025-01-08T09:35:23.121Z
Learning: In the LightClient implementation, signature verification failures should trigger a panic as clients must not proceed with potentially compromised data. This is a security measure rather than an error handling concern.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: integration-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: unit-test
🔇 Additional comments (5)
crates/node_types/lightclient/src/lightclient.rs (5)
9-10: LGTM! Import changes align with PR objective.The switch to
sp1_verifier::Groth16Verifieraligns with the PR's goal of removing the sp1-sdk dependency.
15-18: LGTM! Well-documented struct field.The new
sp1_vkeyfield is properly documented with a clear description of its purpose and source.
29-36: LGTM! Constructor changes maintain struct invariants.The constructor properly initializes the new
sp1_vkeyfield while maintaining existing functionality.
72-78: LGTM! Maintains security requirements.The panic on signature verification failure is intentional and correct, as it prevents the client from proceeding with potentially compromised data.
111-127: LGTM! Proper feature flag and verification handling.The changes correctly:
- Handle mock_prover feature flag
- Use sp1-verifier for SNARK verification
- Maintain security by panicking on verification failure
Helps prepare for @sebasti810 's rebase for wasm-lc, but also fixes #181 by correctly setting the
mock_proverfeature.Summary by CodeRabbit
Release Notes
Dependencies
sp1-zkvmandsp1-sdkto version 3.2.1sp1-verifierdependencyprism-lightclientto include featuresLight Client
LightClientconfigurationTesting