Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Conversation

@vadorovsky
Copy link
Contributor

@vadorovsky vadorovsky commented Aug 2, 2023

Problem

Computing Poseidon[0] hashes is too expensive to be done in a Solana program in one transaction. Poseidon is a zero-knowlege proof friendly hash function, used by the majority of ZK-based projects, including the ones built on top of Solana.

Summary of Changes

This change introduces the sol_poseidon syscall which takes 2D byte slice as an input and then calculates a Poseidon hash using a BN254 curve and the following Poseidon parameters:

  • x^5 S-boxes
  • width - 2 ≤ t ≤ 13
  • inputs - 1 ≤ n ≤ 12
  • 8 full rounds and partial rounds depending on t: [56, 57, 56, 60, 60, 63, 64, 63, 60, 66, 60, 65]

Computation of Poseidon hashes is done with the light-poseidon[1] crate, which is audited[2] and compatible with Circom[3] (BN254 curve, the same parameters and constants).

Proposed compute costs depend on number of inputs and are based on light-poseidon benchmarks[4].

[0] https://www.poseidon-hash.info/
[1] https://crates.io/crates/light-poseidon
[2] https://github.com/Lightprotocol/light-poseidon/blob/main/assets/audit.pdf
[3] https://docs.circom.io/
[4] https://github.com/Lightprotocol/light-poseidon/tree/main#performance

@mergify mergify bot added community Community contribution need:merge-assist labels Aug 2, 2023
@mergify mergify bot requested a review from a team August 2, 2023 01:32
@vadorovsky vadorovsky marked this pull request as draft August 7, 2023 10:54
@joncinque joncinque requested a review from samkim-crypto August 9, 2023 12:02
@vadorovsky vadorovsky marked this pull request as ready for review August 14, 2023 22:20
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 18, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 18, 2023
@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 18, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 18, 2023
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #32680 (de957c2) into master (9d83bb2) will decrease coverage by 0.1%.
The diff coverage is 51.6%.

@@            Coverage Diff            @@
##           master   #32680     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         784      785      +1     
  Lines      212449   212590    +141     
=========================================
+ Hits       174270   174328     +58     
- Misses      38179    38262     +83     

@samkim-crypto
Copy link
Contributor

The syscall implementation looks good to me overall! On my dev machine, it seems like I am getting slightly higher bench numbers, but the CU units do seem reasonable.

I could be missing something, but the PR does seem to make some changes that seem tangential to the Poseidon syscall. Can you rebase with the master branch?

@vadorovsky
Copy link
Contributor Author

vadorovsky commented Aug 21, 2023

I could be missing something, but the PR does seem to make some changes that seem tangential to the Poseidon syscall. Can you rebase with the master branch?

If you mean the changes related to ComputeBudget (and passing references of it) - https://github.com/solana-labs/solana/pull/32680/files#diff-9ac236825ce9b87946d6820e3ec3096bc841818c66037b38b6d98142318f4b0a - then the change is intentionally done here. There are no rebase conflicts in the diff.

To handle different CU costs for different number of inputs, I included the poseidon_cost as a HashMap field in ComputeBudget: https://github.com/solana-labs/solana/pull/32680/files#diff-9ac236825ce9b87946d6820e3ec3096bc841818c66037b38b6d98142318f4b0a

Structs with HashMap cannot implement the Copy trait, so I had to remove it here. And since ComputeBudget doesn't implement Copy anymore, when it's passed as an argument to functions, it can be either moved or passed as a reference. In some places, it was easier to go with the latter (the former annoys the borrow checker) and some places needed making an explicit copy.

I think that removing the Copy implementation is generally a good change - ComputeBudget doesn't get copied each time we pass it to some function, and it's not a small struct. But if you feel like it's too intrusive change, I can think of some other way of including the Poseidon CU (will be probably less convenient than a HashMap though).

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 22, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 22, 2023
Copy link
Contributor

@samkim-crypto samkim-crypto 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 to me overall. Please let me know what you think about my comments below. Also, I think the changes regarding the runtime are indeed good changes, but it probably belongs to separate PR. Let's keep this PR focused on the Poseidon syscall and then we can perhaps have a follow-up for the run time changes (we can also possibly remove the need for hashtables; see below).

@vadorovsky
Copy link
Contributor Author

@samkim-crypto Thanks for review! Your comments should be addressed now.

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 30, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 30, 2023
Copy link
Contributor

@samkim-crypto samkim-crypto 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 to me! Just some nits. I will approve it once these are addressed.

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 30, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 30, 2023
@samkim-crypto
Copy link
Contributor

Looks good to me thanks! There seems to be a conflict with the main branch, so let's resolve that and merge it in.

Computing Poseidon[0] hashes is too expensive to be done in a Solana
program in one transaction. Poseidon is a zero-knowlege proof friendly
hash function, used by the majority of ZK-based projects, including the
ones built on top of Solana.

This change introduces the `sol_poseidon` syscall which takes 2D byte
slice as an input and then calculates a Poseidon hash using a BN254
curve and the following Poseidon parameters:

* x^5 S-boxes
* width - 2 ≤ t ≤ 13
* inputs - 1 ≤ n ≤ 12
* 8 full rounds and partial rounds depending on t: [56, 57, 56, 60, 60,
  63, 64, 63, 60, 66, 60, 65]

Computation of Poseidon hashes is done with the light-poseidon[1]
crate, which is audited[2] and compatible with Circom[3] (BN254 curve,
the same parameters and constants).

Proposed compute costs depend on number of inputs and are based on
light-poseidon benchmarks[4].

[0] https://www.poseidon-hash.info/
[1] https://crates.io/crates/light-poseidon
[2] https://github.com/Lightprotocol/light-poseidon/blob/main/assets/audit.pdf
[3] https://docs.circom.io/
[4] https://github.com/Lightprotocol/light-poseidon/tree/main#performance
@vadorovsky
Copy link
Contributor Author

@samkim-crypto @SwenSchaeferjohann Thanks for reviews! Hopefully the last push.

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Aug 30, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Aug 30, 2023
@samkim-crypto
Copy link
Contributor

Great! Will merge once CI completes. Thank you for all the changes!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Community contribution need:merge-assist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants