Merge bitcoindevkit/bdk#625: Restrict drain_to usage

6a150368674046f796f5c37755896f16d8345fbc Restrict `drain_to` usage (Daniela Brozzoni)

Pull request description:

  ### Description
  Before this commit, you could create a transaction with `drain_to` set
  without specifying recipients, nor `drain_wallet`, nor `utxos`. What would
  happen is that BDK would pick one input from the wallet and send
  that one to `drain_to`, which is quite weird.
  This PR restricts the usage of `drain_to`: if you want to use it as a
  change output, you need to set recipients as well. If you want to send
  a specific utxo to the `drain_to` address, you specify it through
  `add_utxos`. If you want to drain the whole wallet, you set
  `drain_wallet`. In any other case, if `drain_to` is set, we return a
  `NoRecipients` error.

  Fixes #620

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [x] This pull request breaks the existing API - kinda?
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  afilini:
    ACK 6a150368674046f796f5c37755896f16d8345fbc

Tree-SHA512: 69076977df37fcaac92dd99d2f2c9c37098971817d5b0629fc7e3069390eb5789331199b3b7c5d0569d70473f4f37e683a5a0b30e2c6b4e2ec22a5ef1d0f2d77

src/wallet/mod.rs

@@ -809,7 +809,14 @@ where
         let drain_val = (coin_selection.selected_amount() - outgoing).saturating_sub(fee_amount);
 
         if tx.output.is_empty() {
-            if params.drain_to.is_some() {
+            // Uh oh, our transaction has no outputs.
+            // We allow this when:
+            // - We have a drain_to address and the utxos we must spend (this happens,
+            // for example, when we RBF)
+            // - We have a drain_to address and drain_wallet set
+            // Otherwise, we don't know who we should send the funds to, and how much
+            // we should send!
+            if params.drain_to.is_some() && (params.drain_wallet || !params.utxos.is_empty()) {
                 if drain_val.is_dust(&drain_output.script_pubkey) {
                     return Err(Error::InsufficientFunds {
                         needed: drain_output.script_pubkey.dust_value().as_sat(),
@@ -2248,6 +2255,40 @@ pub(crate) mod test {
         assert_eq!(drain_output.value, 30_000 - details.fee.unwrap_or(0));
     }
 
+    #[test]
+    fn test_create_tx_drain_to_and_utxos() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let addr = wallet.get_address(New).unwrap();
+        let utxos: Vec<_> = wallet
+            .get_available_utxos()
+            .unwrap()
+            .into_iter()
+            .map(|(u, _)| u.outpoint)
+            .collect();
+        let mut builder = wallet.build_tx();
+        builder
+            .drain_to(addr.script_pubkey())
+            .add_utxos(&utxos)
+            .unwrap();
+        let (psbt, details) = builder.finish().unwrap();
+
+        assert_eq!(psbt.unsigned_tx.output.len(), 1);
+        assert_eq!(
+            psbt.unsigned_tx.output[0].value,
+            50_000 - details.fee.unwrap_or(0)
+        );
+    }
+
+    #[test]
+    #[should_panic(expected = "NoRecipients")]
+    fn test_create_tx_drain_to_no_drain_wallet_no_utxos() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let drain_addr = wallet.get_address(New).unwrap();
+        let mut builder = wallet.build_tx();
+        builder.drain_to(drain_addr.script_pubkey());
+        builder.finish().unwrap();
+    }
+
     #[test]
     fn test_create_tx_default_fee_rate() {
         let (wallet, _, _) = get_funded_wallet(get_test_wpkh());

src/wallet/tx_builder.rs

@@ -586,6 +586,9 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>> TxBuilder<'a, D, Cs, C
     /// difference is that it is valid to use `drain_to` without setting any ordinary recipients
     /// with [`add_recipient`] (but it is perfectly fine to add recipients as well).
     ///
+    /// If you choose not to set any recipients, you should either provide the utxos that the
+    /// transaction should spend via [`add_utxos`], or set [`drain_wallet`] to spend all of them.
+    ///
     /// When bumping the fees of a transaction made with this option, you probably want to
     /// use [`allow_shrinking`] to allow this output to be reduced to pay for the extra fees.
     ///
@@ -616,6 +619,7 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>> TxBuilder<'a, D, Cs, C
     ///
     /// [`allow_shrinking`]: Self::allow_shrinking
     /// [`add_recipient`]: Self::add_recipient
+    /// [`add_utxos`]: Self::add_utxos
     /// [`drain_wallet`]: Self::drain_wallet
     pub fn drain_to(&mut self, script_pubkey: Script) -> &mut Self {
         self.params.drain_to = Some(script_pubkey);