From 22f97e11b7ab772d30ca9ccd992c9092559ab493 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 30 Nov 2023 10:14:09 +0100 Subject: [PATCH] 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 ffb801c9..e06e5f10 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 1b10817e..bec620e4 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 }) }