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

Am/feat/noise squashing #2149

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Am/feat/noise squashing #2149

wants to merge 7 commits into from

Conversation

IceTDrinker
Copy link
Member

@IceTDrinker IceTDrinker commented Mar 3, 2025

This change is Reviewable

@cla-bot cla-bot bot added the cla-signed label Mar 3, 2025
@IceTDrinker IceTDrinker force-pushed the am/feat/noise-squashing branch 4 times, most recently from d70c7c4 to aaff247 Compare March 4, 2025 10:21
@IceTDrinker IceTDrinker changed the base branch from main to ns/tmp_disable_dylint March 4, 2025 13:56
@IceTDrinker IceTDrinker changed the base branch from ns/tmp_disable_dylint to am/fix/shortint-private-noise-level March 4, 2025 13:57
@IceTDrinker IceTDrinker marked this pull request as ready for review March 4, 2025 13:57
@IceTDrinker IceTDrinker requested review from tmontaigu, mayeul-zama, agnesLeroy and nsarlin-zama and removed request for agnesLeroy March 4, 2025 13:57
@IceTDrinker IceTDrinker force-pushed the am/fix/shortint-private-noise-level branch from 4b23f3f to b7938ff Compare March 4, 2025 15:01
@IceTDrinker IceTDrinker force-pushed the am/feat/noise-squashing branch from aaff247 to 56cc624 Compare March 4, 2025 15:01
@IceTDrinker
Copy link
Member Author

backward data will be added with the HL commits

Base automatically changed from am/fix/shortint-private-noise-level to main March 5, 2025 09:16
- we don't currently have strings on GPU and so don't run clippy for them
- make Numeric CastFrom<Self>, this is not breaking as it's equivalent to
From<Self> in rust which is blanket implemented
- mark CastFrom<Self> inline(always) for the implementations I could find
- update APIs for bootstrappking key generation to support having mixed
integer types for both secret keys, i.e. having a u64 input key and an
an u128 output key

BREAKING: this change is technically breaking for core
- noise squshing consists in running a PBS over a large modulus like
2^128 with parameters which ensure a big gap between like the plaintext
and the noise, like 50+ bits, this can allow to run a noise flooding
step in MPC to protect against certain key recovery attacks
@IceTDrinker IceTDrinker force-pushed the am/feat/noise-squashing branch from 56cc624 to ebe5ae9 Compare March 5, 2025 09:16
Copy link
Contributor

@mayeul-zama mayeul-zama left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 4 of 4 files at r4, 5 of 5 files at r5, 6 of 6 files at r6, 13 of 15 files at r7, all commit messages.
Reviewable status: 33 of 35 files reviewed, 4 unresolved discussions (waiting on @nsarlin-zama and @tmontaigu)


tfhe/src/shortint/noise_squashing/server_key.rs line 67 at r7 (raw file):

                let modulus_switch_noise_reduction_key = noise_squashing_parameters
                    .modulus_switch_noise_reduction_params
                    .map(|p| {

p could be renamed modulus_switch_noise_reduction_params


tfhe/src/shortint/noise_squashing/server_key.rs line 107 at r7 (raw file):

        let output_message_modulus =
            MessageModulus(ciphertext.message_modulus.0 * ciphertext.carry_modulus.0);

We may want to only support squashing clean ciphertexts (empty carry) and use output_message_modulus = message_modulus

This would reduce constraints for the optimizer.


tfhe/src/shortint/noise_squashing/server_key.rs line 176 at r7 (raw file):

            output_message_modulus,
            CarryModulus(1),
            PaddingBit::Yes,

I think we don't need a padding bit as the output is never bootstrapped


tfhe/src/shortint/noise_squashing/private_key.rs line 36 at r7 (raw file):

            message_modulus: ciphertext.message_modulus(),
            carry_modulus: CarryModulus(1),
            padding_bit: PaddingBit::Yes,

I think we don't need a padding bit (related to sibling comment)

@IceTDrinker IceTDrinker requested a review from mayeul-zama March 7, 2025 13:21
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.

Reviewable status: 33 of 35 files reviewed, 4 unresolved discussions (waiting on @mayeul-zama, @nsarlin-zama, and @tmontaigu)


tfhe/src/shortint/noise_squashing/private_key.rs line 36 at r7 (raw file):

Previously, mayeul-zama wrote…

I think we don't need a padding bit (related to sibling comment)

currently this is how it has been optimized, I'm going to stick to it for now and let the R&D know we likely can drop the padding bit


tfhe/src/shortint/noise_squashing/server_key.rs line 67 at r7 (raw file):

Previously, mayeul-zama wrote…

p could be renamed modulus_switch_noise_reduction_params

do you think it's useful here as the full name is just above and it's in a map call ?


tfhe/src/shortint/noise_squashing/server_key.rs line 107 at r7 (raw file):

Previously, mayeul-zama wrote…

We may want to only support squashing clean ciphertexts (empty carry) and use output_message_modulus = message_modulus

This would reduce constraints for the optimizer.

Actually the original constraint was putting the msg bits in the upper part of the plaintext multiplying by 4, the packing requires a multiplication by 5 (if ciphertexts are correlated) and the parameters when regenerated did not change with the new dot product of 5 vs 4, so I think we are okay here following the existing optimization


tfhe/src/shortint/noise_squashing/server_key.rs line 176 at r7 (raw file):

Previously, mayeul-zama wrote…

I think we don't need a padding bit as the output is never bootstrapped

see answer on the other comment about it

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.

2 participants