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

Add RSA support to sha2 compat #726

Merged
merged 6 commits into from
Jul 20, 2023
Merged

Add RSA support to sha2 compat #726

merged 6 commits into from
Jul 20, 2023

Conversation

flaub
Copy link
Member

@flaub flaub commented Jul 19, 2023

@dobbobalina2 Here's my attempt to get Pkcs1v15Sign to work with the sha2 accelerator.

@flaub flaub requested a review from nategraf July 19, 2023 07:12
@flaub flaub self-assigned this Jul 19, 2023
@github-actions
Copy link

Benchmark for Linux-cuda 9afd594

Click to hide benchmark
Test Base PR %
fib/100/execute 5.0±0.08ms 5.0±0.08ms 0.00%
fib/100/prove 1096.5±4.30ms 895.0±5.98ms -18.38%
fib/100/total 1101.6±2.98ms 904.0±9.31ms -17.94%
fib/1000/execute 5.5±0.07ms 5.4±0.06ms -1.82%
fib/1000/prove 1126.3±3.18ms 931.3±6.42ms -17.31%
fib/1000/total 1134.9±4.16ms 952.6±15.74ms -16.06%
fib/10000/execute 9.8±0.12ms 9.7±0.08ms -1.02%
fib/10000/prove 3.8±0.02s 3.4±0.00s -10.53%
fib/10000/total 3.8±0.03s 3.4±0.01s -10.53%

Benchmark for Linux-default 9afd594

Click to hide benchmark
Test Base PR %
fib/100/execute 4.8±0.18ms 4.8±0.18ms 0.00%
fib/100/prove 2.4±0.29s 2.4±0.32s 0.00%
fib/100/total 2.5±0.35s 2.3±0.28s -8.00%
fib/1000/execute 5.3±0.25ms 5.3±0.19ms 0.00%
fib/1000/prove 2.1±0.20s 2.1±0.19s 0.00%
fib/1000/total 2.2±0.27s 2.2±0.16s 0.00%
fib/10000/execute 9.4±0.52ms 9.2±0.33ms -2.13%
fib/10000/prove 7.2±0.38s 7.2±0.42s 0.00%
fib/10000/total 7.4±0.46s 7.2±0.36s -2.70%

Benchmark for macOS-default 9afd594

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.16ms 2.7±0.14ms -3.57%
fib/100/prove 3.7±0.02s 3.7±0.04s 0.00%
fib/100/total 3.7±0.06s 3.7±0.05s 0.00%
fib/1000/execute 2.9±0.17ms 2.9±0.12ms 0.00%
fib/1000/prove 3.7±0.06s 3.7±0.07s 0.00%
fib/1000/total 3.7±0.05s 3.7±0.07s 0.00%
fib/10000/execute 5.1±0.09ms 5.0±0.12ms -1.96%
fib/10000/prove 15.3±0.12s 15.3±0.14s 0.00%
fib/10000/total 15.3±0.09s 15.2±0.10s -0.65%

Benchmark for macOS-metal 9afd594

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.16ms 2.7±0.14ms 0.00%
fib/100/prove 849.6±5.16ms 848.6±5.20ms -0.12%
fib/100/total 875.4±6.51ms 873.8±8.57ms -0.18%
fib/1000/execute 2.9±0.17ms 2.9±0.08ms 0.00%
fib/1000/prove 870.4±7.37ms 869.7±4.06ms -0.08%
fib/1000/total 896.4±6.79ms 893.8±6.27ms -0.29%
fib/10000/execute 5.1±0.10ms 5.0±0.05ms -1.96%
fib/10000/prove 3.3±0.01s 3.3±0.02s 0.00%
fib/10000/total 3.3±0.01s 3.3±0.01s 0.00%

@nategraf
Copy link
Contributor

nategraf commented Jul 19, 2023

Looks like a good addition. In the general case, we can point people towards our RustCrypto fork for 100% compatibility. There was no test to run the RsaCompat so I added one in 2e48ada. (Although I suppose the failure was at build time anyway)

Update: with a moment of thought, I reverted that commit and added a comment instead. e71c957

@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

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

