feat: integrate share encryption proofs [skip-line-limit]#1356
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a new share-encryption ZK proof path (C3a/C3b) with request/response types and prover wiring; renames ShareComputationProofSigned → DkgProofSigned and remaps proof-type nomenclature (T* → C*); captures BFV encryption randomness for ZK witnesses; extends keyshare state to track encryption proofs; increases several timeouts in tests/examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ComputeRequestor
participant Multithread as MultithreadActor
participant ZkProver as ZkProver
participant ProofReq as ProofRequestActor
participant Keyshare as KeyshareStateMachine
Client->>Multithread: ComputeRequest (ZkRequest::ShareEncryption)
Multithread->>ZkProver: Prove ShareEncryption (share_row, ciphertext, u/e0/e1 RNS)
ZkProver-->>Multithread: ZkResponse::ShareEncryption (ShareEncryptionProofResponse)
Multithread->>ProofReq: Deliver proof response
ProofReq->>ProofReq: Store proof in PendingThresholdProofs (sk/e_sm enc maps)
ProofReq-->>Keyshare: Publish DkgProofSigned (C3a/C3b)
Keyshare->>Keyshare: Update state (aggregate encryption proofs -> ReadyForDecryption/Decrypting)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
crates/trbfv/src/shares/bfv_encrypted.rs (2)
39-51:BfvEncryptionWitnesshas no derive macros — consider addingDebugat minimum.Unlike every other struct in this file (which all derive
Debugat least),BfvEncryptionWitnesshas no derives. If any downstream code needs to log or inspect witnesses during development, the lack ofDebugwill be an obstacle. IfPolyandCiphertextdon't implementDebug, a custom implementation orderivativeformatting (as used elsewhere in this file) would work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/shares/bfv_encrypted.rs` around lines 39 - 51, Add a Debug impl for BfvEncryptionWitness so it matches the other structs: update the BfvEncryptionWitness declaration to derive(Debug) (or use the same derivative formatting used elsewhere in this file) and if Poly or Ciphertext lack Debug, provide a custom Debug implementation for BfvEncryptionWitness that formats/inspects the fields (share_row, ciphertext, u_rns, e0_rns, e1_rns) appropriately or uses the existing derivative helper. Ensure the chosen approach matches the file's existing pattern for non-Debug types so the struct can be logged/inspected.
102-140: Code duplication betweenencryptandencrypt_extended.The two methods share nearly identical structure (loop over moduli rows, encode plaintext, encrypt, collect ciphertexts). The only difference is which encrypt call is used and whether witnesses are collected. This is acceptable for clarity, but if the shared logic grows, consider extracting the common loop into a helper that takes a closure/strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/shares/bfv_encrypted.rs` around lines 102 - 140, Refactor the duplicated loop in encrypt and encrypt_extended into a shared helper (e.g., process_share_rows or with_share_encryption) that accepts the ShamirShare, recipient_pk, params, rng and a closure/strategy to perform the per-row encryption (closure should call Plaintext::try_encode then invoke either recipient_pk.try_encrypt or recipient_pk.try_encrypt_extended and return the ciphertext plus optional witness data); then have encrypt call the helper with a closure that returns only ciphertexts and encrypt_extended call it with a closure that returns ciphertexts plus BfvEncryptionWitness items and collect those into witnesses; keep the encoding step (Plaintext::try_encode) and error contexts the same and preserve ArcBytes::from_bytes(&ct.to_bytes()) usage when building ciphertexts.crates/events/src/enclave_event/compute_request/zk.rs (1)
156-163: Missingnewconstructor forShareEncryptionProofResponse.
ShareComputationProofResponse(line 177) has anew()factory method. For consistency, consider adding one here as well — especially since the struct has four fields.Proposed constructor
+impl ShareEncryptionProofResponse { + pub fn new(proof: Proof, dkg_input_type: DkgInputType, recipient_party_id: usize, row_index: usize) -> Self { + Self { proof, dkg_input_type, recipient_party_id, row_index } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/compute_request/zk.rs` around lines 156 - 163, Add a standard constructor for ShareEncryptionProofResponse similar to ShareComputationProofResponse: implement an impl block for ShareEncryptionProofResponse with a pub fn new(proof: Proof, dkg_input_type: DkgInputType, recipient_party_id: usize, row_index: usize) -> Self that returns Self { proof, dkg_input_type, recipient_party_id, row_index } so callers can construct the struct via ShareEncryptionProofResponse::new(...).crates/keyshare/src/threshold_share_collector.rs (1)
23-23: Timeout increased 5× (120s → 600s) — consider making it configurable.The jump from 2 to 10 minutes is substantial. This is presumably needed because C3a/C3b proof generation adds significant per-share latency. A hardcoded 600s default works for now, but as the proof pipeline matures, consider exposing this as a configuration parameter so operators can tune it for their hardware.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_share_collector.rs` at line 23, Replace the hardcoded const DEFAULT_COLLECTION_TIMEOUT with a configurable option: add a timeout field to the collector's configuration or constructor (e.g., a Duration parameter on ThresholdShareCollector::new or a CollectorConfig struct) and use that value as the collection timeout with Duration::from_secs(600) as the fallback default; update usages of DEFAULT_COLLECTION_TIMEOUT to read the instance config (or parameter) instead of the const so operators can tune the timeout at runtime via config/CLI/env.crates/events/src/enclave_event/signed_proof.rs (1)
32-33: Doc comment still says "T0" but variant is nowC0PkBfv.- /// T0 — BFV public key proof (Proof 0). + /// C0 — BFV public key proof (Proof 0). C0PkBfv = 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/signed_proof.rs` around lines 32 - 33, The doc comment for the enum variant C0PkBfv is stale (mentions "T0"); update the documentation comment above the C0PkBfv variant so it references the correct variant name and purpose (e.g., "C0 — BFV public key proof (Proof 0)" or similar) to match the variant identifier C0PkBfv and its intent in signed_proof.rs.crates/events/src/enclave_event/mod.rs (1)
39-39: Consider renaming the module file to match the new type name.The module is still named
share_computation_proof_signed(line 39, re-exported at line 87) but now exportsDkgProofSigned. This creates a naming mismatch that could confuse future readers. Consider renaming the file todkg_proof_signed.rsand updating themod/pub usedeclarations.Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/mod.rs` at line 39, The module file and declarations still use the old name share_computation_proof_signed while the type exported is DkgProofSigned; rename the file to dkg_proof_signed.rs and update the module declaration (mod share_computation_proof_signed -> mod dkg_proof_signed) and the re-export (pub use ...::share_computation_proof_signed::DkgProofSigned -> pub use ...::dkg_proof_signed::DkgProofSigned) so the file, mod name, and exported type are consistent.crates/multithread/src/multithread.rs (2)
652-653: DummySecretKeyallocation — document the intent more explicitly.A random secret key is generated solely to satisfy the
ShareEncryptionCircuitDatastruct constraint but is never used in the proof computation. While the comment hints at this, consider adding a note about why the struct requires it (e.g.,Inputs::computeskips it), so future readers aren't alarmed by the allocation of sensitive key material that's immediately discarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 652 - 653, The dummy secret key allocation (dummy_sk = SecretKey::random(&dkg_params, &mut OsRng)) used only to satisfy ShareEncryptionCircuitData should have a clearer explanatory comment: state that ShareEncryptionCircuitData requires a SecretKey field but Inputs::compute deliberately ignores it (so the key is not used in proof computation) and that generating it here is only to construct the struct and does not persist or influence secrets; update the comment near SecretKey::random/ShareEncryptionCircuitData/Inputs::compute to explicitly document this intent and any safety considerations (no storage, no exposure).
469-498: Duplicate step numbering in comments.Steps 4 and 5/6/7 were re-numbered but step 4 appears twice (lines 458 and 469). Minor comment inconsistency.
Suggested fix
- // 4. Compute parity matrix + // 5. Compute parity matrix let committee = req.committee_size.values(); ... - // 5. Build circuit data + // 6. Build circuit data ... - // 6. Generate proof + // 7. Generate proof ... - // 7. Return response + // 8. Return response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 469 - 498, Update the inline step comments to use a consistent, sequential numbering around the parity matrix and circuit/proof sections: change the duplicate "4." comment before compute_parity_matrix to the correct number (e.g., "5.") and renumber the subsequent comments so the sequence flows into the ShareComputationCircuitData construction and proof generation block where compute_parity_matrix, ShareComputationCircuitData, ShareComputationCircuit, and prove(...) are used, ensuring comments reflect their logical order.crates/zk-prover/src/actors/proof_request.rs (1)
210-270: Variable names still uset1_corr/t2a_corr/t2b_corr— consider aligning with C1/C2 naming.Log messages were updated to C1/C2a/C2b but the correlation ID variable names still use the old
t1_/t2a_/t2b_prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_request.rs` around lines 210 - 270, Rename the correlation-id variables to match the C1/C2a/C2b naming used in logs: change t1_corr -> c1_corr, t2a_corr -> c2a_corr, and t2b_corr -> c2b_corr; update all uses in CorrelationId::new(), self.threshold_correlation.insert(...) calls, the ComputeRequest::zk(...) publish calls, and the corresponding removal/retain calls (self.threshold_correlation.remove/retain and self.pending_threshold.remove) so identifiers match consistently (references: CorrelationId, self.threshold_correlation, ComputeRequest::zk, ZkRequest variants PkGeneration/ShareComputation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 947-972: The loop building C3b proof requests can produce
duplicate (recipient_party_id, row_index) across multiple ESI ciphertexts; fix
by adding an esi_index field to ShareEncryptionProofRequest and using
(esi_index, recipient_party_id, row_index) as the unique key in
PendingThresholdProofs (update proof_request.rs to change its HashMap key and
callers), then populate esi_index when pushing into
e_sm_share_encryption_requests (use the outer loop index over esi_witnesses) and
adjust any matching/lookup logic (e.g., is_complete and insertion paths) to
account for the new three-tuple key so proofs from different ESI indices do not
collide.
In `@crates/tests/tests/integration.rs`:
- Around line 556-598: The expected_count for the share generation phase
undercounts C2/C3 proof-related ComputeRequest/ComputeResponse pairs and
DkgProofSigned events; update the calculation used before calling
nodes.take_history_with_timeout (the second block that defines expected_count
and calls take_history_with_timeout) to include: per-party C2a and C2b compute
request/response pairs, per-party/per-recipient/per-modulus C3a and C3b compute
pairs (accounting for threshold::MODULI length and recipients+ESI), plus the
corresponding DkgProofSigned events (one per proof), so the total reflects the
extra ~340 events the reviewer enumerated; adjust the numeric expression
assigned to expected_count (and related comment/println) near the share
generation timer to match the full event total so the test no longer hangs.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 478-512: The C3a/C3b signing loops (iterating
pending.sk_share_encryption_proofs and pending.e_sm_share_encryption_proofs)
must follow the same fail-fast behavior as C1/C2: if self.sign_proof(...)
returns None or if self.bus.publish(...) returns Err, stop processing and
return/abort instead of just logging and continuing; update the loops around
sign_proof (ProofType::C3aSkShareEncryption and
ProofType::C3bESmShareEncryption) and DkgProofSigned publishing to return early
on any signing or publish failure so that ThresholdShareCreated events are not
emitted with missing proofs. Ensure the control flow mirrors the earlier C1/C2
handling (early return on failure) rather than swallowing errors.
---
Nitpick comments:
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 156-163: Add a standard constructor for
ShareEncryptionProofResponse similar to ShareComputationProofResponse: implement
an impl block for ShareEncryptionProofResponse with a pub fn new(proof: Proof,
dkg_input_type: DkgInputType, recipient_party_id: usize, row_index: usize) ->
Self that returns Self { proof, dkg_input_type, recipient_party_id, row_index }
so callers can construct the struct via ShareEncryptionProofResponse::new(...).
In `@crates/events/src/enclave_event/mod.rs`:
- Line 39: The module file and declarations still use the old name
share_computation_proof_signed while the type exported is DkgProofSigned; rename
the file to dkg_proof_signed.rs and update the module declaration (mod
share_computation_proof_signed -> mod dkg_proof_signed) and the re-export (pub
use ...::share_computation_proof_signed::DkgProofSigned -> pub use
...::dkg_proof_signed::DkgProofSigned) so the file, mod name, and exported type
are consistent.
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 32-33: The doc comment for the enum variant C0PkBfv is stale
(mentions "T0"); update the documentation comment above the C0PkBfv variant so
it references the correct variant name and purpose (e.g., "C0 — BFV public key
proof (Proof 0)" or similar) to match the variant identifier C0PkBfv and its
intent in signed_proof.rs.
In `@crates/keyshare/src/threshold_share_collector.rs`:
- Line 23: Replace the hardcoded const DEFAULT_COLLECTION_TIMEOUT with a
configurable option: add a timeout field to the collector's configuration or
constructor (e.g., a Duration parameter on ThresholdShareCollector::new or a
CollectorConfig struct) and use that value as the collection timeout with
Duration::from_secs(600) as the fallback default; update usages of
DEFAULT_COLLECTION_TIMEOUT to read the instance config (or parameter) instead of
the const so operators can tune the timeout at runtime via config/CLI/env.
In `@crates/multithread/src/multithread.rs`:
- Around line 652-653: The dummy secret key allocation (dummy_sk =
SecretKey::random(&dkg_params, &mut OsRng)) used only to satisfy
ShareEncryptionCircuitData should have a clearer explanatory comment: state that
ShareEncryptionCircuitData requires a SecretKey field but Inputs::compute
deliberately ignores it (so the key is not used in proof computation) and that
generating it here is only to construct the struct and does not persist or
influence secrets; update the comment near
SecretKey::random/ShareEncryptionCircuitData/Inputs::compute to explicitly
document this intent and any safety considerations (no storage, no exposure).
- Around line 469-498: Update the inline step comments to use a consistent,
sequential numbering around the parity matrix and circuit/proof sections: change
the duplicate "4." comment before compute_parity_matrix to the correct number
(e.g., "5.") and renumber the subsequent comments so the sequence flows into the
ShareComputationCircuitData construction and proof generation block where
compute_parity_matrix, ShareComputationCircuitData, ShareComputationCircuit, and
prove(...) are used, ensuring comments reflect their logical order.
In `@crates/trbfv/src/shares/bfv_encrypted.rs`:
- Around line 39-51: Add a Debug impl for BfvEncryptionWitness so it matches the
other structs: update the BfvEncryptionWitness declaration to derive(Debug) (or
use the same derivative formatting used elsewhere in this file) and if Poly or
Ciphertext lack Debug, provide a custom Debug implementation for
BfvEncryptionWitness that formats/inspects the fields (share_row, ciphertext,
u_rns, e0_rns, e1_rns) appropriately or uses the existing derivative helper.
Ensure the chosen approach matches the file's existing pattern for non-Debug
types so the struct can be logged/inspected.
- Around line 102-140: Refactor the duplicated loop in encrypt and
encrypt_extended into a shared helper (e.g., process_share_rows or
with_share_encryption) that accepts the ShamirShare, recipient_pk, params, rng
and a closure/strategy to perform the per-row encryption (closure should call
Plaintext::try_encode then invoke either recipient_pk.try_encrypt or
recipient_pk.try_encrypt_extended and return the ciphertext plus optional
witness data); then have encrypt call the helper with a closure that returns
only ciphertexts and encrypt_extended call it with a closure that returns
ciphertexts plus BfvEncryptionWitness items and collect those into witnesses;
keep the encoding step (Plaintext::try_encode) and error contexts the same and
preserve ArcBytes::from_bytes(&ct.to_bytes()) usage when building ciphertexts.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 210-270: Rename the correlation-id variables to match the
C1/C2a/C2b naming used in logs: change t1_corr -> c1_corr, t2a_corr -> c2a_corr,
and t2b_corr -> c2b_corr; update all uses in CorrelationId::new(),
self.threshold_correlation.insert(...) calls, the ComputeRequest::zk(...)
publish calls, and the corresponding removal/retain calls
(self.threshold_correlation.remove/retain and self.pending_threshold.remove) so
identifiers match consistently (references: CorrelationId,
self.threshold_correlation, ComputeRequest::zk, ZkRequest variants
PkGeneration/ShareComputation).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/share_computation_proof_signed.rscrates/events/src/enclave_event/signed_proof.rscrates/events/src/enclave_event/test_event.rscrates/events/src/enclave_event/threshold_share_pending.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/zk-prover/src/actors/proof_request.rsexamples/CRISP/server/.env.exampleexamples/CRISP/test/crisp.spec.tstemplates/default/tests/integration.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/tests/tests/integration.rs (1)
577-598:⚠️ Potential issue | 🟠 Major
expected_count = 65still excludes C2a/C2b/C3a/C3b proof events — now more severe since this PR activates C3.The share-generation window does not account for:
- C2a (SkShareComputation) — 1 ComputeRequest + 1 ComputeResponse per party = 10 events
- C2b (ESmShareComputation) — 1 ComputeRequest + 1 ComputeResponse per party = 10 events
- C3a (SkShareEncryption) —
5 recipients × N moduli × 5 parties× 2 = ~100 events (2 moduli forinsecure_512threshold preset)- C3b (ESmShareEncryption) —
1 ESI × 5 recipients × N moduli × 5 parties× 2 = ~100 events- Plus DkgProofSigned events for each of the above proofs
Since C3 (share_encryption) is activated by this PR, the excess events will accumulate between this
take_history_with_timeoutcall and the next one. When the decryption-key phase then collects its 16 events, those events will likely include buffered C3-related entries rather thanKeyshareCreated/PublicKeyAggregated, causing theh.last() == PublicKeyAggregatedassertion at line 633 to panic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 577 - 598, The current expected_count used with nodes.take_history_with_timeout(...) omits C2a/C2b/C3a/C3b proof events (and their DkgProofSigned follow-ups) so the share-generation wait will return too early; update the wait to account for those events by either (a) increasing the expected_count variable to include SkShareComputation (C2a), ESmShareComputation (C2b), SkShareEncryption (C3a), ESmShareEncryption (C3b) and their corresponding DkgProofSigned events based on the active threshold preset (e.g., add 10+10+~100+~100 + matching signed events for insecure_512), or (b) replace the fixed-count wait around take_history_with_timeout(0, expected_count, ...) with a loop that polls history until you observe the PublicKeyAggregated event (use h.event_types() / h.last() / scanning) to avoid hardcoding counts; adjust the code that asserts h.last() == PublicKeyAggregated accordingly so it checks the scanned events for PublicKeyAggregated rather than assuming exact event counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tests/tests/integration.rs`:
- Line 388: The test changed the runtime value to let esi_per_ct = 1 but the
adjacent section header comment ("// 2. Trigger E3Requested") still documents
esi_per_ciphertext = 3; update that stale doc-comment to esi_per_ciphertext = 1
to match the actual variable (esi_per_ct) and ensure the comment in the "2.
Trigger E3Requested" block reflects the new value.
---
Duplicate comments:
In `@crates/tests/tests/integration.rs`:
- Around line 577-598: The current expected_count used with
nodes.take_history_with_timeout(...) omits C2a/C2b/C3a/C3b proof events (and
their DkgProofSigned follow-ups) so the share-generation wait will return too
early; update the wait to account for those events by either (a) increasing the
expected_count variable to include SkShareComputation (C2a), ESmShareComputation
(C2b), SkShareEncryption (C3a), ESmShareEncryption (C3b) and their corresponding
DkgProofSigned events based on the active threshold preset (e.g., add
10+10+~100+~100 + matching signed events for insecure_512), or (b) replace the
fixed-count wait around take_history_with_timeout(0, expected_count, ...) with a
loop that polls history until you observe the PublicKeyAggregated event (use
h.event_types() / h.last() / scanning) to avoid hardcoding counts; adjust the
code that asserts h.last() == PublicKeyAggregated accordingly so it checks the
scanned events for PublicKeyAggregated rather than assuming exact event counts.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/tests/tests/integration.rstemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/default/tests/integration.spec.ts
Contributor
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/src/actors/proof_request.rs (1)
184-184:⚠️ Potential issue | 🟡 MinorLog messages still reference "T0" while
ProofTypewas renamed toC0PkBfv.The proof-type enum values have been renamed to C0/C1/C2a/C2b/C3a/C3b, and most log messages use the new naming. However, several T0-related log messages were missed:
- Line 184:
"Requesting T0 proof generation"→ should be C0- Line 572:
"Signed T0 proof for party ..."→ should be C0- Line 579:
"Failed to sign T0 proof payload ..."→ should be C0- Line 603:
"T0 proof request failed for E3 ..."→ should be C0Suggested fix
- info!("Requesting T0 proof generation"); + info!("Requesting C0 PkBfv proof generation");- "Signed T0 proof for party {} (signer: {})", + "Signed C0 proof for party {} (signer: {})",- error!("Failed to sign T0 proof payload: {err} — proof will not be published"); + error!("Failed to sign C0 proof payload: {err} — proof will not be published");- "T0 proof request failed for E3 {}: {err} — key will not be published without proof", + "C0 proof request failed for E3 {}: {err} — key will not be published without proof",Also applies to: 565-565, 572-572, 579-579, 603-603
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_request.rs` at line 184, Several log messages still reference the old "T0" name; update all occurrences to the new C0 naming (matching the renamed ProofType C0PkBfv). Locate the log statements in proof_request.rs that contain "Requesting T0 proof generation", "Signed T0 proof for party", "Failed to sign T0 proof payload", and "T0 proof request failed for E3" and replace "T0" with "C0" so messages read e.g. "Requesting C0 proof generation", "Signed C0 proof for party ...", "Failed to sign C0 proof payload ...", and "C0 proof request failed for E3 ...". Ensure string formatting and any surrounding context (function names/variables) remain unchanged.
♻️ Duplicate comments (1)
crates/zk-prover/src/actors/proof_request.rs (1)
478-516: Past concern about C3a/C3b signing failures has been addressed.Signing failures now use
let Some(...) else { return; }(lines 482-484, 502-504), which preventsThresholdShareCreatedevents from being published with incomplete proof coverage. This is consistent with the C1/C2 early-return pattern.The remaining difference —
bus.publisherrors for DkgProofSigned just log without returning — is also consistent with how C1/C2 proofs handle publish failures (lines 445-475).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_request.rs` around lines 478 - 516, The reviewer marked the previous concern as resolved (signing now uses early-return in sign_proof for ProofType::C3aSkShareEncryption and ProofType::C3bESmShareEncryption in the loops over pending.sk_share_encryption_proofs and pending.e_sm_share_encryption_proofs), so no code change is required; leave the existing logic in proof_request.rs (the for-loops that call self.sign_proof and then self.bus.publish DkgProofSigned) as-is and ignore this duplicate review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Line 184: Several log messages still reference the old "T0" name; update all
occurrences to the new C0 naming (matching the renamed ProofType C0PkBfv).
Locate the log statements in proof_request.rs that contain "Requesting T0 proof
generation", "Signed T0 proof for party", "Failed to sign T0 proof payload", and
"T0 proof request failed for E3" and replace "T0" with "C0" so messages read
e.g. "Requesting C0 proof generation", "Signed C0 proof for party ...", "Failed
to sign C0 proof payload ...", and "C0 proof request failed for E3 ...". Ensure
string formatting and any surrounding context (function names/variables) remain
unchanged.
---
Duplicate comments:
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 478-516: The reviewer marked the previous concern as resolved
(signing now uses early-return in sign_proof for ProofType::C3aSkShareEncryption
and ProofType::C3bESmShareEncryption in the loops over
pending.sk_share_encryption_proofs and pending.e_sm_share_encryption_proofs), so
no code change is required; leave the existing logic in proof_request.rs (the
for-loops that call self.sign_proof and then self.bus.publish DkgProofSigned)
as-is and ignore this duplicate review comment.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tests/tests/integration.rscrates/zk-prover/src/actors/proof_request.rsexamples/CRISP/server/.env.exampleexamples/CRISP/test/crisp.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/server/.env.example
- examples/CRISP/test/crisp.spec.ts
ctrlc03
force-pushed
the
feat/integrate-share-encryption-proofs
branch
from
bc8837e to
a4c7047
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/keyshare/src/threshold_keyshare.rs (1)
947-972:⚠️ Potential issue | 🟡 MinorC3b proof requests can collide on (recipient,row) across ESI ciphertexts.
If multiple ESI ciphertexts are ever supported,
(recipient_party_id, row_index)will not be unique; proofs can overwrite each other downstream. Consider addingesi_indexto the request key or asserting the single‑ESI constraint where requests are built.
Based on learnings: the current circuit accepts only one smudging error per decryption (esi_sss_raw[0]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 947 - 972, The ShareEncryptionProofRequest keys built in the loop over esi_witnesses can collide because (recipient_party_id, row_index) is not unique across multiple ESI ciphertexts; update the construction in the loop that builds e_sm_share_encryption_requests (loop over esi_witnesses and pushing ShareEncryptionProofRequest) to include an esi_index identifier (e.g., add an esi_index field to ShareEncryptionProofRequest and populate it from the outer loop index) or, if multiple ESIs are not supported, assert/validate that esi_witnesses.len() == 1 before building requests to prevent silent overwrites; touch the ShareEncryptionProofRequest type and all downstream uses that depend on recipient_party_id/row_index keys to incorporate the new esi_index or enforce the single‑ESI constraint.crates/zk-prover/src/actors/proof_request.rs (1)
52-110:⚠️ Potential issue | 🟡 MinorC3b proof keying can collide if multiple ESI ciphertexts appear.
C3b proofs are keyed only by
(recipient_party_id, row_index)while the expected count tracks all requests; if multiple ESI ciphertexts are ever supported, proofs from different ESI indices can overwrite each other andis_complete()may never be satisfied. Consider adding anesi_indexto the key or asserting the single‑ESI constraint at the request source.
Based on learnings: the current circuit accepts only one smudging error per decryption (esi_sss_raw[0]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_request.rs` around lines 52 - 110, PendingThresholdProofs currently keys C3b proofs by (recipient_party_id, row_index) which can collide if multiple ESI ciphertexts (different esi_index) are present; update the data model and logic to include an esi_index in the C3b key or enforce single‑ESI at the source: modify PendingThresholdProofs to use e_sm_share_encryption_proofs: HashMap<(usize, usize, usize), Proof> (or similar tuple) and update expected_e_sm_enc_count accordingly, change ThresholdProofKind::ESmShareEncryption to include esi_index, and update store_proof and is_complete to insert/check using the extended key (and any call sites that construct/consume ESmShareEncryption proof keys) so proofs from different ESI indices cannot overwrite each other.
🧹 Nitpick comments (4)
templates/default/tests/integration.spec.ts (1)
164-166: Consider replacing the magicdurationwith a named/configurable constant.
This helps keep test timing knobs centralized (e.g., shared test config or env override) and easier to tune across suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` around lines 164 - 166, Replace the magic literal assigned to duration with a named configurable constant so test timing is centralized; e.g., introduce a constant (TEST_INPUT_WINDOW_DURATION or similar) or read from a shared test config/env and use that instead of the hardcoded 300 where duration is passed into calculateInputWindow(publicClient, duration); update any related references (duration, calculateInputWindow call) to use the new constant so it's easy to tune across suites.crates/keyshare/src/threshold_share_collector.rs (1)
23-23: Consider making the 10‑minute timeout configurable.
Hardcoding 600s may not fit all deployments and can delay failure handling if upstream timeouts are shorter; consider wiring it through config or the parent actor so it can be tuned without a code change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_share_collector.rs` at line 23, DEFAULT_COLLECTION_TIMEOUT is hardcoded to 600s; make the timeout configurable by replacing the const with a configurable Duration field (e.g., collection_timeout) on the ThresholdShareCollector (or its constructor) and wire that field from the parent actor or config object instead of using DEFAULT_COLLECTION_TIMEOUT; update any places that referenced the const (e.g., new()/start()/collect_* functions) to use the instance field and add a sensible default in the parent or config parsing path so existing behavior remains if not overridden.crates/events/src/enclave_event/share_computation_proof_signed.rs (1)
14-18: File name no longer matches the struct name.The file is still named
share_computation_proof_signed.rsbut the struct has been renamed toDkgProofSigned. Consider renaming the file todkg_proof_signed.rs(and updatingmod.rsaccordingly) to keep the naming consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/share_computation_proof_signed.rs` around lines 14 - 18, The struct declared is DkgProofSigned but the file is still named share_computation_proof_signed.rs; rename the file to dkg_proof_signed.rs and update any module declarations that reference the old file name (e.g., entries in mod.rs or use paths) to point to dkg_proof_signed so the module name matches the struct name DkgProofSigned and imports (E3id, SignedProofPayload) continue to resolve.crates/tests/tests/integration.rs (1)
580-584: 3000-second timeout forThresholdShareCreatedevents may be overly generous.With C3a/C3b proofs now generated per-party per-recipient per-modulus-row, the proof generation workload has increased significantly, which justifies a larger timeout. However, 3000s (50 minutes) is quite high — if something is actually broken, the test will just hang for a very long time before failing. Consider whether a lower value (e.g., 600–900s) would be sufficient while still catching legitimate slow runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 580 - 584, The test uses an excessively long 3000s timeout for awaiting ThresholdShareCreated events which can mask failures; change the Duration passed to nodes.take_history_with_timeout (call site using Duration::from_secs(3000)) to a lower, still-generous value such as Duration::from_secs(900) (or 600–900s range) to catch real slow runs but fail faster on broken runs; keep the rest of the call involving shares_timer, expected, and nodes.take_history_with_timeout unchanged and adjust if you observe flakiness during CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 947-972: The ShareEncryptionProofRequest keys built in the loop
over esi_witnesses can collide because (recipient_party_id, row_index) is not
unique across multiple ESI ciphertexts; update the construction in the loop that
builds e_sm_share_encryption_requests (loop over esi_witnesses and pushing
ShareEncryptionProofRequest) to include an esi_index identifier (e.g., add an
esi_index field to ShareEncryptionProofRequest and populate it from the outer
loop index) or, if multiple ESIs are not supported, assert/validate that
esi_witnesses.len() == 1 before building requests to prevent silent overwrites;
touch the ShareEncryptionProofRequest type and all downstream uses that depend
on recipient_party_id/row_index keys to incorporate the new esi_index or enforce
the single‑ESI constraint.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 52-110: PendingThresholdProofs currently keys C3b proofs by
(recipient_party_id, row_index) which can collide if multiple ESI ciphertexts
(different esi_index) are present; update the data model and logic to include an
esi_index in the C3b key or enforce single‑ESI at the source: modify
PendingThresholdProofs to use e_sm_share_encryption_proofs: HashMap<(usize,
usize, usize), Proof> (or similar tuple) and update expected_e_sm_enc_count
accordingly, change ThresholdProofKind::ESmShareEncryption to include esi_index,
and update store_proof and is_complete to insert/check using the extended key
(and any call sites that construct/consume ESmShareEncryption proof keys) so
proofs from different ESI indices cannot overwrite each other.
---
Nitpick comments:
In `@crates/events/src/enclave_event/share_computation_proof_signed.rs`:
- Around line 14-18: The struct declared is DkgProofSigned but the file is still
named share_computation_proof_signed.rs; rename the file to dkg_proof_signed.rs
and update any module declarations that reference the old file name (e.g.,
entries in mod.rs or use paths) to point to dkg_proof_signed so the module name
matches the struct name DkgProofSigned and imports (E3id, SignedProofPayload)
continue to resolve.
In `@crates/keyshare/src/threshold_share_collector.rs`:
- Line 23: DEFAULT_COLLECTION_TIMEOUT is hardcoded to 600s; make the timeout
configurable by replacing the const with a configurable Duration field (e.g.,
collection_timeout) on the ThresholdShareCollector (or its constructor) and wire
that field from the parent actor or config object instead of using
DEFAULT_COLLECTION_TIMEOUT; update any places that referenced the const (e.g.,
new()/start()/collect_* functions) to use the instance field and add a sensible
default in the parent or config parsing path so existing behavior remains if not
overridden.
In `@crates/tests/tests/integration.rs`:
- Around line 580-584: The test uses an excessively long 3000s timeout for
awaiting ThresholdShareCreated events which can mask failures; change the
Duration passed to nodes.take_history_with_timeout (call site using
Duration::from_secs(3000)) to a lower, still-generous value such as
Duration::from_secs(900) (or 600–900s range) to catch real slow runs but fail
faster on broken runs; keep the rest of the call involving shares_timer,
expected, and nodes.take_history_with_timeout unchanged and adjust if you
observe flakiness during CI.
In `@templates/default/tests/integration.spec.ts`:
- Around line 164-166: Replace the magic literal assigned to duration with a
named configurable constant so test timing is centralized; e.g., introduce a
constant (TEST_INPUT_WINDOW_DURATION or similar) or read from a shared test
config/env and use that instead of the hardcoded 300 where duration is passed
into calculateInputWindow(publicClient, duration); update any related references
(duration, calculateInputWindow call) to use the new constant so it's easy to
tune across suites.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
crates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/share_computation_proof_signed.rscrates/events/src/enclave_event/signed_proof.rscrates/events/src/enclave_event/test_event.rscrates/events/src/enclave_event/threshold_share_pending.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/config.rsexamples/CRISP/server/.env.exampleexamples/CRISP/test/crisp.spec.tstemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/trbfv/src/shares/bfv_encrypted.rs
- examples/CRISP/test/crisp.spec.ts
- crates/events/src/enclave_event/test_event.rs
- examples/CRISP/server/.env.example
ctrlc03
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/default/deploy/default.ts`:
- Around line 93-94: The call to updateE3Config currently uses a
possibly-undefined hre.globalOptions.network (variable chain); instead, resolve
the chain name reliably from the provider (e.g., use hre.getChainId() or
provider.request('eth_chainId') and map to a network name) before calling
updateE3Config so you never pass undefined; update the code that sets chain
(referencing hre.globalOptions.network and variable chain) to fall back to a
provider-derived chain name and then call updateE3Config(chain,
path.join(__dirname, '..', 'enclave.config.yaml'), contractMapping).
In `@templates/default/scripts/test_integration.sh`:
- Around line 22-29: The current line using export $(enclave print-env --chain
localhost) can break values with spaces/newlines; replace this word-splitting
approach by enabling automatic export (set -a), then capture and source the
enclave print-env output so all variables (including those with spaces/newlines)
are preserved; specifically modify the script around the export invocation (the
line with "export $(enclave print-env --chain localhost)") to run set -a, feed
enclave print-env --chain localhost into a shell source/read mechanism, then run
set +a afterwards so the environment is exported safely for the concurrently
block that follows.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/CRISP/enclave.config.yamlpackages/enclave-contracts/scripts/utils.tstemplates/default/deploy/default.tstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/scripts/dev_ciphernodes.shtemplates/default/scripts/dev_frontend.shtemplates/default/scripts/dev_server.shtemplates/default/scripts/test_integration.shtemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/default/tests/integration.spec.ts
ctrlc03
force-pushed
the
feat/integrate-share-encryption-proofs
branch
from
e469dab to
5af82c6
Compare
ctrlc03
force-pushed
the
feat/integrate-share-encryption-proofs
branch
from
5af82c6 to
69dc4d9
Compare
cedoor
approved these changes
Feb 26, 2026
This was referenced Feb 27, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #1333
Summary by CodeRabbit
New Features
Chores
Tests