From e867b72259773d9926f2958d476144a62d7b8eed Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Thu, 30 Jan 2025 04:33:44 +0000 Subject: [PATCH 01/14] some wsts cleanup/refactor --- Cargo.lock | 2 +- Cargo.toml | 2 +- protobufs/stacks/signer/v1/messages.proto | 7 +- signer/src/message.rs | 77 ++- signer/src/proto/convert.rs | 42 +- .../src/proto/generated/stacks.signer.v1.rs | 12 + signer/src/testing/message.rs | 2 +- signer/src/testing/wsts.rs | 24 +- signer/src/transaction_coordinator.rs | 75 +-- signer/src/transaction_signer.rs | 144 +++-- signer/src/wsts_state_machine.rs | 555 ++++++++++++++---- .../tests/integration/transaction_signer.rs | 24 +- supply-chain/config.toml | 2 +- 13 files changed, 715 insertions(+), 253 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index edf2813b2..7bf541d75 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7632,7 +7632,7 @@ dependencies = [ [[package]] name = "wsts" version = "10.0.0" -source = "git+https://github.com/Trust-Machines/wsts.git?rev=53ae23f5f35def420877ccc8c0fe3662e64e38a1#53ae23f5f35def420877ccc8c0fe3662e64e38a1" +source = "git+https://github.com/Trust-Machines/wsts.git?rev=a7bf38bd54cddf0b78c0f7c521a5ed1537a684fa#a7bf38bd54cddf0b78c0f7c521a5ed1537a684fa" dependencies = [ "aes-gcm", "bs58 0.5.1", diff --git a/Cargo.toml b/Cargo.toml index fac4cb3a9..92c87eb9f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ stackslib = { git = "https://github.com/stacks-network/stacks-core", rev = "4977 stacks-common = { git = "https://github.com/stacks-network/stacks-core", rev = "49777d3fd73a6dbb610be80c376b7d9389c9871a", default-features = false, features = ["canonical"] } # Trust Machines Dependencies -wsts = { git = "https://github.com/Trust-Machines/wsts.git", rev = "53ae23f5f35def420877ccc8c0fe3662e64e38a1" } +wsts = { git = "https://github.com/Trust-Machines/wsts.git", rev = "a7bf38bd54cddf0b78c0f7c521a5ed1537a684fa" } # Crates.io aquamarine = { version = "0.6.0", default-features = false } diff --git a/protobufs/stacks/signer/v1/messages.proto b/protobufs/stacks/signer/v1/messages.proto index 7d76178a0..4f85d4a75 100644 --- a/protobufs/stacks/signer/v1/messages.proto +++ b/protobufs/stacks/signer/v1/messages.proto @@ -36,7 +36,7 @@ message SignerMessage { // A wsts message. message WstsMessage { // The transaction ID this message relates to, will be a dummy ID for DKG messages - bitcoin.BitcoinTxid txid = 1; + bitcoin.BitcoinTxid txid = 1 [deprecated = true]; // The wsts message oneof inner { // Tell signers to begin DKG by sending DKG public shares @@ -60,6 +60,11 @@ message WstsMessage { // Tell coordinator signature shares crypto.wsts.SignatureShareResponse signature_share_response = 11; } + oneof id { + bitcoin.BitcoinTxid id_bitcoin_txid = 12; + crypto.PublicKey id_rotate_key = 13; + bytes id_arbitrary = 14; + } } // Wraps an inner type with a public key and a signature, diff --git a/signer/src/message.rs b/signer/src/message.rs index 26f7631fe..e8786b415 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -1,14 +1,18 @@ //! Signer message definition for network communication +use rand::rngs::OsRng; +use rand::Rng; use secp256k1::ecdsa::RecoverableSignature; use crate::bitcoin::utxo::Fees; use crate::bitcoin::validation::TxRequestIds; +use crate::keys::PrivateKey; use crate::keys::PublicKey; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::StacksTx; use crate::storage::model; use crate::storage::model::BitcoinBlockHash; +use crate::storage::model::BitcoinTxId; use crate::storage::model::StacksTxId; /// Messages exchanged between signers @@ -224,16 +228,83 @@ pub struct BitcoinPreSignRequest { #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)] pub struct BitcoinPreSignAck; +/// The identifier for a WSTS message. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum WstsMessageId { + /// The WSTS message is related to a Bitcoin transaction. + BitcoinTxid(bitcoin::Txid), + /// The WSTS message is related to a rotate key operation. + RotateKey(PublicKey), + /// The WSTS message isn't specifically related to anything. + Arbitrary([u8; 32]), +} + +impl From for WstsMessageId { + fn from(txid: bitcoin::Txid) -> Self { + Self::BitcoinTxid(txid) + } +} + +impl WstsMessageId { + /// Generate a random [`WstsMessageId::Arbitrary`] WSTS message ID + pub fn random_arbitrary() -> Self { + Self::Arbitrary(OsRng.gen()) + } + + /// Generate a random [`WstsMessageId::BitcoinTxid`] WSTS message ID + pub fn random_bitcoin_txid() -> Self { + let random_bytes: [u8; 32] = OsRng.gen(); + let txid = BitcoinTxId::from(random_bytes).into(); + Self::BitcoinTxid(txid) + } + + /// Generate a random [`WstsMessageId::AggregateKey`] WSTS message ID + pub fn random_rotate_key() -> Self { + let private = PrivateKey::new(&mut OsRng); + let public = PublicKey::from_private_key(&private); + Self::RotateKey(public) + } +} + +impl std::fmt::Display for WstsMessageId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + WstsMessageId::BitcoinTxid(txid) => write!(f, "bitcoin-txid({})", txid), + WstsMessageId::RotateKey(aggregate_key) => { + write!(f, "rotate-key({})", aggregate_key) + } + WstsMessageId::Arbitrary(bytes) => write!(f, "arbitrary({})", hex::encode(bytes)), + } + } +} + /// A wsts message. #[derive(Debug, Clone, PartialEq)] pub struct WstsMessage { - /// The transaction ID this message relates to, - /// will be a dummy ID for DKG messages - pub txid: bitcoin::Txid, + /// The id of the wsts message. + pub id: WstsMessageId, /// The wsts message pub inner: wsts::net::Message, } +impl WstsMessage { + /// Returns the type of the message as a &str. + pub fn type_id(&self) -> &'static str { + match self.inner { + wsts::net::Message::DkgBegin(_) => "dkg-begin", + wsts::net::Message::DkgEndBegin(_) => "dkg-end-begin", + wsts::net::Message::DkgEnd(_) => "dkg-end", + wsts::net::Message::DkgPrivateBegin(_) => "dkg-private-begin", + wsts::net::Message::DkgPrivateShares(_) => "dkg-private-shares", + wsts::net::Message::DkgPublicShares(_) => "dkg-public-shares", + wsts::net::Message::NonceRequest(_) => "nonce-request", + wsts::net::Message::NonceResponse(_) => "nonce-response", + wsts::net::Message::SignatureShareRequest(_) => "signature-share-request", + wsts::net::Message::SignatureShareResponse(_) => "signature-share-response", + } + } +} + /// Convenient type aliases type StacksBlockHash = [u8; 32]; diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 2b95b8ff2..2e24f1338 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -54,6 +54,7 @@ use crate::message::SignerWithdrawalDecision; use crate::message::StacksTransactionSignRequest; use crate::message::StacksTransactionSignature; use crate::message::WstsMessage; +use crate::message::WstsMessageId; use crate::proto; use crate::stacks::contracts::AcceptWithdrawalV1; use crate::stacks::contracts::CompleteDepositV1; @@ -68,6 +69,9 @@ use crate::storage::model::QualifiedRequestId; use crate::storage::model::StacksBlockHash; use crate::storage::model::StacksPrincipal; use crate::storage::model::StacksTxId; +use crate::storage::model::ToLittleEndianOrder; + +use super::wsts_message; /// This trait is to make it easy to handle fields of protobuf structs that /// are `None`, when they should be `Some(_)`. @@ -1058,6 +1062,7 @@ impl TryFrom for SignatureShareResponse { } } impl From for proto::WstsMessage { + #[allow(deprecated)] fn from(value: WstsMessage) -> Self { let inner = match value.inner { wsts::net::Message::DkgBegin(inner) => { @@ -1089,8 +1094,23 @@ impl From for proto::WstsMessage { proto::wsts_message::Inner::SignatureShareResponse(inner.into()) } }; + proto::WstsMessage { - txid: Some(BitcoinTxId::from(value.txid).into()), + txid: match value.id { + WstsMessageId::BitcoinTxid(txid) => { + Some(proto::BitcoinTxid::from(BitcoinTxId::from(txid))) + } + WstsMessageId::RotateKey(_) | WstsMessageId::Arbitrary(_) => None, + }, + id: Some(match value.id { + WstsMessageId::BitcoinTxid(txid) => { + wsts_message::Id::IdBitcoinTxid(proto::BitcoinTxid { + txid: Some(proto::Uint256::from(txid.to_le_bytes())), + }) + } + WstsMessageId::RotateKey(pubkey) => wsts_message::Id::IdRotateKey(pubkey.into()), + WstsMessageId::Arbitrary(key) => wsts_message::Id::IdArbitrary(key.into()), + }), inner: Some(inner), } } @@ -1098,6 +1118,7 @@ impl From for proto::WstsMessage { impl TryFrom for WstsMessage { type Error = Error; + fn try_from(value: proto::WstsMessage) -> Result { let inner = match value.inner.required()? { proto::wsts_message::Inner::DkgBegin(inner) => { @@ -1131,8 +1152,25 @@ impl TryFrom for WstsMessage { wsts::net::Message::SignatureShareResponse(inner.try_into()?) } }; + + #[allow(deprecated)] Ok(WstsMessage { - txid: BitcoinTxId::try_from(value.txid.required()?)?.into(), + id: match value.id { + Some(id) => match id { + wsts_message::Id::IdBitcoinTxid(txid) => { + WstsMessageId::BitcoinTxid(BitcoinTxId::try_from(txid)?.into()) + } + wsts_message::Id::IdRotateKey(pubkey) => { + WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) + } + wsts_message::Id::IdArbitrary(key) => { + WstsMessageId::Arbitrary(key.try_into().map_err(|_| Error::TypeConversion)?) + } + }, + None => WstsMessageId::BitcoinTxid( + BitcoinTxId::try_from(value.txid.required()?)?.into(), + ), + }, inner, }) } diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index 0ffe80218..784947f74 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -287,11 +287,14 @@ pub mod signer_message { #[derive(Clone, PartialEq, ::prost::Message)] pub struct WstsMessage { /// The transaction ID this message relates to, will be a dummy ID for DKG messages + #[deprecated] #[prost(message, optional, tag = "1")] pub txid: ::core::option::Option, /// The wsts message #[prost(oneof = "wsts_message::Inner", tags = "2, 3, 4, 5, 6, 7, 8, 9, 10, 11")] pub inner: ::core::option::Option, + #[prost(oneof = "wsts_message::Id", tags = "12, 13, 14")] + pub id: ::core::option::Option, } /// Nested message and enum types in `WstsMessage`. pub mod wsts_message { @@ -335,6 +338,15 @@ pub mod wsts_message { super::super::super::super::crypto::wsts::SignatureShareResponse, ), } + #[derive(Clone, PartialEq, ::prost::Oneof)] + pub enum Id { + #[prost(message, tag = "12")] + IdBitcoinTxid(super::super::super::super::bitcoin::BitcoinTxid), + #[prost(message, tag = "13")] + IdRotateKey(super::super::super::super::crypto::PublicKey), + #[prost(bytes, tag = "14")] + IdArbitrary(::prost::alloc::vec::Vec), + } } /// Wraps an inner type with a public key and a signature, /// allowing easy verification of the integrity of the inner data. diff --git a/signer/src/testing/message.rs b/signer/src/testing/message.rs index a6a010305..a8244a7f5 100644 --- a/signer/src/testing/message.rs +++ b/signer/src/testing/message.rs @@ -113,7 +113,7 @@ impl fake::Dummy for message::WstsMessage { }; Self { - txid: dummy::txid(config, rng), + id: dummy::txid(config, rng).into(), inner: wsts::net::Message::DkgEndBegin(dkg_end_begin), } } diff --git a/signer/src/testing/wsts.rs b/signer/src/testing/wsts.rs index 2055ebe71..62ca34aeb 100644 --- a/signer/src/testing/wsts.rs +++ b/signer/src/testing/wsts.rs @@ -20,6 +20,7 @@ use crate::ecdsa::SignEcdsa as _; use crate::keys::PrivateKey; use crate::keys::PublicKey; use crate::message; +use crate::message::WstsMessageId; use crate::network; use crate::network::MessageTransfer as _; use crate::storage; @@ -147,9 +148,10 @@ impl Coordinator { .start_public_shares() .expect("failed to start public shares"); - self.send_packet(bitcoin_chain_tip, txid, outbound).await; + self.send_packet(bitcoin_chain_tip, txid.into(), outbound) + .await; - match self.loop_until_result(bitcoin_chain_tip, txid).await { + match self.loop_until_result(bitcoin_chain_tip, txid.into()).await { wsts::state_machine::OperationResult::Dkg(aggregate_key) => { PublicKey::try_from(&aggregate_key).expect("Got the point at infinity") } @@ -161,7 +163,7 @@ impl Coordinator { pub async fn run_signing_round( &mut self, bitcoin_chain_tip: model::BitcoinBlockHash, - txid: bitcoin::Txid, + id: WstsMessageId, msg: &[u8], signature_type: SignatureType, ) -> wsts::taproot::SchnorrProof { @@ -170,9 +172,9 @@ impl Coordinator { .start_signing_round(msg, signature_type) .expect("failed to start signing round"); - self.send_packet(bitcoin_chain_tip, txid, outbound).await; + self.send_packet(bitcoin_chain_tip, id, outbound).await; - match self.loop_until_result(bitcoin_chain_tip, txid).await { + match self.loop_until_result(bitcoin_chain_tip, id).await { wsts::state_machine::OperationResult::SignTaproot(signature) | wsts::state_machine::OperationResult::SignSchnorr(signature) => signature, _ => panic!("unexpected operation result"), @@ -182,7 +184,7 @@ impl Coordinator { async fn loop_until_result( &mut self, bitcoin_chain_tip: model::BitcoinBlockHash, - txid: bitcoin::Txid, + id: WstsMessageId, ) -> wsts::state_machine::OperationResult { let future = async move { loop { @@ -204,7 +206,7 @@ impl Coordinator { if let Some(packet) = outbound_packet { tokio::time::sleep(std::time::Duration::from_secs(1)).await; - self.send_packet(bitcoin_chain_tip, txid, packet).await; + self.send_packet(bitcoin_chain_tip, id, packet).await; } if let Some(result) = operation_result { @@ -272,7 +274,7 @@ impl Signer { .process_inbound_messages(&[packet.clone()]) .expect("message processing failed"); - self.send_packet(bitcoin_chain_tip, wsts_msg.txid, packet.clone()) + self.send_packet(bitcoin_chain_tip, wsts_msg.id, packet.clone()) .await; if let wsts::net::Message::DkgEnd(_) = packet.msg { @@ -312,7 +314,7 @@ impl Signer { .process_inbound_messages(&[packet.clone()]) .expect("message processing failed"); - self.send_packet(bitcoin_chain_tip, wsts_msg.txid, packet.clone()) + self.send_packet(bitcoin_chain_tip, wsts_msg.id, packet.clone()) .await; if let wsts::net::Message::SignatureShareResponse(_) = packet.msg { @@ -339,10 +341,10 @@ trait WstsEntity { async fn send_packet( &mut self, bitcoin_chain_tip: model::BitcoinBlockHash, - txid: bitcoin::Txid, + id: WstsMessageId, packet: wsts::net::Packet, ) { - let payload: message::Payload = message::WstsMessage { txid, inner: packet.msg }.into(); + let payload: message::Payload = message::WstsMessage { id, inner: packet.msg }.into(); let msg = payload .to_message(bitcoin_chain_tip) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 473ab2b4c..113d16aa5 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -38,6 +38,7 @@ use crate::message::BitcoinPreSignRequest; use crate::message::Payload; use crate::message::SignerMessage; use crate::message::StacksTransactionSignRequest; +use crate::message::WstsMessageId; use crate::metrics::Metrics; use crate::metrics::BITCOIN_BLOCKCHAIN; use crate::metrics::STACKS_BLOCKCHAIN; @@ -58,11 +59,11 @@ use crate::stacks::wallet::SignerWallet; use crate::storage::model; use crate::storage::model::StacksTxId; use crate::storage::DbRead as _; -use crate::wsts_state_machine::CoordinatorStateMachine; +use crate::wsts_state_machine::FireCoordinator; +use crate::wsts_state_machine::WstsCoordinator; use bitcoin::hashes::Hash as _; use wsts::net::SignatureType; -use wsts::state_machine::coordinator::Coordinator as _; use wsts::state_machine::coordinator::State as WstsCoordinatorState; use wsts::state_machine::OperationResult as WstsOperationResult; use wsts::state_machine::StateMachine as _; @@ -914,9 +915,9 @@ where transaction: &mut utxo::UnsignedTransaction<'_>, ) -> Result<(), Error> { let sighashes = transaction.construct_digests()?; - let mut coordinator_state_machine = CoordinatorStateMachine::load( + let mut coordinator_state_machine = FireCoordinator::load( &mut self.context.get_storage_mut(), - sighashes.signers_aggregate_key, + sighashes.signers_aggregate_key.into(), signer_public_keys.clone(), self.threshold, self.private_key, @@ -930,7 +931,7 @@ where .coordinate_signing_round( bitcoin_chain_tip, &mut coordinator_state_machine, - txid, + txid.into(), &msg, SignatureType::Taproot(None), ) @@ -957,9 +958,9 @@ where for (deposit, sighash) in sighashes.deposits.into_iter() { let msg = sighash.to_raw_hash().to_byte_array(); - let mut coordinator_state_machine = CoordinatorStateMachine::load( + let mut coordinator_state_machine = FireCoordinator::load( &mut self.context.get_storage_mut(), - deposit.signers_public_key, + deposit.signers_public_key.into(), signer_public_keys.clone(), self.threshold, self.private_key, @@ -971,7 +972,7 @@ where .coordinate_signing_round( bitcoin_chain_tip, &mut coordinator_state_machine, - txid, + txid.into(), &msg, SignatureType::Schnorr, ) @@ -1034,17 +1035,19 @@ where } #[tracing::instrument(skip_all)] - async fn coordinate_signing_round( + async fn coordinate_signing_round( &mut self, bitcoin_chain_tip: &model::BitcoinBlockHash, - coordinator_state_machine: &mut CoordinatorStateMachine, - txid: bitcoin::Txid, + coordinator_state_machine: &mut Coordinator, + id: WstsMessageId, msg: &[u8], signature_type: SignatureType, - ) -> Result { + ) -> Result + where + Coordinator: WstsCoordinator, + { let outbound = coordinator_state_machine - .start_signing_round(msg, signature_type) - .map_err(Error::wsts_coordinator)?; + .start_signing_round(msg, signature_type)?; // We create a signal stream before sending a message so that there // is no race condition with the steam and the getting a response. @@ -1053,7 +1056,7 @@ where .as_signal_stream(signed_message_filter) .filter_map(Self::to_signed_message); - let msg = message::WstsMessage { txid, inner: outbound.msg }; + let msg = message::WstsMessage { id, inner: outbound.msg }; self.send_message(msg, bitcoin_chain_tip).await?; let max_duration = self.signing_round_max_duration; @@ -1061,7 +1064,7 @@ where signal_stream, bitcoin_chain_tip, coordinator_state_machine, - txid, + id, ); let operation_result = tokio::time::timeout(max_duration, run_signing_round) @@ -1096,7 +1099,7 @@ where let (_, signer_set) = self.get_signer_set_and_aggregate_key(chain_tip).await?; let mut state_machine = - CoordinatorStateMachine::new(signer_set, self.threshold, self.private_key); + FireCoordinator::new(signer_set, self.threshold, self.private_key); // Okay let's move the coordinator state machine to the beginning // of the DKG phase. @@ -1112,8 +1115,8 @@ where // around as a bitcoin transaction ID, even when it is not one. We // should probably change this let identifier = self.coordinator_id(chain_tip); - let txid = bitcoin::Txid::from_byte_array(identifier); - let msg = message::WstsMessage { txid, inner: outbound.msg }; + let id = bitcoin::Txid::from_byte_array(identifier).into(); + let msg = message::WstsMessage { id, inner: outbound.msg }; // We create a signal stream before sending a message so that there // is no race condition with the steam and the getting a response. @@ -1131,7 +1134,7 @@ where // Now that DKG has "begun" we need to drive it to completion. let max_duration = self.dkg_max_duration; let dkg_fut = - self.drive_wsts_state_machine(signal_stream, chain_tip, &mut state_machine, txid); + self.drive_wsts_state_machine(signal_stream, chain_tip, &mut state_machine, id); let operation_result = tokio::time::timeout(max_duration, dkg_fut) .await @@ -1144,15 +1147,16 @@ where } #[tracing::instrument(skip_all)] - async fn drive_wsts_state_machine( + async fn drive_wsts_state_machine( &mut self, signal_stream: S, bitcoin_chain_tip: &model::BitcoinBlockHash, - coordinator_state_machine: &mut CoordinatorStateMachine, - txid: bitcoin::Txid, + coordinator_state_machine: &mut Coordinator, + id: WstsMessageId, ) -> Result where S: Stream>, + Coordinator: WstsCoordinator, { // this assumes that the signer set doesn't change for the duration of this call, // but we're already assuming that the bitcoin chain tip doesn't change @@ -1181,11 +1185,6 @@ where continue; }; - let packet = wsts::net::Packet { - msg: wsts_msg.inner, - sig: Vec::new(), - }; - let msg_public_key = msg.signer_public_key; let sender_is_coordinator = @@ -1194,9 +1193,11 @@ where let public_keys = &coordinator_state_machine.get_config().signer_public_keys; let public_key_point = p256k1::point::Point::from(msg_public_key); + let msg = wsts_msg.inner; + // check that messages were signed by correct key let is_authenticated = Self::authenticate_message( - &packet, + &msg, public_keys, public_key_point, sender_is_coordinator, @@ -1207,16 +1208,16 @@ where } let (outbound_packet, operation_result) = - match coordinator_state_machine.process_message(&packet) { + match coordinator_state_machine.process_message(&msg) { Ok(val) => val, Err(err) => { - tracing::warn!(?packet, reason = %err, "ignoring packet"); + tracing::warn!(?msg, reason = %err, "ignoring packet"); continue; } }; if let Some(packet) = outbound_packet { - let msg = message::WstsMessage { txid, inner: packet.msg }; + let msg = message::WstsMessage { id, inner: packet.msg }; self.send_message(msg, bitcoin_chain_tip).await?; } @@ -1232,7 +1233,7 @@ where } fn authenticate_message( - packet: &wsts::net::Packet, + msg: &wsts::net::Message, public_keys: &hashbrown::HashMap, public_key_point: p256k1::point::Point, sender_is_coordinator: bool, @@ -1240,7 +1241,7 @@ where let check_signer_public_key = |signer_id| match public_keys.get(&signer_id) { Some(signer_public_key) if public_key_point != *signer_public_key => { tracing::warn!( - ?packet.msg, + ?msg, reason = "message was signed by the wrong signer", "ignoring packet" ); @@ -1248,7 +1249,7 @@ where } None => { tracing::warn!( - ?packet.msg, + ?msg, reason = "no public key for signer", %signer_id, "ignoring packet" @@ -1257,7 +1258,7 @@ where } _ => true, }; - match &packet.msg { + match msg { wsts::net::Message::DkgBegin(_) | wsts::net::Message::DkgPrivateBegin(_) | wsts::net::Message::DkgEndBegin(_) @@ -1265,7 +1266,7 @@ where | wsts::net::Message::SignatureShareRequest(_) => { if !sender_is_coordinator { tracing::warn!( - ?packet, + ?msg, reason = "got coordinator message from sender who is not coordinator", "ignoring packet" ); diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index ebba4a02d..98ad408af 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -24,6 +24,7 @@ use crate::keys::PublicKeyXOnly; use crate::message; use crate::message::BitcoinPreSignAck; use crate::message::StacksTransactionSignRequest; +use crate::message::WstsMessageId; use crate::metrics::Metrics; use crate::metrics::BITCOIN_BLOCKCHAIN; use crate::metrics::STACKS_BLOCKCHAIN; @@ -500,7 +501,16 @@ where } /// Process WSTS messages - #[tracing::instrument(skip_all, fields(txid = %msg.txid))] + #[tracing::instrument(skip_all, fields( + msg_id = %msg.id, + msg_type = %msg.type_id(), + dkg_signer_id = tracing::field::Empty, + dkg_id = tracing::field::Empty, + dkg_sign_id = tracing::field::Empty, + dkg_iter_id = tracing::field::Empty, + dkg_txid = tracing::field::Empty, + sender_public_key = %msg_public_key, + ))] pub async fn handle_wsts_message( &mut self, msg: &message::WstsMessage, @@ -508,15 +518,19 @@ where msg_public_key: PublicKey, chain_tip_report: &MsgChainTipReport, ) -> Result<(), Error> { + let span = tracing::Span::current(); + match &msg.inner { - WstsNetMessage::DkgBegin(_) => { - tracing::info!("handling DkgBegin"); + WstsNetMessage::DkgBegin(request) => { + span.record("dkg_id", request.dkg_id); if !chain_tip_report.sender_is_coordinator { tracing::warn!("received coordinator message from non-coordinator signer"); return Ok(()); } + tracing::debug!("responding to dkg-begin"); + // Assert that DKG should be allowed to proceed given the current state // and configuration. assert_allow_dkg_begin(&self.context, bitcoin_chain_tip).await?; @@ -534,63 +548,90 @@ where if let Some(pause) = self.dkg_begin_pause { // Let's give the others some slack tracing::debug!( - "Sleeping a bit to give the other peers some slack to get DkgBegin" + "sleeping a bit to give the other peers some slack to get dkg-begin" ); tokio::time::sleep(pause).await; } - let id = StateMachineId::from(bitcoin_chain_tip); - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } - WstsNetMessage::DkgPrivateBegin(_) => { - tracing::info!("handling DkgPrivateBegin"); + WstsNetMessage::DkgPrivateBegin(request) => { + span.record("dkg_id", request.dkg_id); + if !chain_tip_report.sender_is_coordinator { tracing::warn!("received coordinator message from non-coordinator signer"); return Ok(()); } + tracing::debug!("responding to dkg-private-begin"); + let id = StateMachineId::from(bitcoin_chain_tip); - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } - WstsNetMessage::DkgPublicShares(dkg_public_shares) => { - tracing::info!( - signer_id = %dkg_public_shares.signer_id, - "handling DkgPublicShares", - ); + WstsNetMessage::DkgPublicShares(request) => { + span.record("dkg_id", request.dkg_id); + span.record("dkg_signer_id", request.signer_id); + + tracing::debug!("responding to dkg-public-shares"); + let id = StateMachineId::from(bitcoin_chain_tip); - self.validate_sender(&id, dkg_public_shares.signer_id, &msg_public_key)?; - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.validate_sender(&id, request.signer_id, &msg_public_key)?; + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } - WstsNetMessage::DkgPrivateShares(dkg_private_shares) => { - tracing::info!( - signer_id = %dkg_private_shares.signer_id, - "handling DkgPrivateShares" - ); + WstsNetMessage::DkgPrivateShares(request) => { + span.record("dkg_id", request.dkg_id); + span.record("dkg_signer_id", request.signer_id); + + tracing::debug!("responding to dkg-private-shares"); + let id = StateMachineId::from(bitcoin_chain_tip); - self.validate_sender(&id, dkg_private_shares.signer_id, &msg_public_key)?; - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.validate_sender(&id, request.signer_id, &msg_public_key)?; + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } - WstsNetMessage::DkgEndBegin(_) => { - tracing::info!("handling DkgEndBegin"); + WstsNetMessage::DkgEndBegin(request) => { + span.record("dkg_id", request.dkg_id); + if !chain_tip_report.sender_is_coordinator { tracing::warn!("received coordinator message from non-coordinator signer"); return Ok(()); } + + tracing::debug!("responding to dkg-end-begin"); + let id = StateMachineId::from(bitcoin_chain_tip); - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } + WstsNetMessage::DkgEnd(request) => { + span.record("dkg_id", request.dkg_id); + span.record("dkg_signer_id", request.signer_id); + + match &request.status { + DkgStatus::Success => { + tracing::info!("signer reports successful dkg round"); + } + DkgStatus::Failure(fail) => { + // TODO(#414): handle DKG failure + tracing::warn!(reason = ?fail, "signer reports failed dkg round"); + } + } + } WstsNetMessage::NonceRequest(request) => { - tracing::info!("handling NonceRequest"); + span.record("dkg_id", request.dkg_id); + span.record("dkg_sign_id", request.sign_id); + span.record("dkg_iter_id", request.sign_iter_id); + if !chain_tip_report.sender_is_coordinator { tracing::warn!("received coordinator message from non-coordinator signer"); return Ok(()); } + tracing::debug!(signature_type = ?request.signature_type, "responding to nonce-request"); + let db = self.context.get_storage(); let accepted_sighash = Self::validate_bitcoin_sign_request(&db, &request.message).await; @@ -623,51 +664,34 @@ where .await?; self.wsts_state_machines.put(id, state_machine); - self.relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; } WstsNetMessage::SignatureShareRequest(request) => { - tracing::info!("handling SignatureShareRequest"); + span.record("dkg_id", request.dkg_id); + span.record("dkg_sign_id", request.sign_id); + span.record("dkg_iter_id", request.sign_iter_id); + if !chain_tip_report.sender_is_coordinator { tracing::warn!("received coordinator message from non-coordinator signer"); return Ok(()); } + tracing::debug!(signature_type = ?request.signature_type, "responding to signature-share-request"); + let db = self.context.get_storage(); let accepted_sighash = Self::validate_bitcoin_sign_request(&db, &request.message).await?; let id = accepted_sighash.sighash.into(); let response = self - .relay_message(id, msg.txid, &msg.inner, bitcoin_chain_tip) + .relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await; self.wsts_state_machines.pop(&id); response?; } - WstsNetMessage::DkgEnd(dkg_end) => { - match &dkg_end.status { - DkgStatus::Success => { - tracing::info!( - sender_public_key = %msg_public_key, - signer_id = %dkg_end.signer_id, - "handling DkgEnd success from signer" - ); - } - DkgStatus::Failure(fail) => { - // TODO(#414): handle DKG failure - tracing::info!( - sender_public_key = %msg_public_key, - signer_id = %dkg_end.signer_id, - reason = ?fail, - "handling DkgEnd failure", - ); - } - } - } - WstsNetMessage::NonceResponse(_) | WstsNetMessage::SignatureShareResponse(_) => { - tracing::trace!("ignoring message"); - } + WstsNetMessage::NonceResponse(_) | WstsNetMessage::SignatureShareResponse(_) => {} } Ok(()) @@ -739,12 +763,12 @@ where #[tracing::instrument(skip_all)] async fn relay_message( &mut self, - id: StateMachineId, - txid: bitcoin::Txid, + state_machine_id: StateMachineId, + wsts_id: WstsMessageId, msg: &WstsNetMessage, bitcoin_chain_tip: &model::BitcoinBlockHash, ) -> Result<(), Error> { - let Some(state_machine) = self.wsts_state_machines.get_mut(&id) else { + let Some(state_machine) = self.wsts_state_machines.get_mut(&state_machine_id) else { tracing::warn!("missing signing round"); return Err(Error::MissingStateMachine); }; @@ -764,10 +788,10 @@ where // whether it has truly received all relevant messages from its // peers. if let WstsNetMessage::DkgEnd(DkgEnd { status: DkgStatus::Success, .. }) = outbound { - self.store_dkg_shares(&id).await?; - self.wsts_state_machines.pop(&id); + self.store_dkg_shares(&state_machine_id).await?; + self.wsts_state_machines.pop(&state_machine_id); } - let msg = message::WstsMessage { txid, inner: outbound }; + let msg = message::WstsMessage { id: wsts_id, inner: outbound }; self.send_message(msg, bitcoin_chain_tip).await?; } @@ -1129,7 +1153,7 @@ mod tests { // Create a DkgBegin message to be handled by the signer. let msg = message::WstsMessage { - txid: Txid::all_zeros(), + id: Txid::all_zeros().into(), inner: WstsNetMessage::DkgBegin(wsts::net::DkgBegin { dkg_id: 0 }), }; diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index 2f8d29ea0..6c474ca8d 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -1,6 +1,7 @@ //! Utilities for constructing and loading WSTS state machines use std::collections::BTreeMap; +use std::future::Future; use crate::codec::Decode as _; use crate::codec::Encode as _; @@ -14,14 +15,22 @@ use crate::storage; use crate::storage::model; use crate::storage::model::SigHash; -use bitcoin::hashes::Hash as _; use hashbrown::HashMap; use hashbrown::HashSet; use wsts::common::PolyCommitment; +use wsts::net::Message; +use wsts::net::Packet; +use wsts::net::SignatureType; +use wsts::state_machine::coordinator::fire; +use wsts::state_machine::coordinator::frost; +use wsts::state_machine::coordinator::Config; use wsts::state_machine::coordinator::Coordinator as _; +use wsts::state_machine::coordinator::SavedState; use wsts::state_machine::coordinator::State as WstsState; +use wsts::state_machine::OperationResult; use wsts::state_machine::StateMachine as _; use wsts::traits::Signer as _; +use wsts::v2::Aggregator; /// An identifier for signer state machines. /// @@ -30,25 +39,435 @@ use wsts::traits::Signer as _; /// hash bytes while for the signing rounds we identify the state machine /// by the sighash bytes. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct StateMachineId([u8; 32]); +pub enum StateMachineId { + /// Identifier for a DKG state machines + Dkg(model::BitcoinBlockHash), + /// Identifier for a Bitcoin signing state machines + BitcoinSign(SigHash), + /// Identifier for a rotate key verification signing round + RotateKey(PublicKeyXOnly, model::BitcoinBlockHash), + /// Identifier for arbitrary signing state machines + ArbitrarySign([u8; 32]), +} impl From<&model::BitcoinBlockHash> for StateMachineId { fn from(value: &model::BitcoinBlockHash) -> Self { - StateMachineId(value.to_byte_array()) + StateMachineId::Dkg(*value) } } impl From for StateMachineId { fn from(value: SigHash) -> Self { - StateMachineId(value.to_byte_array()) + StateMachineId::BitcoinSign(value) + } +} + +impl From<[u8; 32]> for StateMachineId { + fn from(value: [u8; 32]) -> Self { + StateMachineId::ArbitrarySign(value) + } +} + +/// A trait for converting a message into another type. +pub trait FromMessage { + /// Convert the given message into the implementing type. + fn from_message(message: &Message) -> Self + where + Self: Sized; +} + +impl FromMessage for Packet { + fn from_message(message: &Message) -> Self { + Packet { + msg: message.clone(), + sig: Vec::new(), + } + } +} + +/// Wrapper for a WSTS FIRE coordinator state machine. +#[derive(Debug, Clone, PartialEq)] +pub struct FireCoordinator(fire::Coordinator); + +impl std::ops::Deref for FireCoordinator { + type Target = fire::Coordinator; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::ops::DerefMut for FireCoordinator { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +/// Wrapper for a WSTS FROST coordinator state machine. +#[derive(Debug, Clone, PartialEq)] +pub struct FrostCoordinator(frost::Coordinator); + +impl std::ops::Deref for FrostCoordinator { + type Target = frost::Coordinator; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::ops::DerefMut for FrostCoordinator { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +/// A trait for WSTS state machines. +pub trait WstsCoordinator +where + Self: Sized, +{ + /// Creates a new coordinator state machine. + fn new(signers: I, threshold: u16, message_private_key: PrivateKey) -> Self + where + I: IntoIterator; + + /// Gets the coordinator configuration. + fn get_config(&self) -> Config; + + /// Save the state required to reconstruct the state machine. + fn save(&self) -> SavedState; + + /// Create a new coordinator state machine from the given aggregate + /// key. + /// + /// # Notes + /// + /// The `WstsCoordinator` is a state machine that is responsible for + /// DKG and for facilitating signing rounds. When created the + /// `WstsCoordinator` state machine starts off in the `IDLE` state, + /// where you can either start a signing round or start DKG. This + /// function is for loading the state with the assumption that DKG has + /// already been successfully completed. + fn load( + storage: &S, + aggregate_key: PublicKeyXOnly, + signer_public_keys: impl IntoIterator + Send, + threshold: u16, + signer_private_key: PrivateKey, + ) -> impl Future> + Send + where + S: storage::DbRead + Send + Sync; + + /// Process the given message. + fn process_message( + &mut self, + message: &Message, + ) -> Result<(Option, Option), Error> { + let packet = Packet::from_message(message); + self.process_packet(&packet) + } + + /// Process inbound messages + fn process_inbound_messages( + &mut self, + messages: &[Message], + ) -> Result<(Vec, Vec), Error> { + let packets = messages + .iter() + .map(Packet::from_message) + .collect::>(); + self.process_inbound_packets(&packets) } + + /// Process the given packet. + fn process_packet( + &mut self, + packet: &Packet, + ) -> Result<(Option, Option), Error>; + + /// Process inbound packets + fn process_inbound_packets( + &mut self, + packets: &[Packet], + ) -> Result<(Vec, Vec), Error>; + + /// Start a signing round with the given message and signature type. + fn start_signing_round( + &mut self, + message: &[u8], + signature_type: SignatureType, + ) -> Result; } -#[cfg(any(test, feature = "testing"))] -impl StateMachineId { - /// Create a new identifier - pub fn new(value: [u8; 32]) -> Self { - StateMachineId(value) +impl WstsCoordinator for FireCoordinator { + fn new(signers: I, threshold: u16, message_private_key: PrivateKey) -> Self + where + I: IntoIterator, + { + let signer_public_keys: hashbrown::HashMap = signers + .into_iter() + .enumerate() + .map(|(idx, key)| (idx as u32, key.into())) + .collect(); + + // The number of possible signers is capped at a number well below + // u32::MAX, so this conversion should always work. + let num_signers: u32 = signer_public_keys + .len() + .try_into() + .expect("the number of signers is greater than u32::MAX?"); + let signer_key_ids = (0..num_signers) + .map(|signer_id| (signer_id, std::iter::once(signer_id + 1).collect())) + .collect(); + let config = wsts::state_machine::coordinator::Config { + num_signers, + num_keys: num_signers, + threshold: threshold as u32, + dkg_threshold: num_signers, + message_private_key: message_private_key.into(), + dkg_public_timeout: None, + dkg_private_timeout: None, + dkg_end_timeout: None, + nonce_timeout: None, + sign_timeout: None, + signer_key_ids, + signer_public_keys, + }; + + let wsts_coordinator = fire::Coordinator::new(config); + Self(wsts_coordinator) + } + + fn get_config(&self) -> Config { + self.0.get_config() + } + + fn save(&self) -> SavedState { + self.0.save() + } + + async fn load( + storage: &S, + aggregate_key: PublicKeyXOnly, + signer_public_keys: impl IntoIterator + Send, + threshold: u16, + signer_private_key: PrivateKey, + ) -> Result + where + S: storage::DbRead + Send + Sync, + { + let encrypted_shares = storage + .get_encrypted_dkg_shares(aggregate_key) + .await? + .ok_or(Error::MissingDkgShares(aggregate_key))?; + + let public_dkg_shares: BTreeMap = + BTreeMap::decode(encrypted_shares.public_shares.as_slice())?; + let party_polynomials = public_dkg_shares + .iter() + .flat_map(|(_, share)| share.comms.clone()) + .collect::>(); + + let mut coordinator = Self::new(signer_public_keys, threshold, signer_private_key); + + let aggregate_key = encrypted_shares.aggregate_key.into(); + coordinator + .set_key_and_party_polynomials(aggregate_key, party_polynomials) + .map_err(Error::wsts_coordinator)?; + coordinator.current_dkg_id = 1; + + coordinator + .move_to(WstsState::Idle) + .map_err(Error::wsts_coordinator)?; + + Ok(coordinator) + } + + fn process_message( + &mut self, + message: &Message, + ) -> Result<(Option, Option), Error> { + let packet = Packet::from_message(message); + self.0 + .process_message(&packet) + .map_err(Error::wsts_coordinator) + } + + fn process_inbound_messages( + &mut self, + messages: &[Message], + ) -> Result<(Vec, Vec), Error> { + let packets = messages + .iter() + .map(Packet::from_message) + .collect::>(); + self.0 + .process_inbound_messages(&packets) + .map_err(Error::wsts_coordinator) + } + + fn process_packet( + &mut self, + packet: &Packet, + ) -> Result<(Option, Option), Error> { + self.0 + .process_message(packet) + .map_err(Error::wsts_coordinator) + } + + fn process_inbound_packets( + &mut self, + packets: &[Packet], + ) -> Result<(Vec, Vec), Error> { + self.0 + .process_inbound_messages(packets) + .map_err(Error::wsts_coordinator) + } + + fn start_signing_round( + &mut self, + message: &[u8], + signature_type: SignatureType, + ) -> Result { + self.0 + .start_signing_round(message, signature_type) + .map_err(Error::wsts_coordinator) + } +} + +impl WstsCoordinator for FrostCoordinator { + fn new(signers: I, threshold: u16, message_private_key: PrivateKey) -> Self + where + I: IntoIterator, + { + let signer_public_keys: hashbrown::HashMap = signers + .into_iter() + .enumerate() + .map(|(idx, key)| (idx as u32, key.into())) + .collect(); + + // The number of possible signers is capped at a number well below + // u32::MAX, so this conversion should always work. + let num_signers: u32 = signer_public_keys + .len() + .try_into() + .expect("the number of signers is greater than u32::MAX?"); + let signer_key_ids = (0..num_signers) + .map(|signer_id| (signer_id, std::iter::once(signer_id + 1).collect())) + .collect(); + let config = wsts::state_machine::coordinator::Config { + num_signers, + num_keys: num_signers, + threshold: threshold as u32, + dkg_threshold: num_signers, + message_private_key: message_private_key.into(), + dkg_public_timeout: None, + dkg_private_timeout: None, + dkg_end_timeout: None, + nonce_timeout: None, + sign_timeout: None, + signer_key_ids, + signer_public_keys, + }; + + let wsts_coordinator = frost::Coordinator::new(config); + Self(wsts_coordinator) + } + + fn get_config(&self) -> Config { + self.0.get_config() + } + + fn save(&self) -> SavedState { + self.0.save() + } + + async fn load( + storage: &S, + aggregate_key: PublicKeyXOnly, + signer_public_keys: impl IntoIterator + Send, + threshold: u16, + signer_private_key: PrivateKey, + ) -> Result + where + S: storage::DbRead + Send + Sync, + { + let encrypted_shares = storage + .get_encrypted_dkg_shares(aggregate_key) + .await? + .ok_or(Error::MissingDkgShares(aggregate_key))?; + + let public_dkg_shares: BTreeMap = + BTreeMap::decode(encrypted_shares.public_shares.as_slice())?; + let party_polynomials = public_dkg_shares + .iter() + .flat_map(|(_, share)| share.comms.clone()) + .collect::>(); + + let mut coordinator = Self::new(signer_public_keys, threshold, signer_private_key); + + let aggregate_key = encrypted_shares.aggregate_key.into(); + coordinator + .set_key_and_party_polynomials(aggregate_key, party_polynomials) + .map_err(Error::wsts_coordinator)?; + coordinator.current_dkg_id = 1; + + coordinator + .move_to(WstsState::Idle) + .map_err(Error::wsts_coordinator)?; + + Ok(coordinator) + } + + fn process_message( + &mut self, + message: &Message, + ) -> Result<(Option, Option), Error> { + let packet = Packet::from_message(message); + self.0 + .process_message(&packet) + .map_err(Error::wsts_coordinator) + } + + fn process_inbound_messages( + &mut self, + messages: &[Message], + ) -> Result<(Vec, Vec), Error> { + let packets = messages + .iter() + .map(Packet::from_message) + .collect::>(); + self.0 + .process_inbound_messages(&packets) + .map_err(Error::wsts_coordinator) + } + + fn process_packet( + &mut self, + packet: &Packet, + ) -> Result<(Option, Option), Error> { + self.0 + .process_message(packet) + .map_err(Error::wsts_coordinator) + } + + fn process_inbound_packets( + &mut self, + packets: &[Packet], + ) -> Result<(Vec, Vec), Error> { + self.0 + .process_inbound_messages(packets) + .map_err(Error::wsts_coordinator) + } + + fn start_signing_round( + &mut self, + message: &[u8], + signature_type: SignatureType, + ) -> Result { + self.0 + .start_signing_round(message, signature_type) + .map_err(Error::wsts_coordinator) } } @@ -56,7 +475,7 @@ impl StateMachineId { #[derive(Debug, Clone, PartialEq)] pub struct SignerStateMachine(wsts::state_machine::signer::Signer); -type WstsStateMachine = wsts::state_machine::signer::Signer; +type WstsSigner = wsts::state_machine::signer::Signer; impl SignerStateMachine { /// Create a new state machine @@ -112,7 +531,7 @@ impl SignerStateMachine { return Err(error::Error::InvalidConfiguration); }; - let state_machine = WstsStateMachine::new( + let state_machine = WstsSigner::new( threshold, dkg_threshold, num_parties, @@ -211,7 +630,7 @@ impl SignerStateMachine { } impl std::ops::Deref for SignerStateMachine { - type Target = WstsStateMachine; + type Target = WstsSigner; fn deref(&self) -> &Self::Target { &self.0 @@ -223,115 +642,3 @@ impl std::ops::DerefMut for SignerStateMachine { &mut self.0 } } - -/// Wrapper around a WSTS coordinator state machine -#[derive(Debug, Clone, PartialEq)] -pub struct CoordinatorStateMachine(WstsCoordinator); - -type WstsCoordinator = wsts::state_machine::coordinator::fire::Coordinator; - -impl CoordinatorStateMachine { - /// Create a new state machine - pub fn new(signers: I, threshold: u16, message_private_key: PrivateKey) -> Self - where - I: IntoIterator, - { - let signer_public_keys: hashbrown::HashMap = signers - .into_iter() - .enumerate() - .map(|(idx, key)| (idx as u32, key.into())) - .collect(); - - // The number of possible signers is capped at a number well below - // u32::MAX, so this conversion should always work. - let num_signers: u32 = signer_public_keys - .len() - .try_into() - .expect("the number of signers is greater than u32::MAX?"); - let signer_key_ids = (0..num_signers) - .map(|signer_id| (signer_id, std::iter::once(signer_id + 1).collect())) - .collect(); - let config = wsts::state_machine::coordinator::Config { - num_signers, - num_keys: num_signers, - threshold: threshold as u32, - dkg_threshold: num_signers, - message_private_key: message_private_key.into(), - dkg_public_timeout: None, - dkg_private_timeout: None, - dkg_end_timeout: None, - nonce_timeout: None, - sign_timeout: None, - signer_key_ids, - signer_public_keys, - }; - - let wsts_coordinator = WstsCoordinator::new(config); - Self(wsts_coordinator) - } - - /// Create a new coordinator state machine from the given aggregate - /// key. - /// - /// # Notes - /// - /// The `WstsCoordinator` is a state machine that is responsible for - /// DKG and for facilitating signing rounds. When created the - /// `WstsCoordinator` state machine starts off in the `IDLE` state, - /// where you can either start a signing round or start DKG. This - /// function is for loading the state with the assumption that DKG has - /// already been successfully completed. - pub async fn load( - storage: &mut S, - aggregate_key: X, - signers: I, - threshold: u16, - message_private_key: PrivateKey, - ) -> Result - where - I: IntoIterator, - S: storage::DbRead + storage::DbWrite, - X: Into, - { - let x_only_aggregate_key: PublicKeyXOnly = aggregate_key.into(); - let encrypted_shares = storage - .get_encrypted_dkg_shares(x_only_aggregate_key) - .await? - .ok_or(Error::MissingDkgShares(x_only_aggregate_key))?; - - let public_dkg_shares: BTreeMap = - BTreeMap::decode(encrypted_shares.public_shares.as_slice())?; - let party_polynomials = public_dkg_shares - .iter() - .flat_map(|(_, share)| share.comms.clone()) - .collect::>(); - - let mut coordinator = Self::new(signers, threshold, message_private_key); - - let aggregate_key = encrypted_shares.aggregate_key.into(); - coordinator - .set_key_and_party_polynomials(aggregate_key, party_polynomials) - .map_err(Error::wsts_coordinator)?; - coordinator.current_dkg_id = 1; - - coordinator - .move_to(WstsState::Idle) - .map_err(Error::wsts_coordinator)?; - - Ok(coordinator) - } -} - -impl std::ops::Deref for CoordinatorStateMachine { - type Target = WstsCoordinator; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl std::ops::DerefMut for CoordinatorStateMachine { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index f395961bd..e53024910 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -20,6 +20,7 @@ use signer::keys::PublicKey; use signer::message::BitcoinPreSignRequest; use signer::message::StacksTransactionSignRequest; use signer::message::WstsMessage; +use signer::message::WstsMessageId; use signer::network::in_memory2::WanNetwork; use signer::network::InMemoryNetwork; use signer::network::MessageTransfer; @@ -397,13 +398,14 @@ async fn new_state_machine_per_valid_sighash() { // The message that we will send is for the following sighash. We'll // need to make sure that it is in our database first - let sighash_message = [1; 32]; + let txid: BitcoinTxId = Faker.fake_with_rng(&mut rng); + let sighash: SigHash = Faker.fake_with_rng(&mut rng); let row = BitcoinTxSigHash { - txid: BitcoinTxId::from([0; 32]), + txid: txid.clone(), chain_tip: BitcoinBlockHash::from([0; 32]), prevout_txid: BitcoinTxId::from([0; 32]), prevout_output_index: 0, - sighash: SigHash::from(bitcoin::TapSighash::from_byte_array(sighash_message)), + sighash, prevout_type: model::TxPrevoutType::Deposit, validation_result: signer::bitcoin::validation::InputValidationResult::Ok, is_valid_tx: true, @@ -415,12 +417,12 @@ async fn new_state_machine_per_valid_sighash() { // Now for the nonce request message let mut nonce_request_msg = WstsMessage { - txid: bitcoin::Txid::all_zeros(), + id: WstsMessageId::BitcoinTxid(*txid), inner: wsts::net::Message::NonceRequest(NonceRequest { dkg_id: 1, sign_id: 1, sign_iter_id: 1, - message: sighash_message.to_vec(), + message: sighash.to_byte_array().to_vec(), signature_type: wsts::net::SignatureType::Schnorr, }), }; @@ -441,7 +443,7 @@ async fn new_state_machine_per_valid_sighash() { // We should have a state machine associated with the sighash nonce // request message that we just received. - let id1 = StateMachineId::new(sighash_message); + let id1 = StateMachineId::BitcoinSign(sighash); assert!(tx_signer.wsts_state_machines.contains(&id1)); assert_eq!(tx_signer.wsts_state_machines.len(), 1); @@ -449,10 +451,10 @@ async fn new_state_machine_per_valid_sighash() { // for a sighash that we do not know about. Since the nonce request is // not in the database we should return an error, and the state machine // should not be in the local cache. - let random_message: [u8; 32] = Faker.fake_with_rng(&mut rng); + let random_sighash: SigHash = Faker.fake_with_rng(&mut rng); match &mut nonce_request_msg.inner { wsts::net::Message::NonceRequest(NonceRequest { message, .. }) => { - *message = random_message.to_vec(); + *message = random_sighash.as_byte_array().to_vec(); } _ => panic!("You forgot to update the variant"), }; @@ -466,7 +468,7 @@ async fn new_state_machine_per_valid_sighash() { ) .await; - let id2 = StateMachineId::new(random_message); + let id2 = StateMachineId::BitcoinSign(Faker.fake()); assert!(response.is_err()); assert!(tx_signer.wsts_state_machines.contains(&id1)); assert!(!tx_signer.wsts_state_machines.contains(&id2)); @@ -520,7 +522,7 @@ async fn max_one_state_machine_per_bitcoin_block_hash_for_dkg() { // arbitrary transaction ID. let dkg_id = 2; let dkg_begin_msg = WstsMessage { - txid: bitcoin::Txid::all_zeros(), + id: bitcoin::Txid::all_zeros().into(), inner: wsts::net::Message::DkgBegin(DkgBegin { dkg_id }), }; let msg_public_key = PublicKey::from_private_key(&PrivateKey::new(&mut rng)); @@ -545,7 +547,7 @@ async fn max_one_state_machine_per_bitcoin_block_hash_for_dkg() { // machine gets created, overwriting any existing one. let dkg_id = 1234; let dkg_begin_msg = WstsMessage { - txid: bitcoin::Txid::from_byte_array(Faker.fake_with_rng(&mut rng)), + id: bitcoin::Txid::from_byte_array(Faker.fake_with_rng(&mut rng)).into(), inner: wsts::net::Message::DkgBegin(DkgBegin { dkg_id }), }; diff --git a/supply-chain/config.toml b/supply-chain/config.toml index d6422dcba..8bb6ad780 100644 --- a/supply-chain/config.toml +++ b/supply-chain/config.toml @@ -2735,7 +2735,7 @@ version = "0.2.1" criteria = "safe-to-deploy" [[exemptions.wsts]] -version = "10.0.0@git:53ae23f5f35def420877ccc8c0fe3662e64e38a1" +version = "10.0.0@git:a7bf38bd54cddf0b78c0f7c521a5ed1537a684fa" criteria = "safe-to-deploy" [[exemptions.wyz]] From c742af2d0467203e4713fc2b854d438f8166e579 Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Thu, 30 Jan 2025 04:59:10 +0000 Subject: [PATCH 02/14] think that's it --- protobufs/stacks/signer/v1/messages.proto | 1 - signer/src/message.rs | 12 +++++------- signer/src/proto/convert.rs | 9 ++------- signer/src/proto/generated/stacks.signer.v1.rs | 6 ++---- signer/src/transaction_coordinator.rs | 10 ++++------ signer/src/wsts_state_machine.rs | 8 -------- 6 files changed, 13 insertions(+), 33 deletions(-) diff --git a/protobufs/stacks/signer/v1/messages.proto b/protobufs/stacks/signer/v1/messages.proto index 4f85d4a75..b4bbda046 100644 --- a/protobufs/stacks/signer/v1/messages.proto +++ b/protobufs/stacks/signer/v1/messages.proto @@ -63,7 +63,6 @@ message WstsMessage { oneof id { bitcoin.BitcoinTxid id_bitcoin_txid = 12; crypto.PublicKey id_rotate_key = 13; - bytes id_arbitrary = 14; } } diff --git a/signer/src/message.rs b/signer/src/message.rs index e8786b415..a3681a336 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -235,8 +235,6 @@ pub enum WstsMessageId { BitcoinTxid(bitcoin::Txid), /// The WSTS message is related to a rotate key operation. RotateKey(PublicKey), - /// The WSTS message isn't specifically related to anything. - Arbitrary([u8; 32]), } impl From for WstsMessageId { @@ -245,12 +243,13 @@ impl From for WstsMessageId { } } -impl WstsMessageId { - /// Generate a random [`WstsMessageId::Arbitrary`] WSTS message ID - pub fn random_arbitrary() -> Self { - Self::Arbitrary(OsRng.gen()) +impl From for WstsMessageId { + fn from(txid: crate::storage::model::BitcoinTxId) -> Self { + Self::BitcoinTxid(txid.into()) } +} +impl WstsMessageId { /// Generate a random [`WstsMessageId::BitcoinTxid`] WSTS message ID pub fn random_bitcoin_txid() -> Self { let random_bytes: [u8; 32] = OsRng.gen(); @@ -273,7 +272,6 @@ impl std::fmt::Display for WstsMessageId { WstsMessageId::RotateKey(aggregate_key) => { write!(f, "rotate-key({})", aggregate_key) } - WstsMessageId::Arbitrary(bytes) => write!(f, "arbitrary({})", hex::encode(bytes)), } } } diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 2e24f1338..2b779d14c 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -69,7 +69,6 @@ use crate::storage::model::QualifiedRequestId; use crate::storage::model::StacksBlockHash; use crate::storage::model::StacksPrincipal; use crate::storage::model::StacksTxId; -use crate::storage::model::ToLittleEndianOrder; use super::wsts_message; @@ -1100,16 +1099,15 @@ impl From for proto::WstsMessage { WstsMessageId::BitcoinTxid(txid) => { Some(proto::BitcoinTxid::from(BitcoinTxId::from(txid))) } - WstsMessageId::RotateKey(_) | WstsMessageId::Arbitrary(_) => None, + WstsMessageId::RotateKey(_) => None, }, id: Some(match value.id { WstsMessageId::BitcoinTxid(txid) => { wsts_message::Id::IdBitcoinTxid(proto::BitcoinTxid { - txid: Some(proto::Uint256::from(txid.to_le_bytes())), + txid: Some(proto::Uint256::from(BitcoinTxId::from(txid).into_bytes())), }) } WstsMessageId::RotateKey(pubkey) => wsts_message::Id::IdRotateKey(pubkey.into()), - WstsMessageId::Arbitrary(key) => wsts_message::Id::IdArbitrary(key.into()), }), inner: Some(inner), } @@ -1163,9 +1161,6 @@ impl TryFrom for WstsMessage { wsts_message::Id::IdRotateKey(pubkey) => { WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) } - wsts_message::Id::IdArbitrary(key) => { - WstsMessageId::Arbitrary(key.try_into().map_err(|_| Error::TypeConversion)?) - } }, None => WstsMessageId::BitcoinTxid( BitcoinTxId::try_from(value.txid.required()?)?.into(), diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index 784947f74..cd3093e31 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -293,7 +293,7 @@ pub struct WstsMessage { /// The wsts message #[prost(oneof = "wsts_message::Inner", tags = "2, 3, 4, 5, 6, 7, 8, 9, 10, 11")] pub inner: ::core::option::Option, - #[prost(oneof = "wsts_message::Id", tags = "12, 13, 14")] + #[prost(oneof = "wsts_message::Id", tags = "12, 13")] pub id: ::core::option::Option, } /// Nested message and enum types in `WstsMessage`. @@ -338,14 +338,12 @@ pub mod wsts_message { super::super::super::super::crypto::wsts::SignatureShareResponse, ), } - #[derive(Clone, PartialEq, ::prost::Oneof)] + #[derive(Clone, Copy, PartialEq, ::prost::Oneof)] pub enum Id { #[prost(message, tag = "12")] IdBitcoinTxid(super::super::super::super::bitcoin::BitcoinTxid), #[prost(message, tag = "13")] IdRotateKey(super::super::super::super::crypto::PublicKey), - #[prost(bytes, tag = "14")] - IdArbitrary(::prost::alloc::vec::Vec), } } /// Wraps an inner type with a public key and a signature, diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 113d16aa5..c860065f0 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -916,7 +916,7 @@ where ) -> Result<(), Error> { let sighashes = transaction.construct_digests()?; let mut coordinator_state_machine = FireCoordinator::load( - &mut self.context.get_storage_mut(), + &self.context.get_storage_mut(), sighashes.signers_aggregate_key.into(), signer_public_keys.clone(), self.threshold, @@ -959,7 +959,7 @@ where let msg = sighash.to_raw_hash().to_byte_array(); let mut coordinator_state_machine = FireCoordinator::load( - &mut self.context.get_storage_mut(), + &self.context.get_storage_mut(), deposit.signers_public_key.into(), signer_public_keys.clone(), self.threshold, @@ -1046,8 +1046,7 @@ where where Coordinator: WstsCoordinator, { - let outbound = coordinator_state_machine - .start_signing_round(msg, signature_type)?; + let outbound = coordinator_state_machine.start_signing_round(msg, signature_type)?; // We create a signal stream before sending a message so that there // is no race condition with the steam and the getting a response. @@ -1098,8 +1097,7 @@ where // never changing the signing set. let (_, signer_set) = self.get_signer_set_and_aggregate_key(chain_tip).await?; - let mut state_machine = - FireCoordinator::new(signer_set, self.threshold, self.private_key); + let mut state_machine = FireCoordinator::new(signer_set, self.threshold, self.private_key); // Okay let's move the coordinator state machine to the beginning // of the DKG phase. diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index 6c474ca8d..176676bdc 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -46,8 +46,6 @@ pub enum StateMachineId { BitcoinSign(SigHash), /// Identifier for a rotate key verification signing round RotateKey(PublicKeyXOnly, model::BitcoinBlockHash), - /// Identifier for arbitrary signing state machines - ArbitrarySign([u8; 32]), } impl From<&model::BitcoinBlockHash> for StateMachineId { @@ -62,12 +60,6 @@ impl From for StateMachineId { } } -impl From<[u8; 32]> for StateMachineId { - fn from(value: [u8; 32]) -> Self { - StateMachineId::ArbitrarySign(value) - } -} - /// A trait for converting a message into another type. pub trait FromMessage { /// Convert the given message into the implementing type. From 347db71b089913110574e96132069b0cf0032425 Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Thu, 30 Jan 2025 10:07:28 +0000 Subject: [PATCH 03/14] add message support for a specific dkg wstsmessageid --- protobufs/stacks/signer/v1/messages.proto | 8 +++ signer/src/message.rs | 9 ++- signer/src/proto/convert.rs | 62 ++++++++++++------- .../src/proto/generated/stacks.signer.v1.rs | 11 +++- 4 files changed, 63 insertions(+), 27 deletions(-) diff --git a/protobufs/stacks/signer/v1/messages.proto b/protobufs/stacks/signer/v1/messages.proto index b4bbda046..128036248 100644 --- a/protobufs/stacks/signer/v1/messages.proto +++ b/protobufs/stacks/signer/v1/messages.proto @@ -61,8 +61,16 @@ message WstsMessage { crypto.wsts.SignatureShareResponse signature_share_response = 11; } oneof id { + // If this WSTS message is related to a Bitcoin signing round, this field + // will be set to the related Bitcoin transaction ID. bitcoin.BitcoinTxid id_bitcoin_txid = 12; + // If this WSTS message is related to a rotate-keys transaction, this field + // will be set to the _new_ aggregate public key being verified. crypto.PublicKey id_rotate_key = 13; + // If this WSTS message is related to a DKG round, this field will be set + // to the 32-byte id determined based on the coordinator public key and + // block hash, set by the coordinator. + crypto.Uint256 id_dkg = 14; } } diff --git a/signer/src/message.rs b/signer/src/message.rs index a3681a336..72951aacc 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -231,10 +231,12 @@ pub struct BitcoinPreSignAck; /// The identifier for a WSTS message. #[derive(Debug, Clone, Copy, PartialEq)] pub enum WstsMessageId { - /// The WSTS message is related to a Bitcoin transaction. + /// The WSTS message is related to a Bitcoin transaction signing round. BitcoinTxid(bitcoin::Txid), - /// The WSTS message is related to a rotate key operation. + /// The WSTS message is related to a rotate key verification operation. RotateKey(PublicKey), + /// The WSTS message is related to a DKG round. + Dkg([u8; 32]), } impl From for WstsMessageId { @@ -271,6 +273,9 @@ impl std::fmt::Display for WstsMessageId { WstsMessageId::BitcoinTxid(txid) => write!(f, "bitcoin-txid({})", txid), WstsMessageId::RotateKey(aggregate_key) => { write!(f, "rotate-key({})", aggregate_key) + }, + WstsMessageId::Dkg(id) => { + write!(f, "dkg({})", hex::encode(id)) } } } diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 2b779d14c..2ebc66351 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -1098,7 +1098,8 @@ impl From for proto::WstsMessage { txid: match value.id { WstsMessageId::BitcoinTxid(txid) => { Some(proto::BitcoinTxid::from(BitcoinTxId::from(txid))) - } + }, + WstsMessageId::Dkg(_) => None, WstsMessageId::RotateKey(_) => None, }, id: Some(match value.id { @@ -1108,6 +1109,7 @@ impl From for proto::WstsMessage { }) } WstsMessageId::RotateKey(pubkey) => wsts_message::Id::IdRotateKey(pubkey.into()), + WstsMessageId::Dkg(id) => wsts_message::Id::IdDkg(id.into()), }), inner: Some(inner), } @@ -1161,6 +1163,9 @@ impl TryFrom for WstsMessage { wsts_message::Id::IdRotateKey(pubkey) => { WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) } + wsts_message::Id::IdDkg(id) => { + WstsMessageId::Dkg(id.into()) + } }, None => WstsMessageId::BitcoinTxid( BitcoinTxId::try_from(value.txid.required()?)?.into(), @@ -1770,21 +1775,24 @@ mod tests { U: From, E: std::fmt::Debug, { - // The type T originates from a signer. Let's create a random - // instance of one. - let original: T = Faker.fake_with_rng(&mut OsRng); - // The type U is a protobuf type. Before sending it to other - // signers, we convert our internal type into it's protobuf - // counterpart. We can always infallibly create U from T. - let proto_original = U::from(original.clone()); - - // Some other signer receives an instance of U. This could be a - // malicious actor or a modified version of the signer binary - // where they made some mistake, so converting back to T can fail. - let original_from_proto = T::try_from(proto_original).unwrap(); - // In this case, we know U was created from T correctly, so we - // should be able to convert back without issues. - assert_eq!(original, original_from_proto); + // TODO: proptest + for _ in 0..25 { + // The type T originates from a signer. Let's create a random + // instance of one. + let original: T = Faker.fake_with_rng(&mut OsRng); + // The type U is a protobuf type. Before sending it to other + // signers, we convert our internal type into it's protobuf + // counterpart. We can always infallibly create U from T. + let proto_original = U::from(original.clone()); + + // Some other signer receives an instance of U. This could be a + // malicious actor or a modified version of the signer binary + // where they made some mistake, so converting back to T can fail. + let original_from_proto = T::try_from(proto_original).unwrap(); + // In this case, we know U was created from T correctly, so we + // should be able to convert back without issues. + assert_eq!(original, original_from_proto); + } } /// This test is identical to [`convert_protobuf_types`] tests above, @@ -1825,11 +1833,14 @@ mod tests { U: From, E: std::fmt::Debug, { - let original: T = Unit.fake_with_rng(&mut OsRng); - let proto_original = U::from(original.clone()); + // TODO: proptest + for _ in 0..10 { + let original: T = Unit.fake_with_rng(&mut OsRng); + let proto_original = U::from(original.clone()); - let original_from_proto = T::try_from(proto_original).unwrap(); - assert_eq!(original, original_from_proto); + let original_from_proto = T::try_from(proto_original).unwrap(); + assert_eq!(original, original_from_proto); + } } // The following are tests for structs that do not derive eq @@ -1875,11 +1886,14 @@ mod tests { U: From, E: std::fmt::Debug, { - let original: T = Unit.fake_with_rng(&mut OsRng); - let proto_original = U::from(original.clone()); + // TODO: proptest + for _ in 0..25 { + let original: T = Unit.fake_with_rng(&mut OsRng); + let proto_original = U::from(original.clone()); - let original_from_proto = T::try_from(proto_original).unwrap(); - assert_eq!(wrapper(original), wrapper(original_from_proto)); + let original_from_proto = T::try_from(proto_original).unwrap(); + assert_eq!(wrapper(original), wrapper(original_from_proto)); + } } #[test] diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index cd3093e31..a9d67ee4d 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -293,7 +293,7 @@ pub struct WstsMessage { /// The wsts message #[prost(oneof = "wsts_message::Inner", tags = "2, 3, 4, 5, 6, 7, 8, 9, 10, 11")] pub inner: ::core::option::Option, - #[prost(oneof = "wsts_message::Id", tags = "12, 13")] + #[prost(oneof = "wsts_message::Id", tags = "12, 13, 14")] pub id: ::core::option::Option, } /// Nested message and enum types in `WstsMessage`. @@ -340,10 +340,19 @@ pub mod wsts_message { } #[derive(Clone, Copy, PartialEq, ::prost::Oneof)] pub enum Id { + /// If this WSTS message is related to a Bitcoin signing round, this field + /// will be set to the related Bitcoin transaction ID. #[prost(message, tag = "12")] IdBitcoinTxid(super::super::super::super::bitcoin::BitcoinTxid), + /// If this WSTS message is related to a rotate-keys transaction, this field + /// will be set to the _new_ aggregate public key being verified. #[prost(message, tag = "13")] IdRotateKey(super::super::super::super::crypto::PublicKey), + /// If this WSTS message is related to a DKG round, this field will be set + /// to the 32-byte id determined based on the coordinator public key and + /// block hash, set by the coordinator. + #[prost(message, tag = "14")] + IdDkg(super::super::super::super::crypto::Uint256), } } /// Wraps an inner type with a public key and a signature, From df5111fd6f349cddd44c18ccdcc05ba48c91c162 Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Thu, 30 Jan 2025 10:15:48 +0000 Subject: [PATCH 04/14] add a dkg wstsmessageid variant --- signer/src/message.rs | 22 +--------------------- signer/src/proto/convert.rs | 6 ++---- signer/src/transaction_coordinator.rs | 7 +++---- signer/src/transaction_signer.rs | 7 +++---- 4 files changed, 9 insertions(+), 33 deletions(-) diff --git a/signer/src/message.rs b/signer/src/message.rs index 72951aacc..7d3d4d21f 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -1,18 +1,14 @@ //! Signer message definition for network communication -use rand::rngs::OsRng; -use rand::Rng; use secp256k1::ecdsa::RecoverableSignature; use crate::bitcoin::utxo::Fees; use crate::bitcoin::validation::TxRequestIds; -use crate::keys::PrivateKey; use crate::keys::PublicKey; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::StacksTx; use crate::storage::model; use crate::storage::model::BitcoinBlockHash; -use crate::storage::model::BitcoinTxId; use crate::storage::model::StacksTxId; /// Messages exchanged between signers @@ -251,29 +247,13 @@ impl From for WstsMessageId { } } -impl WstsMessageId { - /// Generate a random [`WstsMessageId::BitcoinTxid`] WSTS message ID - pub fn random_bitcoin_txid() -> Self { - let random_bytes: [u8; 32] = OsRng.gen(); - let txid = BitcoinTxId::from(random_bytes).into(); - Self::BitcoinTxid(txid) - } - - /// Generate a random [`WstsMessageId::AggregateKey`] WSTS message ID - pub fn random_rotate_key() -> Self { - let private = PrivateKey::new(&mut OsRng); - let public = PublicKey::from_private_key(&private); - Self::RotateKey(public) - } -} - impl std::fmt::Display for WstsMessageId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { WstsMessageId::BitcoinTxid(txid) => write!(f, "bitcoin-txid({})", txid), WstsMessageId::RotateKey(aggregate_key) => { write!(f, "rotate-key({})", aggregate_key) - }, + } WstsMessageId::Dkg(id) => { write!(f, "dkg({})", hex::encode(id)) } diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 2ebc66351..1d5c79088 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -1098,7 +1098,7 @@ impl From for proto::WstsMessage { txid: match value.id { WstsMessageId::BitcoinTxid(txid) => { Some(proto::BitcoinTxid::from(BitcoinTxId::from(txid))) - }, + } WstsMessageId::Dkg(_) => None, WstsMessageId::RotateKey(_) => None, }, @@ -1163,9 +1163,7 @@ impl TryFrom for WstsMessage { wsts_message::Id::IdRotateKey(pubkey) => { WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) } - wsts_message::Id::IdDkg(id) => { - WstsMessageId::Dkg(id.into()) - } + wsts_message::Id::IdDkg(id) => WstsMessageId::Dkg(id.into()), }, None => WstsMessageId::BitcoinTxid( BitcoinTxId::try_from(value.txid.required()?)?.into(), diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index c860065f0..6f965e5d6 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -1109,11 +1109,10 @@ where .start_public_shares() .map_err(Error::wsts_coordinator)?; - // We identify the DKG round by a 32-byte hash which we throw - // around as a bitcoin transaction ID, even when it is not one. We - // should probably change this + // We identify the DKG round by a 32-byte hash based on the coordinator + // identity and current bitcoin chain tip. let identifier = self.coordinator_id(chain_tip); - let id = bitcoin::Txid::from_byte_array(identifier).into(); + let id = WstsMessageId::Dkg(identifier); let msg = message::WstsMessage { id, inner: outbound.msg }; // We create a signal stream before sending a message so that there diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 98ad408af..1330b4341 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -612,11 +612,11 @@ where match &request.status { DkgStatus::Success => { - tracing::info!("signer reports successful dkg round"); + tracing::info!(status = "success", "signer reports successful dkg round"); } DkgStatus::Failure(fail) => { // TODO(#414): handle DKG failure - tracing::warn!(reason = ?fail, "signer reports failed dkg round"); + tracing::warn!(status = "failure", reason = ?fail, "signer reports failed dkg round"); } } } @@ -995,7 +995,6 @@ pub enum ChainTipStatus { mod tests { use std::num::{NonZeroU32, NonZeroU64, NonZeroUsize}; - use bitcoin::Txid; use fake::{Fake, Faker}; use network::InMemoryNetwork; use test_case::test_case; @@ -1153,7 +1152,7 @@ mod tests { // Create a DkgBegin message to be handled by the signer. let msg = message::WstsMessage { - id: Txid::all_zeros().into(), + id: WstsMessageId::Dkg(Faker.fake()), inner: WstsNetMessage::DkgBegin(wsts::net::DkgBegin { dkg_id: 0 }), }; From 8f8401648b174b586f3aaa0bf9a59b9ddb9cff0d Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Tue, 4 Feb 2025 12:12:46 +0100 Subject: [PATCH 05/14] fix merge artifacts --- signer/src/transaction_signer.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 6b0cd06ed..5a8bd83f9 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -562,7 +562,6 @@ where WstsNetMessage::DkgPrivateBegin(request) => { span.record("dkg_id", request.dkg_id); - if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( ?chain_tip_report, @@ -602,7 +601,6 @@ where WstsNetMessage::DkgEndBegin(request) => { span.record("dkg_id", request.dkg_id); - if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( ?chain_tip_report, @@ -613,7 +611,6 @@ where tracing::debug!("responding to dkg-end-begin"); - let id = StateMachineId::from(bitcoin_chain_tip); self.relay_message(id, msg.id, &msg.inner, bitcoin_chain_tip) .await?; @@ -637,7 +634,6 @@ where span.record("dkg_sign_id", request.sign_id); span.record("dkg_iter_id", request.sign_iter_id); - if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( ?chain_tip_report, @@ -688,7 +684,6 @@ where span.record("dkg_sign_id", request.sign_id); span.record("dkg_iter_id", request.sign_iter_id); - if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( ?chain_tip_report, @@ -1022,6 +1017,7 @@ pub enum ChainTipStatus { mod tests { use std::num::{NonZeroU32, NonZeroU64, NonZeroUsize}; + use bitcoin::Txid; use fake::{Fake, Faker}; use network::InMemoryNetwork; use test_case::test_case; @@ -1245,7 +1241,7 @@ mod tests { // Create a DkgBegin message to be handled by the signer. let msg = message::WstsMessage { - txid: Txid::all_zeros(), + id: Txid::all_zeros().into(), inner: WstsNetMessage::DkgBegin(wsts::net::DkgBegin { dkg_id: 0 }), }; @@ -1328,7 +1324,7 @@ mod tests { }; let msg = message::WstsMessage { - txid: Txid::all_zeros(), + id: Txid::all_zeros().into(), inner: wsts_message, }; From 8cdb739276f631677f6a1ae4d8b1604ace59a5cd Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Tue, 4 Feb 2025 12:32:42 +0100 Subject: [PATCH 06/14] tracing fields --- signer/src/transaction_signer.rs | 56 ++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 5a8bd83f9..8196c1ff1 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -502,13 +502,12 @@ where /// Process WSTS messages #[tracing::instrument(skip_all, fields( - msg_id = %msg.id, - msg_type = %msg.type_id(), - dkg_signer_id = tracing::field::Empty, - dkg_id = tracing::field::Empty, - dkg_sign_id = tracing::field::Empty, - dkg_iter_id = tracing::field::Empty, - dkg_txid = tracing::field::Empty, + wsts_msg_id = %msg.id, + wsts_msg_type = %msg.type_id(), + wsts_signer_id = tracing::field::Empty, + wsts_dkg_id = tracing::field::Empty, + wsts_sign_id = tracing::field::Empty, + wsts_sign_iter_id = tracing::field::Empty, sender_public_key = %msg_public_key, ))] pub async fn handle_wsts_message( @@ -518,11 +517,17 @@ where msg_public_key: PublicKey, chain_tip_report: &MsgChainTipReport, ) -> Result<(), Error> { + // Constants for tracing. + const WSTS_DKG_ID: &str = "wsts_dkg_id"; + const WSTS_SIGNER_ID: &str = "wsts_signer_id"; + const WSTS_SIGN_ID: &str = "wsts_sign_id"; + const WSTS_SIGN_ITER_ID: &str = "wsts_sign_iter_id"; + // Get the current tracing span. let span = tracing::Span::current(); match &msg.inner { WstsNetMessage::DkgBegin(request) => { - span.record("dkg_id", request.dkg_id); + span.record(WSTS_DKG_ID, request.dkg_id); if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( @@ -560,7 +565,7 @@ where .await?; } WstsNetMessage::DkgPrivateBegin(request) => { - span.record("dkg_id", request.dkg_id); + span.record(WSTS_DKG_ID, request.dkg_id); if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( @@ -577,8 +582,8 @@ where .await?; } WstsNetMessage::DkgPublicShares(request) => { - span.record("dkg_id", request.dkg_id); - span.record("dkg_signer_id", request.signer_id); + span.record(WSTS_DKG_ID, request.dkg_id); + span.record(WSTS_SIGNER_ID, request.signer_id); tracing::debug!("responding to dkg-public-shares"); @@ -588,8 +593,8 @@ where .await?; } WstsNetMessage::DkgPrivateShares(request) => { - span.record("dkg_id", request.dkg_id); - span.record("dkg_signer_id", request.signer_id); + span.record(WSTS_DKG_ID, request.dkg_id); + span.record(WSTS_SIGNER_ID, request.signer_id); tracing::debug!("responding to dkg-private-shares"); @@ -599,7 +604,7 @@ where .await?; } WstsNetMessage::DkgEndBegin(request) => { - span.record("dkg_id", request.dkg_id); + span.record(WSTS_DKG_ID, request.dkg_id); if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( @@ -616,23 +621,26 @@ where .await?; } WstsNetMessage::DkgEnd(request) => { - span.record("dkg_id", request.dkg_id); - span.record("dkg_signer_id", request.signer_id); + span.record(WSTS_DKG_ID, request.dkg_id); + span.record(WSTS_SIGNER_ID, request.signer_id); match &request.status { DkgStatus::Success => { - tracing::info!(status = "success", "signer reports successful dkg round"); + tracing::info!( + wsts_dkg_status = "success", + "signer reports successful dkg round" + ); } DkgStatus::Failure(fail) => { // TODO(#414): handle DKG failure - tracing::warn!(status = "failure", reason = ?fail, "signer reports failed dkg round"); + tracing::warn!(wsts_dkg_status = "failure", reason = ?fail, "signer reports failed dkg round"); } } } WstsNetMessage::NonceRequest(request) => { - span.record("dkg_id", request.dkg_id); - span.record("dkg_sign_id", request.sign_id); - span.record("dkg_iter_id", request.sign_iter_id); + span.record(WSTS_DKG_ID, request.dkg_id); + span.record(WSTS_SIGN_ID, request.sign_id); + span.record(WSTS_SIGN_ITER_ID, request.sign_iter_id); if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( @@ -680,9 +688,9 @@ where .await?; } WstsNetMessage::SignatureShareRequest(request) => { - span.record("dkg_id", request.dkg_id); - span.record("dkg_sign_id", request.sign_id); - span.record("dkg_iter_id", request.sign_iter_id); + span.record(WSTS_DKG_ID, request.dkg_id); + span.record(WSTS_SIGN_ID, request.sign_id); + span.record(WSTS_SIGN_ITER_ID, request.sign_iter_id); if !chain_tip_report.is_from_canonical_coordinator() { tracing::warn!( From 267c5a4694864e3918f6696dcbab6cea8183825e Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Tue, 4 Feb 2025 17:47:36 +0100 Subject: [PATCH 07/14] storage mut --- signer/src/transaction_coordinator.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 6f965e5d6..b9ec7382e 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -916,7 +916,7 @@ where ) -> Result<(), Error> { let sighashes = transaction.construct_digests()?; let mut coordinator_state_machine = FireCoordinator::load( - &self.context.get_storage_mut(), + &self.context.get_storage(), sighashes.signers_aggregate_key.into(), signer_public_keys.clone(), self.threshold, @@ -959,7 +959,7 @@ where let msg = sighash.to_raw_hash().to_byte_array(); let mut coordinator_state_machine = FireCoordinator::load( - &self.context.get_storage_mut(), + &self.context.get_storage(), deposit.signers_public_key.into(), signer_public_keys.clone(), self.threshold, From 716e5c2e7cf4617c5cbd4b73acaa4d30c85aa0bd Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Tue, 4 Feb 2025 17:48:35 +0100 Subject: [PATCH 08/14] lovely cascading changes --- signer/src/testing/transaction_coordinator.rs | 5 +++-- signer/src/testing/transaction_signer.rs | 6 ++++-- signer/src/testing/wsts.rs | 15 ++++++++------- signer/tests/integration/emily.rs | 5 +++-- signer/tests/integration/postgres.rs | 2 +- .../tests/integration/transaction_coordinator.rs | 7 ++++--- 6 files changed, 23 insertions(+), 17 deletions(-) diff --git a/signer/src/testing/transaction_coordinator.rs b/signer/src/testing/transaction_coordinator.rs index 5f60de0b1..7737d103d 100644 --- a/signer/src/testing/transaction_coordinator.rs +++ b/signer/src/testing/transaction_coordinator.rs @@ -1019,8 +1019,9 @@ where .into(); let dkg_txid = testing::dummy::txid(&fake::Faker, rng); - let (aggregate_key, all_dkg_shares) = - signer_set.run_dkg(bitcoin_chain_tip, dkg_txid, rng).await; + let (aggregate_key, all_dkg_shares) = signer_set + .run_dkg(bitcoin_chain_tip, dkg_txid.into(), rng) + .await; let encrypted_dkg_shares = all_dkg_shares.first().unwrap(); diff --git a/signer/src/testing/transaction_signer.rs b/signer/src/testing/transaction_signer.rs index 6dcd53c63..ad43c4c3b 100644 --- a/signer/src/testing/transaction_signer.rs +++ b/signer/src/testing/transaction_signer.rs @@ -213,7 +213,9 @@ where coordinator_signer_info, self.signing_threshold, ); - let aggregate_key = coordinator.run_dkg(bitcoin_chain_tip, dummy_txid).await; + let aggregate_key = coordinator + .run_dkg(bitcoin_chain_tip, dummy_txid.into()) + .await; for handle in event_loop_handles.into_iter() { assert!(handle @@ -260,7 +262,7 @@ async fn run_dkg_and_store_results_for_signers<'s: 'r, 'r, S, Rng>( let dkg_txid = testing::dummy::txid(&fake::Faker, rng); let bitcoin_chain_tip = *chain_tip; let (_, all_dkg_shares) = testing_signer_set - .run_dkg(bitcoin_chain_tip, dkg_txid, rng) + .run_dkg(bitcoin_chain_tip, dkg_txid.into(), rng) .await; for (storage, encrypted_dkg_shares) in stores.into_iter().zip(all_dkg_shares) { diff --git a/signer/src/testing/wsts.rs b/signer/src/testing/wsts.rs index 62ca34aeb..5fad7f508 100644 --- a/signer/src/testing/wsts.rs +++ b/signer/src/testing/wsts.rs @@ -137,7 +137,7 @@ impl Coordinator { pub async fn run_dkg( &mut self, bitcoin_chain_tip: model::BitcoinBlockHash, - txid: bitcoin::Txid, + id: WstsMessageId, ) -> PublicKey { self.wsts_coordinator .move_to(coordinator::State::DkgPublicDistribute) @@ -148,10 +148,9 @@ impl Coordinator { .start_public_shares() .expect("failed to start public shares"); - self.send_packet(bitcoin_chain_tip, txid.into(), outbound) - .await; + self.send_packet(bitcoin_chain_tip, id, outbound).await; - match self.loop_until_result(bitcoin_chain_tip, txid.into()).await { + match self.loop_until_result(bitcoin_chain_tip, id).await { wsts::state_machine::OperationResult::Dkg(aggregate_key) => { PublicKey::try_from(&aggregate_key).expect("Got the point at infinity") } @@ -405,7 +404,7 @@ impl SignerSet { pub async fn run_dkg( &mut self, bitcoin_chain_tip: model::BitcoinBlockHash, - txid: bitcoin::Txid, + id: WstsMessageId, rng: &mut Rng, ) -> (PublicKey, Vec) { let mut signer_handles = Vec::new(); @@ -414,7 +413,7 @@ impl SignerSet { signer_handles.push(handle); } - let aggregate_key = self.coordinator.run_dkg(bitcoin_chain_tip, txid).await; + let aggregate_key = self.coordinator.run_dkg(bitcoin_chain_tip, id).await; for handle in signer_handles { let signer = handle.await.expect("signer crashed"); @@ -554,7 +553,9 @@ mod tests { let signer_info = generate_signer_info(&mut rng, num_signers); let mut signer_set = SignerSet::new(&signer_info, threshold, || network.connect()); - let (_, dkg_shares) = signer_set.run_dkg(bitcoin_chain_tip, txid, &mut rng).await; + let (_, dkg_shares) = signer_set + .run_dkg(bitcoin_chain_tip, txid.into(), &mut rng) + .await; assert_eq!(dkg_shares.len(), num_signers as usize); } diff --git a/signer/tests/integration/emily.rs b/signer/tests/integration/emily.rs index d1cccc320..9a01875a3 100644 --- a/signer/tests/integration/emily.rs +++ b/signer/tests/integration/emily.rs @@ -99,8 +99,9 @@ where .into(); let dkg_txid = testing::dummy::txid(&fake::Faker, rng); - let (aggregate_key, all_dkg_shares) = - signer_set.run_dkg(bitcoin_chain_tip, dkg_txid, rng).await; + let (aggregate_key, all_dkg_shares) = signer_set + .run_dkg(bitcoin_chain_tip, dkg_txid.into(), rng) + .await; let encrypted_dkg_shares = all_dkg_shares.first().unwrap(); signer_set diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index d05b54103..a63124c2d 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -999,7 +999,7 @@ async fn should_return_the_same_last_key_rotation_as_in_memory_store() { testing::wsts::SignerSet::new(&signer_info, threshold, || dummy_wsts_network.connect()); let dkg_txid = testing::dummy::txid(&fake::Faker, &mut rng); let (_, all_shares) = testing_signer_set - .run_dkg(chain_tip, dkg_txid, &mut rng) + .run_dkg(chain_tip, dkg_txid.into(), &mut rng) .await; let shares = all_shares.first().unwrap(); diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index 3d061bea2..fc4d1239a 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -147,8 +147,9 @@ where .into(); let dkg_txid = testing::dummy::txid(&fake::Faker, rng); - let (aggregate_key, all_dkg_shares) = - signer_set.run_dkg(bitcoin_chain_tip, dkg_txid, rng).await; + let (aggregate_key, all_dkg_shares) = signer_set + .run_dkg(bitcoin_chain_tip, dkg_txid.into(), rng) + .await; let encrypted_dkg_shares = all_dkg_shares.first().unwrap(); signer_set @@ -3214,7 +3215,7 @@ async fn test_conservative_initial_sbtc_limits() { let dkg_txid = testing::dummy::txid(&fake::Faker, &mut rng); let (aggregate_key, encrypted_shares) = signer_set - .run_dkg(chain_tip_info.hash.into(), dkg_txid, &mut rng) + .run_dkg(chain_tip_info.hash.into(), dkg_txid.into(), &mut rng) .await; for ((_, db, _, _), dkg_shares) in signers.iter_mut().zip(&encrypted_shares) { From d22ed614c1628045f313fd2d225c23d67a7ef0fa Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 11:59:32 +0100 Subject: [PATCH 09/14] remove wstsmessage.txid infavor of id --- protobufs/stacks/signer/v1/messages.proto | 3 +- signer/src/proto/convert.rs | 28 ++++++------------- .../src/proto/generated/stacks.signer.v1.rs | 4 --- 3 files changed, 9 insertions(+), 26 deletions(-) diff --git a/protobufs/stacks/signer/v1/messages.proto b/protobufs/stacks/signer/v1/messages.proto index 128036248..1cc46c798 100644 --- a/protobufs/stacks/signer/v1/messages.proto +++ b/protobufs/stacks/signer/v1/messages.proto @@ -35,8 +35,7 @@ message SignerMessage { // A wsts message. message WstsMessage { - // The transaction ID this message relates to, will be a dummy ID for DKG messages - bitcoin.BitcoinTxid txid = 1 [deprecated = true]; + reserved 1; // The wsts message oneof inner { // Tell signers to begin DKG by sending DKG public shares diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 1d5c79088..4f64f87ec 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -1095,13 +1095,6 @@ impl From for proto::WstsMessage { }; proto::WstsMessage { - txid: match value.id { - WstsMessageId::BitcoinTxid(txid) => { - Some(proto::BitcoinTxid::from(BitcoinTxId::from(txid))) - } - WstsMessageId::Dkg(_) => None, - WstsMessageId::RotateKey(_) => None, - }, id: Some(match value.id { WstsMessageId::BitcoinTxid(txid) => { wsts_message::Id::IdBitcoinTxid(proto::BitcoinTxid { @@ -1155,19 +1148,14 @@ impl TryFrom for WstsMessage { #[allow(deprecated)] Ok(WstsMessage { - id: match value.id { - Some(id) => match id { - wsts_message::Id::IdBitcoinTxid(txid) => { - WstsMessageId::BitcoinTxid(BitcoinTxId::try_from(txid)?.into()) - } - wsts_message::Id::IdRotateKey(pubkey) => { - WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) - } - wsts_message::Id::IdDkg(id) => WstsMessageId::Dkg(id.into()), - }, - None => WstsMessageId::BitcoinTxid( - BitcoinTxId::try_from(value.txid.required()?)?.into(), - ), + id: match value.id.required()? { + wsts_message::Id::IdBitcoinTxid(txid) => { + WstsMessageId::BitcoinTxid(BitcoinTxId::try_from(txid)?.into()) + } + wsts_message::Id::IdRotateKey(pubkey) => { + WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) + } + wsts_message::Id::IdDkg(id) => WstsMessageId::Dkg(id.into()), }, inner, }) diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index a9d67ee4d..3738c920e 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -286,10 +286,6 @@ pub mod signer_message { /// A wsts message. #[derive(Clone, PartialEq, ::prost::Message)] pub struct WstsMessage { - /// The transaction ID this message relates to, will be a dummy ID for DKG messages - #[deprecated] - #[prost(message, optional, tag = "1")] - pub txid: ::core::option::Option, /// The wsts message #[prost(oneof = "wsts_message::Inner", tags = "2, 3, 4, 5, 6, 7, 8, 9, 10, 11")] pub inner: ::core::option::Option, From 76b9b4971028e57839d23c41324fdc000dae4dba Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 12:14:14 +0100 Subject: [PATCH 10/14] remove save and prefer trait default impls --- signer/src/transaction_coordinator.rs | 1 - signer/src/wsts_state_machine.rs | 58 --------------------------- 2 files changed, 59 deletions(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index e7664f008..a9ceb56a9 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -1152,7 +1152,6 @@ where let signer_set = self.context.state().current_signer_public_keys(); tokio::pin!(signal_stream); - coordinator_state_machine.save(); // Let's get the next message from the network or the // TxSignerEventLoop. // diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index 176676bdc..476470972 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -25,7 +25,6 @@ use wsts::state_machine::coordinator::fire; use wsts::state_machine::coordinator::frost; use wsts::state_machine::coordinator::Config; use wsts::state_machine::coordinator::Coordinator as _; -use wsts::state_machine::coordinator::SavedState; use wsts::state_machine::coordinator::State as WstsState; use wsts::state_machine::OperationResult; use wsts::state_machine::StateMachine as _; @@ -126,9 +125,6 @@ where /// Gets the coordinator configuration. fn get_config(&self) -> Config; - /// Save the state required to reconstruct the state machine. - fn save(&self) -> SavedState; - /// Create a new coordinator state machine from the given aggregate /// key. /// @@ -234,10 +230,6 @@ impl WstsCoordinator for FireCoordinator { self.0.get_config() } - fn save(&self) -> SavedState { - self.0.save() - } - async fn load( storage: &S, aggregate_key: PublicKeyXOnly, @@ -275,29 +267,6 @@ impl WstsCoordinator for FireCoordinator { Ok(coordinator) } - fn process_message( - &mut self, - message: &Message, - ) -> Result<(Option, Option), Error> { - let packet = Packet::from_message(message); - self.0 - .process_message(&packet) - .map_err(Error::wsts_coordinator) - } - - fn process_inbound_messages( - &mut self, - messages: &[Message], - ) -> Result<(Vec, Vec), Error> { - let packets = messages - .iter() - .map(Packet::from_message) - .collect::>(); - self.0 - .process_inbound_messages(&packets) - .map_err(Error::wsts_coordinator) - } - fn process_packet( &mut self, packet: &Packet, @@ -370,10 +339,6 @@ impl WstsCoordinator for FrostCoordinator { self.0.get_config() } - fn save(&self) -> SavedState { - self.0.save() - } - async fn load( storage: &S, aggregate_key: PublicKeyXOnly, @@ -411,29 +376,6 @@ impl WstsCoordinator for FrostCoordinator { Ok(coordinator) } - fn process_message( - &mut self, - message: &Message, - ) -> Result<(Option, Option), Error> { - let packet = Packet::from_message(message); - self.0 - .process_message(&packet) - .map_err(Error::wsts_coordinator) - } - - fn process_inbound_messages( - &mut self, - messages: &[Message], - ) -> Result<(Vec, Vec), Error> { - let packets = messages - .iter() - .map(Packet::from_message) - .collect::>(); - self.0 - .process_inbound_messages(&packets) - .map_err(Error::wsts_coordinator) - } - fn process_packet( &mut self, packet: &Packet, From cb9541747dc90d6fc486f3308536ebddc830b112 Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 12:17:12 +0100 Subject: [PATCH 11/14] remove unused trait methods --- signer/src/wsts_state_machine.rs | 36 -------------------------------- 1 file changed, 36 deletions(-) diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index 476470972..d3d53892f 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -155,30 +155,12 @@ where self.process_packet(&packet) } - /// Process inbound messages - fn process_inbound_messages( - &mut self, - messages: &[Message], - ) -> Result<(Vec, Vec), Error> { - let packets = messages - .iter() - .map(Packet::from_message) - .collect::>(); - self.process_inbound_packets(&packets) - } - /// Process the given packet. fn process_packet( &mut self, packet: &Packet, ) -> Result<(Option, Option), Error>; - /// Process inbound packets - fn process_inbound_packets( - &mut self, - packets: &[Packet], - ) -> Result<(Vec, Vec), Error>; - /// Start a signing round with the given message and signature type. fn start_signing_round( &mut self, @@ -276,15 +258,6 @@ impl WstsCoordinator for FireCoordinator { .map_err(Error::wsts_coordinator) } - fn process_inbound_packets( - &mut self, - packets: &[Packet], - ) -> Result<(Vec, Vec), Error> { - self.0 - .process_inbound_messages(packets) - .map_err(Error::wsts_coordinator) - } - fn start_signing_round( &mut self, message: &[u8], @@ -385,15 +358,6 @@ impl WstsCoordinator for FrostCoordinator { .map_err(Error::wsts_coordinator) } - fn process_inbound_packets( - &mut self, - packets: &[Packet], - ) -> Result<(Vec, Vec), Error> { - self.0 - .process_inbound_messages(packets) - .map_err(Error::wsts_coordinator) - } - fn start_signing_round( &mut self, message: &[u8], From a38d1c9fad69ef20b533420c32286e9f9b03c07b Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 12:23:00 +0100 Subject: [PATCH 12/14] logging stuff --- signer/src/transaction_signer.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index cacff33bb..524239a93 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -200,7 +200,7 @@ where SignerEvent::TxCoordinator(TxCoordinatorEvent::MessageGenerated(msg)) | SignerEvent::P2P(P2PEvent::MessageReceived(msg)) => { if let Err(error) = self.handle_signer_message(&msg).await { - tracing::error!(%error, "error handling signer message"); + tracing::error!(%error, "error processing signer message"); } } _ => {} @@ -519,7 +519,7 @@ where return Ok(()); } - tracing::debug!("responding to dkg-begin"); + tracing::debug!("processing message"); // Assert that DKG should be allowed to proceed given the current state // and configuration. @@ -557,7 +557,7 @@ where return Ok(()); } - tracing::debug!("responding to dkg-private-begin"); + tracing::debug!("processing message"); let id = StateMachineId::from(&chain_tip.block_hash); self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) @@ -567,7 +567,7 @@ where span.record(WSTS_DKG_ID, request.dkg_id); span.record(WSTS_SIGNER_ID, request.signer_id); - tracing::debug!("responding to dkg-public-shares"); + tracing::debug!("processing message"); let id = StateMachineId::from(&chain_tip.block_hash); self.validate_sender(&id, request.signer_id, &msg_public_key)?; @@ -578,7 +578,7 @@ where span.record(WSTS_DKG_ID, request.dkg_id); span.record(WSTS_SIGNER_ID, request.signer_id); - tracing::debug!("responding to dkg-private-shares"); + tracing::debug!("processing message"); let id = StateMachineId::from(&chain_tip.block_hash); self.validate_sender(&id, request.signer_id, &msg_public_key)?; @@ -596,7 +596,7 @@ where return Ok(()); } - tracing::debug!("responding to dkg-end-begin"); + tracing::debug!("processing message"); let id = StateMachineId::from(&chain_tip.block_hash); self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) .await?; @@ -631,7 +631,7 @@ where return Ok(()); } - tracing::debug!(signature_type = ?request.signature_type, "responding to nonce-request"); + tracing::debug!(signature_type = ?request.signature_type, "processing message"); let db = self.context.get_storage(); let accepted_sighash = @@ -681,7 +681,7 @@ where return Ok(()); } - tracing::debug!(signature_type = ?request.signature_type, "responding to signature-share-request"); + tracing::debug!(signature_type = ?request.signature_type, "processing message"); let db = self.context.get_storage(); let accepted_sighash = From 5f99a29c7101d87b2a0ab5c44e2c829c6e2d621c Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 12:56:50 +0100 Subject: [PATCH 13/14] rename message ids --- protobufs/stacks/signer/v1/messages.proto | 6 ++--- signer/src/message.rs | 14 ++++++------ signer/src/proto/convert.rs | 22 +++++++++---------- .../src/proto/generated/stacks.signer.v1.rs | 6 ++--- .../tests/integration/transaction_signer.rs | 2 +- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/protobufs/stacks/signer/v1/messages.proto b/protobufs/stacks/signer/v1/messages.proto index 1cc46c798..35ef81a8d 100644 --- a/protobufs/stacks/signer/v1/messages.proto +++ b/protobufs/stacks/signer/v1/messages.proto @@ -62,14 +62,14 @@ message WstsMessage { oneof id { // If this WSTS message is related to a Bitcoin signing round, this field // will be set to the related Bitcoin transaction ID. - bitcoin.BitcoinTxid id_bitcoin_txid = 12; + bitcoin.BitcoinTxid sweep = 12; // If this WSTS message is related to a rotate-keys transaction, this field // will be set to the _new_ aggregate public key being verified. - crypto.PublicKey id_rotate_key = 13; + crypto.PublicKey dkg_verification = 13; // If this WSTS message is related to a DKG round, this field will be set // to the 32-byte id determined based on the coordinator public key and // block hash, set by the coordinator. - crypto.Uint256 id_dkg = 14; + crypto.Uint256 dkg = 14; } } diff --git a/signer/src/message.rs b/signer/src/message.rs index 7d3d4d21f..05f30f180 100644 --- a/signer/src/message.rs +++ b/signer/src/message.rs @@ -228,31 +228,31 @@ pub struct BitcoinPreSignAck; #[derive(Debug, Clone, Copy, PartialEq)] pub enum WstsMessageId { /// The WSTS message is related to a Bitcoin transaction signing round. - BitcoinTxid(bitcoin::Txid), + Sweep(bitcoin::Txid), /// The WSTS message is related to a rotate key verification operation. - RotateKey(PublicKey), + DkgVerification(PublicKey), /// The WSTS message is related to a DKG round. Dkg([u8; 32]), } impl From for WstsMessageId { fn from(txid: bitcoin::Txid) -> Self { - Self::BitcoinTxid(txid) + Self::Sweep(txid) } } impl From for WstsMessageId { fn from(txid: crate::storage::model::BitcoinTxId) -> Self { - Self::BitcoinTxid(txid.into()) + Self::Sweep(txid.into()) } } impl std::fmt::Display for WstsMessageId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - WstsMessageId::BitcoinTxid(txid) => write!(f, "bitcoin-txid({})", txid), - WstsMessageId::RotateKey(aggregate_key) => { - write!(f, "rotate-key({})", aggregate_key) + WstsMessageId::Sweep(txid) => write!(f, "sweep({})", txid), + WstsMessageId::DkgVerification(aggregate_key) => { + write!(f, "dkg-verification({})", aggregate_key) } WstsMessageId::Dkg(id) => { write!(f, "dkg({})", hex::encode(id)) diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 4f64f87ec..48b7ea7ab 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -1096,13 +1096,13 @@ impl From for proto::WstsMessage { proto::WstsMessage { id: Some(match value.id { - WstsMessageId::BitcoinTxid(txid) => { - wsts_message::Id::IdBitcoinTxid(proto::BitcoinTxid { - txid: Some(proto::Uint256::from(BitcoinTxId::from(txid).into_bytes())), - }) + WstsMessageId::Sweep(txid) => wsts_message::Id::Sweep(proto::BitcoinTxid { + txid: Some(proto::Uint256::from(BitcoinTxId::from(txid).into_bytes())), + }), + WstsMessageId::DkgVerification(pubkey) => { + wsts_message::Id::DkgVerification(pubkey.into()) } - WstsMessageId::RotateKey(pubkey) => wsts_message::Id::IdRotateKey(pubkey.into()), - WstsMessageId::Dkg(id) => wsts_message::Id::IdDkg(id.into()), + WstsMessageId::Dkg(id) => wsts_message::Id::Dkg(id.into()), }), inner: Some(inner), } @@ -1149,13 +1149,13 @@ impl TryFrom for WstsMessage { #[allow(deprecated)] Ok(WstsMessage { id: match value.id.required()? { - wsts_message::Id::IdBitcoinTxid(txid) => { - WstsMessageId::BitcoinTxid(BitcoinTxId::try_from(txid)?.into()) + wsts_message::Id::Sweep(txid) => { + WstsMessageId::Sweep(BitcoinTxId::try_from(txid)?.into()) } - wsts_message::Id::IdRotateKey(pubkey) => { - WstsMessageId::RotateKey(PublicKey::try_from(pubkey)?) + wsts_message::Id::DkgVerification(pubkey) => { + WstsMessageId::DkgVerification(PublicKey::try_from(pubkey)?) } - wsts_message::Id::IdDkg(id) => WstsMessageId::Dkg(id.into()), + wsts_message::Id::Dkg(id) => WstsMessageId::Dkg(id.into()), }, inner, }) diff --git a/signer/src/proto/generated/stacks.signer.v1.rs b/signer/src/proto/generated/stacks.signer.v1.rs index 3738c920e..1bb154989 100644 --- a/signer/src/proto/generated/stacks.signer.v1.rs +++ b/signer/src/proto/generated/stacks.signer.v1.rs @@ -339,16 +339,16 @@ pub mod wsts_message { /// If this WSTS message is related to a Bitcoin signing round, this field /// will be set to the related Bitcoin transaction ID. #[prost(message, tag = "12")] - IdBitcoinTxid(super::super::super::super::bitcoin::BitcoinTxid), + Sweep(super::super::super::super::bitcoin::BitcoinTxid), /// If this WSTS message is related to a rotate-keys transaction, this field /// will be set to the _new_ aggregate public key being verified. #[prost(message, tag = "13")] - IdRotateKey(super::super::super::super::crypto::PublicKey), + DkgVerification(super::super::super::super::crypto::PublicKey), /// If this WSTS message is related to a DKG round, this field will be set /// to the 32-byte id determined based on the coordinator public key and /// block hash, set by the coordinator. #[prost(message, tag = "14")] - IdDkg(super::super::super::super::crypto::Uint256), + Dkg(super::super::super::super::crypto::Uint256), } } /// Wraps an inner type with a public key and a signature, diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index 5702c46ba..9f0eca013 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -340,7 +340,7 @@ async fn new_state_machine_per_valid_sighash() { // Now for the nonce request message let mut nonce_request_msg = WstsMessage { - id: WstsMessageId::BitcoinTxid(*txid), + id: WstsMessageId::Sweep(*txid), inner: wsts::net::Message::NonceRequest(NonceRequest { dkg_id: 1, sign_id: 1, From 68bf8f5337f7f810d56466e20075712341e1125a Mon Sep 17 00:00:00 2001 From: cylewitruk Date: Wed, 5 Feb 2025 17:42:58 +0100 Subject: [PATCH 14/14] remove unneeded allow(deprecated) --- signer/src/proto/convert.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/signer/src/proto/convert.rs b/signer/src/proto/convert.rs index 48b7ea7ab..a8548c56e 100644 --- a/signer/src/proto/convert.rs +++ b/signer/src/proto/convert.rs @@ -1061,7 +1061,6 @@ impl TryFrom for SignatureShareResponse { } } impl From for proto::WstsMessage { - #[allow(deprecated)] fn from(value: WstsMessage) -> Self { let inner = match value.inner { wsts::net::Message::DkgBegin(inner) => { @@ -1146,7 +1145,6 @@ impl TryFrom for WstsMessage { } }; - #[allow(deprecated)] Ok(WstsMessage { id: match value.id.required()? { wsts_message::Id::Sweep(txid) => {