feat(CompressedXofKeySet): impl key gen of ClientKey

[tmontaigu]

This adds the implemention of the ClientKey generation that respects the threshold specs, some points are:

• 2 random generators are used one for everything public (masks) the other for everything private (private keys and noise). These generators are seeded once.
• binary secret keys are generated using fill_slice_with_random_uniform_binary_bits and support pmax param

closes: please link all relevant issues

PR content/description

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 

[IceTDrinker]

first batch of comments on the "small" stuff, will read the xof part afterwards

@IceTDrinker reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 76 at r2 (raw file):

///
/// The hamming weight of the secret key will be in: `(1-pmax)*len..=pmax*len`
/// where pmax is in ]0.5, 1.0]

0.5 inclusive not ok ? I don't recall all the the pmax talk

also I think we agreed on calling it max normalized hamming weight when we looked at it a while back ?

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 109 at r2 (raw file):

///
/// // Check all coefficients are not zero as we just generated a new key
/// assert!(!glwe_secret_key.as_ref().iter().all(|&elt| elt == 0));

here we could have a pmax check in that case

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 114 at r2 (raw file):

    glwe_secret_key: &mut GlweSecretKey<InCont>,
    generator: &mut SecretRandomGenerator<Gen>,
    pmax: f64,

newtype would be welcome IMO

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

    let bounds = RangeInclusive::new(
        ((1.0 - pmax) * glwe_secret_key.polynomial_size().0 as f64) as u128,

u128 seems excessive ?

u64 would be plenty (supposing usize is annoying because changing between 32 and 64 bits systems
u32 likely works just as well

---

tfhe/src/core_crypto/algorithms/lwe_secret_key_generation.rs line 71 at r2 (raw file):

///
/// The hamming weight of the secret key will be in: `(1-pmax)*len..=pmax*len`
/// where pmax is in ]0.5, 1.0]

same remark as the glwe one

---

tfhe/src/core_crypto/algorithms/lwe_secret_key_generation.rs line 101 at r2 (raw file):

///
/// // Check all coefficients are not zero as we just generated a new key
/// assert!(!lwe_secret_key.as_ref().iter().all(|&elt| elt == 0));

same remark as the glwe one

---

tfhe/src/core_crypto/algorithms/lwe_secret_key_generation.rs line 118 at r2 (raw file):

    let bounds = RangeInclusive::new(
        ((1.0 - pmax) * lwe_secret_key.lwe_dimension().0 as f64) as u128,

same remark on the u128

---

tfhe/src/core_crypto/algorithms/lwe_secret_key_generation.rs line 122 at r2 (raw file):

    );

    loop {

given the code is the same do we think it's worth sharing the code for a slice between the two ?

[IceTDrinker]

The naming proposed probably does not make much sense here

For the normalized blabla thing

[tmontaigu]

Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions (waiting on @IceTDrinker)

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 76 at r2 (raw file):

Previously, IceTDrinker wrote…

0.5 inclusive not ok ? I don't recall all the the pmax talk

also I think we agreed on calling it max normalized hamming weight when we looked at it a while back ?

Yes unless i misunderstand the ( the 0.5 is not included

---

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, IceTDrinker wrote…

u128 seems excessive ?

u64 would be plenty (supposing usize is annoying because changing between 32 and 64 bits systems
u32 likely works just as well

yes 128bit is probably too much

[IceTDrinker]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 76 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Yes unless i misunderstand the ( the 0.5 is not included

no you are correct, 0.5 is excluded, all good 👍

[tmontaigu]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

yes 128bit is probably too much

Ah no, I see why I choose u128, CastFrom is included in UnsignedInteger trait, but not CastFrom

[tmontaigu]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Ah no, I see why I choose u128, CastFrom is included in UnsignedInteger trait, but not CastFrom

CastFrom<u128> is included CastFrom<u64> is not (reviewable likes to eat <x> )

[IceTDrinker]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

CastFrom<u128> is included CastFrom<u64> is not (reviewable likes to eat <x> )

ah ok I see

which type needs to be cast into which type ?

[tmontaigu]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, IceTDrinker wrote…

ah ok I see

which type needs to be cast into which type ?

The Scalar element of the secret keys needs to be casted to u128/u64

I could maybe use the Scalar type to count itself, that could potentially lead to problems for secret keys of u8, which is not something we use.

If I use the Scalar type of the secret key I could use OverflowingAdd

[IceTDrinker]

u128 is fine then, it's not in a hot loop and we don't expect to run the loop many times ?

[tmontaigu]

I think we almost don't ever expect the loop to run

[IceTDrinker]

Requesting changes for now to avoid cloning the u32 key, fairly confident we have a solution for that now @nsarlin-zama to confirm

I'll do another pass rechecking the spec

Reviewable status: 6 of 7 files reviewed, 13 unresolved discussions (waiting on @tmontaigu)

---

tfhe/src/high_level_api/xof_key_set.rs line 129 at r2 (raw file):

        let private_separator = *b"TFHEKGen";
        let private_seed = XofSeed::new(private_seed_bytes, private_separator);

do we not keep that configurable maybe ? unsure but I think we tried to keep this configurable whenever possible

---

tfhe/src/high_level_api/xof_key_set.rs line 133 at r2 (raw file):

        let public_separator = *b"TFHE_GEN";
        let mut public_seed_bytes = vec![0u8; security_bits as usize / 8];

I'll recheck the spec to make sure we are ok, for now checking that the code looks ok

---

tfhe/src/high_level_api/xof_key_set.rs line 259 at r2 (raw file):

        let integer_ksk_material = {
            let noise_distrib = match pk_to_sk_ksk_params.destination_key {

maybe I already asked but I think there are function to get keys and matching noise distributions from a ClientKey, @nsarlin-zama maybe you know/remember ?

in any case here it's correct but I prefer shared primitives for tricky code like this

---

tfhe/src/high_level_api/xof_key_set.rs line 297 at r2 (raw file):

                            // TODO: here we copy full secret key.
                            // this should be reworked to avoid this copy once the spec is final
                            let u64_container = ks32_ap

@nsarlin-zama is this a case we have to manage this way ? I think there is a way to make it work without copying/cloning that key but it means changing the KSK to adapt it to 64 bits

---

tfhe/src/high_level_api/xof_key_set.rs line 363 at r2 (raw file):

        let compressed_server_key = CompressedServerKey::from_raw_parts(
            integer_compressed_server_key,
            Some(integer_ksk_material),

Some could be handled at the keygen site ?

---

tfhe/src/high_level_api/xof_key_set.rs line 1895 at r2 (raw file):

        let mut seeder = new_seeder();
        let private_seed_bytes = seeder.seed().0.to_le_bytes().to_vec();

no more XofSeed ?

---

tfhe/src/high_level_api/xof_key_set.rs line 1898 at r2 (raw file):

        let security_bits = 128;
        let pmax = 0.8;
        let mut tag = Tag::default();

no way to create a tag from something at the start ? (not necessary to go ahead with the PR)

[tmontaigu]

Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on @IceTDrinker and @nsarlin-zama)

---

tfhe/src/core_crypto/algorithms/lwe_secret_key_generation.rs line 122 at r2 (raw file):

Previously, IceTDrinker wrote…

given the code is the same do we think it's worth sharing the code for a slice between the two ?

Added a method to the SecretGenerator

---

tfhe/src/high_level_api/xof_key_set.rs line 129 at r2 (raw file):

Previously, IceTDrinker wrote…

do we not keep that configurable maybe ? unsure but I think we tried to keep this configurable whenever possible

In the Tfhe.KeyGen from the paper, its not configurable

---

tfhe/src/high_level_api/xof_key_set.rs line 363 at r2 (raw file):

Previously, IceTDrinker wrote…

Some could be handled at the keygen site ?

I don't understand 🤔, you mean in the let integer_ksk_material = ... above, integer_ksk_material should already be Some(...) ?

---

tfhe/src/high_level_api/xof_key_set.rs line 1895 at r2 (raw file):

Previously, IceTDrinker wrote…

no more XofSeed ?

Nope, Tfhe.KeyGen does not take a xof seed, this is because the XofSeed contains the 8 bytes separator, however these are fixed by the algorithm

---

tfhe/src/high_level_api/xof_key_set.rs line 1898 at r2 (raw file):

Previously, IceTDrinker wrote…

no way to create a tag from something at the start ? (not necessary to go ahead with the PR)

Nope, but I could add a quick From<&str> impl, there's one for u64

[nsarlin-zama]

Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on @IceTDrinker)

---

tfhe/src/high_level_api/xof_key_set.rs line 259 at r2 (raw file):

Previously, IceTDrinker wrote…

maybe I already asked but I think there are function to get keys and matching noise distributions from a ClientKey, @nsarlin-zama maybe you know/remember ?

in any case here it's correct but I prefer shared primitives for tricky code like this

from the client key you should be able to call the encryption_noise method. I don't know if in that case it will be the value you want. Or you could add a method on the parameters, for example for the ct modulus there is: ciphertext_modulus_for_key

---

tfhe/src/high_level_api/xof_key_set.rs line 297 at r2 (raw file):

Previously, IceTDrinker wrote…

@nsarlin-zama is this a case we have to manage this way ? I think there is a way to make it work without copying/cloning that key but it means changing the KSK to adapt it to 64 bits

What we did was to generate the ksk on 32b and then convert it to 64b. That way we copy the ksk but not the secret key.

Ideally I think this kind of thing should be a method on the ap key. If possible we should avoid having ap matches everywhere in the code base, and keep them in ap specific files. Maybe this should be added as one of the ap_ck.new_xxx_keyswitching_key_xxx in the ap client keys methods (with a potential small refacto if there is some code that could be factorized)

[IceTDrinker]

tfhe/src/core_crypto/algorithms/glwe_secret_key_generation.rs line 126 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

The Scalar element of the secret keys needs to be casted to u128/u64

I could maybe use the Scalar type to count itself, that could potentially lead to problems for secret keys of u8, which is not something we use.

If I use the Scalar type of the secret key I could use OverflowingAdd

Let's be on the safe side and not depend on the Scalar type being big enough

[tmontaigu]

The code should now be ok

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

---

tfhe/src/high_level_api/xof_key_set.rs line 259 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

from the client key you should be able to call the encryption_noise method. I don't know if in that case it will be the value you want. Or you could add a method on the parameters, for example for the ct modulus there is: ciphertext_modulus_for_key

encryption_noise is not what I want, but its close as I want the noise that matches the target get, I create a method on the ShortintParameterSet

[IceTDrinker]

tfhe/src/core_crypto/commons/parameters.rs line 419 at r4 (raw file):

pub struct MaxNormalizedHammingWeight(f64);

impl MaxNormalizedHammingWeight {

Still unconvinced by the name, the object represents a range/bounds

[IceTDrinker]

tfhe/src/core_crypto/commons/parameters.rs line 419 at r4 (raw file):

Previously, IceTDrinker wrote…

Still unconvinced by the name, the object represents a range/bounds

damn it had the wrong revision

[IceTDrinker]

@IceTDrinker reviewed 1 of 1 files at r2, 2 of 4 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mayeul-zama and @tmontaigu)

---

tfhe/src/high_level_api/xof_key_set.rs line 129 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

In the Tfhe.KeyGen from the paper, its not configurable

I agree, what I'm saying is that for e.g. rerand we have configurable primitives, I'm wondering if keeping that possibility by taking a XofSeed instead is not something we prefer ?

---

tfhe/src/high_level_api/xof_key_set.rs line 259 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

encryption_noise is not what I want, but its close as I want the noise that matches the target get, I create a method on the ShortintParameterSet

thanks for the change !

---

tfhe/src/high_level_api/xof_key_set.rs line 363 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I don't understand 🤔, you mean in the let integer_ksk_material = ... above, integer_ksk_material should already be Some(...) ?

yes, but if you prefer it like this because we always have the KSK to signal it it's fine, just a nit given others keys are options but don't show it

---

tfhe/src/high_level_api/xof_key_set.rs line 1898 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Nope, but I could add a quick From<&str> impl, there's one for u64

Could be nice yes, let's have it in a separate PR to avoid delaying this one too much

---

tfhe/src/high_level_api/xof_key_set.rs line 279 at r5 (raw file):

                        &mut encryption_rand_gen,
                    )
                    // match pk_to_sk_ksk_params.destination_key {

comments to remove I would think

---

tfhe/src/high_level_api/xof_key_set.rs line 2088 at r5 (raw file):

        let clear_b = rand::random::<u32>();

        {

just curious about the new addition to the test ?

[tmontaigu]

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

---

tfhe/src/high_level_api/xof_key_set.rs line 129 at r2 (raw file):

Previously, IceTDrinker wrote…

I agree, what I'm saying is that for e.g. rerand we have configurable primitives, I'm wondering if keeping that possibility by taking a XofSeed instead is not something we prefer ?

We can, it depends on much we stricly want to follow the tfhe.keygen
I have no opinion on this

---

tfhe/src/high_level_api/xof_key_set.rs line 2088 at r5 (raw file):

Previously, IceTDrinker wrote…

just curious about the new addition to the test ?

When changing what was copied during the generation of the ksk that does pk->sk for ks32 i forgot something, and computation test were not ok, So I added test to check that not using a pke was ok

I just then left them there

[IceTDrinker]

tfhe/src/high_level_api/xof_key_set.rs line 2088 at r5 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

When changing what was copied during the generation of the ksk that does pk->sk for ks32 i forgot something, and computation test were not ok, So I added test to check that not using a pke was ok

I just then left them there

perfect

[IceTDrinker]

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

---

tfhe/src/high_level_api/xof_key_set.rs line 129 at r2 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

We can, it depends on much we stricly want to follow the tfhe.keygen
I have no opinion on this

you can have the strict spec and the more manual one, the strict spec calling the manual one ?

---

tfhe/src/high_level_api/xof_key_set.rs line 279 at r5 (raw file):

Previously, IceTDrinker wrote…

comments to remove I would think

@tmontaigu still here something to check

[IceTDrinker]

Thanks a lot !

@IceTDrinker reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mayeul-zama)

[tmontaigu]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mayeul-zama)

---

tfhe/src/high_level_api/xof_key_set.rs line 279 at r5 (raw file):

Previously, IceTDrinker wrote…

comments to remove I would think

🧇