-
Notifications
You must be signed in to change notification settings - Fork 3
feat: initial tss crate integration #604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the cait-sith threshold signature library with threshold-signatures, enabling support for both ECDSA and future EdDSA signatures. The migration involves comprehensive updates to import statements, type signatures, and protocol implementations across the codebase.
Key changes include:
- Complete replacement of
cait-sithdependency withthreshold-signaturesfrom the NEAR repository - Updated protocol implementations for keygen, resharing, triple generation, presignature, and signature operations
- Increased timeouts for generation/resharing operations due to the new library's performance characteristics (keygen timeout: 10s → 30s, retry attempts: 40 → 80)
Reviewed Changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml / Cargo.lock | Replaced cait-sith dependency with threshold-signatures, updated serde_bytes version |
| chain-signatures/node/src/types.rs | Updated type aliases to use threshold-signatures types, removed Sync bound from TripleProtocol, changed signature protocol return type to Option |
| chain-signatures/node/src/protocol/signature.rs | Reimplemented signature generation with rerandomization using threshold-signatures API, updated to handle optional signature output |
| chain-signatures/node/src/protocol/presignature.rs | Updated presignature generation to use threshold-signatures API with simplified parameters |
| chain-signatures/node/src/protocol/triple.rs | Updated triple generation to use threshold-signatures API |
| chain-signatures/node/src/protocol/cryptography.rs | Updated keygen and reshare protocol implementations with VerifyingKey conversions |
| chain-signatures/node/src/protocol/error.rs | Updated error types and messages, though some naming inconsistencies remain |
| chain-signatures/node/src/kdf.rs | Removed derive_delta function (now handled by threshold-signatures library) |
| chain-signatures/node/src/mesh/mod.rs | Added fallback to old_participants during resharing for better participant lookup |
| integration-tests/* | Updated all imports and type references, adjusted timeouts for slower keygen/reshare |
| doc/*.md | Updated documentation references from cait-sith to threshold-signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ctx.rpc.publish( | ||
| ctx.public_key, | ||
| self.indexed.clone(), | ||
| output, | ||
| self.participants.clone(), | ||
| ); |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature protocol now returns Option<EcdsaSignature> instead of EcdsaSignature. The code correctly handles the None case on lines 875-878, logging and returning early when the node is not the proposer. However, the original logic on lines 886-891 (previously lines 885-893) checked if self.proposer == me before publishing. This check has been removed because only the proposer receives Some(output). Verify that this behavior change is intentional and that non-proposers never accidentally attempt to publish.
| ctx.rpc.publish( | |
| ctx.public_key, | |
| self.indexed.clone(), | |
| output, | |
| self.participants.clone(), | |
| ); | |
| if self.proposer == me { | |
| ctx.rpc.publish( | |
| ctx.public_key, | |
| self.indexed.clone(), | |
| output, | |
| self.participants.clone(), | |
| ); | |
| } else { | |
| tracing::warn!( | |
| ?sign_id, | |
| ?me, | |
| proposer = ?self.proposer, | |
| "Non-proposer received signature output; not publishing" | |
| ); | |
| break Ok(()); | |
| } |
| ProtocolState::Resharing(resharing) => resharing | ||
| .new_participants | ||
| .find(&self.my_id) | ||
| .or_else(|| resharing.old_participants.find(&self.my_id)) | ||
| .map(|(p, info)| (*p, info.clone())), |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Added fallback to old_participants when looking up participant info during resharing. This change on line 176 ensures that nodes in the old participant set (but not in the new set) can still find their participant info. This is a good fix, but verify that this doesn't cause issues if a participant exists in both old and new sets with different info.
| ProtocolState::Resharing(resharing) => resharing | |
| .new_participants | |
| .find(&self.my_id) | |
| .or_else(|| resharing.old_participants.find(&self.my_id)) | |
| .map(|(p, info)| (*p, info.clone())), | |
| ProtocolState::Resharing(resharing) => { | |
| let new = resharing.new_participants.find(&self.my_id); | |
| let old = resharing.old_participants.find(&self.my_id); | |
| if let (Some((_, new_info)), Some((_, old_info))) = (new.as_ref(), old.as_ref()) { | |
| if new_info != old_info { | |
| tracing::warn!( | |
| my_id = ?self.my_id, | |
| "Participant exists in both new and old participants with different info during resharing" | |
| ); | |
| } | |
| } | |
| new.or_else(|| old).map(|(p, info)| (*p, info.clone())) | |
| } |
| fn x_coordinate(point: &k256::AffinePoint) -> k256::Scalar { | ||
| use k256::elliptic_curve::bigint::U256; | ||
| use k256::elliptic_curve::ops::Reduce; | ||
| <k256::Scalar as Reduce<U256>>::reduce_bytes(&point.x()) | ||
| } |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The x_coordinate function has been changed from a generic function over cait_sith::CSCurve to a concrete function for k256::AffinePoint. While this works for the current use case (ECDSA with secp256k1), it removes the ability to use this function with other curves. Given that the PR mentions eventual EdDSA support, consider keeping this function generic or documenting why it's now secp256k1-specific.
| // Rerandomize the presignature using the same public entropy used by | ||
| // derive_delta. `threshold-signatures` exposes a helper for this. | ||
| let msg_hash_bytes: [u8; 32] = indexed.args.payload.to_bytes().into(); | ||
| let tweak = Tweak::new(indexed.args.epsilon); | ||
| let participants_list = ParticipantList::new(&participants).unwrap(); | ||
| let rerand_args = RerandomizationArguments::new( | ||
| ctx.public_key, | ||
| tweak, | ||
| msg_hash_bytes, | ||
| presignature.output.big_r, | ||
| participants_list, | ||
| indexed.args.entropy, | ||
| ); | ||
| let rerandomized: RerandomizedPresignOutput = | ||
| RerandomizedPresignOutput::rerandomize_presign(&presignature.output, &rerand_args) | ||
| .map_err(|e| { | ||
| InitializationError::BadParameters(format!("rerandomization failed: {e:?}")) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The signature generation now uses RerandomizationArguments with the entropy parameter (line 711), which should produce the same rerandomization as the old derive_delta function. However, verify that the threshold-signatures library's rerandomize_presign method uses the entropy in the same way as the removed derive_delta function (which used HKDF with the entropy and request_id).
| let strategy = ConstantBuilder::default() | ||
| .with_delay(std::time::Duration::from_millis(500)) | ||
| .with_max_times(40); | ||
| .with_max_times(80); |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The maximum retry count has been doubled from 40 to 80 (with a 500ms delay, this increases total wait time from 20s to 40s). Similar to the keygen timeout, consider if this is appropriate for production and whether it should be configurable.
e97a9ce to
c3993c4
Compare
c3993c4 to
e1681aa
Compare
e1681aa to
a5e1ae5
Compare
a5e1ae5 to
695dfde
Compare
This does all the initial integration of threshold signatures crate which replaces cait-sith. Pretty much replaces all the usages we had before from cait-sith to now just threshold signatures which also supports eddsa; so we can eventually plumb eddsa in at some point.
Some findings:
@scafuro let me know if there are anything you noticed with this one. I remember you said there were some caveats to how this does keygen/reshare so it makes sense that it is a bit slower: