fix: change initial memory size parameter from 32767 to 0 in witness_calculator

[ichub]

The current behavior of the witness calculator builder code is that when initializing a witness calculator wasm module, it specifies an initial memory size equal to 32767 * 64kb ~= 2gb (the initial argument is interpreted as 'quantity of pages', and each 'page' in wasm is specified to be 64kb). If allocating that quantity of memory fails, the code performs an exponential backoff with a factor of 2 - i.e. it then attempts to allocate 1gb, then 512mb, then 256mb, until allocation succeeds or fails at 0 pages.

This memory allocation strategy poses a problem for us at zupass. Our app is built primarily for phones, where memory is much more limited than on desktop environments. Thus, in practice, we have found that our website crashes with an OOM after ~2 proofs on older devices, which results in a poor user experience for a non-negligible portion of our users.

After identifying the section of code modified in this PR as being the culprit behind the OOM crashes, I forked and modified the circom_runtime and snarkjs libraries with the same change that I am proposing in this pull request. These forks are now included in the zupass repo here. I verified that this change worked properly by making sure that my forked snarkjs depended on my forked circom_runtime, and that the tests defined in my forked snarkjs package indeed passed.

logs from running `yarn test` in my fork of snarkjs

➜  snarkjs git:(main) yarn test
yarn run v1.22.21
$ mocha


  Fflonk test suite
    ✔ fflonk full prove (319ms)

  Full process
    ✔ powersoftau new (43ms)
    ✔ powersoftau contribute  (1050ms)
    ✔ powersoftau export challenge
    ✔ powersoftau challenge contribute (1034ms)
    ✔ powersoftau import response (245ms)
    ✔ powersoftau beacon (1095ms)
    ✔ powersoftau prepare phase2 (15149ms)
    ✔ powersoftau verify (890ms)
    ✔ groth16 setup (973ms)
    ✔ zkey contribute  (153ms)
    ✔ zkey export bellman (456ms)
    ✔ zkey bellman contribute (145ms)
    ✔ zkey import bellman (567ms)
    ✔ zkey beacon (144ms)
    ✔ zkey verify r1cs (1067ms)
    ✔ zkey verify init (99ms)
    ✔ zkey export verificationkey
allocating 5
    ✔ witness calculate
    ✔ checks witness complies with r1cs
    ✔ groth16 proof (115ms)
    ✔ groth16 verify
    ✔ plonk setup (220ms)
    ✔ zkey export verificationkey
    ✔ plonk proof (904ms)
    ✔ plonk verify

  keypair
    ✔ It should calculate the right g2_s for the test vectors (38ms)

  snarkjs: Polynomial tests
    ✔ should return the correct degree
    ✔ should check if two polynomials are equal
    ✔ it should blind coefficients
    ✔ should get the correct coefficient
    ✔ should get the correct length
    ✔ should evaluate a polynomial
    ✔ should add a polynomial
    ✔ should sub a polynomial
    ✔ should mul a polynomial with a scalar
    ✔ should add a scalar
    ✔ should sub a scalar
    ✔ should divide by a polynomial
    ✔ should divide by (X^m - y)
    ✔ should multiply by (X-value)
    ✔ should multiply by X
    ✔ should exp a polynomial
    ✔ should split a polynomial
    ✔ should truncate a polynomial
    ✔ should interpolate a polynomial using Lagrange Interpolation
    ✔ should compute a zerofier polynomial


  47 passing (26s)

✨  Done in 26.48s.

Another change I performed on the zupass end is that I used the npm patch-package package to manually patch these two lines of code during package installation of our dependencies. You can see how I did that here. I tested that proving and verifying our zk proofs worked properly by running the proving and verification code via some UI interactions that zupass supports.

It is my understanding that memory can be allocated dynamically by the wasm runtime. From perusing this package, it seems that the wasm builder code is a c++ program, which uses malloc, which I understand to be possible in wasm due to the existence of the memory.grow instruction. From my current perspective, it seems that specifying an initial memory size is not strictly necessary for the proper functioning of a wasm module. Perhaps this implementation was designed to be the way that it is to increase the performance of a single-proof use-case? I could see it being possible that the current behavior results in faster proving due to not having to allocate wasm memory multiple times.

Anyways, I do not have a full understanding of how the code I am proposing to modify ended up in the state that it is now, and would appreciate some guidance/explanation if possible. Additionally, perhaps there are some technical limitations to changing the initial memory size to be 0 that I am not aware of due to a lack of context.

To conclude: I believe this proposed change to be a strict improvement over the current implementation (holding some space for uncertainty), as it enables a larger quantity of proofs to be generated in memory-constrained environments, which are the environments we are targeting with zupass.

Please let me know if there is any other information needed to get this merged, or how we might arrive to more certainty regarding the correctness of this change.

[OBrezhniev]

Hi @ichub!

Would passing memorySize as one of the options (and redefining it externally) work for you?

If yes, could you please change your PR to implement this change and leaving default value (32767) if non is specified externally through the options?

[artwyman]

Hi, I work with Ivan on Zupass and can help follow up here.

Would passing memorySize as one of the options (and redefining it externally) work for you?

Passing memorySize as an option would be fine for us, but I think it would require some work to make sure such an option could be passed down correctly through the layers. We're using SnarkJS and it doesn't look like it's passing any options at all in relevant cases. My best attempt to identify the relevant call stack is these lines:

https://github.com/iden3/snarkjs/blob/8e43a6aa0625b47058f177b7b660b1fa9c0bcbca/src/groth16_fullprove.js#L31
https://github.com/iden3/snarkjs/blob/8e43a6aa0625b47058f177b7b660b1fa9c0bcbca/src/wtns_calculate.js#L34

circom_runtime/js/witness_calculator.js

Line 23 in be7dbb4

export default async function builder(code, options) {

Those layers in SnarkJS either have no options, or have an unused options arg not passed down to the witness calculator. It does look like some WC options are passed in a debug case here:

https://github.com/iden3/snarkjs/blob/8e43a6aa0625b47058f177b7b660b1fa9c0bcbca/src/wtns_debug.js#L67

@OBrezhniev would you be willing/able to help us get changes into SnarkJS to pass such an option? I'm not sure what the intended structure might be for those layers. Or should we explore some other approach? I could imagine exporting a global config constant, or exporting a function to set such a constant. That feels kinda hacky, though.

[OBrezhniev]

@artwyman, let memorySize = 0; is causing snarkjs browser test fail.

I've made memorySize an option, which you can pass to snarkjs wtns calc.
See #111 and iden3/snarkjs#517

You can test it by changing snarkjs version in your package.json to
"iden3/snarkjs#12da614867ef5699aa9b909a7ec957053eef1f3f"

[artwyman]

Thanks! We'll give this a try and get back to you, probably next week.

[ichub]

Hi @OBrezhniev, thanks for making these changes to both circom_runtime and snarkjs!

I've attempted to integrate the changed packages into Zupass. I hit one small snag to which I have a proposed fix here: iden3/snarkjs#519. Other than that small issue, all your changes look good to me.

[OBrezhniev]

@ichub @artwyman #111 and iden3/snarkjs#517 finally got their way into the release: https://github.com/iden3/snarkjs/releases/tag/v0.7.5
Closing this PR. Thank you for your contributions!