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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
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:1169
in
900e5ed51boutdated
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?
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
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
8cefff322c
furszy force-pushed
on Mar 15, 2025
DrahtBot removed the label
CI failed
on Mar 15, 2025
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-04-05 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me