From 9fdb75cf883c53af97fbbe40a4d7f727611aebac Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 09:38:09 +0100 Subject: [PATCH 01/14] commands: split up spend transaction creation into its own module This moves create_spend_internal in bulk. The interface is still inappropriate and will be adapted in the next commits. --- src/commands/mod.rs | 508 +++++++--------------------------- src/commands/utils.rs | 209 +------------- src/jsonrpc/mod.rs | 8 +- src/lib.rs | 1 + src/spend.rs | 616 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 714 insertions(+), 628 deletions(-) create mode 100644 src/spend.rs diff --git a/src/commands/mod.rs b/src/commands/mod.rs index fa7d65cc1..ffb801c9b 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -7,15 +7,19 @@ mod utils; use crate::{ bitcoin::BitcoinInterface, database::{Coin, DatabaseInterface}, - descriptors, DaemonControl, VERSION, + descriptors, + spend::{ + check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, CandidateCoin, + SpendCreationError, + }, + DaemonControl, VERSION, }; pub use crate::database::{CoinStatus, LabelItem}; -use bdk_coin_select::InsufficientFunds; use utils::{ - deser_addr_assume_checked, deser_amount_from_sats, deser_fromstr, deser_hex, - select_coins_for_spend, ser_amount, ser_hex, ser_to_string, unsigned_tx_max_vbytes, + deser_addr_assume_checked, deser_amount_from_sats, deser_fromstr, deser_hex, ser_amount, + ser_hex, ser_to_string, }; use std::{ @@ -34,19 +38,6 @@ use miniscript::{ }; use serde::{Deserialize, Serialize}; -// We would never create a transaction with an output worth less than this. -// That's 1$ at 20_000$ per BTC. -const DUST_OUTPUT_SATS: u64 = 5_000; - -// Long-term feerate (sats/vb) used for coin selection considerations. -const LONG_TERM_FEERATE_VB: f32 = 10.0; - -// Assume that paying more than 1BTC in fee is a bug. -const MAX_FEE: u64 = bitcoin::blockdata::constants::COIN_VALUE; - -// Assume that paying more than 1000sat/vb in feerate is a bug. -const MAX_FEERATE: u64 = 1_000; - // Timestamp in the header of the genesis block. Used for sanity checks. const MAINNET_GENESIS_TIME: u32 = 1231006505; @@ -58,15 +49,12 @@ pub enum CommandError { AlreadySpent(bitcoin::OutPoint), ImmatureCoinbase(bitcoin::OutPoint), Address(bitcoin::address::Error), - InvalidOutputValue(bitcoin::Amount), + SpendCreation(SpendCreationError), InsufficientFunds( /* in value */ bitcoin::Amount, /* out value */ Option, /* target feerate */ u64, ), - InsaneFees(InsaneFeeInfo), - FetchingTransaction(bitcoin::OutPoint), - SanityCheckFailure(Psbt), UnknownSpend(bitcoin::Txid), // FIXME: when upgrading Miniscript put the actual error there SpendFinalization(String), @@ -78,57 +66,40 @@ pub enum CommandError { RecoveryNotAvailable, /// Overflowing or unhardened derivation index. InvalidDerivationIndex, - CoinSelectionError(InsufficientFunds), RbfError(RbfErrorInfo), } impl fmt::Display for CommandError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::NoOutpointForSelfSend => write!(f, "No provided outpoint for self-send. Need at least one."), + Self::NoOutpointForSelfSend => { + write!(f, "No provided outpoint for self-send. Need at least one.") + } Self::InvalidFeerate(sats_vb) => write!(f, "Invalid feerate: {} sats/vb.", sats_vb), Self::AlreadySpent(op) => write!(f, "Coin at '{}' is already spent.", op), - Self::ImmatureCoinbase(op) => write!(f, "Coin at '{}' is from an immature coinbase transaction.", op), - Self::UnknownOutpoint(op) => write!(f, "Unknown outpoint '{}'.", op), - Self::Address(e) => write!( + Self::ImmatureCoinbase(op) => write!( f, - "Address error: {}", e + "Coin at '{}' is from an immature coinbase transaction.", + op ), - Self::InvalidOutputValue(amount) => write!(f, "Invalid output value '{}'.", amount), - Self::InsufficientFunds(in_val, out_val, feerate) => if let Some(out_val) = out_val { - write!( + Self::UnknownOutpoint(op) => write!(f, "Unknown outpoint '{}'.", op), + Self::Address(e) => write!(f, "Address error: {}", e), + Self::SpendCreation(e) => write!(f, "Creating spend: {}", e), + Self::InsufficientFunds(in_val, out_val, feerate) => { + if let Some(out_val) = out_val { + write!( f, "Cannot create a {} sat/vb transaction with input value {} and output value {}", feerate, in_val, out_val ) - } else { - write!( - f, - "Not enough fund to create a {} sat/vb transaction with input value {}", - feerate, in_val - ) - }, - Self::InsaneFees(info) => write!( - f, - "We assume transactions with a fee larger than {} sats or a feerate larger than {} sats/vb are a mistake. \ - The created transaction {}.", - MAX_FEE, - MAX_FEERATE, - match info { - InsaneFeeInfo::NegativeFee => "would have a negative fee".to_string(), - InsaneFeeInfo::TooHighFee(f) => format!("{} sats in fees", f), - InsaneFeeInfo::InvalidFeerate => "would have an invalid feerate".to_string(), - InsaneFeeInfo::TooHighFeerate(r) => format!("has a feerate of {} sats/vb", r), - }, - ), - Self::FetchingTransaction(op) => { - write!(f, "Could not fetch transaction for coin {}", op) + } else { + write!( + f, + "Not enough fund to create a {} sat/vb transaction with input value {}", + feerate, in_val + ) + } } - Self::SanityCheckFailure(psbt) => write!( - f, - "BUG! Please report this. Failed sanity checks for PSBT '{}'.", - psbt - ), Self::UnknownSpend(txid) => write!(f, "Unknown spend transaction '{}'.", txid), Self::SpendFinalization(e) => { write!(f, "Failed to finalize the spend transaction PSBT: '{}'.", e) @@ -143,36 +114,23 @@ impl fmt::Display for CommandError { Self::RecoveryNotAvailable => write!( f, "No coin currently spendable through this timelocked recovery path." - ), - Self::InvalidDerivationIndex => write!(f, "Unhardened or overflowing BIP32 derivation index."), - Self::CoinSelectionError(e) => write!(f, "Coin selection error: '{}'", e), - Self::RbfError(e) => write!(f, "RBF error: '{}'.", e) + ), + Self::InvalidDerivationIndex => { + write!(f, "Unhardened or overflowing BIP32 derivation index.") + } + Self::RbfError(e) => write!(f, "RBF error: '{}'.", e), } } } impl std::error::Error for CommandError {} -// Sanity check the value of a transaction output. -fn check_output_value(value: bitcoin::Amount) -> Result<(), CommandError> { - // NOTE: the network parameter isn't used upstream - if value.to_sat() > bitcoin::blockdata::constants::MAX_MONEY - || value.to_sat() < DUST_OUTPUT_SATS - { - Err(CommandError::InvalidOutputValue(value)) - } else { - Ok(()) +impl From for CommandError { + fn from(e: SpendCreationError) -> Self { + CommandError::SpendCreation(e) } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum InsaneFeeInfo { - NegativeFee, - InvalidFeerate, - TooHighFee(u64), - TooHighFeerate(u64), -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RbfErrorInfo { MissingFeerate, @@ -196,82 +154,6 @@ impl fmt::Display for RbfErrorInfo { } } -/// A candidate for coin selection when creating a transaction. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct CandidateCoin { - /// The candidate coin. - coin: Coin, - /// Whether or not this coin must be selected by the coin selection algorithm. - must_select: bool, -} - -// Apply some sanity checks on a created transaction's PSBT. -// TODO: add more sanity checks from revault_tx -fn sanity_check_psbt( - spent_desc: &descriptors::LianaDescriptor, - psbt: &Psbt, -) -> Result<(), CommandError> { - let tx = &psbt.unsigned_tx; - - // Must have as many in/out in the PSBT and Bitcoin tx. - if psbt.inputs.len() != tx.input.len() - || psbt.outputs.len() != tx.output.len() - || tx.output.is_empty() - { - return Err(CommandError::SanityCheckFailure(psbt.clone())); - } - - // Compute the transaction input value, checking all PSBT inputs have the derivation - // index set for signing devices to recognize them as ours. - let mut value_in = 0; - for psbtin in psbt.inputs.iter() { - if psbtin.bip32_derivation.is_empty() { - return Err(CommandError::SanityCheckFailure(psbt.clone())); - } - value_in += psbtin - .witness_utxo - .as_ref() - .ok_or_else(|| CommandError::SanityCheckFailure(psbt.clone()))? - .value; - } - - // Compute the output value and check the absolute fee isn't insane. - let value_out: u64 = tx.output.iter().map(|o| o.value).sum(); - let abs_fee = value_in - .checked_sub(value_out) - .ok_or(CommandError::InsaneFees(InsaneFeeInfo::NegativeFee))?; - if abs_fee > MAX_FEE { - return Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFee(abs_fee))); - } - - // Check the feerate isn't insane. - // Add weights together before converting to vbytes to avoid rounding up multiple times - // and increasing the result, which could lead to the feerate in sats/vb falling below 1. - let tx_wu = tx.weight().to_wu() + (spent_desc.max_sat_weight() * tx.input.len()) as u64; - let tx_vb = tx_wu - .checked_add(descriptors::WITNESS_FACTOR as u64 - 1) - .unwrap() - .checked_div(descriptors::WITNESS_FACTOR as u64) - .unwrap(); - let feerate_sats_vb = abs_fee - .checked_div(tx_vb) - .ok_or(CommandError::InsaneFees(InsaneFeeInfo::InvalidFeerate))?; - if !(1..=MAX_FEERATE).contains(&feerate_sats_vb) { - return Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate( - feerate_sats_vb, - ))); - } - - // Check for dust outputs - for txo in psbt.unsigned_tx.output.iter() { - if txo.value < txo.script_pubkey.dust_value().to_sat() { - return Err(CommandError::SanityCheckFailure(psbt.clone())); - } - } - - Ok(()) -} - impl DaemonControl { // Get the derived descriptor for this coin fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc { @@ -362,7 +244,7 @@ impl DaemonControl { }; // Derive all receive and change addresses for the queried range. - let addresses: Result, _> = (start_index_u32..end_index) + let addresses: Result, CommandError> = (start_index_u32..end_index) .map(|index| { let child = bip32::ChildNumber::from_normal_idx(index) .map_err(|_| CommandError::InvalidDerivationIndex)?; @@ -498,13 +380,20 @@ impl DaemonControl { .collect() }; - self.create_spend_internal( - &destinations_checked, - &candidate_coins, - feerate_vb, - 0, // No min fee required. - change_address, - ) + Ok(CreateSpendResult { + psbt: create_spend( + &mut db_conn, + &self.config.main_descriptor, + &self.secp, + &self.bitcoin, + self.config.bitcoin_config.network, + &destinations_checked, + &candidate_coins, + feerate_vb, + 0, // No min fee required. + change_address, + )?, + }) } pub fn update_spend(&self, mut psbt: Psbt) -> Result<(), CommandError> { @@ -559,235 +448,6 @@ impl DaemonControl { Ok(()) } - fn create_spend_internal( - &self, - destinations: &HashMap, - candidate_coins: &[CandidateCoin], - feerate_vb: u64, - min_fee: u64, - change_address: Option, - ) -> Result { - // This method is a bit convoluted, but it's the nature of creating a Bitcoin transaction - // with a target feerate and outputs. In addition, we support different modes (coin control - // vs automated coin selection, self-spend, sweep, etc..) which make the logic a bit more - // intricate. Here is a brief overview of what we're doing here: - // 1. Create a transaction with all the target outputs (if this is a self-send, none are - // added at this step the only output will be added as a change output). - // 2. Automatically select the coins if necessary and determine whether a change output - // will be necessary for this transaction from the set of (automatically or manually) - // selected coins. The output for a self-send is added there. - // The change output is also (ab)used to implement a "sweep" functionality. We allow to - // set it to an external address to send all the inputs' value minus the fee and the - // other output's value to a specific, external, address. - // 3. Add the selected coins as inputs to the transaction. - // 4. Finalize the PSBT and sanity check it before returning it. - - let is_self_send = destinations.is_empty(); - if feerate_vb < 1 { - return Err(CommandError::InvalidFeerate(feerate_vb)); - } - let mut db_conn = self.db.connection(); - - // Create transaction with no inputs and no outputs. - let mut tx = bitcoin::Transaction { - version: 2, - lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), // TODO: randomized anti fee sniping - input: Vec::with_capacity(candidate_coins.iter().filter(|c| c.must_select).count()), - output: Vec::with_capacity(destinations.len()), - }; - // Add the destinations outputs to the transaction and PSBT. At the same time - // sanity check each output's value. - let mut psbt_outs = Vec::with_capacity(destinations.len()); - for (address, &amount) in destinations { - check_output_value(amount)?; - - tx.output.push(bitcoin::TxOut { - value: amount.to_sat(), - script_pubkey: address.script_pubkey(), - }); - // If it's an address of ours, signal it as change to signing devices by adding the - // BIP32 derivation path to the PSBT output. - let bip32_derivation = - if let Some((index, is_change)) = db_conn.derivation_index_by_address(address) { - let desc = if is_change { - self.config.main_descriptor.change_descriptor() - } else { - self.config.main_descriptor.receive_descriptor() - }; - desc.derive(index, &self.secp).bip32_derivations() - } else { - Default::default() - }; - psbt_outs.push(PsbtOut { - bip32_derivation, - ..PsbtOut::default() - }); - } - assert_eq!(tx.output.is_empty(), is_self_send); - - // Now compute whether we'll need a change output while automatically selecting coins to be - // used as input if necessary. - // We need to get the size of a potential change output to select coins / determine whether - // we should include one, so get the change address and create a dummy txo for this purpose. - // The change address may be externally specified for the purpose of a "sweep": the user - // would set the value of some outputs (or none) and fill-in an address to be used for "all - // the rest". This is the same logic as for a change output, except it's external. - struct InternalChangeInfo { - pub desc: descriptors::DerivedSinglePathLianaDesc, - pub index: bip32::ChildNumber, - } - let (change_addr, int_change_info) = if let Some(addr) = change_address { - (addr, None) - } else { - let index = db_conn.change_index(); - let desc = self - .config - .main_descriptor - .change_descriptor() - .derive(index, &self.secp); - ( - desc.address(self.config.bitcoin_config.network), - Some(InternalChangeInfo { desc, index }), - ) - }; - let mut change_txo = bitcoin::TxOut { - value: std::u64::MAX, - script_pubkey: change_addr.script_pubkey(), - }; - // Now select the coins necessary using the provided candidates and determine whether - // there is any leftover to create a change output. - let (selected_coins, change_amount) = { - // At this point the transaction still has no input and no change output, as expected - // by the coins selection helper function. - assert!(tx.input.is_empty()); - assert_eq!(tx.output.len(), destinations.len()); - // TODO: Introduce general conversion error type. - let feerate_vb: f32 = { - let fr: u16 = feerate_vb.try_into().map_err(|_| { - CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate(feerate_vb)) - })?; - fr - } - .try_into() - .expect("u16 must fit in f32"); - let max_sat_wu = self - .config - .main_descriptor - .max_sat_weight() - .try_into() - .expect("Weight must fit in a u32"); - select_coins_for_spend( - candidate_coins, - tx.clone(), - change_txo.clone(), - feerate_vb, - min_fee, - max_sat_wu, - is_self_send, - ) - .map_err(CommandError::CoinSelectionError)? - }; - // If necessary, add a change output. - // For a self-send, coin selection will only find solutions with change and will otherwise - // return an error. In any case, the PSBT sanity check will catch a transaction with no outputs. - if change_amount.to_sat() > 0 { - check_output_value(change_amount)?; - - // If we generated a change address internally, set the BIP32 derivations in the PSBT - // output to tell the signers it's an internal address and make sure to update our next - // change index. Otherwise it's a sweep, so no need to set anything. - // If the change address was set by the caller, check whether it's one of ours. If it - // is, set the BIP32 derivations accordingly. In addition, if it's a change address for - // a later index than we currently have set as next change derivation index, update it. - let bip32_derivation = if let Some(InternalChangeInfo { desc, index }) = int_change_info - { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_change_index(next_index, &self.secp); - desc.bip32_derivations() - } else if let Some((index, is_change)) = - db_conn.derivation_index_by_address(&change_addr) - { - let desc = if is_change { - if db_conn.change_index() < index { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_change_index(next_index, &self.secp); - } - self.config.main_descriptor.change_descriptor() - } else { - self.config.main_descriptor.receive_descriptor() - }; - desc.derive(index, &self.secp).bip32_derivations() - } else { - Default::default() - }; - - // TODO: shuffle once we have Taproot - change_txo.value = change_amount.to_sat(); - tx.output.push(change_txo); - psbt_outs.push(PsbtOut { - bip32_derivation, - ..PsbtOut::default() - }); - } - - // Iterate through selected coins and add necessary information to the PSBT inputs. - let mut psbt_ins = Vec::with_capacity(selected_coins.len()); - let mut spent_txs = HashMap::with_capacity(selected_coins.len()); - for coin in &selected_coins { - // Fetch the transaction that created it if necessary - if let hash_map::Entry::Vacant(e) = spent_txs.entry(coin.outpoint) { - let tx = self - .bitcoin - .wallet_transaction(&coin.outpoint.txid) - .ok_or(CommandError::FetchingTransaction(coin.outpoint))?; - e.insert(tx.0); - } - - tx.input.push(bitcoin::TxIn { - previous_output: coin.outpoint, - sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, - // TODO: once we move to Taproot, anti-fee-sniping using nSequence - ..bitcoin::TxIn::default() - }); - - // Populate the PSBT input with the information needed by signers. - let coin_desc = self.derived_desc(coin); - let witness_script = Some(coin_desc.witness_script()); - let witness_utxo = Some(bitcoin::TxOut { - value: coin.amount.to_sat(), - script_pubkey: coin_desc.script_pubkey(), - }); - let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); - let bip32_derivation = coin_desc.bip32_derivations(); - psbt_ins.push(PsbtIn { - witness_script, - witness_utxo, - bip32_derivation, - non_witness_utxo, - ..PsbtIn::default() - }); - } - - // Finally, create the PSBT with all inputs and outputs, sanity check it and return it. - let psbt = Psbt { - unsigned_tx: tx, - version: 0, - xpub: BTreeMap::new(), - proprietary: BTreeMap::new(), - unknown: BTreeMap::new(), - inputs: psbt_ins, - outputs: psbt_outs, - }; - sanity_check_psbt(&self.config.main_descriptor, &psbt)?; - // TODO: maybe check for common standardness rules (max size, ..)? - - Ok(CreateSpendResult { psbt }) - } - pub fn update_labels(&self, items: &HashMap>) { let mut db_conn = self.db.connection(); db_conn.update_labels(items); @@ -1046,7 +706,12 @@ impl DaemonControl { let mut replacement_vsize = 0; for incremental_feerate in 0.. { let min_fee = descendant_fees.to_sat() + replacement_vsize * incremental_feerate; - let rbf_psbt = match self.create_spend_internal( + let rbf_psbt = match create_spend( + &mut db_conn, + &self.config.main_descriptor, + &self.secp, + &self.bitcoin, + self.config.bitcoin_config.network, &destinations, &candidate_coins, feerate_vb, @@ -1057,7 +722,7 @@ impl DaemonControl { // If we get a coin selection error due to insufficient funds and we want to cancel the // transaction, then set all previous coins as mandatory and add confirmed coins as // optional, unless we have already done this. - Err(CommandError::CoinSelectionError(_)) + Err(SpendCreationError::CoinSelection(_)) if is_cancel && candidate_coins.iter().all(|c| !c.must_select) => { for cand in candidate_coins.iter_mut() { @@ -1067,19 +732,16 @@ impl DaemonControl { continue; } Err(e) => { - return Err(e); + return Err(e.into()); } }; - replacement_vsize = unsigned_tx_max_vbytes(&rbf_psbt.psbt.unsigned_tx, max_sat_weight); + replacement_vsize = unsigned_tx_max_vbytes(&rbf_psbt.unsigned_tx, max_sat_weight); // Make sure it satisfies RBF rule 4. - if rbf_psbt - .psbt - .fee() - .expect("has already been sanity checked") + if rbf_psbt.fee().expect("has already been sanity checked") >= descendant_fees + bitcoin::Amount::from_sat(replacement_vsize) { - return Ok(rbf_psbt); + return Ok(CreateSpendResult { psbt: rbf_psbt }); } } @@ -1234,7 +896,7 @@ impl DaemonControl { let tx = self .bitcoin .wallet_transaction(&coin.outpoint.txid) - .ok_or(CommandError::FetchingTransaction(coin.outpoint))?; + .ok_or(SpendCreationError::FetchingTransaction(coin.outpoint))?; e.insert(tx.0); } @@ -1403,7 +1065,7 @@ pub struct CreateRecoveryResult { #[cfg(test)] mod tests { use super::*; - use crate::{bitcoin::Block, database::BlockInfo, testutils::*}; + use crate::{bitcoin::Block, database::BlockInfo, spend::InsaneFeeInfo, testutils::*}; use bitcoin::{ bip32::{self, ChildNumber}, @@ -1596,7 +1258,9 @@ mod tests { // Insufficient funds for coin selection. assert!(matches!( control.create_spend(&destinations, &[], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); assert_eq!( control.create_spend(&destinations, &[dummy_op], 0, None), @@ -1624,7 +1288,9 @@ mod tests { // and so we get a coin selection error due to insufficient funds. assert!(matches!( control.create_spend(&destinations, &[], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); let res = control .create_spend(&destinations, &[dummy_op], 1, None) @@ -1658,19 +1324,23 @@ mod tests { // If we ask for a too high feerate, or a too large/too small output, it'll fail. assert!(matches!( control.create_spend(&destinations, &[dummy_op], 10_000, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); *destinations.get_mut(&dummy_addr).unwrap() = 100_001; assert!(matches!( control.create_spend(&destinations, &[dummy_op], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); *destinations.get_mut(&dummy_addr).unwrap() = 4_500; assert_eq!( control.create_spend(&destinations, &[dummy_op], 1, None), - Err(CommandError::InvalidOutputValue(bitcoin::Amount::from_sat( - 4_500 - ))) + Err(CommandError::SpendCreation( + SpendCreationError::InvalidOutputValue(bitcoin::Amount::from_sat(4_500)) + )) ); // If we ask to create an output for an address from another network, it will fail. @@ -1718,7 +1388,9 @@ mod tests { // and so we get a coin selection error due to insufficient funds. assert!(matches!( control.create_spend(&destinations, &[], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); // We'd bail out if they tried to create a transaction with a too high feerate. @@ -1742,8 +1414,8 @@ mod tests { // the sats/vb feerate being lower than `feerate_vb`. assert_eq!( control.create_spend(&destinations, &[dummy_op_dup], 1_003, None), - Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate( - 1_001 + Err(CommandError::SpendCreation(SpendCreationError::InsaneFees( + InsaneFeeInfo::TooHighFeerate(1_001) ))) ); @@ -1768,14 +1440,18 @@ mod tests { // Coin selection error due to insufficient funds. assert!(matches!( control.create_spend(&destinations, &[], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); // Set destination amount equal to value of confirmed coins. *destinations.get_mut(&dummy_addr).unwrap() = 80_000; // Coin selection error occurs due to insufficient funds to pay fee. assert!(matches!( control.create_spend(&destinations, &[], 1, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); let confirmed_op_2 = bitcoin::OutPoint { txid: confirmed_op_1.txid, @@ -1850,7 +1526,9 @@ mod tests { let empty_dest = &HashMap::, u64>::new(); assert!(matches!( control.create_spend(empty_dest, &[confirmed_op_3], 5, None), - Err(CommandError::CoinSelectionError(..)) + Err(CommandError::SpendCreation( + SpendCreationError::CoinSelection(..) + )) )); // If we use a lower fee, the self-send will succeed. let res = control diff --git a/src/commands/utils.rs b/src/commands/utils.rs index d5f5e561a..ce0e26441 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -1,17 +1,8 @@ -use bdk_coin_select::{ - change_policy, metrics::LowestFee, Candidate, CoinSelector, DrainWeights, FeeRate, - InsufficientFunds, Target, TXIN_BASE_WEIGHT, -}; -use log::warn; -use std::{convert::TryInto, str::FromStr}; +use std::str::FromStr; -use miniscript::bitcoin::{self, consensus, constants::WITNESS_SCALE_FACTOR, hashes::hex::FromHex}; +use miniscript::bitcoin::{self, consensus, hashes::hex::FromHex}; use serde::{de, Deserialize, Deserializer, Serializer}; -use crate::database::Coin; - -use super::{CandidateCoin, DUST_OUTPUT_SATS, LONG_TERM_FEERATE_VB}; - pub fn deser_fromstr<'de, D, T>(deserializer: D) -> Result where D: Deserializer<'de>, @@ -71,199 +62,3 @@ where let s = Vec::from_hex(&s).map_err(de::Error::custom)?; consensus::deserialize(&s).map_err(de::Error::custom) } - -/// Metric based on [`LowestFee`] that aims to minimize transaction fees -/// with the additional option to only find solutions with a change output. -/// -/// Using this metric with `must_have_change: false` is equivalent to using -/// [`LowestFee`]. -pub struct LowestFeeChangeCondition<'c, C> { - /// The underlying [`LowestFee`] metric to use. - pub lowest_fee: LowestFee<'c, C>, - /// If `true`, only solutions with change will be found. - pub must_have_change: bool, -} - -impl<'c, C> bdk_coin_select::BnbMetric for LowestFeeChangeCondition<'c, C> -where - for<'a, 'b> C: Fn(&'b CoinSelector<'a>, Target) -> bdk_coin_select::Drain, -{ - fn score(&mut self, cs: &CoinSelector<'_>) -> Option { - let drain = (self.lowest_fee.change_policy)(cs, self.lowest_fee.target); - if drain.is_none() && self.must_have_change { - None - } else { - self.lowest_fee.score(cs) - } - } - - fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { - self.lowest_fee.bound(cs) - } - - fn requires_ordering_by_descending_value_pwu(&self) -> bool { - self.lowest_fee.requires_ordering_by_descending_value_pwu() - } -} - -/// Select coins for spend. -/// -/// Returns the selected coins and the change amount, which could be zero. -/// -/// `candidate_coins` are the coins to consider for selection. -/// -/// `base_tx` is the transaction to select coins for. It should be without any inputs -/// and without a change output, but with all non-change outputs added. -/// -/// `change_txo` is the change output to add if needed (with any value). -/// -/// `feerate_vb` is the minimum feerate (in sats/vb). Note that the selected coins -/// and change may result in a slightly lower feerate than this as the underlying -/// function instead uses a minimum feerate of `feerate_vb / 4.0` sats/wu. -/// -/// `min_fee` is the minimum fee (in sats) that the selection must have. -/// -/// `max_sat_weight` is the maximum weight difference of an input in the -/// transaction before and after satisfaction. -/// -/// `must_have_change` indicates whether the transaction must have a change output. -/// If `true`, the returned change amount will be positive. -pub fn select_coins_for_spend( - candidate_coins: &[CandidateCoin], - base_tx: bitcoin::Transaction, - change_txo: bitcoin::TxOut, - feerate_vb: f32, - min_fee: u64, - max_sat_weight: u32, - must_have_change: bool, -) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { - let out_value_nochange = base_tx.output.iter().map(|o| o.value).sum(); - - // Create the coin selector from the given candidates. NOTE: the coin selector keeps track - // of the original ordering of candidates so we can select any mandatory candidates using their - // original indices. - let base_weight: u32 = base_tx - .weight() - .to_wu() - .try_into() - .expect("Transaction weight must fit in u32"); - let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight; - let candidates: Vec = candidate_coins - .iter() - .map(|cand| Candidate { - input_count: 1, - value: cand.coin.amount.to_sat(), - weight: max_input_weight, - is_segwit: true, // We only support receiving on Segwit scripts. - }) - .collect(); - let mut selector = CoinSelector::new(&candidates, base_weight); - for (i, cand) in candidate_coins.iter().enumerate() { - if cand.must_select { - // It's fine because the index passed to `select` refers to the original candidates ordering - // (and in any case the ordering of candidates is still the same in the coin selector). - selector.select(i); - } - } - - // Now set the change policy. We use a policy which ensures no change output is created with a - // lower value than our custom dust limit. NOTE: the change output weight must account for a - // potential difference in the size of the outputs count varint. This is why we take the whole - // change txo as argument and compute the weight difference below. - let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB); - let drain_weights = DrainWeights { - output_weight: { - let mut tx_with_change = base_tx; - tx_with_change.output.push(change_txo); - tx_with_change - .weight() - .to_wu() - .checked_sub(base_weight.into()) - .expect("base_weight can't be larger") - .try_into() - .expect("tx size must always fit in u32") - }, - spend_weight: max_input_weight, - }; - let change_policy = - change_policy::min_value_and_waste(drain_weights, DUST_OUTPUT_SATS, long_term_feerate); - - // Finally, run the coin selection algorithm. We use a BnB with 100k iterations and if it - // couldn't find any solution we fall back to selecting coins by descending value. - let target = Target { - value: out_value_nochange, - feerate: FeeRate::from_sat_per_vb(feerate_vb), - min_fee, - }; - let lowest_fee = LowestFee { - target, - long_term_feerate, - change_policy: &change_policy, - }; - let lowest_fee_change_cond = LowestFeeChangeCondition { - lowest_fee, - must_have_change, - }; - if let Err(e) = selector.run_bnb(lowest_fee_change_cond, 100_000) { - warn!( - "Coin selection error: '{}'. Selecting coins by descending value per weight unit...", - e.to_string() - ); - selector.sort_candidates_by_descending_value_pwu(); - // Select more coins until target is met and change condition satisfied. - loop { - let drain = change_policy(&selector, target); - if selector.is_target_met(target, drain) && (drain.is_some() || !must_have_change) { - break; - } - if !selector.select_next() { - // If the solution must have change, we calculate how much is missing from the current - // selection in order for there to be a change output with the smallest possible value. - let drain = if must_have_change { - bdk_coin_select::Drain { - weights: drain_weights, - value: DUST_OUTPUT_SATS, - } - } else { - drain - }; - let missing = selector.excess(target, drain).unsigned_abs(); - return Err(InsufficientFunds { missing }); - } - } - } - // By now, selection is complete and we can check how much change to give according to our policy. - let drain = change_policy(&selector, target); - let change_amount = bitcoin::Amount::from_sat(drain.value); - Ok(( - selector - .selected_indices() - .iter() - .map(|i| candidate_coins[*i].coin) - .collect(), - change_amount, - )) -} - -/// An unsigned transaction's maximum possible size in vbytes after satisfaction. -/// -/// This assumes all inputs are internal (or have the same `max_sat_weight` value). -/// -/// `tx` is the unsigned transaction. -/// -/// `max_sat_weight` is the maximum weight difference of an input in the -/// transaction before and after satisfaction. Must be in weight units. -pub fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> u64 { - let witness_factor: u64 = WITNESS_SCALE_FACTOR.try_into().unwrap(); - let num_inputs: u64 = tx.input.len().try_into().unwrap(); - let tx_wu: u64 = tx - .weight() - .to_wu() - .checked_add(max_sat_weight.checked_mul(num_inputs).unwrap()) - .unwrap(); - tx_wu - .checked_add(witness_factor.checked_sub(1).unwrap()) - .unwrap() - .checked_div(witness_factor) - .unwrap() -} diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index bf0b92203..e21ac0e71 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -157,9 +157,8 @@ impl From for Error { | commands::CommandError::AlreadySpent(..) | commands::CommandError::ImmatureCoinbase(..) | commands::CommandError::Address(..) - | commands::CommandError::InvalidOutputValue(..) + | commands::CommandError::SpendCreation(..) | commands::CommandError::InsufficientFunds(..) - | commands::CommandError::InsaneFees(..) | commands::CommandError::UnknownSpend(..) | commands::CommandError::SpendFinalization(..) | commands::CommandError::InsaneRescanTimestamp(..) @@ -169,10 +168,7 @@ impl From for Error { | commands::CommandError::RecoveryNotAvailable => { Error::new(ErrorCode::InvalidParams, e.to_string()) } - commands::CommandError::FetchingTransaction(..) - | commands::CommandError::SanityCheckFailure(_) - | commands::CommandError::CoinSelectionError(..) - | commands::CommandError::RescanTrigger(..) => { + commands::CommandError::RescanTrigger(..) => { Error::new(ErrorCode::InternalError, e.to_string()) } commands::CommandError::TxBroadcast(_) => { diff --git a/src/lib.rs b/src/lib.rs index 5f58e04fc..8d4068f33 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,6 +9,7 @@ pub mod descriptors; mod jsonrpc; mod random; pub mod signer; +mod spend; #[cfg(test)] mod testutils; diff --git a/src/spend.rs b/src/spend.rs new file mode 100644 index 000000000..1b10817e5 --- /dev/null +++ b/src/spend.rs @@ -0,0 +1,616 @@ +use crate::{ + bitcoin::BitcoinInterface, + database::{Coin, DatabaseConnection}, + descriptors, +}; + +use std::{ + collections::{ + hash_map::{self, HashMap}, + BTreeMap, + }, + convert::TryInto, + fmt, sync, +}; + +pub use bdk_coin_select::InsufficientFunds; +use bdk_coin_select::{ + change_policy, metrics::LowestFee, Candidate, CoinSelector, DrainWeights, FeeRate, Target, + TXIN_BASE_WEIGHT, +}; +use miniscript::bitcoin::{ + self, + absolute::{Height, LockTime}, + bip32, + constants::WITNESS_SCALE_FACTOR, + psbt::{Input as PsbtIn, Output as PsbtOut, Psbt}, + secp256k1, +}; + +/// We would never create a transaction with an output worth less than this. +/// That's 1$ at 20_000$ per BTC. +pub const DUST_OUTPUT_SATS: u64 = 5_000; + +/// Long-term feerate (sats/vb) used for coin selection considerations. +pub const LONG_TERM_FEERATE_VB: f32 = 10.0; + +/// Assume that paying more than 1BTC in fee is a bug. +pub const MAX_FEE: u64 = bitcoin::blockdata::constants::COIN_VALUE; + +/// Assume that paying more than 1000sat/vb in feerate is a bug. +pub const MAX_FEERATE: u64 = 1_000; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InsaneFeeInfo { + NegativeFee, + InvalidFeerate, + TooHighFee(u64), + TooHighFeerate(u64), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SpendCreationError { + InvalidFeerate(/* sats/vb */ u64), + InvalidOutputValue(bitcoin::Amount), + InsaneFees(InsaneFeeInfo), + SanityCheckFailure(Psbt), + FetchingTransaction(bitcoin::OutPoint), + CoinSelection(InsufficientFunds), +} + +impl fmt::Display for SpendCreationError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::InvalidFeerate(sats_vb) => write!(f, "Invalid feerate: {} sats/vb.", sats_vb), + Self::InvalidOutputValue(amount) => write!(f, "Invalid output value '{}'.", amount), + Self::InsaneFees(info) => write!( + f, + "We assume transactions with a fee larger than {} sats or a feerate larger than {} sats/vb are a mistake. \ + The created transaction {}.", + MAX_FEE, + MAX_FEERATE, + match info { + InsaneFeeInfo::NegativeFee => "would have a negative fee".to_string(), + InsaneFeeInfo::TooHighFee(f) => format!("{} sats in fees", f), + InsaneFeeInfo::InvalidFeerate => "would have an invalid feerate".to_string(), + InsaneFeeInfo::TooHighFeerate(r) => format!("has a feerate of {} sats/vb", r), + }, + ), + Self::FetchingTransaction(op) => { + write!(f, "Could not fetch transaction for coin {}", op) + } + Self::CoinSelection(e) => write!(f, "Coin selection error: '{}'", e), + Self::SanityCheckFailure(psbt) => write!( + f, + "BUG! Please report this. Failed sanity checks for PSBT '{}'.", + psbt + ), + } + } +} + +impl std::error::Error for SpendCreationError {} + +// Sanity check the value of a transaction output. +pub fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationError> { + // NOTE: the network parameter isn't used upstream + if value.to_sat() > bitcoin::blockdata::constants::MAX_MONEY + || value.to_sat() < DUST_OUTPUT_SATS + { + Err(SpendCreationError::InvalidOutputValue(value)) + } else { + Ok(()) + } +} + +// Apply some sanity checks on a created transaction's PSBT. +// TODO: add more sanity checks from revault_tx +pub fn sanity_check_psbt( + spent_desc: &descriptors::LianaDescriptor, + psbt: &Psbt, +) -> Result<(), SpendCreationError> { + let tx = &psbt.unsigned_tx; + + // Must have as many in/out in the PSBT and Bitcoin tx. + if psbt.inputs.len() != tx.input.len() + || psbt.outputs.len() != tx.output.len() + || tx.output.is_empty() + { + return Err(SpendCreationError::SanityCheckFailure(psbt.clone())); + } + + // Compute the transaction input value, checking all PSBT inputs have the derivation + // index set for signing devices to recognize them as ours. + let mut value_in = 0; + for psbtin in psbt.inputs.iter() { + if psbtin.bip32_derivation.is_empty() { + return Err(SpendCreationError::SanityCheckFailure(psbt.clone())); + } + value_in += psbtin + .witness_utxo + .as_ref() + .ok_or_else(|| SpendCreationError::SanityCheckFailure(psbt.clone()))? + .value; + } + + // Compute the output value and check the absolute fee isn't insane. + let value_out: u64 = tx.output.iter().map(|o| o.value).sum(); + let abs_fee = value_in + .checked_sub(value_out) + .ok_or(SpendCreationError::InsaneFees(InsaneFeeInfo::NegativeFee))?; + if abs_fee > MAX_FEE { + return Err(SpendCreationError::InsaneFees(InsaneFeeInfo::TooHighFee( + abs_fee, + ))); + } + + // Check the feerate isn't insane. + // Add weights together before converting to vbytes to avoid rounding up multiple times + // and increasing the result, which could lead to the feerate in sats/vb falling below 1. + let tx_wu = tx.weight().to_wu() + (spent_desc.max_sat_weight() * tx.input.len()) as u64; + let tx_vb = tx_wu + .checked_add(descriptors::WITNESS_FACTOR as u64 - 1) + .unwrap() + .checked_div(descriptors::WITNESS_FACTOR as u64) + .unwrap(); + let feerate_sats_vb = abs_fee + .checked_div(tx_vb) + .ok_or(SpendCreationError::InsaneFees( + InsaneFeeInfo::InvalidFeerate, + ))?; + if !(1..=MAX_FEERATE).contains(&feerate_sats_vb) { + return Err(SpendCreationError::InsaneFees( + InsaneFeeInfo::TooHighFeerate(feerate_sats_vb), + )); + } + + // Check for dust outputs + for txo in psbt.unsigned_tx.output.iter() { + if txo.value < txo.script_pubkey.dust_value().to_sat() { + return Err(SpendCreationError::SanityCheckFailure(psbt.clone())); + } + } + + Ok(()) +} + +/// A candidate for coin selection when creating a transaction. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct CandidateCoin { + /// The candidate coin. + pub coin: Coin, + /// Whether or not this coin must be selected by the coin selection algorithm. + pub must_select: bool, +} + +/// Metric based on [`LowestFee`] that aims to minimize transaction fees +/// with the additional option to only find solutions with a change output. +/// +/// Using this metric with `must_have_change: false` is equivalent to using +/// [`LowestFee`]. +pub struct LowestFeeChangeCondition<'c, C> { + /// The underlying [`LowestFee`] metric to use. + pub lowest_fee: LowestFee<'c, C>, + /// If `true`, only solutions with change will be found. + pub must_have_change: bool, +} + +impl<'c, C> bdk_coin_select::BnbMetric for LowestFeeChangeCondition<'c, C> +where + for<'a, 'b> C: Fn(&'b CoinSelector<'a>, Target) -> bdk_coin_select::Drain, +{ + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { + let drain = (self.lowest_fee.change_policy)(cs, self.lowest_fee.target); + if drain.is_none() && self.must_have_change { + None + } else { + self.lowest_fee.score(cs) + } + } + + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { + self.lowest_fee.bound(cs) + } + + fn requires_ordering_by_descending_value_pwu(&self) -> bool { + self.lowest_fee.requires_ordering_by_descending_value_pwu() + } +} + +/// Select coins for spend. +/// +/// Returns the selected coins and the change amount, which could be zero. +/// +/// `candidate_coins` are the coins to consider for selection. +/// +/// `base_tx` is the transaction to select coins for. It should be without any inputs +/// and without a change output, but with all non-change outputs added. +/// +/// `change_txo` is the change output to add if needed (with any value). +/// +/// `feerate_vb` is the minimum feerate (in sats/vb). Note that the selected coins +/// and change may result in a slightly lower feerate than this as the underlying +/// function instead uses a minimum feerate of `feerate_vb / 4.0` sats/wu. +/// +/// `min_fee` is the minimum fee (in sats) that the selection must have. +/// +/// `max_sat_weight` is the maximum weight difference of an input in the +/// transaction before and after satisfaction. +/// +/// `must_have_change` indicates whether the transaction must have a change output. +/// If `true`, the returned change amount will be positive. +pub fn select_coins_for_spend( + candidate_coins: &[CandidateCoin], + base_tx: bitcoin::Transaction, + change_txo: bitcoin::TxOut, + feerate_vb: f32, + min_fee: u64, + max_sat_weight: u32, + must_have_change: bool, +) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { + let out_value_nochange = base_tx.output.iter().map(|o| o.value).sum(); + + // Create the coin selector from the given candidates. NOTE: the coin selector keeps track + // of the original ordering of candidates so we can select any mandatory candidates using their + // original indices. + let base_weight: u32 = base_tx + .weight() + .to_wu() + .try_into() + .expect("Transaction weight must fit in u32"); + let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight; + let candidates: Vec = candidate_coins + .iter() + .map(|cand| Candidate { + input_count: 1, + value: cand.coin.amount.to_sat(), + weight: max_input_weight, + is_segwit: true, // We only support receiving on Segwit scripts. + }) + .collect(); + let mut selector = CoinSelector::new(&candidates, base_weight); + for (i, cand) in candidate_coins.iter().enumerate() { + if cand.must_select { + // It's fine because the index passed to `select` refers to the original candidates ordering + // (and in any case the ordering of candidates is still the same in the coin selector). + selector.select(i); + } + } + + // Now set the change policy. We use a policy which ensures no change output is created with a + // lower value than our custom dust limit. NOTE: the change output weight must account for a + // potential difference in the size of the outputs count varint. This is why we take the whole + // change txo as argument and compute the weight difference below. + let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB); + let drain_weights = DrainWeights { + output_weight: { + let mut tx_with_change = base_tx; + tx_with_change.output.push(change_txo); + tx_with_change + .weight() + .to_wu() + .checked_sub(base_weight.into()) + .expect("base_weight can't be larger") + .try_into() + .expect("tx size must always fit in u32") + }, + spend_weight: max_input_weight, + }; + let change_policy = + change_policy::min_value_and_waste(drain_weights, DUST_OUTPUT_SATS, long_term_feerate); + + // Finally, run the coin selection algorithm. We use a BnB with 100k iterations and if it + // couldn't find any solution we fall back to selecting coins by descending value. + let target = Target { + value: out_value_nochange, + feerate: FeeRate::from_sat_per_vb(feerate_vb), + min_fee, + }; + let lowest_fee = LowestFee { + target, + long_term_feerate, + change_policy: &change_policy, + }; + let lowest_fee_change_cond = LowestFeeChangeCondition { + lowest_fee, + must_have_change, + }; + if let Err(e) = selector.run_bnb(lowest_fee_change_cond, 100_000) { + log::warn!( + "Coin selection error: '{}'. Selecting coins by descending value per weight unit...", + e.to_string() + ); + selector.sort_candidates_by_descending_value_pwu(); + // Select more coins until target is met and change condition satisfied. + loop { + let drain = change_policy(&selector, target); + if selector.is_target_met(target, drain) && (drain.is_some() || !must_have_change) { + break; + } + if !selector.select_next() { + // If the solution must have change, we calculate how much is missing from the current + // selection in order for there to be a change output with the smallest possible value. + let drain = if must_have_change { + bdk_coin_select::Drain { + weights: drain_weights, + value: DUST_OUTPUT_SATS, + } + } else { + drain + }; + let missing = selector.excess(target, drain).unsigned_abs(); + return Err(InsufficientFunds { missing }); + } + } + } + // By now, selection is complete and we can check how much change to give according to our policy. + let drain = change_policy(&selector, target); + let change_amount = bitcoin::Amount::from_sat(drain.value); + Ok(( + selector + .selected_indices() + .iter() + .map(|i| candidate_coins[*i].coin) + .collect(), + change_amount, + )) +} + +// Get the derived descriptor for this coin +fn derived_desc( + secp: &secp256k1::Secp256k1, + desc: &descriptors::LianaDescriptor, + coin: &Coin, +) -> descriptors::DerivedSinglePathLianaDesc { + let desc = if coin.is_change { + desc.change_descriptor() + } else { + desc.receive_descriptor() + }; + desc.derive(coin.derivation_index, secp) +} + +/// An unsigned transaction's maximum possible size in vbytes after satisfaction. +/// +/// This assumes all inputs are internal (or have the same `max_sat_weight` value). +/// +/// `tx` is the unsigned transaction. +/// +/// `max_sat_weight` is the maximum weight difference of an input in the +/// transaction before and after satisfaction. Must be in weight units. +pub fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> u64 { + let witness_factor: u64 = WITNESS_SCALE_FACTOR.try_into().unwrap(); + let num_inputs: u64 = tx.input.len().try_into().unwrap(); + let tx_wu: u64 = tx + .weight() + .to_wu() + .checked_add(max_sat_weight.checked_mul(num_inputs).unwrap()) + .unwrap(); + tx_wu + .checked_add(witness_factor.checked_sub(1).unwrap()) + .unwrap() + .checked_div(witness_factor) + .unwrap() +} + +pub fn create_spend( + db_conn: &mut Box, + main_descriptor: &descriptors::LianaDescriptor, + secp: &secp256k1::Secp256k1, + bitcoin: &sync::Arc>, + network: bitcoin::Network, + destinations: &HashMap, + candidate_coins: &[CandidateCoin], + feerate_vb: u64, + min_fee: u64, + change_address: Option, +) -> Result { + // This method is a bit convoluted, but it's the nature of creating a Bitcoin transaction + // with a target feerate and outputs. In addition, we support different modes (coin control + // vs automated coin selection, self-spend, sweep, etc..) which make the logic a bit more + // intricate. Here is a brief overview of what we're doing here: + // 1. Create a transaction with all the target outputs (if this is a self-send, none are + // added at this step the only output will be added as a change output). + // 2. Automatically select the coins if necessary and determine whether a change output + // will be necessary for this transaction from the set of (automatically or manually) + // selected coins. The output for a self-send is added there. + // The change output is also (ab)used to implement a "sweep" functionality. We allow to + // set it to an external address to send all the inputs' value minus the fee and the + // other output's value to a specific, external, address. + // 3. Add the selected coins as inputs to the transaction. + // 4. Finalize the PSBT and sanity check it before returning it. + + let is_self_send = destinations.is_empty(); + if feerate_vb < 1 { + return Err(SpendCreationError::InvalidFeerate(feerate_vb)); + } + + // Create transaction with no inputs and no outputs. + let mut tx = bitcoin::Transaction { + version: 2, + lock_time: LockTime::Blocks(Height::ZERO), // TODO: randomized anti fee sniping + input: Vec::with_capacity(candidate_coins.iter().filter(|c| c.must_select).count()), + output: Vec::with_capacity(destinations.len()), + }; + // Add the destinations outputs to the transaction and PSBT. At the same time + // sanity check each output's value. + let mut psbt_outs = Vec::with_capacity(destinations.len()); + for (address, &amount) in destinations { + check_output_value(amount)?; + + tx.output.push(bitcoin::TxOut { + value: amount.to_sat(), + script_pubkey: address.script_pubkey(), + }); + // If it's an address of ours, signal it as change to signing devices by adding the + // BIP32 derivation path to the PSBT output. + let bip32_derivation = + if let Some((index, is_change)) = db_conn.derivation_index_by_address(address) { + let desc = if is_change { + main_descriptor.change_descriptor() + } else { + main_descriptor.receive_descriptor() + }; + desc.derive(index, secp).bip32_derivations() + } else { + Default::default() + }; + psbt_outs.push(PsbtOut { + bip32_derivation, + ..PsbtOut::default() + }); + } + assert_eq!(tx.output.is_empty(), is_self_send); + + // Now compute whether we'll need a change output while automatically selecting coins to be + // used as input if necessary. + // We need to get the size of a potential change output to select coins / determine whether + // we should include one, so get the change address and create a dummy txo for this purpose. + // The change address may be externally specified for the purpose of a "sweep": the user + // would set the value of some outputs (or none) and fill-in an address to be used for "all + // the rest". This is the same logic as for a change output, except it's external. + struct InternalChangeInfo { + pub desc: descriptors::DerivedSinglePathLianaDesc, + pub index: bip32::ChildNumber, + } + let (change_addr, int_change_info) = if let Some(addr) = change_address { + (addr, None) + } else { + let index = db_conn.change_index(); + let desc = main_descriptor.change_descriptor().derive(index, secp); + ( + desc.address(network), + Some(InternalChangeInfo { desc, index }), + ) + }; + let mut change_txo = bitcoin::TxOut { + value: std::u64::MAX, + script_pubkey: change_addr.script_pubkey(), + }; + // Now select the coins necessary using the provided candidates and determine whether + // there is any leftover to create a change output. + let (selected_coins, change_amount) = { + // At this point the transaction still has no input and no change output, as expected + // by the coins selection helper function. + assert!(tx.input.is_empty()); + assert_eq!(tx.output.len(), destinations.len()); + // TODO: Introduce general conversion error type. + let feerate_vb: f32 = { + let fr: u16 = feerate_vb.try_into().map_err(|_| { + SpendCreationError::InsaneFees(InsaneFeeInfo::TooHighFeerate(feerate_vb)) + })?; + fr + } + .try_into() + .expect("u16 must fit in f32"); + let max_sat_wu = main_descriptor + .max_sat_weight() + .try_into() + .expect("Weight must fit in a u32"); + select_coins_for_spend( + candidate_coins, + tx.clone(), + change_txo.clone(), + feerate_vb, + min_fee, + max_sat_wu, + is_self_send, + ) + .map_err(SpendCreationError::CoinSelection)? + }; + // If necessary, add a change output. + // For a self-send, coin selection will only find solutions with change and will otherwise + // return an error. In any case, the PSBT sanity check will catch a transaction with no outputs. + if change_amount.to_sat() > 0 { + check_output_value(change_amount)?; + + // If we generated a change address internally, set the BIP32 derivations in the PSBT + // output to tell the signers it's an internal address and make sure to update our next + // change index. Otherwise it's a sweep, so no need to set anything. + // If the change address was set by the caller, check whether it's one of ours. If it + // is, set the BIP32 derivations accordingly. In addition, if it's a change address for + // a later index than we currently have set as next change derivation index, update it. + let bip32_derivation = if let Some(InternalChangeInfo { desc, index }) = int_change_info { + let next_index = index + .increment() + .expect("Must not get into hardened territory"); + db_conn.set_change_index(next_index, secp); + desc.bip32_derivations() + } else if let Some((index, is_change)) = db_conn.derivation_index_by_address(&change_addr) { + let desc = if is_change { + if db_conn.change_index() < index { + let next_index = index + .increment() + .expect("Must not get into hardened territory"); + db_conn.set_change_index(next_index, secp); + } + main_descriptor.change_descriptor() + } else { + main_descriptor.receive_descriptor() + }; + desc.derive(index, secp).bip32_derivations() + } else { + Default::default() + }; + + // TODO: shuffle once we have Taproot + change_txo.value = change_amount.to_sat(); + tx.output.push(change_txo); + psbt_outs.push(PsbtOut { + bip32_derivation, + ..PsbtOut::default() + }); + } + + // Iterate through selected coins and add necessary information to the PSBT inputs. + let mut psbt_ins = Vec::with_capacity(selected_coins.len()); + let mut spent_txs = HashMap::with_capacity(selected_coins.len()); + for coin in &selected_coins { + // Fetch the transaction that created it if necessary + if let hash_map::Entry::Vacant(e) = spent_txs.entry(coin.outpoint) { + let tx = bitcoin + .wallet_transaction(&coin.outpoint.txid) + .ok_or(SpendCreationError::FetchingTransaction(coin.outpoint))?; + e.insert(tx.0); + } + + tx.input.push(bitcoin::TxIn { + previous_output: coin.outpoint, + sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, + // TODO: once we move to Taproot, anti-fee-sniping using nSequence + ..bitcoin::TxIn::default() + }); + + // Populate the PSBT input with the information needed by signers. + let coin_desc = derived_desc(secp, main_descriptor, coin); + let witness_script = Some(coin_desc.witness_script()); + let witness_utxo = Some(bitcoin::TxOut { + value: coin.amount.to_sat(), + script_pubkey: coin_desc.script_pubkey(), + }); + let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); + let bip32_derivation = coin_desc.bip32_derivations(); + psbt_ins.push(PsbtIn { + witness_script, + witness_utxo, + bip32_derivation, + non_witness_utxo, + ..PsbtIn::default() + }); + } + + // Finally, create the PSBT with all inputs and outputs, sanity check it and return it. + let psbt = Psbt { + unsigned_tx: tx, + version: 0, + xpub: BTreeMap::new(), + proprietary: BTreeMap::new(), + unknown: BTreeMap::new(), + inputs: psbt_ins, + outputs: psbt_outs, + }; + sanity_check_psbt(main_descriptor, &psbt)?; + // TODO: maybe check for common standardness rules (max size, ..)? + + Ok(psbt) +} From 22f97e11b7ab772d30ca9ccd992c9092559ab493 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 10:14:09 +0100 Subject: [PATCH 02/14] spend: let caller update next derivation index This is a first step toward removing the database accesses from the spend PSBT creation helper. It now always take a change address, and return whether it used it. If it did the caller retrieves the information about the change address and if necessary bumps the next derivation index to use. --- src/commands/mod.rs | 90 ++++++++++++++++++++++++++++++++++----------- src/spend.rs | 72 ++++++++++++------------------------ 2 files changed, 92 insertions(+), 70 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index ffb801c9b..e06e5f109 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -6,11 +6,11 @@ mod utils; use crate::{ bitcoin::BitcoinInterface, - database::{Coin, DatabaseInterface}, + database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, spend::{ check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, CandidateCoin, - SpendCreationError, + CreateSpendRes, SpendCreationError, }, DaemonControl, VERSION, }; @@ -175,6 +175,28 @@ impl DaemonControl { addr.require_network(self.config.bitcoin_config.network) .map_err(CommandError::Address) } + + // If we detect the given address as ours, and it has a higher derivation index than our next + // derivation index, update our next derivation index to the one after the address'. + fn maybe_increase_next_deriv_index( + &self, + db_conn: &mut Box, + addr: &bitcoin::Address, + ) { + if let Some((index, is_change)) = db_conn.derivation_index_by_address(addr) { + if is_change && db_conn.change_index() < index { + let next_index = index + .increment() + .expect("Must not get into hardened territory"); + db_conn.set_change_index(next_index, &self.secp); + } else if !is_change && db_conn.receive_index() < index { + let next_index = index + .increment() + .expect("Must not get into hardened territory"); + db_conn.set_receive_index(next_index, &self.secp); + } + } + } } impl DaemonControl { @@ -340,10 +362,23 @@ impl DaemonControl { check_output_value(amount)?; destinations_checked.insert(address, amount); } - // Check also the change address if one has been given. + + // The change address to be used if a change output needs to be created. It may be + // specified by the caller (for instance for the purpose of a sweep, or to avoid us + // creating a new change address on every call). let change_address = change_address .map(|addr| self.validate_address(addr)) - .transpose()?; + .transpose()? + .unwrap_or_else(|| { + let index = db_conn.change_index(); + let desc = self + .config + .main_descriptor + .change_descriptor() + .derive(index, &self.secp); + desc.address(self.config.bitcoin_config.network) + }); + // The candidate coins will be either all optional or all mandatory. // If no coins have been specified, then coins will be selected automatically for // the spend from a set of optional candidates. @@ -380,20 +415,25 @@ impl DaemonControl { .collect() }; - Ok(CreateSpendResult { - psbt: create_spend( - &mut db_conn, - &self.config.main_descriptor, - &self.secp, - &self.bitcoin, - self.config.bitcoin_config.network, - &destinations_checked, - &candidate_coins, - feerate_vb, - 0, // No min fee required. - change_address, - )?, - }) + // Create the PSBT. If there was no error in doing so make sure to update our next + // derivation index in case the change address which we generated or was provided to us was + // for a future derivation index. + let CreateSpendRes { psbt, has_change } = create_spend( + &mut db_conn, + &self.config.main_descriptor, + &self.secp, + &self.bitcoin, + &destinations_checked, + &candidate_coins, + feerate_vb, + 0, // No min fee required. + change_address.clone(), + )?; + if has_change { + self.maybe_increase_next_deriv_index(&mut db_conn, &change_address); + } + + Ok(CreateSpendResult { psbt }) } pub fn update_spend(&self, mut psbt: Psbt) -> Result<(), CommandError> { @@ -706,17 +746,19 @@ impl DaemonControl { let mut replacement_vsize = 0; for incremental_feerate in 0.. { let min_fee = descendant_fees.to_sat() + replacement_vsize * incremental_feerate; - let rbf_psbt = match create_spend( + let CreateSpendRes { + psbt: rbf_psbt, + has_change, + } = match create_spend( &mut db_conn, &self.config.main_descriptor, &self.secp, &self.bitcoin, - self.config.bitcoin_config.network, &destinations, &candidate_coins, feerate_vb, min_fee, - Some(change_address.clone()), + change_address.clone(), ) { Ok(psbt) => psbt, // If we get a coin selection error due to insufficient funds and we want to cancel the @@ -741,6 +783,12 @@ impl DaemonControl { if rbf_psbt.fee().expect("has already been sanity checked") >= descendant_fees + bitcoin::Amount::from_sat(replacement_vsize) { + // In case of success, make sure to update our next derivation index if the change + // address used was from the future. + if has_change { + self.maybe_increase_next_deriv_index(&mut db_conn, &change_address); + } + return Ok(CreateSpendResult { psbt: rbf_psbt }); } } diff --git a/src/spend.rs b/src/spend.rs index 1b10817e5..bec620e47 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -21,7 +21,6 @@ use bdk_coin_select::{ use miniscript::bitcoin::{ self, absolute::{Height, LockTime}, - bip32, constants::WITNESS_SCALE_FACTOR, psbt::{Input as PsbtIn, Output as PsbtOut, Psbt}, secp256k1, @@ -393,18 +392,24 @@ pub fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> .unwrap() } +pub struct CreateSpendRes { + /// The created PSBT. + pub psbt: Psbt, + /// Whether the created PSBT has a change output. + pub has_change: bool, +} + pub fn create_spend( db_conn: &mut Box, main_descriptor: &descriptors::LianaDescriptor, secp: &secp256k1::Secp256k1, bitcoin: &sync::Arc>, - network: bitcoin::Network, destinations: &HashMap, candidate_coins: &[CandidateCoin], feerate_vb: u64, min_fee: u64, - change_address: Option, -) -> Result { + change_addr: bitcoin::Address, +) -> Result { // This method is a bit convoluted, but it's the nature of creating a Bitcoin transaction // with a target feerate and outputs. In addition, we support different modes (coin control // vs automated coin selection, self-spend, sweep, etc..) which make the logic a bit more @@ -466,23 +471,6 @@ pub fn create_spend( // used as input if necessary. // We need to get the size of a potential change output to select coins / determine whether // we should include one, so get the change address and create a dummy txo for this purpose. - // The change address may be externally specified for the purpose of a "sweep": the user - // would set the value of some outputs (or none) and fill-in an address to be used for "all - // the rest". This is the same logic as for a change output, except it's external. - struct InternalChangeInfo { - pub desc: descriptors::DerivedSinglePathLianaDesc, - pub index: bip32::ChildNumber, - } - let (change_addr, int_change_info) = if let Some(addr) = change_address { - (addr, None) - } else { - let index = db_conn.change_index(); - let desc = main_descriptor.change_descriptor().derive(index, secp); - ( - desc.address(network), - Some(InternalChangeInfo { desc, index }), - ) - }; let mut change_txo = bitcoin::TxOut { value: std::u64::MAX, script_pubkey: change_addr.script_pubkey(), @@ -521,37 +509,23 @@ pub fn create_spend( // If necessary, add a change output. // For a self-send, coin selection will only find solutions with change and will otherwise // return an error. In any case, the PSBT sanity check will catch a transaction with no outputs. - if change_amount.to_sat() > 0 { + let has_change = change_amount.to_sat() > 0; + if has_change { check_output_value(change_amount)?; - // If we generated a change address internally, set the BIP32 derivations in the PSBT - // output to tell the signers it's an internal address and make sure to update our next - // change index. Otherwise it's a sweep, so no need to set anything. - // If the change address was set by the caller, check whether it's one of ours. If it - // is, set the BIP32 derivations accordingly. In addition, if it's a change address for - // a later index than we currently have set as next change derivation index, update it. - let bip32_derivation = if let Some(InternalChangeInfo { desc, index }) = int_change_info { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_change_index(next_index, secp); - desc.bip32_derivations() - } else if let Some((index, is_change)) = db_conn.derivation_index_by_address(&change_addr) { - let desc = if is_change { - if db_conn.change_index() < index { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_change_index(next_index, secp); - } - main_descriptor.change_descriptor() + // If the change address is ours, tell the signers by setting the BIP32 derivations in the + // PSBT output. + let bip32_derivation = + if let Some((index, is_change)) = db_conn.derivation_index_by_address(&change_addr) { + let desc = if is_change { + main_descriptor.change_descriptor() + } else { + main_descriptor.receive_descriptor() + }; + desc.derive(index, secp).bip32_derivations() } else { - main_descriptor.receive_descriptor() + Default::default() }; - desc.derive(index, secp).bip32_derivations() - } else { - Default::default() - }; // TODO: shuffle once we have Taproot change_txo.value = change_amount.to_sat(); @@ -612,5 +586,5 @@ pub fn create_spend( sanity_check_psbt(main_descriptor, &psbt)?; // TODO: maybe check for common standardness rules (max size, ..)? - Ok(psbt) + Ok(CreateSpendRes { psbt, has_change }) } From 7c238124bebf38fc93994b183de90c2baf0cb0c3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 11:03:57 +0100 Subject: [PATCH 03/14] spend: don't access the database in the PSBT creation function Instead, pass the address details, if known, as a parameter. --- src/commands/mod.rs | 97 +++++++++++++++++++++++++++++---------------- src/spend.rs | 66 ++++++++++++++++-------------- 2 files changed, 99 insertions(+), 64 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index e06e5f109..1579889c9 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -9,8 +9,8 @@ use crate::{ database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, spend::{ - check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, CandidateCoin, - CreateSpendRes, SpendCreationError, + check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, AddrInfo, + CandidateCoin, CreateSpendRes, SpendCreationError, SpendOutputAddress, }, DaemonControl, VERSION, }; @@ -176,20 +176,61 @@ impl DaemonControl { .map_err(CommandError::Address) } + // Get details about this address, if we know about it. + fn addr_info( + &self, + db_conn: &mut Box, + addr: &bitcoin::Address, + ) -> Option { + db_conn + .derivation_index_by_address(addr) + .map(|(index, is_change)| AddrInfo { index, is_change }) + } + + // Create an address to be used in an output of a spend transaction. + fn spend_addr( + &self, + db_conn: &mut Box, + addr: bitcoin::Address, + ) -> SpendOutputAddress { + SpendOutputAddress { + info: self.addr_info(db_conn, &addr), + addr, + } + } + + // Get the change address for the next derivation index. + fn next_change_addr(&self, db_conn: &mut Box) -> SpendOutputAddress { + let index = db_conn.change_index(); + let desc = self + .config + .main_descriptor + .change_descriptor() + .derive(index, &self.secp); + let addr = desc.address(self.config.bitcoin_config.network); + SpendOutputAddress { + addr, + info: Some(AddrInfo { + index, + is_change: true, + }), + } + } + // If we detect the given address as ours, and it has a higher derivation index than our next // derivation index, update our next derivation index to the one after the address'. fn maybe_increase_next_deriv_index( &self, db_conn: &mut Box, - addr: &bitcoin::Address, + addr_info: &Option, ) { - if let Some((index, is_change)) = db_conn.derivation_index_by_address(addr) { - if is_change && db_conn.change_index() < index { + if let Some(AddrInfo { index, is_change }) = addr_info { + if *is_change && db_conn.change_index() < *index { let next_index = index .increment() .expect("Must not get into hardened territory"); db_conn.set_change_index(next_index, &self.secp); - } else if !is_change && db_conn.receive_index() < index { + } else if !is_change && db_conn.receive_index() < *index { let next_index = index .increment() .expect("Must not get into hardened territory"); @@ -355,29 +396,24 @@ impl DaemonControl { // Check the destination addresses are valid for the network and // sanity check each output's value. - let mut destinations_checked = HashMap::with_capacity(destinations.len()); + let mut destinations_checked = Vec::with_capacity(destinations.len()); for (address, value_sat) in destinations { let address = self.validate_address(address.clone())?; let amount = bitcoin::Amount::from_sat(*value_sat); check_output_value(amount)?; - destinations_checked.insert(address, amount); + let address = self.spend_addr(&mut db_conn, address); + destinations_checked.push((address, amount)); } // The change address to be used if a change output needs to be created. It may be // specified by the caller (for instance for the purpose of a sweep, or to avoid us // creating a new change address on every call). let change_address = change_address - .map(|addr| self.validate_address(addr)) + .map(|addr| { + Ok::<_, CommandError>(self.spend_addr(&mut db_conn, self.validate_address(addr)?)) + }) .transpose()? - .unwrap_or_else(|| { - let index = db_conn.change_index(); - let desc = self - .config - .main_descriptor - .change_descriptor() - .derive(index, &self.secp); - desc.address(self.config.bitcoin_config.network) - }); + .unwrap_or_else(|| self.next_change_addr(&mut db_conn)); // The candidate coins will be either all optional or all mandatory. // If no coins have been specified, then coins will be selected automatically for @@ -418,8 +454,8 @@ impl DaemonControl { // Create the PSBT. If there was no error in doing so make sure to update our next // derivation index in case the change address which we generated or was provided to us was // for a future derivation index. + let change_info = change_address.info; let CreateSpendRes { psbt, has_change } = create_spend( - &mut db_conn, &self.config.main_descriptor, &self.secp, &self.bitcoin, @@ -427,10 +463,10 @@ impl DaemonControl { &candidate_coins, feerate_vb, 0, // No min fee required. - change_address.clone(), + change_address, )?; if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &change_address); + self.maybe_increase_next_deriv_index(&mut db_conn, &change_info); } Ok(CreateSpendResult { psbt }) @@ -681,28 +717,22 @@ impl DaemonControl { .into_iter() .filter_map(|(addr, amt, _)| { if prev_change_address.as_ref() != Some(&addr) { - Some((addr, amt)) + Some((self.spend_addr(&mut db_conn, addr), amt)) } else { None } }) .collect() } else { - HashMap::new() + Vec::new() }; // If there was no previous change address, we set the change address for the replacement // to our next change address. This way, we won't increment the change index with each attempt // at creating the replacement PSBT below. - let change_address = prev_change_address.unwrap_or_else(|| { - let index = db_conn.change_index(); - let desc = self - .config - .main_descriptor - .change_descriptor() - .derive(index, &self.secp); - desc.address(self.config.bitcoin_config.network) - }); + let change_address = prev_change_address + .map(|addr| self.spend_addr(&mut db_conn, addr)) + .unwrap_or_else(|| self.next_change_addr(&mut db_conn)); // If `!is_cancel`, we take the previous coins as mandatory candidates and add confirmed coins as optional. // Otherwise, we take the previous coins as optional candidates and let coin selection find the // best solution that includes at least one of these. If there are insufficient funds to create the replacement @@ -750,7 +780,6 @@ impl DaemonControl { psbt: rbf_psbt, has_change, } = match create_spend( - &mut db_conn, &self.config.main_descriptor, &self.secp, &self.bitcoin, @@ -786,7 +815,7 @@ impl DaemonControl { // In case of success, make sure to update our next derivation index if the change // address used was from the future. if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &change_address); + self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); } return Ok(CreateSpendResult { psbt: rbf_psbt }); diff --git a/src/spend.rs b/src/spend.rs index bec620e47..6e3fab520 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -1,8 +1,4 @@ -use crate::{ - bitcoin::BitcoinInterface, - database::{Coin, DatabaseConnection}, - descriptors, -}; +use crate::{bitcoin::BitcoinInterface, database::Coin, descriptors}; use std::{ collections::{ @@ -21,6 +17,7 @@ use bdk_coin_select::{ use miniscript::bitcoin::{ self, absolute::{Height, LockTime}, + bip32, constants::WITNESS_SCALE_FACTOR, psbt::{Input as PsbtIn, Output as PsbtOut, Psbt}, secp256k1, @@ -392,6 +389,18 @@ pub fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> .unwrap() } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct AddrInfo { + pub index: bip32::ChildNumber, + pub is_change: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SpendOutputAddress { + pub addr: bitcoin::Address, + pub info: Option, +} + pub struct CreateSpendRes { /// The created PSBT. pub psbt: Psbt, @@ -400,15 +409,14 @@ pub struct CreateSpendRes { } pub fn create_spend( - db_conn: &mut Box, main_descriptor: &descriptors::LianaDescriptor, secp: &secp256k1::Secp256k1, bitcoin: &sync::Arc>, - destinations: &HashMap, + destinations: &[(SpendOutputAddress, bitcoin::Amount)], candidate_coins: &[CandidateCoin], feerate_vb: u64, min_fee: u64, - change_addr: bitcoin::Address, + change_addr: SpendOutputAddress, ) -> Result { // This method is a bit convoluted, but it's the nature of creating a Bitcoin transaction // with a target feerate and outputs. In addition, we support different modes (coin control @@ -440,26 +448,25 @@ pub fn create_spend( // Add the destinations outputs to the transaction and PSBT. At the same time // sanity check each output's value. let mut psbt_outs = Vec::with_capacity(destinations.len()); - for (address, &amount) in destinations { - check_output_value(amount)?; + for (address, amount) in destinations { + check_output_value(*amount)?; tx.output.push(bitcoin::TxOut { value: amount.to_sat(), - script_pubkey: address.script_pubkey(), + script_pubkey: address.addr.script_pubkey(), }); // If it's an address of ours, signal it as change to signing devices by adding the // BIP32 derivation path to the PSBT output. - let bip32_derivation = - if let Some((index, is_change)) = db_conn.derivation_index_by_address(address) { - let desc = if is_change { - main_descriptor.change_descriptor() - } else { - main_descriptor.receive_descriptor() - }; - desc.derive(index, secp).bip32_derivations() + let bip32_derivation = if let Some(AddrInfo { index, is_change }) = address.info { + let desc = if is_change { + main_descriptor.change_descriptor() } else { - Default::default() + main_descriptor.receive_descriptor() }; + desc.derive(index, secp).bip32_derivations() + } else { + Default::default() + }; psbt_outs.push(PsbtOut { bip32_derivation, ..PsbtOut::default() @@ -473,7 +480,7 @@ pub fn create_spend( // we should include one, so get the change address and create a dummy txo for this purpose. let mut change_txo = bitcoin::TxOut { value: std::u64::MAX, - script_pubkey: change_addr.script_pubkey(), + script_pubkey: change_addr.addr.script_pubkey(), }; // Now select the coins necessary using the provided candidates and determine whether // there is any leftover to create a change output. @@ -515,17 +522,16 @@ pub fn create_spend( // If the change address is ours, tell the signers by setting the BIP32 derivations in the // PSBT output. - let bip32_derivation = - if let Some((index, is_change)) = db_conn.derivation_index_by_address(&change_addr) { - let desc = if is_change { - main_descriptor.change_descriptor() - } else { - main_descriptor.receive_descriptor() - }; - desc.derive(index, secp).bip32_derivations() + let bip32_derivation = if let Some(AddrInfo { index, is_change }) = change_addr.info { + let desc = if is_change { + main_descriptor.change_descriptor() } else { - Default::default() + main_descriptor.receive_descriptor() }; + desc.derive(index, secp).bip32_derivations() + } else { + Default::default() + }; // TODO: shuffle once we have Taproot change_txo.value = change_amount.to_sat(); From 5d5015553239fb19de0890a6956c8e22ebfec0f4 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 11:26:13 +0100 Subject: [PATCH 04/14] spend: avoid direct access to our Bitcoin backend We introduce a trait to get the wallet transaction correponding to the transaction input in order to encapsulate the spend module. --- src/commands/mod.rs | 36 ++++++++++++++++++++++++++++++++---- src/spend.rs | 32 +++++++++++--------------------- 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 1579889c9..5adf50c90 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -10,7 +10,7 @@ use crate::{ descriptors, spend::{ check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, AddrInfo, - CandidateCoin, CreateSpendRes, SpendCreationError, SpendOutputAddress, + CandidateCoin, CreateSpendRes, SpendCreationError, SpendOutputAddress, TxGetter, }, DaemonControl, VERSION, }; @@ -25,7 +25,7 @@ use utils::{ use std::{ collections::{hash_map, BTreeMap, HashMap, HashSet}, convert::TryInto, - fmt, + fmt, sync, }; use miniscript::{ @@ -154,6 +154,32 @@ impl fmt::Display for RbfErrorInfo { } } +/// A wallet transaction getter which fetches the transaction from our Bitcoin backend with a cache +/// to avoid needless redundant calls. Note the cache holds an Option<> so we also avoid redundant +/// calls when the txid isn't known by our Bitcoin backend. +struct BitcoindTxGetter<'a> { + bitcoind: &'a sync::Arc>, + cache: HashMap>, +} + +impl<'a> BitcoindTxGetter<'a> { + pub fn new(bitcoind: &'a sync::Arc>) -> Self { + Self { + bitcoind, + cache: HashMap::new(), + } + } +} + +impl<'a> TxGetter for BitcoindTxGetter<'a> { + fn get_tx(&mut self, txid: &bitcoin::Txid) -> Option { + if let hash_map::Entry::Vacant(entry) = self.cache.entry(*txid) { + entry.insert(self.bitcoind.wallet_transaction(txid).map(|wtx| wtx.0)); + } + self.cache.get(txid).cloned().flatten() + } +} + impl DaemonControl { // Get the derived descriptor for this coin fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc { @@ -393,6 +419,7 @@ impl DaemonControl { return Err(CommandError::InvalidFeerate(feerate_vb)); } let mut db_conn = self.db.connection(); + let mut tx_getter = BitcoindTxGetter::new(&self.bitcoin); // Check the destination addresses are valid for the network and // sanity check each output's value. @@ -458,7 +485,7 @@ impl DaemonControl { let CreateSpendRes { psbt, has_change } = create_spend( &self.config.main_descriptor, &self.secp, - &self.bitcoin, + &mut tx_getter, &destinations_checked, &candidate_coins, feerate_vb, @@ -605,6 +632,7 @@ impl DaemonControl { feerate_vb: Option, ) -> Result { let mut db_conn = self.db.connection(); + let mut tx_getter = BitcoindTxGetter::new(&self.bitcoin); if is_cancel && feerate_vb.is_some() { return Err(CommandError::RbfError(RbfErrorInfo::SuperfluousFeerate)); @@ -782,7 +810,7 @@ impl DaemonControl { } = match create_spend( &self.config.main_descriptor, &self.secp, - &self.bitcoin, + &mut tx_getter, &destinations, &candidate_coins, feerate_vb, diff --git a/src/spend.rs b/src/spend.rs index 6e3fab520..d129713f2 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -1,13 +1,6 @@ -use crate::{bitcoin::BitcoinInterface, database::Coin, descriptors}; - -use std::{ - collections::{ - hash_map::{self, HashMap}, - BTreeMap, - }, - convert::TryInto, - fmt, sync, -}; +use crate::{database::Coin, descriptors}; + +use std::{collections::BTreeMap, convert::TryInto, fmt}; pub use bdk_coin_select::InsufficientFunds; use bdk_coin_select::{ @@ -401,6 +394,12 @@ pub struct SpendOutputAddress { pub info: Option, } +/// A trait for getting a wallet transaction by its txid. +pub trait TxGetter { + /// Get a wallet transaction. Allows for a cache by making the access mutable. + fn get_tx(&mut self, txid: &bitcoin::Txid) -> Option; +} + pub struct CreateSpendRes { /// The created PSBT. pub psbt: Psbt, @@ -411,7 +410,7 @@ pub struct CreateSpendRes { pub fn create_spend( main_descriptor: &descriptors::LianaDescriptor, secp: &secp256k1::Secp256k1, - bitcoin: &sync::Arc>, + tx_getter: &mut impl TxGetter, destinations: &[(SpendOutputAddress, bitcoin::Amount)], candidate_coins: &[CandidateCoin], feerate_vb: u64, @@ -544,16 +543,7 @@ pub fn create_spend( // Iterate through selected coins and add necessary information to the PSBT inputs. let mut psbt_ins = Vec::with_capacity(selected_coins.len()); - let mut spent_txs = HashMap::with_capacity(selected_coins.len()); for coin in &selected_coins { - // Fetch the transaction that created it if necessary - if let hash_map::Entry::Vacant(e) = spent_txs.entry(coin.outpoint) { - let tx = bitcoin - .wallet_transaction(&coin.outpoint.txid) - .ok_or(SpendCreationError::FetchingTransaction(coin.outpoint))?; - e.insert(tx.0); - } - tx.input.push(bitcoin::TxIn { previous_output: coin.outpoint, sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, @@ -568,7 +558,7 @@ pub fn create_spend( value: coin.amount.to_sat(), script_pubkey: coin_desc.script_pubkey(), }); - let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); + let non_witness_utxo = tx_getter.get_tx(&coin.outpoint.txid); let bip32_derivation = coin_desc.bip32_derivations(); psbt_ins.push(PsbtIn { witness_script, From 0523f0047fd463945cdb2147c2dc13c8538bb0d3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 11:34:29 +0100 Subject: [PATCH 05/14] commands: update next deriv index for any spend output address It's basically free to do now, so we might as well do it. --- src/commands/mod.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 5adf50c90..fba812912 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -479,8 +479,8 @@ impl DaemonControl { }; // Create the PSBT. If there was no error in doing so make sure to update our next - // derivation index in case the change address which we generated or was provided to us was - // for a future derivation index. + // derivation index in case any address in the transaction outputs was ours and from the + // future. let change_info = change_address.info; let CreateSpendRes { psbt, has_change } = create_spend( &self.config.main_descriptor, @@ -492,6 +492,9 @@ impl DaemonControl { 0, // No min fee required. change_address, )?; + for (addr, _) in destinations_checked { + self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); + } if has_change { self.maybe_increase_next_deriv_index(&mut db_conn, &change_info); } @@ -840,8 +843,11 @@ impl DaemonControl { if rbf_psbt.fee().expect("has already been sanity checked") >= descendant_fees + bitcoin::Amount::from_sat(replacement_vsize) { - // In case of success, make sure to update our next derivation index if the change - // address used was from the future. + // In case of success, make sure to update our next derivation index if any address + // used in the transaction outputs was from the future. + for (addr, _) in destinations { + self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); + } if has_change { self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); } From 33be1ff18bf3eeda772efaf29439340b30b5c5e9 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 11:48:27 +0100 Subject: [PATCH 06/14] commands: make create_recovery use the create_spend helper Now that we have sweep capability no need to duplicate half the logic, just reuse the helper. --- src/commands/mod.rs | 127 ++++++++++++++------------------------------ src/spend.rs | 16 ++++-- 2 files changed, 51 insertions(+), 92 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index fba812912..5d83ba3b7 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -9,8 +9,8 @@ use crate::{ database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, spend::{ - check_output_value, create_spend, sanity_check_psbt, unsigned_tx_max_vbytes, AddrInfo, - CandidateCoin, CreateSpendRes, SpendCreationError, SpendOutputAddress, TxGetter, + check_output_value, create_spend, unsigned_tx_max_vbytes, AddrInfo, CandidateCoin, + CreateSpendRes, SpendCreationError, SpendOutputAddress, TxGetter, }, DaemonControl, VERSION, }; @@ -23,17 +23,13 @@ use utils::{ }; use std::{ - collections::{hash_map, BTreeMap, HashMap, HashSet}, + collections::{hash_map, HashMap, HashSet}, convert::TryInto, fmt, sync, }; use miniscript::{ - bitcoin::{ - self, address, bip32, - locktime::absolute, - psbt::{Input as PsbtIn, Output as PsbtOut, PartiallySignedTransaction as Psbt}, - }, + bitcoin::{self, address, bip32, psbt::PartiallySignedTransaction as Psbt}, psbt::PsbtExt, }; use serde::{Deserialize, Serialize}; @@ -455,6 +451,7 @@ impl DaemonControl { .map(|c| CandidateCoin { coin: c, must_select: false, // No coin is mandatory. + sequence: None, // No specific nSequence is required. }) .collect() } else { @@ -474,6 +471,7 @@ impl DaemonControl { .map(|c| CandidateCoin { coin: c, must_select: true, // All coins must be selected. + sequence: None, // No specific nSequence is required. }) .collect() }; @@ -773,6 +771,7 @@ impl DaemonControl { .map(|c| CandidateCoin { coin: *c, must_select: !is_cancel, + sequence: None, // No specific nSequence is required. }) .collect(); let confirmed_cands: Vec = db_conn @@ -785,6 +784,7 @@ impl DaemonControl { Some(CandidateCoin { coin: c, must_select: false, + sequence: None, // No specific nSequence is required. }) } else { None @@ -949,27 +949,9 @@ impl DaemonControl { if feerate_vb < 1 { return Err(CommandError::InvalidFeerate(feerate_vb)); } - let address = self.validate_address(address)?; + let mut tx_getter = BitcoindTxGetter::new(&self.bitcoin); let mut db_conn = self.db.connection(); - - // The transaction template. We'll fill-in the inputs afterward. - let mut psbt = Psbt { - unsigned_tx: bitcoin::Transaction { - version: 2, - lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), // TODO: anti-fee sniping - input: Vec::new(), - output: vec![bitcoin::TxOut { - script_pubkey: address.script_pubkey(), - value: 0xFF_FF_FF_FF, - }], - }, - version: 0, - xpub: BTreeMap::new(), - proprietary: BTreeMap::new(), - unknown: BTreeMap::new(), - inputs: Vec::new(), - outputs: vec![PsbtOut::default()], - }; + let sweep_addr = self.spend_addr(&mut db_conn, self.validate_address(address)?); // Query the coins that we can spend through the specified recovery path (if no recovery // path specified, use the first available one) from the database. @@ -977,72 +959,43 @@ impl DaemonControl { let timelock = timelock.unwrap_or_else(|| self.config.main_descriptor.first_timelock_value()); let height_delta: i32 = timelock.try_into().expect("Must fit, it's a u16"); - let sweepable_coins = db_conn + let sweepable_coins: Vec<_> = db_conn .coins(&[CoinStatus::Unconfirmed, CoinStatus::Confirmed], &[]) .into_values() - .filter(|c| { + .filter_map(|c| { // We are interested in coins available at the *next* block - c.block_info + if c.block_info .map(|b| current_height + 1 >= b.height + height_delta) .unwrap_or(false) - }); - - // Fill-in the transaction inputs and PSBT inputs information. Record the value - // that is fed to the transaction while doing so, to compute the fees afterward. - let mut in_value = bitcoin::Amount::from_sat(0); - let txin_sat_vb = self.config.main_descriptor.max_sat_vbytes(); - let mut sat_vb = 1; // Start at 1 for the segwit marker size, rounded up. - let mut spent_txs = HashMap::new(); - for coin in sweepable_coins { - in_value += coin.amount; - psbt.unsigned_tx.input.push(bitcoin::TxIn { - previous_output: coin.outpoint, - sequence: bitcoin::Sequence::from_height(timelock), - // TODO: once we move to Taproot, anti-fee-sniping using nSequence - ..bitcoin::TxIn::default() - }); - - // Fetch the transaction that created this coin if necessary - if let hash_map::Entry::Vacant(e) = spent_txs.entry(coin.outpoint) { - let tx = self - .bitcoin - .wallet_transaction(&coin.outpoint.txid) - .ok_or(SpendCreationError::FetchingTransaction(coin.outpoint))?; - e.insert(tx.0); - } - - let coin_desc = self.derived_desc(&coin); - sat_vb += txin_sat_vb; - let witness_script = Some(coin_desc.witness_script()); - let witness_utxo = Some(bitcoin::TxOut { - value: coin.amount.to_sat(), - script_pubkey: coin_desc.script_pubkey(), - }); - let non_witness_utxo = spent_txs.get(&coin.outpoint).cloned(); - let bip32_derivation = coin_desc.bip32_derivations(); - psbt.inputs.push(PsbtIn { - witness_script, - witness_utxo, - non_witness_utxo, - bip32_derivation, - ..PsbtIn::default() - }); - } - - // The sweepable_coins iterator may have been empty. - if psbt.unsigned_tx.input.is_empty() { + { + Some(CandidateCoin { + coin: c, + must_select: true, // All coins must be selected. + sequence: Some(bitcoin::Sequence::from_height(timelock)), + }) + } else { + None + } + }) + .collect(); + if sweepable_coins.is_empty() { return Err(CommandError::RecoveryNotAvailable); } - // Compute the value of the single output based on the requested feerate. - let tx_vbytes = (psbt.unsigned_tx.vsize() + sat_vb) as u64; - let absolute_fee = bitcoin::Amount::from_sat(tx_vbytes.checked_mul(feerate_vb).unwrap()); - let output_value = in_value - .checked_sub(absolute_fee) - .ok_or(CommandError::InsufficientFunds(in_value, None, feerate_vb))?; - psbt.unsigned_tx.output[0].value = output_value.to_sat(); - - sanity_check_psbt(&self.config.main_descriptor, &psbt)?; + let sweep_addr_info = sweep_addr.info; + let CreateSpendRes { psbt, has_change } = create_spend( + &self.config.main_descriptor, + &self.secp, + &mut tx_getter, + &[], // No destination, only the change address. + &sweepable_coins, + feerate_vb, + 0, // No min fee required. + sweep_addr, + )?; + if has_change { + self.maybe_increase_next_deriv_index(&mut db_conn, &sweep_addr_info); + } Ok(CreateRecoveryResult { psbt }) } @@ -1184,7 +1137,7 @@ mod tests { locktime::absolute, OutPoint, ScriptBuf, Sequence, Transaction, Txid, Witness, }; - use std::str::FromStr; + use std::{collections::BTreeMap, str::FromStr}; #[test] fn getinfo() { diff --git a/src/spend.rs b/src/spend.rs index d129713f2..363c01b58 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -94,7 +94,7 @@ pub fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationErr // Apply some sanity checks on a created transaction's PSBT. // TODO: add more sanity checks from revault_tx -pub fn sanity_check_psbt( +fn sanity_check_psbt( spent_desc: &descriptors::LianaDescriptor, psbt: &Psbt, ) -> Result<(), SpendCreationError> { @@ -170,6 +170,8 @@ pub struct CandidateCoin { pub coin: Coin, /// Whether or not this coin must be selected by the coin selection algorithm. pub must_select: bool, + /// The nSequence field to set for an input spending this coin. + pub sequence: Option, } /// Metric based on [`LowestFee`] that aims to minimize transaction fees @@ -236,7 +238,7 @@ pub fn select_coins_for_spend( min_fee: u64, max_sat_weight: u32, must_have_change: bool, -) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { +) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { let out_value_nochange = base_tx.output.iter().map(|o| o.value).sum(); // Create the coin selector from the given candidates. NOTE: the coin selector keeps track @@ -339,7 +341,7 @@ pub fn select_coins_for_spend( selector .selected_indices() .iter() - .map(|i| candidate_coins[*i].coin) + .map(|i| candidate_coins[*i]) .collect(), change_amount, )) @@ -543,10 +545,14 @@ pub fn create_spend( // Iterate through selected coins and add necessary information to the PSBT inputs. let mut psbt_ins = Vec::with_capacity(selected_coins.len()); - for coin in &selected_coins { + for cand in &selected_coins { + let coin = &cand.coin; + let sequence = cand + .sequence + .unwrap_or(bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME); tx.input.push(bitcoin::TxIn { previous_output: coin.outpoint, - sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, + sequence, // TODO: once we move to Taproot, anti-fee-sniping using nSequence ..bitcoin::TxIn::default() }); From 6ddda6137cd1a19e446a9a81623290de8ff0d5b8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 12:14:49 +0100 Subject: [PATCH 07/14] spend: make coin selection helpers private They aren't externally called anymore --- src/spend.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/spend.rs b/src/spend.rs index 363c01b58..1b11a3392 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -179,7 +179,7 @@ pub struct CandidateCoin { /// /// Using this metric with `must_have_change: false` is equivalent to using /// [`LowestFee`]. -pub struct LowestFeeChangeCondition<'c, C> { +struct LowestFeeChangeCondition<'c, C> { /// The underlying [`LowestFee`] metric to use. pub lowest_fee: LowestFee<'c, C>, /// If `true`, only solutions with change will be found. @@ -230,7 +230,7 @@ where /// /// `must_have_change` indicates whether the transaction must have a change output. /// If `true`, the returned change amount will be positive. -pub fn select_coins_for_spend( +fn select_coins_for_spend( candidate_coins: &[CandidateCoin], base_tx: bitcoin::Transaction, change_txo: bitcoin::TxOut, From 5894e788b87f7335fb7715036e55add4202a83a8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 12:16:27 +0100 Subject: [PATCH 08/14] spend: move tx size calc helper back to command module It's not needed in spend anymore --- src/commands/mod.rs | 32 +++++++++++++++++++++++++++++--- src/spend.rs | 27 ++------------------------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 5d83ba3b7..08ee7b906 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -9,8 +9,8 @@ use crate::{ database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, spend::{ - check_output_value, create_spend, unsigned_tx_max_vbytes, AddrInfo, CandidateCoin, - CreateSpendRes, SpendCreationError, SpendOutputAddress, TxGetter, + check_output_value, create_spend, AddrInfo, CandidateCoin, CreateSpendRes, + SpendCreationError, SpendOutputAddress, TxGetter, }, DaemonControl, VERSION, }; @@ -29,7 +29,10 @@ use std::{ }; use miniscript::{ - bitcoin::{self, address, bip32, psbt::PartiallySignedTransaction as Psbt}, + bitcoin::{ + self, address, bip32, constants::WITNESS_SCALE_FACTOR, + psbt::PartiallySignedTransaction as Psbt, + }, psbt::PsbtExt, }; use serde::{Deserialize, Serialize}; @@ -176,6 +179,29 @@ impl<'a> TxGetter for BitcoindTxGetter<'a> { } } +/// An unsigned transaction's maximum possible size in vbytes after satisfaction. +/// +/// This assumes all inputs are internal (or have the same `max_sat_weight` value). +/// +/// `tx` is the unsigned transaction. +/// +/// `max_sat_weight` is the maximum weight difference of an input in the +/// transaction before and after satisfaction. Must be in weight units. +fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> u64 { + let witness_factor: u64 = WITNESS_SCALE_FACTOR.try_into().unwrap(); + let num_inputs: u64 = tx.input.len().try_into().unwrap(); + let tx_wu: u64 = tx + .weight() + .to_wu() + .checked_add(max_sat_weight.checked_mul(num_inputs).unwrap()) + .unwrap(); + tx_wu + .checked_add(witness_factor.checked_sub(1).unwrap()) + .unwrap() + .checked_div(witness_factor) + .unwrap() +} + impl DaemonControl { // Get the derived descriptor for this coin fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc { diff --git a/src/spend.rs b/src/spend.rs index 1b11a3392..a57f6f458 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -138,9 +138,9 @@ fn sanity_check_psbt( // and increasing the result, which could lead to the feerate in sats/vb falling below 1. let tx_wu = tx.weight().to_wu() + (spent_desc.max_sat_weight() * tx.input.len()) as u64; let tx_vb = tx_wu - .checked_add(descriptors::WITNESS_FACTOR as u64 - 1) + .checked_add(WITNESS_SCALE_FACTOR as u64 - 1) .unwrap() - .checked_div(descriptors::WITNESS_FACTOR as u64) + .checked_div(WITNESS_SCALE_FACTOR as u64) .unwrap(); let feerate_sats_vb = abs_fee .checked_div(tx_vb) @@ -361,29 +361,6 @@ fn derived_desc( desc.derive(coin.derivation_index, secp) } -/// An unsigned transaction's maximum possible size in vbytes after satisfaction. -/// -/// This assumes all inputs are internal (or have the same `max_sat_weight` value). -/// -/// `tx` is the unsigned transaction. -/// -/// `max_sat_weight` is the maximum weight difference of an input in the -/// transaction before and after satisfaction. Must be in weight units. -pub fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> u64 { - let witness_factor: u64 = WITNESS_SCALE_FACTOR.try_into().unwrap(); - let num_inputs: u64 = tx.input.len().try_into().unwrap(); - let tx_wu: u64 = tx - .weight() - .to_wu() - .checked_add(max_sat_weight.checked_mul(num_inputs).unwrap()) - .unwrap(); - tx_wu - .checked_add(witness_factor.checked_sub(1).unwrap()) - .unwrap() - .checked_div(witness_factor) - .unwrap() -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct AddrInfo { pub index: bip32::ChildNumber, From f3113ba0d20ebed69a21ea50ed3adfe7f7c82a7d Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 12:20:26 +0100 Subject: [PATCH 09/14] commands: remove redundant output value check It's already performed (twice) in spend::create_psbt() --- src/commands/mod.rs | 8 +++----- src/spend.rs | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 08ee7b906..f2df1898e 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -9,8 +9,8 @@ use crate::{ database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, spend::{ - check_output_value, create_spend, AddrInfo, CandidateCoin, CreateSpendRes, - SpendCreationError, SpendOutputAddress, TxGetter, + create_spend, AddrInfo, CandidateCoin, CreateSpendRes, SpendCreationError, + SpendOutputAddress, TxGetter, }, DaemonControl, VERSION, }; @@ -443,13 +443,11 @@ impl DaemonControl { let mut db_conn = self.db.connection(); let mut tx_getter = BitcoindTxGetter::new(&self.bitcoin); - // Check the destination addresses are valid for the network and - // sanity check each output's value. + // Prepare the destination addresses. let mut destinations_checked = Vec::with_capacity(destinations.len()); for (address, value_sat) in destinations { let address = self.validate_address(address.clone())?; let amount = bitcoin::Amount::from_sat(*value_sat); - check_output_value(amount)?; let address = self.spend_addr(&mut db_conn, address); destinations_checked.push((address, amount)); } diff --git a/src/spend.rs b/src/spend.rs index a57f6f458..d6384617c 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -81,7 +81,7 @@ impl fmt::Display for SpendCreationError { impl std::error::Error for SpendCreationError {} // Sanity check the value of a transaction output. -pub fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationError> { +fn check_output_value(value: bitcoin::Amount) -> Result<(), SpendCreationError> { // NOTE: the network parameter isn't used upstream if value.to_sat() > bitcoin::blockdata::constants::MAX_MONEY || value.to_sat() < DUST_OUTPUT_SATS From 0c395bb63c5e7c7f4ad53ae25743efa0f6afb4c7 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 12:39:25 +0100 Subject: [PATCH 10/14] spend: document the create_spend function --- src/spend.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/spend.rs b/src/spend.rs index d6384617c..e03553aa5 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -386,6 +386,31 @@ pub struct CreateSpendRes { pub has_change: bool, } +/// Create a PSBT for a transaction spending some, or all, of `candidate_coins` to `destinations`. +/// Important information for signers will be populated. Will refuse to create outputs worth less +/// than `DUST_OUTPUT_SATS`. Will refuse to create a transaction paying more than `MAX_FEE` +/// satoshis in fees or whose feerate is larger than `MAX_FEERATE` sats/vb. +/// +/// More about the parameters: +/// * `main_descriptor`: the multipath Liana descriptor, used to derive the addresses of the +/// candidate coins. +/// * `secp`: necessary to derive data from the descriptor. +/// * `tx_getter`: an interface to get the wallet transaction for the prevouts of the transaction. +/// Wouldn't be necessary if we only spent Taproot coins. +/// * `destinations`: a list of addresses and amounts, one per recipient i.e. per output in the +/// transaction created. If empty all the `candidate_coins` get spent and a single change output +/// is created to the provided `change_addr`. Can be used to sweep all, or some, coins from the +/// wallet. +/// * `candidate_coins`: a list of coins to consider including as input of the transaction. If +/// `destinations` is empty, they will all be included as inputs of the transaction. Otherwise, a +/// coin selection algorithm will be run to spend the most efficient subset of them to meet the +/// `destinations` requirements. +/// * `feerate_vb`: the feerate to target for this transaction, in satoshi per virtual byte. +/// * `min_fee`: the minimum absolute fee for this transaction. Can be set to 0 for anything but an +/// RBF transaction. +/// * `change_addr`: the address to use for a change output if we need to create one. Can be set to +/// an external address (if combined with an empty list of `destinations` it's useful to sweep some +/// or all coins of a wallet to an external address). pub fn create_spend( main_descriptor: &descriptors::LianaDescriptor, secp: &secp256k1::Secp256k1, From 08ce0ad1d7ac7b4d519ca98a44ecaeb45c7d063c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 14:06:15 +0100 Subject: [PATCH 11/14] spend: update comment about create_spend behaviour --- src/spend.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/spend.rs b/src/spend.rs index e03553aa5..6c462a02b 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -421,17 +421,16 @@ pub fn create_spend( min_fee: u64, change_addr: SpendOutputAddress, ) -> Result { - // This method is a bit convoluted, but it's the nature of creating a Bitcoin transaction - // with a target feerate and outputs. In addition, we support different modes (coin control + // This method does quite a few things. In addition, we support different modes (coin control // vs automated coin selection, self-spend, sweep, etc..) which make the logic a bit more // intricate. Here is a brief overview of what we're doing here: - // 1. Create a transaction with all the target outputs (if this is a self-send, none are - // added at this step the only output will be added as a change output). - // 2. Automatically select the coins if necessary and determine whether a change output - // will be necessary for this transaction from the set of (automatically or manually) - // selected coins. The output for a self-send is added there. - // The change output is also (ab)used to implement a "sweep" functionality. We allow to - // set it to an external address to send all the inputs' value minus the fee and the + // 1. Create a transaction with all the target outputs (if this is a self-send, none are added + // at this step the only output will be added as a change output). + // 2. Automatically select the coins if necessary and determine whether a change output will be + // necessary for this transaction from the set of (automatically or manually) selected + // coins. The output for a self-send is added there. The change output is also (ab)used to + // implement a "sweep" functionality. We allow to set it to an external address to send all + // the inputs' value minus the fee and the // other output's value to a specific, external, address. // 3. Add the selected coins as inputs to the transaction. // 4. Finalize the PSBT and sanity check it before returning it. From c63a120794af593704cede911f666467d3ce4c1e Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 14:18:36 +0100 Subject: [PATCH 12/14] spend: don't use database's Coin type We could use a trait but instead there is just a couple fields we need so simply copy them over. --- src/commands/mod.rs | 51 +++++++++++++++++++++++++-------------------- src/spend.rs | 27 ++++++++++++++---------- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f2df1898e..f0f891d1b 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -202,6 +202,21 @@ fn unsigned_tx_max_vbytes(tx: &bitcoin::Transaction, max_sat_weight: u64) -> u64 .unwrap() } +fn coin_to_candidate( + coin: &Coin, + must_select: bool, + sequence: Option, +) -> CandidateCoin { + CandidateCoin { + outpoint: coin.outpoint, + amount: coin.amount, + deriv_index: coin.derivation_index, + is_change: coin.is_change, + must_select, + sequence, + } +} + impl DaemonControl { // Get the derived descriptor for this coin fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc { @@ -472,10 +487,8 @@ impl DaemonControl { db_conn .coins(&[CoinStatus::Confirmed], &[]) .into_values() - .map(|c| CandidateCoin { - coin: c, - must_select: false, // No coin is mandatory. - sequence: None, // No specific nSequence is required. + .map(|c| { + coin_to_candidate(&c, /*must_select=*/ false, /*sequence=*/ None) }) .collect() } else { @@ -492,11 +505,7 @@ impl DaemonControl { } coins .into_values() - .map(|c| CandidateCoin { - coin: c, - must_select: true, // All coins must be selected. - sequence: None, // No specific nSequence is required. - }) + .map(|c| coin_to_candidate(&c, /*must_select=*/ true, /*sequence=*/ None)) .collect() }; @@ -792,10 +801,8 @@ impl DaemonControl { // transaction in this way, then we set candidates in the same way as for the `!is_cancel` case. let mut candidate_coins: Vec = prev_coins .values() - .map(|c| CandidateCoin { - coin: *c, - must_select: !is_cancel, - sequence: None, // No specific nSequence is required. + .map(|c| { + coin_to_candidate(c, /*must_select=*/ !is_cancel, /*sequence=*/ None) }) .collect(); let confirmed_cands: Vec = db_conn @@ -805,11 +812,9 @@ impl DaemonControl { // Make sure we don't have duplicate candidates in case any of the coins are not // currently set as spending in the DB (and are therefore still confirmed). if !prev_coins.contains_key(&c.outpoint) { - Some(CandidateCoin { - coin: c, - must_select: false, - sequence: None, // No specific nSequence is required. - }) + Some(coin_to_candidate( + &c, /*must_select=*/ false, /*sequence=*/ None, + )) } else { None } @@ -992,11 +997,11 @@ impl DaemonControl { .map(|b| current_height + 1 >= b.height + height_delta) .unwrap_or(false) { - Some(CandidateCoin { - coin: c, - must_select: true, // All coins must be selected. - sequence: Some(bitcoin::Sequence::from_height(timelock)), - }) + Some(coin_to_candidate( + &c, + /*must_select=*/ true, + /*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)), + )) } else { None } diff --git a/src/spend.rs b/src/spend.rs index 6c462a02b..549f8a50f 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -1,4 +1,4 @@ -use crate::{database::Coin, descriptors}; +use crate::descriptors; use std::{collections::BTreeMap, convert::TryInto, fmt}; @@ -166,8 +166,14 @@ fn sanity_check_psbt( /// A candidate for coin selection when creating a transaction. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct CandidateCoin { - /// The candidate coin. - pub coin: Coin, + /// Unique identifier of this coin. + pub outpoint: bitcoin::OutPoint, + /// The value of this coin. + pub amount: bitcoin::Amount, + /// The derivation index used to generate the scriptpubkey of this coin. + pub deriv_index: bip32::ChildNumber, + /// Whether this coin pays to a scriptpubkey derived from the internal keychain. + pub is_change: bool, /// Whether or not this coin must be selected by the coin selection algorithm. pub must_select: bool, /// The nSequence field to set for an input spending this coin. @@ -254,7 +260,7 @@ fn select_coins_for_spend( .iter() .map(|cand| Candidate { input_count: 1, - value: cand.coin.amount.to_sat(), + value: cand.amount.to_sat(), weight: max_input_weight, is_segwit: true, // We only support receiving on Segwit scripts. }) @@ -351,14 +357,14 @@ fn select_coins_for_spend( fn derived_desc( secp: &secp256k1::Secp256k1, desc: &descriptors::LianaDescriptor, - coin: &Coin, + coin: &CandidateCoin, ) -> descriptors::DerivedSinglePathLianaDesc { let desc = if coin.is_change { desc.change_descriptor() } else { desc.receive_descriptor() }; - desc.derive(coin.derivation_index, secp) + desc.derive(coin.deriv_index, secp) } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -547,25 +553,24 @@ pub fn create_spend( // Iterate through selected coins and add necessary information to the PSBT inputs. let mut psbt_ins = Vec::with_capacity(selected_coins.len()); for cand in &selected_coins { - let coin = &cand.coin; let sequence = cand .sequence .unwrap_or(bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME); tx.input.push(bitcoin::TxIn { - previous_output: coin.outpoint, + previous_output: cand.outpoint, sequence, // TODO: once we move to Taproot, anti-fee-sniping using nSequence ..bitcoin::TxIn::default() }); // Populate the PSBT input with the information needed by signers. - let coin_desc = derived_desc(secp, main_descriptor, coin); + let coin_desc = derived_desc(secp, main_descriptor, cand); let witness_script = Some(coin_desc.witness_script()); let witness_utxo = Some(bitcoin::TxOut { - value: coin.amount.to_sat(), + value: cand.amount.to_sat(), script_pubkey: coin_desc.script_pubkey(), }); - let non_witness_utxo = tx_getter.get_tx(&coin.outpoint.txid); + let non_witness_utxo = tx_getter.get_tx(&cand.outpoint.txid); let bip32_derivation = coin_desc.bip32_derivations(); psbt_ins.push(PsbtIn { witness_script, From 990b153107dfae779b054d7ae52bcad574982f5f Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 14:19:20 +0100 Subject: [PATCH 13/14] commands: don't query unconfirmed coins when creating recovery tx They would be discarded immediately in the filter below. --- src/commands/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f0f891d1b..8b5b7edd5 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -989,7 +989,7 @@ impl DaemonControl { timelock.unwrap_or_else(|| self.config.main_descriptor.first_timelock_value()); let height_delta: i32 = timelock.try_into().expect("Must fit, it's a u16"); let sweepable_coins: Vec<_> = db_conn - .coins(&[CoinStatus::Unconfirmed, CoinStatus::Confirmed], &[]) + .coins(&[CoinStatus::Confirmed], &[]) .into_values() .filter_map(|c| { // We are interested in coins available at the *next* block From 0f6941150cdf98da92c40f85526735ec10e9daf1 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 14:32:35 +0100 Subject: [PATCH 14/14] spend: a nicer interface for providing fee informations Allows for a clearer interface: you explicitly set whether you are creating a replacement, and you don't have dangling 0s when you don't which necessitate a comment to explain what they correspond to. --- src/commands/mod.rs | 11 ++++------- src/spend.rs | 22 +++++++++++++++++----- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 8b5b7edd5..fe3a528ab 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -10,7 +10,7 @@ use crate::{ descriptors, spend::{ create_spend, AddrInfo, CandidateCoin, CreateSpendRes, SpendCreationError, - SpendOutputAddress, TxGetter, + SpendOutputAddress, SpendTxFees, TxGetter, }, DaemonControl, VERSION, }; @@ -519,8 +519,7 @@ impl DaemonControl { &mut tx_getter, &destinations_checked, &candidate_coins, - feerate_vb, - 0, // No min fee required. + SpendTxFees::Regular(feerate_vb), change_address, )?; for (addr, _) in destinations_checked { @@ -845,8 +844,7 @@ impl DaemonControl { &mut tx_getter, &destinations, &candidate_coins, - feerate_vb, - min_fee, + SpendTxFees::Rbf(feerate_vb, min_fee), change_address.clone(), ) { Ok(psbt) => psbt, @@ -1018,8 +1016,7 @@ impl DaemonControl { &mut tx_getter, &[], // No destination, only the change address. &sweepable_coins, - feerate_vb, - 0, // No min fee required. + SpendTxFees::Regular(feerate_vb), sweep_addr, )?; if has_change { diff --git a/src/spend.rs b/src/spend.rs index 549f8a50f..1b89167af 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -385,6 +385,17 @@ pub trait TxGetter { fn get_tx(&mut self, txid: &bitcoin::Txid) -> Option; } +/// Specify the fee requirements for a transaction. In both cases set a target feerate in satoshi +/// per virtual byte. For RBF also set a minimum fee in satoshis for this transaction. See +/// https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md for more +/// information about how it should be set. +pub enum SpendTxFees { + /// The target feerate in sats/vb for this transaction. + Regular(u64), + /// The (target feerate, minimum absolute fees) for this transactions. Both in sats. + Rbf(u64, u64), +} + pub struct CreateSpendRes { /// The created PSBT. pub psbt: Psbt, @@ -411,9 +422,7 @@ pub struct CreateSpendRes { /// `destinations` is empty, they will all be included as inputs of the transaction. Otherwise, a /// coin selection algorithm will be run to spend the most efficient subset of them to meet the /// `destinations` requirements. -/// * `feerate_vb`: the feerate to target for this transaction, in satoshi per virtual byte. -/// * `min_fee`: the minimum absolute fee for this transaction. Can be set to 0 for anything but an -/// RBF transaction. +/// * `fees`: the target feerate (in sats/vb) and, if necessary, minimum absolute fee for this tx. /// * `change_addr`: the address to use for a change output if we need to create one. Can be set to /// an external address (if combined with an empty list of `destinations` it's useful to sweep some /// or all coins of a wallet to an external address). @@ -423,8 +432,7 @@ pub fn create_spend( tx_getter: &mut impl TxGetter, destinations: &[(SpendOutputAddress, bitcoin::Amount)], candidate_coins: &[CandidateCoin], - feerate_vb: u64, - min_fee: u64, + fees: SpendTxFees, change_addr: SpendOutputAddress, ) -> Result { // This method does quite a few things. In addition, we support different modes (coin control @@ -441,6 +449,10 @@ pub fn create_spend( // 3. Add the selected coins as inputs to the transaction. // 4. Finalize the PSBT and sanity check it before returning it. + let (feerate_vb, min_fee) = match fees { + SpendTxFees::Regular(feerate) => (feerate, 0), + SpendTxFees::Rbf(feerate, fee) => (feerate, fee), + }; let is_self_send = destinations.is_empty(); if feerate_vb < 1 { return Err(SpendCreationError::InvalidFeerate(feerate_vb));