feat: update circuits configs and benches [skip-line-limit]#1284
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a benchmarking framework under Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CI
participant Runner as run_benchmarks.sh
participant GenToml as generate_prover_toml.sh / zk_cli
participant Bench as benchmark_circuit.sh
participant Report as generate_report.sh
participant Repo as Repository (circuits/bin, targets)
rect rgba(100,150,240,0.5)
User->>Runner: invoke with --config/--mode/flags
end
Runner->>GenToml: generate Prover.toml for circuit+oracle
GenToml->>Repo: call zk_cli (write_toml) / emit Prover.toml
GenToml-->>Runner: Prover.toml path / status
Runner->>Bench: run benchmark_circuit.sh (per circuit/oracle)
Bench->>Repo: build/compile, execute, gate count, vk/proof gen, verify
Bench->>Repo: collect artifacts, timings, opcodes
Bench-->>Runner: write per-circuit JSON result
Runner->>Report: generate_report.sh --input-dir raw --output report.md --git-meta
Report-->>Runner: emit report.md
Runner-->>User: print final report path and raw data location
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
0xjei
force-pushed
the
circuits/configs-benches
branch
2 times, most recently
from
ecc9413 to
19a946c
Compare
cedoor
force-pushed
the
circuits/configs-benches
branch
from
19a946c to
2e3a98f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/circuits/dkg/pk/codegen.rs (1)
49-68:⚠️ Potential issue | 🔴 CriticalRevert the pk circuit codegen to use L_THRESHOLD instead of DKG counterpart parameters.
The pk circuit generates threshold public key components and should use L_THRESHOLD and QIS_THRESHOLD parameters. The PARITY_MATRIX in the Noir config (circuits/lib/src/configs/insecure/dkg.nr, line 26) is explicitly sized to
L_THRESHOLD, which equals 2 for insecure_512. However, the current codegen generatesglobal Lfrompreset.dkg_counterpart().metadata().num_moduli, which equals 1 (DKG parameter set), creating a dimension mismatch between the generated global and the circuit array bounds.This change should use the preset's own threshold metadata, not the DKG counterpart:
preset.metadata().degreefor Npreset.metadata().num_modulifor L (or directly reference L_THRESHOLD via the Noir config)The share_encryption circuit correctly uses DKG parameters (via dkg_counterpart), but pk must use threshold parameters by design.
🤖 Fix all issues with AI agents
In `@circuits/benchmarks/results_secure/report.md`:
- Line 45: The benchmark row for decrypted_shares_aggregation_mod shows
compilation metrics but zeroed execution and downstream metrics, indicating the
execution/proving step failed or was skipped; locate the benchmarking logic that
handles this circuit (search for decrypted_shares_aggregation_mod and the
runner/execute/prove functions used for circuits) and either re-run the
execution to capture proper opcodes/gates/witness/verification/proof timings or,
if execution reliably fails, update the report generation to mark this circuit
as "incomplete" and include the failure/error message so downstream metrics are
not shown as zeros; ensure the fix records and surfaces the actual execution
error instead of leaving zeros.
In `@circuits/benchmarks/scripts/benchmark_circuit.sh`:
- Around line 143-145: The use of date +%s.%N to compute END (and COMPILE_TIME
from START) is not portable on macOS; update the benchmark script to detect and
use a portable timestamp method: check whether date supports %N (or whether
gdate is available) and if not, fall back to a POSIX-safe approach (e.g., use
Python or Perl to emit high-resolution timestamps) so that the variables END,
START and COMPILE_TIME calculations (referenced around NARGO_COMPILE_CMD, START,
END, COMPILE_TIME) always produce a numeric fractional seconds value usable by
bc/awk.
- Around line 302-348: The heredoc that writes to OUTPUT_JSON currently inserts
many string variables raw (e.g., CIRCUIT_NAME, CIRCUIT_PATH_CLEAN, MODE,
ORACLE_TYPE, TIMESTAMP, OS_INFO, ARCH_INFO, CPU_MODEL, NARGO_VERSION,
BB_VERSION) which can break JSON if they contain quotes or control chars; update
the JSON emission in the OUTPUT_JSON heredoc so every string field is
JSON-escaped (use the same technique you used for GATES_OUTPUT and ERROR_MSG)
while leaving numeric/boolean fields unchanged, ensuring system_info and
top-level string fields are wrapped with a JSON-escaping pipeline so the
produced file is always valid JSON.
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 250-265: In the heredoc block that appends to "$OUTPUT_FILE" you
are calling format_time and format_bytes with unquoted variables (e.g., compile,
vk_gen, circuit_size, witness_size, vk_size, proof_size) which can trigger
word-splitting if a value has spaces or is empty; update those calls to pass
quoted variables to the helper functions (use quoted expansions for all
variables used inside format_time/format_bytes and other interpolations like
opcodes/gates) so each value is treated as a single argument and empty strings
don’t break the heredoc output.
In `@circuits/benchmarks/scripts/run_benchmarks.sh`:
- Line 114: Compute counts for CIRCUITS and ORACLES separately (e.g.,
CIRCUITS_COUNT=$(echo "$CIRCUITS" | wc -w | tr -d ' ') and ORACLES_COUNT=$(echo
"$ORACLES" | wc -w | tr -d ' ')), then check if either is zero and if so print a
clear warning and exit non‑zero before computing TOTAL_BENCHMARKS; replace the
single TOTAL_BENCHMARKS=... line with this guard so functions/loops that use
TOTAL_BENCHMARKS (and the progress banner that shows "[1/0]") never run when
there are no benchmarks and the report step won’t operate on an empty raw/
directory.
- Around line 74-77: The script currently does an unconditional cd to
"${BENCHMARKS_DIR}/${BIN_DIR}" when setting CIRCUITS_BASE_DIR which will cause
an opaque failure under set -e if that path doesn't exist; before attempting the
cd in run_benchmarks.sh check that the target directory exists (test -d
"${BENCHMARKS_DIR}/${BIN_DIR}") and if not, emit a clear error via
echo/processLogger (or >&2) naming BENCHMARKS_DIR and BIN_DIR and exit with
non-zero status; then perform the cd/resolve to set CIRCUITS_BASE_DIR only after
the existence check to avoid the cryptic shell "No such file or directory"
error.
- Around line 16-28: The --config and --mode case arms must validate that a
value is present before reading $2; modify the --config and --mode branches to
first check that there is a next argument (e.g., test if $# -lt 2 or if "$2" is
empty or starts with '-') and print a clear error and exit if missing, otherwise
assign CONFIG_FILE="$2" or MODE_OVERRIDE="$2" and then perform the existing mode
validation for MODE_OVERRIDE; reference the case labels handling --config and
--mode and the variables CONFIG_FILE and MODE_OVERRIDE to locate where to add
this guard.
🧹 Nitpick comments (8)
crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (1)
78-111: Duplicate.map_errchains on the sameResult.Lines 80–91 and 100–111 each chain two identical
.map_err(|e| CircuitsErrors::Sample(...))calls. The second.map_erris a no-op since the first already converts the error. This appears to be pre-existing copy-paste duplication.♻️ Suggested cleanup (lines 78–91)
let esi_coeffs = trbfv .generate_smudging_error(sd.z as usize, sd.lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate smudging error: {:?}", - e - )) })?;♻️ Suggested cleanup (lines 98–111)
let esi_sss_u64 = share_manager .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng.clone()) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate error shares: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate error shares: {:?}", - e - )) })?;crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/codegen.rs (1)
100-108: Consider adding assertions for the newly emitted globals.The test verifies the
DecryptedSharesAggregationConfigs::new(QIS, PLAINTEXT_MODULUS, Q_INVERSE_MOD_T)call but doesn't directly assert that thepub global L,pub global QIS,pub global PLAINTEXT_MODULUS, andpub global Q_INVERSE_MOD_Tdeclarations are present in the output. Adding a couple ofassert!checks for these would catch regressions in the template more explicitly.crates/zk-helpers/src/circuits/dkg/pk/codegen.rs (1)
63-64: Extract DKG counterpart metadata to avoid duplicate unwrap.
preset.dkg_counterpart().unwrap().metadata()is called twice. Extracting it into a local variable improves readability and avoids the repeatedunwrap().♻️ Proposed refactor
pub fn generate_configs(preset: BfvPreset, bits: &Bits) -> CodegenConfigs { + let dkg_meta = preset.dkg_counterpart().unwrap().metadata(); format!( r#"pub global N: u32 = {}; pub global L: u32 = {}; /************************************ ------------------------------------- pk (CIRCUIT 0 - DKG BFV PUBLIC KEY) ------------------------------------- ************************************/ // pk - bit parameters pub global {}_BIT_PK: u32 = {}; "#, - preset.dkg_counterpart().unwrap().metadata().degree, - preset.dkg_counterpart().unwrap().metadata().num_moduli, + dkg_meta.degree, + dkg_meta.num_moduli, <PkCircuit as Circuit>::PREFIX, bits.pk_bit, ) }circuits/benchmarks/scripts/generate_prover_toml.sh (2)
93-95: Useread -rainstead of array-from-subshell to avoid word-splitting pitfalls (SC2207).The current pattern
ZK_ARGS=($(get_zk_args ...))relies on unquoted command substitution for word splitting, which is fragile if values ever contain spaces or glob characters.♻️ Proposed fix
-ZK_ARGS=($(get_zk_args "$CIRCUIT_PATH")) +read -ra ZK_ARGS <<< "$(get_zk_args "$CIRCUIT_PATH")" ZK_CIRCUIT="${ZK_ARGS[0]}" ZK_WITNESS="${ZK_ARGS[1]:-}"
99-107: Consider logging stderr separately from stdout.Line 105 merges stderr into stdout (
2>&1), which mixes zk_cli error messages with normal output. If the benchmark pipeline ever parses stdout, this could cause issues. For now this is fine given the script's usage context.circuits/benchmarks/scripts/benchmark_circuit.sh (2)
67-76:WORKSPACE_ROOTis assigned but never used.Lines 68 and 76 assign
WORKSPACE_ROOTbut it's never referenced anywhere in the script. This is dead code that should be removed.🧹 Proposed cleanup
# Determine target directory location # Check if we're in a workspace (target at parent level) or standalone (target in current dir) TARGET_DIR="target" -WORKSPACE_ROOT="$(pwd)" # Check if parent directory has a workspace Nargo.toml # This handles workspace setups (e.g. circuits/bin/dkg with parent Nargo.toml) if [ -f "../Nargo.toml" ]; then if grep -q "^\[workspace\]" "../Nargo.toml" 2>/dev/null; then # We're in a workspace, target is at workspace root TARGET_DIR="../target" - WORKSPACE_ROOT="$(cd .. && pwd)" echo "Detected workspace setup: target directory at ${TARGET_DIR}" fi else
143-143: Shared/tmpfiles create a race condition if benchmarks run concurrently.
/tmp/compile_output.txt,/tmp/execute_output.txt, etc. are hard-coded global paths. If two instances of this script run simultaneously, they'll clobber each other's output. Consider usingmktempinstead.🔧 Suggested approach
+TMPDIR_BENCH=$(mktemp -d) +trap 'rm -rf "$TMPDIR_BENCH"' EXIT + # 1. COMPILE ... - if $NARGO_COMPILE_CMD > /tmp/compile_output.txt 2>&1; then + if $NARGO_COMPILE_CMD > "$TMPDIR_BENCH/compile_output.txt" 2>&1; thenAlso applies to: 158-158, 167-167, 182-182
circuits/benchmarks/scripts/generate_report.sh (1)
45-78: SC2155: Declare and assign separately informat_bytes,format_time, andformat_gates.Per ShellCheck SC2155,
local var=$(cmd)masks the return value ofcmd. Ifbcorawkfails, the error is silently swallowed. Splitting declaration from assignment preserves the exit status.🔧 Example fix for format_bytes
format_bytes() { local bytes=$1 if [ "$bytes" -eq 0 ]; then echo "0 B" elif [ "$bytes" -lt 1024 ]; then echo "${bytes} B" elif [ "$bytes" -lt 1048576 ]; then - local kb=$(echo "scale=5; $bytes/1024" | bc | awk '{printf "%.2f", $0}') + local kb + kb=$(echo "scale=5; $bytes/1024" | bc | awk '{printf "%.2f", $0}') echo "${kb} KB" else - local mb=$(echo "scale=5; $bytes/1048576" | bc | awk '{printf "%.2f", $0}') + local mb + mb=$(echo "scale=5; $bytes/1048576" | bc | awk '{printf "%.2f", $0}') echo "${mb} MB" fi }
cedoor
left a comment
cedoor
left a comment
There was a problem hiding this comment.
LGTM, I removed dkg form _share_encryption bcs there's only 1 circuit with that name. pls add it again to make it consistent if you want to keep it.
yeah maybe is better to have it so the share is tied to DKG; otherwise could be interpreted for threshold |
0xjei
force-pushed
the
circuits/configs-benches
branch
from
7df9239 to
eb62e90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/circuits/dkg/pk/codegen.rs (1)
49-68:⚠️ Potential issue | 🔴 CriticalRevert to using threshold preset L parameter in pk codegen.
The pk circuit should use
L_THRESHOLDfrom the threshold preset, not the DKG counterpart's L. The design pattern is:
- pk and share_computation circuits use threshold's L and QIS (they generate components for threshold operations)
- share_encryption circuit uses DKG's L and QIS (for encrypting shares)
Currently, share_computation correctly imports
L as L_THRESHOLDfrom the threshold preset, while pk now incorrectly pulls L from the DKG counterpart. Forinsecure_512, this changes L from 2 (threshold) to 1 (DKG), altering the circuit semantics. Usepreset.metadata()for N and call the threshold parameters to get the correct L value, consistent with share_computation's pattern.
🤖 Fix all issues with AI agents
In `@circuits/benchmarks/scripts/benchmark_circuit.sh`:
- Line 143: The script currently writes all command outputs to hardcoded /tmp
files (e.g., /tmp/compile_output.txt) which will conflict when benchmarks run
concurrently; change the five uses (outputs for NARGO_COMPILE_CMD,
NARGO_EXECUTE_CMD, NARGO_VK_CMD, NARGO_PROVE_CMD, NARGO_VERIFY_CMD) to create
unique temp files using mktemp (or mktemp -t) for each output, redirect each
command to its own temp file, and ensure those temp files are removed at the end
(trap/cleanup) to avoid leaks.
🧹 Nitpick comments (6)
crates/zk-helpers/src/circuits/dkg/pk/codegen.rs (1)
63-64: Extract repeateddkg_counterpart().unwrap().metadata()into a local variable.
preset.dkg_counterpart().unwrap().metadata()is called twice in succession. Extracting it into a local binding improves readability and makes the precondition (threshold-only preset) explicit in one place.♻️ Suggested refactor
pub fn generate_configs(preset: BfvPreset, bits: &Bits) -> CodegenConfigs { + let dkg_meta = preset.dkg_counterpart().unwrap().metadata(); format!( r#"pub global N: u32 = {}; pub global L: u32 = {}; ... pub global {}_BIT_PK: u32 = {}; "#, - preset.dkg_counterpart().unwrap().metadata().degree, - preset.dkg_counterpart().unwrap().metadata().num_moduli, + dkg_meta.degree, + dkg_meta.num_moduli, <PkCircuit as Circuit>::PREFIX, bits.pk_bit, ) }circuits/benchmarks/scripts/generate_prover_toml.sh (1)
93-95: Useread -rainstead of unquoted command substitution to populate the array.The static analysis tool (SC2207) correctly flags that
ZK_ARGS=($(get_zk_args ...))relies on word-splitting. While it works here since the tokens are simple and controlled,read -rais the idiomatic safe alternative.♻️ Proposed fix
-ZK_ARGS=($(get_zk_args "$CIRCUIT_PATH")) -ZK_CIRCUIT="${ZK_ARGS[0]}" -ZK_WITNESS="${ZK_ARGS[1]:-}" +IFS=' ' read -ra ZK_ARGS <<< "$(get_zk_args "$CIRCUIT_PATH")" +ZK_CIRCUIT="${ZK_ARGS[0]}" +ZK_WITNESS="${ZK_ARGS[1]:-}"circuits/benchmarks/scripts/benchmark_circuit.sh (1)
67-82:WORKSPACE_ROOTis assigned but never used.The variable is set on lines 68 and 76 but never referenced afterward. Either remove it or use it (e.g., for the JSON report).
♻️ Proposed fix
TARGET_DIR="target" -WORKSPACE_ROOT="$(pwd)" # Check if parent directory has a workspace Nargo.toml if [ -f "../Nargo.toml" ]; then if grep -q "^\[workspace\]" "../Nargo.toml" 2>/dev/null; then TARGET_DIR="../target" - WORKSPACE_ROOT="$(cd .. && pwd)" echo "Detected workspace setup: target directory at ${TARGET_DIR}" fi else echo "Detected standalone project: target directory at ${TARGET_DIR}" ficircuits/benchmarks/scripts/generate_report.sh (2)
44-78: SC2155:localcombined with command substitution masks return values.In
format_bytes,format_time, andformat_gates, patterns likelocal kb=$(echo ... | bc ...)prevent checkingbc's exit status. In practice, this is low-risk here since inputs are controlled numeric values, but for robustness you could separate declaration and assignment.♻️ Example fix for one case
- local kb=$(echo "scale=5; $bytes/1024" | bc | awk '{printf "%.2f", $0}') + local kb + kb=$(echo "scale=5; $bytes/1024" | bc | awk '{printf "%.2f", $0}')
117-156: DKG and Threshold loop blocks are nearly identical — consider extracting a helper.The four loops (DKG timing, DKG sizes, Threshold timing, Threshold sizes) share the same structure with only the category filter differing. A
write_timing_table/write_size_tablefunction taking the category as a parameter would halve the code.Also applies to: 168-207
circuits/lib/src/configs/secure/dkg.nr (1)
125-132:SHARE_DECRYPTION_BIT_MSGis not prefixed, unlike the rest of the rename.The other share decryption constants in the threshold configs were renamed to
THRESHOLD_SHARE_DECRYPTION_*, and share encryption constants in DKG configs becameDKG_SHARE_ENCRYPTION_*. This constant at line 132 retains the old unprefixed form. The same pattern appears ininsecure/dkg.nrat line 122, so it's consistently unprefixed across both presets—but inconsistent with the PR's broader naming convention.Worth considering a rename (e.g.,
DKG_SHARE_DECRYPTION_BIT_MSG) for consistency, though this may be intentional per the PR discussion about the share_decryption naming scope.#!/bin/bash # Check all usages of SHARE_DECRYPTION_BIT_MSG to assess rename scope rg -n 'SHARE_DECRYPTION_BIT_MSG' --type-add 'noir:*.nr' --type noir
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 45-78: The format_bytes and format_gates functions currently
perform integer comparisons that will fail if the input is "null" or
non-numeric; update both functions (format_bytes and format_gates) to
validate/coerce their input first—check if the parameter matches a non-negative
integer (e.g., with a regex test) and if not set it to 0 (or a safe numeric
default), then proceed with the existing comparisons and formatting; ensure you
handle empty strings and the literal "null" as non-numeric cases so the [ ...
-eq ] and [ ... -ge ] tests never receive invalid operands.
- Around line 258-259: The heredoc that writes the Markdown table is inserting
literal double quotes around the variables, causing output like | **ACIR
Opcodes** | "2949141" |; fix it by removing the surrounding double-quote
characters so the variables are expanded without extra quotes (adjust the cat >>
... << INNER block where ${opcodes} and ${gates} are used), ensuring the lines
use ${opcodes} and ${gates} directly (no surrounding " characters) to match the
other summary tables.
🧹 Nitpick comments (4)
circuits/benchmarks/scripts/benchmark_circuit.sh (2)
106-114:WORKSPACE_ROOTis assigned but never used.
WORKSPACE_ROOTis set on lines 106 and 114 but is not referenced anywhere else in the script. This is dead code flagged by ShellCheck (SC2034).Suggested fix
TARGET_DIR="target" -WORKSPACE_ROOT="$(pwd)" # Check if parent directory has a workspace Nargo.toml if [ -f "../Nargo.toml" ]; then if grep -q "^\[workspace\]" "../Nargo.toml" 2>/dev/null; then # We're in a workspace, target is at workspace root TARGET_DIR="../target" - WORKSPACE_ROOT="$(cd .. && pwd)" echo "Detected workspace setup: target directory at ${TARGET_DIR}" fi else
370-400: Numeric JSON fields rely onbc/awkoutput being well-formed — consider guarding.If any timed stage encounters an error from
bc(e.g.,get_timestampreturned something unexpected for START or END), the*_TIMEvariables could contain non-numeric content, producing invalid JSON. The${VAR:-0}defaults only protect against empty/unset values, not malformed strings.A lightweight guard on the timing arithmetic (e.g., defaulting to
0ifbcfails) would make the JSON output more resilient:-COMPILE_TIME=$(echo "$END - $START" | bc | awk '{printf "%.9f", $0}') +COMPILE_TIME=$(echo "$END - $START" | bc 2>/dev/null | awk '{printf "%.9f", $0}') +[[ "$COMPILE_TIME" =~ ^[0-9]+\.[0-9]+$ ]] || COMPILE_TIME=0circuits/benchmarks/scripts/generate_report.sh (1)
216-268: DRY opportunity: repeated JSON extraction + table generation could be a function.The DKG and Threshold loops for both timing and size tables (lines 117–156, 168–207) follow the same pattern and only differ by category. Similarly, the detail section re-extracts the same fields. This is fine for now, but if more categories or metrics are added, extracting a shared function would reduce maintenance burden.
circuits/benchmarks/scripts/run_benchmarks.sh (1)
98-110: Secure-mode source patching with trap-based restore is a pragmatic but fragile approach.Modifying
configs/default/mod.nrin-place to swap insecure→secure configs is effective, but carries risk: if the process is killed withSIGKILL(or the machine crashes), the source file remains patched. Thetrap restore_default_mod EXITon line 159 correctly handles normal exits,set -efailures, and trappable signals, which covers the common cases.Consider adding a comment near the patch section noting this caveat, so future maintainers know to check
mod.nrif benchmarks end abnormally. Alternatively, you could check for (and clean up) stale.benchmark_backupfiles at script start.Suggested stale-backup check at startup
+# Detect stale backup from a previously interrupted secure benchmark +STALE_BACKUP="${REPO_ROOT}/circuits/lib/src/configs/default/mod.nr.benchmark_backup" +if [ -f "$STALE_BACKUP" ]; then + echo "⚠️ Found stale benchmark backup — restoring original mod.nr" + cp "$STALE_BACKUP" "${REPO_ROOT}/circuits/lib/src/configs/default/mod.nr" + rm -f "$STALE_BACKUP" +fi + # For secure mode, patch lib to use secure configs (restored at end)Also applies to: 151-159
0xjei
force-pushed
the
circuits/configs-benches
branch
from
5ce0610 to
0f18e3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@circuits/benchmarks/scripts/generate_prover_toml.sh`:
- Around line 93-95: The current array assignment ZK_ARGS=($(get_zk_args
"$CIRCUIT_PATH")) can hide failures because set -e doesn't apply inside that
substitution and is unquoted (ShellCheck SC2207); call get_zk_args and check its
exit status first, e.g. capture its output into a variable
(output="$(get_zk_args "$CIRCUIT_PATH")" || { echo "invalid circuit"; exit 1;
}), then populate the array safely with read -r -a ZK_ARGS <<< "$output" and
keep ZK_CIRCUIT="${ZK_ARGS[0]}" and ZK_WITNESS="${ZK_ARGS[1]:-}" as-is (they are
already quoted).
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Line 145: The jq extraction for total_gates should include a fallback like the
other field to avoid producing "null" and breaking format_gates; update the
assignments that set the gates variable (the jq calls reading
'.gates.total_gates') in generate_report.sh to use the same "// 0" fallback as
'.gates.acir_opcodes // 0' (also apply identical changes to the other
occurrences around the assignments at the places referencing json_file on the
later extraction sites), so that the gates variable always receives a numeric
default usable by format_gates.
🧹 Nitpick comments (3)
crates/zk-helpers/src/circuits/dkg/pk/codegen.rs (1)
63-64: Extract repeateddkg_counterpart().unwrap().metadata()into a local variable.
preset.dkg_counterpart().unwrap().metadata()is called twice. Extracting it avoids the duplicate chain and makes the intent clearer.♻️ Proposed refactor
pub fn generate_configs(preset: BfvPreset, bits: &Bits) -> CodegenConfigs { + let dkg_meta = preset.dkg_counterpart().unwrap().metadata(); format!( r#"pub global N: u32 = {}; pub global L: u32 = {}; /************************************ ------------------------------------- pk (CIRCUIT 0 - DKG BFV PUBLIC KEY) ------------------------------------- ************************************/ // pk - bit parameters pub global {}_BIT_PK: u32 = {}; "#, - preset.dkg_counterpart().unwrap().metadata().degree, - preset.dkg_counterpart().unwrap().metadata().num_moduli, + dkg_meta.degree, + dkg_meta.num_moduli, <PkCircuit as Circuit>::PREFIX, bits.pk_bit, ) }circuits/benchmarks/scripts/benchmark_circuit.sh (1)
106-114:WORKSPACE_ROOTis assigned but never used.ShellCheck SC2034 correctly identifies that
WORKSPACE_ROOT(line 106 and 114) is set but never referenced. If it's not needed externally, remove it to avoid confusion.🔧 Proposed fix
TARGET_DIR="target" -WORKSPACE_ROOT="$(pwd)" # Check if parent directory has a workspace Nargo.toml # This handles workspace setups (e.g. circuits/bin/dkg with parent Nargo.toml) if [ -f "../Nargo.toml" ]; then if grep -q "^\[workspace\]" "../Nargo.toml" 2>/dev/null; then # We're in a workspace, target is at workspace root TARGET_DIR="../target" - WORKSPACE_ROOT="$(cd .. && pwd)" echo "Detected workspace setup: target directory at ${TARGET_DIR}" fi elsecircuits/benchmarks/scripts/generate_report.sh (1)
146-149: Other size fields also lack// 0fallbacks.
circuit_size_bytes,witness_size_bytes,vk_size_bytes, andproof_size_bytescould also benullif a benchmark stage failed. These would similarly breakformat_bytes. Consider adding// 0to all numeric jq extractions for defensive consistency.Same applies to the identical block at lines 197-200 and 246-249.
🔧 Proposed fix example
- circuit_size=$(jq -r '.compilation.circuit_size_bytes' "$json_file") - witness_size=$(jq -r '.execution.witness_size_bytes' "$json_file") - vk_size=$(jq -r '.vk_generation.vk_size_bytes' "$json_file") - proof_size=$(jq -r '.proof_generation.proof_size_bytes' "$json_file") + circuit_size=$(jq -r '.compilation.circuit_size_bytes // 0' "$json_file") + witness_size=$(jq -r '.execution.witness_size_bytes // 0' "$json_file") + vk_size=$(jq -r '.vk_generation.vk_size_bytes // 0' "$json_file") + proof_size=$(jq -r '.proof_generation.proof_size_bytes // 0' "$json_file")
0xjei
force-pushed
the
circuits/configs-benches
branch
from
0f18e3f to
35eaffa
Compare
2 participants
Summary by CodeRabbit
New Features
New Tools
Refactor
Documentation
Chore