Skip to content

test(zk): derive the Noir pin in the fresh config compile test - #2350

Merged
ctrlc03 merged 2 commits into
mainfrom
test/derive-noir-pin-benchmark-test
Oct 24, 2026
Merged

ctrlc03 merged 2 commits into
mainfrom
test/derive-noir-pin-benchmark-test

Conversation

@ctrlc03

@ctrlc03 ctrlc03 commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

What and why

This is a follow-up to the #2349 security review, finding L4. fresh_config_compilation_records_local_sources_not_hydrated_source_identity is #[ignore]d because it runs the real nargo, but it hard-coded 1.0.0-beta.26 as the Noir pin. Under the rc.3 toolchain it fails with Config compiler does not match Rust Noir 1.0.0-beta.26. The test now reads the pin from crates/zk/prover/Cargo.toml with e3_zk_build::pinned_noir_version, as the benchmark runner does (circuit_benchmark/mod.rs), so future toolchain bumps won't break it.

Review findings left unchanged:

  • L1 (noirup pinned by tag in releases.yml): already fixed in chore(zk)!: upgrade toolchain to Aztec v6.0.0 (bb 6.0.0, Noir rc.3) #2349 (0b2b25b7f).
  • L2 (proof rejection detected by string match): already covered against the real bb 6. rust:test:proofs panics if a VK-substitution proof comes back as anything other than ProofRejected, so a change in bb's wording fails CI. only_an_explicit_verifier_result_is_an_invalid_proof covers the matcher itself.
  • L3 (custom bb skips the checksum check): intended. An operator-supplied binary is trusted by design, and chore(zk)!: upgrade toolchain to Aztec v6.0.0 (bb 6.0.0, Noir rc.3) #2349 doesn't change that.
  • L4, second half (integration_child_uses_validated_bb_even_when_path_selects_another_binary): its 5.2.0 comes from synthetic shell stubs that print an arbitrary version, so the test still checks that E3_CUSTOM_BB is forwarded, independent of the real toolchain.
  • I1 (no test of standalone verify on an adversarial proof): still covered against the real bb 6. c0_local_verifier_failures_retry_without_peer_blame (CI zk_prover_e2e suites leg) verifies a proof with substituted public inputs and requires ProofVerificationFailed plus an accusation. That only happens when verify returns Ok(false). I also ran bb 6.0.0 verify on a proof with one byte flipped, on altered public inputs, and on a truncated proof. Each exits 1 with Proof verification failed, the string verify_proof_impl treats as an invalid proof.

Verification

  • cargo test --locked -p e3-zk-benchmarks -- --include-ignored: 15 passed. The fixed test failed before this change with nargo 1.0.0-rc.3.
  • cargo test --locked -p e3-zk-prover --lib c0_local_verifier_failures_retry_without_peer_blame: passed with bb 6.0.0 and the compiled C0 artifacts present, so it ran rather than skipping.
  • cargo fmt --check: clean.

Summary by CodeRabbit

  • Tests
    • Updated the fresh-configuration compilation check to use the Noir version pinned by the project, keeping the tested compiler version aligned with the project’s configuration.

The ignored benchmark test compiles with the real nargo but hard-coded 1.0.0-beta.26, so it failed after the rc.3 toolchain upgrade. Read the pin from crates/zk/prover/Cargo.toml as the benchmark runner does.
@vercel

vercel Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
interfold-dashboard Ready Ready Preview Oct 24, 2026 10:14pm UTC
interfold-docs Ready Ready Preview Oct 24, 2026 10:14pm UTC

Request Review

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 49 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 0b233e0 and 093f248.

Files selected for processing: 2
  • crates/zk/benchmarks/src/circuit_benchmark/mod.rs
  • crates/zk/benchmarks/src/circuit_benchmark/tests.rs

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 6c62fea and 0b233e0.

Files selected for processing: 1
  • crates/zk/benchmarks/src/circuit_benchmark/tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The fresh config compilation test now gets the pinned Noir version from the prover’s Cargo.toml. It passes that version to compile_config and artifacts::load instead of using a hard-coded version.

Changes: Benchmark test update

Layer / File(s) Summary
Derive and use the pinned Noir version
crates/zk/benchmarks/src/circuit_benchmark/tests.rs
The test derives the Noir version from the prover’s Cargo.toml and uses it for config compilation and artifact loading.

Priority: Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: neogiacomo

Merge Risk: Minimal · up to 0b233

No actionable merge-blocking risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: deriving the Noir pin in the fresh config compilation test.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ctrlc03
ctrlc03 merged commit 3b38b08 into main Oct 24, 2026
41 checks passed
@ctrlc03
ctrlc03 deleted the test/derive-noir-pin-benchmark-test branch October 24, 2026 22:45

This branch was successfully deployed

2 active deployments
Preview – interfold-dashboard — 093f248f Deployed Oct 24, 2026 by vercel[bot]
Preview – interfold-docs — 093f248f Deployed Oct 24, 2026 by vercel[bot]
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.

1 participant