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

Use snarkjs API and remove the hardcoding of the snarkjs command line path #782

Merged
merged 8 commits into from
Nov 1, 2023

Conversation

yuetloo
Copy link
Contributor

@yuetloo yuetloo commented Oct 26, 2023

This will fix #514

Tested this PR with:

  • tests/vanilla/testCheckKeys.sh
  • integrationTests/ts/tests/suites.test.ts
  • clrfund's maci v1 integration e2e tests

Note that the snarkjs.d.ts was added because @types/snarkjs doesn't work due to a typo causing path not found error. If the issue was fixed, the snarkjs.d.ts file can be removed and simply import the @types/snarkjs types

I also increased the poll duration in order for the integration tests to pass as they were failing due to the voting period being too short. This could mean that the changes introduced in this PR made the tests slower because they had to wait for threads to end as opposed to previously just being killed at the end of the snarkjs command line.

@ctrlc03 ctrlc03 self-requested a review October 26, 2023 23:11
Copy link
Collaborator

@ctrlc03 ctrlc03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for this.
Gonna first get this PR #775 merged - once we have a properly working CI we can then merge this too

@ctrlc03 ctrlc03 linked an issue Oct 27, 2023 that may be closed by this pull request
@ctrlc03
Copy link
Collaborator

ctrlc03 commented Oct 30, 2023

This will fix #514

Tested this PR with:

  • tests/vanilla/testCheckKeys.sh
  • integrationTests/ts/tests/suites.test.ts
  • clrfund's maci v1 integration e2e tests

Note that the snarkjs.d.ts was added because @types/snarkjs doesn't work due to a typo causing path not found error. If the issue was fixed, the snarkjs.d.ts file can be removed and simply import the @types/snarkjs types

I also increased the poll duration in order for the integration tests to pass as they were failing due to the voting period being too short. This could mean that the changes introduced in this PR made the tests slower because they had to wait for threads to end as opposed to previously just being killed at the end of the snarkjs command line.

@yuetloo can you please pull latest dev updates into this branch? that way we can see if all tests run correctly (we fixed some small bugs in the e2e tests)

Copy link
Member

@baumstern baumstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yuetloo
Copy link
Contributor Author

yuetloo commented Oct 31, 2023

@ctrlc03 , merged with the latest dev branch and passed integrationTests/test-suits

@baumstern
Copy link
Member

There are a few error in Integration Test.

Failure one

