Ns/feat/noise squashed compression #2193
Conversation
nsarlin-zama
force-pushed
the
ns/feat/noise_squashed_compression
branch
3 times, most recently
from
7ca09fe to
c9d4f75
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 14 of 14 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker)
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
decomp_base_log: value.params.packing_ks_base_log, decomp_level_count: value.params.packing_ks_level, modulus_switch_noise_reduction_params: None,
Is it planned to change that later?
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 205 at r3 (raw file):
); noise_squashing_key.squash_ciphertext_noise(&packed, sks)
Don't we only support inputs with clean carries?
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
fn test_empty_list_compression() { let (cks, _) = gen_keys::<ShortintParameterSet>( TEST_PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M128.into(),
Why remove multi bit params in this test?
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
cks.new_compression_decompression_keys(&private_compression_key); for number_to_pack in [0, 1, 128] {
Why do we need to support the empty case ?
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
#[versionize(CompressedSquashedNoiseCiphertextListVersions)] pub struct CompressedSquashedNoiseCiphertextList { pub glwe_ciphertext_list: GlweCiphertextListOwned<u128>,
Don't we want to drop some bits of each integer to save some space?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker and @mayeul-zama)
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
Previously, mayeul-zama wrote…
Why remove multi bit params in this test?
this is a new test, the removal is only relative to a previous force push. I don't know if there is value to test the multibit case here since we do basically nothing
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
Previously, mayeul-zama wrote…
Don't we want to drop some bits of each integer to save some space?
there is no modswitch here, I don't think we can ?
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, mayeul-zama wrote…
Why do we need to support the empty case ?
before this pr this used to panic if the user submitted an empty list.
Even if this is a corner case, this behavior is unexpected and not documented. Since this is easy to support I think we should try to avoid the panic in that case.
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 205 at r3 (raw file):
Previously, mayeul-zama wrote…
Don't we only support inputs with clean carries?
This part comes from the noise squashing test in tfhe/src/shortint/noise_squashing/tests.rs. I think the idea is to pack a pair of ciphertexts using both carry and message space.
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
Previously, mayeul-zama wrote…
Is it planned to change that later?
No, the returned key here is only used for decryption, so there is no need to have modswitch noise reduction parameters.
The workflow here is:
- We start with 2 key pairs:
- (ns1, ns1_priv): NoiseSquashingKey, NoiseSquashingPrivateKey
- (comp, comp_priv): NoiseSquashingCompressionKey, NoiseSquashingCompressionPrivateKey
- We noise squash using ns1, at this point the ct can be decrypted using ns1_priv
- We compress using comp.
- There is no decompression key since it is not modswitched, we can directly unpack from the ct list
- ns1_priv cannot be used anymore since the ct have been keyswitched, but we can generate a decryption key ns2_priv from comp_priv
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker and @nsarlin-zama)
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
this is a new test, the removal is only relative to a previous force push. I don't know if there is value to test the multibit case here since we do basically nothing
Ok, you removed it from the first commit in the third one
I added it in d872747 but it's not very useful indeed
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
there is no modswitch here, I don't think we can ?
I would be surprised if we can't remove at least a few bits
As long as MS_noise is negligible compared to the compressed ct noise, it should be fine (but needs to be taken into account by the optimizer)
There's probably a tradeoff between cost, key size and compressed_ct size
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
before this pr this used to panic if the user submitted an empty list.
Even if this is a corner case, this behavior is unexpected and not documented. Since this is easy to support I think we should try to avoid the panic in that case.
I think that can create issues.
Putting random data in fields (by lack of good values to put) can break conformance for example.
For a user, having a panic in the empty case is not that surprising IMO but it should still be documented.
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 205 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
This part comes from the noise squashing test in
tfhe/src/shortint/noise_squashing/tests.rs. I think the idea is to pack a pair of ciphertexts using both carry and message space.
Ok
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
No, the returned key here is only used for decryption, so there is no need to have modswitch noise reduction parameters.
The workflow here is:
- We start with 2 key pairs:
- (ns1, ns1_priv): NoiseSquashingKey, NoiseSquashingPrivateKey
- (comp, comp_priv): NoiseSquashingCompressionKey, NoiseSquashingCompressionPrivateKey
- We noise squash using ns1, at this point the ct can be decrypted using ns1_priv
- We compress using comp.
- There is no decompression key since it is not modswitched, we can directly unpack from the ct list
- ns1_priv cannot be used anymore since the ct have been keyswitched, but we can generate a decryption key ns2_priv from comp_priv
Indeed without the MS, it's obviously useless
But I think a MS could be useful after the compression (as said in another comment)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker and @mayeul-zama)
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
Previously, mayeul-zama wrote…
Ok, you removed it from the first commit in the third one
I added it in d872747 but it's not very useful indeed
No this is not the same test, this test is a new one for empty lists :)
Unless I'm mistaken I haven't modified your test in this PR
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, mayeul-zama wrote…
I think that can create issues.
Putting random data in fields (by lack of good values to put) can break conformance for example.
For a user, having a panic in the empty case is not that surprising IMO but it should still be documented.
I don't know, I see this as an iterator-like behavior, so for me the empty case should be correctly handled as a limit, I don't see it like a special case.
Our data representation make it a bit special because we have useless metadata in that case but this is not something the user should have to care about.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker and @mayeul-zama)
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
Previously, mayeul-zama wrote…
I would be surprised if we can't remove at least a few bits
As long as MS_noise is negligible compared to the compressed ct noise, it should be fine (but needs to be taken into account by the optimizer)
There's probably a tradeoff between cost, key size and compressed_ct size
Yes maybe!
@IceTDrinker, what do you think?
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I don't know, I see this as an iterator-like behavior, so for me the empty case should be correctly handled as a limit, I don't see it like a special case.
Our data representation make it a bit special because we have useless metadata in that case but this is not something the user should have to care about.
For the conformance this is already handled because the check returns true if the list is empty (this was the case before this pr so it was somewhat expected). On the opposite the code that panics was an out of bound array access without message so it did not feel like a voluntary choice.
I agree that having crafted metadata is not ideal, but I am not sure it is worth redesigning the type either.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mayeul-zama)
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
No this is not the same test, this test is a new one for empty lists :)
Unless I'm mistaken I haven't modified your test in this PR
no multi bit parameters yet for testing the noise squashing (and therefore the compression coming with it)
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Yes maybe!
@IceTDrinker, what do you think?
Optimized circuit requires no mod switch at this time to keep the noise space for the noise flooding.
However it is likely that some form of modswitching could be tolerated, but R&D has not worked on that yet.
One thing to note though is that we can likely drop the bodies that contain no LWE data, when rebuilding the GLWE we would have 0s in their stead, given the blockchain will most likely pack one ciphertext (FheUint e.g.) per list it's a likely useful tradeoff, to be checked.
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
For the conformance this is already handled because the check returns true if the list is empty (this was the case before this pr so it was somewhat expected). On the opposite the code that panics was an out of bound array access without message so it did not feel like a voluntary choice.
I agree that having crafted metadata is not ideal, but I am not sure it is worth redesigning the type either.
this function could return a Result, or we can panic saying creating empty lists makes no sense, either way is fine with me
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 205 at r3 (raw file):
Previously, mayeul-zama wrote…
Ok
Nicolas is correct, we support packing before the noise squashing, this allows on average to divide by 2 the amount of PSB 128 to compute
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
Previously, mayeul-zama wrote…
Indeed without the MS, it's obviously useless
But I think a MS could be useful after the compression (as said in another comment)
hmm, I think I had a SecretEncryptionKeyView for a similar case, in practice the decryption needs much less data to be able to decrypt
So here the returned ciphertext type is the same as the input type, while I follow the logic not sure it serves us well in that case
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mayeul-zama)
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
hmm, I think I had a SecretEncryptionKeyView for a similar case, in practice the decryption needs much less data to be able to decrypt
So here the returned ciphertext type is the same as the input type, while I follow the logic not sure it serves us well in that case
otherwise we could decrypt a range of slots from the list itself, avoids shenanigans with key conversions, wdyt ?
as this compression is a one way path/trap door
There was a problem hiding this comment.
Haven't reviewed everything yet, weekend awaits, but generally from what I'm seeing, looks pretty good
Reviewed 1 of 1 files at r2, 9 of 14 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mayeul-zama)
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 87 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
Optimized circuit requires no mod switch at this time to keep the noise space for the noise flooding.
However it is likely that some form of modswitching could be tolerated, but R&D has not worked on that yet.
One thing to note though is that we can likely drop the bodies that contain no LWE data, when rebuilding the GLWE we would have 0s in their stead, given the blockchain will most likely pack one ciphertext (FheUint e.g.) per list it's a likely useful tradeoff, to be checked.
maybe use the packed integers on full u128, this will save the last few bodies from being put there, and would allow for an easy transition for mod switch if we ever do that (one of my current doubts is that blockchain is unsure on whether they are going to use thist, which makes me think @nsarlin-zama if you can get some compression numbers at the shortint level for 1 to 128 blocks for every power of 2
so 1, 2, 4, etc. 128, will give us all compression timings for 4 bits up to 512 (given we can pack two ciphertexts per slot)
tfhe/src/shortint/list_compression/private_key.rs line 57 at r3 (raw file):
#[versionize(NoiseSquashingCompressionPrivateKeyVersions)] pub struct NoiseSquashingCompressionPrivateKey { pub post_packing_ks_key: GlweSecretKeyOwned<u128>,
the name does not seem fitting, post_packing_secret_key ?
also if you don't need the fields to be pub, don't hesitate to restrict the visibility, let's try to make shortint a bit less open, as it's more a legacy than a conscious choice
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 85 at r3 (raw file):
} let list = LweCiphertextList::from_container(list, lwe_size, ciphertext_modulus);
do we have a backlog issue to manage iterators at the core level for some primitives ? this is janky, though I know it's done this way elsewhere, it does not have to be so painful (not required to move forward with this PR)
tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 136 at r3 (raw file):
decomp_level_count: DecompositionLevelCount(2), modulus_switch_noise_reduction_params: Some(ModulusSwitchNoiseReductionParams { modulus_switch_zeros_count: LweCiphertextCount(19),
This does not seem right 🤔 I'll have to recheck on how parameters were generated
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 106 at r3 (raw file):
pub fn unpack(&self, index: usize) -> Result<SquashedNoiseCiphertext, crate::Error> { if index >= self.count.0 { return Err(crate::Error::new(format!(
you can use crate::error! instead of crate::Error::new(format!(
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @IceTDrinker and @mayeul-zama)
tfhe/src/integer/ciphertext/compressed_ciphertext_list.rs line 176 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
no multi bit parameters yet for testing the noise squashing (and therefore the compression coming with it)
Maybe reviewable makes it hard to see but this test is not about noise squashing :)
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 106 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
you can use crate::error! instead of crate::Error::new(format!(
Thanks, I didn't know about this
tfhe/src/shortint/list_compression/compression.rs line 290 at r1 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
this function could return a Result, or we can panic saying creating empty lists makes no sense, either way is fine with me
IMO a panic is a bad idea, because I think that we should try to avoid panic if the parameters are correct and inputs are checked with conformance (here it panics with both conditions checked). This would allow to draw a clear line between what is a bug and what is not.
Even if it is better, I think this should not return an error because there is nothing fundamentally erroneous about an empty list. At worse this is unexpected if we only think of use-cases where the list is built manually. But I think in most cases it is built from arguments coming from another function and if we consider it an error we put the burden of handling this error to the calling function (this is why a panic here is especially bad because they are contagious).
The empty list is expected to be a valid object even for us (as seen in the conformance implementation:
), so I think user will expect it to be valid too.tfhe/src/shortint/list_compression/noise_squashing_compression.rs line 85 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
do we have a backlog issue to manage iterators at the core level for some primitives ? this is janky, though I know it's done this way elsewhere, it does not have to be so painful (not required to move forward with this PR)
I did not find any issue about this, but yeah this would be nice to have indeed.
tfhe/src/shortint/list_compression/private_key.rs line 57 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
the name does not seem fitting, post_packing_secret_key ?
also if you don't need the fields to be pub, don't hesitate to restrict the visibility, let's try to make shortint a bit less open, as it's more a legacy than a conscious choice
This type is copy pasted from the "normal" CompressionPrivateKey, should I take the opportunity to fix this too ?
tfhe/src/shortint/noise_squashing/private_key.rs line 82 at r3 (raw file):
Previously, IceTDrinker (Arthur Meyre) wrote…
otherwise we could decrypt a range of slots from the list itself, avoids shenanigans with key conversions, wdyt ?
as this compression is a one way path/trap door
Yes I can do this, I agree that it would be better to remove the conversion
nsarlin-zama
force-pushed
the
ns/feat/noise_squashed_compression
branch
from
c9d4f75 to
7aa00b8
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 16 files reviewed, 8 unresolved discussions (waiting on @IceTDrinker and @mayeul-zama)
tfhe/src/shortint/ciphertext/compressed_ciphertext_list.rs line 106 at r3 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Thanks, I didn't know about this
Actually I can't because this macro is only available with the integer feature
nsarlin-zama
force-pushed
the
ns/feat/noise_squashed_compression
branch
from
7aa00b8 to
b544192
Compare
There was a problem hiding this comment.
Reviewed 5 of 14 files at r4, 14 of 14 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @IceTDrinker and @nsarlin-zama)
nsarlin-zama
force-pushed
the
ns/feat/noise_squashed_compression
branch
from
b544192 to
029a062
Compare
| let mask = if log_modulus < Scalar::BITS { | ||
| (Scalar::ONE << log_modulus) - Scalar::ONE | ||
| } else { | ||
| Scalar::MAX |
There was a problem hiding this comment.
Maybe assert_eq!(log_modulus, Scalar::BITS) here
closes: please link all relevant issues
PR content/description
Adds GLWE compression of squashed noise ciphertexts. This is similar to the classical shortint compression except that the resulting ciphertexts are not modswitched.
The following types have been added:
storage_log_modulusbecause the ciphertexts are not modswitchedunpackmethod to extract a single SquashedNoiseCiphertextCheck-list:
This change is