ACIR 0.4

[guipublic]

Problem

ACIR 0.3 has not been reviewed and introduced many controversial changes.

Solution

Change to a new version of ACIR 0.4 which resolve those points

Additional context

I list below the main points.

1. Removal of AND/XOR/SHA256/.. opcodes
   These opcodes have been placed under the blackboxfunction opcode, which for instances means that the solver is not solving them anymore but rely on the solve_blackbox function implemented by every backends.
   This does not make sense as AND/XOR/SHA256/.. are not blackbox at all. Everybody should knows what they mean and we can rely on a common solving method. They should not be backend specific. A backend could support them or not, but there is no point to have a special blackbox function code to identify them: AND/XOR/SHA256/.. are universal.
   Another consequence of having them as a blackboxfunction is that they rely on FunctionInput which requires a num_bit value for every input witness. Our own Aztec proving system, is not even adapted to this interface, and I don't believe we want to change this. Consequently, using different num_bits for the inputs on ACIR will lead to errors, while this was not possible before as the ACIR specification would not make this possible.
   Another example is the RANGE blackboxfunction which now has to return an error when used with more than one input.
   Not only we now have to specify more data in ACIR, but it can also lead to more errors.

2. blackboxfunction opcode
   The blackbox name does not make sense as everything is a blackbox, unless you know how each proving system is implementing them, but then this name should not depend on the knowledge of a particular person.
   What make sense is to allow backends to define functions that are specific for them, which then require an additional identifier and a way for the backend to solve these functions. These are the features that blackboxfunction provides.
   Besides FunctionInput defines the witness inputs for blackboxfunction, so the naming is not coherent there.
   As a result, BlackBoxFunction should be renamed into Function (or AcirFunction, or CustomFunction, or GadgetCall, with similar name for the inputs)

3. FunctionInputs
   A FunctionInput specify a num_bits for each input witness. This does not make sense because ACIR lives inside a finite field and does not handle bits.
   However, any ACIR function (I mean by that a blackboxfunction) may require extra parameters but these are specific to each function.
   So in order to supply parameters in a generic way, I suggest ACIR function define the following inputs:
   Vec<Witness>: the input witnesses
   Vec<u32>: the extra parameters. Note we do not need to use u32, we could use FieldElement, bytes,... but u32 is both versatile and easy to manipulate. If we use bytes, there is the question of big/little endianness, if we use FieldElement, there is the question of the field and its representation (although that should not be an issue at ACIR level)

4. TODOs
   There are many TODOs that should be removed and have issues created instead

5. Improvements
   I noticed a bunch of small improvements which should be implemented:

• term_multiplication() and term_addition() are not clear and should be renamed into push_multiplication_term() and push_linear_term()
• oddrange and truncate directives should be removed once PR635 is merged
• we should move the boolean() and boolean_expr() functions from acir_gen and use them in fallback
• there are some un-useful asserts which check for lengths equality while the lengths are equal by the code
• solve() method is still recursive. We should use the de-recursive version from the BinarySolver PR
• PWG should use a method so that insertion into the solved witness map always ensure they are not already solved with a different value
• signature.rs is empty and should probably be replaced by ecdsa. This one has some code which should belongs to noir
• range_optimizer.rs is empty and should be removed
• The general optimiser is deprecated by the simplification PR and I suppose the two should be merged. I also agree with this comment: XXX: Perhaps this optimisation can be done on the fly

[kevaundray]

Thank you for writing this.

My general comment after writing this is that these issues can be split into a few categories:

• changes which were there prior to any code change -- these have nothing to do with 0.3
• changes which were introduced by 0.3 -- there are about 3 of them, most to do with cosmetics
• PRs for things created after 0.3 in Noir -- these have nothing to do with switching to 0.3

We should split this up and only focus on the changes that were introduced by 0.3 that should be put into 0.3.3 first. I was under the impression that we want to solve these issues first.

There are also quite a few issues where you say we should do something without proper explanation, I have asked for clarification or a separate issue to flesh out the details.

1. They are black box functions by definition. The Aztec backend has been adopted to change this and perhaps we need to change to a heterogenous structure instead so that you cannot supply num_bits to all inputs. There is already an issue open for this.

2. There is already an issue open for possibly renaming BlackBoxFunction to Function or FunctionInput to BlackBoxFunctionInput

3. This is not how you should interpret what's going on here; a field element is interpreted as an integer which can have an associated number of bits. We also shouldn't use FieldElement here because those are witness indices and witness indices ever need to be that large.

4. There should be an issue already created for this.

• I believe this was there prior to 0.3 changes, I think it's a good change.

• There is an issue open to merge upstream changes -- that PR will need to merge ACIR changes into this repo.

• Please create a separate issue for this with explanation.

• Please create a separate issue for this with explanation and link to where these unhelpful length equalities are being done.

• Please create an issue with an explanation

• This would help with UX.

• Please create a separate issue

• Yep, that's right

• This has not been merged and I think we should look into different ways to do it. Re doing it on the fly, I see value in having all passes done in one place

[guipublic]

1. They are black box functions by definition. The Aztec backend has been adopted to change this and perhaps we need to change to a heterogenous structure instead so that you cannot supply num_bits to all inputs. There is already an issue open for this.

This is not my point, my point is about what was previously the AND/XOR/SHA256/.. gates. I explain why they should not be blackboxfunction, and there is no issue open for this as far as I know.

2. There is already an issue open for possibly renaming BlackBoxFunction to Function or FunctionInput to BlackBoxFunctionInput

Thank you, I will add my comment to the issue then.

3. a field element is interpreted as an integer which can have an associated number of bits. We also shouldn't use FieldElement here because those are witness indices and witness indices ever need to be that large.

You are saying that inputs to blackbox functions are integers represented by FieldElements with a number of bits. And I am saying that this is a bad. Function inputs should be more flexible.
What I call extra-parameter is NOT a witness index (but could be, as it can be anything), it is some data that only the function knows how to deal with it. So it depends on the function. It could be a field, but I rather suggested to use u32.

4. There should be an issue already created for this.

Ok

5

I will create separate issues.
Note that it should be obvious when I do not provide explanation, but then I agree that if it is obvious, I should be able to provide a simple explanation, and I will do it.
Note also that creating an issue for every small point is much less practical than commenting in a PR. So I hope that in the future we will use a PR process for any significant change.

[TomAFrench]

Closing this as ACVM is a very different beast from the time of this issue.