Skip to content

bench: integrate Spark component benchmarks into bench_firo#1859

Open
navidR wants to merge 3 commits into
firoorg:masterfrom
navidR:dev/navidr/add-benchmark
Open

bench: integrate Spark component benchmarks into bench_firo#1859
navidR wants to merge 3 commits into
firoorg:masterfrom
navidR:dev/navidr/add-benchmark

Conversation

@navidR

@navidR navidR commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

This PR integrates Spark cryptographic component benchmarks for BPPlus, Grootle, Chaum, and Schnorr into the existing bench_firo runner behind BUILD_BENCH_SPARK, replacing the duplicate standalone benchmark harness and enabling benchmark builds in GitHub CI. It fixes the exposed BUILD_BENCH path by ensuring generated benchmark data directories exist, declaring target-owned link dependencies, propagating crash-hook wrapper link requirements from static libraries, and initializing runtime state needed by existing benchmarks, including mainnet chain params, secp256k1 verification context, and minimal InstantSend state for wallet coin selection. The Spark benchmark sources now use benchmark::State, valid secp_primitives types, and proof verification checks, while the legacy Bitcoin block benchmark keeps structural validation but skips Firo PoW for its foreign Bitcoin Core block vector.

@codeant-ai

codeant-ai Bot commented Jun 1, 2026

Copy link
Copy Markdown

User rahimi.nv@gmail.com does not have a PR Review subscription.

Go to Team management and add this email to the PR Review subscription.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: fd6126bc-c1b5-4dc7-8027-04cc15976214

📥 Commits

Reviewing files that changed from the base of the PR and between 4a8f36d and 95842af.

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci-master.yml is excluded by !**/*.yml
📒 Files selected for processing (12)
  • CMakeLists.txt
  • cmake/script/GenerateHeaderFromRaw.cmake
  • cmake/utilities.cmake
  • src/CMakeLists.txt
  • src/bench/CMakeLists.txt
  • src/bench/bench_bitcoin.cpp
  • src/bench/checkblock.cpp
  • src/bench/coin_selection.cpp
  • src/bench/spark_bpplus.cpp
  • src/bench/spark_chaum.cpp
  • src/bench/spark_grootle.cpp
  • src/bench/spark_schnorr.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/bench/CMakeLists.txt
  • src/CMakeLists.txt
  • src/bench/spark_schnorr.cpp
  • cmake/script/GenerateHeaderFromRaw.cmake
  • CMakeLists.txt
  • src/bench/checkblock.cpp
  • cmake/utilities.cmake
  • src/bench/spark_chaum.cpp
  • src/bench/spark_bpplus.cpp

Summary by CodeRabbit

  • New Features

    • Added Spark micro-benchmarks measuring BP+, Chaum, Grootle, and Schnorr proof-system performance.
  • Chores

    • Introduced a configurable build option to enable/disable Spark benchmarks.
    • Improved build configuration and linking logic; enabled test support when benchmarks are built.
    • Enhanced benchmark data generation and framework setup; refined exception/link handling in the build.

Walkthrough

This PR introduces a comprehensive benchmark suite for four Spark cryptographic protocols by adding a build-time option (BUILD_BENCH_SPARK), refactoring CMake infrastructure for header generation and exception handling, updating the benchmark runner initialization, adapting existing benchmarks for compatibility, and implementing four new benchmark translation units (BP+, Chaum, Grootle, Schnorr) with deterministic test data generation and prove/verify measurements.

Changes

Spark Cryptographic Benchmarks

Layer / File(s) Summary
Build configuration and validation
CMakeLists.txt, src/CMakeLists.txt
Introduces BUILD_BENCH_SPARK cache option (disabled during fuzzing), gates CTest activation by BUILD_TESTS OR BUILD_BENCH, validates BUILD_BENCH_SPARK requires BUILD_BENCH=ON, and reports the option in the configuration summary.
CMake generator and utility updates
cmake/script/GenerateHeaderFromRaw.cmake, cmake/utilities.cmake
Refactors GenerateHeaderFromRaw.cmake to generate inline constexpr std::array<std::byte, N> instead of span plus raw array, and updates apply_wrapped_exception_flags() to apply exception link options (not compile options) with target-type-aware propagation (INTERFACE for static libraries, PRIVATE otherwise).
Benchmark build configuration
src/bench/CMakeLists.txt, src/CMakeLists.txt
Links bench_firo against Boost::filesystem and secp256k1pp, reorders optional libevent generator expressions, and conditionally includes Spark benchmark sources (spark_bpplus.cpp, spark_chaum.cpp, spark_grootle.cpp, spark_schnorr.cpp) when BUILD_BENCH_SPARK=ON.
Benchmark runner initialization
src/bench/bench_bitcoin.cpp
Selects MAIN chain parameters, sets up ECCVerifyHandle, parses -sanity-check to set elapsed_time_for_one (1.0 or 0.001), disables debug log output, and passes this value to BenchRunner::RunAll().
Existing benchmark compatibility updates
src/bench/checkblock.cpp, src/bench/coin_selection.cpp, src/bench/mempool_eviction.cpp
Updates checkblock to use benchmark::data::block413567 with explicit rewind assertions and a modified CheckBlock call; adds ScopedInstantSendManager RAII helper to coin_selection to wire the global InstantSend manager during the benchmark; removes deprecated dPriority parameter from mempool_eviction.
Spark BP+ benchmarks
src/bench/spark_bpplus.cpp
Implements BPPlusTestData (generator initialization, commitment precomputation), BPPlusBatchData (batched proof generation), benchmark helpers for prove/verify/batch-verify, and entrypoints for bit lengths 32/64/128 with output counts 1/2/4/8 and batch sizes.
Spark Chaum benchmarks
src/bench/spark_chaum.cpp
Implements ChaumTestData (random group elements, commitment vectors), benchmark helpers for prove/verify, and entrypoints for 1/2/4/8 commitments.
Spark Grootle benchmarks
src/bench/spark_grootle.cpp
Implements GrootleTestData and GrootleBatchData (parameter randomization, proof precomputation), prove/verify helpers parametrized by (n,m), batch verification support, and benchmark entrypoints across multiple parameter sets.
Spark Schnorr benchmarks
src/bench/spark_schnorr.cpp
Implements test data generation with random secret scalars and public keys, prove/verify helpers for 1/2/4/8 keys using single-key or multi-key library APIs, and corresponding benchmark entrypoints.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • levonpetrosyan93
  • psolstice
  • aleflm
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.99% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main objective of the PR: integrating Spark component benchmarks into the existing bench_firo benchmark runner.
Description check ✅ Passed The description includes a detailed PR intention explaining the changes, benefits (GitHub CI support), and technical details about runtime initialization and validation logic modifications.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bench/spark_chaum.cpp`:
- Around line 57-63: The loop currently calls x[i].randomize() then uses
x[i].inverse() to compute T[i], which can fail if x[i]==0; modify the loop that
sets x[i] so it guarantees a non-zero scalar before computing T[i] (e.g.,
replace x[i].randomize() with a non-zero sampler or add a small retry loop that
calls x[i].randomize() until !x[i].isZero()), then compute T[i] = (U + G *
y[i].negate()) * x[i].inverse(); reference symbols: x, x[i].randomize(),
x[i].inverse(), T, y, z.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2f4d321f-2c9a-46f7-998d-097828d5c4b7

📥 Commits

Reviewing files that changed from the base of the PR and between 5906886 and 4a8f36d.

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci-master.yml is excluded by !**/*.yml
📒 Files selected for processing (13)
  • CMakeLists.txt
  • cmake/script/GenerateHeaderFromRaw.cmake
  • cmake/utilities.cmake
  • src/CMakeLists.txt
  • src/bench/CMakeLists.txt
  • src/bench/bench_bitcoin.cpp
  • src/bench/checkblock.cpp
  • src/bench/coin_selection.cpp
  • src/bench/mempool_eviction.cpp
  • src/bench/spark_bpplus.cpp
  • src/bench/spark_chaum.cpp
  • src/bench/spark_grootle.cpp
  • src/bench/spark_schnorr.cpp

Comment thread src/bench/spark_chaum.cpp
Comment on lines +57 to +63
for (std::size_t i = 0; i < n; ++i) {
x[i].randomize();
y[i].randomize();
z[i].randomize();

S[i] = F * x[i] + G * y[i] + H * z[i];
T[i] = (U + G * y[i].negate()) * x[i].inverse();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid inverting a possibly zero scalar.

Line 63 takes x[i].inverse() immediately after randomization. If x[i] is ever zero, this benchmark can generate invalid inputs or fail nondeterministically in CI. Please resample or otherwise enforce non-zero x[i] before computing T[i].

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bench/spark_chaum.cpp` around lines 57 - 63, The loop currently calls
x[i].randomize() then uses x[i].inverse() to compute T[i], which can fail if
x[i]==0; modify the loop that sets x[i] so it guarantees a non-zero scalar
before computing T[i] (e.g., replace x[i].randomize() with a non-zero sampler or
add a small retry loop that calls x[i].randomize() until !x[i].isZero()), then
compute T[i] = (U + G * y[i].negate()) * x[i].inverse(); reference symbols: x,
x[i].randomize(), x[i].inverse(), T, y, z.

@navidR navidR force-pushed the dev/navidr/add-benchmark branch from 4a8f36d to 95842af Compare June 1, 2026 20:59
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