feat: integrate share proofs in ciphernode [skip-line-limit]#1334
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:
📝 WalkthroughWalkthroughIntegrates T2 share-computation circuits into the threshold key generation flow: adds ShareComputation ZK request/response and signed-event types, wires multi-proof orchestration in the zk-prover, extends keyshare state to track signed proofs, and implements the multithread prover path to generate share-computation proofs. Changes
Sequence DiagramsequenceDiagram
participant KS as KeyShare (ThresholdKeyshare)
participant PR as ProofRequest Actor
participant ZK as ZK Prover (Multithread)
participant EV as Event Bus (EnclaveEventData)
KS->>PR: handle_threshold_share_pending (ThresholdSharePending with 3 requests)
activate PR
PR->>PR: create correlations for PkGen, SkShareComp, ESmShareComp
PR->>ZK: ZkRequest::PkGeneration
PR->>ZK: ZkRequest::ShareComputation (Sk)
PR->>ZK: ZkRequest::ShareComputation (ESm)
deactivate PR
activate ZK
ZK->>ZK: handle_pk_generation_proof
ZK-->>PR: ZkResponse::PkGeneration
ZK->>ZK: handle_share_computation_proof (decrypt → deserialize → parity → prove)
ZK-->>PR: ZkResponse::ShareComputation
ZK->>ZK: handle_share_computation_proof (second)
ZK-->>PR: ZkResponse::ShareComputation
deactivate ZK
activate PR
PR->>PR: handle_threshold_proof_response (resolve by correlation)
PR->>PR: sign_proof for each proof
PR->>EV: Publish ShareComputationProofSigned (Sk)
PR->>EV: Publish ShareComputationProofSigned (ESm)
PR->>EV: Publish ThresholdShareCreated (with signed proofs)
deactivate PR
activate KS
EV->>KS: ShareComputationProofSigned (Sk)
KS->>KS: handle_share_computation_proof_signed (store signed_sk_share_computation_proof)
EV->>KS: ShareComputationProofSigned (ESm)
KS->>KS: handle_share_computation_proof_signed (store signed_e_sm_share_computation_proof)
EV->>KS: ThresholdShareCreated
KS->>KS: transition → ReadyForDecryption / Decrypting (include proofs)
deactivate KS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/trbfv/src/gen_pk_share_and_sk_sss.rs (1)
59-111:⚠️ Potential issue | 🔴 CriticalUnencrypted sensitive cryptographic material persisted to storage — ProofRequestData requires encryption or secure handling.
The
sk_raw,eek_raw,pk0_share_raw, anda_rawfields stored inProofRequestDataare sensitive cryptographic material (secret key polynomial, error polynomial, etc.) serialized as unencryptedArcBytes. These fields are not only held in memory but are persisted to storage via thePersistable<ThresholdKeyshareState>wrapper (threshold_keyshare.rs:322), whereGeneratingThresholdShareDatacontainingProofRequestDatais part of the state hierarchy.Unlike
sk_ssswhich is encrypted viaCipher, these raw polynomial fields pass through unencrypted both in memory and at rest in the database. This creates a significant security exposure for cryptographic secrets.Consider encrypting
ProofRequestDatafields or storing them only transiently without persistence, similar to howsk_sssis handled.crates/zk-helpers/src/ciphernodes_committee.rs (1)
41-41:⚠️ Potential issue | 🟠 Major
unreachable!()becomes reachable now that the enum is deserializable.With
Serialize/Deserializederived, aMediumorLargevariant can now arrive via deserialization (e.g., from config or network messages), hitting theunreachable!()and panicking at runtime. Consider returning an error or a default instead.Proposed fix
- _ => unreachable!(), + other => unimplemented!("Committee size {:?} is not yet supported", other),
🤖 Fix all issues with AI agents
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 430-502: The handlers handle_pk_generation_proof_signed and
handle_share_computation_proof_signed currently assume state::try_into() yields
AggregatingDecryptionKey and will fail (dropping proofs) if the state already
transitioned (e.g., KeyshareState::ReadyForDecryption or
KeyshareState::Decrypting); modify both handlers to accept and update
AggregatingDecryptionKey when it's nested inside those later states (or maintain
a pending_proofs container in the root state) instead of unconditionally calling
try_into(): pattern-match s (the KeyshareState) to find AggregatingDecryptionKey
in KeyshareState::AggregatingDecryptionKey or inside
KeyshareState::ReadyForDecryption / KeyshareState::Decrypting, update the inner
AggregatingDecryptionKey fields (signed_pk_generation_proof or the
signed_*_share_computation_proof set) and write the new state back; ensure you
do not propagate try_into errors for these expected transitions—log unexpected
states but persist the proof so it’s present when KeyshareCreated is emitted.
- Around line 894-901: The T2b proof request currently uses esi_sss_raw[0] which
both can panic if esi_sss_raw is empty and omits proofs for ciphertexts beyond
the first; update the construction of ShareComputationProofRequest
(e_sm_share_computation_request) to handle all ciphertexts: either
change/overload the request to accept all serialized shares (use
secret_sss_raw_list or similar) and pass esi_sss_raw (the full Vec) in place of
secret_sss_raw, or if the protocol only requires a single proof, add an explicit
non-empty check/assert on esi_sss_raw (and validate num_ciphertexts==1) before
accessing [0] and document that constraint; adjust call sites that build/consume
ShareComputationProofRequest accordingly and ensure encrypted_esi_sss /
esi_sss_raw generation and usages (and any loop that encrypts shares) are
consistent with the chosen fix.
In `@crates/trbfv/src/gen_esi_sss.rs`:
- Line 88: The info log "gen_esi_sss:generate_smudging_error..." is misleading;
update the log call near the generate_secret_shares_from_poly invocation to
accurately describe the operation (e.g.,
"gen_esi_sss:generate_secret_shares_from_poly started" or similar) so it matches
the function generate_secret_shares_from_poly and current behavior; locate the
info! invocation in gen_esi_sss.rs and replace the message text accordingly.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 163-197: The T2a/T2b publish error paths currently only log errors
and leave entries in threshold_correlation and pending_threshold, causing
orphaned state; update the Err branches for the ComputeRequest::zk calls that
publish ZkRequest::ShareComputation (for variables t2a_corr and t2b_corr) to
mirror the T1 (PkGeneration) failure handling: remove the correlation entry from
self.threshold_correlation (using t2a_corr/t2b_corr) and remove the pending
entry keyed by e3_id from self.pending_threshold, log the error, and then return
early (or propagate the error) so the method exits without leaving the pending
state. Ensure you reference the existing symbols threshold_correlation,
pending_threshold, t2a_corr, t2b_corr, e3_id, and the Publish call locations
when applying the fix.
🧹 Nitpick comments (12)
examples/CRISP/test/crisp.spec.ts (1)
113-115: Consider replacing the hard sleep with a polling loop, similar towaitForE3Ready.A 130-second
waitForTimeoutis brittle — it either wastes time when things are fast or flakes when they're slow. A polling approach (e.g., periodically checking for the expected "Historic polls" state) would make this more resilient, especially as proof generation times may continue to change.That said, this is a pre-existing pattern and the bump itself is justified by the added share computation overhead.
crates/tests/tests/integration.rs (1)
103-165: Consider extracting a small helper to reduce repetitive fixture copying.Each circuit setup follows the same pattern:
create_dir_all→ copy.json→ copy.vk. With five circuits now, a small closure or helper function would reduce boilerplate and make it easier to add future circuits.♻️ Optional: extract a helper
+ // Helper to copy circuit fixtures + let copy_circuit = |circuit_dir: PathBuf, name: &str| { + let fixtures = fixtures_dir.clone(); + async move { + tokio::fs::create_dir_all(&circuit_dir).await.unwrap(); + tokio::fs::copy( + fixtures.join(format!("{name}.json")), + circuit_dir.join(format!("{name}.json")), + ) + .await + .unwrap(); + tokio::fs::copy( + fixtures.join(format!("{name}.vk")), + circuit_dir.join(format!("{name}.vk")), + ) + .await + .unwrap(); + } + }; + + copy_circuit(circuits_dir.join("dkg").join("pk"), "pk").await; + copy_circuit(circuits_dir.join("threshold").join("pk_generation"), "pk_generation").await; + copy_circuit(circuits_dir.join("dkg").join("sk_share_computation"), "sk_share_computation").await; + copy_circuit(circuits_dir.join("dkg").join("e_sm_share_computation"), "e_sm_share_computation").await;crates/zk-prover/src/circuits/utils.rs (1)
35-44: Code correctly handles negative integers; minor ordering inconsistency with coefficient parsing.The concern about negative
i64values is unfounded—FieldElement::try_from_strcorrectly parses negative decimal strings by reducing them modulop(via arkworks field arithmetic), so passing"-5"is safe and requires no fallback.One minor consistency improvement: here
u64is checked beforei64(lines 35, 40), but the coefficient handler checksi64thenu64(lines 63–64). Aligning the order would improve readability and consistency.crates/trbfv/src/shares/shares.rs (1)
24-26: Consider returning&[Array2<u64>]instead of&Vec<Array2<u64>>.Idiomatic Rust prefers returning a slice reference (
&[T]) from getters rather than&Vec<T>, as it decouples the public API from the internal storage type.♻️ Proposed change
- pub fn moduli_data(&self) -> &Vec<Array2<u64>> { + pub fn moduli_data(&self) -> &[Array2<u64>] { &self.moduli_data }crates/trbfv/src/gen_esi_sss.rs (1)
32-39:TryFromis now infallible — could be simplified toFrom.Since the conversion from
GenEsiSssRequesttoInnerRequestno longer performs any fallible operation (it was previously convertingerror_sizefrom bytes toBigUint),TryFromcan be replaced withFrom. This is a minor nit and can be deferred.crates/test-helpers/src/usecase_helpers.rs (1)
74-75: Hardcodedlambda: 40andnum_ciphertexts: 1— consider making these parameters.These values are fine for testing, but if different tests need different security parameters or multi-ciphertext scenarios, accepting them as function arguments would improve reusability. Low priority since this is a test helper.
crates/multithread/src/multithread.rs (1)
430-434: Consider usingtry_into()instead ofas u32for the committee size casts.
committee.n as u32andcommittee.threshold as u32will silently truncate if the source type isusizeon a 64-bit platform. While committee sizes are practically small, usingu32::try_from(committee.n).map_err(...)would be consistent with the defensive error handling throughout this function.♻️ Suggested safer cast
let circuit_data = ShareComputationCircuitData { dkg_input_type: req.dkg_input_type, secret, secret_sss, parity_matrix, - n_parties: committee.n as u32, - threshold: committee.threshold as u32, + n_parties: u32::try_from(committee.n) + .map_err(|e| make_zk_error(&request, format!("n_parties overflow: {}", e)))?, + threshold: u32::try_from(committee.threshold) + .map_err(|e| make_zk_error(&request, format!("threshold overflow: {}", e)))?, };crates/zk-prover/src/actors/proof_request.rs (1)
259-360:publish_threshold_share_with_proofs— partial publish on signing/publish failure.If signing the first proof succeeds but a subsequent sign or bus publish fails, the function returns early without any rollback. For example, if
signed_sk_sharesigning fails (line 277), the function returns without publishing anything — which is fine. But if theThresholdShareCreatedloop (lines 305-324) partially publishes some shares before a laterPkGenerationProofSignedpublish fails (line 334), some parties will receive shares but the signed proofs won't follow.This is likely acceptable for now since the threshold protocol requires all proofs to verify, so incomplete data won't be accepted downstream. But it's worth noting for reliability.
crates/events/src/enclave_event/compute_request/zk.rs (1)
87-107: Nit: field initialization order inPkGenerationProofRequest::newdiffers from declaration order.The struct declares fields as
pk0_share, a, sk, eek, e_sm, params_preset, committee_sizebut the constructor body assignse_smafterparams_preset(line 103). This is valid Rust but reduces 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/keyshare/src/threshold_keyshare.rs (3)
882-882: HardcodedCiphernodesCommitteeSize::Small.The TODO is noted but this is used in three places (lines 882, 891, 900) for proof request construction. Worth tracking as a follow-up to derive from configuration.
Do you want me to open an issue to track deriving
CiphernodesCommitteeSizefrom configuration?
766-793: Two-phase state mutation inhandle_gen_esi_sss_responseis correct but subtle.The code first stores
esi_sssinGeneratingThresholdShareData, then checks readiness, callshandle_shares_generated(which reads from the same state), and then transitions toAggregatingDecryptionKey. This ordering is intentional and correct —handle_shares_generatedneeds the fullGeneratingThresholdShareData— but it's worth a brief comment at line 776 explaining why the shares-generated handler is called before the state transition.
1147-1152: Errors from proof-signed handlers are silently discarded.
let _ =discards theResultfrom bothhandle_pk_generation_proof_signedandhandle_share_computation_proof_signed. While this is consistent with the pattern used forhandle_threshold_share_createdandhandle_encryption_key_createdin this match block, given the ordering concern noted above, these errors may indicate a real problem (state mismatch). Consider at minimum logging a warning on failure, similar to howE3Failedis handled with awarn!.Proposed improvement
EnclaveEventData::PkGenerationProofSigned(data) => { - let _ = self.handle_pk_generation_proof_signed(TypedEvent::new(data, ec)); + if let Err(err) = self.handle_pk_generation_proof_signed(TypedEvent::new(data, ec)) { + warn!("Failed to handle PkGenerationProofSigned: {err}"); + } } EnclaveEventData::ShareComputationProofSigned(data) => { - let _ = self.handle_share_computation_proof_signed(TypedEvent::new(data, ec)); + if let Err(err) = self.handle_share_computation_proof_signed(TypedEvent::new(data, ec)) { + warn!("Failed to handle ShareComputationProofSigned: {err}"); + } }
ctrlc03
force-pushed
the
feat/integrate-t1-share-computation
branch
from
c85caf7 to
71af852
Compare
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with 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 line 27-40: Add a constructor for ShareComputationProofRequest to match
the API pattern used by PkBfvProofRequest::new and
PkGenerationProofRequest::new: implement an associated function pub fn
new(secret_raw: SensitiveBytes, secret_sss_raw: SensitiveBytes, dkg_input_type:
DkgInputType, params_preset: BfvPreset, committee_size:
CiphernodesCommitteeSize) -> Self that returns a ShareComputationProofRequest
populated from the parameters (and perform any basic validation or
transformation here if needed), and add docs/tests as appropriate so callers use
ShareComputationProofRequest::new instead of direct struct construction.
- Around line 93-101: The returned Self initializer in the constructor/impl
lists fields in an order that doesn’t match the struct declaration: swap the
positions of params_preset and e_sm so the initializer matches the struct's
declared field order (keep pk0_share, a, sk, eek as-is, then params_preset,
e_sm, committee_size) in the function where Self { pk0_share, a, sk, eek,
params_preset, e_sm, committee_size } is returned.
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 696-708: The code silently skips creating and dispatching
GenEsiSss when GeneratingThresholdShareData.ciphernode_selected is None, which
can stall the state machine; update the branch that currently only handles
Some(ciphernode_selected) around self.state.try_get()?.try_into()? and
handle_gen_esi_sss_requested to either log a warning and return an error (or
transition the state) when ciphernode_selected is None so the unexpected
condition is surfaced; ensure the e_sm_raw unwrap remains validated
(e_sm_raw.expect(...)) or convert to a propagated error if missing, referencing
GeneratingThresholdShareData, ciphernode_selected, handle_gen_esi_sss_requested,
and GenEsiSss to locate the logic to change.
- Line 883: Replace the hardcoded CiphernodesCommitteeSize::Small used in the
three proof request constructions with a value derived from actual committee
configuration (e.g., compute from threshold_n or the E3 configuration object) so
circuit selection matches runtime committee size; locate the three occurrences
of CiphernodesCommitteeSize::Small in threshold_keyshare.rs (used in the proof
request builders) and change them to read from the committee size provider
(e.g., a function or field like threshold_n or e3_config.get_committee_size()),
add a small helper or use existing metadata to map that numeric committee size
to the CiphernodesCommitteeSize enum, and ensure the new value is validated or
defaulted if the config is missing.
- Around line 1148-1153: The two branches handling
EnclaveEventData::PkGenerationProofSigned and
EnclaveEventData::ShareComputationProofSigned currently discard errors with "let
_ =", hiding failures from handle_pk_generation_proof_signed and
handle_share_computation_proof_signed; update these branches to surface errors
like other handlers by chaining the call to .trap(...) with a clear context
message (e.g., "pk generation proof signed handling" and "share computation
proof signed handling") or at minimum match the Result and log the Err using the
module logger before returning, referencing the functions
handle_pk_generation_proof_signed, handle_share_computation_proof_signed and the
enum variants so failures are not silently suppressed.
- Around line 490-498: The match's `other` arm currently logs a warning but
leaves `current` unchanged and still calls
`s.new_state(KeyshareState::AggregatingDecryptionKey(updated))`, causing an
unnecessary persist; update the logic in the `ShareComputationProofSigned`
handling so that when the `other` (unexpected `ProofType`) branch is hit you
short-circuit (e.g., return an Err or otherwise exit the handler) instead of
falling through to `s.new_state`, or alternatively set a flag and skip calling
`s.new_state` if `updated` is identical to `current`; locate the match over
`ProofType`, the `other` arm, the `current` identifier, and the
`s.new_state(KeyshareState::AggregatingDecryptionKey(updated))` call and
implement the early return/skip to avoid redundant persistence.
- Around line 105-132: The fields signed_sk_share_computation_proof and
signed_e_sm_share_computation_proof are carried in AggregatingDecryptionKey,
ReadyForDecryption, and Decrypting but never consumed; either remove these
fields (and their serde derives) from the three structs to avoid serializing
dead data, or explicitly consume/publish them where the other proof is used
(e.g., when constructing/emitting KeyshareCreated) and add a TODO comment
explaining future use. Locate the fields named signed_sk_share_computation_proof
and signed_e_sm_share_computation_proof inside the structs
AggregatingDecryptionKey, ReadyForDecryption, and Decrypting and either delete
those field declarations and related uses, or propagate them into the
KeyshareCreated emission/handling code (where signed_pk_generation_proof is
used) so they are read/published, then mark with a TODO if kept for future
features.
In `@crates/multithread/src/multithread.rs`:
- Around line 391-470: The inline step comments in
handle_share_computation_proof and handle_pk_generation_proof are mis-numbered
(duplicate "4" in handle_share_computation_proof and duplicate "3" in
handle_pk_generation_proof); update the comment tokens so steps are sequential
(for handle_share_computation_proof change the second "4" to "5" and increment
subsequent step numbers accordingly, and for handle_pk_generation_proof change
the second "3" to "4" and renumber following steps) while leaving code logic
unchanged — search for the functions handle_share_computation_proof and
handle_pk_generation_proof to locate the comment lines to edit.
- Around line 425-430: Replace the closure currently used to convert elements
when building secret_sss with a direct function reference; specifically, in the
map where you call shared_secret.moduli_data().iter().map(|arr| arr.mapv(|v|
BigInt::from(v))), change the inner closure |v| BigInt::from(v) to the function
reference BigInt::from so the code uses arr.mapv(BigInt::from) when constructing
secret_sss from moduli_data().
In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs`:
- Line 155: The call to share_manager.coeffs_to_poly_level0 currently does an
unnecessary allocation: change the argument from
sk_share.coeffs.clone().as_ref() to sk_share.coeffs.as_ref() so you pass a
reference to the Box<[i64]> without cloning; update the invocation in
gen_pk_share_and_sk_sss.rs where sk_poly is assigned (the call to
coeffs_to_poly_level0) to remove the .clone() and keep .as_ref() directly.
- Around line 131-134: Add a short clarifying comment next to the call to
PublicKeyShare::new_extended explaining that the third returned value (the
generated secret key) is intentionally ignored because sk_share from
SecretKey::random is used instead; reference the variables in the line
(pk0_share, a, _, eek and sk_share) so future readers know the underscore is
discarding the redundant secret key on purpose.
- Around line 137-153: There are two identical ShareManager instances
(share_manager_for_esm and share_manager) and an unnecessary params.clone() when
creating them; remove the duplication by creating a single mutable ShareManager
(let mut share_manager = ShareManager::new(num_ciphernodes as usize, threshold
as usize, params)) before calling trbfv.generate_smudging_error and reuse that
same share_manager for bigints_to_poly and later mutable operations; also remove
the extra params.clone() when constructing ShareManager and avoid cloning coeffs
by passing a borrow into bigints_to_poly (use &esi_coeffs rather than cloning).
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 341-360: The loop currently publishes ThresholdShareCreated to
every recipient_party_id in 0..num_parties including the sender itself; change
the loop to skip self-delivery by checking recipient_party_id against this
actor's own party id (e.g. if recipient_party_id == self.party_id { continue }
or use the appropriate field name like self.local_party_id), so that before
calling share.extract_for_party and self.bus.publish you ignore the self target
and avoid duplicate/wasteful publishing of ThresholdShareCreated messages.
- Around line 273-289: The code uses ProofType::T1SkShareComputation and
ProofType::T1ESmShareComputation (where the “T1” prefix denotes protocol phase)
but the surrounding error messages refer to them as "T2a"/"T2b" (proof
numbering); add a brief clarifying comment immediately above the sign_proof
calls (or above the enum usage) stating that the ProofType names use phase-based
"T1" naming while external/doc labels call them "Proof 2a/2b", so the mismatch
between ProofType::T1SkShareComputation / ProofType::T1ESmShareComputation and
the error strings ("Failed to sign T2a proof …", "Failed to sign T2b proof …")
is intentional and not a typo; leave existing function names and error messages
unchanged.
🧹 Nitpick comments (14)
🤖 Fix all nitpicks with 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 line 27-40: Add a constructor for ShareComputationProofRequest to match the API pattern used by PkBfvProofRequest::new and PkGenerationProofRequest::new: implement an associated function pub fn new(secret_raw: SensitiveBytes, secret_sss_raw: SensitiveBytes, dkg_input_type: DkgInputType, params_preset: BfvPreset, committee_size: CiphernodesCommitteeSize) -> Self that returns a ShareComputationProofRequest populated from the parameters (and perform any basic validation or transformation here if needed), and add docs/tests as appropriate so callers use ShareComputationProofRequest::new instead of direct struct construction. - Around line 93-101: The returned Self initializer in the constructor/impl lists fields in an order that doesn’t match the struct declaration: swap the positions of params_preset and e_sm so the initializer matches the struct's declared field order (keep pk0_share, a, sk, eek as-is, then params_preset, e_sm, committee_size) in the function where Self { pk0_share, a, sk, eek, params_preset, e_sm, committee_size } is returned. In `@crates/keyshare/src/threshold_keyshare.rs`: - Around line 696-708: The code silently skips creating and dispatching GenEsiSss when GeneratingThresholdShareData.ciphernode_selected is None, which can stall the state machine; update the branch that currently only handles Some(ciphernode_selected) around self.state.try_get()?.try_into()? and handle_gen_esi_sss_requested to either log a warning and return an error (or transition the state) when ciphernode_selected is None so the unexpected condition is surfaced; ensure the e_sm_raw unwrap remains validated (e_sm_raw.expect(...)) or convert to a propagated error if missing, referencing GeneratingThresholdShareData, ciphernode_selected, handle_gen_esi_sss_requested, and GenEsiSss to locate the logic to change. - Line 883: Replace the hardcoded CiphernodesCommitteeSize::Small used in the three proof request constructions with a value derived from actual committee configuration (e.g., compute from threshold_n or the E3 configuration object) so circuit selection matches runtime committee size; locate the three occurrences of CiphernodesCommitteeSize::Small in threshold_keyshare.rs (used in the proof request builders) and change them to read from the committee size provider (e.g., a function or field like threshold_n or e3_config.get_committee_size()), add a small helper or use existing metadata to map that numeric committee size to the CiphernodesCommitteeSize enum, and ensure the new value is validated or defaulted if the config is missing. - Around line 1148-1153: The two branches handling EnclaveEventData::PkGenerationProofSigned and EnclaveEventData::ShareComputationProofSigned currently discard errors with "let _ =", hiding failures from handle_pk_generation_proof_signed and handle_share_computation_proof_signed; update these branches to surface errors like other handlers by chaining the call to .trap(...) with a clear context message (e.g., "pk generation proof signed handling" and "share computation proof signed handling") or at minimum match the Result and log the Err using the module logger before returning, referencing the functions handle_pk_generation_proof_signed, handle_share_computation_proof_signed and the enum variants so failures are not silently suppressed. - Around line 490-498: The match's `other` arm currently logs a warning but leaves `current` unchanged and still calls `s.new_state(KeyshareState::AggregatingDecryptionKey(updated))`, causing an unnecessary persist; update the logic in the `ShareComputationProofSigned` handling so that when the `other` (unexpected `ProofType`) branch is hit you short-circuit (e.g., return an Err or otherwise exit the handler) instead of falling through to `s.new_state`, or alternatively set a flag and skip calling `s.new_state` if `updated` is identical to `current`; locate the match over `ProofType`, the `other` arm, the `current` identifier, and the `s.new_state(KeyshareState::AggregatingDecryptionKey(updated))` call and implement the early return/skip to avoid redundant persistence. - Around line 105-132: The fields signed_sk_share_computation_proof and signed_e_sm_share_computation_proof are carried in AggregatingDecryptionKey, ReadyForDecryption, and Decrypting but never consumed; either remove these fields (and their serde derives) from the three structs to avoid serializing dead data, or explicitly consume/publish them where the other proof is used (e.g., when constructing/emitting KeyshareCreated) and add a TODO comment explaining future use. Locate the fields named signed_sk_share_computation_proof and signed_e_sm_share_computation_proof inside the structs AggregatingDecryptionKey, ReadyForDecryption, and Decrypting and either delete those field declarations and related uses, or propagate them into the KeyshareCreated emission/handling code (where signed_pk_generation_proof is used) so they are read/published, then mark with a TODO if kept for future features. In `@crates/multithread/src/multithread.rs`: - Around line 391-470: The inline step comments in handle_share_computation_proof and handle_pk_generation_proof are mis-numbered (duplicate "4" in handle_share_computation_proof and duplicate "3" in handle_pk_generation_proof); update the comment tokens so steps are sequential (for handle_share_computation_proof change the second "4" to "5" and increment subsequent step numbers accordingly, and for handle_pk_generation_proof change the second "3" to "4" and renumber following steps) while leaving code logic unchanged — search for the functions handle_share_computation_proof and handle_pk_generation_proof to locate the comment lines to edit. - Around line 425-430: Replace the closure currently used to convert elements when building secret_sss with a direct function reference; specifically, in the map where you call shared_secret.moduli_data().iter().map(|arr| arr.mapv(|v| BigInt::from(v))), change the inner closure |v| BigInt::from(v) to the function reference BigInt::from so the code uses arr.mapv(BigInt::from) when constructing secret_sss from moduli_data(). In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs`: - Line 155: The call to share_manager.coeffs_to_poly_level0 currently does an unnecessary allocation: change the argument from sk_share.coeffs.clone().as_ref() to sk_share.coeffs.as_ref() so you pass a reference to the Box<[i64]> without cloning; update the invocation in gen_pk_share_and_sk_sss.rs where sk_poly is assigned (the call to coeffs_to_poly_level0) to remove the .clone() and keep .as_ref() directly. - Around line 131-134: Add a short clarifying comment next to the call to PublicKeyShare::new_extended explaining that the third returned value (the generated secret key) is intentionally ignored because sk_share from SecretKey::random is used instead; reference the variables in the line (pk0_share, a, _, eek and sk_share) so future readers know the underscore is discarding the redundant secret key on purpose. - Around line 137-153: There are two identical ShareManager instances (share_manager_for_esm and share_manager) and an unnecessary params.clone() when creating them; remove the duplication by creating a single mutable ShareManager (let mut share_manager = ShareManager::new(num_ciphernodes as usize, threshold as usize, params)) before calling trbfv.generate_smudging_error and reuse that same share_manager for bigints_to_poly and later mutable operations; also remove the extra params.clone() when constructing ShareManager and avoid cloning coeffs by passing a borrow into bigints_to_poly (use &esi_coeffs rather than cloning). In `@crates/zk-prover/src/actors/proof_request.rs`: - Around line 341-360: The loop currently publishes ThresholdShareCreated to every recipient_party_id in 0..num_parties including the sender itself; change the loop to skip self-delivery by checking recipient_party_id against this actor's own party id (e.g. if recipient_party_id == self.party_id { continue } or use the appropriate field name like self.local_party_id), so that before calling share.extract_for_party and self.bus.publish you ignore the self target and avoid duplicate/wasteful publishing of ThresholdShareCreated messages. - Around line 273-289: The code uses ProofType::T1SkShareComputation and ProofType::T1ESmShareComputation (where the “T1” prefix denotes protocol phase) but the surrounding error messages refer to them as "T2a"/"T2b" (proof numbering); add a brief clarifying comment immediately above the sign_proof calls (or above the enum usage) stating that the ProofType names use phase-based "T1" naming while external/doc labels call them "Proof 2a/2b", so the mismatch between ProofType::T1SkShareComputation / ProofType::T1ESmShareComputation and the error strings ("Failed to sign T2a proof …", "Failed to sign T2b proof …") is intentional and not a typo; leave existing function names and error messages unchanged.crates/events/src/enclave_event/compute_request/zk.rs (2)
27-40: Consider adding a constructor forShareComputationProofRequest.
PkBfvProofRequestandPkGenerationProofRequestboth havenew()constructors, butShareComputationProofRequestdoes not. Adding one would keep the API surface consistent and give a single place to add future validation or transformation logic.🤖 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 27 - 40, Add a constructor for ShareComputationProofRequest to match the API pattern used by PkBfvProofRequest::new and PkGenerationProofRequest::new: implement an associated function pub fn new(secret_raw: SensitiveBytes, secret_sss_raw: SensitiveBytes, dkg_input_type: DkgInputType, params_preset: BfvPreset, committee_size: CiphernodesCommitteeSize) -> Self that returns a ShareComputationProofRequest populated from the parameters (and perform any basic validation or transformation here if needed), and add docs/tests as appropriate so callers use ShareComputationProofRequest::new instead of direct struct construction.
93-101: Nit: field initialization order doesn't match declaration order.
e_smandparams_presetare swapped compared to the struct definition (lines 55–72). In Rust this is functionally correct, but matching declaration order improves readability and makes diffs cleaner.Suggested reorder
Self { pk0_share: pk0_share.into(), a: a.into(), sk, eek, - params_preset, e_sm, + params_preset, committee_size, }🤖 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 93 - 101, The returned Self initializer in the constructor/impl lists fields in an order that doesn’t match the struct declaration: swap the positions of params_preset and e_sm so the initializer matches the struct's declared field order (keep pk0_share, a, sk, eek as-is, then params_preset, e_sm, committee_size) in the function where Self { pk0_share, a, sk, eek, params_preset, e_sm, committee_size } is returned.crates/multithread/src/multithread.rs (2)
391-470: Duplicate step numbering in comments.Steps are numbered "4" twice (lines 421 and 432). The second "4" should be "5", and subsequent steps renumbered accordingly.
Similarly in
handle_pk_generation_proof, step "3" appears twice (lines 495 and 511).Proposed fix for handle_share_computation_proof
- // 4. Deserialize Shamir shares (bincode-encoded SharedSecret) + // 4. Deserialize Shamir shares (bincode-encoded SharedSecret) ... - // 4. Compute parity matrix + // 5. Compute parity matrix ... - // 5. Build circuit data + // 6. Build circuit data ... - // 6. Generate proof + // 7. Generate proof ... - // 7. Return response + // 8. Return responseProposed fix for handle_pk_generation_proof
- // 3. Convert Poly → CrtPolynomial + // 4. Convert Poly → CrtPolynomial ... - // 4. Build circuit data + // 5. Build circuit data ... - // 5. Generate proof via Provable trait + // 6. Generate proof via Provable trait ... - // 6. Return response + // 7. Return response🤖 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 391 - 470, The inline step comments in handle_share_computation_proof and handle_pk_generation_proof are mis-numbered (duplicate "4" in handle_share_computation_proof and duplicate "3" in handle_pk_generation_proof); update the comment tokens so steps are sequential (for handle_share_computation_proof change the second "4" to "5" and increment subsequent step numbers accordingly, and for handle_pk_generation_proof change the second "3" to "4" and renumber following steps) while leaving code logic unchanged — search for the functions handle_share_computation_proof and handle_pk_generation_proof to locate the comment lines to edit.
425-430: Nit: closure can be simplified to a function reference.
|v| BigInt::from(v)can be written asBigInt::fromdirectly.Suggested diff
let secret_sss: Vec<Array2<BigInt>> = shared_secret .moduli_data() .iter() - .map(|arr| arr.mapv(|v| BigInt::from(v))) + .map(|arr| arr.mapv(BigInt::from)) .collect();🤖 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 425 - 430, Replace the closure currently used to convert elements when building secret_sss with a direct function reference; specifically, in the map where you call shared_secret.moduli_data().iter().map(|arr| arr.mapv(|v| BigInt::from(v))), change the inner closure |v| BigInt::from(v) to the function reference BigInt::from so the code uses arr.mapv(BigInt::from) when constructing secret_sss from moduli_data().crates/zk-prover/src/actors/proof_request.rs (2)
341-360: Consider whether publishing shares to all parties (including self) is intended.The loop iterates
0..num_partiesand publishes aThresholdShareCreatedfor every recipient, including the sender's ownparty_id. If self-delivery is wasteful or causes duplicate processing downstream, you may want to skip it.#!/bin/bash # Check if ThresholdShareCreated handlers filter out self-delivery rg -n 'ThresholdShareCreated' --type rust -A 10 | head -80🤖 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 341 - 360, The loop currently publishes ThresholdShareCreated to every recipient_party_id in 0..num_parties including the sender itself; change the loop to skip self-delivery by checking recipient_party_id against this actor's own party id (e.g. if recipient_party_id == self.party_id { continue } or use the appropriate field name like self.local_party_id), so that before calling share.extract_for_party and self.bus.publish you ignore the self target and avoid duplicate/wasteful publishing of ThresholdShareCreated messages.
273-289: Naming scheme uses different conventions: T1 variants reference protocol phase, not proof numbering.The
ProofType::T1SkShareComputationandProofType::T1ESmShareComputationvariants are correctly named according to their enum definition. The T1 prefix refers to the protocol phase (not a typo), while the enum documentation labels them as "Proof 2a" and "Proof 2b", creating a naming inconsistency with the surrounding error messages that call them "T2a" and "T2b" proofs. This is confusing but intentional. Adding a clarifying comment explaining the dual naming convention (phase vs. proof numbering) would improve readability.🤖 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 273 - 289, The code uses ProofType::T1SkShareComputation and ProofType::T1ESmShareComputation (where the “T1” prefix denotes protocol phase) but the surrounding error messages refer to them as "T2a"/"T2b" (proof numbering); add a brief clarifying comment immediately above the sign_proof calls (or above the enum usage) stating that the ProofType names use phase-based "T1" naming while external/doc labels call them "Proof 2a/2b", so the mismatch between ProofType::T1SkShareComputation / ProofType::T1ESmShareComputation and the error strings ("Failed to sign T2a proof …", "Failed to sign T2b proof …") is intentional and not a typo; leave existing function names and error messages unchanged.crates/trbfv/src/gen_pk_share_and_sk_sss.rs (3)
155-155: Remove unnecessary.clone()before.as_ref().
sk_share.coeffsis aBox<[i64]>which implementsAsRef<[i64]>, so calling.as_ref()directly works without the intermediate.clone()allocation. The same pattern is already used correctly elsewhere in the codebase (e.g.,crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/sample.rs:86).♻️ Suggested fix
- let sk_poly = share_manager.coeffs_to_poly_level0(sk_share.coeffs.clone().as_ref())?; + let sk_poly = share_manager.coeffs_to_poly_level0(sk_share.coeffs.as_ref())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs` at line 155, The call to share_manager.coeffs_to_poly_level0 currently does an unnecessary allocation: change the argument from sk_share.coeffs.clone().as_ref() to sk_share.coeffs.as_ref() so you pass a reference to the Box<[i64]> without cloning; update the invocation in gen_pk_share_and_sk_sss.rs where sk_poly is assigned (the call to coeffs_to_poly_level0) to remove the .clone() and keep .as_ref() directly.
131-134: Consider adding a clarifying comment explaining why the third return value fromnew_extendedis discarded.The function
PublicKeyShare::new_extendedreturns four values. In this code, the third value is intentionally discarded because the function already hassk_sharefrom the earlierSecretKey::randomcall. A brief comment (e.g.,// Third value (sk) not needed; we already have sk_share from above) would help future maintainers understand this design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs` around lines 131 - 134, Add a short clarifying comment next to the call to PublicKeyShare::new_extended explaining that the third returned value (the generated secret key) is intentionally ignored because sk_share from SecretKey::random is used instead; reference the variables in the line (pk0_share, a, _, eek and sk_share) so future readers know the underscore is discarding the redundant secret key on purpose.
137-153: ConsolidateShareManagerto eliminate redundant allocation and remove unnecessary clone.
share_manager_for_esmandshare_managerare identical instances. Create a singlelet mut share_managerbefore callingbigints_to_poly(which requires&selfonly) and reuse it for subsequent mutable operations. Additionally, remove the unnecessaryclone()at line 155 sincecoeffscan be borrowed directly.♻️ Suggested consolidation
- let share_manager_for_esm = - ShareManager::new(num_ciphernodes as usize, threshold as usize, params.clone()); let esi_coeffs = trbfv.generate_smudging_error( req.num_ciphertexts, req.lambda, &mut *rng.lock().unwrap(), )?; - let e_sm_rns = share_manager_for_esm.bigints_to_poly(&esi_coeffs)?; + + let mut share_manager = + ShareManager::new(num_ciphernodes as usize, threshold as usize, params.clone()); + + let e_sm_rns = share_manager.bigints_to_poly(&esi_coeffs)?; let e_sm_raw = ArcBytes::from_bytes(&e_sm_rns.deref().to_bytes()); let pk0_share_raw = ArcBytes::from_bytes(&pk0_share.to_bytes()); let a_raw = ArcBytes::from_bytes(&a.to_bytes()); let eek_raw = ArcBytes::from_bytes(&eek.to_bytes()); - let mut share_manager = - ShareManager::new(num_ciphernodes as usize, threshold as usize, params.clone()); - - let sk_poly = share_manager.coeffs_to_poly_level0(sk_share.coeffs.clone().as_ref())?; + let sk_poly = share_manager.coeffs_to_poly_level0(sk_share.coeffs.as_ref())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/gen_pk_share_and_sk_sss.rs` around lines 137 - 153, There are two identical ShareManager instances (share_manager_for_esm and share_manager) and an unnecessary params.clone() when creating them; remove the duplication by creating a single mutable ShareManager (let mut share_manager = ShareManager::new(num_ciphernodes as usize, threshold as usize, params)) before calling trbfv.generate_smudging_error and reuse that same share_manager for bigints_to_poly and later mutable operations; also remove the extra params.clone() when constructing ShareManager and avoid cloning coeffs by passing a borrow into bigints_to_poly (use &esi_coeffs rather than cloning).crates/keyshare/src/threshold_keyshare.rs (5)
696-708:expecton Line 704 is safe but consider defensive handling forciphernode_selected.If
ciphernode_selectedisNone(Line 698), theGenEsiSssrequest is silently skipped and the state machine stalls inGeneratingThresholdShare. While this shouldn't happen in the current flow (it's alwaysSomeper Line 594), consider logging a warning or returning an error on theelsebranch to surface unexpected conditions.Proposed fix
- if let Some(ciphernode_selected) = current_state.ciphernode_selected { + let Some(ciphernode_selected) = current_state.ciphernode_selected else { + bail!("ciphernode_selected missing in GeneratingThresholdShareData - state machine cannot proceed"); + }; + { self.handle_gen_esi_sss_requested(TypedEvent::new( GenEsiSss { ciphernode_selected, e_sm_raw: current_state .e_sm_raw .expect("e_sm_raw should be set at this point"), }, ec.clone(), ))?; }🤖 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 696 - 708, The code silently skips creating and dispatching GenEsiSss when GeneratingThresholdShareData.ciphernode_selected is None, which can stall the state machine; update the branch that currently only handles Some(ciphernode_selected) around self.state.try_get()?.try_into()? and handle_gen_esi_sss_requested to either log a warning and return an error (or transition the state) when ciphernode_selected is None so the unexpected condition is surfaced; ensure the e_sm_raw unwrap remains validated (e_sm_raw.expect(...)) or convert to a propagated error if missing, referencing GeneratingThresholdShareData, ciphernode_selected, handle_gen_esi_sss_requested, and GenEsiSss to locate the logic to change.
883-883: HardcodedCiphernodesCommitteeSize::Smallin three proof requests.The TODO on Line 883 acknowledges this should be derived from config. Since the committee size directly impacts circuit selection and proof verification, this should be tracked. Consider deriving it from
threshold_nor the E3 configuration.Would you like me to open an issue to track deriving
CiphernodesCommitteeSizefrom the actual committee configuration?Also applies to: 892-892, 901-901
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/keyshare/src/threshold_keyshare.rs` at line 883, Replace the hardcoded CiphernodesCommitteeSize::Small used in the three proof request constructions with a value derived from actual committee configuration (e.g., compute from threshold_n or the E3 configuration object) so circuit selection matches runtime committee size; locate the three occurrences of CiphernodesCommitteeSize::Small in threshold_keyshare.rs (used in the proof request builders) and change them to read from the committee size provider (e.g., a function or field like threshold_n or e3_config.get_committee_size()), add a small helper or use existing metadata to map that numeric committee size to the CiphernodesCommitteeSize enum, and ensure the new value is validated or defaulted if the config is missing.
1148-1153: Silent error suppression withlet _ =masks proof storage failures.Errors from
handle_pk_generation_proof_signedandhandle_share_computation_proof_signedare silently discarded. If a state mismatch or persistence failure occurs, there's no log trace. Other handlers in this file (e.g.,Handler<TypedEvent<ComputeResponse>>) use thetrappattern for error handling. Consider usingtrapor at minimum logging on error:Proposed fix
EnclaveEventData::PkGenerationProofSigned(data) => { - let _ = self.handle_pk_generation_proof_signed(TypedEvent::new(data, ec)); + if let Err(e) = self.handle_pk_generation_proof_signed(TypedEvent::new(data, ec)) { + warn!("Failed to handle PkGenerationProofSigned: {}", e); + } } EnclaveEventData::ShareComputationProofSigned(data) => { - let _ = self.handle_share_computation_proof_signed(TypedEvent::new(data, ec)); + if let Err(e) = self.handle_share_computation_proof_signed(TypedEvent::new(data, ec)) { + warn!("Failed to handle ShareComputationProofSigned: {}", e); + } }🤖 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 1148 - 1153, The two branches handling EnclaveEventData::PkGenerationProofSigned and EnclaveEventData::ShareComputationProofSigned currently discard errors with "let _ =", hiding failures from handle_pk_generation_proof_signed and handle_share_computation_proof_signed; update these branches to surface errors like other handlers by chaining the call to .trap(...) with a clear context message (e.g., "pk generation proof signed handling" and "share computation proof signed handling") or at minimum match the Result and log the Err using the module logger before returning, referencing the functions handle_pk_generation_proof_signed, handle_share_computation_proof_signed and the enum variants so failures are not silently suppressed.
490-498:otherarm in proof type match still writes unchanged state.When an unexpected
ProofTypeis received, thewarn!fires butcurrent(unchanged) is still passed tos.new_state(...), triggering a redundant persist. Consider returning an error or short-circuiting:Proposed fix
ProofType::T1ESmShareComputation => AggregatingDecryptionKey { signed_e_sm_share_computation_proof: Some(msg.signed_proof), ..current }, other => { warn!( "Unexpected proof type {:?} in ShareComputationProofSigned", other ); - current + bail!("Unexpected proof type {:?} in ShareComputationProofSigned", other); } }; - s.new_state(KeyshareState::AggregatingDecryptionKey(updated)) + s.new_state(KeyshareState::AggregatingDecryptionKey(updated))🤖 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 490 - 498, The match's `other` arm currently logs a warning but leaves `current` unchanged and still calls `s.new_state(KeyshareState::AggregatingDecryptionKey(updated))`, causing an unnecessary persist; update the logic in the `ShareComputationProofSigned` handling so that when the `other` (unexpected `ProofType`) branch is hit you short-circuit (e.g., return an Err or otherwise exit the handler) instead of falling through to `s.new_state`, or alternatively set a flag and skip calling `s.new_state` if `updated` is identical to `current`; locate the match over `ProofType`, the `other` arm, the `current` identifier, and the `s.new_state(KeyshareState::AggregatingDecryptionKey(updated))` call and implement the early return/skip to avoid redundant persistence.
105-132:signed_sk_share_computation_proofandsigned_e_sm_share_computation_proofare carried through all states but never consumed.These two proofs are threaded from
AggregatingDecryptionKey→ReadyForDecryption→Decryptingbut are never read or published. Onlysigned_pk_generation_proofis used (Line 1038 inKeyshareCreated). If these are intended for future use, consider adding a TODO comment; otherwise they're dead data inflating serialized state.🤖 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 105 - 132, The fields signed_sk_share_computation_proof and signed_e_sm_share_computation_proof are carried in AggregatingDecryptionKey, ReadyForDecryption, and Decrypting but never consumed; either remove these fields (and their serde derives) from the three structs to avoid serializing dead data, or explicitly consume/publish them where the other proof is used (e.g., when constructing/emitting KeyshareCreated) and add a TODO comment explaining future use. Locate the fields named signed_sk_share_computation_proof and signed_e_sm_share_computation_proof inside the structs AggregatingDecryptionKey, ReadyForDecryption, and Decrypting and either delete those field declarations and related uses, or propagate them into the KeyshareCreated emission/handling code (where signed_pk_generation_proof is used) so they are read/published, then mark with a TODO if kept for future features.
ctrlc03
force-pushed
the
feat/integrate-t1-share-computation
branch
from
71af852 to
99ed7b9
Compare
ctrlc03
force-pushed
the
feat/integrate-t1-share-computation
branch
from
99ed7b9 to
a66566e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/multithread/src/multithread.rs (1)
423-502: Share computation proof handler is well-structured.The implementation correctly:
- Builds threshold params via
build_pair_for_preset(needed forcompute_parity_matrixand polynomial deserialization).- Conditionally centers the polynomial only for
DkgInputType::SecretKey.- Converts Shamir share data from
u64toBigIntfor circuit compatibility.- Uses consistent error mapping patterns.
Two minor nits: (1) duplicate step numbering in comments — there are two "Step 4" labels at lines 453 and 464; (2)
.mapv(|v| BigInt::from(v))at line 461 could be.mapv(BigInt::from).🤖 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 423 - 502, In handle_share_computation_proof: fix two minor nits — rename the duplicate comment label (the second "Step 4" that precedes the parity matrix/build circuit block) so the step numbers are sequential/unique, and simplify the conversion closure by replacing .mapv(|v| BigInt::from(v)) with .mapv(BigInt::from) on the shared_secret → secret_sss conversion to use the function pointer form.crates/zk-prover/src/actors/proof_request.rs (1)
266-296: Error messages incorrectly refer to "T2a"/"T2b" proofs when these variants are documented as "T1" tier.The
ProofTypeenum incrates/events/src/enclave_event/signed_proof.rsdocumentsT1SkShareComputationandT1ESmShareComputationas "T1 — ... (Proof 2a)" and "T1 — ... (Proof 2b)" respectively, where the numbers refer to proof numbering, not tier classification. However, the error messages at lines 279 and 287 claim these are "T2a" and "T2b" proofs, which contradicts the enum documentation. Update the error messages to reference "T1" tier or the specific proof names to avoid confusion during debugging.🤖 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 266 - 296, In publish_threshold_share_with_proofs, the error messages for failed signature of T1SkShareComputation and T1ESmShareComputation incorrectly call them "T2a"/"T2b"; change these messages to reference the correct tier/name (e.g., "T1SkShareComputation (T1 — Proof 2a)" and "T1ESmShareComputation (T1 — Proof 2b)" or simply "T1") so they match the ProofType enum documentation; update the error strings in the sign_proof failure branches for ProofType::T1SkShareComputation and ProofType::T1ESmShareComputation in publish_threshold_share_with_proofs accordingly.crates/keyshare/src/threshold_keyshare.rs (1)
882-918:CiphernodesCommitteeSize::Smallis hardcoded for all three proof requests at lines 879, 888, and 900.The
TODO: derive from configcomment at line 879 indicates this is a known placeholder. However,MediumandLargecommittee size variants are not yet fully implemented (the match cases are commented out inciphernodes_committee.rs). Before deriving from configuration, the implementation for these variants must be completed and the hardcoded values replaced with config-driven logic.🤖 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 882 - 918, The three ShareComputationProofRequest constructions currently hardcode CiphernodesCommitteeSize::Small; replace those hardcoded values with a committee_size value derived from configuration (as hinted by the TODO) and pass that variable into each ShareComputationProofRequest (e.g., for sk_share_computation_request and e_sm_share_computation_request and the related T1 request). Before doing so, implement the missing Medium and Large variants in ciphernodes_committee.rs (uncomment/complete the match arms and any helper logic) so the config-driven committee_size can return correct values; then read the configured committee size at the start of the function, assign it to a local variable (committee_size) and use that variable when constructing ThresholdSharePending and its embedded ShareComputationProofRequest instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 462-502: The code attempts to always convert the current state
into an AggregatingDecryptionKey via try_into inside
handle_share_computation_proof_signed, which will fail if the state already
moved to ReadyForDecryption; fix by matching the mutable state variant before
converting/updating: inside self.state.try_mutate(&ec, |s| { ... })
pattern-match s (or s.clone()) for
KeyshareState::AggregatingDecryptionKey(current) and only update that branch by
inserting the appropriate signed proof into current (using the existing match on
msg.proof_type); in the non-matching branches (e.g., ReadyForDecryption) simply
log a debug/warn about late proof and return the unchanged state instead of
attempting try_into or propagating an error. This removes the race-prone
try_into call and prevents silent errors.
In `@crates/trbfv/src/gen_esi_sss.rs`:
- Line 88: The info log in gen_esi_sss.rs currently prints
"gen_esi_sss:generate_smudging_error..." which is misleading because the code is
performing generate_secret_shares_from_poly; update the log message in the
generate_secret_shares_from_poly call site to accurately describe the operation
(e.g., "gen_esi_sss:generate_secret_shares_from_poly..." or
"gen_esi_sss:generating secret shares from poly...") so logs match the function
action and aid debugging; locate the log invocation by searching for
info!("gen_esi_sss:generate_smudging_error") within gen_esi_sss and replace the
string accordingly.
---
Nitpick comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 882-918: The three ShareComputationProofRequest constructions
currently hardcode CiphernodesCommitteeSize::Small; replace those hardcoded
values with a committee_size value derived from configuration (as hinted by the
TODO) and pass that variable into each ShareComputationProofRequest (e.g., for
sk_share_computation_request and e_sm_share_computation_request and the related
T1 request). Before doing so, implement the missing Medium and Large variants in
ciphernodes_committee.rs (uncomment/complete the match arms and any helper
logic) so the config-driven committee_size can return correct values; then read
the configured committee size at the start of the function, assign it to a local
variable (committee_size) and use that variable when constructing
ThresholdSharePending and its embedded ShareComputationProofRequest instances.
In `@crates/multithread/src/multithread.rs`:
- Around line 423-502: In handle_share_computation_proof: fix two minor nits —
rename the duplicate comment label (the second "Step 4" that precedes the parity
matrix/build circuit block) so the step numbers are sequential/unique, and
simplify the conversion closure by replacing .mapv(|v| BigInt::from(v)) with
.mapv(BigInt::from) on the shared_secret → secret_sss conversion to use the
function pointer form.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 266-296: In publish_threshold_share_with_proofs, the error
messages for failed signature of T1SkShareComputation and T1ESmShareComputation
incorrectly call them "T2a"/"T2b"; change these messages to reference the
correct tier/name (e.g., "T1SkShareComputation (T1 — Proof 2a)" and
"T1ESmShareComputation (T1 — Proof 2b)" or simply "T1") so they match the
ProofType enum documentation; update the error strings in the sign_proof failure
branches for ProofType::T1SkShareComputation and
ProofType::T1ESmShareComputation in publish_threshold_share_with_proofs
accordingly.
a66566e to
2e6a654
Compare
ctrlc03
force-pushed
the
feat/integrate-t1-share-computation
branch
from
2e6a654 to
55d1ef2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/trbfv/src/shares/shares.rs (1)
24-26: Consider returning&[Array2<u64>]instead of&Vec<Array2<u64>>.Idiomatic Rust prefers
&[T]over&Vec<T>for read-only accessors since it's more general and avoids coupling callers toVec.♻️ Suggested change
- pub fn moduli_data(&self) -> &Vec<Array2<u64>> { - &self.moduli_data + pub fn moduli_data(&self) -> &[Array2<u64>] { + &self.moduli_data }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trbfv/src/shares/shares.rs` around lines 24 - 26, The accessor moduli_data currently returns &Vec<Array2<u64>>; change its signature to return a slice reference & [Array2<u64>] and return &self.moduli_data[..] to be more general and idiomatic. Update the function declaration for moduli_data and adjust any call sites that expect &Vec<Array2<u64>> to accept a slice (&[Array2<u64>]) or to call .to_vec() only where an owned Vec is needed. Ensure the type imports and visibility remain unchanged.crates/multithread/src/multithread.rs (1)
494-498: PreferShareComputationProofResponse::new()for consistency.
PkBfvProofResponse::new(proof)andPkGenerationProofResponse::new(proof)are used in sibling functions; constructingShareComputationProofResponseinline is inconsistent.♻️ Proposed fix
- Ok(ComputeResponse::zk( - ZkResponse::ShareComputation(ShareComputationProofResponse { - proof, - dkg_input_type: req.dkg_input_type, - }), + Ok(ComputeResponse::zk( + ZkResponse::ShareComputation(ShareComputationProofResponse::new( + proof, + req.dkg_input_type, + )), request.correlation_id, request.e3_id, ))🤖 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 494 - 498, Construct the response using ShareComputationProofResponse::new(...) instead of building the struct inline to match sibling usage (like PkBfvProofResponse::new and PkGenerationProofResponse::new); replace the inline ShareComputationProofResponse { proof, dkg_input_type: req.dkg_input_type } used inside ComputeResponse::zk(...) with a call to ShareComputationProofResponse::new(proof, req.dkg_input_type) (or the correct parameter order expected by ShareComputationProofResponse::new) so the code is consistent and uses the type's constructor helper.crates/events/src/enclave_event/compute_request/zk.rs (1)
27-40:ShareComputationProofRequestis missing anew()constructor.Both
PkBfvProofRequestandPkGenerationProofRequestexposenew()constructors;ShareComputationProofRequestdeparts from this pattern.♻️ Proposed constructor
+impl ShareComputationProofRequest { + pub fn new( + secret_raw: SensitiveBytes, + secret_sss_raw: SensitiveBytes, + dkg_input_type: DkgInputType, + params_preset: BfvPreset, + committee_size: CiphernodesCommitteeSize, + ) -> Self { + Self { + secret_raw, + secret_sss_raw, + dkg_input_type, + params_preset, + committee_size, + } + } +}🤖 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 27 - 40, Add a public constructor pub fn new(...) -> Self for ShareComputationProofRequest to match the pattern used by PkBfvProofRequest and PkGenerationProofRequest: accept parameters for secret_raw: SensitiveBytes, secret_sss_raw: SensitiveBytes, dkg_input_type: DkgInputType, params_preset: BfvPreset, and committee_size: CiphernodesCommitteeSize and return a ShareComputationProofRequest with those fields set accordingly; implement it as an inherent impl on ShareComputationProofRequest so callers can construct instances via ShareComputationProofRequest::new(...).
🤖 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 875-889: T2a/T2b share computation requests (the
ShareComputationProofRequest instances, e_sm_share_computation_request and the
earlier t1 request) hardcode CiphernodesCommitteeSize::Small but lack the
matching TODO comment; update the two ShareComputationProofRequest constructions
that set committee_size (e.g., the variable e_sm_share_computation_request and
the T1 request) to include the same "// TODO: derive from config" comment next
to the committee_size assignment so the intent and consistency are clear, or
refactor to read committee size from the shared config variable used elsewhere
if preferred.
- Around line 456-496: Update the doc comment above
handle_share_computation_proof_signed and the related log message that mentions
"T2a/T2b" to use the formal ProofType variant names (T1SkShareComputation and
T1ESmShareComputation) or else use the consistent "Proof 2a/Proof 2b" wording;
locate the doc comment on the handle_share_computation_proof_signed method and
the info!/log call that describes the three proofs and replace the informal
"T2a/T2b" wording with the chosen formal variant names
(ProofType::T1SkShareComputation, ProofType::T1ESmShareComputation) so comments
and logs match the enum definition.
In `@crates/multithread/src/multithread.rs`:
- Around line 453-468: Update the duplicate step label: the comment above the
compute_parity_matrix call is mistakenly labeled "// 4. Compute parity matrix"
but the prior block that deserializes SharedSecret (using variable
secret_sss_bytes → SharedSecret → secret_sss) is already "// 4."; change the
comment preceding compute_parity_matrix (the call to compute_parity_matrix(...
threshold_params.moduli(), committee.n, committee.threshold) where committee
comes from req.committee_size.values()) to "// 5. Compute parity matrix" so the
step numbering is sequential.
---
Duplicate comments:
In `@crates/keyshare/src/threshold_keyshare.rs`:
- Around line 879-889: Previously the code indexed esi_sss_raw with [0], which
could panic; fix by using a safe bounds guard when constructing
e_sm_share_computation_request: call .into_iter().next().ok_or_else(||
anyhow!("esi_sss_raw is empty — expected at least one entry"))? to obtain
secret_sss_raw instead of direct indexing. Locate the
ShareComputationProofRequest creation (variable e_sm_share_computation_request)
and ensure it uses esi_sss_raw.into_iter().next().ok_or_else(...) as shown so
the empty-case returns an error rather than panicking.
In `@crates/trbfv/src/gen_esi_sss.rs`:
- Line 88: The log message in the gen_esi_sss::generate_secret_shares_from_poly
path was misleading and should be updated to accurately reflect the operation;
update the info! call inside the generate_secret_shares_from_poly function (and
any other occurrences in the gen_esi_sss module) to use
"gen_esi_sss:generate_secret_shares_from_poly..." instead of
"gen_esi_sss:generate_smudging_error..." and run a quick grep for the old string
to replace any remaining instances to keep logs consistent.
---
Nitpick comments:
In `@crates/events/src/enclave_event/compute_request/zk.rs`:
- Around line 27-40: Add a public constructor pub fn new(...) -> Self for
ShareComputationProofRequest to match the pattern used by PkBfvProofRequest and
PkGenerationProofRequest: accept parameters for secret_raw: SensitiveBytes,
secret_sss_raw: SensitiveBytes, dkg_input_type: DkgInputType, params_preset:
BfvPreset, and committee_size: CiphernodesCommitteeSize and return a
ShareComputationProofRequest with those fields set accordingly; implement it as
an inherent impl on ShareComputationProofRequest so callers can construct
instances via ShareComputationProofRequest::new(...).
In `@crates/multithread/src/multithread.rs`:
- Around line 494-498: Construct the response using
ShareComputationProofResponse::new(...) instead of building the struct inline to
match sibling usage (like PkBfvProofResponse::new and
PkGenerationProofResponse::new); replace the inline
ShareComputationProofResponse { proof, dkg_input_type: req.dkg_input_type } used
inside ComputeResponse::zk(...) with a call to
ShareComputationProofResponse::new(proof, req.dkg_input_type) (or the correct
parameter order expected by ShareComputationProofResponse::new) so the code is
consistent and uses the type's constructor helper.
In `@crates/trbfv/src/shares/shares.rs`:
- Around line 24-26: The accessor moduli_data currently returns
&Vec<Array2<u64>>; change its signature to return a slice reference &
[Array2<u64>] and return &self.moduli_data[..] to be more general and idiomatic.
Update the function declaration for moduli_data and adjust any call sites that
expect &Vec<Array2<u64>> to accept a slice (&[Array2<u64>]) or to call .to_vec()
only where an owned Vec is needed. Ensure the type imports and visibility remain
unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
568-568: Bareself-hostedlabel may route jobs to incompatible runners.
build_circuitsandzk_prover_e2eboth explicitly downloadbarretenberg-amd64-linux.tar.gz, so they require a Linux x86-64 runner. The bareself-hostedlabel will match any registered self-hosted runner (including macOS or arm64 Linux), which would cause a silent failure when the wrong binary is executed. Add OS and architecture labels to restrict routing:🛠️ Proposed fix
- runs-on: self-hosted # build_circuits (line 568) + runs-on: [self-hosted, linux, x64] - runs-on: self-hosted # zk_prover_e2e (line 621) + runs-on: [self-hosted, linux, x64]The same labels should be applied to any other jobs that install Linux-specific tooling (e.g.,
apt-get,ppa:ethereum/ethereum) —rust_tests,build_enclave_cli,crisp_unit,crisp_e2e,build_e3_support_risc0,test_enclave_init.Also applies to: 621-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 568, Change jobs that currently use the bare runs-on: self-hosted to use a labeled array restricting OS/arch so they only run on Linux x86-64 runners; specifically update runs-on for build_circuits and zk_prover_e2e (and also rust_tests, build_enclave_cli, crisp_unit, crisp_e2e, build_e3_support_risc0, test_enclave_init) from runs-on: self-hosted to something like runs-on: [self-hosted, linux, x86_64] (or your repo's equivalent linux/ubuntu and x86_64 labels) so the jobs that download barretenberg-amd64-linux.tar.gz and use Linux-specific tooling are scheduled only on compatible runners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 37: The workflow currently sets runs-on: self-hosted for
pull_request-triggered jobs which exposes the self-hosted runner to arbitrary PR
code; update the CI so PR-triggered jobs (e.g., the jobs named rust_tests,
build_circuits, build_sdk, zk_prover_integration and any other jobs currently
using runs-on: self-hosted) either run on GitHub-hosted runners instead or are
gated by an environment protection rule (add environment:
<protected-environment-name> with required reviewers) so fork PRs cannot execute
on the self-hosted machine; alternatively split self-hosted jobs to only run on
push/merge while keeping pull_request jobs on GitHub-hosted runners or adopt
ephemeral self-hosted runners for isolation.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 568: Change jobs that currently use the bare runs-on: self-hosted to use
a labeled array restricting OS/arch so they only run on Linux x86-64 runners;
specifically update runs-on for build_circuits and zk_prover_e2e (and also
rust_tests, build_enclave_cli, crisp_unit, crisp_e2e, build_e3_support_risc0,
test_enclave_init) from runs-on: self-hosted to something like runs-on:
[self-hosted, linux, x86_64] (or your repo's equivalent linux/ubuntu and x86_64
labels) so the jobs that download barretenberg-amd64-linux.tar.gz and use
Linux-specific tooling are scheduled only on compatible runners.
1c961b9 to
7313e24
Compare
ctrlc03
force-pushed
the
feat/integrate-t1-share-computation
branch
from
9a7e9a7 to
38c21d2
Compare
hmzakhalid
approved these changes
Feb 23, 2026
0xjei
pushed a commit
that referenced
this pull request
Feb 24, 2026
This was referenced Feb 26, 2026
4 tasks
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.
Integrate sk_share_computation and e_sm_share_computation
fix #1332
Summary by CodeRabbit
New Features
Improvements
Tests & Infrastructure