FAIL ts/__tests__/deployPollWithRandomSigner.test.ts (215.786 s)
  Test deployPollWithSigner
    ✕ Full tree, 4 batches, no bribers (210829 ms)

  ● Test deployPollWithSigner › Full tree, 4 batches, no bribers

    expect(received).toBeTruthy()

    Received: false

      31 |     it(test.description, async () => {
      32 |         const result = await executeSuite(test, expect)
    > 33 |         expect(result).toBeTruthy()
         |                        ^
      34 |
      35 |         let caughtException = false
      36 |         try {

      at ts/__tests__/deployPollWithRandomSigner.test.ts:33:24
      at step (ts/__tests__/deployPollWithRandomSigner.test.ts:33:23)
      at Object.next (ts/__tests__/deployPollWithRandomSigner.test.ts:14:53)
      at fulfilled (ts/__tests__/deployPollWithRandomSigner.test.ts:5:58)

  console.log
    stdout: Merging state subroots 1 / 1
    Executed mergeMaciStateAqSubRoots(); gas used: 712418
    Transaction hash: 0xe875bea54d2e3442f78466747772a307ed74ecb1cf3874e82ab73e5034ef7ae8
    
    All state subtrees have been merged.
    Merging subroots to a main state root...
    Executed mergeStateAq(); gas used: 1004922
    Transaction hash: 0x885540f2e8a81cffc1c8efcbab0656a6c7b24fde45d1d0aef2215dd582fada74
    The state tree has been merged.

      at execute (ts/__tests__/suites.ts:28:13)

  console.log
    rm -rf tally.json subsidy.json proofs/

      at execute (ts/__tests__/suites.ts:25:13)

  console.log
    stdout:

      at execute (ts/__tests__/suites.ts:28:13)

  console.log
    node build/index.js genProofs -x 0xb5E58e7CEe446772A8ACD9975D2242dC6B9666E3 -sk macisk.20df7d4e23b1430e28095e4a2856b4db43496beed4365e979b621f5d80382b2 -o 0 -r ~/rapidsnark/build/prover -wp ./zkeys/ProcessMessages_10-2-1-2_test -wt ./zkeys/TallyVotes_10-1-2_test -zp ./zkeys/ProcessMessages_10-2-1-2_test.0.zkey -zt ./zkeys/TallyVotes_10-1-2_test.0.zkey -t tally.json -f proofs/

      at execute (ts/__tests__/suites.ts:25:13)

  console.log
    stdout:

      at execute (ts/__tests__/suites.ts:28:13)

  console.error
    Error: exec failed: Error: ENOENT: no such file or directory, scandir 'proofs/'
        at Object.readdirSync (node:fs:1438:3)
        at /home/runner/work/maci/maci/cli/ts/proveOnChain.ts:210:30
        at step (/home/runner/work/maci/maci/cli/build/proveOnChain.js:33:23)
        at Object.next (/home/runner/work/maci/maci/cli/build/proveOnChain.js:14:53)
        at fulfilled (/home/runner/work/maci/maci/cli/build/proveOnChain.js:5:58)
        at processTicksAndRejections (node:internal/process/task_queues:96:5)
    
        at execute (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:30:15)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:288:9
        at step (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:33:23)
        at Object.next (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:14:53)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:8:71
        at new Promise (<anonymous>)
        at Object.<anonymous>.__awaiter (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:4:12)
        at executeSuite (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:39:51)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/deployPollWithRandomSigner.test.ts:32:42
        at step (/home/runner/work/maci/maci/integrationTests/ts/__tests__/deployPollWithRandomSigner.test.ts:33:23)

      296 |     }
      297 |     catch(e) {
    > 298 |         console.error(e)
          |                 ^
      299 |         return false
      300 |     } 
      301 |     

      at ts/__tests__/suites.ts:298:17
      at step (ts/__tests__/suites.ts:33:23)
      at Object.next (ts/__tests__/suites.ts:14:53)
      at ts/__tests__/suites.ts:8:71
      at Object.<anonymous>.__awaiter (ts/__tests__/suites.ts:4:12)
      at executeSuite (ts/__tests__/suites.ts:39:51)

Failure two

 console.log
    stdout: Merging message subroots 1 / 4
    Executed mergeMaciStateAqSubRoots(); gas used: 861080
    Transaction hash: 0xe66739f7467c3723e19fcfb1b6a09d48559a4faeee35d671f1d0ae28967d3aeb
    
    All message subtrees have been merged.
    Merging subroots to a main message root...
    Executed mergeMessageAq(); gas used: 71043
    Transaction hash: 0x3a260915402b9f717530e1b8a208448d1a56e211332e66daaca1a350f28d27df
    The message tree has been merged.

      at execute (ts/__tests__/suites.ts:28:13)

  console.log
    node build/index.js mergeSignups -x 0x5F600562E388F4d85e52979C499724045DF3Bd61 -o 0

      at execute (ts/__tests__/suites.ts:25:13)

  console.log
    stdout:

      at execute (ts/__tests__/suites.ts:28:13)

  console.error
    Error: exec failed: Error: the transaction failed.
    Error: VM Exception while processing transaction: reverted with reason string 'PollE01'
    
        at execute (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:30:15)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:198:39
        at step (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:33:23)
        at Object.next (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:14:53)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:8:71
        at new Promise (<anonymous>)
        at Object.<anonymous>.__awaiter (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:4:12)
        at executeSuite (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.ts:39:51)
        at /home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.test.ts:12:46
        at step (/home/runner/work/maci/maci/integrationTests/ts/__tests__/suites.test.ts:33:23)

      296 |     }
      297 |     catch(e) {
    > 298 |         console.error(e)
          |                 ^
      299 |         return false
      300 |     } 
      301 |     

      at ts/__tests__/suites.ts:298:17
      at step (ts/__tests__/suites.ts:33:23)
      at Object.next (ts/__tests__/suites.ts:14:53)
      at ts/__tests__/suites.ts:8:71
      at Object.<anonymous>.__awaiter (ts/__tests__/suites.ts:4:12)
      at executeSuite (ts/__tests__/suites.ts:39:51)

@ctrlc03 ctrlc03 merged commit ba30d51 into privacy-scaling-explorations:dev Nov 1, 2023
8 checks passed
@samajammin samajammin deleted the fix/snark-path branch November 2, 2023 08:50
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

Successfully merging this pull request may close these issues.

Use snarkjs API call instead of CLI call
3 participants