Returned values from main are set to public (Successor) #731

kevaundray · 2023-02-01T21:42:46Z

Related issue(s)

Resolves #455

Description

This is #634 however, the PR reverts back to commit #21c0b and merges the updated master into it.

This PR does not require a new release of ACVM/ACIR since we do not need to have public outputs . A different method was suggested where we could possibly avoid deduplication, however both methods require deduplication, so I have opted to revert back to this simpler change.

Summary of changes

See #634

Return values are no longer supplied by the prover, they are computed.
Setting the return value of the main function to be public boils down to adding witness indices into the public_inputs vector

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.

Additional context

- Add a SetPub Expression - Instead of adding a constrain statement and adding a `return` parameter to main, we now specify that the return type should be set as public. This is then dealt with in the ssa pass.

…nto the evaluator

- `get_or_generate_witness` is not named `generate_witness`

- This can be done as a separate PR. Removing it to make PR easier to review, an issue has been opened up for this

… noir parameters

generate_witness will panic when trying to generate a witness for a constant

codegening an array and calling to_node_ids will produce a single node_id, whereas we want to fetch all elements of the array

- When a public input is return as a public output (return value), it is then added into the public input list twice. Using the current code, we need this duplication since we want to write the return values into the toml files. For proving and verifying we want to remove these duplicate values

- The prover no longer supplies a `return` field in toml. - The ABI will have a `return` field, so when proving, we use the `skip_output` flag to ignore the `return` field in the ABI - As noted in the comments, this will look cleaner when we add a `public_outputs` field

…esponds to private input from the ABI (main parameters)

- Additionally, private ABI inputs are not allowed

- Only create the Return Operation if the return type from main is not Unit

crates/noirc_evaluator/src/ssa/code_gen.rs

crates/noirc_evaluator/src/ssa/acir_gen.rs

crates/noirc_evaluator/src/lib.rs

guipublic

The return value in the prover.toml should not be removed, as this is not a fix for the issue and it removes a feature.

crates/nargo/src/cli/mod.rs

crates/noirc_abi/src/lib.rs

jfecher

Frontend parts look fine to me. I am fine with merging once the existing comments are resolved.

crates/nargo/src/cli/prove_cmd.rs

crates/noirc_evaluator/src/lib.rs

private values in the ABI cannot be returned as public

* master: feat(noir)!: Returned values are no longer required by the prover (noir-lang#731)

* master: Rename methods that use `conditionalize` to be more descriptive (#739) feat(noir)!: Returned values are no longer required by the prover (#731) chore: explicit versions for dependencies (#727) chore: readability improvements (#726) feat(nargo): include short git commit in cli version output (#721) Remove print to console for named proofs in `nargo prove` (#718) chore: clean up serde-related dependencies (#722) Handle out-of-bound errors in CSE (#471) (#673) Remove unused dependencies and only use workspace inheritance on shared deps (#671) feat(std_lib)!: modulus bits/bytes methods, and to_bits -> to_le_bits (#697) Implement numeric generics (#620) Review some TODO in SSA (#698) Replace `toml_map_to_field` and `toml_remap` with traits to map between `InputValue`s and `TomlTypes` (#677) Apply witness visibility on a parameter level rather than witness level (#712)

kevaundray added 30 commits January 12, 2023 20:20

acir - add public_outputs field to acir structure

acc99cd

monomorphisation:

5e3a9c8

- Add a SetPub Expression - Instead of adding a constrain statement and adding a `return` parameter to main, we now specify that the return type should be set as public. This is then dealt with in the ssa pass.

evaluator - acir now has a public_outputs field and so we add one i…

8e53b2f

…nto the evaluator

ssa - Add SetPub operation and opcode

d5c5a0b

ssa - codegen for setpub

8f41456

ssa - setpub acir_gen

96141f7

Merge branch 'master' into kw/acvm3.0

dfaf279

Overwrite merge commit to revert change to get_max_value

2211a93

ssa - acir_gen

387b28e

- `get_or_generate_witness` is not named `generate_witness`

revert: remove public_outputs field

1a8fca5

- This can be done as a separate PR. Removing it to make PR easier to review, an issue has been opened up for this

remove the return type from the abi when converting abi parameters to…

8b06277

… noir parameters

clippy

0adbf27

Merge branch 'master' into kw/fix-return-type

06f792b

ssa - acir_gen : create witnesses for constants

fbf4b26

generate_witness will panic when trying to generate a witness for a constant

ssa - acir_gen : deref node_id when we have an array

aff8d33

codegening an array and calling to_node_ids will produce a single node_id, whereas we want to fetch all elements of the array

Merge branch 'master' into kw/fix-return-type

323f90d

nargo/tests - remove "return" field from toml files

d2a8a7d

cargo fmt imports

a0a543d

remove SetPub Expression

d5bfaf8

ssa - remove SetPub operation

d670b8d

evaluator - add a method which tells you whether a witness index corr…

4d6f86c

…esponds to private input from the ABI (main parameters)

- use Operation::Return to set the values returned from main as public.

e4a9788

- Additionally, private ABI inputs are not allowed

fix clippy

05564f2

ssa-integer: do not skip the return operation during integer overflow

bd9e909

- Only create the Return Operation if the return type from main is not Unit

Merge branch 'master' into kw/fix-return-type

7ff696f

ssa-acir_gen : replace todo with Err Result variant

fb7c6a3

Merge branch 'master' into kw/fix-return-type

6316c38

Merge branch 'master' into kw/fix-return-type

ffdc2d2

kevaundray commented Feb 1, 2023

View reviewed changes

crates/noirc_evaluator/src/ssa/code_gen.rs Show resolved Hide resolved

kevaundray commented Feb 1, 2023

View reviewed changes

crates/noirc_evaluator/src/ssa/acir_gen.rs Show resolved Hide resolved

kevaundray commented Feb 1, 2023

View reviewed changes

crates/noirc_evaluator/src/lib.rs Outdated Show resolved Hide resolved

kevaundray requested review from jfecher and guipublic February 1, 2023 21:53

kevaundray added 2 commits February 1, 2023 22:01

reduce diff

78fc162

move dedup methods to mod.rs file

4a72ccf

guipublic requested changes Feb 2, 2023

View reviewed changes

crates/nargo/src/cli/mod.rs Outdated Show resolved Hide resolved

crates/nargo/src/cli/mod.rs Outdated Show resolved Hide resolved

crates/nargo/src/cli/mod.rs Show resolved Hide resolved

crates/noirc_abi/src/lib.rs Show resolved Hide resolved

remove TODO on return type keyword

c77853d

jfecher reviewed Feb 2, 2023

View reviewed changes

crates/nargo/src/cli/prove_cmd.rs Outdated Show resolved Hide resolved

move deduplication code into compile_circuit_and_witness

c0146b6

TomAFrench reviewed Feb 2, 2023

View reviewed changes

crates/noirc_evaluator/src/lib.rs Show resolved Hide resolved

crates/noirc_evaluator/src/lib.rs Show resolved Hide resolved

TomAFrench mentioned this pull request Feb 2, 2023

feat(nargo): add nargo execute command #725

Merged

5 tasks

kevaundray force-pushed the kw/revert-21c0b-fix-return-type branch from 07c2f1c to de7d81e Compare February 2, 2023 19:42

update num_witnesses_abi_len parameter

ba2e28b

kevaundray force-pushed the kw/revert-21c0b-fix-return-type branch from de7d81e to ba2e28b Compare February 2, 2023 20:50

TomAFrench mentioned this pull request Feb 2, 2023

Nargo producing proofs which cannot be verified. #466

Closed

2 tasks

kevaundray added 3 commits February 2, 2023 21:53

fix example:

bb95372

private values in the ABI cannot be returned as public

use HashMap and check for previous insertion value

5c24c36

fix clippy

5ffaad6

kevaundray requested a review from guipublic February 2, 2023 23:01

guipublic approved these changes Feb 3, 2023

View reviewed changes

kevaundray merged commit 90b6036 into master Feb 3, 2023

kevaundray deleted the kw/revert-21c0b-fix-return-type branch February 3, 2023 11:35

TomAFrench added a commit to TomAFrench/noir that referenced this pull request Feb 3, 2023

Merge branch 'master' into nargo-execute

3b624d6

* master: feat(noir)!: Returned values are no longer required by the prover (noir-lang#731)

TomAFrench mentioned this pull request Feb 3, 2023

Use SetPub instead of creating new public inputs for return values #625

Closed

TomAFrench mentioned this pull request Feb 3, 2023

Return values in Prover.toml should not be supplied by the prover #624

Closed

2 tasks

This was referenced Feb 16, 2023

Binary solver for array equality constraints #520

Closed

chore: remove support for InputValue::Undefined #867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Returned values from main are set to public (Successor) #731

Returned values from main are set to public (Successor) #731

kevaundray commented Feb 1, 2023

guipublic left a comment

jfecher left a comment

Returned values from main are set to public (Successor) #731

Returned values from main are set to public (Successor) #731

Conversation

kevaundray commented Feb 1, 2023

Related issue(s)

Description

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

Additional context

guipublic left a comment

Choose a reason for hiding this comment

jfecher left a comment

Choose a reason for hiding this comment