feat: optimize circuits with u64 modular arithmetic#1552
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces constrained 62-bit modular arithmetic (ModU64) with supporting unconstrained witness helpers, migrates five circuit files from ModU128 to ModU64, optimizes cyclotomic term caching in share decryption, and updates documentation. ChangesModular Arithmetic Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 9
🤖 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 `@circuits/lib/src/configs/default/mod.nr`:
- Around line 7-11: Update the auto-generated header comment that currently
reads "preset: insecure-512" to reflect the secure preset used by the exports
(since this file now pub use super::secure::dkg and pub use
super::secure::threshold); regenerate or edit the top comment produced by
build-circuits.ts so it accurately names the secure preset (e.g., "preset:
secure-512" or the exact secure preset identifier your generator uses) to avoid
confusion when inspecting the module that exports dkg and threshold.
In `@circuits/lib/src/core/dkg/share_computation.nr`:
- Around line 197-198: Add an explicit runtime/assertion at the circuit boundary
that each modulus value used with ModU64 (the q / q_i values consumed by the
centering/parity logic) fits within the 62-bit contract (i.e. q_i < 2^62); if
any q_i violates this, raise a clear error/panic before entering the hot loops.
Place this check before the loops that compute centered using c, half, and q
(the centering/parity code that uses variables c, q, half and produces centered)
so a bad modulus fails loudly rather than silently; apply the same guard for the
other parity-centering site that uses the same assumptions.
In `@circuits/lib/src/core/dkg/share_decryption.nr`:
- Around line 155-159: Add an explicit validation that every modulus in moduli
(the q_l values) is < 2^62 before taking the ModU64 normalization path: in
execute (or at the start of normalize_aggregated) iterate the moduli and
assert/return an error if any q_l >= 2^62 so the code that constructs
ModU64::new(q as u64) and uses reduce_mod/centered is only reached when the
<2^62 invariant holds; reference the normalize_aggregated function, the execute
entry point, the moduli/q_l values, and the ModU64::new/reduce_mod/centered
logic when adding this guard.
In `@circuits/lib/src/core/dkg/share_encryption.nr`:
- Around line 207-220: The code assumes the plaintext modulus t fits in 62 bits
before creating ModU64 (see ModU64::new(t as u64) and later centering logic
using t_half), but Configs::new still accepts any Field; add a validation in
Configs::new that asserts/returns an error if t >= 2^62 (or otherwise document
and coerce safely) so the contract is enforced at config construction time;
update Configs::new to check the provided t value (and any related q_mod_t) and
fail fast with a clear message if it violates the ModU64 domain, then remove the
implicit unchecked cast risk in the code paths that use ModU64, e.g., where
t_mod, t_half, and needs_centering are computed.
In `@circuits/lib/src/core/threshold/decrypted_shares_aggregation.nr`:
- Line 136: Before calling ModU64::new(self.configs.plaintext_modulus as u64)
and the two other ModU64::new(...) sites in decrypted_shares_aggregation.nr (the
calls at the locations flagged), insert an in-circuit check using
assert_max_bit_size::<62>() on the Field/plaintext_modulus value being narrowed
so the value is guaranteed < 2^62; i.e., call assert_max_bit_size::<62>() on the
value (or on self.configs.plaintext_modulus converted to the circuit Field)
immediately before each ModU64::new(...) invocation to prevent silent
truncation.
In `@circuits/lib/src/core/threshold/share_decryption.nr`:
- Around line 314-319: The doc comment for verify_decryption_share_computation
is out of date: the function now takes three parameters (basis_idx, gamma,
cyclo_at_gamma) but the `# Arguments` section documents only two; update the
helper docblock for verify_decryption_share_computation to list all three
arguments and briefly describe `cyclo_at_gamma` (what it represents and any
expected form) so the documentation matches the signature and prevents
confusion.
In `@circuits/lib/src/math/modulo/U64.nr`:
- Around line 68-80: The sub method (ModU64::sub) can return a non-canonical
residue: either require and assert that inputs are reduced (assert lhs < self.m
&& rhs < self.m at entry) or, before calling the unsafe __sub_with_underflow_u64
witness, reduce both operands into the [0, m) range and use those reduced values
for the witness; after computing res_u64, add an assertion assert((res_u64 as
Field) < (self.m as Field)) to guarantee the returned value is reduced, and keep
the existing underflow-verification asserts around the same reduced values.
- Around line 102-105: The div_mod implementation calls unsafe __inv_mod_u64
directly and does not verify that b is invertible, so division by zero (or
non-invertible b modulo m) can silently return 0; change div_mod to reuse the
safe verifier self.inv_mod(b) (or at minimum call self.inv_mod(b) and
propagate/handle its error) and then pass the proven inverse to self.mul_mod(a,
inverse) instead of using __inv_mod_u64 directly so the invertibility is checked
and enforced; reference functions: div_mod, inv_mod, __inv_mod_u64, mul_mod.
In `@circuits/lib/src/math/modulo/unconstrained_U64.nr`:
- Around line 12-17: The helper __reduce_witness_u64 currently narrows the
quotient to u64 which overflows when n >= m * 2^64; change its return type to
u128 and compute the quotient as (n as u128 / m as u128) returning that u128 so
the full floor(n/m) is preserved, and then update any callers of
__reduce_witness_u64 (e.g., reduce_mod/assert_zero_mod paths) to accept a u128
quotient or explicitly convert/check bounds where needed; alternatively, if you
prefer rejection, add a guard that errors when n >= m * 2^64 and keep the u64
return, but the preferred fix is widening the return to u128 in the function
signature and all callsites.
🪄 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: 6390bd59-dcbe-4bde-b173-ecdacd5e2987
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
circuits/lib/src/configs/default/mod.nrcircuits/lib/src/core/dkg/share_computation.nrcircuits/lib/src/core/dkg/share_decryption.nrcircuits/lib/src/core/dkg/share_encryption.nrcircuits/lib/src/core/threshold/decrypted_shares_aggregation.nrcircuits/lib/src/core/threshold/pk_aggregation.nrcircuits/lib/src/core/threshold/share_decryption.nrcircuits/lib/src/lib.nrcircuits/lib/src/math/modulo/U64.nrcircuits/lib/src/math/modulo/mod.nrcircuits/lib/src/math/modulo/unconstrained_U128.nrcircuits/lib/src/math/modulo/unconstrained_U64.nr
zahrajavar
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
circuits/lib/src/core/threshold/pk_aggregation.nr (1)
33-33: 💤 Low valueStale documentation references
ModU128.The docstring on line 33 mentions
ModU128::reduce_mod, but the implementation now usesModU64. Update the doc to match the current implementation.-/// (centered coefficients, `ModU128::reduce_mod` with half-`q` shift); `pk1_agg` equals shared CRS `a`. +/// (centered coefficients, `ModU64::reduce_mod` with half-`q` shift); `pk1_agg` equals shared CRS `a`.🤖 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 `@circuits/lib/src/core/threshold/pk_aggregation.nr` at line 33, Update the stale docstring in pk_aggregation.nr that references ModU128::reduce_mod: change the reference to ModU64::reduce_mod (and any related phrasing about the reduction implementation) so the documentation matches the current implementation where ModU64 is used; ensure the sentence containing "ModU128::reduce_mod" and the surrounding comment about the half-`q` shift and `pk1_agg` is edited to reflect ModU64::reduce_mod.
🤖 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 `@circuits/lib/src/core/threshold/pk_aggregation.nr`:
- Around line 103-104: Before calling ModU64::new(q_l as u64) add an assertion
that q_l fits in 62 bits: call assert_max_bit_size::<62>() on the type/value
used for q_l (same pattern used in decrypted_shares_aggregation.nr) so we
enforce q_l < 2^62 prior to the unsafe cast; ensure the assert is placed
immediately before the ModU64::new(q_l as u64) line and references the same q_l
symbol so the modulus bound is validated at runtime/compile-time before
constructing ModU64.
---
Nitpick comments:
In `@circuits/lib/src/core/threshold/pk_aggregation.nr`:
- Line 33: Update the stale docstring in pk_aggregation.nr that references
ModU128::reduce_mod: change the reference to ModU64::reduce_mod (and any related
phrasing about the reduction implementation) so the documentation matches the
current implementation where ModU64 is used; ensure the sentence containing
"ModU128::reduce_mod" and the surrounding comment about the half-`q` shift and
`pk1_agg` is edited to reflect ModU64::reduce_mod.
🪄 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: 3be8e97a-67b9-4302-9ebd-40c4eb1ce7e2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
circuits/lib/src/core/dkg/share_computation.nrcircuits/lib/src/core/dkg/share_decryption.nrcircuits/lib/src/core/dkg/share_encryption.nrcircuits/lib/src/core/threshold/decrypted_shares_aggregation.nrcircuits/lib/src/core/threshold/pk_aggregation.nrcircuits/lib/src/core/threshold/share_decryption.nrcircuits/lib/src/lib.nrcircuits/lib/src/math/modulo/U64.nrcircuits/lib/src/math/modulo/mod.nrcircuits/lib/src/math/modulo/unconstrained_U128.nrcircuits/lib/src/math/modulo/unconstrained_U64.nr
✅ Files skipped from review due to trivial changes (1)
- circuits/lib/src/lib.nr
0xjei
approved these changes
May 22, 2026
0xjei
left a comment
0xjei
left a comment
Contributor
There was a problem hiding this comment.
good to go, just small coderabbits nits
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.
Replaces ModU128 with a new ModU64 type across all circuits where the modulus fits in 62 bits, and introduces a zero-divisibility shortcut for parity checks. Total reduction: −6.9M constraints (−30.9%) across the DKG phase, −22.2% across all circuits.
Summary by CodeRabbit
New Features
Performance Improvements