feat: benchmark use integration test and CRISP for gas [skip-line-limit]#1528
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughExtends benchmarking pipeline and artifacts: adds report-only regeneration and recovery scripts, records integration/multithread timings and folded artifacts, updates benchmark reports/JSONs, makes keyshare collection timeouts configurable via env vars, and exposes multithread timing APIs for reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant BenchScript as Extract Script
participant Hardhat as Hardhat Benchmark
participant Generator as generate_report.sh
participant Files as Results Dir
Test->>BenchScript: run integration with BENCHMARK_SUMMARY_OUTPUT
BenchScript->>Files: write integration_summary.json (folded artifacts + timings)
BenchScript->>Hardhat: (optional) estimateGas using folded proof hex
Hardhat-->>BenchScript: gas estimates (dkg/dec)
BenchScript->>Files: update crisp_verify_gas.json with sizes/calldata/gas
Generator->>Files: read crisp_verify_gas.json + integration_summary.json
Generator->>Files: write results_/report.md (integrated timings & tables)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 15 minutes and 27 seconds.Comment |
0xjei
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/keyshare/src/threshold_share_collector.rs (1)
38-48: Extract the timeout-env parsing into one shared helper.This
unset/unparseable/0 => defaultlogic is now duplicated across all three collector actors. Pulling it into a smallkeysharehelper will keep the fallback semantics and logging from drifting the next time one collector changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_share_collector.rs` around lines 38 - 48, Extract the env-parsing logic in collection_timeout() into a shared helper (e.g., keyshare::env::parse_collection_timeout or keyshare::collection::collection_timeout_from_env) that encapsulates the constant COLLECTION_TIMEOUT_ENV, the parse-to-u64, and the unset/unparseable/0 => DEFAULT_COLLECTION_TIMEOUT fallback, then replace the current collection_timeout() implementations in each collector actor to call that helper so all three use the same logic and semantics; ensure the helper returns a Duration and preserve existing behavior and logging semantics.circuits/benchmarks/scripts/replay_folded_verify_gas.sh (1)
53-56: Validation could also check forpublic_inputs_hexfields.The validation only checks for
proof_hexexistence but the Hardhat script also requirespublic_inputs_hex(seebenchmarkGasFromRaw.tslines 72-74). Consider extending the check.♻️ Proposed fix
-if ! jq -e '.folded_artifacts.dkg_aggregator.proof_hex and .folded_artifacts.decryption_aggregator.proof_hex' "$SUMMARY_JSON" >/dev/null 2>&1; then - echo "Error: $SUMMARY_JSON must contain .folded_artifacts with dkg_aggregator and decryption_aggregator proof_hex" +if ! jq -e '.folded_artifacts.dkg_aggregator.proof_hex and .folded_artifacts.dkg_aggregator.public_inputs_hex and .folded_artifacts.decryption_aggregator.proof_hex and .folded_artifacts.decryption_aggregator.public_inputs_hex' "$SUMMARY_JSON" >/dev/null 2>&1; then + echo "Error: $SUMMARY_JSON must contain .folded_artifacts with dkg_aggregator and decryption_aggregator proof_hex and public_inputs_hex" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/replay_folded_verify_gas.sh` around lines 53 - 56, Extend the jq validation that reads SUMMARY_JSON to also require the public_inputs_hex fields alongside proof_hex: update the test that currently checks '.folded_artifacts.dkg_aggregator.proof_hex and .folded_artifacts.decryption_aggregator.proof_hex' to include '.folded_artifacts.dkg_aggregator.public_inputs_hex' and '.folded_artifacts.decryption_aggregator.public_inputs_hex' so both proof_hex and public_inputs_hex are present for dkg_aggregator and decryption_aggregator before proceeding; keep using the same jq invocation and error message but mention both fields (proof_hex and public_inputs_hex) in the echo for clarity.circuits/benchmarks/scripts/generate_report.sh (1)
39-69: Consider extracting shared Python helpers to reduce duplication.
hex_len_bytes()andcalldata_gas_from_hex()are duplicated inextract_crisp_verify_gas.sh(lines 138-168). For maintainability, consider sourcing these from a shared helper script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 39 - 69, The two Python-embedded bash functions hex_len_bytes() and calldata_gas_from_hex() are duplicated; extract them into a shared helper script that defines those functions (same names) and then source that helper from both generate_report.sh and extract_crisp_verify_gas.sh before any calls; ensure the helper contains the two functions exactly as used, is readable by both scripts, and update both scripts to remove the local duplicates and add a single source (e.g., . ./shared_hex_helpers.sh) so they call the shared implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Line 433: Remove the unused variable p3_artifact_proof declared in the script
(the empty assignment p3_artifact_proof=""); either delete that line entirely
from the generate_report.sh script or, if it was intended to hold a value,
replace it with the correct assignment and ensure subsequent code uses that
symbol (search for p3_artifact_proof to confirm). Keep the change limited to
removing or properly initializing the p3_artifact_proof variable so there are no
leftover, unused declarations.
In `@crates/tests/tests/integration.rs`:
- Around line 847-854: The benchmark mode currently only switches BfvPreset
(BENCHMARK_MODE -> BfvPreset) while other parameters (lambda, preset subdir,
timeouts) remain hard-coded or taken from DEFAULT_BFV_PRESET, causing mixed
parameter families; create a single helper (e.g., select_benchmark_params or
similar) that reads BENCHMARK_MODE and returns a complete parameter bundle
{preset_subdir, bfv_preset (BfvPreset), lambda, timeouts} and update callers
such as the setup_test_zk_backend() usage site and any places referencing
DEFAULT_BFV_PRESET or the hard-coded lambda to consume this bundle so a secure
run uses the secure preset_subdir/lambda/timeouts and an insecure run uses the
insecure fixtures consistently.
- Around line 855-861: Save the previous values of the three environment
variables (E3_ENCRYPTION_KEY_COLLECTION_TIMEOUT_SECS,
E3_THRESHOLD_SHARE_COLLECTION_TIMEOUT_SECS,
E3_DECRYPTION_KEY_SHARED_COLLECTION_TIMEOUT_SECS) before calling
std::env::set_var in the test (e.g., in test_trbfv_actor), and restore them when
the test exits (restore with std::env::set_var if a previous value existed, or
std::env::remove_var if it did not). Implement this either by capturing
std::env::var_os for each var and restoring at the end of the test or by adding
a small RAII guard (scoped struct) that records original Option<OsString> values
and restores/removes them in Drop so the environment is always restored even on
early returns/panics.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 39-69: The two Python-embedded bash functions hex_len_bytes() and
calldata_gas_from_hex() are duplicated; extract them into a shared helper script
that defines those functions (same names) and then source that helper from both
generate_report.sh and extract_crisp_verify_gas.sh before any calls; ensure the
helper contains the two functions exactly as used, is readable by both scripts,
and update both scripts to remove the local duplicates and add a single source
(e.g., . ./shared_hex_helpers.sh) so they call the shared implementations.
In `@circuits/benchmarks/scripts/replay_folded_verify_gas.sh`:
- Around line 53-56: Extend the jq validation that reads SUMMARY_JSON to also
require the public_inputs_hex fields alongside proof_hex: update the test that
currently checks '.folded_artifacts.dkg_aggregator.proof_hex and
.folded_artifacts.decryption_aggregator.proof_hex' to include
'.folded_artifacts.dkg_aggregator.public_inputs_hex' and
'.folded_artifacts.decryption_aggregator.public_inputs_hex' so both proof_hex
and public_inputs_hex are present for dkg_aggregator and decryption_aggregator
before proceeding; keep using the same jq invocation and error message but
mention both fields (proof_hex and public_inputs_hex) in the echo for clarity.
In `@crates/keyshare/src/threshold_share_collector.rs`:
- Around line 38-48: Extract the env-parsing logic in collection_timeout() into
a shared helper (e.g., keyshare::env::parse_collection_timeout or
keyshare::collection::collection_timeout_from_env) that encapsulates the
constant COLLECTION_TIMEOUT_ENV, the parse-to-u64, and the unset/unparseable/0
=> DEFAULT_COLLECTION_TIMEOUT fallback, then replace the current
collection_timeout() implementations in each collector actor to call that helper
so all three use the same logic and semantics; ensure the helper returns a
Duration and preserve existing behavior and logging semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea2db083-8ee8-40e4-b4f0-c86a50504c26
📒 Files selected for processing (16)
circuits/benchmarks/README.mdcircuits/benchmarks/config.jsoncircuits/benchmarks/regenerate_report.shcircuits/benchmarks/results_insecure/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure/integration_summary.jsoncircuits/benchmarks/results_insecure/report.mdcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/generate_report.shcircuits/benchmarks/scripts/regenerate_report.shcircuits/benchmarks/scripts/replay_folded_verify_gas.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/encryption_key_collector.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/report.rscrates/tests/tests/integration.rs
💤 Files with no reviewable changes (1)
- circuits/benchmarks/config.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/keyshare/src/encryption_key_collector.rs (1)
23-33: Consider sharing the timeout parser with the other collectors.This is the same env-var fallback logic used elsewhere in
crates/keyshare; a shared helper would keep the three startup timeouts from drifting apart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/encryption_key_collector.rs` around lines 23 - 33, The env-var parsing/fallback logic in collection_timeout() (using COLLECTION_TIMEOUT_ENV and DEFAULT_COLLECTION_TIMEOUT) is duplicated across collectors—extract that logic into a shared helper (e.g. parse_timeout_from_env or env_timeout_or_default) in a common module and have collection_timeout() call it; update the other collectors to use the same helper so the E3_ENCRYPTION_KEY_COLLECTION_TIMEOUT_SECS parsing and defaulting behavior is centralized and consistent across all startup timeouts.circuits/benchmarks/scripts/generate_report.sh (1)
39-69: Consider centralizing hex size/calldata helper logic.Line 39–Line 69 duplicates logic now also present in
circuits/benchmarks/scripts/extract_crisp_verify_gas.sh. Keeping one shared helper path would reduce drift risk between extraction and report regeneration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` around lines 39 - 69, The duplicated hex helper functions hex_len_bytes and calldata_gas_from_hex should be moved into a single shared helper script (e.g., create a scripts/shared_hex_utils.sh) and both generate_report.sh and extract_crisp_verify_gas.sh should source that file instead of defining the functions inline; preserve the exact behavior (handling optional "0x", odd-length returns "0", and the same gas calculation) and ensure the functions are exported or declared so they remain callable from the existing shells that invoke them; update both callers to remove their local copies and add a single `source` line referencing the new shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/benchmarks/results_secure/crisp_verify_gas.json`:
- Around line 3-5: This JSON represents an incomplete benchmark snapshot — the
keys "dkg" and "dec" are null while "enclave_contracts" is 1 — so either
regenerate the secure run or exclude this snapshot from final comparisons; to
fix, locate the record containing the fields "dkg", "dec", and
"enclave_contracts" (the JSON object shown), then either (A) re-run the secure
benchmark to produce non-null numeric gas values for "dkg" and "dec" and replace
the file with the successful output, or (B) update the aggregation/filtering
logic to treat entries with null for "dkg" or "dec" as failed and omit them from
final results (or add an explicit "status":"failed" field) so this snapshot is
not included in benchmark comparisons.
---
Nitpick comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 39-69: The duplicated hex helper functions hex_len_bytes and
calldata_gas_from_hex should be moved into a single shared helper script (e.g.,
create a scripts/shared_hex_utils.sh) and both generate_report.sh and
extract_crisp_verify_gas.sh should source that file instead of defining the
functions inline; preserve the exact behavior (handling optional "0x",
odd-length returns "0", and the same gas calculation) and ensure the functions
are exported or declared so they remain callable from the existing shells that
invoke them; update both callers to remove their local copies and add a single
`source` line referencing the new shared helper.
In `@crates/keyshare/src/encryption_key_collector.rs`:
- Around line 23-33: The env-var parsing/fallback logic in collection_timeout()
(using COLLECTION_TIMEOUT_ENV and DEFAULT_COLLECTION_TIMEOUT) is duplicated
across collectors—extract that logic into a shared helper (e.g.
parse_timeout_from_env or env_timeout_or_default) in a common module and have
collection_timeout() call it; update the other collectors to use the same helper
so the E3_ENCRYPTION_KEY_COLLECTION_TIMEOUT_SECS parsing and defaulting behavior
is centralized and consistent across all startup timeouts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdea96d9-f494-4c36-885d-a9cedf0b81bc
📒 Files selected for processing (19)
circuits/benchmarks/README.mdcircuits/benchmarks/config.jsoncircuits/benchmarks/regenerate_report.shcircuits/benchmarks/results_insecure/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure/integration_summary.jsoncircuits/benchmarks/results_insecure/report.mdcircuits/benchmarks/results_secure/crisp_verify_gas.jsoncircuits/benchmarks/results_secure/integration_summary.jsoncircuits/benchmarks/results_secure/report.mdcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/generate_report.shcircuits/benchmarks/scripts/regenerate_report.shcircuits/benchmarks/scripts/replay_folded_verify_gas.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/encryption_key_collector.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/report.rscrates/tests/tests/integration.rs
💤 Files with no reviewable changes (1)
- circuits/benchmarks/config.json
✅ Files skipped from review due to trivial changes (2)
- circuits/benchmarks/regenerate_report.sh
- circuits/benchmarks/results_secure/integration_summary.json
🚧 Files skipped from review as they are similar to previous changes (6)
- circuits/benchmarks/scripts/regenerate_report.sh
- circuits/benchmarks/scripts/replay_folded_verify_gas.sh
- circuits/benchmarks/README.md
- circuits/benchmarks/results_insecure/integration_summary.json
- crates/tests/tests/integration.rs
- circuits/benchmarks/scripts/run_benchmarks.sh
cedoor
previously approved these changes
Apr 29, 2026
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
cedoor
approved these changes
Apr 29, 2026
This was referenced May 22, 2026
This was referenced Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores