Skip to content

chore: update CLI crate with new SDK#2625

Open
stephenh-axiom-xyz wants to merge 4 commits intodevelop-v2.0.0-betafrom
feat/update-cli
Open

chore: update CLI crate with new SDK#2625
stephenh-axiom-xyz wants to merge 4 commits intodevelop-v2.0.0-betafrom
feat/update-cli

Conversation

@stephenh-axiom-xyz
Copy link
Copy Markdown
Contributor

@stephenh-axiom-xyz stephenh-axiom-xyz commented Mar 26, 2026

Resolves INT-6956.

PR Summary: Update CLI Crate with New SDK

Overview

Two commits that update the CLI crate (cargo-openvm) to work with the restructured v2 SDK, and fix minor clippy/fmt issues. The main theme is: CLI commands now use the SDK's internal keygen (aggregation + root keys are computed on-demand and cached to disk) instead of relying on pre-generated keys at fixed global paths.


1. CLI Command Changes (crates/cli/)

prove command

  • prove stark: Loads cached agg/root keys from ${target_dir}/openvm/agg.pk (or ~/.openvm/root.pk) if present, otherwise lets the SDK generate them. After proving, saves keys via save_keys() for future reuse.
  • prove evm: Same key-caching pattern. Adds --agg-pk argument (was missing). Calls prove_evm() instead of prove(). Removes the pre-flight check that required setup to be run first — keys are now generated internally.
  • New helper functions: maybe_with_cached_keys(), save_keys(), target_dir_from_cargo_args().

commit command

Previously only computed and saved app_exe_commit. Now:

  • Computes both app_exe_commit and app_vk_commit (via new StarkProver::app_vk_commit()).
  • Saves an AppExecutionCommit JSON with both values (hex-encoded with 0x prefix).
  • Saves a baseline file (.baseline.json) used by verify stark.
  • Saves agg PK, agg VK, and root PK to disk for reuse by subsequent prove commands.

verify command

  • verify stark: Now reads a baseline file (.baseline.json) instead of an app commit file. Reads agg.vk from ${target_dir}/openvm/ instead of ~/.openvm/agg_stark.vk. Error messages updated to point users to prove stark instead of setup.
  • verify evm: Improved error message formatting with path display.

setup command

Major simplification (115 lines removed, 53 added):

  • No longer generates or persists STARK aggregation keys (agg_stark.pk, agg_stark.vk, agg_halo2.pk, root.asm). Those are now handled per-project by prove/commit.
  • Now only generates the EVM Halo2 verifier Solidity contracts (requires solc).
  • Versioned output directory: ~/.openvm/halo2/src/v{VERSION}/.
  • Replaces --evm and --force-agg-keygen flags with a single --force flag.
  • Always downloads Halo2 params and requires solc.

default.rs

Removed global-path defaults for agg_stark.pk, agg_stark.vk, agg_halo2.pk, root.asm. Remaining:

  • default_params_dir()~/.openvm/params/
  • default_root_pk_path()~/.openvm/root.pk (new)
  • default_evm_halo2_verifier_path()~/.openvm/halo2/

util.rs

New path helpers:

  • get_openvm_dir(), get_agg_pk_path(), get_agg_vk_path(), get_app_baseline_path()
  • Removed read_default_agg_and_halo2_pk().

Dependencies

  • Added openvm-continuations (with root-prover feature) and p3-bn254 to Cargo.toml.
  • Re-enabled evm-verify in default features (was temporarily disabled with a TODO).

2. CI (cli.yml)

Uncommmented the pull_request trigger and narrowed its path filter to just crates/cli/** and the workflow file itself (previously listed sdk, vm, toolchain, etc.).


3. Integration Tests → Shell Scripts

All 6 integration tests were refactored from inline Rust Command calls to delegated bash scripts in tests/scripts/:

Test Script
test_cli_app_e2e cli_app_e2e.sh
test_cli_app_e2e_simplified cli_app_e2e_simplified.sh
test_cli_stark_e2e_simplified cli_stark_e2e_simplified.sh
test_cli_init_build cli_init_build.sh
test_cli_run_mode_pure_default cli_run_mode_pure_default.sh
test_cli_run_segment cli_run_segment.sh
test_cli_run_meter cli_run_meter.sh

Rust side now uses a single run_script() helper. Removed replace_with_local_openvm() (moved into cli_init_build.sh as a perl one-liner). Removed tempfile, itertools, fs imports that are no longer needed.


4. SDK Changes (crates/sdk/)

keygen/mod.rs

  • New RootProvingKey struct (serializable): holds root_pk: Arc<MultiStarkProvingKey<RootSC>> and trace_heights: Vec<usize>.
  • Halo2ProvingKey now derives Serialize, Deserialize (was only Clone).

lib.rs

New methods on Sdk:

  • with_agg_pk() — inject a pre-computed aggregation proving key
  • with_root_pk() — inject a pre-computed root proving key
  • agg_keygen() — convenience to get (AggProvingKey, MultiStarkVerifyingKey)
  • root_pk() — extract root proving key for serialization

prover/stark.rs

New app_vk_commit() method on StarkProver: computes a Poseidon2 hash over the DAG commits (cached_commit + vk_pre_hash) from the leaf, internal-for-leaf, and internal-recursive layers. This is the app_vk_commit exposed by the commit CLI command.

types.rs

  • AppExecutionCommit no longer gated behind #[cfg(feature = "evm-prove")] — available unconditionally.
  • Renamed app_vm_commitapp_vk_commit throughout.
  • hex_bytes32 module now public; serialization uses 0x prefix; deserialization strips it.

solidity.rs

Comment fix: app_vm_commitapp_vk_commit in instance layout documentation.


5. Static Verifier (crates/static-verifier/)

Serialization support

Several key types now derive Serialize, Deserialize to enable CLI key caching:

  • StaticVerifierCircuit
  • StaticVerifierProvingKey
  • Halo2WrapperProvingKey

Halo2ProvingPinning serde

Custom Serialize/Deserialize impl added (the Halo2 ProvingKey doesn't natively support serde). Serializes PK as raw bytes + metadata. Deserializes using ProvingKey::from_bytes::<BaseCircuitBuilder<Fr>>() with saved config_params.

Clippy fixes (commit 2)

  • keygen.rs: Removed duplicate use serde::{Deserialize, Serialize} (was under #[cfg(feature = "evm-prove")], now unconditional).
  • prover.rs: Unused import cleanup.

Key Behavioral Changes for Reviewers

  1. cargo openvm setup is no longer required for STARK proving. Aggregation and root keys are generated on first prove/commit and cached in ${target_dir}/openvm/. setup is now only needed for EVM Solidity verifier generation.

  2. Key storage moved from global to per-project. agg.pk and agg.vk live in ${target_dir}/openvm/ instead of ~/.openvm/. Exception: root.pk still uses ~/.openvm/root.pk (shared across projects).

  3. verify stark now expects a baseline file (.baseline.json) instead of a commit file, and reads agg.vk from the target directory instead of ~/.openvm/.

  4. app_vm_commit renamed to app_vk_commit across SDK types and Solidity interface. This is a breaking change for any external consumers of AppExecutionCommit JSON or the EVM proof ABI.

@github-actions

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@github-actions

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@github-actions

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@stephenh-axiom-xyz
Copy link
Copy Markdown
Contributor Author

@jonathanpwang @shuklaayush have been starting to wonder if we should pair leaf_pk and internal_for_leaf_pk with app_pk more than internal_recursive_pk, as they are different per app_pk

@shuklaayush shuklaayush removed their request for review March 27, 2026 18:28
@jonathanpwang
Copy link
Copy Markdown
Contributor

@jonathanpwang @shuklaayush have been starting to wonder if we should pair leaf_pk and internal_for_leaf_pk with app_pk more than internal_recursive_pk, as they are different per app_pk

This seems good to also help clarify what depends on what. Let's do it in a follow-up (please make a ticket) and also come up with a good name for this body.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

Claude finished @stephenh-axiom-xyz's task in 2m 33s —— View job


Code Review: Update CLI crate with new SDK

  • Gather context and read changed files
  • Review CLI command changes (commit, prove, verify, setup)
  • Review SDK changes (keygen, lib, prover, types, solidity)
  • Review static-verifier changes
  • Review test/script changes
  • Review CI workflow changes
  • Post review feedback

This is a well-structured refactor that moves CLI from pre-generated global keys to on-demand per-project key caching. The setup simplification, baseline-based verification, and shell script tests are all good improvements. Below are issues found in the latest revision (ea2703b):

Issues

1. Incomplete app_vm_commitapp_vk_commit rename in local variables (types.rs)

crates/sdk/src/types.rs:221-222,234,275-277,295: Local variables still use the old app_vm_bytes / app_vm_fr naming while the struct field they map to is now app_vk_commit. Cosmetic but confusing — the variable names suggest the old semantics.

Fix this →

Also crates/sdk/tests/integration.rs.disabled:384 still references app_commit.app_vm_commit — this will fail to compile if the test is ever re-enabled.

2. Solidity interface still uses appVmCommit — intentional or missed?

crates/sdk/src/solidity.rs:30, crates/sdk/contracts/src/IOpenVmHalo2Verifier.sol:5, crates/sdk/contracts/template/OpenVmHalo2Verifier.sol:51,58,77,89,116, and the test contracts all still use appVmCommit. If the rename to "VK commit" is conceptual, the Solidity ABI should eventually match — but this is a breaking on-chain change. Worth adding a comment noting the divergence is intentional (deferred) or creating a follow-up ticket.

3. Duplicated key caching logic in commit.rs vs prove.rs

crates/cli/src/commands/commit.rs:91-134 duplicates the load-cached-keys + save-keys pattern from prove.rs's maybe_with_cached_keys() + save_keys(). Both functions are already pub(crate). Consider calling them from commit.rs to avoid the duplication. The blocker is that commit.rs doesn't have the same RunCargoArgs struct — but target_dir_from_cargo_args only needs manifest_path and target_dir, so a small adapter or extracting just those fields would work.

Fix this →

4. commit command lacks --agg-pk flag (inconsistency with prove)

prove stark and prove evm accept --agg-pk for custom aggregation key paths, but commit (which also needs agg/root keys to compute app_vk_commit) only looks at the default cached path. Users who pass custom --agg-pk to prove would expect the same for commit.

5. verify stark recomputes target_dir / manifest_path twice

crates/cli/src/commands/verify.rs:176-190: The Stark branch calls get_manifest_path_and_dir + get_target_dir at lines 176-177 for agg_vk_path, then again at lines 188-189 inside the else branch for baseline_path. The second call should reuse the values from the first.

Fix this →

6. check_solc_installed returns true on any successful spawn

crates/cli/src/commands/setup.rs:85-90: is_ok() checks that the process spawned, not that it succeeded. If solc exists but --version fails, this returns true. Use .output().map_or(false, |o| o.status.success()).

Fix this →

7. app_vk_commit() does not include internal_recursive_dag_commit

crates/sdk/src/prover/stark.rs:169-189: app_vk_commit() hashes app_dag_commit, leaf_dag_commit, and internal_for_leaf_dag_commit but omits the internal_recursive_dag_commit (which is included in generate_baseline() at line 161-164). Is this intentional? The baseline includes 4 DAG commits but the VK commit only covers 3. If the internal-recursive layer is deterministic from the other layers this may be correct, but it's worth a comment explaining the exclusion.

Minor / Nits

8. cli_init_build.sh fragile relative path

crates/cli/tests/scripts/cli_init_build.sh:13:

local_openvm_path="$(cd ../toolchain/openvm && pwd)"

This depends on cwd being crates/cli at script execution time. Consider using $CARGO_MANIFEST_DIR or computing relative to $0.

9. Shell scripts lack +x permission

The scripts in crates/cli/tests/scripts/ work because they're invoked via bash <script>, but conventionally they should be executable. Not a blocker.

10. with_agg_pk / with_root_pk panic on double-set

crates/sdk/src/lib.rs:671-682,685-707: These public SDK APIs use panic! when the OnceLock is already set. For library consumers beyond the CLI, returning a Result would be safer. This matches the existing with_app_pk pattern so it's not new to this PR, but worth noting as the API surface grows.

Positive Highlights

  • The setup simplification (removing STARK keygen, making it EVM-only) is a big UX win.
  • Per-project key caching with automatic save-on-first-generate is clean.
  • The Halo2ProvingPinning custom serde implementation is well done.
  • Moving tests to shell scripts improves readability and independent runnability.
  • The hex_bytes32 deserializer tolerating both 0x-prefixed and raw hex is good forward/backward compat.
  • CI workflow update correctly addresses the review comment about keeping commented lines.

@github-actions
Copy link
Copy Markdown

group app.proof_time_ms app.cycles leaf.proof_time_ms
fibonacci 3,784 12,000,265 934
keccak 15,640 1,235,218 2,164
regex 1,415 4,136,694 366
ecrecover 661 122,348 273
pairing 918 1,745,757 283
kitchen_sink 2,374 154,763 406

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: ea2703b

Benchmark Workflow

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.

2 participants