refactor: check final vk_hash in crisp fold circuit#1471
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds tooling to compute and embed aggregated VK-hashes: a Rust CLI, codegen support to extract and emit VK-hash literals, scripts to run the CLI during CRISP builds, circuit changes to assert expected VK-hashes, and corresponding verifier/contract and SDK adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Files as VK Hash Files
participant Script as compute_vk_hash.sh
participant Binary as compute-vk-hash (Rust)
participant Codegen as zk-helpers codegen
participant Circuit as CRISP Fold Circuit
participant Contract as Verifier/Contract
Files->>Script: ensure vk_recursive_hash files exist
Script->>Binary: run with vk_hash file paths
Binary->>Binary: read 32-byte blobs -> Fr fields
Binary->>Binary: call compute_vk_hash(fields)
Binary->>Script: print 0x-prefixed 32-byte hex
Script->>Codegen: (during build) provide hex literals
Codegen->>Circuit: embed EXPECTED_KEY_HASH into configs.nr
Circuit->>Circuit: compute_vk_hash(vk_hashes) and assert == EXPECTED
Circuit->>Contract: produce proof public inputs (vk_hash omitted)
Contract->>Contract: verify proof with updated verifier constants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
342-347: Document this wire-format break before the next SDK publish.
encodeSolidityProofis re-exported fromexamples/CRISP/packages/crisp-sdk/src/index.ts:17-20, but the payload silently changed from a 5-field ABI tuple to 4 fields without any signature change. Please add a migration note or version bump so downstream callers do not discover it viapublishInputdecode failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-sdk/src/vote.ts` around lines 342 - 347, The ABI tuple emitted by encodeSolidityProof changed from a 5-field tuple to 4 fields (the current encodeAbiParameters call uses 'bytes, address, bytes32, bytes'), but the function is re-exported (encodeSolidityProof via index.ts) without a signature or version change; add a migration note and version bump and/or provide a compatibility wrapper: update the SDK changelog and README with a clear "wire-format break" entry for encodeSolidityProof, increment the package version before publishing, and optionally add a backward-compatible helper (e.g., encodeSolidityProofV1 or a wrapper in vote.ts that accepts old 5-field inputs and converts to the new 4-field payload) so downstream callers and publishInput decoders do not fail silently.
🤖 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-helpers/src/bin/compute_vk_hash.rs`:
- Around line 30-37: The current field_from_vk_hash_file function uses
Fr::from_be_bytes_mod_order which silently reduces out-of-range 32-byte inputs;
change it to reject non-canonical values by parsing the 32 bytes into the field
bigint and calling Fr::from_bigint (returning an error if it yields None) or by
using CanonicalDeserialize::deserialize_unchecked to enforce canonical
representation; keep the existing filesize check, convert the 32-byte big-endian
slice into the expected bigint type, and return a clear error when the value is
not in [0, p-1] so malformed VK hash files are not silently accepted.
In `@examples/CRISP/scripts/compile_circuits.sh`:
- Around line 44-57: The new bb write_vk calls produce only vk files but
downstream compute_vk_hash.sh expects corresponding vk_hash files; update the
script (around the bb write_vk invocations for targets using
THRESHOLD_TARGET/${name}.json and CRISP_RECURSIVE_VK/crisp) to produce vk_hash
files immediately after each successful bb write_vk by computing and writing the
hash of the generated vk file (e.g., read the just-written
"${RECURSIVE_VK_BASE}/${name}/vk" and write its hash to
"${RECURSIVE_VK_BASE}/${name}/vk_hash", and likewise for
"${CRISP_RECURSIVE_VK}/vk" -> "${CRISP_RECURSIVE_VK}/vk_hash") so
compute_vk_hash.sh finds the expected vk_hash files.
---
Nitpick comments:
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 342-347: The ABI tuple emitted by encodeSolidityProof changed from
a 5-field tuple to 4 fields (the current encodeAbiParameters call uses 'bytes,
address, bytes32, bytes'), but the function is re-exported (encodeSolidityProof
via index.ts) without a signature or version change; add a migration note and
version bump and/or provide a compatibility wrapper: update the SDK changelog
and README with a clear "wire-format break" entry for encodeSolidityProof,
increment the package version before publishing, and optionally add a
backward-compatible helper (e.g., encodeSolidityProofV1 or a wrapper in vote.ts
that accepts old 5-field inputs and converts to the new 4-field payload) so
downstream callers and publishInput decoders do not fail silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37e8e05e-ae3b-4c88-b01a-7d8b5d20ae0b
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/zk-helpers/Cargo.tomlcrates/zk-helpers/src/bin/compute_vk_hash.rscrates/zk-helpers/src/circuits/commitments.rsexamples/CRISP/Cargo.tomlexamples/CRISP/circuits/bin/fold/src/main.nrexamples/CRISP/package.jsonexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/scripts/compile_circuits.shexamples/CRISP/scripts/compute_vk_hash.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
244-264: Good test coverage for VK hash hex literal generation.The test correctly:
- Creates synthetic vk_hash files with distinct values
- Verifies output format (0x prefix, 66 chars)
- Asserts SK and ESM hashes differ
Consider adding a test that verifies the actual hash computation by using known input values and checking against expected outputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs` around lines 244 - 264, Add a new unit test alongside share_computation_expected_vk_hash_hex_literals_differ_for_sk_and_esm that uses deterministic, known vk_hash byte contents (e.g., explicit 32-byte arrays) written to the same directory names, then calls share_computation_expected_vk_hash_hex_literals(&rv) and asserts the returned sk and esm exactly equal the precomputed expected hex strings (including "0x" prefix and 66-char length); reference the function share_computation_expected_vk_hash_hex_literals and the existing test name to place and structure the new test so it verifies exact hash outputs for fixed inputs.crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs (1)
101-113: VerifyFr::from_be_bytes_mod_orderbehavior for non-canonical inputs.If a
vk_hashfile contains bytes representing a value ≥ the BN254 scalar field modulus,from_be_bytes_mod_ordersilently reduces it modulo the field order. This could mask corrupted artifacts. Consider adding a check that the bytes represent a canonical field element.♻️ Optional: Add canonical check
fn fr_from_vk_hash_file(path: &Path) -> Result<Fr, String> { let bytes = fs::read(path).map_err(|e| format!("{}: {e}", path.display()))?; if bytes.len() != 32 { return Err(format!( "{}: expected 32-byte vk_hash, got {}", path.display(), bytes.len() )); } let mut arr = [0u8; 32]; arr.copy_from_slice(&bytes); - Ok(Fr::from_be_bytes_mod_order(&arr)) + // Validate canonical representation (bytes < field modulus) + let fr = Fr::from_be_bytes_mod_order(&arr); + let roundtrip = fr.into_bigint().to_bytes_be(); + if roundtrip != arr { + return Err(format!( + "{}: vk_hash bytes are not a canonical field element", + path.display() + )); + } + Ok(fr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs` around lines 101 - 113, fr_from_vk_hash_file currently uses Fr::from_be_bytes_mod_order which silently reduces non-canonical 32-byte inputs; update the function to detect non-canonical encodings by interpreting the 32-byte big-endian value and comparing it to the BN254 scalar modulus (the field modulus exposed by the Fr type or its associated constant) and return an Err if the value is >= modulus, only calling Fr::from_be_bytes_mod_order (or the canonical constructor) when the value is strictly less than the modulus; include the file path in the error message (e.g., "{}: non-canonical vk_hash >= modulus") and keep the existing behavior for valid 32-byte canonical inputs.
🤖 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/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 244-264: Add a new unit test alongside
share_computation_expected_vk_hash_hex_literals_differ_for_sk_and_esm that uses
deterministic, known vk_hash byte contents (e.g., explicit 32-byte arrays)
written to the same directory names, then calls
share_computation_expected_vk_hash_hex_literals(&rv) and asserts the returned sk
and esm exactly equal the precomputed expected hex strings (including "0x"
prefix and 66-char length); reference the function
share_computation_expected_vk_hash_hex_literals and the existing test name to
place and structure the new test so it verifies exact hash outputs for fixed
inputs.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs`:
- Around line 101-113: fr_from_vk_hash_file currently uses
Fr::from_be_bytes_mod_order which silently reduces non-canonical 32-byte inputs;
update the function to detect non-canonical encodings by interpreting the
32-byte big-endian value and comparing it to the BN254 scalar modulus (the field
modulus exposed by the Fr type or its associated constant) and return an Err if
the value is >= modulus, only calling Fr::from_be_bytes_mod_order (or the
canonical constructor) when the value is strictly less than the modulus; include
the file path in the error message (e.g., "{}: non-canonical vk_hash >=
modulus") and keep the existing behavior for valid 32-byte canonical inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 675f7d4b-f4df-47fd-b5a6-33e6d5ab3c15
📒 Files selected for processing (6)
circuits/bin/dkg/share_computation/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/utils.rs
✅ Files skipped from review due to trivial changes (1)
- crates/zk-helpers/src/bin/zk_cli.rs
3821cd8 to
8f92e8a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
circuits/bin/dkg/share_computation/src/main.nr (1)
67-70: Add an error message to the assertion for better debugging.The other assertions in this file include descriptive error messages (e.g., Line 39:
"base_key_hash mismatch across batches"). Adding a similar message here would improve debugging if verification fails.♻️ Proposed fix to add an error message
assert( (key_hash == SHARE_COMPUTATION_EXPECTED_VK_HASH_SK) | (key_hash == SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM), + "key_hash does not match expected SK or ESM vk_hash", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/dkg/share_computation/src/main.nr` around lines 67 - 70, The assertion comparing key_hash against SHARE_COMPUTATION_EXPECTED_VK_HASH_SK and SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM lacks a descriptive error message; update the assert that checks (key_hash == SHARE_COMPUTATION_EXPECTED_VK_HASH_SK) | (key_hash == SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM) to include a clear message (e.g., "verification key hash mismatch for share computation") so failures show context and the offending key_hash and expected constants (SHARE_COMPUTATION_EXPECTED_VK_HASH_SK, SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM) in the log.crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
140-161: Consider clarifying the fallback behavior whenENCLAVE_CIRCUITS_ROOTis unset.The current logic silently returns empty strings when the environment variable is unset and resolution fails (Line 160), but returns an error if it's explicitly set but points to an invalid path (Lines 146-158). This asymmetry could lead to confusing behavior where a user expects VK hashes to be generated but they're silently omitted.
Consider adding a debug/info log when falling back to empty strings, so users understand why VK hash constants are missing from generated configs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs` around lines 140 - 161, The fallback branch for resolve_enclave_circuits_root that returns (String::new(), String::new()) silently omits VK hashes (affecting vk_sk and vk_esm); update the logic in the match over resolve_enclave_circuits_root() to emit a clear info/debug log when falling back to empty strings (use the project's logging facility, e.g., process_logger or tracing) and include context that ENCLAVE_CIRCUITS_ROOT was unset or resolution failed and that VK literals will be omitted; keep the existing error behavior when explicit_circuits_root is true and resolution fails, and reference the functions/variables resolve_enclave_circuits_root, share_computation_expected_vk_hash_hex_literals, explicit_circuits_root, vk_sk, and vk_esm to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@circuits/bin/dkg/share_computation/src/main.nr`:
- Around line 67-70: The assertion comparing key_hash against
SHARE_COMPUTATION_EXPECTED_VK_HASH_SK and SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM
lacks a descriptive error message; update the assert that checks (key_hash ==
SHARE_COMPUTATION_EXPECTED_VK_HASH_SK) | (key_hash ==
SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM) to include a clear message (e.g.,
"verification key hash mismatch for share computation") so failures show context
and the offending key_hash and expected constants
(SHARE_COMPUTATION_EXPECTED_VK_HASH_SK, SHARE_COMPUTATION_EXPECTED_VK_HASH_ESM)
in the log.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 140-161: The fallback branch for resolve_enclave_circuits_root
that returns (String::new(), String::new()) silently omits VK hashes (affecting
vk_sk and vk_esm); update the logic in the match over
resolve_enclave_circuits_root() to emit a clear info/debug log when falling back
to empty strings (use the project's logging facility, e.g., process_logger or
tracing) and include context that ENCLAVE_CIRCUITS_ROOT was unset or resolution
failed and that VK literals will be omitted; keep the existing error behavior
when explicit_circuits_root is true and resolution fails, and reference the
functions/variables resolve_enclave_circuits_root,
share_computation_expected_vk_hash_hex_literals, explicit_circuits_root, vk_sk,
and vk_esm to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88620d02-6e4d-4ecf-8696-cca067a0e7c8
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
circuits/bin/dkg/share_computation/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcrates/zk-helpers/Cargo.tomlcrates/zk-helpers/src/bin/compute_vk_hash.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/utils.rsexamples/CRISP/Cargo.tomlexamples/CRISP/circuits/bin/fold/src/main.nrexamples/CRISP/package.jsonexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/scripts/compile_circuits.shexamples/CRISP/scripts/compute_vk_hash.sh
✅ Files skipped from review due to trivial changes (4)
- examples/CRISP/Cargo.toml
- examples/CRISP/package.json
- crates/zk-helpers/src/bin/zk_cli.rs
- examples/CRISP/scripts/compute_vk_hash.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- circuits/lib/src/configs/secure/dkg.nr
- examples/CRISP/packages/crisp-sdk/src/vote.ts
- circuits/lib/src/configs/insecure/dkg.nr
- crates/zk-helpers/Cargo.toml
- crates/zk-helpers/src/circuits/dkg/share_computation/utils.rs
- crates/zk-helpers/src/bin/compute_vk_hash.rs
- examples/CRISP/circuits/bin/fold/src/main.nr
07a732b to
bede6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/CRISP/scripts/compile_circuits.sh (1)
41-47:⚠️ Potential issue | 🟠 Major
bb write_vkartifact assumption can break this pipeline.Line 46 and Line 53 assume
vk_hashis emitted for-t noir-recursive-no-zk. If onlyvkis produced,mv .../vk_hashfails and the script exits before fold compilation. Please generate*.vk_recursive_hashin a deterministic post-step from the emittedvk(or update downstream consumers accordingly).#!/bin/bash set -euo pipefail # Run from repository root. Verifies actual bb output artifacts for this mode. tmp_dir="$(mktemp -d)" bb write_vk \ -b "circuits/bin/threshold/target/user_data_encryption_ct0.json" \ -o "$tmp_dir" \ -t noir-recursive-no-zk echo "Artifacts in $tmp_dir:" find "$tmp_dir" -maxdepth 1 -type f -printf "%f\n" | sort # Current script requires both files: test -f "$tmp_dir/vk" test -f "$tmp_dir/vk_hash"Expected result: if
vk_hashis missing, currentmvlogic at Line 46/Line 53 is invalid.Also applies to: 48-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/scripts/compile_circuits.sh` around lines 41 - 47, The script assumes bb write_vk emits both "vk" and "vk_hash" but sometimes only "vk" is produced; update the loop that handles bb write_vk (referencing bb write_vk, THRESHOLD_TARGET, name and the downstream mv of vk and vk_hash) to check for the presence of "${THRESHOLD_TARGET}/vk_hash" and, if missing, deterministically generate "${THRESHOLD_TARGET}/${name}.vk_recursive_hash" from the emitted "${THRESHOLD_TARGET}/vk" (e.g., compute a stable hash of the vk file and write it to the .vk_recursive_hash path), and ensure you still move/rename the emitted "vk" to "${name}.vk_recursive"; also add a clear error path if "vk" itself is missing before attempting the hash generation.
🤖 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-helpers/src/circuits/dkg/share_computation/utils.rs`:
- Around line 84-99: Currently the helper checks ENCLAVE_CIRCUITS_ROOT into
variable p and only returns Some(p) if p.join("bin/dkg/target").is_dir(), but if
the env var is set and invalid the code falls back to auto-discovery; change
this so that when ENCLAVE_CIRCUITS_ROOT is present but the expected target path
is missing the function returns None (or an explicit Err) immediately instead of
continuing to the for-loop. Locate the env::var("ENCLAVE_CIRCUITS_ROOT") branch
(variable p) and replace the fallback behavior with an early return None when
the path check fails, preserving the existing upward discovery only when the env
var is not set.
---
Duplicate comments:
In `@examples/CRISP/scripts/compile_circuits.sh`:
- Around line 41-47: The script assumes bb write_vk emits both "vk" and
"vk_hash" but sometimes only "vk" is produced; update the loop that handles bb
write_vk (referencing bb write_vk, THRESHOLD_TARGET, name and the downstream mv
of vk and vk_hash) to check for the presence of "${THRESHOLD_TARGET}/vk_hash"
and, if missing, deterministically generate
"${THRESHOLD_TARGET}/${name}.vk_recursive_hash" from the emitted
"${THRESHOLD_TARGET}/vk" (e.g., compute a stable hash of the vk file and write
it to the .vk_recursive_hash path), and ensure you still move/rename the emitted
"vk" to "${name}.vk_recursive"; also add a clear error path if "vk" itself is
missing before attempting the hash generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5687269-a6b5-4b77-8a22-ad118d45e56b
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.locktemplates/default/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
circuits/bin/dkg/share_computation/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcrates/Dockerfilecrates/zk-helpers/Cargo.tomlcrates/zk-helpers/src/bin/compute_vk_hash.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/utils.rsexamples/CRISP/Cargo.tomlexamples/CRISP/circuits/bin/fold/src/main.nrexamples/CRISP/package.jsonexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.solexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/scripts/compile_circuits.shexamples/CRISP/scripts/compute_vk_hash.sh
✅ Files skipped from review due to trivial changes (5)
- examples/CRISP/package.json
- examples/CRISP/Cargo.toml
- crates/zk-helpers/src/bin/zk_cli.rs
- examples/CRISP/scripts/compute_vk_hash.sh
- examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- circuits/bin/dkg/share_computation/src/main.nr
- crates/zk-helpers/src/circuits/commitments.rs
- examples/CRISP/circuits/bin/fold/src/main.nr
- examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
- circuits/lib/src/configs/insecure/dkg.nr
- crates/zk-helpers/src/bin/compute_vk_hash.rs
- crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests