Mz/hl oprf any integer

[mayeul-zama]

This change is 

[mayeul-zama]

to fix

[mayeul-zama]

Done

[tmontaigu]

@tmontaigu reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, @soonum, and @tmontaigu)

---

tfhe/src/high_level_api/integers/oprf.rs line 171 at r1 (raw file):

    ///
    /// If the range is a power of 2, the distribution will be uniform (for any `max_distance`) and
    /// the cost smaller.

the cost is smaller

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r1 (raw file):

This method takes a `max_distance` as an Option.
If it is `None`, the distribution is indistinguishable from the uniform in practice. 
Providing a `max_distance` can give better performance at the cost of a distribution further from the uniform. Look at the documentation of the method for more details.

I think See the method's documentation.. or Refer to the method's documentation for mor details sounds better here

[mayeul-zama]

Done

[mayeul-zama]

Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker, @soonum, and @tmontaigu)

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I think See the method's documentation.. or Refer to the method's documentation for mor details sounds better here

Done.

---

tfhe/src/high_level_api/integers/oprf.rs line 171 at r1 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

the cost is smaller

Done.

[IceTDrinker]

First batch of comments (haven't yet checked the main part of the code)

@IceTDrinker reviewed 7 of 10 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, 12 unresolved discussions (waiting on @mayeul-zama, @soonum, and @tmontaigu)

---

Makefile line 1673 at r2 (raw file):

	RUSTFLAGS="$(RUSTFLAGS)" __TFHE_RS_BENCH_TYPE=$(BENCH_TYPE) \
	cargo $(CARGO_RS_CHECK_TOOLCHAIN) bench \
	--bench hlapi-oprf \

can't it be added to the existing benchmarks via a dedicated function ? cc @soonum

---

tfhe/src/integer/server_key/radix_parallel/tests_unsigned/test_oprf.rs line 190 at r2 (raw file):

}

pub fn p_value_upper_bound_oprf_almost_uniformity_from_values(

should those functions be pub or pub(crate) ? valid for all of them

---

tfhe-benchmark/Cargo.toml line 95 at r2 (raw file):

[[bench]]
name = "hlapi-oprf"

let's stop special casing benchmarks

---

tfhe/src/shortint/oprf.rs line 490 at r2 (raw file):

    }

    pub fn uniformity_p_value<F>(f: F, sample_count: usize, distinct_values: u64) -> f64

pub(crate) ?

---

tfhe/src/core_crypto/commons/math/random/tests.rs line 550 at r2 (raw file):

                assert_eq!(theoretical_cdf, 1.0);
                assert_eq!(empirical_cdf, 1.0);
            }

why is this change needed ?

---

tfhe-benchmark/benches/high_level_api/oprf.rs line 5 at r2 (raw file):

use tfhe::{set_server_key, ClientKey, ConfigBuilder, FheUint64, RangeForRandom, Seed, ServerKey};

