feat(tfhe): add safe deserialiation #572

mayeul-zama · 2023-09-20T11:39:11Z

Some tests will need to be added

github-actions · 2023-09-20T11:39:27Z

@slab-ci cpu_fast_test

IceTDrinker · 2023-09-20T12:02:46Z

please don't request reviews on draft PRs :) reduces the inbox github noise

github-actions · 2023-09-22T10:07:33Z

@slab-ci cpu_fast_test

tfhe/Cargo.toml

tfhe/src/safe_serialization.rs

tfhe/src/shortint/ciphertext/mod.rs

github-actions · 2023-09-27T14:22:51Z

@slab-ci cpu_fast_test

github-actions · 2023-09-27T14:52:24Z

@slab-ci cpu_fast_test

IceTDrinker

thanks !

tfhe/src/core_crypto/commons/parameters.rs

tfhe/src/conformance.rs

tfhe/src/safe_serialization.rs

tfhe/src/shortint/parameters/mod.rs

tfhe/src/integer/ciphertext/mod.rs

tfhe/src/core_crypto/entities/lwe_compact_ciphertext_list.rs

github-actions · 2023-09-28T13:26:19Z

@slab-ci cpu_fast_test

github-actions · 2023-09-28T13:26:48Z

@slab-ci cpu_fast_test

github-actions · 2023-09-28T13:39:39Z

@slab-ci cpu_fast_test

github-actions · 2023-09-28T15:30:15Z

@slab-ci cpu_fast_test

github-actions · 2023-09-29T14:27:18Z

@slab-ci cpu_fast_test

IceTDrinker

First batch of comment on the updated PR, haven't checked the tests for now

tfhe/src/shortint/parameters/mod.rs

tfhe/src/conformance.rs

tfhe/src/core_crypto/algorithms/glwe_sample_extraction.rs

tfhe/src/core_crypto/fft_impl/fft64/crypto/wop_pbs/mod.rs

tfhe/src/core_crypto/fft_impl/fft64/crypto/wop_pbs/tests.rs

IceTDrinker · 2023-09-29T15:16:26Z

tfhe/src/lib.rs

