wallet: reuse change dest when re-creating TX with avoidpartialspends

pinheadmz commented at 8:40 pm on February 6, 2023: member

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

DrahtBot commented at 8:40 pm on February 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Concept ACK	instagibbs, jonatack
Stale ACK	willcl-ark, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Wallet on Feb 6, 2023

in test/functional/wallet_change_address.py:28 in d351345238 outdated

20+        self.num_nodes = 3
21+        self.extra_args = [
22+            [],
23+            [],
24+            ["-avoidpartialspends"]
25+        ]

jonatack commented at 9:00 pm on February 6, 2023:

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.

pinheadmz commented at 9:04 pm on February 6, 2023:

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.

instagibbs commented at 9:00 pm on February 6, 2023: member

concept ACK; it doesn’t matter much for Core(as the “gap limit” is very high) but could matter for portability to other wallets.

in test/functional/wallet_change_address.py:57 in d351345238 outdated

52+
53+        self.generate(self.nodes[0], 1)
54+
55+        for i in range(20):
56+            for n in [1, 2]:
57+                self.log.info(f"Send transaction from node {n}: expected change index {i}")

jonatack commented at 9:01 pm on February 6, 2023:

Not sure, but this logging is fairly verbose and perhaps better as debug logging.

0                self.log.debug(f"Send transaction from node {n}: expected change index {i}")

pinheadmz commented at 9:34 pm on February 6, 2023:

thanks, fixed

jonatack commented at 9:02 pm on February 6, 2023: contributor

Concept ACK, test/functional/wallet_importdescriptors.py is failing on the CI and for me locally.

0Traceback (most recent call last):
1  File "/Users/jon/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 134, in main
2    self.run_test()
3  File "/Users/jon/bitcoin/bitcoin/test/functional/wallet_importdescriptors.py", line 487, in run_test
4    assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')
5  File "/Users/jon/bitcoin/bitcoin/test/functional/test_framework/util.py", line 56, in assert_equal
6    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))

in test/functional/test_runner.py:218 in d351345238 outdated

214@@ -215,6 +215,7 @@
215     'rpc_blockchain.py',
216     'rpc_deprecated.py',
217     'wallet_disable.py',
218+    'wallet_change_address.py',

achow101 commented at 9:18 pm on February 6, 2023:

This should have --legacy-wallet and --descriptors variants as the value of hdkeypath is different depending on the wallet type.

Also, this test fails with --legacy-wallet.

pinheadmz commented at 9:35 pm on February 6, 2023:

Thanks! Added the variants. import_descriptors is still failing, will fix that next.

pinheadmz force-pushed on Feb 6, 2023

dooglus commented at 11:19 pm on February 6, 2023: contributor

concept ACK; it doesn’t matter much for Core(as the “gap limit” is very high) but could matter for portability to other wallets.

Isn’t the “gap limit” defined by the -keypool setting, and so may not be very high at all depending on how it has been set?

pinheadmz force-pushed on Feb 7, 2023

in test/functional/wallet_importdescriptors.py:487 in 80104ab14f outdated

480@@ -481,10 +481,10 @@ def run_test(self):
481                             wallet=wmulti_pub)
482 
483         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')
485+        addr = wmulti_pub.getnewaddress('', 'bech32') # uses receive 1
486         assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
487-        change_addr = wmulti_pub.getrawchangeaddress('bech32')
488-        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')

pinheadmz commented at 2:09 am on February 7, 2023:

The address being removed here is derived at 84'/1'/0'/3 and being replaced with 84'/1'/0'/2 – I think the original test vector was succeeding by also skipping change addresses

willcl-ark commented at 9:41 am on February 7, 2023: contributor

ACK 80104ab

In addition to the new test I also confirmed that the repro steps from #27051 were fixed:

 0$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ createwallet 1
 1{
 2  "name": "1",
 3  "warning": ""
 4}
 5
 6$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ createwallet 2
 7{
 8  "name": "2",
 9  "warning": ""
10}
11
12$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=1 -generate 1 > /dev/null
13
14$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=2 -generate 100 > /dev/null
15
16$ set to (/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=2 getnewaddress)
17
18$ set txs (for i in (seq 5); /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=1 sendtoaddress $to 1; end)
19
20$ set addrs (for i in $txs; /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051 getrawtransaction $i true | jq -Mc '.vout[]|{value:.value,addr:.scriptPubKey.address}'; end | grep -v ':1,' | jq -r .addr)
21
22$ for a in $addrs; /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051 -rpcwallet=1 getaddressinfo $a | jq -r .hdkeypath; end
23m/84'/1'/0'/1/0
24m/84'/1'/0'/1/1
25m/84'/1'/0'/1/2
26m/84'/1'/0'/1/3
27m/84'/1'/0'/1/4

achow101 commented at 8:09 pm on February 8, 2023: member

~ACK 80104ab14f9a51cef59cbc584e55959499f1ab2d~

#27053 (review)

fanquake commented at 3:31 pm on February 9, 2023: member

Looks like we should backport this as well?

fanquake added the label Needs backport (24.x) on Feb 9, 2023

in src/wallet/spend.cpp:1097 in 80104ab14f outdated

1091@@ -1092,6 +1092,13 @@ util::Result<CreatedTransactionResult> CreateTransaction(
1092         TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str());
1093         CCoinControl tmp_cc = coin_control;
1094         tmp_cc.m_avoid_partial_spends = true;
1095+
1096+        // Re-use the change destination from the first creation attempt to avoid skipping BIP44 indexes
1097+        const int change_pos = txr_ungrouped.change_pos;

furszy commented at 9:07 pm on February 10, 2023:

nit: this shadows the other change_pos variable. Can overwrite it instead of create a new one.

achow101 commented at 1:06 am on February 11, 2023:

I think this is actually more than a nit, it can have consequences on the change position of the change output for the aps solution.

If the user specified a change position but we didn’t use it, now change_pos would be -1 , so the aps run would put its change in a random position, if it needs one. This could put the change in an unexpected position. So we shouldn’t overwrite nor shadow change_pos.

Good catch!

furszy commented at 12:25 pm on February 11, 2023:

ha, spider sense. Nice. Great catch for you!.

Orthogonal topic; moving forward, one of the follow-ups that have post-#25806 is the removal of this secondary “grouped” tx creation. We will be able to run Coin Selection directly over the aps vs non-aps grouped outputs.

pinheadmz commented at 6:06 pm on February 13, 2023:

Yeah great catch thanks, sorry I didn’t notice that already. I added commit 80ad44d10ca0831869a5e50c36e07e7ff10902ec which includes a test for the edge case:

user requests tx with specific change_position
wallet creates default tx that does not require change (change_pos is overwritten with -1)
wallet creates APS tx as second attempt, that does require change (change is placed in random position)
APS tx is selected and returned, change position is wrong

Looking for feedback on the new test before I squash…

furszy approved

furszy commented at 9:16 pm on February 10, 2023: member

Code ACK 80104ab1

As a happy side note, we will be able to perform the grouped tx fee calculation without require to generate a new tx after #25806.

pinheadmz force-pushed on Feb 13, 2023

in test/functional/wallet_change_address.py:78 in 80ad44d10c outdated

70@@ -62,5 +71,42 @@ def run_test(self):
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+        # Drain wallets
75+        for i in range(1, 3):
76+            node = self.nodes[i]
77+            amount = node.getbalance()
78+            node.sendtoaddress(self.nodes[0].getnewaddress(), amount, subtractfeefromamount=True)

achow101 commented at 8:36 pm on February 14, 2023:

In 80ad44d10ca0831869a5e50c36e07e7ff10902ec “wallet: do not shadow change_pos in CreateTransaction()”

Use sendall instead of sendtoaddress with subtractfeefromamount.

Or just make a new wallet for this test.

pinheadmz commented at 3:49 pm on February 15, 2023:

done, thanks

achow101 commented at 8:39 pm on February 14, 2023: member

One comment on the test, but otherwise looks fine. Please squash.

wallet: reuse change dest when recreating TX with avoidpartialspends 14b4921a91

pinheadmz force-pushed on Feb 15, 2023

achow101 commented at 6:47 pm on February 15, 2023: member

ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e

DrahtBot requested review from furszy on Feb 15, 2023

DrahtBot requested review from willcl-ark on Feb 15, 2023

fanquake merged this on Feb 20, 2023

fanquake closed this on Feb 20, 2023

fanquake commented at 5:24 pm on February 20, 2023: member

Added to #26878 for backporting into 24.x.

fanquake removed the label Needs backport (24.x) on Feb 20, 2023

fanquake removed review request from furszy on Feb 20, 2023

fanquake removed review request from willcl-ark on Feb 20, 2023

fanquake referenced this in commit a62c541ae8 on Feb 22, 2023

sidhujag referenced this in commit 42ff693135 on Feb 25, 2023

glozow referenced this in commit c8c85ca16e on Feb 27, 2023

bitcoin locked this on Feb 20, 2024

wallet: reuse change dest when re-creating TX with avoidpartialspends #27053

Reviews