wallet: re-activate "AmountWithFeeExceedsBalance" error

furszy commented at 2:17 pm on June 2, 2022: member

During a talk with theStack, it was noted that the AmountWithFeeExceedsBalance error inside WalletModel::prepareTransaction is never thrown.

This is because createTransaction does not retrieve the fee when the process fails due to insufficient funds since #20640. 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 createTransaction and adds test coverage for it.

furszy renamed this:
~~wallet: re-activate the not triggered "AmountWithFeeExceedsBalance"~~
wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error
on Jun 2, 2022

laanwj added the label Wallet on Jun 2, 2022

DrahtBot commented at 4:36 pm on June 2, 2022: 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/25269.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	murchandamus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32618 (wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs by achow101)
#32523 (wallet: Remove watchonly behavior and isminetypes by achow101)

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.

DrahtBot added the label Needs rebase on Jun 17, 2022

luke-jr commented at 2:50 am on June 18, 2022: member

Seems like an awful lot of refactoring and optimisations tied into what is in essence a bugfix.

Maybe split them up?

furszy commented at 1:04 pm on June 18, 2022: member

Seems like an awful lot of refactoring and optimisations tied into what is in essence a bugfix. Maybe split them up?

Hey Luke, only the last three commits count for this work. It’s mentioned in the PR description to not generate confusion: This was built on top of #25005 because needed the structure introduced in 5b6124da to implement this flow properly. (where “properly” means not doing an ugly workaround and be able to place the new code on top of a cleaner structure)

Now that #25005 got merged, will rebase it and get rid of all the extra commits.

furszy force-pushed on Jun 18, 2022

bitcoin deleted a comment on Jun 18, 2022

DrahtBot removed the label Needs rebase on Jun 18, 2022

furszy force-pushed on Jun 18, 2022

luke-jr commented at 3:06 pm on June 18, 2022: member

Looks like this works around a regression introduced by #20640

Might be better to make CreateTransaction return a (non-optional) CreatedTransactionResult instead?

DrahtBot added the label Needs rebase on Jul 12, 2022

furszy force-pushed on Jul 15, 2022

furszy commented at 10:40 pm on July 15, 2022: member

Hey @luke-jr thanks for the review, was tackling other PRs before moving back to this one.

Looks like this works around a regression introduced by #20640

Might be better to make CreateTransaction return a (non-optional) CreatedTransactionResult instead?

As we are now returning an std::variant wrapper, that change might not worth it (it would mean re-introduce the string error ref argument and change all the function callers, and the many return statements of CreateTransaction and CreateTransactionInternal).

A possible elegant solution going into the “always return information” path would be to return an object that wraps the transaction creation process information (with the fee, the used coin selection algorithm, change output, etc..) in the error field of the returned std::variant as well. But.. for that, we need #25601 which introduces the generic error functionality.

But aside from that (which I think that would be a good long term goal), maybe might worth to continue with the PR as is now, primarily because it’s unifying the errors that we throw on the GUI and on the RPC server.

Still, have to say that I would like to be able to move this kind of errors from the wallet sources to a class that lives on an upper layer like the wallet interface. So GUI and RPC receive the same errors and the wallet isn’t in charge of describe this type of errors for the user.

DrahtBot removed the label Needs rebase on Jul 15, 2022

DrahtBot added the label Needs rebase on Aug 5, 2022

furszy commented at 4:27 pm on October 8, 2022: member

Based on yesterday’s wallet meeting, have been thinking about 032e5449 further and the possibility to move the error to SelectCoins instead of placing it inside CreateTransaction.

