This repository has been archived by the owner on Apr 9, 2024. It is now read-only.
feat!: Remove solve from PWG trait & introduce separate solvers for each blackbox
#257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
solve from PWG trait & introduce separate solvers for each blackbox
ac64f0e to
dfe9293
Compare
|
@guipublic reviewed via DM while github was broken, I've pushed his suggested changes, so should be good for another review. |
5 tasks
guipublic
approved these changes
May 5, 2023
Merged
|
Much prefer this compared to original implementation as we can enforce removal of dead code in backends by removing bb functions from the trait. |
TomAFrench
added a commit
that referenced
this pull request
May 16, 2023
* master: (49 commits) feat(acvm)!: Add CommonReferenceString backend trait (#231) fix(acir): Hide variants of WitnessMapError and export it from package (#283) feat!: Introduce WitnessMap data structure to avoid leaking internal structure (#252) feat!: use struct variants for blackbox function calls (#269) chore(acvm)!: Backend trait must implement Debug (#275) chore!: remove `OpcodeResolutionError::UnexpectedOpcode` (#274) chore(acvm)!: rename `hash_to_field128_security` to `hash_to_field_128_security` (#271) feat(acvm)!: update black box solver interfaces to match `pwg:black_box::solve` (#268) chore(acir_field): remove unnecessary `to_vec()` (#267) chore(acvm)!: expose separate solvers for AND and XOR opcodes (#266) feat(acvm)!: Simplification pass for ACIR (#151) changes the name of blake to be blakes2s256 (#261) update hash functions (#260) feat!: Remove `solve` from PWG trait & introduce separate solvers for each blackbox (#257) chore: Release 0.11.0 (#250) feat(acvm): Add generic error for failing to solve an opcode (#251) fix(acir): Fix `Expression` multiplication to correctly handle degree 1 terms (#255) chore(acir): organise opcodes definitions (#254) chore: remove usage of `insert_witness` with `insert_value` (#253) feat: Add Keccak Hash function (#259) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue(s)
Works towards a resolution of the conversation at https://github.com/noir-lang/acvm/pull/247/files#r1180617953 and puts ACVM in a better position for self-implementing all of the opcodes (one by one).
Description
Summary of changes
This removes the
solvefunction on the PartialWitnessGenerator trait and moves it into thepwgmodule as a standalone function that takes a backend. It also replacessolve_black_box_function_callwith individual functions for each black box function being solved. This will allow us to systematically implement the blackbox function calls in ACVM and then remove them from the PWG interface for backends. Until (eventually) they are all gone and the backend doesn't need to implement anything for PWG; instead, just conforming to the ACVM implementation.I've split these out into separate functions because leaking the BlackBoxFunc enum to a backend is misleading after #247 lands and keccak is handled by ACVM. However, it seems like we'll need pedersen for quite some time (probably as the only one?).
Dependency additions / changes
(If applicable.)
Test additions / changes
(If applicable.)
Checklist
cargo fmtwith default settings.Additional context
(If applicable.)