-
Notifications
You must be signed in to change notification settings - Fork 1
chore(l1): fix prover timing #46
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
Conversation
ilitteri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the zkvm value at runtime instead of populating the code with feature flags.
|
the problem is that the zkvm variants are behind feature flags themselves, this is ethrex code: pub enum Backend {
#[default]
Exec,
#[cfg(feature = "sp1")]
SP1,
#[cfg(feature = "risc0")]
RISC0,
#[cfg(feature = "zisk")]
ZisK,
}I think we could remove these though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Purpose: Introduce timed execution/proving APIs and adjust reporting to exclude client initialization time, while accommodating ZisK backend constraints (MPI init) by conditionally skipping execution. Also updates all internal dependencies to a feature branch to access new timed APIs.
- Replace manual SystemTime measurement with
execute_timed/prove_timed. - Make execution timing optional (skipped for ZisK) and adjust reporting/logging/output structures accordingly.
- Point all ethrex crate dependencies to
guest_program_timed_apibranch to consume new APIs.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/run.rs | Switched to timed prover APIs, removed manual timing. |
| src/report.rs | Execution result made optional; added unified error detection and updated formatting/messages. |
| src/cli.rs | Added conditional execution skipping for ZisK; introduced optional execution result; (identified feature-gated skip bug). |
| Cargo.toml | Pointed all ethrex dependencies to guest_program_timed_api branch for timed API (noted maintainability concern). |
| Cargo.lock | Locked to commits from guest_program_timed_api branch reflecting dependency branch updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Cargo.toml
Outdated
| ethrex-config = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-config", branch = "guest_program_timed_api" } | ||
| ethrex-storage = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage", branch = "guest_program_timed_api" } | ||
| ethrex-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-common", branch = "guest_program_timed_api" } | ||
| ethrex-vm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-vm", branch = "guest_program_timed_api" } | ||
| ethrex-levm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-levm", branch = "guest_program_timed_api" } | ||
| ethrex-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rpc", branch = "guest_program_timed_api" } | ||
| ethrex-p2p = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-p2p", branch = "guest_program_timed_api" } | ||
| ethrex-trie = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-trie", branch = "guest_program_timed_api" } | ||
| ethrex-rlp = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rlp", branch = "guest_program_timed_api" } | ||
| ethrex-blockchain = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-blockchain", branch = "guest_program_timed_api" } | ||
| ethrex-l2 = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2", branch = "guest_program_timed_api", optional = true } | ||
| ethrex-l2-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-common", branch = "guest_program_timed_api" } | ||
| ethrex-storage-rollup = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage-rollup", branch = "guest_program_timed_api", optional = true } | ||
| ethrex-sdk = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-sdk", branch = "guest_program_timed_api" } | ||
| ethrex-l2-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-rpc", branch = "guest_program_timed_api" } | ||
| ethrex-prover = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-prover", branch = "guest_program_timed_api", default-features = false } | ||
| guest_program = { git = "https://github.com/lambdaclass/ethrex", package = "guest_program", branch = "guest_program_timed_api", default-features = false } |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All internal ethrex dependencies have been switched from branch = "main" to the feature branch guest_program_timed_api. Pinning an entire stack to a moving branch increases reproducibility risk (future branch force-push or divergence) and makes later merges harder. Prefer specifying an exact commit (rev = "<sha>") or reverting to main after the timed API change is published, to maintain stable build inputs.
| ethrex-config = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-config", branch = "guest_program_timed_api" } | |
| ethrex-storage = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage", branch = "guest_program_timed_api" } | |
| ethrex-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-common", branch = "guest_program_timed_api" } | |
| ethrex-vm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-vm", branch = "guest_program_timed_api" } | |
| ethrex-levm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-levm", branch = "guest_program_timed_api" } | |
| ethrex-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rpc", branch = "guest_program_timed_api" } | |
| ethrex-p2p = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-p2p", branch = "guest_program_timed_api" } | |
| ethrex-trie = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-trie", branch = "guest_program_timed_api" } | |
| ethrex-rlp = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rlp", branch = "guest_program_timed_api" } | |
| ethrex-blockchain = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-blockchain", branch = "guest_program_timed_api" } | |
| ethrex-l2 = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2", branch = "guest_program_timed_api", optional = true } | |
| ethrex-l2-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-common", branch = "guest_program_timed_api" } | |
| ethrex-storage-rollup = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage-rollup", branch = "guest_program_timed_api", optional = true } | |
| ethrex-sdk = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-sdk", branch = "guest_program_timed_api" } | |
| ethrex-l2-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-rpc", branch = "guest_program_timed_api" } | |
| ethrex-prover = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-prover", branch = "guest_program_timed_api", default-features = false } | |
| guest_program = { git = "https://github.com/lambdaclass/ethrex", package = "guest_program", branch = "guest_program_timed_api", default-features = false } | |
| ethrex-config = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-config", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-storage = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-common", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-vm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-vm", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-levm = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-levm", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rpc", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-p2p = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-p2p", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-trie = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-trie", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-rlp = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-rlp", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-blockchain = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-blockchain", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-l2 = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2", rev = "abcdef1234567890abcdef1234567890abcdef12", optional = true } | |
| ethrex-l2-common = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-common", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-storage-rollup = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-storage-rollup", rev = "abcdef1234567890abcdef1234567890abcdef12", optional = true } | |
| ethrex-sdk = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-sdk", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-l2-rpc = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-l2-rpc", rev = "abcdef1234567890abcdef1234567890abcdef12" } | |
| ethrex-prover = { git = "https://github.com/lambdaclass/ethrex", package = "ethrex-prover", rev = "abcdef1234567890abcdef1234567890abcdef12", default-features = false } | |
| guest_program = { git = "https://github.com/lambdaclass/ethrex", package = "guest_program", rev = "abcdef1234567890abcdef1234567890abcdef12", default-features = false } |
src/cli.rs
Outdated
| #[cfg(feature = "zisk")] | ||
| let execution_result = None; | ||
| #[cfg(not(feature = "zisk"))] | ||
| let execution_result = Some(exec(backend, cache.clone()).await); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the zisk feature is enabled, this code unconditionally sets execution_result = None for ALL backends, not just Backend::ZisK. This differs from other places (e.g. EthrexReplayCommand and L2 replay) where a runtime check backend != Backend::ZisK is used. As written, enabling the feature will skip execution even for non-ZisK backends, likely unintended and leading to missing execution timing and logic. Suggest changing to a runtime conditional similar to:
let execution_result = if backend == Backend::ZisK { None } else { Some(exec(backend, cache.clone()).await) };
(or keep the existing pattern used elsewhere) so only ZisK skips execution.
| #[cfg(feature = "zisk")] | |
| let execution_result = None; | |
| #[cfg(not(feature = "zisk"))] | |
| let execution_result = Some(exec(backend, cache.clone()).await); | |
| let execution_result = if backend == Backend::ZisK { None } else { Some(exec(backend, cache.clone()).await) }; |
src/cli.rs
Outdated
| #[cfg(feature = "zisk")] | ||
| let execution_result = None; | ||
| #[cfg(not(feature = "zisk"))] | ||
| let execution_result = Some(exec(backend, cache.clone()).await); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same unconditional skip as in replay_block: with the zisk feature enabled this sets execution_result = None for every backend, not only ZisK. This causes all executions to be skipped under that feature, losing execution timing and potentially required side effects. Replace the cfg split with a runtime check on the resolved backend:
let execution_result = if backend == Backend::ZisK { None } else { Some(exec(backend, cache.clone()).await) };
to align behavior with other call sites.
| #[cfg(feature = "zisk")] | |
| let execution_result = None; | |
| #[cfg(not(feature = "zisk"))] | |
| let execution_result = Some(exec(backend, cache.clone()).await); | |
| let execution_result = if backend == Backend::ZisK { | |
| None | |
| } else { | |
| Some(exec(backend, cache.clone()).await) | |
| }; |
Motivation
To measure execution and proving time correctly. This means removing the client initialization from the equation. In the case of ZisK, we use it's own timing exposed via the SDK.
lambdaclass/ethrex#5405