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.

Sign up for GitHub

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

Jump to bottom

chore: Extract bytecode from artifact files for backend integration test inputs #2356

Merged
merged 5 commits into from
Aug 17, 2023
Merged

chore: Extract bytecode from artifact files for backend integration test inputs #2356

merged 5 commits into from
Aug 17, 2023

Conversation

kevaundray
Copy link
Collaborator

Description

This PR allows backends to not needed to parse the PreprocessedProgram. Instead they can parse the bytecode directly, this is inline with the fact that backends do not know about Noir, only ACIR.

Problem*

Resolves

Summary*

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.

@kevaundray kevaundray changed the title chore: Add ability to only save the bytecode chore: Add ability to only save the bytecode when running nargo compile Aug 17, 2023
@TomAFrench
Copy link
Member

Is this just for the purposes of barretenberg testing? A better solution would be to extract the circuit from the json file inside rebuild.sh

@kevaundray
Copy link
Collaborator Author

Is this just for the purposes of barretenberg testing? A better solution would be to extract the circuit from the json file inside rebuild.sh

I think this is also a less leaky abstraction because when backends run end to end tests, they will not want to parse the .json file because thats not a part of the ACVM interface

@kevaundray
Copy link
Collaborator Author

Note, we could also just make a change in the bb script which runs the tests, so it downloads the main.json and then parses the bytecode and deletes it for a file with just the bytecode.

Though then all backends would need to do that too.

Orthogonally, we'd also want to change the interface of bb.js if its expecting more than just the bytecode.

What are your thoughts?

@TomAFrench
Copy link
Member

Note, we could also just make a change in the bb script which runs the tests, so it downloads the main.json and then parses the bytecode and deletes it for a file with just the bytecode.

Though then all backends would need to do that too.

My point is that rebuild.sh can do this step which removes the need for either Nargo or the backends' CI to do this.

@TomAFrench
Copy link
Member

Unless we're expecting users to use this option in normal usage (in which case we need to discuss why) then it doesn't make sense for CI logic to leak into the binary we distribute to users.

@kevaundray
Copy link
Collaborator Author

Unless we're expecting users to use this option in normal usage (in which case we need to discuss why) then it doesn't make sense for CI logic to leak into the binary we distribute to users.

It should be hidden to the user in this PR -- in either case, you could make an argument that the user may want to just compile the bytecode and witness, then run it with a different backend/binary, ie they only use Noir for compilation and then they call some other binary to consume it -- ie dynamic backends.

Unless we are also expecting backends dynamic backends to take in the prepropressedProgram as input?

@kevaundray
Copy link
Collaborator Author

Note, we could also just make a change in the bb script which runs the tests, so it downloads the main.json and then parses the bytecode and deletes it for a file with just the bytecode.
Though then all backends would need to do that too.

My point is that rebuild.sh can do this step which removes the need for either Nargo or the backends' CI to do this.

Yep I understand this argument -- lets fallback onto this based on the conclusions we make to my previous question

@kevaundray
Copy link
Collaborator Author

Spoke with Tom and he made the good point that users will likely want to use the abi so they know how to encode the inputs. I think this is a good enough reason to switch to modifying the rebuild script and we can revisit adding bytecode-only flag when/if it ever comes up again as a viable workflow

@kevaundray kevaundray changed the title chore: Add ability to only save the bytecode when running nargo compile chore: Extract bytecode from ABI files in rebuild script and remove the ABI file Aug 17, 2023
@kevaundray kevaundray marked this pull request as ready for review August 17, 2023 16:00
@TomAFrench TomAFrench changed the title chore: Extract bytecode from ABI files in rebuild script and remove the ABI file chore: Extract bytecode from artifact files for backend integration test inputs Aug 17, 2023
@kevaundray kevaundray added this pull request to the merge queue Aug 17, 2023
Merged via the queue into master with commit 2c5b35d Aug 17, 2023
18 checks passed
@kevaundray kevaundray deleted the kw/save-bytecode branch August 17, 2023 21:18
TomAFrench added a commit that referenced this pull request Aug 19, 2023
@TomAFrench
Merge branch 'master' into assert-eq-builtin
49f0d95 
* master:
  feat(ssa): Merge slices in if statements with witness conditions (#2347)
  chore: Separate frontend `Visibility` and `Distinctness` from the ABI (#2369)
  feat: add syntax for specifying function type environments (#2357)
  chore: Remove unused `Directive::Log` (#2366)
  chore: clippy fixes (#2365)
  chore: Extract bytecode from artifact files for backend integration test inputs (#2356)
  feat: Add trait definition representation in DefCollector and HIR (#2338)
  chore: Remove unused `Intrinsic::Println` (#2358)
  fix: Remove duplicte `T` in `expected T, found T` error on tuple assignment (#2360)
  chore(brillig): Fix brillig radix instruction return vector size (#2350)
  fix: Show types in error message in same order as in source code (#2353)
TomAFrench added a commit that referenced this pull request Aug 22, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
69b0058 
* master: (46 commits)
  chore: Remove `serde` from `noirc_frontend` (#2390)
  chore: allow parenthesizing in two type locations  (#2388)
  chore(ci): automatically delete cache entries associated with closed PRs (#2342)
  feat: Perform more checks for compile-time arithmetic (#2380)
  chore: Remove `noirc_abi::FunctionSignature` and define in terms of HIR (#2372)
  feat: Update to `acvm` 0.22.0 (#2363)
  chore: Update committed ACIR artifacts (#2376)
  feat(ssa): Merge slices in if statements with witness conditions (#2347)
  chore: Separate frontend `Visibility` and `Distinctness` from the ABI (#2369)
  feat: add syntax for specifying function type environments (#2357)
  chore: Remove unused `Directive::Log` (#2366)
  chore: clippy fixes (#2365)
  chore: Extract bytecode from artifact files for backend integration test inputs (#2356)
  feat: Add trait definition representation in DefCollector and HIR (#2338)
  chore: Remove unused `Intrinsic::Println` (#2358)
  fix: Remove duplicte `T` in `expected T, found T` error on tuple assignment (#2360)
  chore(brillig): Fix brillig radix instruction return vector size (#2350)
  fix: Show types in error message in same order as in source code (#2353)
  chore: update noir-source-resolver from `1.1.2` to `^1.1.3` (#2349)
  chore(ci): Avoid writing to cache in workflows triggered by the merge queue (#2341)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phated phated phated left review comments

@TomAFrench TomAFrench TomAFrench approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@kevaundray @TomAFrench @phated