fix(integer): do sum by safe chunk sizes #1512
Merged
Conversation
This file contains 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
agnesLeroy
reviewed
Sep 4, 2024
agnesLeroy
reviewed
Sep 4, 2024
tfhe/src/integer/server_key/radix_parallel/scalar_comparison.rs
Outdated
Show resolved
Hide resolved
IceTDrinker
reviewed
Sep 4, 2024
tmontaigu
force-pushed
the
tm/safer-eq-ne
branch
3 times, most recently
from
96d9842 to
1644cad
Compare
IceTDrinker
reviewed
Sep 5, 2024
tmontaigu
force-pushed
the
tm/safer-eq-ne
branch
3 times, most recently
from
9f109de to
3e72c87
Compare
IceTDrinker
reviewed
Sep 11, 2024
Comment on lines
-2090
to
+2091
| uint32_t max_value = total_modulus - 1; | ||
| uint32_t max_value = (total_modulus - 1) / (params.message_modulus - 1); |
There was a problem hiding this comment.
@agnesLeroy this is fine as is for our set of parameters dedicated to the blockchain but ideally we need to take the min with the noise level see the tfhe/src/integer/server_key/mod.rs file for the final min with noise level
IceTDrinker
approved these changes
Sep 11, 2024
|
integer test failure https://github.com/zama-ai/tfhe-rs/actions/runs/10813176643/job/29996656992 |
IceTDrinker
reviewed
Sep 11, 2024
Parameters are made with with assumptions on the number of leveled add/sub/scalar_mul operations are made, so that the noise level before doing a PBS has a correct level and everything is safe, secure and correct. So the lib implementation has to uphold these assumptions in order to keep the error probability failure correct. In the comparisons, at some point we had a vector of ciphertexts with a degree == 1, so we greedily summed them (e.g with 2_2 params we summed them by chunks of 15), while it is correct with regards to the carry and message space it is however less correct with regards to the noise level. Noise wise, doing this huge sum is correct as long as the noise of each ciphertext is independent from the others in the same chunk. While it may generally be the case we are in, its not guaranteed, and since we do not track that information we have to take the safer approach of assuming the worst case: all noise are dependent. So to fix the issue we compute the correct size of sum chunk by also taking into account the max noise level.
tmontaigu
force-pushed
the
tm/safer-eq-ne
branch
from
3e72c87 to
0e1fc50
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Parameters are made with with assumptions on the number of leveled add/sub/scalar_mul operations are made, so that the noise level before doing a PBS has a correct level and everything is safe, secure and correct.
So the lib implementation has to uphold these assumptions in order to keep the error probability failure correct.
In the comparisons, at some point we had a vector of ciphertexts with a degree == 1, so we greedily summed them (e.g with 2_2 params we summed them by chunks of 15), while it is correct with regards to the carry and message space it is however less correct with regards to the noise level.
Noise wise, doing this huge sum is correct as long as the noise of each ciphertext is independent from the others in the same chunk.
While it may generally be the case we are in, its not guaranteed, and since we do not track that information we have to take the safer approach of assuming the worst case: all noise are dependent.
So to fix the issue we compute the correct size of sum chunk by also taking into account the max noise level.