Update toolchain #494

mayeul-zama · 2023-08-22T15:23:07Z

PR content/description

Update rust toolchain

github-actions · 2023-08-22T15:23:21Z

@slab-ci cpu_fast_test

github-actions · 2023-08-22T15:26:13Z

@slab-ci cpu_fast_test

mayeul-zama · 2023-08-22T16:01:16Z

@slab-ci cpu_fast_test

tmontaigu · 2023-08-22T17:30:03Z

tfhe/src/shortint/engine/server_side/add.rs

@@ -27,7 +27,7 @@ impl ShortintEngine {
    pub(crate) fn smart_add(
        &mut self,
        server_key: &ServerKey,
-        ct_left: &mut Ciphertext,


Normally all the smart functions should take &mut for both input to clean carries if needed,
but these one do not seems to be correctly written

Ok
Do you think we should fix this code by cleaning carries:

in this PR (not sure how big this would be)?

in another fix PR and in the meantime:

keep mut and silence this warning for the smart functions?

remove mut (as in this PR) and reintroduce it in the fix PR?

If we only clean carries of outputs, is it not enough (as all inputs will then be clean)? Or maybe we want to do it only when really needed to avoid it in some cases

smart_ functions are meant to clean carries of input only when needed

I would say we keep the changes in the PR and try to find some time later to fix the shortint smart functions that
needs to be fixed

apps/trivium/src/lib.rs

github-actions · 2023-08-22T17:38:22Z

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

tmontaigu · 2023-08-23T12:22:54Z

@slab-ci cpu_test
@slab-ci cpu_integer_test
@slab-ci cpu_multi_bit_test
@slab-ci cpu_wasm_test

tmontaigu · 2023-08-23T12:26:49Z

Well I saw that Arthur did the same thing as you but also corrected the smart function in this PR: #470 maybe we should just update Arthur's PR

github-actions · 2023-08-25T12:01:12Z

@slab-ci cpu_fast_test

IceTDrinker · 2023-08-28T07:24:44Z

if this is working we could probably port the smart changes from my PR to this one I remember mine not passing tests IIRC
last commit is broken however in this PR

github-actions · 2023-08-28T13:35:27Z

@slab-ci cpu_fast_test

IceTDrinker

First batch of comments looks like some smart ops are still taking some inputs by non mutable reference

IceTDrinker · 2023-08-28T13:50:58Z

apps/trivium/src/lib.rs

+#![allow(clippy::module_inception)]
+#![allow(clippy::should_implement_trait)]


why is this needed ? 🤔

#![allow(clippy::should_implement_trait)]

In 4 places:
method next can be confused for the standard trait method std::iter::Iterator::next
consider implementing the trait std::iter::Iterator or choosing a less ambiguous method name
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait

#![allow(clippy::module_inception)]

In 3 places:
module has the same name as its containing module
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_inception

By adding this lines, I wanted to avoid moving/renaming things which is not rebase friendly

Please let me know if you think we should fix the lints in the PR.
Feel free to add a commit to this PR with theses fixes

which next function is impacted ?

apps/trivium/src/kreyvium/kreyvium_shortint.rs, line 78
apps/trivium/src/kreyvium/kreyvium.rs, line 152
apps/trivium/src/trivium/trivium_shortint.rs, line 66
apps/trivium/src/trivium/trivium.rs, line 124

I'll put the lint on each function

otherwise rename the function to "next_byte" or whatever name fits the returned value ?

or implement iterator for those, but really not sure it's worth it ?

Renamed next_ct and next_bool

IceTDrinker · 2023-08-28T13:52:15Z

tfhe/src/c_api/shortint/server_key/scalar_add.rs

-    ct_left: *mut ShortintCiphertext,
+    ct_left: *const ShortintCiphertext,


this should still be mut right ? as discussed for the smart stuff

code below should probably be updated as well

In smart_scalar_add_assign, if unchecked_scalar_add_assign is impossible, we apply a shifted LUT to the input to get the output (https://github.com/zama-ai/tfhe-rs/blob/241bddccaf87724d123c45d96cb0660357356869/tfhe/src/shortint/engine/server_side/scalar_add.rs#L60C38-L60C38)
So in smart_scalar_add, I think we can either:

clone ct and call smart_scalar_add_assign on it -> input not cleaned, output clean

clean ct (if necessary), clone ct and call unchecked_scalar_add_assign on it -> input cleaned, output not clean

I kept the first version
I think both versions could be useful to have

the first version you describe sounds like a default operation, i.e. output is always clean
the second is a smart operation as far as I can tell

I see what you mean
But it seems to me that the first version can give better performance than the second one if the input is never reused
So with this definition of smart operation, using smart operations can be more costly than using default ones (which is not intuitive)

I think the second version is more consistent with the rest of the codebase so I'll use it

indeed, though smart ops have this approach of cleaning inputs to hopefully gain time on later ops if re-used

that's why default ops have been made default, easier to reason about and not that bad performance characteristics ! 🙂

IceTDrinker · 2023-08-28T13:52:24Z

tfhe/src/c_api/shortint/server_key/scalar_mul.rs

-    ct_left: *mut ShortintCiphertext,
+    ct_left: *const ShortintCiphertext,


IceTDrinker · 2023-08-28T13:52:53Z

tfhe/src/c_api/shortint/server_key/scalar_sub.rs

-    ct_left: *mut ShortintCiphertext,
+    ct_left: *const ShortintCiphertext,


IceTDrinker · 2023-08-28T13:53:00Z

tfhe/src/c_api/shortint/server_key/shift.rs

-    ct: *mut ShortintCiphertext,
+    ct: *const ShortintCiphertext,


IceTDrinker · 2023-08-28T13:53:32Z

tfhe/src/core_crypto/fft_impl/fft128/crypto/ggsw.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::redundant_locals)]


why is this needed ?

Otherwise, the line let output_fft_buffer_re0 = &mut *output_fft_buffer_re0; and the 3 next ones are generating a warning which I believe is a false positive

removing the &mut is not enough ? looks like deref returns a mutable ref to a slice

basically I'm all against relaxing lints at file level, hence why I'm aksing this :)

