-
Notifications
You must be signed in to change notification settings - Fork 291
chore(zk): add batched mode for verification pairings #2980
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
Conversation
df23ed6 to
301146f
Compare
301146f to
7c735ec
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't read all the formulas for now, this will need a thorough review, probably best done in person
@IceTDrinker reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions
tfhe-zk-pok/benches/pke_v2.rs line 33 at r1 (raw file):
let bench_id = format!("{bench_name}::{param_name}_{bits}_bits_packed_{load}_{bound:?}"); println!("{bench_id}");
that's actually a good idea, @soonum maybe we should do that for all bench_id or find a way to force criterion to print them, if it does not do it, we can print it ourselves
tfhe/src/zk/mod.rs line 823 at r1 (raw file):
(public_params, &public_commit), metadata, VerificationPairingMode::default(),
let's be explicit in the code ?
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
let mut lhs6 = None; let eta = G::Zp::rand(&mut rand::thread_rng());
question for us : do we want the rng to be configurable ? also is thread_rng always guaranteed to be crypto secure ?
nsarlin-zama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker and @soonum)
tfhe/src/zk/mod.rs line 823 at r1 (raw file):
Previously, IceTDrinker wrote…
let's be explicit in the code ?
one advantage of using default for this kind of config is that there is only one single source of truth to what version we use. And if with rust analyzer it's easy to jump to the exact variant from there if needed.
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
Previously, IceTDrinker wrote…
question for us : do we want the rng to be configurable ? also is thread_rng always guaranteed to be crypto secure ?
Yes I was wondering that too, but it adds some api overhead.
This is said to be cryptographically secure on 0.8 (https://docs.rs/rand/0.8.5/rand/rngs/struct.StdRng.html) and 0.9 (https://docs.rs/rand/latest/rand/rngs/struct.ThreadRng.html)
soonum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker and @soonum)
tfhe-zk-pok/benches/pke_v2.rs line 33 at r1 (raw file):
Previously, IceTDrinker wrote…
that's actually a good idea, @soonum maybe we should do that for all bench_id or find a way to force criterion to print them, if it does not do it, we can print it ourselves
Agree 100%! How come I never thought of this simple print trick to avoid the chopped bench IDs 🙈
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be ok, I'm not clear on the chi2/chi3 switch in some locations but as noted it is to keep equations unchanged between the proof and verify compute loads, let's do a double check on Monday
@IceTDrinker reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nsarlin-zama)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Yes I was wondering that too, but it adds some api overhead.
This is said to be cryptographically secure on 0.8 (https://docs.rs/rand/0.8.5/rand/rngs/struct.StdRng.html) and 0.9 (https://docs.rs/rand/latest/rand/rngs/struct.ThreadRng.html)
given this is not something that can be exposed to prod today via TFHE-rs let's have a TODO and a backlog issue please
if it gets used in prod, I'm guessing verifiers may want to run the same verification (not sure), also not sure of the security implications
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2069 at r2 (raw file):
q, t: _, msbs_zero_padding_bit_count: _,
same remark here, maybe forward some of those from the calling function ? not sure
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2078 at r2 (raw file):
let B_squared = inf_norm_bound_to_euclidean_squared(B_inf, d + k); let decoded_q = decode_q(q);
could those be forwarded from the calling function to avoid inconsistencies maybe or duplicated code for some of those
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2080 at r2 (raw file):
let decoded_q = decode_q(q); let R = |i: usize, j: usize| R[i + j * 128];
you update the R in the closure to be R_matrix in the main verify function, may be interesting to do the same for consistency
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2293 at r2 (raw file):
let decoded_q = decode_q(q); let R = |i: usize, j: usize| R[i + j * 128];
same remarks as for the other pairing verification function on potentially providing these arguments directly
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2339 at r2 (raw file):
C_hat_h3, C_hat_w: _, }) => lhs2 = Some(pairing(C_r_tilde + g.mul_scalar(eta * chi3), C_hat_h3)),
paper has eta * chi2, is that one of those locations that needs an inversion because of the ph_3 and p_t thing ?
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2395 at r2 (raw file):
s.spawn(|_| { lhs5 = Some(pairing( C_y.mul_scalar(delta_eq) - g.mul_scalar(eta * chi2),
I see chi3 in the ref, is that also one of the location to invert ?
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be checked for the detected inconsistency
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nsarlin-zama)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2395 at r2 (raw file):
s.spawn(|_| { lhs5 = Some(pairing( C_y.mul_scalar(delta_eq) - g.mul_scalar(eta * chi2),
in compute load verify this does not match, we have in the paper
C_y ^{-delta_eq} * g ^{-eta * chi2}
also note that the term lhs5 is taken as lhs5^{-1} in the code while the paper takes it as lhs5^{1}
nsarlin-zama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @IceTDrinker)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2069 at r2 (raw file):
Previously, IceTDrinker wrote…
same remark here, maybe forward some of those from the calling function ? not sure
could be, just that it is already a function with a lot of arguments so I found it better to simplify a bit the signature using this
Maybe I could regroup some of these and create new structs instead ?
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2078 at r2 (raw file):
Previously, IceTDrinker wrote…
could those be forwarded from the calling function to avoid inconsistencies maybe or duplicated code for some of those
Agree, but don't know what's best, this would make the function have a lot of arguments...
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2080 at r2 (raw file):
Previously, IceTDrinker wrote…
you update the R in the closure to be R_matrix in the main verify function, may be interesting to do the same for consistency
I changed it to directly have the closure as parameter
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2293 at r2 (raw file):
Previously, IceTDrinker wrote…
same remarks as for the other pairing verification function on potentially providing these arguments directly
done
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2339 at r2 (raw file):
Previously, IceTDrinker wrote…
paper has eta * chi2, is that one of those locations that needs an inversion because of the ph_3 and p_t thing ?
this is a typo in the paper that will be fixed
7c735ec to
c985062
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @IceTDrinker)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2339 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
this is a typo in the paper that will be fixed
answered the wrong comment, this one is due to the inverted chi2 and chi3
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2395 at r2 (raw file):
Previously, IceTDrinker wrote…
in compute load verify this does not match, we have in the paper
C_y ^{-delta_eq} * g ^{-eta * chi2}
also note that the term lhs5 is taken as lhs5^{-1} in the code while the paper takes it as lhs5^{1}
this is a typo in the paper that will be fixed
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2395 at r2 (raw file):
Previously, IceTDrinker wrote…
I see chi3 in the ref, is that also one of the location to invert ?
yes this is expected, see the comment line 2303
c985062 to
5cb6957
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good to approve, let me know if you find a way that's more satisfying to you to forward some of those parameters, let's not forget to have an issue to keep track of the rng thing
@IceTDrinker reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nsarlin-zama)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2069 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
could be, just that it is already a function with a lot of arguments so I found it better to simplify a bit the signature using this
Maybe I could regroup some of these and create new structs instead ?
not absolutely required, was a wondering what would make more sense
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2078 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Agree, but don't know what's best, this would make the function have a lot of arguments...
not absolutely required, try to see if you see something that can be done
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2080 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I changed it to directly have the closure as parameter
ok
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
Previously, IceTDrinker wrote…
given this is not something that can be exposed to prod today via TFHE-rs let's have a TODO and a backlog issue please
if it gets used in prod, I'm guessing verifiers may want to run the same verification (not sure), also not sure of the security implications
don't forget to create an issue for this when you get the time
nsarlin-zama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
Previously, IceTDrinker wrote…
don't forget to create an issue for this when you get the time
done.
Note that the generated value should not have any visible impact on the consensus.
I guess that in a distributed scenario it is a bit more secure if every node use a different value, since it makes it harder for an attacker to craft a targeted proof.
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IceTDrinker reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nsarlin-zama)
tfhe-zk-pok/src/proofs/pke_v2/mod.rs line 2319 at r2 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
done.
Note that the generated value should not have any visible impact on the consensus.I guess that in a distributed scenario it is a bit more secure if every node use a different value, since it makes it harder for an attacker to craft a targeted proof.
I agree, just that having the same values across nodes would simplify reasoning to compare results
5cb6957 to
00f8d64
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made a new version, maybe it's cleaner that way ?
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @IceTDrinker)
|
I'll take a look, but I was fine with the previous one otherwise create a struct for the verification containing everything we need and the call one version or the other for the pairing verification and call self.stuff for every stuff you need ? |
|
I don't have a strong opinion |
IceTDrinker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good like that! thanks a lot !
@IceTDrinker reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nsarlin-zama)
closes: please link all relevant issues
PR content/description
Adds the batched mode for the pairing in zk verification, replacing eq. (50) + (51) with eq. (52) from the reference paper.
This is added as an optional mode for users of the zk-pok crate, but not customizable in tfhe-rs (a bit like the crs bound).
While doing this pr I reordered a bit the "legacy/2 steps/50+51 mode" which improved its performance, reducing the latency by ~10%. This mean that batched mode is now less interesting than what I got in my initial benchmark, i.e. 7% in compute load proof and noting significant in compute load verify. So I decided to leave the "2 steps" mode by default.
I added a comparative bench and tests inside the tfhe-zk-pok crate, this will require updating the grafana dashboard once/if this is merged.
Check-list:
This change is