Move Compute provider package from CRISP to Enclave #186
Conversation
WalkthroughThis pull request introduces several changes to the Changes
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: 9
🧹 Outside diff range and nitpick comments (21)
packages/compute_provider/src/ciphertext_output.rs (1)
1-18: Consider additional security measures for Enclave environmentSince this code is being moved to an Enclave environment, consider implementing:
- Constant-time operations for hash comparisons to prevent timing attacks
- Memory zeroization for sensitive data
- Audit logging for proof generation attempts
Would you like assistance in implementing these security measures?
packages/compute_provider/Readme.md (5)
3-3: Fix the markdown link and enhance the introductionThe link to the Enclave Protocol is not properly formatted. Additionally, consider adding a brief explanation of FHE for newcomers.
-This project provides a flexible and efficient framework for managing Secure Programs (SP) of the [Enclave Protocol](enclave.gg). It supports both sequential and parallel processing, with the ability to integrate various compute providers. +This project provides a flexible and efficient framework for managing Secure Programs (SP) of the [Enclave Protocol](https://enclave.gg). It supports both sequential and parallel processing of Fully Homomorphic Encryption (FHE) computations, enabling secure data processing while maintaining privacy. The framework is designed to integrate various compute providers.
16-19: Enhance dependency specificationConsider adding version constraints and improving the repository specification for better dependency management.
[dependencies] -compute-provider = { git = "https://github.com/gnosisguild/enclave.git", path = "packages/compute_provider"} +compute-provider = { + git = "https://github.com/gnosisguild/enclave.git", + branch = "main", # or specify a version tag + path = "packages/compute_provider" +}
44-74: Add security considerations and implementation detailsThe Risc0 example should include:
- Security considerations for ZK proof generation
- Implementation details for the
provemethod- Error handling best practices
Consider adding a security considerations section and expanding the implementation details:
impl ComputeProvider for Risc0Provider { type Output = Risc0Output; fn prove(&self, input: &ComputeInput) -> Result<Self::Output> { // Set up the executor environment with security parameters let env = ExecutorEnv::builder() .add_input(&input.serialize()?) .build()?; // Generate the ZK proof let prover = default_prover(); let receipt = prover.prove_elf(env, VOTING_ELF)?; // Verify the proof locally before returning receipt.verify(VerifierContext::default())?; Ok(Risc0Output { result: deserialize_result(&receipt.journal)?, seal: receipt.seal }) } }
79-87: Enhance configuration documentationThe configuration section should include:
- Parameter type information
- Recommended values and constraints
- Performance implications of different configurations
Consider expanding the configuration section:
## Configuration The `ComputeManager::new()` function takes several parameters: - - `provider`: An instance of your compute provider (e.g., `Risc0Provider`) - - `fhe_inputs`: The FHE inputs for the computation - - `fhe_processor`: A function to process the FHE inputs - - `use_parallel`: A boolean indicating whether to use parallel processing - - `batch_size`: An optional batch size for parallel processing, must be a power of 2 +### Parameters + +- `provider: T where T: ComputeProvider` + - Your compute provider implementation (e.g., `Risc0Provider`) + - Must implement the `ComputeProvider` trait + +- `fhe_inputs: FHEInputs` + - The encrypted inputs for computation + - Must be properly formatted and encrypted + +- `fhe_processor: Fn(FHEInputs) -> Result<ComputeResult>` + - Function to process the FHE inputs + - Should handle errors appropriately + +- `use_parallel: bool` + - Enable parallel processing for better performance + - Recommended for large computations + +- `batch_size: Option<usize>` + - Must be a power of 2 when specified + - Recommended values: 32, 64, or 128 depending on input size + - Affects memory usage and processing speed🧰 Tools
🪛 LanguageTool
[uncategorized] ~83-~83: Loose punctuation mark.
Context: ... takes several parameters: -provider: An instance of your compute provider (e...(UNLIKELY_OPENING_PUNCTUATION)
87-87: Add error handling and troubleshooting sectionsConsider adding sections for:
- Common errors and their solutions
- Debugging tips
- Performance optimization guidelines
Would you like me to provide a template for these sections?
packages/compute_provider/src/compute_input.rs (4)
5-5: Consider using a trait instead of a function pointer forFHEProcessorDefining
FHEProcessoras a function pointer restricts flexibility. Using a trait bound allows for passing closures and other callable types, enhancing adaptability.Apply this change:
-pub type FHEProcessor = fn(&FHEInputs) -> Vec<u8>; +pub trait FHEProcessor { + fn process(&self, inputs: &FHEInputs) -> Vec<u8>; +}Update the
processmethod signature:- pub fn process(&self, fhe_processor: FHEProcessor) -> ComputeResult { + pub fn process<P: FHEProcessor>(&self, fhe_processor: &P) -> ComputeResult {And adjust the invocation accordingly:
- let processed_ciphertext = (fhe_processor)(&self.fhe_inputs); + let processed_ciphertext = fhe_processor.process(&self.fhe_inputs);
9-9: Define a dedicated struct for ciphertext pairs to improve clarityCurrently,
ciphertextsis defined asVec<(Vec<u8>, u64)>, which may reduce readability. Defining a struct for the ciphertext and its identifier enhances code clarity.Define a new struct:
pub struct CiphertextPair { pub data: Vec<u8>, pub id: u64, }Update the
FHEInputsstruct:- pub ciphertexts: Vec<(Vec<u8>, u64)>, + pub ciphertexts: Vec<CiphertextPair>,
35-40: Update return type toResultand handle all possible errorsWith the changes to error handling, ensure that the function returns a
Resultand that all code paths are covered.Modify the return statement:
- ComputeResult { + Ok(ComputeResult { ciphertext_hash: processed_hash, params_hash, merkle_root, - } + })
7-11: DerivePartialEqandEqfor structs where appropriateIf instances of
FHEInputsorComputeInputneed to be compared, derivingPartialEqandEqcan be beneficial.Add derives to the structs:
#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] +#[derive(PartialEq, Eq)] pub struct FHEInputs { /* ... */ } #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] +#[derive(PartialEq, Eq)] pub struct ComputeInput { /* ... */ }Also applies to: 13-18
packages/compute_provider/src/merkle_tree.rs (5)
10-12: Consider makingleaf_hashesa private field.Exposing
leaf_hashesas a public field allows external modification, which could compromise the integrity of theMerkleTree. Making it private and providing necessary accessor methods will enhance encapsulation and maintainability.
23-24: Remove unnecessarytrim_start_matches("0x")call.The
hex::encodefunction does not prepend a"0x"to the output. Therefore, callingtrim_start_matches("0x")is redundant and can be safely removed.Suggested change:
let hex_output = hex::encode(Keccak256::digest(&item.0)); -let sanitized_hex = hex_output.trim_start_matches("0x"); +let sanitized_hex = hex_output;
25-28: Optimize conversion fromBigUinttoFr.To improve performance and reduce unnecessary conversions, you can convert
BigUintdirectly toFrwithout converting to a string.Suggested change:
-let numeric_value = BigUint::from_str_radix(sanitized_hex, 16) - .unwrap() - .to_string(); -let fr_element = Fr::from_str(&numeric_value).unwrap(); +let numeric_value = BigUint::from_str_radix(sanitized_hex, 16).unwrap(); +let fr_element = Fr::from(numeric_value);
30-30: Instantiate Poseidon instance outside the loop to improve performance.Creating a new Poseidon instance in each iteration can be inefficient. Instantiate it once before the loop and reuse it for all items.
Suggested change:
+ let mut poseidon_instance = Poseidon::<Fr>::new_circom(2).unwrap(); for item in data { // Existing code inside the loop - let mut poseidon_instance = Poseidon::<Fr>::new_circom(2).unwrap();
45-45: Remove unnecessarytrim_start_matches("0x")call.Since the
hex::encodefunction does not add a"0x"prefix, trimming it is unnecessary.Suggested change:
-let sanitized_node = node.trim_start_matches("0x"); +let sanitized_node = node;packages/compute_provider/src/compute_manager.rs (6)
111-111: Use explicit integer suffixes instead of type castingIn the mapping of results, you use
0 as u64, which involves a type cast.For clarity and to avoid unnecessary casting, use an explicit integer suffix:
.map(|result| (result.1.clone(), 0 as u64)) // The index is not used for the final computation in the parallel case +.map(|result| (result.1.clone(), 0u64)) // The index is not used for the final computation in the parallel case
85-85: Avoid unnecessary cloning of chunks in parallel processingIn the
start_parallelmethod, you clone each chunk when creatingfhe_inputs, which can be inefficient for large datasets.Consider borrowing the chunk data instead of cloning:
let fhe_inputs = FHEInputs { - ciphertexts: chunk.clone(), + ciphertexts: chunk, params: params.to_vec(), };You may need to adjust the definitions and lifetimes to accommodate borrowed data.
69-70: Evaluate the necessity of cloning and usingArcfor shared dataYou clone
ciphertextsandparamsand wrap them inArc, which might introduce unnecessary overhead.If
ciphertextsandparamsare not modified within the parallel iterator, consider removing the clones:let ciphertexts = Arc::new(self.input.fhe_inputs.ciphertexts.clone()); let params = Arc::new(self.input.fhe_inputs.params.clone()); +// If possible, avoid cloning: +let ciphertexts = Arc::new(self.input.fhe_inputs.ciphertexts); +let params = Arc::new(self.input.fhe_inputs.params.clone());Ensure that the data is not mutated to maintain thread safety.
28-29: Validate the default batch size for parallel executionThe default
batch_sizeis hardcoded as2when not provided, which may not be optimal.Consider making the default batch size configurable or calculating it based on input size:
let batch_size = self.batch_size.unwrap_or(2); +// Alternatively, set a more suitable default or calculate dynamically +let batch_size = self.batch_size.unwrap_or_else(|| calculate_optimal_batch_size());
5-5: Confirm that therayonprelude is necessaryYou include
use rayon::prelude::*;, which imports a lot into the namespace.To keep the namespace clean, consider importing only what's needed:
-use rayon::prelude::*; +use rayon::iter::ParallelIterator; +use rayon::prelude::IntoParallelIterator;
61-61: Ensure hash function meets security requirementsYou use
Keccak256for hashing, but depending on security requirements,SHA3-256might be preferred.Consider using
sha3::Sha3_256if compliance with the SHA3 standard is required:-use sha3::{Digest, Keccak256}; +use sha3::{Digest, Sha3_256}; ... - self.input.ciphertext_hash = Keccak256::digest(&ciphertext).to_vec(); + self.input.ciphertext_hash = Sha3_256::digest(&ciphertext).to_vec();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/compute_provider/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/compute_provider/.gitignore(1 hunks)packages/compute_provider/Cargo.toml(1 hunks)packages/compute_provider/Readme.md(1 hunks)packages/compute_provider/src/ciphertext_output.rs(1 hunks)packages/compute_provider/src/compute_input.rs(1 hunks)packages/compute_provider/src/compute_manager.rs(1 hunks)packages/compute_provider/src/lib.rs(1 hunks)packages/compute_provider/src/merkle_tree.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/compute_provider/.gitignore
- packages/compute_provider/Cargo.toml
🧰 Additional context used
🪛 LanguageTool
packages/compute_provider/Readme.md
[uncategorized] ~83-~83: Loose punctuation mark.
Context: ... takes several parameters: - provider: An instance of your compute provider (e...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (6)
packages/compute_provider/src/lib.rs (2)
1-4: LGTM! Well-organized module structure.
The module organization logically separates concerns for FHE computation functionality, with clear separation between input handling, computation management, merkle tree operations, and output processing.
6-8: Consider using explicit exports instead of wildcards.
Using wildcard exports (*) can lead to namespace pollution and make it harder to track dependencies. Consider explicitly listing the types, traits, and functions you want to expose as public API.
Example refactor:
-pub use compute_manager::*;
-pub use compute_input::*;
-pub use ciphertext_output::*;
+pub use compute_manager::{ComputeManager, /* other specific items */};
+pub use compute_input::{ComputeInput, FHEInputs, /* other specific items */};
+pub use ciphertext_output::{ComputeProvider, ComputeResult, /* other specific items */};Let's verify what public items are available in these modules:
✅ Verification successful
Let me try a different approach to find the public items in these modules.
Consider using explicit exports instead of wildcards
Based on the search results, here's the recommended explicit export list:
pub use compute_manager::ComputeManager;
pub use compute_input::{FHEProcessor, FHEInputs, ComputeInput};
pub use ciphertext_output::{ComputeProvider, ComputeResult};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find public items in the modules that are being exported
# This helps identify what specific items should be explicitly exported
echo "Searching for public items in compute_manager..."
ast-grep --pattern 'pub $type $name' packages/compute_provider/src/compute_manager.rs
echo "Searching for public items in compute_input..."
ast-grep --pattern 'pub $type $name' packages/compute_provider/src/compute_input.rs
echo "Searching for public items in ciphertext_output..."
ast-grep --pattern 'pub $type $name' packages/compute_provider/src/ciphertext_output.rs
Length of output: 584
Script:
#!/bin/bash
# Let's try using ripgrep to find public declarations
echo "Searching in compute_manager.rs..."
rg "^pub\s+(struct|enum|type|trait|fn|const|static)" packages/compute_provider/src/compute_manager.rs -A 1
echo -e "\nSearching in compute_input.rs..."
rg "^pub\s+(struct|enum|type|trait|fn|const|static)" packages/compute_provider/src/compute_input.rs -A 1
echo -e "\nSearching in ciphertext_output.rs..."
rg "^pub\s+(struct|enum|type|trait|fn|const|static)" packages/compute_provider/src/ciphertext_output.rs -A 1
Length of output: 912
packages/compute_provider/src/ciphertext_output.rs (1)
1-1: LGTM!
The import is correctly scoped and necessary for the trait definition.
packages/compute_provider/src/compute_input.rs (1)
3-3: Verify the appropriateness of using Keccak256
Ensure that using Keccak256 aligns with the project's cryptographic standards and interoperability requirements.
Run the following script to check for existing cryptographic standards in the codebase:
✅ Verification successful
Let me generate another script to get a clearer picture of how Keccak256 is being used in the codebase.
This will help us understand:
- Where Keccak256 is being used
- What other hash functions are in use
- If there's any documentation about cryptographic standards
Let me check the actual usage context of Keccak256 in the relevant files to understand its purpose.
Based on the gathered information, I can now provide a final response about the appropriateness of using Keccak256.
Using Keccak256 is appropriate and aligned with the project's cryptographic standards
The usage of Keccak256 in this codebase is appropriate for several reasons:
- It's used consistently for hashing ciphertexts and parameters in the compute provider implementation
- It's complemented by Poseidon hash (PoseidonT3) in the smart contracts for on-chain operations
- The implementation follows a clear pattern where:
- Keccak256 is used for off-chain cryptographic operations and data integrity verification
- Poseidon is used for on-chain operations where gas efficiency is important
The codebase demonstrates a well-thought-out hybrid approach using both Keccak256 and Poseidon hash functions in their appropriate contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for hash functions used in the codebase to ensure consistency.
# Search for imports of hash functions
rg 'use .*::{.*};' | rg 'sha|blake|keccak'
# List all instances where hashing is performed
rg -A 2 'digest\(|hash\('
Length of output: 143738
Script:
#!/bin/bash
# Search for Keccak256 imports and usages
rg -l "Keccak256"
# Search for other hash function imports
rg -l "use.*hash|use.*digest"
# Search for specific files that might contain cryptographic standards or documentation
fd -t f "README|CRYPTO|SECURITY|spec"
Length of output: 1286
Script:
#!/bin/bash
# Check the context of Keccak256 usage in compute-related files
rg -B2 -A2 "Keccak256" packages/compute_provider/src/
# Check README files for any cryptographic standards documentation
cat README.md packages/*/README.md 2>/dev/null
# Look for any cryptographic specifications in test files
rg -B2 -A2 "hash|digest|keccak" packages/evm/test/
Length of output: 17217
packages/compute_provider/src/compute_manager.rs (2)
119-123: Ensure consistency in the final_input construction
In constructing final_input, confirm that the leaf_hashes are correctly aggregated and that all necessary data is included.
Run the following script to check for consistency in ComputeInput usage:
✅ Verification successful
All ComputeInput constructions are consistent and include required fields
The verification shows that all instances of ComputeInput construction in the codebase consistently include the three required fields: fhe_inputs, ciphertext_hash, and leaf_hashes. The struct definition in compute_input.rs matches these fields, and all initializations follow the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all instances of `ComputeInput` include necessary fields.
# Expected: All `ComputeInput` initializations include `fhe_inputs`, `ciphertext_hash`, and `leaf_hashes`.
rg -A 5 'ComputeInput \{' --glob '**/*.rs'
Length of output: 2503
89-89: Handle potential errors from the FHE processor
The call to the FHEProcessor might result in errors that are not currently handled.
Verify whether the FHEProcessor can fail and, if so, handle the potential errors appropriately. Consider modifying the function signature to return a Result:
-fn start_parallel(&self) -> (P::Output, Vec<u8>) {
+fn start_parallel(&self) -> Result<(P::Output, Vec<u8>), YourErrorType> {Run the following script to check for error handling in FHEProcessor implementations:
Summary by CodeRabbit
Release Notes
New Features
ComputeManagerstruct for managing computations with a specified provider.MerkleTreestruct for managing and computing hashes.Documentation
Readme.mdwith installation instructions and usage guidelines for the "FHE Compute Manager."Bug Fixes