Fixes #23144.
We currently return a general “Insufficient funds” from Coin Selection when we actually skipped unconfirmed UTXOs that surpassed the mempool ancestors limit.
This PR make the error clearer by returning: “Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool”
Also, added an early return from Coin Selection if the sum of the discarded coins decreases the available balance below the target amount.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See 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.
Reviewers, 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.
39+ CallResult(const bilingual_str& error) : OperationResult(false, error) { }
40+ const std::optional<T>& getObjResult() const { return m_obj_res; }
41+};
42+
43+
44+#endif // BITCOIN_OPERATIONRESULT_H
In 7c9839d3a8aa3bbd0fc9ae46a726022cdb661fc8 “introduce OperationResult and CallResult classes”
nit: missing newline
17+ OperationResult(bool _res, const bilingual_str& _error) : m_res(_res), m_error(_error) { }
18+ OperationResult(bool _res) : m_res(_res) { }
19+
20+ bilingual_str getError() const { return (m_error ? *m_error : bilingual_str()); }
21+ bool getRes() const { return m_res; }
22+ explicit operator bool() const { return m_res; }
In 7c9839d3a8aa3bbd0fc9ae46a726022cdb661fc8 “introduce OperationResult and CallResult classes”
nit: We prefer to use UpperCamelCase for function names. (here and elsewhere)
575+ return res;
576+ }
577
578 // Add preset inputs to result
579- res->AddInput(preset_inputs);
580+ auto resMerged = *res.getObjResult();
In c2ec6661cbb70a388dc67d6b4b47f2ab49b99418 “wallet: make SelectCoins return a CallResult.”
nit: We prefer snake_case for new variables. (here and elsewhere)
563- vCoins, coin_selection_params)}) {
564- return r8;
565+ if (auto r8{AttemptSelection(wallet, value_to_select,
566+ CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::max(), true /* include_partial_groups */),
567+ vCoins, coin_selection_params)}) {
568+ if (fRejectLongChains) {
In c8a43eca7c4081f8ca87be1befbbcdbfdc798a10 “send: return proper error message for too-long-mempool-chain failure.”
I’m not a huge fan of doing all of the coin selection computation just to immediately throw away the result. It seems like there should be a better way to do this, e.g. by changing how we fill vCoins to be guaranteed to have a solution at this point iff long chain utxos are included.
650@@ -651,13 +651,12 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
651 }
652 }
653
654-static bool CreateTransactionInternal(
655+static OperationResult CreateTransactionInternal(
In 30feca7b374256ff62443b2e2f1c65dfa49bb083 “send: move wallet CreateTransaction* flow to use OperationResult/CallResult.”
I don’t think this needs to be an OperationResult. It really can be CallResult<CTransactionRef>. The original really should have just returned CTransactionRef anyways.
Hey, thanks for the review @achow101.
The last 2 commits should be dropped from this PR and a new PR for each one opened. They are orthogonal to the main changes here.
Sounds good, will move them to their own PRs. I added them here basically because I wasn’t totally happy with the extra wasted computation neither, so.. thought on other ways to speed up the process.
Will tackle the code styling changes as well 👍.
89+ const CCoinControl* coinControl,
90+ const CAmount& nMinimumAmount,
91+ const CAmount& nMaximumAmount,
92+ const CAmount& nMinimumSumAmount,
93+ const uint64_t nMaximumCount,
94+ std::vector<COutput>* vUnconfCoins)
In b2846445135f6d6b7902672cb2202dc75088a200 “wallet: add CoinControl filtering capabilities for coins that exceed mempool restrictions.”
nit: For new variables, we prefer snake_case, without the prefix for type. So this could be just unconf_coins.
kk, done both. You caught me doing some further changes.
Have re-coded the mempool ancestors/descendants cache differently, much cleaner and less intrusive. It’s now integrated into the available coin information (the changes are quite small now, it shouldn’t require another PR for it but let me know what you think achow101).
Plus cleaned the CreateTransaction changes a bit more.
Aside from achow101 review changes, the first part:
Have re-coded the mempool ancestors/descendants cache differently, much cleaner and less intrusive. It’s now integrated into the available coin information
Was the move from fbc130aa to d9b8ea6 because now the ancestors and descendants count is available inside COutput. Which simplified the changes greatly.
And the second part was an unification for 76aa7d05 of the previous two emplace_back calls into a single one inside AvailableCoins and moving the ancestors/descendants getter call after the basic checks.
5+#ifndef BITCOIN_OPERATIONRESULT_H
6+#define BITCOIN_OPERATIONRESULT_H
7+
8+#include <util/translation.h>
9+
10+class OperationResult
In f198d27ef7f80b6b1c03f388eae216402f1ff424 “introduce OperationResult and CallResult classes”
Since OperationResult is only used by CallResult now, what do you think about consolidating the two classes?
I left it because we can make use of it in other functions that only return a bool and the error string without the extra object. Like for example CConnman::BindListenPort, Chain::broadcastTransaction and CWallet::SubmitTxMemoryPoolAndRelay, BerkeleyEnvironment::Open, BerkeleyDatabase::Verify etc.
I mean, having the class available would let others use it on different PRs if they are touching any similar function signature.
Saying that, if you are still against merging it without a connected use-case, can just remove it and re-introduce it later when we modify any of those functions as well (or.. make an extra small commit that connects it to one of those functions just to present the use-case for it).
20+ bilingual_str GetError() const { return (m_error ? *m_error : bilingual_str()); }
21+ bool GetRes() const { return m_res; }
22+ explicit operator bool() const { return m_res; }
23+};
24+
25+inline OperationResult ErrorOut(const bilingual_str& error_str)
In f198d27ef7f80b6b1c03f388eae216402f1ff424 “introduce OperationResult and CallResult classes”
This function is no longer used.
yeah.. have been thinking about it too.
The initial proposed solution was simple but came with an extra AttemptSelection call.
The second one introduced the coin control unconfirmed UTXO filtering functionality, which got us two good performance improvement: -> We no longer loop, several times, over coins that will be reject by the mempool restrictions during the whole selection process. -> And we no longer lock the mempool mutex two times per available UTXO for each of the many ‘AttemptSelection/GroupOutputs’ calls to get the ancestors and descendants count. Now is done only once per unconfirmed tx in the mempool.
But.. introduced that ugly extra uncof_coins vector to AvailableCoins.
Now.. next step forward:
has_long_chain_of_unconf which can be used to notify the user about the existence of a long chain of unconfirmed transactions.
Then he/she can know exactly how much unconfirmed balance has calling getbalances after receiving the error.Then.. the last commit is just to make the change look nicer. AvailableCoins now will return a CoinsResult struct instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway).
The last 2 commits should be dropped from this PR and a new PR for each one opened. They are orthogonal to the main changes here.
Just dropped the only_spendable CoinControl filter option commit. Will open a new PR for it next week.
Some extra notes to check this week:
SelectCoins to AvailableCoins (independently from the extra locking improvements), need to check whether that has any impact on the other AvailableCoins function callers (not only the CreateTransaction* flow that already know that is fine). Because now they can be locking the mempool cs when before they were not doing it. So.. in other words, sanity check over the mutexes locking order for callers that aren’t part of the flow being changed here.Then another possible path that I’m thinking if there is not enough quorum over the changes here is move the new mempool info cache and coin control filtering capabilities to a follow-up PR and add here a straightforward, but slower and redundant, approach like:
Loop over the available coins inside CreateTransactionInternal if the SelectCoins call fails to basically get the mempool info on-demand and check if there is any long-chain coin before returning the generic “Insufficient funds” error.
Which will correct the base problem that this PR is aiming to fix without adding the other stuff. Then, moving forward, can push the other improvements on independent PRs so they can be discussed atomically.
Instead of having all of this mempool policy lookup stuff, what if we instead just had GroupOutputs produce all of the sets of OutputGroups that we will end up using during coin selection? Then each filtered set of groups would be passed in for each AttemptSelection. This way, we could also pre-calculate the total value available for a given set of groups and skip AttemptSelections when there is clearly not enough value for that group set. This would also let us get the too-long-mempool-chain error as we could see whether the group set that allows the long mempool chains would have enough value to potentially reach a solution.
A downside of this approach would be the memory use. This would end up consuming a lot more memory as we would be making all of the group sets for each SelectCoins rather than for each AttemptSelection. However we may be able to work around that by using references, but we have to be careful as some of the coin selection algorithms modify the group set that they are given.
Small update here: Now that #25218 was merged, the next step for this is #25685 which restructures the coin selection area further.
Still, meanwhile, will be pushing the new outputs grouping changes in the coming days. Need to add proper test coverage for it first, and.. will try to decouple them from #25685 (plus update the outdated PR description), moving forward :tractor:
We currently return "Insufficient funds" which doesn't really
describe what went wrong; the tx creation failed because of
a long-mempool-chain, not because of a lack of funds.
Also, return early from Coin Selection if the sum of the
discarded coins decreases the available balance below the
target amount.
735+ if (!res_detailed_errors.empty()) return res_detailed_errors.front();
736+
737+ // Let's check if we have long-chain of unconfirmed UTXOs in the not accepted groups
738+ for (const auto& group : not_accepted_groups) {
739+ if (group.m_ancestors >= max_ancestors)
740+ return util::Result<SelectionResult>({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")});
Just because these UTXOs are available it doesn’t mean that including them would cover required amount.
For example let’s imaging you have two coins in a wallet:
If you try to send 50 BTC you should get “insufficient funds” error, but IIUC after this PR you’ll get “Unconfirmed UTXOs are available…” which is unrelated.
715+ // Check if we still have enough balance after applying filters (some coins might be discarded)
716+ CAmount total_discarded = 0;
717+ CAmount total_unconf_long_chain = 0;
718+ for (const auto& group : discarded_groups) {
719+ total_discarded += group.GetSelectionAmount();
720+ if (group.m_ancestors >= max_ancestors || group.m_descendants >= max_descendants) total_unconf_long_chain += group.GetSelectionAmount();
I think this should respect fRejectLongChains flag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.
If fRejectLongChains flag is set (the default) then the most lenient filter has max_ancestors/2 and max_descendants/2. So this condition could never be hit.
In summary I don’t yet understand how one can hit the error described in #23144
- I think this should respect
fRejectLongChainsflag. If user explicitly asked us to ignore long-chain constraints, then we should respect that.
It’s being respected:
If the user asked to ignore long-chain constraint (fRejectLongChains=false) the last filter include the coins, which means that they aren’t discarded, so the discarded_groups vector is empty, so total_unconf_long_chain is zero, so the error is never hit.
- If
fRejectLongChainsflag is set (the default) then the most lenient filter hasmax_ancestors/2andmax_descendants/2. So this condition could never be hit.
At the contraire, the unconfirmed coins are inside the available_coins struct (the user sees them as spendable), and because of the not inclusion of the last filter (due the fRejectLongChains=true flag), they are excluded from the created output groups and included inside the discarded groups vector, which is the one used to hit this condition.
In summary I don’t yet understand how one can hit the error described in #23144
Think in this way:
If the wallet reject long chains (default), the process discard coins that the user sees as spendable and tries to find a result with the remaining ones. Returning an “Insufficient Funds” if it’s not found. The “discarded groups” vector basically represents that. Coin Selection excluded coins seen as spendable by the user.
Yes you’re right, I somehow managed to confuse myself about the filters. Thanks for patient explanation.
Still something doesn’t sit right. So if a coin is discarded because of max_ancestors we give this error message, but if it’s discarded by the filter max_ancestors/2 we just say insufficient funds when the coins are there they just don’t meet the filters. There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, we don’t allow any chains at all. Why give detailed error message in one case and not the other? Maybe we should just count all discarded coins and generalize the error message a bit?
Still something doesn’t sit right. So if a coin is discarded because of max_ancestors we give this error message, but if it’s discarded by the filter max_ancestors/2 we just say insufficient funds when the coins are there they just don’t meet the filters
no, those coins will be added to the last non-customizable filter which has max_ancestors-1 and max_descendants-1.
There are also other reasons when the coins are discarded, for example unsafe utxos. Even you enable unsafe utxos, we don’t allow any chains at all
The unsafe UTXO filter is just redundant, we are currently filtering unsafe UTXO inside the coins fetching process (AvailableCoins).
In other words, when m_include_unsafe_inputs=false no unsafe UTXO arrives at Coin Selection.
Why give detailed error message in one case and not the other? Maybe we should just count all discarded coins and generalize the error message a bit?
The process is already counting all discarded coins and returning early if their exclusion make the available amount go below the target. It’s currently returning an “Insufficient Funds”, but could say something different too.
I made the too-long-mempool-chain error an special case because this error, alike the untrusted coins that the user can see in a different balance (the unconfirmed/untrusted one), is hidden to the user as it depends on inner mempool policies. It’s not enough for the user to set a flag to spend the unsafe coins, it needs to set another one to craft the tx with an unconfirmed output that surpasses the max descendants policy.
712+ std::vector<OutputGroup> discarded_groups;
713+ FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters, discarded_groups);
714+
715+ // Check if we still have enough balance after applying filters (some coins might be discarded)
716+ CAmount total_discarded = 0;
717+ CAmount total_unconf_long_chain = 0;
total_discarded is always equal to total_unconf_long_chain?
m_spend_zero_conf_change flag is disabled. In other words, unconfirmed coins accepted by the mempool limits (ancestors/descendants < 25).