diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5b257e7792a832..19cf536582e70e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3786,6 +3786,13 @@ bool CWallet::CreateTransaction( if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) { CCoinControl tmp_cc = coin_control; tmp_cc.m_avoid_partial_spends = true; + + // Re-use the change destination from the first creation attempt to avoid skipping BIP44 indexes + const int ungrouped_change_pos = nChangePosInOut; + if (ungrouped_change_pos != -1) { + ExtractDestination(tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange); + } + CAmount nFeeRet2; int nChangePosInOut2 = nChangePosIn; bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 56f0faf9d6a28c..f94595b10ededa 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -191,6 +191,8 @@ 'rpc_blockchain.py', 'rpc_deprecated.py', 'wallet_disable.py', + 'wallet_change_address.py --legacy-wallet', + 'wallet_change_address.py --descriptors', 'p2p_addr_relay.py', 'p2p_getaddr_caching.py', 'p2p_getdata.py', diff --git a/test/functional/wallet_change_address.py b/test/functional/wallet_change_address.py new file mode 100755 index 00000000000000..f8bfe9eebf12a6 --- /dev/null +++ b/test/functional/wallet_change_address.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test wallet change address selection""" + +import re + +from test_framework.blocktools import COINBASE_MATURITY +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, +) + + +class WalletChangeAddressTest(BitcoinTestFramework): + def add_options(self, parser): + self.add_wallet_options(parser) + + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 3 + # discardfee is used to make change outputs less likely in the change_pos test + self.extra_args = [ + [], + ["-discardfee=1"], + ["-avoidpartialspends", "-discardfee=1"] + ] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def assert_change_index(self, node, tx, index): + change_index = None + for vout in tx["vout"]: + info = node.getaddressinfo(vout["scriptPubKey"]["address"]) + if (info["ismine"] and info["ischange"]): + change_index = int(re.findall(r'\d+', info["hdkeypath"])[-1]) + break + assert_equal(change_index, index) + + def assert_change_pos(self, wallet, tx, pos): + change_pos = None + for index, output in enumerate(tx["vout"]): + info = wallet.getaddressinfo(output["scriptPubKey"]["address"]) + if (info["ismine"] and info["ischange"]): + change_pos = index + break + assert_equal(change_pos, pos) + + def run_test(self): + self.log.info("Setting up") + # Mine some coins + self.generate(self.nodes[0], COINBASE_MATURITY + 1) + + # Get some addresses from the two nodes + addr1 = [self.nodes[1].getnewaddress() for _ in range(3)] + addr2 = [self.nodes[2].getnewaddress() for _ in range(3)] + addrs = addr1 + addr2 + + # Send 1 + 0.5 coin to each address + [self.nodes[0].sendtoaddress(addr, 1.0) for addr in addrs] + [self.nodes[0].sendtoaddress(addr, 0.5) for addr in addrs] + self.generate(self.nodes[0], 1) + + for i in range(20): + for n in [1, 2]: + self.log.debug(f"Send transaction from node {n}: expected change index {i}") + txid = self.nodes[n].sendtoaddress(self.nodes[0].getnewaddress(), 0.2) + tx = self.nodes[n].getrawtransaction(txid, True) + # find the change output and ensure that expected change index was used + self.assert_change_index(self.nodes[n], tx, i) + + # Start next test with fresh wallets and new coins + self.nodes[1].createwallet("w1") + self.nodes[2].createwallet("w2") + w1 = self.nodes[1].get_wallet_rpc("w1") + w2 = self.nodes[2].get_wallet_rpc("w2") + addr1 = w1.getnewaddress() + addr2 = w2.getnewaddress() + self.nodes[0].sendtoaddress(addr1, 3.0) + self.nodes[0].sendtoaddress(addr1, 0.1) + self.nodes[0].sendtoaddress(addr2, 3.0) + self.nodes[0].sendtoaddress(addr2, 0.1) + self.generate(self.nodes[0], 1) + + sendTo1 = self.nodes[0].getnewaddress() + sendTo2 = self.nodes[0].getnewaddress() + sendTo3 = self.nodes[0].getnewaddress() + + # The avoid partial spends wallet will always create a change output + node = self.nodes[2] + res = w2.send({sendTo1: "1.0", sendTo2: "1.0", sendTo3: "0.9999"}, options={"change_position": 0}) + tx = node.getrawtransaction(res["txid"], True) + self.assert_change_pos(w2, tx, 0) + + # The default wallet will internally create a tx without change first, + # then create a second candidate using APS that requires a change output. + # Ensure that the user-configured change position is kept + node = self.nodes[1] + res = w1.send({sendTo1: "1.0", sendTo2: "1.0", sendTo3: "0.9999"}, options={"change_position": 0}) + tx = node.getrawtransaction(res["txid"], True) + # If the wallet ignores the user's change_position there is still a 25% + # that the random change position passes the test + self.assert_change_pos(w1, tx, 0) + +if __name__ == '__main__': + WalletChangeAddressTest().main() diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index b5ca1db5a7f27c..7b3a136d759a0c 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -321,7 +321,7 @@ def run_test(self): wallet=wmulti_priv) assert_equal(wmulti_priv.getwalletinfo()['keypoolsize'], 1001) # Range end (1000) is inclusive, so 1001 addresses generated - addr = wmulti_priv.getnewaddress() + addr = wmulti_priv.getnewaddress() # uses receive 0 assert_equal(addr, '8vEwYGKBMP3F2juEE36nNqh1uYpBv9QFyB') # Derived at m/84'/0'/0'/0 change_addr = wmulti_priv.getrawchangeaddress() assert_equal(change_addr, '91WxMwg2NHD1PwHChhbAkeCN6nQ8ikdLEx') @@ -329,7 +329,7 @@ def run_test(self): txid = w0.sendtoaddress(addr, 10) self.nodes[0].generate(6) self.sync_all() - wmulti_priv.sendtoaddress(w0.getnewaddress(), 8) + wmulti_priv.sendtoaddress(w0.getnewaddress(), 8) # uses change 1 self.nodes[0].generate(6) self.sync_all() @@ -354,9 +354,9 @@ def run_test(self): wallet=wmulti_pub) assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 1000) # The first one was already consumed by previous import and is detected as used - addr = wmulti_pub.getnewaddress() + addr = wmulti_pub.getnewaddress() # uses receive 1 assert_equal(addr, '91cA4fLGaDCr6b9W2c5j1ph9PDpq9WbEhk') # Derived at m/84'/0'/0'/1 - change_addr = wmulti_pub.getrawchangeaddress() + change_addr = wmulti_pub.getrawchangeaddress() # uses receive 2 assert_equal(change_addr, '91WxMwg2NHD1PwHChhbAkeCN6nQ8ikdLEx') assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999) txid = w0.sendtoaddress(addr, 10) diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py index 6b4cd9869c7142..64cf39cf57a399 100755 --- a/test/functional/wallet_upgradetohd.py +++ b/test/functional/wallet_upgradetohd.py @@ -62,7 +62,7 @@ def run_test(self): assert_equal(keypath, "m/44'/1'/0'/0/%d" % i) else: keypath = node.getaddressinfo(out['scriptPubKey']['addresses'][0])['hdkeypath'] - assert_equal(keypath, "m/44'/1'/0'/1/%d" % (i * 2)) + assert_equal(keypath, "m/44'/1'/0'/1/%d" % i) self.bump_mocktime(1) node.generate(1) @@ -134,7 +134,7 @@ def run_test(self): assert node.upgradetohd(mnemonic) assert_equal(mnemonic, node.dumphdinfo()['mnemonic']) assert_equal(chainid, node.getwalletinfo()['hdchainid']) - node.keypoolrefill(10) + node.keypoolrefill(5) assert balance_after != node.getbalance() node.rescanblockchain() assert_equal(balance_after, node.getbalance()) @@ -177,7 +177,7 @@ def run_test(self): # so we can't compare new balance to balance_non_HD here, # assert_equal(balance_non_HD, node.getbalance()) # won't work assert balance_non_HD != node.getbalance() - node.keypoolrefill(8) + node.keypoolrefill(4) node.rescanblockchain() # All coins should be recovered assert_equal(balance_after, node.getbalance()) @@ -201,7 +201,7 @@ def run_test(self): # so we can't compare new balance to balance_non_HD here, # assert_equal(balance_non_HD, node.getbalance()) # won't work assert balance_non_HD != node.getbalance() - node.keypoolrefill(8) + node.keypoolrefill(4) node.rescanblockchain() # All coins should be recovered assert_equal(balance_after, node.getbalance())