From 5ca1b015a86fdca77a1546f8f92c1416d2016515 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 15:32:37 -0500 Subject: [PATCH 01/16] Add new query for figuring out whether a withdrawal request is safe to reject. --- signer/src/storage/in_memory.rs | 9 ++++++ signer/src/storage/mod.rs | 10 +++++++ signer/src/storage/postgres.rs | 50 ++++++++++++++++++++++++++++----- 3 files changed, 62 insertions(+), 7 deletions(-) diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index 438def183..70a1650df 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -822,6 +822,15 @@ impl super::DbRead for SharedStore { unimplemented!() } + async fn is_withdrawal_inactive( + &self, + _: &model::BitcoinBlockRef, + _: &model::QualifiedRequestId, + _: u64, + ) -> Result { + unimplemented!() + } + async fn get_bitcoin_tx( &self, txid: &model::BitcoinTxId, diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index 2fd7de419..3f730ce22 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -336,6 +336,16 @@ pub trait DbRead { bitcoin_chain_tip: &model::BitcoinBlockHash, ) -> impl Future> + Send; + /// Returns whether we can consider the withdrawal inactive. This means + /// that there is no risk of the withdrawal being confirmed from a fork + /// of blocks less than `min_wait_blocks`. + fn is_withdrawal_inactive( + &self, + chain_tip: &model::BitcoinBlockRef, + id: &model::QualifiedRequestId, + min_wait_blocks: u64, + ) -> impl Future> + Send; + /// Fetch the bitcoin transaction that is included in the block /// identified by the block hash. fn get_bitcoin_tx( diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 5632bea89..0b7f18c74 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -171,6 +171,8 @@ struct PgSignerUtxo { #[sqlx(try_from = "i64")] amount: u64, aggregate_key: PublicKey, + #[sqlx(try_from = "i64")] + block_height: u64, } impl From for SignerUtxo { @@ -401,11 +403,13 @@ impl PgStore { chain_tip: &model::BitcoinBlockHash, output_type: model::TxOutputType, min_block_height: i64, - ) -> Result, Error> { - let pg_utxo = sqlx::query_as::<_, PgSignerUtxo>( + ) -> Result, Error> { + sqlx::query_as::<_, PgSignerUtxo>( r#" WITH bitcoin_blockchain AS ( - SELECT block_hash + SELECT + block_hash + , block_height FROM bitcoin_blockchain_until($1, $2) ), confirmed_sweeps AS ( @@ -422,6 +426,7 @@ impl PgStore { , bo.output_index , bo.amount , ds.aggregate_key + , bb.block_height FROM sbtc_signer.bitcoin_tx_outputs AS bo JOIN sbtc_signer.bitcoin_transactions AS bt USING (txid) JOIN bitcoin_blockchain AS bb USING (block_hash) @@ -440,9 +445,7 @@ impl PgStore { .bind(output_type) .fetch_optional(&self.0) .await - .map_err(Error::SqlxQuery)?; - - Ok(pg_utxo.map(SignerUtxo::from)) + .map_err(Error::SqlxQuery) } /// Return the height of the earliest block in which a donation UTXO @@ -480,6 +483,7 @@ impl PgStore { let output_type = model::TxOutputType::Donation; self.get_utxo(chain_tip, output_type, min_block_height) .await + .map(|pg_utxo| pg_utxo.map(SignerUtxo::from)) } /// Return a block height that is less than or equal to the block that /// confirms the signers' UTXO. @@ -1980,7 +1984,7 @@ impl super::DbRead for PgStore { // happens we try searching for a donation. let output_type = model::TxOutputType::SignersOutput; let fut = self.get_utxo(chain_tip, output_type, min_block_height); - match fut.await? { + match fut.await?.map(SignerUtxo::from) { res @ Some(_) => Ok(res), None => self.get_donation_utxo(chain_tip).await, } @@ -2121,6 +2125,38 @@ impl super::DbRead for PgStore { .map_err(Error::SqlxQuery) } + async fn is_withdrawal_inactive( + &self, + chain_tip: &model::BitcoinBlockRef, + id: &model::QualifiedRequestId, + min_wait_blocks: u64, + ) -> Result { + let min_block_height = sqlx::query_scalar::<_, i64>( + r#" + SELECT MAX(bb.block_height) + 1 + FROM sbtc_signer.bitcoin_withdrawals_outputs AS bwo + JOIN sbtc_signer.bitcoin_blocks AS bb + ON bb.block_hash = bwo.bitcoin_chain_tip + WHERE bwo.request_id = $1 + AND bwo.stacks_block_hash = $2 + "#, + ) + .bind(i64::try_from(id.request_id).map_err(Error::ConversionDatabaseInt)?) + .bind(id.block_hash) + .fetch_one(&self.0) + .await + .map_err(Error::SqlxQuery)?; + + let output_type = model::TxOutputType::SignersOutput; + let chain_tip_hash = &chain_tip.block_hash; + let signer_utxo_fut = self.get_utxo(chain_tip_hash, output_type, min_block_height); + let Some(signer_utxo) = signer_utxo_fut.await? else { + return Ok(true); + }; + + Ok(signer_utxo.block_height + min_wait_blocks < chain_tip.block_height) + } + async fn get_bitcoin_tx( &self, txid: &model::BitcoinTxId, From 50cae48cdd4219e0eae1bf599b1d2fa0d6493936 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 16:15:38 -0500 Subject: [PATCH 02/16] Change the meaning of the new function --- signer/src/storage/in_memory.rs | 4 ++-- signer/src/storage/mod.rs | 13 +++++++------ signer/src/storage/postgres.rs | 12 ++++++------ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/signer/src/storage/in_memory.rs b/signer/src/storage/in_memory.rs index 70a1650df..748cd6a71 100644 --- a/signer/src/storage/in_memory.rs +++ b/signer/src/storage/in_memory.rs @@ -822,10 +822,10 @@ impl super::DbRead for SharedStore { unimplemented!() } - async fn is_withdrawal_inactive( + async fn is_withdrawal_active( &self, - _: &model::BitcoinBlockRef, _: &model::QualifiedRequestId, + _: &model::BitcoinBlockRef, _: u64, ) -> Result { unimplemented!() diff --git a/signer/src/storage/mod.rs b/signer/src/storage/mod.rs index 3f730ce22..aca29ff7f 100644 --- a/signer/src/storage/mod.rs +++ b/signer/src/storage/mod.rs @@ -336,14 +336,15 @@ pub trait DbRead { bitcoin_chain_tip: &model::BitcoinBlockHash, ) -> impl Future> + Send; - /// Returns whether we can consider the withdrawal inactive. This means - /// that there is no risk of the withdrawal being confirmed from a fork - /// of blocks less than `min_wait_blocks`. - fn is_withdrawal_inactive( + /// Returns whether we should consider the withdrawal active. A + /// withdrawal request is considered active if there is a reasonable + /// risk of the withdrawal being confirmed from a fork of blocks less + /// than `min_confirmations`. + fn is_withdrawal_active( &self, - chain_tip: &model::BitcoinBlockRef, id: &model::QualifiedRequestId, - min_wait_blocks: u64, + bitcoin_chain_tip: &model::BitcoinBlockRef, + min_confirmations: u64, ) -> impl Future> + Send; /// Fetch the bitcoin transaction that is included in the block diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 0b7f18c74..873657061 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -2125,11 +2125,11 @@ impl super::DbRead for PgStore { .map_err(Error::SqlxQuery) } - async fn is_withdrawal_inactive( + async fn is_withdrawal_active( &self, - chain_tip: &model::BitcoinBlockRef, id: &model::QualifiedRequestId, - min_wait_blocks: u64, + bitcoin_chain_tip: &model::BitcoinBlockRef, + min_confirmations: u64, ) -> Result { let min_block_height = sqlx::query_scalar::<_, i64>( r#" @@ -2148,13 +2148,13 @@ impl super::DbRead for PgStore { .map_err(Error::SqlxQuery)?; let output_type = model::TxOutputType::SignersOutput; - let chain_tip_hash = &chain_tip.block_hash; + let chain_tip_hash = &bitcoin_chain_tip.block_hash; let signer_utxo_fut = self.get_utxo(chain_tip_hash, output_type, min_block_height); let Some(signer_utxo) = signer_utxo_fut.await? else { - return Ok(true); + return Ok(false); }; - Ok(signer_utxo.block_height + min_wait_blocks < chain_tip.block_height) + Ok(signer_utxo.block_height + min_confirmations >= bitcoin_chain_tip.block_height) } async fn get_bitcoin_tx( From 8e3110141dd56576e9724756fa6c7fe88ad2af01 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 16:16:40 -0500 Subject: [PATCH 03/16] Use the new function in validation and the coordinator --- signer/src/stacks/contracts.rs | 18 ++++++++++++++++++ signer/src/transaction_coordinator.rs | 15 ++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/signer/src/stacks/contracts.rs b/signer/src/stacks/contracts.rs index 41d368f93..f2d74ff52 100644 --- a/signer/src/stacks/contracts.rs +++ b/signer/src/stacks/contracts.rs @@ -59,6 +59,7 @@ use crate::storage::model::ToLittleEndianOrder as _; use crate::storage::DbRead; use crate::DEPOSIT_DUST_LIMIT; use crate::WITHDRAWAL_BLOCKS_EXPIRY; +use crate::WITHDRAWAL_MIN_CONFIRMATIONS; use super::api::StacksInteract; @@ -1029,6 +1030,10 @@ pub enum WithdrawalRejectErrorMsg { /// been either accepted or rejected already. #[error("the smart contract has been updated to indicate that the request has been completed")] RequestCompleted, + /// A withdrawal request is still active if it's reasonably possible + /// for the request to be fulfilled by a sweep transaction on bitcoin. + #[error("the withdrawal request is still active, we cannot reject it yet")] + RequestStillActive, /// Withdrawal request fulfilled #[error("Withdrawal request fulfilled")] RequestFulfilled, @@ -1109,6 +1114,9 @@ impl AsContractCall for RejectWithdrawalV1 { /// 6. Whether the withdrawal request has expired. Fail if it hasn't. /// 7. Whether the withdrawal request is being serviced by a sweep /// transaction that is in the mempool. + /// 8. Whether we need to worry about forks causing the withdrawal to + /// be confirmed by a sweep that was broadcst changing the status of + /// the request from rejected to accepted. async fn validate(&self, ctx: &C, req_ctx: &ReqContext) -> Result<(), Error> where C: Context + Send + Sync, @@ -1191,6 +1199,16 @@ impl AsContractCall for RejectWithdrawalV1 { return Err(WithdrawalRejectErrorMsg::RequestBeingFulfilled.into_error(req_ctx, self)); } + // 8. Check whether the withdrawal request is still active, as in + // it could still be fulfilled. + let withdrawal_is_active = db + .is_withdrawal_active(&self.id, &req_ctx.chain_tip, WITHDRAWAL_MIN_CONFIRMATIONS) + .await?; + + if withdrawal_is_active { + return Err(WithdrawalRejectErrorMsg::RequestStillActive.into_error(req_ctx, self)); + } + Ok(()) } } diff --git a/signer/src/transaction_coordinator.rs b/signer/src/transaction_coordinator.rs index 5df96c97c..b1f86bf4a 100644 --- a/signer/src/transaction_coordinator.rs +++ b/signer/src/transaction_coordinator.rs @@ -66,6 +66,7 @@ use crate::storage::DbRead as _; use crate::wsts_state_machine::FireCoordinator; use crate::wsts_state_machine::FrostCoordinator; use crate::wsts_state_machine::WstsCoordinator; +use crate::WITHDRAWAL_MIN_CONFIRMATIONS; use bitcoin::hashes::Hash as _; use wsts::net::SignatureType; @@ -781,13 +782,25 @@ where // the given withdrawal has been included in a sweep transaction // that could have been submitted. With this check we are more // confident that it is safe to reject the withdrawal. + let qualified_id = request.qualified_id(); let withdrawal_inflight = db - .is_withdrawal_inflight(&request.qualified_id(), &chain_tip.block_hash) + .is_withdrawal_inflight(&qualified_id, &chain_tip.block_hash) .await?; if withdrawal_inflight { return Ok(()); } + // The `DbRead::is_withdrawal_inflight` function considers whether + // we need to worry about a fork making a sweep fulfilling + // withdrawal active in the mempool. + let withdrawal_is_active = db + .is_withdrawal_active(&qualified_id, chain_tip, WITHDRAWAL_MIN_CONFIRMATIONS) + .await?; + + if withdrawal_is_active { + return Ok(()); + } + let sign_request_fut = self.construct_withdrawal_reject_stacks_sign_request( &request, bitcoin_aggregate_key, From 9845f43de70a2ea0537d71249b8ab1a3ed876af1 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 17:34:20 -0500 Subject: [PATCH 04/16] testing WIP --- signer/src/storage/postgres.rs | 4 +- signer/tests/integration/setup.rs | 26 +++- signer/tests/integration/withdrawal_reject.rs | 132 ++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 873657061..670514de6 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -2150,8 +2150,10 @@ impl super::DbRead for PgStore { let output_type = model::TxOutputType::SignersOutput; let chain_tip_hash = &bitcoin_chain_tip.block_hash; let signer_utxo_fut = self.get_utxo(chain_tip_hash, output_type, min_block_height); + // If this returns None, then the sweep itself could be in the + // mempool. If that's the case then this is definitely active. let Some(signer_utxo) = signer_utxo_fut.await? else { - return Ok(false); + return Ok(true); }; Ok(signer_utxo.block_height + min_confirmations >= bitcoin_chain_tip.block_height) diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index 29208e2d3..f443333e7 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -25,6 +25,7 @@ use signer::bitcoin::rpc::BitcoinCoreClient; use signer::bitcoin::rpc::BitcoinTxInfo; use signer::bitcoin::rpc::GetTxResponse; use signer::bitcoin::utxo; +use signer::bitcoin::utxo::Fees; use signer::bitcoin::utxo::SbtcRequests; use signer::bitcoin::utxo::SignerBtcState; use signer::bitcoin::utxo::SignerUtxo; @@ -854,6 +855,29 @@ impl TestSweepSetup2 { let aggregated_signer = &self.signers.signer; let signer_utxo = aggregated_signer.get_utxos(rpc, None).pop().unwrap(); + // Well we want a BitcoinCoreClient, so we create one using the + // context. Not, the best thing to do, sorry. TODO: pass in a + // bitcoin core client object. + let ctx = TestContext::builder() + .with_first_bitcoin_core_client() + .with_in_memory_storage() + .with_mocked_emily_client() + .with_mocked_stacks_client() + .build(); + + let outpoint = OutPoint::new(signer_utxo.txid, signer_utxo.vout); + let btc = ctx.bitcoin_client; + let txids = btc.get_tx_spending_prevout(&outpoint).unwrap(); + + let last_fees = txids + .iter() + .filter_map(|txid| btc.get_mempool_entry(txid).unwrap()) + .map(|entry| Fees { + total: entry.vsize, + rate: entry.fees.base.to_sat() as f64 / entry.vsize as f64, + }) + .max_by(|fee1, fee2| fee1.rate.total_cmp(&fee2.rate)); + let withdrawals = self .withdrawals .iter() @@ -875,7 +899,7 @@ impl TestSweepSetup2 { }, fee_rate: 10.0, public_key: aggregated_signer.keypair.x_only_public_key().0, - last_fees: None, + last_fees, magic_bytes: [b'T', b'3'], }, accept_threshold: 4, diff --git a/signer/tests/integration/withdrawal_reject.rs b/signer/tests/integration/withdrawal_reject.rs index f8e1d3321..31aba7e34 100644 --- a/signer/tests/integration/withdrawal_reject.rs +++ b/signer/tests/integration/withdrawal_reject.rs @@ -19,6 +19,7 @@ use fake::Fake; use rand::SeedableRng; use signer::testing::context::*; use signer::WITHDRAWAL_BLOCKS_EXPIRY; +use signer::WITHDRAWAL_MIN_CONFIRMATIONS; use crate::setup::fetch_canonical_bitcoin_blockchain; use crate::setup::set_withdrawal_completed; @@ -603,3 +604,134 @@ async fn reject_withdrawal_validation_request_being_fulfilled() { testing::storage::drop_db(db).await; } + +/// For this test we check that the `RejectWithdrawalV1::validate` function +/// returns a withdrawal validation error with a RequestStillActive +/// message when the database indicates that it is possible that the +/// withdrawal request is being fulfilled by a sweep transaction in the +/// mempool. +#[tokio::test] +async fn reject_withdrawal_validation_request_still_active() { + // Normal: this generates the blockchain as well as a transaction + // sweeping out the funds for a withdrawal request. This is just setup + // and should be essentially the same between tests. + let db = testing::storage::new_test_database().await; + let mut rng = rand::rngs::StdRng::seed_from_u64(51); + let (rpc, faucet) = regtest::initialize_blockchain(); + + let amount = 1_000_000; + let signers = TestSignerSet::new(&mut rng); + let amounts = [ + SweepAmounts { + amount, + max_fee: amount / 2, + is_deposit: true, + }, + SweepAmounts { + amount, + max_fee: amount / 2, + is_deposit: false, + }, + ]; + + let mut setup = TestSweepSetup2::new_setup(signers, faucet, &amounts); + + let mut ctx = TestContext::builder() + .with_storage(db.clone()) + .with_first_bitcoin_core_client() + .with_mocked_stacks_client() + .with_mocked_emily_client() + .build(); + + // Normal: the request has not been marked as completed in the smart + // contract. + set_withdrawal_incomplete(&mut ctx).await; + + // Normal: the signer follows the bitcoin blockchain and event observer + // should be getting new block events from bitcoin-core. We haven't + // hooked up our block observer, so we need to manually update the + // database with new bitcoin block headers. + fetch_canonical_bitcoin_blockchain(&db, rpc).await; + + // Different: we need to store a row in the dkg_shares table so that we + // have a record of the scriptPubKey that the signers control. We need + // this so that the donation gets picked up correctly below. + setup.store_dkg_shares(&db).await; + + // Normal: The signers normally have a UTXO, so we add one here too. It + // is necessary when checking for whether the withdrawal being + // fulfilled by a sweep transaction that is in the mempool. + setup.store_donation(&db).await; + + // Different: We submit a sweep transaction into the mempool so that + // the TestSweepSetup2 struct has the sweep_tx_info set. We also need + // to submit the transaction in order for + // `TestSweepSetup2::store_bitcoin_withdrawals_outputs` to work as + // expected. + setup.submit_sweep_tx(rpc, faucet); + + // Different: We're adding a row that let the signer know that someone + // may have tried to fulfill the withdrawal request. If that + // transaction is spending the current signer UTXO, then it could + // possibly be in the mempool. Since the signers' UTXO is a donation, + // we're saying that the coordinator may have tried to fulfill the + // withdrawal. + setup.store_bitcoin_withdrawals_outputs(&db).await; + + // Normal: the request and how the signers voted needs to be added to + // the database. Here the bitmap in the withdrawal request object + // corresponds to how the signers voted. + setup.store_withdrawal_requests(&db).await; + setup.store_withdrawal_decisions(&db).await; + + // Normal: We do not reject a withdrawal requests until more than + // WITHDRAWAL_BLOCKS_EXPIRY blocks have been observed since the smart + // contract that created the withdrawal request has bene observed. + faucet.generate_blocks(WITHDRAWAL_BLOCKS_EXPIRY + 1); + + // Normal: the signer follows the bitcoin blockchain and event observer + // should be getting new block events from bitcoin-core. We haven't + // hooked up our block observer, so we need to manually update the + // database with new bitcoin block headers. + fetch_canonical_bitcoin_blockchain(&db, rpc).await; + + // Generate the transaction and corresponding request context. + let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; + + let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; + match validation_result.unwrap_err() { + Error::WithdrawalRejectValidation(ref err) => { + assert_eq!(err.error, WithdrawalRejectErrorMsg::RequestStillActive) + } + err => panic!("unexpected error during validation {err}"), + } + + let _withdrawals = setup.withdrawals.drain(..).collect::>(); + setup.submit_sweep_tx(rpc, faucet); + + faucet.generate_blocks(WITHDRAWAL_MIN_CONFIRMATIONS - 1); + + setup.store_sweep_tx(&db).await; + fetch_canonical_bitcoin_blockchain(&db, rpc).await; + + let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; + + let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; + match validation_result.unwrap_err() { + Error::WithdrawalRejectValidation(ref err) => { + assert_eq!(err.error, WithdrawalRejectErrorMsg::RequestStillActive) + } + err => panic!("unexpected error during validation {err}"), + } + + // Generate more blocks and backfill the DB + faucet.generate_blocks(1); + fetch_canonical_bitcoin_blockchain(&db, rpc).await; + + // Generate the transaction and corresponding request context. + let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; + + reject_withdrawal_tx.validate(&ctx, &req_ctx).await.unwrap(); + + testing::storage::drop_db(db).await; +} From d7037b276af2f3ae3a0dd86bd770e6c00b251b21 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 18:21:22 -0500 Subject: [PATCH 05/16] Fix the tests --- signer/tests/integration/setup.rs | 52 +++++++++++++------ .../integration/transaction_coordinator.rs | 1 + signer/tests/integration/withdrawal_reject.rs | 35 +++++++------ 3 files changed, 56 insertions(+), 32 deletions(-) diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index f443333e7..b2d92db95 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -622,6 +622,15 @@ pub struct SweepTxInfo { pub tx_info: BitcoinTxInfo, } +#[derive(Debug, Clone)] +pub struct BroadcastSweepTxInfo { + /// The block hash of the bitcoin chain tip when the sweep transaction + /// was broadcast + pub block_hash: bitcoin::BlockHash, + /// The transaction that swept in the deposit transaction. + pub txid: bitcoin::Txid, +} + #[derive(Debug, Clone, Copy)] pub struct SweepAmounts { pub amount: u64, @@ -656,6 +665,8 @@ pub struct TestSweepSetup2 { pub donation: OutPoint, /// The transaction that swept in the deposit transaction. pub sweep_tx_info: Option, + /// Information about the sweep transaction when it was broadcast. + pub broadcast_info: Option, /// The stacks blocks confirming the withdrawal requests, along with a /// genesis block. pub stacks_blocks: Vec, @@ -784,6 +795,7 @@ impl TestSweepSetup2 { deposit_block_hash, deposits, sweep_tx_info: None, + broadcast_info: None, donation, signers, stacks_blocks, @@ -848,8 +860,9 @@ impl TestSweepSetup2 { /// This function generates a sweep transaction that sweeps in the /// deposited funds and sweeps out the withdrawal funds in a proper - /// sweep transaction, that is also confirmed on bitcoin. - pub fn submit_sweep_tx(&mut self, rpc: &Client, faucet: &Faucet) { + /// sweep transaction, it broadcasts this transaction to the bitcoin + /// network. + pub fn broadcast_sweep_tx(&mut self, rpc: &Client) { // Okay now we try to peg-in the deposit by making a transaction. // Let's start by getting the signer's sole UTXO. let aggregated_signer = &self.signers.signer; @@ -858,22 +871,16 @@ impl TestSweepSetup2 { // Well we want a BitcoinCoreClient, so we create one using the // context. Not, the best thing to do, sorry. TODO: pass in a // bitcoin core client object. - let ctx = TestContext::builder() - .with_first_bitcoin_core_client() - .with_in_memory_storage() - .with_mocked_emily_client() - .with_mocked_stacks_client() - .build(); - + let settings = Settings::new_from_default_config().unwrap(); + let btc = BitcoinCoreClient::try_from(&settings.bitcoin.rpc_endpoints[0]).unwrap(); let outpoint = OutPoint::new(signer_utxo.txid, signer_utxo.vout); - let btc = ctx.bitcoin_client; let txids = btc.get_tx_spending_prevout(&outpoint).unwrap(); let last_fees = txids .iter() .filter_map(|txid| btc.get_mempool_entry(txid).unwrap()) .map(|entry| Fees { - total: entry.vsize, + total: entry.fees.base.to_sat(), rate: entry.fees.base.to_sat() as f64 / entry.vsize as f64, }) .max_by(|fee1, fee2| fee1.rate.total_cmp(&fee2.rate)); @@ -922,7 +929,22 @@ impl TestSweepSetup2 { unsigned.tx.compute_txid() }; - // Let's sweep in the transaction + let block_header = rpc.get_blockchain_info().unwrap(); + + self.broadcast_info = Some(BroadcastSweepTxInfo { + block_hash: block_header.best_block_hash, + txid, + }); + } + + /// This function generates a sweep transaction that sweeps in the + /// deposited funds and sweeps out the withdrawal funds in a proper + /// sweep transaction, that is also confirmed on bitcoin. + pub fn submit_sweep_tx(&mut self, rpc: &Client, faucet: &Faucet) { + self.broadcast_sweep_tx(rpc); + let txid = self.broadcast_info.as_ref().unwrap().txid; + + // Let's confirm the sweep transaction let block_hash = faucet.generate_blocks(1).pop().unwrap(); let block_header = rpc.get_block_header_info(&block_hash).unwrap(); @@ -965,18 +987,18 @@ impl TestSweepSetup2 { /// validation, where we write to the `bitcoin_withdrawals_outputs` /// table at the end. pub async fn store_bitcoin_withdrawals_outputs(&self, db: &PgStore) { - let sweep = self.sweep_tx_info.as_ref().expect("no sweep tx info set"); + let sweep = self.broadcast_info.as_ref().expect("no sweep tx info set"); for (index, withdrawal) in self.withdrawals.iter().enumerate() { let swept_output = BitcoinWithdrawalOutput { request_id: withdrawal.request.request_id, stacks_txid: withdrawal.request.txid, stacks_block_hash: withdrawal.request.block_hash, - bitcoin_chain_tip: sweep.block_hash, + bitcoin_chain_tip: sweep.block_hash.into(), is_valid_tx: true, validation_result: WithdrawalValidationResult::Ok, output_index: index as u32 + 2, - bitcoin_txid: sweep.tx_info.txid.into(), + bitcoin_txid: sweep.txid.into(), }; db.write_bitcoin_withdrawals_outputs(&[swept_output]) .await diff --git a/signer/tests/integration/transaction_coordinator.rs b/signer/tests/integration/transaction_coordinator.rs index 40d90c3d8..91e08957e 100644 --- a/signer/tests/integration/transaction_coordinator.rs +++ b/signer/tests/integration/transaction_coordinator.rs @@ -3056,6 +3056,7 @@ fn create_test_setup( deposit_block_hash, deposits: vec![(deposit_info, deposit_request, tx_info)], sweep_tx_info: None, + broadcast_info: None, donation, stacks_blocks: vec![stacks_block], signers: test_signers, diff --git a/signer/tests/integration/withdrawal_reject.rs b/signer/tests/integration/withdrawal_reject.rs index 31aba7e34..b40cff2b1 100644 --- a/signer/tests/integration/withdrawal_reject.rs +++ b/signer/tests/integration/withdrawal_reject.rs @@ -663,21 +663,6 @@ async fn reject_withdrawal_validation_request_still_active() { // fulfilled by a sweep transaction that is in the mempool. setup.store_donation(&db).await; - // Different: We submit a sweep transaction into the mempool so that - // the TestSweepSetup2 struct has the sweep_tx_info set. We also need - // to submit the transaction in order for - // `TestSweepSetup2::store_bitcoin_withdrawals_outputs` to work as - // expected. - setup.submit_sweep_tx(rpc, faucet); - - // Different: We're adding a row that let the signer know that someone - // may have tried to fulfill the withdrawal request. If that - // transaction is spending the current signer UTXO, then it could - // possibly be in the mempool. Since the signers' UTXO is a donation, - // we're saying that the coordinator may have tried to fulfill the - // withdrawal. - setup.store_bitcoin_withdrawals_outputs(&db).await; - // Normal: the request and how the signers voted needs to be added to // the database. Here the bitmap in the withdrawal request object // corresponds to how the signers voted. @@ -695,6 +680,21 @@ async fn reject_withdrawal_validation_request_still_active() { // database with new bitcoin block headers. fetch_canonical_bitcoin_blockchain(&db, rpc).await; + // Different: We submit a sweep transaction into the mempool so that + // the TestSweepSetup2 struct has the sweep_tx_info set. We also need + // to submit the transaction in order for + // `TestSweepSetup2::store_bitcoin_withdrawals_outputs` to work as + // expected. + setup.broadcast_sweep_tx(rpc); + + // Different: We're adding a row that let the signer know that someone + // may have tried to fulfill the withdrawal request. If that + // transaction is spending the current signer UTXO, then it could + // possibly be in the mempool. Since the signers' UTXO is a donation, + // we're saying that the coordinator may have tried to fulfill the + // withdrawal. + setup.store_bitcoin_withdrawals_outputs(&db).await; + // Generate the transaction and corresponding request context. let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; @@ -706,12 +706,13 @@ async fn reject_withdrawal_validation_request_still_active() { err => panic!("unexpected error during validation {err}"), } - let _withdrawals = setup.withdrawals.drain(..).collect::>(); + let withdrawals = setup.withdrawals.drain(..).collect::>(); setup.submit_sweep_tx(rpc, faucet); - faucet.generate_blocks(WITHDRAWAL_MIN_CONFIRMATIONS - 1); + faucet.generate_blocks(WITHDRAWAL_MIN_CONFIRMATIONS); setup.store_sweep_tx(&db).await; + setup.withdrawals = withdrawals; fetch_canonical_bitcoin_blockchain(&db, rpc).await; let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; From 998559d0fdd6a5c174dae4d2bae747cdf9d54988 Mon Sep 17 00:00:00 2001 From: djordon Date: Fri, 21 Feb 2025 18:42:34 -0500 Subject: [PATCH 06/16] The height can be missing, fetch a nullable field optional --- signer/src/storage/postgres.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 670514de6..7833da13f 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -2131,7 +2131,7 @@ impl super::DbRead for PgStore { bitcoin_chain_tip: &model::BitcoinBlockRef, min_confirmations: u64, ) -> Result { - let min_block_height = sqlx::query_scalar::<_, i64>( + let min_block_height = sqlx::query_scalar::<_, Option>( r#" SELECT MAX(bb.block_height) + 1 FROM sbtc_signer.bitcoin_withdrawals_outputs AS bwo @@ -2147,6 +2147,12 @@ impl super::DbRead for PgStore { .await .map_err(Error::SqlxQuery)?; + // This means that there are no rows associated with the ID in the + // `bitcoin_withdrawals_outputs` table. + let Some(min_block_height) = min_block_height else { + return Ok(false); + }; + let output_type = model::TxOutputType::SignersOutput; let chain_tip_hash = &bitcoin_chain_tip.block_hash; let signer_utxo_fut = self.get_utxo(chain_tip_hash, output_type, min_block_height); From f274c03ee0f577bd480248720e8997b3551b91f2 Mon Sep 17 00:00:00 2001 From: djordon Date: Sat, 22 Feb 2025 00:47:30 -0500 Subject: [PATCH 07/16] Okay, don't be too silly. Fetch the height of the first confirmed transaction output on or after a given block height. --- signer/src/storage/postgres.rs | 59 ++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 7833da13f..58ca3209e 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -171,8 +171,6 @@ struct PgSignerUtxo { #[sqlx(try_from = "i64")] amount: u64, aggregate_key: PublicKey, - #[sqlx(try_from = "i64")] - block_height: u64, } impl From for SignerUtxo { @@ -403,13 +401,11 @@ impl PgStore { chain_tip: &model::BitcoinBlockHash, output_type: model::TxOutputType, min_block_height: i64, - ) -> Result, Error> { - sqlx::query_as::<_, PgSignerUtxo>( + ) -> Result, Error> { + let pg_utxo = sqlx::query_as::<_, PgSignerUtxo>( r#" WITH bitcoin_blockchain AS ( - SELECT - block_hash - , block_height + SELECT block_hash FROM bitcoin_blockchain_until($1, $2) ), confirmed_sweeps AS ( @@ -426,7 +422,6 @@ impl PgStore { , bo.output_index , bo.amount , ds.aggregate_key - , bb.block_height FROM sbtc_signer.bitcoin_tx_outputs AS bo JOIN sbtc_signer.bitcoin_transactions AS bt USING (txid) JOIN bitcoin_blockchain AS bb USING (block_hash) @@ -445,6 +440,39 @@ impl PgStore { .bind(output_type) .fetch_optional(&self.0) .await + .map_err(Error::SqlxQuery)?; + + Ok(pg_utxo.map(SignerUtxo::from)) + } + + /// This function returns the bitcoin block height of the first + /// confirmed sweep that happened on or after the given minimum block + /// height. + pub async fn get_least_txo_height( + &self, + chain_tip: &model::BitcoinBlockHash, + min_block_height: i64, + ) -> Result, Error> { + sqlx::query_scalar::<_, i64>( + r#" + SELECT bb.block_height + FROM sbtc_signer.bitcoin_tx_inputs AS bi + JOIN sbtc_signer.bitcoin_tx_outputs AS bo + ON bo.txid = bi.txid + JOIN sbtc_signer.bitcoin_transactions AS bt + ON bt.txid = bi.txid + JOIN bitcoin_blockchain_until($1, $2) AS bb + ON bb.block_hash = bt.block_hash + WHERE bo.output_type = 'signers_output' + AND bi.prevout_type = 'signers_input' + ORDER BY bb.block_height ASC + LIMIT 1; + "#, + ) + .bind(chain_tip) + .bind(min_block_height) + .fetch_optional(&self.0) + .await .map_err(Error::SqlxQuery) } @@ -483,7 +511,6 @@ impl PgStore { let output_type = model::TxOutputType::Donation; self.get_utxo(chain_tip, output_type, min_block_height) .await - .map(|pg_utxo| pg_utxo.map(SignerUtxo::from)) } /// Return a block height that is less than or equal to the block that /// confirms the signers' UTXO. @@ -1984,7 +2011,7 @@ impl super::DbRead for PgStore { // happens we try searching for a donation. let output_type = model::TxOutputType::SignersOutput; let fut = self.get_utxo(chain_tip, output_type, min_block_height); - match fut.await?.map(SignerUtxo::from) { + match fut.await? { res @ Some(_) => Ok(res), None => self.get_donation_utxo(chain_tip).await, } @@ -2153,16 +2180,20 @@ impl super::DbRead for PgStore { return Ok(false); }; - let output_type = model::TxOutputType::SignersOutput; let chain_tip_hash = &bitcoin_chain_tip.block_hash; - let signer_utxo_fut = self.get_utxo(chain_tip_hash, output_type, min_block_height); + let least_txo_height = self + .get_least_txo_height(chain_tip_hash, min_block_height) + .await? + .map(u64::try_from) + .transpose() + .map_err(|_| Error::TypeConversion)?; // If this returns None, then the sweep itself could be in the // mempool. If that's the case then this is definitely active. - let Some(signer_utxo) = signer_utxo_fut.await? else { + let Some(least_txo_height) = least_txo_height else { return Ok(true); }; - Ok(signer_utxo.block_height + min_confirmations >= bitcoin_chain_tip.block_height) + Ok(least_txo_height + min_confirmations >= bitcoin_chain_tip.block_height) } async fn get_bitcoin_tx( From c89ab79ca9c317b9fcef80ce7fdc89c6580ecaa4 Mon Sep 17 00:00:00 2001 From: djordon Date: Sat, 22 Feb 2025 01:05:21 -0500 Subject: [PATCH 08/16] Forgot about these db drops --- signer/tests/integration/postgres.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index ee0b18dd6..b1f9e0e2d 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -5471,6 +5471,8 @@ async fn is_withdrawal_inflight_catches_withdrawals_with_rows_in_table() { db.write_bitcoin_txs_sighashes(&[sighash]).await.unwrap(); assert!(db.is_withdrawal_inflight(&id, &chain_tip).await.unwrap()); + + signer::testing::storage::drop_db(db).await; } /// Check that is_withdrawal_inflight correctly picks up withdrawal @@ -5579,4 +5581,6 @@ async fn is_withdrawal_inflight_catches_withdrawals_in_package() { db.write_bitcoin_txs_sighashes(&[sighash1]).await.unwrap(); assert!(db.is_withdrawal_inflight(&id, &chain_tip).await.unwrap()); + + signer::testing::storage::drop_db(db).await; } From b19c2717ba4657429f58d822d33e4ba947fed0ee Mon Sep 17 00:00:00 2001 From: djordon Date: Sat, 22 Feb 2025 02:46:43 -0500 Subject: [PATCH 09/16] Start adding some tests --- signer/src/storage/postgres.rs | 2 +- signer/src/testing/dummy.rs | 2 +- signer/tests/integration/postgres.rs | 18 ++++++++++++++++++ signer/tests/integration/setup.rs | 4 ++-- 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 58ca3209e..4adf329ac 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -448,7 +448,7 @@ impl PgStore { /// This function returns the bitcoin block height of the first /// confirmed sweep that happened on or after the given minimum block /// height. - pub async fn get_least_txo_height( + async fn get_least_txo_height( &self, chain_tip: &model::BitcoinBlockHash, min_block_height: i64, diff --git a/signer/src/testing/dummy.rs b/signer/src/testing/dummy.rs index 2d3279c2e..9d05ba2d8 100644 --- a/signer/src/testing/dummy.rs +++ b/signer/src/testing/dummy.rs @@ -754,7 +754,7 @@ impl fake::Dummy for RotateKeysV1 { impl fake::Dummy for QualifiedRequestId { fn dummy_with_rng(config: &fake::Faker, rng: &mut R) -> Self { QualifiedRequestId { - request_id: config.fake_with_rng(rng), + request_id: config.fake_with_rng::(rng) as u64, txid: config.fake_with_rng(rng), block_hash: config.fake_with_rng(rng), } diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index b1f9e0e2d..a0196ea2f 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -5584,3 +5584,21 @@ async fn is_withdrawal_inflight_catches_withdrawals_in_package() { signer::testing::storage::drop_db(db).await; } + +/// Check that is_withdrawal_active correctly returns false for unknown +/// withdrawal requests. +#[tokio::test] +async fn is_withdrawal_active_unknown_withdrawal() { + let db = testing::storage::new_test_database().await; + let mut rng = rand::rngs::StdRng::seed_from_u64(2); + + let chain_tip = Faker.fake_with_rng(&mut rng); + let qualified_id: QualifiedRequestId = Faker.fake_with_rng(&mut rng); + let active = db + .is_withdrawal_active(&qualified_id, &chain_tip, 1) + .await + .unwrap(); + assert!(!active); + + signer::testing::storage::drop_db(db).await; +} diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index b2d92db95..23cf932f4 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -869,7 +869,7 @@ impl TestSweepSetup2 { let signer_utxo = aggregated_signer.get_utxos(rpc, None).pop().unwrap(); // Well we want a BitcoinCoreClient, so we create one using the - // context. Not, the best thing to do, sorry. TODO: pass in a + // settings. Not, the best thing to do, sorry. TODO: pass in a // bitcoin core client object. let settings = Settings::new_from_default_config().unwrap(); let btc = BitcoinCoreClient::try_from(&settings.bitcoin.rpc_endpoints[0]).unwrap(); @@ -883,7 +883,7 @@ impl TestSweepSetup2 { total: entry.fees.base.to_sat(), rate: entry.fees.base.to_sat() as f64 / entry.vsize as f64, }) - .max_by(|fee1, fee2| fee1.rate.total_cmp(&fee2.rate)); + .max_by_key(|fees| fees.total); let withdrawals = self .withdrawals From 3b8bfbaaee84886e4a2433d635a55b6b8714f2ad Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 00:59:28 -0500 Subject: [PATCH 10/16] Add more comments to the is_withdrawal_active implementation --- signer/src/storage/postgres.rs | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/signer/src/storage/postgres.rs b/signer/src/storage/postgres.rs index 4adf329ac..b8f97a0e4 100644 --- a/signer/src/storage/postgres.rs +++ b/signer/src/storage/postgres.rs @@ -2158,9 +2158,17 @@ impl super::DbRead for PgStore { bitcoin_chain_tip: &model::BitcoinBlockRef, min_confirmations: u64, ) -> Result { - let min_block_height = sqlx::query_scalar::<_, Option>( + // We want to find the first sweep transaction that happened + // *after* the last time the signers considered sweeping the + // withdrawal request. We do this in two stages: + // 1. Find the block height of the last time that the signers + // considered the withdrawal request (the query right below this + // comment). + // 2. Find the least height of any sweep transaction confirmed in a + // block with height greater than the height returned from (1). + let last_considered_height = sqlx::query_scalar::<_, Option>( r#" - SELECT MAX(bb.block_height) + 1 + SELECT MAX(bb.block_height) FROM sbtc_signer.bitcoin_withdrawals_outputs AS bwo JOIN sbtc_signer.bitcoin_blocks AS bb ON bb.block_hash = bwo.bitcoin_chain_tip @@ -2174,12 +2182,17 @@ impl super::DbRead for PgStore { .await .map_err(Error::SqlxQuery)?; - // This means that there are no rows associated with the ID in the - // `bitcoin_withdrawals_outputs` table. - let Some(min_block_height) = min_block_height else { + // We add one because we are interested in sweeps that were + // confirmed after the signers last considered the withdrawal. + let Some(min_block_height) = last_considered_height.map(|x| x + 1) else { + // This means that there are no rows associated with the ID in the + // `bitcoin_withdrawals_outputs` table. return Ok(false); }; + // We now know that the signers considered this withdrawal request + // at least once. We now try to find the height of the oldest + // signer TXO whose height is greater than the height from above. let chain_tip_hash = &bitcoin_chain_tip.block_hash; let least_txo_height = self .get_least_txo_height(chain_tip_hash, min_block_height) @@ -2192,8 +2205,14 @@ impl super::DbRead for PgStore { let Some(least_txo_height) = least_txo_height else { return Ok(true); }; - - Ok(least_txo_height + min_confirmations >= bitcoin_chain_tip.block_height) + // We test whether the TXO height has a minimum number of + // confirmations. If it doesn't have enough confirmations, then it + // is considered active since a fork could put it into the mempool + // again. + let txo_confirmations = bitcoin_chain_tip + .block_height + .saturating_sub(least_txo_height); + Ok(txo_confirmations <= min_confirmations) } async fn get_bitcoin_tx( From d54581faabf3dfb0e5acdd15056a6e4654eb25cb Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 00:59:40 -0500 Subject: [PATCH 11/16] Fix typos in the comments --- signer/src/stacks/contracts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/signer/src/stacks/contracts.rs b/signer/src/stacks/contracts.rs index f2d74ff52..8352afcc7 100644 --- a/signer/src/stacks/contracts.rs +++ b/signer/src/stacks/contracts.rs @@ -1115,8 +1115,8 @@ impl AsContractCall for RejectWithdrawalV1 { /// 7. Whether the withdrawal request is being serviced by a sweep /// transaction that is in the mempool. /// 8. Whether we need to worry about forks causing the withdrawal to - /// be confirmed by a sweep that was broadcst changing the status of - /// the request from rejected to accepted. + /// be confirmed by a sweep that was broadcast changing the status + /// of the request from rejected to accepted. async fn validate(&self, ctx: &C, req_ctx: &ReqContext) -> Result<(), Error> where C: Context + Send + Sync, From 55a0aae4a01e50369815c6558035524368cc6b30 Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 01:25:00 -0500 Subject: [PATCH 12/16] Update the comments, add some new ones --- signer/tests/integration/withdrawal_reject.rs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/signer/tests/integration/withdrawal_reject.rs b/signer/tests/integration/withdrawal_reject.rs index b40cff2b1..7d1514ec1 100644 --- a/signer/tests/integration/withdrawal_reject.rs +++ b/signer/tests/integration/withdrawal_reject.rs @@ -653,7 +653,7 @@ async fn reject_withdrawal_validation_request_still_active() { // database with new bitcoin block headers. fetch_canonical_bitcoin_blockchain(&db, rpc).await; - // Different: we need to store a row in the dkg_shares table so that we + // Normal: we need to store a row in the dkg_shares table so that we // have a record of the scriptPubKey that the signers control. We need // this so that the donation gets picked up correctly below. setup.store_dkg_shares(&db).await; @@ -680,11 +680,9 @@ async fn reject_withdrawal_validation_request_still_active() { // database with new bitcoin block headers. fetch_canonical_bitcoin_blockchain(&db, rpc).await; - // Different: We submit a sweep transaction into the mempool so that - // the TestSweepSetup2 struct has the sweep_tx_info set. We also need - // to submit the transaction in order for - // `TestSweepSetup2::store_bitcoin_withdrawals_outputs` to work as - // expected. + // Different: We broadcast a sweep transaction into the mempool so that + // the TestSweepSetup2 struct has the `broadcast_info` is set, which is + // required for `TestSweepSetup2::store_bitcoin_withdrawals_outputs`. setup.broadcast_sweep_tx(rpc); // Different: We're adding a row that let the signer know that someone @@ -698,6 +696,9 @@ async fn reject_withdrawal_validation_request_still_active() { // Generate the transaction and corresponding request context. let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; + // Right now the withdrawal request is expired, but there is a + // transaction in the mempool that is trying to fulfill it, so + // validation must fail. let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; match validation_result.unwrap_err() { Error::WithdrawalRejectValidation(ref err) => { @@ -706,6 +707,14 @@ async fn reject_withdrawal_validation_request_still_active() { err => panic!("unexpected error during validation {err}"), } + // We want to "replace" the transaction in the mempool with another + // transaction that is not fulfilling the request. If we didn't remove + // the withdrawal, RejectWithdrawalV1 would fail validation for the + // wrong reason after the sweep has been confirmed, since the + // withdrawal would be fulfilled. + // + // So we remove the withdrawals from the TestSweepSetup2 object so + // that they do not get included in the sweep transaction. let withdrawals = setup.withdrawals.drain(..).collect::>(); setup.submit_sweep_tx(rpc, faucet); From 73f04b6ccaff811d15ca6ca82b7499f457b53797 Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 01:54:00 -0500 Subject: [PATCH 13/16] conditionally broadcast the sweep in submit sweep tx --- signer/tests/integration/setup.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index 23cf932f4..339a5b31d 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -941,7 +941,9 @@ impl TestSweepSetup2 { /// deposited funds and sweeps out the withdrawal funds in a proper /// sweep transaction, that is also confirmed on bitcoin. pub fn submit_sweep_tx(&mut self, rpc: &Client, faucet: &Faucet) { - self.broadcast_sweep_tx(rpc); + if self.broadcast_info.is_none() { + self.broadcast_sweep_tx(rpc); + } let txid = self.broadcast_info.as_ref().unwrap().txid; // Let's confirm the sweep transaction From 856e346f565eb336631484851d2785208fe2b9b0 Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 02:00:37 -0500 Subject: [PATCH 14/16] Store sighashes when writing to the database --- signer/tests/integration/setup.rs | 40 ++++++++++++++++ signer/tests/integration/withdrawal_reject.rs | 46 ++++++++++++------- 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/signer/tests/integration/setup.rs b/signer/tests/integration/setup.rs index 339a5b31d..76be17e66 100644 --- a/signer/tests/integration/setup.rs +++ b/signer/tests/integration/setup.rs @@ -45,6 +45,7 @@ use signer::storage::model::BitcoinBlock; use signer::storage::model::BitcoinBlockHash; use signer::storage::model::BitcoinBlockRef; use signer::storage::model::BitcoinTxRef; +use signer::storage::model::BitcoinTxSigHash; use signer::storage::model::BitcoinWithdrawalOutput; use signer::storage::model::DkgSharesStatus; use signer::storage::model::EncryptedDkgShares; @@ -982,6 +983,45 @@ impl TestSweepSetup2 { } } + /// Store the rows in the `bitcoin_tx_sighashes` for the sweep. + /// + /// This simulates the sweep transaction successfully going through + /// validation, where we write to the `bitcoin_tx_sighashes` table at + /// the end. + pub async fn store_bitcoin_tx_sighashes(&self, db: &PgStore) { + let sweep = self.broadcast_info.as_ref().expect("no sweep tx info set"); + + let sighash = BitcoinTxSigHash { + txid: sweep.txid.into(), + chain_tip: sweep.block_hash.into(), + prevout_txid: self.donation.txid.into(), + prevout_output_index: self.donation.vout, + aggregate_key: self.signers.aggregate_key().into(), + will_sign: true, + is_valid_tx: true, + validation_result: signer::bitcoin::validation::InputValidationResult::Ok, + prevout_type: model::TxPrevoutType::SignersInput, + sighash: Faker.fake_with_rng(&mut OsRng), + }; + db.write_bitcoin_txs_sighashes(&[sighash]).await.unwrap(); + + for (_, request, _) in self.deposits.iter() { + let sighash = BitcoinTxSigHash { + txid: sweep.txid.into(), + chain_tip: sweep.block_hash.into(), + prevout_txid: request.outpoint.txid.into(), + prevout_output_index: request.outpoint.vout, + aggregate_key: request.signers_public_key.into(), + will_sign: true, + is_valid_tx: true, + validation_result: signer::bitcoin::validation::InputValidationResult::Ok, + prevout_type: model::TxPrevoutType::SignersInput, + sighash: Faker.fake_with_rng(&mut OsRng), + }; + db.write_bitcoin_txs_sighashes(&[sighash]).await.unwrap(); + } + } + /// Store the rows in the `bitcoin_withdrawals_outputs` for the /// withdrawals. /// diff --git a/signer/tests/integration/withdrawal_reject.rs b/signer/tests/integration/withdrawal_reject.rs index 7d1514ec1..a98c434ae 100644 --- a/signer/tests/integration/withdrawal_reject.rs +++ b/signer/tests/integration/withdrawal_reject.rs @@ -531,9 +531,9 @@ async fn reject_withdrawal_validation_request_being_fulfilled() { // database with new bitcoin block headers. let chain_tip = fetch_canonical_bitcoin_blockchain(&db, rpc).await; - // Different: we need to store a row in the dkg_shares table so that we - // have a record of the scriptPubKey that the signers control. We need - // this so that the donation gets picked up correctly below. + // Normal: we need to store a row in the dkg_shares table so that we + // have a record of the scriptPubKey that the signers control. This is + // needed for the donation. setup.store_dkg_shares(&db).await; // Normal: The signers normally have a UTXO, so we add one here too. It @@ -606,10 +606,9 @@ async fn reject_withdrawal_validation_request_being_fulfilled() { } /// For this test we check that the `RejectWithdrawalV1::validate` function -/// returns a withdrawal validation error with a RequestStillActive -/// message when the database indicates that it is possible that the -/// withdrawal request is being fulfilled by a sweep transaction in the -/// mempool. +/// returns a withdrawal validation error with a RequestStillActive message +/// when the database indicates that it is possible that the withdrawal +/// request to be unintentionally fulfilled after a bitcoin reorg. #[tokio::test] async fn reject_withdrawal_validation_request_still_active() { // Normal: this generates the blockchain as well as a transaction @@ -685,24 +684,27 @@ async fn reject_withdrawal_validation_request_still_active() { // required for `TestSweepSetup2::store_bitcoin_withdrawals_outputs`. setup.broadcast_sweep_tx(rpc); - // Different: We're adding a row that let the signer know that someone + // Different: We're adding rows that let the signer know that someone // may have tried to fulfill the withdrawal request. If that // transaction is spending the current signer UTXO, then it could // possibly be in the mempool. Since the signers' UTXO is a donation, // we're saying that the coordinator may have tried to fulfill the // withdrawal. setup.store_bitcoin_withdrawals_outputs(&db).await; + setup.store_bitcoin_tx_sighashes(&db).await; // Generate the transaction and corresponding request context. let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; // Right now the withdrawal request is expired, but there is a // transaction in the mempool that is trying to fulfill it, so - // validation must fail. + // validation must fail with RequestBeingFulfilled. After the next + // sweep transaction gets confirmed, we must observe + // WITHDRAWAL_MIN_CONFIRMATIONS more blocks. let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; match validation_result.unwrap_err() { Error::WithdrawalRejectValidation(ref err) => { - assert_eq!(err.error, WithdrawalRejectErrorMsg::RequestStillActive) + assert_eq!(err.error, WithdrawalRejectErrorMsg::RequestBeingFulfilled) } err => panic!("unexpected error during validation {err}"), } @@ -716,16 +718,28 @@ async fn reject_withdrawal_validation_request_still_active() { // So we remove the withdrawals from the TestSweepSetup2 object so // that they do not get included in the sweep transaction. let withdrawals = setup.withdrawals.drain(..).collect::>(); + + setup.broadcast_sweep_tx(rpc); setup.submit_sweep_tx(rpc, faucet); + setup.store_sweep_tx(&db).await; - faucet.generate_blocks(WITHDRAWAL_MIN_CONFIRMATIONS); + // This confirms the sweep in the mempool. It is the first sweep after + // trying to fulfill the withdrawal request. Now we must observe + // WITHDRAWAL_MIN_CONFIRMATIONS more blocks before the withdrawal is + // considered inactive and we can reject the withdrawal request. + faucet.generate_block(); - setup.store_sweep_tx(&db).await; - setup.withdrawals = withdrawals; + // Let's add some blocks, but one shy of the number of blocks necessary + // for the withdrawal to be "inactive". + faucet.generate_blocks(WITHDRAWAL_MIN_CONFIRMATIONS - 1); fetch_canonical_bitcoin_blockchain(&db, rpc).await; + // We need to add back the withdrawals so that `make_withdrawal_reject` + // works. + setup.withdrawals = withdrawals; let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; + // Okay, this should fail because we haven't observed enough blocks yet. let validation_result = reject_withdrawal_tx.validate(&ctx, &req_ctx).await; match validation_result.unwrap_err() { Error::WithdrawalRejectValidation(ref err) => { @@ -734,11 +748,11 @@ async fn reject_withdrawal_validation_request_still_active() { err => panic!("unexpected error during validation {err}"), } - // Generate more blocks and backfill the DB - faucet.generate_blocks(1); + // Generate one more block. After seeing that next block, the + // withdrawal should be considered inactive. + faucet.generate_block(); fetch_canonical_bitcoin_blockchain(&db, rpc).await; - // Generate the transaction and corresponding request context. let (reject_withdrawal_tx, req_ctx) = make_withdrawal_reject(&setup, &db).await; reject_withdrawal_tx.validate(&ctx, &req_ctx).await.unwrap(); From 0dc7a3658d6c3db1188dc41640332e12be160da2 Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 02:49:52 -0500 Subject: [PATCH 15/16] Add a new test for the query --- signer/tests/integration/postgres.rs | 37 ++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index a0196ea2f..91fc979f8 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -5602,3 +5602,40 @@ async fn is_withdrawal_active_unknown_withdrawal() { signer::testing::storage::drop_db(db).await; } + +/// Check that is_withdrawal_active correctly returns true when there is a +/// sweep fulfilling a withdrawal request that might be in the mempool. +#[tokio::test] +async fn is_withdrawal_active_for_considered_withdrawal() { + let db = testing::storage::new_test_database().await; + let mut rng = rand::rngs::StdRng::seed_from_u64(4); + + let chain_tip: model::BitcoinBlock = Faker.fake_with_rng(&mut rng); + let qualified_id: QualifiedRequestId = Faker.fake_with_rng(&mut rng); + + let output = BitcoinWithdrawalOutput { + request_id: qualified_id.request_id, + stacks_txid: qualified_id.txid, + stacks_block_hash: qualified_id.block_hash, + bitcoin_chain_tip: chain_tip.block_hash, + is_valid_tx: true, + validation_result: WithdrawalValidationResult::Ok, + output_index: 2, + bitcoin_txid: Faker.fake_with_rng(&mut rng), + }; + db.write_bitcoin_withdrawals_outputs(&[output]) + .await + .unwrap(); + + db.write_bitcoin_block(&chain_tip).await.unwrap(); + + let chain_tip_ref = chain_tip.into(); + + let active = db + .is_withdrawal_active(&qualified_id, &chain_tip_ref, 1) + .await + .unwrap(); + assert!(active); + + signer::testing::storage::drop_db(db).await; +} From d4abff0719c4d32347cd33281346708bc085fd32 Mon Sep 17 00:00:00 2001 From: djordon Date: Sun, 23 Feb 2025 02:58:56 -0500 Subject: [PATCH 16/16] cargo fmt --- signer/tests/integration/postgres.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/signer/tests/integration/postgres.rs b/signer/tests/integration/postgres.rs index 91fc979f8..4bd1b4b91 100644 --- a/signer/tests/integration/postgres.rs +++ b/signer/tests/integration/postgres.rs @@ -5630,7 +5630,7 @@ async fn is_withdrawal_active_for_considered_withdrawal() { db.write_bitcoin_block(&chain_tip).await.unwrap(); let chain_tip_ref = chain_tip.into(); - + let active = db .is_withdrawal_active(&qualified_id, &chain_tip_ref, 1) .await