Skip to content
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

every other change address is unused #27051

Closed
dooglus opened this issue Feb 6, 2023 · 5 comments · Fixed by #27053
Closed

every other change address is unused #27051

dooglus opened this issue Feb 6, 2023 · 5 comments · Fixed by #27053
Labels

Comments

@dooglus
Copy link
Contributor

dooglus commented Feb 6, 2023

I'm seeing 50% of generated change addresses not being used. It's as if two are reserved for every one that is used.

This is happening in version v24.0.1.

Here's a sequence of commands that shows what I mean. Only the odd numbered change hdkeypaths are used:

$ /bin/rm -fr ~/.bitcoin/regtest
$ bitcoin-qt -regtest &
$ bitcoin-cli -regtest createwallet 1 > /dev/null
$ bitcoin-cli -regtest createwallet 2 > /dev/null
$ bitcoin-cli -regtest -rpcwallet=1 -generate 1 > /dev/null
$ bitcoin-cli -regtest -rpcwallet=2 -generate 100 > /dev/null
$ to=$(bitcoin-cli -regtest -rpcwallet=2 getnewaddress)
$ txs=$(for i in {1..5}; do bitcoin-cli -regtest -rpcwallet=1 sendtoaddress $to 1; done)
$ addrs=$(for i in $txs; do bitcoin-cli -regtest getrawtransaction $i true | jq -Mc '.vout[]|{value:.value,addr:.scriptPubKey.address}'; done | grep -v ':1,' | jq -r .addr)
$ for a in $addrs; do bitcoin-cli -regtest -rpcwallet=1 getaddressinfo $a | jq -r .hdkeypath; done
m/84'/1'/0'/1/1
m/84'/1'/0'/1/3
m/84'/1'/0'/1/5
m/84'/1'/0'/1/7
m/84'/1'/0'/1/9

This happens whether or not I create descriptor wallets. For example, with a descriptor wallet I get these descriptors for the change addresses:

wpkh([779aecf0/84'/1'/0'/1/1]029919052bd5b904afbf71d90afbec0a852d1f0181914b6d07bcab028a0d1ac3e4)#94rhf9lu
wpkh([779aecf0/84'/1'/0'/1/3]0360ecbba4ebaac3ab4f62f14b12a136c2cc5e8264ac1be2af1b5e4413944a2c6a)#lwjzrh62
wpkh([779aecf0/84'/1'/0'/1/5]03d3f72c2cc6085bdee5388ff3b96df7d27ac18d85b5cc8d8048e287b52777062e)#5q4fsvmd
wpkh([779aecf0/84'/1'/0'/1/7]02b77a29387f645bb5ed27ef25144ae67a984e056e41b35fce33bb377d199845ab)#mc608kvz
wpkh([779aecf0/84'/1'/0'/1/9]024988fafba17c2d6f5b2b27428e2fb65ca8a7882ac0fae1422e4fdf46c95dbc61)#mwxzye2v

and for a legacy wallet I get these:

wpkh([bcc40f8d/0'/1'/1']03d3cc823cd01dcb23dce0ac53f8b38c3cb8b9ddbdfc789550c280d25cba432932)#3vxlglf6
wpkh([bcc40f8d/0'/1'/3']03016e2243892459754e125954d205136247f67580e9379ca931f2ec374b924aef)#kxwx63p3
wpkh([bcc40f8d/0'/1'/5']0236dd8b3a6ebd26a4e1e9dc1d84bf053438c339babacd202000f44917bd452336)#jaq2hgn4
wpkh([bcc40f8d/0'/1'/7']03785884e52fad4588d2d0e1df48b8ba691dddfb3696667932a3b46cb0202132fa)#0d786n6s
wpkh([bcc40f8d/0'/1'/9']03ba88cdc9a37aa676a1152c6d7d970aa32324b99d8c6f775c4543e016161ea4d9)#vdvr44ar
@dooglus dooglus added the Bug label Feb 6, 2023
@achow101
Copy link
Member

achow101 commented Feb 6, 2023

For anyone who wants to tackle this, it's most likely to be the optimistic avoidpartialspends behavior which does transaction creation twice, and in doing so, also retrieves two change addresses.

@pinheadmz
Copy link
Member

For anyone who wants to tackle this

Taking a look now ;-)

@dooglus
Copy link
Contributor Author

dooglus commented Feb 6, 2023

If I specify -avoidpartialspends on the bitcoin-qt commandline I don't see the wasted change addresses.

@pinheadmz
Copy link
Member

Possible fix: #27053

@dooglus are you able to test/review ?

@dooglus
Copy link
Contributor Author

dooglus commented Feb 6, 2023

@pinheadmz I'm not familiar enough with the code to review it, but I've tested it and it works for me with both legacy and descriptor wallets.

@bitcoin bitcoin deleted a comment from Binsiboi Feb 7, 2023
@bitcoin bitcoin deleted a comment Feb 7, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Feb 25, 2023
…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
@bitcoin bitcoin locked and limited conversation to collaborators Feb 20, 2024
knst pushed a commit to knst/dash that referenced this issue Mar 18, 2024
…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
knst pushed a commit to knst/dash that referenced this issue Mar 18, 2024
…ng TX with 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants