Csprng rework #254

aPere3 · 2022-04-14T11:43:43Z

Resolves:

zama-ai/concrete-core-internal#111
zama-ai/concrete-core-internal#112
zama-ai/concrete_internal#346

Description

When forked, the children of the parent generator sometimes get an incorrect bound whose byte_ctr is higher than 127 (which should not be the case).

Checklist

(Use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
The PR description links to the related issue (to link an issue, use '#XXX'.)
The tests on AWS have been launched and are successful (apply the aws_test to the PR to launch the tests on AWS)
The draft release description has been updated
Check for breaking changes and add them to commit message following the conventional commit [specification][conventional-breaking]

IceTDrinker

Haven't looked at generators and counter yet, some general questions for my understanding :)

Edit: btw sorry if there is some overlap with some explanations you provided but I started writing part of the comments before we talked about this PR!

concrete-csprng/Cargo.toml

concrete-csprng/benches/benchmark.rs

concrete-csprng/src/generators/aes_ctr/block_cipher.rs

concrete-csprng/src/lib.rs

concrete-csprng/Cargo.toml

concrete-csprng/src/seeders/implem/linux.rs

concrete-csprng/src/seeders/mod.rs

concrete-csprng/src/generators/aes_ctr/generic.rs

concrete-csprng/src/generators/aes_ctr/parallel.rs

concrete-csprng/benches/benchmark.rs

concrete-csprng/Cargo.toml

concrete-csprng/src/generators/mod.rs

concrete-csprng/src/generators/aes_ctr/generic.rs

concrete-csprng/src/generators/implem/soft/block_cipher.rs

aPere3 · 2022-04-22T07:21:21Z

Thanks for your thorough reviews @agnesLeroy, @IceTDrinker !

I made the changes you requested !

concrete-csprng/src/generators/aes_ctr/generic.rs

IceTDrinker

It's a bit hard to tell if it's ok with the force push, but it seems you adressed all I saw, thanks !

agnesLeroy

Hello! We went through the PR this morning with Damien, here come some first comments about it! :) He's going to continue reading it on his side. :)

concrete-csprng/src/seeders/mod.rs

concrete-csprng/src/generators/mod.rs

concrete-csprng/src/generators/implem/aesni/block_cipher.rs

concrete-csprng/src/generators/aes_ctr/mod.rs

IceTDrinker

btw would it be worth it to update the aes crate as aes_soft is marked as deprecated? The only bother for the new crate is that it does not seem easy to get the software variant enabled :|

aPere3 · 2022-04-26T12:27:45Z

@IceTDrinker Thanks for reminding us of that, I forgot that you mentioned it a few weeks ago. I see an aes_force_soft configuration flag, in the aes crate, which seems to do what we want. Now the question is, do we integrate it in this (already large) PR ?

IceTDrinker · 2022-04-26T12:33:18Z

@IceTDrinker Thanks for reminding us of that, I forgot that you mentioned it a few weeks ago. I see an aes_force_soft configuration flag, in the aes crate, which seems to do what we want. Now the question is, do we integrate it in this (already large) PR ?

probably not, it's big enough as is IMO :) was just that I thought about it again, I don't know if the config flag is easy to use in the build system (vs. features IIRC) which is a bit of a shame really...

aPere3 · 2022-04-26T12:34:59Z

Yes, I got mixed up, I read configuration flag but thought feature flag. It would be interesting to understand why they chose to do it that way 🕵️

BREAKING_CHANGES: + `try_fork` and `par_try_fork` now return a result. + `remaining_bytes` now returns a `ByteCount`.

BREAKING_CHANGE: this commit completely breaks the previous API

aPere3 · 2022-04-27T09:02:11Z

@IceTDrinker : I asked the question on RustCrypto/block-ciphers#316 .

damienligier

Very good upgrade! there are some spaces in middle of documentation probably because of the formatting. also misused of the verb yield. we should avoid it and use output of produce instead. i added many comments about that but i left some of them. please could you double check all of them?

It could also be useful to re implement the drop (cf @agnesLeroy) to set to zero all the prng values.

concrete-csprng/benches/benchmark.rs

concrete-csprng/src/generators/aes_ctr/block_cipher.rs

damienligier · 2022-04-27T11:59:35Z

concrete-csprng/src/generators/aes_ctr/generic.rs

