chore: refactor proof gen to zk-prover [skip-line-limit]#1372
Merged
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:
📝 WalkthroughWalkthroughAdds a ShareVerificationActor and routes C2/C3/C4 share-proof verification through an ECDSA+ZK multistage pipeline; introduces new events/types for dispatch/collection; reorders builder ZK signer/backend initialization; moves C4 proof orchestration into ProofRequestActor and cleans up imports/lints. Changes
Sequence Diagram(s)sequenceDiagram
participant TK as ThresholdKeyshare
participant EB as EventBus
participant SVA as ShareVerificationActor
participant MT as MultithreadCompute
participant ZKB as ZKBackend
TK->>EB: Publish ShareVerificationDispatched(proofs, pre_dishonest)
EB->>SVA: Deliver verification request
SVA->>SVA: Perform ECDSA validation per party
alt All parties fail ECDSA
SVA->>EB: Publish ShareVerificationComplete(all_dishonest)
EB->>TK: Deliver completion (abort)
else Some parties pass ECDSA
SVA->>EB: Publish ComputeRequest(ZK verification) for passing parties
EB->>MT: Route compute request
MT->>ZKB: Verify ZK proofs (batch)
ZKB-->>MT: Return verification results
MT->>EB: ComputeResponse(results, correlation)
EB->>SVA: Deliver compute response
SVA->>SVA: Aggregate ECDSA + ZK failures
SVA->>EB: Publish ShareVerificationComplete(final_dishonest)
EB->>TK: Deliver completion (proceed/abort)
end
sequenceDiagram
participant PR as ProofRequestActor
participant EB as EventBus
participant MT as MultithreadCompute
participant PK as PrivateKeySigner
PR->>EB: Subscribe to DecryptionShareProofsPending
EB->>PR: Deliver DecryptionShareProofsPending(sk_request, esm_requests[])
PR->>MT: Dispatch ComputeRequest(C4a SK) and ComputeRequest(C4b ESM)s
MT-->>EB: ComputeResponse(C4 proofs)
EB->>PR: Deliver compute response
PR->>PR: Collect proofs, track correlations
alt All C4 proofs collected
PR->>PK: Sign SK + ESM proofs
PK-->>PR: SignedProofPayloads
PR->>EB: Publish DecryptionKeyShared(signed proofs)
else Waiting for remaining proofs
PR->>PR: Keep pending state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/sync/src/sync.rs (1)
53-53: Consider removing unused_net_configif libp2p sync won't be re-enabled soon.The
to_net_config()call is still executed despite the libp2p code being disabled. If the libp2p sync fix is not planned for the near term, consider removing both this line and the commented-out code block (lines 95-104, 109) to reduce maintenance noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sync/src/sync.rs` at line 53, Remove the unused call and variable by deleting the line that assigns let _net_config = snapshot.to_net_config(), and also remove the stale/commented-out libp2p sync block (the commented lines around the libp2p code) to reduce maintenance noise; if you plan to re-enable libp2p soon instead, wrap the call and the block behind a clear feature flag or cfg (rather than leaving dead code) so the snapshot.to_net_config() invocation and any libp2p-related comments are not executed or left dangling.crates/ciphernode-builder/src/ciphernode_builder.rs (1)
438-456: Doubleensure_signer()call may be redundant.
ensure_signer()is already called at line 397, and it's called again at line 445. If the signer is cached after the first call, this is just a minor inefficiency. However, if it's not cached, this could trigger duplicate work.Consider reusing the result from line 397 if the signer is the same, or verify that
ensure_signer()properly caches and returns the existing signer on subsequent calls.♻️ Suggested refactor to reuse signer
If
ensure_signer()returns the same signer and caches it, this is fine. Otherwise, consider:- // We need to supply the Hlc to the bus handle in order to enable it - let addr = provider_cache.ensure_signer().await?.address().to_string(); + // We need to supply the Hlc to the bus handle in order to enable it + let signer = provider_cache.ensure_signer().await?; + let addr = signer.address().to_string(); let bus = event_system.handle()?.enable(&addr);Then later at line 445, you can skip the second call since
signeris already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ciphernode-builder/src/ciphernode_builder.rs` around lines 438 - 456, The code calls provider_cache.ensure_signer() twice (first stored into a signer earlier and again before setup_zk_actors), which can duplicate work; reuse the previously obtained signer variable instead of calling ensure_signer() a second time: remove the second ensure_signer() invocation and pass the existing signer into setup_zk_actors(&bus, backend, signer) (and ensure the earlier signer is in scope before creating ThresholdKeyshareExtension and calling setup_zk_actors); this touches symbols provider_cache.ensure_signer, the signer local, ThresholdKeyshareExtension::create, e3_builder, and setup_zk_actors.crates/events/src/enclave_event/share_verification.rs (1)
29-41: Consider using an enum payload instead of dual optional-like fields.
ShareVerificationDispatchedhas bothshare_proofsanddecryption_proofsfields, but only one is relevant based onkind. This could lead to confusion or bugs if the wrong field is populated for a givenkind.Consider refactoring to an enum payload:
♻️ Suggested alternative design
pub enum VerificationPayload { ShareProofs(Vec<PartyProofsToVerify>), DecryptionProofs(Vec<PartyShareDecryptionProofsToVerify>), } pub struct ShareVerificationDispatched { pub e3_id: E3id, pub payload: VerificationPayload, pub pre_dishonest: BTreeSet<u64>, }This eliminates the
kindfield (derivable from the payload variant) and makes it impossible to populate the wrong field type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/share_verification.rs` around lines 29 - 41, ShareVerificationDispatched currently carries both share_proofs and decryption_proofs plus a VerificationKind, which allows invalid states; replace those fields with a single enum payload (e.g., VerificationPayload with variants ShareProofs(Vec<PartyProofsToVerify>) and DecryptionProofs(Vec<PartyShareDecryptionProofsToVerify>)), remove the redundant VerificationKind, keep e3_id and pre_dishonest, update all constructors/creators to build the appropriate VerificationPayload variant, update all pattern matches/consumers to match on ShareVerificationDispatched.payload instead of checking kind and reading the wrong field, and ensure derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash) is applied to the new enum so serialization and hashing behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1199-1203: The branch where party_proofs_to_verify.is_empty()
returns early and calls
self.proceed_with_decryption_key_calculation(Some(dishonest_set), ec) even when
there may not be enough honest parties; before returning compute the
honest_count (e.g., total_parties - pre_dishonest.len() - 1 if excluding self)
and compare it to self.threshold_m and if honest_count < self.threshold_m return
a protocol error (or appropriate failure path) instead of proceeding; update the
logic around party_proofs_to_verify / pre_dishonest and
proceed_with_decryption_key_calculation to enforce the honest_count >=
threshold_m check.
- Around line 1687-1690: When party_proofs.is_empty() you must re-check that the
remaining committee still meets the protocol threshold before calling
publish_keyshare_created; update the empty-branch in the function containing
party_proofs to call an existing threshold/committee check (e.g., compute
effective committee size or invoke a helper like self.can_reach_threshold /
self.has_enough_parties or check self.committee.len() against self.threshold)
and only call self.publish_keyshare_created(ec) when that check succeeds,
otherwise return an error/log and abort publishing; add/adjust tests to cover
the empty-proof + insufficient-committee case.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 398-473: handle_compute_request_error currently doesn't clean up
state when a C4 DkgShareDecryption publish fails, leaving entries in
decryption_correlation and pending_decryption and stalling E3; update
handle_compute_request_error to check if the failing correlation id exists in
self.decryption_correlation, and if so remove that correlation entry and also
remove the associated pending decryption entry in self.pending_decryption for
the referenced e3_id (and log appropriately) so that the Exchange `#3` flow can
proceed; reference functions/fields: handle_decryption_share_proofs_pending,
handle_compute_request_error, self.decryption_correlation,
self.pending_decryption, DecryptionProofKind.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 185-188: When publish() fails in verify_share_proofs (and
similarly in verify_decryption_proofs), the code currently removes the pending
entry but never emits the ShareVerificationComplete signal; update both
functions so that after detecting Err(err) from self.bus.publish(request, ec)
they (a) log the error, (b) remove the pending entry via
self.pending.remove(&correlation_id), and (c) publish a
ShareVerificationComplete message (or otherwise notify the awaiting
ThresholdKeyshare) for the same correlation_id so the caller won't wait
indefinitely; locate these changes around the self.bus.publish calls and the
correlation_id/pending handling in share_verification.rs.
---
Nitpick comments:
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 438-456: The code calls provider_cache.ensure_signer() twice
(first stored into a signer earlier and again before setup_zk_actors), which can
duplicate work; reuse the previously obtained signer variable instead of calling
ensure_signer() a second time: remove the second ensure_signer() invocation and
pass the existing signer into setup_zk_actors(&bus, backend, signer) (and ensure
the earlier signer is in scope before creating ThresholdKeyshareExtension and
calling setup_zk_actors); this touches symbols provider_cache.ensure_signer, the
signer local, ThresholdKeyshareExtension::create, e3_builder, and
setup_zk_actors.
In `@crates/events/src/enclave_event/share_verification.rs`:
- Around line 29-41: ShareVerificationDispatched currently carries both
share_proofs and decryption_proofs plus a VerificationKind, which allows invalid
states; replace those fields with a single enum payload (e.g.,
VerificationPayload with variants ShareProofs(Vec<PartyProofsToVerify>) and
DecryptionProofs(Vec<PartyShareDecryptionProofsToVerify>)), remove the redundant
VerificationKind, keep e3_id and pre_dishonest, update all constructors/creators
to build the appropriate VerificationPayload variant, update all pattern
matches/consumers to match on ShareVerificationDispatched.payload instead of
checking kind and reading the wrong field, and ensure derive(Serialize,
Deserialize, Debug, Clone, PartialEq, Eq, Hash) is applied to the new enum so
serialization and hashing behavior remains consistent.
In `@crates/sync/src/sync.rs`:
- Line 53: Remove the unused call and variable by deleting the line that assigns
let _net_config = snapshot.to_net_config(), and also remove the
stale/commented-out libp2p sync block (the commented lines around the libp2p
code) to reduce maintenance noise; if you plan to re-enable libp2p soon instead,
wrap the call and the block behind a clear feature flag or cfg (rather than
leaving dead code) so the snapshot.to_net_config() invocation and any
libp2p-related comments are not executed or left dangling.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
crates/ciphernode-builder/src/ciphernode_builder.rscrates/ciphernode-builder/src/eventbus_factory.rscrates/ciphernode-builder/src/evm_system.rscrates/cli/src/ciphernode/setup.rscrates/cli/src/start.rscrates/entrypoint/src/start/aggregator_start.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryption_key_shared.rscrates/events/src/enclave_event/decryption_share_proofs.rscrates/events/src/enclave_event/mod.rscrates/events/src/enclave_event/share_verification.rscrates/events/src/eventstore_router.rscrates/events/src/snapshot_buffer/batch.rscrates/events/src/snapshot_buffer/batch_router.rscrates/events/src/snapshot_buffer/snapshot_buffer.rscrates/evm/src/evm_parser.rscrates/evm/src/evm_read_interface.rscrates/evm/src/fix_historical_order.rscrates/indexer/tests/integration.rscrates/keyshare/src/threshold_keyshare.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/multithread.rscrates/sync/src/sync.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/actors/share_verification.rscrates/zk-prover/src/lib.rscrates/zk-prover/tests/common/mod.rs
💤 Files with no reviewable changes (3)
- crates/entrypoint/src/start/aggregator_start.rs
- crates/events/src/snapshot_buffer/batch.rs
- crates/ciphernode-builder/src/eventbus_factory.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
crates/events/src/enclave_event/proof.rs (1)
99-99: Avoid heap allocation inDisplay::fmt.Line 99 allocates a
Stringviadir_path()on every format call. Since both parts are static, write directly to the formatter (or expose a static full path) to keepDisplayallocation-free.♻️ Proposed change
impl fmt::Display for CircuitName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.dir_path()) + write!(f, "{}/{}", self.group(), self.as_str()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/proof.rs` at line 99, The Display implementation currently calls self.dir_path() which allocates a String on every fmt; change Display::fmt for the type implementing fmt::Display to write directly to the formatter without allocation by composing the static parts (or exposing a &'static str) and using write!(f, ...) with those parts instead of calling dir_path(); update or add a helper that returns a &'static str (or write the known static components directly in fmt) so write!(f, "{}", ...) no longer needs to heap-allocate via dir_path().crates/keyshare/src/threshold_keyshare.rs (4)
1246-1344: ShareVerificationComplete handling is coherent; considersaturating_subfor extra safety.The threshold checks on both C2/C3 and C4 completion paths look correct.
Small hardening: computations like
total - msg.dishonest_parties.len() as u64assumedishonest_parties.len() <= total. That should be true given upstream construction, butsaturating_subwould prevent underflow if a future change ever violates that invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 1246 - 1344, In handle_share_verification_complete, replace explicit subtraction that can underflow (e.g., total - msg.dishonest_parties.len() as u64 and similar honest_count calculations) with saturating_sub so an overly-large dishonest_parties.len() cannot underflow; update the C2/C3 branch where honest_count is computed and the C4 branch where honest_count is derived from self.honest_parties (use .len().saturating_sub(...) or total.saturating_sub(msg.dishonest_parties.len() as u64) as appropriate) before comparing to threshold_m and logging/publishing.
365-393: Transient coordination fields are clear; consider avoiding deep clones forpending_shares.The added fields (
pending_shares,pending_c4_proof_data,honest_parties,early_decryption_key_shares) are well documented as non-persisted, and you correctly reset them onE3RequestComplete.Potential perf/memory win (optional):
pending_shares: Vec<ThresholdShare>is filled via deep clones ((**arc).clone()). IfThresholdShareis large (ciphertexts), storingVec<Arc<ThresholdShare>>would avoid copying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 365 - 393, The current ThresholdKeyshare::new initializes pending_shares as Vec<ThresholdShare>, causing deep clones where callers push (**arc).clone(); change pending_shares to Vec<Arc<ThresholdShare>> (and update the struct field type) and update all call sites that currently clone into pending_shares to push the Arc directly (remove (**arc).clone() patterns), ensuring other uses of pending_shares dereference or clone as needed locally; keep semantics of pending_c4_proof_data, honest_parties, and early_decryption_key_shares unchanged.
1979-2041: DecryptionKeyShared routing is sensible; consider logging when collector is absent but expected parties exist.The state-based routing (buffer early during
AggregatingDecryptionKey, forward to collector inReadyForDecryption) makes sense.Minor observability improvement: if you hit the “ReadyForDecryption but no collector” branch, it’d be helpful to also log whether
honest_partiesindicates that a collector should exist (i.e., expected set non-empty). That makes diagnosing “why are we missing the collector?” easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 1979 - 2041, In the ReadyForDecryption branch where decryption_key_shared_collector is None, compute the number (and/or list) of other honest parties from self.honest_parties (exclude state.party_id) and include that count in the warning log so the message indicates whether a collector was expected; update the warn call inside the EnclaveEventData::DecryptionKeyShared handling (the branch that checks KeyshareState::ReadyForDecryption and decryption_key_shared_collector) to include that computed value and a brief context string to aid debugging.
445-470:ensure_decryption_key_shared_collector()should defensively handleexpected.is_empty()to avoid unnecessary timeout failures.If called when
expectedis empty, the collector schedules a 600-second timeout but can never complete (completion only triggers on receiving a message at line 129). It will eventually timeout and fail. The only current call site at line 1696 guards against this withif !expected.is_empty(), but since this method ispub, it should harden defensively—either return an error for empty expected or have the collector immediately emit completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 445 - 470, ensure_decryption_key_shared_collector currently creates a DecryptionKeySharedCollector even when expected (the set of other parties) is empty, which causes a 600s pointless timeout; update ensure_decryption_key_shared_collector to check expected.is_empty() and handle it defensively: either return an Err(anyhow!(...)) indicating no peers expected, or create the collector via DecryptionKeySharedCollector::setup and immediately mark it completed (so it won't wait for messages) before returning; refer to ensure_decryption_key_shared_collector, the local variable expected, and DecryptionKeySharedCollector::setup when making the change.crates/zk-prover/src/actors/proof_request.rs (1)
383-393: Avoid double-hash lookup on DkgShareDecryption routing (optional simplification).
contains_key()followed byremove()insidehandle_decryption_proof_response()does two lookups. Not a big deal, but you could makehandle_decryption_proof_responsereturn a boolean (“handled”) and call it first, falling back to threshold if it returns false. This also makes it harder to accidentally route unknown correlations into the “silent drop” path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/proof_request.rs` around lines 383 - 393, The current DkgShareDecryption routing does a contains_key() on self.decryption_correlation and then calls handle_decryption_proof_response which likely does a remove(), causing two lookups; change the logic so you call handle_decryption_proof_response(&msg.correlation_id, resp.proof.clone()) first and make that method return a bool (e.g., handled) indicating whether it found/removed the correlation; if it returns false, call handle_threshold_proof_response(&msg.correlation_id, resp.proof.clone()); update handle_decryption_proof_response signature and callers accordingly to avoid the redundant contains_key()/remove() double lookup and prevent accidental silent drops.crates/zk-prover/src/actors/share_verification.rs (3)
386-443: Nice defense-in-depth: cross-check dispatched parties against returned ZK results.The “missing from results ⇒ dishonest” rule is a good safety net against partial/malformed responses.
One improvement: if
PartyVerificationResultcan fail without returningfailed_signed_payload, consider falling back topending.party_signed_payloads(or drop storingparty_signed_payloadsentirely if it’s not needed). Right now that field is stored but not used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/share_verification.rs` around lines 386 - 443, In handle_compute_response, when a PartyVerificationResult indicates failure but result.failed_signed_payload is None, fall back to using pending.party_signed_payloads.get(&result.sender_party_id) (or remove that field if unused) before calling emit_signed_proof_failed; i.e., locate the loop over zk_results, replace the current if let Some(ref signed) = result.failed_signed_payload branch with logic that selects signed = result.failed_signed_payload.as_ref().or_else(|| pending.party_signed_payloads.get(&result.sender_party_id)) and then call emit_signed_proof_failed(&pending.e3_id, signed, addr, &pending.ec) using the derived signed and the existing addr lookup from pending.party_addresses.
92-203: ECDSA-first then ZK dispatch flow is solid; remove dead address-insertion branch + treat empty proof sets as dishonest.A couple of small correctness/hardening tweaks:
- In
verify_share_proofs(), theif let Some((_, Some(addr))) = &result.failed_payloadinside theresult.passedbranch is dead code (passed =>failed_payload == None). Removing it reduces confusion.- In
ecdsa_validate_signed_proofs(), an emptysigned_proofsslice currently returns “passed”. That’s dangerous as defense-in-depth—should be an immediate failure (and the party belongs inpre_dishonest/ECDSA-dishonest).
303-385: Avoid double signature recovery: return recovered address fromecdsa_validate_signed_proofson success.The function recovers addresses during ECDSA validation, then recovers again later to populate
party_addresses(lines 149 and 251). Refactorecdsa_validate_signed_proofs()to return the recoveredAddresswhenpassed == trueso you eliminate redundant cryptographic operations and populateparty_addressesdirectly from the validated result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/share_verification.rs` around lines 303 - 385, Update ecdsa_validate_signed_proofs to return the successfully recovered signer address along with the pass/fail result so callers don't re-run recover_address: change the return type (EcdsaPartyResult or its wrapper) to include Option<Address> recovered_addr (populated when passed == true), set recovered_addr = expected_addr.unwrap() before returning success, and keep failed_payload as-is for failures; then update callers that currently call SignedProofPayload::recover_address() (those populating party_addresses) to use the returned recovered_addr instead of performing another signature recovery. Ensure you only set recovered_addr when validation completes successfully and leave it None on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1410-1459: The filter currently only rejects parties when
clone_share(...).num_moduli() mismatches, but treats clone_share(...) == None as
acceptable; update the closure that builds honest_shares so that for each call
to ts.sk_sss.clone_share(idx) and each ts.esi_sss[..].clone_share(idx) you
explicitly check for None, log a warning mentioning the party (use the same warn
message pattern), push ts.party_id onto dimension_excluded, and return false
when clone_share returns None (before checking num_moduli()); keep the existing
num_moduli checks as-is for the Some case. This ensures missing per-party shares
(None) are treated as exclusion just like wrong-dimension shares.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 135-158: PendingDecryptionProofs::is_complete() currently only
compares esm_proofs.len() with expected_esm_count which allows sparse or
non-contiguous indices and can make the later expect(...) in
sign_and_publish_decryption_key_shared() panic; change is_complete() in
PendingDecryptionProofs to explicitly verify that esm_proofs contains every
index in 0..expected_esm_count (e.g., iterate i in 0..expected_esm_count and
check esm_proofs.contains_key(&i) or compare a constructed index set) so the
invariant about contiguous indices is enforced at the check site and removes the
risk of expect panics in sign_and_publish_decryption_key_shared().
- Around line 398-473: The handler handle_decryption_share_proofs_pending should
defensively ignore duplicate DecryptionShareProofsPending for the same e3_id:
before inserting into pending_decryption, check
pending_decryption.contains_key(&e3_id) and if true log/warn and return early to
avoid overwriting the existing PendingDecryptionProofs and corrupting
decryption_correlation mappings; apply this guard at the start of
handle_decryption_share_proofs_pending (using e3_id from msg.e3_id) so the
subsequent creation of CorrelationId values and publishes only occur for new
E3s.
---
Nitpick comments:
In `@crates/events/src/enclave_event/proof.rs`:
- Line 99: The Display implementation currently calls self.dir_path() which
allocates a String on every fmt; change Display::fmt for the type implementing
fmt::Display to write directly to the formatter without allocation by composing
the static parts (or exposing a &'static str) and using write!(f, ...) with
those parts instead of calling dir_path(); update or add a helper that returns a
&'static str (or write the known static components directly in fmt) so write!(f,
"{}", ...) no longer needs to heap-allocate via dir_path().
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1246-1344: In handle_share_verification_complete, replace explicit
subtraction that can underflow (e.g., total - msg.dishonest_parties.len() as u64
and similar honest_count calculations) with saturating_sub so an overly-large
dishonest_parties.len() cannot underflow; update the C2/C3 branch where
honest_count is computed and the C4 branch where honest_count is derived from
self.honest_parties (use .len().saturating_sub(...) or
total.saturating_sub(msg.dishonest_parties.len() as u64) as appropriate) before
comparing to threshold_m and logging/publishing.
- Around line 365-393: The current ThresholdKeyshare::new initializes
pending_shares as Vec<ThresholdShare>, causing deep clones where callers push
(**arc).clone(); change pending_shares to Vec<Arc<ThresholdShare>> (and update
the struct field type) and update all call sites that currently clone into
pending_shares to push the Arc directly (remove (**arc).clone() patterns),
ensuring other uses of pending_shares dereference or clone as needed locally;
keep semantics of pending_c4_proof_data, honest_parties, and
early_decryption_key_shares unchanged.
- Around line 1979-2041: In the ReadyForDecryption branch where
decryption_key_shared_collector is None, compute the number (and/or list) of
other honest parties from self.honest_parties (exclude state.party_id) and
include that count in the warning log so the message indicates whether a
collector was expected; update the warn call inside the
EnclaveEventData::DecryptionKeyShared handling (the branch that checks
KeyshareState::ReadyForDecryption and decryption_key_shared_collector) to
include that computed value and a brief context string to aid debugging.
- Around line 445-470: ensure_decryption_key_shared_collector currently creates
a DecryptionKeySharedCollector even when expected (the set of other parties) is
empty, which causes a 600s pointless timeout; update
ensure_decryption_key_shared_collector to check expected.is_empty() and handle
it defensively: either return an Err(anyhow!(...)) indicating no peers expected,
or create the collector via DecryptionKeySharedCollector::setup and immediately
mark it completed (so it won't wait for messages) before returning; refer to
ensure_decryption_key_shared_collector, the local variable expected, and
DecryptionKeySharedCollector::setup when making the change.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 383-393: The current DkgShareDecryption routing does a
contains_key() on self.decryption_correlation and then calls
handle_decryption_proof_response which likely does a remove(), causing two
lookups; change the logic so you call
handle_decryption_proof_response(&msg.correlation_id, resp.proof.clone()) first
and make that method return a bool (e.g., handled) indicating whether it
found/removed the correlation; if it returns false, call
handle_threshold_proof_response(&msg.correlation_id, resp.proof.clone()); update
handle_decryption_proof_response signature and callers accordingly to avoid the
redundant contains_key()/remove() double lookup and prevent accidental silent
drops.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 386-443: In handle_compute_response, when a
PartyVerificationResult indicates failure but result.failed_signed_payload is
None, fall back to using
pending.party_signed_payloads.get(&result.sender_party_id) (or remove that field
if unused) before calling emit_signed_proof_failed; i.e., locate the loop over
zk_results, replace the current if let Some(ref signed) =
result.failed_signed_payload branch with logic that selects signed =
result.failed_signed_payload.as_ref().or_else(||
pending.party_signed_payloads.get(&result.sender_party_id)) and then call
emit_signed_proof_failed(&pending.e3_id, signed, addr, &pending.ec) using the
derived signed and the existing addr lookup from pending.party_addresses.
- Around line 303-385: Update ecdsa_validate_signed_proofs to return the
successfully recovered signer address along with the pass/fail result so callers
don't re-run recover_address: change the return type (EcdsaPartyResult or its
wrapper) to include Option<Address> recovered_addr (populated when passed ==
true), set recovered_addr = expected_addr.unwrap() before returning success, and
keep failed_payload as-is for failures; then update callers that currently call
SignedProofPayload::recover_address() (those populating party_addresses) to use
the returned recovered_addr instead of performing another signature recovery.
Ensure you only set recovered_addr when validation completes successfully and
leave it None on failure.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/events/src/enclave_event/proof.rscrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/threshold_keyshare.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/share_verification.rs
ctrlc03
reviewed
Feb 28, 2026
ctrlc03
left a comment
ctrlc03
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! just some comments
ctrlc03
reviewed
Feb 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/zk-prover/src/actors/share_verification.rs (1)
58-63: Use stored payloads as fallback for ZK fault attribution.
party_signed_payloadsis stored (Lines 58-63) but not used whenfailed_signed_payloadis absent (Lines 425-431). Add a fallback emission path frompending.party_signed_payloadsso failed parties still get attributed.Also applies to: 425-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/share_verification.rs` around lines 58 - 63, The code stores per-party signed payloads in party_signed_payloads but when failed_signed_payload is None (in the failure handling around failed_signed_payload usage) it doesn't attribute ZK failures; change the emission logic that currently uses failed_signed_payload to fall back to pending.party_signed_payloads: in the handler where SignedProofFailed (or equivalent) is emitted based on failed_signed_payload, check pending.party_signed_payloads for the corresponding party_id(s) and construct/emit the same SignedProofFailed event using those stored SignedProofPayload entries (keep the existing fields and attribution logic identical), ensuring you reference party_signed_payloads, pending.party_signed_payloads, failed_signed_payload and the SignedProofFailed emission site so every failed party gets attributed even when failed_signed_payload is absent.crates/keyshare/src/threshold_keyshare.rs (1)
1201-1204: Use saturating arithmetic forhonest_countchecks.Lines 1203 and 1268 use raw
u64subtraction. Usingsaturating_submakes these threshold guards robust against malformed/duplicated dishonest sets from events.Also applies to: 1266-1269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` around lines 1201 - 1204, The subtraction computing honest_count (let honest_count = total - pre_dishonest.len() as u64) can underflow if pre_dishonest contains malformed/duplicated entries; change this to use saturating_sub (e.g., let honest_count = total.saturating_sub(pre_dishonest.len() as u64)) where state.threshold_n/threshold_m and pre_dishonest are used; apply the same saturating_sub fix to the other occurrence around lines 1266–1269 so all honest_count/threshold guards use saturating arithmetic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 2016-2027: The ReadyForDecryption arm currently warns and drops
incoming DecryptionKeyShared when self.decryption_key_shared_collector is None;
instead ensure the share is not lost by lazily initializing or buffering it:
modify the KeyshareState::ReadyForDecryption(_) handling to (a) call or add a
helper like ensure_decryption_key_shared_collector_initialized() to create and
assign self.decryption_key_shared_collector if possible, and then forward the
event with collector.do_send(TypedEvent::new(data, ec)), or (b) if
initialization cannot happen immediately, push (data, ec) into a new
pending_decryption_shares Vec/queue on self for later delivery and ensure
pending items are flushed when the collector is set; reference the symbols
KeyshareState::ReadyForDecryption, self.decryption_key_shared_collector,
TypedEvent::new, and the new
pending_decryption_shares/ensure_decryption_key_shared_collector_initialized
helper when implementing this change.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 388-400: The unexpected-response branch in the match inside the
verification handler (matching (&pending.kind, msg.response) against
VerificationKind::{ShareProofs, DecryptionProofs} and ComputeResponseKind::Zk
variants) currently logs and returns, which drops the terminal signal; instead,
when hitting the default branch you must still publish a
ShareVerificationComplete event (marking the dispatched parties dishonest) and
clean up the pending state so ThresholdKeyshare cannot stall. Modify the error
path in the handler that processes ZkResponse::VerifyShareProofs /
VerifyShareDecryptionProofs to create and send the ShareVerificationComplete
message for the pending parties (using the same code path/structure used for
normal completion), mark parties as dishonest, and then return—ensuring the
terminal signal is always emitted even on unexpected ComputeResponseKind.
- Around line 85-89: The actor setup currently only subscribes to
EventType::ComputeResponse (in setup / Self::new(bus).start()), so compute
errors (e.g., EventType::ComputeRequestError) never reach the actor and leave
pending verifications unresolved; add a subscription for
EventType::ComputeRequestError in setup
(bus.subscribe(EventType::ComputeRequestError, addr.clone().into())) and
implement or extend the existing handler that processes ComputeResponse (the
compute response handler / pending resolution logic) to also handle
ComputeRequestError events so it clears the same pending entry and publishes
ShareVerificationComplete for that correlation.
---
Nitpick comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1201-1204: The subtraction computing honest_count (let
honest_count = total - pre_dishonest.len() as u64) can underflow if
pre_dishonest contains malformed/duplicated entries; change this to use
saturating_sub (e.g., let honest_count =
total.saturating_sub(pre_dishonest.len() as u64)) where
state.threshold_n/threshold_m and pre_dishonest are used; apply the same
saturating_sub fix to the other occurrence around lines 1266–1269 so all
honest_count/threshold guards use saturating arithmetic.
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 58-63: The code stores per-party signed payloads in
party_signed_payloads but when failed_signed_payload is None (in the failure
handling around failed_signed_payload usage) it doesn't attribute ZK failures;
change the emission logic that currently uses failed_signed_payload to fall back
to pending.party_signed_payloads: in the handler where SignedProofFailed (or
equivalent) is emitted based on failed_signed_payload, check
pending.party_signed_payloads for the corresponding party_id(s) and
construct/emit the same SignedProofFailed event using those stored
SignedProofPayload entries (keep the existing fields and attribution logic
identical), ensuring you reference party_signed_payloads,
pending.party_signed_payloads, failed_signed_payload and the SignedProofFailed
emission site so every failed party gets attributed even when
failed_signed_payload is absent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/events/src/enclave_event/decryption_share_proofs.rscrates/events/src/enclave_event/mod.rscrates/keyshare/src/threshold_keyshare.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/share_verification.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/zk-prover/src/actors/proof_request.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1201-1204: Compute honest_count defensively by bounding the
reported dishonest set against the known total (state.threshold_n) before using
it in threshold decisions: replace direct uses of pre_dishonest.len() when
computing honest_count (e.g., where threshold = state.threshold_m and total =
state.threshold_n) with a guarded value such as let dishonest =
std::cmp::min(pre_dishonest.len() as u64, state.threshold_n); let honest_count =
total.saturating_sub(dishonest); alternatively return an error if
pre_dishonest.len() as u64 > state.threshold_n; apply this same clamp/check in
both places that compute honest_count (the blocks around threshold =
state.threshold_m / total = state.threshold_n and the similar block near the
second occurrence).
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 302-370: In ecdsa_validate_signed_proofs, add a stage-level
validation for signed.payload.proof_type (in addition to the existing
circuit-name check) so proofs from the wrong ZK stage (e.g., C2/C3 vs C4) are
rejected early: determine the allowed stages for this call (derive from the
label or calling context) and call the proof_type's stage/variant (e.g.,
proof_type.stage() or match on proof_type enum) to ensure it belongs to the
allowed set; if it does not, log the mismatch (using the existing info pattern)
and return an EcdsaPartyResult with passed: false and failed_payload set
(similar to other failures). Ensure you reference ecdsa_validate_signed_proofs,
signed.payload.proof_type, label, and failed_payload when implementing the
check.
- Around line 425-438: The loop in share_verification.rs trusts every
result.sender_party_id; update handle_compute_response (the loop over
zk_results) to ignore any zk result whose sender_party_id was never dispatched
by checking membership (e.g., ensure
pending.party_addresses.contains_key(&result.sender_party_id) or the appropriate
dispatched set on pending) before marking dishonest or calling
emit_signed_proof_failed(&pending.e3_id, signed, addr, &pending.ec); only insert
into all_dishonest and emit SignedProofFailed for senders that were actually
dispatched.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/keyshare/src/threshold_keyshare.rscrates/zk-prover/src/actors/share_verification.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/zk-prover/src/actors/share_verification.rs (1)
358-370: Consider adding stage-level validation forproof_type.The current validation ensures the circuit name matches the declared
proof_type, but doesn't verify that theproof_typeitself belongs to the expected stage (C2/C3 vs C4). A malicious party could submit aProofType::C4aSkShareDecryptionduring C2/C3 verification — it would pass the circuit name check but semantically belongs to the wrong stage.While the data flow separation (different dispatch paths) provides some protection, explicit stage validation would add defense-in-depth.
🛡️ Proposed stage validation
fn ecdsa_validate_signed_proofs( &self, sender_party_id: u64, signed_proofs: &[SignedProofPayload], e3_id_str: &str, label: &str, + expected_stage: ExpectedStage, // C2C3 or C4 ) -> EcdsaPartyResult { let mut expected_addr: Option<Address> = None; for signed in signed_proofs { + // Stage validation + let is_valid_stage = match expected_stage { + ExpectedStage::C2C3 => matches!( + signed.payload.proof_type, + ProofType::C2aSkShareComputation + | ProofType::C2bESmShareComputation + | ProofType::C3aSkShareEncryption + | ProofType::C3bESmShareEncryption + ), + ExpectedStage::C4 => matches!( + signed.payload.proof_type, + ProofType::C4aSkShareDecryption + | ProofType::C4bESmShareDecryption + ), + }; + if !is_valid_stage { + info!( + "{} wrong proof stage for party {}: {:?}", + label, sender_party_id, signed.payload.proof_type + ); + return EcdsaPartyResult { + sender_party_id, + passed: false, + failed_payload: Some((signed.clone(), expected_addr)), + }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/share_verification.rs` around lines 358 - 370, Add an explicit stage-level check before the circuit-name check to ensure signed.payload.proof_type belongs to the expected verification stage (e.g., C2/C3 vs C4) so a C4 variant can't be submitted during C2/C3 verification; in the validation block around the existing expected_circuits check in share_verification.rs, inspect signed.payload.proof_type (and/or its enum variants) and reject mismatched stages by logging with label and sender_party_id and returning the same EcdsaPartyResult failure path (passed: false, failed_payload: Some((signed.clone(), expected_addr))). Ensure you enumerate the allowed ProofType variants for this function’s stage and use that set for the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/zk-prover/src/actors/share_verification.rs`:
- Around line 358-370: Add an explicit stage-level check before the circuit-name
check to ensure signed.payload.proof_type belongs to the expected verification
stage (e.g., C2/C3 vs C4) so a C4 variant can't be submitted during C2/C3
verification; in the validation block around the existing expected_circuits
check in share_verification.rs, inspect signed.payload.proof_type (and/or its
enum variants) and reject mismatched stages by logging with label and
sender_party_id and returning the same EcdsaPartyResult failure path (passed:
false, failed_payload: Some((signed.clone(), expected_addr))). Ensure you
enumerate the allowed ProofType variants for this function’s stage and use that
set for the check.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/keyshare/src/threshold_keyshare.rscrates/zk-prover/src/actors/share_verification.rs
ctrlc03
approved these changes
Feb 28, 2026
This was referenced Mar 4, 2026
This was referenced Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores