refactor: missing circuits in zk-prover [skip-line-limit]#1317
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRenames and expands circuit identifiers (new DKG and threshold variants), wires new Provable implementations for DKG and threshold share-decryption/aggregation/pk-aggregation circuits, updates ProofType → CircuitName mappings and slash reasons, adds dkg_input_type to share-decryption inputs, and extends e2e tests and fixture handling. Changes
Sequence Diagram(s)sequenceDiagram
participant ProofType as ProofType (events)
participant CircuitName as CircuitName enum
participant Provable as Provable impls (zk-prover)
participant Prover as Prover / Proving pipeline
participant Tests as local_e2e_tests
ProofType->>CircuitName: map via ProofType::circuit_name()
CircuitName->>Provable: select impl (resolve_circuit_name / circuit())
Provable->>Prover: provide Params, Input types, and circuit id
Prover->>Provable: request inputs / verify
Tests->>Prover: run e2e flows using fixtures
Prover->>Tests: return proof & commitments
Tests->>Provable: independently compute commitments to verify
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
78-91:⚠️ Potential issue | 🟡 MinorDuplicate
.map_errproduces nested error messages.The first
.map_err(line 80) already converts the error toCircuitsErrors::Sample. The second.map_err(line 86) then wraps thatCircuitsErrors::Sampleagain viaDebugformatting, producing a message like:
Sample("Failed to generate smudging error: Sample(\"Failed to generate smudging error: ...\")").The same issue exists on lines 98–111 for
generate_secret_shares_from_poly.Proposed fix — remove the duplicate `.map_err` calls
let esi_coeffs = trbfv .generate_smudging_error(sd.z as usize, sd.lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate smudging error: {:?}", - e - )) })?;let esi_sss_u64 = share_manager .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng.clone()) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate error shares: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate error shares: {:?}", - e - )) })?;crates/events/src/enclave_event/signed_proof.rs (1)
302-321:⚠️ Potential issue | 🟡 MinorMissing test assertion for
T6DecryptedSharesAggregationinproof_type_circuit_name_mapping.The new variant
T6DecryptedSharesAggregationwas added with itscircuit_name()mapping (Line 67), but theproof_type_circuit_name_mappingtest doesn't verify it. Consider adding coverage for completeness.Proposed fix
assert_eq!( ProofType::T5ShareDecryption.circuit_name(), CircuitName::ThresholdShareDecryption ); + assert_eq!( + ProofType::T6DecryptedSharesAggregation.circuit_name(), + CircuitName::DecryptedSharesAggregation + ); }crates/zk-prover/tests/local_e2e_tests.rs (1)
599-606:⚠️ Potential issue | 🟡 MinorStale skip messages in older commitment consistency tests.
The macro skip message at Line 559 was updated to
"skipping: bb not found or fixtures missing", and the new tests at Lines 750 and 786 use the same updated message. However, the pre-existing commitment consistency tests still print the old"skipping: bb not found"message. Sincesetup_circuit_fixturesnow also returnsNonefor missing fixtures, these messages are inaccurate.Proposed fix
- println!("skipping: bb not found"); + println!("skipping: bb not found or fixtures missing");Apply at Lines 604, 654, 693, 722.
Also applies to: 650-656, 688-694, 717-723
🧹 Nitpick comments (2)
crates/events/src/enclave_event/proof.rs (1)
63-98: Note the redundant group prefix indir_path()for some circuits.
DkgSkShareDecryption.dir_path()produces"dkg/dkg_sk_share_decryption"andThresholdShareDecryption.dir_path()produces"threshold/threshold_share_decryption"— the group name appears twice. Existing circuits likeSkShareEncryptiondon't repeat the group prefix ("dkg/sk_share_encryption"). This is presumably intentional to match the actual circuit binary directory names — just flagging for awareness.crates/zk-prover/tests/local_e2e_tests.rs (1)
339-525: Significant code duplication acrosssetup_*test helpers.All five new setup functions follow an identical pattern (find bb → create prover → copy fixtures → generate sample → return tuple). Consider extracting a generic helper that takes the circuit path, fixture name, and a sample-generation closure to reduce boilerplate — though this can be deferred.
cedoor
changed the title
cedoor
changed the title
- Add Provable for DKG share_decryption (sk/e_sm), pk_aggregation, threshold share_decryption, decrypted_shares_aggregation - Add setup and proof tests for all new circuits in local_e2e_tests - Add commitment consistency tests for pk_aggregation and threshold_share_decryption - Make setup_circuit_fixtures check for fixture existence; tests skip when missing - Update CircuitName and ProofType in e3-events Co-authored-by: Cursor <cursoragent@cursor.com>
cedoor
force-pushed
the
refactor/missing-circits-zk-prover
branch
from
e6686fe to
7686d84
Compare
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests