diff --git a/Cargo.lock b/Cargo.lock index e6cd1dc7..e5cde12d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -49,6 +49,11 @@ version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9e1b586273c5702936fe7b7d6896644d8be71e6314cfe09d3167c95f712589e8" +[[package]] +name = "bdk_coin_select" +version = "0.1.0" +source = "git+https://github.com/evanlinjin/bdk?branch=new_bdk_coin_select#2a06d73ac7a5dca933b19b51078f5279691364ed" + [[package]] name = "bech32" version = "0.9.1" @@ -226,6 +231,7 @@ name = "liana" version = "2.0.0" dependencies = [ "backtrace", + "bdk_coin_select", "bip39", "dirs", "fern", diff --git a/Cargo.toml b/Cargo.toml index f35eb119..f709ba4e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,8 @@ nonblocking_shutdown = [] # For managing transactions (it re-exports the bitcoin crate) miniscript = { version = "10.0", features = ["serde", "compiler", "base64"] } +bdk_coin_select = { git = "https://github.com/evanlinjin/bdk", branch = "new_bdk_coin_select" } + # Don't reinvent the wheel dirs = "5.0" diff --git a/doc/API.md b/doc/API.md index ffd6adf4..bf3084a3 100644 --- a/doc/API.md +++ b/doc/API.md @@ -143,6 +143,9 @@ A coin may have one of the following four statuses: Create a transaction spending one or more of our coins. All coins must exist and not be spent. +If no coins are specified in `outpoints`, they will be selected automatically from the set of +confirmed coins (see [`listcoins`](#listcoins) for coin status definitions). + Will error if the given coins are not sufficient to cover the transaction cost at 90% (or more) of the given feerate. If on the contrary the transaction is more than sufficiently funded, it will create a change output when economically rationale to do so. diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 550810bf..2d0c3014 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -12,9 +12,10 @@ use crate::{ 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, ser_amount, - ser_hex, ser_to_string, + deser_addr_assume_checked, deser_amount_from_sats, deser_fromstr, deser_hex, + select_coins_for_spend, ser_amount, ser_hex, ser_to_string, }; use std::{ @@ -37,6 +38,9 @@ use serde::{Deserialize, Serialize}; // 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; @@ -48,7 +52,7 @@ const MAINNET_GENESIS_TIME: u32 = 1231006505; #[derive(Debug, Clone, PartialEq, Eq)] pub enum CommandError { - NoOutpoint, + NoOutpointForSelfSend, InvalidFeerate(/* sats/vb */ u64), UnknownOutpoint(bitcoin::OutPoint), AlreadySpent(bitcoin::OutPoint), @@ -74,12 +78,13 @@ pub enum CommandError { RecoveryNotAvailable, /// Overflowing or unhardened derivation index. InvalidDerivationIndex, + CoinSelectionError(InsufficientFunds), } impl fmt::Display for CommandError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::NoOutpoint => write!(f, "No provided outpoint. 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), @@ -139,6 +144,7 @@ impl fmt::Display for CommandError { "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), } } } @@ -165,6 +171,15 @@ pub enum InsaneFeeInfo { TooHighFeerate(u64), } +/// 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( @@ -232,11 +247,6 @@ fn sanity_check_psbt( Ok(()) } -// Get the size of a type that can be serialized (txos, transactions, ..) -fn serializable_size(t: &T) -> u64 { - bitcoin::consensus::serialize(t).len().try_into().unwrap() -} - impl DaemonControl { // Get the derived descriptor for this coin fn derived_desc(&self, coin: &Coin) -> descriptors::DerivedSinglePathLianaDesc { @@ -403,86 +413,45 @@ impl DaemonControl { coins_outpoints: &[bitcoin::OutPoint], feerate_vb: u64, ) -> 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, 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. + // 3. Fetch the selected coins from database and add them as inputs to the transaction. + // 4. Finalize the PSBT and sanity check it before returning it. + let is_self_send = destinations.is_empty(); - if coins_outpoints.is_empty() { - return Err(CommandError::NoOutpoint); + // For self-send, the coins must be specified. + if is_self_send && coins_outpoints.is_empty() { + return Err(CommandError::NoOutpointForSelfSend); } if feerate_vb < 1 { return Err(CommandError::InvalidFeerate(feerate_vb)); } let mut db_conn = self.db.connection(); - // Iterate through given outpoints to fetch the coins (hence checking their existence - // at the same time). We checked there is at least one, therefore after this loop the - // list of coins is not empty. - // While doing so, we record the total input value of the transaction to later compute - // fees, and add necessary information to the PSBT inputs. - let mut in_value = bitcoin::Amount::from_sat(0); - let txin_sat_vb = self.config.main_descriptor.max_sat_vbytes(); - let mut sat_vb = 0; - let mut txins = Vec::with_capacity(coins_outpoints.len()); - let mut psbt_ins = Vec::with_capacity(coins_outpoints.len()); - let mut spent_txs = HashMap::with_capacity(coins_outpoints.len()); - let coins = db_conn.coins_by_outpoints(coins_outpoints); - for op in coins_outpoints { - // Get the coin from our in-DB unspent txos - let coin = coins.get(op).ok_or(CommandError::UnknownOutpoint(*op))?; - if coin.is_spent() { - return Err(CommandError::AlreadySpent(*op)); - } - if coin.is_immature { - return Err(CommandError::ImmatureCoinbase(*op)); - } - - // Fetch the transaction that created it if necessary - if !spent_txs.contains_key(op) { - let tx = self - .bitcoin - .wallet_transaction(&op.txid) - .ok_or(CommandError::FetchingTransaction(*op))?; - spent_txs.insert(*op, tx.0); - } - - in_value += coin.amount; - txins.push(bitcoin::TxIn { - previous_output: *op, - 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); - 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(op).cloned(); - let bip32_derivation = coin_desc.bip32_derivations(); - psbt_ins.push(PsbtIn { - witness_script, - witness_utxo, - bip32_derivation, - non_witness_utxo, - ..PsbtIn::default() - }); - } - - // Add the destinations outputs to the transaction and PSBT. At the same time record the - // total output value to later compute fees, and sanity check each output's value. - let mut out_value = bitcoin::Amount::from_sat(0); - let mut txouts = Vec::with_capacity(destinations.len()); + // 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(coins_outpoints.len()), // Will be zero capacity for coin selection. + 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, value_sat) in destinations { let address = self.validate_address(address.clone())?; let amount = bitcoin::Amount::from_sat(*value_sat); check_output_value(amount)?; - out_value = out_value.checked_add(amount).unwrap(); - txouts.push(bitcoin::TxOut { + tx.output.push(bitcoin::TxOut { value: amount.to_sat(), script_pubkey: address.script_pubkey(), }); @@ -504,87 +473,149 @@ impl DaemonControl { ..PsbtOut::default() }); } - assert_eq!(txouts.is_empty(), is_self_send); + assert_eq!(tx.output.is_empty(), is_self_send); - // Now create the transaction, compute its fees and already sanity check if its feerate - // isn't much less than what was asked (and obviously that fees aren't negative). - let mut tx = bitcoin::Transaction { - version: 2, - lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), // TODO: randomized anti fee sniping - input: txins, - output: txouts, + // 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 a change address and create a dummy txo for this purpose. + let change_index = db_conn.change_index(); + let change_desc = self + .config + .main_descriptor + .change_descriptor() + .derive(change_index, &self.secp); + let mut change_txo = bitcoin::TxOut { + value: std::u64::MAX, + script_pubkey: change_desc.script_pubkey(), }; - let nochange_vb = (tx.vsize() + sat_vb) as u64; - let absolute_fee = - in_value - .checked_sub(out_value) - .ok_or(CommandError::InsufficientFunds( - in_value, - Some(out_value), - feerate_vb, - ))?; - let nochange_feerate_vb = absolute_fee.to_sat().checked_div(nochange_vb).unwrap(); - if nochange_feerate_vb.checked_mul(10).unwrap() < feerate_vb.checked_mul(9).unwrap() { - return Err(CommandError::InsufficientFunds( - in_value, - Some(out_value), - feerate_vb, - )); - } - - // If necessary, add a change output. The computation here is a bit convoluted: we infer - // the needed change value from the target feerate and the size of the transaction *with - // an added output* (for the change). - if is_self_send || nochange_feerate_vb > feerate_vb { - // Get the change address to create a dummy change txo. - let change_index = db_conn.change_index(); - let change_desc = self + // Now, either select the coins necessary or use the ones provided (verifying they do in + // fact exist and are still unspent) and determine whether there is any leftover to create a + // change output. + let (selected_coins, change_amount) = { + let candidate_coins: Vec = if coins_outpoints.is_empty() { + // We only select confirmed coins for now. Including unconfirmed ones as well would + // introduce a whole bunch of additional complexity. + db_conn + .coins(&[CoinStatus::Confirmed], &[]) + .into_values() + .map(|c| CandidateCoin { + coin: c, + must_select: false, // No coin is mandatory. + }) + .collect() + } else { + // Query from DB and sanity check the provided coins to spend. + let coins = db_conn.coins(&[], coins_outpoints); + for op in coins_outpoints { + let coin = coins.get(op).ok_or(CommandError::UnknownOutpoint(*op))?; + if coin.is_spent() { + return Err(CommandError::AlreadySpent(*op)); + } + if coin.is_immature { + return Err(CommandError::ImmatureCoinbase(*op)); + } + } + coins + .into_values() + .map(|c| CandidateCoin { + coin: c, + must_select: true, // All coins must be selected. + }) + .collect() + }; + // 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 - .change_descriptor() - .derive(change_index, &self.secp); + .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, + 0, // We only constrain the feerate. + max_sat_wu, + ) + .map_err(CommandError::CoinSelectionError)? + }; + // If necessary, add a change output. + if change_amount.to_sat() > 0 { // Don't forget to update our next change index! let next_index = change_index .increment() .expect("Must not get into hardened territory"); db_conn.set_change_index(next_index, &self.secp); - let mut change_txo = bitcoin::TxOut { - value: std::u64::MAX, - script_pubkey: change_desc.script_pubkey(), - }; - // Serialized size is equal to the virtual size for an output. - let change_vb: u64 = serializable_size(&change_txo); - // We assume the added output does not increase the size of the varint for - // the output count. - let with_change_vb = nochange_vb.checked_add(change_vb).unwrap(); - let with_change_feerate_vb = absolute_fee.to_sat().checked_div(with_change_vb).unwrap(); + check_output_value(change_amount)?; - if with_change_feerate_vb > feerate_vb { - // TODO: try first with the exact feerate, then try again with 90% of the feerate - // if it fails. Otherwise with small transactions and large feerates it's possible - // the feerate increase from the target be dramatically higher. - let target_fee = with_change_vb.checked_mul(feerate_vb).unwrap(); - let change_amount = absolute_fee - .checked_sub(bitcoin::Amount::from_sat(target_fee)) - .unwrap(); - if change_amount.to_sat() >= DUST_OUTPUT_SATS { - check_output_value(change_amount)?; - - // TODO: shuffle once we have Taproot - change_txo.value = change_amount.to_sat(); - tx.output.push(change_txo); - psbt_outs.push(PsbtOut { - bip32_derivation: change_desc.bip32_derivations(), - ..PsbtOut::default() - }); - } else if is_self_send { - return Err(CommandError::InsufficientFunds(in_value, None, feerate_vb)); - } - } else if is_self_send { - return Err(CommandError::InsufficientFunds(in_value, None, feerate_vb)); - } + // TODO: shuffle once we have Taproot + change_txo.value = change_amount.to_sat(); + tx.output.push(change_txo); + psbt_outs.push(PsbtOut { + bip32_derivation: change_desc.bip32_derivations(), + ..PsbtOut::default() + }); + } else if is_self_send { + return Err(CommandError::InsufficientFunds( + selected_coins.iter().map(|c| c.amount).sum(), + None, + feerate_vb, + )); } + // 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, @@ -1203,15 +1234,20 @@ mod tests { let dummy_addr = bitcoin::Address::from_str("bc1qnsexk3gnuyayu92fc3tczvc7k62u22a22ua2kv").unwrap(); let dummy_value = 10_000; - let mut destinations: HashMap, u64> = - [(dummy_addr.clone(), dummy_value)] - .iter() - .cloned() - .collect(); + let mut destinations = , u64>>::new(); assert_eq!( control.create_spend(&destinations, &[], 1), - Err(CommandError::NoOutpoint) + Err(CommandError::NoOutpointForSelfSend) ); + destinations = [(dummy_addr.clone(), dummy_value)] + .iter() + .cloned() + .collect(); + // Insufficient funds for coin selection. + assert!(matches!( + control.create_spend(&destinations, &[], 1), + Err(CommandError::CoinSelectionError(..)) + )); assert_eq!( control.create_spend(&destinations, &[dummy_op], 0), Err(CommandError::InvalidFeerate(0)) @@ -1234,6 +1270,12 @@ mod tests { spend_txid: None, spend_block: None, }]); + // If we try to use coin selection, the unconfirmed coin will not be used as a candidate + // and so we get a coin selection error due to insufficient funds. + assert!(matches!( + control.create_spend(&destinations, &[], 1), + Err(CommandError::CoinSelectionError(..)) + )); let res = control.create_spend(&destinations, &[dummy_op], 1).unwrap(); assert!(res.psbt.inputs[0].non_witness_utxo.is_some()); let tx = res.psbt.unsigned_tx; @@ -1260,23 +1302,15 @@ mod tests { .unwrap(); // If we ask for a too high feerate, or a too large/too small output, it'll fail. - assert_eq!( + assert!(matches!( control.create_spend(&destinations, &[dummy_op], 10_000), - Err(CommandError::InsufficientFunds( - bitcoin::Amount::from_sat(100_000), - Some(bitcoin::Amount::from_sat(10_000)), - 10_000 - )) - ); + Err(CommandError::CoinSelectionError(..)) + )); *destinations.get_mut(&dummy_addr).unwrap() = 100_001; - assert_eq!( + assert!(matches!( control.create_spend(&destinations, &[dummy_op], 1), - Err(CommandError::InsufficientFunds( - bitcoin::Amount::from_sat(100_000), - Some(bitcoin::Amount::from_sat(100_001)), - 1 - )) - ); + Err(CommandError::CoinSelectionError(..)) + )); *destinations.get_mut(&dummy_addr).unwrap() = 4_500; assert_eq!( control.create_spend(&destinations, &[dummy_op], 1), @@ -1324,6 +1358,12 @@ mod tests { control.create_spend(&destinations, &[dummy_op], 1), Err(CommandError::AlreadySpent(dummy_op)) ); + // If we try to use coin selection, the spent coin will not be used as a candidate + // and so we get a coin selection error due to insufficient funds. + assert!(matches!( + control.create_spend(&destinations, &[], 1), + Err(CommandError::CoinSelectionError(..)) + )); // We'd bail out if they tried to create a transaction with a too high feerate. let dummy_op_dup = bitcoin::OutPoint { @@ -1340,13 +1380,140 @@ mod tests { spend_txid: None, spend_block: None, }]); + // Even though 1_000 is the max feerate allowed by our sanity check, we need to + // use 1_003 in order to exceed it and fail this test since coin selection is + // based on a minimum feerate of `feerate_vb / 4.0` sats/wu, which can result in + // the sats/vb feerate being lower than `feerate_vb`. assert_eq!( - control.create_spend(&destinations, &[dummy_op_dup], 1_001), + control.create_spend(&destinations, &[dummy_op_dup], 1_003), Err(CommandError::InsaneFees(InsaneFeeInfo::TooHighFeerate( - 1001 + 1_001 ))) ); + // Add a confirmed unspent coin to be used for coin selection. + let confirmed_op_1 = bitcoin::OutPoint { + txid: dummy_op.txid, + vout: dummy_op.vout + 100, + }; + db_conn.new_unspent_coins(&[Coin { + outpoint: confirmed_op_1, + is_immature: false, + block_info: Some(BlockInfo { + height: 174500, + time: 174500, + }), + amount: bitcoin::Amount::from_sat(80_000), + derivation_index: bip32::ChildNumber::from(42), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + // Coin selection error due to insufficient funds. + assert!(matches!( + control.create_spend(&destinations, &[], 1), + Err(CommandError::CoinSelectionError(..)) + )); + // 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), + Err(CommandError::CoinSelectionError(..)) + )); + let confirmed_op_2 = bitcoin::OutPoint { + txid: confirmed_op_1.txid, + vout: confirmed_op_1.vout + 10, + }; + // Add new confirmed coin to cover the fee. + db_conn.new_unspent_coins(&[Coin { + outpoint: confirmed_op_2, + is_immature: false, + block_info: Some(BlockInfo { + height: 174500, + time: 174500, + }), + amount: bitcoin::Amount::from_sat(20_000), + derivation_index: bip32::ChildNumber::from(43), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + // First, create a transaction using auto coin selection. + let res_auto = control.create_spend(&destinations, &[], 1).unwrap(); + let tx_auto = res_auto.psbt.unsigned_tx; + let mut tx_prev_outpoints = tx_auto + .input + .iter() + .map(|txin| txin.previous_output) + .collect::>(); + tx_prev_outpoints.sort(); + assert_eq!(tx_auto.input.len(), 2); + assert_eq!(tx_prev_outpoints, vec![confirmed_op_1, confirmed_op_2]); + // Output includes change. + assert_eq!(tx_auto.output.len(), 2); + assert_eq!( + tx_auto.output[0].script_pubkey, + dummy_addr.payload.script_pubkey() + ); + assert_eq!(tx_auto.output[0].value, 80_000); + + // Create a second transaction using manual coin selection. + let res_manual = control + .create_spend(&destinations, &[confirmed_op_1, confirmed_op_2], 1) + .unwrap(); + let tx_manual = res_manual.psbt.unsigned_tx; + // Check that manual and auto selection give same outputs (including change). + assert_eq!(tx_auto.output, tx_manual.output); + // Check inputs are also the same. Need to sort as order is not guaranteed by `create_spend`. + let mut auto_input = tx_auto.input; + let mut manual_input = tx_manual.input; + auto_input.sort(); + manual_input.sort(); + assert_eq!(auto_input, manual_input); + + // Add a confirmed coin with a value near the dust limit and check that + // `InsufficientFunds` error is returned if feerate is too high. + let confirmed_op_3 = bitcoin::OutPoint { + txid: confirmed_op_2.txid, + vout: confirmed_op_2.vout + 10, + }; + db_conn.new_unspent_coins(&[Coin { + outpoint: confirmed_op_3, + is_immature: false, + block_info: Some(BlockInfo { + height: 174500, + time: 174500, + }), + amount: bitcoin::Amount::from_sat(5_250), + derivation_index: bip32::ChildNumber::from(56), + is_change: false, + spend_txid: None, + spend_block: None, + }]); + let empty_dest = &HashMap::, u64>::new(); + assert_eq!( + control.create_spend(empty_dest, &[confirmed_op_3], 5), + Err(CommandError::InsufficientFunds( + bitcoin::Amount::from_sat(5_250), + None, + 5 + )) + ); + // If we use a lower fee, the self-send will succeed. + let res = control + .create_spend(empty_dest, &[confirmed_op_3], 1) + .unwrap(); + let tx = res.psbt.unsigned_tx; + let tx_prev_outpoints = tx + .input + .iter() + .map(|txin| txin.previous_output) + .collect::>(); + assert_eq!(tx.input.len(), 1); + assert_eq!(tx_prev_outpoints, vec![confirmed_op_3]); + assert_eq!(tx.output.len(), 1); + // Can't create a transaction that spends an immature coinbase deposit. let imma_op = bitcoin::OutPoint::from_str( "4753a1d74c0af8dd0a0f3b763c14faf3bd9ed03cbdf33337a074fb0e9f6c7810:0", diff --git a/src/commands/utils.rs b/src/commands/utils.rs index ce0e2644..d12b242a 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -1,8 +1,17 @@ -use std::str::FromStr; +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 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>, @@ -62,3 +71,119 @@ where let s = Vec::from_hex(&s).map_err(de::Error::custom)?; consensus::deserialize(&s).map_err(de::Error::custom) } + +/// 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 size difference (in vb) of +/// an input in the transaction before and after satisfaction. +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, +) -> 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, + }; + if let Err(e) = selector.run_bnb( + LowestFee { + target, + long_term_feerate, + change_policy: &change_policy, + }, + 100_000, + ) { + warn!( + "Coin selection error: '{}'. Selecting coins by descending value per weight unit...", + e.to_string() + ); + selector.sort_candidates_by_descending_value_pwu(); + // If more coins still need to be selected to meet target, then `change_policy(&selector, target)` + // will give `Drain::none()`, i.e. no change, and this will simply select more coins until + // they cover the target. + selector.select_until_target_met(target, change_policy(&selector, target))?; + } + // 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, + )) +} diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 78fbb9d1..14865c01 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -151,7 +151,7 @@ impl error::Error for Error {} impl From for Error { fn from(e: commands::CommandError) -> Error { match e { - commands::CommandError::NoOutpoint + commands::CommandError::NoOutpointForSelfSend | commands::CommandError::UnknownOutpoint(..) | commands::CommandError::InvalidFeerate(..) | commands::CommandError::AlreadySpent(..) @@ -170,6 +170,7 @@ impl From for Error { } commands::CommandError::FetchingTransaction(..) | commands::CommandError::SanityCheckFailure(_) + | commands::CommandError::CoinSelectionError(..) | commands::CommandError::RescanTrigger(..) => { Error::new(ErrorCode::InternalError, e.to_string()) } diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 861ae5b1..e492da65 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -399,12 +399,13 @@ def test_create_spend(lianad, bitcoind): assert len(spend_psbt.o) == 4 assert len(spend_psbt.tx.vout) == 4 - # The transaction must contain the spent transaction for each input - spent_txs = [bitcoind.rpc.gettransaction(op[:64]) for op in outpoints] - for i, psbt_in in enumerate(spend_psbt.i): - assert psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] == bytes.fromhex( - spent_txs[i]["hex"] - ) + # The transaction must contain the spent transaction for each input. + # We don't make assumptions about the ordering of PSBT inputs. + assert sorted( + [psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in spend_psbt.i] + ) == sorted( + [bytes.fromhex(bitcoind.rpc.gettransaction(op[:64])["hex"]) for op in outpoints] + ) # We can sign it and broadcast it. sign_and_broadcast(lianad, bitcoind, PSBT.from_base64(res["psbt"])) diff --git a/tests/test_spend.py b/tests/test_spend.py index f14c077d..8ad18493 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -219,3 +219,106 @@ def test_send_to_self(lianad, bitcoind): c for c in lianad.rpc.listcoins()["coins"] if c["spend_info"] is None ) wait_for(lambda: len(list(unspent_coins())) == 1) + + +def test_coin_selection(lianad, bitcoind): + """We can create a spend using coin selection.""" + # Send to an (external) address. + dest_100_000 = {bitcoind.rpc.getnewaddress(): 100_000} + # Coin selection is not possible if we have no coins. + assert len(lianad.rpc.listcoins()["coins"]) == 0 + with pytest.raises( + RpcError, + match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", + ): + lianad.rpc.createspend(dest_100_000, [], 2) + + # Receive a coin in an unconfirmed deposit transaction. + recv_addr = lianad.rpc.getnewaddress()["address"] + deposit = bitcoind.rpc.sendtoaddress(recv_addr, 0.0008) # 80_000 sats + wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 1) + # There are still no confirmed coins to use as candidates for selection. + assert len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 0 + assert len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 1 + with pytest.raises( + RpcError, + match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", + ): + lianad.rpc.createspend(dest_100_000, [], 2) + + # Confirm coin. + bitcoind.generate_block(1, wait_for_mempool=deposit) + wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 1) + + # Insufficient funds for coin selection. + with pytest.raises( + RpcError, + match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", + ): + lianad.rpc.createspend(dest_100_000, [], 2) + + # Reduce spend amount. + dest_30_000 = {bitcoind.rpc.getnewaddress(): 30_000} + res = lianad.rpc.createspend(dest_30_000, [], 2) + assert "psbt" in res + + # The transaction must contain a change output. + spend_psbt = PSBT.from_base64(res["psbt"]) + assert len(spend_psbt.o) == 2 + assert len(spend_psbt.tx.vout) == 2 + + # Sign and broadcast this Spend transaction. + signed_psbt = lianad.signer.sign_psbt(spend_psbt) + lianad.rpc.updatespend(signed_psbt.to_base64()) + spend_txid = signed_psbt.tx.txid().hex() + lianad.rpc.broadcastspend(spend_txid) + + wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 2) + coins = lianad.rpc.listcoins()["coins"] + # Check that change output is unconfirmed. + assert len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 1 + assert len(lianad.rpc.listcoins(["spending"])["coins"]) == 1 + # Check we cannot use coins as candidates if they are spending/spent or unconfirmed. + with pytest.raises( + RpcError, + match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", + ): + lianad.rpc.createspend(dest_30_000, [], 2) + + # Now confirm the Spend. + bitcoind.generate_block(1, wait_for_mempool=spend_txid) + wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 1) + # But its value is not enough for this Spend. + dest_60_000 = {bitcoind.rpc.getnewaddress(): 60_000} + with pytest.raises( + RpcError, + match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", + ): + lianad.rpc.createspend(dest_60_000, [], 2) + + # Get another coin to check coin selection with more than one candidate. + recv_addr = lianad.rpc.getnewaddress()["address"] + deposit = bitcoind.rpc.sendtoaddress(recv_addr, 0.0002) # 20_000 sats + bitcoind.generate_block(1, wait_for_mempool=deposit) + wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 2) + + res = lianad.rpc.createspend(dest_60_000, [], 2) + assert "psbt" in res + + # The transaction must contain a change output. + auto_psbt = PSBT.from_base64(res["psbt"]) + assert len(auto_psbt.o) == 2 + assert len(auto_psbt.tx.vout) == 2 + + # Now create a transaction with manual coin selection using the same outpoints. + outpoints = [ + f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin + ] + res_manual = lianad.rpc.createspend(dest_60_000, outpoints, 2) + manual_psbt = PSBT.from_base64(res_manual["psbt"]) + + # Recipient details are the same for both. + assert auto_psbt.tx.vout[0].nValue == manual_psbt.tx.vout[0].nValue + assert auto_psbt.tx.vout[0].scriptPubKey == manual_psbt.tx.vout[0].scriptPubKey + # Change amount is the same (change address will be different). + assert auto_psbt.tx.vout[1].nValue == manual_psbt.tx.vout[1].nValue