feat: link c0-c3#1495
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds cross-circuit consistency checking between C3 share-encryption proofs and C0 pk-commitment proofs. It extends circuit input layout infrastructure with field extraction methods, introduces new commitment-link implementations to validate pk commitment references, updates the consistency checker to cache multiple proofs per address/type, and publishes proof verification events for local proof completion. Changes
Sequence DiagramsequenceDiagram
participant C3Handler as C3 Proof Handler
participant ConsistencyChecker as Consistency<br/>Checker
participant InputLayout as Input Layout<br/>Extractor
participant C3LinkImpl as C3→C0<br/>Link
participant C0Proofs as C0 Proof<br/>Cache
C3Handler->>ConsistencyChecker: ProofVerificationPassed(C3)
ConsistencyChecker->>ConsistencyChecker: Cache C3 proof data
ConsistencyChecker->>ConsistencyChecker: Find matching links for C3
ConsistencyChecker->>C3LinkImpl: check_consistency(C3, targets=[C0,...])
C3LinkImpl->>InputLayout: extract via ShareEncryption layout
InputLayout->>InputLayout: field_index("expected_pk_commitment")
InputLayout->>InputLayout: extract_field(C3.public_signals)
InputLayout-->>C3LinkImpl: extracted pk commitment
C3LinkImpl->>C0Proofs: iterate target C0 entries
C3LinkImpl->>InputLayout: extract via PkBfv output layout
InputLayout-->>C3LinkImpl: target pk commitment
C3LinkImpl->>C3LinkImpl: Compare C3 vs C0 pk commitment
alt Match found
C3LinkImpl-->>ConsistencyChecker: Ok(consistent)
else No match
C3LinkImpl-->>ConsistencyChecker: Err(mismatch)
ConsistencyChecker->>ConsistencyChecker: Mark source dishonest
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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}
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_links/c0_to_c3.rs`:
- Around line 34-44: The link is oriented incorrectly: change the link so C3 is
the source and C0 is the target and use the receiver-based scope so failures
attribute to the C3 payload; specifically update source_proof_type() to return
ProofType::C3aSkShareEncryption, target_proof_type() to return
ProofType::C0PkBfv, and change scope() from LinkScope::SameParty to
LinkScope::Receiver (or the enum variant representing the receiver-based check),
and apply the same swap in the other duplicated impl blocks referenced (the
blocks around the 46-54 and 95-119 ranges) so C3→C0 receiver checks emit faults
against the C3 proof.
🪄 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: a9294743-a613-437c-9ad7-3e5b4a0af6c4
📒 Files selected for processing (5)
crates/events/src/enclave_event/proof.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-prover/src/actors/commitment_links/c0_to_c3.rscrates/zk-prover/src/actors/commitment_links/c2_to_c3.rscrates/zk-prover/src/actors/commitment_links/mod.rs
ctrlc03
force-pushed
the
feat/link-c0-c3
branch
from
c0709ac to
aff1c2b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/zk-prover/src/actors/commitment_links/c0_to_c3.rs (1)
34-47:⚠️ Potential issue | 🟠 MajorFault attribution direction may be inverted for issue
#1449requirements.Per the past review and issue
#1449's acceptance criteria: "A C3 proof with an incorrect expected_pk_commitment causes the sender to be marked dishonest and the corresponding C3 payload to be reported as failed."With the current orientation (C0 as source, C3 as target),
find_mismatches()inCommitmentConsistencyCheckerreturnsMismatchstructs containing the source proof's attributes. This means violations would be attributed to the C0 submitter (receiver) rather than the C3 submitter (sender).To achieve correct fault attribution, consider reversing the link direction:
- Source: C3 (extract
expected_pk_commitmentfrom input)- Target: C0 (extract
pk_commitmentfrom output)This way, mismatches would attribute to the C3 sender as required.
🤖 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/c0_to_c3.rs` around lines 34 - 47, The current link is oriented with source_proof_type() => ProofType::C0PkBfv and target_proof_type() => ProofType::C3aSkShareEncryption which causes mismatches to attribute to the C0 submitter; change the direction so source_proof_type() returns ProofType::C3aSkShareEncryption and target_proof_type() returns ProofType::C0PkBfv (keep scope() as LinkScope::CrossParty) so find_mismatches() will extract expected_pk_commitment from the C3 input and pk_commitment from the C0 output and thus blame the C3 sender as required by issue `#1449`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/zk-prover/src/actors/commitment_links/c0_to_c3.rs`:
- Around line 34-47: The current link is oriented with source_proof_type() =>
ProofType::C0PkBfv and target_proof_type() => ProofType::C3aSkShareEncryption
which causes mismatches to attribute to the C0 submitter; change the direction
so source_proof_type() returns ProofType::C3aSkShareEncryption and
target_proof_type() returns ProofType::C0PkBfv (keep scope() as
LinkScope::CrossParty) so find_mismatches() will extract expected_pk_commitment
from the C3 input and pk_commitment from the C0 output and thus blame the C3
sender as required by issue `#1449`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8a9dc22-be73-4b64-9442-9abfe1e30b04
📒 Files selected for processing (5)
crates/events/src/enclave_event/proof.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-prover/src/actors/commitment_links/c0_to_c3.rscrates/zk-prover/src/actors/commitment_links/c2_to_c3.rscrates/zk-prover/src/actors/commitment_links/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/events/src/enclave_event/proof.rs
ctrlc03
force-pushed
the
feat/link-c0-c3
branch
from
aff1c2b to
1e2040e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/zk-prover/src/actors/commitment_links/mod.rs (1)
37-42:⚠️ Potential issue | 🟠 Major
SourceMustExistInTargetsis still too weak for the C3→C0 rule.For the new C3 links, this only proves that the claimed
expected_pk_commitmentexists somewhere in the global C0 set. That lets a tampered C3 point at another participant’s validpk_commitmentand still pass, even though it does not match the local receiver’s C0.#1449needs receiver-scoped matching here, or the check needs to stay inShareVerificationActor, where the recipient context exists. A regression test with two valid C0 commitments would catch this.Also applies to: 75-76
🤖 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/mod.rs` around lines 37 - 42, The enum variant SourceMustExistInTargets is too global for the C3→C0 rule and allows a C3 to match any party's C0; change the link check to be receiver-scoped by either (A) replacing or augmenting SourceMustExistInTargets with a variant that includes receiver identity (e.g., SourceMustExistInReceiverTargets or include a recipient_id field) and update the matching logic where commitment links are validated to check expected_pk_commitment against only that receiver's C0 set, or (B) move/retain the C3→C0 recipient-specific validation in ShareVerificationActor (where recipient context exists) and ensure SourceMustExistInTargets is not used for recipient-scoped C3 checks; update tests to add a regression case with two different valid C0 commitments to ensure a tampered C3 cannot point to another participant's pk_commitment.
🤖 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 317-325: The cache insertion currently pushes duplicate
VerifiedProofData into self.verified for the same (address, proof_type), causing
find_mismatches() to report duplicates; add a small helper (e.g.,
insert_verified_proof) that takes (address, proof_type, VerifiedProofData) and
only appends when there is no existing entry with the same data_hash, then
replace the direct .push(...) calls in the CommitmentConsistencyCheckRequested
and ProofVerificationPassed handling (the sites that currently call .push at the
shown snippet and the similar block at lines ~346-354) to call this helper so
multiple distinct C3s are kept but duplicate data_hash entries are rejected.
---
Duplicate comments:
In `@crates/zk-prover/src/actors/commitment_links/mod.rs`:
- Around line 37-42: The enum variant SourceMustExistInTargets is too global for
the C3→C0 rule and allows a C3 to match any party's C0; change the link check to
be receiver-scoped by either (A) replacing or augmenting
SourceMustExistInTargets with a variant that includes receiver identity (e.g.,
SourceMustExistInReceiverTargets or include a recipient_id field) and update the
matching logic where commitment links are validated to check
expected_pk_commitment against only that receiver's C0 set, or (B) move/retain
the C3→C0 recipient-specific validation in ShareVerificationActor (where
recipient context exists) and ensure SourceMustExistInTargets is not used for
recipient-scoped C3 checks; update tests to add a regression case with two
different valid C0 commitments to ensure a tampered C3 cannot point to another
participant's pk_commitment.
🪄 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: 78ab5b0e-f2a7-4f96-9f6e-f300999dce82
📒 Files selected for processing (6)
crates/events/src/enclave_event/proof.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-prover/src/actors/commitment_consistency_checker.rscrates/zk-prover/src/actors/commitment_links/c0_to_c3.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/src/actors/proof_request.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/events/src/enclave_event/proof.rs
- crates/zk-helpers/src/circuits/output_layout.rs
2 participants
fix #1449
Summary by CodeRabbit
Release Notes
New Features
Tests