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

For detailed information about the code coverage, see the test coverage report.

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:

#26732 ([WIP] wallet: tx creation, don’t select outputs from txes that are being replaced by furszy)

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

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

900e5ed51b

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:1139 in 900e5ed51b

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

wallet: re-activate “AmountWithFeeExceedsBalance” error #25269

Code Coverage

Reviews

Conflicts