feat: refactor pedersen hash standard by zac-williamson · Pull Request #2592 · AztecProtocol/aztec-packages

zac-williamson · 2023-09-29T11:21:33Z

This PR is a follow up to #1945 and our Pedersen hash refactor project https://hackmd.io/XYBiWhHPT9C1bo4nrtoo0A?view

The PR updates all existing usage of Pedersen commitments and Pedersen hashes to use the new, more straightforward definition in the hackmd and implemented in #1945

This requires wide changes to the codebase as the barretenberg interface used for the Pedersen hash has changed.

The large static generator lists that were computed for every barretenberg process have been removed, and replaced with a streamlined generator_data class

All uses of pedersen::compress have been removed and replaced with pedersen::hash

We should no longer ever take the x-coordinate of pedersen::commit outside of pedersen::hash`.

The Schnorr signature stdlib method now uses cycle_group instead of its own independent scalar multiplication method

The ACIR fixed-base scalar mul opcode now actually evaluates a fixed-base scalar mul

All of the code that implemented the old Pedersen functionality has been deleted

The stdlib class point has been deleted. All code instances that used point now use cycle_group (don't want two independent types in the stdlib that both represent embedded elliptic curve points)

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

added variable-base method that isn't quite passing tests. tests will fail ~33% of the time!

… infinity! mul function should now be "computationally" complete in the case of an honest Prover (prob. of triggering unsatisfiable constraints from incomplete addition formulae is equiv to solving the dlog problem, for inputs that include points at infinity, duplicates and scalar multipliers that are zero)

…tisation+circuit builder

cycle_group::fixed_base_scalar_mul now working with plookup tables

… tests)

…up-msm

Added TODOs for code that needs to be culled post-refactor removed custom MSM algorithms from schnorr.tcc + reduced constraint cost by ~50%

kevaundray · 2023-10-23T10:40:23Z

+    static crypto::GeneratorContext<curve::Grumpkin> get_generator_context(const size_t hash_index)
    {
-        return plonk::stdlib::pedersen_commitment<Builder>::compress(inputs, hash_index);
+        crypto::GeneratorContext<curve::Grumpkin> result;


Here is where we were previously adding an Aztec domain string and using generatorIndexDomain

kevaundray · 2023-10-23T10:41:31Z

+    static crypto::GeneratorContext<curve::Grumpkin> get_generator_context(const size_t hash_index)
    {
-        return crypto::pedersen_commitment::compress_native(inputs, hash_index);
+        crypto::GeneratorContext<curve::Grumpkin> result;


same comment as in the circuit_types file: https://github.com/AztecProtocol/aztec-packages/pull/2592/files#r1368467887

kevaundray · 2023-10-23T10:42:53Z

      functionData = new FunctionData(FunctionSelector.empty(), false, false, false);
-      amount = new Fr(140);
-      params = [amount, Fr.random()];
+      amount = new Fr(1);
+      params = [amount, new Fr(1)];
      wasm = await CircuitsWasm.get();


This should be changed back

kevaundray · 2023-10-23T10:46:32Z

+  const resultPtr = Buffer.from(wasm.getMemorySlice(0, 4)).readUInt32LE(0);
+  // First 4 bytes is full response length in byters.
+  // Second 4 bytes is vector length in fields.
+  const resultNumFields = Buffer.from(wasm.getMemorySlice(resultPtr + 4, resultPtr + 8)).readUInt32BE(0);


seems we are mixing LE and BE here -- haven't checked why, just noting

kevaundray · 2023-10-23T10:47:56Z

-ALICE="0x25048e8c1b7dea68053d597ac2d920637c99523651edfb123d0632da785970d0"
-BOB="0x115f123bbc6cc6af9890055821cfba23a7c4e8832377a32ccb719a1ba3a86483"
+ALICE="0x0f394e8bd156e15153376a711e3054821c2a1c1047dcfb3745d636a57fb42ab1"
+BOB="0x2b67f90f0044596190644ddafea4152de47bd4781559493860fa7358e19d090a"


The address is reliant on a pedersen hash and since the hash changed, the address, contract address and secret hash all changed with it

kevaundray · 2023-10-23T10:48:19Z

      }
    }
-  }, 60_000);
+  }, 360_000);


These will be changed back -- using an oracle increased the time for tests

kevaundray · 2023-10-23T10:49:10Z

  public async key(): Promise<Fr> {
    return Fr.fromBuffer(
-      pedersenPlookupCommitInputs(
+      pedersenCompressInputs(


This is done in many places, compressInputs is still a janky name -- I will change it in a separate PR to hash

kevaundray · 2023-10-23T10:50:03Z

      const tree = await createDb(db, pedersen, 'test', 32);
      const root = tree.getRoot(false);
-      expect(root.toString('hex')).toEqual('20efbe2c7b675f26ab71689279908bbab33a6963e7e0dcb80e4c46583d094113');
+      expect(root.toString('hex')).toEqual('16642d9ccd8346c403aa4c3fa451178b22534a27035cdaa6ec34ae53b29c50cb');


Again since we changed the hash, the merkle root in this test would also change because we use pedersen hashing for the merkle tree implementation

kevaundray · 2023-10-23T12:15:58Z

    read(inputs_buffer, to_compress);
-    auto r = crypto::pedersen_commitment::compress_native(to_compress, ntohl(*hash_index));
+    const size_t generator_offset = ntohl(*hash_index);
+    crypto::GeneratorContext<curve::Grumpkin> ctx; // todo fix


Note: TODO -- not really sure what this fix is referring to

Ah I thought I got rid of that. There's nothing to fix here I just wasn't happy with the interface

codygunton

You love to see so much cruft go away. At @kevaundray's request I had a look at relations tests and plookup tables, and I didn't find any issues. We'll have to dig deep into this next year, but for now LGTM

sirasistant · 2023-10-24T09:13:08Z

Fixed master merge issues, reenabling auto-merge

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.10.0</summary> ## [0.10.0](aztec-packages-v0.9.0...aztec-packages-v0.10.0) (2023-10-24) ### ⚠ BREAKING CHANGES * Emitting encrypted log by default ([#2926](#2926)) ### Features * Added register-account command to cli ([#2980](#2980)) ([0977a90](0977a90)) * **docs:** Fix portals tutorial formatting ([#2929](#2929)) ([ab19b67](ab19b67)) * Emitting encrypted log by default ([#2926](#2926)) ([1ea2d4f](1ea2d4f)), closes [#2912](#2912) * Goblin translator non-native field relation (Goblin Translator part 6) ([#2871](#2871)) ([c4d8d96](c4d8d96)) * Honk profiling by pass, tsan preset ([#2982](#2982)) ([a1592fd](a1592fd)) * Incorporate docs feedback and add "intermediate" level intros to some pages ([#2598](#2598)) ([78f9f52](78f9f52)) * Nuking `Pokeable` contract ([#2939](#2939)) ([583d6fb](583d6fb)) * Protogalaxy Combiner ([#2436](#2436)) ([a60c70d](a60c70d)) * Protogalaxy perturbator! ([#2624](#2624)) ([509dee6](509dee6)) * Refactor pedersen hash standard ([#2592](#2592)) ([3085676](3085676)) * Widget benchmarking ([#2897](#2897)) ([0e927e9](0e927e9)) ### Bug Fixes * Add @jest/types to box deps ([#2903](#2903)) ([db3fa62](db3fa62)) * Add lint rule for focused tests ([#2901](#2901)) ([fd1a1a8](fd1a1a8)) * Avoid tsc OOM by unignoring an old contract artifact ([#2932](#2932)) ([7310600](7310600)) * Bad it.only in tests ([#2900](#2900)) ([a1f3af1](a1f3af1)) * Boxes boostrap dont use ts-node directly and add .prettierignore ([#2890](#2890)) ([a3b1804](a3b1804)) * Confusing "Unknown complete address" error ([#2967](#2967)) ([3a8f54a](3a8f54a)) * Force jest to quit, otherwise CI can rack up to 3hrs of credits per job. ([#2899](#2899)) ([ba2f671](ba2f671)) * Honk sumcheck performance ([#2925](#2925)) ([5fbfe6e](5fbfe6e)) * Pending commitments contract using the wrong number of arguments ([#2959](#2959)) ([655c322](655c322)) * Prettierignore in boxes ([#2902](#2902)) ([8f7a200](8f7a200)) * Randomness in `AddressNote` ([#2965](#2965)) ([4dc49a9](4dc49a9)) * Yarn lock ([#2923](#2923)) ([7042bc6](7042bc6)) ### Miscellaneous * `Private Data Tree` --> `Note Hash Tree` ([#2945](#2945)) ([abaec9c](abaec9c)), closes [#2906](#2906) * Apply hash abstraction over aztec-nr ([#2958](#2958)) ([52f01ae](52f01ae)) * **docs:** Add Singleton and ImmutableSingleton `view_note` methods ([#2934](#2934)) ([c1497f8](c1497f8)) * Fix box frontend styling ([#2919](#2919)) ([7e9e8cc](7e9e8cc)) * Less noisy benchmark reports ([#2916](#2916)) ([0df166c](0df166c)) * Remove unused nix files ([#2933](#2933)) ([3174f84](3174f84)) * Run all e2e tests against sandbox ([#2891](#2891)) ([6c4e26c](6c4e26c)) * Token box copies noir source files from noir-contracts on bootstrap ([#2940](#2940)) ([a467b96](a467b96)) ### Documentation * Fix: update cheat codes to connect to ethRpcUrl ([#2922](#2922)) ([4ffe9be](4ffe9be)) </details> <details><summary>barretenberg.js: 0.10.0</summary> ## [0.10.0](barretenberg.js-v0.9.0...barretenberg.js-v0.10.0) (2023-10-24) ### Features * Refactor pedersen hash standard ([#2592](#2592)) ([3085676](3085676)) </details> <details><summary>barretenberg: 0.10.0</summary> ## [0.10.0](barretenberg-v0.9.0...barretenberg-v0.10.0) (2023-10-24) ### Features * Goblin translator non-native field relation (Goblin Translator part 6) ([#2871](#2871)) ([c4d8d96](c4d8d96)) * Honk profiling by pass, tsan preset ([#2982](#2982)) ([a1592fd](a1592fd)) * Protogalaxy Combiner ([#2436](#2436)) ([a60c70d](a60c70d)) * Protogalaxy perturbator! ([#2624](#2624)) ([509dee6](509dee6)) * Refactor pedersen hash standard ([#2592](#2592)) ([3085676](3085676)) * Widget benchmarking ([#2897](#2897)) ([0e927e9](0e927e9)) ### Bug Fixes * Honk sumcheck performance ([#2925](#2925)) ([5fbfe6e](5fbfe6e)) ### Miscellaneous * Remove unused nix files ([#2933](#2933)) ([3174f84](3174f84)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.10.0</summary> ## [0.10.0](AztecProtocol/aztec-packages@aztec-packages-v0.9.0...aztec-packages-v0.10.0) (2023-10-24) ### ⚠ BREAKING CHANGES * Emitting encrypted log by default ([#2926](AztecProtocol/aztec-packages#2926)) ### Features * Added register-account command to cli ([#2980](AztecProtocol/aztec-packages#2980)) ([0977a90](AztecProtocol/aztec-packages@0977a90)) * **docs:** Fix portals tutorial formatting ([#2929](AztecProtocol/aztec-packages#2929)) ([ab19b67](AztecProtocol/aztec-packages@ab19b67)) * Emitting encrypted log by default ([#2926](AztecProtocol/aztec-packages#2926)) ([1ea2d4f](AztecProtocol/aztec-packages@1ea2d4f)), closes [#2912](AztecProtocol/aztec-packages#2912) * Goblin translator non-native field relation (Goblin Translator part 6) ([#2871](AztecProtocol/aztec-packages#2871)) ([c4d8d96](AztecProtocol/aztec-packages@c4d8d96)) * Honk profiling by pass, tsan preset ([#2982](AztecProtocol/aztec-packages#2982)) ([a1592fd](AztecProtocol/aztec-packages@a1592fd)) * Incorporate docs feedback and add "intermediate" level intros to some pages ([#2598](AztecProtocol/aztec-packages#2598)) ([78f9f52](AztecProtocol/aztec-packages@78f9f52)) * Nuking `Pokeable` contract ([#2939](AztecProtocol/aztec-packages#2939)) ([583d6fb](AztecProtocol/aztec-packages@583d6fb)) * Protogalaxy Combiner ([#2436](AztecProtocol/aztec-packages#2436)) ([a60c70d](AztecProtocol/aztec-packages@a60c70d)) * Protogalaxy perturbator! ([#2624](AztecProtocol/aztec-packages#2624)) ([509dee6](AztecProtocol/aztec-packages@509dee6)) * Refactor pedersen hash standard ([#2592](AztecProtocol/aztec-packages#2592)) ([3085676](AztecProtocol/aztec-packages@3085676)) * Widget benchmarking ([#2897](AztecProtocol/aztec-packages#2897)) ([0e927e9](AztecProtocol/aztec-packages@0e927e9)) ### Bug Fixes * Add @jest/types to box deps ([#2903](AztecProtocol/aztec-packages#2903)) ([db3fa62](AztecProtocol/aztec-packages@db3fa62)) * Add lint rule for focused tests ([#2901](AztecProtocol/aztec-packages#2901)) ([fd1a1a8](AztecProtocol/aztec-packages@fd1a1a8)) * Avoid tsc OOM by unignoring an old contract artifact ([#2932](AztecProtocol/aztec-packages#2932)) ([7310600](AztecProtocol/aztec-packages@7310600)) * Bad it.only in tests ([#2900](AztecProtocol/aztec-packages#2900)) ([a1f3af1](AztecProtocol/aztec-packages@a1f3af1)) * Boxes boostrap dont use ts-node directly and add .prettierignore ([#2890](AztecProtocol/aztec-packages#2890)) ([a3b1804](AztecProtocol/aztec-packages@a3b1804)) * Confusing "Unknown complete address" error ([#2967](AztecProtocol/aztec-packages#2967)) ([3a8f54a](AztecProtocol/aztec-packages@3a8f54a)) * Force jest to quit, otherwise CI can rack up to 3hrs of credits per job. ([#2899](AztecProtocol/aztec-packages#2899)) ([ba2f671](AztecProtocol/aztec-packages@ba2f671)) * Honk sumcheck performance ([#2925](AztecProtocol/aztec-packages#2925)) ([5fbfe6e](AztecProtocol/aztec-packages@5fbfe6e)) * Pending commitments contract using the wrong number of arguments ([#2959](AztecProtocol/aztec-packages#2959)) ([655c322](AztecProtocol/aztec-packages@655c322)) * Prettierignore in boxes ([#2902](AztecProtocol/aztec-packages#2902)) ([8f7a200](AztecProtocol/aztec-packages@8f7a200)) * Randomness in `AddressNote` ([#2965](AztecProtocol/aztec-packages#2965)) ([4dc49a9](AztecProtocol/aztec-packages@4dc49a9)) * Yarn lock ([#2923](AztecProtocol/aztec-packages#2923)) ([7042bc6](AztecProtocol/aztec-packages@7042bc6)) ### Miscellaneous * `Private Data Tree` --> `Note Hash Tree` ([#2945](AztecProtocol/aztec-packages#2945)) ([abaec9c](AztecProtocol/aztec-packages@abaec9c)), closes [#2906](AztecProtocol/aztec-packages#2906) * Apply hash abstraction over aztec-nr ([#2958](AztecProtocol/aztec-packages#2958)) ([52f01ae](AztecProtocol/aztec-packages@52f01ae)) * **docs:** Add Singleton and ImmutableSingleton `view_note` methods ([#2934](AztecProtocol/aztec-packages#2934)) ([c1497f8](AztecProtocol/aztec-packages@c1497f8)) * Fix box frontend styling ([#2919](AztecProtocol/aztec-packages#2919)) ([7e9e8cc](AztecProtocol/aztec-packages@7e9e8cc)) * Less noisy benchmark reports ([#2916](AztecProtocol/aztec-packages#2916)) ([0df166c](AztecProtocol/aztec-packages@0df166c)) * Remove unused nix files ([#2933](AztecProtocol/aztec-packages#2933)) ([3174f84](AztecProtocol/aztec-packages@3174f84)) * Run all e2e tests against sandbox ([#2891](AztecProtocol/aztec-packages#2891)) ([6c4e26c](AztecProtocol/aztec-packages@6c4e26c)) * Token box copies noir source files from noir-contracts on bootstrap ([#2940](AztecProtocol/aztec-packages#2940)) ([a467b96](AztecProtocol/aztec-packages@a467b96)) ### Documentation * Fix: update cheat codes to connect to ethRpcUrl ([#2922](AztecProtocol/aztec-packages#2922)) ([4ffe9be](AztecProtocol/aztec-packages@4ffe9be)) </details> <details><summary>barretenberg.js: 0.10.0</summary> ## [0.10.0](AztecProtocol/aztec-packages@barretenberg.js-v0.9.0...barretenberg.js-v0.10.0) (2023-10-24) ### Features * Refactor pedersen hash standard ([#2592](AztecProtocol/aztec-packages#2592)) ([3085676](AztecProtocol/aztec-packages@3085676)) </details> <details><summary>barretenberg: 0.10.0</summary> ## [0.10.0](AztecProtocol/aztec-packages@barretenberg-v0.9.0...barretenberg-v0.10.0) (2023-10-24) ### Features * Goblin translator non-native field relation (Goblin Translator part 6) ([#2871](AztecProtocol/aztec-packages#2871)) ([c4d8d96](AztecProtocol/aztec-packages@c4d8d96)) * Honk profiling by pass, tsan preset ([#2982](AztecProtocol/aztec-packages#2982)) ([a1592fd](AztecProtocol/aztec-packages@a1592fd)) * Protogalaxy Combiner ([#2436](AztecProtocol/aztec-packages#2436)) ([a60c70d](AztecProtocol/aztec-packages@a60c70d)) * Protogalaxy perturbator! ([#2624](AztecProtocol/aztec-packages#2624)) ([509dee6](AztecProtocol/aztec-packages@509dee6)) * Refactor pedersen hash standard ([#2592](AztecProtocol/aztec-packages#2592)) ([3085676](AztecProtocol/aztec-packages@3085676)) * Widget benchmarking ([#2897](AztecProtocol/aztec-packages#2897)) ([0e927e9](AztecProtocol/aztec-packages@0e927e9)) ### Bug Fixes * Honk sumcheck performance ([#2925](AztecProtocol/aztec-packages#2925)) ([5fbfe6e](AztecProtocol/aztec-packages@5fbfe6e)) ### Miscellaneous * Remove unused nix files ([#2933](AztecProtocol/aztec-packages#2933)) ([3174f84](AztecProtocol/aztec-packages@3174f84)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

zac-williamson and others added 30 commits August 15, 2023 18:21

added cycle_group class

2acca41

fixed bugs in cycle_group add/sub/dbl

4f885f6

added variable-base method that isn't quite passing tests. tests will fail ~33% of the time!

variable-base scalar multiplication passes tests

fbd9b40

added an elliptic curve point doubling gate to the UltraPlonk arithme…

e339ba0

…tisation+circuit builder

hash to curve

ce45f32

wip

f5a9a5f

fixed linting errors in proof_system/plookup_tables

604ad3f

cycle_group::fixed_base_scalar_mul now working with plookup tables

added refactored pedersen hash methods + stdlib::pedersen_hash (needs…

c09483e

… tests)

fixed, tidy up, comments

36624f3

wip

0608a65

Merge remote-tracking branch 'origin/master' into zw/stdlib-cycle-gro…

dcec4f0

…up-msm

completed merge of master

afad6f0

Merge branch 'master' into zw/stdlib-cycle-group-msm

4283697

revert formatting

43086fa

removed extra ecc gate methods from standard/turbo circuit builder

918410c

wip

0db1732

PR changes

98e5261

Added TODOs for code that needs to be culled post-refactor removed custom MSM algorithms from schnorr.tcc + reduced constraint cost by ~50%

fixed ecc_dbl gates incorrectly fusing into ecc_add gates

af2a6ac

wip

4426370

compiler fixes

fd0de30

compiler fox

a29ebb3

compiler fox

79b17e1

compiler fix

a0c9f92

compiler fix

5e7a4d5

reverted schnorr

8b58e39

updated schnorr to use new cycle_group methods

1bbe81d

compiler fix

87ff132

revert pedersen c_bind

9236506

revert crypto/schnorr

c899d90

kevaundray reviewed Oct 23, 2023

View reviewed changes

Comment thread yarn-project/aztec-nr/aztec/src/hash.nr

kevaundray reviewed Oct 23, 2023

View reviewed changes

codygunton self-requested a review October 23, 2023 15:04

kevaundray added the crypto cryptography label Oct 23, 2023

codygunton approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'master' into zw/use-new-pedersen

5b5d1c5

kevaundray enabled auto-merge (squash) October 24, 2023 00:10

chore: update artifacts after master merge

ed62d95

sirasistant disabled auto-merge October 24, 2023 08:22

Merge branch 'master' into zw/use-new-pedersen

65cc870

sirasistant merged commit 3085676 into master Oct 24, 2023

sirasistant deleted the zw/use-new-pedersen branch October 24, 2023 09:13

AztecBot mentioned this pull request Oct 24, 2023

chore(master): Release 0.10.0 #2893

Merged

codygunton assigned zac-williamson Oct 24, 2023

maramihali mentioned this pull request Apr 18, 2024

plookup_read tests with Pedersen not running AztecProtocol/barretenberg#953

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor pedersen hash standard#2592

feat: refactor pedersen hash standard#2592
sirasistant merged 160 commits into
masterfrom
zw/use-new-pedersen

zac-williamson commented Sep 29, 2023 •

edited by kevaundray

Loading

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

kevaundray Oct 23, 2023

Uh oh!

zac-williamson Oct 23, 2023

Uh oh!

codygunton left a comment

Uh oh!

sirasistant commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

zac-williamson commented Sep 29, 2023 • edited by kevaundray Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codygunton left a comment

Choose a reason for hiding this comment

Uh oh!

sirasistant commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zac-williamson commented Sep 29, 2023 •

edited by kevaundray

Loading