Feat(core): Chunked LweBootstrapKey Generation

[youben11]

PR content/description

This PR makes it possible to generate LWE Bootstrap keys in chunks of a desired size. This might be useful for generating big keys in environment with limited memory (think like generating chunks and writing them to disk).

I'm not sure about the correct value to set for MIN_SUPPORTED_APP_VERSION, so I left it as TODO for now.

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 

[mayeul-zama]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @IceTDrinker)

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 598 at r1 (raw file):

}

/// A generator for producing chunks of an LWE bootstrap key.

Could be put in a separate module to keep modules small

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 661 at r1 (raw file):

    Cont: Container<Element = Scalar>,
{
    enc_generator: EncryptionRandomGenerator<Gen>,

enc_generator and secret_keys could be references instead of owned types

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 718 at r1 (raw file):

}

impl<Gen, Cont, Scalar, NoiseDistribution> Iterator

I think it would be cleaner to impl IntoIterator here
That we we can keep the position field out of LweBootstrapKeyChunkGenerator

type IntoIter could then be

struct  LweBootstrapKeyChunkIterator {
    position: usize,
    generator: LweBootstrapKeyChunkGenerator,
}

enc_generator: EncryptionRandomGenerator<Gen>, could also be a field of LweBootstrapKeyChunkIterator instead of LweBootstrapKeyChunkGenerator

But that would need a standalone into_iter(&mut EncryptionRandomGenerator<Gen>,) method (no part of the trait).

Which makes me think it may be better as is

---

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

pub fn assemble_lwe_bootstrap_key_from_chunks<Scalar, Cont, ContMut>(
    output: &mut LweBootstrapKey<ContMut>,
    chunks: Vec<LweBootstrapKeyChunk<Cont>>,

Could be a iterator of LweBootstrapKeyChunk<Cont>

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 17 at r1 (raw file):

/// are implemented to dereference to the underlying [`GgswCiphertextList`] for ease of use. See
/// [`GgswCiphertextList`] for additional methods.
#[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize, Versionize)]

Do you need to be able to (de)serialize this object or is it only a RAM only object?

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 26 at r1 (raw file):

    // and use Deref to have access to all the primitives of the GgswCiphertextList easily
    ggsw_list: GgswCiphertextList<C>,
}

It could store start index to enable some checks in the assemble function

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 28 at r1 (raw file):

}

impl<Scalar: UnsignedInteger, C: Container<Element = Scalar>> std::ops::Deref

Is this impl necessary? Accessing the field directly of through a get method could work and be less "magic"
It's not very clear to me how you want to use this object

---

tfhe/src/core_crypto/backward_compatibility/entities/lwe_bootstrap_key_chunk.rs line 6 at r1 (raw file):

use crate::core_crypto::prelude::{Container, LweBootstrapKeyChunk, UnsignedInteger};

impl<C: Container> Deprecable for LweBootstrapKeyChunk<C>

Why implement this trait?

[IceTDrinker]

@youben11 when you have addressed the comments by Mayeul I'll read the PR myself

[youben11]

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

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 598 at r1 (raw file):

Previously, mayeul-zama wrote…

Could be put in a separate module to keep modules small

I don't mind doing that, but Arthur had a different opinion IIRC. So whatever works for you.

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 661 at r1 (raw file):

Previously, mayeul-zama wrote…

enc_generator and secret_keys could be references instead of owned types

Done.

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 718 at r1 (raw file):

Previously, mayeul-zama wrote…

I think it would be cleaner to impl IntoIterator here
That we we can keep the position field out of LweBootstrapKeyChunkGenerator

type IntoIter could then be

struct  LweBootstrapKeyChunkIterator {
    position: usize,
    generator: LweBootstrapKeyChunkGenerator,
}

enc_generator: EncryptionRandomGenerator<Gen>, could also be a field of LweBootstrapKeyChunkIterator instead of LweBootstrapKeyChunkGenerator

But that would need a standalone into_iter(&mut EncryptionRandomGenerator<Gen>,) method (no part of the trait).

Which makes me think it may be better as is

The motivation for having the generator was to avoid user mistakes in generating chunks themselves. So the current API seems okay to me, but I'm no pro Rustacian, so I might have bad taste.

---

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

Previously, mayeul-zama wrote…

Could be a iterator of LweBootstrapKeyChunk<Cont>

I think we would loose the benefit of having this chunked generation. The goal is to generate chunks separately first (in an env with limited resources), then assemble those chunks (in an env with more resources).

---

tfhe/src/core_crypto/backward_compatibility/entities/lwe_bootstrap_key_chunk.rs line 6 at r1 (raw file):

Previously, mayeul-zama wrote…

Why implement this trait?

I was mainly following how things are done for other entities, but the idea was to make it safe serializable

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 17 at r1 (raw file):

Previously, mayeul-zama wrote…

Do you need to be able to (de)serialize this object or is it only a RAM only object?

It is meant to be sent over the wire. Mainly a light client generating chunks, then a server assembling them into a full key.

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 26 at r1 (raw file):

Previously, mayeul-zama wrote…

It could store start index to enable some checks in the assemble function

I think it adds some extra complexity which might not be necessary for such low-level API. E.g. If we add the start field then we will have to think about ordering chunks before assembling.

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 28 at r1 (raw file):

Previously, mayeul-zama wrote…

Is this impl necessary? Accessing the field directly of through a get method could work and be less "magic"
It's not very clear to me how you want to use this object

I followed the same methods for the full bsk key. I can't see where those are needed (for both keys), so I don't mind removing some methods that might not be necessary.

[mayeul-zama]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @IceTDrinker and @youben11)

---

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

Previously, youben11 (Ayoub Benaissa) wrote…

I think we would loose the benefit of having this chunked generation. The goal is to generate chunks separately first (in an env with limited resources), then assemble those chunks (in an env with more resources).

Having an iterator here allows to collect the result of an iterator in a Vec and then iterate again on it. But it does not force to use a Vec

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 17 at r1 (raw file):

Previously, youben11 (Ayoub Benaissa) wrote…

It is meant to be sent over the wire. Mainly a light client generating chunks, then a server assembling them into a full key.

Ok
If this type is only supposed to be serialized temporarily and never stored, I guess we don't need to implement Versionize on it.
But it might still be safer to keep it

[nsarlin-zama]

You don't need to implement this, this trait is for types/versions that we don't want to support anymore (in an ideal world, this should never be implemented, but sometimes we have to be pragmatic and relax our principles).

[IceTDrinker]

Looking good, waiting on the few fixes you have to do (including commits messages, but I guess you were waiting for my review)

Mayeul left with the "request changes" so once the CI is green and the PR approved we'll do a bit of overriding to merge

In any case thanks for the contribution !

Reviewed 7 of 9 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mayeul-zama and @youben11)

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 598 at r1 (raw file):

Previously, youben11 (Ayoub Benaissa) wrote…

I don't mind doing that, but Arthur had a different opinion IIRC. So whatever works for you.

does not seem to be a problem for now to me

---

tfhe/src/core_crypto/algorithms/lwe_bootstrap_key_generation.rs line 718 at r1 (raw file):

Previously, youben11 (Ayoub Benaissa) wrote…

The motivation for having the generator was to avoid user mistakes in generating chunks themselves. So the current API seems okay to me, but I'm no pro Rustacian, so I might have bad taste.

Iterators keeping track of where they are is not unheard of, so the current Iterator implementation is good to me

---

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

Previously, mayeul-zama wrote…

Having an iterator here allows to collect the result of an iterator in a Vec and then iterate again on it. But it does not force to use a Vec

as a general rule in rust for contiguous data you would take a slice

&[LweBootstrapKeyChunk<Cont>]

iterator can allow to be more generic but the general design for it is not finalized, so if a contiguous array is good for you then the slice above will work and is more "rusty"

---

tfhe/src/core_crypto/backward_compatibility/entities/lwe_bootstrap_key_chunk.rs line 6 at r1 (raw file):

Previously, youben11 (Ayoub Benaissa) wrote…

I was mainly following how things are done for other entities, but the idea was to make it safe serializable

You don't need deprecable, it's only when we want to drop support for things that were historically supported

---

tfhe/src/core_crypto/entities/lwe_bootstrap_key_chunk.rs line 28 at r1 (raw file):

Previously, youben11 (Ayoub Benaissa) wrote…

I followed the same methods for the full bsk key. I can't see where those are needed (for both keys), so I don't mind removing some methods that might not be necessary.

so this was a bad choice of mine for the Boostrap key (in theory rust only recommends using those for smart pointers), if you remove those impls it means you would have to add accessors for all the metadata and add "as_ggsw_ciphertext_list" "as_mut_ggsw_ciphertext_list" to return a valid ggsw ciphertext list for encryption/assembly

Up to you on this one, but were I writing this, I would not use the Deref/DerefMut thing as a poor man's inheritance-like shortcut