wallet: don't consider unconfirmed TRUC coins with ancestors

glozow commented at 10:10 pm on October 2, 2025: member

There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in CommitTransaction.

This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm.

https://github.com/bitcoin/bitcoin/blob/1ed00a0d39d5190d8ad88a0dd705a09b56d987aa/test/functional/wallet_basic.py#L502-L506

It’s a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it’s quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds.

[wallet] never try to spend from unconfirmed TRUC that already has ancestors e753fadfd0

[test] wallet send 3 generation TRUC dcd42d6d8f

glozow added the label Wallet on Oct 2, 2025

DrahtBot commented at 10:10 pm on October 2, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33528.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

fanquake commented at 3:55 pm on October 3, 2025: member

Is this going to be backported to 30.x?

glozow commented at 5:04 pm on October 3, 2025: member

Is this going to be backported to 30.x?

I think that’d be best so that the behavior is consistent from v30 onwards. But I don’t think this should hold up the release.

fanquake added the label Needs backport (30.x) on Oct 3, 2025

monlovesmango commented at 5:21 pm on October 7, 2025: contributor

Wow so on top of it @glozow :)

A couple things.

When I went back to test the commands I pasted in #33368 (comment) I found that it was still failing silently despite the functional test failing appropriately. I was able to tweak the functional test a bit and get it to also mimic the silent failing behavior in this branch: https://github.com/monlovesmango/bitcoin/tree/pr33528-functional-test-tweak I am still trying to figure out the root cause for why it no longer throws an exception when using send instead of sendall for the second tx, but wanted report back since you might have a much better idea of what is going on.

The other thing is that another scenario I had identified and was waiting report to see if first scenario was even an issue was when tx1 has more than one output and wallet tries to send more than one child there is also a silent failure. Seems like this could be fixed by checking that parent doesn’t already have a child already? Below is the commands for testing this scenario based on v30 Testing Guide setup.

0TRUC_address1=$(bcli30 getnewaddress)
1TRUC_address2=$(bcli30 getnewaddress)
2tx=$(bcli30 -named send outputs='{"'$TRUC_address1'": 1, "'$TRUC_address2'": 2}' fee_rate=25 version=3)
3TRUC_address3=$(bcli30 getnewaddress)
4TRUC_address4=$(bcli30 getnewaddress)
5tx2=$(bcli30 -named send outputs='{"'$TRUC_address3'": 0.5}' options='{"inputs":[{"txid":"'$(echo $tx|jq -r ".txid")'","vout":0}]}' fee_rate=25 version=3)
6tx3=$(bcli30 -named send outputs='{"'$TRUC_address4'": 0.5}' options='{"inputs":[{"txid":"'$(echo $tx|jq -r ".txid")'","vout":1}]}' fee_rate=25 version=3)
7bcli30 getrawtransaction  $(echo $tx2|jq -r ".txid") 1
8bcli30 getrawtransaction  $(echo $tx3|jq -r ".txid") 1

fanquake added this to the milestone 30.1 on Oct 8, 2025

glozow commented at 6:00 pm on October 8, 2025: member

When I went back to test the commands I pasted in #33368 (comment) I found that it was still failing silently despite the functional test failing appropriately.

Nice! This PR is limiting the UTXOs that show up when doing automatic coin selection, and thus errors with “insufficient funds” when it can’t find enough UTXOs for the tx. In your test/commands, you’re pre-selecting those inputs using send.

Btw, when I was testing send commands on regtest, I found that sometimes I accidentally succeeded because the wallet had other confirmed UTXOs to spend. You can sanity check that you’re indeed failing silently using with self.nodes[0].assert_debug_log(["Transaction cannot be broadcast immediately"]).

I think it would be reasonable to stop this transaction from being created and throw a “invalid parameters” error, telling the user they selected inputs that can’t be spent due to policy. Similar to here:

https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/src/wallet/rpc/spend.cpp#L1511-L1516

wallet: don’t consider unconfirmed TRUC coins with ancestors #33528

Code Coverage & Benchmarks

Reviews