Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acvm_js): Add execute_circuit_with_black_box_solver to prevent reinitialization of BlackBoxFunctionSolver #495

Merged
merged 8 commits into from
Aug 18, 2023
Merged

feat(acvm_js): Add execute_circuit_with_black_box_solver to prevent reinitialization of BlackBoxFunctionSolver #495

merged 8 commits into from
Aug 18, 2023

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Aug 16, 2023 •
edited by TomAFrench
Loading

Description

Problem*

It was determined that pedersen generator initialization was occurring for every call to acvm_js's execute_circuit (if the circuit includes a pedersen op).

Resolves AztecProtocol/aztec-packages#1553

Summary*

  1. Renames SimulatedBackend to WasmBlackBoxFunctionSolver
  2. Adds wasmbind to createBlackBoxSolver
  3. Adds wasmbind to executeCircuitWithBlackBoxSolver
    • (accepts object created in 2^)

(keeps the old executeCircuit wasmbind to avoid breaking change)

This massively improves runtime of Aztec's acir-simulator which makes heavy use of execute_circuit.

This PR also includes test node and browser refactors to reflect the API change.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

BEGIN_COMMIT_OVERRIDE
feat(acvm_js): Add execute_circuit_with_black_box_solver to prevent reinitialization of BlackBoxFunctionSolver
chore(acvm)!: Pass BlackBoxFunctionSolver to ACVM by reference
BEGIN_COMMIT_OVERRIDE

@dbanks12 dbanks12 changed the title refactor(acvm_js): expose separate wasmbind function to create SimulationBackend feat(acvm_js): expose separate wasmbind function to create SimulationBackend Aug 16, 2023
@kevaundray
Copy link
Contributor

I think this would be a breaking change since it modifies the execute_circuit

@sirasistant sirasistant changed the title feat(acvm_js): expose separate wasmbind function to create SimulationBackend feat(acvm_js)!: expose separate wasmbind function to create SimulationBackend Aug 17, 2023
TomAFrench
TomAFrench suggested changes Aug 17, 2023
View reviewed changes
acvm_js/src/execute.rs Outdated Show resolved Hide resolved
acvm_js/src/execute.rs Outdated Show resolved Hide resolved
@dbanks12 dbanks12 mentioned this pull request Aug 17, 2023
Merged
@dbanks12
feat(acvm_js): revert breaking change to execute_circuit and add a ne…
2bfac8b 
…w version instead that accepts a WasmBlackBoxFunctionSolver (renamed from SimulatedBackend)
@dbanks12 dbanks12 changed the title feat(acvm_js)!: expose separate wasmbind function to create SimulationBackend feat(acvm_js)!: separate wasmbinds to create 1: WasmBlackBoxFunctionSolver, 2: execute with solver Aug 17, 2023
@dbanks12 dbanks12 changed the title feat(acvm_js)!: separate wasmbinds to create 1: WasmBlackBoxFunctionSolver, 2: execute with solver feat(acvm_js): separate wasmbinds to create 1: WasmBlackBoxFunctionSolver, 2: execute with solver Aug 17, 2023
@dbanks12
Copy link
Collaborator Author

I think this would be a breaking change since it modifies the execute_circuit

Reverted that change as per @TomAFrench's recommendation and added an additional wasmbind function that accepts a solver.

@dbanks12 dbanks12 requested a review from TomAFrench August 17, 2023 15:34
TomAFrench
TomAFrench suggested changes Aug 17, 2023
View reviewed changes
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Blocking so I can do some final checks tomorrow morning.

@TomAFrench
Copy link
Member

Actually, this is breaking as we're modifying the main ACVM struct

@TomAFrench TomAFrench changed the title feat(acvm_js): separate wasmbinds to create 1: WasmBlackBoxFunctionSolver, 2: execute with solver feat(acvm_js): Add execute_circuit_with_black_box_solver to prevent reinitialization of BlackBoxFunctionSolver Aug 18, 2023
@TomAFrench TomAFrench enabled auto-merge August 18, 2023 12:09
@TomAFrench TomAFrench added this pull request to the merge queue Aug 18, 2023
Merged via the queue into master with commit 3877e0e Aug 18, 2023
@TomAFrench TomAFrench deleted the arv/experiment_bb_ref branch August 18, 2023 12:18
TomAFrench added a commit that referenced this pull request Aug 18, 2023
@TomAFrench
Merge branch 'master' into move-wasm-into-acvm
c39fe6d 
* master:
  feat(acvm_js): Add `execute_circuit_with_black_box_solver` to prevent reinitialization of `BlackBoxFunctionSolver` (#495)
  chore: Release 0.22.0 (#470)
  feat: print error location with fmt (#497)
  feat!: Switched from OpcodeLabel to OpcodeLocation and ErrorLocation (#493)
Sakapoi pushed a commit to Sakapoi/acvm_fork that referenced this pull request Aug 18, 2023
@dbanks12 @sirasistant
@TomAFrench @Sakapoi
feat(acvm_js): Add execute_circuit_with_black_box_solver to prevent…
ff3c628 
… reinitialization of `BlackBoxFunctionSolver` (noir-lang#495)

Co-authored-by: sirasistant <[email protected]>
Co-authored-by: Tom French <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomAFrench TomAFrench TomAFrench approved these changes

@sirasistant sirasistant Awaiting requested review from sirasistant

@kevaundray kevaundray Awaiting requested review from kevaundray

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Figure out why the Note Processor is so slow (multi-transfer example)
4 participants
@dbanks12 @kevaundray @TomAFrench @sirasistant