feat: verify share decryption proofs#1366
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a DecryptionKeyShared collection and verification workflow: new Actix DecryptionKeySharedCollector actor, renames decryption-proof fields, integrates collector and honest-party tracking into ThresholdKeyshare, and extends the ZK flow with batch VerifyShareDecryptionProofs request/response and per-party verification results. Changes
Sequence DiagramsequenceDiagram
participant Honest as Honest Parties
participant Collector as DecryptionKeySharedCollector
participant ThresholdKS as ThresholdKeyshare
participant ZK as ZK Verifier
participant Exchange4 as Exchange `#4` (Publication)
Honest->>Collector: send DecryptionKeyShared
Collector->>Collector: validate & store
Collector->>Collector: check if all expected collected
alt all collected
Collector->>ThresholdKS: AllDecryptionKeySharesCollected(shares)
ThresholdKS->>ThresholdKS: build VerifyShareDecryptionProofsRequest
ThresholdKS->>ZK: VerifyShareDecryptionProofsRequest
ZK->>ZK: verify SK decryption proof, then ESM proofs per party
ZK->>ThresholdKS: VerifyShareDecryptionProofsResponse(party_results)
ThresholdKS->>ThresholdKS: update honest_parties, decide success/failure
alt enough honest parties
ThresholdKS->>Exchange4: publish KeyshareCreated
else
ThresholdKS->>ThresholdKS: emit E3Failed
end
else timeout
Collector->>ThresholdKS: DecryptionKeySharedCollectionFailed(missing)
ThresholdKS->>ThresholdKS: emit E3Failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (4)
templates/default/tests/integration.spec.ts (1)
165-166: Avoid hardcoded test duration to prevent config drift.Using
225directly makes this integration test easy to desync from runtime config (E3_DURATION) and SDK defaults. Prefer deriving from one shared source (e.g.,DEFAULT_E3_CONFIGor env-driven test config) so timing-sensitive flows stay aligned.♻️ Proposed change
- const duration = 225 + const duration = DEFAULT_E3_CONFIG.duration const inputWindow = await calculateInputWindow(publicClient, duration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` around lines 165 - 166, Replace the hardcoded duration (225) used to compute inputWindow with a shared config or env-derived value so tests track runtime defaults; update the call to calculateInputWindow(publicClient, duration) to instead derive duration from the centralized source (e.g., DEFAULT_E3_CONFIG.duration or parseInt(process.env.E3_DURATION)) and import or reference that constant where calculateInputWindow is invoked so timing-sensitive tests remain aligned with E3_DURATION/SDK defaults.crates/multithread/src/multithread.rs (1)
870-923: Add logging for C4 verification failures to aid debugging.The existing
handle_verify_share_proofsfunction (Lines 826-852) logs verification failures withinfo!messages including the proof type and error. The newhandle_verify_share_decryption_proofssilently marks parties as dishonest without logging which proof failed or the verification error.🔧 Add logging for debugging and parity with existing verification handler
// Verify SK decryption proof let sk_result = prover.verify(&party.sk_decryption_proof, &e3_id_str, sender); match sk_result { Ok(true) => {} - Ok(false) | Err(_) => { + Ok(false) => { + info!( + "C4a SK decryption proof verification failed for party {}", + sender + ); + return PartyShareDecryptionVerificationResult { + sender_party_id: sender, + all_verified: false, + }; + } + Err(e) => { + info!( + "C4a SK decryption proof verification error for party {}: {}", + sender, e + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } // Verify all ESM decryption proofs - for esm_proof in &party.esm_decryption_proofs { + for (idx, esm_proof) in party.esm_decryption_proofs.iter().enumerate() { let result = prover.verify(esm_proof, &e3_id_str, sender); match result { Ok(true) => continue, - Ok(false) | Err(_) => { + Ok(false) => { + info!( + "C4b ESM decryption proof[{}] verification failed for party {}", + idx, sender + ); + return PartyShareDecryptionVerificationResult { + sender_party_id: sender, + all_verified: false, + }; + } + Err(e) => { + info!( + "C4b ESM decryption proof[{}] verification error for party {}: {}", + idx, sender, e + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 870 - 923, The function handle_verify_share_decryption_proofs silently returns PartyShareDecryptionVerificationResult::all_verified = false on prover.verify failures; add logging similar to handle_verify_share_proofs to record which proof (SK or ESM), the sender_party_id and the verification error or false result to aid debugging. Instrument the sk_result match branch and the esm_proof loop's match branch to call info! or warn! with contextual fields (e.g., "sk_decryption_proof" vs "esm_decryption_proof", sender_party_id, e3_id_str) and the error value when Err(_) or Ok(false) occurs before returning the dishonest PartyShareDecryptionVerificationResult. Ensure log messages mirror the style and level used in handle_verify_share_proofs for parity.crates/keyshare/src/decryption_key_shared_collector.rs (1)
129-144: Consider stopping the actor after successful collection.When all shares are collected (Lines 129-143), the actor sends
AllDecryptionKeySharesCollectedto the parent but continues running. Unlike the timeout path (Line 183), there's noctx.stop()call. This means the actor could receive additional messages after completing its task.Compare with the timeout handler which properly stops the actor at Line 183.
🔧 Stop actor after successful collection
let event: TypedEvent<AllDecryptionKeySharesCollected> = TypedEvent::new( AllDecryptionKeySharesCollected { shares: std::mem::take(&mut self.shares), }, ec, ); self.parent.do_send(event); + ctx.stop(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/decryption_key_shared_collector.rs` around lines 129 - 144, The actor never stops after successfully collecting all shares: in the branch where self.expected.is_empty() and you set self.state = CollectorState::Finished and send the AllDecryptionKeySharesCollected event via self.parent.do_send(...), call ctx.stop() (similar to the timeout path) so the actor is terminated and won't process further messages; you can keep canceling the timeout future via ctx.cancel_future(handle) as you already do (timeout_handle/ctx.cancel_future), then call ctx.stop() immediately after sending the event.crates/events/src/enclave_event/compute_request/zk.rs (1)
313-320: Consider adding failure details for fault attribution.
PartyShareDecryptionVerificationResultlacksfailed_proof_typeandfailed_signed_payloadfields that are present in the analogousPartyVerificationResult(Lines 274-284). This limits the ability to identify which C4 proof failed and to perform fault attribution.If fault attribution for C4 failures is planned for future work (similar to the deferred signature verification noted in learnings), this simpler structure is fine. Otherwise, consider aligning with the more detailed result structure.
🔧 Optional: Add failure details for parity with PartyVerificationResult
/// Verification result for C4 proofs from a single sender. #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct PartyShareDecryptionVerificationResult { /// The party whose C4 proofs were verified. pub sender_party_id: u64, /// Whether ALL C4 proofs from this party verified successfully. pub all_verified: bool, + /// If any C4 proof failed: which type (C4a or C4b). + pub failed_proof_type: Option<ProofType>, + /// If any C4 proof failed: the proof that failed (for fault attribution). + pub failed_proof: Option<Proof>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/compute_request/zk.rs` around lines 313 - 320, PartyShareDecryptionVerificationResult currently only carries sender_party_id and all_verified; add the same failure-detail fields as PartyVerificationResult so C4 faults can be attributed by including failed_proof_type and failed_signed_payload (both optional) on PartyShareDecryptionVerificationResult. Update its definition (struct PartyShareDecryptionVerificationResult) to include Option<FailedProofType> failed_proof_type and Option<SignedPayload> failed_signed_payload (or the corresponding types used by PartyVerificationResult), and ensure serialization/derivation remains consistent with the other struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/default/enclave.config.yaml`:
- Around line 20-22: Duplicate "e3_program" entries under the contracts mapping
cause ambiguous YAML parsing; remove one of the duplicate e3_program keys so
only a single e3_program mapping remains, and if the two entries differ merge
the correct address and deploy_block values into that single e3_program entry
(ensure the final mapping contains the intended address and deploy_block for
e3_program).
---
Nitpick comments:
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 313-320: PartyShareDecryptionVerificationResult currently only
carries sender_party_id and all_verified; add the same failure-detail fields as
PartyVerificationResult so C4 faults can be attributed by including
failed_proof_type and failed_signed_payload (both optional) on
PartyShareDecryptionVerificationResult. Update its definition (struct
PartyShareDecryptionVerificationResult) to include Option<FailedProofType>
failed_proof_type and Option<SignedPayload> failed_signed_payload (or the
corresponding types used by PartyVerificationResult), and ensure
serialization/derivation remains consistent with the other struct.
In `@crates/keyshare/src/decryption_key_shared_collector.rs`:
- Around line 129-144: The actor never stops after successfully collecting all
shares: in the branch where self.expected.is_empty() and you set self.state =
CollectorState::Finished and send the AllDecryptionKeySharesCollected event via
self.parent.do_send(...), call ctx.stop() (similar to the timeout path) so the
actor is terminated and won't process further messages; you can keep canceling
the timeout future via ctx.cancel_future(handle) as you already do
(timeout_handle/ctx.cancel_future), then call ctx.stop() immediately after
sending the event.
In `@crates/multithread/src/multithread.rs`:
- Around line 870-923: The function handle_verify_share_decryption_proofs
silently returns PartyShareDecryptionVerificationResult::all_verified = false on
prover.verify failures; add logging similar to handle_verify_share_proofs to
record which proof (SK or ESM), the sender_party_id and the verification error
or false result to aid debugging. Instrument the sk_result match branch and the
esm_proof loop's match branch to call info! or warn! with contextual fields
(e.g., "sk_decryption_proof" vs "esm_decryption_proof", sender_party_id,
e3_id_str) and the error value when Err(_) or Ok(false) occurs before returning
the dishonest PartyShareDecryptionVerificationResult. Ensure log messages mirror
the style and level used in handle_verify_share_proofs for parity.
In `@templates/default/tests/integration.spec.ts`:
- Around line 165-166: Replace the hardcoded duration (225) used to compute
inputWindow with a shared config or env-derived value so tests track runtime
defaults; update the call to calculateInputWindow(publicClient, duration) to
instead derive duration from the centralized source (e.g.,
DEFAULT_E3_CONFIG.duration or parseInt(process.env.E3_DURATION)) and import or
reference that constant where calculateInputWindow is invoked so
timing-sensitive tests remain aligned with E3_DURATION/SDK defaults.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
crates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryption_key_shared.rscrates/keyshare/src/decryption_key_shared_collector.rscrates/keyshare/src/lib.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rsexamples/CRISP/scripts/dev.shexamples/CRISP/server/.env.exampletemplates/default/deployed_contracts.jsontemplates/default/enclave.config.yamltemplates/default/server/input.tstemplates/default/tests/integration.spec.ts
ctrlc03
force-pushed
the
feat/t3-integration
branch
from
c576e1c to
b2004ad
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/multithread/src/multithread.rs (1)
870-923: Consider adding logging for verification failures.The
handle_verify_share_proofsfunction logs verification failures viainfo!()(lines 829-843), but this handler silently returnsall_verified: falsewithout logging. Adding similar logging would aid debugging in production.🔧 Suggested logging additions
// Verify SK decryption proof let sk_result = prover.verify(&party.sk_decryption_proof, &e3_id_str, sender); match sk_result { Ok(true) => {} Ok(false) | Err(_) => { + info!( + "C4 SK decryption proof verification failed for party {}", + sender + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } // Verify all ESM decryption proofs for esm_proof in &party.esm_decryption_proofs { let result = prover.verify(esm_proof, &e3_id_str, sender); match result { Ok(true) => continue, Ok(false) | Err(_) => { + info!( + "C4 ESM decryption proof verification failed for party {}", + sender + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 870 - 923, The handler handle_verify_share_decryption_proofs currently returns PartyShareDecryptionVerificationResult { all_verified: false } silently when prover.verify fails; add logging analogous to the one in handle_verify_share_proofs using info!() (or appropriate log level) to record the failure including e3_id_str, sender (party.sender_party_id), whether the failed proof was the SK or an ESM (include index or identifier), and the error or boolean result from prover.verify before returning the failure result; place logs in both the SK decryption proof match branch and inside the ESM loop where result is Ok(false) or Err(_).
🤖 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/multithread/src/multithread.rs`:
- Around line 870-923: The handler handle_verify_share_decryption_proofs
currently returns PartyShareDecryptionVerificationResult { all_verified: false }
silently when prover.verify fails; add logging analogous to the one in
handle_verify_share_proofs using info!() (or appropriate log level) to record
the failure including e3_id_str, sender (party.sender_party_id), whether the
failed proof was the SK or an ESM (include index or identifier), and the error
or boolean result from prover.verify before returning the failure result; place
logs in both the SK decryption proof match branch and inside the ESM loop where
result is Ok(false) or Err(_).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryption_key_shared.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rstemplates/default/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/events/src/enclave_event/compute_request/mod.rs
ctrlc03
force-pushed
the
feat/t3-integration
branch
from
b2004ad to
a88bb82
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/events/src/enclave_event/compute_request/zk.rs (1)
313-320: Consider adding fault attribution fields for future work.
PartyShareDecryptionVerificationResultomitsfailed_proof_typeandfailed_signed_payloadfields that are present inPartyVerificationResult(lines 274-284). While the simpler structure works for the current flow, adding these fields would enable future fault attribution for C4 proof failures, similar to C2/C3 verification.This is a minor observation — the current design is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/compute_request/zk.rs` around lines 313 - 320, PartyShareDecryptionVerificationResult currently only contains sender_party_id and all_verified, so it can't carry fault-attribution data like PartyVerificationResult; add optional fields failed_proof_type: Option<ProofType> and failed_signed_payload: Option<SignedPayload> (or similarly-typed optional fields used by PartyVerificationResult) to PartyShareDecryptionVerificationResult so C4 verification failures can record the failing proof type and payload for future attribution; update related serializers/deserializers and any construction sites of PartyShareDecryptionVerificationResult (search for its usages) to populate these fields where available.crates/multithread/src/multithread.rs (1)
883-906: Add logging for verification failures to aid debugging.Unlike
handle_verify_share_proofs(lines 829-851), which logs when proof verification fails or errors, this function silently returnsall_verified: false. Addinginfo!orwarn!logs would help diagnose C4 proof issues in production.🔧 Proposed logging additions
// Verify SK decryption proof let sk_result = prover.verify(&party.sk_decryption_proof, &e3_id_str, sender); match sk_result { Ok(true) => {} Ok(false) | Err(_) => { + info!( + "SK decryption proof verification failed for party {} in E3 {}", + sender, e3_id_str + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } // Verify all ESM decryption proofs for esm_proof in &party.esm_decryption_proofs { let result = prover.verify(esm_proof, &e3_id_str, sender); match result { Ok(true) => continue, Ok(false) | Err(_) => { + info!( + "ESM decryption proof verification failed for party {} in E3 {}", + sender, e3_id_str + ); return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false, }; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/multithread/src/multithread.rs` around lines 883 - 906, The verification blocks for SK and ESM decryption proofs currently return PartyShareDecryptionVerificationResult { sender_party_id: sender, all_verified: false } silently on Ok(false) or Err(_); update both places (the sk_result check using party.sk_decryption_proof and the loop over party.esm_decryption_proofs calling prover.verify) to log the failure/error before returning (use info! or warn!), include the sender id, which proof failed (SK vs ESM) and the underlying error when Err(_) is returned, and keep the existing return value; mirror the logging style used in handle_verify_share_proofs (lines 829-851) for consistency.
🤖 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/events/src/enclave_event/compute_request/zk.rs`:
- Around line 313-320: PartyShareDecryptionVerificationResult currently only
contains sender_party_id and all_verified, so it can't carry fault-attribution
data like PartyVerificationResult; add optional fields failed_proof_type:
Option<ProofType> and failed_signed_payload: Option<SignedPayload> (or
similarly-typed optional fields used by PartyVerificationResult) to
PartyShareDecryptionVerificationResult so C4 verification failures can record
the failing proof type and payload for future attribution; update related
serializers/deserializers and any construction sites of
PartyShareDecryptionVerificationResult (search for its usages) to populate these
fields where available.
In `@crates/multithread/src/multithread.rs`:
- Around line 883-906: The verification blocks for SK and ESM decryption proofs
currently return PartyShareDecryptionVerificationResult { sender_party_id:
sender, all_verified: false } silently on Ok(false) or Err(_); update both
places (the sk_result check using party.sk_decryption_proof and the loop over
party.esm_decryption_proofs calling prover.verify) to log the failure/error
before returning (use info! or warn!), include the sender id, which proof failed
(SK vs ESM) and the underlying error when Err(_) is returned, and keep the
existing return value; mirror the logging style used in
handle_verify_share_proofs (lines 829-851) for consistency.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/events/src/enclave_event/compute_request/mod.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/decryption_key_shared.rscrates/keyshare/src/threshold_keyshare.rscrates/multithread/src/multithread.rscrates/test-helpers/src/lib.rscrates/tests/tests/integration.rstemplates/default/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/events/src/enclave_event/compute_request/mod.rs
hmzakhalid
approved these changes
Feb 28, 2026
This was referenced Mar 25, 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
Tests
Chores