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

fix: allow proving in wasi #250

Closed
wants to merge 6 commits into from

Conversation

nick1udwig
Copy link

In attempting to get the sp1 prover working in wasm32-wasi, we ran into an error originating from https://github.com/rust-lang/rust/blob/master/library/std/src/sys/pal/wasi/os.rs#L268-L270

This fix avoids using tempfile for wasm32, which was traced as the source of the problem.

@nick1udwig nick1udwig changed the title allow proving in wasi fix: allow proving in wasi Feb 15, 2024
@nick1udwig
Copy link
Author

We've run into memory issues with this solution when applying it to non-toy problems. We're working on an alternative approach using https://docs.rs/tempfile/latest/tempfile/fn.tempfile_in.html and an env var to define the temporary directory, since that will work in WASI (tempfile_in() doesn't use the offending temp_dir(): Stebalien/tempfile#138). We'll have a new PR (or a commit here?) with that approach soon.

@nick1udwig nick1udwig marked this pull request as draft February 16, 2024 23:33
@jtguibas
Copy link
Contributor

Hey @hosted-fornet, just wanted to say that it's super awesome you're trying to get this working. There's another team right now trying to get SP1 working on mobile, so this could be quite interesting for them.

@nick1udwig
Copy link
Author

Nice, that sounds like an interesting application: we're building a p2p node that uses wasm32-wasi components as userspace apps. SP1 would be a great unlock for us.

Hopefully we can get our fix out soon so we can all have something more concrete to discuss.

@nick1udwig
Copy link
Author

Ok, so @tadad did some good work tracking down the issue. It turns out the issue is not with the tempfiles; it is a separate issue.

The problem is that WASM modules are limited to 4GB of memory since they are 32bit!

This PR allows at least the smaller cases (fibonacci, e.g.) to work inside WASM modules. However, for more "real world" usecases, WASM is probably not ready for proving without the memory64 proposal. Verification works great, though.

More specifically, adapting the ed25519 example to our system doesn't work because we hit the 4GB memory limit here.

@nick1udwig nick1udwig marked this pull request as ready for review February 20, 2024 01:05
@nick1udwig
Copy link
Author

Just to follow up, it takes something like 20GB of memory, running natively on my machine, to prove the ed25519 example. Manipulating the shard size/threshold has no effect on this memory peak.

@jtguibas
Copy link
Contributor

jtguibas commented Feb 23, 2024

Hey @nick1udwig, I am about to merge in a PR which should make memory performance much better. It could worth retrying this after a new merge from main.

@nick1udwig
Copy link
Author

nick1udwig commented Feb 23, 2024

@jtguibas Thanks for the heads up; will try later today.

@nick1udwig
Copy link
Author

I still see ~20GB memory usage at peak when running the ed25519 example natively (and a capacity overflow in wasm, of course). Should I have seen less memory usage?

The wasm component runs out of memory in the iterator here:

chips.iter().for_each(|chip| {
let mut output = ExecutionRecord::default();
output.index = record.index;
chip.generate_dependencies(&record, &mut output);
record.append(&mut output);
});

@jtguibas
Copy link
Contributor

Hi @nick1udwig, thanks for running it. Yeah hm, lowering the memory usage there is a bit hard to avoid in the current design of the system. I'm going to look into lowering the memory usage even more.

@nick1udwig
Copy link
Author

Gotcha. @dr-frmr has told me that the ed25519 example is unrealistic for production because it'd be a precompile, so maybe I should just be using a different example? I do not understand this technology at more than a "user" level.

@dr-frmr
Copy link
Contributor

dr-frmr commented Mar 8, 2024

Hey @jtguibas, I updated this branch and kept the minimum change to allow the prover to run in Wasm. This is the code we've been using in our Wasm apps, and it seems to work. The memory limits are a separate problem of course, but this allows us to handle small-medium proofs without the tempfile::tempfile() call causing compilation issues.

@jtguibas
Copy link
Contributor

Hi @dr-frmr, in the latest version of the prover, we no longer use tempfile::tempfile(), so hopefully it should work more out of the box now!

@dr-frmr
Copy link
Contributor

dr-frmr commented Mar 11, 2024

Oh, well that's great to hear -- can go ahead and close this then.

@nick1udwig
Copy link
Author

Nice

@nick1udwig nick1udwig closed this Mar 11, 2024
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.

3 participants