feat: add sk_share and e_sm_share#1290
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes extend ZK proof circuit infrastructure to support distinct share computation circuits for different secret types (SecretKey and SmudgingNoise) by adding new CircuitName variants, implementing input-dependent circuit resolution via the Provable trait, updating witness serialization to include type metadata, and adding corresponding end-to-end tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as CircuitInput<br/>(with DkgInputType)
participant Circuit as ShareComputationCircuit<br/>(Provable)
participant Resolver as resolve_circuit_name()
participant CircuitEnum as CircuitName<br/>enum
participant Validator as valid_circuits()
participant Prover as Prover/<br/>Verification
Input->>Circuit: build_witness(input)
activate Circuit
Circuit->>Resolver: resolve_circuit_name(&input)
activate Resolver
alt input.dkg_input_type == SecretKey
Resolver->>CircuitEnum: SkShareComputation
else input.dkg_input_type == SmudgingNoise
Resolver->>CircuitEnum: ESmShareComputation
end
CircuitEnum-->>Resolver: resolved CircuitName
Resolver-->>Circuit: CircuitName
deactivate Resolver
Circuit->>Prover: use resolved circuit name<br/>for dir_path, json_file
deactivate Circuit
Note over Validator: During verification
Validator->>Circuit: valid_circuits()
Circuit-->>Validator: [SkShareComputation,<br/>ESmShareComputation]
Validator->>Prover: check proof.circuit<br/>in valid_circuits()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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🧪 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 |
cedoor
force-pushed
the
feat/proof-1b
branch
from
4f3302e to
c852622
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-prover/tests/local_e2e_tests.rs (1)
273-276:⚠️ Potential issue | 🟡 MinorAdd share computation circuits to
e2e_proof_tests!macro.The new
setup_share_computation_sk_testandsetup_share_computation_e_sm_testhelpers are structurally compatible with the macro and their fixture files (.jsonand.vk) already exist. The current commitment consistency tests only verify mathematical correctness; adding these entries to the macro ensures cryptographic proof verification (circuit.verify()) is tested:e2e_proof_tests! { (pk_generation, setup_pk_generation_test()), (pk_bfv, setup_pk_bfv_test()), + (share_computation_sk, setup_share_computation_sk_test()), + (share_computation_e_sm, setup_share_computation_e_sm_test()), }
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/circuits/utils.rs`:
- Around line 11-14: Remove the three unused imports CrtPolynomial, Polynomial,
and BigInt from the top of utils.rs and keep only AcirField (so try_from_str
stays in scope); specifically edit the use statements to delete references to
CrtPolynomial, Polynomial, and BigInt so that only use acvm::AcirField; (and
retain any other truly used imports like InputValue/InputMap) to eliminate the
unused-import warnings.
🧹 Nitpick comments (4)
crates/zk-prover/tests/common/mod.rs (1)
20-28: No bounds/alignment validation onsignalsslice.Both helpers will panic on out-of-bounds access or if
signals.len()is not a multiple ofFIELD_SIZE. For test-only code this is acceptable, butextract_field_from_endsilently ignores trailing bytes whensignals.len() % FIELD_SIZE != 0, which could mask a test bug if public signals are ever malformed.Consider adding a debug assertion:
🛡️ Optional: guard against misaligned signals
pub fn extract_field_from_end(signals: &[u8], from_end: usize) -> BigInt { + debug_assert_eq!( + signals.len() % FIELD_SIZE, 0, + "signals length {} is not a multiple of FIELD_SIZE {}", + signals.len(), FIELD_SIZE + ); let total_fields = signals.len() / FIELD_SIZE; extract_field(signals, total_fields - 1 - from_end) }crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
246-246: Unnecessary.clone()on aCopytype.
DkgInputTypederivesCopy, so.clone()is redundant. You can just writeinput.dkg_input_type.crates/zk-helpers/src/bin/zk_cli.rs (1)
258-258: Unnecessary.clone()on aCopytype.
DkgInputTypederivesCopy;.clone()can be dropped here (same nit as incomputation.rs).crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
128-190: Test only coversSecretKeyTOML variant; consider adding aSmudgingNoisecase.The test verifies that
"sk_secret"appears in the generated TOML, but there's no corresponding test forSmudgingNoisethat would assert"e_sm_secret"is present. Since theto_jsonbranching is a key part of this PR, a parallel test would increase confidence.
cedoor
force-pushed
the
feat/proof-1b
branch
from
c852622 to
569775a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-prover/tests/local_e2e_tests.rs (1)
273-276:⚠️ Potential issue | 🟡 MinorShare-computation circuits are not covered by the
e2e_proof_tests!macro.The SK and ESM share-computation setup helpers exist but are only used in commitment-consistency tests, which call
prove()but neververify(). The macro-generated tests exercise both prove and verify. Was this intentional (e.g., fixture VKs not yet available), or should these be added?e2e_proof_tests! { (pk_generation, setup_pk_generation_test()), (pk_bfv, setup_pk_bfv_test()), + (share_computation_sk, setup_share_computation_sk_test()), + (share_computation_e_sm, setup_share_computation_e_sm_test()), }
🧹 Nitpick comments (3)
crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
246-246: Unnecessary.clone()on aCopytype.
DkgInputTypederivesCopy, so you can just writedkg_input_type: input.dkg_input_type.Proposed fix
- dkg_input_type: input.dkg_input_type.clone(), + dkg_input_type: input.dkg_input_type,crates/zk-helpers/src/bin/zk_cli.rs (1)
265-265: Unnecessary.clone()on aCopytype.Same nit as in
computation.rs—DkgInputTypeisCopy.- dkg_input_type.clone(), + dkg_input_type,crates/zk-prover/tests/local_e2e_tests.rs (1)
366-428: The SK and ESM commitment-consistency tests are near-identical.Consider extracting a shared helper to reduce duplication, e.g.:
async fn assert_share_computation_commitment( setup: impl Future<Output = Option<(ZkBackend, tempfile::TempDir, ZkProver, ShareComputationCircuit, ShareComputationCircuitInput, BfvPreset, &'static str)>>, ) { let Some((_backend, _temp, prover, circuit, sample, preset, e3_id)) = setup.await else { println!("skipping: bb not found"); return; }; let proof = circuit.prove(&prover, &preset, &sample, e3_id).expect("proof generation should succeed"); let commitment_from_proof = extract_field(&proof.public_signals, 0); let output = ShareComputationCircuit::compute(preset, &sample).expect("computation should succeed"); assert_eq!(commitment_from_proof, output.witness.expected_secret_commitment, "commitment mismatch"); prover.cleanup(e3_id).unwrap(); }This is test code so not critical — just a suggestion.
ctrlc03
force-pushed
the
feat/proof-1b
branch
from
569775a to
67db6e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/traits.rs`:
- Around line 87-93: Fix the typo in the verification error message inside the
check that uses self.valid_circuits() and proof.circuit: update the string
passed to ZkError::VerifyFailed from "circuit mismatch: expected on of {:?}, got
{}" to "circuit mismatch: expected one of {:?}, got {}" so the message reads
"one of" correctly while keeping the existing formatting and use of
self.valid_circuits() and proof.circuit.
🧹 Nitpick comments (1)
crates/zk-prover/tests/local_e2e_tests.rs (1)
109-184: Consider extracting a shared setup helper to reduce duplication.The two setup functions (
setup_share_computation_sk_testandsetup_share_computation_e_sm_test) are nearly identical, differing only in fixture name,DkgInputTypevariant, ande3_id. A parameterized helper would reduce the ~75 lines of duplication.That said, this is test code and the current approach is clear and readable.
♻️ Example: parameterized helper
+async fn setup_share_computation_test( + input_type: DkgInputType, + fixture_name: &str, + e3_id: &'static str, +) -> Option<( + ZkBackend, + tempfile::TempDir, + ZkProver, + ShareComputationCircuit, + ShareComputationCircuitInput, + BfvPreset, + &'static str, +)> { + let committee = CiphernodesCommitteeSize::Small.values(); + let preset = BfvPreset::InsecureThreshold512; + let bb = find_bb().await?; + let (backend, temp) = setup_test_prover(&bb).await; + + setup_circuit_fixtures(&backend, &["dkg", fixture_name], fixture_name).await; + + let sample = + ShareComputationCircuitInput::generate_sample(preset, committee, input_type).ok()?; + let prover = ZkProver::new(&backend); + + Some((backend, temp, prover, ShareComputationCircuit, sample, preset, e3_id)) +} + +async fn setup_share_computation_sk_test() -> Option<(...)> { + setup_share_computation_test(DkgInputType::SecretKey, "sk_share_computation", "1").await +} + +async fn setup_share_computation_e_sm_test() -> Option<(...)> { + setup_share_computation_test(DkgInputType::SmudgingNoise, "e_sm_share_computation", "2").await +}
ctrlc03
force-pushed
the
feat/proof-1b
branch
from
d0b8a48 to
a70b971
Compare
ctrlc03
force-pushed
the
feat/proof-1b
branch
from
e5faeb9 to
e5e92fd
Compare
ctrlc03
force-pushed
the
feat/proof-1b
branch
from
e5e92fd to
b3dce69
Compare
ctrlc03
force-pushed
the
feat/proof-1b
branch
from
b3dce69 to
d2570cd
Compare
3 participants
Add sk_share_computation and e_sm_share computation circuits into zk prover, as well as tests.
Summary by CodeRabbit
Release Notes
New Features
Tests