fix(nargo): correct inconsistent file extension for ACIR hashes

[ilitteri]

Related issue(s)

Resolves #993

Description

Summary of changes

• Made save_and_preprocess_program public for the crate.
• Renamed the extension for the ACIR hash that is being stored (json.sha256 -> json.checksum). For this point, I have a doubt about which you prefer and I think that it'd be nice to have a constant for this extension in order to avoid the same problem in the future.
• Add integration test that test this flow:
  • nargo compile <circuit_name>.
  • nargo prove <proof_name> <circuit_name>.
  • nargo verify <proof_name> <circuit_name>.
• Added a test helper function for the bullet above.

Dependency additions / changes

Test additions / changes

• Add integration test that test this flow:
  • nargo compile <circuit_name>.
  • nargo prove <proof_name> <circuit_name>.
  • nargo verify <proof_name> <circuit_name>.
• Added a test helper function for the bullet above.

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.

Additional context

[ilitteri]

I'm merging both integration tests into one (noir_integration) because there is a problem when running them in parallel.

[TomAFrench]

I think we can avoid doing that. Currently reviewing.

[ilitteri]

I think we can avoid doing that. Currently reviewing.

Thanks!

[TomAFrench]

Thanks for catching this @ilitteri, sorry this slipped through.

My main concern with the current PR is that we could get a minor combinatorial explosion of different paths through the compilation->proving->verification flow if we add more integration tests for cases like this, e.g. Proving and verifying can either automatically generate the ACIR from source or use a build artifact, verification may read a proof from a file or get passed it directly, etc.

If we run integration tests across all of test_data for each potential path we'd end up making CI unnecessarily slow. I think it would be best to have a more focused test just to check that fetch_pk_and_vk will always be able to recover the keys saved by save_and_preprocess_program if you pass in the same circuit.

[ilitteri]

Thanks for catching this @ilitteri, sorry this slipped through.

My main concern with the current PR is that we could get a minor combinatorial explosion of different paths through the compilation->proving->verification flow if we add more integration tests for cases like this, e.g. Proving and verifying can either automatically generate the ACIR from source or use a build artifact, verification may read a proof from a file or get passed it directly, etc.

If we run integration tests across all of test_data for each potential path we'd end up making CI unnecessarily slow. I think it would be best to have a more focused test just to check that fetch_pk_and_vk will always be able to recover the keys saved by save_and_preprocess_program if you pass in the same circuit.

I like the idea of just checking fetch_pk_and_vk. Another alternative (the one that I was trying right now) is using a Mutex. It actually works but it's being polished. If the time is not better, I will go for the fetch_pk_and_vk test.

EDIT: I got 1.5x doing this. I'll go for the unit test.

[ilitteri]

@TomAFrench ready! Do you want me to file an issue for unit testing the other fs submodule functions?

[TomAFrench]

Do you want me to file an issue for unit testing the other fs submodule functions?

Yeah we should roll these out to get more coverage on this submodule, I've added one here #995

Thanks again for this :)

[TomAFrench]

Sorry, that suggestion was semi-pseudocode. I'll fix up any issues and merge.

[ilitteri]

@TomAFrench ready! Do you want me to file an issue for unit testing the other fs submodule functions?

Sorry, that suggestion was semi-pseudocode. I'll fix up any issues and merge.

Oh sorry, my bad.

[ilitteri]

Sorry, that suggestion was semi-pseudocode. I'll fix up any issues and merge.

I have the fix ready to push, let me know if you're working on it too.

[TomAFrench]

I have the fix ready to push, let me know if you're working on it too.

I've got it ready but github won't let me push to the fork branch for some reason. Feel free to push.

[TomAFrench]

Awesome, LGTM.