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

zkVM: add remote proving by using the bonsai sdk #677

Merged
merged 21 commits into from
Jul 10, 2023
Merged

Conversation

SchmErik
Copy link
Contributor

@SchmErik SchmErik commented Jul 7, 2023

This change enables generating proofs remotely by using bonsai using the zkVM API. In order to execute, add the following environment variables before running the host code: BONSAI_API_KEY, BONSAI_API_URL. A new function has been added to generate an executor if these environment variables are set.

This change enables generating proofs remotely by using bonsai using the zkVM API.
In order to execute, add the following environment variables before running the
host code: BONSAI_API_KEY, BONSAI_API_URL. A new function has been added to
generate an executor if these environment variables are set.
@SchmErik SchmErik requested review from flaub and mothran July 7, 2023 05:31
@SchmErik SchmErik marked this pull request as draft July 7, 2023 05:31
@SchmErik
Copy link
Contributor Author

SchmErik commented Jul 7, 2023

This is my initial idea for integrating the bonsai alpha sdk. I haven't changed the examples and I haven't run any workloads but I did change r0vm to see what this looks like. Before I go change all of the examples, I thought this is a good stopping place for initial feedback

@SchmErik SchmErik changed the title zkVM: add remote proving through the bonsai sdk zkVM: add remote proving by using the bonsai sdk Jul 7, 2023
risc0/zkvm/src/exec/mod.rs Outdated Show resolved Hide resolved
}

fn prove_segment(&self, _ctx: &VerifierContext, _segment: &Segment) -> Result<SegmentReceipt> {
Err(anyhow!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider using https://doc.rust-lang.org/std/macro.unimplemented.html instead of returning a failable error? Im not sure either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah... I'm not sure about this. I saw all of the cases that use prove_segment. the unimplemented will panic which I know we're trying to reduce...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flaub do you have any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

I think in this case a panic is probably fine since it indicates a programmer mistake. One way to think about it is if this panics, will it bring down a decentralized node? I'd say in this case perhaps, but I'd argue that was a logical mistake by the programmer of the node software itself, and not related to any outside user input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your clarification!

risc0/zkvm/src/prove/mod.rs Outdated Show resolved Hide resolved
// the proof request succeeded.
let res = session.status(&client)?;
if res.status == "RUNNING" {
std::thread::sleep(Duration::from_secs(15));
Copy link
Contributor

Choose a reason for hiding this comment

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

You might wanna make this a configurable timestep if possible. I might also recommend dropping it down to 5 seconds.

Copy link
Contributor

@mothran mothran left a comment

Choose a reason for hiding this comment

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

It might be helpful for review to use this new higher level API in the rust starter-template first before examples. Plus it would be a good one to convert to test / review you API changes. The SDK integration looks clear to me.

@SchmErik
Copy link
Contributor Author

SchmErik commented Jul 7, 2023

I'm thinking about renaming the Exec trait as Executor and change the existing Executor to something else so that it's easier to understand from the zkVM API user's perspective...

risc0/zkvm/Cargo.toml Outdated Show resolved Hide resolved
@flaub
Copy link
Member

flaub commented Jul 7, 2023

I'm thinking about renaming the Exec trait as Executor and change the existing Executor to something else so that it's easier to understand from the zkVM API user's perspective...

How about we rename Executor to LocalExecutor so its in parity with RemoteExecutor and that frees up Executor to be the trait.

@flaub
Copy link
Member

flaub commented Jul 7, 2023

Should we drop SdkErr::ImageIdExists? I would expect upload_img to be idempotent, so the fact that it already exists should just be transparent and never considered an error.

@flaub
Copy link
Member

flaub commented Jul 7, 2023

I think we should split LocalExecutor and RemoteExecutor into their own modules, since they aren't very related, and leave the Executor trait in the exec module.

risc0/zkvm/src/prove/mod.rs Outdated Show resolved Hide resolved
risc0/zkvm/src/exec/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@flaub flaub left a comment

Choose a reason for hiding this comment

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

I like this

@SchmErik SchmErik marked this pull request as ready for review July 10, 2023 19:21
@SchmErik SchmErik enabled auto-merge (squash) July 10, 2023 19:24
@github-actions
Copy link

Benchmark for Linux-cuda ab105e0

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.10ms 5.1±0.11ms 0.00%
fib/100/prove 1276.9±11.08ms 1044.2±18.91ms -18.22%
fib/100/total 1185.2±22.21ms 1054.5±15.83ms -11.03%
fib/1000/execute 5.6±0.10ms 5.5±0.11ms -1.79%
fib/1000/prove 1200.1±64.49ms 1079.6±14.14ms -10.04%
fib/1000/total 1205.9±11.02ms 1087.1±15.03ms -9.85%
fib/10000/execute 9.9±0.10ms 9.8±0.15ms -1.01%
fib/10000/prove 4.8±0.05s 4.2±0.02s -12.50%
fib/10000/total 4.6±0.06s 4.2±0.02s -8.70%

Benchmark for Linux-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-default ab105e0

Click to hide benchmark
Test Base PR %
fib/100/execute 2.9±0.22ms 2.9±0.10ms 0.00%
fib/100/prove 3.7±0.04s 3.6±0.06s -2.70%
fib/100/total 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/execute 3.0±0.06ms 3.0±0.09ms 0.00%
fib/1000/prove 3.7±0.07s 3.7±0.07s 0.00%
fib/1000/total 3.7±0.02s 3.7±0.05s 0.00%
fib/10000/execute 5.3±0.09ms 5.3±0.10ms 0.00%
fib/10000/prove 15.3±0.20s 15.1±0.09s -1.31%
fib/10000/total 15.3±0.18s 15.2±0.12s -0.65%

Benchmark for macOS-metal ab105e0

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.06ms 2.7±0.13ms -3.57%
fib/100/prove 857.2±2.84ms 848.3±2.90ms -1.04%
fib/100/total 881.5±6.97ms 873.8±6.07ms -0.87%
fib/1000/execute 2.9±0.10ms 2.8±0.06ms -3.45%
fib/1000/prove 872.6±8.98ms 865.1±2.27ms -0.86%
fib/1000/total 899.8±7.63ms 893.9±5.93ms -0.66%
fib/10000/execute 5.1±0.08ms 5.0±0.10ms -1.96%
fib/10000/prove 3.4±0.01s 3.3±0.01s -2.94%
fib/10000/total 3.4±0.01s 3.3±0.01s -2.94%

@github-actions
Copy link

Benchmark for Linux-cuda

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for Linux-default e28b5de

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.73ms 4.8±0.13ms -5.88%
fib/100/prove 2.7±0.52s 2.2±0.48s -18.52%
fib/100/total 2.6±0.45s 1683.7±10.43ms -35.24%
fib/1000/execute 5.3±0.19ms 5.2±0.11ms -1.89%
fib/1000/prove 2.5±0.50s 1699.2±4.35ms -32.03%
fib/1000/total 2.7±0.39s 1716.6±25.55ms -36.42%
fib/10000/execute 9.1±0.06ms 8.9±0.20ms -2.20%
fib/10000/prove 7.3±0.45s 6.4±0.19s -12.33%
fib/10000/total 7.6±0.63s 6.4±0.20s -15.79%

Benchmark for macOS-default e28b5de

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.09ms 2.7±0.10ms -3.57%
fib/100/prove 3.7±0.03s 3.7±0.06s 0.00%
fib/100/total 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/execute 3.0±0.07ms 2.9±0.15ms -3.33%
fib/1000/prove 3.7±0.03s 3.7±0.04s 0.00%
fib/1000/total 3.8±0.08s 3.7±0.06s -2.63%
fib/10000/execute 5.3±0.15ms 5.2±0.05ms -1.89%
fib/10000/prove 15.2±0.11s 15.2±0.10s 0.00%
fib/10000/total 15.3±0.15s 15.3±0.13s 0.00%

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

@SchmErik SchmErik merged commit 9487793 into main Jul 10, 2023
12 checks passed
@SchmErik SchmErik deleted the erik/bonsai-dogfood-v2 branch July 10, 2023 21:19
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.

None yet

3 participants