feat: add cross-circuit commitment consistency checker#1485
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a CommitmentConsistencyChecker actor and extension, link definitions, and plumbing to propagate proof public outputs. The checker subscribes to proof verification events, caches verified public outputs by (address, proof type), evaluates registered CommitmentLink rules (same-party and cross-party), and emits warnings on mismatches. Builder wiring registers the extension. Changes
Sequence DiagramsequenceDiagram
participant ShareVerification as Share Verification
participant ProofVerification as Proof Verification
participant EventBus as Event Bus
participant ConsistencyChecker as Consistency Checker
participant CommitmentLink as Commitment Link
ShareVerification->>ShareVerification: cache public_signals per party
ShareVerification->>ProofVerification: forward signed proofs / signals
ProofVerification->>ProofVerification: verify proof, include public_outputs
ProofVerification->>EventBus: publish ProofVerificationPassed(public_outputs)
EventBus->>ConsistencyChecker: deliver ProofVerificationPassed
ConsistencyChecker->>ConsistencyChecker: store (address, proof_type) -> public_outputs + party_id
ConsistencyChecker->>CommitmentLink: for each link, extract_source_values / check_consistency
CommitmentLink-->>ConsistencyChecker: consistency result (ok|mismatch)
ConsistencyChecker->>ConsistencyChecker: warn! on mismatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 5
🧹 Nitpick comments (2)
crates/zk-prover/src/actors/share_verification.rs (1)
90-91: Keep hash and outputs in the same cached record.Line 463 rejoins
party_proof_hashesandparty_public_signalspurely by vector index. That works today because both vectors are built from the same iteration order, but any future ordering drift insigned_proofs()will pair the wrong hash with the wrongpublic_outputs, and theunwrap_or_default()fallback will silently disable consistency checks. A single per-proof cache entry holdingproof_type,data_hash, andpublic_outputsavoids that footgun.Also applies to: 242-267, 461-466
🤖 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 90 - 91, The cache currently stores party_public_signals (HashMap<u64, Vec<(ProofType, ArcBytes)>>) separately from party_proof_hashes and later rejoins them by vector index, which is fragile; change the cache to store a single per-proof record struct (e.g., ProofCacheEntry) that contains proof_type, data_hash (the proof hash), and public_outputs (ArcBytes) and replace uses of party_public_signals and party_proof_hashes with a single HashMap<u64, Vec<ProofCacheEntry>>; update construction sites (where signed_proofs() is iterated to build caches, referenced in functions around the previous indices at lines ~242-267 and ~461-466) to populate all fields atomically and update lookup/consumption sites to use the combined entry so no index-based rejoining or unwrap_or_default() fallbacks are needed.crates/zk-prover/src/actors/commitment_consistency_checker.rs (1)
121-170: Cross-party check has O(n·m) complexity — acceptable for current committee sizes.The nested iteration over all sources and targets is correct for the C1→C5 link semantics (each node's pk_commitment must appear in the aggregated C5). For large committees, consider indexing by extracted commitment values to reduce comparisons to O(n+m), but this is a nice-to-have optimization.
🤖 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 121 - 170, The nested loops in check_cross_party_link produce O(n·m) comparisons; to optimize, build an index (HashSet or HashMap) of extracted commitment values from the targets (or sources when the new proof is a target) and then probe that index for each source instead of iterating all targets. Concretely, inside check_cross_party_link, replace the inner loop by first collecting extracted values for each target via link.extract_target_values (or link.extract_source_values if the API only provides that for the relevant role), insert those values (or serialized forms) into a HashSet/HashMap keyed by the commitment, then for each src call link.extract_source_values and check membership with O(1) lookups, using link.check_consistency only when a candidate match exists; keep the existing logging and behavior when no index hit is found.
🤖 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/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 542-546: The CommitmentConsistencyCheckerExtension is wired up
(CommitmentConsistencyCheckerExtension::create and e3_builder.with) but the
checker only logs mismatches via warn! in check_same_party_link and
check_cross_party_link and therefore never publishes a ProofVerificationFailed
into the accusation pipeline; modify the checker (the functions
check_same_party_link and check_cross_party_link in
commitment_consistency_checker.rs) to publish a ProofVerificationFailed event to
the bus/accusation pipeline (the same mechanism AccusationManager consumes)
whenever a mismatch is detected, ensuring the published event includes the
offending party ID, commitment details, and context so
AccusationManager/quorum→slash flow can process it.
In `@crates/events/src/enclave_event/proof_verification_passed.rs`:
- Around line 35-36: The new required field public_outputs on the
ProofVerificationPassed event will break deserialization of older persisted
events; make it replay-safe by either adding #[serde(default)] to the
public_outputs field and ensuring ArcBytes implements Default (or provide a
function like default_arc_bytes()), or change the field type to Option<ArcBytes>
with #[serde(default, skip_serializing_if = "Option::is_none")]; update the
struct declaration for ProofVerificationPassed accordingly so older events
without public_outputs can still deserialize.
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 42-53: CommitmentConsistencyChecker currently stores a bus field
that is only used in setup() for subscription and does not emit
ProofVerificationFailed events yet; since this is an intentional phased rollout,
do not remove the bus field—leave CommitmentConsistencyChecker::bus and the
subscription logic in setup() as-is and add or keep a short TODO comment near
the bus field or setup() referencing ProofVerificationFailed to clarify that
event emission will be implemented in a future iteration.
In `@crates/zk-prover/src/actors/commitment_links/c1_to_c5.rs`:
- Around line 67-79: The function check_consistency() currently treats truncated
or missing C5 public_signals as vacuously consistent; change it so any
present-but-too-short target_public_signals is treated as non-consistent.
Concretely, in crates/zk-prover/src/actors/commitment_links/c1_to_c5.rs adjust
the early-return logic around source_values, target_public_signals and
total_fields: do not return true when target_public_signals.len() > 0 but
shorter than expected (i.e., < 2 * FIELD_SIZE or total_fields < 2); instead
return false for those cases while preserving the original true return only for
the case where target_public_signals is completely absent/empty and
source_values is empty (vacuum). Reference symbols: check_consistency(),
source_values, target_public_signals, FIELD_SIZE, total_fields,
source_pk_commitment.
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Line 247: Add an explicit equality check between msg.key.proof and
signed_payload.payload.proof before publishing the success event: compare the
two byte/proof fields (msg.key.proof vs signed_payload.payload.proof) right
after the existing verification step (the place that currently verifies
msg.key.proof) and, if they differ, return an error or publish a failure event
(and log details) instead of proceeding to publish public_outputs; this ensures
the published public_signals (signed_payload.payload.proof.public_signals) are
only emitted when both proof fields are identical.
---
Nitpick comments:
In `@crates/zk-prover/src/actors/commitment_consistency_checker.rs`:
- Around line 121-170: The nested loops in check_cross_party_link produce O(n·m)
comparisons; to optimize, build an index (HashSet or HashMap) of extracted
commitment values from the targets (or sources when the new proof is a target)
and then probe that index for each source instead of iterating all targets.
Concretely, inside check_cross_party_link, replace the inner loop by first
collecting extracted values for each target via link.extract_target_values (or
link.extract_source_values if the API only provides that for the relevant role),
insert those values (or serialized forms) into a HashSet/HashMap keyed by the
commitment, then for each src call link.extract_source_values and check
membership with O(1) lookups, using link.check_consistency only when a candidate
match exists; keep the existing logging and behavior when no index hit is found.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 90-91: The cache currently stores party_public_signals
(HashMap<u64, Vec<(ProofType, ArcBytes)>>) separately from party_proof_hashes
and later rejoins them by vector index, which is fragile; change the cache to
store a single per-proof record struct (e.g., ProofCacheEntry) that contains
proof_type, data_hash (the proof hash), and public_outputs (ArcBytes) and
replace uses of party_public_signals and party_proof_hashes with a single
HashMap<u64, Vec<ProofCacheEntry>>; update construction sites (where
signed_proofs() is iterated to build caches, referenced in functions around the
previous indices at lines ~242-267 and ~461-466) to populate all fields
atomically and update lookup/consumption sites to use the combined entry so no
index-based rejoining or unwrap_or_default() fallbacks are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7288256-7196-4e44-999d-9bdc335e9af8
📒 Files selected for processing (11)
crates/ciphernode-builder/src/ciphernode_builder.rscrates/events/src/enclave_event/proof_verification_passed.rscrates/request/src/context.rscrates/zk-prover/src/actors/commitment_consistency_checker.rscrates/zk-prover/src/actors/commitment_consistency_checker_ext.rscrates/zk-prover/src/actors/commitment_links/c1_to_c5.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/actors/share_verification.rscrates/zk-prover/src/lib.rs
ctrlc03
reviewed
Mar 26, 2026
ctrlc03
left a comment
ctrlc03
left a comment
Collaborator
There was a problem hiding this comment.
let's first merge c1-c5 and reuse the utilities to extract signals?
ctrlc03
approved these changes
Mar 26, 2026
This was referenced Mar 26, 2026
Merged
Merged
This was referenced May 22, 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.
Closes #1472
Adds a generic, extensible framework for detecting cross-circuit commitment mismatches, where a party's ZK proof outputs in one circuit are inconsistent with the public inputs or outputs expected by another circuit. On mismatch, the checker emits
ProofVerificationFailed, which flows into the existingAccusationManagerquorum -> slash pipeline unchanged.How to add a new cross-circuit check
When implementing a new circuit pair where commitments should be cross-validated, follow these steps:
Step 1: Register the output layout
Add your circuit's output field positions to
CircuitOutputLayout::for_circuit():Step 2: Implement
CommitmentLinkStep 3: Register the link
Add it to the checker's link list in
CommitmentConsistencyCheckerExtension::on_event():That's it. The checker handles caching, matching, comparison, and emitting
ProofVerificationFailedautomatically.Summary by CodeRabbit