Adding zkvm-pico support on sha2/sha256 #3
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Author
|
@ArtiomTr this PR is good to review. |
jimmychu0807
commented
Oct 16, 2025
sha2/Cargo.toml
Outdated
| force-soft = [] # Force software implementation | ||
| force-soft-compact = [] # Force compact software implementation | ||
| asm-aarch64 = ["asm"] # DEPRECATED: use `asm` instead | ||
| zkvm-pico = [] # For Brevis pico zkvm compilation |
ArtiomTr
requested changes
Nov 3, 2025
Collaborator
ArtiomTr
left a comment
ArtiomTr
left a comment
There was a problem hiding this comment.
looks good, just one more check needed
Comment on lines
+29
to
+36
| } else if #[cfg(all(target_os = "zkvm", target_vendor = "risc0", target_arch = "riscv32", feature = "zkvm-pico"))] { | ||
| // Brevis Pico target string is also `riscv32im-risc0-zkvm-elf`, so we use an additional feature | ||
| // to distinguish pico. | ||
| // ref: https://github.com/brevis-network/pico/blob/main/sdk/cli/src/build/build.rs#L82 | ||
| mod pico; | ||
| use pico::compress; | ||
| } else if #[cfg(all(target_os = "zkvm", target_vendor = "risc0", target_arch = "riscv32", feature = "zkvm-risc0"))] { | ||
| // zkvm-r0vm |
Collaborator
There was a problem hiding this comment.
looks like this would successfully compile, even if target_vendor = "risc0", but both zkvm-pico and zkvm-risc0 features are disabled? Can you please add:
#[cfg(all(target_os = "zkvm", target_vendor = "risc0", not(any(feature = "zkvm-pico", feature = "zkvm-risc0"))))]
compile_error!("please select at least one feature from [`zkvm-pico`, `zkvm-risc0`]");or something similar, to prevent accidentally keeping precompile off
ArtiomTr
merged commit 0d3a387
into
grandinetech:RustCrypto-hashes/sha2-v0.10.9
5 of 22 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Copied from discord: Investigated the issue if I can use target_os, target_arch to distinguish brevis pico from other zkvms. I believe the ans is not.
Because pico themselves use riscv32im-risc0-zkvm-elf as the target string.
As a result,
target_arch == riscv32,target_vendor == risc0,target_os == zkvm, same as risc0 vm 😓. Here is an example of their check in pico sha256 lib.I raised this issue to them this morning, but there will be a lot of changes, across multiple repos, if they decide to address this issue. So I am not hopeful they will address this anytime soon and we probably have to resolve to add our own (feature) flag.
Also run
cargo fmton sha2 crate.