bug: groth16 circom-adapter incorrect public signals

[erhant]

Hi, I am confident that the public signals are handled incorrectly in the Groth16 circom adapter.

First, lets check the vitalik integration test here. It belongs to the circuit:

template Test() {
	signal input x;
	signal output out;

	signal sym_1;
	signal y;

	sym_1 <== x * x;
	out <== (sym_1 * x) + (x + 5);
}

The circuit code above shows that we have 1 output (which is a public signal), 0 public inputs and 1 private inputs; these are also confirmed by the r1cs file here.

As the test asserts the witness returned from circom_to_lambda is as follows:

["1", "3", "23", "9"] // these are all hexadecimal btw

As per the README & the poseidon test here, the public signals are returned by the slice &witness[..qap.num_of_public_inputs]. There are two things wrong with this:

1. The public output was not considered, so num_of_public_inputs is only 1 due to the constant 1 added in here.
2. Even if the outputs were considered, we still have to consider two slices, e.g. one for the constant 1 and other for the 23 that skips over the private inputs, in other words we care about: ["1", ..., "23", ...] within our witness but we have no way to do this without knowing the number of non-intermediate private inputs.

I think there should be two things done:

• I believe the correct form of pub input calculation should be:

let num_of_pub_inputs =
    circom_r1cs["nPubInputs"].as_u64().unwrap() as usize +
    circom_r1cs["nOutputs"].as_u64().unwrap() as usize +
    1;

• The public signals should not be &witness[..qap.num_of_public_inputs] but instead slice in such a way that it skips over private inputs. Perhaps a QAP method can take a reference to the witness and do this, without letting the user do the slicing manually like this.

However, the QAP is edited in such a way that the proof verification actually passes with the wrong public inputs, as the passing tests show. I have not yet looked deep into what might change there, but would love to contribute if shown a hint 🙏🏻

[erhant]

I've begun a branch https://github.com/erhant/lambdaworks/tree/erhant/fix-circom-adapter to try and fix these, while also refactoring some code structure more according to Cargo recommendations if that is okay as well!

[erhant]

I changed the code a bit so that instead of the following case:

• Circom: ["1", ..outputs, ...inputs, ...other_signals]
• Lambda: ["1", ...inputs, ..outputs, ...other_signals]

we now have Lambda use the same layout as Circom as well; and let the public signals include the constant term by default. This solved all my problems actually, and I kept the order as is mainly due to this line here. It seems to expect k public inputs in order at the start of the witness.

Perhaps the ordering case written above was true but then it got patched, but was forgotten to be updated in the adapter?

[erhant]

I have made a draft PR (#959) with the changes, if you think the logic is alright I can clean it up a bit & update docs and make it ready for review 🤝