feat: add parity matrix to config circuit [skip-line-limit]#1342
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR updates the DKG config verification circuit to add parity matrix verification logic, introduces a public centering function for modular arithmetic, restructures configuration imports, and enhances cross-config consistency checks. The changes also refactor threshold and verification bounds to use updated parameter references. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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: 2
🧹 Nitpick comments (10)
crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs (1)
215-248:test_configs_generation_contains_expecteddoes not assertQ_MOD_T_CENTERED.The new
Q_MOD_T_CENTEREDglobal is emitted by the codegen but not validated by the test. A single additional assertion would catch any future drift betweencomputeand codegen output.🧪 Suggested assertion
assert!(artifacts.configs.contains("SHARE_ENCRYPTION_CONFIGS")); +assert!(artifacts.configs.contains("Q_MOD_T_CENTERED"));🤖 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_encryption/codegen.rs` around lines 215 - 248, Add an assertion in test_configs_generation_contains_expected that the codegen emitted Q_MOD_T_CENTERED; after computing bounds (Bounds::compute(...)) check artifacts.configs contains the formatted constant using the test's prefix and bounds.q_mod_t_centered value, e.g. assert!(artifacts.configs.contains(format!("{}_Q_MOD_T_CENTERED: u32 = {}", prefix, bounds.q_mod_t_centered).as_str())); this ties ShareEncryptionCircuit.codegen output to Bounds::compute.circuits/lib/src/configs/secure/dkg.nr (1)
17-18: Same documentation gap asinsecure/dkg.nr: add a comment explainingQ_MOD_T_CENTERED == Q_MOD_T.
Q mod T = 1082658244788225 < T/2 ≈ 9007199254740992, so the centered value is the same as the raw value. A brief inline comment prevents future maintainers from questioning whether this is a copy-paste bug.✏️ Suggested comment
pub global Q_MOD_T: Field = 1082658244788225; -pub global Q_MOD_T_CENTERED: Field = 1082658244788225; +// Q_MOD_T_CENTERED == Q_MOD_T because Q mod T < T/2 (already in the positive half of the centered range) +pub global Q_MOD_T_CENTERED: Field = 1082658244788225;Based on learnings, documenting centered-range encoding conventions is the established practice in these config files.
🤖 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 short inline comment next to the global constants Q_MOD_T and Q_MOD_T_CENTERED explaining that Q_MOD_T_CENTERED equals Q_MOD_T because Q_MOD_T (1082658244788225) is less than T/2, so the centered representation is identical; update the declarations for Q_MOD_T and Q_MOD_T_CENTERED to include this explanatory note so future maintainers won't treat the duplication as a copy/paste bug.circuits/lib/src/configs/insecure/dkg.nr (1)
17-18: Add a comment clarifying whyQ_MOD_T_CENTERED == Q_MOD_T.Since
Q mod T = 2415755265 < T/2 ≈ 34359701504, the centered representation is identical to the raw value. Without a comment, this looks like an accidental copy-paste. The same applies tosecure/dkg.nr.✏️ Suggested comment
pub global Q_MOD_T: Field = 2415755265; -pub global Q_MOD_T_CENTERED: Field = 2415755265; +// Q_MOD_T_CENTERED == Q_MOD_T because Q mod T < T/2 (already in the positive half of the centered range) +pub global Q_MOD_T_CENTERED: Field = 2415755265;Based on learnings, the convention in this codebase is to document the centered-range encoding in config files when adding new constants of this kind.
🤖 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 short 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 value 2415755265 is less than T/2 so its centered (signed) representation matches the raw value; update the same comment pattern in secure/dkg.nr as well so future readers know this is intentional and not a copy-paste error (refer to symbols Q_MOD_T and Q_MOD_T_CENTERED).crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs (1)
634-643:test_constants_json_roundtripdoesn't assert onq_mod_t_centered.The roundtrip test checks
q_mod_t,moduli,k0is,bits, andbounds, but the newly addedq_mod_t_centeredfield is not verified. Consider adding:assert_eq!(decoded.q_mod_t, constants.q_mod_t); +assert_eq!(decoded.q_mod_t_centered, constants.q_mod_t_centered); assert_eq!(decoded.moduli, constants.moduli);🤖 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_encryption/computation.rs` around lines 634 - 643, The test test_constants_json_roundtrip omits asserting the newly added q_mod_t_centered field; update the test to compare decoded.q_mod_t_centered with constants.q_mod_t_centered (alongside the existing assertions for q_mod_t, moduli, k0is, bits, and bounds) so the JSON roundtrip verifies that field as well.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (1)
142-154: Minor:compute_q_mod_t_centeredrecomputesqinternally.
qis already computed on Line 142, butcompute_q_mod_t_centered(&moduli, t)on Line 144 recomputes it internally. Not a problem since this is config-generation code (not hot-path), but if you ever refactor, a variant that accepts a pre-computedqwould avoid the redundancy.🤖 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 142 - 154, compute_q_mod_t_centered recomputes q even though q is already computed into the local variable q by compute_q_product(&moduli); update the call site to use a variant that accepts the precomputed q (e.g., add and call compute_q_mod_t_centered_with_q(q.clone(), &moduli, t) or similar) or modify compute_q_mod_t_centered to accept Option<&BigInt>/q parameter so it reuses the existing q; change the caller here (where q, q_mod_t, q_mod_t_centered are set) to pass the precomputed q to the new/overloaded function (referencing compute_q_product, q, compute_q_mod_t_centered) so the redundant recomputation is avoided.circuits/benchmarks/scripts/generate_report.sh (2)
211-261: Empty Config tables rendered in insecure mode.The Config section is always emitted (Lines 212-261), but in insecure mode no config JSON files exist, resulting in table headers with no data rows (visible in the insecure report). The comment on Line 211 acknowledges this. Consider conditionally emitting the section only when config data exists, but this is minor since it doesn't affect functionality.
🤖 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 tables are always written even when no "config" JSONs exist; before emitting the Config section headers and running the two for-loops, scan INPUT_DIR for any json file where category_of "$json_file" == "config" (or run a short pre-loop that sets a flag like has_config=true when you find one), and only output the "### Config" / "#### Timing Metrics" and "#### Size & Circuit Metrics" headers and run the detailed loops if that flag is true; use the existing symbols category_of, INPUT_DIR, OUTPUT_FILE and the same json-file checks to keep behavior identical when config files are present.
89-90: Broad glob pattern for config category detection.The pattern
*"config"*would match any circuit path containing "config". This is fine given current naming conventions, but if a future circuit name happens to include "config" (e.g., "threshold/reconfiguration"), it would be mis-categorized.A stricter match like
"config"(exact) or"config/"*could be considered, but this is unlikely to be a problem in practice.🤖 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 branch that categorizes a circuit as "config" uses a broad glob ([[ "$path" == *"config"* ]]) and can misclassify paths that merely contain the substring "config"; update the condition that checks the variable path (the elif block examining "$path") to use a stricter match such as exact equality to "config" or a prefix match like "config/"* (i.e., replace the *"config"* glob with either == "config" or == "config/"*), so only true config directories are categorized as "config".examples/CRISP/circuits/src/utils.nr (1)
19-24: Doc comment is slightly misleading.Line 19 says "The field representation of 1 in centered form", but the value is the centered representation of
q mod t, not necessarily1. It equals1only for certain parameter sets. Consider:-/// * `q_mod_t_centered` - The field representation of 1 in centered form +/// * `q_mod_t_centered` - The centered representation of q mod t🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/circuits/src/utils.nr` around lines 19 - 24, The doc comment for the parameter q_mod_t_centered in check_coefficient_values_with_balance is misleading — it currently says "The field representation of 1 in centered form" but this parameter is the centered field representation of q mod t (not necessarily 1); update the doc comment for q_mod_t_centered to state that it is the centered representation of q mod t (or "the field representation of q mod t in centered form") and clarify it may only equal 1 for certain parameter sets so callers shouldn't assume it's always 1.circuits/benchmarks/results_insecure/report.md (1)
56-66: Empty Config section expected in insecure mode.The Config section with empty tables is consistent with the design that config circuit runs only in secure mode. Based on learnings, benchmark reports should filter or skip incomplete data for circuits running in non-target configurations. The current approach (empty tables) is acceptable but slightly noisy.
🤖 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, Remove or suppress the "Config" section (the "Timing Metrics" and "Size & Circuit Metrics" empty tables) when generating reports for insecure mode; update the report generation logic that emits the "Config" header and those two tables so it checks the run configuration (insecure vs secure) and either skips rendering the section entirely or replaces it with a single concise placeholder line like "Config: not applicable in insecure mode" rather than emitting empty tables, locating the generation points that produce the "Config" header/"Timing Metrics"/"Size & Circuit Metrics" blocks in the report template or renderer and adding the conditional there.circuits/bin/config/src/main.nr (1)
317-318: Consider using the centeredQ_MOD_Tfor a tighter smudging noise bound.Now that
THRESHOLD_Q_MOD_T_CENTEREDis available, this bound formula could potentially use|Q_MOD_T_CENTERED|instead of the rawTHRESHOLD_Q_MOD_Tfor a tighterb_c(and consequentlye_sm_bound). WhenQ_MOD_T > t/2, the centered value's magnitude is smaller, yielding a stricter bound.This may be intentionally deferred — just flagging for consideration.
🤖 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 317 - 318, The current computation of b_c uses the raw THRESHOLD_Q_MOD_T which can be looser; update the formula in the b_c assignment (where b_c is computed from num_ciphertexts, b_fresh, THRESHOLD_Q_MOD_T) to use the magnitude of the centered constant (i.e., |THRESHOLD_Q_MOD_T_CENTERED|) instead of THRESHOLD_Q_MOD_T so the smudging bound (e_sm_bound) is tighter when the centered residue is smaller; ensure THRESHOLD_Q_MOD_T_CENTERED is defined/imported and use its absolute value in the multiplication.
🤖 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/config.json`:
- Line 3: The "config" benchmark is effectively never run under the default
"mode": "insecure" because run_benchmarks.sh guards it; make this behavior
explicit by updating the circuits/benchmarks/config.json entry for "config" to
indicate it is secure-only (e.g., replace the plain "config" string with a
descriptor that includes a modes or secureOnly flag such as
{"name":"config","modes":["secure"]}) and/or add a brief note in the benchmarks
README; reference the "config" entry in circuits/benchmarks/config.json and the
mode check in run_benchmarks.sh so the intent is clear to future readers.
In `@circuits/benchmarks/results_insecure/report.md`:
- Around line 5-6: The report's Git metadata lines currently reference branch
`configs/fixqmt` and commit `689e56cb90251b34e67af87cb7abfed03bedcd1c` which do
not match this PR's branch `configs/parity_matrix`; update the report.md
metadata to reflect the correct branch and commit for this PR by regenerating
the benchmarks (preferred) or manually replacing the `configs/fixqmt` branch and
`689e56cb90251b34e67af87cb7abfed03bedcd1c` commit entries with the current
branch name and commit hash so the report accurately corresponds to this change
set.
---
Nitpick comments:
In `@circuits/benchmarks/results_insecure/report.md`:
- Around line 56-66: Remove or suppress the "Config" section (the "Timing
Metrics" and "Size & Circuit Metrics" empty tables) when generating reports for
insecure mode; update the report generation logic that emits the "Config" header
and those two tables so it checks the run configuration (insecure vs secure) and
either skips rendering the section entirely or replaces it with a single concise
placeholder line like "Config: not applicable in insecure mode" rather than
emitting empty tables, locating the generation points that produce the "Config"
header/"Timing Metrics"/"Size & Circuit Metrics" blocks in the report template
or renderer and adding the conditional there.
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 211-261: The Config tables are always written even when no
"config" JSONs exist; before emitting the Config section headers and running the
two for-loops, scan INPUT_DIR for any json file where category_of "$json_file"
== "config" (or run a short pre-loop that sets a flag like has_config=true when
you find one), and only output the "### Config" / "#### Timing Metrics" and
"#### Size & Circuit Metrics" headers and run the detailed loops if that flag is
true; use the existing symbols category_of, INPUT_DIR, OUTPUT_FILE and the same
json-file checks to keep behavior identical when config files are present.
- Around line 89-90: The current branch that categorizes a circuit as "config"
uses a broad glob ([[ "$path" == *"config"* ]]) and can misclassify paths that
merely contain the substring "config"; update the condition that checks the
variable path (the elif block examining "$path") to use a stricter match such as
exact equality to "config" or a prefix match like "config/"* (i.e., replace the
*"config"* glob with either == "config" or == "config/"*), so only true config
directories are categorized as "config".
In `@circuits/bin/config/src/main.nr`:
- Around line 317-318: The current computation of b_c uses the raw
THRESHOLD_Q_MOD_T which can be looser; update the formula in the b_c assignment
(where b_c is computed from num_ciphertexts, b_fresh, THRESHOLD_Q_MOD_T) to use
the magnitude of the centered constant (i.e., |THRESHOLD_Q_MOD_T_CENTERED|)
instead of THRESHOLD_Q_MOD_T so the smudging bound (e_sm_bound) is tighter when
the centered residue is smaller; ensure THRESHOLD_Q_MOD_T_CENTERED is
defined/imported and use its absolute value in the multiplication.
In `@circuits/lib/src/configs/insecure/dkg.nr`:
- Around line 17-18: Add a short 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 value 2415755265 is less than T/2 so its centered (signed)
representation matches the raw value; update the same comment pattern in
secure/dkg.nr as well so future readers know this is intentional and not a
copy-paste error (refer to symbols Q_MOD_T and Q_MOD_T_CENTERED).
In `@circuits/lib/src/configs/secure/dkg.nr`:
- Around line 17-18: Add a short inline comment next to the global constants
Q_MOD_T and Q_MOD_T_CENTERED explaining that Q_MOD_T_CENTERED equals Q_MOD_T
because Q_MOD_T (1082658244788225) is less than T/2, so the centered
representation is identical; update the declarations for Q_MOD_T and
Q_MOD_T_CENTERED to include this explanatory note so future maintainers won't
treat the duplication as a copy/paste bug.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rs`:
- Around line 215-248: Add an assertion in
test_configs_generation_contains_expected that the codegen emitted
Q_MOD_T_CENTERED; after computing bounds (Bounds::compute(...)) check
artifacts.configs contains the formatted constant using the test's prefix and
bounds.q_mod_t_centered value, e.g.
assert!(artifacts.configs.contains(format!("{}_Q_MOD_T_CENTERED: u32 = {}",
prefix, bounds.q_mod_t_centered).as_str())); this ties
ShareEncryptionCircuit.codegen output to Bounds::compute.
In `@crates/zk-helpers/src/circuits/dkg/share_encryption/computation.rs`:
- Around line 634-643: The test test_constants_json_roundtrip omits asserting
the newly added q_mod_t_centered field; update the test to compare
decoded.q_mod_t_centered with constants.q_mod_t_centered (alongside the existing
assertions for q_mod_t, moduli, k0is, bits, and bounds) so the JSON roundtrip
verifies that field as well.
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs`:
- Around line 142-154: compute_q_mod_t_centered recomputes q even though q is
already computed into the local variable q by compute_q_product(&moduli); update
the call site to use a variant that accepts the precomputed q (e.g., add and
call compute_q_mod_t_centered_with_q(q.clone(), &moduli, t) or similar) or
modify compute_q_mod_t_centered to accept Option<&BigInt>/q parameter so it
reuses the existing q; change the caller here (where q, q_mod_t,
q_mod_t_centered are set) to pass the precomputed q to the new/overloaded
function (referencing compute_q_product, q, compute_q_mod_t_centered) so the
redundant recomputation is avoided.
In `@examples/CRISP/circuits/src/utils.nr`:
- Around line 19-24: The doc comment for the parameter q_mod_t_centered in
check_coefficient_values_with_balance is misleading — it currently says "The
field representation of 1 in centered form" but this parameter is the centered
field representation of q mod t (not necessarily 1); update the doc comment for
q_mod_t_centered to state that it is the centered representation of q mod t (or
"the field representation of q mod t in centered form") and clarify it may only
equal 1 for certain parameter sets so callers shouldn't assume it's always 1.
0xjei
changed the title
0xjei
force-pushed
the
configs/parity_matrix
branch
from
664de42 to
d934e56
Compare
0xjei
approved these changes
Feb 18, 2026
0xjei
force-pushed
the
configs/parity_matrix
branch
from
d934e56 to
16d7a10
Compare
ctrlc03
approved these changes
Feb 18, 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
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.
Adds the parity matrix verification to the config circuit.
Summary by CodeRabbit