From b60d1d29cb8908c354b43c49237acbea373c3dc7 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Tue, 27 Aug 2024 11:54:25 -0400 Subject: [PATCH 1/3] fix(wallet): only mark change address used if `create_tx` succeeds If no drain script is specified in tx params then we get it from the change keychain by looking at the next unused address. 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. Before, 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. Now we only mark the change address used if we successfully create a PSBT and the drain script is used in the change output. --- crates/wallet/src/wallet/mod.rs | 35 +++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/crates/wallet/src/wallet/mod.rs b/crates/wallet/src/wallet/mod.rs index 9f12fac4e..25803f139 100644 --- a/crates/wallet/src/wallet/mod.rs +++ b/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(¶ms, 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) } From 75989d8cde3902f226bfa89aae05803b93a7cf1d Mon Sep 17 00:00:00 2001 From: valued mammal Date: Tue, 27 Aug 2024 12:24:41 -0400 Subject: [PATCH 2/3] test(wallet): Add `test_create_tx_increment_change_index` --- crates/wallet/tests/wallet.rs | 121 ++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index 21f391ca6..2a2a68fa6 100644 --- a/crates/wallet/tests/wallet.rs +++ b/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), + } + // 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(); From 606fa0874db0f10cd1c64de0f1f097b12db3a16d Mon Sep 17 00:00:00 2001 From: valued mammal Date: Mon, 9 Sep 2024 11:51:11 -0400 Subject: [PATCH 3/3] ci: bump actions/upload-artifact to v4 --- .github/workflows/code_coverage.yml | 2 +- .github/workflows/nightly_docs.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/code_coverage.yml b/.github/workflows/code_coverage.yml index 54020bb90..5a91de04e 100644 --- a/.github/workflows/code_coverage.yml +++ b/.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 diff --git a/.github/workflows/nightly_docs.yml b/.github/workflows/nightly_docs.yml index 5fa4ecb5d..59b19a591 100644 --- a/.github/workflows/nightly_docs.yml +++ b/.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