feat: connect c1 and c5 [skip-line-length]

[ctrlc03]

fix #1452

Summary by CodeRabbit

• Bug Fixes

  • Strengthened verification of commitment consistency across parties during aggregation
  • Improved detection and handling of faulty parties with automatic re-aggregation of valid participants
  • Enhanced early validation to reject invalid requests and prevent downstream processing errors

• Chores

  • Adjusted test timeout configurations

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
crisp	Ready	Preview, Comment	Mar 26, 2026 1:15pm
enclave-docs	Ready	Preview, Comment	Mar 26, 2026 1:15pm

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR implements cross-circuit verification between C1 (key generation) and C5 (pk aggregation) proofs. It extracts pk_commitment outputs from C1 proofs of honest parties after C1 verification, passes them through to C5 validation, and verifies that C5's expected threshold pk commitments match the actual C1 commitments. On mismatch, the aggregator re-filters parties and re-aggregates; mismatched parties trigger SignedProofFailed events downstream.

Changes

Cohort / File(s)	Summary
Event definitions crates/events/src/enclave_event/compute_request/zk.rs	Added c1_signed_proofs: Vec<SignedProofPayload> field to PkAggregationProofRequest and new ZkError::C1CommitmentMismatch { mismatched_indices } variant for cross-circuit verification failures.
Proof output extraction infrastructure crates/zk-helpers/src/circuits/output_layout.rs, crates/zk-helpers/src/circuits/mod.rs, crates/events/src/enclave_event/proof.rs	Introduced new output_layout module providing CircuitOutputLayout and methods to extract named fields from proof public signals; added Proof::extract_output() and CircuitName::output_layout() to support extraction of pk_commitment from C1 proofs.
Circuit computation refactoring crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs	Updated Bits::compute() to use preset parameter instead of ignoring it, deriving pk_bit from modulus parameters rather than bounds.
Aggregator C1→C5 verification crates/aggregator/src/publickey_aggregator.rs	Extended C1 verification handling to extract and order C1 signed proofs alongside keyshares; added handle_c5_error() to detect C1 commitment mismatches, filter dishonest parties, and re-aggregate; updated error handling to route mismatches through this new logic.
Multithread commitment cross-check crates/multithread/src/multithread.rs	Added early validation in handle_pk_aggregation_proof() to extract pk_commitment from each C1 signed proof and verify it matches the expected threshold pk commitment; returns C1CommitmentMismatch error with mismatched indices on failure.
Proof request event handling crates/zk-prover/src/actors/proof_request.rs	Updated PendingPkAggregationProof to store incoming request; on C1 commitment mismatch errors, emit SignedProofFailed events for each mismatched party recovered from stored C1 proofs.
Test configuration templates/default/tests/integration.spec.ts	Reduced integration test duration constant from 600 to 500 milliseconds, affecting phase event timeouts.

Sequence Diagram(s)

