noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
821 stars 177 forks source link

chore: add back Pedersen blackbox functions (revert PR 5221) #5318

Closed guipublic closed 6 days ago

guipublic commented 6 days ago

Description

Problem*

This PR adds (back) the blackbox functions for Pedersen hash/commitment, until we get a similar circuit (in term of number of gates) with the Noir implementation.

Summary*

The Noir version is kept and assert to ouput the same value in a test.

Additional Context

This PR reverts the PR #5221

Documentation*

Check one:

PR Checklist*

github-actions[bot] commented 6 days ago

Changes to circuit sizes

Generated at commit: a961c427c04f8c612d00434982620d612b6c3c54, compared to commit: 42d727feb5ce38f50da7daaf4279ae88edde61b4

๐Ÿงพ Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
pedersen_commitment -25 โœ… -69.44% +24,623 โŒ +596.34%
pedersen_hash -40 โœ… -95.24% +23,352 โŒ +432.44%

Full diff report ๐Ÿ‘‡
| Program | ACIR opcodes (+/-) | % | Circuit size (+/-) | % | |:-|-:|-:|-:|-:| | **pedersen_commitment** | 11 (-25) | **-69.44%** | 28,752 (+24,623) | **+596.34%** | | **pedersen_hash** | 2 (-40) | **-95.24%** | 28,752 (+23,352) | **+432.44%** | | **strings** | 38 (-3) | **-7.32%** | 14,383 (+10,789) | **+300.19%** | | **pedersen_check** | 38 (-70) | **-64.81%** | 28,868 (+20,547) | **+246.93%** | | **simple_shield** | 52 (-173) | **-76.89%** | 29,042 (+13,117) | **+82.37%** | | **merkle_insert** | 1,779 (-195) | **-9.88%** | 29,042 (+8,862) | **+43.91%** | | **schnorr** | 526 (-113) | **-17.68%** | 54,398 (+116) | **+0.21%** | | **import** | 5 (-10) | **-66.67%** | 20 (-2,770) | **-99.28%** |
TomAFrench commented 6 days ago

The test case is fixed by this commit https://github.com/AztecProtocol/aztec-packages/pull/7134/commits/b77204a75c0bbe0850994e37e6f6ab21c9478446

@guipublic can you copy that across into this PR?

TomAFrench commented 6 days ago

Ah, I forgot that the separator needs to be a comptime thing. We'll have to remove that argument but can you add another instance with a non-zero separator?