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: Save witness in correct format that bb.js expects #1876

Merged
merged 6 commits into from
Jul 7, 2023

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jul 6, 2023

Description

We are moving to have our ACIR tests run in bb.js for proving and verification while nargo will just handle circuit execution.

Problem*

Resolves

Summary*

This PR changes the format in which witnesses are saved to file. bberg is currently the only consumer of our witness output however this may be breaking for any old programs that still save the witness to file and use it elsewhere (however I see this as unlikely due to the large divergence in the TS bberg packages and Noir master).

In order to be in line with bb.js we also need to enable the RecursiveAggregation opcode in acvm-backend-barretenberg as per this PR: noir-lang/acvm-backend-barretenberg#225

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

…ckend-barretenberg dep to show all old CI passing
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.

This breaks any interop with the ACVM simulator. bb.js should update to accept the witness map format defined by ACIR.

@vezenovm
Copy link
Contributor Author

vezenovm commented Jul 6, 2023

This breaks any interop with the ACVM simulator. bb.js should update to accept the witness map format defined by ACIR.

Ah didn't realize this would break ACVM simulator. I switched this to a feature flag

@Savio-Sou
Copy link
Collaborator

bb.js should update to accept the witness map format defined by ACIR.

@TomAFrench could you open an issue for it on https://github.com/AztecProtocol/barretenberg/?

@kevaundray kevaundray enabled auto-merge July 7, 2023 10:23
@kevaundray kevaundray dismissed TomAFrench’s stale review July 7, 2023 10:24

This has now been put behind a feature flag

@kevaundray kevaundray added this pull request to the merge queue Jul 7, 2023
@kevaundray kevaundray changed the title chore!: Save witness in correct format that bb.js expects chore: Save witness in correct format that bb.js expects Jul 7, 2023
Merged via the queue into master with commit 28d43dc Jul 7, 2023
@kevaundray kevaundray deleted the mv/witness-for-bbjs branch July 7, 2023 10:38
@TomAFrench
Copy link
Member

Reminder that we should not merge PRs which rely on patched versions of our own packages. This PR should have used the 0.6.1 release of acvm-backend-barretenberg.

Using a commit which wasn't in master for acvm-backend-barretenberg resulted in nightly releases failing and 0.8.0 failing on some targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants