Mz/reduce ms noise

[mayeul-zama]

Check-list:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• Relevant issues are marked as resolved/closed, related issues are linked in the description
• Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

---

This change is 

[tmontaigu]

Reviewed 41 of 41 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, and @nsarlin-zama)

---

tfhe/Cargo.toml line 172 at r1 (raw file):

path = "benches/core_crypto/modulus_switch_noise_reduction.rs"
harness = false
required-features = ["shortint"]

Now that I see this, it's a bit weird that core_crypto benches require the shortint feature, but that's for the parameters I guess

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 89 at r1 (raw file):

    assert_eq!(lwe.lwe_size(), encryptions_of_zero.lwe_size());
    assert_ne!(encryptions_of_zero.lwe_ciphertext_count().0, 0);

Do we also need checks on cipherext_modulus, and log_modulus ?

---

tfhe/src/shortint/server_key/mod.rs line 1524 at r1 (raw file):

    acc: &mut GlweCiphertext<OutputCont>,
    buffers: &mut ComputationBuffers,
    enable_modulus_switch_noise_reduction: bool,

I tend to like using enums rather than bools for this kind of things

enum DoModulusSwitchNoiseReduction {
  No,
  Yes
}

But that is a minor thing

[mayeul-zama]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/Cargo.toml line 172 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Now that I see this, it's a bit weird that core_crypto benches require the shortint feature, but that's for the parameters I guess

Yes

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 89 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Do we also need checks on cipherext_modulus, and log_modulus ?

On cipherext_modulus, sure, will add
On log_modulus, I don't see what we could assert

---

tfhe/src/shortint/server_key/mod.rs line 1524 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I tend to like using enums rather than bools for this kind of things

enum DoModulusSwitchNoiseReduction {
  No,
  Yes
}

But that is a minor thing

Used
ModulusSwitchNoiseReduction::Disableand ModulusSwitchNoiseReduction::Enable

[IceTDrinker]

Some questions

