Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yushihang opened this issue Sep 24, 2024 · 2 comments

Comments

@yushihang
Copy link

yushihang commented Sep 24, 2024

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
Copy link
Contributor

@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
Copy link
Author

yushihang commented Sep 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants