fix: bit width inconsistencies in circuits#1504
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce a new generic parameter Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant C4 as DKG Circuit (C4)
participant Agg as Aggregation Helper
participant C6 as Threshold Circuit (C6)
participant Val as Validator
Test->>C4: Generate & prove C4 with DKG shares
C4-->>Test: Commitment output
Test->>Agg: Aggregate DKG decrypted shares → CRT polynomial
Agg->>Agg: Convert decrypted shares to BigInt
Agg->>Agg: Reduce & pack into CRT form
Agg-->>Test: Aggregated CRT polynomial + agg_sk
Test->>C6: Generate & prove C6 with aligned SK
C6-->>Test: expected_sk_commitment output
Test->>Val: Recompute C4 commitment from agg_sk
Val-->>Test: Computed commitment
Test->>Val: Assert C4 commitment == C6 expected_sk_commitment
Val-->>Test: Validation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 (1)
circuits/lib/src/configs/secure/threshold.nr (1)
32857-32858: Consider centralizing the shared52bit-width constant.
USER_DATA_ENCRYPTION_BIT_PKandUSER_DATA_ENCRYPTION_BIT_CTnow share the same literal value (and matchPK_AGGREGATION_BIT_PK). A single source-of-truth constant here would reduce future drift risk.♻️ Small refactor proposal
+pub global THRESHOLD_COMMITMENT_BIT_WIDTH: u32 = 52; -pub global PK_AGGREGATION_BIT_PK: u32 = 52; +pub global PK_AGGREGATION_BIT_PK: u32 = THRESHOLD_COMMITMENT_BIT_WIDTH; -pub global USER_DATA_ENCRYPTION_BIT_PK: u32 = 52; -pub global USER_DATA_ENCRYPTION_BIT_CT: u32 = 52; +pub global USER_DATA_ENCRYPTION_BIT_PK: u32 = THRESHOLD_COMMITMENT_BIT_WIDTH; +pub global USER_DATA_ENCRYPTION_BIT_CT: u32 = THRESHOLD_COMMITMENT_BIT_WIDTH;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/configs/secure/threshold.nr` around lines 32857 - 32858, Both USER_DATA_ENCRYPTION_BIT_PK and USER_DATA_ENCRYPTION_BIT_CT are hardcoded to 52; create a single shared constant (e.g., USER_DATA_ENCRYPTION_BITS: u32 = 52) and replace references to USER_DATA_ENCRYPTION_BIT_PK and USER_DATA_ENCRYPTION_BIT_CT with that constant to avoid drift; also check and consolidate any other matching literals such as PK_AGGREGATION_BIT_PK to reuse the same new constant and update all use-sites (declarations and any modules that import these symbols) accordingly.
🤖 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/polynomial/src/crt_polynomial.rs`:
- Around line 265-268: In bigint_to_u64_mod, guard against a zero modulus before
performing the modulo: check if m == 0 at the start of the function and
immediately return an Err(fhe_math::Error) appropriate for a zero-divisor
instead of computing c % bm; this prevents the panic ("attempt to divide by
zero") when creating bm/using % and keeps the Result contract intact—place the
check before BigInt::from(m) and the r = c % &bm calculation in
bigint_to_u64_mod.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 942-958: Add a direct cross-circuit equality check between the C4
and C6 commitments: after computing c4_big (via
compute_aggregated_shares_commitment) and c6_big (the C6 commitment) and after
the existing ThresholdShareDecryptionCircuit::compute/c6_out assertions, insert
an assert that c4_big == c6_big with a descriptive message so we fail if C4 and
C6 use different commitment formulas or layouts; reference c4_big, c6_big,
compute_aggregated_shares_commitment, ThresholdShareDecryptionCircuit::compute,
c6_out.inputs.expected_sk_commitment, dkg_out.bits.agg_bit and
c6_out.bits.sk_bit to locate the correct spot.
---
Nitpick comments:
In `@circuits/lib/src/configs/secure/threshold.nr`:
- Around line 32857-32858: Both USER_DATA_ENCRYPTION_BIT_PK and
USER_DATA_ENCRYPTION_BIT_CT are hardcoded to 52; create a single shared constant
(e.g., USER_DATA_ENCRYPTION_BITS: u32 = 52) and replace references to
USER_DATA_ENCRYPTION_BIT_PK and USER_DATA_ENCRYPTION_BIT_CT with that constant
to avoid drift; also check and consolidate any other matching literals such as
PK_AGGREGATION_BIT_PK to reuse the same new constant and update all use-sites
(declarations and any modules that import these symbols) accordingly.
🪄 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: 31062749-892b-410e-9158-912c4c4a429f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
circuits/bin/dkg/share_decryption/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/insecure/threshold.nrcircuits/lib/src/configs/secure/dkg.nrcircuits/lib/src/configs/secure/threshold.nrcircuits/lib/src/core/dkg/share_decryption.nrcrates/polynomial/src/crt_polynomial.rscrates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/tests/local_e2e_tests.rs
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.
Aligns Noir DKG/threshold commitment bit widths with
e3-zk-helpersso C1/C5 (pk generation vs pk aggregation) and C2/C4/C6 (share encryption vs per-share vs aggregate share decryption) hash the same coefficient ranges the prover expects. Fixes the secure preset where share-decryption msg bits disagreed with share encryption, and threads explicit per-share vs aggregate parameters through share decryption as needed.Adds
CrtPolynomial::to_fhe_polynomialone3-polynomial(inverse offrom_fhe_polynomial) and updateslocal_e2e_teststo use it instead of duplicating CRT→fhe_math::Polylogic; drops redundant test-only dependencies.Testing
cargo test -p e3-polynomialcargo test -p e3-zk-prover --test local_e2e_tests(withbband built circuit artifacts)Summary by CodeRabbit
Chores
Tests