Mz/glwe ks test

[mayeul-zama]

Check-list:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

---

This change is 

[IceTDrinker]

Some small comments on the parts I've started reading

Reviewed 3 of 9 files at r1, all commit messages.
Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions

---

tfhe/src/core_crypto/commons/math/decomposition/decomposer.rs line 149 at r1 (raw file):

    /// Decode a plaintext value using the decoder to compute the closest representable.
    pub fn decode_plaintext(&self, input: Scalar) -> Scalar {

let's open an issue on that, we have lots of places in the code and/or docstrings doing that

also here input should be a Plaintext and output should be a Cleartext ? How is this called in the GLWE KS thing ?

---

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs line 924 at r1 (raw file):

}

pub fn polynomial_list_wrapping_sub_scalar_mul_assign<Scalar, InputCont, OutputCont, PolyCont>(

doesn't feel like this should have scalar in the name ? What am I missing, both polynomials have the same scalars right ?

---

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs line 948 at r1 (raw file):

}

pub fn polynomial_list_wrapping_sub_scalar_mul_assign_custom_mod<

same thing on the scalar naming ?

[IceTDrinker]

Some more comments

Reviewed 1 of 9 files at r1.
Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @mayeul-zama)

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 237 at r1 (raw file):

    T: UnsignedInteger,
{
    level: usize,

I would think having the slice first would be better layout wise ? Even though rust may be doing whatever it wants, would also be more consistent wit all our entities which have containers

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 322 at r1 (raw file):

    /// assert_eq!(term.level(), DecompositionLevel(3));
    /// ```
    pub fn level(&self) -> DecompositionLevel {

no need for a base log accessor ?

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 351 at r1 (raw file):

        level: DecompositionLevel,
        base_log: DecompositionBaseLog,
        slice: &'a [T],

same thing with the ordering of fields

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 466 at r1 (raw file):

    /// assert_eq!(term.level(), DecompositionLevel(3));
    /// ```
    pub fn level(&self) -> DecompositionLevel {

same thing, need an accessor for base log I would think

---

tfhe/src/core_crypto/commons/math/decomposition/iter.rs line 161 at r1 (raw file):

/// called again.
///
/// Such a pattern can not be implemented with iterators yet (without GATs), which is why this

Not sure I understand this comment here, some GATs have been added to Rust, we use them notably for the ContiguousEntityContainer trait family, so, what's the limitation here ?

The reference I see proposes to have an IntoIterator implementation which would return a type implementing the Iterator trait borrowing from this type

compiler has the same suggestion

error: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
   --> tfhe/src/core_crypto/commons/math/decomposition/iter.rs:191:17
    |
191 |     type Item = &[T];
    |                 ^
    |
note: you can't create an `Iterator` that borrows each `Item` from itself, but you can instead create a new type that borrows your existing type and implement `Iterator` for that new type
   --> tfhe/src/core_crypto/commons/math/decomposition/iter.rs:190:39
    |
190 | impl<T: UnsignedInteger> Iterator for SliceSignedDecompositionIter<T> {
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `tfhe` (lib) due to 1 previous error

The same way we have Vec::iter which returns a specific Iter<'a, T> object which implements IntoIterator

[IceTDrinker]

Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @mayeul-zama)

---

tfhe/src/core_crypto/commons/math/decomposition/iter.rs line 161 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Not sure I understand this comment here, some GATs have been added to Rust, we use them notably for the ContiguousEntityContainer trait family, so, what's the limitation here ?

The reference I see proposes to have an IntoIterator implementation which would return a type implementing the Iterator trait borrowing from this type

compiler has the same suggestion

error: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
   --> tfhe/src/core_crypto/commons/math/decomposition/iter.rs:191:17
    |
191 |     type Item = &[T];
    |                 ^
    |
note: you can't create an `Iterator` that borrows each `Item` from itself, but you can instead create a new type that borrows your existing type and implement `Iterator` for that new type
   --> tfhe/src/core_crypto/commons/math/decomposition/iter.rs:190:39
    |
190 | impl<T: UnsignedInteger> Iterator for SliceSignedDecompositionIter<T> {
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `tfhe` (lib) due to 1 previous error

The same way we have Vec::iter which returns a specific Iter<'a, T> object which implements IntoIterator

Some attemps on my end failed, otherwise use interior mutability with a Cell to wrap the Vec ? potential issues with invalidated iterators, but hoepfully our usage should not suffer from that problem

[IceTDrinker]

Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @mayeul-zama)

---

tfhe/src/core_crypto/commons/math/decomposition/iter.rs line 161 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Some attemps on my end failed, otherwise use interior mutability with a Cell to wrap the Vec ? potential issues with invalidated iterators, but hoepfully our usage should not suffer from that problem

Or rather RefCell for enforcing borrow rules at runtime

[IceTDrinker]

Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @mayeul-zama)

---

tfhe/src/core_crypto/commons/math/decomposition/iter.rs line 161 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Or rather RefCell for enforcing borrow rules at runtime

Actually that's probably not doable, which is a shame, I would still like to see if we can have some better ergonomics than the manual next_term which looks error prone to me

[IceTDrinker]

Reviewable status: 4 of 9 files reviewed, 8 unresolved discussions (waiting on @mayeul-zama)

---

tfhe/src/core_crypto/commons/math/decomposition/iter.rs line 161 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Actually that's probably not doable, which is a shame, I would still like to see if we can have some better ergonomics than the manual next_term which looks error prone to me

Ok thinking about this some more, we said this would be the easy to use API, so maybe let's remove the inner outputs buffer and return owned values here, knowing that we already have an optimized version for the PBS that does the smart thing, we can add/use more optimized primitives later

[IceTDrinker]

Actually maybe a while let (Some(key), Some(decomp)) = (iter.next(), decomp.next_term())

Could do the trick ?

[mayeul-zama]

Done (while let loop)

Reviewable status: 4 of 9 files reviewed, 4 unresolved discussions (waiting on @IceTDrinker)

---

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs line 924 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

doesn't feel like this should have scalar in the name ? What am I missing, both polynomials have the same scalars right ?

Done

---

tfhe/src/core_crypto/algorithms/polynomial_algorithms.rs line 948 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

same thing on the scalar naming ?

Done

---

tfhe/src/core_crypto/commons/math/decomposition/decomposer.rs line 149 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

let's open an issue on that, we have lots of places in the code and/or docstrings doing that

also here input should be a Plaintext and output should be a Cleartext ? How is this called in the GLWE KS thing ?

It's called in a doctest

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 237 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

I would think having the slice first would be better layout wise ? Even though rust may be doing whatever it wants, would also be more consistent wit all our entities which have containers

Done

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 322 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

no need for a base log accessor ?

This one is not needed either

---

tfhe/src/core_crypto/commons/math/decomposition/term.rs line 351 at r1 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

same thing with the ordering of fields

Done

[nsarlin-zama]

Reviewed 3 of 9 files at r1, 4 of 5 files at r2.
Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/glwe_keyswitch.rs line 225 at r2 (raw file):

                &Polynomial::from_container(decomposed.as_slice()),
            );
        }

In my opinion this is slightly more readable because you don't have to check another function to understand the loop. If both iterators are of the same size I believe this is equivalent.

Suggestion:

        while let (Some(level_key_ciphertext), Some(decomposed)) = (
            keyswitch_key_block_iter.next(),
            decomposition_iter.next_term(),
        ) {
            polynomial_list_wrapping_sub_mul_assign(
                &mut output_glwe_ciphertext.as_mut_polynomial_list(),
                &level_key_ciphertext.as_polynomial_list(),
                &Polynomial::from_container(decomposed.as_slice()),
            );
        }

[IceTDrinker]

Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/glwe_keyswitch.rs line 225 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

In my opinion this is slightly more readable because you don't have to check another function to understand the loop. If both iterators are of the same size I believe this is equivalent.

I agree

[mayeul-zama]

Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/glwe_keyswitch.rs line 225 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

I agree

This idea was precisely to add some security by panicking if both iterators are not of the same size

[IceTDrinker]

Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/algorithms/glwe_keyswitch.rs line 225 at r2 (raw file):

Previously, mayeul-zama wrote…

This idea was precisely to add some security by panicking if both iterators are not of the same size

while I understand, no keyswitching algorithm does that check, it's part of the decomposer correctness

[mayeul-zama]

tfhe/src/core_crypto/algorithms/glwe_keyswitch.rs line 225 at r2 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

