Prepare for client-only zkvm usage #908
Conversation
| bytemuck::cast_slice(&self.seal) | ||
| /// Return the seal for this receipt, as a vector of bytes. | ||
| pub fn get_seal_bytes(&self) -> Vec<u8> { | ||
| self.seal.iter().flat_map(|x| x.to_le_bytes()).collect() |
There was a problem hiding this comment.
Possibly breaking change (on big-endian systems)
| pub(crate) segment_limit_po2: Option<u32>, | ||
| pub(crate) session_limit: Option<u64>, |
Benchmark for Linux-cudaBenchmark for Linux-default f87f936Click to hide benchmark
Benchmark for macOS-default f87f936Click to hide benchmark
Benchmark for macOS-metal |
Benchmark for Linux-cuda af328edClick to hide benchmark
Benchmark for Linux-defaultBenchmark for macOS-defaultBenchmark for macOS-metal af328edClick to hide benchmark
|
Benchmark for Linux-cuda 0bf2dd8Click to hide benchmark
Benchmark for Linux-defaultBenchmark for macOS-default 0bf2dd8Click to hide benchmark
Benchmark for macOS-metal |
Benchmark for Linux-cuda 4a27432Click to hide benchmark
Benchmark for Linux-default 4a27432Click to hide benchmark
Benchmark for macOS-default 4a27432Click to hide benchmark
Benchmark for macOS-metal |
flaub
changed the title
Benchmark for Linux-cuda 2991bbfClick to hide benchmark
Benchmark for Linux-default 2991bbfClick to hide benchmark
Benchmark for macOS-default 2991bbfClick to hide benchmark
Benchmark for macOS-metal 2991bbfClick to hide benchmark
|
|
@mothran Once this PR lands, I think we can begin to test how to get Bonsai to use the client API ahead of (or in parallel to) the other planned PRs to get the binary distributed for end users. |
| pub segments: u32, | ||
|
|
||
| /// The data publicly committed by the guest program. | ||
| pub journal: Bytes, |
There was a problem hiding this comment.
Maybe this should be Option<Bytes> since an exit code other than Paused or Halted will not have a journal. Empty bytes also seems reasonable.
There was a problem hiding this comment.
There was a Slack discussion about how "empty journal" hashes to SHA256([]) whereas "no journal" "hashes" to 0. If we want to retain this distinction we probably need a way to represent it here (which Option<Bytes> makes sense for that to me).
There was a problem hiding this comment.
Actually the hashing distinction was only really relevant to the way the circuit generates the 'output' digest.
I guess Option<Bytes> is slightly better for the API, but we should make that change in all the other APIs as well (this is just the client/server API, which is kind of a private API).
There was a problem hiding this comment.
I tried making this change but seems to be non-trivial so I'm going to kick this to a future PR.
| let program = Program::load_elf(&MULTI_TEST_ELF, GUEST_MAX_MEM as u32).unwrap(); | ||
| let image = MemoryImage::new(&program, PAGE_SIZE as u32).unwrap(); | ||
| let binary = image.into(); | ||
| TestClient::new().execute(env, binary).unwrap(); |
There was a problem hiding this comment.
In CI, do we want to make sure these tests all run both with in-process execution and with sub-process execution? (Same question for prover tests).
There was a problem hiding this comment.
There are existing tests for the in-process which is actually more extensive. What I was shooting for in these tests is to get coverage for the client/server message passing specifically. Admittedly these tests are incomplete; there should be more to cover more cases.
I've been pondering how we could get a common collection of tests to be run in in-process or sub-process mode, but the best I can come up with is to use an integration test to ensure that r0vm gets built. These tests are a compromise, it doesn't actually spin up a separate process, it uses a separate thread instead. But it does use all the same messages.
| pub journal: Bytes, | ||
|
|
||
| /// The [ExitCode] of the session. | ||
| pub exit_code: ExitCode, |
There was a problem hiding this comment.
Instead of journal and exit_code, it'd be nice to have this be ReceiptMetadata, which essentially represents the set of public claims about an execution. For composition, being able to get the ReceiptMetadata for an execution is very useful because it allows starting execution and proving for one program as soon as all of it's dependencies have been executed, rather than waiting for dependencies to be proven.
One downside to this would be pushing the journal one layer down, which is the main field a developer is likely interested in. One option would be to have these fields be top-level in SessionInfo and have a get_metadata method that constructs the ReceiptMetadata struct if needed.
There was a problem hiding this comment.
This is a really good point, I like the idea of having a get_metadata. I'll play around with this.
There was a problem hiding this comment.
I like this idea, but it turns out that the executor doesn't yet have the smarts to construct a ReceiptMetadata. Also, I don't know how we can construct one without running the entire program since only the final segment is guaranteed to have outputs and the final output digest.
risc0/zkvm/src/host/api/mod.rs
Outdated
| /// The number of segments. | ||
| pub segments: u32, |
There was a problem hiding this comment.
Segments seems less useful if the cycle count or po2 for the segments is not known. I may be known by context, if the client requested a specific po2. It would be nice if enough information is included here to additional know (at least an upper bound on) the number of cycles this represents.
risc0/zkvm/src/host/api/client.rs
Outdated
| let receipt = bincode::deserialize(&receipt_bytes)?; | ||
| Ok(receipt) | ||
| let receipt_pb = pb::core::Receipt::decode(receipt_bytes)?; | ||
| receipt_pb.try_into() | ||
| } | ||
|
|
||
| /// TODO |
There was a problem hiding this comment.
Not something that needs to be solved within this PR, but I want to note that this architecture is not yet fully documented and we'll need to finish its docs before the next release.
There was a problem hiding this comment.
I actually think client/server is largely transparent to end users. We do need to describe the public bits and how to install the server, but I don't think we need to (or necessarily should) document the internal part since it's undergoing changes and won't be relevant (and in fact confusing) to the majority of users.
There was a problem hiding this comment.
👍 makes sense. I do think that anything that shows up on docs.rs shouldn't have docs that just say "TODO". (Minimally something like "Internal component")
There was a problem hiding this comment.
If you see a TODO, could you mark it or link to it? I probably missed those
There was a problem hiding this comment.
The line this comment is on is one of them. I think they were introduced in a different client/server PR -- I noticed them when reviewing by reading through the generated docs, not on the PR proper. Also lines 66, 100, 137, 177, and 215 of this file, and lines 178, 183, and 189 of https://github.com/risc0/risc0/blob/main/risc0/zkvm/src/host/api/server.rs (as of commit 7a1581f160729b544025afaca525ee3a39394b6e).
| pub segments: u32, | ||
|
|
||
| /// The data publicly committed by the guest program. | ||
| pub journal: Bytes, |
There was a problem hiding this comment.
There was a Slack discussion about how "empty journal" hashes to SHA256([]) whereas "no journal" "hashes" to 0. If we want to retain this distinction we probably need a way to represent it here (which Option<Bytes> makes sense for that to me).
Benchmark for Linux-cuda 7b0166aClick to hide benchmark
Benchmark for Linux-defaultBenchmark for macOS-default 7b0166aClick to hide benchmark
Benchmark for macOS-metal 7b0166aClick to hide benchmark
|
Benchmark for Linux-cuda b5fbe17Click to hide benchmark
Benchmark for Linux-default b5fbe17Click to hide benchmark
Benchmark for macOS-default b5fbe17Click to hide benchmark
Benchmark for macOS-metal b5fbe17Click to hide benchmark
|
Benchmark for Linux-cudaBenchmark for Linux-default 743f0baClick to hide benchmark
Benchmark for macOS-default 743f0baClick to hide benchmark
Benchmark for macOS-metal |
This doesn't yet change any default feature flags, so should still be fairly minimal.
The planned set of PRs include:
cargo risczero installor adjust docs to have users docargo binstall cargo-risczero(depending on whether we packager0vmas standalone or combined withcargo-risczero)