Reviewed 1 of 41 files at r1, 2 of 8 files at r2.
Reviewable status: 36 of 42 files reviewed, 4 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 97 at r2 (raw file):

    C2: Container<Element = Scalar>,
{
    assert_eq!(lwe.lwe_size(), encryptions_of_zero.lwe_size());

missing human readable error messages, also if we return a Result, then return an error instead ?

---

tfhe/src/shortint/server_key/mod.rs line 1609 at r2 (raw file):

    acc: &GlweCiphertext<Vec<u64>>,
    buffers: &mut ComputationBuffers,
    modulus_switch_noise_reduction: ModulusSwitchNoiseReduction,

Feel like this would be error prone, I'm guessing it's linked to the OPRF that must not perform the modulus switch noise reduction ?

If we have only one case that needs to disable the modulus switch reduction I'm tempted to avoid a function with configurable behavior

Essentially if the option is empty always disable the modulus switch reduction, and for the prf call a specialized function that has no modulus switch reduction code anywhere

[IceTDrinker]

Some more comments, I should be nearly done going over the PR

Reviewed 13 of 41 files at r1, 1 of 8 files at r2, all commit messages.
Reviewable status: 37 of 42 files reviewed, 28 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/benches/core_crypto/modulus_switch_noise_reduction.rs line 25 at r2 (raw file):

            EncryptionRandomGenerator::<DefaultRandomGenerator>::new(seeder.seed(), seeder);

        let bound = 188.2e15_f64;

what's this bound ? does it represent a particular value ?

---

tfhe/src/integer/gpu/server_key/mod.rs line 212 at r2 (raw file):

            CudaLweKeyswitchKey::from_lwe_keyswitch_key(&h_key_switching_key, streams);
        let bootstrapping_key = match bootstrapping_key {
            crate::shortint::server_key::compressed::ShortintCompressedBootstrappingKey::Classic{ bsk: h_bootstrap_key, modulus_switch_noise_reduction_key: _ } => {

we probably need a hard error until the GPU supports the drift mitigation technique, wdyt @agnesLeroy ?

---

tfhe/src/c_api/shortint/parameters.rs line 67 at r2 (raw file):

            log2_p_fail: c_params.log2_p_fail,
            encryption_key_choice: c_params.encryption_key_choice.into(),
            modulus_switch_noise_reduction_params: None,

this needs to be handled properly

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 9 at r2 (raw file):

use itertools::Itertools;

fn round<Scalar: UnsignedInteger>(input: Scalar, log_modulus: CiphertextModulusLog) -> Scalar {

this is only correct for power of 2 moduli, we likely need a verification that the noise reduction operation only works on power of 2 moduli

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 36 at r2 (raw file):

}

fn measure_modulus_switch_noise_expectancy_variance<Scalar: UnsignedInteger>(

could be worth saying it's for binary key distribution

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 63 at r2 (raw file):

}

fn measure<Scalar: UnsignedInteger>(

more explicit name please

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 85 at r2 (raw file):

}

pub fn choose_candidate_to_improve_modulus_switch_noise<Scalar, C1, C2>(

indicate in documentation it's binary and in the name

could add the paper reference as well

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 88 at r2 (raw file):

    lwe: &LweCiphertext<C1>,
    encryptions_of_zero: &LweCiphertextList<C2>,
    r_sigma_factor: f64,

new types here would be safer, to be added in shortint parameter sets as well

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 124 at r2 (raw file):

    for (index, encryption_of_zero) in encryptions_of_zero.iter().enumerate() {
        let mask_2 = encryption_of_zero.get_mask();

you can do better than "mask_2" here "encrypted_zero_mask"

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 153 at r2 (raw file):

}

pub fn improve_modulus_switch_noise<Scalar, C1, C2>(

improve_lwe_ciphertext_modulus_switch_noise and something about the fact it is restricted to binary coefficients and a ref to the paper (don't know which function users are supposed to call, so the public facing ones need the binary key info and the paper ref)

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 191 at r2 (raw file):

#[cfg(test)]
mod tests;

follow the pattern and use the alrogithms/test directory please

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 1 at r2 (raw file):

use super::super::test::TestResources;

move this file to the appropriate tests directory

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 21 at r2 (raw file):

    pub lwe_noise_distribution: DynamicDistribution<u64>,
    pub ciphertext_modulus: CiphertextModulus<u64>,
    pub modulus_switch_zeros_count: usize,

LweCiphertextCount

and for the params below the new types you are adding

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 37 at r2 (raw file):

};

#[cfg(feature = "shortint")]

NO, core tests don't depend on higher layers, if needed we copy a set of parameters and keep it as is in core

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 67 at r2 (raw file):

#[test]
fn improve_modulus_switch_noise_test_average_number_checks_parameterized() {
    let expected_average_number_checks = 16.8683590276303_f64;

this should be in the set of tests parameters or have a function that computes it

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 138 at r2 (raw file):

        // Check for NoAddition in all cases
        number_checks += 1;

is the NoAddition part of the # of checks in the paper ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 171 at r2 (raw file):

}

fn operation_error<C1, C2>(

clearer name, this does not convey what it does

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 180 at r2 (raw file):

    C2: ContainerMut<Element = u64>,
{
    let decoded_before = decrypt_lwe_ciphertext(sk, &ct);

this is not decoded as far as I can tell, just decrypted

same below

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 193 at r2 (raw file):

    check_noise_improve_modulus_switch_noise(
        TEST_PARAM,
        Variance(4.834651119161795e32 - 9.68570987092478e+31),

need test params or this value needs to be part of the test params as an "expected_xxx"

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 220 at r2 (raw file):

    );

    let sk_average_bit: f64 =

needed for what ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 255 at r2 (raw file):

        .into_par_iter()
        .map(|_| {
            let lwe = ENCRYPTION_GENERATOR.with(|encryption_generator| {

could just create a new test resource per thread and use the encryption rng

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 297 at r2 (raw file):

    );

    println!(

only the abs value is of interest here ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 332 at r2 (raw file):

    assert!(
        (base_variance - expected_base_variance).abs() / (base_variance + expected_base_variance)
            < 0.1,

does the R&D have confidence intervals for this ? I'm fine-ish to have fairly large values initially but if we have proper confidence interval it's better

---

tests/backward_compatibility/shortint.rs line 49 at r2 (raw file):

            }
        },
        modulus_switch_noise_reduction_params: None,

we likely want to have test parameter sets that will support the mod switch down the line @nsarlin-zama how should this be handled ?

[nsarlin-zama]

Reviewable status: 37 of 42 files reviewed, 29 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, and @tmontaigu)

---

tests/backward_compatibility/shortint.rs line 49 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

we likely want to have test parameter sets that will support the mod switch down the line @nsarlin-zama how should this be handled ?

I think a ServerKey with a ModulusSwitchNoiseReductionKey should be added to the data repo. If it changes the structure of the TestPArameterSet it might require to bump the version of the data repo (this is done by creating a new branch)

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 153 at r2 (raw file):

}

pub fn improve_modulus_switch_noise<Scalar, C1, C2>(

Maybe you could add some comments to explain the general idea

[IceTDrinker]

And the last few comments

Reviewable status: 37 of 42 files reviewed, 32 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

---

tfhe/src/shortint/list_compression/compression.rs line 218 at r2 (raw file):

                &decompression_rescale.acc,
                buffers,
                ModulusSwitchNoiseReduction::Disable,

for the decompression to enforce the invariant could: check that the list of zeros is empty and calling the PBS

---

tfhe/src/shortint/server_key/mod.rs line 1545 at r2 (raw file):

            let ciphertext_modulus = in_buffer.ciphertext_modulus();
            let mut input: LweCiphertext<Vec<u64>> = LweCiphertext::from_container(
                in_buffer.as_view().into_container().to_owned(),

as_ref().to_vec()

we may need a ToOwned implementation for our LweCiphertext in Core

---

tfhe/src/shortint/server_key/mod.rs line 1764 at r2 (raw file):

                    ModulusSwitchNoiseReductionKeyConformanceParameters::try_from(parameter_set),
                ) {
                    (None, Err(())) => true,

should this be

(None, _) ?

[IceTDrinker]

Reviewable status: 33 of 43 files reviewed, 32 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/shortint/server_key/mod.rs line 1609 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Feel like this would be error prone, I'm guessing it's linked to the OPRF that must not perform the modulus switch noise reduction ?

If we have only one case that needs to disable the modulus switch reduction I'm tempted to avoid a function with configurable behavior

Essentially if the option is empty always disable the modulus switch reduction, and for the prf call a specialized function that has no modulus switch reduction code anywhere

I confirm this API is wrong in its current form the nosie reduction thing is in the bootstrapping key above

[mayeul-zama]

Reviewable status: 33 of 43 files reviewed, 24 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/benches/core_crypto/modulus_switch_noise_reduction.rs line 25 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

what's this bound ? does it represent a particular value ?

Changed to use the same bound as in the tests

---

tfhe/src/c_api/shortint/parameters.rs line 67 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this needs to be handled properly

Done

---

tfhe/src/integer/gpu/server_key/mod.rs line 212 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

we probably need a hard error until the GPU supports the drift mitigation technique, wdyt @agnesLeroy ?

Added

---

tfhe/src/shortint/list_compression/compression.rs line 218 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

for the decompression to enforce the invariant could: check that the list of zeros is empty and calling the PBS

Done

---

tfhe/src/shortint/server_key/mod.rs line 1609 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Feel like this would be error prone, I'm guessing it's linked to the OPRF that must not perform the modulus switch noise reduction ?

If we have only one case that needs to disable the modulus switch reduction I'm tempted to avoid a function with configurable behavior

Essentially if the option is empty always disable the modulus switch reduction, and for the prf call a specialized function that has no modulus switch reduction code anywhere

Added multiple functions

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 36 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

could be worth saying it's for binary key distribution

Renamed measure_modulus_switch_noise_expectancy_variance_for_binary_key

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 63 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

more explicit name please

Renamed measure_noise_estimation

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 85 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

indicate in documentation it's binary and in the name

could add the paper reference as well

Renamed choose_candidate_to_improve_modulus_switch_noise_for_binary_key
Added link to the paper

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 88 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

new types here would be safer, to be added in shortint parameter sets as well

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 97 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

missing human readable error messages, also if we return a Result, then return an error instead ?

Added

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 124 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

you can do better than "mask_2" here "encrypted_zero_mask"

Renamed encryption_of_zero_mask

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 153 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

improve_lwe_ciphertext_modulus_switch_noise and something about the fact it is restricted to binary coefficients and a ref to the paper (don't know which function users are supposed to call, so the public facing ones need the binary key info and the paper ref)

Renamed improve_lwe_ciphertext_modulus_switch_noise_for_binary_key

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 191 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

follow the pattern and use the alrogithms/test directory please

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 1 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

move this file to the appropriate tests directory

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 21 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

LweCiphertextCount

and for the params below the new types you are adding

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 37 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

NO, core tests don't depend on higher layers, if needed we copy a set of parameters and keep it as is in core

Removed

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 138 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

is the NoAddition part of the # of checks in the paper ?

"We assume, for syntactic convenience, that Transform(c, (D 1 , . . . , D Z ), cnt) with cnt = 0 outputs the input ciphertext c."
suggests yes

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 171 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

clearer name, this does not convey what it does

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 180 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this is not decoded as far as I can tell, just decrypted

same below

Renamed

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 220 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

needed for what ?

It impacts variance

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 255 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

could just create a new test resource per thread and use the encryption rng

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 297 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

only the abs value is of interest here ?

Sign can be interesting

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 332 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

does the R&D have confidence intervals for this ? I'm fine-ish to have fairly large values initially but if we have proper confidence interval it's better

Used balanced key and reduced interval
https://github.com/zama-ai/tfhe-rs-internal/issues/908

[mayeul-zama]

Reviewable status: 31 of 43 files reviewed, 15 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/shortint/server_key/mod.rs line 1764 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

should this be

(None, _) ?

I don't think so
If we have MS_noise_reduction_params, we should also have a MS_noise_reduction_key

[IceTDrinker]

Reviewable status: 31 of 43 files reviewed, 15 unresolved discussions (waiting on @agnesLeroy, @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/shortint/server_key/mod.rs line 1764 at r2 (raw file):

Previously, mayeul-zama wrote…

I don't think so
If we have MS_noise_reduction_params, we should also have a MS_noise_reduction_key

indeed

[nsarlin-zama]

Reviewable status: 31 of 43 files reviewed, 15 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, @mayeul-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 153 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

Maybe you could add some comments to explain the general idea

Not blocking, but maybe this could be worth having a small high level explanation here, just to give a rough idea of what is happening without having to read the full paper

[nsarlin-zama]

Thanks! I approve to be non blocking as this is not the most urgent but I think think could benefit from a few comments and also it would be a good idea to add data for this new key before we forget it

Reviewable status: 31 of 43 files reviewed, 14 unresolved discussions (waiting on @agnesLeroy, @IceTDrinker, @mayeul-zama, and @tmontaigu)

---

tfhe/src/shortint/server_key/modulus_switch_noise_reduction.rs line 49 at r4 (raw file):

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, NotVersioned)]
pub struct ModulusSwitchNoiseReductionKey {

Here you could also add a comment to give some context

[IceTDrinker]

Reviewed 2 of 7 files at r3, 5 of 10 files at r4, all commit messages.
Reviewable status: 38 of 43 files reviewed, 12 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

---

tests/backward_compatibility/shortint.rs line 49 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I think a ServerKey with a ModulusSwitchNoiseReductionKey should be added to the data repo. If it changes the structure of the TestPArameterSet it might require to bump the version of the data repo (this is done by creating a new branch)

current issue is we don't have the parameters in main for now, @mayeul-zama can you also note that the data repo needs an update once we have the parameters ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 63 at r2 (raw file):

Previously, mayeul-zama wrote…

Renamed measure_noise_estimation

may need binary_key naming, wdyt ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 85 at r2 (raw file):

Previously, mayeul-zama wrote…

Renamed choose_candidate_to_improve_modulus_switch_noise_for_binary_key
Added link to the paper

I don't see the link to the paper nor a doc comment ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 153 at r2 (raw file):

Previously, mayeul-zama wrote…

Renamed improve_lwe_ciphertext_modulus_switch_noise_for_binary_key

Please write a short description for the doc comment

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 67 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this should be in the set of tests parameters or have a function that computes it

does not seem to have been taken into account ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 138 at r2 (raw file):

Previously, mayeul-zama wrote…

"We assume, for syntactic convenience, that Transform(c, (D 1 , . . . , D Z ), cnt) with cnt = 0 outputs the input ciphertext c."
suggests yes

what is cnt here ?

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 193 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

need test params or this value needs to be part of the test params as an "expected_xxx"

not taken into account

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 255 at r2 (raw file):

Previously, mayeul-zama wrote…

Done

no you misunderstood

let mut test_rsc = TestResources::new();

directly in the par iter and then use the test_rsc in the par iter, no need for a refcell or thread local

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 332 at r2 (raw file):

Previously, mayeul-zama wrote…

Used balanced key and reduced interval
https://github.com/zama-ai/tfhe-rs-internal/issues/908

new interval supported by research formulas ?

---

tfhe/src/shortint/server_key/modulus_switched_compression.rs line 142 at r4 (raw file):

                        }
                    };
                    apply_programmable_bootstrap_no_ms_noise_reduction(

this form of compression may need the mod switch reduction don't you think ? as it's essentially storing the coefficient before the blind rotation ?

---

tfhe/benches/core_crypto/modulus_switch_noise_reduction.rs line 3 at r4 (raw file):

use criterion::{black_box, criterion_group, criterion_main, Criterion};
use modulus_switch_noise_reduction::improve_lwe_ciphertext_modulus_switch_noise_for_binary_key;
use tfhe::core_crypto::prelude::modulus_switch_noise_reduction::{

pattern for newtypes used in parameters is to put them all in core_crypto/commons/parameters.rs

---

tfhe/benches/core_crypto/modulus_switch_noise_reduction.rs line 9 at r4 (raw file):

fn modulus_switch_noise_reduction(c: &mut Criterion) {
    // TODO: use shortint params

do you have a follow up issue for that so we don't forget ?

---

tfhe/src/shortint/server_key/mod.rs line 1556 at r4 (raw file):

    let ciphertext_modulus = in_buffer.ciphertext_modulus();
    let mut input: LweCiphertext<Vec<u64>> = LweCiphertext::from_container(
        in_buffer.as_view().into_container().to_owned(),

as_ref().to_vec()

[IceTDrinker]

For the backward data it could already be possible to make it work actually if we use fake parameters potentially

[IceTDrinker]

are the shortint modulus switch parameters publicly re-exported at the shortint::parameters level ? this is the centralized location for everything shortint parameters, potentially the definition of the parameters at the shortint level should be moved there

[IceTDrinker]

the required new types can also be re-exported at that level to be easily accesible from the shortint::parameters module

[IceTDrinker]

Reviewable status: 38 of 43 files reviewed, 13 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

---

tfhe/src/shortint/server_key/modulus_switch_noise_reduction.rs line 131 at r4 (raw file):

        let plaintext_list = PlaintextList::new(0, PlaintextCount(count.0));

        encrypt_lwe_ciphertext_list(

I believe we have a parallel variant for this

[mayeul-zama]

Reviewable status: 38 of 43 files reviewed, 10 unresolved discussions (waiting on @IceTDrinker and @tmontaigu)

---

tfhe/benches/core_crypto/modulus_switch_noise_reduction.rs line 3 at r4 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

pattern for newtypes used in parameters is to put them all in core_crypto/commons/parameters.rs

Moved

---

tfhe/src/shortint/server_key/mod.rs line 1524 at r1 (raw file):

Previously, mayeul-zama wrote…

Used
ModulusSwitchNoiseReduction::Disableand ModulusSwitchNoiseReduction::Enable

Changed to use separate functions

---

tfhe/src/shortint/server_key/mod.rs line 1545 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

as_ref().to_vec()

we may need a ToOwned implementation for our LweCiphertext in Core

Added to_owned_container method to avoid conflicts with #[derive(Clone)]

---

tfhe/src/shortint/server_key/mod.rs line 1556 at r4 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

as_ref().to_vec()

Done

---

tfhe/src/shortint/server_key/modulus_switched_compression.rs line 142 at r4 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this form of compression may need the mod switch reduction don't you think ? as it's essentially storing the coefficient before the blind rotation ?

I don't think so as the input is stored already MSed

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 89 at r1 (raw file):

Previously, mayeul-zama wrote…

On cipherext_modulus, sure, will add
On log_modulus, I don't see what we could assert

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 9 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this is only correct for power of 2 moduli, we likely need a verification that the noise reduction operation only works on power of 2 moduli

Arguably that's implied as log_modulus: CiphertextModulusLog base if 2

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 63 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

may need binary_key naming, wdyt ?

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/mod.rs line 85 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

I don't see the link to the paper nor a doc comment ?

Moved the link to function improve_lwe_ciphertext_modulus_switch_noise_for_binary_key as it's higher level

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 67 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

does not seem to have been taken into account ?

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 138 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

what is cnt here ?

"cnt is a counter"

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 193 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

not taken into account

Done

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 255 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

no you misunderstood

let mut test_rsc = TestResources::new();

directly in the par iter and then use the test_rsc in the par iter, no need for a refcell or thread local

I did that the first time I introduced parallelism
It makes the test much slower

---

tfhe/src/core_crypto/algorithms/modulus_switch_noise_reduction/tests.rs line 332 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

new interval supported by research formulas ?

Not yet
Maybe to add in a new PR

[IceTDrinker]

Looks good, I will wait on @nsarlin-zama to approve as he is blocking for now, otherwise awesome work and thanks a lot !

Reviewed 11 of 11 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

[IceTDrinker]

see some pcc errors I guess

[IceTDrinker]

ah no wait I'm still blocking 🧐 in any case I'm good to approve now, just waiting on Nico to avoid re launching a cycle of reviews

[IceTDrinker]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

    let decrypted_before = decrypt_lwe_ciphertext(sk, &ct);

    operation(&mut ct);

this can be wrong if the encrytped msg is not 0 right ? as the subtraction at the end will subtract plaintexts which are different

so you could take the expected plaintext as input, subtract it before op, do op, and then add it back

[mayeul-zama]

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

this can be wrong if the encrytped msg is not 0 right ? as the subtraction at the end will subtract plaintexts which are different

so you could take the expected plaintext as input, subtract it before op, do op, and then add it back

I don't think there is an issue here

The operation is supposed to not change the message, only add a noise which we measure

Input: Enc(m + noise_in)
Output: Enc(m + noise_out)

We return noise_out - noise_in

[mayeul-zama]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/shortint/server_key/modulus_switch_noise_reduction.rs line 49 at r4 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I was more thinking of a module level documentation ? Because I feel that it can be a bit complicated for someone arriving here to understand what this is (even for us a few month from here)

Added a slightly bigger doc on the key

A module doc would not be visible as the module is private and only the Key type is reexported

---

tfhe/src/shortint/server_key/modulus_switch_noise_reduction.rs line 42 at r13 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

You can add brackets to make the link clickable in rustdoc

Done while changing the comment

[IceTDrinker]

Reviewed 7 of 7 files at r14, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

Previously, mayeul-zama wrote…

I don't think there is an issue here

The operation is supposed to not change the message, only add a noise which we measure

Input: Enc(m + noise_in)
Output: Enc(m + noise_out)

We return noise_out - noise_in

The function is local to the file so it's fine, but I can tell it would be error prone

[mayeul-zama]

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

The function is local to the file so it's fine, but I can tell it would be error prone

I can change the name to make the assumption on the operation clear

[IceTDrinker]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

Previously, mayeul-zama wrote…

I can change the name to make the assumption on the operation clear

if you are chaning it I would provide the plaintext and be done with it :)

[mayeul-zama]

tfhe/src/core_crypto/algorithms/test/modulus_switch_noise_reduction.rs line 334 at r13 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

if you are chaning it I would provide the plaintext and be done with it :)

Renamed

It would need 2 plaintexts (input and output)

[nsarlin-zama]

I have not looked into the details of the implementations, but it looks like impressive work!

Reviewable status: 47 of 50 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker and @tmontaigu)

---

tfhe/src/shortint/server_key/modulus_switch_noise_reduction.rs line 42 at r13 (raw file):

Previously, mayeul-zama wrote…

Done while changing the comment

thanks!

[IceTDrinker]

Reviewed 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tmontaigu)

[IceTDrinker]

it's official ! Thanks a ton @mayeul-zama :)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tmontaigu)