Skip to content

Commit 9407002

Browse files
committed
Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX with avoidpartialspends
14b4921 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes bitcoin/bitcoin#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 bitcoin/bitcoin#14582 ACKs for top commit: achow101: ACK 14b4921 Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
2 parents 0f670e0 + 14b4921 commit 9407002

File tree

4 files changed

+124
-7
lines changed

4 files changed

+124
-7
lines changed

src/wallet/spend.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,6 +1090,13 @@ util::Result<CreatedTransactionResult> CreateTransaction(
10901090
TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str());
10911091
CCoinControl tmp_cc = coin_control;
10921092
tmp_cc.m_avoid_partial_spends = true;
1093+
1094+
// Re-use the change destination from the first creation attempt to avoid skipping BIP44 indexes
1095+
const int ungrouped_change_pos = txr_ungrouped.change_pos;
1096+
if (ungrouped_change_pos != -1) {
1097+
ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange);
1098+
}
1099+
10931100
auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
10941101
// if fee of this alternative one is within the range of the max fee, we use this one
10951102
const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};

test/functional/test_runner.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,8 @@
216216
'rpc_blockchain.py',
217217
'rpc_deprecated.py',
218218
'wallet_disable.py',
219+
'wallet_change_address.py --legacy-wallet',
220+
'wallet_change_address.py --descriptors',
219221
'p2p_addr_relay.py',
220222
'p2p_getaddr_caching.py',
221223
'p2p_getdata.py',
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
#!/usr/bin/env python3
2+
# Copyright (c) 2023 The Bitcoin Core developers
3+
# Distributed under the MIT software license, see the accompanying
4+
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
"""Test wallet change address selection"""
6+
7+
import re
8+
9+
from test_framework.blocktools import COINBASE_MATURITY
10+
from test_framework.test_framework import BitcoinTestFramework
11+
from test_framework.util import (
12+
assert_equal,
13+
)
14+
15+
16+
class WalletChangeAddressTest(BitcoinTestFramework):
17+
def add_options(self, parser):
18+
self.add_wallet_options(parser)
19+
20+
def set_test_params(self):
21+
self.setup_clean_chain = True
22+
self.num_nodes = 3
23+
# discardfee is used to make change outputs less likely in the change_pos test
24+
self.extra_args = [
25+
[],
26+
["-discardfee=1"],
27+
["-avoidpartialspends", "-discardfee=1"]
28+
]
29+
30+
def skip_test_if_missing_module(self):
31+
self.skip_if_no_wallet()
32+
33+
def assert_change_index(self, node, tx, index):
34+
change_index = None
35+
for vout in tx["vout"]:
36+
info = node.getaddressinfo(vout["scriptPubKey"]["address"])
37+
if (info["ismine"] and info["ischange"]):
38+
change_index = int(re.findall(r'\d+', info["hdkeypath"])[-1])
39+
break
40+
assert_equal(change_index, index)
41+
42+
def assert_change_pos(self, wallet, tx, pos):
43+
change_pos = None
44+
for index, output in enumerate(tx["vout"]):
45+
info = wallet.getaddressinfo(output["scriptPubKey"]["address"])
46+
if (info["ismine"] and info["ischange"]):
47+
change_pos = index
48+
break
49+
assert_equal(change_pos, pos)
50+
51+
def run_test(self):
52+
self.log.info("Setting up")
53+
# Mine some coins
54+
self.generate(self.nodes[0], COINBASE_MATURITY + 1)
55+
56+
# Get some addresses from the two nodes
57+
addr1 = [self.nodes[1].getnewaddress() for _ in range(3)]
58+
addr2 = [self.nodes[2].getnewaddress() for _ in range(3)]
59+
addrs = addr1 + addr2
60+
61+
# Send 1 + 0.5 coin to each address
62+
[self.nodes[0].sendtoaddress(addr, 1.0) for addr in addrs]
63+
[self.nodes[0].sendtoaddress(addr, 0.5) for addr in addrs]
64+
self.generate(self.nodes[0], 1)
65+
66+
for i in range(20):
67+
for n in [1, 2]:
68+
self.log.debug(f"Send transaction from node {n}: expected change index {i}")
69+
txid = self.nodes[n].sendtoaddress(self.nodes[0].getnewaddress(), 0.2)
70+
tx = self.nodes[n].getrawtransaction(txid, True)
71+
# find the change output and ensure that expected change index was used
72+
self.assert_change_index(self.nodes[n], tx, i)
73+
74+
# Start next test with fresh wallets and new coins
75+
self.nodes[1].createwallet("w1")
76+
self.nodes[2].createwallet("w2")
77+
w1 = self.nodes[1].get_wallet_rpc("w1")
78+
w2 = self.nodes[2].get_wallet_rpc("w2")
79+
addr1 = w1.getnewaddress()
80+
addr2 = w2.getnewaddress()
81+
self.nodes[0].sendtoaddress(addr1, 3.0)
82+
self.nodes[0].sendtoaddress(addr1, 0.1)
83+
self.nodes[0].sendtoaddress(addr2, 3.0)
84+
self.nodes[0].sendtoaddress(addr2, 0.1)
85+
self.generate(self.nodes[0], 1)
86+
87+
sendTo1 = self.nodes[0].getnewaddress()
88+
sendTo2 = self.nodes[0].getnewaddress()
89+
sendTo3 = self.nodes[0].getnewaddress()
90+
91+
# The avoid partial spends wallet will always create a change output
92+
node = self.nodes[2]
93+
res = w2.send({sendTo1: "1.0", sendTo2: "1.0", sendTo3: "0.9999"}, options={"change_position": 0})
94+
tx = node.getrawtransaction(res["txid"], True)
95+
self.assert_change_pos(w2, tx, 0)
96+
97+
# The default wallet will internally create a tx without change first,
98+
# then create a second candidate using APS that requires a change output.
99+
# Ensure that the user-configured change position is kept
100+
node = self.nodes[1]
101+
res = w1.send({sendTo1: "1.0", sendTo2: "1.0", sendTo3: "0.9999"}, options={"change_position": 0})
102+
tx = node.getrawtransaction(res["txid"], True)
103+
# If the wallet ignores the user's change_position there is still a 25%
104+
# that the random change position passes the test
105+
self.assert_change_pos(w1, tx, 0)
106+
107+
if __name__ == '__main__':
108+
WalletChangeAddressTest().main()

