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

[zk-token-sdk] Add ciphertext validity proof with 3 handles instruction #897

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Apr 18, 2024

Problem

Currently, the zk-token-proof program only handles ciphertext validity proof for grouped ciphertext with 2 handles. A ciphertext validity proof for 3 handles is needed to simplify confidential transfers in spl as explained in #809.

Summary of Changes

Added ciphertext validity proof instructions to the zk-token-proof program. The zk-token-proof program is not activated yet and I will rekey the program feature gate once this PR is merged.

  • c05b7a8: Refactored the existing ciphertext validity proof instructions to make room for the variant with 3 handles.
  • 3b33964: Added instruction data for ciphertext with 3 handles validity proofs.
  • 50bd126: Added the actual 3 handles validity proof instructions in the zk-token-proof program.
  • 0549bd9: Updated tests and bench code.

Fixes #

@samkim-crypto samkim-crypto force-pushed the zk-token-sdk/instructions-3-handles branch from 8d97bf7 to 85b7ee3 Compare April 19, 2024 07:02
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 76.64234% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (ff79531) to head (fbc34df).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #897    +/-   ##
========================================
  Coverage    81.8%    81.9%            
========================================
  Files         853      855     +2     
  Lines      231812   231956   +144     
========================================
+ Hits       189848   189989   +141     
- Misses      41964    41967     +3     

@samkim-crypto samkim-crypto marked this pull request as ready for review April 19, 2024 20:35
@samkim-crypto samkim-crypto requested a review from joncinque April 19, 2024 20:35
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just some tiny points

Choose a reason for hiding this comment

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

It's really great how simple these tests are, and a sign that the code is well factored!

let auditor_keypair = ElGamalKeypair::new_rand();
let auditor_pubkey = auditor_keypair.pubkey();

let amount: u64 = 55;

Choose a reason for hiding this comment

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

For the benches, how important is the amount encrypted? As in, do large numbers make things take longer, or is it inconsequential compared to all the other calculations that need to be done?

Copy link
Author

Choose a reason for hiding this comment

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

Yep it is inconsequential since the proof only certifies that a ciphertext is well-formed. If it were, then the proof would leak information about the encrypted amount since anyone can try verifying the proof and measure the time!

programs/zk-token-proof/src/lib.rs Outdated Show resolved Hide resolved
programs/zk-token-proof/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 316 to 326
/// Accounts expected by this instruction:
///
/// * Creating a proof context account
/// 0. `[writable]` The proof context account
/// 1. `[]` The proof context account owner
///
/// * Otherwise
/// None
///
/// Data expected by this instruction:
/// `GroupedCiphertextValidityProofContext`

Choose a reason for hiding this comment

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

This doesn't mention that it's possible to read from account data, like the other instructions. Can you add that in?

Copy link
Author

Choose a reason for hiding this comment

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

Oh yes, I totally missed that! Thanks!

Comment on lines 339 to 347
/// * Creating a proof context account
/// 0. `[writable]` The proof context account
/// 1. `[]` The proof context account owner
///
/// * Otherwise
/// None
///
/// Data expected by this instruction:
/// `BatchedGroupedCiphertextValidityProofContext`

Choose a reason for hiding this comment

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

Same here, can you point out that it's possible to use account data?

@samkim-crypto samkim-crypto force-pushed the zk-token-sdk/instructions-3-handles branch from 9f6ae10 to e19bbda Compare April 21, 2024 00:29
@samkim-crypto samkim-crypto force-pushed the zk-token-sdk/instructions-3-handles branch from e19bbda to 61a2183 Compare April 21, 2024 23:38
@samkim-crypto samkim-crypto requested a review from joncinque April 22, 2024 01:52
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Sorry just one last little nit, then this is good to go

zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/zk_token_proof_instruction.rs Outdated Show resolved Hide resolved
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

@samkim-crypto samkim-crypto merged commit 8cca3f9 into anza-xyz:master Apr 22, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants