feat(abi)!: add an explicit mapping from ABI params to witness indices

[TomAFrench]

Related issue(s)

Resolves #684

Description

Summary of changes

This PR builds out the logic for passing up an explicit mapping for ABI parameters to their witness indices.

This is done in the Evaluator as rather than pushing a public param's witnesses onto the public_inputs vector we insert all params' witnesses into the param_witnesses BTreeMap<String, Vec<Witness>>. This allows us to get the witness indices for any parameter defined in the ABI.

The Circuit struct still uses a PublicInputs as this can be easily calculated from param_witnesses.

This new param_witnesses map is passed up and stored in the ABI as it's crucial to be able to abi encode inputs into the initial witness. As we're now directly encoding into a witness map, I've split the ABI encode method into encode_to_witness and encode_to_array (similarly for decoding) where the array variant is used for passing to the verifer. (We can likely remove this if we make PublicInputs a mapping as discussed in Slack).

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.
• This PR requires documentation updates when merged.

Additional context

[TomAFrench]

One annoyance that I have with this change is that it's no longer possible to generate the ABI without fully compiling the circuit (as we need to insert the return value indices but only get those after compilation). I'm not sure whether we can address this however.

[kevaundray]

One annoyance that I have with this change is that it's no longer possible to generate the ABI without fully compiling the circuit (as we need to insert the return value indices but only get those after compilation). I'm not sure whether we can address this however.

Is this an important usecase? It is possible for us to do this, but it would require a bit of code change, essentially the ABI would assign the witnesses in a "natural" way and the Evaluator/compiler will need to follow that way. My assumption is that you will want all of these components, circuit + abi in one go, so would prefer the way you have already done it.

[kevaundray]

Gave this a once over, nothing stands out, will look in more detail tomorrow

[TomAFrench]

Is this an important usecase? It is possible for us to do this, but it would require a bit of code change, essentially the ABI would assign the witnesses in a "natural" way and the Evaluator/compiler will need to follow that way. My assumption is that you will want all of these components, circuit + abi in one go, so would prefer the way you have already done it.

This would work except for cases where the return value witness is shared with a public input though, correct? Seems like it could get messy.

It's not crucial but coming from solidity it's nice to be able to look at a function signature function foo(address bar, address baz) and knowing exactly how to encode arguments for it. Our current situation is similar to if there's a require(bar == baz) statement meaning that you should only provide the address once.

I'd say it's not required but we should try to rework how we assemble the ABI as it's a bit messy how it's being put together atm.

[TomAFrench]

It is possible for us to do this, but it would require a bit of code change, essentially the ABI would assign the witnesses in a "natural" way and the Evaluator/compiler will need to follow that way.

Offtopic but it occurs to me that we'll likely have to do something similar anyway to generalise ACIR across proving systems. Barretenberg relies on inputs being at 1..N and this is leaking into ACIR atm so we'll want to push the shuffling of indices to conform to the proving system after ACIR compilation.

[kevaundray]

e that we'll likely have to do something similar anyway to generalise ACIR across proving systems

Yeah we can push the offset problem to barretenberg.

Sidenote: I think we can make Witness(0) be 0 and Witness(1) be 1 in ACIR though. So all proving systems would need to do this offset and whenever they see 0 or 1, they regard it as a constant. Maybe not for 1, but I think it makes sense for 0.

[kevaundray]

LGTM -- waiting for the preprocess PR before merging this one

[kevaundray]

@TomAFrench pulled this out of the queue incase you wanted to merge the clap PR first. Feel free to add it back if not