-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parameters validation for the FV scheme #3870
Conversation
len(params.coeff_modulus) > COEFF_MOD_COUNT_MAX | ||
or len(params.coeff_modulus) < COEFF_MOD_COUNT_MIN | ||
): | ||
raise RuntimeError("Invalid coefficient modulus counts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you specify in the error message the range of the coeffecients allowed and how many was passed?
if params.coeff_modulus[i] >> COEFF_MOD_BIT_COUNT_MAX or not ( | ||
params.coeff_modulus[i] >> (COEFF_MOD_BIT_COUNT_MIN - 1) | ||
): | ||
raise RuntimeError("Invalid coefficient modulus values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
# Compute the product of all coeff moduli | ||
total_coeff_modulus = reduce(lambda x, y: x * y, params.coeff_modulus) | ||
# total_coeff_modulus_bit_count = get_significant_count(total_coeff_modulus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also verify that the total_coeff_modulus is good for our n-bit security level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the code required for the security level checking. but It is currently commented with TODO because I would need to work on how to ask for the security level and if passed then it can be checked otherwise scheme will work without standard security.
|
||
# Check for the range of coefficient modulus primes. | ||
for i in range(len(params.coeff_modulus)): | ||
if params.coeff_modulus[i] >> COEFF_MOD_BIT_COUNT_MAX or not ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
params.coeff_modulus[i] > 2**COEFF_MOD_BIT_COUNT_MAX
Is clearer? I am always nervous about operator precedence. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, bitwise operators will be much better for performance than exponentiation.
# Check all coeff moduli are relatively prime to plain_modulus | ||
for i in range(len(params.coeff_modulus)): | ||
for j in range(i): | ||
if math.gcd(params.coeff_modulus[i], params.plain_modulus) > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the j loop here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By mistake, I copied the above loop and forget to remove the j loop😅
Codecov Report
@@ Coverage Diff @@
## master #3870 +/- ##
==========================================
- Coverage 95.07% 94.90% -0.18%
==========================================
Files 186 199 +13
Lines 18868 20158 +1290
==========================================
+ Hits 17939 19131 +1192
- Misses 929 1027 +98
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
): | ||
raise RuntimeError( | ||
f"Invalid coefficient modulus count {len(params.coeff_modulus)}, " | ||
+ "should be in range [1, 62]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use the variables directly here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[1, 62], in case those values change
): | ||
raise RuntimeError( | ||
f"Invalid coefficient modulus values {params.coeff_modulus[i]}, " | ||
+ "should be in smaller than 60 bit number." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Description
closes #3792
Checks basic properties that the parameters should have while setting up, and tests the generated prime nos if those are correct.
Affected Dependencies
None
How has this been tested?
This code does not need to have dedicated tests and it is verified by already present tests.
Checklist