From 9c1f4729884f0c83fc48e2f4a087faea68ab6270 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 25 Nov 2025 13:59:57 +0400 Subject: [PATCH 01/11] feat: change admin subprotocol from aggregated Schnorr to individual ECDSA signatures --- Cargo.lock | 4 - crates/asm/subprotocols/admin/Cargo.toml | 1 - .../asm/subprotocols/admin/src/authority.rs | 43 +- crates/asm/subprotocols/admin/src/config.rs | 25 +- crates/asm/subprotocols/admin/src/error.rs | 6 +- crates/asm/subprotocols/admin/src/handler.rs | 112 +++-- crates/asm/subprotocols/admin/src/state.rs | 84 ++-- .../bridge-v1/src/state/operator.rs | 2 +- crates/asm/txs/admin/Cargo.toml | 1 - .../txs/admin/src/actions/updates/multisig.rs | 14 +- crates/asm/txs/admin/src/parser.rs | 80 ++-- crates/asm/txs/admin/src/test_utils/mod.rs | 173 +++---- crates/crypto/Cargo.toml | 4 +- crates/crypto/src/lib.rs | 2 +- crates/crypto/src/multisig/config.rs | 431 ------------------ crates/crypto/src/multisig/errors.rs | 63 --- crates/crypto/src/multisig/mod.rs | 61 --- crates/crypto/src/multisig/schemes/mod.rs | 3 - crates/crypto/src/multisig/schemes/schnorr.rs | 80 ---- crates/crypto/src/multisig/signature.rs | 54 --- crates/crypto/src/multisig/traits.rs | 67 --- crates/crypto/src/test_utils/schnorr.rs | 11 +- .../indexed_signatures/config.rs | 297 ++++++++++++ .../indexed_signatures/errors.rs | 102 +++++ .../indexed_signatures/mod.rs | 17 + .../indexed_signatures/pubkey.rs | 173 +++++++ .../indexed_signatures/signature.rs | 207 +++++++++ .../indexed_signatures/verification.rs | 240 ++++++++++ crates/crypto/src/threshold_signing/mod.rs | 17 + .../threshold_signing/musig2/aggregation.rs | 112 +++++ .../src/threshold_signing/musig2/mod.rs | 9 + crates/l1tx/src/utils.rs | 2 +- 32 files changed, 1456 insertions(+), 1041 deletions(-) delete mode 100644 crates/crypto/src/multisig/config.rs delete mode 100644 crates/crypto/src/multisig/errors.rs delete mode 100644 crates/crypto/src/multisig/mod.rs delete mode 100644 crates/crypto/src/multisig/schemes/mod.rs delete mode 100644 crates/crypto/src/multisig/schemes/schnorr.rs delete mode 100644 crates/crypto/src/multisig/signature.rs delete mode 100644 crates/crypto/src/multisig/traits.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/config.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/errors.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/mod.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/signature.rs create mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/verification.rs create mode 100644 crates/crypto/src/threshold_signing/mod.rs create mode 100644 crates/crypto/src/threshold_signing/musig2/aggregation.rs create mode 100644 crates/crypto/src/threshold_signing/musig2/mod.rs diff --git a/Cargo.lock b/Cargo.lock index 5e7441f6ad..d610112075 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13413,7 +13413,6 @@ version = "0.1.0" dependencies = [ "arbitrary", "bitcoin", - "bitvec", "borsh", "rand 0.8.5", "strata-asm-common", @@ -13548,7 +13547,6 @@ version = "0.1.0" dependencies = [ "arbitrary", "bitcoin", - "bitvec", "borsh", "rand 0.8.5", "strata-asm-common", @@ -13927,7 +13925,6 @@ version = "0.3.0-alpha.1" dependencies = [ "arbitrary", "bitcoin", - "bitvec", "borsh", "hex", "k256", @@ -13936,7 +13933,6 @@ dependencies = [ "secp256k1 0.29.1", "serde", "strata-identifiers", - "strata-test-utils", "thiserror 2.0.17", ] diff --git a/crates/asm/subprotocols/admin/Cargo.toml b/crates/asm/subprotocols/admin/Cargo.toml index e48d41d2e5..7f9d1e88bd 100644 --- a/crates/asm/subprotocols/admin/Cargo.toml +++ b/crates/asm/subprotocols/admin/Cargo.toml @@ -22,5 +22,4 @@ thiserror.workspace = true strata-asm-txs-admin = { workspace = true, features = ["test-utils"] } strata-test-utils.workspace = true rand.workspace = true -bitvec.workspace = true bitcoin.workspace = true diff --git a/crates/asm/subprotocols/admin/src/authority.rs b/crates/asm/subprotocols/admin/src/authority.rs index c338f1a612..b505518aab 100644 --- a/crates/asm/subprotocols/admin/src/authority.rs +++ b/crates/asm/subprotocols/admin/src/authority.rs @@ -1,26 +1,25 @@ -use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_txs_admin::actions::MultisigAction; -use strata_crypto::multisig::{ - MultisigError, SchnorrMultisigConfig, SchnorrMultisigSignature, verify_multisig, +use strata_crypto::threshold_signing::{ + SignatureSet, ThresholdConfig, ThresholdSigningError, verify_threshold_signatures, }; use strata_primitives::roles::Role; -/// Manages multisignature operations for a given role and key set, with replay protection via a -/// seqno. -#[derive(Clone, Debug, Eq, PartialEq, Arbitrary, BorshDeserialize, BorshSerialize)] +/// Manages threshold signature operations for a given role and key set, with replay protection via +/// a seqno. +#[derive(Clone, Debug, Eq, PartialEq, BorshDeserialize, BorshSerialize)] pub struct MultisigAuthority { - /// The role of this multisignature authority. + /// The role of this threshold signature authority. role: Role, /// The public keys of all grant-holders authorized to sign. - config: SchnorrMultisigConfig, - /// Sequence number for the multisig configuration. It increases on each valid action. + config: ThresholdConfig, + /// Sequence number for the threshold configuration. It increases on each valid action. /// This is used to prevent replay attacks. seqno: u64, } impl MultisigAuthority { - pub fn new(role: Role, config: SchnorrMultisigConfig) -> Self { + pub fn new(role: Role, config: ThresholdConfig) -> Self { Self { role, config, @@ -28,35 +27,35 @@ impl MultisigAuthority { } } - /// The role authorized to perform multisig operations. + /// The role authorized to perform threshold signature operations. pub fn role(&self) -> Role { self.role } - /// Borrow the current multisig configuration. - pub fn config(&self) -> &SchnorrMultisigConfig { + /// Borrow the current threshold configuration. + pub fn config(&self) -> &ThresholdConfig { &self.config } - /// Mutably borrow the multisig configuration. - pub fn config_mut(&mut self) -> &mut SchnorrMultisigConfig { + /// Mutably borrow the threshold configuration. + pub fn config_mut(&mut self) -> &mut ThresholdConfig { &mut self.config } - /// Verify that `signature` is a valid threshold signature for `action` under the current config - /// and seqno. + /// Verify that `signatures` is a valid threshold signature set for `action` under the current + /// config and seqno. /// - /// Uses the generic multisig verification function to orchestrate the workflow. + /// Uses individual ECDSA signature verification against each signer's public key. pub fn verify_action_signature( &self, action: &MultisigAction, - signature: &SchnorrMultisigSignature, - ) -> Result<(), MultisigError> { + signatures: &SignatureSet, + ) -> Result<(), ThresholdSigningError> { // Compute the msg to sign by combining UpdateAction with sequence no let sig_hash = action.compute_sighash(self.seqno); - // Use the generic multisig verification function - verify_multisig(&self.config, signature, &sig_hash.into()) + // Verify each ECDSA signature against the corresponding public key + verify_threshold_signatures(&self.config, signatures, &sig_hash.into()) } /// Increments the seqno. diff --git a/crates/asm/subprotocols/admin/src/config.rs b/crates/asm/subprotocols/admin/src/config.rs index 6b093702d5..110d28f7b8 100644 --- a/crates/asm/subprotocols/admin/src/config.rs +++ b/crates/asm/subprotocols/admin/src/config.rs @@ -1,22 +1,21 @@ -use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; -use strata_crypto::multisig::SchnorrMultisigConfig; +use strata_crypto::threshold_signing::ThresholdConfig; use strata_primitives::roles::Role; -/// Parameters for the admnistration subprotocol, containing MultisigConfig for each role. +/// Parameters for the admnistration subprotocol, containing ThresholdConfig for each role. /// -/// Design choice: Uses individual named fields rather than `Vec<(Role, MultisigConfig)>` +/// Design choice: Uses individual named fields rather than `Vec<(Role, ThresholdConfig)>` /// to ensure structural completeness - the compiler guarantees all 4 config fields are /// provided when constructing this struct. However, it does NOT prevent logical errors /// like using the same config for multiple roles or mismatched role-field assignments. /// The benefit is avoiding missing fields at compile-time rather than runtime validation. -#[derive(Clone, Debug, Eq, PartialEq, Arbitrary, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, Eq, PartialEq, BorshDeserialize, BorshSerialize)] pub struct AdministrationSubprotoParams { - /// MultisigConfig for [StrataAdministrator](Role::StrataAdministrator). - pub strata_administrator: SchnorrMultisigConfig, + /// ThresholdConfig for [StrataAdministrator](Role::StrataAdministrator). + pub strata_administrator: ThresholdConfig, - /// MultisigConfig for [StrataSequencerManager](Role::StrataSequencerManager). - pub strata_sequencer_manager: SchnorrMultisigConfig, + /// ThresholdConfig for [StrataSequencerManager](Role::StrataSequencerManager). + pub strata_sequencer_manager: ThresholdConfig, /// The confirmation depth (CD) setting: after an update transaction receives this many /// confirmations, the update is enacted automatically. During this confirmation period, @@ -26,8 +25,8 @@ pub struct AdministrationSubprotoParams { impl AdministrationSubprotoParams { pub fn new( - strata_administrator: SchnorrMultisigConfig, - strata_sequencer_manager: SchnorrMultisigConfig, + strata_administrator: ThresholdConfig, + strata_sequencer_manager: ThresholdConfig, confirmation_depth: u32, ) -> Self { Self { @@ -37,14 +36,14 @@ impl AdministrationSubprotoParams { } } - pub fn get_config(&self, role: Role) -> &SchnorrMultisigConfig { + pub fn get_config(&self, role: Role) -> &ThresholdConfig { match role { Role::StrataAdministrator => &self.strata_administrator, Role::StrataSequencerManager => &self.strata_sequencer_manager, } } - pub fn get_all_authorities(self) -> Vec<(Role, SchnorrMultisigConfig)> { + pub fn get_all_authorities(self) -> Vec<(Role, ThresholdConfig)> { vec![ (Role::StrataAdministrator, self.strata_administrator), (Role::StrataSequencerManager, self.strata_sequencer_manager), diff --git a/crates/asm/subprotocols/admin/src/error.rs b/crates/asm/subprotocols/admin/src/error.rs index 6a1fda2da8..22c2c87e3e 100644 --- a/crates/asm/subprotocols/admin/src/error.rs +++ b/crates/asm/subprotocols/admin/src/error.rs @@ -1,5 +1,5 @@ use strata_asm_txs_admin::actions::UpdateId; -use strata_crypto::multisig::MultisigError; +use strata_crypto::threshold_signing::ThresholdSigningError; use thiserror::Error; /// Top-level error type for the administration subprotocol, composed of smaller error categories. @@ -13,7 +13,7 @@ pub enum AdministrationError { #[error("no pending update found for action_id = {0:?}")] UnknownAction(UpdateId), - /// Indicates a multisig error (configuration, aggregation, or signature validation). + /// Indicates a threshold signing error (configuration or signature validation). #[error(transparent)] - Multisig(#[from] MultisigError), + ThresholdSigning(#[from] ThresholdSigningError), } diff --git a/crates/asm/subprotocols/admin/src/handler.rs b/crates/asm/subprotocols/admin/src/handler.rs index e32886af7e..e701de5309 100644 --- a/crates/asm/subprotocols/admin/src/handler.rs +++ b/crates/asm/subprotocols/admin/src/handler.rs @@ -4,7 +4,7 @@ use strata_asm_common::{ }; use strata_asm_proto_checkpoint_v0::CheckpointIncomingMsg; use strata_asm_txs_admin::actions::{MultisigAction, UpdateAction}; -use strata_crypto::multisig::SchnorrMultisigSignature; +use strata_crypto::threshold_signing::SignatureSet; use strata_predicate::PredicateKey; use strata_primitives::{buf::Buf32, roles::ProofType}; @@ -81,12 +81,12 @@ pub(crate) fn handle_pending_updates( } } -/// Processes a multisig action (an admin "change" message) by validating the aggregated signature +/// Processes a multisig action (an admin "change" message) by validating the signature set /// and executing the requested operation. /// /// This function handles the complete lifecycle of a multisig action: /// 1. Determines the required role based on the action type -/// 2. Validates that the aggregated signature meets the multisig requirements for that role +/// 2. Validates that the signature set meets the threshold requirements for that role /// 3. Processes the action based on its type: /// - `Update`: Queues the action for later execution (except sequencer updates which apply /// immediately) @@ -99,7 +99,7 @@ pub(crate) fn handle_pending_updates( pub(crate) fn handle_action( state: &mut AdministrationSubprotoState, action: MultisigAction, - sig: SchnorrMultisigSignature, + sig: SignatureSet, current_height: u64, relayer: &mut impl MsgRelayer, params: &AdministrationSubprotoParams, @@ -177,9 +177,8 @@ fn relay_checkpoint_predicate(relayer: &mut impl MsgRelayer, key: PredicateKey) mod tests { use std::any::Any; - use bitcoin::secp256k1::{SECP256K1, SecretKey}; - use bitvec::prelude::*; use rand::{rngs::OsRng, seq::SliceRandom, thread_rng}; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use strata_asm_common::{AsmLogEntry, InterprotoMsg, MsgRelayer}; use strata_asm_proto_checkpoint_v0::CheckpointIncomingMsg; use strata_asm_txs_admin::{ @@ -187,19 +186,13 @@ mod tests { CancelAction, MultisigAction, UpdateAction, updates::{predicate::PredicateUpdate, seq::SequencerUpdate}, }, - test_utils::create_multisig_signature, + test_utils::create_signature_set, }; - use strata_crypto::{ - EvenSecretKey, - multisig::{ - MultisigError, SchnorrMultisigConfig, SchnorrScheme, signature::AggregatedSignature, - }, + use strata_crypto::threshold_signing::{ + CompressedPublicKey, SignatureSet, ThresholdConfig, ThresholdSigningError, }; use strata_predicate::PredicateKey; - use strata_primitives::{ - buf::{Buf32, Buf64}, - roles::{ProofType, Role}, - }; + use strata_primitives::roles::{ProofType, Role}; use strata_test_utils::ArbitraryGenerator; use super::{handle_action, handle_pending_updates}; @@ -247,25 +240,27 @@ mod tests { fn create_test_params() -> ( AdministrationSubprotoParams, - Vec, - Vec, + Vec, + Vec, ) { - let strata_admin_sks: Vec = - (0..3).map(|_| SecretKey::new(&mut OsRng).into()).collect(); - let strata_admin_pks: Vec = strata_admin_sks + let secp = Secp256k1::new(); + + let strata_admin_sks: Vec = + (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let strata_admin_pks: Vec = strata_admin_sks .iter() - .map(|sk| sk.x_only_public_key(SECP256K1).0.into()) + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_administrator = SchnorrMultisigConfig::try_new(strata_admin_pks, 2).unwrap(); + let strata_administrator = ThresholdConfig::try_new(strata_admin_pks, 2).unwrap(); - let strata_seq_manager_sks: Vec = - (0..3).map(|_| SecretKey::new(&mut OsRng).into()).collect(); - let strata_seq_manager_pks: Vec = strata_seq_manager_sks + let strata_seq_manager_sks: Vec = + (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let strata_seq_manager_pks: Vec = strata_seq_manager_sks .iter() - .map(|sk| sk.x_only_public_key(SECP256K1).0.into()) + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); let strata_sequencer_manager = - SchnorrMultisigConfig::try_new(strata_seq_manager_pks, 2).unwrap(); + ThresholdConfig::try_new(strata_seq_manager_pks, 2).unwrap(); let config = AdministrationSubprotoParams { strata_administrator, @@ -305,7 +300,7 @@ mod tests { let updates = get_strata_administrator_update_actions(5); // Create signer indices (signers 0 and 2) - let signer_indices = bitvec![u8, Lsb0; 1, 0, 1]; + let signer_indices = [0u8, 2u8]; for update in updates { // Capture initial state before processing the update @@ -315,11 +310,11 @@ mod tests { let action = MultisigAction::Update(update.clone()); let sighash = action.compute_sighash(initial_seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); handle_action( &mut state, action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -366,16 +361,16 @@ mod tests { let update = get_strata_administrator_update_actions(1)[0].clone(); // Create signer indices (signers 0 and 2) - let signer_indices = bitvec![u8, Lsb0; 1, 0, 1]; + let signer_indices = [0u8, 2u8]; // Create an action and queue that. let action = MultisigAction::Update(update.clone()); let sighash = action.compute_sighash(initial_seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); let res = handle_action( &mut state, action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -385,11 +380,11 @@ mod tests { // Try queuing it again with same seq no let action = MultisigAction::Update(update.clone()); let sighash = action.compute_sighash(initial_seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); let res = handle_action( &mut state, action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -397,8 +392,8 @@ mod tests { assert!(res.is_err()); assert!(matches!( res, - Err(AdministrationError::Multisig( - MultisigError::InvalidSignature + Err(AdministrationError::ThresholdSigning( + ThresholdSigningError::InvalidSignature { .. } )) )); @@ -406,19 +401,19 @@ mod tests { let seq_no: u64 = ArbitraryGenerator::new().generate(); let action = MultisigAction::Update(update.clone()); let sighash = action.compute_sighash(seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); let res = handle_action( &mut state, action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, ); assert!(matches!( res, - Err(AdministrationError::Multisig( - MultisigError::InvalidSignature + Err(AdministrationError::ThresholdSigning( + ThresholdSigningError::InvalidSignature { .. } )) )); } @@ -442,7 +437,7 @@ mod tests { let update_count = updates.len(); // Create signer indices (signers 0 and 2) - let signer_indices = bitvec![u8, Lsb0; 1, 0, 1]; + let signer_indices = [0u8, 2u8]; for update in updates { let update: UpdateAction = update.into(); @@ -453,13 +448,12 @@ mod tests { let action = MultisigAction::Update(update.clone()); let sighash = action.compute_sighash(initial_seq_no); - let multisig = - create_multisig_signature(&seq_manager_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&seq_manager_sks, &signer_indices, sighash); handle_action( &mut state, action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -537,7 +531,7 @@ mod tests { let current_height = 1000; // create signer indices (signers 0 and 2) - let signer_indices = bitvec![u8, Lsb0; 1, 0, 1]; + let signer_indices = [0u8, 2u8]; // First, queue 5 update actions let updates = get_strata_administrator_update_actions(no_of_updates); @@ -547,12 +541,12 @@ mod tests { let update_action = MultisigAction::Update(update); let sighash = update_action.compute_sighash(seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); handle_action( &mut state, update_action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -574,12 +568,12 @@ mod tests { let initial_queued_len = state.queued().len(); let sighash = cancel_action.compute_sighash(initial_seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); handle_action( &mut state, cancel_action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -611,7 +605,7 @@ mod tests { let (params, _, _) = create_test_params(); let mut state = AdministrationSubprotoState::new(¶ms); let mut relayer = MockRelayer::::new(); - let multisig = AggregatedSignature::::new(BitVec::new(), Buf64::default()); + let sig_set = SignatureSet::new(vec![]).unwrap(); let current_height = 1000; // Generate a random cancel action (likely targeting a non-existent ID) @@ -622,7 +616,7 @@ mod tests { let res = handle_action( &mut state, cancel_action, - multisig, + sig_set, current_height, &mut relayer, ¶ms, @@ -651,16 +645,16 @@ mod tests { let update_action = MultisigAction::Update(update); // create signer indices (signers 0 and 2) - let signer_indices = bitvec![u8, Lsb0; 1, 0, 1]; + let signer_indices = [0u8, 2u8]; let sighash = update_action.compute_sighash(initial_seq_no); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); // Queue the update action handle_action( &mut state, update_action, - multisig.clone(), + sig_set.clone(), current_height, &mut relayer, ¶ms, @@ -670,11 +664,11 @@ mod tests { // Cancel the update action let cancel_action = MultisigAction::Cancel(CancelAction::new(update_id)); let sighash = cancel_action.compute_sighash(initial_seq_no + 1); - let multisig = create_multisig_signature(&admin_sks, signer_indices.clone(), sighash); + let sig_set = create_signature_set(&admin_sks, &signer_indices, sighash); let res = handle_action( &mut state, cancel_action, - multisig.clone(), + sig_set.clone(), current_height, &mut relayer, ¶ms, @@ -686,7 +680,7 @@ mod tests { let res = handle_action( &mut state, cancel_action, - multisig.clone(), + sig_set.clone(), current_height, &mut relayer, ¶ms, diff --git a/crates/asm/subprotocols/admin/src/state.rs b/crates/asm/subprotocols/admin/src/state.rs index 4af681b5e6..c14a44ff16 100644 --- a/crates/asm/subprotocols/admin/src/state.rs +++ b/crates/asm/subprotocols/admin/src/state.rs @@ -1,6 +1,6 @@ use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_txs_admin::actions::UpdateId; -use strata_crypto::multisig::SchnorrMultisigConfigUpdate; +use strata_crypto::threshold_signing::ThresholdConfigUpdate; use strata_primitives::roles::Role; use crate::{ @@ -51,11 +51,11 @@ impl AdministrationSubprotoState { self.authorities.get_mut(role as usize) } - /// Apply a multisig config update for the specified role. + /// Apply a threshold config update for the specified role. pub fn apply_multisig_update( &mut self, role: Role, - update: &SchnorrMultisigConfigUpdate, + update: &ThresholdConfigUpdate, ) -> Result<(), AdministrationError> { if let Some(auth) = self.authority_mut(role) { auth.config_mut().apply_update(update)?; @@ -110,10 +110,11 @@ impl AdministrationSubprotoState { #[cfg(test)] mod tests { - use rand::{Rng, thread_rng}; + use rand::rngs::OsRng; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use strata_asm_txs_admin::actions::UpdateAction; - use strata_crypto::multisig::config::MultisigConfigUpdate; - use strata_primitives::{buf::Buf32, roles::Role}; + use strata_crypto::threshold_signing::{CompressedPublicKey, ThresholdConfig, ThresholdConfigUpdate}; + use strata_primitives::roles::Role; use strata_test_utils::ArbitraryGenerator; use crate::{ @@ -121,10 +122,35 @@ mod tests { state::AdministrationSubprotoState, }; + fn create_test_config() -> AdministrationSubprotoParams { + let secp = Secp256k1::new(); + + // Create admin keys + let admin_sks: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let admin_pks: Vec = admin_sks + .iter() + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) + .collect(); + let strata_administrator = ThresholdConfig::try_new(admin_pks, 2).unwrap(); + + // Create seq manager keys + let seq_sks: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let seq_pks: Vec = seq_sks + .iter() + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) + .collect(); + let strata_sequencer_manager = ThresholdConfig::try_new(seq_pks, 2).unwrap(); + + AdministrationSubprotoParams { + strata_administrator, + strata_sequencer_manager, + confirmation_depth: 2016, + } + } + #[test] fn test_initial_state() { - let mut arb = ArbitraryGenerator::new(); - let config: AdministrationSubprotoParams = arb.generate(); + let config = create_test_config(); let state = AdministrationSubprotoState::new(&config); assert_eq!(state.next_update_id(), 0); @@ -134,7 +160,7 @@ mod tests { #[test] fn test_enqueue_find_and_remove_queued() { let mut arb = ArbitraryGenerator::new(); - let config: AdministrationSubprotoParams = arb.generate(); + let config = create_test_config(); let mut state = AdministrationSubprotoState::new(&config); // Use arbitrary action or fallback to guaranteed queueable action @@ -153,7 +179,7 @@ mod tests { /// Helper to seed queued updates with specific activation heights fn seed_queued(ids: &[u32], activation_heights: &[u64]) -> AdministrationSubprotoState { let mut arb = ArbitraryGenerator::new(); - let config = arb.generate(); + let config = create_test_config(); let mut state = AdministrationSubprotoState::new(&config); for (&id, &activation_height) in ids.iter().zip(activation_heights.iter()) { @@ -223,34 +249,31 @@ mod tests { #[test] fn test_apply_multisig_update() { - let mut arb = ArbitraryGenerator::new(); - let config: AdministrationSubprotoParams = arb.generate(); + let secp = Secp256k1::new(); + let config = create_test_config(); let mut state = AdministrationSubprotoState::new(&config); - let role: Role = arb.generate(); + let role = Role::StrataAdministrator; let initial_auth = state.authority(role).unwrap().config(); - let initial_members: Vec = initial_auth.keys().to_vec(); - - let add_members: Vec = arb.generate(); + let initial_members: Vec = initial_auth.keys().to_vec(); - // Randomly pick some members to remove - let mut rng = thread_rng(); - let mut remove_members = Vec::new(); + // Generate new members to add + let add_sks: Vec = (0..2).map(|_| SecretKey::new(&mut OsRng)).collect(); + let add_members: Vec = add_sks + .iter() + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) + .collect(); - for member in &initial_members { - if rng.gen_bool(0.3) { - // 30% chance to remove each member - remove_members.push(*member); - } - } + // Remove the first member + let remove_members = vec![initial_members[0]]; let new_size = initial_members.len() + add_members.len() - remove_members.len(); - let new_threshold = rng.gen_range(1..=new_size); + let new_threshold = 2u8; - let update = MultisigConfigUpdate::new( + let update = ThresholdConfigUpdate::new( add_members.clone(), remove_members.clone(), - new_threshold as u8, + new_threshold, ); state.apply_multisig_update(role, &update).unwrap(); @@ -258,7 +281,10 @@ mod tests { let updated_auth = state.authority(role).unwrap().config(); // Verify threshold was updated - assert_eq!(updated_auth.threshold(), new_threshold as u8); + assert_eq!(updated_auth.threshold(), new_threshold); + + // Verify size is correct + assert_eq!(updated_auth.keys().len(), new_size); // Verify that specified members were removed for member_to_remove in &remove_members { diff --git a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs index 9785458d6c..b5bdc6844a 100644 --- a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs +++ b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs @@ -5,7 +5,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; use serde::{Deserialize, Serialize}; use strata_bridge_types::OperatorIdx; -use strata_crypto::{multisig::aggregate_schnorr_keys, schnorr::EvenPublicKey}; +use strata_crypto::{schnorr::EvenPublicKey, threshold_signing::aggregate_schnorr_keys}; use strata_primitives::{buf::Buf32, l1::BitcoinXOnlyPublicKey, sorted_vec::SortedVec}; use super::bitmap::OperatorBitmap; diff --git a/crates/asm/txs/admin/Cargo.toml b/crates/asm/txs/admin/Cargo.toml index ec5773ed5c..b6f645cac7 100644 --- a/crates/asm/txs/admin/Cargo.toml +++ b/crates/asm/txs/admin/Cargo.toml @@ -15,7 +15,6 @@ strata-primitives.workspace = true arbitrary.workspace = true bitcoin = { workspace = true, optional = true } -bitvec.workspace = true borsh.workspace = true rand = { workspace = true, optional = true } strata-l1-envelope-fmt.workspace = true diff --git a/crates/asm/txs/admin/src/actions/updates/multisig.rs b/crates/asm/txs/admin/src/actions/updates/multisig.rs index d2bd64f0f5..f8604d511b 100644 --- a/crates/asm/txs/admin/src/actions/updates/multisig.rs +++ b/crates/asm/txs/admin/src/actions/updates/multisig.rs @@ -1,26 +1,26 @@ use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; -use strata_crypto::multisig::SchnorrMultisigConfigUpdate; +use strata_crypto::threshold_signing::ThresholdConfigUpdate; use strata_primitives::roles::Role; -/// An update to a multisig configuration for a specific role: +/// An update to a threshold configuration for a specific role: /// - adds new members /// - removes old members /// - updates the threshold #[derive(Clone, Debug, Eq, PartialEq, Arbitrary, BorshDeserialize, BorshSerialize)] pub struct MultisigUpdate { - config: SchnorrMultisigConfigUpdate, + config: ThresholdConfigUpdate, role: Role, } impl MultisigUpdate { /// Create a `MultisigUpdate` with given config and role. - pub fn new(config: SchnorrMultisigConfigUpdate, role: Role) -> Self { + pub fn new(config: ThresholdConfigUpdate, role: Role) -> Self { Self { config, role } } - /// Borrow the multisig config update. - pub fn config(&self) -> &SchnorrMultisigConfigUpdate { + /// Borrow the threshold config update. + pub fn config(&self) -> &ThresholdConfigUpdate { &self.config } @@ -30,7 +30,7 @@ impl MultisigUpdate { } /// Consume and return the inner config and role. - pub fn into_inner(self) -> (SchnorrMultisigConfigUpdate, Role) { + pub fn into_inner(self) -> (ThresholdConfigUpdate, Role) { (self.config, self.role) } } diff --git a/crates/asm/txs/admin/src/parser.rs b/crates/asm/txs/admin/src/parser.rs index 903351d321..0c0ec46301 100644 --- a/crates/asm/txs/admin/src/parser.rs +++ b/crates/asm/txs/admin/src/parser.rs @@ -1,15 +1,34 @@ -use bitvec::vec::BitVec; +use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_common::TxInputRef; -use strata_crypto::multisig::SchnorrMultisigSignature; +use strata_crypto::threshold_signing::SignatureSet; use strata_l1_envelope_fmt::parser::parse_envelope_payload; use crate::{actions::MultisigAction, errors::AdministrationTxParseError}; -/// Parses a transaction to extract both the multisig action and the aggregated signature. +/// A signed administration payload containing both the action and its signatures. /// -/// This function extracts the administrative action from the taproot leaf script embedded -/// in the transaction's witness data, and parses the aggregated signature from -/// the transaction's auxiliary data. +/// This structure is serialized with Borsh and embedded in the witness envelope. +/// The OP_RETURN only contains the SPS-50 tag (magic bytes, subprotocol ID, tx type). +#[derive(Clone, Debug, Eq, PartialEq, BorshDeserialize, BorshSerialize)] +pub struct SignedPayload { + /// The administrative action being proposed + pub action: MultisigAction, + /// The set of ECDSA signatures authorizing this action + pub signatures: SignatureSet, +} + +impl SignedPayload { + /// Creates a new signed payload combining an action with its signatures. + pub fn new(action: MultisigAction, signatures: SignatureSet) -> Self { + Self { action, signatures } + } +} + +/// Parses a transaction to extract both the multisig action and the signature set. +/// +/// This function extracts the signed payload from the taproot leaf script embedded +/// in the transaction's witness data. The payload contains both the administrative +/// action and its authorizing signatures. /// /// # Arguments /// * `tx` - A reference to the transaction input to parse @@ -17,19 +36,16 @@ use crate::{actions::MultisigAction, errors::AdministrationTxParseError}; /// # Returns /// A tuple containing: /// - `MultisigAction` - The administrative action extracted from the envelope payload -/// - `SchnorrMultisigSignature` - The aggregated signature with signer indices +/// - `SignatureSet` - The set of indexed ECDSA signatures /// /// # Errors /// Returns `AdministrationTxParseError` if: /// - The transaction lacks a taproot leaf script in its witness /// - The envelope payload cannot be parsed -/// - The action cannot be deserialized from the payload -/// - The aggregated signature parsing fails +/// - The signed payload cannot be deserialized pub fn parse_tx( tx: &TxInputRef<'_>, -) -> Result<(MultisigAction, SchnorrMultisigSignature), AdministrationTxParseError> { - // Parse the aggregated signature first - let agg_multisig = parse_aggregated_multisig(tx)?; +) -> Result<(MultisigAction, SignatureSet), AdministrationTxParseError> { let tx_type = tx.tag().tx_type(); // Extract the taproot leaf script from the first input's witness @@ -42,43 +58,9 @@ pub fn parse_tx( // Parse the envelope payload from the script let envelope_payload = parse_envelope_payload(&payload_script.into())?; - // Deserialize the multisig action from the payload - let action = borsh::from_slice(&envelope_payload) + // Deserialize the signed payload (action + signatures) from the envelope + let signed_payload: SignedPayload = borsh::from_slice(&envelope_payload) .map_err(|_| AdministrationTxParseError::MalformedTransaction(tx_type))?; - Ok((action, agg_multisig)) -} - -/// Parses the aggregated signature from transaction auxiliary data. -/// -/// The auxiliary data contains a 64-byte Schnorr signature followed by a bit vector -/// indicating which signers participated in the aggregated signature. -/// -/// # Arguments -/// * `tx` - A reference to the transaction input containing the auxiliary data -/// -/// # Returns -/// A `SchnorrMultisigSignature` containing the aggregated signature and signer indices -/// -/// # Errors -/// Returns `AdministrationTxParseError` if the auxiliary data format is invalid -/// -/// # Data Format -/// The auxiliary data is structured as: -/// - Bytes 0-63: 64-byte Schnorr signature -/// - Bytes 64+: Bit vector representing signer indices -pub fn parse_aggregated_multisig( - tx: &TxInputRef<'_>, -) -> Result { - let data = tx.tag().aux_data(); - - // Extract the 64-byte signature from the beginning of aux data - let mut sig = [0u8; 64]; - sig.copy_from_slice(&data[0..64]); - - // Extract signer indices from the remaining bytes as a bit vector - let signer_indices_bytes = &data[64..]; - let indices: BitVec = BitVec::from_slice(signer_indices_bytes); - - Ok(SchnorrMultisigSignature::new(indices, sig.into())) + Ok((signed_payload.action, signed_payload.signatures)) } diff --git a/crates/asm/txs/admin/src/test_utils/mod.rs b/crates/asm/txs/admin/src/test_utils/mod.rs index 9d0a9233dc..20e268d5aa 100644 --- a/crates/asm/txs/admin/src/test_utils/mod.rs +++ b/crates/asm/txs/admin/src/test_utils/mod.rs @@ -8,64 +8,77 @@ use bitcoin::{ all::{OP_CHECKMULTISIG, OP_ENDIF, OP_IF}, }, script::PushBytesBuf, - secp256k1::{SECP256K1, schnorr::Signature}, + secp256k1::{Message, SECP256K1, SecretKey, schnorr::Signature}, taproot::{LeafVersion, TaprootBuilder}, transaction::Version, }; -use bitvec::vec::BitVec; use rand::{RngCore, rngs::OsRng}; -use strata_crypto::{ - EvenSecretKey, - multisig::{schemes::SchnorrScheme, signature::AggregatedSignature}, - test_utils::schnorr::create_musig2_signature, -}; -use strata_primitives::buf::{Buf32, Buf64}; +use strata_crypto::threshold_signing::{IndexedSignature, SignatureSet}; +use strata_primitives::buf::Buf32; pub(crate) const TEST_MAGIC_BYTES: &[u8; 4] = b"ALPN"; -use crate::{actions::MultisigAction, constants::ADMINISTRATION_SUBPROTOCOL_ID}; +use crate::{ + actions::MultisigAction, + constants::ADMINISTRATION_SUBPROTOCOL_ID, + parser::SignedPayload, +}; -/// Creates an AggregatedSignature for any MultisigAction. +/// Creates an ECDSA recoverable signature for a message hash. /// -/// This function generates the required signature for any administration action +/// Returns a 65-byte signature in the format: recovery_id || r || s +pub fn sign_ecdsa_recoverable(message_hash: &[u8; 32], secret_key: &SecretKey) -> [u8; 65] { + let message = Message::from_digest_slice(message_hash).expect("32 bytes"); + let sig = SECP256K1.sign_ecdsa_recoverable(&message, secret_key); + let (recovery_id, compact) = sig.serialize_compact(); + + let mut result = [0u8; 65]; + result[0] = recovery_id.to_i32() as u8; + result[1..65].copy_from_slice(&compact); + result +} + +/// Creates a SignatureSet for any MultisigAction. +/// +/// This function generates the required signatures for any administration action /// (Update or Cancel) by computing the sighash from the action and sequence number, -/// then creating a MuSig2 signature using the provided private keys. +/// then creating individual ECDSA signatures using the provided private keys. /// /// # Arguments -/// * `privkeys` - Private keys of all signers in the multisig config -/// * `signer_indices` - BitVec indicating which signers are participating in this signature -/// * `action` - The MultisigAction to sign (Update or Cancel) -/// * `seqno` - The sequence number for this operation +/// * `privkeys` - Private keys of all signers in the threshold config +/// * `signer_indices` - Indices of signers participating in this signature +/// * `sighash` - The message hash to sign /// /// # Returns -/// An AggregatedSignature that can be used to authorize this action -pub fn create_multisig_signature( - privkeys: &[EvenSecretKey], - signer_indices: BitVec, +/// A SignatureSet that can be used to authorize this action +pub fn create_signature_set( + privkeys: &[SecretKey], + signer_indices: &[u8], sighash: Buf32, -) -> AggregatedSignature { - // Extract only the private keys for signers indicated by signer_indices - let selected_privkeys: Vec = signer_indices - .iter_ones() - .map(|index| privkeys[index]) +) -> SignatureSet { + let signatures: Vec = signer_indices + .iter() + .map(|&index| { + let sig = sign_ecdsa_recoverable(&sighash.0, &privkeys[index as usize]); + IndexedSignature::new(index, sig) + }) .collect(); - let signature = create_musig2_signature(&selected_privkeys, &sighash.0, None); - let signature_buf = Buf64::from(signature.serialize()); - - AggregatedSignature::new(signer_indices, signature_buf) + SignatureSet::new(signatures).expect("valid signature set") } /// Creates a SPS-50 compliant administration transaction with commit-reveal pattern. /// -/// This function creates only the reveal transaction that contains both the action and signature. +/// This function creates only the reveal transaction that contains both the action and signatures. /// The reveal transaction uses the envelope script format to embed the administration payload /// in a way that's compatible with SPS-50. /// +/// The signed payload (action + signatures) is embedded in the witness envelope, while only +/// the minimal SPS-50 tag (magic bytes, subprotocol ID, tx type) is placed in the OP_RETURN. +/// /// # Arguments -/// * `params` - Network parameters containing rollup configuration -/// * `privkeys` - Private keys of all signers in the multisig config -/// * `signer_indices` - BitVec indicating which signers are participating in this signature +/// * `privkeys` - Private keys of all signers in the threshold config +/// * `signer_indices` - Indices of signers participating in this signature /// * `action` - The MultisigAction to sign and embed (Update or Cancel) /// * `seqno` - The sequence number for this operation /// @@ -73,36 +86,30 @@ pub fn create_multisig_signature( /// A Bitcoin transaction that serves as the reveal transaction containing the administration /// payload pub fn create_test_admin_tx( - privkeys: &[EvenSecretKey], - signer_indices: BitVec, + privkeys: &[SecretKey], + signer_indices: &[u8], action: &MultisigAction, seqno: u64, ) -> Transaction { - // Compute the signature hash and create the aggregated signature + // Compute the signature hash and create the signature set let sighash = action.compute_sighash(seqno); - let signature = create_multisig_signature(privkeys, signer_indices, sighash); - - // Create auxiliary data in the expected format for deposit transactions - let mut aux_data = Vec::new(); - aux_data.extend_from_slice(signature.signature().as_bytes()); // 64 bytes + let signature_set = create_signature_set(privkeys, signer_indices, sighash); - let signer_indices_bytes = signature.signer_indices().to_bitvec().into_vec(); - aux_data.extend_from_slice(&signer_indices_bytes); // variable length bitset as bytes + // Create the signed payload (action + signatures) for the envelope + let signed_payload = SignedPayload::new(action.clone(), signature_set); + let envelope_payload = borsh::to_vec(&signed_payload).expect("borsh serialization failed"); - // Create the complete SPS-50 tagged payload - // Format: [MAGIC_BYTES][SUBPROTOCOL_ID][TX_TYPE][AUX_DATA] + // Create the minimal SPS-50 tag for OP_RETURN (no aux data needed) + // Format: [MAGIC_BYTES][SUBPROTOCOL_ID][TX_TYPE] let mut tagged_payload = Vec::new(); tagged_payload.extend_from_slice(TEST_MAGIC_BYTES); // 4 bytes magic tagged_payload.extend_from_slice(&ADMINISTRATION_SUBPROTOCOL_ID.to_be_bytes()); // 1 byte subprotocol ID tagged_payload.extend_from_slice(&[action.tx_type()]); // 1 byte TxType - tagged_payload.extend_from_slice(&aux_data); // auxiliary data - - let action_payload = borsh::to_vec(action).expect("borsh verification failed"); // Create a minimal reveal transaction structure // This is a simplified version - in practice, this would be created as part of // a proper commit-reveal transaction pair using the btcio writer infrastructure - create_reveal_transaction_stub(action_payload, tagged_payload) + create_reveal_transaction_stub(envelope_payload, tagged_payload) } /// Creates a stub reveal transaction containing the envelope script. @@ -184,10 +191,12 @@ fn build_envelope_script(payload: &[u8]) -> ScriptBuf { #[cfg(test)] mod tests { - use bitcoin::secp256k1::{SECP256K1, SecretKey}; + use bitcoin::secp256k1::{PublicKey, Secp256k1}; use rand::rngs::OsRng; use strata_asm_common::TxInputRef; - use strata_crypto::multisig::{SchnorrMultisigConfig, verify_multisig}; + use strata_crypto::threshold_signing::{ + verify_threshold_signatures, CompressedPublicKey, ThresholdConfig, + }; use strata_l1_txfmt::ParseConfig; use strata_test_utils::ArbitraryGenerator; @@ -195,40 +204,36 @@ mod tests { use crate::parser::parse_tx; #[test] - fn test_create_multisig_update_signature() { + fn test_create_signature_set() { let mut arb = ArbitraryGenerator::new(); let seqno = 1; let threshold = 2; + let secp = Secp256k1::new(); // Generate test private keys - let privkeys: Vec = - (0..3).map(|_| SecretKey::new(&mut OsRng).into()).collect(); - let pubkeys = privkeys + let privkeys: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let pubkeys: Vec = privkeys .iter() - .map(|sk| sk.x_only_public_key(SECP256K1).0.into()) - .collect::>(); - let config = SchnorrMultisigConfig::try_new(pubkeys, threshold).unwrap(); + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) + .collect(); + let config = ThresholdConfig::try_new(pubkeys, threshold).unwrap(); // Create signer indices (signers 0 and 2) - let mut signer_indices = BitVec::::new(); - signer_indices.resize(3, false); - signer_indices.set(0, true); - signer_indices.set(2, true); + let signer_indices = [0u8, 2u8]; - // Create a test multisig update + // Create a test multisig action let action: MultisigAction = arb.generate(); let sighash = action.compute_sighash(seqno); - let signature = create_multisig_signature(&privkeys, signer_indices.clone(), sighash); + let signature_set = create_signature_set(&privkeys, &signer_indices, sighash); - // Verify the signature has the expected structure - assert_eq!(signature.signer_indices().len(), 3); - assert_eq!(signature.signer_indices().count_ones(), 2); - assert!(signature.signer_indices()[0]); - assert!(!signature.signer_indices()[1]); - assert!(signature.signer_indices()[2]); + // Verify the signature set has the expected structure + assert_eq!(signature_set.len(), 2); + let indices: Vec = signature_set.indices().collect(); + assert_eq!(indices, vec![0, 2]); - let res = verify_multisig(&config, &signature, &sighash.0); + // Verify the signatures + let res = verify_threshold_signatures(&config, &signature_set, &sighash.0); assert!(res.is_ok()); } @@ -237,18 +242,21 @@ mod tests { let mut arb = ArbitraryGenerator::new(); let seqno = 1; let threshold = 2; + let secp = Secp256k1::new(); // Generate test private keys + let privkeys: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let pubkeys: Vec = privkeys + .iter() + .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) + .collect(); + let config = ThresholdConfig::try_new(pubkeys, threshold).unwrap(); + // Create signer indices (signers 0 and 2) - let privkeys: Vec = - (0..3).map(|_| SecretKey::new(&mut OsRng).into()).collect(); - let mut signer_indices = BitVec::::new(); - signer_indices.resize(3, false); - signer_indices.set(0, true); - signer_indices.set(2, true); + let signer_indices = [0u8, 2u8]; let action: MultisigAction = arb.generate(); - let tx = create_test_admin_tx(&privkeys, signer_indices, &action, seqno); + let tx = create_test_admin_tx(&privkeys, &signer_indices, &action, seqno); let tag_data_ref = ParseConfig::new(*TEST_MAGIC_BYTES) .try_parse_tx(&tx) .unwrap(); @@ -257,13 +265,8 @@ mod tests { let (p_action, sig) = parse_tx(&tx_input).unwrap(); assert_eq!(action, p_action); - let pubkeys = privkeys - .iter() - .map(|sk| sk.x_only_public_key(SECP256K1).0.into()) - .collect::>(); - let config = SchnorrMultisigConfig::try_new(pubkeys, threshold).unwrap(); - - let res = verify_multisig(&config, &sig, &action.compute_sighash(seqno).0); + // Verify the signatures + let res = verify_threshold_signatures(&config, &sig, &action.compute_sighash(seqno).0); assert!(res.is_ok()); } } diff --git a/crates/crypto/Cargo.toml b/crates/crypto/Cargo.toml index d4bea6db2e..08b5044860 100644 --- a/crates/crypto/Cargo.toml +++ b/crates/crypto/Cargo.toml @@ -10,11 +10,10 @@ workspace = true strata-identifiers.workspace = true arbitrary.workspace = true -bitvec.workspace = true borsh.workspace = true musig2.workspace = true rand = { workspace = true, optional = true } -secp256k1 = { workspace = true, features = ["global-context"] } +secp256k1 = { workspace = true, features = ["global-context", "recovery"] } thiserror.workspace = true [target.'cfg(target_os = "zkvm")'.dependencies] @@ -27,7 +26,6 @@ serde = { workspace = true, optional = true } [dev-dependencies] bitcoin.workspace = true rand.workspace = true -strata-test-utils.workspace = true [features] default = [] diff --git a/crates/crypto/src/lib.rs b/crates/crypto/src/lib.rs index 1a283c5daf..a0362fd6ac 100644 --- a/crates/crypto/src/lib.rs +++ b/crates/crypto/src/lib.rs @@ -1,7 +1,7 @@ //! Cryptographic primitives. -pub mod multisig; pub mod schnorr; +pub mod threshold_signing; #[cfg(feature = "test-utils")] pub mod test_utils; diff --git a/crates/crypto/src/multisig/config.rs b/crates/crypto/src/multisig/config.rs deleted file mode 100644 index a8acb75063..0000000000 --- a/crates/crypto/src/multisig/config.rs +++ /dev/null @@ -1,431 +0,0 @@ -use std::{collections::HashSet, marker::PhantomData}; - -use arbitrary::Arbitrary; -use borsh::{BorshDeserialize, BorshSerialize}; - -use crate::multisig::{errors::MultisigError, traits::CryptoScheme}; - -/// Configuration for a multisignature authority: -/// who can sign (`keys`) and how many of them must sign (`threshold`). -#[derive(Debug, Clone, Eq, PartialEq, BorshSerialize, BorshDeserialize)] -pub struct MultisigConfig { - /// The public keys of all grant-holders authorized to sign. - pub keys: Vec, - /// The minimum number of keys that must sign to approve an action. - pub threshold: u8, - /// Phantom data to carry the crypto scheme type. - #[borsh(skip)] - _phantom: PhantomData, -} - -/// Represents a change to the multisig configuration: -/// * removes specified members from `remove_members` -/// * adds the specified `add_members` -/// * updates the threshold. -#[derive(Debug, Clone, Eq, PartialEq)] -pub struct MultisigConfigUpdate { - add_members: Vec, - remove_members: Vec, - new_threshold: u8, - /// Phantom data to carry the crypto scheme type. - _phantom: PhantomData, -} - -impl MultisigConfig { - /// Create a new multisig configuration. - /// - /// # Errors - /// - /// Returns `MultisigError` if: - /// - `DuplicateAddMember`: The keys list contains duplicate members - /// - `ZeroThreshold`: The threshold is zero - /// - `InvalidThreshold`: The threshold exceeds the total number of keys - pub fn try_new(keys: Vec, threshold: u8) -> Result { - let mut config = MultisigConfig { - keys: vec![], - threshold: 0, - _phantom: PhantomData, - }; - let update = MultisigConfigUpdate::new(keys, vec![], threshold); - config.apply_update(&update)?; - - Ok(config) - } - - pub fn keys(&self) -> &[S::PubKey] { - &self.keys - } - - pub fn threshold(&self) -> u8 { - self.threshold - } -} - -impl<'a, S: CryptoScheme> Arbitrary<'a> for MultisigConfig -where - S::PubKey: Arbitrary<'a>, -{ - fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { - // Generate at least 2 keys, up to a reasonable maximum (e.g., 20) - let keys_count = u.int_in_range(2..=20)?; - let mut keys = Vec::with_capacity(keys_count); - - for _ in 0..keys_count { - keys.push(S::PubKey::arbitrary(u)?); - } - - // Generate threshold between 1 and the number of keys - let threshold = u.int_in_range(1..=keys_count as u8)?; - - Ok(Self { - keys, - threshold, - _phantom: PhantomData, - }) - } -} - -impl MultisigConfigUpdate { - /// Creates a new multisig configuration update. - /// - /// # Arguments - /// - /// * `add_members` - New public keys to add to the configuration - /// * `remove_members` - Public keys to remove from the configuration - /// * `new_threshold` - New threshold value - pub fn new( - add_members: Vec, - remove_members: Vec, - new_threshold: u8, - ) -> Self { - Self { - add_members, - remove_members, - new_threshold, - _phantom: PhantomData, - } - } - - /// Returns the public keys to remove from the configuration. - pub fn remove_members(&self) -> &[S::PubKey] { - &self.remove_members - } - - /// Returns the new members to add. - pub fn add_members(&self) -> &[S::PubKey] { - &self.add_members - } - - /// Returns the new threshold. - pub fn new_threshold(&self) -> u8 { - self.new_threshold - } -} - -impl BorshSerialize for MultisigConfigUpdate -where - S::PubKey: BorshSerialize, -{ - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { - self.add_members.serialize(writer)?; - self.remove_members.serialize(writer)?; - self.new_threshold.serialize(writer) - } -} - -impl BorshDeserialize for MultisigConfigUpdate -where - S::PubKey: BorshDeserialize, -{ - fn deserialize_reader(reader: &mut R) -> std::io::Result { - let add_members = Vec::::deserialize_reader(reader)?; - let remove_members = Vec::::deserialize_reader(reader)?; - let new_threshold = u8::deserialize_reader(reader)?; - - Ok(Self { - add_members, - remove_members, - new_threshold, - _phantom: PhantomData, - }) - } -} - -impl<'a, S: CryptoScheme> Arbitrary<'a> for MultisigConfigUpdate -where - S::PubKey: Arbitrary<'a>, -{ - fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { - let add_members = Vec::::arbitrary(u)?; - let remove_members = Vec::::arbitrary(u)?; - let new_threshold = u8::arbitrary(u)?; - - Ok(Self { - add_members, - remove_members, - new_threshold, - _phantom: PhantomData, - }) - } -} - -impl MultisigConfig { - /// Validates that an update can be applied to this configuration. - /// Ensures no duplicate members in the add list, new members don't already exist in the - /// current configuration, members to remove exist, and the new threshold is within valid - /// bounds. - /// - /// # Errors - /// - /// Returns `MultisigError` if: - /// - `DuplicateAddMember`: The add members list contains duplicate members - /// - `DuplicateRemoveMember`: The remove members list contains duplicate members - /// - `MemberAlreadyExists`: A new member already exists in the current configuration - /// - `MemberNotFound`: A member to remove doesn't exist in the current configuration - /// - `ZeroThreshold`: New threshold is zero - /// - `InvalidThreshold`: New threshold exceeds the total number of keys after update - pub fn validate_update(&self, update: &MultisigConfigUpdate) -> Result<(), MultisigError> { - let members_to_add: HashSet<::PubKey> = - update.add_members().iter().cloned().collect(); - let members_to_remove: HashSet<::PubKey> = - update.remove_members().iter().cloned().collect(); - - // Ensure no duplicate members in the add list - if members_to_add.len() != update.add_members().len() { - return Err(MultisigError::DuplicateAddMember); - } - - // Ensure no duplicate members in the remove list - if members_to_remove.len() != update.remove_members().len() { - return Err(MultisigError::DuplicateRemoveMember); - } - - // Ensure new members don't already exist in current configuration - if members_to_add.iter().any(|m| self.keys.contains(m)) { - return Err(MultisigError::MemberAlreadyExists); - } - - // Ensure new threshold is not zero - if update.new_threshold() == 0 { - return Err(MultisigError::ZeroThreshold); - } - - // Ensure all members to remove exist in current configuration - for member_to_remove in update.remove_members() { - if !self.keys.contains(member_to_remove) { - return Err(MultisigError::MemberNotFound); - } - } - - // Ensure new threshold doesn't exceed updated member count - let updated_size = - self.keys.len() + update.add_members().len() - update.remove_members().len(); - - if (update.new_threshold() as usize) > updated_size { - return Err(MultisigError::InvalidThreshold { - threshold: update.new_threshold(), - total_keys: updated_size, - }); - } - - Ok(()) - } - - /// Applies an update to this configuration by removing old members, adding new members, and - /// updating the threshold. - /// - /// This method handles member removal by explicitly matching public keys to remove, - /// ensuring correctness even when there are concurrent configuration updates. - pub fn apply_update(&mut self, update: &MultisigConfigUpdate) -> Result<(), MultisigError> { - self.validate_update(update)?; - - // Remove members by explicitly matching public keys - self.keys - .retain(|key| !update.remove_members().contains(key)); - - // Add new members - self.keys.extend_from_slice(update.add_members()); - - // Update threshold - self.threshold = update.new_threshold(); - - Ok(()) - } -} - -#[cfg(test)] -mod tests { - use strata_identifiers::Buf32; - use strata_test_utils::ArbitraryGenerator; - - use super::*; - use crate::multisig::{errors::MultisigError, schemes::SchnorrScheme}; - - type TestMultisigConfig = MultisigConfig; - type TestMultisigConfigUpdate = MultisigConfigUpdate; - - fn make_key(id: u8) -> Buf32 { - Buf32::new([id; 32]) - } - - #[test] - fn test_borsh_serde() { - let mut arb = ArbitraryGenerator::new(); - let config: TestMultisigConfig = arb.generate(); - - let borsh_serialized_config = borsh::to_vec(&config).unwrap(); - let borsh_deserialized_config: TestMultisigConfig = - borsh::from_slice(&borsh_serialized_config).unwrap(); - - assert_eq!(config, borsh_deserialized_config); - } - - #[test] - fn test_new_multisig_config() { - let k1 = make_key(1); - let k2 = make_key(2); - - // Try creating config with empty keys - let err = TestMultisigConfig::try_new(vec![], 2).unwrap_err(); - assert_eq!( - err, - MultisigError::InvalidThreshold { - threshold: 2, - total_keys: 0 - } - ); - - // Try creating config with 0 threshold - let err = TestMultisigConfig::try_new(vec![], 0).unwrap_err(); - assert_eq!(err, MultisigError::ZeroThreshold); - - // Try creating config with higher threshold - let err = TestMultisigConfig::try_new(vec![k1, k2], 3).unwrap_err(); - assert_eq!( - err, - MultisigError::InvalidThreshold { - threshold: 3, - total_keys: 2 - } - ); - - // Test successful config creation - let config = TestMultisigConfig::try_new(vec![k1, k2], 1).unwrap(); - assert_eq!(config.keys(), &[k1, k2]); - assert_eq!(config.threshold(), 1); - } - - #[test] - fn test_add_new_members() { - let k1 = make_key(1); - let k2 = make_key(2); - - // Initial config: keys = [k1, k2], threshold = 2 - let mut base = TestMultisigConfig::try_new(vec![k1, k2], 2).unwrap(); - - // Try to set 0 threshold - let update = TestMultisigConfigUpdate::new(vec![], vec![], 0); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::ZeroThreshold); - - // Try to add k2 again → should error MemberAlreadyExists - let update = TestMultisigConfigUpdate::new(vec![k2], vec![], 2); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::MemberAlreadyExists); - - // Try to add k3 twice - let k3 = make_key(3); - let update = TestMultisigConfigUpdate::new(vec![k3, k3], vec![], 2); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::DuplicateAddMember); - - // Add k3 - let update = TestMultisigConfigUpdate::new(vec![k3], vec![], 2); - base.apply_update(&update).unwrap(); - assert_eq!(base.keys(), &[k1, k2, k3]); - } - - #[test] - fn test_remove_old_members() { - let k1 = make_key(1); - let k2 = make_key(2); - let k3 = make_key(3); - let k4 = make_key(4); - let k5 = make_key(5); - let k6 = make_key(6); - - // Initial config: keys = [k1, k2, k3, k4, k5, k6], threshold = 2 - let mut base = TestMultisigConfig::try_new(vec![k1, k2, k3, k4, k5, k6], 2).unwrap(); - - // Try remove k6 twice - let update = TestMultisigConfigUpdate::new(vec![], vec![k1, k1], 2); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::DuplicateRemoveMember); - - // Remove k6 and k1 - let update = TestMultisigConfigUpdate::new(vec![], vec![k6, k1], 2); - base.apply_update(&update).unwrap(); - assert_eq!(base.keys(), &[k2, k3, k4, k5]); - - // Current keys: [k2, k3, k4, k5] - // Remove k3 and k4 - let update = TestMultisigConfigUpdate::new(vec![], vec![k3, k4], 2); - base.apply_update(&update).unwrap(); - assert_eq!(base.keys(), &[k2, k5]); - - // Try to remove k3 again (non-existent member) - let update = TestMultisigConfigUpdate::new(vec![], vec![k3], 2); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::MemberNotFound); - } - - #[test] - fn test_threshold() { - let k1 = make_key(1); - let k2 = make_key(2); - let k3 = make_key(3); - - // Initial config: keys = [k1, k2, k3], threshold = 2 - let mut base = TestMultisigConfig::try_new(vec![k1, k2, k3], 2).unwrap(); - - // Try setting threshold to 0 - let update = TestMultisigConfigUpdate::new(vec![], vec![], 0); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!(err, MultisigError::ZeroThreshold); - - // Try removing two members - let update = TestMultisigConfigUpdate::new(vec![], vec![k1, k3], 2); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!( - err, - MultisigError::InvalidThreshold { - threshold: 2, - total_keys: 1 - } - ); - - // Removing first and last member with threshold 1 - let update = TestMultisigConfigUpdate::new(vec![], vec![k1, k3], 1); - base.apply_update(&update).unwrap(); - assert_eq!(base.keys(), &[k2]); - assert_eq!(base.threshold(), 1); - - // Try removing the only member without adding any new member - let update = TestMultisigConfigUpdate::new(vec![], vec![k2], 1); - let err = base.apply_update(&update).unwrap_err(); - assert_eq!( - err, - MultisigError::InvalidThreshold { - threshold: 1, - total_keys: 0 - } - ); - - let k4 = make_key(4); - let k5 = make_key(5); - let update = TestMultisigConfigUpdate::new(vec![k4, k5], vec![k2], 2); - base.apply_update(&update).unwrap(); - assert_eq!(base.keys(), &[k4, k5]); - assert_eq!(base.threshold(), 2); - } -} diff --git a/crates/crypto/src/multisig/errors.rs b/crates/crypto/src/multisig/errors.rs deleted file mode 100644 index 5962a1ec30..0000000000 --- a/crates/crypto/src/multisig/errors.rs +++ /dev/null @@ -1,63 +0,0 @@ -use thiserror::Error; - -/// Single error type for all multisig operations across all cryptographic schemes. -#[derive(Debug, Clone, Error, PartialEq, Eq)] -pub enum MultisigError { - /// Insufficient keys selected for aggregation. - #[error("insufficient keys selected: provided {provided}, required at least {required}")] - InsufficientKeys { - /// Number of keys provided. - provided: usize, - /// Number of keys required. - required: usize, - }, - - /// Invalid public key at a specific index. - #[error("invalid public key at index {index}: {reason}")] - InvalidPubKey { - /// The index of the invalid key. - index: usize, - /// The reason why the key is invalid. - reason: String, - }, - - /// The provided threshold is invalid. - #[error("invalid threshold {threshold}: must not exceed {total_keys}")] - InvalidThreshold { - /// The threshold value provided. - threshold: u8, - /// The total keys in the multisig. - total_keys: usize, - }, - - /// The provided threshold is invalid. - #[error("zero threshold")] - ZeroThreshold, - - /// The aggregated signature is invalid. - #[error("invalid signature")] - InvalidSignature, - - /// Key aggregation context creation failed. - #[error("key aggregation context creation failed: {reason}")] - AggregationContextFailed { - /// The reason why context creation failed. - reason: String, - }, - - /// A new member to be added already exists in the multisig configuration. - #[error("cannot add member: already exists in multisig configuration")] - MemberAlreadyExists, - - /// Attempted to add duplicate members in a single operation. - #[error("duplicate members in add request: cannot add the same member multiple times")] - DuplicateAddMember, - - /// Attempted to remove duplicate members in a single operation. - #[error("duplicate members in remove request: cannot remove the same member multiple times")] - DuplicateRemoveMember, - - /// A member to be removed does not exist in the multisig configuration. - #[error("cannot remove member: not found in multisig configuration")] - MemberNotFound, -} diff --git a/crates/crypto/src/multisig/mod.rs b/crates/crypto/src/multisig/mod.rs deleted file mode 100644 index f4acef36a2..0000000000 --- a/crates/crypto/src/multisig/mod.rs +++ /dev/null @@ -1,61 +0,0 @@ -pub mod config; -pub mod errors; -pub mod schemes; -pub mod signature; -pub mod traits; - -// Re-export the Schnorr scheme and Schnorr aggregation -pub use schemes::{aggregate_schnorr_keys, SchnorrScheme}; - -// Type aliases for Schnorr-based multisig -pub type SchnorrMultisigConfig = config::MultisigConfig; -pub type SchnorrMultisigConfigUpdate = config::MultisigConfigUpdate; -pub type SchnorrMultisigSignature = signature::AggregatedSignature; - -// Re-export the single error type -pub use errors::MultisigError; - -use crate::multisig::traits::CryptoScheme; - -/// Generic multisig verification function that takes a multisig configuration, aggregated -/// signature, and message hash, then performs the full verification process using the provided -/// cryptographic scheme. -/// -/// # Arguments -/// * `config` - The multisig configuration containing keys and threshold -/// * `signature` - The aggregated signature containing signer indices and aggregated signature -/// * `message_hash` - The message hash that was signed -/// -/// # Returns -/// Returns `Ok(())` if verification succeeds, or an error if: -/// - Insufficient keys (threshold is not achieved) -/// - Key aggregation fails -/// - Signature verification fails -pub fn verify_multisig( - config: &config::MultisigConfig, - signature: &signature::AggregatedSignature, - message_hash: &[u8; 32], -) -> Result<(), MultisigError> { - // Check threshold - let selected_count = signature.signer_indices().count_ones(); - if selected_count < config.threshold() as usize { - return Err(MultisigError::InsufficientKeys { - provided: selected_count, - required: config.threshold() as usize, - }); - } - - // Aggregate selected keys - let selected_keys = signature - .signer_indices() - .iter_ones() - .map(|index| &config.keys()[index]); - let aggregated_key = S::aggregate(selected_keys)?; - - // Verify signature - if !S::verify(&aggregated_key, message_hash, signature.signature()) { - return Err(MultisigError::InvalidSignature); - } - - Ok(()) -} diff --git a/crates/crypto/src/multisig/schemes/mod.rs b/crates/crypto/src/multisig/schemes/mod.rs deleted file mode 100644 index 931d0ba36d..0000000000 --- a/crates/crypto/src/multisig/schemes/mod.rs +++ /dev/null @@ -1,3 +0,0 @@ -pub mod schnorr; - -pub use schnorr::{aggregate_schnorr_keys, SchnorrScheme}; diff --git a/crates/crypto/src/multisig/schemes/schnorr.rs b/crates/crypto/src/multisig/schemes/schnorr.rs deleted file mode 100644 index 14a92986d2..0000000000 --- a/crates/crypto/src/multisig/schemes/schnorr.rs +++ /dev/null @@ -1,80 +0,0 @@ -use musig2::KeyAggContext; -use secp256k1::{Parity, PublicKey, XOnlyPublicKey}; -use strata_identifiers::{Buf32, Buf64}; - -use crate::{ - multisig::{errors::MultisigError, traits::CryptoScheme}, - schnorr::verify_schnorr_sig, -}; - -/// Schnorr signature scheme using MuSig2 key aggregation. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct SchnorrScheme; - -impl CryptoScheme for SchnorrScheme { - type PubKey = Buf32; // FIXME:? - type Signature = Buf64; - type AggregatedKey = Buf32; - - /// Aggregates public keys using MuSig2 key aggregation. - fn aggregate<'k>( - keys: impl Iterator, - ) -> Result - where - Self::PubKey: 'k, - { - let a = aggregate_schnorr_keys(keys)?; - Ok(Buf32::from(a.serialize())) - } - - /// Verifies a Schnorr signature against a message hash using a(n aggregated) public key. - fn verify( - key: &Self::AggregatedKey, - message_hash: &[u8; 32], - signature: &Self::Signature, - ) -> bool { - // Use the existing verification function from strata_primitives - verify_schnorr_sig(signature, &Buf32::from(*message_hash), key) - } -} - -/// Aggregates a collection of Schnorr public keys using MuSig2 key aggregation. -/// -/// # Arguments -/// * `keys` - An iterator over 32-byte public keys to aggregate -/// -/// # Returns -/// Returns the aggregated public key on success, or an error if: -/// - Any key is not a valid x-only public key -/// - MuSig2 key aggregation context creation fails -/// -/// # Errors -/// * `MultisigError::InvalidPubKey` - If a key is not a valid x-only public key -/// * `MultisigError::AggregationContextFailed` - If MuSig2 context creation fails -pub fn aggregate_schnorr_keys<'k>( - keys: impl Iterator, -) -> Result -where -{ - let public_keys = keys - .enumerate() - .map(|(index, op)| { - XOnlyPublicKey::from_slice(op.as_ref()) - .map_err(|e| MultisigError::InvalidPubKey { - index, - reason: e.to_string(), - }) - .map(|x_only| PublicKey::from_x_only_public_key(x_only, Parity::Even)) - }) - .collect::, MultisigError>>()?; - - let agg_pubkey = KeyAggContext::new(public_keys) - .map_err(|e| MultisigError::AggregationContextFailed { - reason: e.to_string(), - })? - .aggregated_pubkey::() - .x_only_public_key() - .0; - - Ok(agg_pubkey) -} diff --git a/crates/crypto/src/multisig/signature.rs b/crates/crypto/src/multisig/signature.rs deleted file mode 100644 index 7df4ae92f5..0000000000 --- a/crates/crypto/src/multisig/signature.rs +++ /dev/null @@ -1,54 +0,0 @@ -use std::marker::PhantomData; - -use bitvec::{slice::BitSlice, vec::BitVec}; - -use crate::multisig::traits::CryptoScheme; - -/// An aggregated signature over a subset of signers in a MultisigConfig, -/// identified by their positions in the config's key list. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct AggregatedSignature { - indices: BitVec, - signature: S::Signature, - /// Phantom data to carry the crypto scheme type. - _phantom: PhantomData, -} - -impl AggregatedSignature { - /// Create a new `AggregatedSignature` with given signer indices and aggregated signature. - pub fn new(indices: BitVec, signature: S::Signature) -> Self { - Self { - indices, - signature, - _phantom: PhantomData, - } - } - - /// Borrow the aggregated signature. - pub fn signature(&self) -> &S::Signature { - &self.signature - } - - /// Borrow the signer indices slice. - pub fn signer_indices(&self) -> &BitSlice { - &self.indices - } - - /// Consume and return the inner `(indices, signature)`. - pub fn into_inner(self) -> (BitVec, S::Signature) { - (self.indices, self.signature) - } -} - -impl Default for AggregatedSignature -where - S::Signature: Default, -{ - fn default() -> Self { - Self { - indices: BitVec::::default(), - signature: S::Signature::default(), - _phantom: PhantomData, - } - } -} diff --git a/crates/crypto/src/multisig/traits.rs b/crates/crypto/src/multisig/traits.rs deleted file mode 100644 index 04ec55edf7..0000000000 --- a/crates/crypto/src/multisig/traits.rs +++ /dev/null @@ -1,67 +0,0 @@ -use std::{fmt::Debug, hash::Hash}; - -use arbitrary::Arbitrary; -use borsh::{BorshDeserialize, BorshSerialize}; - -use crate::multisig::errors::MultisigError; - -/// A generic trait representing a cryptographic scheme for multisignature operations. -/// -/// This trait focuses on the core cryptographic operations: key aggregation and signature -/// verification. Higher-level multisig workflow is handled by generic functions that use this -/// trait. -pub trait CryptoScheme: Clone + Debug + Send + Sync + 'static { - /// The type used to represent a public key in this scheme. - type PubKey: Clone - + Debug - + PartialEq - + Eq - + Hash - + Send - + Sync - + BorshSerialize - + BorshDeserialize - + for<'a> Arbitrary<'a>; - - /// The type used to represent a signature in this scheme. - type Signature: Clone - + Debug - + PartialEq - + Eq - + Send - + Sync - + BorshSerialize - + BorshDeserialize - + Default; - - /// The type used to represent an aggregated public key. - type AggregatedKey: Clone + Debug + PartialEq + Eq + Send + Sync; - - /// Aggregates multiple public keys into a single aggregated key. - /// - /// # Arguments - /// * `keys` - An iterator over public keys to aggregate - /// - /// # Returns - /// Returns the aggregated public key on success, or an error if aggregation fails. - fn aggregate<'k>( - keys: impl Iterator, - ) -> Result - where - Self::PubKey: 'k; - - /// Verifies a signature against a message hash using an aggregated public key. - /// - /// # Arguments - /// * `key` - The aggregated public key to verify against - /// * `message_hash` - The message hash that was signed - /// * `signature` - The signature to verify - /// - /// # Returns - /// Returns `true` if the signature is valid, `false` otherwise. - fn verify( - key: &Self::AggregatedKey, - message_hash: &[u8; 32], - signature: &Self::Signature, - ) -> bool; -} diff --git a/crates/crypto/src/test_utils/schnorr.rs b/crates/crypto/src/test_utils/schnorr.rs index 994c112f50..2c93789085 100644 --- a/crates/crypto/src/test_utils/schnorr.rs +++ b/crates/crypto/src/test_utils/schnorr.rs @@ -3,7 +3,7 @@ use rand::{rngs::OsRng, RngCore}; use secp256k1::{PublicKey, Secp256k1, XOnlyPublicKey}; use strata_identifiers::Buf32; -use crate::{multisig::aggregate_schnorr_keys, schnorr::EvenSecretKey}; +use crate::{schnorr::EvenSecretKey, threshold_signing::musig2::aggregate_schnorr_keys}; /// Creates a MuSig2 signature from multiple operators. /// @@ -128,10 +128,11 @@ mod tests { use bitcoin::{ hashes::Hash, key::TapTweak, - secp256k1::{Secp256k1, SecretKey}, + secp256k1::Secp256k1, TapNodeHash, }; use rand::rngs::OsRng; + use secp256k1::SecretKey; use super::*; @@ -149,7 +150,11 @@ mod tests { // Generate test private keys for 3 operators let operator_privkeys: Vec = (0..3) - .map(|_| EvenSecretKey::from(SecretKey::new(&mut OsRng))) + .map(|_| { + let mut sk_bytes = [0u8; 32]; + OsRng.fill_bytes(&mut sk_bytes); + EvenSecretKey::from(SecretKey::from_slice(&sk_bytes).unwrap()) + }) .collect(); // Test without tweak diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/config.rs b/crates/crypto/src/threshold_signing/indexed_signatures/config.rs new file mode 100644 index 0000000000..cf506b41e9 --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/config.rs @@ -0,0 +1,297 @@ +//! Configuration types for threshold signing. + +use std::collections::HashSet; + +use arbitrary::Arbitrary; +use borsh::{BorshDeserialize, BorshSerialize}; + +use super::{CompressedPublicKey, ThresholdSigningError}; + +/// Configuration for a threshold signature authority. +/// +/// Defines who can sign (`keys`) and how many must sign (`threshold`). +#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] +pub struct ThresholdConfig { + /// Public keys of all authorized signers. + keys: Vec, + /// Minimum number of signatures required. + threshold: u8, +} + +impl ThresholdConfig { + /// Create a new threshold configuration. + /// + /// # Errors + /// + /// Returns `ThresholdSigningError` if: + /// - `DuplicateAddMember`: The keys list contains duplicate members + /// - `ZeroThreshold`: The threshold is zero + /// - `InvalidThreshold`: The threshold exceeds the total number of keys + pub fn try_new( + keys: Vec, + threshold: u8, + ) -> Result { + let mut config = ThresholdConfig { + keys: vec![], + threshold: 0, + }; + let update = ThresholdConfigUpdate::new(keys, vec![], threshold); + config.apply_update(&update)?; + Ok(config) + } + + /// Get the public keys. + pub fn keys(&self) -> &[CompressedPublicKey] { + &self.keys + } + + /// Get the threshold. + pub fn threshold(&self) -> u8 { + self.threshold + } + + /// Get the number of authorized signers. + pub fn len(&self) -> usize { + self.keys.len() + } + + /// Check if there are no authorized signers. + pub fn is_empty(&self) -> bool { + self.keys.is_empty() + } + + /// Validates that an update can be applied to this configuration. + pub fn validate_update( + &self, + update: &ThresholdConfigUpdate, + ) -> Result<(), ThresholdSigningError> { + let members_to_add: HashSet = + update.add_members().iter().cloned().collect(); + let members_to_remove: HashSet = + update.remove_members().iter().cloned().collect(); + + // Ensure no duplicate members in the add list + if members_to_add.len() != update.add_members().len() { + return Err(ThresholdSigningError::DuplicateAddMember); + } + + // Ensure no duplicate members in the remove list + if members_to_remove.len() != update.remove_members().len() { + return Err(ThresholdSigningError::DuplicateRemoveMember); + } + + // Ensure new members don't already exist in current configuration + if members_to_add.iter().any(|m| self.keys.contains(m)) { + return Err(ThresholdSigningError::MemberAlreadyExists); + } + + // Ensure new threshold is not zero + if update.new_threshold() == 0 { + return Err(ThresholdSigningError::ZeroThreshold); + } + + // Ensure all members to remove exist in current configuration + for member_to_remove in update.remove_members() { + if !self.keys.contains(member_to_remove) { + return Err(ThresholdSigningError::MemberNotFound); + } + } + + // Ensure new threshold doesn't exceed updated member count + let updated_size = + self.keys.len() + update.add_members().len() - update.remove_members().len(); + + if (update.new_threshold() as usize) > updated_size { + return Err(ThresholdSigningError::InvalidThreshold { + threshold: update.new_threshold(), + total_keys: updated_size, + }); + } + + Ok(()) + } + + /// Applies an update to this configuration. + pub fn apply_update( + &mut self, + update: &ThresholdConfigUpdate, + ) -> Result<(), ThresholdSigningError> { + self.validate_update(update)?; + + // Remove members by matching public keys + self.keys + .retain(|key| !update.remove_members().contains(key)); + + // Add new members + self.keys.extend_from_slice(update.add_members()); + + // Update threshold + self.threshold = update.new_threshold(); + + Ok(()) + } +} + +impl std::hash::Hash for CompressedPublicKey { + fn hash(&self, state: &mut H) { + self.serialize().hash(state); + } +} + +/// Represents a change to the threshold configuration. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ThresholdConfigUpdate { + add_members: Vec, + remove_members: Vec, + new_threshold: u8, +} + +impl ThresholdConfigUpdate { + /// Creates a new threshold configuration update. + pub fn new( + add_members: Vec, + remove_members: Vec, + new_threshold: u8, + ) -> Self { + Self { + add_members, + remove_members, + new_threshold, + } + } + + /// Returns the public keys to add. + pub fn add_members(&self) -> &[CompressedPublicKey] { + &self.add_members + } + + /// Returns the public keys to remove. + pub fn remove_members(&self) -> &[CompressedPublicKey] { + &self.remove_members + } + + /// Returns the new threshold. + pub fn new_threshold(&self) -> u8 { + self.new_threshold + } + + /// Consume and return the inner components. + pub fn into_inner(self) -> (Vec, Vec, u8) { + (self.add_members, self.remove_members, self.new_threshold) + } +} + +impl BorshSerialize for ThresholdConfigUpdate { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + self.add_members.serialize(writer)?; + self.remove_members.serialize(writer)?; + self.new_threshold.serialize(writer) + } +} + +impl BorshDeserialize for ThresholdConfigUpdate { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let add_members = Vec::::deserialize_reader(reader)?; + let remove_members = Vec::::deserialize_reader(reader)?; + let new_threshold = u8::deserialize_reader(reader)?; + + Ok(Self { + add_members, + remove_members, + new_threshold, + }) + } +} + +impl<'a> Arbitrary<'a> for ThresholdConfigUpdate { + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + let add_members = Vec::::arbitrary(u)?; + let remove_members = Vec::::arbitrary(u)?; + // Generate a threshold between 1 and max(1, len(add_members)) + let max_threshold = add_members.len().max(1); + let new_threshold = u.int_in_range(1..=(max_threshold as u8))?; + Ok(Self { + add_members, + remove_members, + new_threshold, + }) + } +} + +#[cfg(test)] +mod tests { + use secp256k1::{Secp256k1, SecretKey}; + + use super::*; + + fn make_key(seed: u8) -> CompressedPublicKey { + let secp = Secp256k1::new(); + let mut sk_bytes = [0u8; 32]; + sk_bytes[31] = seed.max(1); // Ensure non-zero + let sk = SecretKey::from_slice(&sk_bytes).unwrap(); + CompressedPublicKey::from(secp256k1::PublicKey::from_secret_key(&secp, &sk)) + } + + #[test] + fn test_config_creation() { + let keys = vec![make_key(1), make_key(2), make_key(3)]; + let config = ThresholdConfig::try_new(keys.clone(), 2).unwrap(); + + assert_eq!(config.keys().len(), 3); + assert_eq!(config.threshold(), 2); + } + + #[test] + fn test_config_zero_threshold() { + let keys = vec![make_key(1)]; + let result = ThresholdConfig::try_new(keys, 0); + assert!(matches!(result, Err(ThresholdSigningError::ZeroThreshold))); + } + + #[test] + fn test_config_threshold_exceeds_keys() { + let keys = vec![make_key(1), make_key(2)]; + let result = ThresholdConfig::try_new(keys, 3); + assert!(matches!( + result, + Err(ThresholdSigningError::InvalidThreshold { .. }) + )); + } + + #[test] + fn test_config_update_add_member() { + let keys = vec![make_key(1), make_key(2)]; + let mut config = ThresholdConfig::try_new(keys, 2).unwrap(); + + let update = ThresholdConfigUpdate::new(vec![make_key(3)], vec![], 2); + config.apply_update(&update).unwrap(); + + assert_eq!(config.keys().len(), 3); + } + + #[test] + fn test_config_update_remove_member() { + let k1 = make_key(1); + let k2 = make_key(2); + let k3 = make_key(3); + + let mut config = ThresholdConfig::try_new(vec![k1, k2, k3], 2).unwrap(); + + let update = ThresholdConfigUpdate::new(vec![], vec![k2], 2); + config.apply_update(&update).unwrap(); + + assert_eq!(config.keys().len(), 2); + assert!(!config.keys().contains(&k2)); + } + + #[test] + fn test_config_borsh_roundtrip() { + let keys = vec![make_key(1), make_key(2)]; + let config = ThresholdConfig::try_new(keys, 2).unwrap(); + + let encoded = borsh::to_vec(&config).unwrap(); + let decoded: ThresholdConfig = borsh::from_slice(&encoded).unwrap(); + + assert_eq!(config, decoded); + } +} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs b/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs new file mode 100644 index 0000000000..ce32183b51 --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs @@ -0,0 +1,102 @@ +//! Error types for threshold signing operations. + +use std::fmt; + +/// Errors that can occur during threshold signing operations. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ThresholdSigningError { + /// Not enough signatures to meet the threshold. + InsufficientSignatures { + provided: usize, + required: usize, + }, + /// Invalid public key data. + InvalidPublicKey { + index: usize, + reason: String, + }, + /// Invalid threshold value. + InvalidThreshold { + threshold: u8, + total_keys: usize, + }, + /// Threshold cannot be zero. + ZeroThreshold, + /// Signature verification failed. + InvalidSignature { + index: u8, + }, + /// Invalid signature format. + InvalidSignatureFormat, + /// Duplicate signer index in signature set. + DuplicateSignerIndex(u8), + /// Signer index out of bounds. + SignerIndexOutOfBounds { + index: u8, + max: usize, + }, + /// Member already exists in the configuration. + MemberAlreadyExists, + /// Duplicate member in add list. + DuplicateAddMember, + /// Duplicate member in remove list. + DuplicateRemoveMember, + /// Member not found in the configuration. + MemberNotFound, + /// Invalid message hash. + InvalidMessageHash, +} + +impl fmt::Display for ThresholdSigningError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::InsufficientSignatures { provided, required } => { + write!( + f, + "insufficient signatures: provided {}, required {}", + provided, required + ) + } + Self::InvalidPublicKey { index, reason } => { + write!(f, "invalid public key at index {}: {}", index, reason) + } + Self::InvalidThreshold { + threshold, + total_keys, + } => { + write!( + f, + "invalid threshold: {} exceeds total keys {}", + threshold, total_keys + ) + } + Self::ZeroThreshold => write!(f, "threshold cannot be zero"), + Self::InvalidSignature { index } => { + write!(f, "invalid signature at index {}", index) + } + Self::InvalidSignatureFormat => write!(f, "invalid signature format"), + Self::DuplicateSignerIndex(index) => { + write!(f, "duplicate signer index: {}", index) + } + Self::SignerIndexOutOfBounds { index, max } => { + write!(f, "signer index {} out of bounds (max: {})", index, max) + } + Self::MemberAlreadyExists => write!(f, "member already exists"), + Self::DuplicateAddMember => write!(f, "duplicate member in add list"), + Self::DuplicateRemoveMember => write!(f, "duplicate member in remove list"), + Self::MemberNotFound => write!(f, "member not found"), + Self::InvalidMessageHash => write!(f, "invalid message hash"), + } + } +} + +impl std::error::Error for ThresholdSigningError {} + +impl From for ThresholdSigningError { + fn from(e: secp256k1::Error) -> Self { + Self::InvalidPublicKey { + index: 0, + reason: e.to_string(), + } + } +} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/mod.rs b/crates/crypto/src/threshold_signing/indexed_signatures/mod.rs new file mode 100644 index 0000000000..98b02b8afb --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/mod.rs @@ -0,0 +1,17 @@ +//! Individual ECDSA signature set for threshold signing (M-of-N). +//! +//! This module provides types and functions for verifying a set of individual +//! ECDSA signatures against a threshold configuration. Used by the admin +//! subprotocol for hardware wallet compatibility. + +mod config; +mod errors; +mod pubkey; +mod signature; +mod verification; + +pub use config::{ThresholdConfig, ThresholdConfigUpdate}; +pub use errors::ThresholdSigningError; +pub use pubkey::CompressedPublicKey; +pub use signature::{IndexedSignature, SignatureSet}; +pub use verification::verify_threshold_signatures; diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs b/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs new file mode 100644 index 0000000000..3147bab951 --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs @@ -0,0 +1,173 @@ +//! Compressed ECDSA public key type with Borsh serialization. + +use std::ops::Deref; + +use arbitrary::Arbitrary; +use borsh::{BorshDeserialize, BorshSerialize}; +use secp256k1::{PublicKey, Secp256k1, SecretKey}; +#[cfg(feature = "serde")] +use serde::{Deserialize, Serialize}; + +use super::ThresholdSigningError; + +/// A compressed secp256k1 public key (33 bytes). +/// +/// This is a thin wrapper around `secp256k1::PublicKey` that adds Borsh +/// serialization support. Unlike `EvenPublicKey`, this type does not +/// enforce even parity - it accepts any valid compressed public key. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CompressedPublicKey(PublicKey); + +impl CompressedPublicKey { + /// Create a new `CompressedPublicKey` from a byte slice. + /// + /// The slice must be exactly 33 bytes in compressed format (0x02 or 0x03 prefix). + pub fn from_slice(data: &[u8]) -> Result { + let pk = PublicKey::from_slice(data).map_err(|e| ThresholdSigningError::InvalidPublicKey { + index: 0, + reason: e.to_string(), + })?; + Ok(Self(pk)) + } + + /// Get the inner `secp256k1::PublicKey`. + pub fn as_inner(&self) -> &PublicKey { + &self.0 + } + + /// Serialize to 33-byte compressed format. + pub fn serialize(&self) -> [u8; 33] { + self.0.serialize() + } +} + +impl Deref for CompressedPublicKey { + type Target = PublicKey; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl AsRef for CompressedPublicKey { + fn as_ref(&self) -> &PublicKey { + &self.0 + } +} + +impl From for CompressedPublicKey { + fn from(pk: PublicKey) -> Self { + Self(pk) + } +} + +impl From for PublicKey { + fn from(pk: CompressedPublicKey) -> Self { + pk.0 + } +} + +impl<'a> Arbitrary<'a> for CompressedPublicKey { + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + // Generate 32 bytes for a secret key + let mut sk_bytes = [0u8; 32]; + u.fill_buffer(&mut sk_bytes)?; + // Ensure we have a valid secret key (non-zero) + if sk_bytes.iter().all(|&b| b == 0) { + sk_bytes[31] = 1; + } + let secp = Secp256k1::new(); + let sk = SecretKey::from_slice(&sk_bytes) + .map_err(|_| arbitrary::Error::IncorrectFormat)?; + let pk = PublicKey::from_secret_key(&secp, &sk); + Ok(Self(pk)) + } +} + +impl BorshSerialize for CompressedPublicKey { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + let bytes = self.0.serialize(); + writer.write_all(&bytes) + } +} + +impl BorshDeserialize for CompressedPublicKey { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let mut buf = [0u8; 33]; + reader.read_exact(&mut buf)?; + let pk = PublicKey::from_slice(&buf) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + Ok(Self(pk)) + } +} + +#[cfg(feature = "serde")] +impl Serialize for CompressedPublicKey { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + let compressed = self.0.serialize(); + let hex_string = hex::encode(compressed); + serializer.serialize_str(&hex_string) + } +} + +#[cfg(feature = "serde")] +impl<'de> Deserialize<'de> for CompressedPublicKey { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error as DeError; + + let hex_string: String = Deserialize::deserialize(deserializer)?; + let bytes = hex::decode(&hex_string).map_err(DeError::custom)?; + let pk = PublicKey::from_slice(&bytes).map_err(DeError::custom)?; + Ok(Self(pk)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_compressed_pubkey_roundtrip() { + // Generate a test key + use secp256k1::{Secp256k1, SecretKey}; + let secp = Secp256k1::new(); + let sk = SecretKey::from_slice(&[0x01; 32]).unwrap(); + let pk = PublicKey::from_secret_key(&secp, &sk); + + let compressed = CompressedPublicKey::from(pk); + + // Test serialization roundtrip + let bytes = compressed.serialize(); + let restored = CompressedPublicKey::from_slice(&bytes).unwrap(); + assert_eq!(compressed, restored); + } + + #[test] + fn test_compressed_pubkey_borsh_roundtrip() { + use secp256k1::{Secp256k1, SecretKey}; + let secp = Secp256k1::new(); + let sk = SecretKey::from_slice(&[0x02; 32]).unwrap(); + let pk = PublicKey::from_secret_key(&secp, &sk); + + let compressed = CompressedPublicKey::from(pk); + + // Borsh roundtrip + let encoded = borsh::to_vec(&compressed).unwrap(); + assert_eq!(encoded.len(), 33); + let decoded: CompressedPublicKey = borsh::from_slice(&encoded).unwrap(); + assert_eq!(compressed, decoded); + } + + #[test] + fn test_invalid_pubkey_slice() { + let invalid = [0u8; 33]; + let result = CompressedPublicKey::from_slice(&invalid); + assert!(result.is_err()); + } +} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/signature.rs b/crates/crypto/src/threshold_signing/indexed_signatures/signature.rs new file mode 100644 index 0000000000..46b861fd0b --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/signature.rs @@ -0,0 +1,207 @@ +//! Signature types for threshold signing. + +use borsh::{BorshDeserialize, BorshSerialize}; + +use super::ThresholdSigningError; + +/// An individual ECDSA signature with its signer index. +/// +/// The signature is in recoverable format (65 bytes): `recovery_id || r || s`. +/// This format is used for hardware wallet compatibility (Ledger/Trezor native output). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IndexedSignature { + /// Index of the signer in the ThresholdConfig keys array (0-255). + pub index: u8, + /// 65-byte recoverable ECDSA signature (recovery_id || r || s). + /// Using recoverable format for hardware wallet compatibility (Ledger/Trezor native output). + pub signature: [u8; 65], +} + +impl IndexedSignature { + /// Create a new indexed signature. + pub fn new(index: u8, signature: [u8; 65]) -> Self { + Self { index, signature } + } + + /// Get the recovery ID (first byte of the signature). + pub fn recovery_id(&self) -> u8 { + self.signature[0] + } + + /// Get the r component (bytes 1-32). + pub fn r(&self) -> &[u8; 32] { + self.signature[1..33].try_into().unwrap() + } + + /// Get the s component (bytes 33-64). + pub fn s(&self) -> &[u8; 32] { + self.signature[33..65].try_into().unwrap() + } + + /// Get the compact signature (r || s) without recovery ID. + pub fn compact(&self) -> [u8; 64] { + let mut compact = [0u8; 64]; + compact.copy_from_slice(&self.signature[1..65]); + compact + } +} + +impl BorshSerialize for IndexedSignature { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + self.index.serialize(writer)?; + writer.write_all(&self.signature) + } +} + +impl BorshDeserialize for IndexedSignature { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let index = u8::deserialize_reader(reader)?; + let mut signature = [0u8; 65]; + reader.read_exact(&mut signature)?; + Ok(Self { index, signature }) + } +} + +/// A set of indexed ECDSA signatures for threshold verification. +/// +/// Signatures are sorted by index and must not contain duplicates. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct SignatureSet { + /// Sorted signatures by index, no duplicates. + signatures: Vec, +} + +impl SignatureSet { + /// Create a new signature set from a vector of indexed signatures. + /// + /// The signatures will be sorted by index and checked for duplicates. + pub fn new(mut signatures: Vec) -> Result { + // Sort by index + signatures.sort_by_key(|s| s.index); + + // Check for duplicate indices + for window in signatures.windows(2) { + if window[0].index == window[1].index { + return Err(ThresholdSigningError::DuplicateSignerIndex(window[0].index)); + } + } + + Ok(Self { signatures }) + } + + /// Create an empty signature set. + pub fn empty() -> Self { + Self { + signatures: Vec::new(), + } + } + + /// Get the signatures. + pub fn signatures(&self) -> &[IndexedSignature] { + &self.signatures + } + + /// Get the number of signatures. + pub fn len(&self) -> usize { + self.signatures.len() + } + + /// Check if the signature set is empty. + pub fn is_empty(&self) -> bool { + self.signatures.is_empty() + } + + /// Iterate over signer indices. + pub fn indices(&self) -> impl Iterator + '_ { + self.signatures.iter().map(|s| s.index) + } + + /// Consume and return the inner signatures. + pub fn into_inner(self) -> Vec { + self.signatures + } +} + +impl BorshSerialize for SignatureSet { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + // Write count as u8 (max 255 signatures) + let count = self.signatures.len() as u8; + count.serialize(writer)?; + // Write each signature + for sig in &self.signatures { + sig.serialize(writer)?; + } + Ok(()) + } +} + +impl BorshDeserialize for SignatureSet { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let count = u8::deserialize_reader(reader)?; + let mut signatures = Vec::with_capacity(count as usize); + for _ in 0..count { + signatures.push(IndexedSignature::deserialize_reader(reader)?); + } + SignatureSet::new(signatures) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_sig(index: u8) -> IndexedSignature { + let mut signature = [0u8; 65]; + signature[0] = 27; // recovery id + signature[1] = index; // put index in r for easy identification + IndexedSignature::new(index, signature) + } + + #[test] + fn test_signature_set_creation() { + let sigs = vec![make_sig(2), make_sig(0), make_sig(1)]; + let set = SignatureSet::new(sigs).unwrap(); + + // Should be sorted + assert_eq!(set.signatures()[0].index, 0); + assert_eq!(set.signatures()[1].index, 1); + assert_eq!(set.signatures()[2].index, 2); + } + + #[test] + fn test_signature_set_duplicate_index() { + let sigs = vec![make_sig(1), make_sig(1)]; + let result = SignatureSet::new(sigs); + assert!(matches!( + result, + Err(ThresholdSigningError::DuplicateSignerIndex(1)) + )); + } + + #[test] + fn test_signature_set_borsh_roundtrip() { + let sigs = vec![make_sig(0), make_sig(2), make_sig(5)]; + let set = SignatureSet::new(sigs).unwrap(); + + let encoded = borsh::to_vec(&set).unwrap(); + let decoded: SignatureSet = borsh::from_slice(&encoded).unwrap(); + + assert_eq!(set, decoded); + } + + #[test] + fn test_indexed_signature_components() { + let mut signature = [0u8; 65]; + signature[0] = 27; // recovery id + signature[1..33].copy_from_slice(&[0xAA; 32]); // r + signature[33..65].copy_from_slice(&[0xBB; 32]); // s + + let sig = IndexedSignature::new(5, signature); + + assert_eq!(sig.index, 5); + assert_eq!(sig.recovery_id(), 27); + assert_eq!(sig.r(), &[0xAA; 32]); + assert_eq!(sig.s(), &[0xBB; 32]); + } +} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs b/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs new file mode 100644 index 0000000000..76551b8d93 --- /dev/null +++ b/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs @@ -0,0 +1,240 @@ +//! ECDSA signature verification for threshold signing. + +use secp256k1::{ecdsa::{RecoverableSignature, RecoveryId}, Message, SECP256K1}; + +use super::{SignatureSet, ThresholdConfig, ThresholdSigningError}; + +/// Verifies a set of ECDSA signatures against a threshold configuration. +/// +/// # Arguments +/// +/// * `config` - The threshold configuration containing authorized public keys +/// * `signatures` - The set of indexed ECDSA signatures to verify +/// * `message_hash` - The 32-byte message hash that was signed +/// +/// # Verification Steps +/// +/// 1. Check that the number of signatures meets the threshold +/// 2. For each signature, verify that: +/// - The signer index is within bounds +/// - The ECDSA signature is valid for the corresponding public key +/// +/// # Returns +/// +/// * `Ok(())` if all signatures are valid and threshold is met +/// * `Err(ThresholdSigningError)` otherwise +pub fn verify_threshold_signatures( + config: &ThresholdConfig, + signatures: &SignatureSet, + message_hash: &[u8; 32], +) -> Result<(), ThresholdSigningError> { + // Check threshold is met + if signatures.len() < config.threshold() as usize { + return Err(ThresholdSigningError::InsufficientSignatures { + provided: signatures.len(), + required: config.threshold() as usize, + }); + } + + // Create the message for verification + let message = Message::from_digest_slice(message_hash) + .map_err(|_| ThresholdSigningError::InvalidMessageHash)?; + + // Verify each signature + for indexed_sig in signatures.signatures() { + // Check index is in bounds + let index = indexed_sig.index as usize; + if index >= config.keys().len() { + return Err(ThresholdSigningError::SignerIndexOutOfBounds { + index: indexed_sig.index, + max: config.keys().len(), + }); + } + + // Get the expected public key + let expected_pubkey = config.keys()[index].as_inner(); + + // Parse the recoverable signature + let recovery_id = RecoveryId::from_i32(indexed_sig.recovery_id() as i32) + .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; + + let recoverable_sig = RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) + .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; + + // Recover the public key from the signature + let recovered_pubkey = SECP256K1 + .recover_ecdsa(&message, &recoverable_sig) + .map_err(|_| ThresholdSigningError::InvalidSignature { + index: indexed_sig.index, + })?; + + // Verify the recovered key matches the expected key + if &recovered_pubkey != expected_pubkey { + return Err(ThresholdSigningError::InvalidSignature { + index: indexed_sig.index, + }); + } + } + + Ok(()) +} + +/// Sign a message hash with ECDSA and return a recoverable signature. +/// +/// This is a helper function for testing and creating signatures. +#[cfg(test)] +fn sign_ecdsa_recoverable( + message_hash: &[u8; 32], + secret_key: &secp256k1::SecretKey, +) -> [u8; 65] { + let message = Message::from_digest_slice(message_hash).expect("32 bytes"); + let sig = SECP256K1.sign_ecdsa_recoverable(&message, secret_key); + let (recovery_id, compact) = sig.serialize_compact(); + + let mut result = [0u8; 65]; + result[0] = recovery_id.to_i32() as u8; + result[1..65].copy_from_slice(&compact); + result +} + +#[cfg(test)] +mod tests { + use secp256k1::{Secp256k1, SecretKey}; + + use super::*; + use crate::threshold_signing::indexed_signatures::{CompressedPublicKey, IndexedSignature}; + + fn generate_keypair(seed: u8) -> (SecretKey, CompressedPublicKey) { + let secp = Secp256k1::new(); + let mut sk_bytes = [0u8; 32]; + sk_bytes[31] = seed.max(1); + let sk = SecretKey::from_slice(&sk_bytes).unwrap(); + let pk = CompressedPublicKey::from(secp256k1::PublicKey::from_secret_key(&secp, &sk)); + (sk, pk) + } + + #[test] + fn test_verify_threshold_signatures_success() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Sign with keys 0 and 1 + let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); + let sig1 = sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = SignatureSet::new(vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1), + ]) + .unwrap(); + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(result.is_ok()); + } + + #[test] + fn test_verify_insufficient_signatures() { + let (_sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Only sign with one key + let sig1 = sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = SignatureSet::new(vec![IndexedSignature::new(1, sig1)]).unwrap(); + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSigningError::InsufficientSignatures { .. }) + )); + } + + #[test] + fn test_verify_invalid_signature() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + let wrong_message_hash = [0xCD; 32]; + + // Sign with correct message + let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); + // Sign with wrong message + let sig1_wrong = sign_ecdsa_recoverable(&wrong_message_hash, &sk2); + + let signatures = SignatureSet::new(vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1_wrong), + ]) + .unwrap(); + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSigningError::InvalidSignature { index: 1 }) + )); + } + + #[test] + fn test_verify_wrong_signer() { + let (sk1, pk1) = generate_keypair(1); + let (_sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Both signatures from sk1, but one claims to be from index 1 + let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); + let sig1_from_wrong_key = sign_ecdsa_recoverable(&message_hash, &sk1); + + let signatures = SignatureSet::new(vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1_from_wrong_key), // Claims to be key 1, but signed by key 0 + ]) + .unwrap(); + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSigningError::InvalidSignature { index: 1 }) + )); + } + + #[test] + fn test_verify_index_out_of_bounds() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); + let sig_oob = sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = SignatureSet::new(vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(99, sig_oob), // Out of bounds + ]) + .unwrap(); + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSigningError::SignerIndexOutOfBounds { index: 99, .. }) + )); + } +} diff --git a/crates/crypto/src/threshold_signing/mod.rs b/crates/crypto/src/threshold_signing/mod.rs new file mode 100644 index 0000000000..ede7580442 --- /dev/null +++ b/crates/crypto/src/threshold_signing/mod.rs @@ -0,0 +1,17 @@ +//! Threshold signing module for multi-party signature schemes. +//! +//! This module provides two sub-modules: +//! - `musig2`: MuSig2 key aggregation for the bridge subprotocol (N-of-N Schnorr) +//! - `indexed_signatures`: Individual ECDSA signatures for the admin subprotocol (M-of-N threshold) + +pub mod indexed_signatures; +pub mod musig2; + +// Re-export commonly used types from indexed_signatures +pub use indexed_signatures::{ + CompressedPublicKey, IndexedSignature, SignatureSet, ThresholdConfig, ThresholdConfigUpdate, + ThresholdSigningError, verify_threshold_signatures, +}; + +// Re-export MuSig2 key aggregation +pub use musig2::{aggregate_schnorr_keys, Musig2Error}; diff --git a/crates/crypto/src/threshold_signing/musig2/aggregation.rs b/crates/crypto/src/threshold_signing/musig2/aggregation.rs new file mode 100644 index 0000000000..713e232932 --- /dev/null +++ b/crates/crypto/src/threshold_signing/musig2/aggregation.rs @@ -0,0 +1,112 @@ +//! MuSig2 key aggregation for Schnorr signatures. + +use musig2::KeyAggContext; +use secp256k1::{Parity, PublicKey, XOnlyPublicKey}; +use strata_identifiers::Buf32; +use thiserror::Error; + +/// Errors that can occur during MuSig2 operations. +#[derive(Debug, Clone, Error, PartialEq, Eq)] +pub enum Musig2Error { + /// Invalid public key at a specific index. + #[error("invalid public key at index {index}: {reason}")] + InvalidPubKey { + /// The index of the invalid key. + index: usize, + /// The reason why the key is invalid. + reason: String, + }, + + /// Key aggregation context creation failed. + #[error("key aggregation context creation failed: {reason}")] + AggregationContextFailed { + /// The reason why context creation failed. + reason: String, + }, +} + +/// Aggregates a collection of Schnorr public keys using MuSig2 key aggregation. +/// +/// This function is used by the bridge subprotocol to create an aggregated +/// public key from all operator keys. The resulting key is used for: +/// - Generating deposit addresses (taproot) +/// - Verifying aggregated signatures on withdrawal transactions +/// +/// # Arguments +/// * `keys` - An iterator over 32-byte x-only public keys to aggregate +/// +/// # Returns +/// Returns the aggregated x-only public key on success. +/// +/// # Errors +/// * `Musig2Error::InvalidPubKey` - If a key is not a valid x-only public key +/// * `Musig2Error::AggregationContextFailed` - If MuSig2 context creation fails +/// +/// # Example +/// ```ignore +/// use strata_crypto::threshold_signing::musig2::aggregate_schnorr_keys; +/// use strata_identifiers::Buf32; +/// +/// let keys: Vec = operator_keys.iter().map(|k| k.into()).collect(); +/// let aggregated_key = aggregate_schnorr_keys(keys.iter())?; +/// ``` +pub fn aggregate_schnorr_keys<'k>( + keys: impl Iterator, +) -> Result { + let public_keys = keys + .enumerate() + .map(|(index, op)| { + XOnlyPublicKey::from_slice(op.as_ref()) + .map_err(|e| Musig2Error::InvalidPubKey { + index, + reason: e.to_string(), + }) + .map(|x_only| PublicKey::from_x_only_public_key(x_only, Parity::Even)) + }) + .collect::, Musig2Error>>()?; + + let agg_pubkey = KeyAggContext::new(public_keys) + .map_err(|e| Musig2Error::AggregationContextFailed { + reason: e.to_string(), + })? + .aggregated_pubkey::() + .x_only_public_key() + .0; + + Ok(agg_pubkey) +} + +#[cfg(test)] +mod tests { + use secp256k1::{Secp256k1, SecretKey}; + + use super::*; + + #[test] + fn test_aggregate_two_keys() { + let secp = Secp256k1::new(); + + let sk1 = SecretKey::from_slice(&[0x01; 32]).unwrap(); + let sk2 = SecretKey::from_slice(&[0x02; 32]).unwrap(); + + let pk1 = sk1.x_only_public_key(&secp).0; + let pk2 = sk2.x_only_public_key(&secp).0; + + let buf1 = Buf32::from(pk1.serialize()); + let buf2 = Buf32::from(pk2.serialize()); + + let keys = [buf1, buf2]; + let result = aggregate_schnorr_keys(keys.iter()); + + assert!(result.is_ok()); + } + + #[test] + fn test_aggregate_invalid_key() { + let invalid_key = Buf32::from([0u8; 32]); // All zeros is invalid + let keys = [invalid_key]; + + let result = aggregate_schnorr_keys(keys.iter()); + assert!(matches!(result, Err(Musig2Error::InvalidPubKey { .. }))); + } +} diff --git a/crates/crypto/src/threshold_signing/musig2/mod.rs b/crates/crypto/src/threshold_signing/musig2/mod.rs new file mode 100644 index 0000000000..80c67802b8 --- /dev/null +++ b/crates/crypto/src/threshold_signing/musig2/mod.rs @@ -0,0 +1,9 @@ +//! MuSig2 key aggregation for N-of-N Schnorr signatures. +//! +//! This module provides key aggregation functionality for the bridge subprotocol, +//! where all operators must sign (N-of-N). The aggregated public key is used +//! for taproot addresses and signature verification. + +mod aggregation; + +pub use aggregation::{aggregate_schnorr_keys, Musig2Error}; diff --git a/crates/l1tx/src/utils.rs b/crates/l1tx/src/utils.rs index 889f9f202c..46bf7e66f7 100644 --- a/crates/l1tx/src/utils.rs +++ b/crates/l1tx/src/utils.rs @@ -11,7 +11,7 @@ use strata_asm_txs_bridge_v1::{ withdrawal_fulfillment::WithdrawalFulfillmentInfo as BridgeV1WithdrawInfo, }; use strata_asm_types::{DepositInfo, WithdrawalFulfillmentInfo}; -use strata_crypto::multisig::aggregate_schnorr_keys; +use strata_crypto::threshold_signing::aggregate_schnorr_keys; use strata_params::{OperatorConfig, RollupParams}; use strata_primitives::{buf::Buf32, l1::BitcoinAddress}; From 4d5bdcc544a1d409ce61b2e7af91e979eebeb0fc Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 25 Nov 2025 14:42:26 +0400 Subject: [PATCH 02/11] fix: lint --- crates/asm/subprotocols/admin/src/handler.rs | 14 ++++------- crates/asm/subprotocols/admin/src/state.rs | 13 +++++----- crates/asm/txs/admin/src/test_utils/mod.rs | 6 ++--- crates/crypto/src/lib.rs | 2 +- crates/crypto/src/test_utils/schnorr.rs | 7 +----- .../indexed_signatures/errors.rs | 24 ++++--------------- .../indexed_signatures/pubkey.rs | 12 +++++----- .../indexed_signatures/verification.rs | 18 +++++++------- crates/crypto/src/threshold_signing/mod.rs | 5 ++-- 9 files changed, 37 insertions(+), 64 deletions(-) diff --git a/crates/asm/subprotocols/admin/src/handler.rs b/crates/asm/subprotocols/admin/src/handler.rs index e701de5309..978236ac0d 100644 --- a/crates/asm/subprotocols/admin/src/handler.rs +++ b/crates/asm/subprotocols/admin/src/handler.rs @@ -177,8 +177,8 @@ fn relay_checkpoint_predicate(relayer: &mut impl MsgRelayer, key: PredicateKey) mod tests { use std::any::Any; - use rand::{rngs::OsRng, seq::SliceRandom, thread_rng}; use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + use rand::{rngs::OsRng, seq::SliceRandom, thread_rng}; use strata_asm_common::{AsmLogEntry, InterprotoMsg, MsgRelayer}; use strata_asm_proto_checkpoint_v0::CheckpointIncomingMsg; use strata_asm_txs_admin::{ @@ -238,15 +238,10 @@ mod tests { } } - fn create_test_params() -> ( - AdministrationSubprotoParams, - Vec, - Vec, - ) { + fn create_test_params() -> (AdministrationSubprotoParams, Vec, Vec) { let secp = Secp256k1::new(); - let strata_admin_sks: Vec = - (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); + let strata_admin_sks: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); let strata_admin_pks: Vec = strata_admin_sks .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) @@ -259,8 +254,7 @@ mod tests { .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_sequencer_manager = - ThresholdConfig::try_new(strata_seq_manager_pks, 2).unwrap(); + let strata_sequencer_manager = ThresholdConfig::try_new(strata_seq_manager_pks, 2).unwrap(); let config = AdministrationSubprotoParams { strata_administrator, diff --git a/crates/asm/subprotocols/admin/src/state.rs b/crates/asm/subprotocols/admin/src/state.rs index c14a44ff16..7e23b90179 100644 --- a/crates/asm/subprotocols/admin/src/state.rs +++ b/crates/asm/subprotocols/admin/src/state.rs @@ -110,10 +110,12 @@ impl AdministrationSubprotoState { #[cfg(test)] mod tests { - use rand::rngs::OsRng; use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + use rand::rngs::OsRng; use strata_asm_txs_admin::actions::UpdateAction; - use strata_crypto::threshold_signing::{CompressedPublicKey, ThresholdConfig, ThresholdConfigUpdate}; + use strata_crypto::threshold_signing::{ + CompressedPublicKey, ThresholdConfig, ThresholdConfigUpdate, + }; use strata_primitives::roles::Role; use strata_test_utils::ArbitraryGenerator; @@ -270,11 +272,8 @@ mod tests { let new_size = initial_members.len() + add_members.len() - remove_members.len(); let new_threshold = 2u8; - let update = ThresholdConfigUpdate::new( - add_members.clone(), - remove_members.clone(), - new_threshold, - ); + let update = + ThresholdConfigUpdate::new(add_members.clone(), remove_members.clone(), new_threshold); state.apply_multisig_update(role, &update).unwrap(); diff --git a/crates/asm/txs/admin/src/test_utils/mod.rs b/crates/asm/txs/admin/src/test_utils/mod.rs index 20e268d5aa..55bbcf1cb5 100644 --- a/crates/asm/txs/admin/src/test_utils/mod.rs +++ b/crates/asm/txs/admin/src/test_utils/mod.rs @@ -19,9 +19,7 @@ use strata_primitives::buf::Buf32; pub(crate) const TEST_MAGIC_BYTES: &[u8; 4] = b"ALPN"; use crate::{ - actions::MultisigAction, - constants::ADMINISTRATION_SUBPROTOCOL_ID, - parser::SignedPayload, + actions::MultisigAction, constants::ADMINISTRATION_SUBPROTOCOL_ID, parser::SignedPayload, }; /// Creates an ECDSA recoverable signature for a message hash. @@ -195,7 +193,7 @@ mod tests { use rand::rngs::OsRng; use strata_asm_common::TxInputRef; use strata_crypto::threshold_signing::{ - verify_threshold_signatures, CompressedPublicKey, ThresholdConfig, + CompressedPublicKey, ThresholdConfig, verify_threshold_signatures, }; use strata_l1_txfmt::ParseConfig; use strata_test_utils::ArbitraryGenerator; diff --git a/crates/crypto/src/lib.rs b/crates/crypto/src/lib.rs index a0362fd6ac..a9654a3025 100644 --- a/crates/crypto/src/lib.rs +++ b/crates/crypto/src/lib.rs @@ -1,8 +1,8 @@ //! Cryptographic primitives. pub mod schnorr; -pub mod threshold_signing; #[cfg(feature = "test-utils")] pub mod test_utils; +pub mod threshold_signing; pub use schnorr::*; diff --git a/crates/crypto/src/test_utils/schnorr.rs b/crates/crypto/src/test_utils/schnorr.rs index 2c93789085..459e8e1fe6 100644 --- a/crates/crypto/src/test_utils/schnorr.rs +++ b/crates/crypto/src/test_utils/schnorr.rs @@ -125,12 +125,7 @@ pub fn create_agg_pubkey_from_privkeys(operators_privkeys: &[EvenSecretKey]) -> #[cfg(test)] mod tests { - use bitcoin::{ - hashes::Hash, - key::TapTweak, - secp256k1::Secp256k1, - TapNodeHash, - }; + use bitcoin::{hashes::Hash, key::TapTweak, secp256k1::Secp256k1, TapNodeHash}; use rand::rngs::OsRng; use secp256k1::SecretKey; diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs b/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs index ce32183b51..c6066f39d5 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs +++ b/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs @@ -6,35 +6,21 @@ use std::fmt; #[derive(Debug, Clone, PartialEq, Eq)] pub enum ThresholdSigningError { /// Not enough signatures to meet the threshold. - InsufficientSignatures { - provided: usize, - required: usize, - }, + InsufficientSignatures { provided: usize, required: usize }, /// Invalid public key data. - InvalidPublicKey { - index: usize, - reason: String, - }, + InvalidPublicKey { index: usize, reason: String }, /// Invalid threshold value. - InvalidThreshold { - threshold: u8, - total_keys: usize, - }, + InvalidThreshold { threshold: u8, total_keys: usize }, /// Threshold cannot be zero. ZeroThreshold, /// Signature verification failed. - InvalidSignature { - index: u8, - }, + InvalidSignature { index: u8 }, /// Invalid signature format. InvalidSignatureFormat, /// Duplicate signer index in signature set. DuplicateSignerIndex(u8), /// Signer index out of bounds. - SignerIndexOutOfBounds { - index: u8, - max: usize, - }, + SignerIndexOutOfBounds { index: u8, max: usize }, /// Member already exists in the configuration. MemberAlreadyExists, /// Duplicate member in add list. diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs b/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs index 3147bab951..bd4f68a630 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs +++ b/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs @@ -23,10 +23,11 @@ impl CompressedPublicKey { /// /// The slice must be exactly 33 bytes in compressed format (0x02 or 0x03 prefix). pub fn from_slice(data: &[u8]) -> Result { - let pk = PublicKey::from_slice(data).map_err(|e| ThresholdSigningError::InvalidPublicKey { - index: 0, - reason: e.to_string(), - })?; + let pk = + PublicKey::from_slice(data).map_err(|e| ThresholdSigningError::InvalidPublicKey { + index: 0, + reason: e.to_string(), + })?; Ok(Self(pk)) } @@ -77,8 +78,7 @@ impl<'a> Arbitrary<'a> for CompressedPublicKey { sk_bytes[31] = 1; } let secp = Secp256k1::new(); - let sk = SecretKey::from_slice(&sk_bytes) - .map_err(|_| arbitrary::Error::IncorrectFormat)?; + let sk = SecretKey::from_slice(&sk_bytes).map_err(|_| arbitrary::Error::IncorrectFormat)?; let pk = PublicKey::from_secret_key(&secp, &sk); Ok(Self(pk)) } diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs b/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs index 76551b8d93..d1d4e11868 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs +++ b/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs @@ -1,6 +1,9 @@ //! ECDSA signature verification for threshold signing. -use secp256k1::{ecdsa::{RecoverableSignature, RecoveryId}, Message, SECP256K1}; +use secp256k1::{ + ecdsa::{RecoverableSignature, RecoveryId}, + Message, SECP256K1, +}; use super::{SignatureSet, ThresholdConfig, ThresholdSigningError}; @@ -58,8 +61,9 @@ pub fn verify_threshold_signatures( let recovery_id = RecoveryId::from_i32(indexed_sig.recovery_id() as i32) .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; - let recoverable_sig = RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) - .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; + let recoverable_sig = + RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) + .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; // Recover the public key from the signature let recovered_pubkey = SECP256K1 @@ -83,10 +87,7 @@ pub fn verify_threshold_signatures( /// /// This is a helper function for testing and creating signatures. #[cfg(test)] -fn sign_ecdsa_recoverable( - message_hash: &[u8; 32], - secret_key: &secp256k1::SecretKey, -) -> [u8; 65] { +fn sign_ecdsa_recoverable(message_hash: &[u8; 32], secret_key: &secp256k1::SecretKey) -> [u8; 65] { let message = Message::from_digest_slice(message_hash).expect("32 bytes"); let sig = SECP256K1.sign_ecdsa_recoverable(&message, secret_key); let (recovery_id, compact) = sig.serialize_compact(); @@ -202,7 +203,8 @@ mod tests { let signatures = SignatureSet::new(vec![ IndexedSignature::new(0, sig0), - IndexedSignature::new(1, sig1_from_wrong_key), // Claims to be key 1, but signed by key 0 + IndexedSignature::new(1, sig1_from_wrong_key), /* Claims to be key 1, but signed by + * key 0 */ ]) .unwrap(); diff --git a/crates/crypto/src/threshold_signing/mod.rs b/crates/crypto/src/threshold_signing/mod.rs index ede7580442..4328b066c7 100644 --- a/crates/crypto/src/threshold_signing/mod.rs +++ b/crates/crypto/src/threshold_signing/mod.rs @@ -9,9 +9,8 @@ pub mod musig2; // Re-export commonly used types from indexed_signatures pub use indexed_signatures::{ - CompressedPublicKey, IndexedSignature, SignatureSet, ThresholdConfig, ThresholdConfigUpdate, - ThresholdSigningError, verify_threshold_signatures, + verify_threshold_signatures, CompressedPublicKey, IndexedSignature, SignatureSet, + ThresholdConfig, ThresholdConfigUpdate, ThresholdSigningError, }; - // Re-export MuSig2 key aggregation pub use musig2::{aggregate_schnorr_keys, Musig2Error}; From b08f24d61be82e01e6ac096e73ddcda1b19ae67e Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Mon, 1 Dec 2025 22:20:26 +0400 Subject: [PATCH 03/11] fix(crypto): address PR review comments for threshold signature refactoring --- .../asm/subprotocols/admin/src/authority.rs | 8 +- crates/asm/subprotocols/admin/src/config.rs | 2 +- crates/asm/subprotocols/admin/src/error.rs | 4 +- crates/asm/subprotocols/admin/src/handler.rs | 10 +- crates/asm/subprotocols/admin/src/state.rs | 4 +- .../bridge-v1/src/state/operator.rs | 2 +- .../txs/admin/src/actions/updates/multisig.rs | 2 +- crates/asm/txs/admin/src/parser.rs | 2 +- crates/asm/txs/admin/src/test_utils/mod.rs | 12 +- crates/crypto/src/lib.rs | 2 +- crates/crypto/src/test_utils/schnorr.rs | 2 +- .../indexed}/config.rs | 29 ++- .../src/threshold_signature/indexed/errors.rs | 68 +++++ .../indexed}/mod.rs | 4 +- .../indexed}/pubkey.rs | 6 +- .../indexed}/signature.rs | 10 +- .../indexed/verification.rs | 209 +++++++++++++++ .../indexed/verification/ecdsa.rs | 79 ++++++ crates/crypto/src/threshold_signature/mod.rs | 16 ++ .../musig2/aggregation.rs | 2 +- .../musig2/mod.rs | 0 .../indexed_signatures/errors.rs | 88 ------- .../indexed_signatures/verification.rs | 242 ------------------ crates/crypto/src/threshold_signing/mod.rs | 16 -- crates/l1tx/src/utils.rs | 2 +- 25 files changed, 428 insertions(+), 393 deletions(-) rename crates/crypto/src/{threshold_signing/indexed_signatures => threshold_signature/indexed}/config.rs (91%) create mode 100644 crates/crypto/src/threshold_signature/indexed/errors.rs rename crates/crypto/src/{threshold_signing/indexed_signatures => threshold_signature/indexed}/mod.rs (81%) rename crates/crypto/src/{threshold_signing/indexed_signatures => threshold_signature/indexed}/pubkey.rs (98%) rename crates/crypto/src/{threshold_signing/indexed_signatures => threshold_signature/indexed}/signature.rs (95%) create mode 100644 crates/crypto/src/threshold_signature/indexed/verification.rs create mode 100644 crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs create mode 100644 crates/crypto/src/threshold_signature/mod.rs rename crates/crypto/src/{threshold_signing => threshold_signature}/musig2/aggregation.rs (97%) rename crates/crypto/src/{threshold_signing => threshold_signature}/musig2/mod.rs (100%) delete mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/errors.rs delete mode 100644 crates/crypto/src/threshold_signing/indexed_signatures/verification.rs delete mode 100644 crates/crypto/src/threshold_signing/mod.rs diff --git a/crates/asm/subprotocols/admin/src/authority.rs b/crates/asm/subprotocols/admin/src/authority.rs index b505518aab..2e7f2adbf9 100644 --- a/crates/asm/subprotocols/admin/src/authority.rs +++ b/crates/asm/subprotocols/admin/src/authority.rs @@ -1,7 +1,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_txs_admin::actions::MultisigAction; -use strata_crypto::threshold_signing::{ - SignatureSet, ThresholdConfig, ThresholdSigningError, verify_threshold_signatures, +use strata_crypto::threshold_signature::{ + SignatureSet, ThresholdConfig, ThresholdSignatureError, verify_threshold_signatures, }; use strata_primitives::roles::Role; @@ -50,12 +50,12 @@ impl MultisigAuthority { &self, action: &MultisigAction, signatures: &SignatureSet, - ) -> Result<(), ThresholdSigningError> { + ) -> Result<(), ThresholdSignatureError> { // Compute the msg to sign by combining UpdateAction with sequence no let sig_hash = action.compute_sighash(self.seqno); // Verify each ECDSA signature against the corresponding public key - verify_threshold_signatures(&self.config, signatures, &sig_hash.into()) + verify_threshold_signatures(&self.config, signatures.signatures(), &sig_hash.into()) } /// Increments the seqno. diff --git a/crates/asm/subprotocols/admin/src/config.rs b/crates/asm/subprotocols/admin/src/config.rs index 110d28f7b8..caa7263ec7 100644 --- a/crates/asm/subprotocols/admin/src/config.rs +++ b/crates/asm/subprotocols/admin/src/config.rs @@ -1,5 +1,5 @@ use borsh::{BorshDeserialize, BorshSerialize}; -use strata_crypto::threshold_signing::ThresholdConfig; +use strata_crypto::threshold_signature::ThresholdConfig; use strata_primitives::roles::Role; /// Parameters for the admnistration subprotocol, containing ThresholdConfig for each role. diff --git a/crates/asm/subprotocols/admin/src/error.rs b/crates/asm/subprotocols/admin/src/error.rs index 22c2c87e3e..1e4c188dfc 100644 --- a/crates/asm/subprotocols/admin/src/error.rs +++ b/crates/asm/subprotocols/admin/src/error.rs @@ -1,5 +1,5 @@ use strata_asm_txs_admin::actions::UpdateId; -use strata_crypto::threshold_signing::ThresholdSigningError; +use strata_crypto::threshold_signature::ThresholdSignatureError; use thiserror::Error; /// Top-level error type for the administration subprotocol, composed of smaller error categories. @@ -15,5 +15,5 @@ pub enum AdministrationError { /// Indicates a threshold signing error (configuration or signature validation). #[error(transparent)] - ThresholdSigning(#[from] ThresholdSigningError), + ThresholdSigning(#[from] ThresholdSignatureError), } diff --git a/crates/asm/subprotocols/admin/src/handler.rs b/crates/asm/subprotocols/admin/src/handler.rs index 978236ac0d..81a01dcf9e 100644 --- a/crates/asm/subprotocols/admin/src/handler.rs +++ b/crates/asm/subprotocols/admin/src/handler.rs @@ -4,7 +4,7 @@ use strata_asm_common::{ }; use strata_asm_proto_checkpoint_v0::CheckpointIncomingMsg; use strata_asm_txs_admin::actions::{MultisigAction, UpdateAction}; -use strata_crypto::threshold_signing::SignatureSet; +use strata_crypto::threshold_signature::SignatureSet; use strata_predicate::PredicateKey; use strata_primitives::{buf::Buf32, roles::ProofType}; @@ -188,8 +188,8 @@ mod tests { }, test_utils::create_signature_set, }; - use strata_crypto::threshold_signing::{ - CompressedPublicKey, SignatureSet, ThresholdConfig, ThresholdSigningError, + use strata_crypto::threshold_signature::{ + CompressedPublicKey, SignatureSet, ThresholdConfig, ThresholdSignatureError, }; use strata_predicate::PredicateKey; use strata_primitives::roles::{ProofType, Role}; @@ -387,7 +387,7 @@ mod tests { assert!(matches!( res, Err(AdministrationError::ThresholdSigning( - ThresholdSigningError::InvalidSignature { .. } + ThresholdSignatureError::InvalidSignature { .. } )) )); @@ -407,7 +407,7 @@ mod tests { assert!(matches!( res, Err(AdministrationError::ThresholdSigning( - ThresholdSigningError::InvalidSignature { .. } + ThresholdSignatureError::InvalidSignature { .. } )) )); } diff --git a/crates/asm/subprotocols/admin/src/state.rs b/crates/asm/subprotocols/admin/src/state.rs index 7e23b90179..ea2853fb5f 100644 --- a/crates/asm/subprotocols/admin/src/state.rs +++ b/crates/asm/subprotocols/admin/src/state.rs @@ -1,6 +1,6 @@ use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_txs_admin::actions::UpdateId; -use strata_crypto::threshold_signing::ThresholdConfigUpdate; +use strata_crypto::threshold_signature::ThresholdConfigUpdate; use strata_primitives::roles::Role; use crate::{ @@ -113,7 +113,7 @@ mod tests { use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use rand::rngs::OsRng; use strata_asm_txs_admin::actions::UpdateAction; - use strata_crypto::threshold_signing::{ + use strata_crypto::threshold_signature::{ CompressedPublicKey, ThresholdConfig, ThresholdConfigUpdate, }; use strata_primitives::roles::Role; diff --git a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs index b5bdc6844a..bd3cd14ea1 100644 --- a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs +++ b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs @@ -5,7 +5,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; use serde::{Deserialize, Serialize}; use strata_bridge_types::OperatorIdx; -use strata_crypto::{schnorr::EvenPublicKey, threshold_signing::aggregate_schnorr_keys}; +use strata_crypto::{schnorr::EvenPublicKey, threshold_signature::aggregate_schnorr_keys}; use strata_primitives::{buf::Buf32, l1::BitcoinXOnlyPublicKey, sorted_vec::SortedVec}; use super::bitmap::OperatorBitmap; diff --git a/crates/asm/txs/admin/src/actions/updates/multisig.rs b/crates/asm/txs/admin/src/actions/updates/multisig.rs index f8604d511b..60239e7fbd 100644 --- a/crates/asm/txs/admin/src/actions/updates/multisig.rs +++ b/crates/asm/txs/admin/src/actions/updates/multisig.rs @@ -1,6 +1,6 @@ use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; -use strata_crypto::threshold_signing::ThresholdConfigUpdate; +use strata_crypto::threshold_signature::ThresholdConfigUpdate; use strata_primitives::roles::Role; /// An update to a threshold configuration for a specific role: diff --git a/crates/asm/txs/admin/src/parser.rs b/crates/asm/txs/admin/src/parser.rs index 0c0ec46301..f7190a5858 100644 --- a/crates/asm/txs/admin/src/parser.rs +++ b/crates/asm/txs/admin/src/parser.rs @@ -1,6 +1,6 @@ use borsh::{BorshDeserialize, BorshSerialize}; use strata_asm_common::TxInputRef; -use strata_crypto::threshold_signing::SignatureSet; +use strata_crypto::threshold_signature::SignatureSet; use strata_l1_envelope_fmt::parser::parse_envelope_payload; use crate::{actions::MultisigAction, errors::AdministrationTxParseError}; diff --git a/crates/asm/txs/admin/src/test_utils/mod.rs b/crates/asm/txs/admin/src/test_utils/mod.rs index 55bbcf1cb5..91c79c9ffa 100644 --- a/crates/asm/txs/admin/src/test_utils/mod.rs +++ b/crates/asm/txs/admin/src/test_utils/mod.rs @@ -13,7 +13,7 @@ use bitcoin::{ transaction::Version, }; use rand::{RngCore, rngs::OsRng}; -use strata_crypto::threshold_signing::{IndexedSignature, SignatureSet}; +use strata_crypto::threshold_signature::{IndexedSignature, SignatureSet}; use strata_primitives::buf::Buf32; pub(crate) const TEST_MAGIC_BYTES: &[u8; 4] = b"ALPN"; @@ -192,7 +192,7 @@ mod tests { use bitcoin::secp256k1::{PublicKey, Secp256k1}; use rand::rngs::OsRng; use strata_asm_common::TxInputRef; - use strata_crypto::threshold_signing::{ + use strata_crypto::threshold_signature::{ CompressedPublicKey, ThresholdConfig, verify_threshold_signatures, }; use strata_l1_txfmt::ParseConfig; @@ -231,7 +231,7 @@ mod tests { assert_eq!(indices, vec![0, 2]); // Verify the signatures - let res = verify_threshold_signatures(&config, &signature_set, &sighash.0); + let res = verify_threshold_signatures(&config, signature_set.signatures(), &sighash.0); assert!(res.is_ok()); } @@ -264,7 +264,11 @@ mod tests { assert_eq!(action, p_action); // Verify the signatures - let res = verify_threshold_signatures(&config, &sig, &action.compute_sighash(seqno).0); + let res = verify_threshold_signatures( + &config, + sig.signatures(), + &action.compute_sighash(seqno).0, + ); assert!(res.is_ok()); } } diff --git a/crates/crypto/src/lib.rs b/crates/crypto/src/lib.rs index a9654a3025..36f2841565 100644 --- a/crates/crypto/src/lib.rs +++ b/crates/crypto/src/lib.rs @@ -3,6 +3,6 @@ pub mod schnorr; #[cfg(feature = "test-utils")] pub mod test_utils; -pub mod threshold_signing; +pub mod threshold_signature; pub use schnorr::*; diff --git a/crates/crypto/src/test_utils/schnorr.rs b/crates/crypto/src/test_utils/schnorr.rs index 459e8e1fe6..a6f4dde251 100644 --- a/crates/crypto/src/test_utils/schnorr.rs +++ b/crates/crypto/src/test_utils/schnorr.rs @@ -3,7 +3,7 @@ use rand::{rngs::OsRng, RngCore}; use secp256k1::{PublicKey, Secp256k1, XOnlyPublicKey}; use strata_identifiers::Buf32; -use crate::{schnorr::EvenSecretKey, threshold_signing::musig2::aggregate_schnorr_keys}; +use crate::{schnorr::EvenSecretKey, threshold_signature::musig2::aggregate_schnorr_keys}; /// Creates a MuSig2 signature from multiple operators. /// diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs similarity index 91% rename from crates/crypto/src/threshold_signing/indexed_signatures/config.rs rename to crates/crypto/src/threshold_signature/indexed/config.rs index cf506b41e9..16a72e07a6 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -5,7 +5,7 @@ use std::collections::HashSet; use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; -use super::{CompressedPublicKey, ThresholdSigningError}; +use super::{CompressedPublicKey, ThresholdSignatureError}; /// Configuration for a threshold signature authority. /// @@ -23,14 +23,14 @@ impl ThresholdConfig { /// /// # Errors /// - /// Returns `ThresholdSigningError` if: + /// Returns `ThresholdSignatureError` if: /// - `DuplicateAddMember`: The keys list contains duplicate members /// - `ZeroThreshold`: The threshold is zero /// - `InvalidThreshold`: The threshold exceeds the total number of keys pub fn try_new( keys: Vec, threshold: u8, - ) -> Result { + ) -> Result { let mut config = ThresholdConfig { keys: vec![], threshold: 0, @@ -64,7 +64,7 @@ impl ThresholdConfig { pub fn validate_update( &self, update: &ThresholdConfigUpdate, - ) -> Result<(), ThresholdSigningError> { + ) -> Result<(), ThresholdSignatureError> { let members_to_add: HashSet = update.add_members().iter().cloned().collect(); let members_to_remove: HashSet = @@ -72,28 +72,28 @@ impl ThresholdConfig { // Ensure no duplicate members in the add list if members_to_add.len() != update.add_members().len() { - return Err(ThresholdSigningError::DuplicateAddMember); + return Err(ThresholdSignatureError::DuplicateAddMember); } // Ensure no duplicate members in the remove list if members_to_remove.len() != update.remove_members().len() { - return Err(ThresholdSigningError::DuplicateRemoveMember); + return Err(ThresholdSignatureError::DuplicateRemoveMember); } // Ensure new members don't already exist in current configuration if members_to_add.iter().any(|m| self.keys.contains(m)) { - return Err(ThresholdSigningError::MemberAlreadyExists); + return Err(ThresholdSignatureError::MemberAlreadyExists); } // Ensure new threshold is not zero if update.new_threshold() == 0 { - return Err(ThresholdSigningError::ZeroThreshold); + return Err(ThresholdSignatureError::ZeroThreshold); } // Ensure all members to remove exist in current configuration for member_to_remove in update.remove_members() { if !self.keys.contains(member_to_remove) { - return Err(ThresholdSigningError::MemberNotFound); + return Err(ThresholdSignatureError::MemberNotFound); } } @@ -102,7 +102,7 @@ impl ThresholdConfig { self.keys.len() + update.add_members().len() - update.remove_members().len(); if (update.new_threshold() as usize) > updated_size { - return Err(ThresholdSigningError::InvalidThreshold { + return Err(ThresholdSignatureError::InvalidThreshold { threshold: update.new_threshold(), total_keys: updated_size, }); @@ -115,7 +115,7 @@ impl ThresholdConfig { pub fn apply_update( &mut self, update: &ThresholdConfigUpdate, - ) -> Result<(), ThresholdSigningError> { + ) -> Result<(), ThresholdSignatureError> { self.validate_update(update)?; // Remove members by matching public keys @@ -245,7 +245,10 @@ mod tests { fn test_config_zero_threshold() { let keys = vec![make_key(1)]; let result = ThresholdConfig::try_new(keys, 0); - assert!(matches!(result, Err(ThresholdSigningError::ZeroThreshold))); + assert!(matches!( + result, + Err(ThresholdSignatureError::ZeroThreshold) + )); } #[test] @@ -254,7 +257,7 @@ mod tests { let result = ThresholdConfig::try_new(keys, 3); assert!(matches!( result, - Err(ThresholdSigningError::InvalidThreshold { .. }) + Err(ThresholdSignatureError::InvalidThreshold { .. }) )); } diff --git a/crates/crypto/src/threshold_signature/indexed/errors.rs b/crates/crypto/src/threshold_signature/indexed/errors.rs new file mode 100644 index 0000000000..9bcf10bc2d --- /dev/null +++ b/crates/crypto/src/threshold_signature/indexed/errors.rs @@ -0,0 +1,68 @@ +//! Error types for threshold signature operations. + +use thiserror::Error; + +/// Errors that can occur during threshold signature operations. +#[derive(Debug, Clone, PartialEq, Eq, Error)] +pub enum ThresholdSignatureError { + /// Not enough signatures to meet the threshold. + #[error("insufficient signatures: provided {provided}, required {required}")] + InsufficientSignatures { provided: usize, required: usize }, + + /// Invalid public key data. + #[error("invalid public key at index {index}: {reason}")] + InvalidPublicKey { index: usize, reason: String }, + + /// Invalid threshold value. + #[error("invalid threshold: {threshold} exceeds total keys {total_keys}")] + InvalidThreshold { threshold: u8, total_keys: usize }, + + /// Threshold cannot be zero. + #[error("threshold cannot be zero")] + ZeroThreshold, + + /// Signature verification failed. + #[error("invalid signature at index {index}")] + InvalidSignature { index: u8 }, + + /// Invalid signature format. + #[error("invalid signature format")] + InvalidSignatureFormat, + + /// Duplicate signer index in signature set. + #[error("duplicate signer index: {0}")] + DuplicateSignerIndex(u8), + + /// Signer index out of bounds. + #[error("signer index {index} out of bounds (max: {max})")] + SignerIndexOutOfBounds { index: u8, max: usize }, + + /// Member already exists in the configuration. + #[error("member already exists")] + MemberAlreadyExists, + + /// Duplicate member in add list. + #[error("duplicate member in add list")] + DuplicateAddMember, + + /// Duplicate member in remove list. + #[error("duplicate member in remove list")] + DuplicateRemoveMember, + + /// Member not found in the configuration. + #[error("member not found")] + MemberNotFound, + + /// Invalid message hash. + #[error("invalid message hash")] + InvalidMessageHash, +} + +impl From for ThresholdSignatureError { + fn from(e: secp256k1::Error) -> Self { + Self::InvalidPublicKey { + index: 0, + reason: e.to_string(), + } + } +} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/mod.rs b/crates/crypto/src/threshold_signature/indexed/mod.rs similarity index 81% rename from crates/crypto/src/threshold_signing/indexed_signatures/mod.rs rename to crates/crypto/src/threshold_signature/indexed/mod.rs index 98b02b8afb..ada6ef39d5 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/mod.rs +++ b/crates/crypto/src/threshold_signature/indexed/mod.rs @@ -1,4 +1,4 @@ -//! Individual ECDSA signature set for threshold signing (M-of-N). +//! Individual ECDSA signature set for threshold signatures (M-of-N). //! //! This module provides types and functions for verifying a set of individual //! ECDSA signatures against a threshold configuration. Used by the admin @@ -11,7 +11,7 @@ mod signature; mod verification; pub use config::{ThresholdConfig, ThresholdConfigUpdate}; -pub use errors::ThresholdSigningError; +pub use errors::ThresholdSignatureError; pub use pubkey::CompressedPublicKey; pub use signature::{IndexedSignature, SignatureSet}; pub use verification::verify_threshold_signatures; diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs b/crates/crypto/src/threshold_signature/indexed/pubkey.rs similarity index 98% rename from crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs rename to crates/crypto/src/threshold_signature/indexed/pubkey.rs index bd4f68a630..64cc4daad8 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/pubkey.rs +++ b/crates/crypto/src/threshold_signature/indexed/pubkey.rs @@ -8,7 +8,7 @@ use secp256k1::{PublicKey, Secp256k1, SecretKey}; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use super::ThresholdSigningError; +use super::ThresholdSignatureError; /// A compressed secp256k1 public key (33 bytes). /// @@ -22,9 +22,9 @@ impl CompressedPublicKey { /// Create a new `CompressedPublicKey` from a byte slice. /// /// The slice must be exactly 33 bytes in compressed format (0x02 or 0x03 prefix). - pub fn from_slice(data: &[u8]) -> Result { + pub fn from_slice(data: &[u8]) -> Result { let pk = - PublicKey::from_slice(data).map_err(|e| ThresholdSigningError::InvalidPublicKey { + PublicKey::from_slice(data).map_err(|e| ThresholdSignatureError::InvalidPublicKey { index: 0, reason: e.to_string(), })?; diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs similarity index 95% rename from crates/crypto/src/threshold_signing/indexed_signatures/signature.rs rename to crates/crypto/src/threshold_signature/indexed/signature.rs index 46b861fd0b..32864f9687 100644 --- a/crates/crypto/src/threshold_signing/indexed_signatures/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -2,7 +2,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; -use super::ThresholdSigningError; +use super::ThresholdSignatureError; /// An individual ECDSA signature with its signer index. /// @@ -75,14 +75,16 @@ impl SignatureSet { /// Create a new signature set from a vector of indexed signatures. /// /// The signatures will be sorted by index and checked for duplicates. - pub fn new(mut signatures: Vec) -> Result { + pub fn new(mut signatures: Vec) -> Result { // Sort by index signatures.sort_by_key(|s| s.index); // Check for duplicate indices for window in signatures.windows(2) { if window[0].index == window[1].index { - return Err(ThresholdSigningError::DuplicateSignerIndex(window[0].index)); + return Err(ThresholdSignatureError::DuplicateSignerIndex( + window[0].index, + )); } } @@ -175,7 +177,7 @@ mod tests { let result = SignatureSet::new(sigs); assert!(matches!( result, - Err(ThresholdSigningError::DuplicateSignerIndex(1)) + Err(ThresholdSignatureError::DuplicateSignerIndex(1)) )); } diff --git a/crates/crypto/src/threshold_signature/indexed/verification.rs b/crates/crypto/src/threshold_signature/indexed/verification.rs new file mode 100644 index 0000000000..fba9cfd0b9 --- /dev/null +++ b/crates/crypto/src/threshold_signature/indexed/verification.rs @@ -0,0 +1,209 @@ +//! ECDSA signature verification for threshold signatures. + +use super::{IndexedSignature, SignatureSet, ThresholdConfig, ThresholdSignatureError}; + +mod ecdsa; + +/// Verifies a set of ECDSA signatures against a threshold configuration. +/// +/// # Arguments +/// +/// * `config` - The threshold configuration containing authorized public keys +/// * `signatures` - Slice of indexed ECDSA signatures to verify +/// * `message_hash` - The 32-byte message hash that was signed +/// +/// # Verification Steps +/// +/// 1. Construct and validate SignatureSet (checks for duplicates) +/// 2. Check that the number of signatures meets the threshold +/// 3. For each signature, verify that: +/// - The signer index is within bounds +/// - The ECDSA signature is valid for the corresponding public key +/// +/// # Returns +/// +/// * `Ok(())` if all signatures are valid and threshold is met +/// * `Err(ThresholdSignatureError)` otherwise +pub fn verify_threshold_signatures( + config: &ThresholdConfig, + signatures: &[IndexedSignature], + message_hash: &[u8; 32], +) -> Result<(), ThresholdSignatureError> { + // Construct and validate SignatureSet (checks for duplicates) + let signature_set = SignatureSet::new(signatures.to_vec())?; + + // Check threshold is met + if signature_set.len() < config.threshold() as usize { + return Err(ThresholdSignatureError::InsufficientSignatures { + provided: signature_set.len(), + required: config.threshold() as usize, + }); + } + + // Delegate to ECDSA-specific verification + ecdsa::verify_ecdsa_signatures(config, &signature_set, message_hash) +} + +#[cfg(test)] +mod tests { + use secp256k1::{Secp256k1, SecretKey}; + + use super::*; + use crate::threshold_signature::indexed::CompressedPublicKey; + + fn generate_keypair(seed: u8) -> (SecretKey, CompressedPublicKey) { + let secp = Secp256k1::new(); + let mut sk_bytes = [0u8; 32]; + sk_bytes[31] = seed.max(1); + let sk = SecretKey::from_slice(&sk_bytes).unwrap(); + let pk = CompressedPublicKey::from(secp256k1::PublicKey::from_secret_key(&secp, &sk)); + (sk, pk) + } + + #[test] + fn test_verify_threshold_signatures_success() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Sign with keys 0 and 1 + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + let sig1 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1), + ]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(result.is_ok()); + } + + #[test] + fn test_verify_insufficient_signatures() { + let (_sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Only sign with one key + let sig1 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = vec![IndexedSignature::new(1, sig1)]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSignatureError::InsufficientSignatures { .. }) + )); + } + + #[test] + fn test_verify_invalid_signature() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + let wrong_message_hash = [0xCD; 32]; + + // Sign with correct message + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + // Sign with wrong message + let sig1_wrong = ecdsa::sign_ecdsa_recoverable(&wrong_message_hash, &sk2); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1_wrong), + ]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSignatureError::InvalidSignature { index: 1 }) + )); + } + + #[test] + fn test_verify_wrong_signer() { + let (sk1, pk1) = generate_keypair(1); + let (_sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Both signatures from sk1, but one claims to be from index 1 + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + let sig1_from_wrong_key = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1_from_wrong_key), /* Claims to be key 1, but signed by + * key 0 */ + ]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSignatureError::InvalidSignature { index: 1 }) + )); + } + + #[test] + fn test_verify_index_out_of_bounds() { + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + let sig_oob = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk2); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(99, sig_oob), // Out of bounds + ]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSignatureError::SignerIndexOutOfBounds { index: 99, .. }) + )); + } + + #[test] + fn test_verify_duplicate_signer_rejected() { + let (sk1, pk1) = generate_keypair(1); + let (_sk2, pk2) = generate_keypair(2); + + let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Same signer index twice (should fail) + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + let sig0_dup = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(0, sig0_dup), + ]; + + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!(matches!( + result, + Err(ThresholdSignatureError::DuplicateSignerIndex(0)) + )); + } +} diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs new file mode 100644 index 0000000000..b20ce31eb8 --- /dev/null +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -0,0 +1,79 @@ +//! ECDSA-specific signature verification implementation. + +use secp256k1::{ + ecdsa::{RecoverableSignature, RecoveryId}, + Message, SECP256K1, +}; + +use crate::threshold_signature::indexed::{SignatureSet, ThresholdConfig, ThresholdSignatureError}; + +/// Verifies each ECDSA signature in the set against the corresponding public key. +/// +/// This function performs the actual ECDSA signature recovery and verification. +/// It assumes the SignatureSet has already been validated for duplicates. +pub(super) fn verify_ecdsa_signatures( + config: &ThresholdConfig, + signatures: &SignatureSet, + message_hash: &[u8; 32], +) -> Result<(), ThresholdSignatureError> { + // Create the message for verification + let message = Message::from_digest_slice(message_hash) + .map_err(|_| ThresholdSignatureError::InvalidMessageHash)?; + + // Verify each signature + for indexed_sig in signatures.signatures() { + // Check index is in bounds + let index = indexed_sig.index as usize; + if index >= config.keys().len() { + return Err(ThresholdSignatureError::SignerIndexOutOfBounds { + index: indexed_sig.index, + max: config.keys().len(), + }); + } + + // Get the expected public key + let expected_pubkey = config.keys()[index].as_inner(); + + // Parse the recoverable signature + let recovery_id = RecoveryId::from_i32(indexed_sig.recovery_id() as i32) + .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?; + + let recoverable_sig = + RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) + .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?; + + // Recover the public key from the signature + let recovered_pubkey = SECP256K1 + .recover_ecdsa(&message, &recoverable_sig) + .map_err(|_| ThresholdSignatureError::InvalidSignature { + index: indexed_sig.index, + })?; + + // Verify the recovered key matches the expected key + if &recovered_pubkey != expected_pubkey { + return Err(ThresholdSignatureError::InvalidSignature { + index: indexed_sig.index, + }); + } + } + + Ok(()) +} + +/// Sign a message hash with ECDSA and return a recoverable signature. +/// +/// This is a helper function for testing and creating signatures. +#[cfg(test)] +pub(super) fn sign_ecdsa_recoverable( + message_hash: &[u8; 32], + secret_key: &secp256k1::SecretKey, +) -> [u8; 65] { + let message = Message::from_digest_slice(message_hash).expect("32 bytes"); + let sig = SECP256K1.sign_ecdsa_recoverable(&message, secret_key); + let (recovery_id, compact) = sig.serialize_compact(); + + let mut result = [0u8; 65]; + result[0] = recovery_id.to_i32() as u8; + result[1..65].copy_from_slice(&compact); + result +} diff --git a/crates/crypto/src/threshold_signature/mod.rs b/crates/crypto/src/threshold_signature/mod.rs new file mode 100644 index 0000000000..4f6d20cbf0 --- /dev/null +++ b/crates/crypto/src/threshold_signature/mod.rs @@ -0,0 +1,16 @@ +//! Threshold signature module for multi-party signature schemes. +//! +//! This module provides two sub-modules: +//! - `musig2`: MuSig2 key aggregation for the bridge subprotocol (N-of-N Schnorr) +//! - `indexed`: Individual ECDSA signatures for the admin subprotocol (M-of-N threshold) + +pub mod indexed; +pub mod musig2; + +// Re-export commonly used types from indexed +pub use indexed::{ + verify_threshold_signatures, CompressedPublicKey, IndexedSignature, SignatureSet, + ThresholdConfig, ThresholdConfigUpdate, ThresholdSignatureError, +}; +// Re-export MuSig2 key aggregation +pub use musig2::{aggregate_schnorr_keys, Musig2Error}; diff --git a/crates/crypto/src/threshold_signing/musig2/aggregation.rs b/crates/crypto/src/threshold_signature/musig2/aggregation.rs similarity index 97% rename from crates/crypto/src/threshold_signing/musig2/aggregation.rs rename to crates/crypto/src/threshold_signature/musig2/aggregation.rs index 713e232932..3049ff59e9 100644 --- a/crates/crypto/src/threshold_signing/musig2/aggregation.rs +++ b/crates/crypto/src/threshold_signature/musig2/aggregation.rs @@ -44,7 +44,7 @@ pub enum Musig2Error { /// /// # Example /// ```ignore -/// use strata_crypto::threshold_signing::musig2::aggregate_schnorr_keys; +/// use strata_crypto::threshold_signature::musig2::aggregate_schnorr_keys; /// use strata_identifiers::Buf32; /// /// let keys: Vec = operator_keys.iter().map(|k| k.into()).collect(); diff --git a/crates/crypto/src/threshold_signing/musig2/mod.rs b/crates/crypto/src/threshold_signature/musig2/mod.rs similarity index 100% rename from crates/crypto/src/threshold_signing/musig2/mod.rs rename to crates/crypto/src/threshold_signature/musig2/mod.rs diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs b/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs deleted file mode 100644 index c6066f39d5..0000000000 --- a/crates/crypto/src/threshold_signing/indexed_signatures/errors.rs +++ /dev/null @@ -1,88 +0,0 @@ -//! Error types for threshold signing operations. - -use std::fmt; - -/// Errors that can occur during threshold signing operations. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum ThresholdSigningError { - /// Not enough signatures to meet the threshold. - InsufficientSignatures { provided: usize, required: usize }, - /// Invalid public key data. - InvalidPublicKey { index: usize, reason: String }, - /// Invalid threshold value. - InvalidThreshold { threshold: u8, total_keys: usize }, - /// Threshold cannot be zero. - ZeroThreshold, - /// Signature verification failed. - InvalidSignature { index: u8 }, - /// Invalid signature format. - InvalidSignatureFormat, - /// Duplicate signer index in signature set. - DuplicateSignerIndex(u8), - /// Signer index out of bounds. - SignerIndexOutOfBounds { index: u8, max: usize }, - /// Member already exists in the configuration. - MemberAlreadyExists, - /// Duplicate member in add list. - DuplicateAddMember, - /// Duplicate member in remove list. - DuplicateRemoveMember, - /// Member not found in the configuration. - MemberNotFound, - /// Invalid message hash. - InvalidMessageHash, -} - -impl fmt::Display for ThresholdSigningError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::InsufficientSignatures { provided, required } => { - write!( - f, - "insufficient signatures: provided {}, required {}", - provided, required - ) - } - Self::InvalidPublicKey { index, reason } => { - write!(f, "invalid public key at index {}: {}", index, reason) - } - Self::InvalidThreshold { - threshold, - total_keys, - } => { - write!( - f, - "invalid threshold: {} exceeds total keys {}", - threshold, total_keys - ) - } - Self::ZeroThreshold => write!(f, "threshold cannot be zero"), - Self::InvalidSignature { index } => { - write!(f, "invalid signature at index {}", index) - } - Self::InvalidSignatureFormat => write!(f, "invalid signature format"), - Self::DuplicateSignerIndex(index) => { - write!(f, "duplicate signer index: {}", index) - } - Self::SignerIndexOutOfBounds { index, max } => { - write!(f, "signer index {} out of bounds (max: {})", index, max) - } - Self::MemberAlreadyExists => write!(f, "member already exists"), - Self::DuplicateAddMember => write!(f, "duplicate member in add list"), - Self::DuplicateRemoveMember => write!(f, "duplicate member in remove list"), - Self::MemberNotFound => write!(f, "member not found"), - Self::InvalidMessageHash => write!(f, "invalid message hash"), - } - } -} - -impl std::error::Error for ThresholdSigningError {} - -impl From for ThresholdSigningError { - fn from(e: secp256k1::Error) -> Self { - Self::InvalidPublicKey { - index: 0, - reason: e.to_string(), - } - } -} diff --git a/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs b/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs deleted file mode 100644 index d1d4e11868..0000000000 --- a/crates/crypto/src/threshold_signing/indexed_signatures/verification.rs +++ /dev/null @@ -1,242 +0,0 @@ -//! ECDSA signature verification for threshold signing. - -use secp256k1::{ - ecdsa::{RecoverableSignature, RecoveryId}, - Message, SECP256K1, -}; - -use super::{SignatureSet, ThresholdConfig, ThresholdSigningError}; - -/// Verifies a set of ECDSA signatures against a threshold configuration. -/// -/// # Arguments -/// -/// * `config` - The threshold configuration containing authorized public keys -/// * `signatures` - The set of indexed ECDSA signatures to verify -/// * `message_hash` - The 32-byte message hash that was signed -/// -/// # Verification Steps -/// -/// 1. Check that the number of signatures meets the threshold -/// 2. For each signature, verify that: -/// - The signer index is within bounds -/// - The ECDSA signature is valid for the corresponding public key -/// -/// # Returns -/// -/// * `Ok(())` if all signatures are valid and threshold is met -/// * `Err(ThresholdSigningError)` otherwise -pub fn verify_threshold_signatures( - config: &ThresholdConfig, - signatures: &SignatureSet, - message_hash: &[u8; 32], -) -> Result<(), ThresholdSigningError> { - // Check threshold is met - if signatures.len() < config.threshold() as usize { - return Err(ThresholdSigningError::InsufficientSignatures { - provided: signatures.len(), - required: config.threshold() as usize, - }); - } - - // Create the message for verification - let message = Message::from_digest_slice(message_hash) - .map_err(|_| ThresholdSigningError::InvalidMessageHash)?; - - // Verify each signature - for indexed_sig in signatures.signatures() { - // Check index is in bounds - let index = indexed_sig.index as usize; - if index >= config.keys().len() { - return Err(ThresholdSigningError::SignerIndexOutOfBounds { - index: indexed_sig.index, - max: config.keys().len(), - }); - } - - // Get the expected public key - let expected_pubkey = config.keys()[index].as_inner(); - - // Parse the recoverable signature - let recovery_id = RecoveryId::from_i32(indexed_sig.recovery_id() as i32) - .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; - - let recoverable_sig = - RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) - .map_err(|_| ThresholdSigningError::InvalidSignatureFormat)?; - - // Recover the public key from the signature - let recovered_pubkey = SECP256K1 - .recover_ecdsa(&message, &recoverable_sig) - .map_err(|_| ThresholdSigningError::InvalidSignature { - index: indexed_sig.index, - })?; - - // Verify the recovered key matches the expected key - if &recovered_pubkey != expected_pubkey { - return Err(ThresholdSigningError::InvalidSignature { - index: indexed_sig.index, - }); - } - } - - Ok(()) -} - -/// Sign a message hash with ECDSA and return a recoverable signature. -/// -/// This is a helper function for testing and creating signatures. -#[cfg(test)] -fn sign_ecdsa_recoverable(message_hash: &[u8; 32], secret_key: &secp256k1::SecretKey) -> [u8; 65] { - let message = Message::from_digest_slice(message_hash).expect("32 bytes"); - let sig = SECP256K1.sign_ecdsa_recoverable(&message, secret_key); - let (recovery_id, compact) = sig.serialize_compact(); - - let mut result = [0u8; 65]; - result[0] = recovery_id.to_i32() as u8; - result[1..65].copy_from_slice(&compact); - result -} - -#[cfg(test)] -mod tests { - use secp256k1::{Secp256k1, SecretKey}; - - use super::*; - use crate::threshold_signing::indexed_signatures::{CompressedPublicKey, IndexedSignature}; - - fn generate_keypair(seed: u8) -> (SecretKey, CompressedPublicKey) { - let secp = Secp256k1::new(); - let mut sk_bytes = [0u8; 32]; - sk_bytes[31] = seed.max(1); - let sk = SecretKey::from_slice(&sk_bytes).unwrap(); - let pk = CompressedPublicKey::from(secp256k1::PublicKey::from_secret_key(&secp, &sk)); - (sk, pk) - } - - #[test] - fn test_verify_threshold_signatures_success() { - let (sk1, pk1) = generate_keypair(1); - let (sk2, pk2) = generate_keypair(2); - let (_sk3, pk3) = generate_keypair(3); - - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); - - let message_hash = [0xAB; 32]; - - // Sign with keys 0 and 1 - let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); - let sig1 = sign_ecdsa_recoverable(&message_hash, &sk2); - - let signatures = SignatureSet::new(vec![ - IndexedSignature::new(0, sig0), - IndexedSignature::new(1, sig1), - ]) - .unwrap(); - - let result = verify_threshold_signatures(&config, &signatures, &message_hash); - assert!(result.is_ok()); - } - - #[test] - fn test_verify_insufficient_signatures() { - let (_sk1, pk1) = generate_keypair(1); - let (sk2, pk2) = generate_keypair(2); - let (_sk3, pk3) = generate_keypair(3); - - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); - - let message_hash = [0xAB; 32]; - - // Only sign with one key - let sig1 = sign_ecdsa_recoverable(&message_hash, &sk2); - - let signatures = SignatureSet::new(vec![IndexedSignature::new(1, sig1)]).unwrap(); - - let result = verify_threshold_signatures(&config, &signatures, &message_hash); - assert!(matches!( - result, - Err(ThresholdSigningError::InsufficientSignatures { .. }) - )); - } - - #[test] - fn test_verify_invalid_signature() { - let (sk1, pk1) = generate_keypair(1); - let (sk2, pk2) = generate_keypair(2); - - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); - - let message_hash = [0xAB; 32]; - let wrong_message_hash = [0xCD; 32]; - - // Sign with correct message - let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); - // Sign with wrong message - let sig1_wrong = sign_ecdsa_recoverable(&wrong_message_hash, &sk2); - - let signatures = SignatureSet::new(vec![ - IndexedSignature::new(0, sig0), - IndexedSignature::new(1, sig1_wrong), - ]) - .unwrap(); - - let result = verify_threshold_signatures(&config, &signatures, &message_hash); - assert!(matches!( - result, - Err(ThresholdSigningError::InvalidSignature { index: 1 }) - )); - } - - #[test] - fn test_verify_wrong_signer() { - let (sk1, pk1) = generate_keypair(1); - let (_sk2, pk2) = generate_keypair(2); - - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); - - let message_hash = [0xAB; 32]; - - // Both signatures from sk1, but one claims to be from index 1 - let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); - let sig1_from_wrong_key = sign_ecdsa_recoverable(&message_hash, &sk1); - - let signatures = SignatureSet::new(vec![ - IndexedSignature::new(0, sig0), - IndexedSignature::new(1, sig1_from_wrong_key), /* Claims to be key 1, but signed by - * key 0 */ - ]) - .unwrap(); - - let result = verify_threshold_signatures(&config, &signatures, &message_hash); - assert!(matches!( - result, - Err(ThresholdSigningError::InvalidSignature { index: 1 }) - )); - } - - #[test] - fn test_verify_index_out_of_bounds() { - let (sk1, pk1) = generate_keypair(1); - let (sk2, pk2) = generate_keypair(2); - - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); - - let message_hash = [0xAB; 32]; - - let sig0 = sign_ecdsa_recoverable(&message_hash, &sk1); - let sig_oob = sign_ecdsa_recoverable(&message_hash, &sk2); - - let signatures = SignatureSet::new(vec![ - IndexedSignature::new(0, sig0), - IndexedSignature::new(99, sig_oob), // Out of bounds - ]) - .unwrap(); - - let result = verify_threshold_signatures(&config, &signatures, &message_hash); - assert!(matches!( - result, - Err(ThresholdSigningError::SignerIndexOutOfBounds { index: 99, .. }) - )); - } -} diff --git a/crates/crypto/src/threshold_signing/mod.rs b/crates/crypto/src/threshold_signing/mod.rs deleted file mode 100644 index 4328b066c7..0000000000 --- a/crates/crypto/src/threshold_signing/mod.rs +++ /dev/null @@ -1,16 +0,0 @@ -//! Threshold signing module for multi-party signature schemes. -//! -//! This module provides two sub-modules: -//! - `musig2`: MuSig2 key aggregation for the bridge subprotocol (N-of-N Schnorr) -//! - `indexed_signatures`: Individual ECDSA signatures for the admin subprotocol (M-of-N threshold) - -pub mod indexed_signatures; -pub mod musig2; - -// Re-export commonly used types from indexed_signatures -pub use indexed_signatures::{ - verify_threshold_signatures, CompressedPublicKey, IndexedSignature, SignatureSet, - ThresholdConfig, ThresholdConfigUpdate, ThresholdSigningError, -}; -// Re-export MuSig2 key aggregation -pub use musig2::{aggregate_schnorr_keys, Musig2Error}; diff --git a/crates/l1tx/src/utils.rs b/crates/l1tx/src/utils.rs index 46bf7e66f7..424b7eb5a2 100644 --- a/crates/l1tx/src/utils.rs +++ b/crates/l1tx/src/utils.rs @@ -11,7 +11,7 @@ use strata_asm_txs_bridge_v1::{ withdrawal_fulfillment::WithdrawalFulfillmentInfo as BridgeV1WithdrawInfo, }; use strata_asm_types::{DepositInfo, WithdrawalFulfillmentInfo}; -use strata_crypto::threshold_signing::aggregate_schnorr_keys; +use strata_crypto::threshold_signature::aggregate_schnorr_keys; use strata_params::{OperatorConfig, RollupParams}; use strata_primitives::{buf::Buf32, l1::BitcoinAddress}; From 3124d6552cb6f4717d6957c90b906be6abf1dc44 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 00:51:15 +0400 Subject: [PATCH 04/11] fix(crypto): use NonZero for threshold, encapsulate signature fields, and fix zkvm serde --- .../src/threshold_signature/indexed/config.rs | 44 ++++++++++++++----- .../src/threshold_signature/indexed/errors.rs | 9 ++-- .../src/threshold_signature/indexed/pubkey.rs | 16 +++++-- .../threshold_signature/indexed/signature.rs | 25 +++++++---- .../indexed/verification/ecdsa.rs | 8 ++-- 5 files changed, 73 insertions(+), 29 deletions(-) diff --git a/crates/crypto/src/threshold_signature/indexed/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs index 16a72e07a6..79786d435f 100644 --- a/crates/crypto/src/threshold_signature/indexed/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -1,6 +1,6 @@ //! Configuration types for threshold signing. -use std::collections::HashSet; +use std::{collections::HashSet, num::NonZero}; use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; @@ -10,12 +10,14 @@ use super::{CompressedPublicKey, ThresholdSignatureError}; /// Configuration for a threshold signature authority. /// /// Defines who can sign (`keys`) and how many must sign (`threshold`). -#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] +/// The threshold is stored as `NonZero` to enforce at the type level +/// that it can never be zero. +#[derive(Debug, Clone, PartialEq, Eq)] pub struct ThresholdConfig { /// Public keys of all authorized signers. keys: Vec, - /// Minimum number of signatures required. - threshold: u8, + /// Minimum number of signatures required (always >= 1). + threshold: NonZero, } impl ThresholdConfig { @@ -31,11 +33,14 @@ impl ThresholdConfig { keys: Vec, threshold: u8, ) -> Result { + // Validate threshold is non-zero + let threshold = NonZero::new(threshold).ok_or(ThresholdSignatureError::ZeroThreshold)?; + let mut config = ThresholdConfig { keys: vec![], - threshold: 0, + threshold, }; - let update = ThresholdConfigUpdate::new(keys, vec![], threshold); + let update = ThresholdConfigUpdate::new(keys, vec![], threshold.get()); config.apply_update(&update)?; Ok(config) } @@ -45,9 +50,9 @@ impl ThresholdConfig { &self.keys } - /// Get the threshold. + /// Get the threshold value. pub fn threshold(&self) -> u8 { - self.threshold + self.threshold.get() } /// Get the number of authorized signers. @@ -125,13 +130,32 @@ impl ThresholdConfig { // Add new members self.keys.extend_from_slice(update.add_members()); - // Update threshold - self.threshold = update.new_threshold(); + // Update threshold - safe because validate_update already checked it's non-zero + self.threshold = + NonZero::new(update.new_threshold()).expect("validate_update ensures non-zero"); Ok(()) } } +impl BorshSerialize for ThresholdConfig { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + self.keys.serialize(writer)?; + self.threshold.get().serialize(writer) + } +} + +impl BorshDeserialize for ThresholdConfig { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let keys = Vec::::deserialize_reader(reader)?; + let threshold_u8 = u8::deserialize_reader(reader)?; + let threshold = NonZero::new(threshold_u8).ok_or_else(|| { + std::io::Error::new(std::io::ErrorKind::InvalidData, "threshold cannot be zero") + })?; + Ok(Self { keys, threshold }) + } +} + impl std::hash::Hash for CompressedPublicKey { fn hash(&self, state: &mut H) { self.serialize().hash(state); diff --git a/crates/crypto/src/threshold_signature/indexed/errors.rs b/crates/crypto/src/threshold_signature/indexed/errors.rs index 9bcf10bc2d..02dd50f108 100644 --- a/crates/crypto/src/threshold_signature/indexed/errors.rs +++ b/crates/crypto/src/threshold_signature/indexed/errors.rs @@ -10,8 +10,11 @@ pub enum ThresholdSignatureError { InsufficientSignatures { provided: usize, required: usize }, /// Invalid public key data. - #[error("invalid public key at index {index}: {reason}")] - InvalidPublicKey { index: usize, reason: String }, + #[error("invalid public key{}: {reason}", index.map(|i| format!(" at index {}", i)).unwrap_or_default())] + InvalidPublicKey { + index: Option, + reason: String, + }, /// Invalid threshold value. #[error("invalid threshold: {threshold} exceeds total keys {total_keys}")] @@ -61,7 +64,7 @@ pub enum ThresholdSignatureError { impl From for ThresholdSignatureError { fn from(e: secp256k1::Error) -> Self { Self::InvalidPublicKey { - index: 0, + index: None, reason: e.to_string(), } } diff --git a/crates/crypto/src/threshold_signature/indexed/pubkey.rs b/crates/crypto/src/threshold_signature/indexed/pubkey.rs index 64cc4daad8..bf423474bc 100644 --- a/crates/crypto/src/threshold_signature/indexed/pubkey.rs +++ b/crates/crypto/src/threshold_signature/indexed/pubkey.rs @@ -5,7 +5,7 @@ use std::ops::Deref; use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; use secp256k1::{PublicKey, Secp256k1, SecretKey}; -#[cfg(feature = "serde")] +#[cfg(all(feature = "serde", not(target_os = "zkvm")))] use serde::{Deserialize, Serialize}; use super::ThresholdSignatureError; @@ -15,6 +15,14 @@ use super::ThresholdSignatureError; /// This is a thin wrapper around `secp256k1::PublicKey` that adds Borsh /// serialization support. Unlike `EvenPublicKey`, this type does not /// enforce even parity - it accepts any valid compressed public key. +/// +/// **Why no parity enforcement?** This key is used for ECDSA signature +/// verification (not Schnorr/BIP340). ECDSA signatures work with both +/// even and odd parity keys, unlike Schnorr which requires even parity +/// for x-only public keys. +/// +/// Serializes the key as a 33-byte compressed point where the first byte +/// indicates the y-coordinate parity (0x02 for even, 0x03 for odd). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct CompressedPublicKey(PublicKey); @@ -25,7 +33,7 @@ impl CompressedPublicKey { pub fn from_slice(data: &[u8]) -> Result { let pk = PublicKey::from_slice(data).map_err(|e| ThresholdSignatureError::InvalidPublicKey { - index: 0, + index: None, reason: e.to_string(), })?; Ok(Self(pk)) @@ -101,7 +109,7 @@ impl BorshDeserialize for CompressedPublicKey { } } -#[cfg(feature = "serde")] +#[cfg(all(feature = "serde", not(target_os = "zkvm")))] impl Serialize for CompressedPublicKey { fn serialize(&self, serializer: S) -> Result where @@ -113,7 +121,7 @@ impl Serialize for CompressedPublicKey { } } -#[cfg(feature = "serde")] +#[cfg(all(feature = "serde", not(target_os = "zkvm")))] impl<'de> Deserialize<'de> for CompressedPublicKey { fn deserialize(deserializer: D) -> Result where diff --git a/crates/crypto/src/threshold_signature/indexed/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs index 32864f9687..dccc6449d4 100644 --- a/crates/crypto/src/threshold_signature/indexed/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -11,10 +11,10 @@ use super::ThresholdSignatureError; #[derive(Debug, Clone, PartialEq, Eq)] pub struct IndexedSignature { /// Index of the signer in the ThresholdConfig keys array (0-255). - pub index: u8, + index: u8, /// 65-byte recoverable ECDSA signature (recovery_id || r || s). /// Using recoverable format for hardware wallet compatibility (Ledger/Trezor native output). - pub signature: [u8; 65], + signature: [u8; 65], } impl IndexedSignature { @@ -23,6 +23,11 @@ impl IndexedSignature { Self { index, signature } } + /// Get the signer index. + pub fn index(&self) -> u8 { + self.index + } + /// Get the recovery ID (first byte of the signature). pub fn recovery_id(&self) -> u8 { self.signature[0] @@ -30,12 +35,16 @@ impl IndexedSignature { /// Get the r component (bytes 1-32). pub fn r(&self) -> &[u8; 32] { - self.signature[1..33].try_into().unwrap() + self.signature[1..33] + .try_into() + .expect("signature[1..33] is always 32 bytes") } /// Get the s component (bytes 33-64). pub fn s(&self) -> &[u8; 32] { - self.signature[33..65].try_into().unwrap() + self.signature[33..65] + .try_into() + .expect("signature[33..65] is always 32 bytes") } /// Get the compact signature (r || s) without recovery ID. @@ -166,9 +175,9 @@ mod tests { let set = SignatureSet::new(sigs).unwrap(); // Should be sorted - assert_eq!(set.signatures()[0].index, 0); - assert_eq!(set.signatures()[1].index, 1); - assert_eq!(set.signatures()[2].index, 2); + assert_eq!(set.signatures()[0].index(), 0); + assert_eq!(set.signatures()[1].index(), 1); + assert_eq!(set.signatures()[2].index(), 2); } #[test] @@ -201,7 +210,7 @@ mod tests { let sig = IndexedSignature::new(5, signature); - assert_eq!(sig.index, 5); + assert_eq!(sig.index(), 5); assert_eq!(sig.recovery_id(), 27); assert_eq!(sig.r(), &[0xAA; 32]); assert_eq!(sig.s(), &[0xBB; 32]); diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index b20ce31eb8..4b240556dc 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -23,10 +23,10 @@ pub(super) fn verify_ecdsa_signatures( // Verify each signature for indexed_sig in signatures.signatures() { // Check index is in bounds - let index = indexed_sig.index as usize; + let index = indexed_sig.index() as usize; if index >= config.keys().len() { return Err(ThresholdSignatureError::SignerIndexOutOfBounds { - index: indexed_sig.index, + index: indexed_sig.index(), max: config.keys().len(), }); } @@ -46,13 +46,13 @@ pub(super) fn verify_ecdsa_signatures( let recovered_pubkey = SECP256K1 .recover_ecdsa(&message, &recoverable_sig) .map_err(|_| ThresholdSignatureError::InvalidSignature { - index: indexed_sig.index, + index: indexed_sig.index(), })?; // Verify the recovered key matches the expected key if &recovered_pubkey != expected_pubkey { return Err(ThresholdSignatureError::InvalidSignature { - index: indexed_sig.index, + index: indexed_sig.index(), }); } } From 52c2955d28d07b290170bdb9ca9bacc914815889 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 14:33:34 +0400 Subject: [PATCH 05/11] fix(crypto): add BIP-137 recovery ID normalization for hardware wallet compatibility --- crates/asm/subprotocols/admin/src/error.rs | 4 +- crates/asm/subprotocols/admin/src/handler.rs | 4 +- .../src/threshold_signature/indexed/config.rs | 19 +++ .../src/threshold_signature/indexed/mod.rs | 2 +- .../threshold_signature/indexed/signature.rs | 52 ++++++-- .../indexed/verification.rs | 65 ++++++++++ .../indexed/verification/ecdsa.rs | 112 +++++++++++++++++- 7 files changed, 244 insertions(+), 14 deletions(-) diff --git a/crates/asm/subprotocols/admin/src/error.rs b/crates/asm/subprotocols/admin/src/error.rs index 1e4c188dfc..51b5b97082 100644 --- a/crates/asm/subprotocols/admin/src/error.rs +++ b/crates/asm/subprotocols/admin/src/error.rs @@ -13,7 +13,7 @@ pub enum AdministrationError { #[error("no pending update found for action_id = {0:?}")] UnknownAction(UpdateId), - /// Indicates a threshold signing error (configuration or signature validation). + /// Indicates a threshold signature error (configuration or signature validation). #[error(transparent)] - ThresholdSigning(#[from] ThresholdSignatureError), + ThresholdSignature(#[from] ThresholdSignatureError), } diff --git a/crates/asm/subprotocols/admin/src/handler.rs b/crates/asm/subprotocols/admin/src/handler.rs index 81a01dcf9e..4600772d37 100644 --- a/crates/asm/subprotocols/admin/src/handler.rs +++ b/crates/asm/subprotocols/admin/src/handler.rs @@ -386,7 +386,7 @@ mod tests { assert!(res.is_err()); assert!(matches!( res, - Err(AdministrationError::ThresholdSigning( + Err(AdministrationError::ThresholdSignature( ThresholdSignatureError::InvalidSignature { .. } )) )); @@ -406,7 +406,7 @@ mod tests { ); assert!(matches!( res, - Err(AdministrationError::ThresholdSigning( + Err(AdministrationError::ThresholdSignature( ThresholdSignatureError::InvalidSignature { .. } )) )); diff --git a/crates/crypto/src/threshold_signature/indexed/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs index 79786d435f..643a823be0 100644 --- a/crates/crypto/src/threshold_signature/indexed/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -7,6 +7,12 @@ use borsh::{BorshDeserialize, BorshSerialize}; use super::{CompressedPublicKey, ThresholdSignatureError}; +/// Maximum number of signers allowed in a threshold configuration. +/// +/// This limit is derived from the signer index being a `u8` (0-255), +/// which allows for at most 256 unique signers. +pub const MAX_SIGNERS: usize = 256; + /// Configuration for a threshold signature authority. /// /// Defines who can sign (`keys`) and how many must sign (`threshold`). @@ -148,6 +154,19 @@ impl BorshSerialize for ThresholdConfig { impl BorshDeserialize for ThresholdConfig { fn deserialize_reader(reader: &mut R) -> std::io::Result { let keys = Vec::::deserialize_reader(reader)?; + + // Validate key count doesn't exceed maximum (prevents DoS via large allocations) + if keys.len() > MAX_SIGNERS { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "too many signers: {} exceeds maximum {}", + keys.len(), + MAX_SIGNERS + ), + )); + } + let threshold_u8 = u8::deserialize_reader(reader)?; let threshold = NonZero::new(threshold_u8).ok_or_else(|| { std::io::Error::new(std::io::ErrorKind::InvalidData, "threshold cannot be zero") diff --git a/crates/crypto/src/threshold_signature/indexed/mod.rs b/crates/crypto/src/threshold_signature/indexed/mod.rs index ada6ef39d5..02429834c8 100644 --- a/crates/crypto/src/threshold_signature/indexed/mod.rs +++ b/crates/crypto/src/threshold_signature/indexed/mod.rs @@ -10,7 +10,7 @@ mod pubkey; mod signature; mod verification; -pub use config::{ThresholdConfig, ThresholdConfigUpdate}; +pub use config::{ThresholdConfig, ThresholdConfigUpdate, MAX_SIGNERS}; pub use errors::ThresholdSignatureError; pub use pubkey::CompressedPublicKey; pub use signature::{IndexedSignature, SignatureSet}; diff --git a/crates/crypto/src/threshold_signature/indexed/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs index dccc6449d4..ab5805a7e4 100644 --- a/crates/crypto/src/threshold_signature/indexed/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -6,14 +6,28 @@ use super::ThresholdSignatureError; /// An individual ECDSA signature with its signer index. /// -/// The signature is in recoverable format (65 bytes): `recovery_id || r || s`. -/// This format is used for hardware wallet compatibility (Ledger/Trezor native output). +/// The signature is in recoverable format (65 bytes): `header || r || s`. +/// +/// # Hardware Wallet Compatibility +/// +/// The first byte (header) can be in two formats: +/// +/// 1. **Raw recovery ID** (0-3): Used by some signing libraries +/// 2. **BIP-137 format** (27-42): Used by Bitcoin message signing in hardware wallets +/// - 27-30: Uncompressed P2PKH +/// - 31-34: Compressed P2PKH (most common for Ledger/Trezor) +/// - 35-38: SegWit P2SH-P2WPKH +/// - 39-42: Native SegWit P2WPKH +/// +/// The verification code normalizes both formats to extract the raw recovery ID (0-3). #[derive(Debug, Clone, PartialEq, Eq)] pub struct IndexedSignature { /// Index of the signer in the ThresholdConfig keys array (0-255). index: u8, - /// 65-byte recoverable ECDSA signature (recovery_id || r || s). - /// Using recoverable format for hardware wallet compatibility (Ledger/Trezor native output). + /// 65-byte recoverable ECDSA signature (header || r || s). + /// + /// The header byte contains the recovery ID, possibly with BIP-137 address type encoding. + /// See struct-level documentation for format details. signature: [u8; 65], } @@ -28,7 +42,14 @@ impl IndexedSignature { self.index } - /// Get the recovery ID (first byte of the signature). + /// Get the header byte (first byte of the signature). + /// + /// This byte contains the recovery ID, possibly encoded in BIP-137 format. + /// The verification code handles normalization automatically. + /// + /// # Format + /// - Raw: 0-3 (recovery ID directly) + /// - BIP-137: 27-42 (encodes address type + recovery ID) pub fn recovery_id(&self) -> u8 { self.signature[0] } @@ -204,15 +225,32 @@ mod tests { #[test] fn test_indexed_signature_components() { let mut signature = [0u8; 65]; - signature[0] = 27; // recovery id + // BIP-137 format: 27 = uncompressed P2PKH with recid 0 + signature[0] = 27; signature[1..33].copy_from_slice(&[0xAA; 32]); // r signature[33..65].copy_from_slice(&[0xBB; 32]); // s let sig = IndexedSignature::new(5, signature); assert_eq!(sig.index(), 5); - assert_eq!(sig.recovery_id(), 27); + assert_eq!(sig.recovery_id(), 27); // Raw header byte (verification normalizes this) assert_eq!(sig.r(), &[0xAA; 32]); assert_eq!(sig.s(), &[0xBB; 32]); } + + #[test] + fn test_indexed_signature_raw_recid() { + let mut signature = [0u8; 65]; + // Raw format: recid 1 directly + signature[0] = 1; + signature[1..33].copy_from_slice(&[0xCC; 32]); // r + signature[33..65].copy_from_slice(&[0xDD; 32]); // s + + let sig = IndexedSignature::new(3, signature); + + assert_eq!(sig.index(), 3); + assert_eq!(sig.recovery_id(), 1); // Raw recovery ID + assert_eq!(sig.r(), &[0xCC; 32]); + assert_eq!(sig.s(), &[0xDD; 32]); + } } diff --git a/crates/crypto/src/threshold_signature/indexed/verification.rs b/crates/crypto/src/threshold_signature/indexed/verification.rs index fba9cfd0b9..f8dbc535c6 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification.rs @@ -206,4 +206,69 @@ mod tests { Err(ThresholdSignatureError::DuplicateSignerIndex(0)) )); } + + #[test] + fn test_verify_bip137_format_signatures() { + // Test that BIP-137 format signatures (as produced by hardware wallets) work + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Sign with BIP-137 format (compressed P2PKH, header 31-34) + let sig0 = ecdsa::sign_ecdsa_bip137(&message_hash, &sk1); + let sig1 = ecdsa::sign_ecdsa_bip137(&message_hash, &sk2); + + // Verify the header bytes are in BIP-137 range + assert!(sig0[0] >= 31 && sig0[0] <= 34); + assert!(sig1[0] >= 31 && sig1[0] <= 34); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1), + ]; + + // Should still verify successfully with BIP-137 format + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!( + result.is_ok(), + "BIP-137 format signatures should verify successfully" + ); + } + + #[test] + fn test_verify_mixed_format_signatures() { + // Test that a mix of raw and BIP-137 format signatures work together + let (sk1, pk1) = generate_keypair(1); + let (sk2, pk2) = generate_keypair(2); + let (_sk3, pk3) = generate_keypair(3); + + let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + + let message_hash = [0xAB; 32]; + + // Sign with raw format + let sig0 = ecdsa::sign_ecdsa_recoverable(&message_hash, &sk1); + // Sign with BIP-137 format + let sig1 = ecdsa::sign_ecdsa_bip137(&message_hash, &sk2); + + // Verify the formats are different + assert!(sig0[0] <= 3, "sig0 should be raw format"); + assert!(sig1[0] >= 31, "sig1 should be BIP-137 format"); + + let signatures = vec![ + IndexedSignature::new(0, sig0), + IndexedSignature::new(1, sig1), + ]; + + // Should still verify successfully with mixed formats + let result = verify_threshold_signatures(&config, &signatures, &message_hash); + assert!( + result.is_ok(), + "Mixed format signatures should verify successfully" + ); + } } diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index 4b240556dc..3232866258 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -7,10 +7,41 @@ use secp256k1::{ use crate::threshold_signature::indexed::{SignatureSet, ThresholdConfig, ThresholdSignatureError}; +/// Normalizes a recovery ID header byte to the raw recovery ID (0-3). +/// +/// Hardware wallets (Ledger/Trezor) follow BIP-137 and encode additional address type +/// information in the header byte. This function extracts just the recovery ID needed +/// for ECDSA public key recovery. +/// +/// # Header byte formats (BIP-137): +/// - `0-3`: Raw recovery ID (already normalized) +/// - `27-30`: Uncompressed P2PKH (subtract 27) +/// - `31-34`: Compressed P2PKH (subtract 31) +/// - `35-38`: SegWit P2SH-P2WPKH (subtract 35) +/// - `39-42`: Native SegWit P2WPKH (subtract 39) +/// +/// # Returns +/// The raw recovery ID (0-3) or an error if the header is invalid. +fn normalize_recovery_id(header: u8) -> Result { + let recid = match header { + 0..=3 => header, // Raw format + 27..=30 => header - 27, // Uncompressed P2PKH + 31..=34 => header - 31, // Compressed P2PKH + 35..=38 => header - 35, // SegWit P2SH + 39..=42 => header - 39, // Native SegWit + _ => return Err(ThresholdSignatureError::InvalidSignatureFormat), + }; + Ok(recid as i32) +} + /// Verifies each ECDSA signature in the set against the corresponding public key. /// /// This function performs the actual ECDSA signature recovery and verification. /// It assumes the SignatureSet has already been validated for duplicates. +/// +/// # Hardware Wallet Compatibility +/// Supports signatures from hardware wallets (Ledger/Trezor) that use BIP-137 format +/// with header bytes 27-42, as well as raw format with recovery ID 0-3. pub(super) fn verify_ecdsa_signatures( config: &ThresholdConfig, signatures: &SignatureSet, @@ -34,8 +65,9 @@ pub(super) fn verify_ecdsa_signatures( // Get the expected public key let expected_pubkey = config.keys()[index].as_inner(); - // Parse the recoverable signature - let recovery_id = RecoveryId::from_i32(indexed_sig.recovery_id() as i32) + // Normalize the recovery ID from BIP-137 header format to raw 0-3 + let recid_raw = normalize_recovery_id(indexed_sig.recovery_id())?; + let recovery_id = RecoveryId::from_i32(recid_raw) .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?; let recoverable_sig = @@ -63,6 +95,7 @@ pub(super) fn verify_ecdsa_signatures( /// Sign a message hash with ECDSA and return a recoverable signature. /// /// This is a helper function for testing and creating signatures. +/// Returns raw format (recovery_id 0-3 in first byte). #[cfg(test)] pub(super) fn sign_ecdsa_recoverable( message_hash: &[u8; 32], @@ -77,3 +110,78 @@ pub(super) fn sign_ecdsa_recoverable( result[1..65].copy_from_slice(&compact); result } + +/// Sign a message hash with ECDSA and return a BIP-137 format signature. +/// +/// This simulates hardware wallet output with compressed P2PKH format (header 31-34). +#[cfg(test)] +pub(super) fn sign_ecdsa_bip137( + message_hash: &[u8; 32], + secret_key: &secp256k1::SecretKey, +) -> [u8; 65] { + let mut sig = sign_ecdsa_recoverable(message_hash, secret_key); + // Convert raw recid (0-3) to BIP-137 compressed P2PKH format (31-34) + sig[0] += 31; + sig +} + +#[cfg(test)] +mod normalization_tests { + use super::*; + + #[test] + fn test_normalize_raw_recovery_id() { + assert_eq!(normalize_recovery_id(0).unwrap(), 0); + assert_eq!(normalize_recovery_id(1).unwrap(), 1); + assert_eq!(normalize_recovery_id(2).unwrap(), 2); + assert_eq!(normalize_recovery_id(3).unwrap(), 3); + } + + #[test] + fn test_normalize_bip137_uncompressed_p2pkh() { + // 27-30 = uncompressed P2PKH + assert_eq!(normalize_recovery_id(27).unwrap(), 0); + assert_eq!(normalize_recovery_id(28).unwrap(), 1); + assert_eq!(normalize_recovery_id(29).unwrap(), 2); + assert_eq!(normalize_recovery_id(30).unwrap(), 3); + } + + #[test] + fn test_normalize_bip137_compressed_p2pkh() { + // 31-34 = compressed P2PKH (most common for Ledger/Trezor) + assert_eq!(normalize_recovery_id(31).unwrap(), 0); + assert_eq!(normalize_recovery_id(32).unwrap(), 1); + assert_eq!(normalize_recovery_id(33).unwrap(), 2); + assert_eq!(normalize_recovery_id(34).unwrap(), 3); + } + + #[test] + fn test_normalize_bip137_segwit_p2sh() { + // 35-38 = SegWit P2SH-P2WPKH + assert_eq!(normalize_recovery_id(35).unwrap(), 0); + assert_eq!(normalize_recovery_id(36).unwrap(), 1); + assert_eq!(normalize_recovery_id(37).unwrap(), 2); + assert_eq!(normalize_recovery_id(38).unwrap(), 3); + } + + #[test] + fn test_normalize_bip137_native_segwit() { + // 39-42 = Native SegWit P2WPKH + assert_eq!(normalize_recovery_id(39).unwrap(), 0); + assert_eq!(normalize_recovery_id(40).unwrap(), 1); + assert_eq!(normalize_recovery_id(41).unwrap(), 2); + assert_eq!(normalize_recovery_id(42).unwrap(), 3); + } + + #[test] + fn test_normalize_invalid_values() { + // Values between 4 and 26 are invalid + for v in 4..27 { + assert!(normalize_recovery_id(v).is_err()); + } + // Values above 42 are invalid + for v in 43..=255 { + assert!(normalize_recovery_id(v).is_err()); + } + } +} From 5cf0e391be1c6d3c34a22650ab97adf471cd97b8 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 18:21:25 +0400 Subject: [PATCH 06/11] fix(crypto): address PR review comments for threshold signature refactoring --- crates/asm/subprotocols/admin/src/authority.rs | 8 ++++---- .../crypto/src/threshold_signature/indexed/config.rs | 10 ++++++++-- .../crypto/src/threshold_signature/indexed/pubkey.rs | 4 ++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/asm/subprotocols/admin/src/authority.rs b/crates/asm/subprotocols/admin/src/authority.rs index 2e7f2adbf9..86192d5e55 100644 --- a/crates/asm/subprotocols/admin/src/authority.rs +++ b/crates/asm/subprotocols/admin/src/authority.rs @@ -42,10 +42,11 @@ impl MultisigAuthority { &mut self.config } - /// Verify that `signatures` is a valid threshold signature set for `action` under the current - /// config and seqno. + /// Verifies a set of ECDSA signatures against a threshold configuration. /// - /// Uses individual ECDSA signature verification against each signer's public key. + /// This function is intentionally ECDSA-specific as part of the hardware wallet + /// compatibility design (BIP-137 format support). A trait-based abstraction + /// could be added in the future if multiple signature schemes are needed. pub fn verify_action_signature( &self, action: &MultisigAction, @@ -54,7 +55,6 @@ impl MultisigAuthority { // Compute the msg to sign by combining UpdateAction with sequence no let sig_hash = action.compute_sighash(self.seqno); - // Verify each ECDSA signature against the corresponding public key verify_threshold_signatures(&self.config, signatures.signatures(), &sig_hash.into()) } diff --git a/crates/crypto/src/threshold_signature/indexed/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs index 643a823be0..9a87b0e6d2 100644 --- a/crates/crypto/src/threshold_signature/indexed/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -1,6 +1,6 @@ //! Configuration types for threshold signing. -use std::{collections::HashSet, num::NonZero}; +use std::{collections::HashSet, hash::Hash, num::NonZero}; use arbitrary::Arbitrary; use borsh::{BorshDeserialize, BorshSerialize}; @@ -72,6 +72,12 @@ impl ThresholdConfig { } /// Validates that an update can be applied to this configuration. + /// + /// # Note + /// + /// This method is called automatically by [`apply_update`]. External callers + /// may use this for dry-run validation, but there's no need to call it + /// before `apply_update` as validation is always performed. pub fn validate_update( &self, update: &ThresholdConfigUpdate, @@ -175,7 +181,7 @@ impl BorshDeserialize for ThresholdConfig { } } -impl std::hash::Hash for CompressedPublicKey { +impl Hash for CompressedPublicKey { fn hash(&self, state: &mut H) { self.serialize().hash(state); } diff --git a/crates/crypto/src/threshold_signature/indexed/pubkey.rs b/crates/crypto/src/threshold_signature/indexed/pubkey.rs index bf423474bc..b98411632e 100644 --- a/crates/crypto/src/threshold_signature/indexed/pubkey.rs +++ b/crates/crypto/src/threshold_signature/indexed/pubkey.rs @@ -45,6 +45,10 @@ impl CompressedPublicKey { } /// Serialize to 33-byte compressed format. + /// + /// Serializes the key as a byte-encoded pair of values. In compressed form + /// the y-coordinate is represented by only a single bit, as x determines + /// it up to one bit. pub fn serialize(&self) -> [u8; 33] { self.0.serialize() } From 8ace955de9eed891512c414575e9527363b9fdd5 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 18:50:36 +0400 Subject: [PATCH 07/11] fix(crypto): use NonZero for threshold --- crates/asm/subprotocols/admin/src/handler.rs | 8 +- crates/asm/subprotocols/admin/src/state.rs | 12 ++- crates/asm/txs/admin/src/test_utils/mod.rs | 6 +- .../src/threshold_signature/indexed/config.rs | 73 +++++++++---------- .../src/threshold_signature/indexed/errors.rs | 4 - .../indexed/verification.rs | 22 ++++-- .../indexed/verification/ecdsa.rs | 2 +- 7 files changed, 65 insertions(+), 62 deletions(-) diff --git a/crates/asm/subprotocols/admin/src/handler.rs b/crates/asm/subprotocols/admin/src/handler.rs index 4600772d37..2a2538ffe4 100644 --- a/crates/asm/subprotocols/admin/src/handler.rs +++ b/crates/asm/subprotocols/admin/src/handler.rs @@ -175,7 +175,7 @@ fn relay_checkpoint_predicate(relayer: &mut impl MsgRelayer, key: PredicateKey) #[cfg(test)] mod tests { - use std::any::Any; + use std::{any::Any, num::NonZero}; use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use rand::{rngs::OsRng, seq::SliceRandom, thread_rng}; @@ -246,7 +246,8 @@ mod tests { .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_administrator = ThresholdConfig::try_new(strata_admin_pks, 2).unwrap(); + let strata_administrator = + ThresholdConfig::try_new(strata_admin_pks, NonZero::new(2).unwrap()).unwrap(); let strata_seq_manager_sks: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); @@ -254,7 +255,8 @@ mod tests { .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_sequencer_manager = ThresholdConfig::try_new(strata_seq_manager_pks, 2).unwrap(); + let strata_sequencer_manager = + ThresholdConfig::try_new(strata_seq_manager_pks, NonZero::new(2).unwrap()).unwrap(); let config = AdministrationSubprotoParams { strata_administrator, diff --git a/crates/asm/subprotocols/admin/src/state.rs b/crates/asm/subprotocols/admin/src/state.rs index ea2853fb5f..9a802c8c91 100644 --- a/crates/asm/subprotocols/admin/src/state.rs +++ b/crates/asm/subprotocols/admin/src/state.rs @@ -110,6 +110,8 @@ impl AdministrationSubprotoState { #[cfg(test)] mod tests { + use std::num::NonZero; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use rand::rngs::OsRng; use strata_asm_txs_admin::actions::UpdateAction; @@ -133,7 +135,8 @@ mod tests { .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_administrator = ThresholdConfig::try_new(admin_pks, 2).unwrap(); + let strata_administrator = + ThresholdConfig::try_new(admin_pks, NonZero::new(2).unwrap()).unwrap(); // Create seq manager keys let seq_sks: Vec = (0..3).map(|_| SecretKey::new(&mut OsRng)).collect(); @@ -141,7 +144,8 @@ mod tests { .iter() .map(|sk| CompressedPublicKey::from(PublicKey::from_secret_key(&secp, sk))) .collect(); - let strata_sequencer_manager = ThresholdConfig::try_new(seq_pks, 2).unwrap(); + let strata_sequencer_manager = + ThresholdConfig::try_new(seq_pks, NonZero::new(2).unwrap()).unwrap(); AdministrationSubprotoParams { strata_administrator, @@ -270,7 +274,7 @@ mod tests { let remove_members = vec![initial_members[0]]; let new_size = initial_members.len() + add_members.len() - remove_members.len(); - let new_threshold = 2u8; + let new_threshold = NonZero::new(2).unwrap(); let update = ThresholdConfigUpdate::new(add_members.clone(), remove_members.clone(), new_threshold); @@ -280,7 +284,7 @@ mod tests { let updated_auth = state.authority(role).unwrap().config(); // Verify threshold was updated - assert_eq!(updated_auth.threshold(), new_threshold); + assert_eq!(updated_auth.threshold(), new_threshold.get()); // Verify size is correct assert_eq!(updated_auth.keys().len(), new_size); diff --git a/crates/asm/txs/admin/src/test_utils/mod.rs b/crates/asm/txs/admin/src/test_utils/mod.rs index 91c79c9ffa..2abf1f1c68 100644 --- a/crates/asm/txs/admin/src/test_utils/mod.rs +++ b/crates/asm/txs/admin/src/test_utils/mod.rs @@ -189,6 +189,8 @@ fn build_envelope_script(payload: &[u8]) -> ScriptBuf { #[cfg(test)] mod tests { + use std::num::NonZero; + use bitcoin::secp256k1::{PublicKey, Secp256k1}; use rand::rngs::OsRng; use strata_asm_common::TxInputRef; @@ -205,7 +207,7 @@ mod tests { fn test_create_signature_set() { let mut arb = ArbitraryGenerator::new(); let seqno = 1; - let threshold = 2; + let threshold = NonZero::new(2).unwrap(); let secp = Secp256k1::new(); // Generate test private keys @@ -239,7 +241,7 @@ mod tests { fn test_admin_tx() { let mut arb = ArbitraryGenerator::new(); let seqno = 1; - let threshold = 2; + let threshold = NonZero::new(2).unwrap(); let secp = Secp256k1::new(); // Generate test private keys diff --git a/crates/crypto/src/threshold_signature/indexed/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs index 9a87b0e6d2..690c304ef1 100644 --- a/crates/crypto/src/threshold_signature/indexed/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -33,20 +33,16 @@ impl ThresholdConfig { /// /// Returns `ThresholdSignatureError` if: /// - `DuplicateAddMember`: The keys list contains duplicate members - /// - `ZeroThreshold`: The threshold is zero /// - `InvalidThreshold`: The threshold exceeds the total number of keys pub fn try_new( keys: Vec, - threshold: u8, + threshold: NonZero, ) -> Result { - // Validate threshold is non-zero - let threshold = NonZero::new(threshold).ok_or(ThresholdSignatureError::ZeroThreshold)?; - let mut config = ThresholdConfig { keys: vec![], threshold, }; - let update = ThresholdConfigUpdate::new(keys, vec![], threshold.get()); + let update = ThresholdConfigUpdate::new(keys, vec![], threshold); config.apply_update(&update)?; Ok(config) } @@ -102,11 +98,6 @@ impl ThresholdConfig { return Err(ThresholdSignatureError::MemberAlreadyExists); } - // Ensure new threshold is not zero - if update.new_threshold() == 0 { - return Err(ThresholdSignatureError::ZeroThreshold); - } - // Ensure all members to remove exist in current configuration for member_to_remove in update.remove_members() { if !self.keys.contains(member_to_remove) { @@ -118,9 +109,9 @@ impl ThresholdConfig { let updated_size = self.keys.len() + update.add_members().len() - update.remove_members().len(); - if (update.new_threshold() as usize) > updated_size { + if (update.new_threshold().get() as usize) > updated_size { return Err(ThresholdSignatureError::InvalidThreshold { - threshold: update.new_threshold(), + threshold: update.new_threshold().get(), total_keys: updated_size, }); } @@ -142,9 +133,8 @@ impl ThresholdConfig { // Add new members self.keys.extend_from_slice(update.add_members()); - // Update threshold - safe because validate_update already checked it's non-zero - self.threshold = - NonZero::new(update.new_threshold()).expect("validate_update ensures non-zero"); + // Update threshold + self.threshold = update.new_threshold(); Ok(()) } @@ -192,7 +182,7 @@ impl Hash for CompressedPublicKey { pub struct ThresholdConfigUpdate { add_members: Vec, remove_members: Vec, - new_threshold: u8, + new_threshold: NonZero, } impl ThresholdConfigUpdate { @@ -200,7 +190,7 @@ impl ThresholdConfigUpdate { pub fn new( add_members: Vec, remove_members: Vec, - new_threshold: u8, + new_threshold: NonZero, ) -> Self { Self { add_members, @@ -220,12 +210,18 @@ impl ThresholdConfigUpdate { } /// Returns the new threshold. - pub fn new_threshold(&self) -> u8 { + pub fn new_threshold(&self) -> NonZero { self.new_threshold } /// Consume and return the inner components. - pub fn into_inner(self) -> (Vec, Vec, u8) { + pub fn into_inner( + self, + ) -> ( + Vec, + Vec, + NonZero, + ) { (self.add_members, self.remove_members, self.new_threshold) } } @@ -234,7 +230,7 @@ impl BorshSerialize for ThresholdConfigUpdate { fn serialize(&self, writer: &mut W) -> std::io::Result<()> { self.add_members.serialize(writer)?; self.remove_members.serialize(writer)?; - self.new_threshold.serialize(writer) + self.new_threshold.get().serialize(writer) } } @@ -242,7 +238,10 @@ impl BorshDeserialize for ThresholdConfigUpdate { fn deserialize_reader(reader: &mut R) -> std::io::Result { let add_members = Vec::::deserialize_reader(reader)?; let remove_members = Vec::::deserialize_reader(reader)?; - let new_threshold = u8::deserialize_reader(reader)?; + let threshold_u8 = u8::deserialize_reader(reader)?; + let new_threshold = NonZero::new(threshold_u8).ok_or_else(|| { + std::io::Error::new(std::io::ErrorKind::InvalidData, "threshold cannot be zero") + })?; Ok(Self { add_members, @@ -258,7 +257,9 @@ impl<'a> Arbitrary<'a> for ThresholdConfigUpdate { let remove_members = Vec::::arbitrary(u)?; // Generate a threshold between 1 and max(1, len(add_members)) let max_threshold = add_members.len().max(1); - let new_threshold = u.int_in_range(1..=(max_threshold as u8))?; + let threshold_u8 = u.int_in_range(1..=(max_threshold as u8))?; + // Safe: threshold_u8 is always >= 1 + let new_threshold = NonZero::new(threshold_u8).expect("threshold is always >= 1"); Ok(Self { add_members, remove_members, @@ -284,26 +285,16 @@ mod tests { #[test] fn test_config_creation() { let keys = vec![make_key(1), make_key(2), make_key(3)]; - let config = ThresholdConfig::try_new(keys.clone(), 2).unwrap(); + let config = ThresholdConfig::try_new(keys.clone(), NonZero::new(2).unwrap()).unwrap(); assert_eq!(config.keys().len(), 3); assert_eq!(config.threshold(), 2); } - #[test] - fn test_config_zero_threshold() { - let keys = vec![make_key(1)]; - let result = ThresholdConfig::try_new(keys, 0); - assert!(matches!( - result, - Err(ThresholdSignatureError::ZeroThreshold) - )); - } - #[test] fn test_config_threshold_exceeds_keys() { let keys = vec![make_key(1), make_key(2)]; - let result = ThresholdConfig::try_new(keys, 3); + let result = ThresholdConfig::try_new(keys, NonZero::new(3).unwrap()); assert!(matches!( result, Err(ThresholdSignatureError::InvalidThreshold { .. }) @@ -313,9 +304,10 @@ mod tests { #[test] fn test_config_update_add_member() { let keys = vec![make_key(1), make_key(2)]; - let mut config = ThresholdConfig::try_new(keys, 2).unwrap(); + let mut config = ThresholdConfig::try_new(keys, NonZero::new(2).unwrap()).unwrap(); - let update = ThresholdConfigUpdate::new(vec![make_key(3)], vec![], 2); + let update = + ThresholdConfigUpdate::new(vec![make_key(3)], vec![], NonZero::new(2).unwrap()); config.apply_update(&update).unwrap(); assert_eq!(config.keys().len(), 3); @@ -327,9 +319,10 @@ mod tests { let k2 = make_key(2); let k3 = make_key(3); - let mut config = ThresholdConfig::try_new(vec![k1, k2, k3], 2).unwrap(); + let mut config = + ThresholdConfig::try_new(vec![k1, k2, k3], NonZero::new(2).unwrap()).unwrap(); - let update = ThresholdConfigUpdate::new(vec![], vec![k2], 2); + let update = ThresholdConfigUpdate::new(vec![], vec![k2], NonZero::new(2).unwrap()); config.apply_update(&update).unwrap(); assert_eq!(config.keys().len(), 2); @@ -339,7 +332,7 @@ mod tests { #[test] fn test_config_borsh_roundtrip() { let keys = vec![make_key(1), make_key(2)]; - let config = ThresholdConfig::try_new(keys, 2).unwrap(); + let config = ThresholdConfig::try_new(keys, NonZero::new(2).unwrap()).unwrap(); let encoded = borsh::to_vec(&config).unwrap(); let decoded: ThresholdConfig = borsh::from_slice(&encoded).unwrap(); diff --git a/crates/crypto/src/threshold_signature/indexed/errors.rs b/crates/crypto/src/threshold_signature/indexed/errors.rs index 02dd50f108..ec635e3476 100644 --- a/crates/crypto/src/threshold_signature/indexed/errors.rs +++ b/crates/crypto/src/threshold_signature/indexed/errors.rs @@ -20,10 +20,6 @@ pub enum ThresholdSignatureError { #[error("invalid threshold: {threshold} exceeds total keys {total_keys}")] InvalidThreshold { threshold: u8, total_keys: usize }, - /// Threshold cannot be zero. - #[error("threshold cannot be zero")] - ZeroThreshold, - /// Signature verification failed. #[error("invalid signature at index {index}")] InvalidSignature { index: u8 }, diff --git a/crates/crypto/src/threshold_signature/indexed/verification.rs b/crates/crypto/src/threshold_signature/indexed/verification.rs index f8dbc535c6..21b493f05d 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification.rs @@ -46,6 +46,8 @@ pub fn verify_threshold_signatures( #[cfg(test)] mod tests { + use std::num::NonZero; + use secp256k1::{Secp256k1, SecretKey}; use super::*; @@ -66,7 +68,8 @@ mod tests { let (sk2, pk2) = generate_keypair(2); let (_sk3, pk3) = generate_keypair(3); - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + let config = + ThresholdConfig::try_new(vec![pk1, pk2, pk3], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -89,7 +92,8 @@ mod tests { let (sk2, pk2) = generate_keypair(2); let (_sk3, pk3) = generate_keypair(3); - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + let config = + ThresholdConfig::try_new(vec![pk1, pk2, pk3], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -110,7 +114,7 @@ mod tests { let (sk1, pk1) = generate_keypair(1); let (sk2, pk2) = generate_keypair(2); - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + let config = ThresholdConfig::try_new(vec![pk1, pk2], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; let wrong_message_hash = [0xCD; 32]; @@ -137,7 +141,7 @@ mod tests { let (sk1, pk1) = generate_keypair(1); let (_sk2, pk2) = generate_keypair(2); - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + let config = ThresholdConfig::try_new(vec![pk1, pk2], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -163,7 +167,7 @@ mod tests { let (sk1, pk1) = generate_keypair(1); let (sk2, pk2) = generate_keypair(2); - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + let config = ThresholdConfig::try_new(vec![pk1, pk2], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -187,7 +191,7 @@ mod tests { let (sk1, pk1) = generate_keypair(1); let (_sk2, pk2) = generate_keypair(2); - let config = ThresholdConfig::try_new(vec![pk1, pk2], 2).unwrap(); + let config = ThresholdConfig::try_new(vec![pk1, pk2], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -214,7 +218,8 @@ mod tests { let (sk2, pk2) = generate_keypair(2); let (_sk3, pk3) = generate_keypair(3); - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + let config = + ThresholdConfig::try_new(vec![pk1, pk2, pk3], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; @@ -246,7 +251,8 @@ mod tests { let (sk2, pk2) = generate_keypair(2); let (_sk3, pk3) = generate_keypair(3); - let config = ThresholdConfig::try_new(vec![pk1, pk2, pk3], 2).unwrap(); + let config = + ThresholdConfig::try_new(vec![pk1, pk2, pk3], NonZero::new(2).unwrap()).unwrap(); let message_hash = [0xAB; 32]; diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index 3232866258..f237e821cf 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -55,7 +55,7 @@ pub(super) fn verify_ecdsa_signatures( for indexed_sig in signatures.signatures() { // Check index is in bounds let index = indexed_sig.index() as usize; - if index >= config.keys().len() { + if index > config.keys().len() { return Err(ThresholdSignatureError::SignerIndexOutOfBounds { index: indexed_sig.index(), max: config.keys().len(), From 11e876eddd50bcd8b82b393aca2c28257cc54a99 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 19:41:08 +0400 Subject: [PATCH 08/11] fix(crypto): use derive for Borsh serialization and fix rustdoc link --- .../bridge-v1/src/state/operator.rs | 2 +- crates/crypto/src/lib.rs | 3 + .../musig2/aggregation.rs | 0 .../{threshold_signature => }/musig2/mod.rs | 0 crates/crypto/src/test_utils/schnorr.rs | 2 +- .../src/threshold_signature/indexed/config.rs | 62 +------------------ .../threshold_signature/indexed/signature.rs | 45 +------------- crates/crypto/src/threshold_signature/mod.rs | 3 - crates/l1tx/src/utils.rs | 2 +- 9 files changed, 11 insertions(+), 108 deletions(-) rename crates/crypto/src/{threshold_signature => }/musig2/aggregation.rs (100%) rename crates/crypto/src/{threshold_signature => }/musig2/mod.rs (100%) diff --git a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs index bd3cd14ea1..98ef5ec037 100644 --- a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs +++ b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs @@ -5,7 +5,7 @@ use borsh::{BorshDeserialize, BorshSerialize}; use serde::{Deserialize, Serialize}; use strata_bridge_types::OperatorIdx; -use strata_crypto::{schnorr::EvenPublicKey, threshold_signature::aggregate_schnorr_keys}; +use strata_crypto::{aggregate_schnorr_keys, schnorr::EvenPublicKey}; use strata_primitives::{buf::Buf32, l1::BitcoinXOnlyPublicKey, sorted_vec::SortedVec}; use super::bitmap::OperatorBitmap; diff --git a/crates/crypto/src/lib.rs b/crates/crypto/src/lib.rs index 36f2841565..768f0a07e8 100644 --- a/crates/crypto/src/lib.rs +++ b/crates/crypto/src/lib.rs @@ -1,8 +1,11 @@ //! Cryptographic primitives. +pub mod musig2; pub mod schnorr; #[cfg(feature = "test-utils")] pub mod test_utils; pub mod threshold_signature; +// Re-export MuSig2 key aggregation +pub use musig2::{aggregate_schnorr_keys, Musig2Error}; pub use schnorr::*; diff --git a/crates/crypto/src/threshold_signature/musig2/aggregation.rs b/crates/crypto/src/musig2/aggregation.rs similarity index 100% rename from crates/crypto/src/threshold_signature/musig2/aggregation.rs rename to crates/crypto/src/musig2/aggregation.rs diff --git a/crates/crypto/src/threshold_signature/musig2/mod.rs b/crates/crypto/src/musig2/mod.rs similarity index 100% rename from crates/crypto/src/threshold_signature/musig2/mod.rs rename to crates/crypto/src/musig2/mod.rs diff --git a/crates/crypto/src/test_utils/schnorr.rs b/crates/crypto/src/test_utils/schnorr.rs index a6f4dde251..411a953955 100644 --- a/crates/crypto/src/test_utils/schnorr.rs +++ b/crates/crypto/src/test_utils/schnorr.rs @@ -3,7 +3,7 @@ use rand::{rngs::OsRng, RngCore}; use secp256k1::{PublicKey, Secp256k1, XOnlyPublicKey}; use strata_identifiers::Buf32; -use crate::{schnorr::EvenSecretKey, threshold_signature::musig2::aggregate_schnorr_keys}; +use crate::{musig2::aggregate_schnorr_keys, schnorr::EvenSecretKey}; /// Creates a MuSig2 signature from multiple operators. /// diff --git a/crates/crypto/src/threshold_signature/indexed/config.rs b/crates/crypto/src/threshold_signature/indexed/config.rs index 690c304ef1..ed6cfc227e 100644 --- a/crates/crypto/src/threshold_signature/indexed/config.rs +++ b/crates/crypto/src/threshold_signature/indexed/config.rs @@ -18,7 +18,7 @@ pub const MAX_SIGNERS: usize = 256; /// Defines who can sign (`keys`) and how many must sign (`threshold`). /// The threshold is stored as `NonZero` to enforce at the type level /// that it can never be zero. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct ThresholdConfig { /// Public keys of all authorized signers. keys: Vec, @@ -71,7 +71,7 @@ impl ThresholdConfig { /// /// # Note /// - /// This method is called automatically by [`apply_update`]. External callers + /// This method is called automatically by [`Self::apply_update`]. External callers /// may use this for dry-run validation, but there's no need to call it /// before `apply_update` as validation is always performed. pub fn validate_update( @@ -140,37 +140,6 @@ impl ThresholdConfig { } } -impl BorshSerialize for ThresholdConfig { - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { - self.keys.serialize(writer)?; - self.threshold.get().serialize(writer) - } -} - -impl BorshDeserialize for ThresholdConfig { - fn deserialize_reader(reader: &mut R) -> std::io::Result { - let keys = Vec::::deserialize_reader(reader)?; - - // Validate key count doesn't exceed maximum (prevents DoS via large allocations) - if keys.len() > MAX_SIGNERS { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!( - "too many signers: {} exceeds maximum {}", - keys.len(), - MAX_SIGNERS - ), - )); - } - - let threshold_u8 = u8::deserialize_reader(reader)?; - let threshold = NonZero::new(threshold_u8).ok_or_else(|| { - std::io::Error::new(std::io::ErrorKind::InvalidData, "threshold cannot be zero") - })?; - Ok(Self { keys, threshold }) - } -} - impl Hash for CompressedPublicKey { fn hash(&self, state: &mut H) { self.serialize().hash(state); @@ -178,7 +147,7 @@ impl Hash for CompressedPublicKey { } /// Represents a change to the threshold configuration. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct ThresholdConfigUpdate { add_members: Vec, remove_members: Vec, @@ -226,31 +195,6 @@ impl ThresholdConfigUpdate { } } -impl BorshSerialize for ThresholdConfigUpdate { - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { - self.add_members.serialize(writer)?; - self.remove_members.serialize(writer)?; - self.new_threshold.get().serialize(writer) - } -} - -impl BorshDeserialize for ThresholdConfigUpdate { - fn deserialize_reader(reader: &mut R) -> std::io::Result { - let add_members = Vec::::deserialize_reader(reader)?; - let remove_members = Vec::::deserialize_reader(reader)?; - let threshold_u8 = u8::deserialize_reader(reader)?; - let new_threshold = NonZero::new(threshold_u8).ok_or_else(|| { - std::io::Error::new(std::io::ErrorKind::InvalidData, "threshold cannot be zero") - })?; - - Ok(Self { - add_members, - remove_members, - new_threshold, - }) - } -} - impl<'a> Arbitrary<'a> for ThresholdConfigUpdate { fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { let add_members = Vec::::arbitrary(u)?; diff --git a/crates/crypto/src/threshold_signature/indexed/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs index ab5805a7e4..ffe49e7df6 100644 --- a/crates/crypto/src/threshold_signature/indexed/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -20,7 +20,7 @@ use super::ThresholdSignatureError; /// - 39-42: Native SegWit P2WPKH /// /// The verification code normalizes both formats to extract the raw recovery ID (0-3). -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct IndexedSignature { /// Index of the signer in the ThresholdConfig keys array (0-255). index: u8, @@ -76,26 +76,10 @@ impl IndexedSignature { } } -impl BorshSerialize for IndexedSignature { - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { - self.index.serialize(writer)?; - writer.write_all(&self.signature) - } -} - -impl BorshDeserialize for IndexedSignature { - fn deserialize_reader(reader: &mut R) -> std::io::Result { - let index = u8::deserialize_reader(reader)?; - let mut signature = [0u8; 65]; - reader.read_exact(&mut signature)?; - Ok(Self { index, signature }) - } -} - /// A set of indexed ECDSA signatures for threshold verification. /// /// Signatures are sorted by index and must not contain duplicates. -#[derive(Debug, Clone, PartialEq, Eq, Default)] +#[derive(Debug, Clone, PartialEq, Eq, Default, BorshSerialize, BorshDeserialize)] pub struct SignatureSet { /// Sorted signatures by index, no duplicates. signatures: Vec, @@ -154,31 +138,6 @@ impl SignatureSet { } } -impl BorshSerialize for SignatureSet { - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { - // Write count as u8 (max 255 signatures) - let count = self.signatures.len() as u8; - count.serialize(writer)?; - // Write each signature - for sig in &self.signatures { - sig.serialize(writer)?; - } - Ok(()) - } -} - -impl BorshDeserialize for SignatureSet { - fn deserialize_reader(reader: &mut R) -> std::io::Result { - let count = u8::deserialize_reader(reader)?; - let mut signatures = Vec::with_capacity(count as usize); - for _ in 0..count { - signatures.push(IndexedSignature::deserialize_reader(reader)?); - } - SignatureSet::new(signatures) - .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string())) - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/crypto/src/threshold_signature/mod.rs b/crates/crypto/src/threshold_signature/mod.rs index 4f6d20cbf0..23e4b3c30a 100644 --- a/crates/crypto/src/threshold_signature/mod.rs +++ b/crates/crypto/src/threshold_signature/mod.rs @@ -5,12 +5,9 @@ //! - `indexed`: Individual ECDSA signatures for the admin subprotocol (M-of-N threshold) pub mod indexed; -pub mod musig2; // Re-export commonly used types from indexed pub use indexed::{ verify_threshold_signatures, CompressedPublicKey, IndexedSignature, SignatureSet, ThresholdConfig, ThresholdConfigUpdate, ThresholdSignatureError, }; -// Re-export MuSig2 key aggregation -pub use musig2::{aggregate_schnorr_keys, Musig2Error}; diff --git a/crates/l1tx/src/utils.rs b/crates/l1tx/src/utils.rs index 424b7eb5a2..ba2971d3e6 100644 --- a/crates/l1tx/src/utils.rs +++ b/crates/l1tx/src/utils.rs @@ -11,7 +11,7 @@ use strata_asm_txs_bridge_v1::{ withdrawal_fulfillment::WithdrawalFulfillmentInfo as BridgeV1WithdrawInfo, }; use strata_asm_types::{DepositInfo, WithdrawalFulfillmentInfo}; -use strata_crypto::threshold_signature::aggregate_schnorr_keys; +use strata_crypto::aggregate_schnorr_keys; use strata_params::{OperatorConfig, RollupParams}; use strata_primitives::{buf::Buf32, l1::BitcoinAddress}; From 5e45b583f533a34b504d21125b2b0801167c948b Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 20:17:48 +0400 Subject: [PATCH 09/11] fix(crypto): improve comment & fix off-by-one error --- crates/crypto/src/threshold_signature/indexed/errors.rs | 2 +- .../crypto/src/threshold_signature/indexed/signature.rs | 4 ++++ .../threshold_signature/indexed/verification/ecdsa.rs | 9 ++++++--- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/crates/crypto/src/threshold_signature/indexed/errors.rs b/crates/crypto/src/threshold_signature/indexed/errors.rs index ec635e3476..8b50aa8ab8 100644 --- a/crates/crypto/src/threshold_signature/indexed/errors.rs +++ b/crates/crypto/src/threshold_signature/indexed/errors.rs @@ -20,7 +20,7 @@ pub enum ThresholdSignatureError { #[error("invalid threshold: {threshold} exceeds total keys {total_keys}")] InvalidThreshold { threshold: u8, total_keys: usize }, - /// Signature verification failed. + /// Signature verification failed for the given signer index. #[error("invalid signature at index {index}")] InvalidSignature { index: u8 }, diff --git a/crates/crypto/src/threshold_signature/indexed/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs index ffe49e7df6..7f1d124e3c 100644 --- a/crates/crypto/src/threshold_signature/indexed/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -20,6 +20,10 @@ use super::ThresholdSignatureError; /// - 39-42: Native SegWit P2WPKH /// /// The verification code normalizes both formats to extract the raw recovery ID (0-3). +/// +/// The signer includes their own index (position in `ThresholdConfig::keys`) when creating +/// an `IndexedSignature`. Verification uses that index to fetch the expected public key and +/// compare it against the recovered key from the signature. #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct IndexedSignature { /// Index of the signer in the ThresholdConfig keys array (0-255). diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index f237e821cf..9c5a2660ed 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -55,10 +55,12 @@ pub(super) fn verify_ecdsa_signatures( for indexed_sig in signatures.signatures() { // Check index is in bounds let index = indexed_sig.index() as usize; - if index > config.keys().len() { + let keys_len = config.keys().len(); + // Reject indices at/above the key count to avoid panicking on the lookup; report the last valid slot. + if index >= keys_len { return Err(ThresholdSignatureError::SignerIndexOutOfBounds { index: indexed_sig.index(), - max: config.keys().len(), + max: keys_len.saturating_sub(1), }); } @@ -74,7 +76,8 @@ pub(super) fn verify_ecdsa_signatures( RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?; - // Recover the public key from the signature + // Hardware wallets emit recoverable signatures (with a header). Recover first so we + // honor the header and then compare to the configured public key. let recovered_pubkey = SECP256K1 .recover_ecdsa(&message, &recoverable_sig) .map_err(|_| ThresholdSignatureError::InvalidSignature { From f758030785b20bb837dc1ac6fe6ec5b6804f9b9d Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Tue, 2 Dec 2025 20:32:25 +0400 Subject: [PATCH 10/11] fix(crypto): fix lint --- .../src/threshold_signature/indexed/verification/ecdsa.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index 9c5a2660ed..df53cf8fcf 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -56,7 +56,8 @@ pub(super) fn verify_ecdsa_signatures( // Check index is in bounds let index = indexed_sig.index() as usize; let keys_len = config.keys().len(); - // Reject indices at/above the key count to avoid panicking on the lookup; report the last valid slot. + // Reject indices at/above the key count to avoid panicking on the lookup; report the last + // valid slot. if index >= keys_len { return Err(ThresholdSignatureError::SignerIndexOutOfBounds { index: indexed_sig.index(), From e4bc334e91a2506a34f266059578ffa60c737d46 Mon Sep 17 00:00:00 2001 From: Hamid Bateni Date: Mon, 8 Dec 2025 09:38:43 +0400 Subject: [PATCH 11/11] fix(crypto): threshold signature doc cleanups and HashSet dedupe in SignatureSet --- .../asm/subprotocols/admin/src/authority.rs | 10 +++---- crates/asm/txs/admin/src/test_utils/mod.rs | 2 +- .../src/threshold_signature/indexed/mod.rs | 4 +-- .../threshold_signature/indexed/signature.rs | 30 ++++++++----------- .../indexed/verification/ecdsa.rs | 9 +++--- 5 files changed, 26 insertions(+), 29 deletions(-) diff --git a/crates/asm/subprotocols/admin/src/authority.rs b/crates/asm/subprotocols/admin/src/authority.rs index 86192d5e55..711e8a0166 100644 --- a/crates/asm/subprotocols/admin/src/authority.rs +++ b/crates/asm/subprotocols/admin/src/authority.rs @@ -6,7 +6,7 @@ use strata_crypto::threshold_signature::{ use strata_primitives::roles::Role; /// Manages threshold signature operations for a given role and key set, with replay protection via -/// a seqno. +/// a sequence number. #[derive(Clone, Debug, Eq, PartialEq, BorshDeserialize, BorshSerialize)] pub struct MultisigAuthority { /// The role of this threshold signature authority. @@ -43,10 +43,10 @@ impl MultisigAuthority { } /// Verifies a set of ECDSA signatures against a threshold configuration. - /// - /// This function is intentionally ECDSA-specific as part of the hardware wallet - /// compatibility design (BIP-137 format support). A trait-based abstraction - /// could be added in the future if multiple signature schemes are needed. + // + // This function is intentionally ECDSA-specific as part of the hardware wallet + // compatibility design (BIP-137 format support). A trait-based abstraction + // could be added in the future if multiple signature schemes are needed. pub fn verify_action_signature( &self, action: &MultisigAction, diff --git a/crates/asm/txs/admin/src/test_utils/mod.rs b/crates/asm/txs/admin/src/test_utils/mod.rs index 2abf1f1c68..74b1cd22de 100644 --- a/crates/asm/txs/admin/src/test_utils/mod.rs +++ b/crates/asm/txs/admin/src/test_utils/mod.rs @@ -22,7 +22,7 @@ use crate::{ actions::MultisigAction, constants::ADMINISTRATION_SUBPROTOCOL_ID, parser::SignedPayload, }; -/// Creates an ECDSA recoverable signature for a message hash. +/// Creates an ECDSA signature with recoverable public key for a message hash. /// /// Returns a 65-byte signature in the format: recovery_id || r || s pub fn sign_ecdsa_recoverable(message_hash: &[u8; 32], secret_key: &SecretKey) -> [u8; 65] { diff --git a/crates/crypto/src/threshold_signature/indexed/mod.rs b/crates/crypto/src/threshold_signature/indexed/mod.rs index 02429834c8..a83e8757cd 100644 --- a/crates/crypto/src/threshold_signature/indexed/mod.rs +++ b/crates/crypto/src/threshold_signature/indexed/mod.rs @@ -1,6 +1,6 @@ -//! Individual ECDSA signature set for threshold signatures (M-of-N). +//! ECDSA signature set for threshold signatures (M-of-N). //! -//! This module provides types and functions for verifying a set of individual +//! This module provides types and functions for verifying a set of //! ECDSA signatures against a threshold configuration. Used by the admin //! subprotocol for hardware wallet compatibility. diff --git a/crates/crypto/src/threshold_signature/indexed/signature.rs b/crates/crypto/src/threshold_signature/indexed/signature.rs index 7f1d124e3c..ebf4e8e923 100644 --- a/crates/crypto/src/threshold_signature/indexed/signature.rs +++ b/crates/crypto/src/threshold_signature/indexed/signature.rs @@ -1,10 +1,12 @@ //! Signature types for threshold signing. +use std::collections::HashSet; + use borsh::{BorshDeserialize, BorshSerialize}; use super::ThresholdSignatureError; -/// An individual ECDSA signature with its signer index. +/// An ECDSA signature with its signer index. /// /// The signature is in recoverable format (65 bytes): `header || r || s`. /// @@ -82,7 +84,7 @@ impl IndexedSignature { /// A set of indexed ECDSA signatures for threshold verification. /// -/// Signatures are sorted by index and must not contain duplicates. +/// Signatures are guaranteed duplicate-free. #[derive(Debug, Clone, PartialEq, Eq, Default, BorshSerialize, BorshDeserialize)] pub struct SignatureSet { /// Sorted signatures by index, no duplicates. @@ -92,17 +94,12 @@ pub struct SignatureSet { impl SignatureSet { /// Create a new signature set from a vector of indexed signatures. /// - /// The signatures will be sorted by index and checked for duplicates. - pub fn new(mut signatures: Vec) -> Result { - // Sort by index - signatures.sort_by_key(|s| s.index); - - // Check for duplicate indices - for window in signatures.windows(2) { - if window[0].index == window[1].index { - return Err(ThresholdSignatureError::DuplicateSignerIndex( - window[0].index, - )); + /// The signatures will be checked for duplicates. + pub fn new(signatures: Vec) -> Result { + let mut seen = HashSet::new(); + for sig in &signatures { + if !seen.insert(sig.index) { + return Err(ThresholdSignatureError::DuplicateSignerIndex(sig.index)); } } @@ -158,10 +155,9 @@ mod tests { let sigs = vec![make_sig(2), make_sig(0), make_sig(1)]; let set = SignatureSet::new(sigs).unwrap(); - // Should be sorted - assert_eq!(set.signatures()[0].index(), 0); - assert_eq!(set.signatures()[1].index(), 1); - assert_eq!(set.signatures()[2].index(), 2); + assert_eq!(set.signatures()[0].index(), 2); + assert_eq!(set.signatures()[1].index(), 0); + assert_eq!(set.signatures()[2].index(), 1); } #[test] diff --git a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs index df53cf8fcf..e9206515d0 100644 --- a/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs +++ b/crates/crypto/src/threshold_signature/indexed/verification/ecdsa.rs @@ -36,8 +36,9 @@ fn normalize_recovery_id(header: u8) -> Result { /// Verifies each ECDSA signature in the set against the corresponding public key. /// -/// This function performs the actual ECDSA signature recovery and verification. -/// It assumes the SignatureSet has already been validated for duplicates. +/// This function recovers a public key from each ECDSA signature, then checks it +/// against the configured key for that index. The `SignatureSet` is already +/// deduped. /// /// # Hardware Wallet Compatibility /// Supports signatures from hardware wallets (Ledger/Trezor) that use BIP-137 format @@ -77,8 +78,8 @@ pub(super) fn verify_ecdsa_signatures( RecoverableSignature::from_compact(&indexed_sig.compact(), recovery_id) .map_err(|_| ThresholdSignatureError::InvalidSignatureFormat)?; - // Hardware wallets emit recoverable signatures (with a header). Recover first so we - // honor the header and then compare to the configured public key. + // Hardware wallets emit recoverable signatures with headers; recover the pubkey + // (honoring the header) and compare to the configured public key. let recovered_pubkey = SECP256K1 .recover_ecdsa(&message, &recoverable_sig) .map_err(|_| ThresholdSignatureError::InvalidSignature {