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.

Sign up for GitHub

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

Jump to bottom

feat(zk): implement faster pke proof #1302

Merged
merged 1 commit into from
Sep 6, 2024
Merged

feat(zk): implement faster pke proof #1302

merged 1 commit into from
Sep 6, 2024

Conversation

sarah-quinones
Copy link

PR content/description

implements Benoit's new private key encryption zk proof

Check-list:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Relevant issues are marked as resolved/closed, related issues are linked in the description
  • Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

@cla-bot cla-bot bot added the cla-signed label Jun 26, 2024
@IceTDrinker
Copy link
Member

cargo builds is red, could be good to have a review with JB potentially

@sarah-quinones sarah-quinones force-pushed the sk/feat/zk-pke-v2 branch 2 times, most recently from b3cd597 to d56f3f8 Compare June 26, 2024 13:46
IceTDrinker
IceTDrinker reviewed Jun 27, 2024
View reviewed changes
tfhe-zk-pok/src/proofs/mod.rs Outdated
Comment on lines 144 to 148
pub mod pke_v2 {
pub use super::pke_v2_impl::*;

pub use super::pke_v2_impl::crs_gen_cs as crs_gen;
}
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for the indirection here from pke_v2 to pke_v2_impl ?

Copy link
Author

Choose a reason for hiding this comment

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

just cause i wanted a quick way to swap out pke for pke_v2 for benchmarks. i'll fix it

Copy link
Member

Choose a reason for hiding this comment

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

got it

Copy link
Author

Choose a reason for hiding this comment

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

i removed the indirection

IceTDrinker
IceTDrinker reviewed Jun 27, 2024
View reviewed changes
tfhe-zk-pok/src/curve_api.rs Outdated
Comment on lines 355 to 359
#[cfg(target_family = "wasm")]
{
msm::msm_wnaf_g1_446(bases, scalars)
}
#[cfg(not(target_family = "wasm"))]
{
Self::Affine::multi_mul_scalar(bases, scalars)
}
Copy link
Member

Choose a reason for hiding this comment

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

is the if cfg!(target_family = "wasm") { usable here ? I think it makes it easier to read

Copy link
Author

Choose a reason for hiding this comment

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

yeah

Copy link
Author

Choose a reason for hiding this comment

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

fixed

IceTDrinker
IceTDrinker reviewed Jun 27, 2024
View reviewed changes
tfhe-zk-pok/src/curve_api.rs
Comment on lines +400 to 398
#[track_caller]
fn multi_mul_scalar(bases: &[Self::Affine], scalars: &[bls12_446::Zp]) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

this function can fail ?

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 elsewhere

Copy link
Author

Choose a reason for hiding this comment

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

yeah, if the slices aren't the same size

@IceTDrinker
Copy link
Member

this will need a rebase to have proper CI

@nsarlin-zama nsarlin-zama mentioned this pull request Aug 20, 2024
Merged
6 tasks
IceTDrinker
IceTDrinker reviewed Aug 27, 2024
View reviewed changes
tfhe-zk-pok/src/proofs/pke_v2.rs
t: u64,
rng: &mut dyn RngCore,
) -> PublicParams<G> {
crs_gen_cs(d, k, B, q, t, rng)
Copy link
Member

Choose a reason for hiding this comment

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

what's the cs vs. ghl notation ? It seems the CRS we use in the test is the ghl one ? @sarah-ek

@cla-bot cla-bot
Copy link

cla-bot bot commented Sep 6, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: sarah el kazdadi.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla-signed label Sep 6, 2024
@IceTDrinker
feat(zk): implement faster pke proof
6c68ae8 
- original work by Sarah El kazdadi

co-authored-by: sarah el kazdadi <sarah.elkazdadi@zama.ai>
@cla-bot cla-bot bot added the cla-signed label Sep 6, 2024
@IceTDrinker IceTDrinker requested a review from jborfila September 6, 2024 09:38
@IceTDrinker IceTDrinker merged commit ce9da12 into main Sep 6, 2024
141 checks passed
@IceTDrinker IceTDrinker deleted the sk/feat/zk-pke-v2 branch September 6, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceTDrinker IceTDrinker IceTDrinker left review comments

@jborfila jborfila jborfila approved these changes

Assignees
No one assigned
Labels
approved cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sarah-quinones @IceTDrinker @jborfila @zama-bot