pub fn oprf_any_range(c: &mut Criterion) {

this should be added in the main hlapi bench

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 7 at r2 (raw file):

The goal is to give to the server the possibility to generate a random value, which will be obtained in an encrypted format and will remain unknown to the server.

The main method for this is `FheUint::generate_oblivious_pseudo_random_custom_range` which return an integer taken almost uniformly in the range `[0; excluded_upper_bound[`.

return -> returns

I think it would be better to go with the framing of the math note indicating it comes from a certain distribution and that said distribution has a certain guaranteed distance to the Uniform distribution by default

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 9 at r2 (raw file):

The main method for this is `FheUint::generate_oblivious_pseudo_random_custom_range` which return an integer taken almost uniformly in the range `[0; excluded_upper_bound[`.
This method takes a `max_distance` as an Option.
If it is `None`, the distribution is indistinguishable from the uniform in practice. 

Give the mathematical guarantee

there is a trailing whitespace

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r2 (raw file):

This method takes a `max_distance` as an Option.
If it is `None`, the distribution is indistinguishable from the uniform in practice. 
Providing a `max_distance` can give better performance at the cost of a distribution further from the uniform. Refer to the method's documentation for more details.

might be better to explain that the larger the distance the more dissimilar the effective dsitribution is from the uniform (again use the maths notations and explanation on distance)

[mayeul-zama]

Reviewable status: 2 of 11 files reviewed, 6 unresolved discussions (waiting on @IceTDrinker, @soonum, and @tmontaigu)

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 7 at r2 (raw file):

Previously, IceTDrinker wrote…

return -> returns

I think it would be better to go with the framing of the math note indicating it comes from a certain distribution and that said distribution has a certain guaranteed distance to the Uniform distribution by default

Done

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 9 at r2 (raw file):

Previously, IceTDrinker wrote…

Give the mathematical guarantee

there is a trailing whitespace

Done

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r2 (raw file):

Previously, IceTDrinker wrote…

might be better to explain that the larger the distance the more dissimilar the effective dsitribution is from the uniform (again use the maths notations and explanation on distance)

Done

---

tfhe/src/core_crypto/commons/math/random/tests.rs line 550 at r2 (raw file):

Previously, IceTDrinker wrote…

why is this change needed ?

Because in some test (ex: random bits = 2, range=[0,5[), the theoretical_pdf of the last element is 0, so the theoretical_cdf of the second to last is already 1.0

---

tfhe/src/integer/server_key/radix_parallel/tests_unsigned/test_oprf.rs line 190 at r2 (raw file):

Previously, IceTDrinker wrote…

should those functions be pub or pub(crate) ? valid for all of them

Changed to pub(crate)

---

tfhe/src/shortint/oprf.rs line 490 at r2 (raw file):

Previously, IceTDrinker wrote…

pub(crate) ?

Done

---

tfhe-benchmark/benches/high_level_api/oprf.rs line 5 at r2 (raw file):

Previously, IceTDrinker wrote…

this should be added in the main hlapi bench

Done

---

Makefile line 1673 at r2 (raw file):

Previously, IceTDrinker wrote…

can't it be added to the existing benchmarks via a dedicated function ? cc @soonum

Removed

---

tfhe-benchmark/Cargo.toml line 95 at r2 (raw file):

Previously, IceTDrinker wrote…

let's stop special casing benchmarks

Done

[soonum]

tfhe-benchmark/benches/high_level_api/oprf.rs line 25 at r3 (raw file):

        let range = RangeForRandom::new_from_excluded_upper_bound(excluded_upper_bound);

        let bench_id_oprf = format!("bound_{excluded_upper_bound}");

The bench ID must include the bench_name in the form: format!("{bench_name}::bound_{excluded_upper_bound}")

[soonum]

@soonum reviewed 2 of 10 files at r1, 6 of 9 files at r3, all commit messages.
Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, and @tmontaigu)

[mayeul-zama]

tfhe-benchmark/benches/high_level_api/oprf.rs line 25 at r3 (raw file):

Previously, soonum (David Testé) wrote…

The bench ID must include the bench_name in the form: format!("{bench_name}::bound_{excluded_upper_bound}")

I removed it because the bench group name is also printed.
In the end, it was printing hlapi::oprf_any_range/hlapi::oprf_any_range::bound_... which is redundant
It's also one of the causes of the truncating of some bench names when they include param names

Maybe we should change this convention, what do you think?

[tmontaigu]

tfhe/src/high_level_api/integers/oprf.rs line 196 at r3 (raw file):

        seed: Seed,
        range: &RangeForRandom,
        max_distance: Option<f64>,

Do we need some validation on the max_distance ? What happens if someone gives a distance that's incorrect/big

[tmontaigu]

@tmontaigu reviewed 4 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IceTDrinker, @mayeul-zama, and @soonum)

---

tfhe/src/high_level_api/integers/oprf.rs line 196 at r3 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Do we need some validation on the max_distance ? What happens if someone gives a distance that's incorrect/big

Since there is a look with a break, could a user cause a an infinite loop ?

---

tfhe/src/high_level_api/integers/oprf.rs line 655 at r3 (raw file):

        // The distribution is not exactly uniform
        // This check ensures than with the given low max_distance,
        // the distribution is indistinguishable from the uniform with athe given sample count

with athe -> at ?

[IceTDrinker]

A few comments/questions, the tests are looking good

@IceTDrinker reviewed 9 files and all commit messages, made 10 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @mayeul-zama and @pdroalves).

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r6 (raw file):

It follows a distribution close to the uniform.

This function guarantees the the norm-1 distance (defined as ∆(P,Q) := 1/2 Sum[ω∈Ω] |P(ω) − Q(ω)|)

the the -> the

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 14 at r6 (raw file):

The higher the distance, the more dissimilar the actual distribution is from the target uniform distribution.

A safe value for `max_distance` is `2^-128`. It is the default value if `None` is provided.

Not sure it's safe for all applications, you can keep the information about the default value

---

tfhe/src/high_level_api/integers/oprf.rs line 174 at r6 (raw file):

    /// If the range is a power of 2, the distribution is uniform (for any `max_distance`) and
    /// the cost is smaller.
    ///

this is still limited to 64 bits right ? this should show up somehow maybe, or we should consider managing larger ranges later, could you maybe ask someon from Blockchain what the initial usage is expected to be ?

---

tfhe/src/high_level_api/integers/oprf.rs line 465 at r6 (raw file):

pub struct RangeForRandom {
    excluded_upper_bound: u64,

maybe make this NonZero ? given an excluded upper bound of 0 does not make sense ?

can be a try new if needed to avoid requesting users from passing a NonZeroU64

also this representation does not allow to generate on the full 2^64 range right ?

so we could shave an enum like CiphertextModulus to handle that, or better yet we could use CiphertextModulus as the field is private so that 0 represents 2^64 and we can then extend to 128 bits

---

tfhe/src/high_level_api/integers/oprf.rs line 504 at r6 (raw file):

    let remainder = mod_pow_2(random_bit_count, excluded_upper_bound) as f64;

    remainder * (excluded_upper_bound as f64 - remainder)

subtract in integer domain and convert at the last moment to keep precision

---

tfhe/src/high_level_api/integers/oprf.rs line 595 at r6 (raw file):

