feat: finalisation proofs [skip-line-limit]#1377
Merged
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 multi-stage threshold decryption and aggregation ZK-proof flows (C6: per-share decryption proofs, C7: decrypted-shares aggregation proofs), new event types and ZK request/response variants, wires proofs through aggregators/keyshares/proof-request actors, and propagates a BFV Changes
Sequence Diagram(s)sequenceDiagram
participant Keyshare as ThresholdKeyshare
participant ProofReq as ProofRequestActor
participant ZK as ZkBackend
participant Aggregator as ThresholdPlaintextAggregator
participant PKAgg as PublicKeyAggregator
Note over Keyshare,Aggregator: C6 (per-share decryption proof) + C7 (aggregation proof) flow
Keyshare->>ProofReq: Publish ShareDecryptionProofPending (C6 request)
ProofReq->>ZK: Send ThresholdShareDecryption ZkRequest
ZK-->>ProofReq: ThresholdShareDecryption ZkResponse (proofs)
ProofReq->>Keyshare: Publish DecryptionshareCreated (signed_decryption_proofs)
Keyshare->>Aggregator: Deliver DecryptionShare (with signed_decryption_proofs)
Aggregator->>Aggregator: collect shares + c6_proofs
Aggregator->>ProofReq: Publish ShareVerificationDispatched / VerifyC6Proofs
ProofReq->>ZK: Send VerifyC6Proofs ZkRequest
ZK-->>ProofReq: VerifyC6Proofs ZkResponse (verification results)
ProofReq->>Aggregator: Publish ShareVerificationComplete (honest parties)
Aggregator->>Aggregator: compute TrBFV with honest shares -> plaintext
Aggregator->>ProofReq: Publish AggregationProofPending (C7 request)
ProofReq->>ZK: Send DecryptedSharesAggregation ZkRequest
ZK-->>ProofReq: DecryptedSharesAggregation ZkResponse (C7 proofs)
ProofReq->>Aggregator: Publish AggregationProofSigned (signed C7 proofs)
Aggregator->>* : Emit PlaintextAggregated (decrypted_output + aggregation_proofs)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
ctrlc03
changed the title
ctrlc03
force-pushed
the
feat/finalisation-proofs
branch
from
70e390b to
78da5d4
Compare
![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: 9
🧹 Nitpick comments (1)
crates/ciphernode-builder/src/ciphernode_builder.rs (1)
481-486: Avoid hardcodingInsecureThreshold512in the builder path.Using a fixed insecure preset here can drift from other preset selections and makes secure rollout harder. Prefer using a configurable preset source (same strategy used for other builder paths).
♻️ Minimal improvement
- let aggregator_preset = BfvPreset::InsecureThreshold512; + let aggregator_preset = DEFAULT_BFV_PRESET;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 481 - 486, The code currently hardcodes BfvPreset::InsecureThreshold512 when adding ThresholdPlaintextAggregatorExtension (see BfvPreset::InsecureThreshold512, e3_builder, ThresholdPlaintextAggregatorExtension::create); replace this hardcoded preset with the same configurable preset source used elsewhere in the builder (e.g., pull the preset from the builder/config or the existing preset variable used by other paths) and pass that preset into ThresholdPlaintextAggregatorExtension::create using the same pattern as other extensions that read from configuration, ensuring you still supply &bus and &sortition.
🤖 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 40-47: The VerifyingC6 state currently omits the durable C6 proof
data, so persist the verifier input by adding a field like c6_proofs:
Vec<ArcBytes> (or verifier_input: ArcBytes) to the VerifyingC6 struct and
include it in serde serialization; update the code paths that
construct/transition into VerifyingC6 (the state transition that currently
builds the temporary snapshot of C6 proofs) to copy the snapshot's C6 proofs
into this new field, and adjust any constructors/serializers/deserializers and
places that read VerifyingC6 (e.g., the snapshot creation and publish/dispatch
logic) so retries can re-dispatch using the persisted c6_proofs instead of only
the ephemeral snapshot.
- Around line 305-316: The threshold check currently uses honest_party_ids.len()
but the code proceeds using honest_shares (from state.shares), allowing
invalid/verifier-only IDs to satisfy the gate; change the check to use
honest_shares.len() (i.e., ensure!(honest_shares.len() > state.threshold_m as
usize, ...)) and update the error message to reference honest shares count
rather than verifier ID count so the gate is based on actual collected shares
(referencing honest_party_ids, honest_shares, state.threshold_m, and
state.shares to locate the change).
- Around line 403-418: Before constructing and publishing PlaintextAggregated,
validate that data.proofs length matches the expected plaintext/shares indices:
check that data.proofs.len() equals state.plaintext.len() (or state.shares.len()
if that represents indices) and return an error (or abort the transition) if
they differ; perform this check immediately before creating the Complete state
in self.state.try_mutate and only set
ThresholdPlaintextAggregatorState::Complete and emit PlaintextAggregated when
the counts match, referencing symbols: state.plaintext, state.shares,
data.proofs, self.state.try_mutate, ThresholdPlaintextAggregatorState::Complete,
and PlaintextAggregated.
In `@crates/events/src/enclave_event/decryptionshare_created.rs`:
- Around line 21-22: The new required field decryption_proofs on the
DecryptionshareCreated event will break deserialization of older persisted
payloads; update the DecryptionshareCreated struct by adding #[serde(default)]
to the decryption_proofs field (pub decryption_proofs: Vec<Proof>) so missing
values default to an empty Vec, matching the pattern used by
ThresholdShareCreated and preserving backward compatibility during
replay/hydration.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 403-407: The aggregated_pk field is only kept in actor memory
(aggregated_pk: Option<ArcBytes>) but is required during hydration and used
later (causing a crash when absent); persist it into the actor's saved state
instead of only assigning self.aggregated_pk in the PublicKeyAggregated handler.
Update the persisted state struct to include aggregated_pk (alongside
pending_c4_verification_shares), change the PublicKeyAggregated event handling
code to write into the persisted state (not just self.aggregated_pk), and ensure
serialization/hydration paths read/write aggregated_pk so restarts restore the
value.
- Around line 1998-2032: The actor transitions into
KeyshareState::GeneratingDecryptionProof (via self.state.try_mutate and the
GeneratingDecryptionProof struct) before calling
self.bus.publish(ComputeRequest::zk(... ZkRequest::ThresholdShareDecryption
...)), which means a publish failure leaves state without enough data to retry;
either move the publish step to happen before the state transition so the state
change only occurs after successful self.bus.publish, or include all fields
required to rebuild the C6 request (e.g., ciphertext_output,
aggregated_pk_bytes, sk_poly_sum, es_poly_sum, d_share_bytes, params_preset and
correlation ids) inside GeneratingDecryptionProof when you transition state so a
failed publish can be retried by reading state and republishing via
self.bus.publish; update the code around self.state.try_mutate,
GeneratingDecryptionProof and the
ComputeRequest::zk/ZkRequest::ThresholdShareDecryption construction accordingly.
In `@crates/multithread/src/multithread.rs`:
- Around line 1148-1171: The current mapping over req.party_proofs treats an
empty c6_proofs vector as vacuously true; update the closure to first check if
party.c6_proofs.is_empty() and immediately return a PartyC6VerificationResult
with all_verified: false for that sender (use sender = party.sender_party_id),
otherwise iterate and call prover.verify_proof(c6_proof, &e3_id_str, sender) as
before and set all_verified true only if at least one proof was present and all
verify_proof calls returned Ok(true).
- Around line 1194-1209: The code indexes shares[i] without verifying each
party's shares vector length, which can panic; before calling
try_poly_from_bytes (or before the per-index loop), validate that every entry in
req.d_share_polys has shares.len() == num_indices and return an
Err(make_zk_error(&request, format!(...))) if any mismatch is found. You can
either add a pre-check iterating req.d_share_polys to assert lengths, or inside
the loop check each (_, shares) and early-return a descriptive zk error
referencing the index i and the offending party, then proceed to call
try_poly_from_bytes only after the length is confirmed.
- Around line 358-383: The loop in multithread.rs indexes req.es_poly_sum and
req.d_share_bytes without guards and can panic when es_poly_sum is empty or
d_share_bytes is shorter than ciphertext_bytes; add explicit bounds checks
before the loop (or at loop start) that validate req.es_poly_sum.len() > 0 and
req.d_share_bytes.len() >= req.ciphertext_bytes.len(), returning a typed
ComputeRequestError (via make_zk_error or a new ComputeRequestError variant)
with clear messages like "empty es_poly_sum" or "d_share_bytes too short"
instead of letting the modulo/indexing panic; update any callers or error
conversions if needed so make_zk_error produces the correct ComputeRequestError
type.
---
Nitpick comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 481-486: The code currently hardcodes
BfvPreset::InsecureThreshold512 when adding
ThresholdPlaintextAggregatorExtension (see BfvPreset::InsecureThreshold512,
e3_builder, ThresholdPlaintextAggregatorExtension::create); replace this
hardcoded preset with the same configurable preset source used elsewhere in the
builder (e.g., pull the preset from the builder/config or the existing preset
variable used by other paths) and pass that preset into
ThresholdPlaintextAggregatorExtension::create using the same pattern as other
extensions that read from configuration, ensuring you still supply &bus and
&sortition.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/aggregator/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryptionshare_created.rscrates/events/src/enclave_event/plaintext_aggregated.rscrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/Cargo.tomlcrates/multithread/src/multithread.rscrates/tests/tests/integration.rstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.ts
ctrlc03
force-pushed
the
feat/finalisation-proofs
branch
from
78da5d4 to
d70ff69
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/keyshare/src/threshold_keyshare.rs (1)
1984-1988: Defensive: Consider clearer error message whenaggregated_pkis missing.The fallback chain
self.aggregated_pk.clone().or_else(|| state.aggregated_pk.clone())is correct, but the error message could indicate which source was checked.💡 Optional improvement
let aggregated_pk_bytes = self .aggregated_pk .clone() .or_else(|| state.aggregated_pk.clone()) - .ok_or_else(|| anyhow!("Aggregated public key not available for C6 proof"))?; + .ok_or_else(|| anyhow!( + "Aggregated public key not available for C6 proof (neither transient nor persisted)" + ))?;🤖 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 1984 - 1988, The code fetching aggregated_pk for the C6 proof should produce a clearer error when missing; update the ok_or_else call on the aggregated_pk retrieval (the expression assigning aggregated_pk_bytes that uses self.aggregated_pk.clone().or_else(|| state.aggregated_pk.clone())) to return an error string that indicates both sources were checked (e.g., mention "self.aggregated_pk" and "state.aggregated_pk" and that neither was present) so debugging shows which fields were inspected for the C6 proof.crates/events/src/enclave_event/compute_request/zk.rs (1)
407-414: Consider adding fault attribution data toPartyC6VerificationResult.Unlike
PartyVerificationResult(used for C2/C3/C4),PartyC6VerificationResultlacksfailed_signed_payloadandrecovered_addressfields. If C6 proofs will need fault attribution in the future, adding these fields now would maintain consistency and avoid a breaking change later.💡 Optional enhancement for consistency
pub struct PartyC6VerificationResult { /// The party whose C6 proofs were verified. pub sender_party_id: u64, /// Whether ALL C6 proofs from this party verified successfully. pub all_verified: bool, + /// Index of the first failed proof (if any), for fault attribution. + pub failed_proof_index: Option<usize>, }🤖 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 407 - 414, PartyC6VerificationResult currently only has sender_party_id and all_verified, so it cannot carry fault-attribution info like PartyVerificationResult; to fix, extend struct PartyC6VerificationResult by adding the same optional fields failed_signed_payload (e.g., Option<Vec<u8>> or appropriate payload type) and recovered_address (e.g., Option<AccountAddress/Vec<u8>> or the project’s address type), update any serde/derive uses and constructors/factory functions that build PartyC6VerificationResult to populate these fields (defaulting to None when not present) and adjust places that pattern-match or construct PartyC6VerificationResult accordingly so the new optional fields are handled.crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
606-609: Clarify:dispatch_c6_verificationcalled after state transition.The dispatch happens after
add_sharetransitions toVerifyingC6. Ifdispatch_c6_verificationfails (line 609), the state has already transitioned but no verification was dispatched. However, sincec6_proofsis now persisted inVerifyingC6, a retry mechanism could re-read from state and re-dispatch.Consider adding a comment explaining the recovery path, or restructuring to dispatch before confirming the state check.
🤖 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 606 - 609, The call to dispatch_c6_verification(...) happens after add_share transitions self.state to ThresholdPlaintextAggregatorState::VerifyingC6, so if dispatch_c6_verification fails the state will remain VerifyingC6 with persisted c6_proofs but no verification sent; update the code to either (A) attempt dispatch before committing the state transition or (B) add a clear recovery comment and implement a retry path that re-reads self.state and re-dispatches when in VerifyingC6 (e.g., from the state machine entry point), and ensure dispatch_c6_verification errors are surfaced/handled so callers can retry or trigger the re-dispatch; reference ThresholdPlaintextAggregatorState::VerifyingC6, add_share, and dispatch_c6_verification when making the change.
🤖 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 606-609: The call to dispatch_c6_verification(...) happens after
add_share transitions self.state to
ThresholdPlaintextAggregatorState::VerifyingC6, so if dispatch_c6_verification
fails the state will remain VerifyingC6 with persisted c6_proofs but no
verification sent; update the code to either (A) attempt dispatch before
committing the state transition or (B) add a clear recovery comment and
implement a retry path that re-reads self.state and re-dispatches when in
VerifyingC6 (e.g., from the state machine entry point), and ensure
dispatch_c6_verification errors are surfaced/handled so callers can retry or
trigger the re-dispatch; reference
ThresholdPlaintextAggregatorState::VerifyingC6, add_share, and
dispatch_c6_verification when making the change.
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 407-414: PartyC6VerificationResult currently only has
sender_party_id and all_verified, so it cannot carry fault-attribution info like
PartyVerificationResult; to fix, extend struct PartyC6VerificationResult by
adding the same optional fields failed_signed_payload (e.g., Option<Vec<u8>> or
appropriate payload type) and recovered_address (e.g.,
Option<AccountAddress/Vec<u8>> or the project’s address type), update any
serde/derive uses and constructors/factory functions that build
PartyC6VerificationResult to populate these fields (defaulting to None when not
present) and adjust places that pattern-match or construct
PartyC6VerificationResult accordingly so the new optional fields are handled.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1984-1988: The code fetching aggregated_pk for the C6 proof should
produce a clearer error when missing; update the ok_or_else call on the
aggregated_pk retrieval (the expression assigning aggregated_pk_bytes that uses
self.aggregated_pk.clone().or_else(|| state.aggregated_pk.clone())) to return an
error string that indicates both sources were checked (e.g., mention
"self.aggregated_pk" and "state.aggregated_pk" and that neither was present) so
debugging shows which fields were inspected for the C6 proof.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/aggregator/src/ext.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryptionshare_created.rscrates/events/src/enclave_event/plaintext_aggregated.rscrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/Cargo.tomlcrates/multithread/src/multithread.rscrates/tests/tests/integration.rstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/multithread/Cargo.toml
- crates/aggregator/src/ext.rs
- crates/keyshare/src/threshold_share_collector.rs
- crates/events/src/enclave_event/compute_request/mod.rs
- templates/default/enclave.config.yaml
- crates/multithread/src/multithread.rs
- templates/default/tests/integration.spec.ts
- crates/ciphernode-builder/src/ciphernode_builder.rs
- crates/events/src/enclave_event/plaintext_aggregated.rs
ctrlc03
changed the title
Collaborator
|
Thanks @ctrlc03 , for putting this together. Before I do a full review, I think we need to align the T4/T5/T6 orchestration with the existing actor responsibilities. Right now C0–C4 all follow the same pipeline But T4/T5/T6 bypass both of these actors, |
cedoor
reviewed
Mar 4, 2026
ctrlc03
force-pushed
the
feat/finalisation-proofs
branch
from
d70ff69 to
ec9de8f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
221-245:⚠️ Potential issue | 🟠 MajorCurrent C6 flow can dead-end on first dishonest share set (no recovery path).
At Line 233, the actor leaves
Collectingas soon asm+1shares arrive. Then Line 309 hard-fails if any of that first batch is dishonest. That creates a liveness failure even when enough honest shares may still arrive later.💡 Suggested direction (recover instead of hard-fail)
pub struct VerifyingC6 { threshold_m: u64, threshold_n: u64, shares: HashMap<u64, Vec<ArcBytes>>, c6_proofs: HashMap<u64, Vec<SignedProofPayload>>, + seed: Seed, ciphertext_output: Vec<ArcBytes>, params: ArcBytes, } // transition Collecting -> VerifyingC6 Ok(ThresholdPlaintextAggregatorState::VerifyingC6( VerifyingC6 { shares, c6_proofs, + seed: current.seed, ciphertext_output, threshold_m, threshold_n, params, }, )) // in handle_c6_verification_complete -ensure!( - honest_shares.len() > state.threshold_m as usize, - "Not enough honest shares after C6 verification: {} honest shares, {} required", - honest_shares.len(), - state.threshold_m + 1 -); +if honest_shares.len() <= state.threshold_m as usize { + let honest_share_map: HashMap<u64, Vec<ArcBytes>> = state + .shares + .iter() + .filter(|(id, _)| !dishonest_parties.contains(id)) + .map(|(id, s)| (*id, s.clone())) + .collect(); + let honest_proof_map: HashMap<u64, Vec<SignedProofPayload>> = state + .c6_proofs + .iter() + .filter(|(id, _)| !dishonest_parties.contains(id)) + .map(|(id, p)| (*id, p.clone())) + .collect(); + + self.state.try_mutate(&ec, |_| { + Ok(ThresholdPlaintextAggregatorState::Collecting(Collecting { + threshold_m: state.threshold_m, + threshold_n: state.threshold_n, + shares: honest_share_map, + c6_proofs: honest_proof_map, + seed: state.seed.clone(), + ciphertext_output: state.ciphertext_output.clone(), + params: state.params.clone(), + })) + })?; + return Ok(()); +}Also applies to: 309-314
🤖 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 221 - 245, The code currently transitions from ThresholdPlaintextAggregatorState::Collecting to VerifyingC6 as soon as shares.len() > threshold_m, and then hard-fails if any share in that first batch is dishonest; change this to a recoverable flow: when verifying in the VerifyingC6 handling (the block that examines shares and c6_proofs), do not hard-return on the first invalid proof—instead remove or mark invalid shares/c6_proofs, log the invalid IDs, and transition back to ThresholdPlaintextAggregatorState::Collecting (recreating a Collecting with the remaining valid shares, remaining c6_proofs, same params/seed/thresholds) so the actor can accept further shares until a valid set of threshold_m+1 honest shares is collected or a timeout occurs; update the logic around the VerifyingC6 handler (the code that currently panics/returns Err at the first dishonesty, referenced by the VerifyingC6 struct and the code paths that examine shares and c6_proofs) to perform incremental verification, removal of invalid entries, and safe state transition back to Collecting rather than hard-failing.
♻️ Duplicate comments (3)
crates/events/src/enclave_event/decryptionshare_created.rs (1)
21-22:⚠️ Potential issue | 🟠 MajorMake the new proofs field serde-compatible with historical events.
Line 22 adds a required field to a serialized event struct. Add
#[serde(default)]so older payloads without this field still deserialize.💡 Suggested fix
pub struct DecryptionshareCreated { pub party_id: u64, pub decryption_share: Vec<ArcBytes>, // per index depending on what is required for the // ciphertext pub e3_id: E3id, pub node: String, /// C6 proofs: one signed proof of correct decryption per ciphertext index. + #[serde(default)] pub signed_decryption_proofs: Vec<SignedProofPayload>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/decryptionshare_created.rs` around lines 21 - 22, The new required field signed_decryption_proofs on the DecryptionShareCreated event will break deserialization of historical events; add a serde default so older payloads without this field still deserialize by annotating the signed_decryption_proofs field with #[serde(default)] (keep the field type Vec<SignedProofPayload> unchanged) in the DecryptionShareCreated struct in decryptionshare_created.rs so missing values become an empty Vec on deserialize.crates/multithread/src/multithread.rs (1)
359-385:⚠️ Potential issue | 🟠 MajorValidate
es_poly_sumcardinality before modulo reuse.Line 384 currently accepts any non-zero length and reuses entries via modulo. This can silently accept malformed C6 inputs and build mismatched witnesses. Validate
es_poly_sum.len()is either1ornum_indices.💡 Suggested fix
let num_indices = req.ciphertext_bytes.len(); if req.es_poly_sum.is_empty() { return Err(make_zk_error(&request, "empty es_poly_sum".to_string())); } + if req.es_poly_sum.len() != 1 && req.es_poly_sum.len() != num_indices { + return Err(make_zk_error( + &request, + format!( + "es_poly_sum length mismatch: expected 1 or {}, got {}", + num_indices, + req.es_poly_sum.len() + ), + )); + } if req.d_share_bytes.len() < num_indices { return Err(make_zk_error( &request,🤖 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 359 - 385, The code currently allows any non-zero es_poly_sum length and reuses entries via es_idx = i % req.es_poly_sum.len(), which can silently accept malformed inputs; update validation before the loop to require req.es_poly_sum.len() == 1 || req.es_poly_sum.len() == num_indices and return a ZK error otherwise (use make_zk_error with a clear message), so the loop that uses es_idx and e3_trbfv::helpers::try_poly_from_sensitive_bytes only runs when the cardinality is valid; locate validation near the existing checks for req.es_poly_sum.is_empty() and req.d_share_bytes.len() and add the new check referencing es_poly_sum and num_indices.crates/zk-prover/src/actors/share_verification.rs (1)
102-106:⚠️ Potential issue | 🟠 MajorFail closed when a party provides zero signed proofs.
With kind expansion on Line 102-105, an empty
signed_proofsvector can still pass this path. Add an explicit empty check before ECDSA/ZK dispatch and mark that party dishonest.💡 Suggested fix
for party in &party_proofs { + if party.signed_proofs.is_empty() { + ecdsa_dishonest.insert(party.sender_party_id); + continue; + } + let result = self.ecdsa_validate_signed_proofs( party.sender_party_id, &party.signed_proofs, &e3_id_str, label, );Also applies to: 133-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/share_verification.rs` around lines 102 - 106, The code currently dispatches to verify_share_proofs for VerificationKind::ShareProofs, ::ThresholdDecryptionProofs, and ::PkGenerationProofs without guarding against an empty msg.share_proofs; add an explicit check that msg.share_proofs.is_empty() before calling self.verify_share_proofs and if empty treat the sender as dishonest (set/flag using the same mechanism as msg.pre_dishonest) and skip verification; apply the same empty-check + dishonest-marking change to the other similar dispatch block referenced (the block around the 133-141 region) so empty signed_proofs vectors are always treated as dishonest instead of proceeding to ECDSA/ZK verification.
🧹 Nitpick comments (2)
crates/events/src/enclave_event/signed_proof.rs (1)
70-70: Add tests for the newC5PkAggregationmappings.Line 70 and Line 86 add new behavior, but this file’s tests don’t assert the C5 circuit/slash mappings yet.
✅ Proposed test additions
#[test] fn proof_type_circuit_names_mapping() { @@ assert_eq!( ProofType::T6DecryptedSharesAggregation.circuit_names(), vec![ CircuitName::DecryptedSharesAggregationBn, CircuitName::DecryptedSharesAggregationMod, ] ); + assert_eq!( + ProofType::C5PkAggregation.circuit_names(), + vec![CircuitName::PkAggregation] + ); + assert_eq!( + ProofType::C5PkAggregation.slash_reason(), + "E3_BAD_PK_AGGREGATION_PROOF" + ); }Also applies to: 86-86
🤖 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` at line 70, Add unit tests that assert the new ProofType::C5PkAggregation maps to the expected circuit and slash entries: create tests in the signed_proof.rs test module that call the mapping function(s) handling ProofType (the code paths around the match that returns vec![CircuitName::PkAggregation]) and verify the returned Vec contains CircuitName::PkAggregation; also add a corresponding test for the slash mapping behavior added around line 86 (assert the slash mapping includes the same PkAggregation entry). Use the existing test style in this file (same helpers/assert macros) and reference ProofType::C5PkAggregation and CircuitName::PkAggregation in your assertions.crates/ciphernode-builder/src/ciphernode_builder.rs (1)
471-496: Use one preset source for both aggregator extensions.Line 471 uses
DEFAULT_BFV_PRESET, while Line 491 hardcodesInsecureThreshold512. Unifying this reduces C5/C7 config drift risk.💡 Suggested refactor
- let aggregator_preset = BfvPreset::InsecureThreshold512; + let aggregator_preset = DEFAULT_BFV_PRESET; e3_builder = e3_builder.with(ThresholdPlaintextAggregatorExtension::create( &bus, &sortition, aggregator_preset, ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 471 - 496, The code uses two different BFV presets for aggregator extensions (DEFAULT_BFV_PRESET for PublicKeyAggregatorExtension::create and BfvPreset::InsecureThreshold512 for ThresholdPlaintextAggregatorExtension::create); unify them by selecting a single preset source (e.g., reuse DEFAULT_BFV_PRESET or introduce a single AGGREGATOR_BFV_PRESET constant) and use that same aggregator_preset variable when calling both PublicKeyAggregatorExtension::create and ThresholdPlaintextAggregatorExtension::create so both extensions share the identical preset.
🤖 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 282-284: The handler currently only checks msg.kind ==
VerificationKind::ThresholdDecryptionProofs before mutating C6/C7 state, and
later consumes AggregationProofSigned without verifying the message's e3_id; add
explicit e3_id scoping checks (compare msg.e3_id or the
AggregationProofSigned.e3_id with the actor's current e3_id) at the start of the
ThresholdDecryptionProofs branch and before consuming AggregationProofSigned so
that only events matching the actor's e3_id can drive C6/C7 transitions; update
the branches that process AggregationProofSigned and any places that route
events into this actor (the same logic used where msg.kind is checked) to
early-return if e3_id does not match.
In `@crates/events/src/enclave_event/aggregation_proof_pending.rs`:
- Around line 23-24: Ensure you validate alignment between the struct fields
plaintext (Vec<ArcBytes>) and shares (Vec<(u64, Vec<ArcBytes>)>) before
dispatching the C7 proof: iterate each entry in shares and assert that its inner
Vec<ArcBytes>.len() equals plaintext.len(), returning an early Err/logging and
aborting dispatch if any mismatch is found; add this check at the start of the
C7-proof dispatch code path (the code that uses aggregation_proof_pending's
plaintext and shares) so malformed witnesses are rejected before calling the
proof routines.
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 223-234: The three per-output vectors ciphertext_bytes,
es_poly_sum, and d_share_bytes can be length-mismatched; add a request-level
validation that enforces ciphertext_bytes.len() == es_poly_sum.len() ==
d_share_bytes.len() before any processing (e.g., in the struct constructor, a
TryFrom/validate method, or an impl for TryInto/FromRequest used to build the
request) and return a clear error if they differ; reference the fields
ciphertext_bytes, es_poly_sum, and d_share_bytes and make sure all call sites
that construct this request use the new validator or TryFrom so the invariant is
guaranteed at creation time.
- Around line 385-393: Add C7 semantic validation at the request boundary to
reject invalid thresholds and mismatched per-ciphertext share shapes: ensure
threshold_m > 0 and threshold_m <= threshold_n by validating the fields
threshold_m and threshold_n, and ensure each entry of d_share_polys (the
Vec<(u64, Vec<ArcBytes>)>) has its inner Vec length equal to plaintext.len()
(i.e., every party provides one ArcBytes per ciphertext); perform these checks
in the request construction/validation path (e.g., the struct's validate() or
TryFrom/deserialize handler in this module) and return an error when any check
fails.
In `@crates/events/src/enclave_event/plaintext_aggregated.rs`:
- Around line 20-21: The new required field aggregation_proofs on
PlaintextAggregated breaks deserialization of older events; to restore
replay/backward compatibility annotate the aggregation_proofs field with
#[serde(default)] so missing values deserialize as an empty Vec; update the
aggregation_proofs declaration in the PlaintextAggregated struct (same symbol
name) analogous to other event structs like threshold_share_created.rs that
already use #[serde(default)].
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 50-51: Update the enum documentation for ProofType to remove the
stale statement that aggregation proofs are excluded and instead state that
aggregation proofs are included (e.g., reflect that C5PkAggregation = 9
represents the public key aggregation proof). Edit the doc comment above the
ProofType enum (and any doc comment near the C5PkAggregation variant) to
accurately describe that aggregation-proof inclusion is supported and what
C5PkAggregation represents.
- Line 86: The new enum mapping ProofType::C5PkAggregation =>
"E3_BAD_PK_AGGREGATION_PROOF" is never exercised because slash_reason() is not
invoked by the slashing flow or tests; add wiring and tests: ensure any code
path that creates a slashing entry for bad PK aggregation calls slash_reason()
(or otherwise uses the same string constant) so the new variant is emitted, add
a corresponding test constant in CommitteeExpulsion.spec.ts (e.g. const
E3_BAD_PK_AGGREGATION_PROOF =
keccak256(toUtf8Bytes("E3_BAD_PK_AGGREGATION_PROOF"))), include that hashed
reason in the contract initialization/slash policy setup used by the spec, and
extend CommitteeExpulsion.spec.ts to assert the new reason is recognized/causes
the expected slashing behavior; update any contract mapping/initializer that
seeds slash reasons so the new hashed constant is present.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 2074-2081: The handler for EnclaveEventData::PublicKeyAggregated
is currently ignoring the Result from self.state.try_mutate, which can drop
persistence failures and later break C6 construction; change the code in the
EnclaveEventData::PublicKeyAggregated branch to check the Result from
self.state.try_mutate(&ec, |mut s| { s.aggregated_pk = Some(pk); Ok(s) }), and
on Err either return or propagate the error (or log the error and return Err) so
failures are not silently ignored; ensure the aggregated_pk persistence error is
handled (using the same logging/Err pattern used elsewhere in this module) so
downstream code that expects an persisted aggregated_pk for C6 construction will
not run with missing data.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 686-743: In handle_share_decryption_proof_response, validate that
the received proofs vector has the expected count before signing/publishing:
compare proofs.len() against the expected number stored on the pending entry
(e.g., a field on pending such as expected_proof_count or expected_proofs_len),
and if they differ log an error (including e3_id and pending.party_id) and
return without signing or publishing DecryptionshareCreated /
DecryptionShareProofSigned; do this check immediately after obtaining pending
and before the signing loop that calls sign_proof and before publishing those
two events.
---
Outside diff comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 221-245: The code currently transitions from
ThresholdPlaintextAggregatorState::Collecting to VerifyingC6 as soon as
shares.len() > threshold_m, and then hard-fails if any share in that first batch
is dishonest; change this to a recoverable flow: when verifying in the
VerifyingC6 handling (the block that examines shares and c6_proofs), do not
hard-return on the first invalid proof—instead remove or mark invalid
shares/c6_proofs, log the invalid IDs, and transition back to
ThresholdPlaintextAggregatorState::Collecting (recreating a Collecting with the
remaining valid shares, remaining c6_proofs, same params/seed/thresholds) so the
actor can accept further shares until a valid set of threshold_m+1 honest shares
is collected or a timeout occurs; update the logic around the VerifyingC6
handler (the code that currently panics/returns Err at the first dishonesty,
referenced by the VerifyingC6 struct and the code paths that examine shares and
c6_proofs) to perform incremental verification, removal of invalid entries, and
safe state transition back to Collecting rather than hard-failing.
---
Duplicate comments:
In `@crates/events/src/enclave_event/decryptionshare_created.rs`:
- Around line 21-22: The new required field signed_decryption_proofs on the
DecryptionShareCreated event will break deserialization of historical events;
add a serde default so older payloads without this field still deserialize by
annotating the signed_decryption_proofs field with #[serde(default)] (keep the
field type Vec<SignedProofPayload> unchanged) in the DecryptionShareCreated
struct in decryptionshare_created.rs so missing values become an empty Vec on
deserialize.
In `@crates/multithread/src/multithread.rs`:
- Around line 359-385: The code currently allows any non-zero es_poly_sum length
and reuses entries via es_idx = i % req.es_poly_sum.len(), which can silently
accept malformed inputs; update validation before the loop to require
req.es_poly_sum.len() == 1 || req.es_poly_sum.len() == num_indices and return a
ZK error otherwise (use make_zk_error with a clear message), so the loop that
uses es_idx and e3_trbfv::helpers::try_poly_from_sensitive_bytes only runs when
the cardinality is valid; locate validation near the existing checks for
req.es_poly_sum.is_empty() and req.d_share_bytes.len() and add the new check
referencing es_poly_sum and num_indices.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 102-106: The code currently dispatches to verify_share_proofs for
VerificationKind::ShareProofs, ::ThresholdDecryptionProofs, and
::PkGenerationProofs without guarding against an empty msg.share_proofs; add an
explicit check that msg.share_proofs.is_empty() before calling
self.verify_share_proofs and if empty treat the sender as dishonest (set/flag
using the same mechanism as msg.pre_dishonest) and skip verification; apply the
same empty-check + dishonest-marking change to the other similar dispatch block
referenced (the block around the 133-141 region) so empty signed_proofs vectors
are always treated as dishonest instead of proceeding to ECDSA/ZK verification.
---
Nitpick comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 471-496: The code uses two different BFV presets for aggregator
extensions (DEFAULT_BFV_PRESET for PublicKeyAggregatorExtension::create and
BfvPreset::InsecureThreshold512 for
ThresholdPlaintextAggregatorExtension::create); unify them by selecting a single
preset source (e.g., reuse DEFAULT_BFV_PRESET or introduce a single
AGGREGATOR_BFV_PRESET constant) and use that same aggregator_preset variable
when calling both PublicKeyAggregatorExtension::create and
ThresholdPlaintextAggregatorExtension::create so both extensions share the
identical preset.
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Line 70: Add unit tests that assert the new ProofType::C5PkAggregation maps to
the expected circuit and slash entries: create tests in the signed_proof.rs test
module that call the mapping function(s) handling ProofType (the code paths
around the match that returns vec![CircuitName::PkAggregation]) and verify the
returned Vec contains CircuitName::PkAggregation; also add a corresponding test
for the slash mapping behavior added around line 86 (assert the slash mapping
includes the same PkAggregation entry). Use the existing test style in this file
(same helpers/assert macros) and reference ProofType::C5PkAggregation and
CircuitName::PkAggregation in your assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a68c3a85-bfd0-463e-8b08-0a7f24e1fed0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
crates/aggregator/src/ext.rscrates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/aggregation_proof_pending.rscrates/events/src/enclave_event/aggregation_proof_signed.rscrates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryption_share_proof_signed.rscrates/events/src/enclave_event/decryptionshare_created.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/pk_aggregation_proof_pending.rscrates/events/src/enclave_event/pk_aggregation_proof_signed.rscrates/events/src/enclave_event/plaintext_aggregated.rscrates/events/src/enclave_event/share_decryption_proof_pending.rscrates/events/src/enclave_event/share_verification.rscrates/events/src/enclave_event/signed_proof.rscrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/Cargo.tomlcrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/share_verification.rstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/default/enclave.config.yaml
- templates/default/tests/integration.spec.ts
- templates/default/deployed_contracts.json
- crates/multithread/Cargo.toml
Contributor
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/aggregator/src/publickey_aggregator.rs (1)
322-341:⚠️ Potential issue | 🟠 MajorMissing
e3_idvalidation before processing the signed proof event.Similar to the previous handler, this doesn't verify that
msg.e3_idmatchesself.e3_id. Per the context snippet,PkAggregationProofSignedcontains ane3_idfield that should be validated.Proposed fix
fn handle_pk_aggregation_proof_signed( &mut self, msg: TypedEvent<PkAggregationProofSigned>, ) -> Result<()> { let (msg, ec) = msg.into_components(); + if msg.e3_id != self.e3_id { + return Ok(()); + } + let PublicKeyAggregatorState::GeneratingC5Proof {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 322 - 341, In handle_pk_aggregation_proof_signed, validate that the incoming event's e3_id matches the aggregator's expected self.e3_id before continuing: after extracting (msg, ec) from TypedEvent<PkAggregationProofSigned>, compare msg.e3_id to self.e3_id and return an Err (with a clear message) if they differ; keep this check analogous to the previous handler so invalid/mismatched events are rejected early and do not proceed to the GeneratingC5Proof state handling.
♻️ Duplicate comments (2)
crates/aggregator/src/threshold_plaintext_aggregator.rs (2)
276-284:⚠️ Potential issue | 🟠 MajorAdd
e3_idscoping check before processing C6 verification.The handler only filters by
msg.kind(line 282) without verifying thatmsg.e3_idmatchesself.e3_id. SinceShareVerificationCompleteevents are routed directly to this actor (lines 531-533), a foreign event from a different E3 could incorrectly mutate this actor's state.🛡️ Proposed fix
pub fn handle_c6_verification_complete( &mut self, msg: TypedEvent<ShareVerificationComplete>, ) -> Result<()> { let (msg, ec) = msg.into_components(); + if msg.e3_id != self.e3_id { + return Ok(()); + } + if msg.kind != VerificationKind::ThresholdDecryptionProofs { return Ok(()); }🤖 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 276 - 284, The handler handle_c6_verification_complete currently only checks msg.kind; add a guard that compares msg.e3_id with self.e3_id and return Ok(()) if they differ so events for other E3 instances are ignored; locate the check in handle_c6_verification_complete (and use the ShareVerificationComplete/msg.e3_id and the aggregator's self.e3_id) and perform this equality check before any state mutation or further processing.
418-428:⚠️ Potential issue | 🟠 MajorAdd
e3_idscoping check before processing C7 aggregation proof.Similar to the C6 handler, this handler processes
AggregationProofSignedwithout verifying thatmsg.e3_idmatchesself.e3_id. Per the context snippet,AggregationProofSignedincludes ane3_idfield that should be validated.🛡️ Proposed fix
pub fn handle_aggregation_proof_signed( &mut self, msg: TypedEvent<AggregationProofSigned>, ) -> Result<()> { let (msg, ec) = msg.into_components(); + + if msg.e3_id != self.e3_id { + return Ok(()); + } + let state: GeneratingC7Proof = self .state .get() .ok_or(anyhow!("Could not get state"))? .try_into()?;🤖 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 418 - 428, The handler handle_aggregation_proof_signed currently processes AggregationProofSigned without verifying the e3_id; add a guard at the start (after extracting msg and state in handle_aggregation_proof_signed) to compare msg.e3_id with self.e3_id and return an error (or early Ok) if they differ. Ensure this check occurs before using the GeneratingC7Proof state or performing any further processing so only matching e3_id messages are accepted.
🧹 Nitpick comments (1)
crates/aggregator/src/publickey_aggregator.rs (1)
307-311: Consider documenting unused fields at the struct level.The inline comments explaining that
committee_nandcommittee_thresholdare unused by the circuit are helpful here, but if these fields are truly not needed, consider adding documentation onPkAggregationProofRequestitself or removing them from the struct to avoid confusion for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 307 - 311, The inline comments indicate that the fields committee_n and committee_threshold of PkAggregationProofRequest are unused by the circuit; update the code by either (A) adding clear struct-level documentation on PkAggregationProofRequest explaining that committee_n and committee_threshold are present for compatibility/serialization only and are unused by the circuit (mention committee_h as the real source of truth), or (B) remove committee_n and committee_threshold from the PkAggregationProofRequest type and then update all constructors/usages (places that build the struct with committee_n, committee_h, or committee_threshold) to stop supplying them; choose one approach and make sure to adjust any related deserialization/compatibility code accordingly.
🤖 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/publickey_aggregator.rs`:
- Around line 211-219: In handle_c1_verification_complete you currently only
filter by VerificationKind::PkGenerationProofs; add an e3_id check to ensure
msg.e3_id == self.e3_id before processing. Specifically, in the method
handle_c1_verification_complete(&mut self, msg:
TypedEvent<ShareVerificationComplete>) -> Result<()>, after extracting (msg, ec)
and after the kind check, compare msg.e3_id to self.e3_id and return Ok(())
(optionally log/debug) if they differ so you don't handle events for other E3
computations.
---
Outside diff comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 322-341: In handle_pk_aggregation_proof_signed, validate that the
incoming event's e3_id matches the aggregator's expected self.e3_id before
continuing: after extracting (msg, ec) from
TypedEvent<PkAggregationProofSigned>, compare msg.e3_id to self.e3_id and return
an Err (with a clear message) if they differ; keep this check analogous to the
previous handler so invalid/mismatched events are rejected early and do not
proceed to the GeneratingC5Proof state handling.
---
Duplicate comments:
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 276-284: The handler handle_c6_verification_complete currently
only checks msg.kind; add a guard that compares msg.e3_id with self.e3_id and
return Ok(()) if they differ so events for other E3 instances are ignored;
locate the check in handle_c6_verification_complete (and use the
ShareVerificationComplete/msg.e3_id and the aggregator's self.e3_id) and perform
this equality check before any state mutation or further processing.
- Around line 418-428: The handler handle_aggregation_proof_signed currently
processes AggregationProofSigned without verifying the e3_id; add a guard at the
start (after extracting msg and state in handle_aggregation_proof_signed) to
compare msg.e3_id with self.e3_id and return an error (or early Ok) if they
differ. Ensure this check occurs before using the GeneratingC7Proof state or
performing any further processing so only matching e3_id messages are accepted.
---
Nitpick comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 307-311: The inline comments indicate that the fields committee_n
and committee_threshold of PkAggregationProofRequest are unused by the circuit;
update the code by either (A) adding clear struct-level documentation on
PkAggregationProofRequest explaining that committee_n and committee_threshold
are present for compatibility/serialization only and are unused by the circuit
(mention committee_h as the real source of truth), or (B) remove committee_n and
committee_threshold from the PkAggregationProofRequest type and then update all
constructors/usages (places that build the struct with committee_n, committee_h,
or committee_threshold) to stop supplying them; choose one approach and make
sure to adjust any related deserialization/compatibility code accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de4337ce-f211-4681-bfd9-78eb6b535f09
📒 Files selected for processing (2)
crates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/multithread/src/multithread.rs (2)
1167-1178:⚠️ Potential issue | 🟠 MajorUse exact per-party share length checks in C7.
Line 1168 only rejects short vectors; oversized vectors are accepted and silently truncated at Line 1189. Prefer exact length equality to avoid proving over partially ignored payloads.
💡 Suggested fix
- for (party_id, shares) in &req.d_share_polys { - if shares.len() < num_indices { + for (party_id, shares) in &req.d_share_polys { + if shares.len() != num_indices { return Err(make_zk_error( &request, format!( - "party {} has {} shares but {} expected", + "party {} has {} shares but exactly {} expected", party_id, shares.len(), num_indices ), )); } }Also applies to: 1186-1190
🤖 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 1167 - 1178, The check in the C7 verification loop currently only rejects too-short share vectors and allows oversized vectors that get truncated later; update the validation in the loop over req.d_share_polys (and the analogous check at the other block around the second validation) to require exact equality (shares.len() == num_indices) instead of just length >= num_indices, returning the same make_zk_error when sizes differ so oversized payloads are rejected rather than silently truncated.
358-369:⚠️ Potential issue | 🟠 MajorTighten C6 witness cardinality validation before modulo/index access.
Line 383 silently reuses
es_poly_sumentries via modulo when length is neither1nornum_indices, and Line 361 permits extrad_share_bytes. Please enforce exact expected shapes up front.💡 Suggested fix
let num_indices = req.ciphertext_bytes.len(); if req.es_poly_sum.is_empty() { return Err(make_zk_error(&request, "empty es_poly_sum".to_string())); } - if req.d_share_bytes.len() < num_indices { + if req.d_share_bytes.len() != num_indices { return Err(make_zk_error( &request, format!( - "d_share_bytes too short: {} < {}", + "d_share_bytes length mismatch: {} != {}", req.d_share_bytes.len(), num_indices ), )); } + if req.es_poly_sum.len() != 1 && req.es_poly_sum.len() != num_indices { + return Err(make_zk_error( + &request, + format!( + "es_poly_sum length mismatch: expected 1 or {}, got {}", + num_indices, + req.es_poly_sum.len() + ), + )); + }Also applies to: 383-389
🤖 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 358 - 369, Validate witness cardinalities exactly before any modulo/index access: change the checks so that req.es_poly_sum.len() must be either 1 or exactly num_indices (reject other lengths) and req.d_share_bytes.len() must equal num_indices (not just >=). Return Err via make_zk_error with clear messages when these exact-shape conditions fail and place these checks before the code path that uses modulo/indexing (the code referencing req.es_poly_sum and req.d_share_bytes around the current modulo/index access lines).
🧹 Nitpick comments (1)
crates/events/src/enclave_event/signed_proof.rs (1)
48-49: Add explicit unit assertions for the new C5 mappings.Please add coverage for
C5PkAggregationin tests to lockcircuit_names()andslash_reason()behavior.🧪 Proposed test additions
#[test] fn proof_type_circuit_names_mapping() { @@ assert_eq!( ProofType::T6DecryptedSharesAggregation.circuit_names(), vec![ CircuitName::DecryptedSharesAggregationBn, CircuitName::DecryptedSharesAggregationMod, ] ); + assert_eq!( + ProofType::C5PkAggregation.circuit_names(), + vec![CircuitName::PkAggregation] + ); } + +#[test] +fn proof_type_slash_reason_mapping() { + assert_eq!( + ProofType::C5PkAggregation.slash_reason(), + "E3_BAD_PK_AGGREGATION_PROOF" + ); +}Also applies to: 68-69, 84-85
🤖 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 48 - 49, Add unit tests that explicitly assert the new enum variant C5PkAggregation is covered by the mapping functions: write tests calling circuit_names(C5PkAggregation) and slash_reason(C5PkAggregation) and assert they return the expected string/name and reason values (matching the other C* variants). Ensure the tests live alongside existing tests for C1–C4 (and corresponding cases at the same locations as the other C5 mappings referenced) so circuit_names() and slash_reason() behavior is locked for C5PkAggregation and will fail if regressions occur.
🤖 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/publickey_aggregator.rs`:
- Around line 326-331: The handler handle_pk_aggregation_proof_signed is missing
scoping on msg.e3_id so a PkAggregationProofSigned from a different E3 run can
trigger state transitions; add an early check comparing msg.e3_id to this
aggregator's current e3_id (e.g., return Ok(()) if they differ) before
performing any state updates or transitions, and apply the same e3_id guard to
the other logic in this function (the blocks referenced around the 332-369
range) to ensure only events for the active E3 run are processed.
In `@crates/multithread/src/multithread.rs`:
- Around line 1159-1162: After sorting req.d_share_polys with
req.d_share_polys.sort_by_key(|(id, _)| *id), add a strict-duplicate check that
iterates adjacent pairs and rejects the request (return Err or early error) if
any two consecutive party IDs are equal, ensuring strictly increasing IDs for
the Noir circuit; apply the same post-sort duplicate validation to the other C7
input sort (the other sort_by_key call for the C7 inputs) so both collections
fail fast on duplicate party_id entries.
---
Duplicate comments:
In `@crates/multithread/src/multithread.rs`:
- Around line 1167-1178: The check in the C7 verification loop currently only
rejects too-short share vectors and allows oversized vectors that get truncated
later; update the validation in the loop over req.d_share_polys (and the
analogous check at the other block around the second validation) to require
exact equality (shares.len() == num_indices) instead of just length >=
num_indices, returning the same make_zk_error when sizes differ so oversized
payloads are rejected rather than silently truncated.
- Around line 358-369: Validate witness cardinalities exactly before any
modulo/index access: change the checks so that req.es_poly_sum.len() must be
either 1 or exactly num_indices (reject other lengths) and
req.d_share_bytes.len() must equal num_indices (not just >=). Return Err via
make_zk_error with clear messages when these exact-shape conditions fail and
place these checks before the code path that uses modulo/indexing (the code
referencing req.es_poly_sum and req.d_share_bytes around the current
modulo/index access lines).
---
Nitpick comments:
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 48-49: Add unit tests that explicitly assert the new enum variant
C5PkAggregation is covered by the mapping functions: write tests calling
circuit_names(C5PkAggregation) and slash_reason(C5PkAggregation) and assert they
return the expected string/name and reason values (matching the other C*
variants). Ensure the tests live alongside existing tests for C1–C4 (and
corresponding cases at the same locations as the other C5 mappings referenced)
so circuit_names() and slash_reason() behavior is locked for C5PkAggregation and
will fail if regressions occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed8a15a3-d8e1-4967-ab4c-abd2fc90e232
📒 Files selected for processing (8)
crates/aggregator/src/publickey_aggregator.rscrates/aggregator/src/threshold_plaintext_aggregator.rscrates/events/src/enclave_event/decryptionshare_created.rscrates/events/src/enclave_event/keyshare_created.rscrates/events/src/enclave_event/plaintext_aggregated.rscrates/events/src/enclave_event/publickey_aggregated.rscrates/events/src/enclave_event/signed_proof.rscrates/multithread/src/multithread.rs
hmzakhalid
reviewed
Mar 5, 2026
hmzakhalid
left a comment
hmzakhalid
left a comment
Collaborator
There was a problem hiding this comment.
Thanks Ale!
Left a few comments
hmzakhalid
reviewed
Mar 5, 2026
ctrlc03
force-pushed
the
feat/finalisation-proofs
branch
from
83564c9 to
bf7e462
Compare
hmzakhalid
approved these changes
Mar 5, 2026
This was referenced Mar 16, 2026
This was referenced Mar 24, 2026
This was referenced May 3, 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.
fix #1369 and fix #1370
Summary by CodeRabbit
New Features
Improvements
Tests