+pub struct AesCtrGenerator<BlockCipher: AesBlockCipher> {
+    // The block cipher used in the background
+    pub(crate) block_cipher: BlockCipher,
+    // The state corresponding to the latest yielded byte.


is it the counter? the next randomness will be obtained from: this state+1?

concrete-csprng/src/generators/aes_ctr/generic.rs

concrete-csprng/src/lib.rs

aPere3 · 2022-05-03T13:48:31Z

Hello @damienligier ,

Thank you for this review !

I tried to address most points in the follow-up PR on zama-ai/concrete-core#8 . I integrated most of your remarks, and closed the related conversations here. The few remaining ones, I left open in case you want more informations / take actions.

Related to your remark on re-implementing the Drop trait, you raise a very valid point, and I would love to see this done ! That being said, it is tricky to do properly, as we have to use volatile memory operations and compiler fences to ensure the compiler does not get rid / reorder the clean-up code in a wrong way.

I would prefer to add that in a future PR, focused on this matter, as this one is already pretty big, what do you think ?

I close this PR so that we can leave the floor for the libs team here, but feel free to answer in the conversation 😉

Best,

Alex.

cla-bot bot added the cla-signed label Apr 14, 2022

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from b22a01d to 1293596 Compare April 14, 2022 12:08

aPere3 requested a review from agnesLeroy April 14, 2022 12:08

aPere3 marked this pull request as draft April 14, 2022 14:27

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from 54a7391 to 3ef48df Compare April 20, 2022 09:30

aPere3 marked this pull request as ready for review April 20, 2022 09:31

aPere3 requested a review from IceTDrinker April 20, 2022 09:31

aPere3 changed the title ~~fix(concrete_csprng): fixes a bug in concrete_csprng fork~~ Csprng rework Apr 20, 2022

IceTDrinker reviewed Apr 20, 2022

View reviewed changes

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch 2 times, most recently from 7172dd1 to df4c15e Compare April 21, 2022 07:38

agnesLeroy reviewed Apr 21, 2022

View reviewed changes

agnesLeroy requested a review from damienligier April 21, 2022 09:52

IceTDrinker reviewed Apr 21, 2022

View reviewed changes

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from df4c15e to 75fd849 Compare April 22, 2022 07:17

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch 2 times, most recently from 4a0f82a to fa4b4d9 Compare April 22, 2022 07:46

IceTDrinker reviewed Apr 22, 2022

View reviewed changes

concrete-csprng/src/generators/aes_ctr/generic.rs Outdated Show resolved Hide resolved

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from fa4b4d9 to 8c1d140 Compare April 22, 2022 08:44

IceTDrinker approved these changes Apr 22, 2022

View reviewed changes

aPere3 mentioned this pull request Apr 25, 2022

Plug new prng api #262

Closed

6 tasks

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch 2 times, most recently from 452eb1d to 2d19e32 Compare April 25, 2022 12:27

agnesLeroy reviewed Apr 26, 2022

View reviewed changes

IceTDrinker reviewed Apr 26, 2022

View reviewed changes

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from 2d19e32 to 31d7c49 Compare April 26, 2022 13:43

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from 31d7c49 to df7470c Compare April 26, 2022 13:49

aPere3 added 2 commits April 27, 2022 10:21

fix(concrete_csprng): fixes a bug in concrete_csprng fork

7390180

BREAKING_CHANGES: + `try_fork` and `par_try_fork` now return a result. + `remaining_bytes` now returns a `ByteCount`.

feat(concrete_csprng): refactor the csprng to prepare core backend split

2c0e93d

BREAKING_CHANGE: this commit completely breaks the previous API

aPere3 force-pushed the bug/concrete_csprng_byte_ctr branch from df7470c to 2c0e93d Compare April 27, 2022 08:23

damienligier suggested changes May 2, 2022

View reviewed changes

aPere3 closed this May 3, 2022

tmontaigu deleted the bug/concrete_csprng_byte_ctr branch August 8, 2022 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Csprng rework #254

Csprng rework #254

aPere3 commented Apr 14, 2022 •

edited

Loading

IceTDrinker left a comment •

edited

Loading

aPere3 commented Apr 22, 2022

IceTDrinker left a comment

agnesLeroy left a comment

IceTDrinker left a comment

aPere3 commented Apr 26, 2022

IceTDrinker commented Apr 26, 2022

aPere3 commented Apr 26, 2022

aPere3 commented Apr 27, 2022

damienligier left a comment •

edited

Loading

damienligier Apr 27, 2022

aPere3 May 3, 2022

aPere3 commented May 3, 2022

Csprng rework #254

Csprng rework #254

Conversation

aPere3 commented Apr 14, 2022 • edited Loading

Resolves:

Description

Checklist

IceTDrinker left a comment • edited Loading

Choose a reason for hiding this comment

aPere3 commented Apr 22, 2022

IceTDrinker left a comment

Choose a reason for hiding this comment

agnesLeroy left a comment

Choose a reason for hiding this comment

IceTDrinker left a comment

Choose a reason for hiding this comment

aPere3 commented Apr 26, 2022

IceTDrinker commented Apr 26, 2022

aPere3 commented Apr 26, 2022

aPere3 commented Apr 27, 2022

damienligier left a comment • edited Loading

Choose a reason for hiding this comment

damienligier Apr 27, 2022

Choose a reason for hiding this comment

aPere3 May 3, 2022

Choose a reason for hiding this comment

aPere3 commented May 3, 2022

aPere3 commented Apr 14, 2022 •

edited

Loading

IceTDrinker left a comment •

edited

Loading

damienligier left a comment •

edited

Loading