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

Feedback on nargo, its usage and possible simplification #3772

Closed
kevaundray opened this issue Dec 11, 2023 · 5 comments
Closed

Feedback on nargo, its usage and possible simplification #3772

kevaundray opened this issue Dec 11, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@kevaundray
Copy link
Collaborator

Problem

Was speaking with Charlie and he noted a few points that I'd like to note down here in making the code simpler:

  • Remove the concept of partial implementations, so a backend needs to implement all opcodes.

The advantage being that backends no longer need to send us a config file and we do not need to implement fallbacks in acvm

  • Add a program_width flag or a key in Nargo.toml

If we assume that all backends support all opcodes, then the difference between backends would be the program_width parameter. This could be a flag, ie nargo compile --program_width 3 or we could put it in the Nargo.toml file.

Rationale: The main motivation for the above suggestions is to make nargo no longer be responsible for proving and for backends.

Happy Case

If the above is implemented, then we have a workflow that looks approximately like the following:

nargo compile --program_width 3
nargo execute witness
bb prove acir.gz witness.gz // This will return an error if the program width supported by bb is not 3.

Another idea that was brought up from Istanbul is that instead of having bb prove, we can have a proxy prover instead. The proxy prover can set the backend etc and is more similar to what we have with the backend interface.

Note: The multiplexer makes a quite large difference since without a multiplexer, how the user gets the backends is then the responsibility of the backend

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@kevaundray kevaundray added enhancement New feature or request P-MEDIUM labels Dec 11, 2023
@kevaundray
Copy link
Collaborator Author

@guipublic also agrees with removing the partial implementation feature (spoke on Slack)

@TomAFrench
Copy link
Member

I'm mildly concerned about removing support for partial opcode support as this could remove a bunch of flexibility for alternative backends. e.g. do we want to be able to target more constrained but widely used proving systems such as groth16?

Placing program width in the Nargo.toml file requires the user to understand what their backend needs which I doubt 99.9% of users would understand (And if we then required backends to expose this information so it's easy for the user to get then there's the question of "why not read this automatically?"). This seems to be a bet on users just using barretenberg and not needing to move off of the default.

@guipublic
Copy link
Contributor

My point was that we are still developing Noir and modifying blackbox functions, adding and removing opcodes.
It's easier for now to assume the backend can process the opcodes rather than having to maintain the backup mechanism by adding/removing backup for the changed opcodes.

@kevaundray
Copy link
Collaborator Author

I'm mildly concerned about removing support for partial opcode support as this could remove a bunch of flexibility for alternative backends. e.g. do we want to be able to target more constrained but widely used proving systems such as groth16?

Placing program width in the Nargo.toml file requires the user to understand what their backend needs which I doubt 99.9% of users would understand (And if we then required backends to expose this information so it's easy for the user to get then there's the question of "why not read this automatically?"). This seems to be a bet on users just using barretenberg and not needing to move off of the default.

Ahh didn't see this before making my draft PR.

I think it just means that backends will be responsible for implementing fallbacks instead of us, right now our opcodes are always changing and I doubt we will prioritize implementing the rest of the fallbacks just due to the complexity of some of them and it not being a priority. We could say someone else can implement them, but then we would also need to audit it and give some sort of blessing post 1.0.

For Groth16 or R1CS in general, there should be enough R1CS libraries to support most of the primitives that we put into ACIR -- I do think that this is shifting the complexity more onto backend implementors but I think this is fine at least in the short term to reduce our code complexity.

This also does incentivize us to make our acir opcodes more primitive so that the jobs of backends is easier

github-merge-queue bot pushed a commit that referenced this issue Dec 15, 2023
# Description

Related to #3772 

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*



## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
@TomAFrench
Copy link
Member

Closing this as the remaining feedback is captured in #3854

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

3 participants