wallet: fix amount computed as boolean in coin selection

furszy commented at 12:50 pm on March 21, 2026: member

Stumbled upon this tiny bug. This has been working by accident.

The comparison is evaluated first, so total_amount ends up holding a boolean instead of the actual amount. It can be verified by adding the value to the returned error message and running the wallet_create_tx.py test.

Note: I assume something like -Wparentheses in CI (or similar) should help us catching similar issues elsewhere.

DrahtBot added the label Wallet on Mar 21, 2026

DrahtBot commented at 12:50 pm on March 21, 2026: 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	fjahr, andrewtoth, luke-jr, achow101
Approach NACK	l0rinc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

in src/wallet/spend.cpp:942 in 68a210a934 outdated

938@@ -939,7 +939,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
939             if (group.m_ancestors >= max_ancestors || group.m_max_cluster_count >= max_cluster_count) total_unconf_long_chain += group.GetSelectionAmount();
940         }
941 
942-        if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) {

fjahr commented at 9:50 pm on March 21, 2026:

nit: In the commit message “The comparison is evaluated first” is a bit ambiguous because it happens before total_amount assignment but after the subtraction of total_discarded, so at first this confused me a bit. Could say “The comparison is evaluated before the assignment” instead.

furszy commented at 1:41 am on March 22, 2026:

Sure. Done as suggested. Thx.

fjahr commented at 9:53 pm on March 21, 2026: contributor

utACK 68a210a9342f8b8d1476ef55835ea9ca4fedaf6e

Apparently we don’t have test coverage of this, so that would be nice to add.

wallet: fix amount computed as boolean in coin selection

The comparison is evaluated before the assignment, so total_amount
ends up holding a boolean instead of the actual amount:
total_amount = (a - b < c)
which is not what we want here. This has been working by accident.

0026b330c4

furszy force-pushed on Mar 22, 2026

furszy commented at 1:40 am on March 22, 2026: member

Apparently we don’t have test coverage of this, so that would be nice to add.

Funny enough, we do have a test covering this path in wallet_create_tx.py, but it passes by accident. The total amount of the long chain of unconfirmed txs dominates the condition, so total_amount being incorrect doesn’t affect the outcome.

I checked the test values before submission:

0total_amount = 1  <- bool..
1total_unconf_long_chain = 2494916000
2value_to_select = 30042000

So the condition still holds regardless.

Obviously, adding a new targeted test case where total_unconf_long_chain is lower than value_to_select (so total_amount actually matters), would be great. Task open for anyone else willing to take it :). It is a good entry point to the wallet.

fjahr commented at 12:35 pm on March 22, 2026: contributor

utACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab

andrewtoth approved

andrewtoth commented at 5:21 pm on March 22, 2026: contributor

utACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab

l0rinc commented at 0:16 am on March 23, 2026: contributor

Concept ACK. We should probably look for other similar precedence bugs as well.

While this fix corrected an obviously wrong line, it did not come with a test that actually demonstrated why the previous test passed by accident or why the new fix is actually correct. In this case that matters, especially because the same author introduced both the buggy line in acf0119d24 and the original test coverage in f3221d373a. So that’s a strong approach NACK.

The missing piece seems quite small. A test like this makes the intended behavior explicit by checking that the wallet can distinguish between “there is still some usable balance” and “completing this payment would require spending from the too-long unconfirmed chain”:

 0diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py
 1@@ -92,6 +92,7 @@
 2         self.nodes[0].createwallet("too_long")
 3         test_wallet = self.nodes[0].get_wallet_rpc("too_long")
 4
 5+        df_wallet.send(outputs=[{test_wallet.getnewaddress(): 1}])
 6         tx_data = df_wallet.send(outputs=[{test_wallet.getnewaddress(): 25}], options={"change_position": 0})
 7         txid = tx_data['txid']
 8         vout = 1
 9@@ -106,7 +107,7 @@
10         # Sending one more chained transaction will fail
11         options = {"minconf": 0, "include_unsafe": True, 'add_inputs': True}
12         assert_raises_rpc_error(-4, "Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool",
13-                                test_wallet.send, outputs=[{test_wallet.getnewaddress(): 0.3}], options=options)
14+                                test_wallet.send, outputs=[{test_wallet.getnewaddress(): 25}], options=options)

Please verify if the change is correct, but locally it does fail before the fix and pass with it.

furszy commented at 2:55 am on March 23, 2026: member

While this fix corrected an obviously wrong line, it did not come with a test that actually demonstrated why the previous test passed by accident or why the new fix is actually correct. In this case that matters, especially because the same author introduced both the buggy line in acf0119 and the original test coverage in f3221d3. So that’s a strong approach NACK.

Thanks for making things clear for everyone.

Everyone can see git. If you want to ponder my contributions, start with the other thousand tests and bug fixes I have made across this codebase. The vast majority for code I didn’t even write. Not just one tiny bug, which was introduced with a proper test case that exercises the path, just not covering all the possibilities, that slipped through review. Feel more than welcome to take a look: https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+author%3Afurszy+is%3Aclosed.

The explanation for why the test passes by accident and why the fix is correct is three comments above yours: #34888 (comment).

More importantly, using code review to diminish other contributors in order to raise your own reputation is so sad. We all see what you are doing.

pinheadmz commented at 10:18 am on March 23, 2026: member

@l0rinc suggesting a test is great thanks, why do you need anything like “especially because this author” in your comment? @furszy i can see you were triggered by this and it feels bad. My recommendation is to try and not respond with additional personal attacks. If you have the word “you” in a draft comment, take the opportunity to bring the thread back to code and maturity.

If either of you wants to respond to me, send a DM.

l0rinc commented at 10:34 am on March 23, 2026: contributor

The author wrote that he “stumbled” on a bug, which turned out to have been introduced by him - and instead of learning from the mistake, he sends other people to fix his tests. I find this unprofessional, especially after the above outburst…

pinheadmz commented at 11:16 am on March 23, 2026: member

Comments about professionalism or whatever belong in meta/, in DMs, or my preference - nowhere. This repo is for focusing on code changes. Please stay on topic or you may be banned for a day to cool off.