From ef0cbe911662143aa22ebc77747c15d5637f1a9c Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Thu, 10 Sep 2026 11:43:59 +0000 Subject: [PATCH 1/2] test(key-wallet): pin the guard that keeps an explicit change address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fund_from` only asks the funding account for a change address when the builder has none, so a caller's `set_change_address` survives `add_funding`. Nothing tested that. The guard reads as a redundant `is_none` check next to an unconditional assignment, and removing it would compile, pass every test, and silently override the caller's address — while also advancing the change pool to derive one that is then discarded. The test asserts both halves: the explicit address is still there afterwards, and the account hands out the same next change address a pristine one would. Removing the guard fails it on the first assertion. The doc on `add_funding` now says the same thing in words, since the behaviour is a contract of the funding call rather than an implementation detail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SbgCpMiBjnpvW4CyEEsKXw --- .../transaction_builder.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 2a456ca91..81e55f11f 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -157,6 +157,11 @@ impl TransactionBuilder { /// must therefore not be held across an `await` between `add_funding` and /// `build_signed` or `assemble_unsigned`, since suspending there reopens the /// read-then-reserve window for a concurrent build. + /// + /// A change address already set by [`set_change_address`](Self::set_change_address) + /// wins: the funding account is only asked for one when the builder has none. + /// Deriving unconditionally would both discard the caller's choice and burn a + /// pool address, since `next_change_address` advances the pool state. pub fn add_funding(self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self { self.fund_from(funds_acc, acc, true) } @@ -2201,6 +2206,37 @@ mod tests { /// for the same outpoint, and since coin selection does not deduplicate, /// `SelectionStrategy::All` spends it twice — a transaction Core rejects /// for duplicate prevouts. + /// The guard in `fund_from` that keeps an explicit change address is + /// untested, so nothing stops it being dropped as a redundant `is_none` + /// check. Losing it would silently override the caller's address and, worse, + /// advance the change pool for an address that is then thrown away. + #[test] + fn add_funding_keeps_an_explicit_change_address() { + let ctx = TestWalletContext::new_random(); + let account = + ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); + + let mut funds = ManagedCoreFundsAccount::dummy_bip44(); + let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true); + funds.utxos.insert(utxo.outpoint, utxo); + + let explicit = Address::dummy(Network::Testnet, 1); + let builder = TransactionBuilder::new() + .set_current_height(200) + .set_change_address(explicit.clone()) + .add_funding(&mut funds, &account); + + assert_eq!(builder.change_addr.as_ref(), Some(&explicit)); + + // Nor was a pool address burned to produce one that would be thrown away: + // `funds` still hands out the same address a pristine account would. + let mut control = ManagedCoreFundsAccount::dummy_bip44(); + assert_eq!( + funds.next_change_address(Some(&account.account_xpub), true).expect("change address"), + control.next_change_address(Some(&account.account_xpub), true).expect("change address"), + ); + } + #[test] fn add_funding_does_not_duplicate_a_pre_seeded_input() { let ctx = TestWalletContext::new_random(); From 8262e044d988db91d4956944ca643aad4c4a3ce0 Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Thu, 10 Sep 2026 12:37:10 +0000 Subject: [PATCH 2/2] test(key-wallet): test change address is preserved --- .../transaction_builder.rs | 52 +++++++------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 81e55f11f..a59e0c282 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -2028,12 +2028,29 @@ mod tests { funds.reservations().reserve(&[reserved.outpoint], 200, ReservationToken::next()); - let builder = - TransactionBuilder::new().set_current_height(200).add_funding(&mut funds, &account); + // Set change address to test the branch that avoids it being replaced + // by add_funding() + let explicit = Address::dummy(Network::Testnet, 1); + let builder = TransactionBuilder::new() + .set_current_height(200) + .set_change_address(explicit.clone()) + .add_funding(&mut funds, &account); let candidates: Vec = builder.inputs.iter().map(|utxo| utxo.outpoint).collect(); assert!(!candidates.contains(&reserved.outpoint)); assert!(candidates.contains(&free.outpoint)); + + assert_eq!( + builder.change_addr.as_ref(), + Some(&explicit), + "funding must not override a change address the caller chose" + ); + // Nor may it burn a pool address to derive one it then discards. + let mut control = ManagedCoreFundsAccount::dummy_bip44(); + assert_eq!( + funds.next_change_address(Some(&account.account_xpub), true).expect("change address"), + control.next_change_address(Some(&account.account_xpub), true).expect("change address"), + ); } #[test] @@ -2206,37 +2223,6 @@ mod tests { /// for the same outpoint, and since coin selection does not deduplicate, /// `SelectionStrategy::All` spends it twice — a transaction Core rejects /// for duplicate prevouts. - /// The guard in `fund_from` that keeps an explicit change address is - /// untested, so nothing stops it being dropped as a redundant `is_none` - /// check. Losing it would silently override the caller's address and, worse, - /// advance the change pool for an address that is then thrown away. - #[test] - fn add_funding_keeps_an_explicit_change_address() { - let ctx = TestWalletContext::new_random(); - let account = - ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); - - let mut funds = ManagedCoreFundsAccount::dummy_bip44(); - let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true); - funds.utxos.insert(utxo.outpoint, utxo); - - let explicit = Address::dummy(Network::Testnet, 1); - let builder = TransactionBuilder::new() - .set_current_height(200) - .set_change_address(explicit.clone()) - .add_funding(&mut funds, &account); - - assert_eq!(builder.change_addr.as_ref(), Some(&explicit)); - - // Nor was a pool address burned to produce one that would be thrown away: - // `funds` still hands out the same address a pristine account would. - let mut control = ManagedCoreFundsAccount::dummy_bip44(); - assert_eq!( - funds.next_change_address(Some(&account.account_xpub), true).expect("change address"), - control.next_change_address(Some(&account.account_xpub), true).expect("change address"), - ); - } - #[test] fn add_funding_does_not_duplicate_a_pre_seeded_input() { let ctx = TestWalletContext::new_random();