fix(wallet): only mark change address used if create_tx succeeds

.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();