fix: config circuit [skip-line-limit]#1526
Merged
Merged
Conversation
0xjei
added
noir
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughUpdated threshold/BFV parameter-bound verification formulas in the configuration circuit, adjusted global search constants for parameter space exploration, and clarified documentation for the r_1j upper-bound calculation formula. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
circuits/bin/config/src/main.nr (2)
343-344:⚠️ Potential issue | 🟡 MinorStale docstring references "80-bit statistical security."
The summary on line 344 still mentions an 80-bit statistical security bound for
e_sm, but the implementation inverify_e_sm_boundnow uses2^60(lambda = 60). Please update the docstring to reflect the new security parameter.📝 Proposed doc fix
-/// - Smudging noise e_sm (large bound for 80-bit statistical security) +/// - Smudging noise e_sm (large bound for 60-bit statistical security)🤖 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 343 - 344, The docstring still claims "80-bit statistical security" for the smudging noise bound e_sm but the code in verify_e_sm_bound/variable e_sm uses lambda = 60 (2^60); update the docstring to reflect the new security parameter (60-bit statistical security / 2^60 bound on e_sm) and any accompanying descriptive text so it matches the implementation (reference verify_e_sm_bound and e_sm).
375-403:⚠️ Potential issue | 🟡 MinorUpdate docstring to match lambda = 60.
The docstring for
verify_e_sm_boundis now inconsistent with the implementation:
- Line 379:
lambda = 80— should belambda = 60.- Line 381:
The 2^80 factor provides 80 bits of statistical security— should reflect2^60and 60-bit statistical security.Since this is the public-facing rationale for a security-critical bound, please ensure the docstring documents the current security level (and ideally why the parameter was reduced, if applicable, so reviewers don't reintroduce 2^80 by mistake).
📝 Proposed doc fix
/// Verifies e_sm_bound (smudging noise bound). /// /// The smudging noise bound is critical for threshold decryption security. /// It's computed as: e_sm_bound = 2^lambda * N_CIPHERTEXTS * (b_fresh + Q_MOD_T) -/// where b_fresh = N * e_norm + b_enc + N * b_e * sk_norm and lambda = 80. +/// where b_fresh = N * e_norm + b_enc + N * b_e * sk_norm and lambda = 60. /// -/// The 2^80 factor provides 80 bits of statistical security, ensuring that +/// The 2^60 factor provides 60 bits of statistical security, ensuring that /// decryption shares don't leak secret key information.🤖 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 375 - 403, The docstring for verify_e_sm_bound incorrectly states lambda = 80 and references 2^80 / 80-bit security while the code uses 2^60; update the docstring to say lambda = 60 and that the factor is 2^60 (60-bit statistical security), and optionally add a brief note explaining why lambda was reduced (or point reviewers to the change) to prevent accidental reversion to 2^80; ensure this change is made in the function comment immediately above fn verify_e_sm_bound().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@circuits/bin/config/src/main.nr`:
- Around line 343-344: The docstring still claims "80-bit statistical security"
for the smudging noise bound e_sm but the code in verify_e_sm_bound/variable
e_sm uses lambda = 60 (2^60); update the docstring to reflect the new security
parameter (60-bit statistical security / 2^60 bound on e_sm) and any
accompanying descriptive text so it matches the implementation (reference
verify_e_sm_bound and e_sm).
- Around line 375-403: The docstring for verify_e_sm_bound incorrectly states
lambda = 80 and references 2^80 / 80-bit security while the code uses 2^60;
update the docstring to say lambda = 60 and that the factor is 2^60 (60-bit
statistical security), and optionally add a brief note explaining why lambda was
reduced (or point reviewers to the change) to prevent accidental reversion to
2^80; ensure this change is made in the function comment immediately above fn
verify_e_sm_bound().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3076d126-c233-4c98-a1b4-e2b79f973221
📒 Files selected for processing (3)
circuits/bin/config/src/main.nrcircuits/lib/src/configs/secure/threshold.nrcrates/zk-helpers/src/circuits/threshold/share_decryption/computation.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.
This PR fixes the config circuit wrong
esmandr1_boundscomputations due to being not aligned with latest changes for the new parameter set (lambda = 60) and other fixes. Note that this does not change anything in terms of parameter generation or configs (just that the circuit was doing the wrong checks).Summary by CodeRabbit
Bug Fixes
Documentation