feat(hlapi): add tag system #1468

tmontaigu · 2024-08-12T11:38:35Z

Tag

The Tag allows to store bytes alongside of entities (keys, and ciphertext) the main purpose of this system is to tag / identify ciphertext with their keys.

When encrypted, a ciphertext gets the tag of the key used to encrypt it.
Ciphertexts resulting from operations (add, sub, etc.) get the tag from the ServerKey used
PublicKey gets its tag from the ClientKey that was used to create it
ServerKey gets its tag from the ClientKey that was used to create it

User can change the tag of any entities at any point.

BREAKING CHANGE: Many of the into_raw_parts and from_raw_parts changed to accommodate the addition of the `tag``

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

tfhe/src/high_level_api/tests/tags_on_entities.rs

IceTDrinker · 2024-08-13T13:21:36Z

tests are red at the moment

tfhe/src/high_level_api/tag.rs

nsarlin-zama

nice API !

tfhe/src/high_level_api/keys/public.rs

tfhe/src/high_level_api/keys/client.rs

tfhe/src/high_level_api/tag.rs

tfhe/src/high_level_api/global_state.rs

IceTDrinker

I think the SmallVec is too smart for what we are trying to do (there may even be issues depending on the wasm vec size) I think an Option for the tag should be enough, not bigger than a vec https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c0cf100d311d5780007cbf38a26f4515

and allows to have an empty tag that has smaller overhead than the SmallVec which needs to be initialized with 0s and then copied around even if it's morally empty

tfhe/src/high_level_api/tag.rs

IceTDrinker · 2024-08-14T09:08:39Z

tfhe/src/high_level_api/tag.rs

+use crate::high_level_api::backward_compatibility::tag::TagVersions;
+use tfhe_versionable::{Unversionize, UnversionizeError, Versionize, VersionizeOwned};
+
+const STACK_ARRAY_SIZE: usize = std::mem::size_of::<Vec<u8>>() - 1;


this may not be constant across compiler versions and perhaps even platforms/hw ?

could be an issue with WASM as well ?

This would just mean that if for example sizeof::Vec is 3*32 = 96, then, a 128 bit number would be stored in a Vec rather than a the stack.

So when creating, no problem, when deserializing no problem either, and if a 64 bit CPU deserialize that, as it would fit on stack, the 128 bits would live on the stack

all right I need to check the details of the small vec stuff then

That's why it's important to convert it to a plain array before serialization (as you did)

tfhe/src/high_level_api/tag.rs

IceTDrinker · 2024-08-14T09:10:31Z

tfhe/src/high_level_api/tag.rs

+
+const STACK_ARRAY_SIZE: usize = std::mem::size_of::<Vec<u8>>() - 1;
+
+/// Simple short optimized vec, where if the data is small enough


I feel like the SmallVec optimization might be premature and has other implications that cause more trouble at the moment than it solves

What other implications ?

the size, compatibility of compiler versions, the fact the default case copies an array of 0s around

just like with just a Vec, the compiler would copy the bytes of the internal data (ptr, size, capacity) of the vec

The SmallVec size is 8 bytes (likely sizeof::<usize>) bigger than a plain Vec

I do agree that the SmallVec thing might be premature, but the complexity of it is not that high I think

Also, the vec of the Tag which only gets and return bytes or simple u64/u128 and the fact that the serialization/versioning returns bytes protects us in a sense.

IceTDrinker · 2024-08-14T09:22:40Z

tfhe/src/high_level_api/compact_list.rs

 #[cfg(feature = "zk-pok")]
 #[derive(Clone, Serialize, Deserialize)]
-pub struct ProvenCompactCiphertextList(crate::integer::ciphertext::ProvenCompactCiphertextList);
+pub struct ProvenCompactCiphertextList {
+    inner: crate::integer::ciphertext::ProvenCompactCiphertextList,
+    tag: Tag,
+}
+
+#[cfg(feature = "zk-pok")]
+impl Tagged for ProvenCompactCiphertextList {


should we create a zk mod here and have a feature gate on it + a pub re export ?

allows to limit the number of #cfg and I find it less error prone

tfhe/src/high_level_api/integers/unsigned/compressed.rs

tfhe/src/high_level_api/integers/unsigned/scalar_ops.rs

IceTDrinker

last comments on small vec + tests

IceTDrinker · 2024-08-14T14:43:52Z

tfhe/src/high_level_api/tests/tags_on_entities.rs

+    use crate::ProvenCompactCiphertextList;
+
+    let config =
+        ConfigBuilder::with_custom_parameters(PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M64).build();


to mimich the blockchain case better could we use the dedicated PK params + casting params ?

I will try to add it, but since the tag is for the whole key set it won't change how it behaves

It’s to make sure it gets from the public key through the keyswitch

I don't get it

what I mean is for the BC use case we have

PKE -> KS -> PBS

and we want to make sure the tagging works well in that scenario

IceTDrinker · 2024-08-14T14:47:20Z

tfhe/src/high_level_api/tag.rs

+                    bytes: r_bytes,
+                    len: r_len,
+                },
+            ) => l_vec == &r_bytes[..usize::from(*r_len)],


wondering if a short circuit by checking the l_vec.len() == r_len is worth it here and below

IceTDrinker · 2024-08-14T14:49:00Z

tfhe/src/high_level_api/tag.rs

+        }
+    }
+
+    pub fn as_u64(&self) -> u64 {


as_le_u64 I would suggest then, or docstring that makes it clear how the data is interpreted and the truncating behavior of the as_u64

IceTDrinker · 2024-08-14T14:49:12Z

tfhe/src/high_level_api/tag.rs

+        u64::from_le_bytes(le_bytes)
+    }
+
+    pub fn as_u128(&self) -> u128 {


same thing for the endianness

tfhe/src/high_level_api/tag.rs

IceTDrinker · 2024-08-14T14:51:43Z

tfhe/src/high_level_api/tag.rs

+    pub fn set_u64(&mut self, value: u64) {
+        let le_bytes = value.to_le_bytes();
+        self.set_data(le_bytes.as_slice());
+    }
+
+    pub fn set_u128(&mut self, value: u128) {
+        let le_bytes = value.to_le_bytes();
+        self.set_data(le_bytes.as_slice());
+    }
+


info about endianness

IceTDrinker · 2024-08-14T14:55:54Z

tfhe/src/high_level_api/tag.rs

+
+/// Tag
+///
+/// The `Tag` allows to store bytes alongside of entities (keys, and ciphertext)


alongside of -> alongside
keys, and ciphertexts

(missing s)

IceTDrinker · 2024-08-14T14:58:07Z

tfhe/src/high_level_api/tag.rs

+/// The `Tag` allows to store bytes alongside of entities (keys, and ciphertext)
+/// the main purpose of this system is to `tag` / identify ciphertext with their keys.
+///
+/// tfhe-rs does not interpret or check this data, it only stores it and passes it around


TFHE-rs I think is the accepted spelling for public resources

IceTDrinker

minor things, next one should be the one to merge

IceTDrinker · 2024-08-29T12:45:28Z

tfhe/src/high_level_api/backward_compatibility/tag.rs

-#[derive(VersionsDispatch)]
-pub(in crate::high_level_api) enum SmallVecVersions {
-    #[allow(unused)] // Unused because V1 does not exists yet
-    V0(SmallVec),
-}
-


no Versions for the SmallVec ?

IceTDrinker · 2024-08-29T12:49:32Z

tfhe/src/high_level_api/tag.rs

 /// the main purpose of this system is to `tag` / identify ciphertext with their keys.
 ///
+/// TFHE-RS does not interpret or check this data, it only stores it and passes it around


missing only this I believe and go for merge

Tag The `Tag` allows to store bytes alongside of entities (keys, and ciphertext) the main purpose of this system is to `tag` / identify ciphertext with their keys. * When encrypted, a ciphertext gets the tag of the key used to encrypt it. * Ciphertexts resulting from operations (add, sub, etc.) get the tag from the ServerKey used * PublicKey gets its tag from the ClientKey that was used to create it * ServerKey gets its tag from the ClientKey that was used to create it User can change the tag of any entities at any point. BREAKING CHANGE: Many of the into_raw_parts and from_raw_parts changed to accommodate the addition of the `tag``

