Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tfhe): add zk-pok code base #1050

Merged
merged 2 commits into from
Apr 9, 2024
Merged

feat(tfhe): add zk-pok code base #1050

merged 2 commits into from
Apr 9, 2024

Conversation

IceTDrinker
Copy link
Member

  • integration of work done by Sarah in the repo

@cla-bot cla-bot bot added the cla-signed label Apr 4, 2024
@tmontaigu tmontaigu force-pushed the am/feat/zk-proofs branch 6 times, most recently from 480907d to 24a579e Compare April 5, 2024 15:26
@IceTDrinker
Copy link
Member Author

cuda C API tests likely need the ZK pok feature @tmontaigu

Copy link
Member Author

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments that were waiting apparently, will continue this afternoon

Makefile Show resolved Hide resolved
tfhe/Cargo.toml Outdated
@@ -69,6 +69,8 @@ paste = "1.0.7"
fs2 = { version = "0.4.3", optional = true }
# While we wait for repeat_n in rust standard library
itertools = "0.11.0"
rand_core = { version = "0.6.4", features = ["std"] }
tfhe-zk-pok = { version = "0.1.0", path = "../tfhe-zk-pok", optional = true}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting, missing space before } , we'll add toml formatter at some point I guess but I don't think I found one that's easy to setup with e.g. rust

tfhe/Cargo.toml Outdated
@@ -270,6 +273,10 @@ required-features = ["boolean", "shortint", "internal-keycache"]

# Real use-case examples

[[example]]
name = "demo"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zk-pok-demo maybe ?

@@ -241,6 +241,7 @@ impl ServerKey {
let counter_num_blocks = ((num_bits_in_ciphertext - 1).ilog2() + 1 + 1)
.div_ceil(self.message_modulus().0.ilog2()) as usize;

// 11111000
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's that ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was explaining something via zoom and used that file as a whiteboard

Copy link
Member Author

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the core part there are some confusions between parallel and non parallel variants it seems, and some debug code is still there, I marked the biggest ones.

Also the private param of the CRS should not be available in the CRS structure otherwise I believe the security of the ZK falls, ping me if you need help

fn next_u32(&mut self) -> u32 {
let mut bytes = [0u8; std::mem::size_of::<u32>()];
bytes.iter_mut().for_each(|b| *b = self.generate_next());
u32::from_ne_bytes(bytes)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the uniform generation from the already implemented Uniform distribution if needed or use from_le_bytes, I believe from_le_bytes was preferred to avoid endianness issues cross platform

tfhe/src/core_crypto/commons/math/random/uniform.rs

fn next_u64(&mut self) -> u64 {
let mut bytes = [0u8; std::mem::size_of::<u64>()];
bytes.iter_mut().for_each(|b| *b = self.generate_next());
u64::from_ne_bytes(bytes)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing as above

}
}

impl<T> BoundedDistribution<T, T::Signed> for TUniform<T>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't low supposed to be equal to high given the trait definition above ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Low = High in the trait definition is just to set Low as by default being the same type as High, we it can be anything else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok got it, we may want to be consistent here and have everything with a single type wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can

Comment on lines 159 to 161
fn low_bound(&self) -> Bound<T::Signed> {
Bound::Included(self.min_value_inclusive())
}

fn high_bound(&self) -> Bound<T> {
Bound::Included(self.max_value_inclusive().into_unsigned())
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason here to have different signedness for both bounds ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for flexibility

<input
type="button"
id="compactPublicKeyZeroKnowledgeBench"
value="Compact zk bench"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nit needs bench with a capital B to be picked up by our filter and I had issues with that last week

you adjusted time outs right ?

slice_wrapping_add_assign(output_mask.as_mut(), mask_noise);

// Fill the body chunk afterward manually as it most likely will be smaller than
// the full convolution result. b convolved r + Delta * m + e2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

careful with the comment and the reverse we did, as we can see above I added a rev at some point in the comment to match the zk order, the comment seems to be missing this ?

Comment on lines 2239 to 2841
// Fill the temp buffer with b convolved with r
// // Fill the temp buffer with b convolved with r
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary diff

Comment on lines 2850 to 2852
let zk_body_convolved = polymul_rev(pk_body.as_ref(), binary_random_slice);

assert_eq!(&pk_body_convolved, &zk_body_convolved);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug can be removed

Comment on lines 2255 to 2256
// Fill the body chunk afterwards manually as it most likely will be smaller than
// the full convolution result. rev(b convolved r) + Delta * m + e2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should keep this comment here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is still here I believe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing the rev(...)

);
}

/// Encrypt and generates a zero-knowledge proof of an input cleartext list in an output
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parallel variant of

>(
lwe_compact_public_key: &LweCompactPublicKey<KeyCont>,
output: &mut LweCompactCiphertextList<OutputCont>,
messages: &InputCont,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed that during review

https://github.com/zama-ai/tfhe-rs-internal/issues/520

don't want to break things now, too late to make small fixes, but I think we should stick to plaintexts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or whatever type + encoding information

- integration of work done by Sarah in the repo

Co-authored-by: sarah el kazdadi <[email protected]>
@zama-bot zama-bot removed the approved label Apr 8, 2024
@IceTDrinker IceTDrinker merged commit 2c106e8 into main Apr 9, 2024
36 checks passed
@IceTDrinker IceTDrinker deleted the am/feat/zk-proofs branch April 9, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants