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

The latest commit (ffd6b2a) doesn't work in WASM from TS #198

Closed
iAmMichaelConnor opened this issue Apr 8, 2023 · 3 comments · Fixed by AztecProtocol/aztec-packages#263
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@iAmMichaelConnor
Copy link
Contributor

This commit points to bberg master, following @suyash67's gruelling rebase (thanks!)

But if you try to run the e2e tests from aztec3-packages (after pointing to aztec3-circuits master and bberg master), you get an error like:
LinkError: WebAssembly.instantiate(): memory import 15 is smaller than initial 23, got 20

Note: there's a new -DUSE_TURBO=true env variable that should be set when cmake-ing bberg, but that doesn't help.

I haven't tried running this repo's c++ tests with a wasm build locally.

This should be a high priority to fix, as we really oughtn't push more code on top of this broken branch...

@iAmMichaelConnor iAmMichaelConnor added bug Something isn't working help wanted Extra attention is needed labels Apr 8, 2023
@github-project-automation github-project-automation bot moved this to Todo in A3 Apr 8, 2023
@ludamad
Copy link
Contributor

ludamad commented Apr 8, 2023

It seems that we need to change the initial=20 line in the wasm wrapper to initial=23 (might be worth bumping to 30?). Not sure why we need 3+ more memory pages now, though. However, more issues are hit past this

 FAIL  src/kernel/kernel.test.ts (7.711 s)
  ● abis wasm bindings › gets dummy kernel data

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      12 |   });
      13 |
    > 14 |   it('gets dummy kernel data', async () => {
         |   ^
      15 |     await expect(getDummyPreviousKernelData(wasm)).resolves.toBeDefined();
      16 |   });
      17 |

      at kernel/kernel.test.ts:14:3
      at kernel/kernel.test.ts:7:1

  ● abis wasm bindings › computes function tree

    Can only handle one async call at a time: abis__compute_function_tree(59455424,4,102796352)

      85 |   public async call(name: string, ...args: any) {
      86 |     if (this.state) {
    > 87 |       throw new Error(`Can only handle one async call at a time: ${name}(${args})`);
         |             ^
      88 |     }
      89 |     this.state = { continuation: false };
      90 |     let result = this.callExport(name, ...args);

      at AsyncCallState.call (../../foundation/src/wasm/wasm/async_call_state.ts:87:13)
      at CircuitsWasm.asyncCall (wasm/circuits_wasm.ts:184:38)
      at computeFunctionTree (kernel/kernel.ts:39:14)
      at Object.<anonymous> (kernel/kernel.test.ts:21:24)

@ludamad
Copy link
Contributor

ludamad commented Apr 8, 2023

AztecProtocol/aztec-packages#216 Looks like it's not exactly broken, just takes more time and memory and both have limits in the repo. I don't think any dev here should be stalled, but I am curious what has changed in bberg

@iAmMichaelConnor
Copy link
Contributor Author

but I am curious what has changed in bberg

A huge amount - for a couple of months (I think), we've been working on a separate aztec3 branch of bberg. Only on Thursday evening did we finally merge that branch with master. All the while, master was being enhanced significantly by the crypto team. I'm not sure what exactly has increased the memory footprint, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants