From dc241cf39e1a505b68cf67b80b79d9a0e9b2ecfe Mon Sep 17 00:00:00 2001 From: Cyle Witruk <35576205+cylewitruk@users.noreply.github.com> Date: Fri, 7 Feb 2025 22:15:53 +0100 Subject: [PATCH] feat: DKG verification pre-key-rotation (#1301) * update for wsts 11 * use wsts 11.0.0 from crates * fix rebase * use wsts-12.0.0 from crates * add FrostCoordinator to wsts_state_machine so we can run signing rounds where all signers must participate * wip * run a dkg signing before deploying contracts * new wsts commit * wip * seems to work * working version * remove dead code * minor wsts logging tweaks * rename wstsmessageid variant * it works, now need to clean up * downgrade wsts * proto backwards compatability fixes * some wsts cleanup/refactor * think that's it * add message support for a specific dkg wstsmessageid * add a dkg wstsmessageid variant * use block hash instead of random data * fmt * remove 100% requirement for stacks signing of rotate keys * bump p256k1 to 7.2.2 * pop stash * wip * working verifications * saving * fixing tests * seems to work * remove const generic, use 0 amount and add test for random keypair * vet bitcoinconsensus * working on cleaning up * fix merge artifacts * tracing fields * migration needs to update block hash/height * storage mut * lovely cascading changes * remove wstsmessage.txid infavor of id * remove save and prefer trait default impls * remove unused trait methods * logging stuff * rename message ids * use tracing constants * remove stale comment * remove box from some types * missed dkg_begin_pause * leftover dbg!() * refactor some validation * various pr comments * mut thing * remove unneeded allow(deprecated) * import some changes manually from parent branch to help a confused merge tool * more diff-reducing imports * confused merge tool * reduce diff * add validation for nonceresponse + signatureshareresponse * newline * pr comments * pr comments utxo * utxo comments * remove error conversion method * Squashed commit of the following: commit 2168f580c389dcd58f49d0e15358f802a490cc52 Author: Cyle Witruk <35576205+cylewitruk@users.noreply.github.com> Date: Thu Feb 6 16:08:22 2025 +0100 feat: consensus on successful DKG prior to rotate-keys submission (#1285) * update for wsts 11 * use wsts 11.0.0 from crates * fix rebase * use wsts-12.0.0 from crates * add FrostCoordinator to wsts_state_machine so we can run signing rounds where all signers must participate * wip * run a dkg signing before deploying contracts * new wsts commit * wip * seems to work * working version * remove dead code * minor wsts logging tweaks * rename wstsmessageid variant * it works, now need to clean up * downgrade wsts * proto backwards compatability fixes * some wsts cleanup/refactor * think that's it * add message support for a specific dkg wstsmessageid * add a dkg wstsmessageid variant * use block hash instead of random data * fmt * remove 100% requirement for stacks signing of rotate keys * bump p256k1 to 7.2.2 * fix merge artifacts * tracing fields * storage mut * lovely cascading changes * remove wstsmessage.txid infavor of id * remove save and prefer trait default impls * remove unused trait methods * logging stuff * rename message ids * use tracing constants * remove stale comment * remove box from some types * missed dkg_begin_pause * leftover dbg!() * refactor some validation * various pr comments * mut thing * remove unneeded allow(deprecated) * confused merge tool --------- Co-authored-by: Joey Yandle * Change the DKG shares status type. Lots of small follow-up modifications * Update the migration query * Fix up remaining queries * Oops forgot this one * Forgot this rename * Rename the new status field to match the new column * Fix up the tests and add a new one * Change the return value of some of the queries and simplify the validation check. Also add a test. * Change it back * Update the tests * Clean up the comments in the shares enum * rename the rotate keys error variant * Use a better error variant when extracting the started_at from the state machine Id * Remove some of our unused error variants * Match the behavior in the in memory store with the postgres implementation * We do not need these errors anymore either * address nits --------- Co-authored-by: Joey Yandle Co-authored-by: djordon Co-authored-by: Francesco Leacche --- Cargo.lock | 10 + Cargo.toml | 1 + signer/Cargo.toml | 1 + .../0012__dkg_verification_extensions.sql | 52 ++++ signer/src/bitcoin/utxo.rs | 186 ++++++++++++ signer/src/block_observer.rs | 4 + signer/src/error.rs | 26 +- signer/src/stacks/contracts.rs | 20 +- signer/src/storage/in_memory.rs | 29 ++ signer/src/storage/mod.rs | 22 ++ signer/src/storage/model.rs | 31 ++ signer/src/storage/postgres.rs | 55 +++- signer/src/testing/dummy.rs | 8 + signer/src/testing/request_decider.rs | 13 +- signer/src/testing/wsts.rs | 7 +- signer/src/transaction_coordinator.rs | 174 ++++++----- signer/src/transaction_signer.rs | 280 +++++++++++------- signer/src/wsts_state_machine.rs | 11 +- signer/tests/integration/postgres.rs | 227 +++++++++++++- signer/tests/integration/request_decider.rs | 2 +- signer/tests/integration/rotate_keys.rs | 81 +++++ signer/tests/integration/setup.rs | 6 + .../integration/transaction_coordinator.rs | 35 +-- .../tests/integration/transaction_signer.rs | 5 +- supply-chain/audits.toml | 11 + 25 files changed, 1074 insertions(+), 223 deletions(-) create mode 100644 signer/migrations/0012__dkg_verification_extensions.sql diff --git a/Cargo.lock b/Cargo.lock index fe6124217..a48ecaedf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -820,6 +820,15 @@ dependencies = [ "serde", ] +[[package]] +name = "bitcoinconsensus" +version = "0.106.0+26.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e12cba9cce5043cdda968e07b9df6d05ec6b0b38aa27a9a40bb575cf3e521ae9" +dependencies = [ + "cc", +] + [[package]] name = "bitcoincore-rpc" version = "0.19.0" @@ -5728,6 +5737,7 @@ dependencies = [ "aquamarine", "axum", "bitcoin", + "bitcoinconsensus", "bitcoincore-rpc", "bitcoincore-rpc-json", "bitcoincore-zmq", diff --git a/Cargo.toml b/Cargo.toml index 5fe1bea01..9fcaed320 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ backoff = { version = "0.4.0", default-features = false, features = ["tokio"] } base64 = { version = "0.22.1", default-features = false, features = ["alloc"] } bincode = { version = "1.3.3", default-features = false } bitcoin = { version = "0.32.5", default-features = false, features = ["serde", "rand-std"] } +bitcoinconsensus = { version = "0.106.0", default-features = false } bitcoincore-rpc = { version = "0.19.0", default-features = false } bitcoincore-rpc-json = { version = "0.19.0", default-features = false } bitcoincore-zmq = { version = "1.5.2", default-features = false, features = ["async"] } diff --git a/signer/Cargo.toml b/signer/Cargo.toml index 2aab423a0..133897305 100644 --- a/signer/Cargo.toml +++ b/signer/Cargo.toml @@ -14,6 +14,7 @@ testing = ["fake", "mockall", "sbtc/testing"] aquamarine.workspace = true axum.workspace = true bitcoin.workspace = true +bitcoinconsensus.workspace = true bitcoincore-rpc.workspace = true bitcoincore-rpc-json.workspace = true bitcoincore-zmq.workspace = true diff --git a/signer/migrations/0012__dkg_verification_extensions.sql b/signer/migrations/0012__dkg_verification_extensions.sql new file mode 100644 index 000000000..632409ccc --- /dev/null +++ b/signer/migrations/0012__dkg_verification_extensions.sql @@ -0,0 +1,52 @@ + +CREATE TYPE sbtc_signer.dkg_shares_status AS ENUM ( + 'unverified', + 'verified', + 'failed' +); + +-- Add the new columns to the `dkg_shares` table. We're not adding indexes for +-- now because the table is so small that the overhead likely outweighs the +-- benefits. +ALTER TABLE sbtc_signer.dkg_shares + ADD COLUMN dkg_shares_status sbtc_signer.dkg_shares_status, + ADD COLUMN started_at_bitcoin_block_hash BYTEA, + ADD COLUMN started_at_bitcoin_block_height BIGINT; + + +UPDATE sbtc_signer.dkg_shares +SET dkg_shares_status = 'unverified'; + +-- These are all DKG shares associated scriptPubKeys that have been successfully spent +UPDATE sbtc_signer.dkg_shares +SET dkg_shares_status = 'verified' +FROM sbtc_signer.bitcoin_tx_inputs +WHERE sbtc_signer.dkg_shares.script_pubkey = sbtc_signer.bitcoin_tx_inputs.script_pubkey; + + +-- Fill in the started at bitcoin blockhash and block height. The timestamp +-- of when we write the DKG shares row to the database should correspond +-- with the tenure of the block that started the DKG round. +WITH block_times AS ( + SELECT + bb1.block_hash + , bb1.block_height + , bb1.created_at + , bb2.created_at AS ended_at + FROM sbtc_signer.bitcoin_blocks AS bb2 + JOIN sbtc_signer.bitcoin_blocks AS bb1 + ON bb2.parent_hash = bb1.block_hash +) +UPDATE sbtc_signer.dkg_shares +SET + started_at_bitcoin_block_hash = block_times.block_hash + , started_at_bitcoin_block_height = block_times.block_height +FROM block_times +WHERE sbtc_signer.dkg_shares.created_at >= block_times.created_at + AND sbtc_signer.dkg_shares.created_at < block_times.ended_at; + +-- Make the new column `NOT NULL` now that they should all have a value. +ALTER TABLE sbtc_signer.dkg_shares + ALTER COLUMN dkg_shares_status SET NOT NULL, + ALTER COLUMN started_at_bitcoin_block_hash SET NOT NULL, + ALTER COLUMN started_at_bitcoin_block_height SET NOT NULL; diff --git a/signer/src/bitcoin/utxo.rs b/signer/src/bitcoin/utxo.rs index f8a1e549a..a5b3a52d7 100644 --- a/signer/src/bitcoin/utxo.rs +++ b/signer/src/bitcoin/utxo.rs @@ -5,6 +5,7 @@ use std::ops::Deref as _; use std::sync::LazyLock; use bitcoin::absolute::LockTime; +use bitcoin::consensus::Encodable; use bitcoin::hashes::Hash as _; use bitcoin::sighash::Prevouts; use bitcoin::sighash::SighashCache; @@ -712,6 +713,20 @@ impl SignerUtxo { } } +/// A struct for constructing a mock transaction that can be signed. This is +/// used as part of the verification process after a new DKG round has been +/// completed. +/// +/// The Bitcoin transaction has the following layout: +/// 1. The first input is spending the signers' UTXO. +/// 2. There is only one output which is an OP_RETURN and an amount equal to 0. +pub struct UnsignedMockTransaction { + /// The Bitcoin transaction that needs to be signed. + tx: Transaction, + /// The signers' UTXO used as an input to this transaction. + utxo: SignerUtxo, +} + /// Given a set of requests, create a BTC transaction that can be signed. /// /// This BTC transaction in this struct has correct amounts but no witness @@ -802,6 +817,94 @@ impl SignatureHashes<'_> { } } +impl UnsignedMockTransaction { + const AMOUNT: u64 = 0; + + /// Construct an unsigned mock transaction. + /// + /// This will use the provided `aggregate_key` to construct + /// a [`Transaction`] with a single input and output with value 0. + pub fn new(signer_public_key: XOnlyPublicKey) -> Self { + let utxo = SignerUtxo { + outpoint: OutPoint::null(), + amount: Self::AMOUNT, + public_key: signer_public_key, + }; + + let tx = Transaction { + version: Version::TWO, + lock_time: LockTime::ZERO, + input: vec![utxo.as_tx_input(&DUMMY_SIGNATURE)], + output: vec![TxOut { + value: Amount::from_sat(Self::AMOUNT), + script_pubkey: ScriptBuf::new_op_return([]), + }], + }; + + Self { tx, utxo } + } + + /// Gets the sighash for the signers' input UTXO which needs to be signed + /// before the transaction can be broadcast. + pub fn compute_sighash(&self) -> Result { + let prevouts = [self.utxo.as_tx_output()]; + let mut sighasher = SighashCache::new(&self.tx); + + sighasher + .taproot_key_spend_signature_hash(0, &Prevouts::All(&prevouts), TapSighashType::All) + .map_err(Into::into) + } + + /// Tests if the provided taproot [`Signature`] is valid for spending the + /// signers' UTXO. This function will return [`Error::BitcoinConsensus`] + /// error if the signature fails verification, passing the underlying error + /// from [`bitcoinconsensus`]. + pub fn verify_signature(&self, signature: &Signature) -> Result<(), Error> { + // Create a copy of the transaction so that we don't modify the + // transaction stored in the struct. + let mut tx = self.tx.clone(); + + // Set the witness data on the input from the provided signature. + tx.input[0].witness = Witness::p2tr_key_spend(signature); + + // Encode the transaction to bytes (needed by the bitcoinconsensus + // library). + let mut tx_bytes: Vec = Vec::new(); + tx.consensus_encode(&mut tx_bytes) + .map_err(Error::BitcoinIo)?; + + // Get the prevout for the signers' UTXO. + let prevout = self.utxo.as_tx_output(); + let prevout_script_bytes = prevout.script_pubkey.as_script().as_bytes(); + + // Create the bitcoinconsensus UTXO object. + let prevout_utxo = bitcoinconsensus::Utxo { + script_pubkey: prevout_script_bytes.as_ptr(), + script_pubkey_len: prevout_script_bytes.len() as u32, + value: Self::AMOUNT as i64, + }; + + // We specify the flags to include all pre-taproot and taproot + // verifications explicitly. + // https://github.com/rust-bitcoin/rust-bitcoinconsensus/blob/master/src/lib.rs + let flags = bitcoinconsensus::VERIFY_ALL_PRE_TAPROOT | bitcoinconsensus::VERIFY_TAPROOT; + + // Verify that the transaction updated with the provided signature can + // successfully spend the signers' UTXO. Note that the amount is not + // used in the verification process for taproot spends, only the + // signature. + bitcoinconsensus::verify_with_flags( + prevout_script_bytes, + Self::AMOUNT, + &tx_bytes, + Some(&[prevout_utxo]), + 0, + flags, + ) + .map_err(Error::BitcoinConsensus) + } +} + impl<'a> UnsignedTransaction<'a> { /// Construct an unsigned transaction. /// @@ -1457,6 +1560,7 @@ mod tests { use std::str::FromStr; use super::*; + use bitcoin::key::TapTweak; use bitcoin::BlockHash; use bitcoin::CompressedPublicKey; use bitcoin::Txid; @@ -1652,6 +1756,88 @@ mod tests { } } + /// This test verifies that our implementation of Bitcoin script + /// verification using [`bitcoinconsensus`] works as expected. This + /// functionality is used in the verification of WSTS signing after a new + /// DKG round has completed. + #[test] + fn mock_signer_utxo_signing_and_spending_verification() { + let secp = secp256k1::Secp256k1::new(); + + // Generate a key pair which will serve as the signers' aggregate key. + let secret_key = SecretKey::new(&mut OsRng); + let keypair = secp256k1::Keypair::from_secret_key(&secp, &secret_key); + let tweaked = keypair.tap_tweak(&secp, None); + let (aggregate_key, _) = keypair.x_only_public_key(); + + // Create a new transaction using the aggregate key. + let unsigned = UnsignedMockTransaction::new(aggregate_key); + + let tapsig = unsigned + .compute_sighash() + .expect("failed to compute taproot sighash"); + + // Sign the taproot sighash. + let message = secp256k1::Message::from_digest_slice(tapsig.as_byte_array()) + .expect("Failed to create message"); + + // [1] Verify the correct signature, which should succeed. + let schnorr_sig = secp.sign_schnorr(&message, &tweaked.to_inner()); + let taproot_sig = bitcoin::taproot::Signature { + signature: schnorr_sig, + sighash_type: TapSighashType::All, + }; + unsigned + .verify_signature(&taproot_sig) + .expect("signature verification failed"); + + // [2] Verify the correct signature, but with a different sighash type, + // which should fail. + let taproot_sig = bitcoin::taproot::Signature { + signature: schnorr_sig, + sighash_type: TapSighashType::None, + }; + unsigned + .verify_signature(&taproot_sig) + .expect_err("signature verification should have failed"); + + // [3] Verify an incorrect signature with the correct sighash type, + // which should fail. In this case we've created the signature using + // the untweaked keypair. + let schnorr_sig = secp.sign_schnorr(&message, &keypair); + let taproot_sig = bitcoin::taproot::Signature { + signature: schnorr_sig, + sighash_type: TapSighashType::All, + }; + unsigned + .verify_signature(&taproot_sig) + .expect_err("signature verification should have failed"); + + // [4] Verify an incorrect signature with the correct sighash type, which + // should fail. In this case we use a completely newly generated keypair. + let secret_key = SecretKey::new(&mut OsRng); + let keypair = secp256k1::Keypair::from_secret_key(&secp, &secret_key); + let schnorr_sig = secp.sign_schnorr(&message, &keypair); + let taproot_sig = bitcoin::taproot::Signature { + signature: schnorr_sig, + sighash_type: TapSighashType::All, + }; + unsigned + .verify_signature(&taproot_sig) + .expect_err("signature verification should have failed"); + + // [5] Same as [4], but using its tweaked key. + let tweaked = keypair.tap_tweak(&secp, None); + let schnorr_sig = secp.sign_schnorr(&message, &tweaked.to_inner()); + let taproot_sig = bitcoin::taproot::Signature { + signature: schnorr_sig, + sighash_type: TapSighashType::All, + }; + unsigned + .verify_signature(&taproot_sig) + .expect_err("signature verification should have failed"); + } + #[ignore = "For generating the SOLO_(DEPOSIT|WITHDRAWAL)_SIZE constants"] #[test] fn create_deposit_only_tx() { diff --git a/signer/src/block_observer.rs b/signer/src/block_observer.rs index 55030b7cd..ae197e9c1 100644 --- a/signer/src/block_observer.rs +++ b/signer/src/block_observer.rs @@ -694,6 +694,7 @@ mod tests { use crate::keys::PublicKey; use crate::keys::SignerScriptPubKey as _; use crate::storage; + use crate::storage::model::DkgSharesStatus; use crate::testing::block_observer::TestHarness; use crate::testing::context::*; @@ -996,6 +997,9 @@ mod tests { public_shares: Vec::new(), signer_set_public_keys: vec![aggregate_key], signature_share_threshold: 1, + dkg_shares_status: DkgSharesStatus::Unverified, + started_at_bitcoin_block_hash: block_hash.into(), + started_at_bitcoin_block_height: 1, }; storage.write_encrypted_dkg_shares(&shares).await.unwrap(); diff --git a/signer/src/error.rs b/signer/src/error.rs index cc2d8330d..558af5558 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -16,15 +16,33 @@ use crate::storage::model::SigHash; /// Top-level signer error #[derive(Debug, thiserror::Error)] pub enum Error { + /// Unexpected [`StateMachineId`] in the given context. + #[error("unexpected state machine id in the given context: {0:?}")] + UnexpectedStateMachineId(Box), + + /// An IO error was returned from the [`bitcoin`] library. This is usually an + /// error that occurred during encoding/decoding of bitcoin types. + #[error("an io error was returned from the bitcoin library: {0}")] + BitcoinIo(#[source] bitcoin::io::Error), + + /// An error was returned from the bitcoinconsensus library. + #[error("error returned from libbitcoinconsensus: {0}")] + BitcoinConsensus(bitcoinconsensus::Error), + /// We have received a request/response which has been deemed invalid in /// the current context. #[error("invalid signing request")] InvalidSigningOperation, - /// The pre-rotate-key frost verification signing round was not reported as - /// successful. - #[error("rotate-key frost verification signing round not reported as successful")] - FrostVerificationNotSuccessful, + /// No mock transaction was found after DKG successfully completed. Spending + /// a signer UTXO locked by the new aggregate key could not be verified. + #[error("no mock transaction found when attempting to sign")] + MissingMockTransaction, + + /// The rotate-key frost verification signing round failed for the aggregate + /// key. + #[error("rotate-key frost verification signing failed for aggregate key: {0}")] + DkgVerificationFailed(PublicKeyXOnly), /// No WSTS FROST state machine was found for the given aggregate key. This /// state machine is used during the DKG verification signing round diff --git a/signer/src/stacks/contracts.rs b/signer/src/stacks/contracts.rs index cdf8e7487..e78c0824d 100644 --- a/signer/src/stacks/contracts.rs +++ b/signer/src/stacks/contracts.rs @@ -52,6 +52,7 @@ use crate::stacks::wallet::SignerWallet; use crate::storage::model::BitcoinBlockHash; use crate::storage::model::BitcoinBlockRef; use crate::storage::model::BitcoinTxId; +use crate::storage::model::DkgSharesStatus; use crate::storage::model::ToLittleEndianOrder as _; use crate::storage::DbRead; use crate::DEPOSIT_DUST_LIMIT; @@ -1097,6 +1098,7 @@ impl AsContractCall for RotateKeysV1 { ClarityValue::UInt(self.signatures_required as u128), ] } + /// Validates that the rotate-keys-wrapper satisfies the following /// criteria: /// @@ -1105,9 +1107,10 @@ impl AsContractCall for RotateKeysV1 { /// DKG run. /// 3. That the aggregate key matches the one that was output as part of /// the most recent DKG. - /// 4. That the signature threshold matches the one that was used in the + /// 4. That the DKG shares are in the verified state. + /// 5. That the signature threshold matches the one that was used in the /// most recent DKG. - /// 5. That there are no other rotate-keys contract calls with these same + /// 6. That there are no other rotate-keys contract calls with these same /// details already confirmed on the canonical Stacks blockchain. async fn validate(&self, ctx: &C, req_ctx: &ReqContext) -> Result<(), Error> where @@ -1139,13 +1142,18 @@ impl AsContractCall for RotateKeysV1 { return Err(RotateKeysErrorMsg::AggregateKeyMismatch.into_error(req_ctx, self)); } - // 4. That the signature threshold matches the one that was used in the + // 4. That the DKG shares are in the verified state. + if !matches!(latest_dkg.dkg_shares_status, DkgSharesStatus::Verified) { + return Err(RotateKeysErrorMsg::DkgSharesNotVerified.into_error(req_ctx, self)); + } + + // 5. That the signature threshold matches the one that was used in the // most recent DKG. if self.signatures_required != latest_dkg.signature_share_threshold { return Err(RotateKeysErrorMsg::SignaturesRequiredMismatch.into_error(req_ctx, self)); } - // 5. That there are no other rotate-keys contract calls with these same + // 6. That there are no other rotate-keys contract calls with these same // details already confirmed on the canonical Stacks blockchain. let key_rotation_exists_fut = db.key_rotation_exists( &req_ctx.chain_tip.block_hash, @@ -1207,6 +1215,10 @@ pub enum RotateKeysErrorMsg { /// There is already a key rotation with the same details. #[error("there is already a key rotation with the same details")] KeyRotationExists, + /// The aggregate key is known but the associated secret shares have + /// not passed verification. + #[error("the shares associated with the aggregate key have not passes verification")] + DkgSharesNotVerified, } impl RotateKeysErrorMsg { diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index af978cd48..f20690026 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -25,6 +25,7 @@ use crate::storage::model::WithdrawalCreateEvent; use crate::storage::model::WithdrawalRejectEvent; use crate::DEPOSIT_LOCKTIME_BLOCK_BUFFER; +use super::model::DkgSharesStatus; use super::util::get_utxo; /// A store wrapped in an Arc> for interior mutability @@ -1276,4 +1277,32 @@ impl super::DbWrite for SharedStore { }); Ok(()) } + + async fn revoke_dkg_shares(&self, aggregate_key: X) -> Result + where + X: Into + Send, + { + let mut store = self.lock().await; + if let Some((_, shares)) = store.encrypted_dkg_shares.get_mut(&aggregate_key.into()) { + if shares.dkg_shares_status == DkgSharesStatus::Unverified { + shares.dkg_shares_status = DkgSharesStatus::Failed; + return Ok(true); + } + } + Ok(false) + } + + async fn verify_dkg_shares(&self, aggregate_key: X) -> Result + where + X: Into + Send, + { + let mut store = self.lock().await; + if let Some((_, shares)) = store.encrypted_dkg_shares.get_mut(&aggregate_key.into()) { + if shares.dkg_shares_status == DkgSharesStatus::Unverified { + shares.dkg_shares_status = DkgSharesStatus::Verified; + return Ok(true); + } + } + Ok(false) + } } diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index d6ff5b827..fdc316703 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -497,4 +497,26 @@ pub trait DbWrite { &self, withdrawals_outputs: &[model::BitcoinWithdrawalOutput], ) -> impl Future> + Send; + + /// Marks the stored DKG shares for the provided aggregate key as revoked + /// and thus should no longer be used. + /// + /// This can be due to a failed DKG process, the key having been + /// compromised, or any other reason that would require the shares for the + /// provided aggregate key to not be used in the signing of transactions. + fn revoke_dkg_shares( + &self, + aggregate_key: X, + ) -> impl Future> + Send + where + X: Into + Send; + + /// Marks the stored DKG shares as verified, meaning that the shares have + /// been used to sign a transaction input spending a UTXO locked by itself. + fn verify_dkg_shares( + &self, + aggregate_key: X, + ) -> impl Future> + Send + where + X: Into + Send; } diff --git a/signer/src/storage/model.rs b/signer/src/storage/model.rs index f7c5a94c9..1fcbb1651 100644 --- a/signer/src/storage/model.rs +++ b/signer/src/storage/model.rs @@ -463,6 +463,16 @@ pub struct EncryptedDkgShares { /// (shares) that are needed in order to construct a signature. #[sqlx(try_from = "i32")] pub signature_share_threshold: u16, + /// The current status of the DKG shares. + pub dkg_shares_status: DkgSharesStatus, + /// The block hash of the chain tip of the canonical bitcoin blockchain + /// when the DKG round associated with these shares started. + pub started_at_bitcoin_block_hash: BitcoinBlockHash, + /// The block height of the chain tip of the canonical bitcoin blockchain + /// when the DKG round associated with these shares started. + #[sqlx(try_from = "i64")] + #[cfg_attr(feature = "testing", dummy(faker = "0..i64::MAX as u64"))] + pub started_at_bitcoin_block_height: u64, } /// Persisted public DKG shares from other signers @@ -543,6 +553,20 @@ impl From for BitArray<[u8; 16]> { } } +/// The possible states for DKG shares. +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord, sqlx::Type)] +#[sqlx(type_name = "dkg_shares_status", rename_all = "snake_case")] +#[cfg_attr(feature = "testing", derive(fake::Dummy))] +pub enum DkgSharesStatus { + /// The DKG shares have not passed or failed verification. + Unverified, + /// The DKG shares have passed verification. + Verified, + /// The DKG shares have failed verification or the shares have not + /// passed verification within our configured window. + Failed, +} + /// The types of transactions the signer is interested in. #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord, sqlx::Type, strum::Display)] #[sqlx(type_name = "transaction_type", rename_all = "snake_case")] @@ -817,6 +841,7 @@ impl std::fmt::Display for BitcoinBlockHash { #[cfg_attr(feature = "testing", derive(fake::Dummy))] pub struct BitcoinBlockRef { /// The height of the block in the bitcoin blockchain. + #[cfg_attr(feature = "testing", dummy(faker = "0..u32::MAX as u64"))] #[sqlx(try_from = "i64")] pub block_height: u64, /// Bitcoin block hash. It uniquely identifies the bitcoin block. @@ -838,6 +863,12 @@ impl From<&BitcoinBlock> for BitcoinBlockRef { } } +impl AsRef for BitcoinBlockRef { + fn as_ref(&self) -> &BitcoinBlockHash { + &self.block_hash + } +} + /// The Stacks block ID. This is different from the block header hash. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)] #[serde(transparent)] diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 4033ff4a1..3e9110b18 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -1490,6 +1490,9 @@ impl super::DbRead for PgStore { , public_shares , signer_set_public_keys , signature_share_threshold + , dkg_shares_status + , started_at_bitcoin_block_hash + , started_at_bitcoin_block_height FROM sbtc_signer.dkg_shares WHERE substring(aggregate_key FROM 2) = $1; "#, @@ -1513,6 +1516,9 @@ impl super::DbRead for PgStore { , public_shares , signer_set_public_keys , signature_share_threshold + , dkg_shares_status + , started_at_bitcoin_block_hash + , started_at_bitcoin_block_height FROM sbtc_signer.dkg_shares ORDER BY created_at DESC LIMIT 1; @@ -2430,6 +2436,9 @@ impl super::DbWrite for PgStore { &self, shares: &model::EncryptedDkgShares, ) -> Result<(), Error> { + let started_at_bitcoin_block_height = i64::try_from(shares.started_at_bitcoin_block_height) + .map_err(Error::ConversionDatabaseInt)?; + sqlx::query( r#" INSERT INTO sbtc_signer.dkg_shares ( @@ -2440,8 +2449,11 @@ impl super::DbWrite for PgStore { , script_pubkey , signer_set_public_keys , signature_share_threshold + , dkg_shares_status + , started_at_bitcoin_block_hash + , started_at_bitcoin_block_height ) - VALUES ($1, $2, $3, $4, $5, $6, $7) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) ON CONFLICT DO NOTHING"#, ) .bind(shares.aggregate_key) @@ -2451,6 +2463,9 @@ impl super::DbWrite for PgStore { .bind(&shares.script_pubkey) .bind(&shares.signer_set_public_keys) .bind(i32::from(shares.signature_share_threshold)) + .bind(shares.dkg_shares_status) + .bind(shares.started_at_bitcoin_block_hash) + .bind(started_at_bitcoin_block_height) .execute(&self.0) .await .map_err(Error::SqlxQuery)?; @@ -2853,6 +2868,44 @@ impl super::DbWrite for PgStore { Ok(()) } + + async fn revoke_dkg_shares(&self, aggregate_key: X) -> Result + where + X: Into + Send, + { + sqlx::query( + r#" + UPDATE sbtc_signer.dkg_shares + SET dkg_shares_status = 'failed' + WHERE substring(aggregate_key FROM 2) = $1 + AND dkg_shares_status = 'unverified'; -- only allow failing pending entries + "#, + ) + .bind(aggregate_key.into()) + .execute(&self.0) + .await + .map(|res| res.rows_affected() > 0) + .map_err(Error::SqlxQuery) + } + + async fn verify_dkg_shares(&self, aggregate_key: X) -> Result + where + X: Into + Send, + { + sqlx::query( + r#" + UPDATE sbtc_signer.dkg_shares + SET dkg_shares_status = 'verified' + WHERE substring(aggregate_key FROM 2) = $1 + AND dkg_shares_status = 'unverified'; -- only allow verifying pending entries + "#, + ) + .bind(aggregate_key.into()) + .execute(&self.0) + .await + .map(|res| res.rows_affected() > 0) + .map_err(Error::SqlxQuery) + } } #[cfg(test)] diff --git a/signer/src/testing/dummy.rs b/signer/src/testing/dummy.rs index 7b805ed83..593162560 100644 --- a/signer/src/testing/dummy.rs +++ b/signer/src/testing/dummy.rs @@ -71,6 +71,7 @@ use crate::storage::model::BitcoinBlockHash; use crate::storage::model::BitcoinTx; use crate::storage::model::BitcoinTxId; use crate::storage::model::CompletedDepositEvent; +use crate::storage::model::DkgSharesStatus; use crate::storage::model::EncryptedDkgShares; use crate::storage::model::QualifiedRequestId; use crate::storage::model::RotateKeysTransaction; @@ -240,6 +241,7 @@ pub fn encrypted_dkg_shares( rng: &mut R, signer_private_key: &[u8; 32], group_key: PublicKey, + status: DkgSharesStatus, ) -> model::EncryptedDkgShares { let party_state = wsts::traits::PartyState { polynomial: None, @@ -272,6 +274,9 @@ pub fn encrypted_dkg_shares( script_pubkey: group_key.signers_script_pubkey().into(), signer_set_public_keys: vec![fake::Faker.fake_with_rng(rng)], signature_share_threshold: 1, + dkg_shares_status: status, + started_at_bitcoin_block_hash: Faker.fake_with_rng(rng), + started_at_bitcoin_block_height: Faker.fake_with_rng::(rng) as u64, } } @@ -441,6 +446,9 @@ impl fake::Dummy for EncryptedDkgShares { public_shares: Vec::new(), signer_set_public_keys, signature_share_threshold: config.signatures_required, + dkg_shares_status: DkgSharesStatus::Verified, + started_at_bitcoin_block_hash: Faker.fake_with_rng(rng), + started_at_bitcoin_block_height: Faker.fake_with_rng::(rng) as u64, } } } diff --git a/signer/src/testing/request_decider.rs b/signer/src/testing/request_decider.rs index c6fa4b07c..d52626af6 100644 --- a/signer/src/testing/request_decider.rs +++ b/signer/src/testing/request_decider.rs @@ -18,6 +18,7 @@ use crate::network::MessageTransfer as _; use crate::request_decider::RequestDeciderEventLoop; use crate::storage; use crate::storage::model; +use crate::storage::model::DkgSharesStatus; use crate::storage::DbRead; use crate::storage::DbWrite; use crate::testing; @@ -177,6 +178,7 @@ where &handle.context.get_storage_mut(), group_key, signer_set.clone(), + DkgSharesStatus::Verified, ) .await; @@ -342,6 +344,7 @@ where &handle.context.get_storage_mut(), group_key, signer_set.clone(), + DkgSharesStatus::Verified, ) .await; } @@ -539,12 +542,18 @@ async fn store_dummy_dkg_shares( storage: &S, group_key: PublicKey, signer_set: BTreeSet, + status: DkgSharesStatus, ) where R: rand::CryptoRng + rand::RngCore, S: storage::DbWrite, { - let mut shares = - testing::dummy::encrypted_dkg_shares(&fake::Faker, rng, signer_private_key, group_key); + let mut shares = testing::dummy::encrypted_dkg_shares( + &fake::Faker, + rng, + signer_private_key, + group_key, + status, + ); shares.signer_set_public_keys = signer_set.into_iter().collect(); storage diff --git a/signer/src/testing/wsts.rs b/signer/src/testing/wsts.rs index 5fad7f508..4c37a909a 100644 --- a/signer/src/testing/wsts.rs +++ b/signer/src/testing/wsts.rs @@ -420,6 +420,11 @@ impl SignerSet { self.signers.push(signer) } + let started_at = model::BitcoinBlockRef { + block_hash: bitcoin_chain_tip, + block_height: 0, + }; + ( aggregate_key, self.signers @@ -427,7 +432,7 @@ impl SignerSet { .map(|signer| { signer .wsts_signer - .get_encrypted_dkg_shares(rng) + .get_encrypted_dkg_shares(rng, &started_at) .expect("failed to get encrypted shares") }) .collect(), diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 8f3f795d7..7280cafc7 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -17,6 +17,7 @@ use sha2::Digest; use crate::bitcoin::utxo; use crate::bitcoin::utxo::Fees; +use crate::bitcoin::utxo::UnsignedMockTransaction; use crate::bitcoin::BitcoinInteract; use crate::bitcoin::TransactionLookupHint; use crate::context::Context; @@ -376,6 +377,10 @@ where return Ok(()); } + tracing::info!("our aggregate key differs from the one in the registry contract; a key rotation may be necessary"); + + // Load the Stacks wallet. + tracing::debug!("loading the signer stacks wallet"); let wallet = self.get_signer_wallet(bitcoin_chain_tip).await?; // current_aggregate_key define which wallet can sign stacks tx interacting @@ -383,6 +388,13 @@ where // the first rotate key tx. let signing_key = ¤t_aggregate_key.unwrap_or(*aggregate_key); + // Perform DKG verification before submitting the rotate key transaction. + tracing::info!("🔐 beginning DKG verification before submitting rotate-key transaction"); + self.perform_dkg_verification(bitcoin_chain_tip, &last_dkg.aggregate_key, &wallet) + .await?; + tracing::info!("🔐 DKG verification successful"); + + // Construct, sign and submit the rotate key transaction. tracing::info!("preparing to submit a rotate-key transaction"); let txid = self .construct_and_sign_rotate_key_transaction( @@ -694,6 +706,85 @@ where Ok(()) } + /// Performs verification of the DKG process by running a FROST signing + /// round using the new key. This is done to assert that all signers have + /// successfully signed with the new aggregate key before proceeding with + /// the actual rotate keys transaction. + /// + /// The idea behind this is that since the rotate-keys contract call is a + /// Stacks transaction and thus only signed using the signers' private keys, + /// we have no guarantees at this point that there wasn't a fault in the DKG + /// process. By running a FROST signing round, we can cryptographically + /// assert that all signers have signed with the new aggregate key, and thus + /// have valid private shares before we proceed with the actual rotate keys + /// transaction. This is guaranteed by the FROST coordinator, which requires + /// 100% signing participation vs. FIRE which only uses `threshold` signers. + #[tracing::instrument(skip_all)] + async fn perform_dkg_verification( + &mut self, + bitcoin_chain_tip: &model::BitcoinBlockHash, + aggregate_key: &PublicKey, + wallet: &SignerWallet, + ) -> Result<(), Error> { + let (x_only_pubkey, _) = aggregate_key.x_only_public_key(); + + // Note that while we specify the threshold as `signatures_required` in + // the coordinator below, the FROST coordinator implicitly requires all + // signers to participate. + tracing::info!(%aggregate_key, "🔐 preparing to coordinate a FROST signing round to verify the aggregate key"); + let mut frost_coordinator = FrostCoordinator::load( + &self.context.get_storage(), + aggregate_key.into(), + wallet.public_keys().iter().cloned(), + wallet.signatures_required(), + self.private_key, + ) + .await?; + + // We create an `UnsignedMockTransaction` which tries to spend an input + // locked by the new aggregate key in the same way that the signer + // UTXO's are locked. This transaction is then used to compute the + // sighash that the signers will sign. + // + // This transaction does not spend from a valid (existing) UTXO and is + // never broadcast to the Bitcoin network. + let mock_tx = UnsignedMockTransaction::new(x_only_pubkey); + let tap_sighash = mock_tx.compute_sighash()?; + + // Perform the signing round. We will not use the resulting signature + // for anything here, rather each signer will also construct an + // `UnsignedMockTransaction` upon completion of the signing rounds and + // attempt to spend the locked UTXO input with the resulting signature. + // If script signature validation fails for any of the signers, they + // will mark the DKG round as failed and will refuse to sign the rotate + // keys transaction. + tracing::info!("🔐 beginning verification signing round"); + let signature = self.coordinate_signing_round( + bitcoin_chain_tip, + &mut frost_coordinator, + WstsMessageId::DkgVerification(*aggregate_key), + tap_sighash.as_byte_array(), + SignatureType::Taproot(None), + ) + .await + .inspect_err(|error| { + tracing::warn!(%error, "🔐 failed to assert that all signers have signed with the new aggregate key; aborting"); + })?; + + // Verify the signature against the mock transaction. This signer's + // tx-signer will also perform this verification, but we want to exit + // early if the signature is invalid to avoid moving on to the + // rotate-key contract call unnecessarily. + mock_tx.verify_signature(&signature) + .inspect_err(|error| { + tracing::warn!(%error, "🔐 signing round completed successfully, but the signature failed validation; aborting"); + })?; + + tracing::info!("🔐 all signers have signed with the new aggregate key; proceeding"); + + Ok(()) + } + /// Construct and coordinate signing round for a `rotate-keys-wrapper` transaction. #[tracing::instrument(skip_all)] async fn construct_and_sign_rotate_key_transaction( @@ -725,50 +816,6 @@ where let multi_tx = MultisigTx::new_tx(&contract_call, wallet, tx_fee); let tx = multi_tx.tx(); - // We run a DKG signing round on the current bitcoin chain tip block - // hash using the new aggregate key using the FROST coordinator, which - // requires 100% signing participation vs. FIRE which only uses - // {threshold} signers. - // - // The idea behind this is that since the rotate-keys contract call is a - // Stacks transaction and thus only signed using the signers' private - // keys, we have no guarantees at this point that there wasn't a fault - // in the DKG process. By running a FROST signing round, we can - // cryptographically assert that all signers have signed with the new - // aggregate key, and thus have valid private shares before we proceed - // with the actual rotate keys transaction. - // - // Note that while we specify the threshold as `signatures_required` in - // the coordinator below, the FROST coordinator implicitly requires all - // signers to participate. - tracing::info!("running a FROST signing round on random data to assert that all signers have signed with the new aggregate key"); - let mut coordinator_state_machine = FrostCoordinator::load( - &self.context.get_storage(), - rotate_key_aggregate_key.into(), - wallet.public_keys().iter().cloned(), - wallet.signatures_required(), - self.private_key, - ) - .await?; - - // We use the current bitcoin chain tip block hash as the data to sign - // as it is benign and can be validated by the signers. This may need - // to change in the future if we de-couple DKG from blocks. - let to_sign = bitcoin_chain_tip.as_byte_array().as_slice(); - self.coordinate_signing_round( - bitcoin_chain_tip, - &mut coordinator_state_machine, - WstsMessageId::DkgVerification(*rotate_key_aggregate_key), - to_sign, - SignatureType::Schnorr - ).await - .inspect_err(|error| { - tracing::error!(%error, "failed to assert that all signers have signed random data with the new aggregate key; aborting"); - })?; - tracing::info!( - "all signers have signed random data with the new aggregate key; proceeding" - ); - // We can now proceed with the actual rotate key transaction. let sign_request = StacksTransactionSignRequest { aggregate_key: *aggregate_key, @@ -966,7 +1013,7 @@ where transaction: &mut utxo::UnsignedTransaction<'_>, ) -> Result<(), Error> { let sighashes = transaction.construct_digests()?; - let mut coordinator_state_machine = FireCoordinator::load( + let mut fire_coordinator = FireCoordinator::load( &self.context.get_storage(), sighashes.signers_aggregate_key.into(), signer_public_keys.clone(), @@ -982,7 +1029,7 @@ where let signature = self .coordinate_signing_round( bitcoin_chain_tip, - &mut coordinator_state_machine, + &mut fire_coordinator, message_id, &msg, SignatureType::Taproot(None), @@ -1010,7 +1057,7 @@ where for (deposit, sighash) in sighashes.deposits.into_iter() { let msg = sighash.to_raw_hash().to_byte_array(); - let mut coordinator_state_machine = FireCoordinator::load( + let mut fire_coordinator = FireCoordinator::load( &self.context.get_storage(), deposit.signers_public_key.into(), signer_public_keys.clone(), @@ -1023,7 +1070,7 @@ where let signature = self .coordinate_signing_round( bitcoin_chain_tip, - &mut coordinator_state_machine, + &mut fire_coordinator, message_id, &msg, SignatureType::Schnorr, @@ -1090,7 +1137,7 @@ where async fn coordinate_signing_round( &mut self, bitcoin_chain_tip: &model::BitcoinBlockHash, - coordinator_state_machine: &mut Coordinator, + coordinator: &mut Coordinator, id: WstsMessageId, msg: &[u8], signature_type: SignatureType, @@ -1098,7 +1145,7 @@ where where Coordinator: WstsCoordinator, { - let outbound = coordinator_state_machine.start_signing_round(msg, signature_type)?; + let outbound = 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. @@ -1111,12 +1158,8 @@ where self.send_message(msg, bitcoin_chain_tip).await?; let max_duration = self.signing_round_max_duration; - let run_signing_round = self.drive_wsts_state_machine( - signal_stream, - bitcoin_chain_tip, - coordinator_state_machine, - id, - ); + let run_signing_round = + self.drive_wsts_state_machine(signal_stream, bitcoin_chain_tip, coordinator, id); let operation_result = tokio::time::timeout(max_duration, run_signing_round) .await @@ -1200,7 +1243,7 @@ where &mut self, signal_stream: S, bitcoin_chain_tip: &model::BitcoinBlockHash, - coordinator_state_machine: &mut Coordinator, + coordinator: &mut Coordinator, id: WstsMessageId, ) -> Result where @@ -1235,7 +1278,7 @@ where let sender_is_coordinator = given_key_is_coordinator(msg_public_key, bitcoin_chain_tip, &signer_set); - let public_keys = &coordinator_state_machine.get_config().signer_public_keys; + let public_keys = &coordinator.get_config().signer_public_keys; let public_key_point = p256k1::point::Point::from(msg_public_key); let msg = wsts_msg.inner; @@ -1252,14 +1295,13 @@ where continue; } - let (outbound_packet, operation_result) = - match coordinator_state_machine.process_message(&msg) { - Ok(val) => val, - Err(err) => { - tracing::warn!(?msg, reason = %err, "ignoring message"); - continue; - } - }; + let (outbound_packet, operation_result) = match coordinator.process_message(&msg) { + Ok(val) => val, + Err(err) => { + tracing::warn!(?msg, reason = %err, "ignoring message"); + continue; + } + }; if let Some(packet) = outbound_packet { let msg = message::WstsMessage { id, inner: packet.msg }; diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs index c140b4101..79057c712 100644 --- a/signer/src/transaction_signer.rs +++ b/signer/src/transaction_signer.rs @@ -9,6 +9,7 @@ use std::collections::BTreeSet; use std::num::NonZeroUsize; use std::time::Duration; +use crate::bitcoin::utxo::UnsignedMockTransaction; use crate::bitcoin::validation::BitcoinTxContext; use crate::context::Context; use crate::context::P2PEvent; @@ -31,6 +32,7 @@ use crate::metrics::Metrics; use crate::metrics::BITCOIN_BLOCKCHAIN; use crate::metrics::STACKS_BLOCKCHAIN; use crate::network; +use crate::signature::TaprootSignature; use crate::stacks::contracts::AsContractCall as _; use crate::stacks::contracts::ContractCall; use crate::stacks::contracts::ReqContext; @@ -144,7 +146,7 @@ pub struct TxSignerEventLoop { /// verification of the Stacks rotate-keys transaction. pub dkg_verification_state_machines: LruCache, /// Results of DKG verification rounds. - pub dkg_verification_results: LruCache, + pub dkg_verification_results: LruCache, } /// This struct represents a signature hash and the public key that locks @@ -498,6 +500,7 @@ where deployer: self.context.config().signer.deployer, }; let ctx = &self.context; + tracing::info!("running validation on stacks transaction"); match &request.contract_tx { StacksTx::ContractCall(ContractCall::AcceptWithdrawalV1(contract)) => { @@ -510,19 +513,6 @@ where contract.validate(ctx, &req_ctx).await? } StacksTx::ContractCall(ContractCall::RotateKeysV1(contract)) => { - let aggregate_key = contract.aggregate_key.x_only_public_key().0.into(); - let frost_result = self - .dkg_verification_results - .peek(&StateMachineId::RotateKey( - aggregate_key, - chain_tip.block_hash, - )); - - if !matches!(frost_result, Some(true)) { - tracing::warn!("no successful frost signing round for the pre-rotate-keys verification signing round; refusing to sign"); - return Err(Error::FrostVerificationNotSuccessful); - } - contract.validate(ctx, &req_ctx).await? } StacksTx::SmartContract(smart_contract) => { @@ -585,7 +575,7 @@ where self.threshold, self.signer_private_key, )?; - let id = StateMachineId::Dkg(chain_tip.block_hash); + let id = StateMachineId::Dkg(*chain_tip); self.wsts_state_machines.put(id, state_machine); if let Some(pause) = self.dkg_begin_pause { @@ -612,7 +602,7 @@ where tracing::debug!("processing message"); - let id = StateMachineId::Dkg(chain_tip.block_hash); + let id = StateMachineId::Dkg(*chain_tip); self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) .await?; } @@ -622,7 +612,7 @@ where tracing::debug!("processing message"); - let id = StateMachineId::Dkg(chain_tip.block_hash); + let id = StateMachineId::Dkg(*chain_tip); self.validate_sender(&id, request.signer_id, &msg_public_key)?; self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) .await?; @@ -633,7 +623,7 @@ where tracing::debug!("processing message"); - let id = StateMachineId::Dkg(chain_tip.block_hash); + let id = StateMachineId::Dkg(*chain_tip); self.validate_sender(&id, request.signer_id, &msg_public_key)?; self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) .await?; @@ -650,7 +640,7 @@ where } tracing::debug!("processing message"); - let id = StateMachineId::Dkg(chain_tip.block_hash); + let id = StateMachineId::Dkg(*chain_tip); self.relay_message(id, msg.id, &msg.inner, &chain_tip.block_hash) .await?; } @@ -662,7 +652,7 @@ where DkgStatus::Success => { tracing::info!( wsts_dkg_status = "success", - "signer reports successful dkg round" + "signer reports successful DKG round" ); } DkgStatus::Failure(reason) => { @@ -698,7 +688,6 @@ where } WstsMessageId::Sweep(txid) => { span.record("txid", txid.to_string()); - tracing::info!("processing message"); let accepted_sighash = Self::validate_bitcoin_sign_request(&db, &request.message).await; @@ -739,20 +728,22 @@ where Self::validate_dkg_verification_message( &db, &new_key, - chain_tip, - &request.message, + Some(&request.message), ) .await?; - let state_machine_id = self - .ensure_dkg_verification_state_machine(chain_tip.block_hash, new_key) + let (state_machine_id, _, mock_tx) = self + .ensure_dkg_verification_state_machine(&chain_tip.block_hash, new_key) + .await?; + + let tap_sighash = mock_tx.compute_sighash()?; + if tap_sighash.as_byte_array() != request.message.as_slice() { + tracing::warn!("🔐 sighash mismatch for DKG verification signing"); + return Err(Error::InvalidSigningOperation); + } + + self.handle_dkg_verification_message(state_machine_id, &msg.inner) .await?; - self.process_dkg_verification_message( - new_key, - state_machine_id, - &msg.inner, - ) - .await?; (state_machine_id, new_key) } @@ -794,7 +785,7 @@ where } WstsMessageId::Sweep(txid) => { span.record("txid", txid.to_string()); - tracing::info!( + tracing::debug!( signature_type = ?request.signature_type, "processing message" ); @@ -819,21 +810,26 @@ where Self::validate_dkg_verification_message( &db, &new_key, - chain_tip, - &request.message, + Some(&request.message), ) .await?; - tracing::debug!( + tracing::info!( signature_type = ?request.signature_type, - "processing message" + "🔐 responding to signature-share-request for DKG verification signing" ); - let key = key.into(); - let state_machine_id = self - .ensure_dkg_verification_state_machine(chain_tip.block_hash, key) + let (state_machine_id, _, mock_tx) = self + .ensure_dkg_verification_state_machine(&chain_tip.block_hash, new_key) .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + + let tap_sighash = mock_tx.compute_sighash()?; + if tap_sighash.as_byte_array() != request.message.as_slice() { + tracing::warn!("🔐 sighash mismatch for DKG verification signing"); + return Err(Error::InvalidSigningOperation); + } + + self.handle_dkg_verification_message(state_machine_id, &msg.inner) .await?; state_machine_id } @@ -856,12 +852,26 @@ where return Ok(()); }; - let key = key.into(); + let new_key: PublicKeyXOnly = key.into(); + + Self::validate_dkg_verification_message( + &self.context.get_storage(), + &new_key, + Some(&request.message), + ) + .await?; - let state_machine_id = self - .ensure_dkg_verification_state_machine(chain_tip.block_hash, key) + let (state_machine_id, _, mock_tx) = self + .ensure_dkg_verification_state_machine(&chain_tip.block_hash, new_key) .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + + let tap_sighash = mock_tx.compute_sighash()?; + if tap_sighash.as_byte_array() != request.message.as_slice() { + tracing::warn!("🔐 sighash mismatch for DKG verification signing"); + return Err(Error::InvalidSigningOperation); + } + + self.handle_dkg_verification_message(state_machine_id, &msg.inner) .await?; } WstsNetMessage::SignatureShareResponse(request) => { @@ -874,12 +884,20 @@ where return Ok(()); }; - let key = key.into(); + let new_key = key.into(); - let state_machine_id = self - .ensure_dkg_verification_state_machine(chain_tip.block_hash, key) + Self::validate_dkg_verification_message( + &self.context.get_storage(), + &new_key, + None, + ) + .await?; + + let (state_machine_id, _, _) = self + .ensure_dkg_verification_state_machine(&chain_tip.block_hash, new_key) .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + + self.handle_dkg_verification_message(state_machine_id, &msg.inner) .await?; } } @@ -896,33 +914,37 @@ where async fn validate_dkg_verification_message( storage: &DB, new_key: &PublicKeyXOnly, - chain_tip: &model::BitcoinBlockRef, - message: &[u8], + message: Option<&[u8]>, ) -> Result<(), Error> where DB: DbRead, { - let current_key = storage + let latest_key = storage .get_latest_encrypted_dkg_shares() .await? .ok_or(Error::NoDkgShares)? .aggregate_key .into(); - if *new_key != current_key { - tracing::warn!("aggregate key mismatch for DKG verification signing"); + // Ensure that the new key matches the current aggregate key. + if *new_key != latest_key { + tracing::warn!("🔐 aggregate key mismatch for DKG verification signing"); return Err(Error::AggregateKeyMismatch( - Box::new(current_key), + Box::new(latest_key), Box::new(*new_key), )); } - if (chain_tip.block_hash.as_ref()) != message { - tracing::warn!( - data = %hex::encode(message), - data_len = message.len(), - "data received for DKG verification signing does not match current bitcoin chain tip block hash" - ); + // If we don't have a message (i.e. from `SignatureShareResponse`) then + // we can exit early. + let Some(message) = message else { + return Ok(()); + }; + + // Ensure that the received message is 32 bytes long (the length of the + // sighash we'll be signing). + if message.len() != 32 { + tracing::warn!("🔐 data received for DKG verification signing is not 32 bytes"); return Err(Error::InvalidSigningOperation); } @@ -981,9 +1003,14 @@ where .get(id) .ok_or(Error::MissingStateMachine)?; - let encrypted_dkg_shares = state_machine.get_encrypted_dkg_shares(&mut self.rng)?; + let StateMachineId::Dkg(started_at) = id else { + return Err(Error::UnexpectedStateMachineId(Box::new(*id))); + }; + + let encrypted_dkg_shares = + state_machine.get_encrypted_dkg_shares(&mut self.rng, started_at)?; - tracing::debug!("storing DKG shares"); + tracing::debug!("🔐 storing DKG shares"); self.context .get_storage_mut() .write_encrypted_dkg_shares(&encrypted_dkg_shares) @@ -992,8 +1019,6 @@ where Ok(()) } - // Backported from the feat/mock-signing due to confused merge tool - #[allow(dead_code)] async fn create_frost_coordinator( storage: &S, aggregate_key: PublicKeyXOnly, @@ -1019,7 +1044,7 @@ where num_signers = signing_set.len(), %aggregate_key, threshold = %dkg_shares.signature_share_threshold, - "🔐 creating now frost coordinator to track pre-rotate-key validation signing round" + "🔐 creating now FROST coordinator to track DKG verification signing round" ); FrostCoordinator::load( @@ -1041,89 +1066,113 @@ where /// which is being verified. async fn ensure_dkg_verification_state_machine( &mut self, - bitcoin_chain_tip: model::BitcoinBlockHash, + bitcoin_chain_tip: &model::BitcoinBlockHash, aggregate_key: PublicKeyXOnly, - ) -> Result { - let state_machine_id = StateMachineId::RotateKey(aggregate_key, bitcoin_chain_tip); + ) -> Result< + ( + StateMachineId, + &mut FrostCoordinator, + &UnsignedMockTransaction, + ), + Error, + > { + let state_machine_id = StateMachineId::RotateKey(aggregate_key, *bitcoin_chain_tip); if !self .dkg_verification_state_machines .contains(&state_machine_id) { let storage = self.context.get_storage(); - - // Load the DKG shares for the given aggregate key. - let dkg_shares = storage - .get_encrypted_dkg_shares(aggregate_key) - .await? - .ok_or_else(|| { - tracing::warn!("no DKG shares found for requested aggregate key"); - Error::MissingDkgShares(aggregate_key) - })?; - - // Get the signing set's public keys in a BTreeSet to deduplicate - // and sort them for the coordinator. - let signing_set: BTreeSet = dkg_shares - .signer_set_public_keys - .into_iter() - .collect::>(); - - // This `as` cast should always be safe as our signer cap is 128. - let num_signers = signing_set.len() as u16; - - // Create the WSTS FROST coordinator. - tracing::debug!(%num_signers, "creating new FROST coordinator to track DKG verification signing round"); - let coordinator = FrostCoordinator::load( - &storage, - aggregate_key, - signing_set, - dkg_shares.signature_share_threshold, - self.signer_private_key, - ) - .await?; - - // Insert the state machine into the cache if not existing. + let coordinator = + Self::create_frost_coordinator(&storage, aggregate_key, self.signer_private_key) + .await?; self.dkg_verification_state_machines .put(state_machine_id, coordinator); } - Ok(state_machine_id) + let state_machine = self + .dkg_verification_state_machines + .get_mut(&state_machine_id) + .ok_or(Error::MissingFrostStateMachine(aggregate_key))?; + + let mock_tx = self + .dkg_verification_results + .get_or_insert(state_machine_id, || { + UnsignedMockTransaction::new(aggregate_key.into()) + }); + + Ok((state_machine_id, state_machine, mock_tx)) } #[tracing::instrument(skip_all)] - async fn process_dkg_verification_message( + async fn handle_dkg_verification_message( &mut self, - aggregate_key: PublicKeyXOnly, id: StateMachineId, msg: &WstsNetMessage, ) -> Result<(), Error> { + // We should only be handling messages for the DKG verification state + // machine. We'll grab the aggregate key from the id as well. + let aggregate_key = match id { + StateMachineId::RotateKey(aggregate_key, _) => aggregate_key, + _ => { + tracing::warn!("🔐 unexpected state machine id for DKG verification signing round"); + return Err(Error::UnexpectedStateMachineId(Box::new(id))); + } + }; + let state_machine = self.dkg_verification_state_machines.get_mut(&id); let Some(state_machine) = state_machine else { - tracing::warn!("missing FROST coordinator for dkg verification message"); + tracing::warn!("🔐 missing FROST coordinator for DKG verification"); return Err(Error::MissingFrostStateMachine(aggregate_key)); }; - tracing::trace!(?msg, "processing FROST coordinator message"); + tracing::trace!(?msg, "🔐 processing FROST coordinator message"); let (_, result) = state_machine.process_message(msg)?; match result { - Some(OperationResult::SignSchnorr(_)) => { - tracing::info!("successfully completed DKG verification signing round"); - self.dkg_verification_results.put(id, true); + Some(OperationResult::SignTaproot(sig)) => { + tracing::info!("🔐 successfully completed DKG verification signing round"); self.dkg_verification_state_machines.pop(&id); + + let Some(mock_tx) = self.dkg_verification_results.pop(&id) else { + tracing::warn!( + "🔐 missing mock transaction for DKG verification signing round" + ); + return Err(Error::MissingMockTransaction); + }; + + // Perform verification of the signature. + tracing::info!("🔐 verifying that the signature can be used to spend a UTXO locked by the new aggregate key"); + let signature: TaprootSignature = sig.into(); + mock_tx + .verify_signature(&signature) + .inspect_err(|e| tracing::warn!(?e, "🔐 signature verification failed"))?; + tracing::info!("🔐 signature verification successful"); + + self.context + .get_storage_mut() + .verify_dkg_shares(aggregate_key) + .await?; + tracing::info!( + "🔐 DKG shares entry has been marked as verified; it is now able to be used" + ); } Some(OperationResult::SignError(error)) => { tracing::warn!( ?msg, ?error, - "failed to complete DKG verification signing round" + "🔐 failed to complete DKG verification signing round" ); - self.dkg_verification_results.put(id, false); + self.dkg_verification_results.pop(&id); + return Err(Error::DkgVerificationFailed(aggregate_key)); } None => {} result => { - tracing::warn!(?result, "unexpected FROST coordinator result"); + tracing::warn!( + ?result, + "🔐 unexpected result received from the FROST coordinator" + ); } } @@ -1143,6 +1192,10 @@ where return Err(Error::MissingStateMachine); }; + // If this is a DKG verification then we need to process the message in + // the frost coordinator as well to be able to properly follow the + // signing round (which is otherwise handled by the signer state + // machine). let mut frost_coordinator = if let StateMachineId::RotateKey(_, _) = state_machine_id { self.dkg_verification_state_machines .get_mut(&state_machine_id) @@ -1158,6 +1211,9 @@ where .process(outbound_message) .map_err(Error::Wsts)?; + // Process the message in the frost coordinator as well, if we have + // one. Note that we _do not_ send any messages to the network; the + // frost coordinator is only following the round. if let Some(ref mut frost_coordinator) = frost_coordinator { frost_coordinator.process_message(outbound_message)?; } @@ -1538,8 +1594,8 @@ mod tests { threshold: 1, rng: rand::rngs::OsRng, dkg_begin_pause: None, - dkg_verification_results: LruCache::new(NonZeroUsize::new(5).unwrap()), dkg_verification_state_machines: LruCache::new(NonZeroUsize::new(5).unwrap()), + dkg_verification_results: LruCache::new(NonZeroUsize::new(5).unwrap()), }; // Create a DkgBegin message to be handled by the signer. diff --git a/signer/src/wsts_state_machine.rs b/signer/src/wsts_state_machine.rs index d3d53892f..132efb56c 100644 --- a/signer/src/wsts_state_machine.rs +++ b/signer/src/wsts_state_machine.rs @@ -13,6 +13,7 @@ use crate::keys::PublicKeyXOnly; use crate::keys::SignerScriptPubKey as _; use crate::storage; use crate::storage::model; +use crate::storage::model::DkgSharesStatus; use crate::storage::model::SigHash; use hashbrown::HashMap; @@ -40,15 +41,15 @@ use wsts::v2::Aggregator; #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum StateMachineId { /// Identifier for a DKG state machines - Dkg(model::BitcoinBlockHash), + Dkg(model::BitcoinBlockRef), /// Identifier for a Bitcoin signing state machines BitcoinSign(SigHash), /// Identifier for a rotate key verification signing round RotateKey(PublicKeyXOnly, model::BitcoinBlockHash), } -impl From<&model::BitcoinBlockHash> for StateMachineId { - fn from(value: &model::BitcoinBlockHash) -> Self { +impl From<&model::BitcoinBlockRef> for StateMachineId { + fn from(value: &model::BitcoinBlockRef) -> Self { StateMachineId::Dkg(*value) } } @@ -484,6 +485,7 @@ impl SignerStateMachine { pub fn get_encrypted_dkg_shares( &self, rng: &mut Rng, + started_at: &model::BitcoinBlockRef, ) -> Result { let saved_state = self.signer.save(); let aggregate_key = PublicKey::try_from(&saved_state.group_key)?; @@ -523,6 +525,9 @@ impl SignerStateMachine { public_shares, signer_set_public_keys, signature_share_threshold, + dkg_shares_status: DkgSharesStatus::Unverified, + started_at_bitcoin_block_hash: started_at.block_hash, + started_at_bitcoin_block_height: started_at.block_height, }) } } diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index a63124c2d..e3bf6ed8e 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; use std::collections::HashSet; -use std::io::Read; +use std::io::Read as _; use std::time::Duration; use bitcoin::hashes::Hash as _; @@ -10,10 +10,12 @@ use blockstack_lib::clarity::vm::types::PrincipalData; use blockstack_lib::clarity::vm::Value as ClarityValue; use blockstack_lib::codec::StacksMessageCodec; use blockstack_lib::types::chainstate::StacksAddress; +use fake::Faker; use futures::future::join_all; -use futures::StreamExt; -use rand::seq::IteratorRandom; -use rand::seq::SliceRandom; +use futures::StreamExt as _; +use rand::seq::IteratorRandom as _; +use rand::seq::SliceRandom as _; +use signer::storage::model::DkgSharesStatus; use time::OffsetDateTime; use signer::bitcoin::validation::DepositConfirmationStatus; @@ -1321,7 +1323,10 @@ async fn fetching_deposit_request_votes() { num_keys: 7, signatures_required: 4, }; - let shares: EncryptedDkgShares = signer_set_config.fake_with_rng(&mut rng); + let shares = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..signer_set_config.fake_with_rng(&mut rng) + }; store.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -1533,7 +1538,10 @@ async fn fetching_withdrawal_request_votes() { num_keys: 7, signatures_required: 4, }; - let shares: EncryptedDkgShares = signer_set_config.fake_with_rng(&mut rng); + let shares = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..signer_set_config.fake_with_rng(&mut rng) + }; store.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -1778,6 +1786,9 @@ async fn is_signer_script_pub_key_checks_dkg_shares_for_script_pubkeys() { aggregate_key, signer_set_public_keys: vec![fake::Faker.fake_with_rng(&mut rng)], signature_share_threshold: 1, + dkg_shares_status: Faker.fake_with_rng(&mut rng), + started_at_bitcoin_block_hash: fake::Faker.fake_with_rng(&mut rng), + started_at_bitcoin_block_height: fake::Faker.fake_with_rng::(&mut rng) as u64, }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); mem.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -1822,8 +1833,11 @@ async fn get_signers_script_pubkeys_returns_non_empty_vec_old_rows() { , signer_set_public_keys , signature_share_threshold , created_at + , dkg_shares_status + , started_at_bitcoin_block_hash + , started_at_bitcoin_block_height ) - VALUES ($1, $2, $3, $4, $5, $6, $7, CURRENT_TIMESTAMP - INTERVAL '366 DAYS') + VALUES ($1, $2, $3, $4, $5, $6, $7, CURRENT_TIMESTAMP - INTERVAL '366 DAYS', $8, $9, $10) ON CONFLICT DO NOTHING"#, ) .bind(shares.aggregate_key) @@ -1833,6 +1847,9 @@ async fn get_signers_script_pubkeys_returns_non_empty_vec_old_rows() { .bind(&shares.script_pubkey) .bind(&shares.signer_set_public_keys) .bind(shares.signature_share_threshold as i32) + .bind(shares.dkg_shares_status) + .bind(shares.started_at_bitcoin_block_hash) + .bind(shares.started_at_bitcoin_block_height as i64) .execute(db.pool()) .await .unwrap(); @@ -3372,3 +3389,199 @@ async fn compare_in_memory_stacks_chain_tip() { signer::testing::storage::drop_db(pg_store).await; } + +#[tokio::test] +async fn write_and_get_dkg_shares_is_pending() { + let db = testing::storage::new_test_database().await; + + let insert = EncryptedDkgShares { + aggregate_key: fake::Faker.fake(), + tweaked_aggregate_key: fake::Faker.fake(), + encrypted_private_shares: vec![], + script_pubkey: fake::Faker.fake(), + public_shares: vec![], + signer_set_public_keys: vec![], + signature_share_threshold: 1, + dkg_shares_status: DkgSharesStatus::Unverified, + ..fake::Faker.fake() + }; + + db.write_encrypted_dkg_shares(&insert).await.unwrap(); + + let select = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + assert_eq!(insert, select); + + signer::testing::storage::drop_db(db).await; +} + +#[test(tokio::test)] +async fn verify_dkg_shares_succeeds() { + let db = testing::storage::new_test_database().await; + + // We start with a pending entry. + let insert = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..Faker.fake() + }; + + // Write the dkg_shares entry. + db.write_encrypted_dkg_shares(&insert).await.unwrap(); + + // Now to verify the shares. + let result = db.verify_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(result, "verify failed, when it should succeed"); + + // Get the dkg_shares entry. + let select = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + // Assert that the status is now verified and that the block hash and height + // are correct, and that the rest of the fields remain the same. + let compare = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Verified, + ..insert.clone() + }; + assert_eq!(select, compare); + + signer::testing::storage::drop_db(db).await; +} + +#[tokio::test] +async fn revoke_dkg_shares_succeeds() { + let db = testing::storage::new_test_database().await; + + // We start with a pending entry. + let insert = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..Faker.fake() + }; + + // Write the dkg_shares entry. + db.write_encrypted_dkg_shares(&insert).await.unwrap(); + + // Now try to fail the keys. + let result = db.revoke_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(result, "revoke failed, when it should succeed"); + + // Get the dkg_shares entry we just inserted. + let select = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + // Assert that the status is now revoked and that the rest of the fields + // remain the same. + let compare = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Failed, + ..insert.clone() + }; + assert_eq!(select, compare); + + signer::testing::storage::drop_db(db).await; +} + +/// This test checks that DKG shares verification status follows a one-way state transition: +/// +/// 1. Unverified -> Verified: Once shares are verified, they cannot be revoked +/// 2. Unverified -> Failed: Once shares are marked as failed, they cannot be verified +/// +/// The test verifies both transition paths: +/// - Unverified -> Verified -> (attempt revoke, stays Verified) +/// - Unverified -> Failed -> (attempt verify, stays Failed) +#[tokio::test] +async fn verification_status_one_way_street() { + let db = testing::storage::new_test_database().await; + + // We start with a pending entry. + let insert = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..Faker.fake() + }; + + // Write the dkg_shares entry. + db.write_encrypted_dkg_shares(&insert).await.unwrap(); + + // Now try to verify. + let result = db.verify_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(result, "verify failed, when it should succeed"); + + let select1 = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + assert_eq!(select1.dkg_shares_status, DkgSharesStatus::Verified); + + // Now try to revoke. This shouldn't have any effect because we have + // verified the shares already. + let result = db.revoke_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(!result, "revoking succeeded, when it should fail"); + + // Get the dkg_shares entry we just inserted. + let select2 = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + let compare = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Verified, + ..insert + }; + + assert_eq!(select1, select2); + assert_eq!(select1, compare); + + // We start with a pending entry. + let insert = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Unverified, + ..Faker.fake() + }; + + // Write the dkg_shares entry. + db.write_encrypted_dkg_shares(&insert).await.unwrap(); + + // Now try to revoke. + let result = db.revoke_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(result, "revoke failed, when it should succeed"); + + let select1 = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + assert_eq!(select1.dkg_shares_status, DkgSharesStatus::Failed); + + // Now try to verify them. This should be a no-op, since the keys have + // already been marked as failed. + let result = db.verify_dkg_shares(insert.aggregate_key).await.unwrap(); + assert!(!result, "verify succeeded, when it should fail"); + + // Get the dkg_shares entry we just inserted. + let select2 = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + let compare = EncryptedDkgShares { + dkg_shares_status: DkgSharesStatus::Failed, + ..insert + }; + + assert_eq!(select1, select2); + assert_eq!(select1, compare); + + signer::testing::storage::drop_db(db).await; +} diff --git a/signer/tests/integration/request_decider.rs b/signer/tests/integration/request_decider.rs index 8d91a917d..afc0a8296 100644 --- a/signer/tests/integration/request_decider.rs +++ b/signer/tests/integration/request_decider.rs @@ -77,7 +77,7 @@ async fn create_signer_database() -> PgStore { signer::testing::storage::new_test_database().await } -#[tokio::test] +#[test_log::test(tokio::test)] async fn should_store_decisions_for_pending_deposit_requests() { let num_signers = 3; let signing_threshold = 2; diff --git a/signer/tests/integration/rotate_keys.rs b/signer/tests/integration/rotate_keys.rs index 8a1856b1a..eae957faa 100644 --- a/signer/tests/integration/rotate_keys.rs +++ b/signer/tests/integration/rotate_keys.rs @@ -12,6 +12,7 @@ use signer::stacks::contracts::RotateKeysErrorMsg; use signer::stacks::contracts::RotateKeysV1; use signer::stacks::wallet::SignerWallet; use signer::storage::model::BitcoinBlock; +use signer::storage::model::DkgSharesStatus; use signer::storage::model::EncryptedDkgShares; use signer::storage::model::RotateKeysTransaction; use signer::storage::model::StacksPrincipal; @@ -43,6 +44,7 @@ struct TestRotateKeySetup { /// Bitcoin chain tip used when generating current setup pub chain_tip: BitcoinBlock, } + impl TestRotateKeySetup { pub async fn new( db: &PgStore, @@ -107,6 +109,7 @@ impl TestRotateKeySetup { /// Store mocked shares in dkg_shares table. pub async fn store_dkg_shares(&self, db: &PgStore) { let aggregate_key: PublicKey = self.aggregate_key(); + let shares = EncryptedDkgShares { script_pubkey: aggregate_key.signers_script_pubkey().into(), tweaked_aggregate_key: aggregate_key.signers_tweaked_pubkey().unwrap(), @@ -115,6 +118,9 @@ impl TestRotateKeySetup { aggregate_key, signer_set_public_keys: self.signer_keys.clone(), signature_share_threshold: self.signatures_required, + dkg_shares_status: DkgSharesStatus::Verified, + started_at_bitcoin_block_hash: self.chain_tip.block_hash, + started_at_bitcoin_block_height: self.chain_tip.block_height, }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); } @@ -513,3 +519,78 @@ async fn rotate_key_validation_replay() { testing::storage::drop_db(db).await; } + +async fn set_verification_status(db: &PgStore, aggregate_key: PublicKey, status: DkgSharesStatus) { + sqlx::query( + r#" + UPDATE sbtc_signer.dkg_shares + SET dkg_shares_status = $1 + WHERE aggregate_key = $2 + "#, + ) + .bind(status) + .bind(aggregate_key) + .execute(db.pool()) + .await + .unwrap(); +} + +#[tokio::test] +async fn rotate_key_validation_not_verfied() { + // Normal: preamble + let mut db = testing::storage::new_test_database().await; + let mut rng = rand::rngs::StdRng::seed_from_u64(51); + + let test_model_params = testing::storage::model::Params { + num_bitcoin_blocks: 20, + num_stacks_blocks_per_bitcoin_block: 3, + num_deposit_requests_per_block: 0, + num_withdraw_requests_per_block: 0, + num_signers_per_request: 0, + consecutive_blocks: false, + }; + let test_data = TestData::generate(&mut rng, &[], &test_model_params); + test_data.write_to(&mut db).await; + + let ctx = TestContext::builder() + .with_storage(db.clone()) + .with_mocked_clients() + .build(); + + let setup = TestRotateKeySetup::new(&db, 2, 3, &mut rng).await; + + // Normal: we store setup dkg shares + setup.store_dkg_shares(&db).await; + + // Different: mark the shares as failed. + set_verification_status(&db, setup.aggregate_key(), DkgSharesStatus::Failed).await; + + // Normal: we get the rotate key from the setup + let (rotate_key_tx, req_ctx) = make_rotate_key(&setup); + + let validate_future = rotate_key_tx.validate(&ctx, &req_ctx); + match validate_future.await.unwrap_err() { + Error::RotateKeysValidation(ref err) => { + assert_eq!(err.error, RotateKeysErrorMsg::DkgSharesNotVerified) + } + err => panic!("unexpected error during validation {err}"), + } + + // Different: mark the shares as unverified. + set_verification_status(&db, setup.aggregate_key(), DkgSharesStatus::Unverified).await; + + let validate_future = rotate_key_tx.validate(&ctx, &req_ctx); + match validate_future.await.unwrap_err() { + Error::RotateKeysValidation(ref err) => { + assert_eq!(err.error, RotateKeysErrorMsg::DkgSharesNotVerified) + } + err => panic!("unexpected error during validation {err}"), + } + + // Well this is the regular happy path. Everything should validate now. + set_verification_status(&db, setup.aggregate_key(), DkgSharesStatus::Verified).await; + + rotate_key_tx.validate(&ctx, &req_ctx).await.unwrap(); + + testing::storage::drop_db(db).await; +} diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index b9fec89f6..39cbe1071 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -380,6 +380,9 @@ impl TestSweepSetup { aggregate_key, signer_set_public_keys: self.signer_keys.clone(), signature_share_threshold: self.signatures_required, + dkg_shares_status: model::DkgSharesStatus::Verified, + started_at_bitcoin_block_hash: self.deposit_block_hash.into(), + started_at_bitcoin_block_height: 0, }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); } @@ -978,6 +981,9 @@ impl TestSweepSetup2 { aggregate_key, signer_set_public_keys: self.signers.keys.clone(), signature_share_threshold: self.signatures_required, + dkg_shares_status: model::DkgSharesStatus::Verified, + started_at_bitcoin_block_hash: self.deposit_block_hash.into(), + started_at_bitcoin_block_height: 0, }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); } diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index a91114df4..b5795b9bc 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -60,6 +60,7 @@ use signer::stacks::contracts::RotateKeysV1; use signer::stacks::contracts::SmartContract; use signer::storage::model::BitcoinBlockHash; use signer::storage::model::BitcoinTx; +use signer::storage::model::DkgSharesStatus; use signer::storage::postgres::PgStore; use signer::testing::stacks::DUMMY_SORTITION_INFO; use signer::testing::stacks::DUMMY_TENURE_INFO; @@ -846,6 +847,9 @@ async fn run_dkg_from_scratch() { let mut ctx = TestContext::builder() .with_storage(db.clone()) .with_mocked_clients() + .modify_settings(|config| { + config.signer.private_key = kp.secret_key().into(); + }) .build(); // When the signer binary starts up in main(), it sets the current @@ -925,20 +929,10 @@ async fn run_dkg_from_scratch() { } }); - let tx_signer_processes = signers - .iter() - .map(|(context, _, kp, net)| TxSignerEventLoop { - network: net.spawn(), - threshold: context.config().signer.bootstrap_signatures_required as u32, - context: context.clone(), - context_window: 10000, - wsts_state_machines: LruCache::new(NonZeroUsize::new(100).unwrap()), - signer_private_key: kp.secret_key().into(), - rng: rand::rngs::OsRng, - dkg_begin_pause: None, - dkg_verification_state_machines: LruCache::new(NonZeroUsize::new(5).unwrap()), - dkg_verification_results: LruCache::new(NonZeroUsize::new(5).unwrap()), - }); + let tx_signer_processes = signers.iter().map(|(context, _, _, net)| { + TxSignerEventLoop::new(context.clone(), net.spawn(), OsRng) + .expect("failed to create TxSignerEventLoop") + }); // We only proceed with the test after all processes have started, and // we use this counter to notify us when that happens. @@ -1088,6 +1082,11 @@ async fn run_subsequent_dkg() { }) .build(); + // 2. Populate each database with the same data, so that they + // have the same view of the canonical bitcoin blockchain. + // This ensures that they participate in DKG. + data.write_to(&db).await; + // When the signer binary starts up in main(), it sets the current // signer set public keys in the context state using the values in // the bootstrap_signing_set configuration parameter. Later, this @@ -1102,6 +1101,7 @@ async fn run_subsequent_dkg() { db.write_encrypted_dkg_shares(&EncryptedDkgShares { aggregate_key: aggregate_key_1, signer_set_public_keys: signer_set_public_keys.iter().copied().collect(), + dkg_shares_status: DkgSharesStatus::Verified, ..Faker.fake() }) .await @@ -1142,11 +1142,6 @@ async fn run_subsequent_dkg() { }) .await; - // 2. Populate each database with the same data, so that they - // have the same view of the canonical bitcoin blockchain. - // This ensures that they participate in DKG. - data.write_to(&db).await; - let network = network.connect(&ctx); signers.push((ctx, db, kp, network)); @@ -2682,6 +2677,7 @@ async fn test_get_btc_state_with_no_available_sweep_transactions() { let dkg_shares = model::EncryptedDkgShares { aggregate_key: aggregate_key.clone(), script_pubkey: aggregate_key.signers_script_pubkey().into(), + dkg_shares_status: DkgSharesStatus::Unverified, ..Faker.fake_with_rng(&mut rng) }; db.write_encrypted_dkg_shares(&dkg_shares).await.unwrap(); @@ -2816,6 +2812,7 @@ async fn test_get_btc_state_with_available_sweep_transactions_and_rbf() { let dkg_shares = model::EncryptedDkgShares { aggregate_key: aggregate_key.clone(), script_pubkey: aggregate_key.signers_script_pubkey().into(), + dkg_shares_status: DkgSharesStatus::Unverified, ..Faker.fake_with_rng(&mut rng) }; db.write_encrypted_dkg_shares(&dkg_shares).await.unwrap(); diff --git a/signer/tests/integration/transaction_signer.rs b/signer/tests/integration/transaction_signer.rs index 624e4b67f..045b8a1d0 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -292,7 +292,6 @@ async fn new_state_machine_per_valid_sighash() { // creates two bitcoin block. let setup = TestSweepSetup2::new_setup(signers, faucet, &[]); - // Store the necessary data for passing validation setup.store_dkg_shares(&db).await; // Initialize the transaction signer event loop @@ -470,7 +469,7 @@ async fn max_one_state_machine_per_bitcoin_block_hash_for_dkg() { // We should have a state machine associated with the current chain tip // request message that we just received. - let id1 = StateMachineId::from(&chain_tip.block_hash); + let id1 = StateMachineId::from(&chain_tip); let state_machine = tx_signer.wsts_state_machines.get(&id1).unwrap(); assert_eq!(state_machine.dkg_id, dkg_id); assert_eq!(tx_signer.wsts_state_machines.len(), 1); @@ -502,7 +501,7 @@ async fn max_one_state_machine_per_bitcoin_block_hash_for_dkg() { .await .unwrap(); - let id2 = StateMachineId::from(&report.chain_tip.block_hash); + let id2 = StateMachineId::from(&report.chain_tip); let state_machine = tx_signer.wsts_state_machines.get(&id2).unwrap(); assert_eq!(state_machine.dkg_id, dkg_id); assert_eq!(tx_signer.wsts_state_machines.len(), 2); diff --git a/supply-chain/audits.toml b/supply-chain/audits.toml index bec4872dd..f116c5733 100644 --- a/supply-chain/audits.toml +++ b/supply-chain/audits.toml @@ -1,6 +1,17 @@ # cargo-vet audits file +[[audits.bitcoinconsensus]] +who = "cylewitruk " +criteria = "safe-to-deploy" +version = "0.106.0+26.0" +notes = """ +I have reviewed both bitcoinconsensus and sys-libbitcoinconsensus which it uses to build the relevant bitcoin-core sources. +The bitcoin-core sources are indeed from bitcoin-core v26. +Note that this library does use C bindings and thus FFI and unsafe. There are panic-able code paths if used incorrectly. +This library is provided by the rust-bitcoin GH organization which also maintains the bitcoin and secp256k1 crates which we use extensively. +""" + [[audits.p256k1]] who = "cylewitruk " criteria = "safe-to-deploy"