wallet: re-activate "AmountWithFeeExceedsBalance" error #25269
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/25269. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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 5b6124d 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. |
d506b50 to
9adfbc0
Compare
9adfbc0 to
6c260d1
Compare
|
Looks like this works around a regression introduced by #20640 Might be better to make |
6c260d1 to
30971a0
Compare
|
Hey @luke-jr thanks for the review, was tackling other PRs before moving back to this one.
As we are now returning an 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 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. |
There was a problem hiding this comment.
Based on yesterday's wallet meeting, have been thinking about 032e544 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).
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. |
…error messages 76dc547 gui: create tx, launch error dialog if backend throws runtime_error (furszy) f4d7947 wallet: coin selection, add duplicated inputs checks (furszy) 0aa065b wallet: return accurate error messages from Coin Selection (furszy) 7e8340a wallet: make SelectCoins flow return util::Result (furszy) e5e147f wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy) Pull request description: Work decoupled from #25806, which cleanup and improves the Coin Selection flow further. Adding the capability to propagate specific error messages from the Coin Selection process to the user. Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally. Letting us instruct the user how to proceed under certain circumstances. The following error messages were added: 1) If the selection result exceeds the maximum transaction weight, we now will return: -> "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". 2) If the user pre-selected inputs and disallowed the automatic coin selection process (no other inputs are allowed), we now will return: -> "The preselected coins total amount does not cover the transaction target. Please allow other inputs to be automatically selected or include more coins manually". 3) The double-counted preset inputs during Coin Selection error will now throw an "internal bug detected" message instead of crashing the node. The essence of this work comes from several comments: 1. bitcoin/bitcoin#26560 (comment) 2. bitcoin/bitcoin#25729 (comment) 3. bitcoin/bitcoin#25269 (review) 4. bitcoin/bitcoin#23144 (which is connected to #24845) ACKs for top commit: ishaanam: crACK 76dc547 achow101: ACK 76dc547 aureleoules: ACK 76dc547 theStack: ACK 76dc547 🌇 Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
…ssages 76dc547 gui: create tx, launch error dialog if backend throws runtime_error (furszy) f4d7947 wallet: coin selection, add duplicated inputs checks (furszy) 0aa065b wallet: return accurate error messages from Coin Selection (furszy) 7e8340a wallet: make SelectCoins flow return util::Result (furszy) e5e147f wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy) Pull request description: Work decoupled from bitcoin#25806, which cleanup and improves the Coin Selection flow further. Adding the capability to propagate specific error messages from the Coin Selection process to the user. Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally. Letting us instruct the user how to proceed under certain circumstances. The following error messages were added: 1) If the selection result exceeds the maximum transaction weight, we now will return: -> "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". 2) If the user pre-selected inputs and disallowed the automatic coin selection process (no other inputs are allowed), we now will return: -> "The preselected coins total amount does not cover the transaction target. Please allow other inputs to be automatically selected or include more coins manually". 3) The double-counted preset inputs during Coin Selection error will now throw an "internal bug detected" message instead of crashing the node. The essence of this work comes from several comments: 1. bitcoin#26560 (comment) 2. bitcoin#25729 (comment) 3. bitcoin#25269 (review) 4. bitcoin#23144 (which is connected to bitcoin#24845) ACKs for top commit: ishaanam: crACK 76dc547 achow101: ACK 76dc547 aureleoules: ACK 76dc547 theStack: ACK 76dc547 🌇 Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
30971a0 to
900e5ed
Compare
|
Needs rebase, if still relevant |
900e5ed to
8cefff3
Compare
| if (!err.empty()) return util::Error{err}; | ||
|
|
||
| // Check if we have enough balance but cannot cover the fees | ||
| CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount(); |
There was a problem hiding this comment.
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
21dc635 to
5731295
Compare
|
Updated per feedback. Thanks murchandamus! |
5731295 to
e0673d5
Compare
…error This was previously implemented at the GUI level but has been broken since bitcoin#20640
e0673d5 to
98b7f10
Compare
| CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount(); | ||
| // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be | ||
| // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled. | ||
| if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) { |
There was a problem hiding this comment.
In 98b7f10 "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?
|
🐙 This pull request conflicts with the target branch and needs rebase. |
During a talk with theStack, it was noted that the
AmountWithFeeExceedsBalanceerror insideWalletModel::prepareTransactionis never thrown.This is because
createTransactiondoes 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 insideWalletModel::prepareTransactionto trigger theAmountWithFeeExceedsBalanceerror.This PR re-implements the feature inside
createTransactionand adds test coverage for it.