wallet: remove PreSelectedInputs and re-activate "AmountWithFeeExceedsBalance" error

stratospher commented at 9:06 am on January 15, 2026: contributor

picks up #25269.

This PR re-implements the code path so that an error message is thrown when a transaction’s total amount (including fees) exceeds the available balance. It also refactors the wallet’s coin selection code.

the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: make total_effective_amount optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure set<shared_ptr<COutput>> has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening CoinsResult.coins map to vector
the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the PreSelectedInputs struct with CoinsResult and removes PreSelectedInputs.
the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - AmountWithFeeExceedsBalance error inside WalletModel::prepareTransaction is never thrown and remains an unused code path. This is because createTransaction does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside WalletModel::prepareTransaction to trigger the AmountWithFeeExceedsBalance error.

This PR re-implements the feature inside CreateTransactionInternal and adds test coverage for it.

on master	on PR

the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.

DrahtBot added the label Wallet on Jan 15, 2026

DrahtBot commented at 9:06 am on January 15, 2026: 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/34299.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, achow101

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

furszy commented at 2:48 pm on January 15, 2026: member

Thanks for picking it up!

furszy commented at 2:16 am on January 21, 2026: member

q: in af3f3bfd57bf8869030153ecf3695f07ae7dd805, wouldn’t be better to replace PreSelectedInputs for CoinsResult? (and then remove PreSelectedInputs).

The outputs we add through FetchSelectedInputs can be of type UNKNOWN during the CoinsResult.Add call (their type doesn’t matter, they must be selected anyway).

davidgumberg commented at 5:26 am on January 23, 2026: contributor

reviewing https://github.com/bitcoin/bitcoin/pull/34299/commits/c467325aaf187d7f056bb1ea1cec6b7c4250af2e I wonder if it might be better to go the other direction and make effective amount be not optional, e.g.:

https://github.com/bitcoin/bitcoin/compare/master...davidgumberg:bitcoin:2026-01-22-effective-not-optional (https://github.com/bitcoin/bitcoin/commit/9deb85666db3d4c3e5b1406e6f22d10c8eb5c5c8)

It seems to me the only way for a COutput to be constructed without fees or a feerate is by calling AvailableCoins() with std::nullopt, which only happens when we are just listing coins e.g. listCoins for the GUI or the listunspent RPC, in which case what is the harm in setting effective_value = value?

Maybe best of both worlds is a bool flag in CoinsResult indicating whether or not it was feerate aware, seems clunkier on the surface, but in practice it would get set in one place (at construction) and get checked in ~one place (when being used for coin selection).

Maybe I’m overthinking this, and definitely that commit is correct as-is, feel free to disregard.

in test/functional/wallet_fundrawtransaction.py:1559 in f66d5bc099

1554+        # Set up wallet with 2 utxos: 0.3 BTC and 0.15 BTC
1555+        default_wallet.sendtoaddress(wallet.getnewaddress(), 0.3)
1556+        txid2 = default_wallet.sendtoaddress(wallet.getnewaddress(), 0.15)
1557+        self.generate(self.nodes[0], 1)
1558+        utxos = wallet.listunspent(1)
1559+        vout2 = next((u["vout"] for u in utxos if u["txid"] == txid2), 0)

achow101 commented at 10:58 pm on January 26, 2026:

In f66d5bc0990cc4f82c40aea3e13232fb6b883d31 “wallet: introduce “tx amount exceeds balance when fees are included” error”

Providing the default for next() shouldn’t be necessary.

This could probably be replaced by using utxo = wallet.listunspent()[0] instead of looking for the 0.15’s info. It doesn’t seem like that amount specifically is really relevant to the test, just that there is something being preselected.

stratospher commented at 12:00 pm on January 30, 2026:

you’re right! but I just added 2 additional test cases in the latest push (“Attempt to send 0.15 BTC using only the 0.15 BTC preselected utxo”) which needs that specific 0.15 utxo to be selected. 🫣 mostly to understand this behaviour better:

0assert(!preset_inputs.Size() || preset_inputs.GetEffectiveTotalAmount().has_value());
1assert(!available_coins.Size() || available_coins.GetEffectiveTotalAmount().has_value());

it didn’t really hit it though. so could remove the last 2 tests (it’s covered in other tests anyways). but felt like a logical continuation of the other tests, so maybe nice to have.

stratospher force-pushed on Jan 30, 2026

stratospher renamed this:
~~wallet: re-activate "AmountWithFeeExceedsBalance" error~~
wallet: remove PreSelectedInputs and re-activate "AmountWithFeeExceedsBalance" error
on Jan 30, 2026

stratospher commented at 11:46 am on January 30, 2026: contributor

