diff --git a/Cargo.lock b/Cargo.lock index e6cd1dc76..9db2aa2a4 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#7e0d4a5a8cc88e354a09c676efb3d80d33c261e8" + [[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 c8a46ad17..00608cb57 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 501562c1f..86dccc114 100644 --- a/doc/API.md +++ b/doc/API.md @@ -120,6 +120,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 fb2c2b228..20c8c7017 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -12,9 +12,10 @@ use crate::{ pub use crate::database::{CoinStatus, LabelItem}; +use log::info; 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 used for coin selection considerations. +const LONG_TERM_FEERATE_VB: u64 = 10; + // Assume that paying more than 1BTC in fee is a bug. const MAX_FEE: u64 = bitcoin::blockdata::constants::COIN_VALUE; @@ -72,12 +76,13 @@ pub enum CommandError { /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), RecoveryNotAvailable, + CoinSelectionError(String), } 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::NoOutpoint => 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), @@ -136,6 +141,7 @@ impl fmt::Display for CommandError { f, "No coin currently spendable through this timelocked recovery path." ), + Self::CoinSelectionError(s) => write!(f, "Coin selection failed: '{}'", s), } } } @@ -337,14 +343,63 @@ impl DaemonControl { feerate_vb: u64, ) -> Result { let is_self_send = destinations.is_empty(); - if coins_outpoints.is_empty() { + let use_coin_selection = coins_outpoints.is_empty(); + // For self-send, the coins must be specified. + if is_self_send && use_coin_selection { return Err(CommandError::NoOutpoint); } if feerate_vb < 1 { return Err(CommandError::InvalidFeerate(feerate_vb)); } + // Check addresses and values. + let mut destinations_checked = HashMap::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 mut db_conn = self.db.connection(); + // Get the change address to create a dummy change txo. + 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(), + }; + // If change output is indeed required, the next change index in DB will be updated below. + let mut selected_change = bitcoin::Amount::from_sat(0); // For coin selection, will tell us how much change required. + let selected_coins_outpoints: Vec; + let final_coins_outpoints: &[bitcoin::OutPoint] = if use_coin_selection { + log::info!("No outpoints specified. Selecting coins..."); + let candidate_coins: Vec = db_conn + .coins(&[CoinStatus::Confirmed], &[]) + .into_values() + .collect(); + let (selected_coins, change_amount) = select_coins_for_spend( + candidate_coins, + &destinations_checked, + feerate_vb, + self.config.main_descriptor.max_sat_weight(), + change_txo.clone(), + ) + .map_err(|e| CommandError::CoinSelectionError(e.to_string()))?; + selected_change = change_amount; + info!( + "Coin selection gives change of {} sats", + selected_change.to_sat() + ); + selected_coins_outpoints = selected_coins.iter().map(|c| c.outpoint).collect(); + &selected_coins_outpoints[..] + } else { + coins_outpoints + }; + // 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. @@ -353,11 +408,11 @@ impl DaemonControl { 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 { + let mut txins = Vec::with_capacity(final_coins_outpoints.len()); + let mut psbt_ins = Vec::with_capacity(final_coins_outpoints.len()); + let mut spent_txs = HashMap::with_capacity(final_coins_outpoints.len()); + let coins = db_conn.coins_by_outpoints(final_coins_outpoints); + for op in final_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() { @@ -408,11 +463,7 @@ impl DaemonControl { let mut out_value = bitcoin::Amount::from_sat(0); let mut txouts = Vec::with_capacity(destinations.len()); 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)?; + for (address, amount) in destinations_checked { out_value = out_value.checked_add(amount).unwrap(); txouts.push(bitcoin::TxOut { @@ -465,26 +516,19 @@ impl DaemonControl { )); } - // If necessary, add a change output. The computation here is a bit convoluted: we infer + // If necessary, add a change output. + // In the case of coin selection, the change amount has already been determined. + // Otherwise, 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 - .config - .main_descriptor - .change_descriptor() - .derive(change_index, &self.secp); + if (use_coin_selection && selected_change.to_sat() > 0) + || (!use_coin_selection && (is_self_send || nochange_feerate_vb > feerate_vb)) + { // 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 @@ -492,14 +536,19 @@ impl DaemonControl { 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 { + if with_change_feerate_vb > feerate_vb || use_coin_selection { // 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(); + let change_amount = if use_coin_selection { + // Change amount determined by coin selection. + selected_change + } else { + let target_fee = with_change_vb.checked_mul(feerate_vb).unwrap(); + absolute_fee + .checked_sub(bitcoin::Amount::from_sat(target_fee)) + .unwrap() + }; if change_amount.to_sat() >= DUST_OUTPUT_SATS { check_output_value(change_amount)?; @@ -998,15 +1047,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) ); + 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)) diff --git a/src/commands/utils.rs b/src/commands/utils.rs index ce0e26441..c58f3bc63 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -1,8 +1,19 @@ +use std::{collections::HashMap, error, fmt}; + +use bdk_coin_select::{ + change_policy, metrics::LowestFee, Candidate, CoinSelector, DrainWeights, FeeRate, Target, + TXIN_BASE_WEIGHT, +}; +use log::warn; use std::str::FromStr; -use miniscript::bitcoin::{self, consensus, hashes::hex::FromHex}; +use miniscript::bitcoin::{self, consensus, hashes::hex::FromHex, locktime::absolute}; use serde::{de, Deserialize, Deserializer, Serializer}; +use crate::database::Coin; + +use super::{DUST_OUTPUT_SATS, LONG_TERM_FEERATE_VB}; + pub fn deser_fromstr<'de, D, T>(deserializer: D) -> Result where D: Deserializer<'de>, @@ -62,3 +73,96 @@ where let s = Vec::from_hex(&s).map_err(de::Error::custom)?; consensus::deserialize(&s).map_err(de::Error::custom) } + +#[derive(Debug)] +pub struct CoinSelectionError(String); + +impl std::fmt::Display for CoinSelectionError { + fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result { + write!(f, "Coin selection error: '{}'", self.0) + } +} + +impl error::Error for CoinSelectionError {} + +/// Select coins for spend. +pub fn select_coins_for_spend( + candidate_coins: Vec, + destinations: &HashMap, + feerate_vb: u64, + max_sat_weight: usize, + change_txo: bitcoin::TxOut, +) -> Result<(Vec, bitcoin::Amount), CoinSelectionError> { + let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight as u32; + let candidates: Vec = candidate_coins + .iter() + .map(|coin| Candidate { + input_count: 1, + value: coin.amount.to_sat(), + weight: max_input_weight, + is_segwit: true, // We only support receiving on Segwit scripts. + }) + .collect(); + // Transaction base weight is calculated from transaction with no inputs and no change output. + let tx = bitcoin::Transaction { + input: Vec::new(), + output: destinations + .iter() + .map(|(address, amt)| bitcoin::TxOut { + value: amt.to_sat(), + script_pubkey: address.script_pubkey(), + }) + .collect(), + lock_time: absolute::LockTime::Blocks(absolute::Height::ZERO), + version: 2, + }; + let base_weight = tx.weight().to_wu() as u32; + let feerate = FeeRate::from_sat_per_vb(feerate_vb as f32); + let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB as f32); + let target = Target { + value: destinations.values().map(|v| v.to_sat()).sum(), + feerate, + min_fee: 0, // Non-zero value only required for replacement transactions. + }; + let drain_weights = DrainWeights { + output_weight: { + let mut tx_with_change = tx.clone(); + tx_with_change.output.push(change_txo); + tx_with_change.weight().to_wu() as u32 - base_weight + }, + spend_weight: max_input_weight, + }; + // Change policy ensures any change output is not too small and that transaction waste is reduced. + let change_policy = + change_policy::min_value_and_waste(drain_weights, DUST_OUTPUT_SATS, long_term_feerate); + + let mut selector = CoinSelector::new(&candidates, base_weight); + 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() + ); + // If selection not possible by BnB, order coins by descending value ready for selection below. + selector.sort_candidates_by_descending_value_pwu(); + } + selector + .select_until_target_met(target, change_policy(&selector, target)) + .map_err(|e| CoinSelectionError(e.to_string()))?; + 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]) + .collect(), + change_amount, + )) +} diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index 6af7b9e86..bbaa87544 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -169,6 +169,7 @@ impl From for Error { } commands::CommandError::FetchingTransaction(..) | commands::CommandError::SanityCheckFailure(_) + | commands::CommandError::CoinSelectionError(..) | commands::CommandError::RescanTrigger(..) => { Error::new(ErrorCode::InternalError, e.to_string()) }