-
Notifications
You must be signed in to change notification settings - Fork 146
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
docs(tfhe): add boolean sha256 tutorial #283
Conversation
Hey @JoseSK999 no need to open new PRs for the changes we request, force pushing to the PR branch is fine by us (and makes less noise with additional PRs) |
Let's keep this one for the other reviews |
b07f729
to
054b385
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @JoseSK999 thanks a lot for this work, it is very good indeed.
Several suggestions/questions that you could fix, for the last few modifications (like location of the tutorial in the docs) we'll probably do it ourselves.
In any case this contribution is very much appreciated.
Thanks!
carry | ||
} | ||
|
||
// 2 (homomorphic) bitwise ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quantify the cost of each function using homomorphic bitwise operations as unit (kinda)
let result: Vec<Ciphertext> = (0..32) | ||
.into_par_iter() | ||
.map(|i| sk.xor(&a[i], &b[i])) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about something like
let mut result = a.clone();
result.par_iter_mut().zip(a.par_iter().zip(b.par_iter())).map(|(dst, (lhs, rhs))| *dst = sk.xor(lhs, rhs));
result
?
same thing for similar funcs, not sure if it has an impact on perfs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit more efficient, thanks! We have to use for_each to consume the iterator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah my code snippet was slightly wrong
for (i, elem) in result.into_iter().enumerate() { | ||
array[i] = elem; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the above is not relevant, something like the following avoids indexing which has range checks for each indexing op :)
array.as_mut().copy.from_slice(&result);
to be applied where relevant in your code
array | ||
} | ||
|
||
fn and(a: &[Ciphertext; 32], b: &[Ciphertext; 32], sk: &ServerKey) -> [Ciphertext; 32] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
array | ||
} | ||
|
||
fn mux(condition: &[Ciphertext; 32], then: &[Ciphertext; 32], otherwise: &[Ciphertext; 32], sk: &ServerKey) -> [Ciphertext; 32] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
trivial_bools(&hex_to_bools(0x5be0cd19), &sk), | ||
]; | ||
|
||
let chunks = padded_input.chunks(512); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use chunks_exact as we know the length is a multiple of 512 :)
tfhe/examples/sha256_bool/padding.rs
Outdated
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use crate::sha256::bools_to_hex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this use statement does not resolve properly and prevents tests from running
test command used:
RUSTFLAGS="-C target-cpu=native" cargo +stable test --example sha256_bool --profile release --features=x86_64-unix,boolean -p tfhe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I forgot to add "_function" in the test
tfhe/docs/tutorial/sha256_bool.md
Outdated
We see a function called ``trivial_bools`` that we use along our implementation to initialize an array of 32 Ciphertexts. This is because we cannot copy the same Ciphertext for each position of the array as we do with simple bools, so we create 32 different trivial encryptions. We will also use this function in order to trivially encrypt constant values to operate homomorphically with them. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand here, you could clone x to be the mutable result and the call rotate/shift ops from rust right ?
https://doc.rust-lang.org/std/primitive.slice.html#method.rotate_right for the shift may need to do some manual management at the end
I guess the code could be updated as well then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. About the highlighted text, I meant that we cannot initialize arrays of ciphertexts like this:
let a = [sk.trivial_encrypt(false); 32];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok yes
tfhe/docs/tutorial/sha256_bool.md
Outdated
```rust | ||
fn xor(a: &[Ciphertext; 32], b: &[Ciphertext; 32], sk: &ServerKey) -> [Ciphertext; 32] { | ||
let result: Vec<Ciphertext> = (0..32) | ||
.into_par_iter() | ||
.map(|i| sk.xor(&a[i], &b[i])) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be updated depending on how the code gets updated with the suggestions made
tfhe/docs/tutorial/sha256_bool.md
Outdated
|
||
Our implementation uses Brent-Kung by default, but Ladner-Fischer can be enabled when needed by using the ```--ladner-fischer``` command line argument. | ||
|
||
For more information about parallel prefix adders you can read [this paper](https://www.iosrjournals.org/iosr-jece/papers/Vol6-Issue1/A0610106.pdf) or [this other](https://www.ijert.org/research/design-and-implementation-of-parallel-prefix-adder-for-improving-the-performance-of-carry-lookahead-adder-IJERTV4IS120608.pdf). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this other -> this other paper
I have updated everything, thanks for the feedback and the nice words! |
Btw @IceTDrinker I just found out that perhaps it's better to implement fn trivial_bools(bools: &[bool; 32], sk: &ServerKey) -> [Ciphertext; 32] {
let array = array::from_fn(|i| sk.trivial_encrypt(bools[i]));
array
}
fn initialize_w(sk: &ServerKey) -> [[Ciphertext; 32]; 64] {
let array = array::from_fn(|_| trivial_bools(&[false; 32], sk));
array
} |
It looks better/shorter than the current code, you should be able to drop the temporary to make it even shorter fn trivial_bools(bools: &[bool; 32], sk: &ServerKey) -> [Ciphertext; 32] {
array::from_fn(|i| sk.trivial_encrypt(bools[i]))
} |
relates zama-ai/bounty-program#39 |
a899ef1
to
6c7a936
Compare
This is the updated PR with the squashed commits.