Is it necessary to add validation of the public signal values after the ZKProof generation?

[yushihang]

Thanks to @Chengxuan for the help during our discussion this afternoon.

After the discussion, I found that the main reason for the verification failure of the ZKProof generated concurrently for transfers on the contract side is related to the calculateWTNSBin() function.

This function is provided by the witness_calculator.js file, which is generated after compiling the circom file. It reads the witness data from the globally unique WebAssembly instance’s SharedRWMemory functions，which are not promise safe.

async _doCalculateWitness(input, sanityCheck) {
    //input is assumed to be a map from signals to arrays of bigints
    this.instance.exports.init(this.sanityCheck || sanityCheck ? 1 : 0);
    const keys = Object.keys(input);
    var input_counter = 0;
    keys.forEach((k) => {
      const h = fnvHash(k);
      const hMSB = parseInt(h.slice(0, 8), 16);
      const hLSB = parseInt(h.slice(8, 16), 16);
      const fArr = flatArray(input[k]);
      let signalSize = this.instance.exports.getInputSignalSize(hMSB, hLSB);
      if (signalSize < 0) {
        throw new Error(`Signal ${k} not found\n`);
      }
      if (fArr.length < signalSize) {
        throw new Error(`Not enough values for input signal ${k}\n`);
      }
      if (fArr.length > signalSize) {
        throw new Error(`Too many values for input signal ${k}\n`);
      }
      for (let i = 0; i < fArr.length; i++) {
        const arrFr = toArray32(normalize(fArr[i], this.prime), this.n32);
        for (let j = 0; j < this.n32; j++) {
          this.instance.exports.writeSharedRWMemory(j, arrFr[this.n32 - 1 - j]);
        }
        try {
          this.instance.exports.setInputSignal(hMSB, hLSB, i);
          input_counter++;
        } catch (err) {
          // console.log(`After adding signal ${i} of ${k}`)
          throw new Error(err);
        }
      }
    });
    if (input_counter < this.instance.exports.getInputSize()) {
      throw new Error(
        `Not all inputs have been set. Only ${input_counter} out of ${this.instance.exports.getInputSize()}`
      );
    }
  }

When using promises for concurrent ZKProof generations, this may lead to multiple different requests returning the same witness value, resulting in identical (and incorrect) contents in the generated public.json and proof.json.

I noticed that @Chengxuan has already submitted a PR #79 to address this issue 👍.

And from the perspective of better checking and protection, is it necessary to validate whether the input values of the circuit correspond to the data in the public signals, after the prove() function call is completed and the public signal is generated?

For example, we could check whether nullifiers[0] is equal to publicSignal[n], and so on.

This may allow us to discover this issue with the ZKProof before it is sent for verification on the contract side.”

[Chengxuan]

@yushihang thanks for raising the questions here and sharing your analysis.

Yes. I agree a local check will catch the invalid proof earlier. We should add those checks in.

Separately, I had a chat with @jimthematrix on this issue, the concurrency issue in the js witness calculator is something he's already aware of.

#84 has also been raised to cover concurrent proof generation exploration in golang SDK.

[yushihang]

Hi @Chengxuan

Thank you for your response and detailed explanation.

In our testing, we discovered a solution that might solve the issue of “Wrong witness data generated during concurrent requests using promises".

Whether this solution is suitable for all scenarios needs to be confirmed after a review by the Iden3 team. Therefore, I tried to submit a pull request here: iden3/circom#299.

In the meantime, we will also try using the solution you provided to address the issue.

Thanks very much.