From cfa0f91dd36bc81d5820baf975930e94063ef691 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 15 Jun 2023 14:40:03 +0100 Subject: [PATCH] commands: auto-select coins if none provided When creating a new spend, if coin outpoints are not provided, then coins will be selected automatically. This automatic selection is such that the transaction fee is minimized, taking into account the cost of creating any change output now and the cost of spending it in the future. If change is added, it must reduce the transaction waste and be above the dust threshold. This same policy is applied also in the case of manual coin selection, replacing the previous logic for determining the change amount. This ensures that creating a spend with auto-selection and another with manual selection using the same auto-selected coins will give the same change amount. --- Cargo.lock | 6 + Cargo.toml | 2 + doc/API.md | 3 + src/commands/mod.rs | 501 ++++++++++++++++++++++++++++-------------- src/commands/utils.rs | 127 ++++++++++- src/jsonrpc/mod.rs | 3 +- tests/test_rpc.py | 13 +- tests/test_spend.py | 103 +++++++++ 8 files changed, 583 insertions(+), 175 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e6cd1dc76..e5cde12de 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 f35eb119f..f709ba4ed 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 ffd6adf43..bf3084a31 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 550810bf7..2d0c3014d 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); - - // 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, + 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 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(); - - 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)); + 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( + 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 ce0e26441..d12b242a0 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 78fbb9d14..14865c013 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 861ae5b13..e492da65d 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 f14c077d6..8ad184935 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