it's an easy way to over disable stuff

if this is really a false positive then I would prefer a function level disable or even a block disable as these rust attributes can be put in a lot of places

could the lines be removed maybe ? maybe the compiler can deref things on its own

This does not work :(
I think that's the first thing I tried

last thing I'm thinking of: annotating the types on the left ? so that deref does not remove the reference

Removing the variables and adding &mut where they were used works (the * is not even needed)

tfhe/src/js_on_wasm_api/mod.rs

tfhe/src/lib.rs

github-actions · 2023-08-28T14:00:16Z

@slab-ci cpu_fast_test

github-actions · 2023-08-28T16:09:15Z

@slab-ci cpu_fast_test

github-actions · 2023-08-29T08:48:58Z

@slab-ci cpu_fast_test

github-actions · 2023-08-29T10:47:21Z

@slab-ci cpu_fast_test

github-actions · 2023-08-29T12:51:48Z

@slab-ci cpu_fast_test

github-actions · 2023-08-29T13:31:29Z

@slab-ci cpu_fast_test

mayeul-zama · 2023-08-29T13:51:29Z

@slab-ci cpu_fast_test
@slab-ci cpu_test
@slab-ci cpu_integer_test
@slab-ci cpu_multi_bit_test
@slab-ci cpu_wasm_test

github-actions · 2023-08-29T16:05:08Z

@slab-ci cpu_fast_test

mayeul-zama · 2023-08-30T07:08:58Z

tfhe/src/shortint/engine/server_side/mod.rs

@@ -603,9 +603,29 @@ impl ShortintEngine {
    where
        F: Fn(u64, u64) -> u64,
    {
+        // TODO this looks wrong for the Accumulator generation


Should this be checked/fixed now (this commit comes from @IceTDrinker PR) ?

it should have been fixed by Thomas IIRC check the main branch

In commit a0946ac?

think so yes

if you are confident with the current version launch all the test suites and we'll redo a thorough review of the new code

why is the TODO still here ? I thought we had it fixed in main 🤔

I will remove it

is the change you made still required ?

I took it from your PR
But I think so as it cleans the input if necessary instead of cleaning the result (cloned from the input)

my PR predates the fix from Thomas, i.e. the fix from Thomas should be the one to keep

apps/trivium/src/kreyvium/mod.rs

tfhe/src/shortint/engine/server_side/scalar_bitwise_op.rs

tfhe/src/shortint/engine/server_side/shift.rs

tfhe/src/shortint/engine/server_side/scalar_sub.rs

tfhe/src/shortint/engine/server_side/scalar_mul.rs

mayeul-zama · 2023-08-30T13:25:48Z

I think it would be better to split this PR in 2, to handle behavioral changes in a separate PR (2 commits feat(shortint): cleanup input if necessary in **) since I think of similar changes in other files
WDYT?

IceTDrinker · 2023-08-30T13:35:24Z

I think it would be better to split this PR in 2, to handle behavioral changes in a separate PR (2 commits feat(shortint): cleanup input if necessary in **) since I think of similar changes in other files WDYT?

fine by me yes

github-actions · 2023-08-30T14:30:49Z

@slab-ci cpu_fast_test

IceTDrinker

Thanks a lot @mayeul-zama for this tedious work 🙏

github-actions · 2023-08-30T14:35:07Z

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

mayeul-zama requested a review from IceTDrinker August 22, 2023 15:23

cla-bot bot added the cla-signed label Aug 22, 2023

mayeul-zama force-pushed the update_toolchain branch from f3dda23 to 3ae3124 Compare August 22, 2023 15:25

tmontaigu reviewed Aug 22, 2023

View reviewed changes

apps/trivium/src/lib.rs Outdated Show resolved Hide resolved

tmontaigu approved these changes Aug 22, 2023

View reviewed changes

mayeul-zama force-pushed the update_toolchain branch from a2a4efc to 89fdde3 Compare August 28, 2023 13:35

IceTDrinker reviewed Aug 28, 2023

View reviewed changes

mayeul-zama force-pushed the update_toolchain branch from 89fdde3 to d84cd9f Compare August 28, 2023 14:00

mayeul-zama force-pushed the update_toolchain branch from d6f5d37 to c0bd0cd Compare August 29, 2023 08:48

mayeul-zama force-pushed the update_toolchain branch from c0bd0cd to 4841137 Compare August 29, 2023 10:47

mayeul-zama force-pushed the update_toolchain branch from 4841137 to 4bec46e Compare August 29, 2023 12:51

mayeul-zama force-pushed the update_toolchain branch from 4bec46e to 959b71b Compare August 29, 2023 13:31

mayeul-zama requested a review from IceTDrinker August 29, 2023 14:27

mayeul-zama requested a review from tmontaigu August 29, 2023 14:27

mayeul-zama commented Aug 30, 2023

View reviewed changes