Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds DKG fold attestation support across Rust and Solidity flows, updates committee ordering and slashing evidence handling, expands benchmark generation and reporting, and refreshes integration, template, and CRISP local-development wiring to support proof-aggregation and non-aggregation modes. ChangesEnd-to-end DKG fold attestations, committee publication, and slashing evidence
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
circuits/benchmarks/scripts/generate_report.sh (2)
548-551:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard jq reads when integration blob is absent.
Lines 549–551 run jq on
$blobeven when it’s empty; underset -ethis can terminate report generation in no-summary runs.Suggested fix
- local nodes_spawned network_model testmode_harness - nodes_spawned=$(jq -r '.benchmark_config.nodes_spawned // empty' <<<"$blob") - network_model=$(jq -r '.benchmark_config.network_model // empty' <<<"$blob") - testmode_harness=$(jq -r 'if .benchmark_config.testmode_harness != null then .benchmark_config.testmode_harness else empty end' <<<"$blob") + local nodes_spawned network_model testmode_harness + nodes_spawned="" + network_model="" + testmode_harness="" + if [ -n "$blob" ]; then + nodes_spawned=$(jq -r '.benchmark_config.nodes_spawned // empty' <<<"$blob" 2>/dev/null || true) + network_model=$(jq -r '.benchmark_config.network_model // empty' <<<"$blob" 2>/dev/null || true) + testmode_harness=$(jq -r 'if .benchmark_config.testmode_harness != null then .benchmark_config.testmode_harness else empty end' <<<"$blob" 2>/dev/null || true) + fi🤖 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 `@circuits/benchmarks/scripts/generate_report.sh` around lines 548 - 551, The jq reads on $blob (populating nodes_spawned, network_model, testmode_harness) must be guarded so they don't run when $blob is empty; modify generate_report.sh to check if $blob is non-empty (e.g., if [ -n "$blob" ] or similar) before calling jq and otherwise set nodes_spawned, network_model, and testmode_harness to empty/default values, ensuring under set -e the script won't exit when there is no integration blob.
503-503:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
phase_timingsfallback for “Entire Test” extraction.Line 503 only reads
.timings_seconds; for current summaries that use.phase_timings, the run-config wall-clock row can be silently omitted.Suggested fix
- entire_test_sec=$(jq -r '.timings_seconds[]? | select(.label == "Entire Test") | .seconds' <<<"$blob" | head -1) + entire_test_sec=$(jq -r ' + ((.phase_timings // .timings_seconds // [])[] + | select(.label == "Entire Test") + | if type == "object" then .seconds else . end) // empty + ' <<<"$blob" | head -1)🤖 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 `@circuits/benchmarks/scripts/generate_report.sh` at line 503, The extraction for entire_test_sec only looks at .timings_seconds and misses summaries that use .phase_timings; update the logic that computes entire_test_sec to fall back to .phase_timings when the jq query on .timings_seconds returns empty (i.e., check .timings_seconds[]? | select(.label == "Entire Test") | .seconds first, and if not present run the same select against .phase_timings[]?); reference the existing variable name entire_test_sec and the jq invocation that queries ".timings_seconds[]? | select(.label == \"Entire Test\") | .seconds" and ensure the final value is set from either source.circuits/benchmarks/scripts/replay_folded_verify_gas.sh (1)
63-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty folded proof hex in preflight validation.
Line 63 uses jq truthiness, so empty strings can pass the check and cause invalid replay input.
Suggested fix
-if ! jq -e '.folded_artifacts.dkg_aggregator.proof_hex and .folded_artifacts.decryption_aggregator.proof_hex' "$SUMMARY_JSON" >/dev/null 2>&1; then +if ! jq -e ' + (.folded_artifacts.dkg_aggregator.proof_hex? | type == "string" and length > 2) and + (.folded_artifacts.decryption_aggregator.proof_hex? | type == "string" and length > 2) +' "$SUMMARY_JSON" >/dev/null 2>&1; then🤖 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 `@circuits/benchmarks/scripts/replay_folded_verify_gas.sh` around lines 63 - 64, The preflight jq check using SUMMARY_JSON currently only tests truthiness so empty strings for .folded_artifacts.dkg_aggregator.proof_hex or .folded_artifacts.decryption_aggregator.proof_hex can pass; change the jq expression used in the if to explicitly reject empty strings (e.g. test that both fields exist and are not "" or that their length > 0) so the script echoes the error and exits when either proof_hex is empty; update the condition that references SUMMARY_JSON and the jq path .folded_artifacts.dkg_aggregator.proof_hex / .folded_artifacts.decryption_aggregator.proof_hex accordingly.crates/zk-prover/src/actors/accusation_manager.rs (2)
765-920:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound incoming accusation deadlines to the configured validity window.
verify_accusation_signatureproves who pickeddeadline, but it never checks that the value respectsvote_validity_secs. A malicious accuser can choose an arbitrarily distant deadline, every honest voter will reuse it, and the on-chainblock.timestamp <= deadlinecheck will keep the slash evidence live far longer than governance intended.Possible guard
+ let now = self.clock.unix_now_secs(); + let max_deadline = now.saturating_add(self.vote_validity_secs); + if accusation.deadline < now || accusation.deadline > max_deadline { + warn!( + "Ignoring accusation from {} with out-of-window deadline {}", + accusation.accuser, accusation.deadline + ); + return; + } + let accusation_id = Self::accusation_id(&accusation);🤖 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 `@crates/zk-prover/src/actors/accusation_manager.rs` around lines 765 - 920, The accusation handler currently trusts Accusation.deadline from the signer (verify_accusation_signature only checks signer, not range), allowing a malicious accuser to set arbitrarily distant deadlines; update the logic that accepts an accusation (after verify_accusation_signature and before inserting PendingAccusation / creating AccusationVote) to enforce vote_validity_secs: compute a max_deadline = now + self.vote_validity_secs and reject or clamp any accusation.deadline > max_deadline (preferably reject and log a warning), and ensure the created AccusationVote.deadline uses the bounded value; apply the same bounding when handling forwarded C3a/C3b branches where you "Adopt the accuser's deadline" so votes and pending state never carry an out-of-range deadline.
800-889:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't trust
accused_party_idfrom a forwarded accusation.In the C3a/C3b branch,
sender_party_idcomes straight from the accusation even though only the forwarded payload's signer is authenticated. A malicious accuser can pair a valid payload fromaccusedwith a different party id and make honest peers re-verify against the wrong slot, turning a valid proof into a false agreement vote.Suggested validation
+ if self + .committee + .get(accusation.accused_party_id as usize) + .copied() + != Some(accusation.accused) + { + warn!( + "Forwarded accusation party_id {} does not match accused {}", + accusation.accused_party_id, accusation.accused + ); + return; + } + let data_hash = Self::compute_payload_hash(forwarded);packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
366-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd array-length consistency guard before persisting DKG anchors.
partyIds,skAgg, andesmAggare persisted without checking equal lengths. A mismatched return can corrupt per-party attribution used for slashing evidence.🔧 Suggested fix
( uint256[] memory partyIds, bytes32[] memory skAgg, bytes32[] memory esmAgg ) = dkgFoldAttestationVerifier.verify( address(this), block.chainid, e3Id, proof, dkgAttestationBundle ); + require( + partyIds.length == skAgg.length && skAgg.length == esmAgg.length, + AttestationBindingCountMismatch() + ); dkgPartyIds[e3Id] = partyIds; dkgSkAggCommits[e3Id] = skAgg; dkgEsmAggCommits[e3Id] = esmAgg;🤖 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 `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` around lines 366 - 380, The three arrays returned from dkgFoldAttestationVerifier.verify (partyIds, skAgg, esmAgg) are stored into dkgPartyIds[e3Id], dkgSkAggCommits[e3Id], and dkgEsmAggCommits[e3Id] without checking lengths; add a guard after the verify call that requires partyIds.length == skAgg.length && partyIds.length == esmAgg.length and revert with a clear error string (e.g., "DKG anchor length mismatch") if not, before assigning to the storage mappings, to prevent inconsistent per-party attribution and slashing corruption.
♻️ Duplicate comments (3)
circuits/benchmarks/scripts/extract_crisp_verify_gas.sh (1)
166-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten folded-proof presence check before replay.
Line 166 still treats missing keys as valid (
null != ""), so EVM replay can execute with invalid folded artifacts.Suggested fix
-elif [ ! -s "$TMP_JSON_FOLDED" ] || ! jq -e '(.dkg_aggregator.proof_hex != "") and (.decryption_aggregator.proof_hex != "")' "$TMP_JSON_FOLDED" >/dev/null 2>&1; then +elif [ ! -s "$TMP_JSON_FOLDED" ] || ! jq -e ' + (.dkg_aggregator.proof_hex? | type == "string" and length > 2) and + (.decryption_aggregator.proof_hex? | type == "string" and length > 2) +' "$TMP_JSON_FOLDED" >/dev/null 2>&1; then🤖 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 `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh` around lines 166 - 167, The current jq check treats missing/null keys as valid because null != "" passes; update the condition that inspects TMP_JSON_FOLDED so it verifies both .dkg_aggregator.proof_hex and .decryption_aggregator.proof_hex exist and are non-empty strings (e.g., ensure has/type and != "" or coalesce null to "" before comparing) so the shell branch skips EVM replay when folded proofs are missing or empty; modify the jq expression used in the if condition that references TMP_JSON_FOLDED and the fields .dkg_aggregator.proof_hex and .decryption_aggregator.proof_hex accordingly.crates/tests/tests/integration.rs (1)
271-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the local-build fallback preset-verified.
Checking only for
.build-stamp.jsonpresence still lets a stale secure stamp authorize an insecurecircuits/binbuild, or vice versa, so this path can still silently mix BFV presets and circuit artifacts. Parse the stamp and require it to matchpreset_subdirbefore staging local artifacts; otherwise fall back toensure_installed(). Based on learnings: In Rust tests, ensure that test suites consistently use either insecure BFV parameter presets or production/secure presets during transitions.🤖 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 `@crates/tests/tests/integration.rs` around lines 271 - 275, The current check only tests for preset_build_stamp.exists() which can allow mismatched secure/insecure artifacts; update the logic around dist_marker / circuits_bin_marker / preset_build_stamp so you parse the JSON in preset_build_stamp (use the same structure produced elsewhere and check the preserved preset_subdir field) and only call copy_dir_recursive(&dist_preset, &preset_out) when the parsed stamp's preset_subdir equals the current preset_subdir; if it does not match (or parse fails) fall back to calling ensure_installed() instead of staging local artifacts; apply this check where circuits_bin_marker and preset_build_stamp are evaluated so tests consistently use either insecure BFV presets or secure presets during transitions.examples/CRISP/client/.env.example (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a placeholder for
VITE_CRISP_TOKENin the example env.Hardcoding a concrete token address here is likely to drift and can cause confusing local setup failures. Prefer an explicit placeholder (or empty value) so users must set their environment-specific token.
Suggested fix
-VITE_CRISP_TOKEN=0x5081a39b8A5f0E35a8D959395a630b68B74Dd30f +VITE_CRISP_TOKEN=<YOUR_CRISP_TOKEN_ADDRESS>🤖 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 `@examples/CRISP/client/.env.example` at line 5, The example env file currently hardcodes a concrete token value for VITE_CRISP_TOKEN; replace that concrete token with a clear placeholder or empty value so users must supply their own token (update the VITE_CRISP_TOKEN entry in examples/CRISP/client/.env.example to something like VITE_CRISP_TOKEN=YOUR_CRISP_TOKEN or VITE_CRISP_TOKEN=). Ensure the placeholder is obvious and consistent with other example env entries.
🧹 Nitpick comments (1)
packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts (1)
59-68: ⚡ Quick winShell command injection risk via
presetvariable.The
presetvalue originates fromBENCHMARK_PRESETenv var or.active-preset.jsonand is interpolated directly into a shell command string. Malicious input likeinsecure-512; rm -rf /could execute arbitrary commands.Consider using
execFileSyncwith an argument array or sanitizing/validating the preset value.♻️ Option 1: Validate preset format
function readBenchmarkPreset(): string { const fromEnv = process.env.BENCHMARK_PRESET?.trim(); - if (fromEnv) return fromEnv; + if (fromEnv) { + if (!/^[a-zA-Z0-9_-]+$/.test(fromEnv)) { + throw new Error(`Invalid BENCHMARK_PRESET format: ${fromEnv}`); + } + return fromEnv; + } const activePath = path.join(REPO_ROOT, "circuits/bin/.active-preset.json");♻️ Option 2: Use spawnSync with args array (preferred)
+import { spawnSync } from "node:child_process"; - execSync( - [ - "pnpm generate:verifiers", - "--circuits dkg_aggregator,decryption_aggregator", - "--no-compile", - "--write", - `--preset ${preset}`, - `--output-dir ${benchDir}`, - ].join(" "), - { cwd: REPO_ROOT, stdio: "inherit" }, - ); + const result = spawnSync( + "pnpm", + [ + "generate:verifiers", + "--circuits", "dkg_aggregator,decryption_aggregator", + "--no-compile", + "--write", + "--preset", preset, + "--output-dir", benchDir, + ], + { cwd: REPO_ROOT, stdio: "inherit" }, + ); + if (result.status !== 0) { + throw new Error(`generate:verifiers failed with status ${result.status}`); + }🤖 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 `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts` around lines 59 - 68, The code invokes execSync to run "pnpm generate:verifiers" with a backfilled `--preset ${preset}` which allows shell injection via the preset variable; update the call in benchmarkGasFromRaw.ts (the execSync call that builds the command string for "pnpm generate:verifiers" with benchDir and REPO_ROOT) to avoid string interpolation: either validate/sanitize `preset` against an allowlist/regex (e.g., only alphanumerics, dashes, underscores) before using it, or replace execSync with execFileSync/spawnSync and pass the command and arguments as an array (e.g., command "pnpm" and args ["generate:verifiers","--circuits","dkg_aggregator,decryption_aggregator","--no-compile","--write",`--preset`, preset, `--output-dir`, benchDir]) so the preset cannot inject shell metacharacters.
🤖 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 `@crates/tests/tests/integration.rs`:
- Around line 1162-1170: The test mutates the process-global env var
BENCHMARK_DKG_FOLD_ATTESTATION_VERIFIER without restoring it; wrap the set_var
call in the same RAII helper you used for the timeout env vars so the original
value is restored at scope exit: when benchmark_proof_aggregation_enabled() &&
std::env::var("BENCHMARK_DKG_FOLD_ATTESTATION_VERIFIER").is_err() create a
Scoped/guard (the same RAII pattern used for the timeout env vars) that sets
BENCHMARK_DKG_FOLD_ATTESTATION_VERIFIER to
"0x7969c5eD335650692Bc04293B07F5BF2e7A673C0" and drops at the end of the test to
restore the prior value (or unset it) instead of calling std::env::set_var
directly.
---
Outside diff comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 548-551: The jq reads on $blob (populating nodes_spawned,
network_model, testmode_harness) must be guarded so they don't run when $blob is
empty; modify generate_report.sh to check if $blob is non-empty (e.g., if [ -n
"$blob" ] or similar) before calling jq and otherwise set nodes_spawned,
network_model, and testmode_harness to empty/default values, ensuring under set
-e the script won't exit when there is no integration blob.
- Line 503: The extraction for entire_test_sec only looks at .timings_seconds
and misses summaries that use .phase_timings; update the logic that computes
entire_test_sec to fall back to .phase_timings when the jq query on
.timings_seconds returns empty (i.e., check .timings_seconds[]? | select(.label
== "Entire Test") | .seconds first, and if not present run the same select
against .phase_timings[]?); reference the existing variable name entire_test_sec
and the jq invocation that queries ".timings_seconds[]? | select(.label ==
\"Entire Test\") | .seconds" and ensure the final value is set from either
source.
In `@circuits/benchmarks/scripts/replay_folded_verify_gas.sh`:
- Around line 63-64: The preflight jq check using SUMMARY_JSON currently only
tests truthiness so empty strings for .folded_artifacts.dkg_aggregator.proof_hex
or .folded_artifacts.decryption_aggregator.proof_hex can pass; change the jq
expression used in the if to explicitly reject empty strings (e.g. test that
both fields exist and are not "" or that their length > 0) so the script echoes
the error and exits when either proof_hex is empty; update the condition that
references SUMMARY_JSON and the jq path
.folded_artifacts.dkg_aggregator.proof_hex /
.folded_artifacts.decryption_aggregator.proof_hex accordingly.
In `@crates/zk-prover/src/actors/accusation_manager.rs`:
- Around line 765-920: The accusation handler currently trusts
Accusation.deadline from the signer (verify_accusation_signature only checks
signer, not range), allowing a malicious accuser to set arbitrarily distant
deadlines; update the logic that accepts an accusation (after
verify_accusation_signature and before inserting PendingAccusation / creating
AccusationVote) to enforce vote_validity_secs: compute a max_deadline = now +
self.vote_validity_secs and reject or clamp any accusation.deadline >
max_deadline (preferably reject and log a warning), and ensure the created
AccusationVote.deadline uses the bounded value; apply the same bounding when
handling forwarded C3a/C3b branches where you "Adopt the accuser's deadline" so
votes and pending state never carry an out-of-range deadline.
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 366-380: The three arrays returned from
dkgFoldAttestationVerifier.verify (partyIds, skAgg, esmAgg) are stored into
dkgPartyIds[e3Id], dkgSkAggCommits[e3Id], and dkgEsmAggCommits[e3Id] without
checking lengths; add a guard after the verify call that requires
partyIds.length == skAgg.length && partyIds.length == esmAgg.length and revert
with a clear error string (e.g., "DKG anchor length mismatch") if not, before
assigning to the storage mappings, to prevent inconsistent per-party attribution
and slashing corruption.
---
Duplicate comments:
In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh`:
- Around line 166-167: The current jq check treats missing/null keys as valid
because null != "" passes; update the condition that inspects TMP_JSON_FOLDED so
it verifies both .dkg_aggregator.proof_hex and .decryption_aggregator.proof_hex
exist and are non-empty strings (e.g., ensure has/type and != "" or coalesce
null to "" before comparing) so the shell branch skips EVM replay when folded
proofs are missing or empty; modify the jq expression used in the if condition
that references TMP_JSON_FOLDED and the fields .dkg_aggregator.proof_hex and
.decryption_aggregator.proof_hex accordingly.
In `@crates/tests/tests/integration.rs`:
- Around line 271-275: The current check only tests for
preset_build_stamp.exists() which can allow mismatched secure/insecure
artifacts; update the logic around dist_marker / circuits_bin_marker /
preset_build_stamp so you parse the JSON in preset_build_stamp (use the same
structure produced elsewhere and check the preserved preset_subdir field) and
only call copy_dir_recursive(&dist_preset, &preset_out) when the parsed stamp's
preset_subdir equals the current preset_subdir; if it does not match (or parse
fails) fall back to calling ensure_installed() instead of staging local
artifacts; apply this check where circuits_bin_marker and preset_build_stamp are
evaluated so tests consistently use either insecure BFV presets or secure
presets during transitions.
In `@examples/CRISP/client/.env.example`:
- Line 5: The example env file currently hardcodes a concrete token value for
VITE_CRISP_TOKEN; replace that concrete token with a clear placeholder or empty
value so users must supply their own token (update the VITE_CRISP_TOKEN entry in
examples/CRISP/client/.env.example to something like
VITE_CRISP_TOKEN=YOUR_CRISP_TOKEN or VITE_CRISP_TOKEN=). Ensure the placeholder
is obvious and consistent with other example env entries.
---
Nitpick comments:
In `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts`:
- Around line 59-68: The code invokes execSync to run "pnpm generate:verifiers"
with a backfilled `--preset ${preset}` which allows shell injection via the
preset variable; update the call in benchmarkGasFromRaw.ts (the execSync call
that builds the command string for "pnpm generate:verifiers" with benchDir and
REPO_ROOT) to avoid string interpolation: either validate/sanitize `preset`
against an allowlist/regex (e.g., only alphanumerics, dashes, underscores)
before using it, or replace execSync with execFileSync/spawnSync and pass the
command and arguments as an array (e.g., command "pnpm" and args
["generate:verifiers","--circuits","dkg_aggregator,decryption_aggregator","--no-compile","--write",`--preset`,
preset, `--output-dir`, benchDir]) so the preset cannot inject shell
metacharacters.
🪄 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: a8ce00c8-9da9-4572-9cc9-47095f9689fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
circuits/.gitignorecircuits/benchmarks/README.mdcircuits/benchmarks/results_insecure_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_agg/integration_summary.jsoncircuits/benchmarks/results_insecure_agg/report.mdcircuits/benchmarks/results_insecure_no_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_no_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_no_agg/integration_summary.jsoncircuits/benchmarks/results_insecure_no_agg/report.mdcircuits/benchmarks/results_secure_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_agg/integration_summary.jsoncircuits/benchmarks/results_secure_agg/report.mdcircuits/benchmarks/results_secure_no_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_no_agg/integration_summary.jsoncircuits/benchmarks/results_secure_no_agg/report.mdcircuits/benchmarks/scripts/check_circuit_preset_artifacts.shcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/generate_report.shcircuits/benchmarks/scripts/replay_folded_verify_gas.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/Dockerfilecrates/ciphernode-builder/src/ciphernode_builder.rscrates/config/src/app_config.rscrates/entrypoint/src/start/start.rscrates/events/src/enclave_event/accusation_quorum_reached.rscrates/events/src/enclave_event/accusation_vote.rscrates/events/src/enclave_event/proof_failure_accusation.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/lib.rscrates/evm/src/slashing_manager_sol_writer.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/test-helpers/src/lib.rscrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/gen_esi_sss.rscrates/trbfv/src/gen_pk_share_and_sk_sss.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/actors/accusation_manager_ext.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/actors/node_proof_aggregator.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/prover.rscrates/zk-prover/tests/slashing_integration_tests.rsdocs/pages/internals/dkg.mdxexamples/CRISP/Readme.mdexamples/CRISP/client/.env.exampleexamples/CRISP/client/src/components/Cards/PollCardResult.tsxexamples/CRISP/crisp.dev.env.exampleexamples/CRISP/docs/PROOF_AGGREGATION_AND_ZK.mdexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/.env.exampleexamples/CRISP/packages/crisp-contracts/README.mdexamples/CRISP/packages/crisp-contracts/deploy/deploy.tsexamples/CRISP/packages/crisp-contracts/deploy/syncCrispEnv.tsexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/scripts/crisp_deploy.shexamples/CRISP/scripts/lib/dev_config.shexamples/CRISP/scripts/setup.shexamples/CRISP/server/.env.exampleexamples/CRISP/test/crisp.spec.tspackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/ISlashingManager.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/slashing/SlashingManager.solpackages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/hardhat.config.tspackages/enclave-contracts/package.jsonpackages/enclave-contracts/scripts/benchmarkGasFromRaw.tspackages/enclave-contracts/scripts/deployAndSave/enclaveToken.tspackages/enclave-contracts/scripts/deployAndSave/slashingManager.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/ciphernode.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Pricing/DustRotation.spec.tspackages/enclave-contracts/test/Pricing/PullPaymentsAndAllowlist.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-contracts/test/Slashing/SlashingManager.spec.tspackages/enclave-contracts/test/fixtures/bfv_vk_binding/folded_artifacts.jsonpackages/enclave-contracts/test/fixtures/helpers.tsscripts/build-circuits.tsscripts/generate-verifiers.tstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/hardhat.config.tstests/integration/enclave.config.yaml
💤 Files with no reviewable changes (5)
- packages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.json
- circuits/benchmarks/results_secure_agg/integration_summary.json
- circuits/benchmarks/results_insecure_agg/crisp_verify_gas.json
- circuits/benchmarks/results_secure_agg/crisp_verify_gas.json
- circuits/benchmarks/results_insecure_agg/integration_summary.json
✅ Files skipped from review due to trivial changes (24)
- packages/enclave-contracts/.gitignore
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- examples/CRISP/crisp.dev.env.example
- examples/CRISP/packages/crisp-contracts/.env.example
- crates/Dockerfile
- examples/CRISP/Readme.md
- circuits/benchmarks/results_secure_agg/benchmark_run_meta.json
- packages/enclave-contracts/contracts/interfaces/ISlashingManager.sol
- circuits/benchmarks/results_secure_no_agg/report.md
- circuits/benchmarks/results_insecure_agg/benchmark_run_meta.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
- examples/CRISP/docs/PROOF_AGGREGATION_AND_ZK.md
- circuits/benchmarks/results_secure_no_agg/crisp_verify_gas.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- circuits/benchmarks/results_insecure_no_agg/benchmark_run_meta.json
- examples/CRISP/packages/crisp-contracts/deployed_contracts.json
- circuits/benchmarks/results_secure_no_agg/integration_summary.json
- circuits/benchmarks/results_insecure_no_agg/integration_summary.json
- circuits/benchmarks/results_insecure_agg/report.md
- packages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.json
- circuits/benchmarks/results_insecure_no_agg/report.md
- circuits/benchmarks/results_secure_agg/report.md
- docs/pages/internals/dkg.mdx
- packages/enclave-contracts/deployed_contracts.json
Diagnostic console.error calls + test-side log buffer added in 142034a to identify the post-reload cast-vote race. CI confirmed the race was in waitForWalletSession (other agent's fix) — actor-signed signatures recover cleanly, end-to-end is green. Restore the three touched files to their pre-instrumentation state.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/keyshare/src/threshold_keyshare.rs (1)
499-511:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly clear the deferred flag after
KeyshareCreatedpublishes successfully.If
publish_keyshare_created()errors here,pending_keyshare_publishhas already been reset, so the deferred publish is dropped permanently.Proposed fix
- self.pending_keyshare_publish = false; - self.publish_keyshare_created(ec) + let result = self.publish_keyshare_created(ec); + if result.is_ok() { + self.pending_keyshare_publish = false; + } + result🤖 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 `@crates/keyshare/src/threshold_keyshare.rs` around lines 499 - 511, In try_finish_deferred_keyshare_publish, don't clear self.pending_keyshare_publish before calling publish_keyshare_created; instead, call publish_keyshare_created(ec) first and only set self.pending_keyshare_publish = false after that call returns Ok (or set it in the success arm), propagating any Err from publish_keyshare_created so the deferred flag remains set on failure; refer to the try_finish_deferred_keyshare_publish method, the pending_keyshare_publish field, and the publish_keyshare_created(...) call to locate and update the logic accordingly.crates/evm/src/ciphernode_registry_sol.rs (1)
692-695:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject aggregated DKG publishes that are missing the attestation bundle.
When
dkg_aggregator_proofis present, sendingBytes::new()here just pushes an invalid proof-aggregation publish on-chain. This should fail fast locally instead of attempting a revert-pronepublishCommitteecall.Proposed fix
+ if dkg_aggregator_proof.is_some() != dkg_attestation_bundle.is_some() { + anyhow::bail!( + "aggregated DKG publish requires both the aggregator proof and the attestation bundle" + ); + } + let attestation_bundle: Bytes = match dkg_attestation_bundle { Some(b) => Bytes::copy_from_slice(b), None => Bytes::new(), };🤖 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 `@crates/evm/src/ciphernode_registry_sol.rs` around lines 692 - 695, The code currently constructs attestation_bundle as Bytes::new() when dkg_attestation_bundle is None, allowing an invalid aggregated DKG publish to be sent if dkg_aggregator_proof is present; change the logic to early-fail when an aggregator proof is provided but the attestation bundle is missing: check dkg_aggregator_proof and dkg_attestation_bundle together and return an error (or Err variant / early return) if dkg_aggregator_proof.is_some() && dkg_attestation_bundle.is_none(), otherwise construct attestation_bundle only from Some(b); ensure this prevents calling publishCommittee with an empty attestation_bundle and surfaces a clear error from the surrounding function (reference variables: dkg_attestation_bundle, dkg_aggregator_proof, attestation_bundle, and the publishCommittee call).circuits/benchmarks/scripts/generate_report.sh (1)
548-551:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard these
jqreads when no integration blob is available.
emit_audit_warnings()explicitly supports raw-only runs, but these assignments still parse"$blob"unconditionally. With an empty blob,jqexits non-zero underset -e, so report generation can abort before the script reaches the intended “No integration summary” fallback.Suggested fix
- local nodes_spawned network_model testmode_harness - nodes_spawned=$(jq -r '.benchmark_config.nodes_spawned // empty' <<<"$blob") - network_model=$(jq -r '.benchmark_config.network_model // empty' <<<"$blob") - testmode_harness=$(jq -r 'if .benchmark_config.testmode_harness != null then .benchmark_config.testmode_harness else empty end' <<<"$blob") + local nodes_spawned="" network_model="" testmode_harness="" + if [ -n "$blob" ]; then + nodes_spawned=$(jq -r '.benchmark_config.nodes_spawned // empty' <<<"$blob") + network_model=$(jq -r '.benchmark_config.network_model // empty' <<<"$blob") + testmode_harness=$(jq -r 'if .benchmark_config.testmode_harness != null then .benchmark_config.testmode_harness else empty end' <<<"$blob") + fi🤖 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 `@circuits/benchmarks/scripts/generate_report.sh` around lines 548 - 551, The jq reads for nodes_spawned, network_model and testmode_harness must be skipped when there is no integration blob to avoid jq failing under set -e; update generate_report.sh so that before executing the jq assignments for nodes_spawned, network_model and testmode_harness you check that blob is non-empty (e.g. if [[ -n "$blob" ]]; then ... fi) and only run the three jq commands inside that branch, leaving the variables empty otherwise so emit_audit_warnings() and the “No integration summary” fallback proceed safely.crates/zk-prover/src/actors/accusation_manager.rs (1)
781-920:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate incoming accusation deadlines before re-signing them.
on_accusation_received()reusesaccusation.deadlineverbatim for our vote, but never checks whether that deadline is still live or even within the localvote_validity_secswindow. A malicious accuser can therefore gather signatures on already-expired evidence that will always fail on chain, or on arbitrarily long-lived votes that bypass the freshness bound this PR is introducing.🤖 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 `@crates/zk-prover/src/actors/accusation_manager.rs` around lines 781 - 920, The code in on_accusation_received reuses accusation.deadline directly when building AccusationVote (voter signature) without checking freshness; before creating/signing the AccusationVote validate that accusation.deadline is not already expired and is within our allowed freshness window (self.vote_validity_secs) relative to current time (use the same clock source used elsewhere, e.g., actor ctx or system time). If the deadline has already passed or exceeds now + self.vote_validity_secs, abstain (log and return) or clamp the deadline to now + self.vote_validity_secs (prefer abstain for expired, clamp for too-far future) and only then proceed to build/sign the AccusationVote; update references around AccusationVote, pending insertion, and any call to on_vote_timeout to use the validated/clamped deadline.
♻️ Duplicate comments (3)
examples/CRISP/client/.env.example (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a placeholder instead of a fixed CRISP token value.
Line 7 still hardcodes a deployment-specific token address; this can become stale and cause incorrect local setup defaults.
Suggested fix
-VITE_CRISP_TOKEN=0x5081a39b8A5f0E35a8D959395a630b68B74Dd30f +VITE_CRISP_TOKEN=<YOUR_CRISP_TOKEN_HERE>🤖 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 `@examples/CRISP/client/.env.example` at line 7, The .env example currently hardcodes a real token value for VITE_CRISP_TOKEN which can be stale or leak secrets; replace the fixed value with a clear placeholder (e.g., VITE_CRISP_TOKEN=<YOUR_CRISP_TOKEN_HERE>) and update any README instructions to tell users to insert their own token for VITE_CRISP_TOKEN when setting up locally, ensuring no real token remains in examples.circuits/benchmarks/scripts/extract_crisp_verify_gas.sh (1)
166-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat missing
proof_hexthe same as empty before replay.
null != ""is truthy injq, so this guard still passes when eitherproof_hexfield is missing. That lets the replay branch run with incomplete folded artifacts and can skew the reported Pi_DKG / Pi_dec gas numbers.♻️ Minimal fix
-elif [ ! -s "$TMP_JSON_FOLDED" ] || ! jq -e '(.dkg_aggregator.proof_hex != "") and (.decryption_aggregator.proof_hex != "")' "$TMP_JSON_FOLDED" >/dev/null 2>&1; then +elif [ ! -s "$TMP_JSON_FOLDED" ] || ! jq -e ' + ((.dkg_aggregator.proof_hex // "") | length > 2) and + ((.decryption_aggregator.proof_hex // "") | length > 2) +' "$TMP_JSON_FOLDED" >/dev/null 2>&1; then🤖 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 `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh` around lines 166 - 168, The jq guard currently treats missing proof_hex as non-empty because null != "" is true; update the condition used against TMP_JSON_FOLDED so missing and empty are equivalent by normalizing with a fallback or explicit existence check (e.g., use the null-coalescing pattern or check for has + not null) for both .dkg_aggregator.proof_hex and .decryption_aggregator.proof_hex, and keep the rest of the branch that echoes the skip message and writes '{}' to TMP_JSON_ENCLAVE unchanged; target the jq invocation that currently reads TMP_JSON_FOLDED in the elif line to apply this change.crates/tests/tests/integration.rs (1)
299-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExistence-only
.build-stamp.jsonchecking can still mix circuit presets.This branch trusts
circuits/binforpreset_subdirwhendist/circuits/<preset>/.build-stamp.jsonmerely exists, but it never reads the stamp to confirm that the current local build actually targeted that preset. A stale secure stamp plus a later insecure local build still satisfies this check and stages the wrong artifacts intopreset_out, which reintroduces mixed-parameter runs.Based on learnings: In Rust tests, ensure that test suites consistently use either insecure BFV parameter presets or production/secure presets during transitions. Do not mix insecure and production parameters within the same test run or across tests during the transition window.
🤖 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 `@crates/tests/tests/integration.rs` around lines 299 - 315, The code currently treats the mere existence of preset_build_stamp as proof the local circuits match preset_subdir, allowing stale stamps to mix presets; change the logic to open and parse the .build-stamp.json (preset_build_stamp) and validate its recorded preset/target field (or checksum/commit field) equals the current preset_subdir (and/or matches the circuits/bin identity) before trusting circuits_bin_marker. If the stamp's preset does not match, behave as if the stamp is missing (call ZkBackend::ensure_installed to download fresh circuits) and avoid populating preset_out from the local circuits/bin. Reference symbols: preset_build_stamp, circuits_bin_marker, preset_subdir, preset_out, and ZkBackend::ensure_installed.
🤖 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 `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 586-618: The code currently computes a single
dkg_fold_verifier_addr and accusation_vote_validity_secs at startup; change
these to be keyed maps by chain id (use e3_id.chain_id()) instead of globals so
each enabled chain can have its own settings. Specifically, replace the
single-valued dkg_fold_verifier_addr and accusation_vote_validity_secs with
HashMap<ChainId, ...>, populate them by calling
Self::fetch_dkg_fold_attestation_verifier_from_registry and
Self::fetch_accusation_vote_validity_from_registry per chain (with the same
BENCHMARK_* env-var fallback applied per chain), and update places that
construct AccusationManagerExtension and any E3-building logic to look up values
by e3_id.chain_id() rather than using the global variables; reference functions
fetch_dkg_fold_attestation_verifier_from_registry,
fetch_accusation_vote_validity_from_registry and the identifiers
dkg_fold_verifier_addr and accusation_vote_validity_secs when making the
changes.
In `@crates/multithread/src/multithread.rs`:
- Around line 537-539: The current code locks the shared RNG (rng.lock(),
rng_guard, rng_mut) before the match causing unrelated branches to serialize;
remove the pre-match lock and instead acquire the RNG mutex only inside the
branches that actually need randomness (GenPkShareAndSkSss and GenEsiSss), e.g.
call rng.lock().expect(...) and bind the mutable RNG reference only within those
branch scopes where it is used; ensure the lock is dropped promptly (do not hold
it across awaits or long-running work) and update uses of rng_mut in those
branches accordingly.
In `@docs/pages/internals/dkg.mdx`:
- Around line 567-573: The text overstates that accusationVoteValidity = 0 is an
absolute hard stop; update the description to say it "practically disables"
slashing or describes it as a zero-second window instead, and mention that
because the on-chain check uses block.timestamp <= deadline a vote with deadline
= now can still succeed if included in the same block; reference the terms
accusationVoteValidity, deadline, proposeSlash and quorum M in the paragraph so
readers know the exact parameters affected and the on-chain race condition.
In `@examples/CRISP/client/src/utils/methods.ts`:
- Around line 25-28: getChain currently only special-cases 31337 and otherwise
ignores explicit VITE_CHAIN_ID values by falling back to DEV, so update getChain
to honor VITE_CHAIN_ID for all configured networks: read chainId from
import.meta.env.VITE_CHAIN_ID, normalize it to a string/number, then return
anvil when it matches 31337 (string or number) and return sepolia when it
matches the sepolia identifier(s) you support (e.g., 'sepolia' or its chainId
like '11155111'), and only if chainId is missing/empty fall back to
import.meta.env.DEV ? anvil : sepolia; reference the getChain function, the
chainId const, and the anvil/sepolia symbols when making the change.
In `@examples/CRISP/packages/crisp-contracts/deploy/syncCrispEnv.ts`:
- Around line 112-123: The loop over mockMappings only sets
serverUpdates[envKey] when deploymentAddress(contractName, chain) returns an
addr, leaving stale MOCK_* entries in server/.env; update the loop in
syncCrispEnv.ts so that for each [envKey, contractName] you call
deploymentAddress(contractName, chain) and if addr exists set
serverUpdates[envKey] = addr, otherwise remove any previous value by deleting
serverUpdates[envKey] (or setting it to an empty string) so old addresses aren’t
left behind when a deployment is absent.
In `@examples/CRISP/scripts/lib/dev_config.sh`:
- Around line 79-80: The script currently runs cd "${REPO_ROOT}" then executes
pnpm build:circuits without checking cd succeeded; update the dev_config.sh so
the directory change is verified before running pnpm (e.g., check the exit
status of cd "${REPO_ROOT}" and exit non‑zero on failure, or chain the commands
so pnpm build:circuits --preset "${CRISP_BFV_PRESET}" --skip-if-built only runs
if cd succeeded), referencing the existing cd "${REPO_ROOT}" and the pnpm
build:circuits invocation.
In `@examples/CRISP/scripts/setup.sh`:
- Around line 7-16: The remaining setup steps still run relative to the caller
cwd; update each command after load_crisp_dev_config so they are executed from
REPO_ROOT (or SCRIPT_DIR as appropriate) — e.g., wrap the pnpm install, pnpm
build:ts and the subsequent commands referenced in the review to run with an
explicit (cd "${REPO_ROOT}" && ...) or use pushd/popd so every invocation is
path-safe; locate the commands using the SCRIPT_DIR and REPO_ROOT variables in
this script and convert them to use the explicit cd pattern so the script is
cwd-independent.
In `@packages/enclave-contracts/scripts/benchmarkGasFromRaw.ts`:
- Around line 29-44: readBenchmarkPreset() currently prefers BENCHMARK_PRESET or
local .active-preset.json and ignores the preset embedded in the replayed proof
artifacts (folded input), causing non-reproducible verifier selection; update
the logic so that wherever readBenchmarkPreset() is used (and specifically at
the points that load foldedPath), you first attempt to extract the producing
benchmark preset from the folded proof metadata (the object loaded from
foldedPath) and return that if present, only falling back to BENCHMARK_PRESET /
.active-preset.json when the artifact has no preset; modify
readBenchmarkPreset() (or add an overload/readFromArtifact helper) to accept the
parsed folded artifact (or read it prior to calling) and prefer artifact.preset
before existing fallbacks, and apply the same change at the other locations
referenced (around the blocks at the earlier and later call sites).
- Around line 50-75: The code builds shell command strings with execSync in
ensureHonkVerifierContractDir and interpolates preset directly; replace those
execSync string-invocations with child_process.execFileSync calls that pass the
executable and an args array so preset is a separate argument (e.g.
execFileSync('pnpm',
['generate:verifiers','--circuits','dkg_aggregator,decryption_aggregator','--no-compile','--write','--preset',
preset,'--output-dir', benchDir], {cwd: REPO_ROOT, stdio: 'inherit'}) ) and
likewise call execFileSync('pnpm', ['hardhat','compile'], { cwd:
path.join(REPO_ROOT,'packages/enclave-contracts'), stdio: 'inherit' }); this
removes shell interpolation risk while preserving behavior in
ensureHonkVerifierContractDir.
---
Outside diff comments:
In `@circuits/benchmarks/scripts/generate_report.sh`:
- Around line 548-551: The jq reads for nodes_spawned, network_model and
testmode_harness must be skipped when there is no integration blob to avoid jq
failing under set -e; update generate_report.sh so that before executing the jq
assignments for nodes_spawned, network_model and testmode_harness you check that
blob is non-empty (e.g. if [[ -n "$blob" ]]; then ... fi) and only run the three
jq commands inside that branch, leaving the variables empty otherwise so
emit_audit_warnings() and the “No integration summary” fallback proceed safely.
In `@crates/evm/src/ciphernode_registry_sol.rs`:
- Around line 692-695: The code currently constructs attestation_bundle as
Bytes::new() when dkg_attestation_bundle is None, allowing an invalid aggregated
DKG publish to be sent if dkg_aggregator_proof is present; change the logic to
early-fail when an aggregator proof is provided but the attestation bundle is
missing: check dkg_aggregator_proof and dkg_attestation_bundle together and
return an error (or Err variant / early return) if
dkg_aggregator_proof.is_some() && dkg_attestation_bundle.is_none(), otherwise
construct attestation_bundle only from Some(b); ensure this prevents calling
publishCommittee with an empty attestation_bundle and surfaces a clear error
from the surrounding function (reference variables: dkg_attestation_bundle,
dkg_aggregator_proof, attestation_bundle, and the publishCommittee call).
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 499-511: In try_finish_deferred_keyshare_publish, don't clear
self.pending_keyshare_publish before calling publish_keyshare_created; instead,
call publish_keyshare_created(ec) first and only set
self.pending_keyshare_publish = false after that call returns Ok (or set it in
the success arm), propagating any Err from publish_keyshare_created so the
deferred flag remains set on failure; refer to the
try_finish_deferred_keyshare_publish method, the pending_keyshare_publish field,
and the publish_keyshare_created(...) call to locate and update the logic
accordingly.
In `@crates/zk-prover/src/actors/accusation_manager.rs`:
- Around line 781-920: The code in on_accusation_received reuses
accusation.deadline directly when building AccusationVote (voter signature)
without checking freshness; before creating/signing the AccusationVote validate
that accusation.deadline is not already expired and is within our allowed
freshness window (self.vote_validity_secs) relative to current time (use the
same clock source used elsewhere, e.g., actor ctx or system time). If the
deadline has already passed or exceeds now + self.vote_validity_secs, abstain
(log and return) or clamp the deadline to now + self.vote_validity_secs (prefer
abstain for expired, clamp for too-far future) and only then proceed to
build/sign the AccusationVote; update references around AccusationVote, pending
insertion, and any call to on_vote_timeout to use the validated/clamped
deadline.
---
Duplicate comments:
In `@circuits/benchmarks/scripts/extract_crisp_verify_gas.sh`:
- Around line 166-168: The jq guard currently treats missing proof_hex as
non-empty because null != "" is true; update the condition used against
TMP_JSON_FOLDED so missing and empty are equivalent by normalizing with a
fallback or explicit existence check (e.g., use the null-coalescing pattern or
check for has + not null) for both .dkg_aggregator.proof_hex and
.decryption_aggregator.proof_hex, and keep the rest of the branch that echoes
the skip message and writes '{}' to TMP_JSON_ENCLAVE unchanged; target the jq
invocation that currently reads TMP_JSON_FOLDED in the elif line to apply this
change.
In `@crates/tests/tests/integration.rs`:
- Around line 299-315: The code currently treats the mere existence of
preset_build_stamp as proof the local circuits match preset_subdir, allowing
stale stamps to mix presets; change the logic to open and parse the
.build-stamp.json (preset_build_stamp) and validate its recorded preset/target
field (or checksum/commit field) equals the current preset_subdir (and/or
matches the circuits/bin identity) before trusting circuits_bin_marker. If the
stamp's preset does not match, behave as if the stamp is missing (call
ZkBackend::ensure_installed to download fresh circuits) and avoid populating
preset_out from the local circuits/bin. Reference symbols: preset_build_stamp,
circuits_bin_marker, preset_subdir, preset_out, and ZkBackend::ensure_installed.
In `@examples/CRISP/client/.env.example`:
- Line 7: The .env example currently hardcodes a real token value for
VITE_CRISP_TOKEN which can be stale or leak secrets; replace the fixed value
with a clear placeholder (e.g., VITE_CRISP_TOKEN=<YOUR_CRISP_TOKEN_HERE>) and
update any README instructions to tell users to insert their own token for
VITE_CRISP_TOKEN when setting up locally, ensuring no real token remains in
examples.
🪄 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: 346781cf-77fd-4236-af7a-81cf2b9e0c7d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
circuits/.gitignorecircuits/benchmarks/README.mdcircuits/benchmarks/results_insecure_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_agg/integration_summary.jsoncircuits/benchmarks/results_insecure_agg/report.mdcircuits/benchmarks/results_insecure_no_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_no_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_no_agg/integration_summary.jsoncircuits/benchmarks/results_insecure_no_agg/report.mdcircuits/benchmarks/results_secure_agg/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_agg/integration_summary.jsoncircuits/benchmarks/results_secure_agg/report.mdcircuits/benchmarks/results_secure_no_agg/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_no_agg/integration_summary.jsoncircuits/benchmarks/results_secure_no_agg/report.mdcircuits/benchmarks/scripts/check_circuit_preset_artifacts.shcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/generate_report.shcircuits/benchmarks/scripts/replay_folded_verify_gas.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/Dockerfilecrates/ciphernode-builder/src/ciphernode_builder.rscrates/config/src/app_config.rscrates/entrypoint/src/start/start.rscrates/events/src/enclave_event/accusation_quorum_reached.rscrates/events/src/enclave_event/accusation_vote.rscrates/events/src/enclave_event/proof_failure_accusation.rscrates/evm/src/ciphernode_registry_sol.rscrates/evm/src/lib.rscrates/evm/src/slashing_manager_sol_writer.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/test-helpers/src/lib.rscrates/test-helpers/src/usecase_helpers.rscrates/tests/tests/integration.rscrates/trbfv/src/gen_esi_sss.rscrates/trbfv/src/gen_pk_share_and_sk_sss.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/src/actors/accusation_manager.rscrates/zk-prover/src/actors/accusation_manager_ext.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/actors/node_proof_aggregator.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/prover.rscrates/zk-prover/tests/slashing_integration_tests.rsdocs/pages/cryptography.mdxdocs/pages/internals/dkg.mdxexamples/CRISP/Readme.mdexamples/CRISP/client/.env.exampleexamples/CRISP/client/.npmrcexamples/CRISP/client/src/components/Cards/PollCardResult.tsxexamples/CRISP/client/src/providers/Web3Provider.tsxexamples/CRISP/client/src/utils/methods.tsexamples/CRISP/client/vercel.jsonexamples/CRISP/crisp.dev.env.exampleexamples/CRISP/docs/PROOF_AGGREGATION_AND_ZK.mdexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/.env.exampleexamples/CRISP/packages/crisp-contracts/README.mdexamples/CRISP/packages/crisp-contracts/deploy/deploy.tsexamples/CRISP/packages/crisp-contracts/deploy/syncCrispEnv.tsexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/scripts/crisp_deploy.shexamples/CRISP/scripts/lib/dev_config.shexamples/CRISP/scripts/setup.shexamples/CRISP/server/.env.exampleexamples/CRISP/test/crisp.spec.tsexamples/CRISP/test/wallet-setup/basic.setup.tspackages/enclave-contracts/.gitignorepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/enclave-contracts/contracts/interfaces/ISlashingManager.solpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solpackages/enclave-contracts/contracts/slashing/SlashingManager.solpackages/enclave-contracts/contracts/verifiers/bfv/BfvDecryptionVerifier.solpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/hardhat.config.tspackages/enclave-contracts/package.jsonpackages/enclave-contracts/scripts/benchmarkGasFromRaw.tspackages/enclave-contracts/scripts/deployAndSave/enclaveToken.tspackages/enclave-contracts/scripts/deployAndSave/slashingManager.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/tasks/ciphernode.tspackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Pricing/DustRotation.spec.tspackages/enclave-contracts/test/Pricing/PullPaymentsAndAllowlist.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-contracts/test/Slashing/SlashingManager.spec.tspackages/enclave-contracts/test/fixtures/bfv_vk_binding/folded_artifacts.jsonpackages/enclave-contracts/test/fixtures/helpers.tsscripts/build-circuits.tsscripts/generate-verifiers.tstemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/hardhat.config.tstests/integration/enclave.config.yaml
💤 Files with no reviewable changes (5)
- packages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.json
- circuits/benchmarks/results_secure_agg/crisp_verify_gas.json
- circuits/benchmarks/results_insecure_agg/integration_summary.json
- circuits/benchmarks/results_secure_agg/integration_summary.json
- circuits/benchmarks/results_insecure_agg/crisp_verify_gas.json
✅ Files skipped from review due to trivial changes (20)
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- circuits/benchmarks/results_insecure_no_agg/benchmark_run_meta.json
- examples/CRISP/client/.npmrc
- circuits/.gitignore
- packages/enclave-contracts/.gitignore
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
- examples/CRISP/packages/crisp-contracts/.env.example
- examples/CRISP/crisp.dev.env.example
- circuits/benchmarks/results_secure_no_agg/crisp_verify_gas.json
- circuits/benchmarks/results_insecure_no_agg/integration_summary.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- circuits/benchmarks/results_secure_no_agg/report.md
- examples/CRISP/packages/crisp-contracts/README.md
- circuits/benchmarks/results_insecure_no_agg/crisp_verify_gas.json
- circuits/benchmarks/results_insecure_agg/report.md
- circuits/benchmarks/results_secure_agg/report.md
- circuits/benchmarks/results_insecure_no_agg/report.md
- examples/CRISP/docs/PROOF_AGGREGATION_AND_ZK.md
- circuits/benchmarks/results_secure_no_agg/integration_summary.json
- examples/CRISP/packages/crisp-contracts/deployed_contracts.json
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
3 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.
Several reviewers (and review LLMs) have flagged a concern that the committee-hash binding is set-level rather than per-party, and that "there is no cryptographic chain from 'fold proof at row i is bad' to 'address topNodes[i] produced it.'"
That chain is on this branch, in the Phase B fold-attestation work. For each honest party,
DkgFoldAttestationVerifierenforces on-chain:isCommitteeMemberActive(e3Id, node)for the claimed operator at thatpartyId.ecrecover(EIP-712 digest of (chainId, e3Id, partyId, skAggCommit, esmAggCommit), signature) == node.skAggCommit/esmAggCommitequal the proof's public inputs at that party's slot.Plus the bundle must contain exactly
hattestations (derived from the proof shape) in strict ascendingpartyIdorder — so an aggregator holding fold proofs from all members can neither reassign attribution nor pick an H-subset.Per-party commitments are persisted to
dkgPartyIds,dkgSkAggCommits,dkgEsmAggCommitsso downstream slashing can attribute a bad fold at rowitotopNodes[dkgPartyIds[e3Id][i]].Files to audit:
packages/enclave-contracts/contracts/verifiers/DkgFoldAttestationVerifier.solpackages/enclave-contracts/contracts/lib/DkgFoldAttestationLib.sol_verifyAndStoreDkgAnchorsinpackages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.solDocs:
docs/pages/cryptography.mdx— "Per-node attestations and topNodes binding" subsectiondocs/pages/internals/dkg.mdx— "Proof Aggregation and On-Chain Anchoring" sectionSummary by CodeRabbit
New Features
Documentation