wallet: Coin Selection, return accurate error messages #26661

furszy · 2022-12-08T14:13:27Z

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:

DrahtBot · 2022-12-08T14:13:30Z

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.

fanquake · 2022-12-08T16:42:51Z

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

2022-12-08T14:16:39.658085Z [test] [wallet/wallet.h:832] [WalletLogPrintf] [default wallet] Setting spkMan to active: id = cc3df596507261fb5c2d5dd4fec08d1b067902e56730e5887a030a95d2a63d29, type = bech32m, internal = true
2022-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
wallet/test/coinselector_tests.cpp(999): �[1;31;49merror: in "coinselector_tests/check_max_weight": check result has failed�[0;39;49m
test_bitcoin: ./util/result.h:52: const T& util::Result<T>::value() const [with T = wallet::SelectionResult]: Assertion `has_value()' failed.
Aborted (core dumped)
make[3]: *** [Makefile:21461: wallet/test/coinselector_tests.cpp.test] Error 1

furszy · 2022-12-08T20:06:48Z

all good now, thanks @fanquake 👍🏼.

achow101 · 2022-12-12T18:45:24Z

src/wallet/spend.cpp

        }
        // Coin Selection failed.
-        return util::Result<SelectionResult>(util::Error());
+        return res_detailed_errors.empty() ? util::Result<SelectionResult>(util::Error()) : res_detailed_errors.front();


In 8f89737 "wallet: return accurate error messages from Coin Selection"

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

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.

achow101 · 2022-12-12T18:49:41Z

src/wallet/coinselection.h

+        // Store sum of combined input sets to check that the results have no shared UTXOs
+        const size_t expected_count = m_selected_inputs.size() + inputs.size();
+        util::insert(m_selected_inputs, inputs);
+        if (!Assume(m_selected_inputs.size() == expected_count)) {


In e7a94dd "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.

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.

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

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.

src/wallet/coinselection.h

achow101 · 2022-12-14T18:50:11Z

ACK 4086040

furszy · 2022-12-15T13:41:31Z

Rebased, conflicts solved.

Plus, tackled #26661 (comment).

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.

S3RK · 2022-12-15T18:48:56Z

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 · 2022-12-16T14:35:13Z

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.

ishaanam

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

src/wallet/coinselection.h

and remove 'CoinEligibilityFilter' default constructor to prevent mistakes.

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".

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.

only will ever happen if something unexpected happened.

aureleoules

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

ishaanam · 2022-12-23T04:35:20Z

crACK 76dc547

theStack

Concept ACK

src/wallet/spend.cpp

theStack

ACK 76dc547 🌇

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).

achow101 · 2023-01-03T23:42:43Z

ACK 76dc547

…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

hebasto · 2023-02-12T16:57:16Z

src/wallet/spend.cpp

+        // future: add "error level" so the worst one can be picked instead.
+        std::vector<util::Result<SelectionResult>> res_detailed_errors;
+        for (const auto& select_filter : ordered_filters) {
+            if (auto res{AttemptSelection(wallet, value_to_select, select_filter.filter, available_coins,


The local variable res in here shadows another one:

bitcoin/src/wallet/spend.cpp

Line 658 in 76dc547

util::Result<SelectionResult> res = [&] {

Fixed in #27087.

DrahtBot added the Wallet label Dec 8, 2022

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from 0d06b2f to 8edee42 Compare December 8, 2022 18:48

DrahtBot added the Needs rebase label Dec 9, 2022

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from 8edee42 to ab8d2c3 Compare December 9, 2022 22:12

DrahtBot removed the Needs rebase label Dec 9, 2022

DrahtBot mentioned this pull request Dec 10, 2022

RPC: Accept options as named-only parameters #26485

Merged

achow101 reviewed Dec 12, 2022

View reviewed changes

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from ab8d2c3 to 8b8088d Compare December 12, 2022 21:37

DrahtBot added the Needs rebase label Dec 13, 2022

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from 8b8088d to 4086040 Compare December 14, 2022 01:26

DrahtBot removed the Needs rebase label Dec 14, 2022

DrahtBot added the Needs rebase label Dec 14, 2022

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from 4086040 to f258394 Compare December 15, 2022 13:21

DrahtBot removed the Needs rebase label Dec 15, 2022

DrahtBot mentioned this pull request Dec 16, 2022

[Draft / POC] Silent Payments #24897

Closed

furszy mentioned this pull request Dec 19, 2022

wallet: coin selection, don't return results that exceed the max allowed weight #26720

Merged

ishaanam reviewed Dec 21, 2022

View reviewed changes

src/wallet/coinselection.h Outdated Show resolved Hide resolved

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

e5e147f

and remove 'CoinEligibilityFilter' default constructor to prevent mistakes.

furszy added 2 commits December 21, 2022 23:14

wallet: make SelectCoins flow return util::Result

7e8340a

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from f258394 to cc06e59 Compare December 22, 2022 02:15

furszy added 2 commits December 21, 2022 23:20

wallet: coin selection, add duplicated inputs checks

f4d7947

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.

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

76dc547

only will ever happen if something unexpected happened.

furszy force-pushed the 2022_wallet_coin_selection_accurate_errors branch from cc06e59 to 76dc547 Compare December 22, 2022 02:20

aureleoules approved these changes Dec 22, 2022

View reviewed changes

theStack reviewed Dec 24, 2022

View reviewed changes

src/wallet/spend.cpp Show resolved Hide resolved

theStack approved these changes Dec 27, 2022

View reviewed changes

maflcko requested a review from achow101 January 3, 2023 17:35

achow101 merged commit 3f8591d into bitcoin:master Jan 3, 2023

hebasto mentioned this pull request Feb 12, 2023

refactor, wallet: Avoid variable shadowing #27087

Closed

hebasto reviewed Feb 12, 2023

View reviewed changes

furszy mentioned this pull request Apr 3, 2023

Mark "too-long-mempool-chain error msg" completed NYDIG/bitcoin-task-bounty#32

Merged

furszy deleted the 2022_wallet_coin_selection_accurate_errors branch May 27, 2023 01:46

bitcoin locked and limited conversation to collaborators May 26, 2024

wallet: Coin Selection, return accurate error messages #26661

wallet: Coin Selection, return accurate error messages #26661

Uh oh!

Conversation

furszy commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Dec 8, 2022

Uh oh!

furszy commented Dec 8, 2022

Uh oh!

achow101 Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

S3RK Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Dec 14, 2022

Uh oh!

furszy commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

S3RK commented Dec 15, 2022

Uh oh!

furszy commented Dec 16, 2022

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Dec 23, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 3, 2023

Uh oh!

hebasto Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Dec 8, 2022 •

edited

Loading

DrahtBot commented Dec 8, 2022 •

edited

Loading

furszy Dec 12, 2022 •

edited

Loading

furszy commented Dec 15, 2022 •

edited

Loading