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

chore!: Replace computing hashes in circuits wasm, with computing them in ts via bb.js pedersen call. #3114

Merged
merged 37 commits into from
Nov 7, 2023

Conversation

charlielye
Copy link
Contributor

@charlielye charlielye commented Oct 28, 2023

  • This was originally going to use our TS pedersen lib to do all pedersen computations, but wasm is 10x faster, so we still want to use wasm.
  • Introduce elliptic version of pedersen as it can be much faster than @noble/curves (WARNING: Jest totally messes with benchmarks and can't be trusted for timings with this stuff). Leaving noble version in for now in case want to revisit, but not exported via index. The elliptic version is also just there as reference (maybe useful in future).
  • The pedersen api required being passed a wasm, and that wasm was obtained via async call. This creates a tonne of async function overhead and is generally a bit of a faff.
  • This adds foundation/crypto/pedersen/pedersen.wasm.ts (and defaults to using it via index.ts) that mimics our pure TS pedersen api, but uses the bb.js wasm.
  • Adds a hand rolled synchronous pedersen bind caller to bb.js (I should probably bring back making bindgen produce the sync api, but also figured might adopt Adams msgpack approach so not worth effort).
  • foundation/crypto/pedersen leverages top-level await to load the wasm as a global, so we can remove a tonne of async await calls.
  • We cleanup some of the c_binds in barretenberg.
  • We still use circuits.wasm for a couple of more complex calls ive not tackled yet, e.g. hashVK.
  • We now have dependency on bb.js, pulled in via portal yarn thingy. What does this mean for releases? (edit: This would probably have broken canary. This PR disabled canary. chore: Disable canary. #3244)
  • Pulls the bbmalloc WASM_EXPORTS into own header so it doesn't mess with the bindgen util.
  • Broke some circular import dependencies as I was hitting weird errors.
  • All pedersen calls now made on foundation/crypto/pedersen, got rid of circuits/barretenberg/crypto/pedersen wrapper.
  • Enable colorizing debug output in e2e tests via DEBUG_COLOR=1.
  • Change some test() to it() (was faffing with other test runner when profiling pedersen).
  • Timer uses performance.now rather than Date.getTime()
  • Introduces madge as a tool to detect circular deps.

TODO in subsequent PR's.

  • Move all the hashing functions in abis.ts to exist as hash functions on the various types.
  • Implement hash functions for e.g. vkHash and a couple others that were to complicated for doing right now.
  • Move the types out of circuits.js into types project.
  • Ultimately remove need for circuits.js / circuits.wasm?

@charlielye charlielye marked this pull request as draft October 29, 2023 15:18
Copy link

socket-security bot commented Nov 1, 2023

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@types/elliptic 6.4.16 None +1 25.5 kB types
@types/bn.js 5.1.3 None +0 13.8 kB types
madge 6.1.0 filesystem, environment +70 58.3 MB kamiazya
typescript 3.9.10 None +0 54.1 MB typescript-bot

🚮 Removed packages: [email protected]

@charlielye charlielye changed the title chore: Initial work at replacing wasm hashing with pure TS. chore!: Replace computing hashes in circuits wasm, with computing them in ts via bb.js pedersen call. Nov 2, 2023
@charlielye charlielye marked this pull request as ready for review November 2, 2023 17:19
@AztecBot
Copy link
Collaborator

AztecBot commented Nov 3, 2023

Benchmark results

No metrics with a significant change found.

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit e8945f80 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 128 txs
l1_rollup_calldata_size_in_bytes 45,444 179,588 716,132
l1_rollup_calldata_gas 222,972 868,196 3,449,432
l1_rollup_execution_gas 842,059 3,595,304 22,204,801
l2_block_processing_time_in_ms 2,020 7,599 (-1%) 30,047 (+1%)
note_successful_decrypting_time_in_ms 300 873 (-3%) 3,219 (-2%)
note_trial_decrypting_time_in_ms 3.30 (-17%) 81.8 (-22%) 134 (+1%)
l2_block_building_time_in_ms 13,178 52,234 (-2%) 209,447 (-1%)
l2_block_rollup_simulation_time_in_ms 11,832 46,908 (-2%) 188,206 (-1%)
l2_block_public_tx_process_time_in_ms 1,302 (-1%) 5,190 (-1%) 20,743 (-2%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 21,730 (+1%) 42,345 (-1%)
note_history_successful_decrypting_time_in_ms 2,030 (-1%) 3,931 (-5%)
note_history_trial_decrypting_time_in_ms 122 (+2%) 177 (+24%)
node_database_size_in_bytes 1,628,864 1,100,279
pxe_database_size_in_bytes 29,748 59,307

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 770 61,697 18,905
private-kernel-ordering 123 (-1%) 24,297 8,153
base-rollup 1,766 656,311 814
root-rollup 77.4 (-2%) 4,072 1,097
private-kernel-inner 786 81,568 18,905
public-kernel-private-input 41.1 (-2%) 41,519 18,841
public-kernel-non-first-iteration 25.9 (-3%) 41,497 18,841
merge-rollup 0.940 (-3%) 2,592 873

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts 1 deployed contracts
tx_size_in_bytes 8,787 27,547

PhilWindle pushed a commit that referenced this pull request Nov 6, 2023
In prep to merge
#3114
The above would break the canary as bb.js is now required to deployed as
it's new used by yarn-project.
This is inevitable as we start proving anyway.
The issue is bb.js is currently deployed independently in GA workflow.
This will need to be changed to deploy as part of CCI workflow.
There are other changes that should be made to "canary" anyway to
simplify our build/deploy, so disabling now seem ok.
@ludamad ludamad self-assigned this Nov 6, 2023
@@ -441,6 +510,16 @@ export function computeCallStackItemHash(
export function computePrivateCallStackItemHash(wasm: IWasmModule, callStackItem: PrivateCallStackItem): Fr {
const value = wasmSyncCall(wasm, 'abis__compute_private_call_stack_item_hash', callStackItem, 32);
return Fr.fromBuffer(value);
// return Fr.fromBuffer(
// pedersenHashWithHashIndex(
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left it in as my next PR will actually make that commented code work. Just commented because discovered it's too much work for this PR.

// pedersenHashWithHashIndex(
// [
// callStackItem.contractAddress.toBuffer(),
// callStackItem.functionData.toBuffer(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

@@ -29,11 +29,11 @@ WASM_EXPORT void schnorr_multisig_construct_signature_round_1(

WASM_EXPORT void schnorr_multisig_construct_signature_round_2(
uint8_t const* message,
fq::in_buf private_key,
Copy link
Collaborator

Choose a reason for hiding this comment

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

mind tl;dr'ing why we're switching field types?

Copy link
Contributor Author

@charlielye charlielye Nov 7, 2023

Choose a reason for hiding this comment

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

This was an oopsie. Fixed. It was actually another api call that was meant to become Fq.
Typescript didnt raise error due to Fq and Fr being structurally the same, although this PR changes that as I optimised Fr slightly (made it Uint8Array under the hood rather than bigint as the conversion seriously impacts performance when doing lots of hashing).

@ludamad
Copy link
Collaborator

ludamad commented Nov 6, 2023

I'll keep this review light on the principle of engineers reviewing their own work first and foremost, so kept the review to 'am I comfortable with what I read here'. I've made minor comments, otherwise I saw a lot of propagation of async/await removal and read the comments above, LGTM.

One thing missing: The motivation, just for posterity, is to move away off of circuits dependency gradually, right?

@charlielye
Copy link
Contributor Author

charlielye commented Nov 7, 2023

One thing missing: The motivation, just for posterity, is to move away off of circuits dependency gradually, right?

Yeah. The ultimate reason is to have much more of a pure TypeScript reference client rather than this extremely strong coupling with the C++ code/wasm. So bb.js can hopefully act as a lib for building proofs with a simple api, noir has their TS packages we can use, and then hopefully all of our yarn-project can just be pure TS.

(edit: I see the argument that circuits.js was a circuits encapsulating library, but with circuits now being written in Noir, it doesn't make sense to have a wasm for defining how we hash all these structs)

A side reason for this is just having the wasm plumbed into a hash function we use all over the place seems a bit off. It's nicer when the api is how it is in this PR, as we can easily switch how the hash is computed. Wether its a wasm, TS, or something else, is now encapsulated within the foundation/crypto/pedersen module.

@charlielye charlielye removed the request for review from spalladino November 7, 2023 16:28
@charlielye charlielye enabled auto-merge (squash) November 7, 2023 16:35
@charlielye charlielye merged commit 87eeb71 into master Nov 7, 2023
78 checks passed
@charlielye charlielye deleted the cl/ts_use_new_pedersen branch November 7, 2023 20:33
ludamad pushed a commit that referenced this pull request Nov 17, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.15.0</summary>

##
[0.15.0](aztec-packages-v0.14.2...aztec-packages-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Features

* **bb:** Add msan preset
([#3284](#3284))
([bcf025c](bcf025c))
* Enable merge and root rollup circuits in noir
([#3248](#3248))
([68555fc](68555fc))
* Protogalaxy combiner quotient
([#3245](#3245))
([db0f3ab](db0f3ab))
* Public kernel in noir
([#3186](#3186))
([15a522b](15a522b))
* Ultra honk arith from ultra
([#3274](#3274))
([ec2b805](ec2b805))


### Bug Fixes

* Debug build
([#3283](#3283))
([aca2624](aca2624))
* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))
* Main.md typo
([#3278](#3278))
([cb87c4d](cb87c4d))
* Typo fix roundup
([#3302](#3302))
([9dd778d](9dd778d))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](#3318))
([4229173](4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](#3303))
([13f1a1d](13f1a1d))
* Clean up Plonk widgets
([#3305](#3305))
([4623d91](4623d91))
* **docs:** Aztec.nr logging page
([#3281](#3281))
([11e6ca7](11e6ca7))
* **docs:** Update netlify.toml and fix build
([#3304](#3304))
([df76636](df76636))
* Explicitly instantiate Goblin translator relations
([#3239](#3239))
([e3b5fb0](e3b5fb0))
* Plain struct flavor entities
([#3277](#3277))
([f109512](f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](#3330))
([23d1e2d](23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Revert build-debug folder for debug preset
([#3324](#3324))
([43a2e6b](43a2e6b))
* Towards plain struct flavor entities
([#3216](#3216))
([3ba89cf](3ba89cf))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

<details><summary>barretenberg.js: 0.15.0</summary>

##
[0.15.0](barretenberg.js-v0.14.2...barretenberg.js-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Bug Fixes

* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))


### Miscellaneous

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

<details><summary>barretenberg: 0.15.0</summary>

##
[0.15.0](barretenberg-v0.14.2...barretenberg-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Features

* **bb:** Add msan preset
([#3284](#3284))
([bcf025c](bcf025c))
* Protogalaxy combiner quotient
([#3245](#3245))
([db0f3ab](db0f3ab))
* Ultra honk arith from ultra
([#3274](#3274))
([ec2b805](ec2b805))


### Bug Fixes

* Debug build
([#3283](#3283))
([aca2624](aca2624))
* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](#3318))
([4229173](4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](#3303))
([13f1a1d](13f1a1d))
* Clean up Plonk widgets
([#3305](#3305))
([4623d91](4623d91))
* Explicitly instantiate Goblin translator relations
([#3239](#3239))
([e3b5fb0](e3b5fb0))
* Plain struct flavor entities
([#3277](#3277))
([f109512](f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](#3330))
([23d1e2d](23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Revert build-debug folder for debug preset
([#3324](#3324))
([43a2e6b](43a2e6b))
* Towards plain struct flavor entities
([#3216](#3216))
([3ba89cf](3ba89cf))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Nov 19, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@aztec-packages-v0.14.2...aztec-packages-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Features

* **bb:** Add msan preset
([#3284](AztecProtocol/aztec-packages#3284))
([bcf025c](AztecProtocol/aztec-packages@bcf025c))
* Enable merge and root rollup circuits in noir
([#3248](AztecProtocol/aztec-packages#3248))
([68555fc](AztecProtocol/aztec-packages@68555fc))
* Protogalaxy combiner quotient
([#3245](AztecProtocol/aztec-packages#3245))
([db0f3ab](AztecProtocol/aztec-packages@db0f3ab))
* Public kernel in noir
([#3186](AztecProtocol/aztec-packages#3186))
([15a522b](AztecProtocol/aztec-packages@15a522b))
* Ultra honk arith from ultra
([#3274](AztecProtocol/aztec-packages#3274))
([ec2b805](AztecProtocol/aztec-packages@ec2b805))


### Bug Fixes

* Debug build
([#3283](AztecProtocol/aztec-packages#3283))
([aca2624](AztecProtocol/aztec-packages@aca2624))
* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))
* Main.md typo
([#3278](AztecProtocol/aztec-packages#3278))
([cb87c4d](AztecProtocol/aztec-packages@cb87c4d))
* Typo fix roundup
([#3302](AztecProtocol/aztec-packages#3302))
([9dd778d](AztecProtocol/aztec-packages@9dd778d))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](AztecProtocol/aztec-packages#3318))
([4229173](AztecProtocol/aztec-packages@4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](AztecProtocol/aztec-packages#3303))
([13f1a1d](AztecProtocol/aztec-packages@13f1a1d))
* Clean up Plonk widgets
([#3305](AztecProtocol/aztec-packages#3305))
([4623d91](AztecProtocol/aztec-packages@4623d91))
* **docs:** Aztec.nr logging page
([#3281](AztecProtocol/aztec-packages#3281))
([11e6ca7](AztecProtocol/aztec-packages@11e6ca7))
* **docs:** Update netlify.toml and fix build
([#3304](AztecProtocol/aztec-packages#3304))
([df76636](AztecProtocol/aztec-packages@df76636))
* Explicitly instantiate Goblin translator relations
([#3239](AztecProtocol/aztec-packages#3239))
([e3b5fb0](AztecProtocol/aztec-packages@e3b5fb0))
* Plain struct flavor entities
([#3277](AztecProtocol/aztec-packages#3277))
([f109512](AztecProtocol/aztec-packages@f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](AztecProtocol/aztec-packages#3330))
([23d1e2d](AztecProtocol/aztec-packages@23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Revert build-debug folder for debug preset
([#3324](AztecProtocol/aztec-packages#3324))
([43a2e6b](AztecProtocol/aztec-packages@43a2e6b))
* Towards plain struct flavor entities
([#3216](AztecProtocol/aztec-packages#3216))
([3ba89cf](AztecProtocol/aztec-packages@3ba89cf))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

<details><summary>barretenberg.js: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@barretenberg.js-v0.14.2...barretenberg.js-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Bug Fixes

* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))


### Miscellaneous

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

<details><summary>barretenberg: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@barretenberg-v0.14.2...barretenberg-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Features

* **bb:** Add msan preset
([#3284](AztecProtocol/aztec-packages#3284))
([bcf025c](AztecProtocol/aztec-packages@bcf025c))
* Protogalaxy combiner quotient
([#3245](AztecProtocol/aztec-packages#3245))
([db0f3ab](AztecProtocol/aztec-packages@db0f3ab))
* Ultra honk arith from ultra
([#3274](AztecProtocol/aztec-packages#3274))
([ec2b805](AztecProtocol/aztec-packages@ec2b805))


### Bug Fixes

* Debug build
([#3283](AztecProtocol/aztec-packages#3283))
([aca2624](AztecProtocol/aztec-packages@aca2624))
* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](AztecProtocol/aztec-packages#3318))
([4229173](AztecProtocol/aztec-packages@4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](AztecProtocol/aztec-packages#3303))
([13f1a1d](AztecProtocol/aztec-packages@13f1a1d))
* Clean up Plonk widgets
([#3305](AztecProtocol/aztec-packages#3305))
([4623d91](AztecProtocol/aztec-packages@4623d91))
* Explicitly instantiate Goblin translator relations
([#3239](AztecProtocol/aztec-packages#3239))
([e3b5fb0](AztecProtocol/aztec-packages@e3b5fb0))
* Plain struct flavor entities
([#3277](AztecProtocol/aztec-packages#3277))
([f109512](AztecProtocol/aztec-packages@f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](AztecProtocol/aztec-packages#3330))
([23d1e2d](AztecProtocol/aztec-packages@23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Revert build-debug folder for debug preset
([#3324](AztecProtocol/aztec-packages#3324))
([43a2e6b](AztecProtocol/aztec-packages@43a2e6b))
* Towards plain struct flavor entities
([#3216](AztecProtocol/aztec-packages#3216))
([3ba89cf](AztecProtocol/aztec-packages@3ba89cf))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants