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

pinheadmz · 2023-02-06T20:40:44Z

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 #14582

DrahtBot · 2023-02-06T20:40:47Z

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.

instagibbs · 2023-02-06T21:00:23Z

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

jonatack

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

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

jonatack · 2023-02-06T21:00:15Z

test/functional/wallet_change_address.py

+            [],
+            [],
+            ["-avoidpartialspends"]
+        ]


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.

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.

jonatack · 2023-02-06T21:01:14Z

test/functional/wallet_change_address.py

+
+        for i in range(20):
+            for n in [1, 2]:
+                self.log.info(f"Send transaction from node {n}: expected change index {i}")


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

Suggested change

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

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

thanks, fixed

achow101 · 2023-02-06T21:18:08Z

test/functional/test_runner.py

@@ -215,6 +215,7 @@
    'rpc_blockchain.py',
    'rpc_deprecated.py',
    'wallet_disable.py',
+    'wallet_change_address.py',


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.

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

dooglus · 2023-02-06T23:19:07Z

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 · 2023-02-07T02:09:06Z

test/functional/wallet_importdescriptors.py

        assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
-        change_addr = wmulti_pub.getrawchangeaddress('bech32')
-        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')


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 · 2023-02-07T09:41:07Z

ACK 80104ab

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

Details

$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ createwallet 1
{
  "name": "1",
  "warning": ""
}

$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ createwallet 2
{
  "name": "2",
  "warning": ""
}

$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=1 -generate 1 > /dev/null

$ /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=2 -generate 100 > /dev/null

$ set to (/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=2 getnewaddress)

$ set txs (for i in (seq 5); /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051/ -rpcwallet=1 sendtoaddress $to 1; end)

$ 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)

$ for a in $addrs; /home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_27051 -rpcwallet=1 getaddressinfo $a | jq -r .hdkeypath; end
m/84'/1'/0'/1/0
m/84'/1'/0'/1/1
m/84'/1'/0'/1/2
m/84'/1'/0'/1/3
m/84'/1'/0'/1/4

achow101 · 2023-02-08T20:09:30Z

~~ACK 80104ab~~

#27053 (comment)

fanquake · 2023-02-09T15:31:02Z

Looks like we should backport this as well?

furszy

Code ACK 80104ab

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.

furszy · 2023-02-10T21:07:18Z

src/wallet/spend.cpp

@@ -1092,6 +1092,13 @@ util::Result<CreatedTransactionResult> CreateTransaction(
        TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str());
        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 change_pos = txr_ungrouped.change_pos;


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

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!

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.

Yeah great catch thanks, sorry I didn't notice that already. I added commit 80ad44d 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...

achow101

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

test/functional/wallet_change_address.py

achow101 · 2023-02-15T18:47:29Z

ACK 14b4921

fanquake · 2023-02-20T17:24:45Z

Added to #26878 for backporting into 24.x.

Github-Pull: bitcoin#27053 Rebased-From: 14b4921

…th avoidpartialspends 14b4921 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes 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#14582 ACKs for top commit: achow101: ACK 14b4921 Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659

784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow) debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow) ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow) 50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow) 648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow) ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov) 29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov) 5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov) a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) 64e7db6 Zero out wallet master key upon lock (John Moffett) b7e242e Correctly limit overview transaction list (John Moffett) cff6718 depends: fix systemtap download URL (fanquake) 7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke) 07397cd addrdb: Only call Serialize() once (Martin Zumsande) 91f83db hash: add HashedSourceWriter (Martin Zumsande) 5c824ac For feebump, ignore abandoned descendant spends (John Moffett) 428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow) cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner) 342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner) Pull request description: Backports: * #26595 * #26675 * #26679 * #26761 * #26837 * #26909 * #26924 * #26944 * bitcoin-core/gui#704 * #27053 * #27080 ACKs for top commit: instagibbs: ACK 784a754 achow101: ACK 784a754 hebasto: ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows: Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43

DrahtBot added the Wallet label Feb 6, 2023

pinheadmz mentioned this pull request Feb 6, 2023

every other change address is unused #27051

Closed

jonatack reviewed Feb 6, 2023

View reviewed changes

achow101 reviewed Feb 6, 2023

View reviewed changes

pinheadmz force-pushed the change-addresses branch 2 times, most recently from 937a07f to 52c51fb Compare February 6, 2023 21:33

pinheadmz force-pushed the change-addresses branch 2 times, most recently from 3d12efc to 80104ab Compare February 7, 2023 01:50

pinheadmz commented Feb 7, 2023

View reviewed changes

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

furszy approved these changes Feb 10, 2023

View reviewed changes

pinheadmz force-pushed the change-addresses branch from 80104ab to 80ad44d Compare February 13, 2023 18:04

achow101 reviewed Feb 14, 2023

View reviewed changes

test/functional/wallet_change_address.py Outdated Show resolved Hide resolved

wallet: reuse change dest when recreating TX with avoidpartialspends

14b4921

pinheadmz force-pushed the change-addresses branch from 80ad44d to 14b4921 Compare February 15, 2023 15:48

DrahtBot requested review from furszy and willcl-ark February 15, 2023 18:47

fanquake merged commit 9407002 into bitcoin:master Feb 20, 2023

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

fanquake removed request for furszy and willcl-ark February 20, 2023 17:24

fanquake mentioned this pull request Feb 20, 2023

[24.x] Backports #26878

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2023

wallet: reuse change dest when recreating TX with avoidpartialspends

a62c541

Github-Pull: bitcoin#27053 Rebased-From: 14b4921

bitcoin locked and limited conversation to collaborators Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pinheadmz commented Feb 6, 2023

DrahtBot commented Feb 6, 2023 •

edited

instagibbs commented Feb 6, 2023

jonatack left a comment

jonatack Feb 6, 2023

pinheadmz Feb 6, 2023

jonatack Feb 6, 2023

pinheadmz Feb 6, 2023

achow101 Feb 6, 2023

pinheadmz Feb 6, 2023

dooglus commented Feb 6, 2023

pinheadmz Feb 7, 2023

willcl-ark commented Feb 7, 2023

achow101 commented Feb 8, 2023 •

edited

fanquake commented Feb 9, 2023

furszy left a comment

furszy Feb 10, 2023

achow101 Feb 11, 2023

furszy Feb 11, 2023

pinheadmz Feb 13, 2023

achow101 left a comment

achow101 commented Feb 15, 2023

fanquake commented Feb 20, 2023

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

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

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

Conversation

pinheadmz commented Feb 6, 2023

DrahtBot commented Feb 6, 2023 • edited

Reviews

instagibbs commented Feb 6, 2023

jonatack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dooglus commented Feb 6, 2023

Choose a reason for hiding this comment

willcl-ark commented Feb 7, 2023

achow101 commented Feb 8, 2023 • edited

fanquake commented Feb 9, 2023

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 left a comment

Choose a reason for hiding this comment

achow101 commented Feb 15, 2023

fanquake commented Feb 20, 2023

DrahtBot commented Feb 6, 2023 •

edited

achow101 commented Feb 8, 2023 •

edited