chore: refactor ccc before ZK verification [skip-line-limit]#1490
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughShareVerificationActor now inserts a pre-ZK commitment-consistency phase: after ECDSA recovery it publishes per-party proof data for a CommitmentConsistencyChecker, receives inconsistent party IDs, merges them into the dishonest set, and only dispatches ZK verification for consistency-passing parties; violations are emitted to the accusation flow. Changes
Sequence DiagramsequenceDiagram
participant SVA as ShareVerificationActor
participant CCC as CommitmentConsistencyChecker
participant ZK as ZK Prover
participant AM as AccusationManager
rect rgba(200,200,255,0.5)
SVA->>SVA: Phase 1 — ECDSA recovery\ncache addresses + public signals
end
rect rgba(200,255,200,0.5)
SVA->>CCC: Publish CommitmentConsistencyCheckRequested\n(party_proofs, correlation_id)
CCC->>CCC: Evaluate CommitmentLinks\ncheck same-/cross-party consistency
alt mismatch detected
CCC->>AM: Emit CommitmentConsistencyViolation\n(accused_party, address, proof_type, data_hash)
end
CCC->>SVA: CommitmentConsistencyCheckComplete\n{ inconsistent_parties }
end
rect rgba(255,200,200,0.5)
SVA->>SVA: Mark inconsistent parties dishonest\nfilter proofs for ZK
SVA->>ZK: Dispatch ComputeRequest (ZK verify)\nfor remaining parties
ZK->>SVA: ComputeResponse (verification result)
SVA->>AM: Publish ShareVerificationComplete / failures
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
🧹 Nitpick comments (1)
crates/tests/tests/integration.rs (1)
917-917: Replace hardcoded proof-pass count with the computed variable.Line 917 still hardcodes
+ 9even thoughc6_proof_countis already derived above. Using the variable avoids silent drift ifthreshold_nornum_votes_per_voterchanges.Suggested patch
- let expected_count = 1 + 3 + 1 + 2 + 2 + 9 + 1 + 2 + 1 + 2 + 1 + c6_fold_events + 1; + let expected_count = + 1 + 3 + 1 + 2 + 2 + c6_proof_count + 1 + 2 + 1 + 2 + 1 + c6_fold_events + 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` at line 917, The expected_count calculation currently hardcodes "+ 9" instead of using the computed c6_proof_count, so replace the literal with the variable: update the expression that assigns expected_count to use c6_proof_count (which was derived from threshold_n and num_votes_per_voter) in place of the hardcoded 9; ensure the final sum uses the c6_proof_count identifier so future changes to threshold_n or num_votes_per_voter are reflected automatically.
🤖 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/tests/tests/integration.rs`:
- Line 917: The expected_count calculation currently hardcodes "+ 9" instead of
using the computed c6_proof_count, so replace the literal with the variable:
update the expression that assigns expected_count to use c6_proof_count (which
was derived from threshold_n and num_votes_per_voter) in place of the hardcoded
9; ensure the final sum uses the c6_proof_count identifier so future changes to
threshold_n or num_votes_per_voter are reflected automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a585fa73-0769-4d2e-9f5c-cdc6f6f836ff
📒 Files selected for processing (1)
crates/tests/tests/integration.rs
ctrlc03
reviewed
Mar 27, 2026
hmzakhalid
changed the title
ctrlc03
reviewed
Mar 27, 2026
hmzakhalid
changed the title
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/zk-prover/src/actors/commitment_consistency_checker.rs (1)
147-199:⚠️ Potential issue | 🔴 CriticalCross-party links cannot blindly blame the source side.
For
C1 -> C5,crates/zk-prover/src/actors/commitment_links/c1_to_c5.rsonly asks whether a given source commitment appears in the target proof. If the targetC5is the bad artifact, both of these helpers still accuse or exclude the source party. Cross-party mismatches need symmetric attribution or an inconclusive result before they feed slashing or pre-ZK filtering.Also applies to: 215-247
🤖 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 147 - 199, check_cross_party_link currently always blames the source party when a cross-party consistency check fails; change it to determine attribution symmetrically using the CommitmentLink helpers (e.g., for C1->C5 use link.extract_source_values and link.check_consistency results to decide whether the source or target is at fault or the result is inconclusive) rather than unconditionally calling emit_violation on the source. Update check_cross_party_link to: for each source/target pair, compute whether the target contains the expected source commitment and whether the source value is uniquely missing or the target contains an invalid value; if only targets are missing/invalid call emit_violation for the target (using tgt.party_id, tgt.address, tgt_type, tgt.data_hash), if only sources are at fault call emit_violation for the source, and if the evidence is ambiguous do not emit a violation but record/log an inconclusive finding (preserving link.name(), self.e3_id in logs). Use CommitmentLink::extract_source_values and CommitmentLink::check_consistency to drive the logic and avoid changing external APIs.
♻️ Duplicate comments (1)
crates/zk-prover/src/actors/commitment_consistency_checker.rs (1)
358-399:⚠️ Potential issue | 🟠 MajorPre-ZK mismatches still never reach the accusation flow.
This handler only returns
inconsistent_parties. Thehandle_consistency_check_completepath incrates/zk-prover/src/actors/share_verification.rsthen merges those parties into the dishonest set and filters them out before ZK dispatch, so they never emitProofVerificationPassedandemit_violation()is never reached for them. Today the new gate excludes them locally, but it does not trigger fault attribution or slashing.🤖 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 358 - 399, The consistency checker currently publishes CommitmentConsistencyCheckComplete with inconsistent_parties but the handler simply merges them into the dishonest set and filters them out, so no accusation is emitted; update the handler handle_consistency_check_complete in share_verification.rs to explicitly process CommitmentConsistencyCheckComplete.inconsistent_parties by (1) invoking emit_violation (or the existing accusation/slashing routine) for each inconsistent party id, and (2) publishing any ProofVerificationFailed/accusation events for those parties before or instead of filtering them out, while keeping the normal ProofVerificationPassed flow for non-inconsistent parties; ensure you reference CommitmentConsistencyCheckComplete, handle_consistency_check_complete, emit_violation, and ProofVerificationPassed to locate and update the logic.
🤖 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/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 314-325: The code overwrites earlier proofs because verified is
keyed by (address, proof_type) and stores a single VerifiedProofData; change
verified to map to a Vec<VerifiedProofData> (or otherwise accumulate multiple
entries) so every proof instance from party.proofs is retained; update the
insertion site where self.verified.insert((address, proof_type),
VerifiedProofData { ... }) to push into the vector for that key, and modify
check_links (and any code paths referenced at the other occurrence around the
340-355 region) to iterate over all VerifiedProofData entries for a given
(address, proof_type) when performing consistency checks; mirror the design used
by PendingConsistencyCheck.party_public_signals in share_verification.rs to
ensure multiple C3a/C3b/C4b proofs are preserved and validated.
---
Outside diff comments:
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 147-199: check_cross_party_link currently always blames the source
party when a cross-party consistency check fails; change it to determine
attribution symmetrically using the CommitmentLink helpers (e.g., for C1->C5 use
link.extract_source_values and link.check_consistency results to decide whether
the source or target is at fault or the result is inconclusive) rather than
unconditionally calling emit_violation on the source. Update
check_cross_party_link to: for each source/target pair, compute whether the
target contains the expected source commitment and whether the source value is
uniquely missing or the target contains an invalid value; if only targets are
missing/invalid call emit_violation for the target (using tgt.party_id,
tgt.address, tgt_type, tgt.data_hash), if only sources are at fault call
emit_violation for the source, and if the evidence is ambiguous do not emit a
violation but record/log an inconclusive finding (preserving link.name(),
self.e3_id in logs). Use CommitmentLink::extract_source_values and
CommitmentLink::check_consistency to drive the logic and avoid changing external
APIs.
---
Duplicate comments:
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 358-399: The consistency checker currently publishes
CommitmentConsistencyCheckComplete with inconsistent_parties but the handler
simply merges them into the dishonest set and filters them out, so no accusation
is emitted; update the handler handle_consistency_check_complete in
share_verification.rs to explicitly process
CommitmentConsistencyCheckComplete.inconsistent_parties by (1) invoking
emit_violation (or the existing accusation/slashing routine) for each
inconsistent party id, and (2) publishing any ProofVerificationFailed/accusation
events for those parties before or instead of filtering them out, while keeping
the normal ProofVerificationPassed flow for non-inconsistent parties; ensure you
reference CommitmentConsistencyCheckComplete, handle_consistency_check_complete,
emit_violation, and ProofVerificationPassed to locate and update the logic.
🪄 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: c20ac4a4-7497-4654-8a8f-1ef2e28e1447
📒 Files selected for processing (7)
agent/flow-trace/00_INDEX.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdagent/flow-trace/05_FAILURE_REFUND_SLASHING.mdcrates/events/src/enclave_event/commitment_consistency.rscrates/events/src/enclave_event/mod.rscrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/actors/commitment_consistency_checker.rs
✅ Files skipped from review due to trivial changes (2)
- agent/flow-trace/00_INDEX.md
- agent/flow-trace/05_FAILURE_REFUND_SLASHING.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/events/src/enclave_event/mod.rs
- crates/events/src/enclave_event/commitment_consistency.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 279-291: The loop that collects pre-ZK mismatches (for link in
&self.links { for m in self.find_mismatches(...) { ...
inconsistent_parties.insert(m.party_id); } }) currently only logs and excludes
parties via inconsistent_parties but does not emit any
CommitmentConsistencyViolation (or a dedicated pre-ZK failure event) for the
accusation pipeline; modify this block to also publish a violation event for
each pre-ZK mismatch (using the same mechanism the post-ZK path uses to create
CommitmentConsistencyViolation) or create and publish a new
PreZkCommitmentFailure event referencing self.e3_id, link.name(), m.party_id and
a data_hash (use m.data_hash if available or a sentinel [0u8;32] and document
it), so the accusation manager receives these pre-ZK detections while still
keeping the inconsistent_parties insertion and existing warning log.
🪄 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: 026046a7-5f71-4a05-b24f-583cf6dbb28b
📒 Files selected for processing (1)
crates/zk-prover/src/actors/commitment_consistency_checker.rs
ctrlc03
approved these changes
Mar 28, 2026
This was referenced Mar 29, 2026
Merged
Merged
This was referenced Apr 7, 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
2 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.
Refactors the share verification pipeline from a 2-phase (ECDSA → ZK) to a 3-phase (ECDSA → commitment consistency check → ZK) flow.
Summary by CodeRabbit
Documentation
New Features
Tests