feat: add pk_trfv_generation_circuit to zk-prover#1281
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors the threshold PK generation circuit infrastructure by: updating security parameters for insecure preset (increasing bit bounds for E_SM), reorganizing module structure (removing pkbfv, introducing dkg and threshold modules), extracting polynomial conversion utilities, implementing the Provable trait for PkGenerationCircuit, and adding comprehensive end-to-end tests for the threshold PK workflow. The CircuitName enum variant is renamed from PkTrbfv to PkGeneration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/circuits/utils.rs`:
- Around line 1-6: This file is missing the SPDX license header; add the
standard header comment "// SPDX-License-Identifier: LGPL-3.0-only" as the very
first line of the file (above the existing imports such as use
std::collections::BTreeMap; and the referenced symbols ZkError, FieldElement,
CrtPolynomial, Polynomial, InputValue) so the CI license-header check passes.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Line 186: Rename the test function to fix the typo in its name: change
test_pk_trbfv_committment_consistency to test_pk_trbfv_commitment_consistency
wherever it is declared and referenced (e.g., the async fn
test_pk_trbfv_committment_consistency declaration and any modulary/test harness
references); ensure tests still compile by updating any mentions in attributes,
mod maps, or test invocations that reference the old symbol.
🧹 Nitpick comments (4)
crates/zk-prover/src/circuits/utils.rs (1)
8-45: Utility functions look correct; minor DRY opportunity.Both
crt_polynomial_to_arrayandpolynomial_to_input_valueshare near-identical inner loops for converting coefficients toInputValue::Fieldvia a string round-trip. Consider extracting a small helper (e.g.,coefficients_to_fields) to reduce duplication. Not blocking.Sketch
+fn coefficients_to_fields(coeffs: &[impl ToString]) -> Result<Vec<InputValue>, ZkError> { + coeffs + .iter() + .map(|b| { + let s = b.to_string(); + FieldElement::try_from_str(&s) + .map(InputValue::Field) + .ok_or_else(|| ZkError::SerializationError(format!("invalid field element: {}", s))) + }) + .collect() +}crates/zk-prover/src/circuits/pk_generation.rs (2)
18-18: UseInputMap::new()instead ofBTreeMap::new()for consistency.
InputMapis already imported (line 17) and is used inpkbfv.rsviaInputMap::new(). UsingBTreeMap::new()here works (sinceInputMapis aBTreeMapalias) but is inconsistent and makes theBTreeMapimport unnecessary.Proposed fix
use noirc_abi::InputMap; -use std::collections::BTreeMap; impl Provable for PkGenerationCircuit {- let mut inputs = BTreeMap::new(); + let mut inputs = InputMap::new();Also applies to: 36-36
33-33: Needless double-borrow oninput.
inputis already&Self::Input(i.e.,&PkGenerationCircuitInput), so&inputcreates&&PkGenerationCircuitInput. Rust auto-derefs this, but it's a minor lint. Compare withpkbfv.rswhich avoids this.Proposed fix
- let witness = Witness::compute(preset.clone(), &input) + let witness = Witness::compute(preset.clone(), input)crates/zk-prover/tests/local_e2e_tests.rs (1)
81-130: Consider using descriptivee3_idvalues to match existing test conventions.All three new tests use
e3_id = "0", while the existing BFV tests use descriptive IDs like"test-pk-bfv-001","test-verify-001","test-commitment-001". If these tests ever run in parallel sharing state, collisions on"0"could cause interference.Also applies to: 132-183, 185-266
ctrlc03
force-pushed
the
feat/proof-1
branch
from
c32051b to
d83d805
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/events/src/enclave_event/proof.rs`:
- Around line 41-42: Update the stale doc comment for the enum variant
PkGeneration to match the renamed variant; replace "TrBFV public key share proof
(T1)" with a comment that describes PkGeneration (e.g., "Public key generation
proof (T1)" or similar clear wording) so the documentation aligns with the
PkGeneration variant name in enclave_event::proof.rs.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 110-112: The tests call CiphernodesCommitteeSize::Small.values()
but the type isn't imported into the test module; add a use statement to bring
CiphernodesCommitteeSize into scope (so the three uses in
PkGenerationCircuitInput::generate_sample(...) compile), e.g. add a `use` for
CiphernodesCommitteeSize at the top of the file or test module so the symbol is
available to the calls in the tests.
- Around line 14-21: Remove the duplicate PkCircuit import and correct the
module path for PkGenerationCircuit imports: replace the current use of
e3_zk_helpers::threshold::pk_generation with
e3_zk_helpers::circuits::threshold::pk_generation so PkGenerationCircuit and
PkGenerationCircuitInput are imported from the proper module; ensure you still
import PkCircuitInput and compute_* helpers (compute_dkg_pk_commitment,
compute_share_computation_e_sm_commitment,
compute_share_computation_sk_commitment, compute_threshold_pk_commitment) from
their correct modules, and add a new import use
e3_zk_helpers::CiphernodesCommitteeSize so the references to
CiphernodesCommitteeSize at lines 111, 162, and 215 resolve.
🧹 Nitpick comments (4)
crates/zk-prover/src/circuits/threshold/pk_generation.rs (2)
17-18: UnnecessaryBTreeMapimport; preferInputMap::new()for consistency.
InputMapis already imported on line 17 and is a type alias forBTreeMap<String, InputValue>. UsingInputMap::new()on line 36 (as done inpkbfv.rs) would be consistent and let you drop the explicitstd::collections::BTreeMapimport.Proposed fix
use noirc_abi::InputMap; -use std::collections::BTreeMap;- let mut inputs = BTreeMap::new(); + let mut inputs = InputMap::new();
33-33: Redundant double reference&input.
inputis already&Self::Input, so&inputcreates&&PkGenerationCircuitInput. While Rust auto-deref handles this, it's cleaner to passinputdirectly (matching the pattern inpkbfv.rs).Proposed fix
- let witness = Witness::compute(preset.clone(), &input) + let witness = Witness::compute(preset.clone(), input)crates/zk-prover/tests/local_e2e_tests.rs (2)
80-129: Significant test setup duplication across the three new tests.All three
test_pk_generation_*tests repeat identical setup code (~30 lines each): findingbb, setting up the prover, copying fixtures, creating the sample, etc. Consider extracting a shared helper (similar to how BFV tests could also benefit) to reduce duplication and maintenance burden.Also applies to: 131-182, 184-277
230-232: Magic number1027for total fields assertion.The hardcoded
1027(1024 pk fields + 3 commitments) is brittle and will silently break if circuit parameters change. Consider deriving this from the preset or documenting the formula explicitly with named constants.
ctrlc03
force-pushed
the
feat/proof-1
branch
from
5f8a402 to
2ef451a
Compare
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
ctrlc03
force-pushed
the
feat/proof-1
branch
from
1e6b62f to
a89bfa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Line 236: The inline comment on the calculation for offset is incorrect;
update the comment next to the expression that computes offset (the line using
variables offset, total_fields, and field_size) so it reflects the actual
arithmetic and result for (total_fields - 3) * field_size — e.g. compute (1027 -
3) * 32 = 32768 — or remove the stale multiplication example entirely to avoid
confusion.
🧹 Nitpick comments (3)
crates/zk-prover/src/circuits/utils.rs (1)
14-51: Consider extracting the shared coefficient-conversion loop.Lines 21–27 and 41–45 contain identical logic converting polynomial coefficients into
Vec<InputValue::Field>via string serialization. A small helper likecoefficients_to_field_values(coeffs: &[BigUint]) -> Result<Vec<InputValue>, ZkError>would eliminate the duplication and make both public functions one-liners around the shared core.♻️ Sketch
+fn coefficients_to_field_values( + coeffs: &[impl std::fmt::Display], +) -> Result<Vec<InputValue>, ZkError> { + coeffs + .iter() + .map(|b| { + let s = b.to_string(); + FieldElement::try_from_str(&s) + .map(InputValue::Field) + .ok_or_else(|| ZkError::SerializationError(format!("invalid field element: {}", s))) + }) + .collect() +}crates/zk-prover/tests/local_e2e_tests.rs (2)
81-130: Consider extracting the repeated test setup into a shared helper.All three new tests (and the existing BFV tests below) duplicate the same ~25 lines of boilerplate: finding
bb, setting up the prover backend, creating the circuit directory, and copying fixture files. A helper likesetup_pk_generation_test() -> (ZkProver, ...)would reduce noise and make the test intent clearer.Also applies to: 132-183, 185-278
231-233: Add context for the magic number1027.
assert_eq!(total_fields, 1027)is opaque — briefly document the expected structure (e.g., 1024 polynomial coefficients + 3 commitment fields, or however it breaks down) so future readers understand what this assertion guards.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores