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: split binfmt feature to a new "risc0-binfmt" crate for modularity #725

Merged
merged 8 commits into from
Jul 20, 2023

Conversation

SchmErik
Copy link
Contributor

This PR splits off the binfmt feature to a separate crate in order to avoid a circular dependency with a zkVM capability to be added in the near future. This capability requires risc0-zkvm to be dependent on risc0-build and the zkVM crate will contain guest code used in cases where the executor runs into a fault.

We need to drop risc0-zkvm as a dependency for risc0-build to avoid future
circular dependencies. In order to do so, we introduce a new crate,
risc0-binfmt that contains all code needed by risc0-build that used to be a
part of risc0-zkvm. This crate contains Program, MemoryImage, and SystemState
struct definitions along with several functions that operate on it.
This change removes files that implement MemoryImage, Program, SystemState, and
the binfmt feature of risc0-zkvm. Instead, it uses the new risc0-binfmt crate.
@github-actions
Copy link

Benchmark for Linux-cuda 14121a0

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.09ms 5.0±0.10ms 0.00%
fib/100/prove 1089.6±89.44ms 1018.4±30.39ms -6.53%
fib/100/total 1038.9±13.06ms 984.1±10.39ms -5.27%
fib/1000/execute 5.5±0.07ms 5.4±0.09ms -1.82%
fib/1000/prove 1066.6±8.08ms 1027.7±7.60ms -3.65%
fib/1000/total 1063.1±11.27ms 1027.8±10.79ms -3.32%
fib/10000/execute 9.8±0.09ms 9.8±0.10ms 0.00%
fib/10000/prove 4.4±0.01s 3.8±0.02s -13.64%
fib/10000/total 4.3±0.07s 3.8±0.01s -11.63%

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 14121a0

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.12ms 2.7±0.10ms -3.57%
fib/100/prove 3.7±0.08s 3.7±0.08s 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.08ms -3.33%
fib/1000/prove 3.7±0.05s 3.7±0.05s 0.00%
fib/1000/total 3.7±0.07s 3.7±0.06s 0.00%
fib/10000/execute 5.3±0.07ms 5.1±0.07ms -3.77%
fib/10000/prove 15.3±0.18s 15.3±0.17s 0.00%
fib/10000/total 15.3±0.12s 15.2±0.10s -0.65%

Benchmark for macOS-metal 14121a0

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.09ms 2.7±0.14ms 0.00%
fib/100/prove 851.7±5.00ms 849.7±4.80ms -0.23%
fib/100/total 875.3±5.62ms 874.4±5.11ms -0.10%
fib/1000/execute 2.9±0.07ms 2.9±0.07ms 0.00%
fib/1000/prove 871.9±4.19ms 870.5±4.30ms -0.16%
fib/1000/total 893.4±4.80ms 891.3±5.09ms -0.24%
fib/10000/execute 5.1±0.06ms 5.0±0.08ms -1.96%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.01s 0.00%

@github-actions
Copy link

Benchmark for Linux-cuda 02c1115

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.09ms 5.0±0.09ms 0.00%
fib/100/prove 1026.0±17.07ms 1022.9±9.57ms -0.30%
fib/100/total 1047.7±25.97ms 1009.1±12.85ms -3.68%
fib/1000/execute 5.5±0.09ms 5.4±0.08ms -1.82%
fib/1000/prove 1079.3±22.91ms 1060.1±22.16ms -1.78%
fib/1000/total 1103.3±58.72ms 1068.7±10.83ms -3.14%
fib/10000/execute 9.8±0.11ms 9.7±0.10ms -1.02%
fib/10000/prove 4.2±0.02s 4.0±0.02s -4.76%
fib/10000/total 4.2±0.01s 4.1±0.03s -2.38%

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 02c1115

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.07ms 2.6±0.12ms -3.70%
fib/100/prove 3.7±0.06s 3.7±0.05s 0.00%
fib/100/total 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/execute 3.0±0.11ms 2.9±0.09ms -3.33%
fib/1000/prove 3.7±0.05s 3.7±0.07s 0.00%
fib/1000/total 3.7±0.05s 3.7±0.06s 0.00%
fib/10000/execute 5.2±0.05ms 5.0±0.06ms -3.85%
fib/10000/prove 15.3±0.12s 15.2±0.13s -0.65%
fib/10000/total 15.3±0.15s 15.2±0.09s -0.65%

Benchmark for macOS-metal 02c1115

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.13ms 2.7±0.09ms 0.00%
fib/100/prove 850.8±5.07ms 847.4±5.34ms -0.40%
fib/100/total 876.7±7.96ms 870.3±6.51ms -0.73%
fib/1000/execute 2.9±0.09ms 2.9±0.08ms 0.00%
fib/1000/prove 870.7±4.57ms 867.0±4.14ms -0.42%
fib/1000/total 896.3±7.67ms 891.2±5.21ms -0.57%
fib/10000/execute 5.0±0.05ms 4.9±0.07ms -2.00%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.01s 0.00%

@SchmErik SchmErik requested a review from flaub July 19, 2023 03:51
Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 18 to 19
tracing = { version = "0.1", default-features = false, features = ["attributes"] }
tracing-subscriber = { version = "0.3", features = ["env-filter"] }
Copy link
Member

Choose a reason for hiding this comment

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

Is this related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, So... oddly enough. This PR made this example fail on cargo test with the following error and adding this helped the test pass. I'm not sure what exactly changed but it fails to compile without these additional deps now.

% cargo test
   Compiling risc0-binfmt v0.16.1 (/Users/erik/risc0/risc0/binfmt)
   Compiling risc0-build v0.16.1 (/Users/erik/risc0/risc0/build)
   Compiling risc0-zkvm v0.16.1 (/Users/erik/risc0/risc0/zkvm)
   Compiling digital-signature-methods v0.1.0 (/Users/erik/risc0/examples/digital-signature/methods)
   Compiling digital-signature-core v0.1.0 (/Users/erik/risc0/examples/digital-signature/core)
sign: Starting build for riscv32im-risc0-zkvm-elf digital-signature-methods(build), risc0-zkvm                                                                           
sign:     Updating crates.io index
sign:     Finished release [optimized] target(s) in 1.37s-signature-methods(build)                                                                                       
   Compiling digital-signature v0.1.0 (/Users/erik/risc0/examples/digital-signature)
error[E0433]: failed to resolve: could not find `tracing_subscriber` in the list of imported crates
  --> digital-signature/src/lib.rs:72:5
   |
72 |     #[test]
   |     ^^^^^^^ could not find `tracing_subscriber` in the list of imported crates
   |
   = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0433]: failed to resolve: could not find `tracing` in the list of imported crates
  --> digital-signature/src/lib.rs:72:5
   |
72 |     #[test]
   |     ^^^^^^^ could not find `tracing` in the list of imported crates
   |
   = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0433`.
error: could not compile `digital-signature` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

risc0/binfmt/src/elf.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

Benchmark for Linux-cuda 4fb7f17

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.08ms 5.0±0.10ms -1.96%
fib/100/prove 1034.5±1.39ms 1019.8±14.34ms -1.42%
fib/100/total 1039.4±0.69ms 1010.3±12.29ms -2.80%
fib/1000/execute 5.5±0.07ms 5.5±0.10ms 0.00%
fib/1000/prove 1059.0±1.87ms 1047.1±12.95ms -1.12%
fib/1000/total 1064.9±1.57ms 1052.6±15.06ms -1.16%
fib/10000/execute 9.8±0.11ms 9.8±0.13ms 0.00%
fib/10000/prove 4.0±0.01s 3.3±0.00s -17.50%
fib/10000/total 4.1±0.02s 3.3±0.00s -19.51%

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 4fb7f17

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.13ms 2.7±0.09ms 0.00%
fib/100/prove 3.7±0.04s 3.7±0.05s 0.00%
fib/100/total 3.7±0.04s 3.7±0.06s 0.00%
fib/1000/execute 3.0±0.11ms 2.9±0.08ms -3.33%
fib/1000/prove 3.7±0.04s 3.7±0.05s 0.00%
fib/1000/total 3.7±0.03s 3.7±0.06s 0.00%
fib/10000/execute 5.2±0.07ms 5.1±0.10ms -1.92%
fib/10000/prove 15.3±0.17s 15.2±0.12s -0.65%
fib/10000/total 15.3±0.17s 15.3±0.13s 0.00%

Benchmark for macOS-metal 4fb7f17

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.12ms 2.7±0.17ms 0.00%
fib/100/prove 851.2±3.57ms 849.7±5.73ms -0.18%
fib/100/total 876.7±10.96ms 875.4±5.32ms -0.15%
fib/1000/execute 3.0±0.04ms 2.9±0.08ms -3.33%
fib/1000/prove 871.2±4.58ms 868.8±5.58ms -0.28%
fib/1000/total 899.7±8.66ms 897.9±9.39ms -0.20%
fib/10000/execute 5.0±0.05ms 4.9±0.08ms -2.00%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.02s 0.00%

@SchmErik SchmErik requested a review from flaub July 19, 2023 06:18
@SchmErik SchmErik enabled auto-merge (squash) July 20, 2023 05:15
@github-actions
Copy link

Benchmark for Linux-cuda bf89333

Click to hide benchmark
Test Base PR %
fib/100/execute 5.1±0.11ms 5.0±0.09ms -1.96%
fib/100/prove 1222.3±2.61ms 913.1±3.75ms -25.30%
fib/100/total 1230.2±2.85ms 918.4±3.74ms -25.35%
fib/1000/execute 5.5±0.08ms 5.5±0.07ms 0.00%
fib/1000/prove 1242.2±1.73ms 953.2±4.76ms -23.27%
fib/1000/total 1252.6±3.32ms 957.8±3.12ms -23.54%
fib/10000/execute 9.9±0.07ms 9.8±0.11ms -1.01%
fib/10000/prove 3.8±0.01s 3.5±0.03s -7.89%
fib/10000/total 3.8±0.01s 3.5±0.01s -7.89%

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

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

Benchmark for macOS-metal bf89333

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.14ms 2.7±0.17ms 0.00%
fib/100/prove 847.7±3.91ms 845.3±4.63ms -0.28%
fib/100/total 882.1±7.27ms 874.4±6.20ms -0.87%
fib/1000/execute 2.9±0.07ms 2.9±0.13ms 0.00%
fib/1000/prove 868.6±6.17ms 864.5±3.84ms -0.47%
fib/1000/total 896.8±7.60ms 895.5±4.10ms -0.14%
fib/10000/execute 5.0±0.06ms 5.0±0.06ms 0.00%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.01s 0.00%

@SchmErik SchmErik merged commit b783786 into main Jul 20, 2023
12 checks passed
@SchmErik SchmErik deleted the erik/risc0-binfmt-v2 branch July 20, 2023 06:15
capossele pushed a commit that referenced this pull request Aug 7, 2023
…ty (#725)

* Add a new package risc0-binfmt

We need to drop risc0-zkvm as a dependency for risc0-build to avoid future
circular dependencies. In order to do so, we introduce a new crate,
risc0-binfmt that contains all code needed by risc0-build that used to be a
part of risc0-zkvm. This crate contains Program, MemoryImage, and SystemState
struct definitions along with several functions that operate on it.

* drop binfmt feature and use risc0-binfmt instead

This change removes files that implement MemoryImage, Program, SystemState, and
the binfmt feature of risc0-zkvm. Instead, it uses the new risc0-binfmt crate.
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use alloc::collections::BTreeMap;

Choose a reason for hiding this comment

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

This makes risc0_zkvm and risc0_binfmt to be no_std incompatible

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