    #[test]
    fn test_fuzzing_against_oracle() {

fuzzing does not seem appropriate in the name given we will have fuzzing coming

---

tfhe/src/high_level_api/integers/oprf.rs line 632 at r6 (raw file):

                assert!(
                    (theoretical_distance - actual_distance).abs()

are they supposed to be equal maybe here given we have a perfect actual distance ?

I might be wrong

---

tfhe/src/high_level_api/integers/oprf.rs line 680 at r6 (raw file):

        let p_value_limit: f64 = 0.001;

        let params = PARAM_MESSAGE_2_CARRY_2_KS_PBS_TUNIFORM_2M128;

switch to KS32 I believe the tests have been update to that default ?

---

tfhe/src/high_level_api/integers/oprf.rs line 688 at r6 (raw file):

        let message_modulus = params.message_modulus;

        // [0.7, 0.1] for `max_distance` chosen to have `num_input_random_bits` be [2, 4]

could have dedicated values in a slighlty more complicated tuple for each, but this will do

[mayeul-zama]

@mayeul-zama made 9 comments and resolved 5 discussions.
Reviewable status: 7 of 13 files reviewed, 5 unresolved discussions (waiting on @IceTDrinker, @pdroalves, @soonum, and @tmontaigu).

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 10 at r6 (raw file):

Previously, IceTDrinker wrote…

the the -> the

Done

---

tfhe/docs/fhe-computation/advanced-features/encrypted-prf.md line 14 at r6 (raw file):

Previously, IceTDrinker wrote…

Not sure it's safe for all applications, you can keep the information about the default value

Removed "safe" mention

---

tfhe/src/high_level_api/integers/oprf.rs line 174 at r6 (raw file):

Previously, IceTDrinker wrote…

this is still limited to 64 bits right ? this should show up somehow maybe, or we should consider managing larger ranges later, could you maybe ask someon from Blockchain what the initial usage is expected to be ?

Added "Currently the range can only be in the form [0, excluded_upper_bound[ with any excluded_upper_bound in [1, 2^64[."

---

tfhe/src/high_level_api/integers/oprf.rs line 465 at r6 (raw file):

Previously, IceTDrinker wrote…

maybe make this NonZero ? given an excluded upper bound of 0 does not make sense ?

can be a try new if needed to avoid requesting users from passing a NonZeroU64

also this representation does not allow to generate on the full 2^64 range right ?

so we could shave an enum like CiphertextModulus to handle that, or better yet we could use CiphertextModulus as the field is private so that 0 represents 2^64 and we can then extend to 128 bits

Agree for the NonZeroU64

https://github.com/zama-ai/tfhe-rs-internal/issues/1254 for big power of 2 support

---

tfhe/src/high_level_api/integers/oprf.rs line 504 at r6 (raw file):

Previously, IceTDrinker wrote…

subtract in integer domain and convert at the last moment to keep precision

Done

---

tfhe/src/high_level_api/integers/oprf.rs line 595 at r6 (raw file):

Previously, IceTDrinker wrote…

fuzzing does not seem appropriate in the name given we will have fuzzing coming

Done

---

tfhe/src/high_level_api/integers/oprf.rs line 632 at r6 (raw file):

Previously, IceTDrinker wrote…

are they supposed to be equal maybe here given we have a perfect actual distance ?

I might be wrong

We use f64 so errors can accumulate a bit in the actual distance

---

tfhe/src/high_level_api/integers/oprf.rs line 680 at r6 (raw file):

Previously, IceTDrinker wrote…

switch to KS32 I believe the tests have been update to that default ?

Done

---

tfhe/src/high_level_api/integers/oprf.rs line 688 at r6 (raw file):

Previously, IceTDrinker wrote…

could have dedicated values in a slighlty more complicated tuple for each, but this will do

Done

[IceTDrinker]

@IceTDrinker reviewed 7 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mayeul-zama and @pdroalves).

---

tfhe/src/integer/server_key/radix_parallel/tests_unsigned/test_oprf.rs line 49 at r8 (raw file):

                seed,
                num_input_random_bits,
                NonZeroU64::new(excluded_upper_bound).unwrap(),

these unwraps are fine here as it would indicate the tests are calling the primitive wrong

---

tfhe/src/integer/server_key/radix_parallel/tests_long_run/test_random_op_sequence.rs line 510 at r8 (raw file):

                seed,
                num_input_random_bits,
                NonZeroU64::new(excluded_upper_bound).unwrap(),

here we may need to unwrap_or(1) or some other value ?

or we may have a crash if we aren't lucky because of the unwrap ?

because in long run tests inputs can be completely random

[mayeul-zama]

@mayeul-zama made 1 comment and resolved 2 discussions.
Reviewable status: 12 of 13 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker and @pdroalves).

---

tfhe/src/integer/server_key/radix_parallel/tests_long_run/test_random_op_sequence.rs line 510 at r8 (raw file):

Previously, IceTDrinker wrote…

here we may need to unwrap_or(1) or some other value ?

or we may have a crash if we aren't lucky because of the unwrap ?

because in long run tests inputs can be completely random

Done

[IceTDrinker]

Thanks !

@IceTDrinker reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pdroalves).

[IceTDrinker]

https://github.com/zama-ai/tfhe-rs/actions/runs/20815976308/job/59791710786

crash in the build with the change I suggested

[IceTDrinker]

@IceTDrinker reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pdroalves).