chore: pre check decrypted shares match c6 inputs#1500
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 per-party runtime verification that recomputes threshold-decryption share commitments from raw share bytes and compares them to C6 proof outputs; expands aggregator crate dependencies; broadens polynomial representation handling; adds a unit test for commitment computation; tweaks a small comment and shortens an integration test timeout. Changes
Sequence Diagram(s)sequenceDiagram
participant Aggregator as Aggregator
participant Party as PartyShare
participant C6 as C6ProofStore
participant BFV as BFVContext
Aggregator->>Party: collect verified honest_shares (bytes + proof refs)
loop per honest party
Aggregator->>C6: extract d_commitment from C6 public outputs
Aggregator->>BFV: build BFV params/context (moduli, d_bit)
BFV-->>Aggregator: params/context
Aggregator->>Party: deserialize share bytes -> Poly
Aggregator->>Aggregator: Poly.reverse() + center(moduli) -> CRT poly
Aggregator->>Aggregator: compute threshold decryption share commitment (d)
Aggregator->>C6: compare computed d to extracted d_commitment
alt mismatch
Aggregator->>Aggregator: mark party dishonest, remove from honest_shares
end
end
Aggregator->>Aggregator: ensure honest_shares.len() > threshold -> proceed or abort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 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/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 353-370: The code uses signed.recover_address() to decide who to
slash and publishes SignedProofFailed (with
ProofType::C6ThresholdShareDecryption) even though the SignedProofPayload
wrapper signature hasn't been verified or bound to the broadcaster, which allows
a malicious sender to blame an honest signer; change the logic in the loop over
state.c6_proofs so you only publish SignedProofFailed when the wrapper identity
is verified/bound to the committee sender (e.g., call verify_address() or
otherwise confirm the SignedProofPayload maps to the broadcaster or that
prover.verify() has validated the wrapper) — still exclude the mismatched share
locally, but gate calls to self.bus.publish(SignedProofFailed { ...
signed_payload: signed.clone() ... }) on a successful wrapper verification (and
keep using recover_address() only for logging when wrapper unverified).
🪄 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: 47e3ca38-2db4-4123-814b-3aa6ea32dc81
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/aggregator/Cargo.tomlcrates/aggregator/src/threshold_plaintext_aggregator.rscrates/multithread/src/multithread.rs
ctrlc03
force-pushed
the
chore/pre-c7-check-ag
branch
from
bab67a8 to
5205da6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
488-490: Remove duplicated doc sentence fragment.The first line is repeated/incomplete and can be collapsed into one clear summary line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 488 - 490, The doc comment contains a duplicated/incomplete first line; collapse them into a single clear summary sentence that reads something like: "Verify that each honest party's raw decryption share bytes deserialize and match the `d_commitment` output in their verified C6 proof, returning the party IDs..." Edit the comment above the relevant function (reference `d_commitment` and the C6 proof verification in this file) to remove the repeated fragment and keep one concise summary line.
🤖 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/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 499-563: Several pre-check branches skip or continue without
marking the party as mismatched (so bad/missing data can still be treated as
honest); for each failing branch in the loop over (party_id, shares) —
specifically the None cases from c6_proofs.get(party_id), proofs.first(),
shares.first(), the missing d_commitment extract via
c6_output_layout.extract_field, and the Err case from crt.center(&moduli) — add
mismatched.insert(*party_id) before continuing (or return mismatched if the code
path earlier uses an early return like build_pair_for_preset), so failures in
e3_fhe_params::build_pair_for_preset, threshold_params.ctx_at_level(0),
c6_proofs.get, proofs.first, c6_output_layout.extract_field,
try_poly_from_bytes, and crt.center all mark the party via
mismatched.insert(*party_id) instead of silently continuing.
---
Nitpick comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 488-490: The doc comment contains a duplicated/incomplete first
line; collapse them into a single clear summary sentence that reads something
like: "Verify that each honest party's raw decryption share bytes deserialize
and match the `d_commitment` output in their verified C6 proof, returning the
party IDs..." Edit the comment above the relevant function (reference
`d_commitment` and the C6 proof verification in this file) to remove the
repeated fragment and keep one concise summary line.
🪄 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: b786bb3e-d42f-4120-bc83-959093076df3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/aggregator/Cargo.tomlcrates/aggregator/src/threshold_plaintext_aggregator.rscrates/multithread/src/multithread.rscrates/polynomial/src/crt_polynomial.rscrates/zk-helpers/src/circuits/threshold/share_decryption/computation.rstemplates/default/tests/integration.spec.ts
✅ Files skipped from review due to trivial changes (2)
- crates/aggregator/Cargo.toml
- crates/multithread/src/multithread.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
17-19: Unused imports detected.
ProofType,SignedProofFailed, and theerrormacro are imported but not used anywhere in this file. Consider removing them unless they're planned for immediate use.🧹 Suggested cleanup
use e3_events::{ - prelude::*, trap, AggregationProofPending, AggregationProofSigned, BusHandle, CircuitName, + prelude::*, trap, AggregationProofPending, AggregationProofSigned, BusHandle, CircuitName, CommitteeMemberExpelled, ComputeRequest, ComputeResponse, ComputeResponseKind, CorrelationId, - DecryptedSharesAggregationProofRequest, DecryptionshareCreated, Die, E3id, EType, EnclaveEvent, - EnclaveEventData, EventContext, PartyProofsToVerify, PlaintextAggregated, Proof, ProofType, - Seed, Sequenced, ShareVerificationComplete, ShareVerificationDispatched, SignedProofFailed, - SignedProofPayload, TypedEvent, VerificationKind, ZkResponse, + DecryptedSharesAggregationProofRequest, DecryptionshareCreated, Die, E3id, EType, EnclaveEvent, + EnclaveEventData, EventContext, PartyProofsToVerify, PlaintextAggregated, Proof, + Seed, Sequenced, ShareVerificationComplete, ShareVerificationDispatched, + SignedProofPayload, TypedEvent, VerificationKind, ZkResponse, };-use tracing::{debug, error, info, trace, warn}; +use tracing::{debug, info, trace, warn};Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 17 - 19, The import list contains unused symbols—remove ProofType, SignedProofFailed, and the error macro from the top-level use/import statement to eliminate dead imports; locate the use block that currently lists EnclaveEventData, EventContext, PartyProofsToVerify, PlaintextAggregated, Proof, ProofType, Seed, Sequenced, ShareVerificationComplete, ShareVerificationDispatched, SignedProofFailed, SignedProofPayload, TypedEvent, VerificationKind, ZkResponse and delete ProofType, SignedProofFailed and the error macro from that list (or re-add only when you immediately use them).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 17-19: The import list contains unused symbols—remove ProofType,
SignedProofFailed, and the error macro from the top-level use/import statement
to eliminate dead imports; locate the use block that currently lists
EnclaveEventData, EventContext, PartyProofsToVerify, PlaintextAggregated, Proof,
ProofType, Seed, Sequenced, ShareVerificationComplete,
ShareVerificationDispatched, SignedProofFailed, SignedProofPayload, TypedEvent,
VerificationKind, ZkResponse and delete ProofType, SignedProofFailed and the
error macro from that list (or re-add only when you immediately use them).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2003d2b-04db-4a1f-b271-f160aa43fda8
📒 Files selected for processing (2)
crates/aggregator/src/threshold_plaintext_aggregator.rstemplates/default/tests/integration.spec.ts
0xjei
previously approved these changes
Apr 2, 2026
Member
0xjei
left a comment
0xjei
left a comment
There was a problem hiding this comment.
utACK - just left a minor code reuse; but can be ignored
cedoor
previously approved these changes
Apr 2, 2026
ctrlc03
force-pushed
the
chore/pre-c7-check-ag
branch
from
0c28d0b to
63bd400
Compare
cedoor
approved these changes
Apr 2, 2026
ctrlc03
force-pushed
the
chore/pre-c7-check-ag
branch
from
63bd400 to
344a1aa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
501-516:⚠️ Potential issue | 🟠 MajorFail-open on setup failures undermines the pre-check.
When
build_pair_for_preset,C6Bounds::compute, orC6Bits::computefails, the method returns an empty mismatch set, effectively passing all shares through unchecked. Since this is a defense-in-depth check against proof/share substitution attacks, consider failing closed (return all party IDs as mismatched) rather than silently skipping verification.🛡️ Proposed fail-closed fix
fn verify_shares_match_c6_commitments( &self, honest_shares: &[(u64, Vec<ArcBytes>)], c6_proofs: &HashMap<u64, Vec<SignedProofPayload>>, ) -> BTreeSet<u64> { let mut mismatched = BTreeSet::new(); + let all_party_ids: BTreeSet<u64> = honest_shares.iter().map(|(id, _)| *id).collect(); let Ok((threshold_params, _)) = e3_fhe_params::build_pair_for_preset(self.params_preset) else { warn!("Could not build BFV params for d_commitment check — skipping"); - return mismatched; + return all_party_ids; }; let Ok(bounds) = C6Bounds::compute(self.params_preset, &()) else { warn!("Could not compute bounds for d_commitment check — skipping"); - return mismatched; + return all_party_ids; }; let Ok(bits) = C6Bits::compute(self.params_preset, &bounds) else { warn!("Could not compute bits for d_commitment check — skipping"); - return mismatched; + return all_party_ids; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 501 - 516, The current early-returns on Err from e3_fhe_params::build_pair_for_preset, C6Bounds::compute, and C6Bits::compute leave the pre-check in a "fail-open" state by returning the existing (empty) mismatched set; instead, on any such failure populate and return mismatched as the full set of all participant/party IDs (i.e., fail-closed), and include the error details in the warning log. Concretely, inside the Err branches for build_pair_for_preset, C6Bounds::compute, and C6Bits::compute, replace the return mismatched with code that logs the error and constructs mismatched to contain all party IDs from the aggregator state (use whatever field holds participants/party IDs in this struct), then return that full mismatched set.
🤖 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/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 584-597: The code silently truncates big-endian bytes from
computed.to_bytes_be() before padding, which can hide overflow; update the block
handling be_bytes/computed_padded (around computed.to_bytes_be(),
computed_padded, be_bytes, c6_d_bytes, and the mismatched insertion for
party_id) to assert that be_bytes.len() <= 32 (e.g., assert!(be_bytes.len() <=
32, "...")) and then copy the entire be_bytes slice into computed_padded without
the .min(32) truncation so any oversized BigInt fails loudly rather than being
silently truncated.
- Around line 532-558: The current verification only checks the first C6 proof
and share because the code uses proofs.first() and shares.first(); change this
to iterate over all ciphertext indices by zipping proofs and shares (e.g., for
(idx, (proof, share_bytes)) in proofs.iter().zip(shares.iter()).enumerate()),
call c6_output_layout.extract_field(&proof.payload.proof.public_signals,
"d_commitment") for each proof, and perform the commitment verification
per-index; on failure use warn! with the index (e.g., "Could not extract
d_commitment[{}] for party {} — marking as mismatched") and call
mismatched.insert(*party_id) to continue to the next party.
---
Duplicate comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 501-516: The current early-returns on Err from
e3_fhe_params::build_pair_for_preset, C6Bounds::compute, and C6Bits::compute
leave the pre-check in a "fail-open" state by returning the existing (empty)
mismatched set; instead, on any such failure populate and return mismatched as
the full set of all participant/party IDs (i.e., fail-closed), and include the
error details in the warning log. Concretely, inside the Err branches for
build_pair_for_preset, C6Bounds::compute, and C6Bits::compute, replace the
return mismatched with code that logs the error and constructs mismatched to
contain all party IDs from the aggregator state (use whatever field holds
participants/party IDs in this struct), then return that full mismatched set.
🪄 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: c6a028e2-ceea-43bf-a928-a240f414abd3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/aggregator/Cargo.tomlcrates/aggregator/src/threshold_plaintext_aggregator.rscrates/multithread/src/multithread.rscrates/polynomial/src/crt_polynomial.rscrates/zk-helpers/src/circuits/threshold/share_decryption/computation.rstemplates/default/tests/integration.spec.ts
✅ Files skipped from review due to trivial changes (2)
- crates/aggregator/Cargo.toml
- crates/multithread/src/multithread.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- templates/default/tests/integration.spec.ts
- crates/polynomial/src/crt_polynomial.rs
- crates/zk-helpers/src/circuits/threshold/share_decryption/computation.rs
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.
Summary by CodeRabbit
Bug Fixes
Chores
Tests