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

Ns/test zk bad noise #1783

Merged
merged 6 commits into from
Nov 25, 2024
Merged

Ns/test zk bad noise #1783

merged 6 commits into from
Nov 25, 2024

Conversation

nsarlin-zama
Copy link
Contributor

@nsarlin-zama nsarlin-zama commented Nov 14, 2024

PR content/description

Adds a test for zk proofs with a larger noise than the accepted one.
This pr also include small refactor of the zk proofs for easier testing, and a few fixes. Each one is done in a separate commit.

The tests for v1 and v2 are not exactly analogous to more effectively test the edge cases of the proven norms

@cla-bot cla-bot bot added the cla-signed label Nov 14, 2024
@nsarlin-zama nsarlin-zama requested review from IceTDrinker and removed request for IceTDrinker November 14, 2024 16:44
@nsarlin-zama nsarlin-zama force-pushed the ns/test_zk_bad_noise branch 7 times, most recently from feb4af8 to ef68e74 Compare November 21, 2024 09:45
@nsarlin-zama nsarlin-zama force-pushed the ns/test_zk_bad_noise branch 3 times, most recently from c131303 to e75a72d Compare November 21, 2024 13:59
Copy link
Member

@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.

Haven't read the test part yet, some small comments

tfhe-zk-pok/src/proofs/pke.rs Outdated Show resolved Hide resolved
Comment on lines +191 to +193
let B_squared = inf_norm_bound_to_euclidean_squared(B_inf, d + k);
let (n, D, B_bound_squared, _) =
compute_crs_params(d, k, B_squared, t, msbs_zero_padding_bit_count, bound_type);
Copy link
Member

Choose a reason for hiding this comment

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

We still need the m_bound_bar here right ? and then when it's recomputed in the proof it needs to be smaller ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since its value only depends on B_inf_bar, d_bar (from the crs) and k (which is checked to be < k_bar), m_bound in the proof cannot go above m_bound_bar.

tfhe-zk-pok/src/proofs/pke_v2.rs Show resolved Hide resolved
tfhe-zk-pok/src/proofs/pke_v2.rs Show resolved Hide resolved
tfhe-zk-pok/src/proofs/pke_v2.rs Outdated Show resolved Hide resolved
tfhe-zk-pok/src/proofs/pke_v2.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

I think there are a few small mistakes for some random bad term values, otherwise looks very solid, thanks a lot for the hard work !

tfhe-zk-pok/src/proofs/pke.rs Outdated Show resolved Hide resolved
tfhe-zk-pok/src/proofs/pke.rs Outdated Show resolved Hide resolved
tfhe-zk-pok/src/proofs/pke_v2.rs Outdated Show resolved Hide resolved
};

let B_with_slack_squared = inf_norm_bound_to_euclidean_squared(B, d + k);
let B_with_slack = isqrt(B_with_slack_squared) as u64;
Copy link
Member

Choose a reason for hiding this comment

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

no problem with floor vs round vs ceil here in this sqrt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we want to always be within the bound so I think that floor is ok

Comment on lines +2746 to +2750
let bound_squared =
B_with_slack_squared - (e_sqr_norm - sqr(orig_value as u128));
isqrt(bound_squared) as i64
Copy link
Member

Choose a reason for hiding this comment

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

same question no shenanigans with floor/round/ceil ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same answer. The goal is to have a value x that is always accepted and x+1 is always refused, so I think that floor is the right choice

tfhe-zk-pok/src/proofs/pke_v2.rs Outdated Show resolved Hide resolved
// ==== Generate test noise vectors with random coeffs and one completely out of bounds ===
let mut testcase_bad_e1 = testcase.clone();
let bad_idx = rng.gen::<usize>() % d;
let bad_term = (rng.gen::<i64>() % i64::MAX - B as i64) + B as i64;
Copy link
Member

Choose a reason for hiding this comment

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

does the bad term behave properly as well ? I'm having some doubts now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be ok if we take it as a positive value and nok if we take the negative one, I'm fixing it for this case

Comment on lines 1482 to 1483
// Generate a value between B + 1 and i64::MAX to make sure that it is out of bounds
let bad_term = (rng.gen::<i64>() % i64::MAX - (B + 1) as i64) + (B + 1) as i64;
Copy link
Member

Choose a reason for hiding this comment

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

does that work given this is signed ? I feel like the modulo operator can return signed values ?

Copy link
Member

Choose a reason for hiding this comment

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

you might be looking for rem_euclid, or just generate the thing in a signed manner or use the possibility to generate random values from a range of values, I have a hard time reading this one

Copy link
Contributor Author

@nsarlin-zama nsarlin-zama Nov 22, 2024

Choose a reason for hiding this comment

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

Oh yeah I missed the gen::<i64>, I fixed it to generate u64 then cast

@nsarlin-zama nsarlin-zama merged commit c5caacf into main Nov 25, 2024
110 checks passed
@nsarlin-zama nsarlin-zama deleted the ns/test_zk_bad_noise branch November 25, 2024 13:34
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.

3 participants