sequenceDiagram
    participant Aggregator as PublicKeyAggregator
    participant Multithread as Multithread<br/>(ZkProver)
    participant ProofActor as ProofRequest<br/>Actor
    participant Events as Event Bus

    Note over Aggregator,Events: After C1 verification completes
    Aggregator->>Aggregator: Extract C1 signed proofs<br/>from honest parties
    Aggregator->>Aggregator: Re-order proofs to match<br/>keyshare_bytes ordering
    Aggregator->>Events: Publish PkAggregationProofRequest<br/>with c1_signed_proofs
    
    Note over Multithread: C5 proof generation phase
    Multithread->>Multithread: Extract pk_commitment from<br/>each c1_signed_proofs[i]
    alt Commitment mismatch detected
        Multithread->>Multithread: Identify mismatched indices
        Multithread->>Events: Publish ComputeRequestError<br/>(C1CommitmentMismatch)
        Events->>Aggregator: handle_c5_error()
        Aggregator->>Aggregator: Filter out mismatched parties<br/>from keyshare_bytes & c1_signed_proofs
        Aggregator->>Aggregator: Recompute aggregate pk<br/>from remaining parties
        Aggregator->>Events: Publish PkAggregationProofRequest<br/>(filtered request)
    else All commitments match
        Multithread->>Multithread: Proceed to C5 proof generation
    end
    
    Note over ProofActor: Mismatch handling downstream
    ProofActor->>ProofActor: Recover faulting node address<br/>from c1_signed_proofs[idx]
    ProofActor->>Events: Emit SignedProofFailed<br/>for each mismatched party

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• chore: refactor proof gen to zk-prover [skip-line-limit] #1372: Modifies ZK proof handling and compute-request verification flow in overlapping modules (crates/events/src/enclave_event/compute_request/zk.rs and multithread paths).
• feat: finalisation proofs [skip-line-limit] #1377: Directly related—both modify PK-aggregation/C5 flow by adding c1_signed_proofs to requests, introducing C1CommitmentMismatch error handling, and updating PublicKeyAggregator C1/C5 logic.
• fix: simplify CI and integration test timeout #1416: Both PRs modify the same integration test file's duration constant (templates/default/tests/integration.spec.ts).

Suggested reviewers

• cedoor
• 0xjei
• hmzakhalid

Poem

🐰 From C1 proofs we hop and gather,
Extracting commitments that truly matter,
C5 now verifies with mighty care,
Only honest keys shall pass the dare! ✨
Re-aggregate when fraud is found,
Cross-circuit trust, safe and sound. 🔐

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Most changes align with issue #1452, but the implementation includes fault attribution via handle_c5_error and C1CommitmentMismatch error handling that goes beyond the issue's stated requirement to 'reuse existing aggregator failure/validation paths.'	Remove handle_c5_error and fault attribution logic for C1CommitmentMismatch; rely on existing ComputeRequestError handling to treat mismatches as verification failures without special fault attribution.
Linked Issues check	❓ Inconclusive	The PR implements core requirements: extracts C1 pk_commitments from honest parties, passes them to C5, and validates C5 inputs match C1 commitments. However, it stores full SignedProofPayload instead of just commitments, and implements fault attribution (handle_c5_error) despite feedback recommending removal.	Clarify whether to keep or remove fault attribution for C1CommitmentMismatch errors; consider simplifying to store only pk_commitment bytes instead of full SignedProofPayload as suggested in review feedback.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'feat: connect c1 and c5' directly summarizes the main objective of the PR, which is to establish verification between C1 and C5 circuits to ensure C5 uses C1 pk_commitments of honest parties.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/connect-c1-and-c5

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 (1)

crates/aggregator/src/publickey_aggregator.rs (1)

441-470: ⚠️ Potential issue | 🔴 Critical

Validate the received C5 proof before caching it.

This handler still stores msg.signed_proof.payload.proof verbatim. The new check in crates/multithread/src/multithread.rs only validates the locally built request; it does not protect this externally received PkAggregationProofSigned path. A buggy or malicious prover can still send a C5 proof whose public inputs bind different expected_threshold_pk_commitments, and we will publish PublicKeyAggregated anyway. Parse the signed proof’s public_signals here and reject it before setting c5_proof_pending when those commitments differ from the cached C1 commitments. You’ll need input extraction for this; extract_output only covers the trailing output region.

🤖 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 441 - 470, The
handler currently caches msg.signed_proof.payload.proof into c5_proof_pending
without validating its public_signals; update the branch that matches
PublicKeyAggregatorState::GeneratingC5Proof to parse the signed proof’s
public_signals (extract the proof's public inputs from
msg.signed_proof.payload.proof using the same extraction logic used elsewhere or
a new input extractor since extract_output only handles trailing output), derive
the expected_threshold_pk_commitments from those public_signals, and compare
them to the cached C1 commitments stored in the state (e.g. data available via
c1_signed_proofs or the field that contains the cached commitments); if they
differ, reject/ignore the incoming proof (do not set c5_proof_pending and
optionally log the mismatch), otherwise set c5_proof_pending: Some(c5_proof) and
proceed as before (updating last_ec).