The commit, as is now, it does not cover the “fee exceeds balance” error in a 100%. It’s only covering the “fee exceeds balance” if the selection fails for the not-inputs fees, it is not contemplating the inputs fees (this is not different to what we were doing before #20640).

So, need to move the error to SelectCoins, and add a mechanism to keep track of the inputs fee total amount (data that we don’t have in CreateTransaction if SelectCoins fail).

But.. ideally, before implementing this changes, would love to get #25806 merged as it simplifies the whole coin selection process greatly (which depends on #25685, which is ready to go).

bitcoin deleted a comment on Oct 10, 2022

fanquake commented at 5:10 pm on December 6, 2022: member

But.. ideally, before implementing this changes, would love to get #25806 merged (which depends on #25685, which is ready to go).

25685 has been merged, and it looks like 25806 has just been rebased. How about marking this PR draft for now, given the dependency on 25806, as well as adding a note at the top of the PR description, indicating to reviewers that they should review 25806 first.

furszy marked this as a draft on Dec 23, 2022

achow101 referenced this in commit 3f8591d46b on Jan 3, 2023

sidhujag referenced this in commit cab438784e on Jan 4, 2023

furszy closed this on Jun 22, 2023

furszy reopened this on May 21, 2024

furszy force-pushed on May 21, 2024

furszy marked this as ready for review on May 21, 2024

DrahtBot removed the label Needs rebase on May 21, 2024

furszy renamed this:
~~wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error~~
wallet: re-activate "AmountWithFeeExceedsBalance" error
on May 22, 2024

in src/wallet/spend.cpp:1168 in 900e5ed51b outdated

1135+        if (!err.empty()) return util::Error{err};
1136+
1137+        // Check if we have enough balance but cannot cover the fees
1138+        CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
1139+        if (available_balance >= recipients_sum) {
1140+            CAmount available_effective_balance = preset_inputs.total_amount + available_coins.GetEffectiveTotalAmount().value_or(available_coins.GetTotalAmount());

murchandamus commented at 5:43 pm on June 5, 2024:

It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the preset_inputs not also be reduced to their effective amount in line 1139?

furszy commented at 7:04 pm on June 5, 2024:

It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the preset_inputs not also be reduced to their effective amount in line 1139?

Thats because preset_inputs.total_amount could either be the full amount or the effective one (see PreSelectedInputs). I didn’t want to introduce more changes within this PR but we should split the amounts (probably replacing PreSelectedinputs usage for CoinsResult).

murchandamus commented at 5:43 pm on June 5, 2024: contributor

Concept ACK

achow101 requested review from achow101 on Oct 15, 2024

achow101 requested review from theStack on Oct 15, 2024

DrahtBot added the label CI failed on Mar 14, 2025

DrahtBot commented at 11:32 am on March 15, 2025: contributor

Needs rebase, if still relevant

furszy force-pushed on Mar 15, 2025

DrahtBot removed the label CI failed on Mar 15, 2025

in src/wallet/spend.cpp:1166 in 8cefff322c outdated

1160@@ -1161,7 +1161,19 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1161     if (!select_coins_res) {
1162         // 'SelectCoins' either returns a specific error message or, if empty, means a general "Insufficient funds".
1163         const bilingual_str& err = util::ErrorString(select_coins_res);
1164-        return util::Error{err.empty() ?_("Insufficient funds") : err};
1165+        if (!err.empty()) return util::Error{err};
1166+
1167+        // Check if we have enough balance but cannot cover the fees
1168+        CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();

murchandamus commented at 11:22 pm on May 12, 2025:

Note to reviewers: preset_inputs.total_amount will be the sum of the outputs’ effective values if “subtract fee from outputs” is disabled, but will be the nominal amount if SFFO is active. See src/wallet/spend.h:164–172

in src/wallet/spend.cpp:1168 in 8cefff322c outdated

1165+        if (!err.empty()) return util::Error{err};
1166+
1167+        // Check if we have enough balance but cannot cover the fees
1168+        CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
1169+        if (available_balance >= recipients_sum) {
1170+            CAmount available_effective_balance = preset_inputs.total_amount + available_coins.GetEffectiveTotalAmount().value_or(available_coins.GetTotalAmount());

murchandamus commented at 11:51 pm on May 12, 2025:

I’m wondering whether there is a bug here. It seems to me that this is assuming that the effective value is only available when SFFO is not being used, but COutput.effective_value is set and aggregated whenever the COutputs have a feerate. Does this not explicitly need to handle SFFO? I.e., maybe this needs to be something along the lines of:

0            CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : available_coins.GetEffectiveTotalAmount();
1            CAmount available_effective_balance = preset_inputs.total_amount +available_coins_total_amount;

furszy commented at 10:31 am on May 26, 2025:

good catch!, fixed. This also revealed that the optional field CoinsResult::total_effective_amount has never being optional (it was being set to 0 during construction).

achow101 commented at 11:45 pm on May 29, 2025:

Since fees are not included when SFFO, I don’t think this is reachable as should available_balance is always greater than recipients_sum in that case, so I don’t think this needs any special SFFO treatment. Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return “Insufficent Funds”, so this whole check could probably be guarded by an if (m_subtract_fee_outputs) to skip it when SFFO is on.

furszy commented at 12:00 pm on May 30, 2025:

ha.. true. It seems I’m not putting much brain power here.. updated per feedback. Thanks.

ryanofsky commented at 7:40 pm on October 27, 2025:

re: #25269 (review)

Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return “Insufficent Funds”, so this whole check could probably be guarded by an if (m_subtract_fee_outputs) to skip it when SFFO is on.

I don’t think I follow the logic behind this but it’s possible I’m misunderstanding. If user is trying to send less money than their balance, and it fails because fees are too high it would seem like an “exceeds your balance when fees are included” message (especially one that specifies what fee the wallet was targeting) would be clearer than an “insufficient funds” message. It seems like should be true independently of whether the sender or recipient is paying the fees.

So I guess I don’t understand the reason for a m_subtract_fee_outputs special case here, and it would seem simpler and better to drop. I do that see in practice that effective values are ignored when SFFO is used, so it should be safe to Assume(!coin_selection_params.m_subtract_fee_outputs) here if no other code changes, but this would seem like unnecessary assumption and not something this code needs to concern itself with.

ryanofsky commented at 11:36 pm on October 27, 2025:

re: #25269 (review)

Thinking about this more I can see why SFFO should make the “amount exceeds your balance when fees are included” condition harder to trigger than when SFFO is not used, because when SFFO is enabled the wallet is able to reduce the amounts recipients receive to cover any fees, whereas without SFFO the amount received needs to be fixed. But I’d still expect the (available_balance >= recipients_sum && available_effective_balance < selection_target) condition to be able to trigger when SFFO is used if there are coins are which are not economical to send. Also even if there is other code making this condition difficult or impossible to trigger when SFFO is used, I still don’t see a reason this code should be checking for SFFO specially. Even with SFFO, an error message that mentions fees and includes the attempted fee would still be more descriptive and helpful than the generic “insufficient funds” message.

ryanofsky commented at 1:31 am on October 28, 2025:

re: #25269 (review)

Update: think I finally figured this out. In the SFFO case the fee exceeds balance error can still happen but the available_balance >= recipients_sum condition will not detect it. So I think the change I would suggest would just be dropping the m_subtract_fee_outputs code and writing a more descriptive comment:

 0--- a/src/wallet/spend.cpp
 1+++ b/src/wallet/spend.cpp
 2@@ -1164,12 +1164,19 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 3 
 4         // Check if we have enough balance but cannot cover the fees
 5         CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
 6-        // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be
 7-        // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled.
 8-        if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) {
 9+        if (available_balance >= recipients_sum) {
10             CAmount available_effective_balance = preset_inputs.total_amount + *available_coins.GetEffectiveTotalAmount();
11             if (available_effective_balance < selection_target) {
12-                return util::Error{_("The total transaction amount exceeds your balance when fees are included")};
13+                // Note: this fees exceeds balance error will not currently
14+                // trigger when SFFO (m_subtract_fee_outputs) is enabled because
15+                // the effective balance will equal the original balance in this
16+                // case. This is not ideal because it fails to provide the most
17+                // descriptive error message, but it's not worth making this
18+                // code more complex by checking the sizes of outputs fees can
19+                // be subtracted from. Additionally, fees exceeding the balance
20+                // should happen more rarely in the SFFO case given the ability
21+                // to take fees from outputs.
22+                return util::Error{strprintf(_("The total exceeds your balance when the %s transaction fee is included."), FormatMoney(selection_target - recipients_sum))};
23             }
24         }
25

in test/functional/wallet_fundrawtransaction.py:1532 in 8cefff322c outdated

1533+        self.nodes[0].sendtoaddress(wallet.getnewaddress(), 0.3)
1534+        self.generate(self.nodes[0], 1)
1535+
1536+        rawtx = wallet.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(): 0.3}])
1537+        expected_err_msg = "The total transaction amount exceeds your balance when fees are included"
1538+        assert_raises_rpc_error(-4, expected_err_msg, wallet.fundrawtransaction, rawtx)

murchandamus commented at 11:52 pm on May 12, 2025:

Could this test case be extended by attempting the same transaction with SFFO and showing that it succeeds?

furszy commented at 10:32 am on May 26, 2025:

sure, added.

murchandamus commented at 11:53 pm on May 12, 2025: contributor

I’m wondering whether this is correct when SFFO is active.

wallet: fix, make 'total_effective_amount' optional actually optional 568753018c

furszy force-pushed on May 26, 2025

furszy commented at 10:33 am on May 26, 2025: member

Updated per feedback. Thanks murchandamus!

furszy force-pushed on May 30, 2025

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

This was previously implemented at the GUI level but has been broken since #20640

98b7f1061f

furszy force-pushed on May 30, 2025

in src/wallet/spend.cpp:1169 in 98b7f1061f

1165+
1166+        // Check if we have enough balance but cannot cover the fees
1167+        CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
1168+        // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be
1169+        // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled.
1170+        if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) {

achow101 commented at 0:17 am on June 12, 2025:

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

I’m pretty sure that !coin_selection_params.m_subtract_fee_outputs is not necessary as it should not be possible for SelectCoins to fail but available_balance >= recipients_sum with SFFO.

Perhaps this could be made into an Assume inside the if?

DrahtBot commented at 9:38 pm on July 7, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jul 7, 2025

DrahtBot commented at 2:21 am on October 4, 2025: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

achow101 requested review from achow101 on Oct 22, 2025

achow101 commented at 3:10 pm on October 22, 2025: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

Marking as up for grabs.

achow101 closed this on Oct 22, 2025

achow101 added the label Up for grabs on Oct 22, 2025

in src/wallet/spend.h:68 in 568753018c outdated

65 private:
66     /** Sum of all available coins raw value */
67     CAmount total_amount{0};
68     /** Sum of all available coins effective value (each output value minus fees required to spend it) */
69-    std::optional<CAmount> total_effective_amount{0};
70+    std::optional<CAmount> total_effective_amount;

ryanofsky commented at 6:26 pm on October 27, 2025:

In commit “wallet: fix, make ’total_effective_amount’ optional actually optional” (568753018c5ffcddf4612464cd53502fa559f6fa)

I wonder if this change requires updates to any code that might be assuming total_effective_amount is not null like: https://github.com/bitcoin/bitcoin/blob/56e9703968e26353fd4663e07a7bba198a272d12/src/wallet/spend.cpp#L226

Also it’s not really clear what the connection is between this change in the rest of the PR. It would help to mention some motivation in the commit description.

in src/wallet/spend.cpp:1172 in 98b7f1061f

1168+        // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be
1169+        // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled.
1170+        if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) {
1171+            CAmount available_effective_balance = preset_inputs.total_amount + *available_coins.GetEffectiveTotalAmount();
1172+            if (available_effective_balance < selection_target) {
1173+                return util::Error{_("The total transaction amount exceeds your balance when fees are included")};

ryanofsky commented at 6:40 pm on October 27, 2025:

In commit “wallet: introduce “tx amount exceeds balance when fees are included” error” (98b7f1061ff104cdffebe1c2968c9f829d1a72f9)

Can this error message include amount of fees that was in the original error message. It seems like knowing how much the wallet is trying to spend could help the user address the problem. It looks like equivalent amount would be selection_target - recipient_sum here.

in src/wallet/spend.cpp:1170 in 98b7f1061f

1166+        // Check if we have enough balance but cannot cover the fees
1167+        CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
1168+        // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be
1169+        // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled.
1170+        if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) {
1171+            CAmount available_effective_balance = preset_inputs.total_amount + *available_coins.GetEffectiveTotalAmount();

ryanofsky commented at 7:28 pm on October 27, 2025:

In commit “wallet: introduce “tx amount exceeds balance when fees are included” error” (98b7f1061ff104cdffebe1c2968c9f829d1a72f9)

Is it safe to assume GetEffectiveTotalAmount() returns not null here after previous commit? Might be better to use value_or

ryanofsky commented at 8:01 pm on October 27, 2025: contributor

Code review ACK 98b7f1061ff104cdffebe1c2968c9f829d1a72f9 for the second commit, but I am unsure about the first commit (see below). I started reviewing this because https://github.com/bitcoin-core/gui/pull/807 depends on it, but this seems like a nice change independently that provides a clearer error message and fixes a GUI feature that was accidentally broken in #20640.

wallet: re-activate “AmountWithFeeExceedsBalance” error #25269

Code Coverage & Benchmarks

Reviews

Conflicts