chore(zk_ee): introduce proving_env feature flag for conditional compilation#595
chore(zk_ee): introduce proving_env feature flag for conditional compilation#595vv-dev-ai wants to merge 2 commits into
Conversation
…ilation Replace cfg(target_arch = "riscv32") with cfg(feature = "proving_env") in places where the intent is proving vs forward mode detection, not actual RISC-V hardware detection. This separates the semantic concern (proving environment) from the hardware concern (RISC-V architecture). The proving_env feature propagates through the dependency chain: proof_running_system/proving -> basic_system/proving -> zk_ee/proving_env proof_running_system/proving -> basic_bootloader/proving_env -> zk_ee/proving_env Hardware-specific code (assembly in cycle_marker, delegated_u256) and pointer-width-dependent code (serialization) remain unchanged.
There was a problem hiding this comment.
Pull request overview
This PR introduces a proving_env Cargo feature to represent “proving environment vs forward mode” semantics, replacing several prior cfg(target_arch = "riscv32") gates that were being used as a proxy for proving-mode behavior.
Changes:
- Adds
zk_ee/proving_envfeature and propagates it throughbasic_systemandproof_running_systemproving feature sets (plus abasic_bootloader/proving_envpass-through). - Switches multiple conditional-compilation sites from
target_arch = "riscv32"tofeature = "proving_env"for error context handling, contextualization, cheap cloning, and logging macros. - Updates error-context tests and bootloader cycle marker logging guards to follow the new feature gate.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
zk_ee/src/utils/cheap_clone.rs |
Replaces RISCV-arch gating with proving_env for CheapCloneRiscV impl selection; updates doc comments. |
zk_ee/src/system/mod.rs |
Updates logger_log! macro cfg gates to use proving_env (active vs no-op). |
zk_ee/src/system/errors/context/tests.rs |
Replaces numerous target_arch-based test cfg blocks with proving_env. |
zk_ee/src/system/errors/context/nonempty.rs |
Gates the non-empty context implementation on not(proving_env) and updates an internal cfg! check + docs. |
zk_ee/src/system/errors/context/mod.rs |
Switches ErrorContext type alias selection to proving_env. |
zk_ee/src/system/errors/context/empty.rs |
Gates the empty context implementation on proving_env and updates docs. |
zk_ee/src/system/errors/context/contextualized.rs |
Updates contextualization logic to skip context evaluation under proving_env. |
zk_ee/Cargo.toml |
Adds the proving_env feature definition and documentation. |
proof_running_system/Cargo.toml |
Extends proving feature to enable zk_ee/proving_env and basic_bootloader/proving_env. |
basic_system/Cargo.toml |
Extends proving feature to enable zk_ee/proving_env. |
basic_bootloader/src/bootloader/transaction_flow/zk/mod.rs |
Updates cycle marker logging guards to not(proving_env). |
basic_bootloader/Cargo.toml |
Adds proving_env feature that forwards to zk_ee/proving_env. |
Comments suppressed due to low confidence (1)
zk_ee/src/system/errors/context/nonempty.rs:49
nonempty.rsis already module-gated with#![cfg(not(feature = "proving_env"))], soif cfg!(not(feature = "proving_env"))is a compile-time constant and theifis redundant. Consider removing theifand keeping only thematch visibility { ... }to simplify the control flow.
if cfg!(not(feature = "proving_env")) {
match visibility {
ValueVisibility::AnyForwardRun => perform_push_value(),
ValueVisibility::DetailedOnly if cfg!(feature = "detailed_errors") => {
perform_push_value()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Using this trait allows using Copy instead of Clone in the proving | ||
| /// environment. It is useful for types like errors which contain less fields | ||
| /// when compiled for proving. | ||
| pub trait CheapCloneRiscV { | ||
| /// Clone for standard target, Copy if on RISC-V. | ||
| /// Clone for forward mode, Copy in proving environment. | ||
| /// Used for performance on small types |
There was a problem hiding this comment.
The trait name CheapCloneRiscV no longer matches its documented behavior (it’s about proving_env, not RISC-V). To avoid confusing API consumers, consider introducing a better-named alias/trait (and optionally deprecating the old name) or at least document that the name is legacy.
There was a problem hiding this comment.
[AI] Done: Renamed CheapCloneRiscV to CheapCloneProvingEnv and updated all usages across the codebase (zk_ee, evm_interpreter, system_hooks).
There was a problem hiding this comment.
[AI] Done: Renamed CheapCloneRiscV to CheapCloneProvingEnv and updated all usages across the codebase.
| @@ -498,17 +498,17 @@ mod tests { | |||
| }; | |||
|
|
|||
| // Normal expressions should always be evaluated (on non-RISC-V) | |||
There was a problem hiding this comment.
This comment still references “non-RISC-V”, but the behavior is now gated by feature = "proving_env". Please update the wording to match the new condition (e.g. “in forward mode” / “when not in proving environment”).
| // Normal expressions should always be evaluated (on non-RISC-V) | |
| // Normal expressions should always be evaluated when not in the proving environment (forward mode) |
There was a problem hiding this comment.
[AI] Done: Updated the comment from 'non-RISC-V' to 'in forward mode / non-proving environment' to match the new proving_env feature gating.
There was a problem hiding this comment.
[AI] Done: Updated comment wording from 'non-RISC-V' to 'forward mode / non-proving environment'.
The trait name CheapCloneRiscV was a leftover from when the code used cfg(target_arch = "riscv32") instead of the proving_env feature flag. Rename it to CheapCloneProvingEnv to accurately reflect that the trait is about the proving environment, not about RISC-V hardware. Also update the "non-RISC-V" comment in tests to reference forward/proving mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[AI] Thank you for the review overview. All feedback has been addressed in commit 644a24c. |
|
[AI] This PR has been open for 21 days awaiting review. Please let me know if any changes are needed or if this should be closed. |
What
Introduces a
proving_envfeature flag to replacecfg(target_arch = "riscv32")in places where the intent is "proving mode vs forward mode", not actual RISC-V hardware detection.Changes:
proving_envfeature tozk_ee, propagated throughbasic_system/proving,basic_bootloader/proving_env, andproof_running_system/provingcfg(target_arch = "riscv32")→cfg(feature = "proving_env")in:CheapCloneRiscVtrait (Copy vs Clone dispatch)logger_log!macro (active vs no-op selection)Contextualizedtrait (context evaluation vs skip)target_archunchanged where it's genuinely about hardware: cycle_marker CSR assembly, delegated_u256 RISC-V instructions, no_std gatingtarget_pointer_widthunchanged for serialization/arithmetic (actual hardware concern)Why
The codebase uses
cfg(target_arch = "riscv32")as a proxy for "we're in the proving environment" in many places where the actual intent is about proving vs forward mode behavior, not RISC-V hardware. This creates a semantic mismatch. Using a dedicated feature flag:crypto/provingandbasic_system/provingIs this a breaking change?
Checklist