IceTDrinker

Thanks a lot !

cla-bot bot added the cla-signed label Aug 12, 2024

tmontaigu requested review from IceTDrinker and nsarlin-zama August 12, 2024 11:38

tmontaigu commented Aug 12, 2024

View reviewed changes

tfhe/src/high_level_api/tests/tags_on_entities.rs Outdated Show resolved Hide resolved

tmontaigu force-pushed the tm/tag-system branch 3 times, most recently from 35efd30 to bc4cd3d Compare August 13, 2024 11:02

nsarlin-zama reviewed Aug 13, 2024

View reviewed changes

tfhe/src/high_level_api/tag.rs Outdated Show resolved Hide resolved

nsarlin-zama reviewed Aug 13, 2024

View reviewed changes

tfhe/src/high_level_api/tag.rs Show resolved Hide resolved

nsarlin-zama reviewed Aug 13, 2024

View reviewed changes

tmontaigu force-pushed the tm/tag-system branch from ec5fe0e to 6bba729 Compare August 14, 2024 07:55

IceTDrinker reviewed Aug 14, 2024

View reviewed changes

tfhe/src/high_level_api/global_state.rs Show resolved Hide resolved

IceTDrinker reviewed Aug 14, 2024

View reviewed changes

tmontaigu force-pushed the tm/tag-system branch from 6bba729 to 2dc3ede Compare August 14, 2024 11:56

