feat: onchain verification for C7, C5 [skip-line-limit]#1443
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an optional proof-aggregation flag across request/event/actor flows, threads it into SDK/UI/actors, wires PK-aggregation proofs into on‑chain publication and verifier interfaces, changes PK aggregation verification and centering in circuits, and increases MAX_MSG_NON_ZERO_COEFFS 80→100 with CRISP encoding updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Enclave as Enclave (on-chain)
participant Nodes as Ciphernodes
participant Aggregator as PublicKeyAggregator
participant PKVerifier as IPkVerifier (on-chain)
participant Registry as CiphernodeRegistry
Client->>Enclave: requestE3(..., proofAggregationEnabled)
Enclave-->>Nodes: emit ThresholdSharePending (proof_aggregation_enabled)
Nodes->>Aggregator: submit C5 proofs (Some<Proof> / None)
Aggregator->>Aggregator: fold proofs (skip if disabled)
Aggregator->>Enclave: publishCommittee(publicKey, proof?)
Enclave->>PKVerifier: pkVerifier.verify(proof)
PKVerifier-->>Enclave: bytes32 pkCommitment
Enclave->>Registry: publishCommittee(..., proof)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
cedoor
force-pushed
the
feat/onchain-circuit-verification
branch
from
f69fd53 to
80780e8
Compare
cedoor
changed the title
cedoor
force-pushed
the
feat/onchain-circuit-verification
branch
from
14b175d to
3d252bb
Compare
cedoor
changed the title
Made-with: Cursor
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/scripts/deployEnclave.ts (1)
41-49:⚠️ Potential issue | 🟡 MinorUse the signer's provider to get the network name instead of
hre.globalOptions.network.
hre.globalOptions.networkcan be undefined and defaults to"localhost"when undefined, which would causeautoCleanForLocalhost()to run incorrectly on non-local networks if called from a non-CLI context. The codebase already uses the provider-based approach in newer deployment scripts for reliability.🔧 Suggested fix
export const deployEnclave = async ( withMocks?: boolean, withZKVerification?: boolean, ) => { const { ethers } = await hre.network.connect(); + const [owner] = await ethers.getSigners(); // Auto-clean state for local networks to prevent stale state issues - const networkName = hre.globalOptions.network ?? "localhost"; + const networkName = (await owner.provider?.getNetwork())?.name ?? "localhost"; await autoCleanForLocalhost(networkName); - - const [owner] = await ethers.getSigners(); const ownerAddress = await owner.getAddress();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/deployEnclave.ts` around lines 41 - 49, The code uses hre.globalOptions.network which can be undefined; update deployEnclave to derive the network name from the signer's provider instead: after the existing const { ethers } = await hre.network.connect(), call await ethers.provider.getNetwork() and use its .name (fallback to "localhost" if empty) when passing into autoCleanForLocalhost; update any references to networkName so autoCleanForLocalhost(networkName) uses the provider-derived name (functions to check: deployEnclave and autoCleanForLocalhost invocation).
🧹 Nitpick comments (2)
crates/evm/src/helpers.rs (1)
326-345: Consider verifying decoded content matches input values.The test validates structural correctness (decodes as
(bytes, bytes32[])) but doesn't verify the decoded values match the originalraw_proofandpublic_signals. Adding value assertions would catch potential encoding/ordering bugs.💡 Optional enhancement to verify decoded values
let tuple_type = DynSolType::Tuple(vec![ DynSolType::Bytes, DynSolType::Array(Box::new(DynSolType::FixedBytes(32))), ]); // Pair encode_zk_proof's abi_encode_params with abi_decode_params (not abi_decode). - tuple_type.abi_decode_params(&encoded).expect( + let decoded = tuple_type.abi_decode_params(&encoded).expect( "encoded proof should decode as (bytes, bytes32[]) - matches contract abi.decode", ); + + // Verify decoded values match input + if let alloy_dyn_abi::DynSolValue::Tuple(parts) = decoded { + if let alloy_dyn_abi::DynSolValue::Bytes(decoded_proof) = &parts[0] { + assert_eq!(decoded_proof, &raw_proof, "raw proof should match"); + } + if let alloy_dyn_abi::DynSolValue::Array(inputs) = &parts[1] { + assert_eq!(inputs.len(), 2, "should have 2 public inputs"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/helpers.rs` around lines 326 - 345, Test test_encode_zk_proof_abi_format only checks structure; update it to decode the params returned by encode_zk_proof and assert the decoded values equal the original raw_proof and public_signals: call tuple_type.abi_decode_params(&encoded), extract the first element as bytes and compare to raw_proof, extract the second element as the bytes32 array (each FixedBytes(32)) and compare concatenated or per-element equality to public_signals, and add assertions so encode_zk_proof/Proof encoding errors in ordering or content are caught.packages/enclave-contracts/test/Enclave.spec.ts (1)
84-85: Add one explicit failing-proof test to lock verification behavior.Given this suite now wires PK verification, add a negative-path assertion (e.g., malformed proof or wrong commitment) to ensure proof validation is actually enforced and not only happy-pathed.
Also applies to: 241-254, 297-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Enclave.spec.ts` around lines 84 - 85, Add a negative-path test that verifies PK proof validation by calling encodePkProof and registry.publishCommittee with a malformed proof or wrong commitment and asserting the call reverts/throws; specifically, reuse the existing setup that computes proof via encodePkProof(ethers.keccak256(publicKey)) but alter the proof (e.g., flip a byte or pass a random buffer) or change the commitment value and then assert registry.publishCommittee(e3Id, nodes, publicKey, badProof) (or with wrong commitment) fails; add this assertion alongside the existing happy-path tests that call encodePkProof and registry.publishCommittee so the suite enforces verification (also add similar negative tests where publishCommittee is invoked in the other related test blocks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 236-278: When shouldHaveZKVerification is false we currently call
deployMocks() then silently skip setting verifiers if
mockDecryptionVerifierAddress or mockPkVerifierAddress is falsy, leaving the BFV
scheme partially configured; change deploy logic (after deployMocks() returns)
to validate that both mockDecryptionVerifierAddress and mockPkVerifierAddress
are present when shouldHaveZKVerification === false and throw or exit
immediately if either is missing so the process fails fast; update the block
surrounding deployMocks(), mockDecryptionVerifierAddress, mockPkVerifierAddress,
shouldHaveZKVerification, and before calling enclave.setDecryptionVerifier /
enclave.setPkVerifier / enclave.enableE3Program to enforce this check.
---
Outside diff comments:
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 41-49: The code uses hre.globalOptions.network which can be
undefined; update deployEnclave to derive the network name from the signer's
provider instead: after the existing const { ethers } = await
hre.network.connect(), call await ethers.provider.getNetwork() and use its .name
(fallback to "localhost" if empty) when passing into autoCleanForLocalhost;
update any references to networkName so autoCleanForLocalhost(networkName) uses
the provider-derived name (functions to check: deployEnclave and
autoCleanForLocalhost invocation).
---
Nitpick comments:
In `@crates/evm/src/helpers.rs`:
- Around line 326-345: Test test_encode_zk_proof_abi_format only checks
structure; update it to decode the params returned by encode_zk_proof and assert
the decoded values equal the original raw_proof and public_signals: call
tuple_type.abi_decode_params(&encoded), extract the first element as bytes and
compare to raw_proof, extract the second element as the bytes32 array (each
FixedBytes(32)) and compare concatenated or per-element equality to
public_signals, and add assertions so encode_zk_proof/Proof encoding errors in
ordering or content are caught.
In `@packages/enclave-contracts/test/Enclave.spec.ts`:
- Around line 84-85: Add a negative-path test that verifies PK proof validation
by calling encodePkProof and registry.publishCommittee with a malformed proof or
wrong commitment and asserting the call reverts/throws; specifically, reuse the
existing setup that computes proof via
encodePkProof(ethers.keccak256(publicKey)) but alter the proof (e.g., flip a
byte or pass a random buffer) or change the commitment value and then assert
registry.publishCommittee(e3Id, nodes, publicKey, badProof) (or with wrong
commitment) fails; add this assertion alongside the existing happy-path tests
that call encodePkProof and registry.publishCommittee so the suite enforces
verification (also add similar negative tests where publishCommittee is invoked
in the other related test blocks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f5f2490-3ec4-4efa-8f82-6056569a9e8d
📒 Files selected for processing (14)
crates/evm/src/helpers.rscrates/zk-prover/tests/local_e2e_tests.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.exampleexamples/CRISP/server/src/config.rspackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/scripts/deployAndSave/bfvPkVerifier.tspackages/enclave-contracts/scripts/deployAndSave/mockPkVerifier.tspackages/enclave-contracts/scripts/deployAndSave/verifiers.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/scripts/deployMocks.tspackages/enclave-contracts/test/Enclave.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/CRISP/server/src/config.rs
- packages/enclave-contracts/.gitignore
- packages/enclave-contracts/scripts/deployAndSave/mockPkVerifier.ts
- examples/CRISP/enclave.config.yaml
- packages/enclave-contracts/scripts/deployMocks.ts
- crates/zk-prover/tests/local_e2e_tests.rs
- packages/enclave-contracts/scripts/deployAndSave/verifiers.ts
- packages/enclave-contracts/scripts/deployAndSave/bfvPkVerifier.ts
- Override default Provable path that built base-circuit inputs for the wrapper - Recursive variant runs the full base→chunk→batch→final pipeline - Other variants return a clear ProveFailed error Made-with: Cursor
ctrlc03
approved these changes
Mar 20, 2026
This was referenced Mar 20, 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.
Summary by CodeRabbit
New Features
Configuration Changes
Improvements