feat: bind circuits of proof agg to vk hashes [skip-line-limit]#1543
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
agent/flow-trace/04_DKG_AND_COMPUTATION.md (1)
843-843: 💤 Low valueConsider clarifying "trailing 100 coefficients".
The phrase "checks trailing 100 coeffs vs output hash" is somewhat cryptic. For reader clarity, consider briefly explaining what these coefficients represent (e.g., "checks the last 100 coefficients of the decrypted result against the output hash" or similar context).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md` at line 843, Replace the cryptic phrase "checks trailing 100 coeffs vs output hash" with a clearer description such as: "checks the last 100 coefficients of the decrypted result against the expected output hash (these coefficients correspond to the least-significant polynomial coefficients or final output chunks used to verify result integrity)". Update the line containing "checks trailing 100 coeffs vs output hash" to this expanded wording so readers understand what is being compared and why.packages/enclave-contracts/test/BfvPkVerifier.spec.ts (1)
67-74: ⚡ Quick winAdd the exact two-input boundary case.
This only covers
publicInputs.length === 1. With the VK-hash fields now occupying the front of the proof inputs, an off-by-one length check that accidentally accepts exactly two entries would still slip through the suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/enclave-contracts/test/BfvPkVerifier.spec.ts` around lines 67 - 74, Add a new test that mirrors the existing "returns false when publicInputs has only one entry" but uses exactly two public input entries to cover the boundary case where an off-by-one would accept length === 2; use the same fixtures and helpers (deployWithMockCircuit, encodeProof) and call bfvPkVerifier.verify.staticCall(pkCommitment, proof) with a proof built from two inputs (e.g., [pkCommitment, ethers.keccak256("0x1234")]) and assert the result is false so the verifier rejects the two-input case as well.packages/enclave-contracts/test/BfvDecryptionVerifier.spec.ts (1)
27-35: ⚡ Quick winDon’t let short vectors overwrite the VK-hash slots.
When
totalInputs < MESSAGE_COEFFS_COUNT + 2,offsetdrops below2, so the message loop clobbersarr[0]/arr[1]. That means the short-input tests can fail because the hashes were overwritten instead of because the length guard rejected them, and it leaves the new 101-input boundary unpinned.Possible fix
function buildPublicInputsWithMessage( messageCoeffs: bigint[], totalInputs = 402, subCircuitHashes: [string, string] = [ EXPECTED_C6_FOLD_KEY_HASH, EXPECTED_C7_KEY_HASH, ], ): string[] { + if (totalInputs < MESSAGE_COEFFS_COUNT + 2) { + throw new Error( + "totalInputs must reserve two VK-hash slots plus the message tail", + ); + } const arr: string[] = new Array(totalInputs); arr[0] = subCircuitHashes[0]; arr[1] = subCircuitHashes[1]; for (let i = 2; i < totalInputs; i++) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/enclave-contracts/test/BfvDecryptionVerifier.spec.ts` around lines 27 - 35, The loop writing message coefficients can underrun the reserved VK-hash slots when totalInputs < MESSAGE_COEFFS_COUNT + 2 because offset becomes < 2; change the loop so it never writes below index 2. Concretely, compute a start index like const start = Math.max(2, offset) (using the existing offset calculation) and iterate for (let i = start; i < totalInputs; i++) so arr[0]/arr[1] (populated from subCircuitHashes) cannot be clobbered; keep the existing length guard logic intact so short vectors still fail as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.sol`:
- Around line 55-63: The current length guard uses MESSAGE_COEFFS_COUNT but
misses the two vk-hash header slots, so publicInputs may be too short to contain
expectedC6FoldKeyHash and expectedC7KeyHash plus MESSAGE_COEFFS_COUNT plaintext
coeffs; update the check in the verifier to require at least
MESSAGE_COEFFS_COUNT + 2 entries (change the condition that reads
publicInputs.length < MESSAGE_COEFFS_COUNT to require publicInputs.length <
MESSAGE_COEFFS_COUNT + 2) and keep the subsequent references to publicInputs[0]
and publicInputs[1] as the vk-hash header positions.
In `@packages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.sol`:
- Around line 50-53: The current malformed-input guard only checks
publicInputs.length < 2 but the verifier reads publicInputs[0], publicInputs[1]
and expects a trailing pkCommitment at publicInputs[publicInputs.length - 1];
update the check in the function that uses publicInputs (the early-return block
that references expectedNodesFoldKeyHash and expectedVkHash) to require
publicInputs.length >= 3 (i.e., change the condition to publicInputs.length < 3)
so that the subsequent accesses and the call to circuitVerifier.verify cannot
run on a structurally invalid public input array.
In `@packages/enclave-contracts/test/BfvVkBindingIntegration.spec.ts`:
- Around line 129-176: The tests assume compiled VK artifact files exist; to
avoid hard CI failures, gate the entire "deploy-time VK staleness checks"
describe by detecting those artifacts and skipping the suite when absent:
implement a small helper (e.g., hasCompiledVkArtifacts) that checks for the VK
hash files that deployHonkAndBfv reads (the artifact-reading logic referenced
around the code that loads VK hash artifact files), then replace
describe("deploy-time VK staleness checks", ...) with a conditional that uses
describe.skip when the helper returns false and normal describe when true; keep
the inner tests (which call deployHonkAndBfv,
assertBfvPkVerifierSubCircuitVkHashes, and
assertBfvDecryptionVerifierSubCircuitVkHashes) unchanged.
---
Nitpick comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Line 843: Replace the cryptic phrase "checks trailing 100 coeffs vs output
hash" with a clearer description such as: "checks the last 100 coefficients of
the decrypted result against the expected output hash (these coefficients
correspond to the least-significant polynomial coefficients or final output
chunks used to verify result integrity)". Update the line containing "checks
trailing 100 coeffs vs output hash" to this expanded wording so readers
understand what is being compared and why.
In `@packages/enclave-contracts/test/BfvDecryptionVerifier.spec.ts`:
- Around line 27-35: The loop writing message coefficients can underrun the
reserved VK-hash slots when totalInputs < MESSAGE_COEFFS_COUNT + 2 because
offset becomes < 2; change the loop so it never writes below index 2.
Concretely, compute a start index like const start = Math.max(2, offset) (using
the existing offset calculation) and iterate for (let i = start; i <
totalInputs; i++) so arr[0]/arr[1] (populated from subCircuitHashes) cannot be
clobbered; keep the existing length guard logic intact so short vectors still
fail as intended.
In `@packages/enclave-contracts/test/BfvPkVerifier.spec.ts`:
- Around line 67-74: Add a new test that mirrors the existing "returns false
when publicInputs has only one entry" but uses exactly two public input entries
to cover the boundary case where an off-by-one would accept length === 2; use
the same fixtures and helpers (deployWithMockCircuit, encodeProof) and call
bfvPkVerifier.verify.staticCall(pkCommitment, proof) with a proof built from two
inputs (e.g., [pkCommitment, ethers.keccak256("0x1234")]) and assert the result
is false so the verifier rejects the two-input case as well.
🪄 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: ef143dde-325c-4794-8bb6-78e1fedddbe2
📒 Files selected for processing (16)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/benchmarks/results_insecure/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure/integration_summary.jsoncircuits/benchmarks/results_insecure/report.mdpackages/enclave-contracts/contracts/test/RevertOnVerifyCircuitVerifier.solpackages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.solpackages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.solpackages/enclave-contracts/ignition/modules/bfvDecryptionVerifier.tspackages/enclave-contracts/ignition/modules/bfvPkVerifier.tspackages/enclave-contracts/scripts/benchmarkGasFromRaw.tspackages/enclave-contracts/scripts/deployAndSave/bfvDecryptionVerifier.tspackages/enclave-contracts/scripts/deployAndSave/bfvPkVerifier.tspackages/enclave-contracts/scripts/utils.tspackages/enclave-contracts/test/BfvDecryptionVerifier.spec.tspackages/enclave-contracts/test/BfvPkVerifier.spec.tspackages/enclave-contracts/test/BfvVkBindingIntegration.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.sol (1)
30-38: 💤 Low valueConsider adding a zero-address check for
_circuitVerifier.If accidentally deployed with
address(0), the contract becomes non-functional sincecircuitVerifier.verify()will revert. A simple require statement would prevent deployment errors.🛡️ Optional defensive check
constructor( address _circuitVerifier, bytes32 _expectedNodesFoldKeyHash, bytes32 _expectedC5KeyHash ) { + require(_circuitVerifier != address(0), "Invalid circuit verifier"); circuitVerifier = ICircuitVerifier(_circuitVerifier); expectedNodesFoldKeyHash = _expectedNodesFoldKeyHash; expectedC5KeyHash = _expectedC5KeyHash; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.sol` around lines 30 - 38, Add a deployment-time guard in the constructor to ensure the passed verifier address is not zero: check `_circuitVerifier` with a require before assigning `circuitVerifier = ICircuitVerifier(_circuitVerifier)` so the contract cannot be constructed with `address(0)`; reference the constructor and `circuitVerifier`/`ICircuitVerifier` symbols when making this change and include a clear revert message like "circuit verifier cannot be zero address".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.sol`:
- Around line 30-38: Add a deployment-time guard in the constructor to ensure
the passed verifier address is not zero: check `_circuitVerifier` with a require
before assigning `circuitVerifier = ICircuitVerifier(_circuitVerifier)` so the
contract cannot be constructed with `address(0)`; reference the constructor and
`circuitVerifier`/`ICircuitVerifier` symbols when making this change and include
a clear revert message like "circuit verifier cannot be zero address".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5d82775-dfa5-4c98-b221-4042fb9e11fe
📒 Files selected for processing (9)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deploy/deploy.tsexamples/CRISP/server/.env.examplepackages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.solpackages/enclave-contracts/contracts/verifiers/bfv/BfvPkVerifier.solpackages/enclave-contracts/test/BfvDecryptionVerifier.spec.tspackages/enclave-contracts/test/BfvPkVerifier.spec.tspackages/enclave-contracts/test/BfvVkBindingIntegration.spec.ts
✅ Files skipped from review due to trivial changes (1)
- agent/flow-trace/04_DKG_AND_COMPUTATION.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.sol
- packages/enclave-contracts/test/BfvPkVerifier.spec.ts
- packages/enclave-contracts/test/BfvVkBindingIntegration.spec.ts
- packages/enclave-contracts/test/BfvDecryptionVerifier.spec.ts
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 #1524
Summary by CodeRabbit
New Features
Documentation
Chores