feat(cli): Verify return against ABI and Prover.toml

[aakoshh]

Description

Problem*

Followup to #6757
Related to #6516

Summary*

Modified the execute command to:

• check that if the circuit should return a value according to the ABI, then there is indeed Some value we can parse from the WitnessMap
• check that if there is a return entry in Prover.toml then it matches the actual value returned by the circuit

These checks are done after the witness is saved, so we can inspect what went into the file if necessary; the main goal is to be able to use this integration testing, since the ABI parser already looks for the return key. build.rs will now check return values in execution_success, if present.

Added the test case from #6516 as extra coverage in addition to the unit test in #6757

Additional Context

Example of running the test with wrong output in Prover.toml:

❯ cargo test -q -p nargo_cli --test execute return_twice::forcebrillig_false_inliner_0

running 1 test
F
failures:

---- tests::execution_success::test_return_twice::forcebrillig_false_inliner_0_expects stdout ----
thread 'tests::execution_success::test_return_twice::forcebrillig_false_inliner_0_expects' panicked at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ops/function.rs:250:5:
Unexpected failure.
code=1
stderr=```"Unexpected return value: expected Vec([Field(100), Field(20)]); got Some(Vec([Field(100), Field(100)]))\n"```
command=`"/Users/aakoshh/Work/aztec/noir/target/debug/nargo" "--program-dir" "/Users/aakoshh/Work/aztec/noir/test_programs/execution_success/return_twice" "execute" "--force" "--inliner-aggressiveness" "0" "--check-return"`
code=1
stdout=
[return_twice] Circuit witness successfully solved
[return_twice] Circuit output: Vec([Field(100), Field(100)])
[return_twice] Witness saved to /Users/aakoshh/Work/aztec/noir/test_programs/execution_success/return_twice/target/return_twice.gz


stderr="Unexpected return value: expected Vec([Field(100), Field(20)]); got Some(Vec([Field(100), Field(100)]))\n"

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::execution_success::test_return_twice::forcebrillig_false_inliner_0_expects

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1664 filtered out; finished in 0.59s

error: test failed, to rerun pass `-p nargo_cli --test execute`

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[github-actions]

Compilation Sample

Program	Compilation Time	%
sha256_regression	0m1.463s	-8%
regression_4709	0m1.530s	-3%
ram_blowup_regression	0m17.412s	-2%
private-kernel-tail	0m1.469s	21%
private-kernel-reset	0m9.072s	-6%
private-kernel-inner	0m2.355s	-18%
parity-root	0m1.011s	6%
noir-contracts	2m51.793s	4%

[github-actions]

Peak Memory Sample

Program	Peak Memory	%
keccak256	78.64M	0%
workspace	121.87M	-1%
regression_4709	294.72M	0%
ram_blowup_regression	2.44G	0%
private-kernel-tail	209.05M	0%
private-kernel-reset	862.21M	0%
private-kernel-inner	307.94M	0%
parity-root	174.50M	0%

[TomAFrench]

Is this reading necessary to be honest so it's kind of surprised that we didn't error out when ABI decoding already.

If we have a witness map from which we want to decode return values and we find empty witnesses at indices that should hold return values then we should just error and that should avoid the need for this flag I think.

[aakoshh]

@TomAFrench the reason it did not complain about the gaps in the witness map is explained here:

Unlike for the circuit inputs, we tolerate not being able to find the witness values for the return value.
This is because the user may be decoding a partial witness map for which is hasn't been calculated yet.
If a return value is expected, this should be checked for by the user.

You are right that based on the ABI we can tell if the circuit has any return values. I'm not sure if execute can always demand that abi.decode returns Some; it sounds like fair assumption in the context of the execute command 🤷

But I'd argue that the flag still has value to easily assert the actual value of what is being returned, not just that something is, at least in the context of integration testing, where we use Prover.toml for inputs and it readily handles the output as well. At least it can catch inconsistencies like in the diamond_deps example.

[TomAFrench]

Ok, sorry I didn't read fully as I was on the tube. I'll have a look at this properly tomorrow.

[aakoshh]

I added another check to see if the return value is empty when it should be non-empty according to the ABI; all the tests are passing so I suppose that means this is a reasonable expectation. Again doing it after saving the (partial) witness map to file, but could be one earlier if the value cannot be printed.

Maybe we could compare return in Prover.toml whenever it is present, without any flags?

[jfecher]

Maybe we could compare return in Prover.toml whenever it is present, without any flags?

This sounds good to me. I thought we already compared the return value against the verifier toml by default so it'd be good to have that behavior.

[asterite]

Looks great!