q: in af3f3bf, wouldn’t be better to replace PreSelectedInputs for CoinsResult? (and then remove PreSelectedInputs). @furszy makes sense - I didn’t realise PreSelectedInputs was a slimmed down version of CoinsResult. So went with the minimal diff approach initially by tweaking PreSelectedInputs to have separate fields for total amount and effective amount. Makes sense to get rid of PreSelectedInputs and just use CoinsResult instead. I’ve done it in commit 21458f81283b11b57af34b9efecdbae0ddc7a539.

stratospher commented at 11:59 am on January 30, 2026: contributor

@davidgumberg I think this is a really thoughtful question. from what I understand so far:

Maybe best of both worlds is a bool flag in CoinsResult indicating whether or not it was feerate aware, seems clunkier on the surface, but in practice it would get set in one place (at construction) and get checked in ~one place (when being used for coin selection).

The 2 approaches are functionally equivalent:

bool feerate_aware = falseCAmount total_effective_amount = 0	->	std::optional<CAmount> total_effective_amount is UNSET
bool feerate_aware = trueCAmount total_effective_amount = xxx	->	std::optional<CAmount> total_effective_amount is SET

Both approaches seem to have their own strengths:

bool+ CAmount is more intuitive + easier to read for a human.
optional has less chance of errors + impossible to have inconsistent state - it tightly couples feerate_aware and total_effective_amount together so that they’re the same thing and you don’t forget to update one.

So seems to be a matter of taste. I’m ok with either - maybe optional is slightly better (though I agree it’s frustrating to think what the code should do when the optional is unset - and maybe that’s a good thing that we think..). Also looks like this was introduced in #25083.

DrahtBot added the label CI failed on Jan 30, 2026

DrahtBot commented at 12:52 pm on January 30, 2026: contributor

🚧 At least one of the CI tasks failed. Task Valgrind, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/21514518851/job/61989464750 LLM reason (✨ experimental): Fuzz target wallet_create_transaction failed due to an uninitialized value causing a conditional jump crash.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

in src/wallet/coinselection.h:327 in 2202ab5975

318@@ -319,11 +319,18 @@ enum class SelectionAlgorithm : uint8_t
319 
320 std::string GetAlgorithmName(const SelectionAlgorithm algo);
321 
322+struct COutputPtrComparator {
323+    bool operator()(const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) const {
324+        return *a < *b;
325+    }
326+};
327+using COutputSet = std::set<std::shared_ptr<COutput>, COutputPtrComparator>;

furszy commented at 3:13 pm on January 30, 2026:

just a general note, we no longer use the C prefix for classes or structs. That was something from the early days.

furszy commented at 3:16 pm on January 30, 2026: member

Cool update, thanks. The rabbit hole just keeps getting deeper.

stratospher force-pushed on Feb 2, 2026

in test/functional/wallet_fundrawtransaction.py:1583 in 2be0279a2a

1578+        wallet.fundrawtransaction(hexstring=rawtx, options={"subtractFeeFromOutputs":[0]})
1579+
1580+        self.log.info("Attempt to send 0.15 BTC using only the 0.15 BTC preselected utxo")
1581+        rawtx = wallet.createrawtransaction(inputs=[{"txid": txid2, "vout": vout2}], outputs=[{default_wallet.getnewaddress(): 0.15}])
1582+        assert_raises_rpc_error(-4, "The preselected coins total amount does not cover the transaction target. "
1583+                                             "Please allow other inputs to be automatically selected or include more coins manually",

achow101 commented at 8:57 pm on February 2, 2026:

In 2be0279a2ad1dd372b9df9500cd4b50ec8d27a04 “wallet: introduce “tx amount exceeds balance when fees are included” error”

Can use ERR_NOT_ENOUGH_PRESET_INPUTS for the error message rather than retyping the very long string.

stratospher force-pushed on Feb 3, 2026

stratospher commented at 11:26 am on February 3, 2026: contributor

I guess the CI failure is a valgrind false positive - #29635 on this line ( CAmount available_coins_total_amount = available_coins.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs).value_or(0);). not sure what to do about it.

ran the docker container locally but couldn’t reproduce it:

0root@4bb4a3dc3420:/bitcoin# ./build_fuzz/test/fuzz/test_runner.py qa-assets/fuzz_corpora/wallet_create_transaction wallet_create_transaction --valgrind
11 of 224 detected fuzz target(s) selected: wallet_create_transaction
2Fuzzing harnesses lacking a corpus: wallet_create_transaction
3Please consider adding a fuzz corpus at https://github.com/bitcoin-core/qa-assets
4Check if using libFuzzer ... True
5Summary:
6wallet_create_transaction [#2](/bitcoin-bitcoin/2/)	DONE   cov: 2268 ft: 2269 corp: 1/1b lim: 4 exec/s: 0 rss: 383Mb

achow101 commented at 10:18 pm on February 3, 2026: member

ACK f0c8b74d469acc808860912dbd48782afbdb9308

in src/wallet/interfaces.cpp:414 in b6c3012ac1

410@@ -411,12 +411,13 @@ class WalletImpl : public Wallet
411             CoinSelectionParams params(rng);
412             // Note: for now, swallow any error.
413             if (auto res = FetchSelectedInputs(*m_wallet, coin_control, params)) {
414-                total_amount += res->total_amount;
415+                total_amount += res->GetAppropriateTotal(params.m_subtract_fee_outputs).value_or(res->GetTotalAmount());

furszy commented at 1:40 am on February 4, 2026:

In b6c3012ac10c69bd77c62b2d8ef60ed0b943914d:

This method expects to use GetTotalAmount(), not the effective value. See that we provide an empty CoinSelectionParams, so the effective fee rate is always 0.

stratospher commented at 7:17 am on February 4, 2026:

oh nice catch! done.

in src/wallet/interfaces.cpp:420 in b6c3012ac1

416             }
417         }
418 
419         // And fetch the wallet available coins
420         if (coin_control.m_allow_other_inputs) {
421+            // we don't have fee details here - so there's no effective value possible.

furszy commented at 1:42 am on February 4, 2026:

In b6c3012ac10c69bd77c62b2d8ef60ed0b943914d: Can safely remove this comment, the entire method expects to use GetTotalAmount().

in src/wallet/spend.cpp:831 in b6c3012ac1

828     }
829 
830+    OutputSet preset_coin_set;
831+    for (const auto &output: pre_set_inputs.All()) {
832+            preset_coin_set.insert(std::make_shared<COutput>(output));
833+    }

furszy commented at 2:55 am on February 4, 2026:

In b6c3012ac10c69bd77c62b2d8ef60ed0b943914d: Extra tab here, and also it is const auto& output.

furszy commented at 3:02 am on February 4, 2026: member

Code reviewed, found a few tiny details. Otherwise looking good.

stratospher force-pushed on Feb 4, 2026

furszy commented at 7:09 pm on February 4, 2026: member

I think you can fix valgrind’s issue initializing the total_effective_amount:

 0diff --git a/src/wallet/spend.h b/src/wallet/spend.h
 1--- a/src/wallet/spend.h
 2+++ b/src/wallet/spend.h
 3@@ -69,7 +69,7 @@
 4     /** Sum of all available coins raw value */
 5     CAmount total_amount{0};
 6     /** Sum of all available coins effective value (each output value minus fees required to spend it) */
 7-    std::optional<CAmount> total_effective_amount;
 8+    std::optional<CAmount> total_effective_amount{std::nullopt};
 9 };
10 
11 struct CoinFilterParams {

(this is a guess but worth to try it)

stratospher force-pushed on Feb 5, 2026

maflcko commented at 7:37 am on February 5, 2026: member

I think you can fix valgrind’s issue initializing the total_effective_amount:

 0diff --git a/src/wallet/spend.h b/src/wallet/spend.h
 1--- a/src/wallet/spend.h
 2+++ b/src/wallet/spend.h
 3@@ -69,7 +69,7 @@
 4     /** Sum of all available coins raw value */
 5     CAmount total_amount{0};
 6     /** Sum of all available coins effective value (each output value minus fees required to spend it) */
 7-    std::optional<CAmount> total_effective_amount;
 8+    std::optional<CAmount> total_effective_amount{std::nullopt};
 9 };
