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

Adjust image_id to be the hash of (merkle_root, pc) #566

Merged
merged 3 commits into from
May 18, 2023
Merged

Conversation

flaub
Copy link
Member

@flaub flaub commented May 17, 2023

No description provided.

@flaub flaub requested review from shkoo, jbruestle and mothran May 17, 2023 20:52
@flaub flaub self-assigned this May 17, 2023
@flaub
Copy link
Member Author

flaub commented May 17, 2023

The check for exit_code is failing and I plan to fix this soon. But I wanted to get eyes on this in the meantime.

@flaub flaub requested a review from spaugh May 17, 2023 20:53
@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 60b0060

Click to hide benchmark
Test Base PR %
fib/100/execute 55.6±0.33ms 55.6±0.22ms 0.00%
fib/100/prove 5.1±0.03s 5.1±0.03s 0.00%
fib/100/total 5.1±0.03s 5.1±0.03s 0.00%
fib/1000/execute 78.5±2.42ms 76.8±0.38ms -2.17%
fib/1000/prove 5.1±0.03s 5.1±0.03s 0.00%
fib/1000/total 5.2±0.03s 5.2±0.02s 0.00%
fib/10000/execute 288.9±1.29ms 284.6±1.31ms -1.49%
fib/10000/prove 21.1±0.06s 21.0±0.13s -0.47%
fib/10000/total 21.4±0.17s 21.4±0.12s 0.00%

Benchmark for macOS-default 60b0060

Click to hide benchmark
Test Base PR %
fib/100/execute 28.4±0.16ms 28.3±0.09ms -0.35%
fib/100/prove 3.7±0.05s 3.7±0.05s 0.00%
fib/100/total 3.7±0.04s 3.7±0.06s 0.00%
fib/1000/execute 38.0±0.14ms 37.4±0.07ms -1.58%
fib/1000/prove 3.7±0.07s 3.7±0.04s 0.00%
fib/1000/total 3.8±0.04s 3.7±0.05s -2.63%
fib/10000/execute 132.2±0.25ms 128.0±0.12ms -3.18%
fib/10000/prove 15.2±0.20s 15.2±0.12s 0.00%
fib/10000/total 15.5±0.12s 15.4±0.14s -0.65%

Benchmark for macOS-metal

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

@flaub flaub marked this pull request as ready for review May 18, 2023 02:36
@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 5ce76f0

Click to hide benchmark
Test Base PR %
fib/100/execute 56.3±0.63ms 56.0±1.09ms -0.53%
fib/100/prove 5.1±0.04s 5.1±0.03s 0.00%
fib/100/total 5.1±0.02s 5.1±0.02s 0.00%
fib/1000/execute 78.2±0.46ms 77.7±0.63ms -0.64%
fib/1000/prove 5.1±0.03s 5.1±0.03s 0.00%
fib/1000/total 5.2±0.02s 5.2±0.01s 0.00%
fib/10000/execute 292.7±2.50ms 289.5±1.11ms -1.09%
fib/10000/prove 21.2±0.14s 21.1±0.15s -0.47%
fib/10000/total 21.4±0.14s 21.3±0.13s -0.47%

Benchmark for macOS-default 5ce76f0

Click to hide benchmark
Test Base PR %
fib/100/execute 29.0±0.03ms 28.2±0.03ms -2.76%
fib/100/prove 3.7±0.06s 3.7±0.08s 0.00%
fib/100/total 3.7±0.05s 3.7±0.08s 0.00%
fib/1000/execute 38.5±0.09ms 37.7±0.13ms -2.08%
fib/1000/prove 3.7±0.04s 3.7±0.06s 0.00%
fib/1000/total 3.8±0.04s 3.7±0.05s -2.63%
fib/10000/execute 133.4±0.29ms 132.0±0.17ms -1.05%
fib/10000/prove 15.3±0.13s 15.2±0.11s -0.65%
fib/10000/total 15.5±0.14s 15.4±0.11s -0.65%

Benchmark for macOS-metal 5ce76f0

Click to hide benchmark
Test Base PR %
fib/100/execute 28.3±0.14ms 28.2±0.02ms -0.35%
fib/100/prove 851.7±5.38ms 848.9±4.23ms -0.33%
fib/100/total 910.0±5.07ms 901.7±9.78ms -0.91%
fib/1000/execute 37.4±0.09ms 37.3±0.09ms -0.27%
fib/1000/prove 868.8±5.50ms 866.1±5.04ms -0.31%
fib/1000/total 934.1±5.34ms 932.7±5.80ms -0.15%
fib/10000/execute 129.4±0.14ms 128.4±0.23ms -0.77%
fib/10000/prove 3.4±0.01s 3.3±0.01s -2.94%
fib/10000/total 3.5±0.03s 3.5±0.01s 0.00%

@@ -316,7 +318,7 @@ impl<'a> Executor<'a> {
}

fn advance(&mut self, opcode: OpCode, op_result: OpCodeResult) -> Option<ExitCode> {
log::debug!(
log::trace!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I just hit this and thought it should be trace


/// Compute and return the ImageID of the given `(merkle_root, pc)` pair.
pub fn compute_image_id(merkle_root: &Digest, pc: u32) -> Digest {
use risc0_zkp::core::{digest::DIGEST_WORDS, hash::sha::Sha256};
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally think we should avoid in-function imports because it can be hard to track down import scoping when introducing new feature flags. But its only a small nit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but in this particular case, I couldn't figure out how to get it to work because the rest of the file needs Sha256 but is a different trait with the same name.

@flaub flaub merged commit 235e75b into main May 18, 2023
12 checks passed
@flaub flaub deleted the flaub/image_id branch May 18, 2023 16:52
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

4 participants