refactor: update benches to reflect the new metrics [skip-line-limit]#1527
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 (3)
📝 WalkthroughWalkthroughThis PR introduces benchmark gas estimation and reporting infrastructure for circuit verification. It adds scripts to extract CRISP verifier gas metrics, computes calldata gas from proof artifacts, generates on-chain gas estimates via verifier contract simulation, and replaces detailed benchmark reports with a condensed protocol-oriented format showing circuit constraints, performance, gas, and role/phase analysis. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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. Comment |
![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: 4
🧹 Nitpick comments (6)
crates/tests/tests/integration.rs (1)
1338-1377: Optional: simplify hex/JSON construction in the benchmark dump.The block is correct and only runs when
BENCHMARK_FOLDED_OUTPUTis set. The localto_hexhelper allocates per byte viaformat!; usinghex::encode(available in workspace) andserde_json::json!would reduce this to a few lines and avoid hand-rolled JSON construction. This requires addinghexandserde_jsonas workspace dependencies tocrates/tests/Cargo.toml, which is a minor overhead given both are widely used elsewhere in the codebase.♻️ Sketch
- fn to_hex(bytes: &[u8]) -> String { - let mut out = String::from("0x"); - for b in bytes { - out.push_str(&format!("{:02x}", b)); - } - out - } - let json = format!( - concat!( - "{{\n", - " \"dkg_aggregator\": {{\n", - ... - ), - to_hex(&dkg_proof.data), - ... - ); + let to_hex = |b: &[u8]| format!("0x{}", hex::encode(b)); + let json = serde_json::json!({ + "dkg_aggregator": { + "proof_hex": to_hex(&dkg_proof.data), + "public_inputs_hex": to_hex(&dkg_proof.public_signals), + }, + "decryption_aggregator": { + "proof_hex": to_hex(&dec_proof.data), + "public_inputs_hex": to_hex(&dec_proof.public_signals), + }, + }).to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 1338 - 1377, The benchmark dump builds hex strings with a local to_hex and hand-rolled JSON; replace that with hex::encode for byte->hex and serde_json::json! to build the JSON object and then write serde_json::to_string_pretty(...) to path (preserve the same fields: dkg_aggregator (dkg_aggregator_proof.data/public_signals) and decryption_aggregator (decryption_aggregator_proofs.first().data/public_signals)), updating the code that currently references to_hex, the BENCHMARK_FOLDED_OUTPUT branch, fs::write and the dkg_aggregator_proof / decryption_aggregator_proofs variables; also add hex and serde_json to crates/tests/Cargo.toml workspace dependencies.packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts (2)
35-52: Document the implicit "8 little-endian bytes per coefficient" contract.
plaintextHashFromPublicInputsassumes:
- the last 100 public inputs are message coefficients,
- each coefficient fits in 8 bytes,
- they should be packed little-endian before hashing.
These three assumptions must match the on-chain verifier's
plaintextHashderivation exactly; any divergence produces a hash mismatch andestimateGasreverts. A short comment referencing the source spec (and ideally a constant formessageCoeffsCount/bytesPerCoeff) would prevent silent breakage if the message layout changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts` around lines 35 - 52, plaintextHashFromPublicInputs currently embeds three implicit assumptions (last 100 inputs are message coefficients, each coefficient is 8 bytes, and coefficients are packed little-endian) which must exactly match the on-chain verifier; update the function by extracting messageCoeffsCount and bytesPerCoeff into named constants, add a concise comment referencing the verifier/spec that defines the packing/layout, and validate/guard against inputs that violate coeff size or layout so any divergence is obvious (use the constants inside plaintextHashFromPublicInputs to compute offset, buffer size, and byte shifts).
10-18:findRawJsonreturns the firstreaddirSyncmatch in OS-dependent order.
fs.readdirSyncdoes not guarantee any particular ordering across platforms/filesystems, so when multiple files inrawDirhappen to contain the same fragment (e.g.share_encryptionmatching variants), you may pick a different artifact between runs and ship inconsistent gas numbers in the report.Sort the entries before iterating, or assert that exactly one file matches.
♻️ Proposed fix
function findRawJson(rawDir: string, fragment: string): any { - const entries = fs.readdirSync(rawDir).filter((f) => f.endsWith(".json")); - for (const f of entries) { - if (!f.includes(fragment)) continue; - const full = path.join(rawDir, f); - return JSON.parse(fs.readFileSync(full, "utf8")); - } - throw new Error(`Missing raw benchmark JSON for fragment: ${fragment}`); + const entries = fs + .readdirSync(rawDir) + .filter((f) => f.endsWith(".json") && f.includes(fragment)) + .sort(); + if (entries.length === 0) { + throw new Error(`Missing raw benchmark JSON for fragment: ${fragment}`); + } + return JSON.parse(fs.readFileSync(path.join(rawDir, entries[0]), "utf8")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts` around lines 10 - 18, findRawJson currently returns the first filesystem entry in OS-dependent order which can yield inconsistent results when multiple JSON filenames contain the same fragment; update findRawJson to deterministically select the file by first filtering entries to those that include fragment, then sort that filtered list (or assert its length is 1) before picking the file to read and JSON.parse it; reference the function findRawJson and variables entries/rawDir/fragment when making the change so the selection is deterministic (e.g., sort the filtered filenames lexically or throw if multiple matches).circuits/benchmarks/scripts/extract_crisp_verify_gas.sh (2)
49-51:OUTPUT_DIRresolution fails opaquely if--output's parent directory doesn't exist.
cd "$(dirname "$OUTPUT_JSON")" && pwdaborts underset -ewith a confusingcd: No such file or directoryif the user passes a path whose parent (e.g.results_secure/) hasn't been created yet. A defensivemkdir -pplus a clearer error makes onboarding and CI failures easier to debug.♻️ Proposed fix
ENCLAVE_CONTRACTS_DIR="${REPO_ROOT}/packages/enclave-contracts" -OUTPUT_DIR="$(cd "$(dirname "$OUTPUT_JSON")" && pwd)" +OUTPUT_PARENT="$(dirname "$OUTPUT_JSON")" +mkdir -p "$OUTPUT_PARENT" +OUTPUT_DIR="$(cd "$OUTPUT_PARENT" && pwd)" RAW_DIR="${OUTPUT_DIR}/raw"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh` around lines 49 - 51, The script currently sets OUTPUT_DIR with OUTPUT_DIR="$(cd "$(dirname "$OUTPUT_JSON")" && pwd)" which fails with a cryptic "cd: No such file or directory" when the parent directory of --output (OUTPUT_JSON) does not exist; update the logic around OUTPUT_DIR/RAW_DIR to defensively create the parent directory (use mkdir -p on the dirname of OUTPUT_JSON before resolving OUTPUT_DIR) and then resolve OUTPUT_DIR, and when mkdir or cd fails emit a clear error mentioning OUTPUT_JSON so failures are easier to debug; ensure changes target the OUTPUT_DIR/RAW_DIR initialization in this script.
60-77: Three test stages run unconditionally even after upstream failure.The folded-export step depends on artifacts built in the previous compilation, and the enclave-contracts step (line 71) reads
BENCHMARK_FOLDED_JSON="$TMP_JSON_FOLDED"produced by the folded step. If the folded step fails, the third stage will hit thefindRawJson/folded-fallback branch inbenchmarkGasFromRaw.tsand likely throw — which is recorded as just another non-zero exit code in the JSON. Consider short-circuiting (or at least surfacing in the JSONnote) when an upstream stage fails, so consumers don't treat the resultingnullgas values as legitimate measurements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh` around lines 60 - 77, The script currently runs three stages unconditionally which lets downstream stages run with missing artifacts; after each stage run (check the exit codes CRISP_TEST_EXIT_CODE and FOLDED_TEST_EXIT_CODE) short-circuit on failure by either exiting immediately with a non-zero status or by creating a minimal JSON result that includes a clear "note" field and avoids invoking the next stage; specifically, update extract_crisp_verify_gas.sh to if CRISP_TEST_EXIT_CODE != 0 then write a folded/enclave JSON with a note and exit, and if FOLDED_TEST_EXIT_CODE != 0 then avoid calling the pnpm hardhat run that invokes benchmarkGasFromRaw.ts (which reads BENCHMARK_FOLDED_JSON="$TMP_JSON_FOLDED"), instead emit the appropriate TMP_JSON_ENCLAVE with a diagnostic note and set ENCLAVE_TEST_EXIT_CODE non-zero (or exit), so consumers don’t treat null/invalid gas values as valid.circuits/benchmarks/scripts/generate_report.sh (1)
242-242: Replacelswithfindto handle non-alphanumeric names safely (SC2012).Parsing
lsoutput is fragile when filenames contain unusual characters. Usefind(or anullglobglob) so the first JSON is selected reliably and predictably.♻️ Proposed fix
-first_json=$(ls "$INPUT_DIR"/*.json 2>/dev/null | head -1) +first_json=$(find "$INPUT_DIR" -maxdepth 1 -type f -name '*.json' -print 2>/dev/null | sort | head -n 1) if [ -n "$first_json" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/benchmarks/scripts/generate_report.sh` at line 242, The current use of ls to populate first_json is fragile for filenames with special characters; update the logic that sets the first_json variable (using INPUT_DIR) to use find instead of ls: run find on INPUT_DIR limited to the top-level (maxdepth 1) for files matching *.json and stop after the first match, ensuring you handle special characters safely (use a null-safe output like -print0 and read with a null delimiter or -print -quit depending on your shell) and preserve the existing behavior when no JSON exists.
🤖 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/README.md`:
- Around line 52-53: The README's row list uses `user-data-enc` but the report
generator (`generate_report.sh`) emits `user_data_encryption`; update the README
entry in the "Circuit Benchmarks" row list to `user_data_encryption` so it
matches the identifier emitted by generate_report.sh (or alternatively change
the three occurrences in generate_report.sh from `user_data_encryption` to
`user-data-enc` if you prefer the README version).
In `@circuits/benchmarks/results_secure/report.md`:
- Around line 5-6: The secure report
(circuits/benchmarks/results_secure/report.md) was regenerated on branch name
`fix/configs-circuit` (commit `eca330d9a…`) while the insecure report shows `Git
Branch: main`; to align provenance, regenerate the secure report on the merge
target (either regenerate both reports on `main` after merging or regenerate the
secure report on the merge branch that matches the insecure report) so the `Git
Branch:` and commit fields in report.md match across secure and insecure
reports; update the file header to reflect the correct branch/commit if you
regenerate.
In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh`:
- Around line 32-47: The script creates temporary files TMP_LOG_CRISP,
TMP_LOG_FOLDED, TMP_LOG_ENCLAVE, TMP_JSON_ENCLAVE, and TMP_JSON_FOLDED with
mktemp but only removes them later, causing leaks on early exits; add a cleanup
function (e.g., cleanup_tmp_files) that removes those five temp files and
register it with trap to run on EXIT (and optionally on ERR), then replace the
existing manual removals with a call to that cleanup or rely on the trap so any
early-return paths (including the CRISP_CONTRACTS_DIR not found branch and
failures during pnpm build:circuits) always remove the temp files. Ensure the
trap is set immediately after the mktemp lines and references the same TMP_*
variable names used in the script.
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Around line 216-222: The current run_benchmarks.sh flow can leave a stale
"${GAS_JSON_FILE}" from a prior successful run and then silently reuse it when
extract_crisp_verify_gas.sh fails; before invoking extract_crisp_verify_gas.sh
(or alternately write to a temp file and atomically mv on success), remove or
unlink the existing GAS_JSON_FILE so a failed extraction cannot leak old data;
update the block around GAS_JSON_FILE and the extract_crisp_verify_gas.sh call
(or adjust the post-extract move) to ensure generate_report.sh only sees a gas
JSON if the current extraction succeeded (refer to GAS_JSON_FILE,
extract_crisp_verify_gas.sh, generate_report.sh and the verify_gas_for_artifact
check).
---
Nitpick comments:
In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh`:
- Around line 49-51: The script currently sets OUTPUT_DIR with OUTPUT_DIR="$(cd
"$(dirname "$OUTPUT_JSON")" && pwd)" which fails with a cryptic "cd: No such
file or directory" when the parent directory of --output (OUTPUT_JSON) does not
exist; update the logic around OUTPUT_DIR/RAW_DIR to defensively create the
parent directory (use mkdir -p on the dirname of OUTPUT_JSON before resolving
OUTPUT_DIR) and then resolve OUTPUT_DIR, and when mkdir or cd fails emit a clear
error mentioning OUTPUT_JSON so failures are easier to debug; ensure changes
target the OUTPUT_DIR/RAW_DIR initialization in this script.
- Around line 60-77: The script currently runs three stages unconditionally
which lets downstream stages run with missing artifacts; after each stage run
(check the exit codes CRISP_TEST_EXIT_CODE and FOLDED_TEST_EXIT_CODE)
short-circuit on failure by either exiting immediately with a non-zero status or
by creating a minimal JSON result that includes a clear "note" field and avoids
invoking the next stage; specifically, update extract_crisp_verify_gas.sh to if
CRISP_TEST_EXIT_CODE != 0 then write a folded/enclave JSON with a note and exit,
and if FOLDED_TEST_EXIT_CODE != 0 then avoid calling the pnpm hardhat run that
invokes benchmarkGasFromRaw.ts (which reads
BENCHMARK_FOLDED_JSON="$TMP_JSON_FOLDED"), instead emit the appropriate
TMP_JSON_ENCLAVE with a diagnostic note and set ENCLAVE_TEST_EXIT_CODE non-zero
(or exit), so consumers don’t treat null/invalid gas values as valid.
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Line 242: The current use of ls to populate first_json is fragile for
filenames with special characters; update the logic that sets the first_json
variable (using INPUT_DIR) to use find instead of ls: run find on INPUT_DIR
limited to the top-level (maxdepth 1) for files matching *.json and stop after
the first match, ensuring you handle special characters safely (use a null-safe
output like -print0 and read with a null delimiter or -print -quit depending on
your shell) and preserve the existing behavior when no JSON exists.
In `@crates/tests/tests/integration.rs`:
- Around line 1338-1377: The benchmark dump builds hex strings with a local
to_hex and hand-rolled JSON; replace that with hex::encode for byte->hex and
serde_json::json! to build the JSON object and then write
serde_json::to_string_pretty(...) to path (preserve the same fields:
dkg_aggregator (dkg_aggregator_proof.data/public_signals) and
decryption_aggregator
(decryption_aggregator_proofs.first().data/public_signals)), updating the code
that currently references to_hex, the BENCHMARK_FOLDED_OUTPUT branch, fs::write
and the dkg_aggregator_proof / decryption_aggregator_proofs variables; also add
hex and serde_json to crates/tests/Cargo.toml workspace dependencies.
In `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts`:
- Around line 35-52: plaintextHashFromPublicInputs currently embeds three
implicit assumptions (last 100 inputs are message coefficients, each coefficient
is 8 bytes, and coefficients are packed little-endian) which must exactly match
the on-chain verifier; update the function by extracting messageCoeffsCount and
bytesPerCoeff into named constants, add a concise comment referencing the
verifier/spec that defines the packing/layout, and validate/guard against inputs
that violate coeff size or layout so any divergence is obvious (use the
constants inside plaintextHashFromPublicInputs to compute offset, buffer size,
and byte shifts).
- Around line 10-18: findRawJson currently returns the first filesystem entry in
OS-dependent order which can yield inconsistent results when multiple JSON
filenames contain the same fragment; update findRawJson to deterministically
select the file by first filtering entries to those that include fragment, then
sort that filtered list (or assert its length is 1) before picking the file to
read and JSON.parse it; reference the function findRawJson and variables
entries/rawDir/fragment when making the change so the selection is deterministic
(e.g., sort the filtered filenames lexically or throw if multiple matches).
🪄 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: 8162c52c-5086-4cb8-830b-fde1e985f344
📒 Files selected for processing (13)
circuits/benchmarks/README.mdcircuits/benchmarks/results_insecure/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure/report.mdcircuits/benchmarks/results_secure/crisp_verify_gas.jsoncircuits/benchmarks/results_secure/report.mdcircuits/benchmarks/scripts/benchmark_circuit.shcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/generate_report.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/tests/tests/integration.rsexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tspackages/enclave-contracts/scripts/benchmarkGasFromRaw.tspackages/enclave-contracts/tasks/enclave.ts
This was referenced May 18, 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.
This PR updates the previous benchmark scripts for the circuits to record the new metrics. Note that the reports, both secure and insecure, have been regenerated following this metric.
Summary by CodeRabbit
Documentation
New Features