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

feat(nargo): Update nargo to use preprocessing interface #765

Merged
merged 27 commits into from
Feb 16, 2023

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 7, 2023

Related issue(s)

Resolves issue #820

Resolves issue noir-lang/acvm#65 and noir-lang/acvm#46 in ACVM

Description

This PR adds the ability to generate proving and verification keys using nargo.

The preprocess interface changes can be found in the ACVM: noir-lang/acvm#71 (merged)
The barretenberg methods to enable implementing this can be found here: noir-lang/aztec-connect#21 (merged)
The aztec_backend PR integrating the barretenberg changes above are in this PR: noir-lang/acvm-backend-barretenberg#50 (yet to be merged)

The aztec_backend dependency in Cargo.toml references the branch in the aztec_backend PR listed above.

Summary of changes

There is a new preprocess_with_path method under the nargo compile that allows a developer to generate the proving and verification keys necessary for the new ACVM interface methods. The proving and verification keys are written to the target directory with the file extensions .pk and .vk respectively.

We also now write a sha256 checksum of the ACIR to the target directory during compilation. If someone has provided a name for their circuit build artifacts we later use this checksum during nargo prove and nargo verify to determine whether the circuit someone is trying to prove or verify a has changed (otherwise the proving and verification keys will be incorrect). If the circuit has changed we issue an error telling the user that they should run nargo compile again to regenerate the proving and verification keys. If a circuit name has not been provided for nargo prove we fallback to regenerating the proving and verification keys keys.

Regenerating the keys on a nargo prove or nargo verify is technically what was happening before so this isn't going backwards. This is also mainly useful for a dev quickly iterating on circuits who would like to just run nargo prove once. It will just be that if someone wants to get the performance benefits from not re-generating proving and verification keys, they will have to run nargo compile and pass a circuit_name to nargo prove and nargo verify. And then if someone is iterating on circuits and don't want to be re-running nargo compile, nargo prove --verify is their best choice.

Dependency additions / changes

Currently referencing this PR in aztec_backend: noir-lang/acvm-backend-barretenberg#50. More details can be found in the Description section above.

Test additions / changes

I altered the prove_and_verify helper method for our noir integration tests to generate proving and verification keys before calling our nargo commands for proving and verifying.

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.
  • This PR requires documentation updates when merged.

Additional context

To achieve the fastest proving and verification, a user should construct their build artifacts once with nargo compile.

I used time on my local intel Mac to get a sense of how the nargo commands have changed. These are testing the times for circuits basing their proving and verification off of the build artifacts.

1_mul - CURRENT MASTER
nargo prove p - 1.82s
nargo verify p - 1.90s

1_mul - NEW
nargo compile c - 1.04s
nargo prove p c - 0.90s (2x speedup)
nargo verify p c - 0.73s (2.6x speedup)

8_integration - CURRENT MASTER
nargo prove p - 7.76s
nargo verify p - 7.75s

8_integration - NEW
nargo compile c - 7.60s
nargo prove p c - 5.38s (1.44x speedup)
nargo verify p c - 0.95s (8.15x speedup)

9_conditional - CURRENT MASTER
nargo prove p - 14.61s
nargo verify p - 12.91s

9_conditional - NEW
nargo compile c - 23.58s
nargo prove p c - 20.63s (slow down :/ looking at how we can reduce this, most likely due to reading in a large proving key + circuit)
nargo verify p c - 1.22s (10.5x speedup)

@vezenovm
Copy link
Contributor Author

This PR should not be merged until noir-lang/acvm-backend-barretenberg#50 is in master. But it is ready for any feedback as all tests are passing using the new preprocess methods for proving and verification.

@vezenovm vezenovm marked this pull request as ready for review February 15, 2023 00:36
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.

I'm not a huge fan of the requirement to nargo compile before nargo prove. Ideally using preprocessed keys would be optional with the fallback being compiling the local program and using that.

I think it would also be nice to encapsulate the loading of keys/ACIR to remove the mutable path manipulations from business logic (Something similar to how read_inputs_from_file does it)

crates/nargo/src/cli/mod.rs Outdated Show resolved Hide resolved
crates/nargo/src/cli/compile_cmd.rs Outdated Show resolved Hide resolved
crates/nargo/src/cli/compile_cmd.rs Outdated Show resolved Hide resolved
crates/nargo/src/errors.rs Outdated Show resolved Hide resolved
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.

Looks good, can we add "preprocess" to CSpell as we're getting a lot of warnings coming up?

crates/nargo/Cargo.toml Outdated Show resolved Hide resolved
crates/nargo/src/cli/mod.rs Show resolved Hide resolved
crates/nargo/src/errors.rs Outdated Show resolved Hide resolved
crates/nargo/src/errors.rs Outdated Show resolved Hide resolved
@TomAFrench
Copy link
Member

This LGTM so happy to merge once the backend dep is updated.

TomAFrench
TomAFrench previously approved these changes Feb 16, 2023
@vezenovm
Copy link
Contributor Author

@TomAFrench Had to dismiss due to merge conflicts if you could approve once more :)

@kevaundray
Copy link
Contributor

I'll approve in Tom's stead

@kevaundray kevaundray added this pull request to the merge queue Feb 16, 2023
Merged via the queue into master with commit b3f1556 Feb 16, 2023
@kevaundray kevaundray deleted the mv/preprocess branch February 16, 2023 20:47
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.

3 participants