+
+pub mod conformance;
+
+pub trait Named {


I think I would prefer not having anything other than modules and imports in the root lib.rs, not sure if there are some conventions around that in rust

I can create a module named but I don't see the benefit as it would only be 3 lines long

could it be part of the safe_deserialization ? as it's used by it ?

Sure, but it forces the impl Named to be behing a #[cfg(safe-deserialization)] which is why I put it out

hmm how bad does it complicate things ?

It's not too bad

Would https://doc.rust-lang.org/std/any/fn.type_name.html be an alternative to having a custom Named trait ?

from the page you linked

This is intended for diagnostic use. The exact contents and format of the string returned are not specified,

it was the first solution but did not seem robust

Yeah, and also just moving the struct from one module to another would change the string thus breaking serialization

tfhe/src/shortint/ciphertext/mod.rs

tfhe/src/safe_serialization.rs

tfhe/src/shortint/ciphertext/mod.rs

tfhe/src/high_level_api/integers/types/compact.rs

tfhe/src/core_crypto/entities/lwe_ciphertext.rs

tfhe/src/integer/ciphertext/mod.rs

IceTDrinker · 2023-09-29T15:52:35Z

tfhe/src/high_level_api/integers/types/compact.rs

+    use super::*;
+    use crate::conformance::ParameterSetConformant;
+    use crate::core_crypto::prelude::UnsignedInteger;
+    use crate::shortint::parameters::PARAM_MESSAGE_2_CARRY_2_KS_PBS;


let's plan adding more tests later down the line using the create_parameterized_test pattern from elsewhere in the crate

I don't really see the benefit to hav this test with many parameter sets (except if they increase coverage, for exemple by using a non native modulus)

it's just a matter of completeness :)

plus the macro instantiation of tests is really not that bad

github-actions · 2023-10-03T08:37:27Z

@slab-ci cpu_fast_test

IceTDrinker

Nearly there, we are down to the last details, thanks a lot for all the work up to this point !

tfhe/src/core_crypto/algorithms/glwe_sample_extraction.rs

tfhe/src/core_crypto/algorithms/misc.rs

tfhe/src/high_level_api/integers/types/compact.rs

IceTDrinker · 2023-10-03T09:17:46Z

tfhe/src/high_level_api/integers/types/compact.rs

+    use super::*;
+    use crate::conformance::ParameterSetConformant;
+    use crate::core_crypto::prelude::UnsignedInteger;
+    use crate::shortint::parameters::PARAM_MESSAGE_2_CARRY_2_KS_PBS;


it's just a matter of completeness :)

plus the macro instantiation of tests is really not that bad

tfhe/src/high_level_api/integers/types/compact.rs

tfhe/src/safe_deserialization.rs

tfhe/src/shortint/ciphertext/mod.rs

github-actions · 2023-10-03T17:10:21Z

@slab-ci cpu_fast_test

IceTDrinker · 2023-10-03T17:30:47Z

btw have you made progress on having the HL part in the C API ?

tmontaigu · 2023-10-04T08:47:39Z

tfhe/src/safe_deserialization.rs

+    options
+        .with_limit(VERSION_LENGTH_LIMIT)
+        .serialize_into::<_, String>(&mut writer, &SERIALIZATION_VERSION.to_owned())?;
+
+    options
+        .with_limit(TYPE_NAME_LENGTH_LIMIT)
+        .serialize_into::<_, String>(&mut writer, &T::NAME.to_owned())?;


Do we really need these .to_owned() ? passign the &'static str should work

For deserialization, I think we have to use the String type
I'm not sure a serialized &str is guaranted to be deserializable as a String which is why I did it this way

github-actions · 2023-10-04T11:30:48Z

@slab-ci cpu_fast_test

github-actions · 2023-10-04T13:06:06Z

@slab-ci cpu_fast_test

github-actions · 2023-10-04T15:01:53Z

@slab-ci cpu_fast_test

tfhe/src/conformance.rs

IceTDrinker

All right let's start with this, thanks a lot for the work !

github-actions · 2023-10-04T15:10:29Z

Pull Request has been approved 🎉
Launching full test suite...
@slab-ci cpu_test
@slab-ci cpu_integer_test
@slab-ci cpu_multi_bit_test
@slab-ci cpu_wasm_test
@slab-ci csprng_randomness_testing

cla-bot bot added the cla-signed label Sep 20, 2023

mayeul-zama requested a review from IceTDrinker September 20, 2023 11:39

mayeul-zama force-pushed the mz/safe_deserialization branch from 63cabe6 to 03bf39d Compare September 22, 2023 10:07

IceTDrinker reviewed Sep 22, 2023

View reviewed changes

tfhe/Cargo.toml Outdated Show resolved Hide resolved

IceTDrinker reviewed Sep 22, 2023

View reviewed changes

tfhe/src/safe_serialization.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Sep 22, 2023

View reviewed changes

tfhe/src/shortint/ciphertext/mod.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Sep 22, 2023

View reviewed changes

tfhe/src/shortint/ciphertext/mod.rs Outdated Show resolved Hide resolved

mayeul-zama force-pushed the mz/safe_deserialization branch from 03bf39d to aa5a479 Compare September 27, 2023 14:22

mayeul-zama force-pushed the mz/safe_deserialization branch from aa5a479 to 7896560 Compare September 27, 2023 14:52

IceTDrinker reviewed Sep 27, 2023

View reviewed changes

mayeul-zama commented Sep 28, 2023

View reviewed changes

tfhe/src/core_crypto/entities/lwe_compact_ciphertext_list.rs Outdated Show resolved Hide resolved

mayeul-zama force-pushed the mz/safe_deserialization branch from 7896560 to ce3ed52 Compare September 28, 2023 13:26

mayeul-zama force-pushed the mz/safe_deserialization branch from ce3ed52 to 665f7ce Compare September 28, 2023 13:26

mayeul-zama force-pushed the mz/safe_deserialization branch from 1a34ad4 to e121038 Compare September 28, 2023 15:29

mayeul-zama force-pushed the mz/safe_deserialization branch from e121038 to 571d1ee Compare September 29, 2023 14:27

IceTDrinker reviewed Sep 29, 2023

View reviewed changes

tfhe/src/core_crypto/entities/lwe_ciphertext.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Sep 29, 2023

View reviewed changes

tfhe/src/integer/ciphertext/mod.rs Outdated Show resolved Hide resolved

IceTDrinker reviewed Sep 29, 2023

View reviewed changes

mayeul-zama force-pushed the mz/safe_deserialization branch from 410e8f0 to f9d5a4b Compare October 3, 2023 08:37

mayeul-zama requested review from tmontaigu and IceTDrinker October 3, 2023 08:55

IceTDrinker reviewed Oct 3, 2023

View reviewed changes

mayeul-zama force-pushed the mz/safe_deserialization branch from f9d5a4b to ec9df18 Compare October 3, 2023 17:10

tmontaigu reviewed Oct 4, 2023

View reviewed changes

mayeul-zama force-pushed the mz/safe_deserialization branch from ec9df18 to 4531f3a Compare October 4, 2023 11:30

mayeul-zama added 2 commits October 4, 2023 15:05

chore(test): add type hint for rust-analyzer

a8337bc

feat(core): add to_equivalent_lwe_dimension

10dd007

mayeul-zama force-pushed the mz/safe_deserialization branch from 4531f3a to fe48db0 Compare October 4, 2023 13:05

mayeul-zama added 5 commits October 4, 2023 16:05

feat(global): add safe deserialization

0ad253e

feat(global): impl ParameterSetConformant for ciphertexts

18d6bd4

test(hlapi): test compact integer conformance

076cc78

test(serialization): add serialization test

d614095

test(serialization): run tests in CI

57f954d

mayeul-zama force-pushed the mz/safe_deserialization branch from fe48db0 to 57f954d Compare October 4, 2023 15:01

IceTDrinker reviewed Oct 4, 2023

View reviewed changes

tfhe/src/conformance.rs Show resolved Hide resolved

IceTDrinker approved these changes Oct 4, 2023

View reviewed changes

github-actions bot added the approved label Oct 4, 2023

mayeul-zama requested review from tmontaigu and IceTDrinker October 4, 2023 15:14

mayeul-zama merged commit 492d348 into main Oct 5, 2023
20 checks passed

mayeul-zama deleted the mz/safe_deserialization branch October 5, 2023 07:15

feat(tfhe): add safe deserialiation #572

feat(tfhe): add safe deserialiation #572

Conversation

mayeul-zama commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

IceTDrinker commented Sep 20, 2023

github-actions bot commented Sep 22, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 29, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2023

IceTDrinker commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023

IceTDrinker left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2023