while I understand, no keyswitching algorithm does that check, it's part of the decomposer correctness

Removed

[IceTDrinker]

I'm nearly done 🫠

Reviewed 2 of 9 files at r1, 3 of 5 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, 12 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/core_crypto/commons/math/decomposition/decomposer.rs line 149 at r1 (raw file):

Previously, mayeul-zama wrote…

It's called in a doctest

ok !

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 15 at r3 (raw file):

///
/// A key switching key is a vector of GLev ciphertexts (described on the bottom of
/// [`this page`](`crate::core_crypto::entities::GgswCiphertext#Glev-ciphertext`)).

seems the fragment needs a lower g so #glev-ciphertext to work on the compiled docs locally

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 71 at r3 (raw file):

///
/// ###### algorithm:
/// 1. set $\mathsf{cCT}=\left( 0 , \cdots , 0 ,  B\_{\mathsf{in}} \right) \in

cCT -> CT

typo from the original PR I guess

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 76 at r3 (raw file):

///    \mathsf{decompProduct}\left( A\_i , \overline{\mathsf{CT}\_i} \right)$
/// 3. output $\mathsf{CT}\_{\mathsf{out}}$
#[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]

probably need versionize which did not exist back then

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 110 at r3 (raw file):

) -> usize {
    // One ciphertext per level encrypted under the output key
    decomp_level_count.0 * output_glwe_size.0 * poly_size.0

nit can use glwe_ciphertext_size from the glwe_ciphertext.rs module

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 114 at r3 (raw file):

impl<Scalar: UnsignedInteger, C: Container<Element = Scalar>> GlweKeyswitchKey<C> {
    /// Create an [`GlweKeyswitchKey`] from an existing container.

nit Create a Glwe

"an Lwe" vs "a Glwe", at least I think that's how it is elsewhere

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 118 at r3 (raw file):

    /// # Note
    ///
    /// This function only wraps a container in the appropriate type. If you want to generate an

check all an/a when GlweKeyswitchKey is used in this file

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 195 at r3 (raw file):

        );
        assert!(
            container.container_len() % (decomp_level_count.0 * output_glwe_size.0 * poly_size.0)

might be possible to use the key size primitive above

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 217 at r3 (raw file):

    /// Return the [`DecompositionBaseLog`] of the [`LweKeyswitchKey`].
    ///
    /// See [`LweKeyswitchKey::from_container`] for usage.

wrong link back, probably the same throughout

---

tfhe/src/core_crypto/entities/glwe_keyswitch_key.rs line 224 at r3 (raw file):

    /// Return the [`DecompositionLevelCount`] of the [`LweKeyswitchKey`].
    ///
    /// See [`LweKeyswitchKey::from_container`] for usage.

here too

---

tfhe/src/core_crypto/algorithms/glwe_keyswitch_key_generation.rs line 190 at r3 (raw file):

                .fill_slice_with_recomposition_summand(message_polynomial.as_mut());

            slice_wrapping_scalar_div_assign(

a comment similar to lwe_keyswitch_key_generation to explain the div would be welcome IMO

[IceTDrinker]

Ok finished the last few lines/files I think it should be good

Reviewed 1 of 9 files at r1, 1 of 2 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)