Merge bitcoindevkit#1579: fix(wallet): only mark change address used …

…if `create_tx` succeeds

606fa08 ci: bump actions/upload-artifact to v4 (valued mammal)
75989d8 test(wallet): Add `test_create_tx_increment_change_index` (valued mammal)
b60d1d2 fix(wallet): only mark change address used if `create_tx` succeeds (valued mammal)

Pull request description:

  If no drain script is specified in tx params then we get it from the change keychain by looking at the next unused address. Before this PR we marked the index used regardless of whether a change output is finally added. Then if creating a psbt failed, we never restored the unused status of the change address, so creating the next tx would have revealed an extra one.

  We want to mark the change address used so that other callers won't attempt to use the same address between the time we create the tx and when it appears on chain. With this PR we only mark the change address used if we successfully create a psbt and the drain script is used in the change output.

  fixes bitcoindevkit#1578

  ### Notes to the reviewers

  An early idea was to unmark the change address used if we fail to create a tx due to `InsufficientFunds`, but after looking into it I figure it doesn't totally make sense to mark the address used before we've determined that a change output is necessary. Further, `create_tx` can fail in other ways besides running coin selection, so I moved the `mark_used` logic to the end of the function.

  ### Changelog notice

  Fixed an issue that caused an unused internal address to be skipped when creating transactions (bitcoindevkit#1578)

  ### 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] 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:
  notmandatory:
    ACK 606fa08

Tree-SHA512: 4715494d901efccff38d636f0538f193ff32db1de44f8d56a98bb0136483f3a8ce1315901bb98117d6870d5b7e4a3bdf3d208f005e2adc0b29625f84a9e8974e

.github/workflows/code_coverage.yml

@@ -51,7 +51,7 @@ jobs:
         with:
           github-token: ${{ secrets.GITHUB_TOKEN }}
       - name: Upload artifact
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v4
         with:
           name: coverage-report
           path: coverage-report.html

.github/workflows/nightly_docs.yml

@@ -22,7 +22,7 @@ jobs:
         env:
           RUSTDOCFLAGS: '--cfg docsrs -Dwarnings'
       - name: Upload artifact
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v4
         with:
           name: built-docs
           path: ./target/doc/*
@@ -44,7 +44,7 @@ jobs:
       - name: Remove old latest
         run: rm -rf ./docs/.vuepress/public/docs-rs/bdk/nightly/latest
       - name: Download built docs
-        uses: actions/download-artifact@v1
+        uses: actions/download-artifact@v4
         with:
           name: built-docs
           path: ./docs/.vuepress/public/docs-rs/bdk/nightly/latest

crates/wallet/src/wallet/mod.rs

@@ -88,7 +88,7 @@ use crate::descriptor::{
 use crate::psbt::PsbtUtils;
 use crate::signer::SignerError;
 use crate::types::*;
-use crate::wallet::coin_selection::Excess::{Change, NoChange};
+use crate::wallet::coin_selection::Excess::{self, Change, NoChange};
 use crate::wallet::error::{BuildFeeBumpError, CreateTxError, MiniscriptPsbtError};
 
 use self::coin_selection::Error;
@@ -1468,17 +1468,28 @@ impl Wallet {
             self.preselect_utxos(&params, Some(current_height.to_consensus_u32()));
 
         // get drain script
+        let mut drain_index = Option::<(KeychainKind, u32)>::None;
         let drain_script = match params.drain_to {
             Some(ref drain_recipient) => drain_recipient.clone(),
             None => {
                 let change_keychain = self.map_keychain(KeychainKind::Internal);
-                let ((index, spk), index_changeset) = self
+                let (index, spk) = self
                     .indexed_graph
                     .index
-                    .next_unused_spk(change_keychain)
-                    .expect("keychain must exist");
-                self.indexed_graph.index.mark_used(change_keychain, index);
-                self.stage.merge(index_changeset.into());
+                    .unused_keychain_spks(change_keychain)
+                    .next()
+                    .unwrap_or_else(|| {
+                        let (next_index, _) = self
+                            .indexed_graph
+                            .index
+                            .next_index(change_keychain)
+                            .expect("keychain must exist");
+                        let spk = self
+                            .peek_address(change_keychain, next_index)
+                            .script_pubkey();
+                        (next_index, spk)
+                    });
+                drain_index = Some((change_keychain, index));
                 spk
             }
         };
@@ -1577,6 +1588,18 @@ impl Wallet {
         params.ordering.sort_tx_with_aux_rand(&mut tx, rng);
 
         let psbt = self.complete_transaction(tx, coin_selection.selected, params)?;
+
+        // recording changes to the change keychain
+        if let (Excess::Change { .. }, Some((keychain, index))) = (excess, drain_index) {
+            let (_, index_changeset) = self
+                .indexed_graph
+                .index
+                .reveal_to_target(keychain, index)
+                .expect("must not be None");
+            self.stage.merge(index_changeset.into());
+            self.mark_used(keychain, index);
+        }
+
         Ok(psbt)
     }
 

crates/wallet/tests/wallet.rs

@@ -1392,6 +1392,127 @@ fn test_create_tx_global_xpubs_with_origin() {
     assert_eq!(psbt.xpub.get(&key), Some(&(fingerprint, path)));
 }
 
+#[test]
+fn test_create_tx_increment_change_index() {
+    // Test derivation index and unused index of change keychain when creating a transaction
+    // Cases include wildcard and non-wildcard descriptors with and without an internal keychain
+    // note the test assumes that the first external address is revealed since we're using
+    // `receive_output`
+    struct TestCase {
+        name: &'static str,
+        descriptor: &'static str,
+        change_descriptor: Option<&'static str>,
+        // amount to send
+        to_send: u64,
+        // (derivation index, next unused index) of *change keychain*
+        expect: (Option<u32>, u32),
+    }
+    // total wallet funds
+    let amount = 10_000;
+    let recipient = Address::from_str("bcrt1q3qtze4ys45tgdvguj66zrk4fu6hq3a3v9pfly5")
+        .unwrap()
+        .assume_checked()
+        .script_pubkey();
+    let (desc, change_desc) = get_test_tr_single_sig_xprv_with_change_desc();
+    [
+        TestCase {
+            name: "two wildcard, builder error",
+            descriptor: desc,
+            change_descriptor: Some(change_desc),
+            to_send: amount + 1,
+            // should not use or derive change index
+            expect: (None, 0),
+        },
+        TestCase {
+            name: "two wildcard, create change",
+            descriptor: desc,
+            change_descriptor: Some(change_desc),
+            to_send: 5_000,
+            // should use change index
+            expect: (Some(0), 1),
+        },
+        TestCase {
+            name: "two wildcard, no change",
+            descriptor: desc,
+            change_descriptor: Some(change_desc),
+            to_send: 9_850,
+            // should not use change index
+            expect: (None, 0),
+        },
+        TestCase {
+            name: "one wildcard, create change",
+            descriptor: desc,
+            change_descriptor: None,
+            to_send: 5_000,
+            // should use change index of external keychain
+            expect: (Some(1), 2),
+        },
+        TestCase {
+            name: "one wildcard, no change",
+            descriptor: desc,
+            change_descriptor: None,
+            to_send: 9_850,
+            // should not use change index
+            expect: (Some(0), 1),
+        },
+        TestCase {
+            name: "single key, create change",
+            descriptor: get_test_tr_single_sig(),
+            change_descriptor: None,
+            to_send: 5_000,
+            // single key only has one derivation index (0)
+            expect: (Some(0), 0),
+        },
+        TestCase {
+            name: "single key, no change",
+            descriptor: get_test_tr_single_sig(),
+            change_descriptor: None,
+            to_send: 9_850,
+            expect: (Some(0), 0),
+        },
+    ]
+    .into_iter()
+    .for_each(|test| {
+        // create wallet
+        let (params, change_keychain) = match test.change_descriptor {
+            Some(change_desc) => (
+                Wallet::create(test.descriptor, change_desc),
+                KeychainKind::Internal,
+            ),
+            None => (
+                Wallet::create_single(test.descriptor),
+                KeychainKind::External,
+            ),
+        };
+        let mut wallet = params
+            .network(Network::Regtest)
+            .create_wallet_no_persist()
+            .unwrap();
+        // fund wallet
+        receive_output(&mut wallet, amount, ConfirmationTime::unconfirmed(0));
+        // create tx
+        let mut builder = wallet.build_tx();
+        builder.add_recipient(recipient.clone(), Amount::from_sat(test.to_send));
+        let res = builder.finish();
+        if !test.name.contains("error") {
+            assert!(res.is_ok());
+        }
+        let (exp_derivation_index, exp_next_unused) = test.expect;
+        assert_eq!(
+            wallet.derivation_index(change_keychain),
+            exp_derivation_index,
+            "derivation index test {}",
+            test.name,
+        );
+        assert_eq!(
+            wallet.next_unused_address(change_keychain).index,
+            exp_next_unused,
+            "next unused index test {}",
+            test.name,
+        );
+    });
+}
+
 #[test]
 fn test_add_foreign_utxo() {
     let (mut wallet1, _) = get_funded_wallet_wpkh();