feat: stop encrypting DKG shares to self#1529
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR implements a protocol shift in the DKG share decryption flow, transitioning from decrypting ChangesDKG Share Decryption with Own-Party Separation
Sequence Diagram(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🤖 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 1179-1188: The code currently passes party_id as the slot index to
BfvEncryptedShares::encrypt_all_extended and to the C3 skip logic, which is
wrong when collected_encryption_keys is filtered or reordered; instead compute
the self slot by finding the position of the local party_id inside
collected_encryption_keys and use that index as the skip index. Concretely:
locate where collected_encryption_keys is available, find the index via a
position lookup of entries matching party_id (e.g.,
collected_encryption_keys.iter().position(|(id, _)| id == &party_id)), handle
the none case with an error, and pass that computed usize as the skip_idx to
encrypt_all_extended and any C3 skip checks rather than using party_id directly
(references: collected_encryption_keys, party_id,
BfvEncryptedShares::encrypt_all_extended, C3 skip logic).
In `@crates/multithread/src/multithread.rs`:
- Around line 1194-1210: After deserializing own_share_raw into
own_plaintext_share you must also validate each inner row length matches the BFV
coefficient count (the expected number of coefficients used for plaintext
polynomials) to avoid variable-length shares; iterate over own_plaintext_share
(use enumerate to get the row index), and for each row check row.len() ==
expected_coefficient_count (the BFV/poly coefficient count used elsewhere in the
module), returning Err(make_zk_error(&request, format!("own_plaintext_share[{}]
has {} coefficients, expected {}", i, row.len(), expected_coefficient_count)))
on mismatch instead of proceeding.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 201-204: The loop currently calls
data.secret_key.try_decrypt(&party_cts[mod_idx]).unwrap() which panics on
decryption failure; change it to propagate the error as a CircuitsErrors so the
method returns a proper ComputeRequestError instead of crashing. Replace the
unwrap usage in the loop that produces decrypted_pt (inside the function that
iterates mod_idx and computes share_coeffs) with error handling that maps the
try_decrypt Err into an appropriate CircuitsErrors variant (including context
like which mod_idx/party_cts failed) and returns it (using ? or map_err(...)?),
ensuring the rest of the function keeps its existing error type flow.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 1244-1280: The match arm for
share.extract_for_party(positional_idx) currently treats every None as an
expected self-slot and just traces; change it to only skip (trace) when the None
corresponds to the sender’s own slot and treat any other None as an error:
inside the None branch compare the missing slot to the sender identity (use the
module’s sender/self positional identifier available in this actor—e.g., the
field that represents our own positional index or party id used elsewhere with
real_party_id/positional_idx), keep the trace and skip only for the own-slot
case, and for any other positional_idx log an error (include real_party_id and
positional_idx) so missing external recipient shares aren’t silently dropped
instead of being surfaced for upstream handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6201a406-25b2-497b-aeba-c561c80f41f3
📒 Files selected for processing (9)
crates/events/src/enclave_event/compute_request/zk.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/multithread.rscrates/trbfv/src/shares/bfv_encrypted.rscrates/zk-helpers/src/circuits/dkg/share_decryption/circuit.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_decryption/sample.rscrates/zk-prover/src/actors/proof_request.rs
10 tasks
0xjei
approved these changes
May 14, 2026
0xjei
left a comment
0xjei
left a comment
Contributor
There was a problem hiding this comment.
utACK - looks good as far as CI passes. Let's wait for @zahrajavar review!
zahrajavar
approved these changes
May 15, 2026
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.
During threshold key generation, every node was previously encrypting a share of its own secret to itself, sending it over the wire, then decrypting it back, a wasteful round-trip with no security benefit (a node already knows its own share). This PR removes the self-loop end-to-end.
Summary by CodeRabbit