wallet: Coin Selection, return accurate error messages

furszy commented at 2:13 pm on December 8, 2022: member

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:

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”.
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”.
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:

#26560 (review)
#25729 (review)
#25269#pullrequestreview-1135240825
#23144 (which is connected to #24845)

DrahtBot commented at 2:13 pm on December 8, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, ishaanam, theStack, achow101

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:

#26720 (wallet: coin selection, don’t return results that exceed the max allowed weight by furszy)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#23829 (refactor: use braced init for integer literals instead of c style casts by PastaPastaPasta)

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 Wallet on Dec 8, 2022

fanquake commented at 4:42 pm on December 8, 2022: member

https://github.com/bitcoin/bitcoin/pull/26661/checks?check_run_id=9969494686:

02022-12-08T14:16:39.658085Z [test] [wallet/wallet.h:832] [WalletLogPrintf] [default wallet] Setting spkMan to active: id = cc3df596507261fb5c2d5dd4fec08d1b067902e56730e5887a030a95d2a63d29, type = bech32m, internal = true
12022-12-08T14:16:40.589188Z [test] [wallet/coinselection.cpp:331] [KnapsackSolver] [selectcoins] Coin selection best subset: 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 0.033 total 49.50
2wallet/test/coinselector_tests.cpp(999): �[1;31;49merror: in "coinselector_tests/check_max_weight": check result has failed�[0;39;49m
3test_bitcoin: ./util/result.h:52: const T& util::Result<T>::value() const [with T = wallet::SelectionResult]: Assertion `has_value()' failed.
4Aborted (core dumped)
5make[3]: *** [Makefile:21461: wallet/test/coinselector_tests.cpp.test] Error 1

furszy force-pushed on Dec 8, 2022

furszy commented at 8:06 pm on December 8, 2022: member

all good now, thanks @fanquake 👍🏼.

DrahtBot added the label Needs rebase on Dec 9, 2022

furszy force-pushed on Dec 9, 2022

DrahtBot removed the label Needs rebase on Dec 9, 2022

in src/wallet/spend.cpp:707 in 8f897377b4 outdated

703+                if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res);
704+            }
705         }
706         // Coin Selection failed.
707-        return util::Result<SelectionResult>(util::Error());
708+        return res_detailed_errors.empty() ? util::Result<SelectionResult>(util::Error()) : res_detailed_errors.front();

achow101 commented at 6:45 pm on December 12, 2022:

In 8f897377b4d9661b4d6b46122cd24319e5af99e7 “wallet: return accurate error messages from Coin Selection”

This seems to be equivalent to returning early in the loop.

furszy commented at 9:01 pm on December 12, 2022:

I initially thought it in that way but it is not what we want (this is why I mention there that, probably in a follow-up, we should add a “severity level” to the errors that are returned from coin selection).

So, the rationale is the following one: The loop takes an expanded coins eligibility filter on each round; if we return early, then we will not perform some selection rounds that include more coins which could make us find a solution for the target.

E.g. let’s say that we have 1515 UTXO of 0,033 BTC (all of them with depth > 6) and 1 received UTXO of 50 BTC with depth=1. Now, let’s say that the user want to spend 49.5 BTC: The 1515 low value UTXOs will be included in the first round of the loop (because of their depth) while the 50 BTC UTXO will not (because of its depth and the fact that we received it). So, the first AttemptSelection round will fail due a selection result size exceeding the maximum allowed weight, so returning early in the loop would avoid us from finding the good selection result. Which in this case, it can only be found on the second round of the loop.

in src/wallet/coinselection.h:314 in e7a94dd2e3 outdated

308+    util::Result<bool> InsertInputs(const T& inputs)
309+    {
310+        // Store sum of combined input sets to check that the results have no shared UTXOs
311+        const size_t expected_count = m_selected_inputs.size() + inputs.size();
312+        util::insert(m_selected_inputs, inputs);
313+        if (!Assume(m_selected_inputs.size() == expected_count)) {

achow101 commented at 6:49 pm on December 12, 2022:

In e7a94dd2e32f3f0beb0f172ee741e00c674e380b “wallet: change SelectionResult::Merge assert to error”

I’m not sure that Assume here is useful. Even for our tests, getting back an expected error is still sufficient to fail tests, we don’t need to kill the program.

furszy commented at 9:35 pm on December 12, 2022:

I added it because atm we are simply not supporting the shared UTXOs merge, so this shouldn’t happen in our tests neither (we shouldn’t be able to trigger this error using the regular transaction creation process, and if we do, then it’s a bad bug that should be fixed).

but could remove it too. I’m not having a strong preference for it neither.

S3RK commented at 8:46 am on December 14, 2022:

I have a noob cpp question. Can we somehow enforce that returned error is not ignored by the calling code?

furszy commented at 1:26 pm on December 14, 2022:

I have a noob cpp question. Can we somehow enforce that returned error is not ignored by the calling code?

yes, by throwing an exception instead of returning a result error message. Which might not be bad. In this case, could be a runtime_error.

in src/wallet/coinselection.h:179 in f9986bc3ff outdated

175@@ -176,13 +176,13 @@ struct CoinEligibilityFilter
176 {
177     /** Minimum number of confirmations for outputs that we sent to ourselves.
178      * We may use unconfirmed UTXOs sent from ourselves, e.g. change outputs. */
179-    const int conf_mine;
180+    const int conf_mine{0};

achow101 commented at 6:57 pm on December 12, 2022:

In f9986bc3ff8f9dc597b5e8b14fceb1df3d473f6e “wallet: CoinEligibilityFilter, set default value for every member”

Is this commit necessary? I don’t think we ever create CoinEligibilityFilter without using the constructors which initialize these. If the concern is that we might accidentally create a CoinEligibilityFilter without using one of those constructors, I would rather that we delete the default constructor rather than setting potentially dangerous default values.

furszy commented at 9:18 pm on December 12, 2022:

Good point and catch. Fixed. The only fields that could not be set in the constructors are max_descendants and m_include_partial_groups. So could add default values for those (my concern was having those fields with garbage), plus remove the default constructor.

furszy force-pushed on Dec 12, 2022

DrahtBot added the label Needs rebase on Dec 13, 2022

furszy force-pushed on Dec 14, 2022

DrahtBot removed the label Needs rebase on Dec 14, 2022

achow101 commented at 6:50 pm on December 14, 2022: member

ACK 4086040f7ada5acfb5b0e84acfece24afbc6b3a5

DrahtBot added the label Needs rebase on Dec 14, 2022

furszy force-pushed on Dec 15, 2022

furszy commented at 1:41 pm on December 15, 2022: member

Rebased, conflicts solved.

Plus, tackled #26661 (review).

Now instead of returning an error message for the duplicated inputs selection error, we will throw a runtime_error.

Rationale: This is a Coin Selection library restriction (at least for the time being), and no process should be able to trigger this issue using the transaction creation process, so it makes more sense to throw a runtime_error if happens, so the error severity level is clearly stated, and we tell users/devs to report the bug if happens.

Second point: I think that it’s better for the library structure to return util::Result error messages for stuff that the user can solve (e.g. a transaction exceeding the maximum weight or a lack of pre-selected inputs) and exceptions/runtime_errors for unexpected/bad stuff that the user cannot solve alone and should report.

DrahtBot removed the label Needs rebase on Dec 15, 2022

S3RK commented at 6:48 pm on December 15, 2022: contributor

Second point: I think that it’s better for the library structure to return util::Result error messages for stuff that the user can solve (e.g. a transaction exceeding the maximum weight or a lack of pre-selected inputs) and exceptions/runtime_errors for unexpected/bad stuff that the user cannot solve alone and should report.

I like this distinction for when to use errors vs exceptions. If we have an agreement amount wallet devs, we could even document it as a dev guideline for wallet.

furszy commented at 2:35 pm on December 16, 2022: member

Second point: I think that it’s better for the library structure to return util::Result error messages for stuff that the user can solve (e.g. a transaction exceeding the maximum weight or a lack of pre-selected inputs) and exceptions/runtime_errors for unexpected/bad stuff that the user cannot solve alone and should report.

I like this distinction for when to use errors vs exceptions. If we have an agreement amount wallet devs, we could even document it as a dev guideline for wallet.

Absolutely. Let’s see If we are all aligned, can follow-up this work with a doc PR.

in src/wallet/coinselection.h:194 in 87e018b36a outdated

188     const bool m_include_partial_groups{false};
189 
190+    CoinEligibilityFilter() = delete;
191     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {}
192     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {}
193     CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {}

ishaanam commented at 9:44 pm on December 21, 2022:

In 87e018b36a0f342411b9a70835d278a26181b484 “wallet: CoinEligibilityFilter, remove default constructor”

Re #26661 (review)

The only fields that could not be set in the constructors are max_descendants and m_include_partial_groups

It appears that max_descendants is set in all of the constructors, so does it still make sense for it to have a default value?

furszy commented at 2:17 am on December 22, 2022:

yeah true, fixed. Squashed changes inside the first commit e5e147fe.

ishaanam commented at 10:13 pm on December 21, 2022: contributor

Approach ACK, looks good, I just have one comment.

wallet: refactor eight consecutive 'AttemptSelection' calls into a loop

and remove 'CoinEligibilityFilter' default constructor to prevent
mistakes.

e5e147fe97

wallet: make SelectCoins flow return util::Result 7e8340ab1a

wallet: return accurate error messages from Coin Selection

and not the general "Insufficient funds" when the wallet
actually have funds.

Two new error messages:

1) If the selection result exceeds the maximum transaction weight,
   we now will return: "The inputs size exceeds the maximum weight".

2) If the user preselected 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".

0aa065b14e

furszy force-pushed on Dec 22, 2022

wallet: coin selection, add duplicated inputs checks

As no process should be able to trigger this error
using the regular transaction creation process, throw
a runtime_error if happens to tell users/devs to
report the bug if happens.

f4d79477ff

gui: create tx, launch error dialog if backend throws runtime_error

only will ever happen if something unexpected happened.

76dc547ee7

furszy force-pushed on Dec 22, 2022

aureleoules approved

aureleoules commented at 1:57 pm on December 22, 2022: member

ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 I verified the tests correctly test the new behavior and overall this is a great refactor.

ishaanam commented at 4:35 am on December 23, 2022: contributor

crACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15

in src/wallet/spend.cpp:672 in e5e147fe97 outdated

691-                if (auto r7{AttemptSelection(wallet, value_to_select,
692-                    CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs=*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true),
693-                    available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
694-                    return r7;
695-                }
696+                ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});

theStack commented at 11:39 am on December 24, 2022:

nit:

0                ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs=*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});

furszy commented at 2:53 pm on December 24, 2022:

will do if have to re-touch 👍🏼

theStack commented at 11:40 am on December 24, 2022: contributor

Concept ACK

theStack approved

theStack commented at 4:23 pm on December 27, 2022: contributor

ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 :city_sunrise:

Nice refactoring PR. This increases the readability of AutomaticCoinSelection and also improves user experience by being more specific about why coin selection failed. Tested the last commit by throwing an arbitrary std::runtime_error within the introduced try/catch block and verified that a message box with the error string pops up if we try to send funds via bitcoin-qt.

Regarding the functional tests, a potential follow-up idea would be to put the introduced error message constant ERR_NOT_ENOUGH_PRESET_INPUTS to some shared module to avoid duplicated definition (I guess this could make sense for many other error strings too).

maflcko requested review from achow101 on Jan 3, 2023

achow101 commented at 11:42 pm on January 3, 2023: member

ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15

achow101 merged this on Jan 3, 2023

achow101 closed this on Jan 3, 2023

sidhujag referenced this in commit cab438784e on Jan 4, 2023

in src/wallet/spend.cpp:696 in 76dc547ee7

721+        // Walk-through the filters until the solution gets found.
722+        // If no solution is found, return the first detailed error (if any).
723+        // future: add "error level" so the worst one can be picked instead.
724+        std::vector<util::Result<SelectionResult>> res_detailed_errors;
725+        for (const auto& select_filter : ordered_filters) {
726+            if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins,

hebasto commented at 4:57 pm on February 12, 2023:

The local variable res in here shadows another one: https://github.com/bitcoin/bitcoin/blob/76dc547ee7b05864e7b1b6c55fc0301d47aa3a15/src/wallet/spend.cpp#L658

Fixed in #27087.

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

wallet: Coin Selection, return accurate error messages #26661

Reviews

Conflicts