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-proof] Round compute units to nice numbers #33910

Merged
merged 1 commit into from
Oct 27, 2023
Merged

[zk-token-proof] Round compute units to nice numbers #33910

merged 1 commit into from
Oct 27, 2023

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Oct 27, 2023

Problem

The compute units that are assigned to each zk-token-proof instructions are very specific numbers that are computed by benching the instructions on dev machines and assigning 1 CU per 33ns. Since these numbers are approximate anyway, it would be nicer to round off these CUs for easier communication as pointed out in #31788 (comment).

Summary of Changes

Rounded off the numbers for the zk-token-proof CUs. For the u128 range proof compute units, I went a little aggressive with the rounding since it will be really nice to fit that under the 200k compute budget limit.

Note: the zk-token-proof program has not yet been activated on any of the servers.

Fixes #

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

codecov bot commented Oct 27, 2023

Codecov Report

Merging #33910 (68a69f8) into master (1814b2b) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #33910     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         809      809             
  Lines      218062   218062             
=========================================
- Hits       178672   178649     -23     
- Misses      39390    39413     +23     

@samkim-crypto samkim-crypto added v1.17 PRs that should be backported to v1.17 and removed work in progress This isn't quite right yet labels Oct 27, 2023
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm - sounds a good idea 👍🏼

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 b0bf24b into solana-labs:master Oct 27, 2023
20 checks passed
mergify bot pushed a commit that referenced this pull request Oct 27, 2023
round zk-token-proof compute units to nice numbers

(cherry picked from commit b0bf24b)
samkim-crypto added a commit that referenced this pull request Oct 28, 2023
… of #33910) (#33915)

[zk-token-proof] Round compute units to nice numbers (#33910)

round zk-token-proof compute units to nice numbers

(cherry picked from commit b0bf24b)

Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
… of solana-labs#33910) (solana-labs#33915)

[zk-token-proof] Round compute units to nice numbers (solana-labs#33910)

round zk-token-proof compute units to nice numbers

(cherry picked from commit b0bf24b)

Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
… of solana-labs#33910) (solana-labs#33915)

[zk-token-proof] Round compute units to nice numbers (solana-labs#33910)

round zk-token-proof compute units to nice numbers

(cherry picked from commit b0bf24b)

Co-authored-by: samkim-crypto <skim13@cs.stanford.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants