Skip to content

Implemented zkvm-pico host and guest programs#386

Merged
povi merged 1 commit into
grandinetech:developfrom
jimmychu0807:feature/zkvm-pico
Nov 27, 2025
Merged

Implemented zkvm-pico host and guest programs#386
povi merged 1 commit into
grandinetech:developfrom
jimmychu0807:feature/zkvm-pico

Conversation

@jimmychu0807

@jimmychu0807 jimmychu0807 commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

In addition to the zkvm-pico host and guest code, there are a few notable issues worth to be reviewed and discussed in this PR, summarized below.

  • zkvm/host/src/backend.rs has been refactored to backend dir, and each zkvm host program is named as backend/.rs. Refer to backend/mod.rs.
  • Added zkvm/host/src/pico.rs as pico host program.
  • Added zkvm/host/pico_helper so compiling the zkvm-host pico program (cargo build -p zkvm_host --features pico) will also compile the zkvm-guest pico program.
  • In bls-zkcrypto package code, added a new zkvm-pico feature to support running pico library that take a different parameter type on hash_to_curve() function in secret_key.rs and signature.rs.
  • The rust-toolchain.toml has changed to using nightly-2025-08-04 nightly version. This is necessary to compile pico-sdk for the host and guest program. Need to discuss further.
  • The root Cargo.toml, I have commented out universal-precompile for bls12_381 and sha2. Once this PR looks good. I can make another PR of the corresponding bls12_381 and sha2 lib into grandinetech/universal-precompile.
  • Added a fix on ssz/src/shared.rs on the actual > maximum check on read_list().
    cc @povi author of the ssz-reading optimization code - please check if the fix is actually proper 🙏, @Dyslex7c
  • Added a .editorconfig file for better editor display support.

This PR updates rust-toolchain from stable to nightly version. If this is too drastic to the main branch, I can comment out the nightly version line and add a comment that compiling zkvm-pico requires switching from stable rust toolchain to nightly.

@ArtiomTr, @povi please have a look on this PR when you have time. Thanks.

@CLAassistant

CLAassistant commented Sep 25, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread Cargo.toml Outdated
Comment on lines +308 to +310
# bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }
# For Brevis Pico
bls12_381 = { git = "https://github.com/jimmychu0807/brevis-bls12_381.git", branch = "jc/dev" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change breaks all other zkvms, so need to be sorted out before merging into develop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is what I'm not sure of. Brevis Pico has their own tweak of bls12_381 and sha2. How can I make these crates compatible with other zkvms? (Actually each zkvm seems to have their own tweak on these two libs)

Comment thread Cargo.toml Outdated
sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0', features = ['compress'] }
# sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0', features = ['compress'] }
# For Brevis Pico
sha2 = { git = 'https://github.com/brevis-network/hashes.git', branch = 'sha2-0.10.8-v1.0.0', features = ['compress'] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as bls crate

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pointed to
git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'RustCrypto-hashes/sha2-v0.10.9' now.
Once grandinetech/universal-precompiles#3 is merged, I will point back to grandine/universal-precompiles.git.

Comment thread bls/bls-zkcrypto/Cargo.toml Outdated
rand_chacha = { workspace = true }

[features]
zkvm-pico = []

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Better not to have special features for zkvms, but to detect them with target, e.g. for risc0:

#[cfg(target_os = "zkvm", target_vendor = "risc0")]

@jimmychu0807 jimmychu0807 Sep 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought so at the beginning. For pico, the target value they use is riscv32im-risc0-zkvm-elf, which is very similar to r0vm.

Comment thread bls/bls-zkcrypto/src/secret_key.rs Outdated
Comment on lines +74 to +79
#[cfg(feature = "zkvm-pico")]
let h = <G2Projective as HashToCurve<ExpandMsgXmd<Sha256>>>::hash_to_curve(
message.as_ref(),
DOMAIN_SEPARATION_TAG,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe that it is old zkcrypto API, that was in last published version 0.8.0, but grandine is using newer one, from master branch. So you have to update precompiles to latest version (like with risc0, sp1 and ziren in grandinetech/universal-precompiles repo).

Also, since bls12_381 is currently set into Cargo.toml, does main grandine binary compile now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is still not fixed, there should be single api for hash_to_curve, not dependent on the backend

Comment thread rust-toolchain.toml Outdated
Comment on lines +2 to +4
# channel = 'stable-2025-06-26'
# Need nightly rust toolchain to build zkvm_host with `pico` feature
channel = 'nightly-2025-08-04'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I personally don't think this is a good idea, but @povi & @sauliusgrigaitis know better, maybe they're okay with that.

However, is it possible to avoid somehow? Maybe we can put pico host code into separate crate, outside of main workspace, and compile it separately, with separate toolchain?

@jimmychu0807 jimmychu0807 Sep 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about I keep the stable version line, and add a comment below for those who want to compile zkvm pico? Essentially:

channel = 'stable-2025-06-26'
# To buld zkvm_host with `pico` feature, comment the above and use the following nighly version.
# channel = 'nightly-2025-08-04'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That works too, but adds some tedious work to run tests, which can be automated. Also, one more idea just came into my mind: you can use cargo +nightly build to build zkvm host with nightly version. So you can add target to makefile, to avoid manually commenting lines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, by restoring back to stable version. Added lines in Makefile with cargo +nightly-<version> build to build zkvm-pico host and guest code.

Comment thread ssz/src/shared.rs Outdated

if actual > maximum {
// Note: ignore the check `actual > maximum` when maximum == 0. Is this proper?
if maximum > 0 && actual > maximum {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see this as very dangerous check (as it is essentially skipping check). Would be better to find exact place where List has max length of zero, and see what's happening there?

Still this place needs to be reviewed by someone who works with ssz more closely, maybe @povi can consult here?

@jimmychu0807 jimmychu0807 Sep 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@povi would like to add more context here. Without this fix (maximum > 0 and also below), even the simplest test consensus spec tests mainnet electra empty block transition will fail inside the guest code on the line:

BeaconState::<P>::from_ssz_at_phase(phase, &state_ssz)

The list maximum passed in is 0, but there are content inside.

After reading this comment, if you also think there is a better approach, let me know and I will revert this fix in this PR and let you send a proper fix to ssz crate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, don't remove the check. If the maximum is 0, that most likely means that BeaconState is from the different phase than it tries to decode from. Please re-check from which phase is BeaconState and what is the phase parameter that is being used in from_ssz_at_phase

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will restore this file to its previous state. When this PR is completed & merged, I will try to look into this ssz issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

restored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line is remaining here, so the zkvm test could pass. Let me know if you feel it is better left off. I will revert the fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reverted


const GUEST_PROGRAM: &str = "zkvm-guest-pico.elf";

pub fn build_program(path: &str) -> Result<()> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this helper lives in its own crate? Maybe you can move it to backend/pico?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. Can move this in backend/pico.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Moved the function to backend/pico/build_helper.rs.

Comment thread zkvm/host/src/backend/pico.rs Outdated

let proof_set = prover
.prove(stdin_builder)
.expect("Failed to generate proof");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is better to not panic here, but return error

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread zkvm/host/src/backend/pico.rs Outdated
Comment on lines +83 to +87
anyhow!(format!(
"Failed to load ELF file from {}: {}",
zkvm_guest_path.display(),
err
))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe this may be simplified with anyhow's context feature? It should keep original error, while providing new error message (like saying "Failed to load ELF file from {some path}", and then showing e.g. "File not found" error below)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@jimmychu0807

Copy link
Copy Markdown
Contributor Author

latest CI result can be seen here:
https://github.com/jimmychu0807/grandine/actions

Comment thread Cargo.toml Outdated
Comment on lines +307 to +308
# bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }
bls12_381 = { git = "https://github.com/jimmychu0807/grandine-universal-precompiles.git", branch = "zkcrypto/bls12_381" }

@jimmychu0807 jimmychu0807 Oct 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once grandinetech/universal-precompiles#4 is merged, modify to

Suggested change
# bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }
bls12_381 = { git = "https://github.com/jimmychu0807/grandine-universal-precompiles.git", branch = "zkcrypto/bls12_381" }
bls12_381 = { git = "https://github.com/grandine/universal-precompiles.git", branch = "zkcrypto/bls12_381" }

Comment thread Cargo.toml Outdated
Comment on lines +620 to +621
# bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }
bls12_381 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'zkcrypto/bls12_381' }

@jimmychu0807 jimmychu0807 Oct 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once grandinetech/universal-precompiles#4 is merged, modify to

Suggested change
# bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }
bls12_381 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'zkcrypto/bls12_381' }
bls12_381 = { git = "https://github.com/grandine/universal-precompiles.git", branch = "zkcrypto/bls12_381" }

Comment thread Cargo.toml Outdated
Comment on lines +443 to +444
# sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0', features = ['compress'] }
sha2 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'RustCrypto-hashes/sha2-v0.10.9', features = ['compress'] }

@jimmychu0807 jimmychu0807 Oct 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once grandinetech/universal-precompiles#3 is merged, modify to

Suggested change
# sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0', features = ['compress'] }
sha2 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'RustCrypto-hashes/sha2-v0.10.9', features = ['compress'] }
sha2 = { git = "https://github.com/grandine/universal-precompiles.git", branch = "RustCrypto-hashes/sha2-v0.10.9" }

Comment thread Cargo.toml Outdated
Comment on lines +616 to +617
# sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0' }
sha2 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'RustCrypto-hashes/sha2-v0.10.9' }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once grandinetech/universal-precompiles#3 is merged, modify to