Benchmark for macOS-default 21531b5

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.11ms 2.7±0.16ms -3.57%
fib/100/prove 3.7±0.03s 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.09ms 2.9±0.04ms -3.33%
fib/1000/prove 3.7±0.05s 3.7±0.06s 0.00%
fib/1000/total 3.7±0.06s 3.7±0.06s 0.00%
fib/10000/execute 5.1±0.08ms 5.0±0.07ms -1.96%
fib/10000/prove 15.2±0.18s 15.2±0.12s 0.00%
fib/10000/total 15.3±0.08s 15.1±0.09s -1.31%

Benchmark for macOS-metal 21531b5

Click to hide benchmark
Test Base PR %
fib/100/execute 2.7±0.10ms 2.7±0.17ms 0.00%
fib/100/prove 851.0±3.03ms 849.5±6.49ms -0.18%
fib/100/total 883.9±7.81ms 876.9±9.44ms -0.79%
fib/1000/execute 2.9±0.07ms 2.9±0.04ms 0.00%
fib/1000/prove 875.0±3.19ms 869.6±5.10ms -0.62%
fib/1000/total 895.3±7.84ms 883.9±12.36ms -1.27%
fib/10000/execute 5.0±0.08ms 4.9±0.09ms -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%

@flaub
Copy link
Member Author

flaub commented Jul 19, 2023

Is there any advantage to using risc0_zkp::core::hash::sha::rust_crypto::Sha256 as opposed to the patched RustCrypto fork? One option is to just drop this entire rust_crypto.rs to make it less confusing for folks. Could we have risc0-zkvm rely on the patched fork also?

@flaub
Copy link
Member Author

flaub commented Jul 20, 2023

@nategraf I added a more complete test, based on a reference from Discord.

@kevinnassery There's a private key here, but it's only for testing purposes.

@flaub flaub enabled auto-merge (squash) July 20, 2023 00:35
@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 2286770

Click to hide benchmark
Test Base PR %
fib/100/execute 4.7±0.09ms 4.7±0.05ms 0.00%
fib/100/prove 1706.9±135.00ms 1683.2±6.49ms -1.39%
fib/100/total 1717.4±80.28ms 1708.2±58.70ms -0.54%
fib/1000/execute 5.1±0.21ms 5.1±0.14ms 0.00%
fib/1000/prove 1723.9±54.67ms 1688.7±6.00ms -2.04%
fib/1000/total 1727.4±52.63ms 1714.5±6.62ms -0.75%
fib/10000/execute 8.9±0.19ms 8.8±0.14ms -1.12%
fib/10000/prove 6.5±0.37s 6.4±0.16s -1.54%
fib/10000/total 6.5±0.17s 6.4±0.21s -1.54%

Benchmark for macOS-default 2286770

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.17ms 2.7±0.10ms -3.57%
fib/100/prove 3.7±0.06s 3.7±0.06s 0.00%
fib/100/total 3.7±0.05s 3.7±0.05s 0.00%
fib/1000/execute 3.0±0.10ms 3.0±0.11ms 0.00%
fib/1000/prove 3.7±0.05s 3.7±0.06s 0.00%
fib/1000/total 3.7±0.04s 3.7±0.05s 0.00%
fib/10000/execute 5.1±0.05ms 5.0±0.05ms -1.96%
fib/10000/prove 15.2±0.08s 15.2±0.13s 0.00%
fib/10000/total 15.3±0.13s 15.3±0.11s 0.00%

Benchmark for macOS-metal 2286770

Click to hide benchmark
Test Base PR %
fib/100/execute 2.8±0.09ms 2.8±0.19ms 0.00%
fib/100/prove 854.8±5.96ms 850.0±4.90ms -0.56%
fib/100/total 877.3±7.51ms 871.1±7.15ms -0.71%
fib/1000/execute 3.0±0.14ms 2.9±0.13ms -3.33%
fib/1000/prove 876.2±5.88ms 871.0±3.14ms -0.59%
fib/1000/total 895.0±6.75ms 894.6±7.13ms -0.04%
fib/10000/execute 5.1±0.09ms 5.1±0.10ms 0.00%
fib/10000/prove 3.3±0.01s 3.3±0.01s 0.00%
fib/10000/total 3.3±0.00s 3.3±0.01s 0.00%

@flaub flaub merged commit e123a64 into main Jul 20, 2023
12 checks passed
@flaub flaub deleted the flaub/rsa branch July 20, 2023 01:58
capossele pushed a commit that referenced this pull request Aug 7, 2023
---------

Co-authored-by: Victor Graf <victor@risczero.com>
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

2 participants