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..6b0a2b3bf --- /dev/null +++ b/signer/migrations/0012__dkg_verification_extensions.sql @@ -0,0 +1,75 @@ +-- Enum table for DKG shares status +CREATE TABLE sbtc_signer.dkg_shares_status ( + -- The id of the status, not auto-incremented as we want to control the values. + id INTEGER PRIMARY KEY, + -- The name of the status. + key TEXT NOT NULL, + -- Brief description of what the status means. + description TEXT NOT NULL +); + +-- Insert the initial entries. +INSERT INTO sbtc_signer.dkg_shares_status (id, key, description) VALUES + (0, 'PENDING', 'DKG round successful, pending verification via signing round'), + (1, 'VERIFIED', 'Successfully verified via signing round'), + (2, 'KEY_REVOKED', 'The DKG key has been revoked and should not be used'); + +-- 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 + -- Contains the current + ADD COLUMN dkg_shares_status_id INTEGER DEFAULT 0 REFERENCES sbtc_signer.dkg_shares_status (id), + ADD COLUMN verified_at_bitcoin_block_hash BYTEA DEFAULT NULL, + ADD COLUMN verified_at_bitcoin_block_height BIGINT DEFAULT NULL, + ADD CONSTRAINT fk_dkg_shares_bitcoin_block_hash + FOREIGN KEY (verified_at_bitcoin_block_hash) + REFERENCES sbtc_signer.bitcoin_blocks (block_hash), + ADD CONSTRAINT chk_verified_at + CHECK ( + (dkg_shares_status_id = 1 AND verified_at_bitcoin_block_hash IS NOT NULL AND verified_at_bitcoin_block_height IS NOT NULL) + OR (dkg_shares_status_id <> 1 AND verified_at_bitcoin_block_hash IS NULL AND verified_at_bitcoin_block_height IS NULL) + ); + +-- Set all of the current `dkg_shares` to `3` (revoked) to start with. Confirmed +-- DKG shares will be updated to `1` (verified) in the next step. +UPDATE sbtc_signer.dkg_shares +SET dkg_shares_status_id = 3; + +-- Update the `dkg_shares` which have been included in a +-- `rotate_keys_transactions` which can also be tied to a bitcoin block to `1` +-- (verified) and set the `verified_at_*` fields to the bitcoin block +-- hash/height corresponding to the block where these were anchored. +-- +-- This update is not fork aware, but at the time of writing there is no forks +-- that should be problematic (i.e. we shouldn't have any rotate-keys events +-- that have been orphaned). +WITH updated_shares AS ( + SELECT + s.aggregate_key, + bb.block_hash AS verified_at_bitcoin_block_hash, + bb.block_height AS verified_at_bitcoin_block_height + FROM sbtc_signer.dkg_shares s + INNER JOIN sbtc_signer.rotate_keys_transactions rkt + ON s.aggregate_key = rkt.aggregate_key + INNER JOIN sbtc_signer.stacks_transactions stx + ON rkt.txid = stx.txid + INNER JOIN sbtc_signer.stacks_blocks sb + ON stx.block_hash = sb.block_hash + INNER JOIN sbtc_signer.bitcoin_blocks bb + ON sb.bitcoin_anchor = bb.block_hash + ORDER BY bb.block_height DESC + LIMIT 1 +) +UPDATE sbtc_signer.dkg_shares +SET + dkg_shares_status_id = 1, + verified_at_bitcoin_block_hash = updated_shares.verified_at_bitcoin_block_hash, + verified_at_bitcoin_block_height = updated_shares.verified_at_bitcoin_block_height +FROM updated_shares +WHERE sbtc_signer.dkg_shares.aggregate_key = updated_shares.aggregate_key; + +-- Make the `dkg_shares_status_id` column `NOT NULL` now that they should all +-- have a value. +ALTER TABLE sbtc_signer.dkg_shares + ALTER COLUMN dkg_shares_status_id SET NOT NULL; \ No newline at end of file diff --git a/signer/src/bitcoin/utxo.rs b/signer/src/bitcoin/utxo.rs index f8a1e549a..513ff326c 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,22 @@ 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 with the bytes [0x01, +/// 0x02, 0x03] as the data and amount equal to the UTXO's value (i.e. the +/// transaction has a zero-fee). +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 +819,94 @@ impl SignatureHashes<'_> { } } +impl UnsignedMockTransaction { + const AMOUNT: u64 = 0; + + /// Construct an unsigned mock transaction. + /// + /// This will use the provided `aggregate_key` and `amount` to + /// construct a [`Transaction`] with a single input and output. + 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 +1562,7 @@ mod tests { use std::str::FromStr; use super::*; + use bitcoin::key::TapTweak; use bitcoin::BlockHash; use bitcoin::CompressedPublicKey; use bitcoin::Txid; @@ -1652,6 +1758,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..75d11dea7 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,7 @@ mod tests { public_shares: Vec::new(), signer_set_public_keys: vec![aggregate_key], signature_share_threshold: 1, + status: DkgSharesStatus::Pending, }; storage.write_encrypted_dkg_shares(&shares).await.unwrap(); diff --git a/signer/src/error.rs b/signer/src/error.rs index cc2d8330d..15a93b502 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -16,15 +16,46 @@ 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), + + /// The DKG shares for the provided aggregate key have not been verified. + #[error("the dkg shares for the provided aggregate key have not been verified: {0}")] + DkgSharesNotVerified(Box), + + /// The DKG shares for the provided aggregate key have been revoked. + #[error("the provided aggregate key has been revoked: {0}")] + DkgSharesRevoked(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, + /// 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 pre-rotate-key frost verification signing round was not reported as /// successful. #[error("rotate-key frost verification signing round not reported as successful")] - FrostVerificationNotSuccessful, + DkgVerificationNotSuccessful, + + /// The rotate-key frost verification signing round failed for the aggregate + /// key. + #[error("rotate-key frost verification signing failed for aggregate key: {0}")] + DkgVerificationFailed(Box), /// No WSTS FROST state machine was found for the given aggregate key. This /// state machine is used during the DKG verification signing round @@ -368,6 +399,24 @@ pub enum Error { #[error("encountered an error while rolling back an sqlx transaction: {0}")] SqlxRollbackTransaction(#[source] sqlx::Error), + /// An error occurred while attempting to deserialize a row into a struct. + #[error("encountered an error while attempting to convert a row to a struct: {0}")] + SqlxFromRow(Cow<'static, str>), + + /// An error occurred while attempting to serialize a struct into a row. + #[error("encountered an error while attempting to convert a struct to a row: {0}")] + SqlxToRow(Cow<'static, str>), + + /// The number of rows updated by a query did not match the expected updated + /// row count. + #[error("expected {expected:?} rows to be updated, but {actual} were updated")] + SqlxUpdatedRowsExpectation { + /// The number of rows that were expected to be updated. + expected: std::ops::Range, + /// The number of rows that were actually updated. + actual: u64, + }, + /// An error when attempting to read a migration script. #[error("failed to read migration script: {0}")] ReadSqlMigration(Cow<'static, str>), diff --git a/signer/src/stacks/contracts.rs b/signer/src/stacks/contracts.rs index cdf8e7487..63a89dfb0 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,28 @@ 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. + match latest_dkg.status { + DkgSharesStatus::Pending => { + return Err(Error::DkgSharesNotVerified(Box::new( + latest_dkg.aggregate_key.into(), + ))); + } + DkgSharesStatus::Revoked => { + return Err(Error::DkgSharesRevoked(Box::new( + latest_dkg.aggregate_key.into(), + ))); + } + DkgSharesStatus::Verified(_) => {} + } + + // 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, diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index af978cd48..644a2a9ff 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,34 @@ 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()) { + shares.status = DkgSharesStatus::Revoked; + Ok(true) + } else { + Ok(false) + } + } + + async fn verify_dkg_shares( + &self, + aggregate_key: X, + bitcoin_block: &model::BitcoinBlockRef, + ) -> Result + where + X: Into + Send, + { + let mut store = self.lock().await; + if let Some((_, shares)) = store.encrypted_dkg_shares.get_mut(&aggregate_key.into()) { + shares.status = DkgSharesStatus::Verified(*bitcoin_block); + Ok(true) + } else { + Ok(false) + } + } } diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index d6ff5b827..e276c2046 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -497,4 +497,27 @@ 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, + bitcoin_block: &model::BitcoinBlockRef, + ) -> impl Future> + Send + where + X: Into + Send; } diff --git a/signer/src/storage/model.rs b/signer/src/storage/model.rs index f7c5a94c9..8d934a79a 100644 --- a/signer/src/storage/model.rs +++ b/signer/src/storage/model.rs @@ -440,7 +440,7 @@ pub struct SweptWithdrawalRequest { /// /// This struct represents the output of a successful run of distributed /// key generation (DKG) that was run by a set of signers. -#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, sqlx::FromRow)] +#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "testing", derive(fake::Dummy))] pub struct EncryptedDkgShares { /// The aggregate key for these shares @@ -461,8 +461,22 @@ pub struct EncryptedDkgShares { /// In WSTS each signer may contribute a fixed portion of a single /// signature. This value specifies the total number of portions /// (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 status: DkgSharesStatus, +} + +impl EncryptedDkgShares { + pub(super) const AGGREGATE_KEY: &str = "aggregate_key"; + pub(super) const TWEAKED_AGGREGATE_KEY: &str = "tweaked_aggregate_key"; + pub(super) const SCRIPT_PUBKEY: &str = "script_pubkey"; + pub(super) const ENCRYPTED_PRIVATE_SHARES: &str = "encrypted_private_shares"; + pub(super) const PUBLIC_SHARES: &str = "public_shares"; + pub(super) const SIGNER_SET_PUBLIC_KEYS: &str = "signer_set_public_keys"; + pub(super) const SIGNATURE_SHARE_THRESHOLD: &str = "signature_share_threshold"; + pub(super) const DKG_SHARES_STATUS_ID: &str = "dkg_shares_status_id"; + pub(super) const VERIFIED_AT_BITCOIN_BLOCK_HASH: &str = "verified_at_bitcoin_block_hash"; + pub(super) const VERIFIED_AT_BITCOIN_BLOCK_HEIGHT: &str = "verified_at_bitcoin_block_height"; } /// Persisted public DKG shares from other signers @@ -543,6 +557,18 @@ impl From for BitArray<[u8; 16]> { } } +/// Possible states for `dkg_shares` entries. +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord, strum::Display)] +#[cfg_attr(feature = "testing", derive(fake::Dummy))] +pub enum DkgSharesStatus { + /// DKG round successful, pending verification via signing round. + Pending, + /// Successfully verified via signing round. + Verified(BitcoinBlockRef), + /// The DKG key has been revoked and should not be used. + Revoked, +} + /// 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 +843,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 +865,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..189019167 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_id + , verified_at_bitcoin_block_hash + , verified_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_id + , verified_at_bitcoin_block_hash + , verified_at_bitcoin_block_height FROM sbtc_signer.dkg_shares ORDER BY created_at DESC LIMIT 1; @@ -2430,6 +2436,21 @@ impl super::DbWrite for PgStore { &self, shares: &model::EncryptedDkgShares, ) -> Result<(), Error> { + let mut verified_at_bitcoin_block_hash: Option = None; + let mut verified_at_bitcoin_block_height: Option = None; + + let dkg_shares_status_id = match shares.status { + model::DkgSharesStatus::Pending => 0, + model::DkgSharesStatus::Verified(block) => { + verified_at_bitcoin_block_hash = Some(block.block_hash); + let block_height = + i64::try_from(block.block_height).map_err(Error::ConversionDatabaseInt)?; + verified_at_bitcoin_block_height = Some(block_height); + 1 + } + model::DkgSharesStatus::Revoked => 2, + }; + sqlx::query( r#" INSERT INTO sbtc_signer.dkg_shares ( @@ -2440,8 +2461,11 @@ impl super::DbWrite for PgStore { , script_pubkey , signer_set_public_keys , signature_share_threshold + , dkg_shares_status_id + , verified_at_bitcoin_block_hash + , verified_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 +2475,9 @@ impl super::DbWrite for PgStore { .bind(&shares.script_pubkey) .bind(&shares.signer_set_public_keys) .bind(i32::from(shares.signature_share_threshold)) + .bind(dkg_shares_status_id) + .bind(verified_at_bitcoin_block_hash) + .bind(verified_at_bitcoin_block_height) .execute(&self.0) .await .map_err(Error::SqlxQuery)?; @@ -2853,6 +2880,99 @@ impl super::DbWrite for PgStore { Ok(()) } + + async fn revoke_dkg_shares(&self, aggregate_key: X) -> Result + where + X: Into + Send, + { + let mut tx = self.0.begin().await.map_err(Error::SqlxBeginTransaction)?; + + let result = sqlx::query( + r#" + UPDATE sbtc_signer.dkg_shares + SET + dkg_shares_status_id = 2 + , verified_at_bitcoin_block_hash = NULL + , verified_at_bitcoin_block_height = NULL + WHERE + substring(aggregate_key FROM 2) = $1; + "#, + ) + .bind(aggregate_key.into()) + .execute(&mut *tx) + .await + .map_err(Error::SqlxQuery)?; + + let updated_rows = result.rows_affected(); + + if updated_rows > 1 { + tracing::warn!( + "expected to update 0..1 rows, but updated {} rows; rolling back transaction", + updated_rows + ); + tx.rollback() + .await + .map_err(Error::SqlxRollbackTransaction)?; + return Err(Error::SqlxUpdatedRowsExpectation { + expected: 0..1, + actual: updated_rows, + }); + } + + tx.commit().await.map_err(Error::SqlxCommitTransaction)?; + + Ok(updated_rows == 1) + } + + async fn verify_dkg_shares( + &self, + aggregate_key: X, + bitcoin_block: &model::BitcoinBlockRef, + ) -> Result + where + X: Into + Send, + { + let mut tx = self.0.begin().await.map_err(Error::SqlxBeginTransaction)?; + + let result = sqlx::query( + r#" + UPDATE sbtc_signer.dkg_shares + SET + dkg_shares_status_id = 1 + , verified_at_bitcoin_block_hash = $2 + , verified_at_bitcoin_block_height = $3 + WHERE + substring(aggregate_key FROM 2) = $1 + AND dkg_shares_status_id = 0; -- only allow verifying pending entries + "#, + ) + .bind(aggregate_key.into()) + .bind(bitcoin_block.block_hash) + .bind(i64::try_from(bitcoin_block.block_height).map_err(Error::ConversionDatabaseInt)?) + .execute(&mut *tx) + .await + .map_err(Error::SqlxQuery)?; + + let updated_rows = result.rows_affected(); + + if updated_rows > 1 { + tracing::warn!( + "expected to update 0..1 rows, but updated {} rows; rolling back transaction", + updated_rows + ); + tx.rollback() + .await + .map_err(Error::SqlxRollbackTransaction)?; + return Err(Error::SqlxUpdatedRowsExpectation { + expected: 0..1, + actual: updated_rows, + }); + } + + tx.commit().await.map_err(Error::SqlxCommitTransaction)?; + + Ok(updated_rows == 1) + } } #[cfg(test)] diff --git a/signer/src/storage/sqlx.rs b/signer/src/storage/sqlx.rs index 9f9ad9335..3dfba53c1 100644 --- a/signer/src/storage/sqlx.rs +++ b/signer/src/storage/sqlx.rs @@ -12,6 +12,9 @@ use bitcoin::hashes::Hash as _; use sqlx::encode::IsNull; use sqlx::error::BoxDynError; use sqlx::postgres::PgArgumentBuffer; +use sqlx::postgres::PgRow; +use sqlx::FromRow; +use sqlx::Row as _; use crate::keys::PublicKey; use crate::keys::PublicKeyXOnly; @@ -24,6 +27,10 @@ use crate::storage::model::StacksBlockHash; use crate::storage::model::StacksPrincipal; use crate::storage::model::StacksTxId; +use super::model::BitcoinBlockRef; +use super::model::DkgSharesStatus; +use super::model::EncryptedDkgShares; + // For the [`ScriptPubKey`] impl<'r> sqlx::Decode<'r, sqlx::Postgres> for ScriptPubKey { @@ -306,3 +313,66 @@ impl sqlx::postgres::PgHasArrayType for SigHash { <[u8; 32] as sqlx::postgres::PgHasArrayType>::array_type_info() } } + +impl<'r> FromRow<'r, PgRow> for EncryptedDkgShares { + fn from_row(row: &'r PgRow) -> Result { + let block_hash: Option> = + row.try_get(EncryptedDkgShares::VERIFIED_AT_BITCOIN_BLOCK_HASH)?; + let block_height: Option = + row.try_get(EncryptedDkgShares::VERIFIED_AT_BITCOIN_BLOCK_HEIGHT)?; + + let verified_at_bitcoin_block = block_hash + .zip(block_height) + .map(|(hash, height)| { + let hash: [u8; 32] = hash + .as_slice() + .try_into() + .map_err(|e| sqlx::Error::Decode(Box::new(e)))?; + Ok::(BitcoinBlockRef { + block_hash: hash.into(), + block_height: height as u64, + }) + }) + .transpose()?; + + let status_id: i32 = row.try_get(EncryptedDkgShares::DKG_SHARES_STATUS_ID)?; + let status = match status_id { + 0 => DkgSharesStatus::Pending, + 1 => { + let verified_at_bitcoin_block = verified_at_bitcoin_block.ok_or_else(|| { + let message = format!( + "{} is '1' but {} or {} is NULL", + EncryptedDkgShares::DKG_SHARES_STATUS_ID, + EncryptedDkgShares::VERIFIED_AT_BITCOIN_BLOCK_HASH, + EncryptedDkgShares::VERIFIED_AT_BITCOIN_BLOCK_HEIGHT + ); + sqlx::Error::Decode(Box::new(crate::error::Error::SqlxFromRow(message.into()))) + })?; + DkgSharesStatus::Verified(verified_at_bitcoin_block) + } + 2 => DkgSharesStatus::Revoked, + _ => { + let message = format!( + "{} is not in [0, 1, 2]", + EncryptedDkgShares::DKG_SHARES_STATUS_ID + ); + return Err(sqlx::Error::Decode(Box::new( + crate::error::Error::SqlxFromRow(message.into()), + ))); + } + }; + + Ok(Self { + aggregate_key: row.try_get(EncryptedDkgShares::AGGREGATE_KEY)?, + tweaked_aggregate_key: row.try_get(EncryptedDkgShares::TWEAKED_AGGREGATE_KEY)?, + script_pubkey: row.try_get(EncryptedDkgShares::SCRIPT_PUBKEY)?, + encrypted_private_shares: row.try_get(EncryptedDkgShares::ENCRYPTED_PRIVATE_SHARES)?, + public_shares: row.try_get(EncryptedDkgShares::PUBLIC_SHARES)?, + signer_set_public_keys: row.try_get(EncryptedDkgShares::SIGNER_SET_PUBLIC_KEYS)?, + signature_share_threshold: row + .try_get::(EncryptedDkgShares::SIGNATURE_SHARE_THRESHOLD)? + as u16, + status, + }) + } +} diff --git a/signer/src/testing/dummy.rs b/signer/src/testing/dummy.rs index 7b805ed83..5206216dc 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,7 @@ 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, + status, } } @@ -441,6 +444,7 @@ impl fake::Dummy for EncryptedDkgShares { public_shares: Vec::new(), signer_set_public_keys, signature_share_threshold: config.signatures_required, + status: DkgSharesStatus::Verified(Faker.fake_with_rng(rng)), } } } diff --git a/signer/src/testing/request_decider.rs b/signer/src/testing/request_decider.rs index c6fa4b07c..2ec3db597 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(test_data.bitcoin_blocks[0].clone().into()), ) .await; @@ -342,6 +344,7 @@ where &handle.context.get_storage_mut(), group_key, signer_set.clone(), + DkgSharesStatus::Verified(test_data.bitcoin_blocks[0].clone().into()), ) .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/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 8f3f795d7..83dd5515a 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,13 +377,23 @@ where return Ok(()); } - let wallet = self.get_signer_wallet(bitcoin_chain_tip).await?; + 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 // with the registry smart contract; fallbacks to `aggregate_key` if it's // 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 +705,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 +815,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 +1012,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 +1028,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 +1056,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 +1069,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 +1136,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 +1144,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 +1157,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 +1242,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 +1277,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 +1294,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..db1557880 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) => { @@ -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,16 +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?; - self.process_dkg_verification_message( - new_key, + + 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( + &chain_tip.block_hash, state_machine_id, &msg.inner, ) @@ -794,7 +789,7 @@ where } WstsMessageId::Sweep(txid) => { span.record("txid", txid.to_string()); - tracing::info!( + tracing::debug!( signature_type = ?request.signature_type, "processing message" ); @@ -819,22 +814,31 @@ 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) - .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + 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( + &chain_tip.block_hash, + state_machine_id, + &msg.inner, + ) + .await?; state_machine_id } }; @@ -856,13 +860,36 @@ where return Ok(()); }; - let key = key.into(); + let new_key: PublicKeyXOnly = key.into(); - let state_machine_id = self - .ensure_dkg_verification_state_machine(chain_tip.block_hash, key) - .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + Self::validate_dkg_verification_message( + &self.context.get_storage(), + &new_key, + Some(&request.message), + ) + .await?; + + if request.message.len() != 32 { + tracing::warn!("🔐 data received for DKG verification signing is not 32 bytes"); + return Err(Error::InvalidSigningOperation); + } + + 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( + &chain_tip.block_hash, + state_machine_id, + &msg.inner, + ) + .await?; } WstsNetMessage::SignatureShareResponse(request) => { span.record(WSTS_DKG_ID, request.dkg_id); @@ -874,13 +901,25 @@ 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) - .await?; - self.process_dkg_verification_message(key, state_machine_id, &msg.inner) + 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.handle_dkg_verification_message( + &chain_tip.block_hash, + state_machine_id, + &msg.inner, + ) + .await?; } } @@ -896,33 +935,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); } @@ -983,7 +1026,7 @@ where let encrypted_dkg_shares = state_machine.get_encrypted_dkg_shares(&mut self.rng)?; - tracing::debug!("storing DKG shares"); + tracing::debug!("🔐 storing DKG shares"); self.context .get_storage_mut() .write_encrypted_dkg_shares(&encrypted_dkg_shares) @@ -992,8 +1035,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 +1060,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 +1082,120 @@ 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 = UnsignedMockTransaction::new(aggregate_key.into()); + let mock_tx = self + .dkg_verification_results + .get_or_insert(state_machine_id, || mock_tx); + + 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, + bitcoin_chain_tip: &model::BitcoinBlockHash, 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!("🔐 \x1b[1;32msignature verification successful\x1b[0m"); + + let storage = self.context.get_storage_mut(); + let bitcoin_chain_tip_block = + storage + .get_bitcoin_block(bitcoin_chain_tip) + .await? + .ok_or(Error::MissingBitcoinBlock(*bitcoin_chain_tip))?; + + self.context + .get_storage_mut() + .verify_dkg_shares(aggregate_key, &bitcoin_chain_tip_block.into()) + .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(Box::new(aggregate_key))); } None => {} result => { - tracing::warn!(?result, "unexpected FROST coordinator result"); + tracing::warn!( + ?result, + "🔐 unexpected result received from the FROST coordinator" + ); } } @@ -1143,6 +1215,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 +1234,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 +1617,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..f016bc17e 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; @@ -523,6 +524,7 @@ impl SignerStateMachine { public_shares, signer_set_public_keys, signature_share_threshold, + status: DkgSharesStatus::Pending, }) } } diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index a63124c2d..7583180ab 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -10,10 +10,13 @@ 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 signer::keys::PublicKeyXOnly; +use signer::storage::model::DkgSharesStatus; use time::OffsetDateTime; use signer::bitcoin::validation::DepositConfirmationStatus; @@ -1321,7 +1324,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 { + status: DkgSharesStatus::Pending, + ..signer_set_config.fake_with_rng(&mut rng) + }; store.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -1533,7 +1539,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 { + status: DkgSharesStatus::Pending, + ..signer_set_config.fake_with_rng(&mut rng) + }; store.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -1778,6 +1787,7 @@ 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, + status: Faker.fake_with_rng(&mut rng), }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); mem.write_encrypted_dkg_shares(&shares).await.unwrap(); @@ -3372,3 +3382,259 @@ 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, + status: DkgSharesStatus::Pending, + }; + + 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 { + status: DkgSharesStatus::Pending, + ..Faker.fake() + }; + + // We first need to write a bitcoin block to the database due to the FK + // when marking dkg shares as verified. + let block: BitcoinBlock = fake::Faker.fake(); + db.write_bitcoin_block(&block) + .await + .expect("failed to write block"); + + // 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, &block.clone().into()) + .await + .expect("failed to mark shares as verified"); + + assert!(result, "verify_dkg_shares returned false"); + + // 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 { + status: DkgSharesStatus::Verified(block.into()), + ..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 { + status: DkgSharesStatus::Pending, + ..Faker.fake() + }; + + // We first need to write a bitcoin block to the database due to the FK + // when marking dkg shares as verified. + let block: BitcoinBlock = fake::Faker.fake(); + db.write_bitcoin_block(&block) + .await + .expect("failed to write block"); + + // 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 + .expect("failed to mark shares as revoked"); + + // This would only return false if the status was not pending. + assert!(result, "revoke_dkg_shares returned false"); + + // 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 { + status: DkgSharesStatus::Revoked, + ..insert.clone() + }; + assert_eq!(select, compare); + + signer::testing::storage::drop_db(db).await; +} + +#[tokio::test] +async fn revoke_verified_dkg_shares_succeeds() { + let db = testing::storage::new_test_database().await; + + // We start with a pending entry. + let insert = EncryptedDkgShares { + status: DkgSharesStatus::Pending, + ..Faker.fake() + }; + + // We first need to write a bitcoin block to the database due to the FK + // when marking dkg shares as verified. + let block: BitcoinBlock = fake::Faker.fake(); + db.write_bitcoin_block(&block) + .await + .expect("failed to write block"); + + // 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, &block.clone().into()) + .await + .expect("failed to mark shares as verified"); + + // This would only return false if the status was not pending. We don't do + // any further checks here because that's handled in + // `test_write_and_verify_encrypted_dkg_shares`. + assert!(result, "verify_dkg_shares returned false"); + + // Now try to revoke. + let result = db + .revoke_dkg_shares(insert.aggregate_key) + .await + .expect("failed to mark shares as revoked"); + + // This would only return false if the dkg_shares entry wasn't found. We + // don't do any further checks here because that's handled in + // `test_write_and_revoke_encrypted_dkg_shares`. + assert!(result, "revoke_dkg_shares returned false"); + + // 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 { + status: DkgSharesStatus::Revoked, + ..insert.clone() + }; + assert_eq!(select, compare); + + // Now we'll check the database directly that the verified_at* fields have + // been nulled. + let aggregate_key: PublicKeyXOnly = insert.aggregate_key.into(); + let (block_hash, block_height): (Option>, Option) = sqlx::query_as( + r#" + SELECT verified_at_bitcoin_block_hash, verified_at_bitcoin_block_height + FROM sbtc_signer.dkg_shares + WHERE substring(aggregate_key FROM 2) = $1 + "#, + ) + .bind(aggregate_key) + .fetch_one(db.pool()) + .await + .expect("failed to query database"); + + assert!(block_hash.is_none()); + assert!(block_height.is_none()); + + signer::testing::storage::drop_db(db).await; +} + +#[tokio::test] +async fn verify_revoked_dkg_shares_fails() { + let db = testing::storage::new_test_database().await; + + // We start with a pending entry. + let insert = EncryptedDkgShares { + status: DkgSharesStatus::Pending, + ..Faker.fake() + }; + + // We first need to write a bitcoin block to the database due to the FK + // when marking dkg shares as verified. + let block: BitcoinBlock = fake::Faker.fake(); + db.write_bitcoin_block(&block) + .await + .expect("failed to write block"); + + // 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 + .expect("failed to mark shares as revoked"); + + // This would only return false if the dkg_shares entry wasn't found. We + // don't do any further checks here because that's handled in + // `test_write_and_revoke_encrypted_dkg_shares`. + assert!(result, "revoke_dkg_shares returned false"); + + // Now try to verify. This should fail. + let result = db + .verify_dkg_shares(insert.aggregate_key, &block.clone().into()) + .await + .expect("failed to mark shares as verified"); + + // This should only return true if the status is pending, which it is now + // revoked. + assert!(!result, "verify_dkg_shares returned true"); + + // Get the dkg_shares entry. It should be revoked. + let select = db + .get_encrypted_dkg_shares(insert.aggregate_key) + .await + .expect("database error") + .expect("no shares found"); + + // Assert that the status is still revoked and that the rest of the fields + // remain the same. + let compare = EncryptedDkgShares { + status: DkgSharesStatus::Revoked, + ..insert.clone() + }; + assert_eq!(select, compare); +} 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..e3e99d9b3 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; @@ -107,6 +108,8 @@ 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 verified_at_block = self.chain_tip.clone().into(); + let shares = EncryptedDkgShares { script_pubkey: aggregate_key.signers_script_pubkey().into(), tweaked_aggregate_key: aggregate_key.signers_tweaked_pubkey().unwrap(), @@ -115,6 +118,7 @@ impl TestRotateKeySetup { aggregate_key, signer_set_public_keys: self.signer_keys.clone(), signature_share_threshold: self.signatures_required, + status: DkgSharesStatus::Verified(verified_at_block), }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); } diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index b9fec89f6..afe3ecce7 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -36,6 +36,7 @@ use signer::keys::PublicKey; use signer::keys::SignerScriptPubKey; use signer::storage::model; use signer::storage::model::BitcoinBlockHash; +use signer::storage::model::BitcoinBlockRef; use signer::storage::model::BitcoinTxRef; use signer::storage::model::EncryptedDkgShares; use signer::storage::model::QualifiedRequestId; @@ -372,6 +373,10 @@ impl TestSweepSetup { /// associated with the signers aggregate key. pub async fn store_dkg_shares(&self, db: &PgStore) { let aggregate_key: PublicKey = self.aggregated_signer.keypair.public_key().into(); + let verified_at = BitcoinBlockRef { + block_hash: self.deposit_block_hash.into(), + block_height: 0, + }; let shares = EncryptedDkgShares { script_pubkey: aggregate_key.signers_script_pubkey().into(), tweaked_aggregate_key: aggregate_key.signers_tweaked_pubkey().unwrap(), @@ -380,6 +385,7 @@ impl TestSweepSetup { aggregate_key, signer_set_public_keys: self.signer_keys.clone(), signature_share_threshold: self.signatures_required, + status: model::DkgSharesStatus::Verified(verified_at), }; db.write_encrypted_dkg_shares(&shares).await.unwrap(); } @@ -970,6 +976,11 @@ impl TestSweepSetup2 { .expect("failed to encrypt"); let public_shares: BTreeMap = BTreeMap::new(); + let verified_at = BitcoinBlockRef { + block_hash: self.deposit_block_hash.into(), + block_height: 0, + }; + let shares = EncryptedDkgShares { script_pubkey: aggregate_key.signers_script_pubkey().into(), tweaked_aggregate_key: aggregate_key.signers_tweaked_pubkey().unwrap(), @@ -978,6 +989,7 @@ impl TestSweepSetup2 { aggregate_key, signer_set_public_keys: self.signers.keys.clone(), signature_share_threshold: self.signatures_required, + status: model::DkgSharesStatus::Verified(verified_at), }; 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..407145061 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(), + status: DkgSharesStatus::Verified(data.bitcoin_blocks[0].clone().into()), ..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(), + status: DkgSharesStatus::Pending, ..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(), + status: DkgSharesStatus::Pending, ..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..ced999036 100644 --- a/signer/tests/integration/transaction_signer.rs +++ b/signer/tests/integration/transaction_signer.rs @@ -293,6 +293,11 @@ async fn new_state_machine_per_valid_sighash() { let setup = TestSweepSetup2::new_setup(signers, faucet, &[]); // Store the necessary data for passing validation + let dummy_block = model::BitcoinBlock { + block_hash: setup.deposit_block_hash.into(), + ..Faker.fake_with_rng(&mut rng) + }; + db.write_bitcoin_block(&dummy_block).await.unwrap(); setup.store_dkg_shares(&db).await; // Initialize the transaction signer event loop 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"