test/functional/wallet_importdescriptors.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -448,14 +448,14 @@ def run_test(self):
448448
wallet=wmulti_priv)
449449

450450
assert_equal(wmulti_priv.getwalletinfo()['keypoolsize'], 1001) # Range end (1000) is inclusive, so 1001 addresses generated
451-
addr = wmulti_priv.getnewaddress('', 'bech32')
451+
addr = wmulti_priv.getnewaddress('', 'bech32') # uses receive 0
452452
assert_equal(addr, 'bcrt1qdt0qy5p7dzhxzmegnn4ulzhard33s2809arjqgjndx87rv5vd0fq2czhy8') # Derived at m/84'/0'/0'/0
453-
change_addr = wmulti_priv.getrawchangeaddress('bech32')
454-
assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e')
453+
change_addr = wmulti_priv.getrawchangeaddress('bech32') # uses change 0
454+
assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e') # Derived at m/84'/1'/0'/0
455455
assert_equal(wmulti_priv.getwalletinfo()['keypoolsize'], 1000)
456456
txid = w0.sendtoaddress(addr, 10)
457457
self.generate(self.nodes[0], 6)
458-
send_txid = wmulti_priv.sendtoaddress(w0.getnewaddress(), 8)
458+
send_txid = wmulti_priv.sendtoaddress(w0.getnewaddress(), 8) # uses change 1
459459
decoded = wmulti_priv.gettransaction(txid=send_txid, verbose=True)['decoded']
460460
assert_equal(len(decoded['vin'][0]['txinwitness']), 4)
461461
self.sync_all()
@@ -481,10 +481,10 @@ def run_test(self):
481481
wallet=wmulti_pub)
482482

483483
assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 1000) # The first one was already consumed by previous import and is detected as used
484-
addr = wmulti_pub.getnewaddress('', 'bech32')
484+
addr = wmulti_pub.getnewaddress('', 'bech32') # uses receive 1
485485
assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
486-
change_addr = wmulti_pub.getrawchangeaddress('bech32')
487-
assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')
486+
change_addr = wmulti_pub.getrawchangeaddress('bech32') # uses change 2
487+
assert_equal(change_addr, 'bcrt1qp6j3jw8yetefte7kw6v5pc89rkgakzy98p6gf7ayslaveaxqyjusnw580c') # Derived at m/84'/1'/0'/2
488488
assert send_txid in self.nodes[0].getrawmempool(True)
489489
assert send_txid in (x['txid'] for x in wmulti_pub.listunspent(0))
490490
assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999)

0 commit comments

Comments
 (0)