🤖 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 872-915: Persist the original threshold_m in the GeneratingC5Proof
state and re-enforce the minimum-honest-party check before republishing
PkAggregationProofPending: ensure remaining_count >= threshold_m + 1 (return Err
if not) and set committee_threshold: threshold_m in the constructed
PkAggregationProofRequest (instead of 0); update uses of GeneratingC5Proof to
carry threshold_m through to this code path so you can both validate
remaining_count and populate the request.
- Around line 29-58: The current derive_c1_commitments function uses filter_map
which drops entries when proof.extract_output("pk_commitment") fails, shifting
indices and breaking the one-to-one mapping with keyshare_bytes; change it to
preserve positions by using map instead and return a Vec<Option<ArcBytes>> (or
Vec<Result<ArcBytes, YourError>>) so failures are represented per-index rather
than removed: iterate over signed_proofs with .map(|opt|
opt.as_ref().and_then(|sp|
sp.payload.proof.extract_output("pk_commitment").or_else(|| {
tracing::warn!(...); None }))) or return Result on extraction errors, update
callers (e.g., where keyshare_bytes and C1CommitmentMismatch are checked) to
handle the Option/Result and convert extraction failures into explicit
proof-failure/C1CommitmentMismatch handling rather than relying on shifted
indices.

In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 64-65: The new field c1_commitments on PkAggregationProofRequest
must get a serde default to preserve backward compatibility; update the struct
by adding #[serde(default)] to the c1_commitments declaration (the Vec<ArcBytes>
field) so older serialized payloads deserialize to an empty vector while keeping
the existing validation in crates/multithread/src/multithread.rs that checks C1
commitments intact.

In `@crates/multithread/src/multithread.rs`:
- Around line 348-361: The code currently only validates c1_commitments length
but not the per-party keyshare count; before calling PkAggInputs::compute, add a
guard that checks req.keyshare_bytes.len() == req.committee_h and return
Err(make_zk_error(&request, format!("keyshare_bytes length {} != committee_h
{}", req.keyshare_bytes.len(), req.committee_h))) if it mismatches, so
PkAggInputs::compute (which iterates 0..committee_h) cannot panic or silently
truncate.

---

Outside diff comments:
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 441-470: The handler currently caches
msg.signed_proof.payload.proof into c5_proof_pending without validating its
public_signals; update the branch that matches
PublicKeyAggregatorState::GeneratingC5Proof to parse the signed proof’s
public_signals (extract the proof's public inputs from
msg.signed_proof.payload.proof using the same extraction logic used elsewhere or
a new input extractor since extract_output only handles trailing output), derive
the expected_threshold_pk_commitments from those public_signals, and compare
them to the cached C1 commitments stored in the state (e.g. data available via
c1_signed_proofs or the field that contains the cached commitments); if they
differ, reject/ignore the incoming proof (do not set c5_proof_pending and
optionally log the mismatch), otherwise set c5_proof_pending: Some(c5_proof) and
proceed as before (updating last_ec).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f3bb0269-c91f-4c89-a03d-21cc5d5125b5

📥 Commits

Reviewing files that changed from the base of the PR and between 12e2752 and aa655ee.

📒 Files selected for processing (7)

• crates/aggregator/src/publickey_aggregator.rs
• crates/events/src/enclave_event/compute_request/zk.rs
• crates/events/src/enclave_event/proof.rs
• crates/multithread/src/multithread.rs
• crates/zk-helpers/src/circuits/mod.rs
• crates/zk-helpers/src/circuits/output_layout.rs
• crates/zk-helpers/src/circuits/threshold/pk_generation/computation.rs

[hmzakhalid]

Thanks Ale!
Could we add an integration test as well?
multithread detecting a mismatch -> error event -> aggregator receiving it -> handling

[cedoor]

LGTM