From 931e6cd2e0dfd179ecdf656f102ad2e640ac8747 Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 18:46:23 -0500 Subject: [PATCH 1/9] Remove some dead code --- signer/src/transaction_signer.rs | 68 +------------------------------- 1 file changed, 1 insertion(+), 67 deletions(-) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 020435f29..1724eeb67 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -232,16 +232,6 @@ where .await?; } - ( - message::Payload::BitcoinTransactionSignRequest(request), - true, - ChainTipStatus::Canonical, - ) => { - tracing::debug!("handling bitcoin transaction sign request"); - self.handle_bitcoin_transaction_sign_request(request, &msg.bitcoin_chain_tip) - .await?; - } - (message::Payload::WstsMessage(wsts_msg), _, _) => { self.handle_wsts_message( wsts_msg, @@ -282,6 +272,7 @@ where } // Message types ignored by the transaction signer (message::Payload::StacksTransactionSignature(_), _, _) + | (message::Payload::BitcoinTransactionSignRequest(_), _, _) | (message::Payload::BitcoinTransactionSignAck(_), _, _) | (message::Payload::SignerDepositDecision(_), _, _) | (message::Payload::SignerWithdrawalDecision(_), _, _) => (), @@ -390,63 +381,6 @@ where Ok(()) } - #[tracing::instrument(skip_all)] - async fn handle_bitcoin_transaction_sign_request( - &mut self, - request: &message::BitcoinTransactionSignRequest, - bitcoin_chain_tip: &model::BitcoinBlockHash, - ) -> Result<(), Error> { - let is_valid_sign_request = self - .is_valid_bitcoin_transaction_sign_request(request) - .await?; - - if is_valid_sign_request { - let new_state_machine = SignerStateMachine::load( - &self.context.get_storage_mut(), - request.aggregate_key, - self.threshold, - self.signer_private_key, - ) - .await?; - - let txid = request.tx.compute_txid(); - - self.wsts_state_machines.insert(txid, new_state_machine); - - let msg = message::BitcoinTransactionSignAck { - txid: request.tx.compute_txid(), - }; - - self.send_message(msg, bitcoin_chain_tip).await?; - } else { - tracing::warn!("received invalid sign request"); - } - - Ok(()) - } - - async fn is_valid_bitcoin_transaction_sign_request( - &self, - _request: &message::BitcoinTransactionSignRequest, - ) -> Result { - let signer_pub_key = self.signer_public_key(); - let _accepted_deposit_requests = self - .context - .get_storage() - .get_accepted_deposit_requests(&signer_pub_key) - .await?; - - // TODO(286): Validate transaction - // - Ensure all inputs are either accepted deposit requests - // or directly spendable by the signers. - // - Ensure all outputs are either accepted withdrawals - // or pays to an approved signer set. - // - Ensure the transaction fee is lower than the minimum - // `max_fee` of any request. - - Ok(true) - } - #[tracing::instrument(skip_all)] async fn handle_stacks_transaction_sign_request( &mut self, From 4a933c5460c10a5894ea7f6a5fd498b68abee4a2 Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 18:49:27 -0500 Subject: [PATCH 2/9] Use the x-only coordinates when searching for DKG shares --- signer/src/error.rs | 2 +- signer/src/keys.rs | 6 ++++++ signer/src/storage/in_memory.rs | 15 +++++++++------ signer/src/storage/mod.rs | 9 ++++++--- signer/src/storage/postgres.rs | 14 +++++++++----- signer/src/transaction_signer.rs | 2 +- signer/src/wsts_state_machine.rs | 16 ++++++++++------ 7 files changed, 42 insertions(+), 22 deletions(-) diff --git a/signer/src/error.rs b/signer/src/error.rs index dfb7d6722..c514c76c7 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -387,7 +387,7 @@ pub enum Error { /// Missing dkg shares #[error("missing dkg shares for the given aggregate key: {0}")] - MissingDkgShares(crate::keys::PublicKey), + MissingDkgShares(crate::keys::PublicKeyXOnly), /// Missing public key #[error("missing public key")] diff --git a/signer/src/keys.rs b/signer/src/keys.rs index 7348f1182..744124363 100644 --- a/signer/src/keys.rs +++ b/signer/src/keys.rs @@ -290,6 +290,12 @@ impl From<&PublicKey> for PublicKeyXOnly { } } +impl std::fmt::Display for PublicKeyXOnly { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + impl PublicKeyXOnly { /// Creates a public key directly from a slice. pub fn from_slice(data: &[u8]) -> Result { diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index a5ee610ce..1fe62774a 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -85,7 +85,7 @@ pub struct Store { HashMap>, /// Encrypted DKG shares - pub encrypted_dkg_shares: BTreeMap, + pub encrypted_dkg_shares: BTreeMap, /// Rotate keys transactions pub rotate_keys_transactions: HashMap, @@ -574,15 +574,18 @@ impl super::DbRead for SharedStore { .contains_key(&block_id.into())) } - async fn get_encrypted_dkg_shares( + async fn get_encrypted_dkg_shares( &self, - aggregate_key: &PublicKey, - ) -> Result, Error> { + aggregate_key: X, + ) -> Result, Error> + where + X: Into + Send, + { Ok(self .lock() .await .encrypted_dkg_shares - .get(aggregate_key) + .get(&aggregate_key.into()) .map(|(_, shares)| shares.clone())) } @@ -1109,7 +1112,7 @@ impl super::DbWrite for SharedStore { shares: &model::EncryptedDkgShares, ) -> Result<(), Error> { self.lock().await.encrypted_dkg_shares.insert( - shares.aggregate_key, + shares.aggregate_key.into(), (time::OffsetDateTime::now_utc(), shares.clone()), ); diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index bd489c3f1..ad22ec0cb 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -22,6 +22,7 @@ use crate::bitcoin::validation::DepositRequestReport; use crate::bitcoin::validation::WithdrawalRequestReport; use crate::error::Error; use crate::keys::PublicKey; +use crate::keys::PublicKeyXOnly; use crate::stacks::events::CompletedDepositEvent; use crate::stacks::events::WithdrawalAcceptEvent; use crate::stacks::events::WithdrawalCreateEvent; @@ -204,10 +205,12 @@ pub trait DbRead { /// Return the applicable DKG shares for the /// given aggregate key - fn get_encrypted_dkg_shares( + fn get_encrypted_dkg_shares( &self, - aggregate_key: &PublicKey, - ) -> impl Future, Error>> + Send; + aggregate_key: X, + ) -> impl Future, Error>> + Send + where + X: Into + Send; /// Return the most recent DKG shares, and return None if the table is /// empty. diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 3802740b7..e551fd1a5 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -1452,10 +1452,14 @@ impl super::DbRead for PgStore { .map_err(Error::SqlxQuery) } - async fn get_encrypted_dkg_shares( + async fn get_encrypted_dkg_shares( &self, - aggregate_key: &PublicKey, - ) -> Result, Error> { + aggregate_key: X, + ) -> Result, Error> + where + X: Into + Send, + { + let key: PublicKeyXOnly = aggregate_key.into(); sqlx::query_as::<_, model::EncryptedDkgShares>( r#" SELECT @@ -1467,10 +1471,10 @@ impl super::DbRead for PgStore { , signer_set_public_keys , signature_share_threshold FROM sbtc_signer.dkg_shares - WHERE aggregate_key = $1; + WHERE substring(aggregate_key FROM 2) = $1; "#, ) - .bind(aggregate_key) + .bind(key) .fetch_optional(&self.0) .await .map_err(Error::SqlxQuery) diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 1724eeb67..48a9763a1 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -440,7 +440,7 @@ where let public_key = self.signer_public_key(); let Some(shares) = db.get_encrypted_dkg_shares(&request.aggregate_key).await? else { - return Err(Error::MissingDkgShares(request.aggregate_key)); + return Err(Error::MissingDkgShares(request.aggregate_key.into())); }; // There is one check that applies to all Stacks transactions, and // that check is that the current signer is in the signing set diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index 3f0754956..30da2400a 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -8,6 +8,7 @@ use crate::error; use crate::error::Error; use crate::keys::PrivateKey; use crate::keys::PublicKey; +use crate::keys::PublicKeyXOnly; use crate::keys::SignerScriptPubKey as _; use crate::storage; use crate::storage::model; @@ -91,7 +92,7 @@ impl SignerStateMachine { let encrypted_shares = storage .get_encrypted_dkg_shares(&aggregate_key) .await? - .ok_or(error::Error::MissingDkgShares(aggregate_key))?; + .ok_or(error::Error::MissingDkgShares(aggregate_key.into()))?; let decrypted = wsts::util::decrypt( &signer_private_key.to_bytes(), @@ -232,9 +233,9 @@ impl CoordinatorStateMachine { /// 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( + pub async fn load( storage: &mut S, - aggregate_key: PublicKey, + aggregate_key: X, signers: I, threshold: u16, message_private_key: PrivateKey, @@ -242,11 +243,13 @@ impl CoordinatorStateMachine { 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(&aggregate_key) + .get_encrypted_dkg_shares(x_only_aggregate_key) .await? - .ok_or(Error::MissingDkgShares(aggregate_key))?; + .ok_or(Error::MissingDkgShares(x_only_aggregate_key))?; let public_dkg_shares: BTreeMap = BTreeMap::decode(encrypted_shares.public_shares.as_slice())?; @@ -257,8 +260,9 @@ impl CoordinatorStateMachine { 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.into(), party_polynomials) + .set_key_and_party_polynomials(aggregate_key, party_polynomials) .map_err(Error::wsts_coordinator)?; coordinator.current_dkg_id = 1; From 8f5704561d13d6402465a8b14d8ed8c9c30e6deb Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 18:52:19 -0500 Subject: [PATCH 3/9] always load the correct dkg shares for each sighash --- signer/src/transaction_coordinator.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 3c096948b..7743df8f0 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -561,13 +561,8 @@ where .await?; for mut transaction in transaction_package { - self.sign_and_broadcast( - bitcoin_chain_tip, - aggregate_key, - signer_public_keys, - &mut transaction, - ) - .await?; + self.sign_and_broadcast(bitcoin_chain_tip, signer_public_keys, &mut transaction) + .await?; // TODO: if this (considering also fallback clients) fails, we will // need to handle the inconsistency of having the sweep tx confirmed @@ -909,13 +904,12 @@ where async fn sign_and_broadcast( &mut self, bitcoin_chain_tip: &model::BitcoinBlockHash, - aggregate_key: &PublicKey, signer_public_keys: &BTreeSet, transaction: &mut utxo::UnsignedTransaction<'_>, ) -> Result<(), Error> { let mut coordinator_state_machine = CoordinatorStateMachine::load( &mut self.context.get_storage_mut(), - *aggregate_key, + transaction.signer_utxo.public_key, signer_public_keys.clone(), self.threshold, self.private_key, @@ -959,7 +953,7 @@ where let mut coordinator_state_machine = CoordinatorStateMachine::load( &mut self.context.get_storage_mut(), - *aggregate_key, + deposit.signers_public_key, signer_public_keys.clone(), self.threshold, self.private_key, From 34593e5c207073f04945612178bd87eb52678668 Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 18:56:23 -0500 Subject: [PATCH 4/9] clippy --- signer/tests/integration/stacks_events_observer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/signer/tests/integration/stacks_events_observer.rs b/signer/tests/integration/stacks_events_observer.rs index 338ac6045..8c78c6acf 100644 --- a/signer/tests/integration/stacks_events_observer.rs +++ b/signer/tests/integration/stacks_events_observer.rs @@ -109,7 +109,7 @@ async fn test_new_blocks_sends_update_deposits_to_emily() { .expect("Wiping Emily database in test setup failed."); let body = COMPLETED_DEPOSIT_WEBHOOK.to_string(); - let new_block_event = serde_json::from_str::(&body).unwrap(); + serde_json::from_str::(&body).unwrap(); let deposit_completed_event = get_registry_event_from_webhook(&body, |event| match event { RegistryEvent::CompletedDeposit(event) => Some(event), _ => panic!("Expected CompletedDeposit event"), From 23c1810267d3be15ec290ec82533e471ec78b8fa Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 18:59:40 -0500 Subject: [PATCH 5/9] Add a comment --- signer/src/storage/postgres.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index e551fd1a5..c6f171adf 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -1459,6 +1459,9 @@ impl super::DbRead for PgStore { where X: Into + Send, { + // The first two characters are for the aggregate key column + // represent the parity byte. Since the input is an x-only public + // key, we don't have a parity byte, so we lop it off. let key: PublicKeyXOnly = aggregate_key.into(); sqlx::query_as::<_, model::EncryptedDkgShares>( r#" From c3b1a67e8782cc5f4a3bb293d783da6ed534807f Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 19:07:51 -0500 Subject: [PATCH 6/9] Prevent DKG from running multiple times --- signer/src/error.rs | 5 +++++ signer/src/transaction_signer.rs | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/signer/src/error.rs b/signer/src/error.rs index c514c76c7..00bc888f3 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -411,6 +411,11 @@ pub enum Error { #[error("DKG has not been run")] NoDkgShares, + /// TODO: We don't want to be able to run DKG more than once. Soon this + /// restriction will be lifted. + #[error("DKG has already been run, can only run once")] + DkgHasAlreadyRun, + /// Too many signer utxos #[error("too many signer utxos")] TooManySignerUtxos, diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index 48a9763a1..2b58b937d 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -502,6 +502,17 @@ where return Ok(()); } + let dkg_shares = self + .context + .get_storage() + .get_latest_encrypted_dkg_shares() + .await?; + + if dkg_shares.is_some() { + tracing::warn!("we do not support running DKG more than once"); + return Err(Error::DkgHasAlreadyRun); + } + let signer_public_keys = self.get_signer_public_keys(bitcoin_chain_tip).await?; let state_machine = SignerStateMachine::new( From 856d1f4273c79d9beb6b3eb96f8c13752f6c3f30 Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 21:32:04 -0500 Subject: [PATCH 7/9] Update the comments --- signer/src/storage/postgres.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index c6f171adf..8d56504a0 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -1459,9 +1459,10 @@ impl super::DbRead for PgStore { where X: Into + Send, { - // The first two characters are for the aggregate key column - // represent the parity byte. Since the input is an x-only public - // key, we don't have a parity byte, so we lop it off. + // The aggregate_key column stores compressed public keys, which + // always include a parity byte. Since the input is an x-only + // public key, we don't have a parity byte, so we lop it off when + // filtering. let key: PublicKeyXOnly = aggregate_key.into(); sqlx::query_as::<_, model::EncryptedDkgShares>( r#" @@ -1644,7 +1645,7 @@ impl super::DbRead for PgStore { // If we've swept funds before, then will have a signer output and // a minimum UTXO height, so let's try that first. let Some(min_block_height) = self.minimum_utxo_height().await? else { - // If the above functon returns None then we know that there + // If the above function returns None then we know that there // have been no confirmed sweep transactions thus far, so let's // try looking for a donation UTXO. return self.get_donation_utxo(chain_tip).await; @@ -1653,7 +1654,7 @@ impl super::DbRead for PgStore { // transaction. Let's look for the UTXO in a block after our // min_block_height. Note that `Self::get_utxo` returns `None` only // when a reorg has affected all sweep transactions. If this - // happens we try seaching for a donation. + // happens we try searching for a donation. let output_type = model::TxOutputType::SignersOutput; let fut = self.get_utxo(chain_tip, output_type, min_block_height); match fut.await? { @@ -1857,7 +1858,7 @@ impl super::DbRead for PgStore { _chain_tip: &model::BitcoinBlockHash, _context_window: u16, ) -> Result, Error> { - // TODO: This can use a similiar query to + // TODO: This can use a similar query to // `get_swept_deposit_requests()`, but using withdrawal tables instead // of deposit. unimplemented!() @@ -2836,9 +2837,9 @@ mod tests { // address in the transaction, then we will consider it a relevant // transaction. Now what if someone tries to pull a fast one by // deploying their own modified version of the sBTC smart contracts - // and creating contract calls against that? We'll the address of + // and creating contract calls against that? Well the address of // these contract calls won't match the ones that we are interested - // in and we will filter them out. We test that now, + // in, and we will filter them out. We test that now, let contract_call = TransactionContractCall { // This is the address of the poser that deployed their own // versions of the sBTC smart contracts. From 5a352d6912b9512f87c044c1dbb22f41c69cc950 Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 21:32:16 -0500 Subject: [PATCH 8/9] Remove test line --- signer/tests/integration/stacks_events_observer.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/signer/tests/integration/stacks_events_observer.rs b/signer/tests/integration/stacks_events_observer.rs index 8c78c6acf..d07daaab4 100644 --- a/signer/tests/integration/stacks_events_observer.rs +++ b/signer/tests/integration/stacks_events_observer.rs @@ -109,7 +109,6 @@ async fn test_new_blocks_sends_update_deposits_to_emily() { .expect("Wiping Emily database in test setup failed."); let body = COMPLETED_DEPOSIT_WEBHOOK.to_string(); - serde_json::from_str::(&body).unwrap(); let deposit_completed_event = get_registry_event_from_webhook(&body, |event| match event { RegistryEvent::CompletedDeposit(event) => Some(event), _ => panic!("Expected CompletedDeposit event"), From 86642684946fb78d68643d7417010881dcdfdf3d Mon Sep 17 00:00:00 2001 From: djordon Date: Thu, 12 Dec 2024 21:34:03 -0500 Subject: [PATCH 9/9] grammar and such --- signer/src/storage/postgres.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 8d56504a0..3a72d939e 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -1460,8 +1460,8 @@ impl super::DbRead for PgStore { X: Into + Send, { // The aggregate_key column stores compressed public keys, which - // always include a parity byte. Since the input is an x-only - // public key, we don't have a parity byte, so we lop it off when + // always include a parity byte. Since the input here is an x-only + // public key we don't have a parity byte, so we lop it off when // filtering. let key: PublicKeyXOnly = aggregate_key.into(); sqlx::query_as::<_, model::EncryptedDkgShares>(