Suggested change
# sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0' }
sha2 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'RustCrypto-hashes/sha2-v0.10.9' }
sha2 = { git = "https://github.com/grandine/universal-precompiles.git", branch = "RustCrypto-hashes/sha2-v0.10.9" }

@povi

povi commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

@jimmychu0807, I see build fails (jimmychu0807@afb0dac). Please make sure CI checks pass

Comment thread bls/bls-zkcrypto/src/secret_key.rs Outdated
Comment on lines +74 to +79
#[cfg(feature = "zkvm-pico")]
let h = <G2Projective as HashToCurve<ExpandMsgXmd<Sha256>>>::hash_to_curve(
message.as_ref(),
DOMAIN_SEPARATION_TAG,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is still not fixed, there should be single api for hash_to_curve, not dependent on the backend

Comment thread ssz/src/shared.rs Outdated
Comment on lines +127 to +128
// Note: ignore the check `actual > maximum` when maximum == 0. Is this proper?
if maximum > 0 && actual > maximum {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be removed, maximum check is required - error is caused because state gets deserialized with incorrect phase

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

@jimmychu0807

jimmychu0807 commented Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

@ArtiomTr

I believe the PR is now good for review. Thanks.

@jimmychu0807 jimmychu0807 requested a review from ArtiomTr October 31, 2025 14:48
Comment thread .github/workflows/ci.yml Outdated
with:
root-reserve-mb: 2048
# Leaving 20 GB for rust crates download and installation under `/tmp`
root-reserve-mb: 20480

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I encountered running out of space error because rust crates are downloaded under /tmp dir for installation. So here, we increase the space reserved for root dir.

@ArtiomTr ArtiomTr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also please add documentation in zkvm/README.md on how to run pico tests.

Comment thread Cargo.toml Outdated
anonymous_parameters = 'warn'
deprecated_in_future = 'warn'
deprecated_safe = 'warn'
deprecated_safe = { level = 'warn', priority = -1 }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why this change?

Comment on lines -24 to -26
[patch.'https://github.com/zkcrypto/bls12_381.git']
bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why bls patch removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need this line to compile the guest, as shown in the workflows/zkvm-test.yml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ah yes, because of feature thing, okay

Comment thread zkvm/guest/sp1/Cargo.toml
Comment on lines -17 to -22

[patch.crates-io]
sha2 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'sha2-v0.10.9-up.0' }

[patch.'https://github.com/zkcrypto/bls12_381.git']
bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.0' }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why these removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The guest compiles fine without these two lines

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, but guest also compiles without any patch at all - the point of patches is not to provide functionality that otherwise is unavailable, but to speed up existing crates. By removing these sections, performance should degrade.

Comment thread zkvm/host/Cargo.toml Outdated

[build-dependencies]
sp1-helper = { workspace = true, optional = true }
anyhow = { workspace = true, optional = true }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why anyhow in build dependencies?

Comment thread .github/workflows/zkvm-test.yml Outdated
Comment on lines +9 to +13
test_case: [
# These two test cases can be completed in a reasonably short time
"consensus spec tests mainnet electra empty block transition",
"pectra-devnet-6 without epoch transition"
]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it is better to structure pipeline the other way - by doing matrix on "backend", not on test case. Then you won't need to install each backend twice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also, after thinking a bit more about this, maybe this job should be put inside ./.github/workflows/zkvm-test.yml workflow, rather than separate one? I believe two jobs - default lint/build/format CI and zkvm testing - should run at the same events, so we can colocate them properly

@jimmychu0807 jimmychu0807 Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

./.github/workflows/zkvm-test.yml

Do you intent to mean ./.github/workflows/ci.yml? It is now in zkvm-test.yml.

@jimmychu0807 jimmychu0807 Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it will be better to have all zkVM related build/tests separated from the main ones, the ci.yml build/tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yeah, sorry, meant ci.yaml. I proposed to keep them joint, as that is still CI, checking if code works, not separate pipeline. They both need to run at the same time, so don't see a reason to keep them separate

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it is better to structure pipeline the other way - by doing matrix on "backend", not on test case. Then you won't need to install each backend twice.

I agree this can be structured to parallelize run for different zkVMs.

Comment thread .github/workflows/zkvm-test.yml Outdated
- name: Installing r0vm
run: |
curl -L https://risczero.com/install | bash
ls -lah $HOME

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this really needed?

Comment thread .github/workflows/zkvm-test.yml Outdated
Comment on lines +55 to +58
/home/runner/.risc0/bin/rzup install rust 1.88.0
/home/runner/.risc0/bin/rzup install cpp 2024.1.5
/home/runner/.risc0/bin/rzup install r0vm 3.0.1
/home/runner/.risc0/bin/rzup install cargo-risczero 3.0.1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shouldn't it work by providing not whole path, but just executable name rzup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

still doesn't answer my question, why you use /home/runner/.risc0/bin/rzup instead of simple rzup? Docs you pointed also show rzup usage, not full path

@jimmychu0807 jimmychu0807 Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It couldn't find rzup (not in the path). Shown here:
https://github.com/jimmychu0807/grandine/actions/runs/19332684742/job/55299564273#step:2:41

So using the full path here.
Updated: at the end I added an export PATH... command so we can run rzup directly. But it is not in the default PATH even after sourcing ~/.bashrc.

Comment thread .github/workflows/zkvm-test.yml Outdated
Comment on lines +79 to +85
pushd ./
cd ..
git clone https://github.com/brevis-network/pico pico
cd pico/sdk/cli
cargo install --locked --force --path .
cargo pico --version
popd

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe all of this can be simplified to

cargo +nightly-2025-08-04 install --git https://github.com/brevis-network/pico pico-cli

as mentioned in their docs

Comment thread Cargo.toml Outdated
Comment on lines +71 to +75
exclude = [
# Excluding the zkvm guest library will make the compiled guest code size a lot smaller, from
# ~200 MB -> 2.3 MB.
'zkvm/guest/pico'
]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cool, is it default exclude flag value? If so, we can simplify our makefile by putting excludes here:

EXCLUDES = --workspace --exclude zkvm_host --exclude zkvm_guest_risc0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since 'zkvm/guest/pico' is already excluded from workspace, there is no zkvm_guest_pico from the "workspace" perspective. Isn't it?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

okay then I have zero clue what's going here - how you exclude package from workspace, that isn't there in the first place? https://doc.rust-lang.org/cargo/reference/workspaces.html#the-members-and-exclude-fields

@jimmychu0807 jimmychu0807 Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The exclude inside the Cargo.toml is excluding the specified child path to be included as a package.

The EXCLUDES that we set in Makefile is excluding whatever packages already recognized by the workspace, thus using the crate name.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

okay, I checked locally, and found some things:

  • The comment is misleading, as this exclude required because otherwise it will error out, as guest code believes that it lives in workspace:
      thread 'main' panicked at sdk/cli/src/build/build.rs:140:10:
      cargo metadata command failed: CargoMetadata { stderr: "error: current package believes it's in a workspace when it's not:\ncurrent:   /home/necolt/workspace/grandine/zkvm/guest/pico/Cargo.toml\nworkspace: /home/necolt/workspace/grandine/Cargo.toml\n\nthis may be fixable by adding `zkvm/guest/pico` to the `workspace.members` array of the manifest located at: /home/necolt/workspace/grandine/Cargo.toml\nAlternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.\n" }
      note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    
  • As suggested by error, two things can be done: either adding an exclude to workspace cargo.toml, or adding [workspace] section to guest's cargo.toml.
  • The same issue was with SP1's guest code, and it was solved by adding empty [workspace] section.
  • For the sake of consistency, please remove these excludes and add empty workspace section to pico's Cargo.toml.
  • Double check guest code size: if empty [workspace] section produces larger guest code, than [excludes] field, then remove empty workspace sections in both sp1 and pico guest code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added an empty [workspace] and removed the [excludes] field. The output elf file size is the same.

@ArtiomTr

Copy link
Copy Markdown
Collaborator

Also, can you please squash all commits into one, to keep history clean? Thank you

@jimmychu0807 jimmychu0807 force-pushed the feature/zkvm-pico branch 3 times, most recently from bd4fd02 to e4104c4 Compare November 14, 2025 06:28
Comment thread Makefile Outdated
cargo check --features default-networks $(EXCLUDES)

build:
cargo build --release --bin grandine --features default-networks --workspace --exclude zkvm_host --exclude zkvm_guest_risc0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe this line not needed

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +85 to +167
risc0-test:
runs-on: ubuntu-latest
steps:
# Checkout code first to access local github action below
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup
uses: ./.github/actions/setup
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Installing r0vm
# Refer to docs:
# https://dev.risczero.com/api/zkvm/install#using-rzup-in-ci
run: |
curl -L https://risczero.com/install | bash
source /home/runner/.bashrc
export PATH="/home/runner/.risc0/bin:$PATH"
rzup install rust 1.88.0
rzup install cpp 2024.1.5
rzup install r0vm 3.0.1
rzup install cargo-risczero 3.0.1
cargo risczero --version

- name: Running r0vm test
run: |
IFS=$'\n'
for TEST in $TEST_CASES; do
echo "Running test: $TEST"
RUST_LOG=info RISC0_DEV_MODE=1 cargo run -p zkvm_host --features risc0 --release -- --test "$TEST" execute
done

# zkVM test - SP1 implementation
sp1-test:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup
uses: ./.github/actions/setup
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Installing SP1
run: |
curl -L https://sp1up.succinct.xyz | bash
/home/runner/.sp1/bin/sp1up -v 5.2.1 -- token ${{ secrets.GITHUB_TOKEN }}

- name: Running SP1 test
run: |
IFS=$'\n'
for TEST in $TEST_CASES; do
echo "Running test: $TEST"
RUST_LOG=info cargo run -p zkvm_host --features sp1 --release -- --test "$TEST" execute
done

# zkVM test - Brevis Pico implementation
pico-test:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup
uses: ./.github/actions/setup
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Installing Brevis Pico
run: |
rustup install nightly-2025-08-04
rustup component add rust-src --toolchain nightly-2025-08-04
cargo +nightly-2025-08-04 install --git https://github.com/brevis-network/pico pico-cli

- name: Running Brevis Pico test
run: |
IFS=$'\n'
for TEST in $TEST_CASES; do
echo "Running test: $TEST"
RUST_LOG=info cargo +nightly-2025-08-04 run -p zkvm_host --features pico --release -- --test "$TEST" execute
done

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

still believe this all can be done more cleanly with matrix strategy, something like:

matrix:
  backend: [risc0, sp1, pico]
  include:
    - backend: risc0
      install: |
        curl -L https://risczero.com/install | bash
        source /home/runner/.bashrc
        export PATH="/home/runner/.risc0/bin:$PATH"
        rzup install rust 1.88.0
        rzup install cpp 2024.1.5
        rzup install r0vm 3.0.1
        rzup install cargo-risczero 3.0.1
        cargo risczero --version
      execute: RUST_LOG=info RISC0_DEV_MODE=1 cargo run -p zkvm_host --features risc0 --release -- --test "$TEST" execute
    
    - backend: sp1
      install: ...
      execute: ...
      
    - backend: pico
      install: ...
      execute: ...
      
steps:
  # preparation steps here
  
  - name: Installing ${{ matrix.backend }} zkvm
    run: ${{ matrix.install }}
    
  - name: Running ${{ matrix.backend }} tests
    run: |
      IFS=$'\n'
      for TEST in $TEST_CASES; do
        echo "Running test: $TEST"
        ${{ matrix.execute }}
      done

Then no need for setup phase in separate file - everything can be in one place. Also, simplifies adding new zkvm

Comment thread Cargo.toml Outdated
Comment on lines +71 to +75
exclude = [
# Excluding the zkvm guest library will make the compiled guest code size a lot smaller, from
# ~200 MB -> 2.3 MB.
'zkvm/guest/pico'
]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

okay, I checked locally, and found some things:

  • The comment is misleading, as this exclude required because otherwise it will error out, as guest code believes that it lives in workspace:
      thread 'main' panicked at sdk/cli/src/build/build.rs:140:10:
      cargo metadata command failed: CargoMetadata { stderr: "error: current package believes it's in a workspace when it's not:\ncurrent:   /home/necolt/workspace/grandine/zkvm/guest/pico/Cargo.toml\nworkspace: /home/necolt/workspace/grandine/Cargo.toml\n\nthis may be fixable by adding `zkvm/guest/pico` to the `workspace.members` array of the manifest located at: /home/necolt/workspace/grandine/Cargo.toml\nAlternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.\n" }
      note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    
  • As suggested by error, two things can be done: either adding an exclude to workspace cargo.toml, or adding [workspace] section to guest's cargo.toml.
  • The same issue was with SP1's guest code, and it was solved by adding empty [workspace] section.
  • For the sake of consistency, please remove these excludes and add empty workspace section to pico's Cargo.toml.
  • Double check guest code size: if empty [workspace] section produces larger guest code, than [excludes] field, then remove empty workspace sections in both sp1 and pico guest code.

Comment thread zkvm/README.md
```
Again, replace `<test_case>` with one of the four test cases mentioned above.

## Adding new zkvm

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you please also mention in Adding new zkvm section, that you need to add new ZKVM to ci.yaml workflow?

@jimmychu0807 jimmychu0807 force-pushed the feature/zkvm-pico branch 4 times, most recently from e078ba1 to 4a974a6 Compare November 14, 2025 10:40
@jimmychu0807

Copy link
Copy Markdown
Contributor Author

I didn't change the disk-space-freer. I tried jlumbroso/free-disk-space and it gave other errors.

So sticking with easimon/maximize-build-space@master for now. When the CI is complete:
https://github.com/jimmychu0807/grandine/actions/runs/19362033854

See if the PR is good to merge. Thanks.

@ArtiomTr

Copy link
Copy Markdown
Collaborator

I didn't change the disk-space-freer. I tried jlumbroso/free-disk-space and it gave other errors.

That's because it uninstalls too much, and now required dependencies for grandine must be installed back:

      - name: Install system dependencies (Ubuntu)
        if: runner.os == 'Linux'
        run: |
          sudo DEBIAN_FRONTEND='noninteractive' apt-get install \
            --no-install-recommends -yq \
            ca-certificates \
            libssl-dev \
            clang \
            cmake \
            unzip \
            protobuf-compiler \
            libz-dev

See if the PR is good to merge. Thanks.

The flaky CI issue is quite a big deal, needs to be sorted out before merging this PR.

@jimmychu0807

jimmychu0807 commented Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

But this CI has been used in Grandine since I started working on this. I'm not sure I still have time to work on this CI issue. If not, I would suggest fixing this in another PR.

It passes (this time):
https://github.com/jimmychu0807/grandine/actions/runs/19362033854/job/55398968665

@jimmychu0807

jimmychu0807 commented Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

@ArtiomTr, I have updated ci.yaml using jlumbroso/free-disk-space@main action and added the section of code to add back system dependencies.

The CI is running fine for the last push:
https://github.com/jimmychu0807/grandine/actions/runs/19460867176

@ArtiomTr ArtiomTr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 👍

@jimmychu0807

Copy link
Copy Markdown
Contributor Author

@ArtiomTr see if this PR is good to merge. Thanks.

@jimmychu0807

Copy link
Copy Markdown
Contributor Author

Hi @povi, this is the integration of Brevis Pico in the Grandine zkvm crate (both host and guest). Please see if you have time to review and get it merged to the develop branch. Thank you!

@povi povi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, let's merge

@povi povi merged commit bc5c0db into grandinetech:develop Nov 27, 2025
1 check passed
@jimmychu0807 jimmychu0807 deleted the feature/zkvm-pico branch November 27, 2025 14:21
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.

4 participants