New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: reuse change dest when re-creating TX with avoidpartialspends #27053
Merged
+124
−7
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -448,14 +448,14 @@ 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('', 'bech32') | ||
addr = wmulti_priv.getnewaddress('', 'bech32') # uses receive 0 | ||
assert_equal(addr, 'bcrt1qdt0qy5p7dzhxzmegnn4ulzhard33s2809arjqgjndx87rv5vd0fq2czhy8') # Derived at m/84'/0'/0'/0 | ||
change_addr = wmulti_priv.getrawchangeaddress('bech32') | ||
assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e') | ||
change_addr = wmulti_priv.getrawchangeaddress('bech32') # uses change 0 | ||
assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e') # Derived at m/84'/1'/0'/0 | ||
assert_equal(wmulti_priv.getwalletinfo()['keypoolsize'], 1000) | ||
txid = w0.sendtoaddress(addr, 10) | ||
self.generate(self.nodes[0], 6) | ||
send_txid = wmulti_priv.sendtoaddress(w0.getnewaddress(), 8) | ||
send_txid = wmulti_priv.sendtoaddress(w0.getnewaddress(), 8) # uses change 1 | ||
decoded = wmulti_priv.gettransaction(txid=send_txid, verbose=True)['decoded'] | ||
assert_equal(len(decoded['vin'][0]['txinwitness']), 4) | ||
self.sync_all() | ||
|
@@ -481,10 +481,10 @@ 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('', 'bech32') | ||
addr = wmulti_pub.getnewaddress('', 'bech32') # uses receive 1 | ||
assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1 | ||
change_addr = wmulti_pub.getrawchangeaddress('bech32') | ||
assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The address being removed here is derived at |
||
change_addr = wmulti_pub.getrawchangeaddress('bech32') # uses change 2 | ||
assert_equal(change_addr, 'bcrt1qp6j3jw8yetefte7kw6v5pc89rkgakzy98p6gf7ayslaveaxqyjusnw580c') # Derived at m/84'/1'/0'/2 | ||
assert send_txid in self.nodes[0].getrawmempool(True) | ||
assert send_txid in (x['txid'] for x in wmulti_pub.listunspent(0)) | ||
assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't check, but if there is an existing functional test with a similar test setup it might be faster to add this test there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started by adding what I wanted to wallet_groups.py but it was getting messy, can still be done though with some refactoring but nice thing about starting clean is I get to use index 0, and the other test checks wallet balances so adding new TXs along the way would affect all that.