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 range proof generation error types #34065

Merged
merged 8 commits into from
Nov 21, 2023
Merged

[zk-token-sdk] Add range proof generation error types #34065

merged 8 commits into from
Nov 21, 2023

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Nov 14, 2023

Problem

#33507, #33509, #33510, and #33525.

Summary of Changes

Fixes #

@samkim-crypto samkim-crypto added the work in progress This isn't quite right yet label Nov 14, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Merging #34065 (7d9d95a) into master (0e6dd54) will increase coverage by 0.0%.
The diff coverage is 86.4%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34065   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         819      819           
  Lines      219145   219162   +17     
=======================================
+ Hits       179529   179561   +32     
+ Misses      39616    39601   -15     

@samkim-crypto samkim-crypto removed the work in progress This isn't quite right yet label Nov 17, 2023
Copy link
Contributor

@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 good for the most part! For backporting, we should just focus on the bits required by the runtime, ie. the new checks on the verification functions. We can probably leave off all the client-side bits

zk-token-sdk/src/range_proof/inner_product.rs Show resolved Hide resolved
Comment on lines 38 to 43
#[derive(Error, Clone, Debug, Eq, PartialEq)]
pub enum RangeProofGeneratorError {
#[error("maximum generator length exceeded")]
MaximumGeneratorLengthExceeded,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this error over RangeProofGenerationError? They both have MaximumGeneratorLengthExceeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, both proof generation and verification uses a subroutine to generate "group generators" that could return an if the generator length is too large. I made it so that if the subroutine errors during proof generation, it returns RangeProofGenerationError::Maximum...Exceeded (here) and if it errors during verification, it returns RangeProofVerificationError::Maximum...Exceeded (here).

zk-token-sdk/src/range_proof/generators.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/range_proof/generators.rs Outdated Show resolved Hide resolved
zk-token-sdk/src/range_proof/inner_product.rs Show resolved Hide resolved
Comment on lines +108 to +109
// `j` is guaranteed to be at most `u64::BITS` (a 6-bit number) and therefore,
// casting is lossless and right shift can be safely unwrapped
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

zk-token-sdk/src/range_proof/util.rs Show resolved Hide resolved
zk-token-sdk/src/range_proof/util.rs Outdated Show resolved Hide resolved
joncinque
joncinque previously approved these changes Nov 20, 2023
Copy link
Contributor

@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 ded278f into solana-labs:master Nov 21, 2023
32 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.

2 participants