Bls script#698
Conversation
…ripts - Added .gitignore to exclude output directory - Created package.json and package-lock.json for dependency management - Implemented multiplexer circuit in multiplexer.circom - Added build script for compiling circuits and generating proof parameters - Included test vectors for multiplexer in the circom-prover
…generation - Modified build_circuits.sh to include multiple contributions for BLS power series - Enhanced QAP root of unity calculation in qap.rs to accommodate different field sizes - Updated multiplexer_final.zkey with new proof parameters
Deploying mopro with
|
| Latest commit: |
31c3b16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c662efc8.mopro.pages.dev |
| Branch Preview URL: | https://bls-script.mopro.pages.dev |
- Introduced mux.circom circuit implementing a multiplexer - Added input_mux.json for input data - Updated package.json and package-lock.json to include circomlib dependency - Modified build_circuits.sh to compile the new mux circuit and generate proof parameters - Added new test vectors for mux in the circom-prover
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds FFT algorithm improvements to support snarkjs-compatible root selection in the Arkworks prover, introduces a test mux circuit, and validates the changes with a Rust integration test. The build infrastructure compiles the circuit, generates a Groth16 proof, and verifies it; the Rust test exercises this flow end-to-end. ChangesSnarkjs-Compatible FFT and Mux Circuit Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 4
🤖 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 `@circom-prover/src/prover/ark_circom/qap.rs`:
- Around line 81-93: Add an explicit radix-2 precondition check before taking
the manual FFT path: verify that domain_size is a power of two when k != 1 and
return or panic with a clear error if not. Modify the branch around the k != 1
handling (the block using manual_ifft, apply_powers, manual_fft) to call a small
predicate (or inline check) that asserts bit_reverse/manual_fft-safe domain
sizes; reference the symbols domain_size, k, manual_ifft, manual_fft,
bit_reverse and ensure the same guard is added for the other occurrence
mentioned (the block covering lines ~155-211) so non-power-of-two domains fail
fast instead of producing invalid polynomials.
- Around line 64-79: The exponent math can underflow when computing the coset
shift (inc) because s - power - 1 may be negative; add a guarded check before
computing snarkjs_omega / inc: ensure power < s (use the existing s and power
locals) and if not return Err(SynthesisError::PolynomialDegreeTooLarge). Use
this check before the block that computes snarkjs_omega_inv and the inc exponent
so you avoid unsigned underflow when calculating exp for inc and fail early with
SynthesisError::PolynomialDegreeTooLarge.
In `@circom/circuits/multiplexer.circom`:
- Around line 7-8: The signal x is underconstrained because it uses witness-only
assignment (`x <-- 1`) so no R1CS ties x to 1; change that to a constrained
assignment (use `x <== 1` or otherwise add an explicit constraint enforcing x ==
1) so the circuit constrains x; keep the existing multiplication constraint `out
<== in * x` intact so out is correctly tied to in and the now-constrained x.
In `@circom/package.json`:
- Around line 5-7: The package.json "test" script is hardcoded to fail; update
the "test" entry in package.json (the "test" script under "scripts") to either
run a real test command for this package (e.g., invoke your CI test runner or a
small verification that build artifacts exist) or replace the failing command
with a no-op success command so npm test exits 0; ensure the "build" script
("build": "sh scripts/build_circuits.sh") remains unchanged unless you want the
test to call it, and verify npm test in CI/local passes after the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f5689d4-dc0b-4714-905a-572e29afe6d4
⛔ Files ignored due to path filters (4)
circom-prover/test-vectors/multiplexer.wasmis excluded by!**/*.wasmcircom-prover/test-vectors/multiplier2_bls.wasmis excluded by!**/*.wasmcircom-prover/test-vectors/mux.wasmis excluded by!**/*.wasmcircom/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
circom-prover/src/lib.rscircom-prover/src/prover/ark_circom/qap.rscircom-prover/test-vectors/multiplexer_final.zkeycircom-prover/test-vectors/mux.zkeycircom/.gitignorecircom/circuits/multiplexer.circomcircom/circuits/mux.circomcircom/input_mux.jsoncircom/package.jsoncircom/scripts/build_circuits.sh
| let s = F::TWO_ADICITY as u64; | ||
| let power = ark_std::log2(domain_size) as u64; | ||
| let k: u64 = if s == 32 { 5 } else { 1 }; | ||
|
|
||
| // Get snarkjs-compatible generator for this domain size | ||
| let snarkjs_omega = { | ||
| let exp = 1u64 << (s - power); | ||
| F::TWO_ADIC_ROOT_OF_UNITY.pow([k]).pow([exp]) | ||
| }; | ||
| D::distribute_powers_and_mul_by_const(&mut a, root_of_unity, F::one()); | ||
| D::distribute_powers_and_mul_by_const(&mut b, root_of_unity, F::one()); | ||
| let snarkjs_omega_inv = snarkjs_omega.inverse().unwrap(); | ||
|
|
||
| domain.fft_in_place(&mut a); | ||
| domain.fft_in_place(&mut b); | ||
| // Coset shift = snarkjs w[power+1] | ||
| let inc = { | ||
| let exp = 1u64 << (s - power - 1); | ||
| F::TWO_ADIC_ROOT_OF_UNITY.pow([k]).pow([exp]) | ||
| }; |
There was a problem hiding this comment.
Guard exponent math to avoid unsigned underflow in shift counts.
At Line 77, s - power - 1 can underflow for boundary sizes (power >= s), which can break proof generation with invalid shift counts. Add checked arithmetic and fail early with SynthesisError::PolynomialDegreeTooLarge.
Suggested fix
- let s = F::TWO_ADICITY as u64;
- let power = ark_std::log2(domain_size) as u64;
+ let s = F::TWO_ADICITY as u64;
+ let power = ark_std::log2(domain_size) as u64;
+ if power >= s {
+ return Err(SynthesisError::PolynomialDegreeTooLarge);
+ }
let k: u64 = if s == 32 { 5 } else { 1 };
// Get snarkjs-compatible generator for this domain size
let snarkjs_omega = {
- let exp = 1u64 << (s - power);
+ let exp = 1u64 << (s - power);
F::TWO_ADIC_ROOT_OF_UNITY.pow([k]).pow([exp])
};
let snarkjs_omega_inv = snarkjs_omega.inverse().unwrap();
// Coset shift = snarkjs w[power+1]
let inc = {
- let exp = 1u64 << (s - power - 1);
+ let exp = 1u64 << (s - power - 1);
F::TWO_ADIC_ROOT_OF_UNITY.pow([k]).pow([exp])
};🤖 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 `@circom-prover/src/prover/ark_circom/qap.rs` around lines 64 - 79, The
exponent math can underflow when computing the coset shift (inc) because s -
power - 1 may be negative; add a guarded check before computing snarkjs_omega /
inc: ensure power < s (use the existing s and power locals) and if not return
Err(SynthesisError::PolynomialDegreeTooLarge). Use this check before the block
that computes snarkjs_omega_inv and the inc exponent so you avoid unsigned
underflow when calculating exp for inc and fail early with
SynthesisError::PolynomialDegreeTooLarge.
| if k != 1 { | ||
| // Use manual DFT with snarkjs's roots | ||
| let n_inv = F::from(domain_size as u64).inverse().unwrap(); | ||
|
|
||
| manual_ifft(&mut a, snarkjs_omega_inv, n_inv, domain_size); | ||
| manual_ifft(&mut b, snarkjs_omega_inv, n_inv, domain_size); | ||
|
|
||
| apply_powers(&mut a, inc); | ||
| apply_powers(&mut b, inc); | ||
|
|
||
| manual_fft(&mut a, snarkjs_omega, domain_size); | ||
| manual_fft(&mut b, snarkjs_omega, domain_size); | ||
|
|
There was a problem hiding this comment.
Add an explicit radix-2 precondition before manual FFT path.
manual_fft/bit_reverse assume n is a power of two. In the k != 1 path this is currently implicit; add a hard guard so wrong domains fail fast instead of producing invalid witness polynomials.
Suggested fix
if k != 1 {
+ if !domain_size.is_power_of_two() {
+ return Err(SynthesisError::PolynomialDegreeTooLarge);
+ }
// Use manual DFT with snarkjs's roots
let n_inv = F::from(domain_size as u64).inverse().unwrap();Also applies to: 155-211
🤖 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 `@circom-prover/src/prover/ark_circom/qap.rs` around lines 81 - 93, Add an
explicit radix-2 precondition check before taking the manual FFT path: verify
that domain_size is a power of two when k != 1 and return or panic with a clear
error if not. Modify the branch around the k != 1 handling (the block using
manual_ifft, apply_powers, manual_fft) to call a small predicate (or inline
check) that asserts bit_reverse/manual_fft-safe domain sizes; reference the
symbols domain_size, k, manual_ifft, manual_fft, bit_reverse and ensure the same
guard is added for the other occurrence mentioned (the block covering lines
~155-211) so non-power-of-two domains fail fast instead of producing invalid
polynomials.
| signal x <-- 1; | ||
| out <== in * x; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Circom 2.x, what is the semantic difference between <--and<==for signals, and does<-- add an R1CS constraint?
💡 Result:
In Circom 2.x, the primary semantic difference between the assignment operators <-- and <== lies in whether they generate an R1CS (Rank-1 Constraint System) constraint during compilation [1]. The operator <== performs an assignment to a signal and automatically adds an R1CS constraint asserting that the signal is equal to the provided expression [2][1]. Because R1CS constraints must be quadratic (i.e., they must be expressible as products of two linear combinations), you can only use <== with expressions that satisfy this property [3]. In contrast, the operator <-- performs only an assignment to a signal without adding any R1CS constraint [2][1]. This operation is often referred to as a "hint" or "out-of-circuit computation" [3]. It is useful for computations that are difficult or impossible to represent directly as quadratic constraints (such as bitwise operations, divisions, or ternary logic) [3][1][4]. However, because <-- adds no constraints to the circuit, failing to manually add the corresponding constraints using the === operator will result in an underconstrained circuit, which is a critical security vulnerability [5][1][4]. In summary: - <-- does NOT add an R1CS constraint [2][1]. It is used for witness generation only [1]. - <== DOES add an R1CS constraint [2][1]. It is the safer, preferred method for quadratic assignments [1]. Developers are strongly discouraged from using <-- unless absolutely necessary and are advised to pair it with explicit === constraints to ensure the circuit is correctly constrained [1][4]. Tools such as circom --inspect can help identify assignments using <-- that could potentially be replaced by <== [6][5].
Citations:
- 1: https://docs.circom.io/circom-language/signals/
- 2: https://docs.circom.io/circom-language/constraint-generation/
- 3: https://rareskills.io/post/compute-then-constrain
- 4: https://blog.zksecurity.xyz/posts/circom-pitfalls-1/
- 5: https://github.com/pluto/circom-correctly-constrained
- 6: https://docs.circom.io/circom-language/code-quality/inspect/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
rg -n "multiplexer\.circom" -S . || true
FILE="circom/circuits/multiplexer.circom"
if [ -f "$FILE" ]; then
echo "=== $FILE (context) ==="
nl -ba "$FILE" | sed -n '1,120p'
else
echo "File not found: $FILE"
fiRepository: zkmopro/mopro
Length of output: 1922
Fix underconstrained assignment of x
Line 7 uses <-- (witness-only assignment) so it generates no R1CS constraint tying x to 1; line 8 only constrains out = in * x, leaving x free and allowing arbitrary out values consistent with the equation.
Suggested fix
- signal x <-- 1;
- out <== in * x;
+ var x = 1;
+ out <== in * x;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signal x <-- 1; | |
| out <== in * x; | |
| var x = 1; | |
| out <== in * x; |
🤖 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 `@circom/circuits/multiplexer.circom` around lines 7 - 8, The signal x is
underconstrained because it uses witness-only assignment (`x <-- 1`) so no R1CS
ties x to 1; change that to a constrained assignment (use `x <== 1` or otherwise
add an explicit constraint enforcing x == 1) so the circuit constrains x; keep
the existing multiplication constraint `out <== in * x` intact so out is
correctly tied to in and the now-constrained x.
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "build": "sh scripts/build_circuits.sh" |
There was a problem hiding this comment.
npm test is hardcoded to fail.
Line 6 will always fail local/CI test entrypoints. Replace with a real check (or a no-op success) so the package doesn’t report false failures.
Suggested fix
"scripts": {
- "test": "echo \"Error: no test specified\" && exit 1",
+ "test": "npm run build",
"build": "sh scripts/build_circuits.sh"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "scripts": { | |
| "test": "echo \"Error: no test specified\" && exit 1", | |
| "build": "sh scripts/build_circuits.sh" | |
| "scripts": { | |
| "test": "npm run build", | |
| "build": "sh scripts/build_circuits.sh" |
🤖 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 `@circom/package.json` around lines 5 - 7, The package.json "test" script is
hardcoded to fail; update the "test" entry in package.json (the "test" script
under "scripts") to either run a real test command for this package (e.g.,
invoke your CI test runner or a small verification that build artifacts exist)
or replace the failing command with a no-op success command so npm test exits 0;
ensure the "build" script ("build": "sh scripts/build_circuits.sh") remains
unchanged unless you want the test to call it, and verify npm test in CI/local
passes after the change.
1 participant
Summary by CodeRabbit
New Features
Improvements