10 
11 struct CoinFilterParams {

(this is a guess but worth to try it)

I’d be surprised if this worked around the issue. It is triggered in an GCC -O2 pass, so it seems more involved. This false-positive should be properly reduced and reported upstream. However, this is going to take some time (not even considering the time for the upstream fix to propagate). I am happy to take care of this, but I don’t think it makes sense to block pull requests based on unrelated upstream bugs. I’d say the task should be (temporarily?) removed, see #34492 for now.

furszy commented at 3:38 pm on February 5, 2026: member

I’d be surprised if this worked around the issue. It is triggered in an GCC -O2 pass, so it seems more involved. This false-positive should be properly reduced and reported upstream. However, this is going to take some time (not even considering the time for the upstream fix to propagate). I am happy to take care of this, but I don’t think it makes sense to block pull requests based on unrelated upstream bugs. I’d say the task should be (temporarily?) removed, see #34492 for now.

I wouldn’t argue against removing it, but also wouldn’t argue against people who want to keep it. Just thinking that there might be a workaround that doesn’t involve much code so everyone is happy and we can upstream this bug without any rush. E.g..

0if (coin_selection_params.m_subtract_fee_outputs) {
1    available_coins_total_amount = available_coins.GetTotalAmount();
2} else if (auto op_eff_amount = available_coins.GetEffectiveTotalAmount(); op_eff_amount.has_value()) {
3    available_coins_total_amount = *op_eff_amount;
4}

furszy commented at 8:25 pm on February 5, 2026: member

ok yes, with the change I pasted above (https://github.com/bitcoin/bitcoin/pull/34299#issuecomment-3854457310), valgrind job passes: https://github.com/furszy/bitcoin-core/actions/runs/21726050704/job/62668278343

so anyway, I would be fine either way. Just don’t see why we should wait until there is an agreement over removing or not the CI job. We can move forward here with a simple workaround.

achow101 commented at 9:35 pm on February 5, 2026: member

ACK 3d31717a4b6cf1601524c40b24581662dcdfd535

wallet: fix, make 'total_effective_amount' optional actually optional

this is not needed for the remaining commits but good to fix
and came up in #25269 review.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

fefa3be782

wallet: ensure COutput added in set are unique

before #25806, set<COutput> was used and would not
contain same COutputs in the set.

now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.

so preserve previous behaviour by making sure values
in the set are also distinct

7072d825e3

wallet: correctly reserve in CoinsResult::All()

coins.size() would be the number of the OutputType keys in the map.
whereas Size() would return total number of COutput objects when
flattening the map.

d8ea921d01

wallet: introduce GetAppropriateTotal() in CoinsResult

returns the total amount (if SFFO), otherwise the effective amount.
previously, this was the logic in calculating
PreSelectedInputs::total_amount when PreSelectedInputs::Insert()
was called.

return optional to force callers to explicitly handle the case
when effective amount optional is not set.

e5474079f1

stratospher force-pushed on Feb 6, 2026

maflcko commented at 10:32 am on February 6, 2026: member

The valgrind fuzz still fails after the latest push (https://github.com/bitcoin/bitcoin/actions/runs/21738372240/job/62708257754?pr=34299), fwiw. At some point there is a risk of introducing bugs while refactoring when chasing false-positives.

I’ll close and open the pull to rebase it.

maflcko closed this on Feb 6, 2026

maflcko reopened this on Feb 6, 2026

stratospher commented at 10:49 am on February 6, 2026: contributor

sorry I actually tried a variation of furszy’s suggestion which didn’t work. but thanks! would be nice to use a 1-liner.

this variation failed on valgrind - so optional + value_or wasn’t the problem.

0  CAmount available_coins_total_amount = 0;
1    if (auto total = available_coins.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs); total.has_value()) {
2        available_coins_total_amount = *total;
3    }

apparently optional + ternary was the problem for valgrind. (tried this on my personal fork and it worked)

0    std::optional<CAmount> GetAppropriateTotal(bool subtract_fee_outputs) const {
1        if (subtract_fee_outputs) {
2            std::optional<CAmount> result = total_amount;
3            return result;
4        } else {
5            std::optional<CAmount> result = total_effective_amount;
6            return result;
7        }
8    }

walllet: use CoinsResult instead of PreSelectedInputs

PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.

CoinsResult has a cleaner interface with separate fields
for both these amounts.

2 behavioural changes:

1. no more default assert error if effective value is unset
    - previously PreSelectedInputs::Insert() called
      COutput::GetEffectiveValue() which assert failed
      if the optional was unset.
    - now we don't default assert anymore.
      * in GUI/getAvailableBalance better not to assert.
      * SelectCoins's preselected inputs always contain a
        feerate, so effective amount should be set.
        explicitly added an assertion to ensure this.

2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
   populate CoinsResult's coins map. it's discarded later.

7819da2c16

wallet: remove PreSelectedInputs b7fa609ed1

wallet: introduce "tx amount exceeds balance when fees are included" error

This was previously implemented at the GUI level but we never hit that
code path.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

48161f6a05

stratospher force-pushed on Feb 6, 2026

DrahtBot removed the label CI failed on Feb 6, 2026

furszy commented at 2:35 pm on February 6, 2026: member

utACK 48161f6

DrahtBot requested review from achow101 on Feb 6, 2026

achow101 commented at 10:23 pm on February 6, 2026: member

ACK 48161f6a0503d7dde693ef544f0d3285c8b93adc

achow101 merged this on Feb 6, 2026

achow101 closed this on Feb 6, 2026

stratospher deleted the branch on Feb 7, 2026

wallet: remove PreSelectedInputs and re-activate “AmountWithFeeExceedsBalance” error #34299

Code Coverage & Benchmarks

Reviews