Conversation
99e585f to
0c3dddc
Compare
Cargo.toml
Outdated
| # bls12_381 = { git = 'https://github.com/grandinetech/universal-precompiles.git', tag = 'bls12_381-6bb9695-up.1' } | ||
| # bls12_381 = { package = "sp1_bls12_381", path = "../zisk-bls12_381" } | ||
| # bls12_381 = { path = "../grandine-universal-precompiles-bls12_381" } | ||
| bls12_381 = { git = 'https://github.com/jimmychu0807/grandine-universal-precompiles.git', branch = 'jc/zkvm-zisk-bls12_381' } | ||
|
|
There was a problem hiding this comment.
putting here this comment just as a reminder to change it once precompiles are merged
zkvm/guest/zisk/src/main.rs
Outdated
| let VMGuestInput { | ||
| config, | ||
| state_ssz, | ||
| block_ssz, | ||
| cache_ssz, | ||
| phase_bytes, | ||
| } = bincode::deserialize(input).unwrap(); |
There was a problem hiding this comment.
don't use double encoding here -- you're currently serializing state&block&cache into SSZ, and then serialize SSZ bytes into bincode. Instead, you can pass those 3 directly as bytes into zkvm, the same as with other backends.
There was a problem hiding this comment.
From what I found, ziskemu only takes input in a file format, not bytes directly.
- ref 1: Zisk docs
- ref 2: ere also writes the input bytes to a temp file
So the issue becomes, I either not using bincode and add byte separators and then concatenate the 5 inputs myself, or using bincode and let the lib handles this for me.
For ref, input is serialized here:
https://github.com/grandinetech/grandine/pull/475/changes#r2625605370
There was a problem hiding this comment.
From what I found,
ziskemuonly takes input in a file format, not bytes directly.
Yeah, but you still can write bytes directly into file, so in that sense, it accepts bytes as input.
So the issue becomes, I either not using bincode and add byte separators and then concatenate the 5 inputs myself, or using bincode and let the lib handles this for me.
I just see it as with every other backend - you just read directly from byte array. As far as I remember, when we tried using bincode instead of ssz, we saw pretty large performance decrease. But you can try both approaches and compare, which one performs better:
- Using bincode (your current implementation).
- Doing as every other vm - reading 4
usize's from byte stream, then reading 4 slices with appropriate lengths, something like so:let mut buffer = [0u8; size_of::<usize>()]; input.read_exact(&mut buffer)?; let state_ssz_len = usize::from_be_bytes(&v); // same for block_ssz_len, cache_ssz_len and phase_bytes_len let mut state_ssz = vec![0u8; state_ssz_len]; input.read_exact(&mut state_ssz)?; // same for block_ssz, cache_ssz and phase_bytes
There was a problem hiding this comment.
updated this section of code to plainly write bytes to a file and updated the guest code to read bytes from the file.
This is indeed faster than using bincode as it is simpler.
zkvm/host/src/backend/zisk.rs
Outdated
|
|
||
| // Gather back the last set_output() from the VM guest output. | ||
| // Note: maybe able to read from the output dir the public output. | ||
| let state_root = Self::collect_result(&output)?; |
There was a problem hiding this comment.
I want to keep this line here. Because I have never able to execute to this point actually (prove mode). So keeping this line let future dev know they could either read from the output dir (I'm not sure what's the output file name), or collect the result from the screen output.
There was a problem hiding this comment.
I believe you can just comment out whole guest program, keep empty main, and then run prove mode locally, so that you can find where file is located.
It is better to check proving mode before merging, because after preparing all zkvms, we want to conduct some kind of report. So we will likely hand out code to zisk team and ask them to run proving on their rig.
There was a problem hiding this comment.
Great suggestion. I am able to create the proof file. I realize Zisk proof mode doesn't output the result, so I have to run self.execute() first to get the result and the proof output.
Have also updated Proof::verify() and Proof::save() methods accordingly with the output path.
|
@ArtiomTr I have handled your comments above to the best of my knowledge. I see two remaining issues to discuss:
|
ArtiomTr
left a comment
There was a problem hiding this comment.
Looks good, few changes needed. Also, looks like pipeline currently fails? This needs to be fixed before merging
zkvm/guest/zisk/src/main.rs
Outdated
| let VMGuestInput { | ||
| config, | ||
| state_ssz, | ||
| block_ssz, | ||
| cache_ssz, | ||
| phase_bytes, | ||
| } = bincode::deserialize(input).unwrap(); |
There was a problem hiding this comment.
From what I found,
ziskemuonly takes input in a file format, not bytes directly.
Yeah, but you still can write bytes directly into file, so in that sense, it accepts bytes as input.
So the issue becomes, I either not using bincode and add byte separators and then concatenate the 5 inputs myself, or using bincode and let the lib handles this for me.
I just see it as with every other backend - you just read directly from byte array. As far as I remember, when we tried using bincode instead of ssz, we saw pretty large performance decrease. But you can try both approaches and compare, which one performs better:
- Using bincode (your current implementation).
- Doing as every other vm - reading 4
usize's from byte stream, then reading 4 slices with appropriate lengths, something like so:let mut buffer = [0u8; size_of::<usize>()]; input.read_exact(&mut buffer)?; let state_ssz_len = usize::from_be_bytes(&v); // same for block_ssz_len, cache_ssz_len and phase_bytes_len let mut state_ssz = vec![0u8; state_ssz_len]; input.read_exact(&mut state_ssz)?; // same for block_ssz, cache_ssz and phase_bytes
zkvm/host/src/backend/zisk.rs
Outdated
|
|
||
| // Gather back the last set_output() from the VM guest output. | ||
| // Note: maybe able to read from the output dir the public output. | ||
| let state_root = Self::collect_result(&output)?; |
There was a problem hiding this comment.
I believe you can just comment out whole guest program, keep empty main, and then run prove mode locally, so that you can find where file is located.
It is better to check proving mode before merging, because after preparing all zkvms, we want to conduct some kind of report. So we will likely hand out code to zisk team and ask them to run proving on their rig.
ArtiomTr
left a comment
There was a problem hiding this comment.
The code looks good, however, when I try to prove locally, I get this error:
Error: Failed to copy file from "/home/necolt/workspace/grandine/zkvm/guest/zisk/artifacts/vadcop_final_proof.compressed.bin" to "/home/necolt/workspace/grandine/zkvm/host/proof.bin"
Caused by:
No such file or directory (os error 2)
And indeed, I don't have such file:
/zkvm/guest/zisk/artifacts/vadcop_final_proof.compressed.bin
Contents of whole artifacts directory:
input.bin
result.json
vadcop_final_proof.bin
Maybe zisk won't generate .compressed proof by default?
zkvm/host/src/backend/zisk.rs
Outdated
| let _ = fs::copy(&proof_path, path.as_ref()) | ||
| .map_err(|e| anyhow!("Copy file failed. stderr:\n{}", e.to_string()))?; |
There was a problem hiding this comment.
You can make this error a bit nicer, by providing a bit more context:
| let _ = fs::copy(&proof_path, path.as_ref()) | |
| .map_err(|e| anyhow!("Copy file failed. stderr:\n{}", e.to_string()))?; | |
| fs::copy(&proof_path, path.as_ref()).context(format!( | |
| "Failed to copy file from {proof_path:?} to {:?}", | |
| path.as_ref() | |
| ))?; |
Mentioning this, because I've got this error:
Error: Copy file failed. stderr:
No such file or directory (os error 2)
As you can see, it's not very helpful :). With suggested change, it looks like:
Error: Failed to copy file from "/home/necolt/workspace/grandine/zkvm/guest/zisk/artifacts/vadcop_final_proof.compressed.bin" to "/home/necolt/workspace/grandine/zkvm/host/proof.bin"
Caused by:
No such file or directory (os error 2)
Which is much nicer in my opinion
zkvm/host/src/backend/zisk.rs
Outdated
| } | ||
|
|
||
| fn get_guest_dir() -> PathBuf { | ||
| Path::new(env!("CARGO_MANIFEST_DIR")).join("../guest/zisk") |
There was a problem hiding this comment.
to make path a bit nicer, you can do:
| Path::new(env!("CARGO_MANIFEST_DIR")).join("../guest/zisk") | |
| Path::new(env!("CARGO_MANIFEST_DIR")).parent().expect("project cannot be at root directory").join("guest/zisk") |
this way paths will look better when trying to print them:
instead of:
/<project-path>/grandine/zkvm/host/../guest/zisk
you'll see:
/<project-path>/grandine/zkvm/guest/zisk
2b8796e to
46a27e6
Compare
|
@ArtiomTr I have updated the rest of the PRs based on the comment and squashed all commits into one. |
zkvm/guest/pico/Cargo.lock
Outdated
| "serde", | ||
| "serde_utils", | ||
| "sha2 0.10.9 (git+https://github.com/grandinetech/universal-precompiles.git?tag=sha2-v0.10.9-up.1)", | ||
| "sha2 0.10.9 (git+https://github.com/jimmychu0807/grandine-universal-precompiles.git?branch=jc%2Fzkvm-zisk-sha2)", |
There was a problem hiding this comment.
Please don't include cryptography (or other) dependencies from a personal repo, thanks.
There was a problem hiding this comment.
@povi Nice catch! I wasn't aware of that.
Now I have rebuilt the guest code of pico, sp1, and ziren and have the corresponding Cargo.lock updated.
Co-authored-by: Artiom Tretjakovas <hi@sirse.dev>
46a27e6 to
0132371
Compare
@ArtiomTr @povi
Zisk integration is completed in this PR.
sha2 and bls12_381 is pointing to my own repository, so you may want to review and merge these two PRs:
and then update
Cargo.tomlpointing back to grandinetech repo.