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

feat!: use struct variants for blackbox function calls#269

Merged
sirasistant merged 18 commits intomasterfrom
arv/bb_individual_structs
May 11, 2023
Merged

feat!: use struct variants for blackbox function calls#269
sirasistant merged 18 commits intomasterfrom
arv/bb_individual_structs

Conversation

@sirasistant
Copy link
Copy Markdown
Contributor

@sirasistant sirasistant commented May 8, 2023 •
edited
Loading

Related issue(s)

Description

Summary of changes

Define different types for the variants of BB function call

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@sirasistant sirasistant requested a review from kevaundray May 8, 2023 13:34
@kevaundray kevaundray changed the title (WIP) DO NOT MERGE: use struct variants for bb func calls chore:(WIP) DO NOT MERGE: use struct variants for bb func calls May 8, 2023
@sirasistant sirasistant changed the title chore:(WIP) DO NOT MERGE: use struct variants for bb func calls chore: use struct variants for bb func calls (WIP) DO NOT MERGE May 8, 2023
@TomAFrench TomAFrench changed the title chore: use struct variants for bb func calls (WIP) DO NOT MERGE chore!: use struct variants for bb func calls (WIP) DO NOT MERGE May 8, 2023
kevaundray
kevaundray reviewed May 8, 2023
View reviewed changes
Comment thread acvm/src/lib.rs Outdated
@sirasistant sirasistant changed the title chore!: use struct variants for bb func calls (WIP) DO NOT MERGE chore!: use struct variants for bb func calls DO NOT MERGE May 9, 2023
@sirasistant sirasistant mentioned this pull request May 9, 2023
Merged
5 tasks
@TomAFrench
Copy link
Copy Markdown
Member

TomAFrench commented May 9, 2023

Relevant issue: #29 (cc @guipublic as you're in favour of keeping these homogeneous)

I can see a couple of places up in noir-lang/noir which rely on the fact that BlackBoxFuncCall is homogenous. If we're going to move forward with this I'd like to see a draft PR in noir-lang/noir which patches the ACVM dependency to use this PR.

To do this we'll need to make a draft PR in aztec_backend to update it to use this version of ACVM as well. Branching off from noir-lang/acvm-backend-barretenberg#165 will address most of the existing breaking changes. I can help with this if needed.

Still looking through the PR.

@TomAFrench TomAFrench mentioned this pull request May 9, 2023
Closed
1 task
TomAFrench
TomAFrench reviewed May 9, 2023
View reviewed changes
Comment thread acvm/src/pwg/signature/ecdsa.rs
Comment thread acvm/src/pwg/signature/ecdsa.rs Outdated
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
Comment thread acvm/src/pwg/signature/ecdsa.rs Outdated
Comment thread acvm/src/pwg/signature/ecdsa.rs Outdated
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
@sirasistant sirasistant mentioned this pull request May 9, 2023
Merged
phated
phated reviewed May 9, 2023
View reviewed changes
Comment thread acvm/src/pwg/signature/ecdsa.rs Outdated
@sirasistant
Copy link
Copy Markdown
Contributor Author

sirasistant commented May 9, 2023

@TomAFrench I pushed a draft PR to the aztec_backend, will do the same for noir next!

TomAFrench
TomAFrench reviewed May 9, 2023
View reviewed changes
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
@sirasistant sirasistant mentioned this pull request May 10, 2023
Closed
6 tasks
@sirasistant sirasistant marked this pull request as ready for review May 10, 2023 14:15
@sirasistant sirasistant changed the title chore!: use struct variants for bb func calls DO NOT MERGE feat!: use struct variants for blackbox function calls May 10, 2023
@sirasistant
Copy link
Copy Markdown
Contributor Author

sirasistant commented May 10, 2023

@TomAFrench Pushed a draft PR in the noir repo to use this ACVM version

phated
phated reviewed May 10, 2023
View reviewed changes
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
@TomAFrench TomAFrench mentioned this pull request May 11, 2023
Merged
5 tasks
TomAFrench
TomAFrench reviewed May 11, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for opening up the draft PRs! This is looking good to me. Just a couple of notes here and there and then we can start merging this in.

Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
Comment thread acir/src/circuit/opcodes/black_box_function_call.rs Outdated
Comment thread acvm/src/lib.rs Outdated
TomAFrench
TomAFrench approved these changes May 11, 2023
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

@sirasistant sirasistant added this pull request to the merge queue May 11, 2023
Merged via the queue into master with commit a83333b May 11, 2023
@github-actions github-actions Bot mentioned this pull request May 11, 2023
Merged
@TomAFrench TomAFrench deleted the arv/bb_individual_structs branch May 11, 2023 09:30
TomAFrench added a commit that referenced this pull request May 11, 2023
@TomAFrench
Merge branch 'master' into phated/witness-map
b90f400 
* master:
  feat!: use struct variants for blackbox function calls (#269)
@TomAFrench TomAFrench mentioned this pull request May 14, 2023
Closed
TomAFrench added a commit that referenced this pull request May 16, 2023
@TomAFrench
Merge branch 'master' into joss/oracle-predicate
e8cfc5b 
* 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)
  ...
@vezenovm vezenovm mentioned this pull request May 16, 2023
Closed
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@phated phated Awaiting requested review from phated

@kevaundray kevaundray Awaiting requested review from kevaundray

1 more reviewer

@TomAFrench TomAFrench TomAFrench approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sirasistant @TomAFrench @phated @kevaundray