From 618e0d3700d1f3728d3f475cf09cfda70e27b638 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 16 Jun 2021 12:43:32 +1000 Subject: [PATCH] Replace set_single_recipient with drain_to What set_single_recipient does turns out to be useful with multiple recipients. Effectively, set_single_recipient was simply creating a change output that was arbitrarily required to be the only output. But what if you want to send excess funds to one address but still have additional recipients who receive a fixed value? Generalizing this to `drain_to` simplifies the logic and removes several error cases while also allowing new use cases. "maintain_single_recipient" is also replaced with "allow_shrinking" which has more general semantics. --- CHANGELOG.md | 7 +- src/error.rs | 4 - src/wallet/mod.rs | 227 +++++++++++++++++---------------------- src/wallet/tx_builder.rs | 95 ++++++++++------ 4 files changed, 167 insertions(+), 166 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c404b2..8ae2ff95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Wallet -#### Added -- Bitcoin core RPC added as blockchain backend -- Add a `verify` feature that can be enable to verify the unconfirmed txs we download against the consensus rules + +- Added Bitcoin core RPC added as blockchain backend +- Added a `verify` feature that can be enable to verify the unconfirmed txs we download against the consensus rules +- Removed and replaced `set_single_recipient` with more general `allow_shrinking`. ## [v0.8.0] - [v0.7.0] diff --git a/src/error.rs b/src/error.rs index 8d622276..540f7419 100644 --- a/src/error.rs +++ b/src/error.rs @@ -24,10 +24,6 @@ pub enum Error { Generic(String), /// This error is thrown when trying to convert Bare and Public key script to address ScriptDoesntHaveAddressForm, - /// Found multiple outputs when `single_recipient` option has been specified - SingleRecipientMultipleOutputs, - /// `single_recipient` option is selected but neither `drain_wallet` nor `manually_selected_only` are - SingleRecipientNoInputs, /// Cannot build a tx without recipients NoRecipients, /// `manually_selected_only` option is selected but no utxo has been passed diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 55dd7110..29f43087 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -565,21 +565,6 @@ where output: vec![], }; - let recipients = match ¶ms.single_recipient { - Some(recipient) => vec![(recipient, 0)], - None => params.recipients.iter().map(|(r, v)| (r, *v)).collect(), - }; - if params.single_recipient.is_some() - && !params.manually_selected_only - && !params.drain_wallet - && params.bumping_fee.is_none() - { - return Err(Error::SingleRecipientNoInputs); - } - if recipients.is_empty() { - return Err(Error::NoRecipients); - } - if params.manually_selected_only && params.utxos.is_empty() { return Err(Error::NoUtxosSelected); } @@ -591,12 +576,12 @@ where let calc_fee_bytes = |wu| (wu as f32) * fee_rate.as_sat_vb() / 4.0; fee_amount += calc_fee_bytes(tx.get_weight()); - for (index, (script_pubkey, satoshi)) in recipients.into_iter().enumerate() { - let value = match params.single_recipient { - Some(_) => 0, - None if satoshi.is_dust() => return Err(Error::OutputBelowDustLimit(index)), - None => satoshi, - }; + let recipients = params.recipients.iter().map(|(r, v)| (r, *v)); + + for (index, (script_pubkey, value)) in recipients.enumerate() { + if value.is_dust() { + return Err(Error::OutputBelowDustLimit(index)); + } if self.is_mine(script_pubkey)? { received += value; @@ -651,54 +636,47 @@ where }) .collect(); - // prepare the change output - let change_output = match params.single_recipient { - Some(_) => None, - None => { - let change_script = self.get_change_address()?; - let change_output = TxOut { - script_pubkey: change_script, - value: 0, - }; + // prepare the drain output + let mut drain_output = { + let script_pubkey = match params.drain_to { + Some(ref drain_recipient) => drain_recipient.clone(), + None => self.get_change_address()?, + }; - // take the change into account for fees - fee_amount += calc_fee_bytes(serialize(&change_output).len() * 4); - Some(change_output) + TxOut { + script_pubkey, + value: 0, } }; + + fee_amount += calc_fee_bytes(serialize(&drain_output).len() * 4); + let mut fee_amount = fee_amount.ceil() as u64; - let change_val = (coin_selection.selected_amount() - outgoing).saturating_sub(fee_amount); + let drain_val = (coin_selection.selected_amount() - outgoing).saturating_sub(fee_amount); - match change_output { - None if change_val.is_dust() => { - // single recipient, but the only output would be below dust limit - // TODO: or OutputBelowDustLimit? - return Err(Error::InsufficientFunds { - needed: DUST_LIMIT_SATOSHI, - available: change_val, - }); - } - Some(_) if change_val.is_dust() => { - // skip the change output because it's dust -- just include it in the fee. - fee_amount += change_val; - } - Some(mut change_output) => { - change_output.value = change_val; - received += change_val; - - tx.output.push(change_output); - } - None => { - // there's only one output, send everything to it - tx.output[0].value = change_val; - - // the single recipient is our address - if self.is_mine(&tx.output[0].script_pubkey)? { - received = change_val; + if tx.output.is_empty() { + if params.drain_to.is_some() { + if drain_val.is_dust() { + return Err(Error::InsufficientFunds { + needed: DUST_LIMIT_SATOSHI, + available: drain_val, + }); } + } else { + return Err(Error::NoRecipients); } } + if drain_val.is_dust() { + fee_amount += drain_val; + } else { + drain_output.value = drain_val; + if self.is_mine(&drain_output.script_pubkey)? { + received += drain_val; + } + tx.output.push(drain_output); + } + // sort input/outputs according to the chosen algorithm params.ordering.sort_tx(&mut tx); @@ -725,10 +703,6 @@ where /// *repalce by fee* (RBF). If the transaction can be fee bumped then it returns a [`TxBuilder`] /// pre-populated with the inputs and outputs of the original transaction. /// - /// **NOTE**: if the original transaction was made with [`TxBuilder::set_single_recipient`], - /// the [`TxBuilder::maintain_single_recipient`] flag should be enabled to correctly reduce the - /// only output's value in order to increase the fees. - /// /// ## Example /// /// ```no_run @@ -2008,13 +1982,11 @@ pub(crate) mod test { } #[test] - fn test_create_tx_single_recipient_drain_wallet() { + fn test_create_tx_drain_wallet_and_drain_to() { let (wallet, _, _) = get_funded_wallet(get_test_wpkh()); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, details) = builder.finish().unwrap(); assert_eq!(psbt.global.unsigned_tx.output.len(), 1); @@ -2024,6 +1996,33 @@ pub(crate) mod test { ); } + #[test] + fn test_create_tx_drain_wallet_and_drain_to_and_with_recipient() { + let (wallet, _, _) = get_funded_wallet(get_test_wpkh()); + let addr = Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt").unwrap(); + let drain_addr = wallet.get_address(New).unwrap(); + let mut builder = wallet.build_tx(); + builder + .add_recipient(addr.script_pubkey(), 20_000) + .drain_to(drain_addr.script_pubkey()) + .drain_wallet(); + let (psbt, details) = builder.finish().unwrap(); + dbg!(&psbt); + let outputs = psbt.global.unsigned_tx.output; + + assert_eq!(outputs.len(), 2); + let main_output = outputs + .iter() + .find(|x| x.script_pubkey == addr.script_pubkey()) + .unwrap(); + let drain_output = outputs + .iter() + .find(|x| x.script_pubkey == drain_addr.script_pubkey()) + .unwrap(); + assert_eq!(main_output.value, 20_000,); + assert_eq!(drain_output.value, 30_000 - details.fee.unwrap_or(0)); + } + #[test] fn test_create_tx_default_fee_rate() { let (wallet, _, _) = get_funded_wallet(get_test_wpkh()); @@ -2054,7 +2053,7 @@ pub(crate) mod test { let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .fee_absolute(100); let (psbt, details) = builder.finish().unwrap(); @@ -2073,7 +2072,7 @@ pub(crate) mod test { let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .fee_absolute(0); let (psbt, details) = builder.finish().unwrap(); @@ -2093,7 +2092,7 @@ pub(crate) mod test { let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .fee_absolute(60_000); let (_psbt, _details) = builder.finish().unwrap(); @@ -2134,13 +2133,13 @@ pub(crate) mod test { #[test] #[should_panic(expected = "InsufficientFunds")] - fn test_create_tx_single_recipient_dust_amount() { + fn test_create_tx_drain_to_dust_amount() { let (wallet, _, _) = get_funded_wallet(get_test_wpkh()); let addr = wallet.get_address(New).unwrap(); // very high fee rate, so that the only output would be below dust let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .fee_rate(FeeRate::from_sat_per_vb(453.0)); builder.finish().unwrap(); @@ -2201,9 +2200,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("wpkh([d34db33f/44'/0'/0']tpubDEnoLuPdBep9bzw5LoGYpsxUQYheRQ9gcgrJhJEcdKFB9cWQRyYmkCyRoTqeD4tJYiVVgt6A3rN6rWn9RYhR9sBsGxji29LYWHuKKbdb1ev/0/*)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert_eq!(psbt.inputs[0].bip32_derivation.len(), 1); @@ -2227,9 +2224,7 @@ pub(crate) mod test { let addr = testutils!(@external descriptors, 5); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert_eq!(psbt.outputs[0].bip32_derivation.len(), 1); @@ -2250,9 +2245,7 @@ pub(crate) mod test { get_funded_wallet("sh(pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert_eq!( @@ -2275,9 +2268,7 @@ pub(crate) mod test { get_funded_wallet("wsh(pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert_eq!(psbt.inputs[0].redeem_script, None); @@ -2300,9 +2291,7 @@ pub(crate) mod test { get_funded_wallet("sh(wsh(pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW)))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); let script = Script::from( @@ -2322,9 +2311,7 @@ pub(crate) mod test { get_funded_wallet("sh(pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert!(psbt.inputs[0].non_witness_utxo.is_some()); @@ -2338,7 +2325,7 @@ pub(crate) mod test { let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .only_witness_utxo() .drain_wallet(); let (psbt, _) = builder.finish().unwrap(); @@ -2353,9 +2340,7 @@ pub(crate) mod test { get_funded_wallet("sh(wpkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert!(psbt.inputs[0].witness_utxo.is_some()); @@ -2367,9 +2352,7 @@ pub(crate) mod test { get_funded_wallet("wsh(pk(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (psbt, _) = builder.finish().unwrap(); assert!(psbt.inputs[0].non_witness_utxo.is_some()); @@ -3003,7 +2986,7 @@ pub(crate) mod test { let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .enable_rbf(); let (psbt, mut original_details) = builder.finish().unwrap(); @@ -3027,7 +3010,7 @@ pub(crate) mod test { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder .fee_rate(FeeRate::from_sat_per_vb(2.5)) - .maintain_single_recipient() + .allow_shrinking(addr.script_pubkey()) .unwrap(); let (psbt, details) = builder.finish().unwrap(); @@ -3047,7 +3030,7 @@ pub(crate) mod test { let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .drain_wallet() .enable_rbf(); let (psbt, mut original_details) = builder.finish().unwrap(); @@ -3070,7 +3053,7 @@ pub(crate) mod test { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder - .maintain_single_recipient() + .allow_shrinking(addr.script_pubkey()) .unwrap() .fee_absolute(300); let (psbt, details) = builder.finish().unwrap(); @@ -3101,7 +3084,7 @@ pub(crate) mod test { let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .add_utxo(outpoint) .unwrap() .manually_selected_only() @@ -3130,7 +3113,7 @@ pub(crate) mod test { let mut builder = wallet.build_fee_bump(txid).unwrap(); builder .drain_wallet() - .maintain_single_recipient() + .allow_shrinking(addr.script_pubkey()) .unwrap() .fee_rate(FeeRate::from_sat_per_vb(5.0)); let (_, details) = builder.finish().unwrap(); @@ -3145,7 +3128,7 @@ pub(crate) mod test { // them, and make sure that `bump_fee` doesn't try to add more. This fails because we've // told the wallet it's not allowed to add more inputs AND it can't reduce the value of the // existing output. In other words, bump_fee + manually_selected_only is always an error - // unless you've also set "maintain_single_recipient". + // unless you've also set "allow_shrinking OR there is a change output". let incoming_txid = crate::populate_test_db!( wallet.database.borrow_mut(), testutils! (@tx ( (@external descriptors, 0) => 25_000 ) (@confirmations 1)), @@ -3158,7 +3141,7 @@ pub(crate) mod test { let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .add_utxo(outpoint) .unwrap() .manually_selected_only() @@ -3324,11 +3307,11 @@ pub(crate) mod test { Some(100), ); - // initially make a tx without change by using `set_single_recipient` + // initially make a tx without change by using `set_drain_recipient` let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .add_utxo(OutPoint { txid: incoming_txid, vout: 0, @@ -3356,7 +3339,7 @@ pub(crate) mod test { .set_tx(&original_details) .unwrap(); - // now bump the fees without using `maintain_single_recipient`. the wallet should add an + // now bump the fees without using `allow_shrinking`. the wallet should add an // extra input and a change output, and leave the original output untouched let mut builder = wallet.build_fee_bump(txid).unwrap(); builder.fee_rate(FeeRate::from_sat_per_vb(50.0)); @@ -3602,9 +3585,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); @@ -3619,9 +3600,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("wpkh([d34db33f/84h/1h/0h]tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); @@ -3636,9 +3615,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/44'/0'/0'/0/*)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); @@ -3653,9 +3630,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("sh(wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*))"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); @@ -3671,9 +3646,7 @@ pub(crate) mod test { get_funded_wallet("wpkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); let finalized = wallet.sign(&mut psbt, Default::default()).unwrap(); @@ -3688,9 +3661,7 @@ pub(crate) mod test { let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"); let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); - builder - .set_single_recipient(addr.script_pubkey()) - .drain_wallet(); + builder.drain_to(addr.script_pubkey()).drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); psbt.inputs[0].bip32_derivation.clear(); @@ -3772,7 +3743,7 @@ pub(crate) mod test { let addr = wallet.get_address(New).unwrap(); let mut builder = wallet.build_tx(); builder - .set_single_recipient(addr.script_pubkey()) + .drain_to(addr.script_pubkey()) .sighash(sighash) .drain_wallet(); let (mut psbt, _) = builder.finish().unwrap(); diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs index 92718c88..20ea1955 100644 --- a/src/wallet/tx_builder.rs +++ b/src/wallet/tx_builder.rs @@ -133,7 +133,7 @@ pub struct TxBuilder<'a, B, D, Cs, Ctx> { pub(crate) struct TxParams { pub(crate) recipients: Vec<(Script, u64)>, pub(crate) drain_wallet: bool, - pub(crate) single_recipient: Option