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)!: define preprocessed artifacts for programs/contracts #1126

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Apr 10, 2023

Related issue(s)

Related to #1102

Description

Summary of changes

This PR defines generic preprocessed artifacts for Noir programs and contracts as defined in #1102 ("Backend-specific Artifact").

Rather than the current json/pk/vk/checksum files we now have a single json file which we can use. This means that we don't need to be concerned about the keys and the acir being in sync as they're stored together.

This does mean that we now need to load the proving key when verifying a proof however my preference for solving this is to derive specialized "verifier artifacts" which just contain the information required for verifying the circuit. Nargo would then accept the full or verifier artifact when verifying a circuit but that's OOS for this PR.

Note: PreprocessedContract does NOT match the desired spec for A3 contracts, these can be derived from this artifact however. I don't think that Nargo is the right place for performing this translation so the A3 team may need to maintain a small tool to translate between them. Thoughts @kevaundray?

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.

Documentation needs

  • This PR requires documentation updates when merged.

Nargo compilation outputs have been changed. This shouldn't affect any actions which the user has to take but discussion about files created needs to be update.

nargo preprocess command has been removed as it's not meaningful until we have backend-agnostic artifacts. The nargo preprocess command would then create the artifacts made in this PR.

Additional context

@TomAFrench TomAFrench changed the title feat(nargo): define generic preprocessed artifacts for programs/contracts feat(nargo)!: define generic preprocessed artifacts for programs/contracts Apr 10, 2023
@phated
Copy link
Contributor

phated commented Apr 10, 2023

This does mean that we now need to load the proving key when verifying a proof however my preference for solving this is to derive specialized "verifier artifacts" which just contain the information required for verifying the circuit. Nargo would then accept the full or verifier artifact when verifying a circuit but that's OOS for this PR.

Why would we have specialized artifacts instead of just switching to a serialization protocol that can read individual pieces of data out of the artifact?

@TomAFrench
Copy link
Member Author

TomAFrench commented Apr 10, 2023

Why would we have specialized artifacts instead of just switching to a serialization protocol that can read individual pieces of data out of the artifact?

Both are options. Choosing a serialisation format which allows us to read subsets of the artifact is obviously better when you're storing the full artifact anyway but only need a portion of it for the action you're performing. However, if we know that we're never going to prove against the circuit (e.g. we want to verify proofs on a mobile device but it doesn't have the resources to create proofs) then we can save bandwidth/disk space using specialised artifacts.

@TomAFrench TomAFrench changed the title feat(nargo)!: define generic preprocessed artifacts for programs/contracts feat(nargo)!: define preprocessed artifacts for programs/contracts Apr 11, 2023
@TomAFrench
Copy link
Member Author

Removing generic from the title as this causes some confusion in relation to #1102. For clarity these artifacts aren't generic wrt to backends but are "generic" in the sense that they're not tailored to any particular usecase.

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions I had

* master:
  feat: import core logic in cli from `nargo` crate (#1142)
  chore: enforce `clippy::semicolon_if_nothing_returned` linting rule (#1139)
  chore: borrow instead of cloning witness vectors in IR gen (#1127)
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- one more comment on creating an issue for the hardcoded backend name

@TomAFrench TomAFrench added this pull request to the merge queue Apr 13, 2023
Merged via the queue into master with commit 7528f59 Apr 13, 2023
@TomAFrench TomAFrench deleted the program-artifact branch April 13, 2023 10:19
TomAFrench added a commit that referenced this pull request Apr 13, 2023
* master:
  feat(nargo)!: define preprocessed artifacts for programs/contracts (#1126)
  feat: import core logic in cli from `nargo` crate (#1142)
  chore: enforce `clippy::semicolon_if_nothing_returned` linting rule (#1139)
  chore: borrow instead of cloning witness vectors in IR gen (#1127)
  fix: compiler identifying imported functions as being part of a contract (#1112)
  feat: Add new `Vec` type to frontend (#1103)
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