IceTDrinker reviewed Aug 14, 2024

View reviewed changes

tmontaigu force-pushed the tm/tag-system branch 2 times, most recently from 44c6291 to cd89af4 Compare August 29, 2024 10:13

IceTDrinker reviewed Aug 29, 2024

View reviewed changes

tmontaigu force-pushed the tm/tag-system branch 3 times, most recently from da069ad to c4228e5 Compare September 2, 2024 09:15

tmontaigu force-pushed the tm/tag-system branch from c4228e5 to 44ec753 Compare September 4, 2024 09:09

IceTDrinker approved these changes Sep 4, 2024

View reviewed changes

zama-bot added the approved label Sep 4, 2024

tmontaigu merged commit 426f3bd into main Sep 5, 2024
89 checks passed

tmontaigu deleted the tm/tag-system branch September 5, 2024 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hlapi): add tag system #1468

feat(hlapi): add tag system #1468

tmontaigu commented Aug 12, 2024

IceTDrinker commented Aug 13, 2024

nsarlin-zama left a comment

IceTDrinker left a comment

IceTDrinker Aug 14, 2024

tmontaigu Aug 14, 2024

IceTDrinker Aug 14, 2024

nsarlin-zama Aug 14, 2024

IceTDrinker Aug 14, 2024

tmontaigu Aug 14, 2024

IceTDrinker Aug 14, 2024

tmontaigu Aug 14, 2024

IceTDrinker Aug 14, 2024

tmontaigu Aug 14, 2024 •

edited

Loading

tmontaigu Aug 14, 2024

IceTDrinker Aug 14, 2024

IceTDrinker left a comment

IceTDrinker Aug 14, 2024

tmontaigu Aug 16, 2024

IceTDrinker Aug 16, 2024

tmontaigu Aug 16, 2024

IceTDrinker Aug 16, 2024

tmontaigu Aug 16, 2024

IceTDrinker Aug 14, 2024

IceTDrinker Aug 14, 2024

IceTDrinker Aug 14, 2024

IceTDrinker Aug 14, 2024

IceTDrinker Aug 14, 2024 •

edited

Loading

IceTDrinker Aug 14, 2024

IceTDrinker left a comment

IceTDrinker Aug 29, 2024

IceTDrinker Aug 29, 2024

IceTDrinker Sep 3, 2024

IceTDrinker left a comment


		const STACK_ARRAY_SIZE: usize = std::mem::size_of::<Vec<u8>>() - 1;

		/// Simple short optimized vec, where if the data is small enough

feat(hlapi): add tag system #1468

feat(hlapi): add tag system #1468

Conversation

tmontaigu commented Aug 12, 2024

PR content/description

Check-list:

IceTDrinker commented Aug 13, 2024

nsarlin-zama left a comment

Choose a reason for hiding this comment

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

tmontaigu Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

IceTDrinker Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

IceTDrinker left a comment

Choose a reason for hiding this comment

tmontaigu Aug 14, 2024 •

edited

Loading

IceTDrinker Aug 14, 2024 •

edited

Loading