fix: bake zk version.json into the binary#1359
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR simplifies ZkBackend construction and switches ZK configuration to an embedded compile-time versions.json. ZkBackend::new now takes three parameters; a new with_config exists for explicit configs. with_default_dir is now synchronous. Call sites and tests updated to the new API. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/zk-prover/src/config.rs (3)
351-412: Consider gating download integration tests with#[ignore].Both
test_download_and_verify_bbandtest_download_checksum_mismatch_on_corruptionfetch a full bb binary tarball over the network (120 s timeout). They run by default incargo test, which will slow CI on every PR.🔧 Suggested annotation
+ #[ignore = "requires network access; run with `cargo test -- --ignored`"] #[tokio::test] async fn test_download_and_verify_bb() {+ #[ignore = "requires network access; run with `cargo test -- --ignored`"] #[tokio::test] async fn test_download_checksum_mismatch_on_corruption() {Also applies to: 414-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/config.rs` around lines 351 - 412, The network-heavy integration tests test_download_and_verify_bb and test_download_checksum_mismatch_on_corruption should be gated so they don't run by default; add the #[ignore] attribute above each async test (e.g., annotate the #[tokio::test] functions test_download_and_verify_bb and test_download_checksum_mismatch_on_corruption with #[ignore] so developers/CI only run them explicitly via cargo test -- --ignored), leaving the existing timeout and test logic unchanged.
343-349:test_default_matches_versions_jsonis redundant withtest_zk_config_defaultand its name is misleading.Both tests assert non-emptiness of
required_bb_versionandbb_checksums. If the intent is to verify the embedded JSON round-trips correctly, the test should re-parseVERSIONS_JSONdirectly and assert field equality rather than just non-emptiness.♻️ Suggested consolidation
- #[test] - fn test_default_matches_versions_json() { - let config = ZkConfig::default(); - assert!(!config.required_bb_version.is_empty()); - assert!(!config.required_circuits_version.is_empty()); - assert!(!config.bb_checksums.is_empty()); - } + #[test] + fn test_default_matches_versions_json() { + let expected: ZkConfig = serde_json::from_str(VERSIONS_JSON).unwrap(); + let config = ZkConfig::default(); + assert_eq!(config.required_bb_version, expected.required_bb_version); + assert_eq!(config.required_circuits_version, expected.required_circuits_version); + assert_eq!(config.bb_checksums, expected.bb_checksums); + }And merge the
!bb_download_url.contains("{version}")assertion fromtest_zk_config_defaultinto this test to eliminate the overlap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/config.rs` around lines 343 - 349, The two tests test_default_matches_versions_json and test_zk_config_default overlap; remove the redundant test and consolidate its intent into a single clearer test: in the test that exercises ZkConfig::default (or rename to test_zk_config_roundtrip), parse VERSIONS_JSON directly (use the same deserialization used by ZkConfig::from_versions_json), then assert that the fields equal those in ZkConfig::default() (e.g., required_bb_version, required_circuits_version, bb_checksums), and also include the existing assertion that bb_download_url does not contain "{version}" (the !bb_download_url.contains("{version}") check). Ensure the new test name reflects round-trip/consistency and remove the old redundant test_default_matches_versions_json.
78-80: Misleading error message: the panic is runtime, not compile-time.
include_str!embeds the string at compile time, butserde_json::from_strdeserialises it at runtime. Ifversions.jsonis syntactically valid JSON but missing a requiredZkConfigfield (e.g., after a struct field is added without updating the file), the binary willpanic!on first use ofZkConfig::default(), not during compilation.🔧 Suggested fix
- serde_json::from_str(VERSIONS_JSON) - .expect("versions.json is invalid — this is a build-time bug") + serde_json::from_str(VERSIONS_JSON) + .expect("embedded versions.json failed to deserialize — update versions.json to match ZkConfig fields")If a true compile-time check is desired, consider adding a
build.rsthat validates the JSON against the expected structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/config.rs` around lines 78 - 80, The panic message on serde_json::from_str(VERSIONS_JSON).expect(...) is misleading (it says "build-time bug") because deserialization runs at runtime; update the expect message to accurately state this is a runtime deserialization error (e.g., "versions.json is invalid — runtime deserialization error") and mention the failing symbol VERSIONS_JSON/serde_json::from_str so future readers know where it originates; optionally, if you want true compile-time validation, add a build script (build.rs) that validates VERSIONS_JSON against ZkConfig before compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/zk-prover/src/config.rs`:
- Around line 351-412: The network-heavy integration tests
test_download_and_verify_bb and test_download_checksum_mismatch_on_corruption
should be gated so they don't run by default; add the #[ignore] attribute above
each async test (e.g., annotate the #[tokio::test] functions
test_download_and_verify_bb and test_download_checksum_mismatch_on_corruption
with #[ignore] so developers/CI only run them explicitly via cargo test --
--ignored), leaving the existing timeout and test logic unchanged.
- Around line 343-349: The two tests test_default_matches_versions_json and
test_zk_config_default overlap; remove the redundant test and consolidate its
intent into a single clearer test: in the test that exercises ZkConfig::default
(or rename to test_zk_config_roundtrip), parse VERSIONS_JSON directly (use the
same deserialization used by ZkConfig::from_versions_json), then assert that the
fields equal those in ZkConfig::default() (e.g., required_bb_version,
required_circuits_version, bb_checksums), and also include the existing
assertion that bb_download_url does not contain "{version}" (the
!bb_download_url.contains("{version}") check). Ensure the new test name reflects
round-trip/consistency and remove the old redundant
test_default_matches_versions_json.
- Around line 78-80: The panic message on
serde_json::from_str(VERSIONS_JSON).expect(...) is misleading (it says
"build-time bug") because deserialization runs at runtime; update the expect
message to accurately state this is a runtime deserialization error (e.g.,
"versions.json is invalid — runtime deserialization error") and mention the
failing symbol VERSIONS_JSON/serde_json::from_str so future readers know where
it originates; optionally, if you want true compile-time validation, add a build
script (build.rs) that validates VERSIONS_JSON against ZkConfig before
compilation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/zk-prover/src/backend/tests.rscrates/zk-prover/src/config.rscrates/zk-prover/src/prover.rs
|
this looks great. only issue for me is that tests are not passing with the new bb version being out of sync with what we have just upgraded to. (``3.0.0-nightly.20260102`) |
3 participants
This PR removes the runtime network fetch of versions.json from GitHub and the duplicate
BB_VERSION/CIRCUITS_VERSIONconstants, instead embedding versions.json into the binary at compile time viainclude_str!. It also simplifies the API by removing theZkConfigparameter fromZkBackend::new(), making version/checksum data an internal implementation detail locked to the CLI release.Summary by CodeRabbit
Refactor
Performance