fix: update q_mod_t references to centered [skip-line-limit]#1340
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a "config" benchmark circuit, introduces centered Q_MOD_T constants and a public center() helper across DKG/Threshold code, threads centered q_mod_t through zk-helpers codegen/computation, updates CRISP example to use centered constants, and extends benchmark scripts/reports to handle the new config circuit. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as run_benchmarks.sh
participant ProverGen as generate_prover_toml.sh
participant zk_cli as zk_cli
participant Reporter as generate_report.sh
Runner->>ProverGen: request Prover.toml for circuit "config"
ProverGen-->>Runner: returns "_no_zk_cli" marker (no-witness)
Runner-->>Runner: skip zk_cli invocation for config
Runner->>Reporter: provide collected metrics (including config)
Reporter->>Reporter: render Config section in report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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: 1
🧹 Nitpick comments (2)
circuits/bin/config/src/main.nr (1)
74-87:verify_dkg_q_mod_t_centeredduplicates logic already incompute_q_mod_t.The threshold counterpart (line 197) uses
compute_q_mod_t(THRESHOLD_QIS, t), but the DKG version inlines the same product-of-moduli loop. Consider using the helper here too for consistency:♻️ Use compute_q_mod_t helper
fn verify_dkg_q_mod_t_centered() { let t = DKG_PLAINTEXT_MODULUS; - let m = ModU128::new(t); - - let mut product: Field = 1; - for i in 0..DKG_L { - product = m.mul_mod(product, DKG_QIS[i]); - } + let raw = compute_q_mod_t(DKG_QIS, t); assert( - center(product, t) == DKG_Q_MOD_T_CENTERED, + center(raw, t) == DKG_Q_MOD_T_CENTERED, "DKG Q_MOD_T centered form verification failed", ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/config/src/main.nr` around lines 74 - 87, The function verify_dkg_q_mod_t_centered duplicates the product-of-moduli logic already implemented in compute_q_mod_t; replace the manual loop in verify_dkg_q_mod_t_centered with a call to compute_q_mod_t(DKG_QIS, t) (or the appropriate signature) to compute the product, then call center(product, t) and assert equality with DKG_Q_MOD_T_CENTERED; reference verify_dkg_q_mod_t_centered, compute_q_mod_t, DKG_QIS, DKG_L, center, and DKG_Q_MOD_T_CENTERED when making the change to ensure consistent behavior with the threshold counterpart.circuits/benchmarks/scripts/generate_report.sh (1)
89-90: Broad pattern match for "config" category could produce false positives.The pattern
*"config"*matches any circuit_path containing "config" anywhere (e.g., a hypotheticaldkg/reconfigwould be classified as "config"). The DKG and Threshold patterns are more specific with/dkg/and/threshold/(delimited by slashes). Consider tightening this, e.g., checking for a path that ends withconfigor matches*/config/*/config/*.Suggested tighter match
- elif [[ "$path" == *"config"* ]]; then + elif [[ "$path" == *"/config" || "$path" == *"/config/"* ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 89 - 90, The current broad match in the generate_report.sh branch (elif [[ "$path" == *"config"* ]]) can misclassify paths; tighten the pattern to match config as a path segment (e.g., check for "/config/" or a trailing "/config") by replacing the condition on the variable path in that elif to use patterns like */config/* or */config to only match when "config" is a directory or final segment; update the same elif that references "$path" so subsequent logic still falls through correctly for non-config matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/lib/src/core/dkg/share_encryption.nr`:
- Around line 193-200: The code passes a centered Field value q_mod_t_centered
into t_mod.mul_mod which breaks ModU128::mul_mod; instead pass the non-centered
raw q_mod_t (the value in range [0,t)) into t_mod.mul_mod and only call
center(...) on the mul_mod result. Update the compute_scaled_message logic to
use the raw q_mod_t (not q_mod_t_centered) when calling t_mod.mul_mod, then set
k1_coeffs[i] = center(q_times_m_mod_t, t); keep references to q_mod_t_centered,
t_mod.mul_mod, center, and message.coefficients so the change is localized to
that loop.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 89-90: The current broad match in the generate_report.sh branch
(elif [[ "$path" == *"config"* ]]) can misclassify paths; tighten the pattern to
match config as a path segment (e.g., check for "/config/" or a trailing
"/config") by replacing the condition on the variable path in that elif to use
patterns like */config/* or */config to only match when "config" is a directory
or final segment; update the same elif that references "$path" so subsequent
logic still falls through correctly for non-config matches.
In `@circuits/bin/config/src/main.nr`:
- Around line 74-87: The function verify_dkg_q_mod_t_centered duplicates the
product-of-moduli logic already implemented in compute_q_mod_t; replace the
manual loop in verify_dkg_q_mod_t_centered with a call to
compute_q_mod_t(DKG_QIS, t) (or the appropriate signature) to compute the
product, then call center(product, t) and assert equality with
DKG_Q_MOD_T_CENTERED; reference verify_dkg_q_mod_t_centered, compute_q_mod_t,
DKG_QIS, DKG_L, center, and DKG_Q_MOD_T_CENTERED when making the change to
ensure consistent behavior with the threshold counterpart.
Contributor
Author
|
converting to draft because I was missing this:
since |
0xjei
force-pushed
the
configs/fixqmt
branch
from
ed76df1 to
65bca0d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
circuits/benchmarks/scripts/generate_report.sh (2)
211-261: Config section will render empty table headers in insecure-mode reports.Since
run_benchmarks.shskips the config circuit in insecure mode, there will be no config JSON files in the raw directory. This section will still emit the### Configheading and empty markdown tables (headers only, no data rows) in the insecure report. Consider conditionally emitting this section only when config JSON files are present:♻️ Suggested approach
-# Config (validate_secure_configs) - only present in secure mode -cat >> "$OUTPUT_FILE" << EOF +# Config (validate_secure_configs) - only present in secure mode +HAS_CONFIG=false +for json_file in "$INPUT_DIR"/*.json; do + [ -f "$json_file" ] || continue + [ "$(category_of "$json_file")" = "config" ] || continue + HAS_CONFIG=true + break +done + +if [ "$HAS_CONFIG" = true ]; then +cat >> "$OUTPUT_FILE" << EOF ### Config ... EOF +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 211 - 261, The Config section currently always writes headings/tables to OUTPUT_FILE even when there are no config JSONs; update the script to first detect whether any files in "$INPUT_DIR" match category_of == "config" (e.g., by scanning with the same for-loop logic or a helper like category_of) and only emit the "### Config" block and the two tables when at least one config JSON exists. Concretely, add a quick pre-check (using the existing category_of function and INPUT_DIR glob) to set a flag (e.g., has_config) before writing the headings, then wrap the two cat >> "$OUTPUT_FILE" blocks and the two for-loops that append rows in an if [ "$has_config" = "true" ] conditional so no empty headers are printed when no config JSONs are present.
81-94: Minor inconsistency incategory_ofpattern matching.The dkg and threshold patterns use path-separator anchored globs (
*"/dkg/"*,*"/threshold/"*), but the config check uses a bare substring match (*"config"*). This works today because dkg/threshold are matched first, but a future circuit with "config" in its name under another category could be misclassified.Consider tightening the match to be consistent, e.g.
*"/config"*or an exact check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 81 - 94, The category_of function's config check is too loose; update the config pattern to match a path separator like the others so circuits with "config" in their filename aren't misclassified—modify the branch in category_of that currently matches "*\"config\"*" to use a directory-aware glob such as "*\"/config/\"*" (or another consistent directory-bound pattern) so the jq-derived path variable and the function's conditional logic correctly detect the config directory.circuits/bin/config/src/main.nr (1)
51-67: Consider enforcing the precondition with an assertion.The docstring states
rawmust be in[0, modulus), but this isn't enforced. Ifraw >= modulus, the output would be silently incorrect. Since this is a verification circuit where correctness is paramount, an explicitassertwould be a cheap safeguard.Suggested addition
pub fn center(raw: Field, modulus: Field) -> Field { + assert(raw as u128 < modulus as u128, "center: raw must be in [0, modulus)"); let needs_centering = if (modulus as u128 % 2) == 1 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/config/src/main.nr` around lines 51 - 67, Add an explicit precondition check at the start of the center function to assert that raw is within [0, modulus) (i.e., raw >= 0 and raw < modulus) so invalid inputs cannot silently produce wrong results; update the center function (before computing needs_centering) to perform this assert with a clear message referencing raw and modulus.circuits/benchmarks/results_insecure/report.md (1)
56-66: Empty Config section in insecure report.The Config subsection has table headers but no data rows because the config circuit is skipped in insecure mode. Consider having the report generator either omit the Config section entirely when there's no data, or add a note like "N/A – config circuit runs only in secure mode" for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/results_insecure/report.md` around lines 56 - 66, The Config subsection is empty in the insecure report; update the report generator (e.g., the function that renders the config section such as renderConfigSection or generateInsecureReport) to detect when config data is absent and either omit the entire "Config" heading and tables or emit a single-row note like "N/A – config circuit runs only in secure mode" under the Timing Metrics and Size & Circuit Metrics tables; implement this conditional branch where the report builds the "Config" subsection so the insecure report no longer shows empty table headers.
🤖 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/benchmarks/results_insecure/report.md`:
- Around line 56-66: The Config subsection is empty in the insecure report;
update the report generator (e.g., the function that renders the config section
such as renderConfigSection or generateInsecureReport) to detect when config
data is absent and either omit the entire "Config" heading and tables or emit a
single-row note like "N/A – config circuit runs only in secure mode" under the
Timing Metrics and Size & Circuit Metrics tables; implement this conditional
branch where the report builds the "Config" subsection so the insecure report no
longer shows empty table headers.
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 211-261: The Config section currently always writes
headings/tables to OUTPUT_FILE even when there are no config JSONs; update the
script to first detect whether any files in "$INPUT_DIR" match category_of ==
"config" (e.g., by scanning with the same for-loop logic or a helper like
category_of) and only emit the "### Config" block and the two tables when at
least one config JSON exists. Concretely, add a quick pre-check (using the
existing category_of function and INPUT_DIR glob) to set a flag (e.g.,
has_config) before writing the headings, then wrap the two cat >> "$OUTPUT_FILE"
blocks and the two for-loops that append rows in an if [ "$has_config" = "true"
] conditional so no empty headers are printed when no config JSONs are present.
- Around line 81-94: The category_of function's config check is too loose;
update the config pattern to match a path separator like the others so circuits
with "config" in their filename aren't misclassified—modify the branch in
category_of that currently matches "*\"config\"*" to use a directory-aware glob
such as "*\"/config/\"*" (or another consistent directory-bound pattern) so the
jq-derived path variable and the function's conditional logic correctly detect
the config directory.
In `@circuits/bin/config/src/main.nr`:
- Around line 51-67: Add an explicit precondition check at the start of the
center function to assert that raw is within [0, modulus) (i.e., raw >= 0 and
raw < modulus) so invalid inputs cannot silently produce wrong results; update
the center function (before computing needs_centering) to perform this assert
with a clear message referencing raw and modulus.
Contributor
Author
|
@coderabbitai review |
zahrajavar
approved these changes
Feb 17, 2026
0xjei
force-pushed
the
configs/fixqmt
branch
from
65bca0d to
5b4ba05
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
circuits/benchmarks/scripts/generate_report.sh (1)
89-90: Consider anchoring theconfigpattern with a directory separator for consistency.The
dkgandthresholdbranches both use/dkg/and/threshold/(with surrounding slashes), making them path-component-anchored. The new branch uses*"config"*, which is a broader glob. While harmless today (dkg/threshold checks run first), aligning the pattern is more defensive.♻️ Proposed alignment
- elif [[ "$path" == *"config"* ]]; then + elif [[ "$path" == *"/config"* ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 89 - 90, Change the loose glob match for config to a path-component-anchored pattern: replace the conditional elif [[ "$path" == *"config"* ]] with elif [[ "$path" == *"/config/"* ]] (matching the existing style used for "/dkg/" and "/threshold/") so the branch only triggers for a config directory component.circuits/lib/src/configs/insecure/dkg.nr (1)
17-18: Optional: document whyQ_MOD_T_CENTERED == Q_MOD_TSince
Q_MOD_T = 2415755265 < t/2 ≈ 34,359,701,504, centering is a no-op for this preset. A brief inline comment (e.g.,// already in [0, t/2), centering is a no-op) would make the equality self-explanatory to future readers without requiring them to compute it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/configs/insecure/dkg.nr` around lines 17 - 18, Add a brief inline comment next to the Q_MOD_T and Q_MOD_T_CENTERED declarations explaining why they are equal (e.g., that Q_MOD_T = 2415755265 is already < t/2 so centering is a no-op), referencing the symbols Q_MOD_T and Q_MOD_T_CENTERED so readers understand the equality without computing t/2 themselves; place the comment on the same line or immediately above the declarations for clarity.circuits/lib/src/configs/secure/dkg.nr (1)
17-18: Optional: document whyQ_MOD_T_CENTERED == Q_MOD_TSame pattern as the insecure preset:
Q_MOD_T = 1082658244788225 < t/2 ≈ 9,007,199,254,740,992, so centering is a no-op. A brief inline comment would save readers the mental arithmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/configs/secure/dkg.nr` around lines 17 - 18, Add a brief inline comment next to the Q_MOD_T and Q_MOD_T_CENTERED declarations explaining that Q_MOD_T_CENTERED equals Q_MOD_T because the constant 1082658244788225 is less than t/2 (so centering is a no-op); reference the symbols Q_MOD_T and Q_MOD_T_CENTERED and state the inequality (1082658244788225 < t/2 ≈ 9,007,199,254,740,992) to make the rationale explicit for readers.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (2)
366-372: Recommend: assert onq_mod_tandq_mod_t_centeredintest_configs_computeThe test only checks
q_inverse_mod_t > 0, leaving the two new fields unverified. For theInsecureThreshold512preset (t=68719403009 per insecure configs), both values can be checked against known-good constants.✅ Suggested additions
assert_eq!(configs.moduli.len(), configs.l); assert!(configs.q_inverse_mod_t > 0); + // q_mod_t is always non-negative + assert!(!configs.q_mod_t.is_zero()); + // centered value must be within (-t/2, t/2] + let t = BigInt::from(configs.plaintext_modulus); + let t_half = t.clone() / 2; + assert!(configs.q_mod_t_centered >= -t_half.clone()); + assert!(configs.q_mod_t_centered <= t_half);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs` around lines 366 - 372, The test test_configs_compute currently only asserts q_inverse_mod_t; update it to also assert the new fields q_mod_t and q_mod_t_centered for the BfvPreset::InsecureThreshold512 case by computing the expected constants (using the known t for InsecureThreshold512) and comparing them against configs.q_mod_t and configs.q_mod_t_centered after calling Configs::compute; keep the existing q_inverse_mod_t check and add two assert_eq! lines referencing configs.q_mod_t and configs.q_mod_t_centered to validate the new fields.
143-144: Minor: redundantq_productcomputation
qis already computed at line 142 viautils::compute_q_product(&moduli).compute_q_mod_t_centeredat line 144 recomputesqinternally (it callscompute_q_productagain). Consider derivingq_mod_t_centereddirectly from the already-computedq_mod_tandtto avoid the redundant multiplication:♻️ Suggested refactor
- let q_mod_t = compute_q_mod_t(&q, t); - let q_mod_t_centered = compute_q_mod_t_centered(&moduli, t); + let q_mod_t = compute_q_mod_t(&q, t); + let t_bn = BigInt::from(t); + let q_mod_t_centered = crate::center(&BigInt::from(q_mod_t.clone()), &t_bn);(Requires
centerto be re-exported fromcrate. Alternatively, acompute_q_mod_t_centered_from_q(q: &BigUint, t: u64) -> BigInthelper would eliminate the double product.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs` around lines 143 - 144, The code recomputes q inside compute_q_mod_t_centered even though q was already computed as q (via utils::compute_q_product(&moduli)) and q_mod_t is available; change the call to derive the centered value from the existing q_mod_t and t instead of recomputing q: either add a helper compute_q_mod_t_centered_from_q(q: &BigUint, t: u64) -> BigInt and call it here, or re-export center and compute the centered value from q_mod_t and t directly; update the call site replacing compute_q_mod_t_centered(&moduli, t) with the new helper or inline derivation to avoid duplicate compute_q_product work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 211-261: The script unconditionally emits the "Config" section
headers and tables even when no config JSONs exist; before printing the "###
Config" header and each subsequent table header (and likewise before the
detailed "config" iteration inside the for category in dkg threshold config
loop), first check whether any file in $INPUT_DIR yields category_of
"$json_file" = "config" (e.g., scan the glob for a matching file and set a flag
or break early), and only write the headers and run the corresponding for-loop
body (the blocks using category_of, format_time, format_bytes, format_gates, and
echo >> "$OUTPUT_FILE") if that flag indicates at least one config JSON exists;
apply the same presence-guarding approach to the detail loop's "config"
iteration so empty tables/headings are never emitted.
---
Duplicate comments:
In `@circuits/benchmarks/results_insecure/report.md`:
- Around line 56-66: The script generate_report.sh currently emits the "###
Config" heading and two empty tables unconditionally; update generate_report.sh
to only print the "### Config" heading and the Timing Metrics / Size & Circuit
Metrics tables when the corresponding metrics data exists (e.g., when arrays or
variables holding compile/execute/prove/verify and size metrics are non-empty or
when MODE indicates secure report); guard the printing logic (the block that
writes the tables and the standalone "### Config" in the Circuit Details
section) behind a conditional check so the tables and heading are skipped for
the insecure report mode.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 89-90: Change the loose glob match for config to a
path-component-anchored pattern: replace the conditional elif [[ "$path" ==
*"config"* ]] with elif [[ "$path" == *"/config/"* ]] (matching the existing
style used for "/dkg/" and "/threshold/") so the branch only triggers for a
config directory component.
In `@circuits/lib/src/configs/insecure/dkg.nr`:
- Around line 17-18: Add a brief inline comment next to the Q_MOD_T and
Q_MOD_T_CENTERED declarations explaining why they are equal (e.g., that Q_MOD_T
= 2415755265 is already < t/2 so centering is a no-op), referencing the symbols
Q_MOD_T and Q_MOD_T_CENTERED so readers understand the equality without
computing t/2 themselves; place the comment on the same line or immediately
above the declarations for clarity.
In `@circuits/lib/src/configs/secure/dkg.nr`:
- Around line 17-18: Add a brief inline comment next to the Q_MOD_T and
Q_MOD_T_CENTERED declarations explaining that Q_MOD_T_CENTERED equals Q_MOD_T
because the constant 1082658244788225 is less than t/2 (so centering is a
no-op); reference the symbols Q_MOD_T and Q_MOD_T_CENTERED and state the
inequality (1082658244788225 < t/2 ≈ 9,007,199,254,740,992) to make the
rationale explicit for readers.
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs`:
- Around line 366-372: The test test_configs_compute currently only asserts
q_inverse_mod_t; update it to also assert the new fields q_mod_t and
q_mod_t_centered for the BfvPreset::InsecureThreshold512 case by computing the
expected constants (using the known t for InsecureThreshold512) and comparing
them against configs.q_mod_t and configs.q_mod_t_centered after calling
Configs::compute; keep the existing q_inverse_mod_t check and add two assert_eq!
lines referencing configs.q_mod_t and configs.q_mod_t_centered to validate the
new fields.
- Around line 143-144: The code recomputes q inside compute_q_mod_t_centered
even though q was already computed as q (via utils::compute_q_product(&moduli))
and q_mod_t is available; change the call to derive the centered value from the
existing q_mod_t and t instead of recomputing q: either add a helper
compute_q_mod_t_centered_from_q(q: &BigUint, t: u64) -> BigInt and call it here,
or re-export center and compute the centered value from q_mod_t and t directly;
update the call site replacing compute_q_mod_t_centered(&moduli, t) with the new
helper or inline derivation to avoid duplicate compute_q_product work.
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
3 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.
q_mod_ttoq_mod_t_centeredbin/configcircuit the centered form.bin/configto benchmarks when--mode secureSummary by CodeRabbit
New Features
Chores