feat: connect c4 and c6 [skip-line-limit]#1473
Closed
ctrlc03 wants to merge 14 commits into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe changes extend ZK proof verification infrastructure by introducing public input field extraction from circuits, splitting the C4 DKG proof type into C4a and C4b variants, adding new C4-to-C6 commitment consistency links, enhancing verification with signal caching and pre-dispatch gates, and threading signed proof payloads through the verification pipeline. Changes
Sequence DiagramsequenceDiagram
participant Client as Proof Originator
participant SV as ShareVerification<br/>Actor
participant Cache as C4 Signal<br/>Cache
participant CL as C4→C6<br/>Commitment Link
participant PV as ProofVerification<br/>Actor
participant CC as Commitment<br/>Consistency Checker
participant EventBus as Event Bus
Note over SV,PV: ThresholdDecryptionProofs Flow
Client->>SV: ShareVerificationDispatched<br/>(with C4 & C6 proofs)
SV->>SV: Identify VerificationKind::<br/>ThresholdDecryptionProofs
SV->>PV: Request ZK verification<br/>for C4 proofs
PV->>PV: Verify C4 proofs
PV->>SV: ZkVerificationResponse<br/>(C4 success)
SV->>Cache: Store C4 public_signals<br/>per party
SV->>CL: Pre-dispatch gate:<br/>check C4→C6 consistency<br/>for each party
alt Party Fails C4→C6 Check
SV->>EventBus: SignedProofFailed<br/>(party added to pre_dishonest)
else Party Passes
SV->>PV: Request C6 ZK verification
PV->>PV: Verify C6 proofs
PV->>SV: ZkVerificationResponse<br/>(C6 success)
SV->>EventBus: ProofVerificationPassed<br/>(with signed_payload)
end
SV->>Cache: Evict C4 signals<br/>after completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/keyshare/src/threshold_keyshare.rs (1)
2135-2144:⚠️ Potential issue | 🟠 MajorThis still leaves the C5→C6 binding disabled.
ThresholdKeyshareis the main producer ofThresholdShareDecryptionProofRequest, but it always sendsc5_pk_commitment: None. So the new field never carries the C5-certified commitment on the normal path, and C6 cannot actually be bound to what C5 attested. Please persist the C5 commitment when the aggregated key/proof arrives and thread it through here.🤖 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 2135 - 2144, The code always sets c5_pk_commitment: None when building a ThresholdShareDecryptionProofRequest in ThresholdKeyshare, so the C5→C6 binding is never carried; persist the C5 commitment when the aggregated key/proof is received (the place that processes the aggregated_pk_bytes / incoming aggregated proof) and store it on the ThresholdKeyshare instance or state, then replace c5_pk_commitment: None with that stored commitment when constructing ThresholdShareDecryptionProofRequest (alongside aggregated_pk_bytes and proof_aggregation_enabled) so the C5-certified commitment is threaded through to C6.crates/aggregator/src/publickey_aggregator.rs (1)
394-407:⚠️ Potential issue | 🔴 CriticalCritical:
keyshare_bytesandc1_signed_proofsare misaligned in state.At line 397,
keyshare_bytes: honest_keysharesstores the original insertion-order vector, butc1_signed_proofs(line 398) was re-aligned at lines 357-369 to match theOrderedSet-orderedkeyshare_bytes(line 356).This creates an index mismatch: when
handle_c1_commitment_mismatchreceives a mismatched index, it will look up the wrong keyshare or proof.The
PkAggregationProofPendingevent published at line 380 useskeyshare_bytes.clone()(OrderedSet order), so the prover operates on different indices than what's stored in state.🐛 Proposed fix
self.state.try_mutate(&ec, |_| { Ok(PublicKeyAggregatorState::GeneratingC5Proof { public_key: pubkey.clone(), - keyshare_bytes: honest_keyshares, + keyshare_bytes: keyshare_bytes.clone(), c1_signed_proofs, nodes: honest_nodes_set,🤖 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 394 - 407, The state stores keyshare_bytes and c1_signed_proofs out of sync: when you re-order keyshares to match OrderedSet earlier (the vector used for the PkAggregationProofPending event and c1_signed_proofs), you must save that same ordered vector into PublicKeyAggregatorState::GeneratingC5Proof so indices align; update the construction so keyshare_bytes is the re-ordered/aligned vector (the same variable used to build c1_signed_proofs and the PkAggregationProofPending payload) and ensure c1_signed_proofs is created from that same aligned vector (so handle_c1_commitment_mismatch looks up matching entries).
🧹 Nitpick comments (3)
crates/aggregator/src/publickey_aggregator.rs (3)
872-879: Consider adding threshold check before re-aggregation.The code only verifies
remaining_count > 0before re-aggregating. Ifremaining_count <= threshold_m, the subsequent C5 proof will fail. While the comment notes the circuit enforces this, adding an early check would avoid wasted computation and provide a clearer error message.Note:
threshold_mis not currently available inGeneratingC5Proofstate, so this would require carrying it forward fromVerifyingC1.🤖 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 872 - 879, The code only checks remaining_count == 0 before re-aggregation; add an explicit threshold check so re-aggregation is skipped if too few honest shares remain: propagate threshold_m from VerifyingC1 into the GeneratingC5Proof state (carry it through the state transition that creates GeneratingC5Proof), then before using remaining_keyshares compare remaining_count against threshold_m (require remaining_count > threshold_m) and return an Err with a clear message like "insufficient honest parties after C1 filtering: remaining_count X <= threshold_m Y" to avoid wasted C5 computation and give a precise failure reason.
28-59: Verbose debug logging should usedebug!ortrace!level.The
tracing::info!calls on lines 36-45 and 48-52 emit potentially large hex dumps of public signals for every C1 proof processed. This is useful during development but may flood production logs.Consider using
debug!ortrace!level, or gating behind a feature flag.🔧 Suggested change
- tracing::info!( + tracing::debug!( "C1 proof[{}]: circuit={:?}, public_signals_len={}, signals_hex={}", i, proof.circuit, proof.public_signals.len(), proof.public_signals[..std::cmp::min(128, proof.public_signals.len())] .iter() .map(|b| format!("{:02x}", b)) .collect::<String>() ); let commitment = proof.extract_output("pk_commitment"); if let Some(ref c) = commitment { - tracing::info!( + tracing::debug!( "C1 proof[{}]: extracted pk_commitment={}", i, c.iter().map(|b| format!("{:02x}", b)).collect::<String>() );🤖 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 28 - 59, The current verbose tracing inside derive_c1_commitments uses tracing::info! for large hex dumps and should be demoted to tracing::debug! or tracing::trace! (or wrapped behind a feature flag) to avoid flooding production logs; update the two tracing::info! calls and the tracing::warn!/info! that print extracted pk_commitment to use debug!/trace! (or conditionally log when a debug feature or env var is enabled) so the same diagnostic data is preserved for development but not emitted at info level in production, locating the changes in the derive_c1_commitments function around proof.public_signals logging and the pk_commitment extraction log.
357-369: Keyshare-to-proof mapping assumes unique keyshares.The
HashMapkeyed byks.to_vec()will overwrite entries if two honest parties submitted identical keyshare bytes. While this would be a protocol violation (andOrderedSetalready deduplicates keyshares), consider adding a debug assertion or log if duplicates are detected during the map build.This is a defensive measure; the current logic is correct under normal operation.
🤖 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 357 - 369, The code builds ks_to_proof from honest_keyshares and c1_signed_proofs using ks.to_vec() as key which will silently overwrite if duplicate keyshare bytes exist; add a defensive check while constructing ks_to_proof (e.g., iterate over honest_keyshares.zip(c1_signed_proofs) and on each insert check the previous value) and emit a debug_assert or a log/error if an insert would replace an existing entry to catch duplicate keyshares; reference the honest_keyshares, c1_signed_proofs, keyshare_bytes, and ks_to_proof identifiers when implementing this check so the later map lookup logic remains unchanged.
🤖 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 859-867: The current use of OrderedSet (backed by BTreeSet)
reorders elements so filtering `nodes` and `keyshare_bytes` by the same
`mismatched_indices` misaligns entries; fix by preserving (node, keyshare)
pairings and filtering those pairs before creating any OrderedSet. Concretely,
change the logic around `nodes`, `keyshare_bytes`, `mismatched_indices` and
`honest_entries` to iterate `honest_entries` as (node, keyshare) tuples, filter
out entries whose index is in `mismatched_indices`, then collect the remaining
nodes and keyshares from the filtered pairs (or construct OrderedSet from the
paired sequence) so indices remain aligned; update code that references `nodes`
or `keyshare_bytes` to use the filtered paired results instead of independently
sorted OrderedSets.
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 64-65: The new required field c1_commitments in
PkAggregationProofRequest (which is nested in PkAggregationProofPending) changes
the on‑wire shape and will break mixed-version deserialization; make this a
versioned addition instead: add a version tag/enum for PkAggregationProofRequest
or wrap PkAggregationProofPending in a versioned envelope, keep c1_commitments
optional or behind a new version variant, and update
serialization/deserialization logic to accept old versions (treat missing
c1_commitments as empty/None) while emitting the new version when serializing;
ensure references to PkAggregationProofRequest, PkAggregationProofPending, and
c1_commitments are updated so older nodes can still decode requests.
In `@crates/multithread/src/multithread.rs`:
- Around line 348-398: Check and enforce that all three arrays have the expected
length before zipping: validate req.c1_commitments.len(),
req.keyshare_bytes.len() (or the actual keyshare field name), and expected.len()
(from PkAggInputs::compute) are equal to req.committee_h (or all equal to each
other) and return a Zk error if not; then replace the zip over
expected.iter().zip(req.c1_commitments.iter()) in the C1 commitment loop with an
index-based loop (e.g., for i in 0..expected.len()) that indexes into
expected[i], req.c1_commitments[i], and req.keyshare_bytes[i] so no element is
silently skipped and mismatched_indices (and the ComputeRequestError::new path)
correctly capture all discrepancies.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 166-198: The cross-check loop marks some parties as failed
(pre_dishonest via check_c6_party_against_c4 and emit_signed_proof_failed) but
verify_proofs is still called with the original msg.share_proofs, allowing
already-failed parties to be dispatched to ZK; before calling verify_proofs for
VerificationKind::ThresholdDecryptionProofs, filter msg.share_proofs to exclude
any party whose sender_party_id is present in pre_dishonest (e.g. build a new
Vec filtered_share_proofs from msg.share_proofs where party.sender_party_id ∉
pre_dishonest) and pass that filtered_share_proofs (and the merged
pre_dishonest) into verify_proofs so only non-failed parties are validated and
sent to ZK (refer to check_c6_party_against_c4, emit_signed_proof_failed,
pre_dishonest, verify_proofs, and VerifyShareProofs).
---
Outside diff comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 394-407: The state stores keyshare_bytes and c1_signed_proofs out
of sync: when you re-order keyshares to match OrderedSet earlier (the vector
used for the PkAggregationProofPending event and c1_signed_proofs), you must
save that same ordered vector into PublicKeyAggregatorState::GeneratingC5Proof
so indices align; update the construction so keyshare_bytes is the
re-ordered/aligned vector (the same variable used to build c1_signed_proofs and
the PkAggregationProofPending payload) and ensure c1_signed_proofs is created
from that same aligned vector (so handle_c1_commitment_mismatch looks up
matching entries).
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 2135-2144: The code always sets c5_pk_commitment: None when
building a ThresholdShareDecryptionProofRequest in ThresholdKeyshare, so the
C5→C6 binding is never carried; persist the C5 commitment when the aggregated
key/proof is received (the place that processes the aggregated_pk_bytes /
incoming aggregated proof) and store it on the ThresholdKeyshare instance or
state, then replace c5_pk_commitment: None with that stored commitment when
constructing ThresholdShareDecryptionProofRequest (alongside aggregated_pk_bytes
and proof_aggregation_enabled) so the C5-certified commitment is threaded
through to C6.
---
Nitpick comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 872-879: The code only checks remaining_count == 0 before
re-aggregation; add an explicit threshold check so re-aggregation is skipped if
too few honest shares remain: propagate threshold_m from VerifyingC1 into the
GeneratingC5Proof state (carry it through the state transition that creates
GeneratingC5Proof), then before using remaining_keyshares compare
remaining_count against threshold_m (require remaining_count > threshold_m) and
return an Err with a clear message like "insufficient honest parties after C1
filtering: remaining_count X <= threshold_m Y" to avoid wasted C5 computation
and give a precise failure reason.
- Around line 28-59: The current verbose tracing inside derive_c1_commitments
uses tracing::info! for large hex dumps and should be demoted to tracing::debug!
or tracing::trace! (or wrapped behind a feature flag) to avoid flooding
production logs; update the two tracing::info! calls and the
tracing::warn!/info! that print extracted pk_commitment to use debug!/trace! (or
conditionally log when a debug feature or env var is enabled) so the same
diagnostic data is preserved for development but not emitted at info level in
production, locating the changes in the derive_c1_commitments function around
proof.public_signals logging and the pk_commitment extraction log.
- Around line 357-369: The code builds ks_to_proof from honest_keyshares and
c1_signed_proofs using ks.to_vec() as key which will silently overwrite if
duplicate keyshare bytes exist; add a defensive check while constructing
ks_to_proof (e.g., iterate over honest_keyshares.zip(c1_signed_proofs) and on
each insert check the previous value) and emit a debug_assert or a log/error if
an insert would replace an existing entry to catch duplicate keyshares;
reference the honest_keyshares, c1_signed_proofs, keyshare_bytes, and
ks_to_proof identifiers when implementing this check so the later map lookup
logic remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c4c1c5d-3afc-411d-bd29-fb23b92f99ae
📒 Files selected for processing (9)
crates/aggregator/src/publickey_aggregator.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/proof.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/zk-helpers/src/circuits/mod.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-helpers/src/circuits/threshold/pk_generation/computation.rscrates/zk-prover/src/actors/share_verification.rs
ctrlc03
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
crates/multithread/src/multithread.rs (1)
348-361:⚠️ Potential issue | 🔴 CriticalValidate
keyshare_bytesbeforePkAggInputs::compute.Only
c1_commitmentsandexpectedare length-checked here.PkAggInputs::computewalks0..committee_hand indexespk0_shares, so a short or extrareq.keyshare_byteslist still turns this path into a panic instead of a ZKInvalidParamserror.♻️ Proposed fix
if req.c1_commitments.len() != req.committee_h { return Err(make_zk_error( &request, format!( "c1_commitments length {} != committee_h {}", req.c1_commitments.len(), req.committee_h ), )); } + if req.keyshare_bytes.len() != req.committee_h { + return Err(make_zk_error( + &request, + format!( + "keyshare_bytes length {} != committee_h {}", + req.keyshare_bytes.len(), + req.committee_h + ), + )); + } let pk_agg_inputs = PkAggInputs::compute(req.params_preset.clone(), &circuit_data) .map_err(|e| make_zk_error(&request, format!("PkAggInputs::compute: {}", e)))?;🤖 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 348 - 361, Validate req.keyshare_bytes length against req.committee_h before calling PkAggInputs::compute: check that req.keyshare_bytes.len() == req.committee_h and if not return Err(make_zk_error(&request, format!("InvalidParams: keyshare_bytes length {} != committee_h {}", req.keyshare_bytes.len(), req.committee_h))). This prevents PkAggInputs::compute from indexing pk0_shares (and similar) and panicking; ensure the validation occurs prior to invoking PkAggInputs::compute.crates/aggregator/src/publickey_aggregator.rs (1)
353-380:⚠️ Potential issue | 🔴 CriticalPersist C5 retry state in the same order you send to multithread.
PkAggregationProofPending.keyshare_bytesis built inOrderedSetorder, butGeneratingC5Proof.keyshare_bytesstoreshonest_keyshares(submission order) andhandle_c1_commitment_mismatchresolves returned indices againsthonest_party_ids(party-id order). If those orders differ, amismatched_indicesresponse removes/blames the wrong party and re-aggregates the wrong share set.Also applies to: 405-410, 813-823
🤖 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 353 - 380, The retry state is being persisted using submission/party-id order (honest_keyshares / honest_nodes) while the multithreaded C5 work and PkAggregationProofPending.keyshare_bytes use OrderedSet ordering, causing mismatched blame/aggregation when orders differ; fix by normalizing everything to the OrderedSet order: build and persist GeneratingC5Proof.keyshare_bytes and any retry state from the OrderedSet-derived keyshare_bytes (use the same keyshare_bytes, ks_to_proof and ks_to_node alignment logic shown) and modify handle_c1_commitment_mismatch to interpret incoming indices against the OrderedSet order (honest_keyshares_set / honest_nodes_set) so that c1_signed_proofs, nodes_aligned, and persisted keyshare_bytes all share the same ordering.
🤖 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 891-898: After filtering out C1-mismatched parties
(remaining_keyshares), re-check the threshold using threshold_m: current code
only checks remaining_count == 0, which allows continuing with <= threshold_m
parties; change the check to ensure remaining_count > threshold_m (i.e., return
an error if remaining_count <= threshold_m) so we abort the retry path when
there are not enough honest parties to meet the decryption threshold; update the
error message in the branch that inspects remaining_keyshares to reference this
threshold and include context (e.g., mention
honest_keyshares/remaining_keyshares and threshold_m) so the failure is clear.
- Around line 28-59: derive_c1_commitments currently uses filter_map and drops
None results from proof.extract_output("pk_commitment"), shortening
c1_commitments and breaking the 1:1 contract with keyshare_bytes; change
derive_c1_commitments to preserve indices by returning Vec<Option<ArcBytes>>
(update its signature) and replace the filter_map with a plain map that returns
the extracted Option (still logging success/failure as before), and then update
any callers (e.g., handle_pk_aggregation_proof) to accept and handle the
Vec<Option<ArcBytes>> so missing pk_commitment values are detected and handled
rather than silently removed.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 112-115: The c4_cache (HashMap<E3id, HashMap<u64, C4Commitments>>)
is never evicted, leaking per-E3 data for the lifetime of the process; add an
explicit eviction path that removes the c4_cache entry when an E3 completes or
fails. Specifically, in the actor handlers that finalize an E3 (e.g., the
ThresholdDecryptionProofs completion/error handler—look for
handle_threshold_decryption_proofs or similar) add code to call
c4_cache.remove(&e3id) (or clear the inner map) and also implement/handle an
E3RequestComplete message/hook (e.g., handle_e3_request_complete) to remove the
same entry; ensure the removal uses the E3id used to populate the cache and is
done in the same actor/context to avoid concurrency issues.
---
Duplicate comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 353-380: The retry state is being persisted using
submission/party-id order (honest_keyshares / honest_nodes) while the
multithreaded C5 work and PkAggregationProofPending.keyshare_bytes use
OrderedSet ordering, causing mismatched blame/aggregation when orders differ;
fix by normalizing everything to the OrderedSet order: build and persist
GeneratingC5Proof.keyshare_bytes and any retry state from the OrderedSet-derived
keyshare_bytes (use the same keyshare_bytes, ks_to_proof and ks_to_node
alignment logic shown) and modify handle_c1_commitment_mismatch to interpret
incoming indices against the OrderedSet order (honest_keyshares_set /
honest_nodes_set) so that c1_signed_proofs, nodes_aligned, and persisted
keyshare_bytes all share the same ordering.
In `@crates/multithread/src/multithread.rs`:
- Around line 348-361: Validate req.keyshare_bytes length against
req.committee_h before calling PkAggInputs::compute: check that
req.keyshare_bytes.len() == req.committee_h and if not return
Err(make_zk_error(&request, format!("InvalidParams: keyshare_bytes length {} !=
committee_h {}", req.keyshare_bytes.len(), req.committee_h))). This prevents
PkAggInputs::compute from indexing pk0_shares (and similar) and panicking;
ensure the validation occurs prior to invoking PkAggInputs::compute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3748817-14fe-464f-8b79-31e629fae1cf
📒 Files selected for processing (3)
crates/aggregator/src/publickey_aggregator.rscrates/multithread/src/multithread.rscrates/zk-prover/src/actors/share_verification.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/keyshare/src/threshold_keyshare.rs (1)
2146-2154: This still leaves the C5→C6 binding unset.
c5_pk_commitmentis hard-coded toNonehere, so the new request shape never carries a C5 commitment at its only producer. Since the current threshold-share-decryption prover path also doesn't consume that field yet, this change does not actually bind C6 to the C5-certified aggregated key. Either thread the real commitment through here or mark the field as reserved/future work so the API does not imply that check exists today.🤖 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 2146 - 2154, The request is setting ThresholdShareDecryptionProofRequest.c5_pk_commitment to None which leaves the C5→C6 binding unimplemented; update the code to pass the actual C5 commitment into the request (e.g., use the commitment available on the decryption context such as decrypting.c5_pk_commitment or the corresponding value on state) so the proof can assert the C5-certified aggregated key, or if that commitment is intentionally unavailable yet, explicitly mark the field as reserved/future work (add a clear TODO comment and rename/annotate the field usage) so the API does not misleadingly imply the check exists. Ensure you modify the construction of ThresholdShareDecryptionProofRequest to use the real commitment symbol (c5_pk_commitment) or add the reserve annotation where the request is created.crates/zk-prover/src/actors/commitment_consistency_checker.rs (1)
182-204: Potential duplicateSignedProofFailedemissions for cross-party links.When a single source is inconsistent with multiple targets,
emit_fault(src, ec)is called once per mismatching target. This could flood downstream consumers with duplicate fault events for the same party/proof.Consider tracking already-faulted sources and emitting only once per source:
♻️ Suggested deduplication
fn check_cross_party_link( &self, link: &dyn CommitmentLink, new_proof_type: ProofType, ec: &e3_events::EventContext<e3_events::Sequenced>, ) { // ... existing code ... + let mut faulted_sources: HashSet<Address> = HashSet::new(); // For each (source, target) pair, check consistency. for src in &sources { let source_values = link.extract_source_values(&src.public_signals); if source_values.is_empty() { continue; } for tgt in &targets { if !link.check_consistency(&source_values, &tgt.public_signals) { warn!(/* ... */); - self.emit_fault(src, ec); + if faulted_sources.insert(src.address) { + self.emit_fault(src, ec); + } } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs` around lines 182 - 204, The loop in commitment_consistency_checker currently calls self.emit_fault(src, ec) for every target mismatch, risking duplicate SignedProofFailed events; modify the logic in the for src in &sources loop to track which source party/proof has already had a fault emitted (e.g., a HashSet keyed by src.party_id plus a proof identifier or src reference) and only call self.emit_fault(src, ec) the first time a mismatch is detected for that source; use the existing link.extract_source_values, link.check_consistency, and self.emit_fault symbols to locate the code and ensure the dedup set is checked/updated before emitting to prevent repeated emissions for the same source across multiple target mismatches.crates/zk-prover/src/actors/commitment_links/c4a_to_c6.rs (1)
40-42: Potential panic if extracted field has unexpected length.
copy_from_slicepanics ifbytes.len() != FIELD_BYTE_LEN. While the circuit layout should guarantee correct sizing, adding a length check would be more defensive:🛡️ Proposed defensive check
fn extract_source_values(&self, public_signals: &[u8]) -> Vec<FieldValue> { let layout = CircuitName::DkgShareDecryption.output_layout(); let Some(bytes) = layout.extract_field(public_signals, "commitment") else { return vec![]; }; + if bytes.len() != FIELD_BYTE_LEN { + return vec![]; + } let mut value = [0u8; FIELD_BYTE_LEN]; value.copy_from_slice(bytes); vec![value] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/commitment_links/c4a_to_c6.rs` around lines 40 - 42, The current use of value.copy_from_slice(bytes) in the c4a_to_c6 conversion can panic if bytes.len() != FIELD_BYTE_LEN; add a defensive length check before copying (e.g., verify bytes.len() == FIELD_BYTE_LEN) and handle the mismatch gracefully by returning an Err or a meaningful error variant from the surrounding function (or mapping to a safe default), rather than letting copy_from_slice panic; update the function that constructs vec![value] (the block that allocates value and calls copy_from_slice) to perform the check and propagate or log the error with context.
🤖 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/events/src/enclave_event/signed_proof.rs`:
- Around line 42-51: The enum ProofType was renumbered, which breaks consumers
that encode proof_type as u8 (e.g., ProofPayload::digest() and the slashing
writer in crates/evm/src/slashing_manager_sol_writer.rs); restore stable numeric
values by not shifting existing variants and instead append a new explicit value
for the C4b variant (keep C6ThresholdShareDecryption,
C7DecryptedSharesAggregation, C5PkAggregation pinned to their original numeric
values) and set explicit = N for each enum variant to prevent future drift;
update the enum in signed_proof.rs (e.g., add a new C4bESmShareDecryption with a
new unused numeric ID and ensure C5/C6/C7 retain their original numbers) and add
a regression test that asserts numeric values of ProofType variants and that
ProofPayload::digest() encodes proof_type as the expected u8 to prevent
accidental renumbering.
---
Nitpick comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 2146-2154: The request is setting
ThresholdShareDecryptionProofRequest.c5_pk_commitment to None which leaves the
C5→C6 binding unimplemented; update the code to pass the actual C5 commitment
into the request (e.g., use the commitment available on the decryption context
such as decrypting.c5_pk_commitment or the corresponding value on state) so the
proof can assert the C5-certified aggregated key, or if that commitment is
intentionally unavailable yet, explicitly mark the field as reserved/future work
(add a clear TODO comment and rename/annotate the field usage) so the API does
not misleadingly imply the check exists. Ensure you modify the construction of
ThresholdShareDecryptionProofRequest to use the real commitment symbol
(c5_pk_commitment) or add the reserve annotation where the request is created.
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 182-204: The loop in commitment_consistency_checker currently
calls self.emit_fault(src, ec) for every target mismatch, risking duplicate
SignedProofFailed events; modify the logic in the for src in &sources loop to
track which source party/proof has already had a fault emitted (e.g., a HashSet
keyed by src.party_id plus a proof identifier or src reference) and only call
self.emit_fault(src, ec) the first time a mismatch is detected for that source;
use the existing link.extract_source_values, link.check_consistency, and
self.emit_fault symbols to locate the code and ensure the dedup set is
checked/updated before emitting to prevent repeated emissions for the same
source across multiple target mismatches.
In `@crates/zk-prover/src/actors/commitment_links/c4a_to_c6.rs`:
- Around line 40-42: The current use of value.copy_from_slice(bytes) in the
c4a_to_c6 conversion can panic if bytes.len() != FIELD_BYTE_LEN; add a defensive
length check before copying (e.g., verify bytes.len() == FIELD_BYTE_LEN) and
handle the mismatch gracefully by returning an Err or a meaningful error variant
from the surrounding function (or mapping to a safe default), rather than
letting copy_from_slice panic; update the function that constructs vec![value]
(the block that allocates value and calls copy_from_slice) to perform the check
and propagate or log the error with context.
🪄 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: 3ab30f1a-b78c-49c2-b752-e175f87a0a91
📒 Files selected for processing (15)
crates/aggregator/src/publickey_aggregator.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/proof.rscrates/events/src/enclave_event/proof_verification_passed.rscrates/events/src/enclave_event/signed_proof.rscrates/keyshare/src/threshold_keyshare.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-prover/src/actors/commitment_consistency_checker.rscrates/zk-prover/src/actors/commitment_links/c1_to_c5.rscrates/zk-prover/src/actors/commitment_links/c4a_to_c6.rscrates/zk-prover/src/actors/commitment_links/c4b_to_c6.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/actors/share_verification.rs
✅ Files skipped from review due to trivial changes (2)
- crates/zk-prover/src/actors/commitment_links/mod.rs
- crates/aggregator/src/publickey_aggregator.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/events/src/enclave_event/proof.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
1 participant
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.
first merge #1460
Summary by CodeRabbit
New Features
Refactor