Returned values from main are set to public (Deprecated)

[kevaundray]

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves #455

Description

Summary of changes

Instead of creating a return parameter in the main function, we set the return type to public in the SSA pass.

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

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

(If applicable.)

[kevaundray]

• The return field is no longer being added as a parameter to main in the monomorphisation pass, so we need to add it somewhere else. -- This can be done when we parse the ABI. (Though in a separate PR)
• return should now be empty and we get the value from executing the circuit

[kevaundray]

Clippy errors are fixed in #614 so will merge that and rebase

[kevaundray]

For clarity, we should define what happens when a public input is added as a public output:

When a public input becomes a public output. It should be a no-op, since the witness was already public.

Lets go over how to make it a no-op:

We could check for public witness indices and then not add the witness indice into the public inputs vector in SSA if we see that it is already present. -- This is not possible because we need to map the "return" type to its corresponding public witness indices and this mechanism would break if we did not have the duplicates, since it relies on alignment of public witness indices to decide what values to map the "return" type to.

This can be fixed in a separate PR by having a separate public output field which means that we no longer need to rely on the "return" type being last in the abi parameters.

For the verifier, we don't want to add the same public input twice, so when we are encoding the ABI, we should also take in the public witness indices for public inputs and public outputs, so that we avoid duplicates.

As an example:

fn main(x : pub Field) -> pub Field {
 x
}

Should only produce 1 public witness index when sent to the proving system to verify a proof.

Even though the verifier.toml can look like:

x = "5"
return = "5"

[TomAFrench]

I think this currently breaks proving when passing an expected return value as its index is no longer directly after the other inputs.

noir/crates/nargo/src/cli/prove_cmd.rs

Lines 97 to 104 in ff2786f

	let mut solved_witness: BTreeMap<Witness, FieldElement> = encoded_inputs
	.into_iter()
	.enumerate()
	.map(|(index, witness_value)| {
	let witness = Witness::new(WITNESS_OFFSET + (index as u32));
	(witness, witness_value)
	})
	.collect();

We'd either need to have the Circuit struct track the indices of all inputs (rather than just private ones) so we can write the inputs similarly to how we currently read the public inputs from the solved witness

noir/crates/nargo/src/cli/prove_cmd.rs

Lines 68 to 74 in ff2786f

	let encoded_public_inputs: Vec<FieldElement> = compiled_program
	.circuit
	.public_inputs
	.0
	.iter()
	.map(|index| solved_witness[index])
	.collect();

or alternatively we lean into #624 and never insert the expected return value into the witness and have Circuit expose some way to easily pull out the return value witnesses. I think this way makes the most sense.

[kevaundray]

I think this currently breaks proving when passing an expected return value as its index is no longer directly after the other inputs.

noir/crates/nargo/src/cli/prove_cmd.rs

Lines 97 to 104 in ff2786f

	let mut solved_witness: BTreeMap<Witness, FieldElement> = encoded_inputs
	.into_iter()
	.enumerate()
	.map(|(index, witness_value)| {
	let witness = Witness::new(WITNESS_OFFSET + (index as u32));
	(witness, witness_value)
	})
	.collect();

We'd either need to have the Circuit struct track the indices of all inputs (rather than just private ones) so we can write the inputs similarly to how we currently read the public inputs from the solved witness

noir/crates/nargo/src/cli/prove_cmd.rs

Lines 68 to 74 in ff2786f

	let encoded_public_inputs: Vec<FieldElement> = compiled_program
	.circuit
	.public_inputs
	.0
	.iter()
	.map(|index| solved_witness[index])
	.collect();

or alternatively we lean into #624 and never insert the expected return value into the witness and have Circuit expose some way to easily pull out the return value witnesses. I think this way makes the most sense.

Yeah that makes sense; agree with exposing some way to easilly pull out the return value. I was initially going to merge the public outputs change to do this, but decided to make the PR smaller.

[guipublic]

I don't like the setpub and the deduplication which makes things not clear to me.
I think the process should be the following:

1. Instead of adding a new setpub instruction when we return from main, we use the already existing return instruction
2. When we process the return in acir_gen, we generate, if needed, a witness corresponding to the values of the returned expression. Then we have the following cases:

• If it is a private input (i.e private input in the ABI), we return an error stating that this witness should be public
• else, if it does not belongs to the public inputs, we add it to the public inputs vector

In any case, we also add this witness to a new "returned_witness" vector, which keep track of the returned witnesses. This vector may have duplicates, while the public inputs have no duplicate by construction.

3. When the circuit is created, we update the ABI by setting the "returned_witness".
4. The "returned_witness" is used to handle properly return value in prover and verifier.toml

Let's see this with a couple of example:

fn main(pub x) -> Field {
 x
}
x=3 in prover.toml

will have one public witness and that's all
in the abi, we have return="3" and returned_witness=[1]
if x is not declared as pub, we will issue an error.

fn main(pub x) -> Field,Field {
 (x,x)
}
x = 3 in prover.toml

will have one public witness and that's all.
in the abi, we have return="3" and returned_witness=[1,1]

[kevaundray]

Blocked currently by #649 since we don't want to panic when private inputs are set to public outputs

[guipublic]

You should not modify the return behaviour in prover.toml
The public inputs should not have duplicate

[kevaundray]

From slack call; Guillaume noted that if the public outputs was just a subset of public inputs, we can therefore remove duplication code -- This PR can then wait until after the migration PR

EDIT: after completing the PR, it seems that we indeed do still need deduplication for the case where the verifier.toml contains values that map to the same witness index

[kevaundray]

This issue has been succeeded by #731 . We have reverted back to the original method since both methods require deduplication whereas the initial method does not require us to cut a release of ACIR and is simpler to reason about.