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] Add benches for proof instructions #32071

Merged
merged 12 commits into from
Jun 17, 2023
Merged

[zk-token-proof] Add benches for proof instructions #32071

merged 12 commits into from
Jun 17, 2023

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jun 12, 2023

Problem

The instructions for the zk-token-proof program have been assigned compute units, but there is no benchmark tests for these instructions.

Summary of Changes

Added benchmark code for the zk-token-proof program.

Fixes #

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

codecov bot commented Jun 12, 2023

Codecov Report

Merging #32071 (c8c4e89) into master (de024bf) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #32071     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         768      768             
  Lines      209118   209118             
=========================================
- Hits       171387   171357     -30     
- Misses      37731    37761     +30     

@samkim-crypto
Copy link
Contributor Author

@taozhu-chicago, can I ask you for some advise in this PR?

I wanted to add the benchmarks that I used test and assign the compute units for the zk-token-proof instructions.

The processor for these instructions basically works as follows:

  1. Verify the zk proof (here)
  2. (optionally) Stores some data in an account if the proof succeeds here)

Currently, the benchmark code that I added in this PR tests step 1. Zkp's are expensive, so I imagine step 2 above is going to be very small compared to step 1, but do you think the bench also take into account step 2 as well (and CU's updated)? If so, are there examples of benchmarking code that tests native programs copying data into an account?

@tao-stones
Copy link
Contributor

so I imagine step 2 above is going to be very small compared to step 1, but do you think the bench also take into account step 2 as well (and CU's updated)? If so, are there examples of benchmarking code that tests native programs copying data into an account?

I would also assume writing some data into account would have relatively small cost. iirc @alessandrod did bench tests on loading account with different size of data, loading small amount of data (less than few Kilo bytes ?) costs almost nothing; but as data grew larger, the cost increase exponentially. I am not aware of other bench test on native programs copying data to account.

@samkim-crypto
Copy link
Contributor Author

Had a direct communication with @alessandrod. The cost of storing context data into a specified context data account should be free given that:

  • The context data is at most 650 bytes
  • The context state account should be pre-allocated via the system program and the proof program simply writes data to this account. The cost of writing this data to the account should already be accounted for in the system program.

For this PR, I will just include the benchmark code. On a follow-up PR, I will address:

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

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Should we add these to CI?

@yihau
Copy link
Contributor

yihau commented Jun 16, 2023

I think if we want to add this one to CI. part1.sh and part2.sh are both okay to be the candidate. their test time are similar. btw, we don't do too much thing to those benchmark results so I don't have too much can share atm. we can add them first and rerog later!

@samkim-crypto
Copy link
Contributor Author

Okay thanks! I will add them to to the CI.

@yihau One question is, does the format of the output of the benches to append to the logs important? The bench scripts in the ci outputs json format, but I can't seem to find that option available with criterion without using the cargo-criterion cli tool. Maybe for this PR, I can just add the scripts to run the benchmark tests and then we can address the logs on a separate PR?

@yihau
Copy link
Contributor

yihau commented Jun 17, 2023

sure. afaik banking-bench and accounts-bench are not json output format either. 🪖

@yihau
Copy link
Contributor

yihau commented Jun 17, 2023

the CI part looks good to me 🫶

@samkim-crypto samkim-crypto merged commit 875b95a into solana-labs:master Jun 17, 2023
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023
* add bench test for `VerifyPubkeyValidity`

* add bench test for `VerifyRangeProofU64`

* add bench test for `VerifyWithdraw`

* add bench test for `VerifyZeroBalance`

* add bench test for `VerifyGroupedCiphertextValidity`

* add bench test for `VerifyCiphertextCommitmentEquality`

* add bench test for `VerifyCiphertextCiphertextEquality`

* add bench test for `VerifyBatchedGroupedCiphertextValidity`

* add bench test for `VerifyBatchedRangeProofu64`, `VerifyBatchedRangeProofU128`, `VerifyBatchedRangeProofU256`

* add bench test for `VerifyTransfer` and `VerifyTransferWithFee`

* use add `criterion` to workspace cargo

* add bench to ci
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023
* add bench test for `VerifyPubkeyValidity`

* add bench test for `VerifyRangeProofU64`

* add bench test for `VerifyWithdraw`

* add bench test for `VerifyZeroBalance`

* add bench test for `VerifyGroupedCiphertextValidity`

* add bench test for `VerifyCiphertextCommitmentEquality`

* add bench test for `VerifyCiphertextCiphertextEquality`

* add bench test for `VerifyBatchedGroupedCiphertextValidity`

* add bench test for `VerifyBatchedRangeProofu64`, `VerifyBatchedRangeProofU128`, `VerifyBatchedRangeProofU256`

* add bench test for `VerifyTransfer` and `VerifyTransferWithFee`

* use add `criterion` to workspace cargo

* add bench to ci
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Aug 15, 2023
* add bench test for `VerifyPubkeyValidity`

* add bench test for `VerifyRangeProofU64`

* add bench test for `VerifyWithdraw`

* add bench test for `VerifyZeroBalance`

* add bench test for `VerifyGroupedCiphertextValidity`

* add bench test for `VerifyCiphertextCommitmentEquality`

* add bench test for `VerifyCiphertextCiphertextEquality`

* add bench test for `VerifyBatchedGroupedCiphertextValidity`

* add bench test for `VerifyBatchedRangeProofu64`, `VerifyBatchedRangeProofU128`, `VerifyBatchedRangeProofU256`

* add bench test for `VerifyTransfer` and `VerifyTransferWithFee`

* use add `criterion` to workspace cargo

* add bench to ci
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.

4 participants