feat: integrate pk_generation proof in ciphernode [skip-line-limit]#1311
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 introduces end-to-end PK generation proof support across the codebase. It adds new ZK request/response types for PK generation proofs, creates two new event types (PkGenerationProofSigned, ThresholdSharePending), extends the threshold keyshare state machine to store signed proofs, wires PK generation through the zk-prover actor and multithread backend, and updates TRBFV to expose raw polynomial bytes and support pre-generated smudging noise. Contract build IDs are updated. Changes
Sequence DiagramsequenceDiagram
participant KA as KeyshareActor
participant PM as ProofRequestActor
participant MT as Multithread/ZKBackend
participant EH as Events Handler
KA->>KA: GeneratingThresholdShare → AggregatingDecryptionKey
KA->>EH: Emit ThresholdSharePending(e3_id, full_share, proof_request)
EH->>PM: Handle ThresholdSharePending
PM->>PM: Store pending_threshold[correlation_id]
PM->>PM: Build PkGenerationProofRequest from proof_request
PM->>MT: Send ZkRequest::PkGeneration(proof_request)
MT->>MT: Deserialize polynomials from raw bytes
MT->>MT: Convert FH polys to CrtPolynomial
MT->>MT: Build BFV params from threshold preset
MT->>MT: Execute PkGenerationCircuit → proof
MT->>PM: Return PkGenerationProofResponse(proof)
PM->>PM: Sign proof payload
PM->>EH: Emit PkGenerationProofSigned(e3_id, party_id, signed_proof)
PM->>EH: Emit ThresholdShareCreated(e3_id, share) [per party]
EH->>KA: Handle PkGenerationProofSigned
KA->>KA: Store signed_pk_generation_proof
KA->>KA: ReadyForDecryption / Decrypting (with proof)
KA->>EH: Emit KeyshareCreated(..., Some(signed_proof))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
ctrlc03
changed the title
ctrlc03
force-pushed
the
feat/integrate-t1-ciphernode
branch
from
d9182e4 to
5d56275
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
36-43:⚠️ Potential issue | 🟠 Major
unreachable!()will panic onMedium/Large— usetodo!()instead.
MediumandLargeare now public, serializable, and deserializable. Any code path that constructs one of these variants and callsvalues()will panic with a confusing "internal error: entered unreachable code" message. Since these variants are intentionally deferred (per the TODO), usetodo!()orunimplemented!()to clearly signal that these code paths are planned but not yet implemented.Additionally, consider whether
PkGenerationProofRequestshould validatecommittee_sizeearly (e.g., at construction or deserialization) to prevent the panic from propagating deep into the proof generation pipeline.Proposed fix
CiphernodesCommitteeSize::Small => CiphernodesCommittee { n: 5, h: 5, threshold: 2, }, - _ => unreachable!(), + other => todo!("Committee values not yet defined for {:?}", other),
🤖 Fix all issues with AI agents
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Line 802: The code currently hardcodes CiphernodesCommitteeSize::Small which
will produce PK generation proofs tied to the wrong committee size; replace this
literal by deriving the CiphernodesCommitteeSize from the actual
committee/aggregator configuration (e.g., read the committee size or member
count used by the aggregator/Committee struct and map it to the appropriate
CiphernodesCommitteeSize variant) and use that derived value wherever
CiphernodesCommitteeSize::Small appears in threshold_keyshare.rs so that circuit
parameters for pk generation match the real committee size and avoid
verification mismatches.
- Around line 418-447: handle_pk_generation_proof_signed currently only updates
state when try_into::<AggregatingDecryptionKey>() succeeds, so if the actor has
already advanced to ReadyForDecryption the signed proof is silently dropped;
modify handle_pk_generation_proof_signed to accept and store msg.signed_proof in
both AggregatingDecryptionKey and ReadyForDecryption variants (i.e., attempt to
convert current state into AggregatingDecryptionKey and, if that fails, try
ReadyForDecryption) by mutating s.new_state to preserve existing fields and set
signed_pk_generation_proof, and log a warning/error if the state is neither
variant so the failure isn’t silent; reference functions/types:
handle_pk_generation_proof_signed, AggregatingDecryptionKey, ReadyForDecryption,
try_into, KeyshareCreated, handle_calculate_decryption_key_response,
ThresholdSharePending.
In `@crates/trbfv/src/gen_esi_sss.rs`:
- Around line 89-119: The Some(e_sm_raw) branch returns exactly one SharedSecret
while the else branch returns esi_per_ct items, causing a length mismatch; in
the block handling e_sm_raw (around try_poly_from_bytes,
ShareManager::generate_secret_shares_from_poly, and the SharedSecret creation),
either assert that esi_per_ct == 1 to enforce the invariant or expand/replicate
the single generated SharedSecret into a Vec of length esi_per_ct (e.g., clone
or repeat the SharedSecret) so both branches produce the same number of elements
for GenEsiSssResponse.esi_sss.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 108-111: The error handler for publish failures is removing the
correlation ID from the wrong map (self.pending) causing orphaned entries in
self.pending_threshold; change the removal to
self.pending_threshold.remove(&correlation_id) in the branch that handles Err
returned by self.bus.publish(request) so the T1 pending map is correctly cleaned
up (leave the error log as-is or augment if desired).
🧹 Nitpick comments (5)
crates/trbfv/src/gen_pk_share_and_sk_sss.rs (1)
131-151: Variable shadowing ofsk_poly— works correctly but worth a note.
sk_polyfromPublicKeyShare::new_extended(Line 131) is later shadowed by thecoeffs_to_poly_level0result (Line 156). This is safe since the raw bytes are captured on Line 150 before the shadowing. However, renaming one of them (e.g.,sk_poly_raw_sourceorsk_poly_for_sss) would improve readability.crates/multithread/src/multithread.rs (1)
349-418: LGTM — well-structured T1 proof generation handler.The
handle_pk_generation_prooffunction correctly deserializes polynomial data, converts to CRT representation, and generates the proof through theProvabletrait. Error handling is consistent.One minor observation: the
make_zk_errorhelper could also be used inhandle_pk_bfv_proof(line 428-436) to reduce the deserialization error boilerplate there too, but this is purely optional.crates/events/src/enclave_event/compute_request/zk.rs (1)
33-87: LGTM —PkGenerationProofRequestis well-defined.The struct and constructor correctly capture all polynomial inputs needed for T1 proof generation. The
Derivative(Debug)annotations ensure large byte arrays are hex-formatted in debug output.Minor nit: the constructor body initializes fields in a different order (
params_presetbeforee_sm) than the struct declaration (wheree_smprecedesparams_preset). Not a bug (Rust matches by name), but aligning them would improve readability.Optional: align field initialization order
Self { pk0_share: pk0_share.into(), a: a.into(), sk: sk.into(), eek: eek.into(), - params_preset, e_sm: e_sm.into(), + params_preset, committee_size, }crates/keyshare/src/threshold_keyshare.rs (2)
464-466: Unnecessary nested match onZkvariant.The
match zk { _ => Ok(()) }is equivalent to just_ => Ok(())in the outer match. This can be simplified, but it's fine if you're planning to add specific ZK response handling soon.
539-539: Remove commented-out code.This line is dead code from the refactoring. The ESI SSS generation is now triggered from
handle_gen_pk_share_and_sk_sss_response(line 720), so this remnant can be cleaned up.
ctrlc03
force-pushed
the
feat/integrate-t1-ciphernode
branch
from
e4ca009 to
3904bfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
32-54:⚠️ Potential issue | 🟡 Minor
unreachable!()will panic at runtime forMediumandLargevariants.Since
MediumandLargeare now public, serializable, and deserializable, any code path that constructs one of these variants and callsvalues()will panic. This is especially risky since the enum can now be deserialized from external input (e.g., viaPkGenerationProofRequest.committee_size). Consider returningunimplemented!("Medium committee size not yet supported")for a clearer panic message, or better yet, return aResultto handle this gracefully.Minimal improvement: clearer panic messages
match self { CiphernodesCommitteeSize::Small => CiphernodesCommittee { n: 5, h: 5, threshold: 2, }, - _ => unreachable!(), + CiphernodesCommitteeSize::Medium => unimplemented!("Medium committee size not yet configured"), + CiphernodesCommitteeSize::Large => unimplemented!("Large committee size not yet configured"), }
🤖 Fix all issues with AI agents
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 1031-1033: The call handling
EnclaveEventData::PkGenerationProofSigned currently discards errors with "let _
= self.handle_pk_generation_proof_signed(data)"; change this to propagate or log
the Result from handle_pk_generation_proof_signed instead (e.g., match or
.unwrap_or_else/log_on_err) so failures are not silent — update the match arm
handling EnclaveEventData::PkGenerationProofSigned to inspect the Result from
self.handle_pk_generation_proof_signed(data) and emit a processLogger/error/trap
consistent with other handlers when Err or when signed_pk_generation_proof is
None.
🧹 Nitpick comments (6)
crates/zk-prover/src/actors/proof_request.rs (1)
237-248: Both pending maps are correctly drained on error.Minor nit: the
if letblock on lines 244–248 is missing a trailing semicolon, unlike the preceding block on line 237. This compiles fine but is inconsistent.Suggested fix
if let Some(pending) = self.pending_threshold.remove(msg.correlation_id()) { error!( "T1 PkShareGeneration proof request failed for E3 {}: {err} — threshold share will not be published without proof", pending.e3_id - ) + ); }crates/events/src/enclave_event/compute_request/zk.rs (1)
67-87: Field initialization order innew()doesn't match struct definition.The struct defines fields in order:
pk0_share,a,sk,eek,e_sm,params_preset,committee_size. The constructor body initializese_smafterparams_preset(Line 83), mismatching the declaration order. Not a bug, but harms readability.Suggested reorder
Self { pk0_share: pk0_share.into(), a: a.into(), sk: sk.into(), eek: eek.into(), - params_preset, e_sm: e_sm.into(), + params_preset, committee_size, }crates/multithread/src/multithread.rs (1)
349-355:make_zk_erroris only used inhandle_pk_generation_proofbut not inhandle_pk_bfv_proof.Consider using it in
handle_pk_bfv_proofas well (lines 429-436) to reduce the repeated error-construction boilerplate, or keep them consistent by inlining both.crates/keyshare/src/threshold_keyshare.rs (3)
539-539: Remove commented-out code.This line appears to be a leftover from the flow reorganization where
GenEsiSssis now triggered fromhandle_gen_pk_share_and_sk_sss_responseinstead.Suggested fix
- // self.handle_gen_esi_sss_requested(GenEsiSss(current.ciphernode_selected.clone()))?;
605-632: Readiness check after storingesi_sssre-extracts state from persistence — verify no stale-read risk.Lines 608–613 re-read the full
GeneratingThresholdShareDatafrom persistent state immediately after mutating it. This is fine in a single-threaded actix context, but the double extraction (once for mutation at line 596, once at line 608) is slightly wasteful. Consider returning the readiness signal from thetry_mutateclosure to avoid the second read.That said, the bigger concern: if
handle_shares_generated()(line 617) fails, the state is stillGeneratingThresholdShare, buthandle_gen_esi_sss_responsereturnsOk(())without the transition toAggregatingDecryptionKey. The next retry or event would re-enter readiness check and callhandle_shares_generatedagain — which should be idempotent (it only reads and publishes). So no data corruption, but the error is silently swallowed.
464-466: Wildcard match onComputeResponseKind::Zkdiscards all ZK responses.This
_ => Ok(())silently drops any ZK compute response. Currently this is fine since ZK proof responses are handled byProofRequestActor, but if future ZK response variants need handling here, this wildcard will silently swallow them. A brief comment explaining the intent would help.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/config.rs`:
- Around line 16-17: The constant VERSIONS_MANIFEST_URL in
crates/zk-prover/src/config.rs currently points at the feature branch
"feat/integrate-t1-ciphernode" which will be deleted; update the string assigned
to VERSIONS_MANIFEST_URL to a stable reference (e.g., the main branch URL or a
tagged release) so remote fetch in fetch_or_default() will remain valid; locate
the const VERSIONS_MANIFEST_URL and replace the hardcoded feature-branch URL
with the chosen stable URL (or add a comment indicating it must point to
main/tag).
8da5e2a to
e2d8062
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
41-41:⚠️ Potential issue | 🟡 Minor
unreachable!()is misleading for unimplemented variants — usetodo!()instead.
MediumandLargeare public, serializable variants that can be constructed and passed tovalues(). Usingunreachable!()asserts this code path is impossible, which is incorrect.todo!()correctly communicates intent and produces a clearer panic message during debugging.Proposed fix
- _ => unreachable!(), + _ => todo!("Medium and Large committee sizes not yet implemented"),
🧹 Nitpick comments (5)
crates/multithread/src/multithread.rs (1)
349-355:make_zk_errorhelper reduces boilerplate — good extraction.Consider whether this could also be used in
handle_pk_bfv_proof(line 428-435) to further reduce duplication. Not blocking.crates/trbfv/src/gen_pk_share_and_sk_sss.rs (1)
130-156: Variable shadowing ofsk_polyreduces readability.
sk_polyis first bound at line 131 (fromPublicKeyShare::new_extended) and its raw bytes are captured at line 150. Then it is shadowed at line 156 (fromshare_manager.coeffs_to_poly_level0). While functionally correct because the raw bytes are serialized before the shadow, this is easy to misread.Consider renaming to make the two distinct uses clear:
Suggested rename
- let (pk0_share, a, sk_poly, eek) = + let (pk0_share, a, sk_poly_extended, eek) = { PublicKeyShare::new_extended(&sk_share, crp.clone(), &mut *rng.lock().unwrap())? }; ... - let sk_raw = ArcBytes::from_bytes(&sk_poly.to_bytes()); + let sk_raw = ArcBytes::from_bytes(&sk_poly_extended.to_bytes());crates/keyshare/src/threshold_keyshare.rs (3)
539-539: Remove commented-out code.This dead line adds noise. If the ordering change (firing
gen_esi_sssfromhandle_gen_pk_share_and_sk_sss_responseinstead) is intentional and settled, delete the comment.- // self.handle_gen_esi_sss_requested(GenEsiSss(current.ciphernode_selected.clone()))?;
593-631: Readiness-gate logic is sound but tightly coupled tohandle_gen_esi_sss_response.The 5-field readiness check (lines 609-613) followed by
handle_shares_generated()and then the state transition works correctly given the current sequential flow (pk_share_and_sk_sss→ firesesi_sss→esi_sssresponse triggers readiness check). However, if a future change introduces another path that setsesi_sssbeforepk_share_and_sk_sss, this gate won't fire. Consider extracting the readiness check + transition into a shared helper (e.g.,try_advance_from_generating()) to make it reusable.
464-466: Wildcard match onZkcompute responses discards all variants silently.This catch-all
_ => Ok(())means any futureZkresponse variant will be silently ignored. Consider logging attrace!level or using an explicit empty match to get a compiler warning when new variants are added.Suggested change
ComputeResponseKind::Zk(zk) => match zk { - _ => Ok(()), + other => { + trace!("Unhandled Zk response: {:?}", other); + Ok(()) + } },
ctrlc03
changed the title
ctrlc03
force-pushed
the
feat/integrate-t1-ciphernode
branch
from
6990171 to
1bb023a
Compare
hmzakhalid
left a comment
hmzakhalid
left a comment
There was a problem hiding this comment.
Thanks ale, everything look great. Left a few comments
2 participants
fix #1288
verification of the proof by the aggregator is to be done in a later PR (next step is e_sm/sk computation + encryption)
Summary by CodeRabbit
Release Notes
New Features
Chores