From e65e5b487b815ed963fdcc5abc52b4b3f51b9cc9 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 20 Feb 2023 17:11:53 +0000 Subject: [PATCH] Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX with avoidpartialspends 14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes https://github.com/bitcoin/bitcoin/issues/27051 When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain. If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected. I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582 ACKs for top commit: achow101: ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659 --- src/wallet/wallet.cpp | 7 ++ test/functional/test_runner.py | 2 + test/functional/wallet_change_address.py | 108 ++++++++++++++++++++ test/functional/wallet_importdescriptors.py | 8 +- test/functional/wallet_upgradetohd.py | 8 +- 5 files changed, 125 insertions(+), 8 deletions(-) create mode 100755 test/functional/wallet_change_address.py 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())