wallet: fix crash in coin selection with enormous unconfirmed clusters #34962

pull cprkrn wants to merge 1 commits into bitcoin:master from cprkrn:test-feebumper-enormous-cluster changing 2 files +107 −3

cprkrn commented at 3:57 AM on March 31, 2026: none
calculateIndividualBumpFees returns an empty map when GatherClusters aggregates >500 txs across input clusters. FetchSelectedInputs and AvailableCoins then crash on map_of_bump_fees.at().

Fix:
- FetchSelectedInputs: return a descriptive error when pre-selected inputs span enormous clusters
- AvailableCoins: drop unconfirmed outputs and fall back to confirmed inputs
Same class of bug fixed in #34870 for calculateCombinedBumpFee, but in the coin selection path. Also addresses #29711 (wallet crash with >500 unconfirmed txs).
DrahtBot added the label Wallet on Mar 31, 2026
DrahtBot commented at 3:57 AM on March 31, 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. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #34698 (wallet: handle MiniMiner bump fee calculation failures by shuv-amp)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
cprkrn commented at 4:00 AM on March 31, 2026: none

@furszy this is the same issue you fixed in #34870 but in the coin selection path (FetchSelectedInputs/AvailableCoins). Found it while writing a functional test for #34902.
cprkrn force-pushed on Mar 31, 2026
cprkrn force-pushed on Mar 31, 2026

in test/functional/wallet_bumpfee.py:870 in c6198f4bda outdated

 865 | +
 866 | +    unconfirmed_utxos = w.listunspent(minconf=0, maxconf=0, addresses=[wallet_address], include_unsafe=True)
 867 | +    assert_equal(len(unconfirmed_utxos), NUM_CLUSTERS)
 868 | +    inputs = [{"txid": u["txid"], "vout": u["vout"]} for u in unconfirmed_utxos]
 869 | +
 870 | +    # This should error gracefully, not crash with map::at

instagibbs commented at 12:17 PM on March 31, 2026:

no need to give implementation details for the test, higher level reason for the test suffices

instagibbs commented at 12:25 PM on March 31, 2026: member

do we want to remove the limit instead? IIUC the limit was used for mempool reasons prior, but now the only consumer is the wallet. Is there a problem returning >500 for the wallet?

edit: oh maybe it was only used for that since at least 29.0, ignore me
cprkrn commented at 3:08 PM on March 31, 2026: none

It's redundant with cluster mempool capping at 64 tx/cluster. Could either raise or remove it.
DrahtBot added the label CI failed on Mar 31, 2026
achow101 commented at 11:17 PM on March 31, 2026: member

Each commit needs to compile and pass tests on its own. The first commit introduces a test that fails.

wallet: fix crash in coin selection when clusters exceed GatherClusters limit

calculateIndividualBumpFees returns an empty map when MiniMiner can't
calculate (m_ready_to_calculate is false due to GatherClusters finding
>500 txs). FetchSelectedInputs and AvailableCoins then crash on
map_of_bump_fees.at() with an unhandled key-not-found exception.

FetchSelectedInputs: return a descriptive error when the map is empty.
AvailableCoins: drop unconfirmed outputs and fall back to confirmed
inputs, so the wallet can still create transactions.

Same class of bug fixed for calculateCombinedBumpFee in #34870.

Refs #34902, #29711

b5dbe4abdc

cprkrn force-pushed on Apr 1, 2026
DrahtBot removed the label CI failed on Apr 3, 2026
DrahtBot added the label CI failed on Apr 4, 2026
DrahtBot removed the label CI failed on Apr 7, 2026
murchandamus commented at 8:59 PM on April 16, 2026: member

Thanks for looking into this!

Falling back to only considering confirmed UTXOs sounds like the right approach for general Coin Selection questions, but aren’t we here looking at a scenario where the user has manually picked specific UTXOs to be used (CoinControl)?

do we want to remove the limit instead? IIUC the limit was used for mempool reasons prior, but now the only consumer is the wallet. Is there a problem returning >500 for the wallet?

It's redundant with cluster mempool capping at 64 tx/cluster. Could either raise or remove it.

That sounds problematic to me. The call can pass several unrelated inputs, which then in sum could be related to more than 64 transactions. In the case of the inputs being provided by Coin Control all gathered transactions would become part of a single cluster when we create the requested transaction, and the transaction would not propagate among nodes with Cluster Mempool.

Perhaps in the case of manually selected inputs, the limit should actually be reduced to throw an error if gathering more than 63 transactions?

Contributors

Labels

Linked (view graph)

#29711 Wallet: Nondescript error message for 502nd unconfirmed transaction #34870 wallet: feebumper, fix crash when combined bump fee is unavailable

wallet: fix crash in coin selection with enormous unconfirmed clusters #34962

Reviews

Conflicts