Tm/noise asserts #1782

tmontaigu · 2024-11-14T11:08:23Z

This adds the noise-asserts feature, which will make
PBS functions do a noise level check.

This also adds an extra MaxNoiseLevel parameter
to Ciphertext::set_noise_level that is used when the noise-asserts
feature is on, to check that the given new-noise level does not
exceed the given MaxNoiseLevel. In case of problems, the code will panic

By default these checks will also be make in cfg(test)

Closes zama-ai/tfhe-rs-internal#313

mayeul-zama

Instead of asserting on max noise level, I seems to me that it would be better to do operations differently if the noise_level is going to be too big (as is done for the degree).

This would mean we could have more confidence in noise validity (not just in tests or when activating a feature) without the risk of panics.

I guess this would need more work
That being said, this PR is an improvement over the current state

mayeul-zama · 2024-11-15T10:37:51Z

tfhe/src/shortint/ciphertext/standard.rs

@@ -138,7 +138,13 @@ impl Ciphertext {
        self.noise_level
    }

-    pub fn set_noise_level(&mut self, noise_level: NoiseLevel) {
+    #[cfg_attr(any(feature = "noise-asserts", test), track_caller)]


Could be a simple #[track_caller] to avoid some complexity with features

Yep went a bit too far here 😁

mayeul-zama · 2024-11-15T10:41:00Z

tfhe/src/shortint/server_key/mod.rs

@@ -913,6 +922,14 @@ impl ServerKey {
        ct: &Ciphertext,
        acc: &ManyLookupTableOwned,
    ) -> Vec<Ciphertext> {
+        if cfg!(any(feature = "noise-asserts", test)) && ct.noise_level() != NoiseLevel::UNKNOWN {


NoiseLevel::UNKNOWN and NoiseLevel::MAX have the same value which means a NoiseLevel::MAX would not be checked here
It seems to me that it would be better to panic if we have NoiseLevel::MAX here

I think it would be better to make the values of unknown and max different, because I do need the distinction because of some things done in the casting keys

The idea is that if the noise level is unknown we may allow to do the PBS, but not if'ts at MAX

In the casting keys, at some point, there's a ciphertext that is set with noise_level unknown and we do a PBS on it, so either we allow to do PBS on NoiseLevel::UNKNOWN noise level, or We change the thing in the casting key to not be at NoiseLevel::UNKNOWN

so here to note that the MAX is usize::MAX i.e. an impossible value to reach, UNKNOWN was set to MAX as a safety

maybe we need to think about using

MAX = usize::MAX - 1
UNKNOWN = usize::MAX

and see if that could make sense in the general case

given that a noise of usize::MAX - 1

would break anything

also maybe time to change the noise level stuff to use u32 or u64 instead of usize, given that those don't refer to memory

not necessarily in this PR but it begs the question

mayeul-zama · 2024-11-15T10:50:10Z

tfhe/src/integer/server_key/radix_parallel/scalar_sub.rs

-            lhs_b.set_noise_level(lhs_b.noise_level() + borrow.noise_level());
+            lhs_b.set_noise_level(
+                lhs_b.noise_level() + borrow.noise_level(),
+                self.key.max_noise_level,


ServerKey.max_degree is initialized with MaxDegree::integer_radix_server_key in radix integers
This keeps space for carry propagation in all operations (except in carry propagation I guess, not sure if there are degree checks there)

I think we should do something similar for noise level
Or are noise checks in carry propagation enough? If so, I guess it would mean some operations could panic in carry propagation even if we could have prevented it in the previous operation (adding a PBS if the max_noise_level_keep_space_for_carry_propagation is too big)

In practice not sure this is a problem as noise and degree checks are often redundant.

While I do think max_degree and max_noise_level should be aligned I don't know if its better to align max_degree to max_noise_level or the reverse

The only use of the ServerKey.max_degree having space for 1 carry addition is for the carry_propagation algorithm if it's the sequential one, not parallel.

All other algorithm don't really rely on this

max_noise_level is separate from the max degree and depends on the optimization of parameters

I think it's the question about "what do the parameters allow ?" i.e. the max_max_degree and the max_max_noise_level

and

"what are my constraints for these integers" which are the max_degree and max_noise_level

dont know if this is the right PR for this change (having both notions)

maybe NoiseLevel::MAX makes no sense to be honest

tmontaigu · 2024-11-15T11:41:53Z

Instead of asserting on max noise level, I seems to me that it would be better to do operations differently if the noise_level is going to be too big (as is done for the degree).

It's not the goal here, the goal is to have some checks that enable us in test to know when we, by mistake crossed a limit on the noise level. And since those are at shortint level, its already too late to do the things differently (like you add two shortint blocks, but the sum exceeds the noise level, its too late, you don't have another way to do the addition)

Later at integer level we will add pre-checks on noise level to pre-clean if needed

tfhe/src/shortint/ciphertext/common.rs

tfhe/src/shortint/key_switching_key/mod.rs

IceTDrinker · 2024-11-15T13:41:14Z

tfhe/src/shortint/list_compression/compression.rs

-                    unchecked_scalar_mul_assign(&mut ct, message_modulus.0 as u8);
+                    let max_noise_level =
+                        MaxNoiseLevel::new((ct.noise_level() * message_modulus.0).get());


this noise comes from "nowhere" right, we have no key for a reference max noise ?

@mayeul-zama the key has no information of the multiplication required to compress ?

The compression parameters take into account this shift of the message (which is clean, see assert NoiseLevel::NOMINAL above) to the carry bits.
In my understanding, it means that, in this compression pattern contract, the multiplication required to compress is by message_modulus.

@fakub could you confirm?

What I mean is, does it have the constant to multiply with in the key ?

It's message_modulus which is not stored in CompressionKey

well that's not good at all, means keys can be easily misused

When misused, it would most likely fail a few lines above on

assert_eq!( lwe_size, ct.ct.lwe_size(), "All ciphertexts do not have the same lwe size as the packing keyswitch key" );

But indeed it would be safer also check against the CompressionKey expected message_modulus

Not sure I followed everything, is there something to fix in this PR ?

not for you but keys are missing an information as far as I can tell

agnesLeroy · 2024-11-15T13:43:43Z

Is this something we should also port to the GPU backend you think?

IceTDrinker · 2024-11-15T13:45:00Z

Is this something we should also port to the GPU backend you think?

Can be, depending on how you track the noise in your implem, this way during tests you can catch when noise gets too high, but I don't know how you manage the degree/noise data in CUDA :)

tmontaigu · 2024-11-19T13:15:08Z

note that switching from usize -> u64 is a API break (small), and a potential serialization break depending on the serialization format (bincode should be fine)

IceTDrinker · 2024-11-19T13:16:33Z

note that switching from usize -> u64 is a API (small), and a potential serialization break depending on the serialization format (bincode should be fine)

if needed tfhe-versionable should help manage that

tfhe/src/c_api/shortint/parameters.rs

tfhe/src/integer/server_key/mod.rs

tfhe/src/shortint/server_key/bivariate_pbs.rs

IceTDrinker

I see lots of small things that could be changed to make everything consistent, do we say it's for another PR ?

tmontaigu · 2024-11-20T12:41:00Z

I'm already working on making the Degree,MaxDegree,MessageModulus,CarryModulus use u64, I can add the commit to this PR once its done

IceTDrinker · 2024-11-20T12:42:49Z

I'm already working on make the Degree,MaxDegree,MessageModulus,CarryModulus use u64, I can add the commit to this PR once its done

fine by me

IceTDrinker

Thanks a lot !

IceTDrinker · 2024-11-22T11:33:41Z

Some integer test broke with the noise check

This adds the noise-asserts feature, which will make PBS functions do a noise level check. This also adds an extra MaxNoiseLevel parameter to Ciphertext::set_noise_level that is used when the noise-asserts feature is on, to check that the given new-noise level does not exceed the given MaxNoiseLevel. In case of problems, the code will panic By default these checks will also be make in cfg(test)

Change from usize to u64 for MaxNoiseLevel and NoiseLevel This is an API break as `new` and `get` handle/returns u64 instead of usize This is also a potential serialization break depending on the serializer used (bincode should be fine as it serializes usize as u64)

This switches from usize to u64 for shortint's metdata: * Degree * MaxDegree * CarryModulus * MessageModulus The reasoning is that usize should be preferred when the value is used as some kind of index, memory access, etc, and not numbers like these metadata are. This is a breaking API change This is also a somewhat breaking serialization change depending on the serialization format (bincode should be ok as it encodes usize as u64)

IceTDrinker · 2024-11-26T13:23:11Z

Looking good

cla-bot bot added the cla-signed label Nov 14, 2024

tmontaigu force-pushed the tm/noise-asserts branch 2 times, most recently from 87ac779 to ff11669 Compare November 14, 2024 17:18

tmontaigu requested review from IceTDrinker and mayeul-zama November 14, 2024 17:21

tmontaigu marked this pull request as ready for review November 14, 2024 18:06

mayeul-zama reviewed Nov 15, 2024

View reviewed changes

IceTDrinker reviewed Nov 15, 2024

View reviewed changes

tfhe/src/shortint/ciphertext/common.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Nov 15, 2024

View reviewed changes

tfhe/src/shortint/key_switching_key/mod.rs Show resolved Hide resolved

IceTDrinker reviewed Nov 15, 2024

View reviewed changes

tmontaigu force-pushed the tm/noise-asserts branch from ff11669 to 12d0a5b Compare November 19, 2024 13:13

tmontaigu force-pushed the tm/noise-asserts branch from 12d0a5b to a305a7c Compare November 20, 2024 10:19

tmontaigu requested a review from IceTDrinker November 20, 2024 11:32

IceTDrinker reviewed Nov 20, 2024

View reviewed changes

tfhe/src/c_api/shortint/parameters.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Nov 20, 2024

View reviewed changes

tfhe/src/integer/server_key/mod.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Nov 20, 2024

View reviewed changes

tfhe/src/shortint/server_key/bivariate_pbs.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Nov 20, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

tmontaigu force-pushed the tm/noise-asserts branch 2 times, most recently from ecc92d8 to 0fbe2f1 Compare November 21, 2024 17:27

tmontaigu requested a review from IceTDrinker November 22, 2024 09:28

IceTDrinker approved these changes Nov 22, 2024

View reviewed changes

zama-bot added the approved label Nov 22, 2024

tmontaigu force-pushed the tm/noise-asserts branch from 0fbe2f1 to 38dbd31 Compare November 22, 2024 12:04

zama-bot removed the approved label Nov 22, 2024

tmontaigu force-pushed the tm/noise-asserts branch 2 times, most recently from 7ad5f9a to c74d53a Compare November 22, 2024 16:42

tmontaigu added the approved label Nov 25, 2024

tmontaigu added 3 commits November 25, 2024 19:16

tmontaigu force-pushed the tm/noise-asserts branch from c74d53a to 2ffe4e8 Compare November 25, 2024 18:17

zama-bot removed the approved label Nov 25, 2024

tmontaigu added the approved label Nov 26, 2024

tmontaigu merged commit ade9a66 into main Nov 26, 2024
106 checks passed

tmontaigu deleted the tm/noise-asserts branch November 26, 2024 13:57

Tm/noise asserts #1782

Tm/noise asserts #1782

Conversation

tmontaigu commented Nov 14, 2024 • edited Loading

mayeul-zama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmontaigu Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmontaigu Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

tmontaigu Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmontaigu commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnesLeroy commented Nov 15, 2024

IceTDrinker commented Nov 15, 2024

tmontaigu commented Nov 19, 2024 • edited Loading

IceTDrinker commented Nov 19, 2024

IceTDrinker left a comment

Choose a reason for hiding this comment

tmontaigu commented Nov 20, 2024 • edited Loading

IceTDrinker commented Nov 20, 2024

This comment was marked as outdated.

IceTDrinker left a comment

Choose a reason for hiding this comment

IceTDrinker commented Nov 22, 2024

IceTDrinker commented Nov 26, 2024

tmontaigu commented Nov 14, 2024 •

edited

Loading

tmontaigu Nov 15, 2024 •

edited

Loading

tmontaigu Nov 15, 2024 •

edited

Loading

tmontaigu Nov 15, 2024 •

edited

Loading

tmontaigu commented Nov 15, 2024 •

edited

Loading

tmontaigu commented Nov 19, 2024 •

edited

Loading

tmontaigu commented Nov 20, 2024 •

edited

Loading