Am/chore/various preparatory changes

[IceTDrinker]

Those are various small changes/fixes found/needed for the incoming integer/HL noise squashing

---

This change is 

[mayeul-zama]

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 7 of 7 files at r4, 1 of 1 files at r5, 7 of 7 files at r6, 8 of 8 files at r7, 3 of 3 files at r8, 11 of 11 files at r9, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nsarlin-zama and @tmontaigu)

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

{
    if num_bits_set >= T::BITS as u32 {
        return unpadded_value;

In the case > shouldn't we abort or return an error?

---

tfhe/src/integer/block_decomposition.rs line 147 at r9 (raw file):

    // are 1s if sign bit is `1` else `0`
    let padding = (T::MAX * sign_bit) << num_bits_set;
    padding | unpadded_value

I guess we could simply do a << followed by a signed >> here instead

---

tfhe/src/integer/block_decomposition.rs line 388 at r9 (raw file):

        for limb in input {
            if !recomposer.add_unmasked(limb) {
                break;

Shouldn't we return an error in that case?

---

tfhe/src/shortint/noise_squashing/server_key.rs line 112 at r8 (raw file):

        let ct_noise_level = ciphertext.noise_level();
        assert!(
            ct_noise_level.get() > src_server_key.max_noise_level.get(),

Don't we require that the input have a noise level of 1 (or 0)?

---

tfhe/src/high_level_api/integers/signed/mod.rs line 17 at r6 (raw file):

pub(in crate::high_level_api) use compressed::CompressedSignedRadixCiphertext;
pub(in crate::high_level_api) use inner::{
    SignedRadixCiphertext, SignedRadixCiphertextVersionOwned,

Should be part of a previous commit

---

tfhe/src/high_level_api/integers/signed/base.rs line 113 at r6 (raw file):

        ciphertext: impl Into<SignedRadixCiphertext>,
        tag: Tag,
    ) -> Self {

Should be part of a previous commit

---

tfhe/src/shortint/noise_squashing/private_key.rs line 63 at r3 (raw file):

impl ClientKey {
    pub fn new_noise_squashing_private_key(

Do we need this function as a method of ClientKey?

[IceTDrinker]

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

---

tfhe/src/high_level_api/integers/signed/base.rs line 113 at r6 (raw file):

Previously, mayeul-zama wrote…

Should be part of a previous commit

you mean this is commited in the wrong commit ?

---

tfhe/src/high_level_api/integers/signed/mod.rs line 17 at r6 (raw file):

Previously, mayeul-zama wrote…

Should be part of a previous commit

same question

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

Previously, mayeul-zama wrote…

In the case > shouldn't we abort or return an error?

Same thing not my code

---

tfhe/src/integer/block_decomposition.rs line 147 at r9 (raw file):

Previously, mayeul-zama wrote…

I guess we could simply do a << followed by a signed >> here instead

Probably, which makes me think there may be a problem with the sign extend in the first place, it's not code from me, I moved it here

---

tfhe/src/integer/block_decomposition.rs line 388 at r9 (raw file):

Previously, mayeul-zama wrote…

Shouldn't we return an error in that case?

not my code, unclear to me

---

tfhe/src/shortint/noise_squashing/private_key.rs line 63 at r3 (raw file):

Previously, mayeul-zama wrote…

Do we need this function as a method of ClientKey?

was following the list compression pattern, though in the case of the compression it's to check something on the key itself, so I guess can be removed

---

tfhe/src/shortint/noise_squashing/server_key.rs line 112 at r8 (raw file):

Previously, mayeul-zama wrote…

Don't we require that the input have a noise level of 1 (or 0)?

so the assert itself is wrong, but we manage packing for the big PBS so we accept the max noise level as the input ciphertext can be packed

[IceTDrinker]

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

---

tfhe/src/high_level_api/integers/signed/base.rs line 113 at r6 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

you mean this is commited in the wrong commit ?

found the commit

[IceTDrinker]

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

---

tfhe/src/integer/block_decomposition.rs line 388 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

not my code, unclear to me

ok so this is to mimick the as cast from reading other comments, like u32 as u16 will drop the higher bits, so once enough were filled no need to add anymore let me know if I'm in the right @tmontaigu

[IceTDrinker]

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

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Same thing not my code

same thing as the other when doing a cast that truncates high bits would be discarded

[IceTDrinker]

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

---

tfhe/src/high_level_api/integers/signed/base.rs line 113 at r6 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

found the commit

Done

---

tfhe/src/high_level_api/integers/signed/mod.rs line 17 at r6 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

same question

Done

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

same thing as the other when doing a cast that truncates high bits would be discarded

So this behaves as expected according to the docstring

---

tfhe/src/integer/block_decomposition.rs line 147 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Probably, which makes me think there may be a problem with the sign extend in the first place, it's not code from me, I moved it here

Done thanks!

---

tfhe/src/shortint/noise_squashing/private_key.rs line 63 at r3 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

was following the list compression pattern, though in the case of the compression it's to check something on the key itself, so I guess can be removed

done

---

tfhe/src/shortint/noise_squashing/server_key.rs line 112 at r8 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

so the assert itself is wrong, but we manage packing for the big PBS so we accept the max noise level as the input ciphertext can be packed

assert updated and using the proper MaxNoiseLevel primitive which I forgot about

[mayeul-zama]

Reviewed 32 of 32 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IceTDrinker, @nsarlin-zama, and @tmontaigu)

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

So this behaves as expected according to the docstring

Indeed
But the name is extend so this case is probably a misuse
Panicking is safer IMO

---

tfhe/src/integer/block_decomposition.rs line 147 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

Done thanks!

Is T guaranteed to be a signed type?

[IceTDrinker]

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

---

tfhe/src/integer/block_decomposition.rs line 138 at r9 (raw file):

Previously, mayeul-zama wrote…

Indeed
But the name is extend so this case is probably a misuse
Panicking is safer IMO

well it's a sign extend which is a no op if bigger than the destination type

see this playground to see how this works : https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=fd95b097a46bd461749534beed8c2dec on usual types when truncating early

---

tfhe/src/integer/block_decomposition.rs line 147 at r9 (raw file):

Previously, mayeul-zama wrote…

Is T guaranteed to be a signed type?

yes the trait SignExtendable requires SignedNumric

[tmontaigu]

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

---

tfhe/src/integer/block_decomposition.rs line 388 at r9 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

ok so this is to mimick the as cast from reading other comments, like u32 as u16 will drop the higher bits, so once enough were filled no need to add anymore let me know if I'm in the right @tmontaigu

Yes returning an error is not what I inteded as it's meant to be like a as cast, if you add block beyond T::BITS they are ignored (just as as truncates), the return value is just there to allow caller to know when they can stopcalling add is it would be poinless

---

tfhe/src/integer/block_decomposition.rs line 98 at r11 (raw file):

    + std::ops::BitOr<Self, Output = Self>
    + std::ops::Mul<Self, Output = Self>
    + CastFrom<Self>

This CastFrom bound looks weird, I don't think its needed

[nsarlin-zama]

Reviewed 1 of 2 files at r2, 32 of 32 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mayeul-zama)

[IceTDrinker]

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

---

tfhe/src/integer/block_decomposition.rs line 98 at r11 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

This CastFrom bound looks weird, I don't think its needed

ok, it was moved there unchanged, can update

[nsarlin-zama]

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

---

tfhe/src/integer/block_decomposition.rs line 98 at r11 (raw file):

Previously, IceTDrinker (Arthur Meyre) wrote…

ok, it was moved there unchanged, can update

Not related to this PR but I think it would be nice to have impl<T> CastFrom<T> for T to remove all the CastFrom<Self> but at the moment it is not compatible with the macro that derives it for builtin integer types.

[IceTDrinker]

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

---

tfhe/src/integer/block_decomposition.rs line 98 at r11 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

Not related to this PR but I think it would be nice to have impl<T> CastFrom<T> for T to remove all the CastFrom<Self> but at the moment it is not compatible with the macro that derives it for builtin integer types.

tried that, but for some reasons linked to how some stuff is implemented I ran into the "multiple implementations" problem at some point

[mayeul-zama]

Thanks!

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