-
Notifications
You must be signed in to change notification settings - Fork 162
Enable zkProgram.compile() with cache in browsers #2404
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
base: main
Are you sure you want to change the base?
Conversation
- Register caml_fp_srs_get_lagrange_basis_ptr and _read_from_ptr functions - Register caml_fq_srs_get_lagrange_basis_ptr and _read_from_ptr functions - These enable zkProgram.compile() with cache writing in web environments - Follows existing pointer pattern used by lagrange_commitments_whole_domain - Phase 2 of 4: Worker spec registration complete
- Implement platform detection (Node.js vs Web) in getLagrangeBasis - Web environments use new pointer-based functions (_ptr + _read_from_ptr) - Node.js continues using original direct function call - Update TODO comment to reflect fixed functionality - Phase 3 of 4: TypeScript integration complete
CHANGELOG.md
Outdated
|
||
- Internal o1js and protocol constants, hashes and prefixes are now exported via | ||
the `Core´ namespace. https://github.com/o1-labs/o1js/pull/2421 | ||
- Added `caml_fp_srs_get_lagrange_basis_ptr` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an implementation detail, I don't think it belongs in a changelog
CHANGELOG.md
Outdated
https://github.com/o1-labs/o1js/pull/2388 | ||
|
||
- Fixed issue where `zkProgram.compile()` was not able to use a browser cache. | ||
Previously, the `get_lagrange_basis` functions were disabled on the web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, this is an implementation detail. Maybe replace get_lagrange_basis
with a generic reading from cache..
@@ -1,1621 +1,4 @@ | |||
// @generated this file is auto-generated - don't edit it directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these files shouldn't be part of this PR
args: [wasm.WasmFqSrs, undefined /* number */], | ||
res: undefined /* number, ptr */, | ||
}, | ||
// New get_lagrange_basis pointer functions for web compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this workaround, we should clean up caml_fp_srs_get_lagrange_basis
a bit (see line 111 in this file)
src/bindings/crypto/bindings/srs.ts
Outdated
if (cache.canWrite) { | ||
// TODO: this code path will throw on the web since `caml_${f}_srs_get_lagrange_basis` is not properly implemented | ||
// using a writable cache in the browser seems to be fairly uncommon though, so it's at least an 80/20 solution | ||
// getLagrangeBasis now works on web via environment-aware pointer functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just removing the initial TODO comment here would have been enough to clarify this is now working on the web
src/bindings/crypto/bindings/srs.ts
Outdated
|
||
if (isWeb) { | ||
// Web: Use new pointer-based functions for worker compatibility | ||
const ptr = wasm[`caml_${f}_srs_get_lagrange_basis_ptr`](srs, n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's cleaner to keep using the patter above for wasm functions
let getLagrangeBasisPtr = ..
continuing the discussion on slack |
…tion working on web via environment-aware pointer functions
These files are auto-generated and only had formatting changes that shouldn't be included in the PR.
… readability and clarity
Summary
Enables
ZkProgram.compile({ cache })
to work in browsers. Previously, using a cache in web environments with aZkProgram
was disabled because the underlying lagrange basis functions were not compatible with the web worker API and would cause compilation to hang indefinitely.Background
The
ZkProgram.compile({ cache })
functionality was broken in browsers because:WasmVector
types incompatible with web worker transferworker-spec.js
to prevent hangingChanges
worker-spec.js
srs.ts
with environment-awaregetLagrangeBasis
functionRelated PRs
This is part 3 of 3 for the complete fix:
Testing
Manually tested in browser environment: