feat: add pvss and pvss-cli rust crates with C0 [skip-line-limit]#1217
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a new PVSS (Publicly Verifiable Secret Sharing) crate and CLI tool with BFV circuit integration. It adds parameter type classification (THRESHOLD vs DKG) to fhe-params, implements a PK-BFV circuit with codegen and computation workflows, and provides a CLI for circuit discovery and artifact generation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as pvss-cli
participant Reg as Registry
participant Circuit as PkBfvCircuit
participant Comp as Computation
participant FS as FileSystem
User->>CLI: Run with circuit name & preset
CLI->>Reg: Initialize & register PkBfvCircuit
CLI->>Reg: Lookup circuit, validate parameter type
Reg-->>CLI: Return circuit metadata
CLI->>Circuit: Build BFV params from preset
Circuit->>Comp: Generate sample (public key)
Comp-->>Circuit: Return PublicKey
CLI->>Circuit: Call codegen(preset, public_key)
Circuit->>Comp: Compute Constants, Bounds
Comp-->>Circuit: Return derived values
Circuit->>Comp: Compute Bits from Bounds
Comp-->>Circuit: Return pk_bit
Circuit->>Comp: Compute Witness from PublicKey
Comp-->>Circuit: Return pk0is, pk1is
Circuit->>Comp: Reduce Witness to ZKP modulus
Comp-->>Circuit: Return reduced coefficients
Circuit->>Circuit: Generate TOML, template, configs, wrapper
Circuit-->>CLI: Return Artifacts
CLI->>FS: Write artifacts (toml, template, configs, wrapper)
FS-->>CLI: Confirmed
CLI-->>User: Success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0xjei
changed the title
0xjei
changed the title
0xjei
changed the title
![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
🤖 Fix all issues with AI agents
In `@crates/pvss/src/registry.rs`:
- Around line 19-23: The RegistryError::InvalidInput enum variant is defined but
unused; either remove the InvalidInput variant from the RegistryError definition
or annotate it with #[allow(dead_code)] and add a concise comment explaining its
intended future use (e.g., reserved for validation errors when registering
circuits) so linters/maintainers understand why it remains; update the
RegistryError enum accordingly and ensure no other code references InvalidInput
before deleting it.
🧹 Nitpick comments (2)
crates/pvss/src/registry.rs (2)
31-37: Consider implementingDefaulttrait.Since
new()returns an empty registry with no arguments, implementingDefaultwould be idiomatic and allow usage with..Default::default()patterns.♻️ Suggested implementation
+impl Default for CircuitRegistry { + fn default() -> Self { + Self::new() + } +} + impl CircuitRegistry { /// Build an empty registry. pub fn new() -> Self {
76-79: Note:list_circuits()returns names in arbitrary order.
HashMap::keys()iteration order is not guaranteed. If deterministic ordering is needed (e.g., for CLI output), consider sorting the result or usingBTreeMap.♻️ Optional: Return sorted names
/// List all registered circuit names. pub fn list_circuits(&self) -> Vec<String> { - self.circuits.keys().cloned().collect() + let mut names: Vec<String> = self.circuits.keys().cloned().collect(); + names.sort(); + names }
0xjei
changed the title
ctrlc03
force-pushed
the
feat/codegen-compute
branch
from
2b3c657 to
3256e18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/pvss/src/pk_bfv/codegen.rs`:
- Around line 68-82: The template currently defines the Noir main inputs pk0is
and pk1is as private; update the generated template in generate_template so the
function signature exposes these commitment parameters as public inputs (make
pk0is and pk1is pub) so the verifier can enforce them; locate generate_template
in pk_bfv/codegen.rs and change the main signature that declares pk0is and pk1is
(types [Polynomial<N>; L]) to mark both as pub, leaving the rest (BfvPkCommit
usage and PkBfvCircuit::PREFIX references) unchanged.
🧹 Nitpick comments (3)
crates/fhe-params/src/presets.rs (1)
60-67: Consider using PascalCase for enum variants.Rust convention uses PascalCase for enum variants (
Threshold,Dkg) rather than SCREAMING_CASE, which is typically reserved for constants. This would be more consistent with the rest of the codebase (e.g.,BfvPreset::InsecureThresholdBfv512).♻️ Suggested change
pub enum ParameterType { /// Threshold BFV (TRBFV) parameters - THRESHOLD, + Threshold, /// DKG parameters (BFV) - DKG, + Dkg, }Note: This would require updating usages in
metadata()and any consumers.crates/pvss-cli/src/main.rs (1)
95-108: Redundant parameter building and manual dispatch pattern.Two observations:
Duplicate work:
paramsis built on line 96, butcodegen()internally callsBfvParamSet::from(preset).build_arc()again (seecrates/pvss/src/pk_bfv/codegen.rs:44). Thesamplegenerated here uses the firstparams, but codegen builds its own.Manual dispatch: The match on
circuit_namewill require updates for every new circuit. Consider having circuits implement a trait that enables dynamic dispatch for codegen, or use a different pattern.♻️ Suggested fix for duplicate params
Either remove the redundant
paramsbuild here and pass only thepresetto the sample generation, or refactorcodegen()to accept pre-built params:// Generate artifacts based on circuit name from registry. - let params = BfvParamSet::from(preset).build_arc(); - let sample = sample::generate_sample(¶ms); let circuit_name = circuit_meta.name(); let artifacts = match circuit_name { name if name == <PkBfvCircuit as Circuit>::NAME => { + let params = BfvParamSet::from(preset).build_arc(); + let sample = sample::generate_sample(¶ms); let circuit = PkBfvCircuit; circuit.codegen(PkBfvCodegenInput { preset, public_key: sample.public_key, })? } name => return Err(anyhow!("circuit {} not yet implemented", name)), };This moves sample generation inside the match, ensuring params are built only once per circuit path.
crates/pvss/src/pk_bfv/circuit.rs (1)
55-69: Computation logic duplicated withcodegen().
CircuitComputation::compute()performs the sameBounds::compute(),Bits::compute(), andWitness::compute()calls ascodegen::codegen()(seecrates/pvss/src/pk_bfv/codegen.rs:46-48). Consider havingcodegen()internally callcompute()to centralize the computation logic.♻️ Suggested refactor in codegen.rs
pub fn codegen(preset: BfvPreset, public_key: PublicKey) -> Result<Artifacts, CodegenError> { let params = BfvParamSet::from(preset).build_arc(); // Reuse CircuitComputation let circuit = PkBfvCircuit; let output = circuit.compute(¶ms, &public_key)?; let zkp_witness = output.witness.reduce_to_zkp_modulus(); let toml = generate_toml(zkp_witness)?; let configs = generate_configs(¶ms, &output.bits); // ... rest of artifact generation }
0xjei
force-pushed
the
feat/codegen-compute
branch
from
3256e18 to
97c10fe
Compare
cedoor
left a comment
cedoor
left a comment
There was a problem hiding this comment.
Checked the code together, LGTM now!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/Dockerfile (1)
88-98: Addpvss-clito the binary crate condition in the stub file generation.The Dockerfile script on line 90 only creates
main.rsfor./cli/and./enclaveup/, butpvss-cliis also a binary crate withsrc/main.rs. The script will instead createsrc/lib.rsfor it, breaking the dependency-caching build on line 100.Add
pvss-clito the condition:- if [ "$d" = "./cli/" ] || [ "$d" = "./enclaveup/" ]; then \ + if [ "$d" = "./cli/" ] || [ "$d" = "./enclaveup/" ] || [ "$d" = "./pvss-cli/" ]; then \
🧹 Nitpick comments (7)
crates/fhe-params/src/presets.rs (1)
60-67: Consider using PascalCase for enum variants.Rust convention for enum variants is PascalCase rather than SCREAMING_CASE (which is typically reserved for constants). Consider renaming to
ThresholdandDkgfor idiomatic Rust.♻️ Suggested change
/// Parameter type for BFV presets #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum ParameterType { /// Threshold BFV (TRBFV) parameters - THRESHOLD, + Threshold, /// DKG parameters (BFV) - DKG, + Dkg, }crates/Dockerfile (1)
83-84: Consider maintaining alphabetical order for consistency.The existing Cargo.toml COPY instructions appear to be ordered alphabetically. These new entries would fit better after
polynomialand beforeprogram-server.♻️ Suggested reordering
Move lines 83-84 to after line 70 (polynomial/benches):
COPY crates/polynomial/Cargo.toml ./polynomial/Cargo.toml COPY crates/polynomial/benches ./polynomial/benches +COPY crates/pvss/Cargo.toml ./pvss/Cargo.toml +COPY crates/pvss-cli/Cargo.toml ./pvss-cli/Cargo.toml COPY crates/program-server/Cargo.toml ./program-server/Cargo.tomlAnd remove from the current location at lines 83-84.
crates/pvss/src/types.rs (2)
38-46: Consider adding accessors forCiphernodesCommitteefields.The struct fields
n,h, andthresholdare private, but there are no getter methods. If external code needs to read these values, accessors would be required.♻️ Optional: Add getter methods
#[derive(Debug, Clone, PartialEq, Eq)] pub struct CiphernodesCommittee { /// Total number of parties (N_PARTIES). n: usize, /// Number of honest parties (H). h: usize, /// Threshold value (T). threshold: usize, } + +impl CiphernodesCommittee { + pub fn n(&self) -> usize { self.n } + pub fn h(&self) -> usize { self.h } + pub fn threshold(&self) -> usize { self.threshold } +}
73-86: Consider using PascalCase for enum variants.Rust convention uses PascalCase for enum variants (
Insecure,Production) rather than SCREAMING_CASE (INSECURE,PRODUCTION).♻️ Optional: Use idiomatic naming
#[derive(Debug, Clone, Copy)] pub enum SecurityLevel { - INSECURE, - PRODUCTION, + Insecure, + Production, } impl SecurityLevel { pub fn as_str(self) -> &'static str { match self { - SecurityLevel::INSECURE => "insecure", - SecurityLevel::PRODUCTION => "production", + SecurityLevel::Insecure => "insecure", + SecurityLevel::Production => "production", } } }crates/pvss/src/utils.rs (1)
14-23: Prefer slice parameter over&Vec.Using
&[Vec<BigInt>]instead of&Vec<Vec<BigInt>>is more idiomatic and allows callers to pass arrays, slices, or vectors.♻️ More flexible parameter type
-pub fn map_witness_2d_vector_to_json(values: &Vec<Vec<BigInt>>) -> Vec<serde_json::Value> { +pub fn map_witness_2d_vector_to_json(values: &[Vec<BigInt>]) -> Vec<serde_json::Value> {crates/pvss/src/traits.rs (1)
13-19: Clarify the relationship betweenComputationandCircuitComputation.There are two similar traits:
Computation(standalone, static method) andCircuitComputation(extendsCircuit, instance method). Consider documenting when each should be used, or consolidating if they serve the same purpose.crates/pvss/src/circuits/pk_bfv/computation.rs (1)
160-167: Consider cachingget_zkp_modulus()to avoid duplicate calls.Minor optimization:
get_zkp_modulus()is called twice. If this involves computation, storing the result locally would be slightly more efficient.♻️ Optional refactor
impl ReduceToZkpModulus for Witness { fn reduce_to_zkp_modulus(&self) -> Witness { + let zkp_mod = get_zkp_modulus(); Witness { - pk0is: reduce_coefficients_2d(&self.pk0is, &get_zkp_modulus()), - pk1is: reduce_coefficients_2d(&self.pk1is, &get_zkp_modulus()), + pk0is: reduce_coefficients_2d(&self.pk0is, &zkp_mod), + pk1is: reduce_coefficients_2d(&self.pk1is, &zkp_mod), } } }
3 participants
This PR will include all the necessary changes & refactoring for PVSS rust (codegen & compute = old zkfhe-generator).
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.