wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection

furszy commented at 1:19 pm on July 23, 2022: member

# Context (Current Flow on Master)

In the transaction creation process, in order to select which coins the new transaction will spend, we first obtain all the available coins known by the wallet, which means walking-through the wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.

This coins vector is then provided to the Coin Selection process, which first checks if the user has manually selected any input (which could be internal, aka known by the wallet, or external), and if it does, it fetches them by searching each of them inside the wallet and/or inside the Coin Control external tx data.

Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection process walks-through the entire available coins vector once more just to erase coins that are in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside the same transaction).

# Process Workflow Changes

Now, a new method, FetchCoins will be responsible for:

Lookup the user pre-selected-inputs (which can be internal or external).
And, fetch the available coins in the wallet (excluding the already fetched ones).

Which will occur prior to the Coin Selection process. Which allows us to never include the pre-selected-inputs inside the available coins vector in the first place, as well as doing other nice improvements (written below).

So, Coin Selection can perform its main responsibility without mixing it with having to fetch internal/external coins nor any slow and unneeded duplicate coins verification.

# Summarizing the Improvements:

If any pre-selected-input lookup fail, the process will return the error right away. (before, the wallet was fetching all the wallet available coins, walking through the entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
The pre-selected-inputs lookup failure causes are properly described on the return error. (before, we were returning an “Insufficient Funds” error for everything, even if the failure was due a not solvable external input)
Faster Coin Selection: no longer need to “remove the pre-set inputs from the available coins vector so that Coin Selection doesn’t pick them” (which meant to loop-over the entire available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).

Now, the available coins vector, which is built after the pre-selected-inputs fetching, doesn’t include the already selected inputs in the first place.
Faster transaction creation for transactions that only use manually selected inputs.

We now will return early, as soon as we finish fetching the pre-selected-inputs and not perform the resources expensive calculation of walking-through the entire wallet txes map to obtain the available coins (coins that we will not use).

Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting m_allow_other_inputs=false to disallow the wallet to include coins automatically.

Result on this PR (tip f6d0bb2d):

ns/op	op/s	err%	total	benchmark
1,048,675.00	953.58	0.3%	0.06	`WalletCreateTransaction`

vs

Result on master (tip 4a4289e2):

ns/op	op/s	err%	total	benchmark
96,373,458.20	10.38	0.2%	5.30	`WalletCreateTransaction`

The benchmark took to run in master: 96.37 milliseconds, while in this PR: 1 millisecond 🚀 .

DrahtBot added the label Wallet on Jul 23, 2022

DrahtBot commented at 3:16 pm on July 23, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26154 (refactor: Move coin_control variable to test setup section by yancyribbens)
#26152 (Bump unconfirmed ancestor transactions to target feerate by Xekyo)
#26066 (wallet: Refactor and document CoinControl by aureleoules)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors by achow101)
#25730 (RPC: listunspent, add “include immature coinbase” flag by furszy)

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 Needs rebase on Jul 28, 2022

furszy force-pushed on Jul 29, 2022

DrahtBot removed the label Needs rebase on Jul 29, 2022

furszy force-pushed on Jul 29, 2022

furszy commented at 5:19 pm on July 29, 2022: member

rebased, conflicts solved.

ishaanam commented at 9:41 pm on August 1, 2022: contributor

Concept ACK, this seems like a good improvement.

achow101 commented at 7:51 pm on August 2, 2022: member

It’s not clear to me that this provides any significant benefit over a much simpler change of just excluding pre-selected inputs from AvailableCoins. The only other improvement this provides is better error reporting, but I think it would be better to just go through SelectCoins and improve its error reporting instead of pulling things out so that errors are reported elsewhere.

furszy commented at 10:16 pm on August 2, 2022: member

It’s not clear to me that this provides any significant benefit over a much simpler change of just excluding pre-selected inputs from AvailableCoins. The only other improvement this provides is better error reporting, but I think it would be better to just go through SelectCoins and improve its error reporting instead of pulling things out so that errors are reported elsewhere.

There are several benefits, (aside from what I wrote in the PR description):

Sources Architecture:

Do make sense for you to be fetching, and verifying, coins in a function whose main responsibility is perform the Coin Selection process?

The Coin Selection should receive coins, and certain user/wallet parameters, so it can produce all the possible solutions for a certain target amount, picking and returning the best of them.

Just imagine how this would looks like if Coin Selection would be a separated library (which, for its increasing specialization, could easily be one in the future). You would never include the pre-selected-inputs fetching and verification process into the Coin Selection responsibilities at all because cannot/shouldn’t do it there.

Tangible Improvements:

As we will do the pre-selected-inputs fetching and verification prior to calling AvailableCoins, if there is an error when the wallet fetches any of the preset-inputs, the transaction creation process can fail right away without calling AvailableCoins (Function that, as we know, walks-through the entire wallet’s transactions map and performs all the internal available coins checks).

This prevents us from wasting resources that we are currently wasting for, literally, no reason.

In master, we are first looping-through the entire wallet’s map inside AvailableCoins, which is a resource intensive task if you have a big wallet, just to fail right after it finishes, discarding the coins result, because one of the pre-set inputs has an error.
If the user disallowed the “other inputs selection” by setting m_allow_other_inputs=false (which means: “no coins from the AvailableCoins result function will be used during Coin Selection”): 

In master: we loop-through the entire wallet’s map inside AvailableCoins first, then we discard the computed result (because we will not use it..), then we fetch and use the pre-selected-inputs only. Which is time and processing power wasted for no reason.

In this PR: we simply select the preset inputs, skip AvailableCoins if m_allow_other_inputs=false and provides them to SelectCoins.

but I think it would be better to just go through SelectCoins and improve its error reporting instead of pulling things out so that errors are reported elsewhere.

We are currently failing inside the Coin Selection process due:

“Invalid pre-selected input”
“Not found pre-selected input”
“Not solvable pre-selected input”

These errors have nothing to do with the Coin Selection process. Why keep them there?

It’s not that I located these errors on a random place. I placed them inside a “Fetch Coins” procedure whose main responsibility is fetch the coins that the user pre-selected and fetch the available coins in the wallet. Which seems totally reasonable for me.

I mean, this errors have been obscured inside Coin Selection all this time, not being properly notified, because they should had never been placed there in the first place. It’s all about a proper responsibility division to make the process clearer and cleaner so it’s less error-prone (and in this case, in some occasions, actually faster and less resource consuming).

furszy commented at 6:23 pm on August 3, 2022: member

To make the speed improvements more visible, pushed a new benchmark f6d0bb2d.

Basically, the new bench measures the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting m_allow_other_inputs=false to disallow the wallet to include coins automatically.

This is the result on this PR (tip f6d0bb2d):

ns/op	op/s	err%	total	benchmark
1,048,675.00	953.58	0.3%	0.06	`WalletCreateTransaction`

vs

This is the result on current master (tip 4a4289e2):

ns/op	op/s	err%	total	benchmark
96,373,458.20	10.38	0.2%	5.30	`WalletCreateTransaction`

Summary

The benchmark took to run in master: 96.37 milliseconds, while in this PR: 1 millisecond 🚀 .

Updated PR description with the benchmark results.

furszy force-pushed on Aug 3, 2022

in src/wallet/spend.cpp:110 in 23448cae6f outdated

104@@ -105,6 +105,54 @@ void CoinsResult::clear()
105     other.clear();
106 }
107 
108+// Fetch the coin control selected inputs and wrap them inside a COutput.
109+// Coins could be internal (from the wallet) or external.
110+BResult<CoinsResult> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,

achow101 commented at 10:49 pm on August 3, 2022:

In 23448cae6f4a5f00766df9220860b16311266417 “wallet: encapsulate pre-selected-inputs lookup into its own function”

I think this should return a SelectionResult rather than CoinsResult. It would elimiate the second iteration inside of SelectCoins.

It could also return the selected value too so the caller could reduce the target before SelectCoins and eliminate an additional iteration of the preselcted inputs.

furszy commented at 5:27 pm on August 4, 2022:

good thought 👍🏼

Most likely, will need to do this change incrementally (right after 23448cae) because if FetchSelectedInputs returns a SelectionResult, need to add a method for merging two SelectionResult objects (the pre-set-inputs result and the wallet’s selection result).

We are currently creating an OutputGroup at the beginning of SelectCoins with the pre-set-inputs just to merge it at the end of the function with the wallet’s selection algorithm result. The reason is that SelectResult only has a function AddInputs(OutputGroups) and not an AddInputs(set<COutputs>) nor a merge method.

Side note, based on a quick code review: the selection result built from the combination of pre-set-inputs and the wallet selected ones should be having an inaccurate waste calculation – will do some tests in another PR –).

in src/wallet/spend.cpp:156 in 84b4ffe85b outdated

152@@ -153,6 +153,31 @@ BResult<CoinsResult> FetchSelectedInputs(const CWallet& wallet, const CCoinContr
153     return result;
154 }
155 
156+BResult<CoinsFund> FetchCoins(const CWallet& wallet, const CCoinControl& coin_control,

achow101 commented at 10:51 pm on August 3, 2022:

In 84b4ffe85ba0267c49080e9a096e45d6af63e720 “wallet: remove fetch pre-selected-inputs responsibility from SelectCoins”

I don’t think it’s necessary to have another function here to wrap these two calls. They could just be in CreateTransactionInternal

furszy commented at 1:13 pm on August 6, 2022:

True. It’s not used anywhere else for now. Have removed the function and the CoinsFunds struct as well.

in src/wallet/spend.cpp:167 in 84b4ffe85b outdated

162+    if (coin_control.HasSelected()) {
163+        result.pre_selected_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params);
164+        if (!result.pre_selected_inputs) return result.pre_selected_inputs.GetError();
165+
166+        // If no other inputs are allowed, return early
167+        if (!coin_control.m_allow_other_inputs) return result;

achow101 commented at 10:52 pm on August 3, 2022:

M In 84b4ffe85ba0267c49080e9a096e45d6af63e720 “wallet: remove fetch pre-selected-inputs responsibility from SelectCoins”

This check could be moved to the top of AvailableCoins and it would achieve the same effect. Doing so would also not change AvailableCoins’s semantics.

furszy commented at 4:28 pm on August 5, 2022:

I added it here (inside the coin_control.HasSelected() block) to not change the current behavior and minimize the amount of changes.

In master, even if m_allow_other_inputs is false, we are automatically falling back to select coins from the wallet if there are no pre-set inputs inside coin control.

This basically is because the default value for m_allow_other_inputs is false, and some RPC commands that don’t allow inputs manual selection such as sendtoaddress and sendmany are not setting it to true. So.. they are always falling back to select coins from the wallet.

So.. if I move this check below, will need to set m_allow_other_inputs=true in every caller that does not add any input manually (or.. change the default value to be true inside coin control).

but.. let me see if those two RPC commands are the only affected ones. Maybe it’s a straightforward change. I do agree that the current master behavior of automatically falling back to select coins from the wallet even when m_allow_other_inputs=false is messy and not an expected behavior.

in src/wallet/spend.cpp:609 in 84b4ffe85b outdated

601@@ -575,24 +602,17 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons
602     return best_result;
603 }
604 
605-std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
606+std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsFund& coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
607 {
608     CAmount value_to_select = nTargetValue;
609 
610+    // Add the manually selected coins first

achow101 commented at 10:56 pm on August 3, 2022:

In 84b4ffe85ba0267c49080e9a096e45d6af63e720 “wallet: remove fetch pre-selected-inputs responsibility from SelectCoins”

I’m wondering if we should just make this function only return a SelectionResult for the automatic coin selection and move the preset input stuff to the caller. If FetchSelectedInputs made a SelectionResult for the preset inputs, then the only preset input things left in SelectCoins would be the merging of the SelectionResults, and that can be done in the caller too.

furszy commented at 1:11 pm on August 6, 2022:

Been thinking about this and pushed some refactoring that you might like a bit more.

The idea was based on try to not mix the Coin Selection code with the inputs fetching while still be able to remove the manual coins selection stuff from the automatic coin selection, the extra loops for the pre-set inputs and the FetchCoins wrapper function.

So, have decoupled the SelectCoins into two functions:

AutomaticCoinSelection: Which is basically the previously called SelectCoins without the manually selected inputs stuff. So, it will purely receive a set of coins and be in charge of selecting the best subset of them to cover the target amount.
SelectCoins: In charge of selecting all the user manually selected coins first (“pre-set inputs”), and if coin_control ’m_allow_other_inputs=true’, call ‘AutomaticCoinSelection’ to select a set of coins such that the target - pre_set_inputs.total_amount is met. Then merge both results.

DrahtBot added the label Needs rebase on Aug 5, 2022

furszy force-pushed on Aug 6, 2022

DrahtBot removed the label Needs rebase on Aug 6, 2022

furszy commented at 1:29 pm on August 6, 2022: member

Took me a bit but, while was tackling the feedback, ended up re-organizing the commits changes better. The rationale for each modification should be clearer now.

What did was:

Eliminate the pre-set inputs second iteration inside SelectCoins by adding an AddInputs(std::vector<COutput>&) method to SelectionResult (so we don’t need to iterate over the inputs just to insert them inside an ephemeral OutputGroup whom is simply not used –> SelectionResult::AddInput(OutputGroup&) takes directly the m_outputs internal field..).
Remove the FetchCoins wrapper function and the CoinsFund struct: Moved the internals to CreateTransactionInternal, who will first call FetchSelectedInputs then call AvailableCoins.
Decouple the SelectCoins into two functions:
- AutomaticCoinSelection: Which is basically the previously called SelectCoins without the manually selected inputs stuff. So, it will purely receive a set of coins and be in charge of selecting the best subset of them to cover the target amount.
- SelectCoins: In charge of selecting all the user manually selected coins first (“pre-set inputs”), and if coin_control ’m_allow_other_inputs=true’, call ‘AutomaticCoinSelection’ to select a set of coins such that the target - pre_set_inputs.total_amount is met. Then merge both results.
Have set CoinControl::m_allow_other_inputs default value to true to correct a misleading behavior (convo started here):
- As several flows in the wallet such as sendtoaddress, sendmany and even the GUI don’t change the m_allow_other_inputs param, and the default value for it has been false. The process was having a workaround patch to automatically fall back to select coins from the wallet if m_allow_other_inputs=false and no manual inputs were selected.

furszy force-pushed on Aug 6, 2022

in src/wallet/spend.cpp:113 in 4946384e2f outdated

104@@ -105,6 +105,61 @@ void CoinsResult::clear()
105     other.clear();
106 }
107 
108+struct SelectedInputs
109+{
110+    std::set<COutput> coins;
111+    // If subtract fee from outputs is active, the 'total_amount'
112+    // will be the sum of each output effective value
113+    // instead of the sum of the outputs amount

achow101 commented at 7:25 pm on August 9, 2022:

In 4946384e2f56333274baf7fab1dfd41b046e12a1 “wallet: encapsulate pre-selected-inputs lookup into its own function”

This comment is backwards. The total amount uses the effective value normally, and real value when SFFO.

furszy commented at 11:01 pm on August 9, 2022:

ups, pushed.

in src/wallet/spend.h:142 in bc16c074ca outdated

154+        } else {
155+            total_amount += output.GetEffectiveValue();
156+        }
157+        coins.insert(output);
158+    }
159+};

achow101 commented at 7:29 pm on August 9, 2022:

In bc16c074cac96e2ee9a2f3e5d2705b6168f76f80 “wallet: remove fetch pre-selected-inputs responsibility from SelectCoins”

ISTM This should just be in the previous commit.

furszy commented at 11:01 pm on August 9, 2022:

done

furszy force-pushed on Aug 9, 2022

furszy commented at 11:02 pm on August 9, 2022: member

thanks achow101, feedback tackled.

in src/wallet/spend.cpp:881 in c8f3be406d outdated

884-                                              coin_selection_params.m_effective_feerate,
885-                                              1,            /*nMinimumAmount*/
886-                                              MAX_MONEY,    /*nMaximumAmount*/
887-                                              MAX_MONEY,    /*nMinimumSumAmount*/
888-                                              0);           /*nMaximumCount*/
889+    // Fetch manually select coins

aureleoules commented at 12:25 pm on August 11, 2022:

0    // Fetch manually selected coins

furszy commented at 11:38 pm on August 11, 2022:

pushed

in src/wallet/spend.cpp:110 in a53748e332 outdated

104@@ -105,6 +105,47 @@ void CoinsResult::clear()
105     other.clear();
106 }
107 
108+// Fetch and validate the coin control selected inputs.
109+// Coins could be internal (from the wallet) or external.
110+util::Result<SelectedInputs> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,

josibake commented at 12:31 pm on August 11, 2022:

I’d recommend calling this GetPreSelectedInputs. we tend to use Get* throughout the codebase and since this function is for pre-selected inputs only, it makes sense to make the function name specific

furszy commented at 7:13 pm on August 11, 2022:

The rationale behind the Fetch* prefix instead of Get* was that we aren’t only accessing wallet’s UTXOs. We are fetching external UTXO provided by the coin control object as well.

So, with the Fetch* prefix I aimed to make that distinction clear. If the function would be only getting wallet internal data, then yeah, this should had been a Get* for sure.

in src/wallet/spend.h:129 in a53748e332 outdated

124@@ -125,6 +125,26 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
125 std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
126                         const CoinSelectionParams& coin_selection_params);
127 
128+// User manually selected inputs that must be part of the transaction
129+struct SelectedInputs

josibake commented at 12:32 pm on August 11, 2022:

why not call this PreSelectedInputs or ManualInputs? SelectedInputs doesn’t really tell me who selected them and could easily be confused with a SelectionResult

furszy commented at 10:47 pm on August 11, 2022:

sure

josibake commented at 12:39 pm on August 11, 2022: member

Concept ACK

I really like that this simplifies SelectCoins to “give me coins I am allowed to spend and I will select a subset to fund this transaction.” Also, +1 for the fail-fast if we are trying to use pre-selected coins and encounter an error. Thanks for taking the time to benchmark this, too. Makes a compelling case for the performance benefits.

Will do a more thorough code review tomorrow but left some initial feedback

josibake commented at 12:44 pm on August 11, 2022: member

Regarding the bench, it would be nice to have a benchmark for when a user has a few pre-selected inputs and also uses automatic coin selection. My hunch is that that is the more representative scenario

in src/wallet/spend.cpp:907 in c8f3be406d outdated

903+                                         coin_selection_params.m_effective_feerate,
904+                                         1,            /*nMinimumAmount*/
905+                                         MAX_MONEY,    /*nMaximumAmount*/
906+                                         MAX_MONEY,    /*nMinimumSumAmount*/
907+                                         0);           /*nMaximumCount*/
908+    }

aureleoules commented at 3:14 pm on August 11, 2022:

if I’m not mistaken this would avoid a default initialization of a CoinsResult.

0    CoinsResult available_coins{coin_control.m_allow_other_inputs ? AvailableCoins(wallet,
1                                         &coin_control,
2                                         coin_selection_params.m_effective_feerate,
3                                         1,            /*nMinimumAmount*/
4                                         MAX_MONEY,    /*nMaximumAmount*/
5                                         MAX_MONEY,    /*nMinimumSumAmount*/
6                                         0)            /*nMaximumCount*/
7    : CoinsResult{}};

furszy commented at 11:30 pm on August 11, 2022:

Yeah, it would.

But, I’m a ~0 there, as CoinsResult will only have a map and a CAmount after #25734, this will make the code a bit harder to read for a little benefit (AvailableCoins has too many params so you could easily get confused by one of them and don’t read the CoinsResult default initialization at the end).

Probably with something like da44df91 the readability wouldn’t be a problem anymore. And we could initialize CoinsResults in a single line.

in src/wallet/spend.cpp:604 in c8f3be406d outdated

664+    return op_selection_result;
665+}
666+
667+std::optional<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
668+{
669+    CAmount value_to_select = nTargetValue;

aureleoules commented at 3:24 pm on August 11, 2022:

i think you can remove the value_to_select variable and use nTargetValue directly

furszy commented at 11:32 pm on August 11, 2022:

yeah great, less changes if we use value_to_select instead. (value_to_select is used in every AttemptSelection call)

aureleoules commented at 4:17 pm on August 11, 2022: member

Thanks for the all documentation you’ve provided about this PR @furszy, it made the review easier to understand.

4cb629f3df8d3d6ea6ba0d1560b5cc3caf4ee01a: I verified that AvailableCoins returned an empty CoinsResult if m_allow_other_inputs is false, so no need to loop through coins.

65d8ba52e828a3c44b962db2f1f8e230b09aab88: In my understanding, this commit allows AvailableCoins to retrieve only coins that are not already pre-selected by the user, which makes the two sets of coins disjoints. Thus there is no need to remove pre-selected coins from available_coins anymore.

As a side note, I think it would be great to add comments to the functions below to explain that these are user pre-selected coins. https://github.com/bitcoin/bitcoin/blob/243d7bde7859777ebe9442477da1b77419bd3f32/src/wallet/coincontrol.h#L66-L74 Same here https://github.com/bitcoin/bitcoin/blob/243d7bde7859777ebe9442477da1b77419bd3f32/src/wallet/coincontrol.h#L135

a53748e3323a33829134cb575830933ac1893323: I verified FetchSelectedInputs is only a wrapping of the pre-selected-inputs lookup from AvailableCoins with a better error handler.

e94e9ce68dc2fdb06a2da17020e067baede72d1e: I believe this commit makes the Coin Selection flow much easier to read and also avoids iterating over the wallet’s tx map, which increases performance.

nits in commit description: “we can fail right away without ~~compute~~ computing the wallet…” “In charge of ~~select~~ selecting all the user manually selected coins…”

3e5bfcc75db1f2abf875fbcd1ed68f8166683ea4: Verified that if there are sufficient funds with the manually selected coins, there is no need to call the automatic coin selection.

I ran the benchmark both on master and this PR and got similar results as the PR description! I would agree with @josibake regarding having a benchmark for “when a user has a few pre-selected inputs and also uses automatic coin selection” though.

furszy force-pushed on Aug 11, 2022

furszy commented at 0:10 am on August 12, 2022: member

Thanks for the review guys :smile:.

Regarding the bench, it would be nice to have a benchmark for when a user has a few pre-selected inputs and also uses automatic coin selection. My hunch is that that is the more representative scenario

You know @josibake, I didn’t add it here mainly because this PR improves a particular scenario; when the user pre-selected inputs and disallowed the wallet automatic coin selection. This PR doesn’t improve, at least not directly, the preset inputs + automatic coin selection scenario.

But sure, will add another bench case for it :). Was planning to push it on a follow-up work to measure other changes but can sneak it here as well.

As a side note, I think it would be great to add comments to the functions below to explain that these are user pre-selected coins.

Agree @aureleoules, shouldn’t be called setSelected and have some comments there. But would say to tackle it in a separate PR so the entire CCoinControl class receives some love.

furszy force-pushed on Aug 12, 2022

furszy commented at 6:40 pm on August 12, 2022: member

As requested, added a basic benchmark for the preset inputs + automatic coin selection scenario in 631f766. In future PRs, we could continue expanding the bechmarks, adding cases for different parts of the transaction creation process.

DrahtBot added the label Needs rebase on Aug 17, 2022

furszy force-pushed on Aug 17, 2022

DrahtBot removed the label Needs rebase on Aug 17, 2022

furszy force-pushed on Aug 17, 2022

furszy commented at 6:03 pm on August 17, 2022: member

rebased on master, conflict solves. Ready to go

furszy force-pushed on Aug 17, 2022

furszy force-pushed on Aug 18, 2022

DrahtBot added the label Needs rebase on Aug 19, 2022

furszy force-pushed on Aug 19, 2022

furszy commented at 1:03 pm on August 19, 2022: member

rebased, conflicts solved.

DrahtBot removed the label Needs rebase on Aug 19, 2022

DrahtBot added the label Needs rebase on Aug 22, 2022

furszy force-pushed on Aug 24, 2022

furszy commented at 6:33 pm on August 24, 2022: member

rebased, conflicts solved. Wasn’t so simple this time.

DrahtBot removed the label Needs rebase on Aug 24, 2022

furszy force-pushed on Aug 26, 2022

furszy force-pushed on Aug 27, 2022

w0xlt approved

w0xlt commented at 6:28 pm on August 30, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25685/commits/08012f03bf2cc9ec4f8b21d5643f3423acb1b5d8

furszy commented at 1:58 pm on September 6, 2022: member

Friendly ping to previous reviewers @aureleoules @josibake @achow101. Would be nice to move forward with this one, it will unlock #25806 which will unlock #24845.

in src/wallet/rpc/spend.cpp:1140 in 0d6da1668d outdated

1136@@ -1137,7 +1137,7 @@ RPCHelpMan send()
1137             {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1138                 Cat<std::vector<RPCArg>>(
1139                 {
1140-                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
1141+                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{true}, "If inputs are specified, automatically include more if they are not enough."},

achow101 commented at 3:58 pm on September 6, 2022:

In 0d6da1668d6762b1071529c39658812ba68720d8 “wallet: skip available coins fetch if “other inputs” are disallowed”

We should avoid changing the defaults of the RPC interface as that may result in users scripts breaking/doing unexpected things. While the default value of CCoinControl can be changed, the RPCs that actually used the default of false should now be setting that rather than their defaults also changing.

furszy commented at 9:01 pm on September 8, 2022:

This is more a bugfix than a behavior change actually. Let me expand:

Before, the default value was not false all the time, it was a hidden true. We were having a bug/workaround to always select coins automatically from the wallet even when the user explicitly set add_inputs=false (which means “don’t select coins automatically”).

This is why, even when I changed the field default value inside coin control, I didn’t need to change any test on that commit. They actually were all depending on the bug to work properly! (including the GUI..).

Can try it by setting add_inputs=false in any of the fund* RPC commands without pre-setting any input. The process will automatically select coins from the wallet even when you are explicitly blocking it (the proper expected behavior is fail due “insufficient funds”). I’m going to add test coverage for this.. so the previous bad behavior and the fix commit can be contrasted. It was a truly misleading behavior.

Side from that: I know what you mean, if the user pre-set inputs, the previous default value was false. So, to not change behavior in this scenario, by default, no automatic coin selection should be performed on the RPC commands. Will add coverage for this as well.. –> not needed since #26053. Thanks.

furszy commented at 6:49 pm on September 9, 2022:

Pushed #26053 to fulfill this purposes, make the bug visible and to try to fix it for 24.0. Once that one gets merged, changes in 0d6da166 will be smaller and simpler to review.

in src/wallet/coinselection.cpp:447 in e6b9e23699 outdated

443@@ -444,6 +444,12 @@ void SelectionResult::AddInput(const OutputGroup& group)
444     m_use_effective = !group.m_subtract_fee_outputs;
445 }
446 
447+void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool m_subtract_fee_outputs)

achow101 commented at 4:00 pm on September 6, 2022:

In e6b9e23699a5dc8e08711004f51c877fa3912db1 “wallet: encapsulate pre-selected-inputs lookup into its own function”

nit: only member variables should have the m_ prefix.

furszy commented at 9:07 pm on September 8, 2022:

yeah, removing it..

in src/bench/wallet_create_tx.cpp:120 in 08012f03bf outdated

137+                                                                                        {{/*num_of_internal_inputs=*/4}}); }
138+
139+static void WalletCreateTxUsePresetInputsAndCoinSelection(benchmark::Bench& bench) { WalletCreateTx(bench, OutputType::BECH32, /*allow_other_inputs=*/true,
140+                                                                                                    {{/*num_of_internal_inputs=*/4}}); }
141 
142-BENCHMARK(WalletCreateTxUseOnlyPresetInputs);

achow101 commented at 4:05 pm on September 6, 2022:

In 08012f03bf2cc9ec4f8b21d5643f3423acb1b5d8 “bench: add benchmark for tx creation using pre-set inputs and coin selection”

nit: This should be in the previous commit.

furszy commented at 9:07 pm on September 8, 2022:

ok, squashing it.

achow101 referenced this in commit 2e3cd26a1a on Sep 14, 2022

DrahtBot added the label Needs rebase on Sep 14, 2022

furszy force-pushed on Sep 15, 2022

furszy commented at 4:59 pm on September 15, 2022: member

Rebased on master, achow101 feedback tackled. Thanks.

Changes:

Removed m_ prefix from AddInputs function signature.
Squashed benchmark 08012f03 inside 2d9ea15.
Added test case for when the user explicitly disallows the automatic coin selection and does not provide inputs (on master, this test fails because the wallet invalidly fall back to select coins automatically even when the user provided add_inputs=false).

DrahtBot removed the label Needs rebase on Sep 15, 2022

aureleoules commented at 2:24 pm on September 16, 2022: member

ACK 2d9ea15fe9773c3efe349579413b4a4e56c30454. Reviewed with range-diff since my last review. Also verified all commits compile.

aureleoules commented at 3:06 pm on September 16, 2022: member

Here are my bench results (with sudo pyperf system tune).

PR

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
3,165,243.33	315.93	1.6%	18,936,699.67	7,255,808.00	2.610	507,961.67	2.3%	0.10	`WalletCreateTxUseOnlyPresetInputs`
2,503,088,950.33	0.40	2.9%	12,628,647,472.00	5,467,638,592.00	2.310	2,036,987,578.67	0.3%	84.82	`WalletCreateTxUsePresetInputsAndCoinSelection`

Master

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
66,423,839.67	15.05	0.4%	102,368,093.00	152,193,632.00	0.673	22,866,396.33	0.6%	2.20	`WalletCreateTxUseOnlyPresetInputs`
2,546,070,421.67	0.39	5.5%	12,710,666,931.00	5,631,797,760.00	2.257	2,052,977,444.33	0.3%	86.15	:wavy_dash: `WalletCreateTxUsePresetInputsAndCoinSelection` (Unstable with ~3.0 iters. Increase `minEpochIterations` to e.g. 30)

You know josibake, I didn’t add it here mainly because this PR improves a particular scenario; when the user pre-selected inputs and disallowed the wallet automatic coin selection. This PR doesn’t improve, at least not directly, the preset inputs + automatic coin selection scenario.

Indeed :+1: WalletCreateTxUseOnlyPresetInputs is significantly faster now and WalletCreateTxUsePresetInputsAndCoinSelection has not changed.

Unstable with ~3.0 iters. Increase minEpochIterations to e.g. 30

nit: maybe increase iterations? not sure how important this is.

in src/bench/wallet_create_tx.cpp:99 in 2d9ea15fe9 outdated

94+    // Generate destinations
95+    CScript dest = GetScriptForDestination(getNewDestination(wallet, output_type));
96+
97+    // Generate chain; each coinbase will have two outputs to fill-up the wallet
98+    const auto& params = Params();
99+    unsigned int chain_size = 150000; // 150k blocks means 300k UTXO for the wallet (minus 200 due COINBASE_MATURITY)

achow101 commented at 5:49 pm on September 16, 2022:

In 2d9ea15fe9773c3efe349579413b4a4e56c30454 “bench: benchmark transaction creation process”

150000 blocks is excessive. This makes the benchmark extremely slow to run. It increases the runtime of make check by several orders of magnitude. A few hundred blocks should be enough to illustrate the performance difference, I don’t think we need more than a thousand, let alone 150,000.

furszy commented at 6:49 pm on September 16, 2022:

I initially set it low, at 2k (4k utxo), but it wasn’t really illustrative. This becomes a problem when your wallet has a high tx volume. Otherwise, traverse 4k utxo is quite fast, almost imperceptible. e.g.

PR

ns/op	op/s	err%	total	benchmark
1,427,966.60	700.30	1.2%	0.08	`WalletCreateTxUseOnlyPresetInputs`

Master

ns/op	op/s	err%	total	benchmark
1,576,250.00	634.42	0.5%	0.09	`WalletCreateTxUseOnlyPresetInputs`

Let me try with 20k instead. We should try to get a middle-ground between the high value (agree that it’s too high) and a number that can provide meaningful information on the context where this becomes a real problem for the user.

achow101 commented at 8:56 pm on September 19, 2022:

20k is still a lot and the setup does still take quite a long time. While the real time difference may not be that noticeable, I think ns/op and `op/s sufficiently illustrate the difference even when this is run with 2000 blocks.

furszy commented at 2:15 pm on September 20, 2022:

I’m not so convinced there. I do understand the make check time concern but that is limiting our testing capabilities. Some big issues aren’t problems at all in a small scale.

As time isn’t constant between runs and the difference is so small with 2000 blocks, stuff like this can easily go unnoticed for a really long time. Which kinda break the purpose of having a benchmark for it.

E.g. lets say that we have this benchmark in the code with the 2000 blocks param. And someone introduces a similar problematic (traverse the whole wallet map and do other stuff for no reason). Would you care about it if the benchmark returns a time diff between 60-100 ns? or would you just say “it’s ok”?. We could be hurting badly big wallets with our changes without noticing it at all.

Saying that, will decrease the number to 10k blocks. So we get closer to each other’s number. Would you be ok with it? I think that the 2k blocks number is too small to be helpful on this context.

As future thoughts, would be nice to be able to re-use setups in the benchmarks. So every WalletCreateTx* would not require to create a new chain per run. Plus, would be good to not run all the benchmarks by default in make check. Only people working in the specific area should run them (or even better, maybe only PRs with certain labels should run them and compare the results with master’s result).

achow101 commented at 6:54 pm on September 21, 2022:

Even with 2000 blocks I am still able to measure a difference. It’s also a lot more noticeable (and everything is a lot slower) if configured with --enable-debug.

Using 5000 is quite a bit faster than 10000, and the improvement is still quite noticeable without debug.

furszy commented at 11:33 pm on September 21, 2022:

Interesting, that is why we have been seeing different pictures of this. Without the debug mode, the difference that have been seeing locally is almost imperceptible with 2k (less than 100 ns). That also explains why the CI was timing out before, benchmarks there are running on debug mode too.

furszy force-pushed on Sep 16, 2022

furszy commented at 8:30 pm on September 16, 2022: member

Updated per feedback, thanks aureleoules and achow101.

Per aureleoules review: Increased the bench iterations from 3 to 5.
Per achow101 review: Decreased the number of blocks used in the benchmarks from 150k to 20k so make check doesn’t take too much time on low-end devices and we still continue benchmarking the tx creation process on a wallet with a reasonable amount of txes/utxo.

aureleoules commented at 10:10 pm on September 16, 2022: member

re-ACK 1c7417711d2b9353350a8b5b2e580850cd1b5d3b

furszy force-pushed on Sep 20, 2022

aureleoules commented at 2:43 pm on September 20, 2022: member

ACK 40452b8be29c85fbd2e96f93fa1b9ad37e6d44d8

PR

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
3,252,252.20	307.48	0.7%	19,062,206.80	7,444,723.20	2.560	504,438.20	2.5%	0.18	`WalletCreateTxUseOnlyPresetInputs`

Master

ns/op	op/s	err%	ins/op	cyc/op	IPC	bra/op	miss%	total	benchmark
7,162,568.40	139.61	1.1%	24,654,767.60	16,410,489.60	1.502	1,935,246.40	1.3%	0.39	`WalletCreateTxUseOnlyPresetInputs`

DrahtBot added the label Needs rebase on Sep 21, 2022

furszy force-pushed on Sep 21, 2022

furszy commented at 1:14 pm on September 21, 2022: member

rebased, conflicts solved.

DrahtBot removed the label Needs rebase on Sep 21, 2022

furszy force-pushed on Sep 21, 2022

furszy commented at 11:40 pm on September 21, 2022: member

Updated per feedback. Only changed the benchmark chain size, from 10k to 5k. Tiny diff.

aureleoules approved

aureleoules commented at 4:32 pm on October 3, 2022: member

re-ACK 044953de5905d722fe07d0fb529125574bdb4902 - only reduced benchmark chain size since last my ACK

DrahtBot added the label Needs rebase on Oct 4, 2022

furszy force-pushed on Oct 4, 2022

DrahtBot removed the label Needs rebase on Oct 4, 2022

furszy commented at 0:00 am on October 5, 2022: member

Rebased after #26203 merge.

Plus, to be clear over #26203#pullrequestreview-1128801074, have decoupled what previously was in aa5fdbad in two commits: 71f5817 and 8326d17.

Ready to go.

furszy force-pushed on Oct 5, 2022

theStack commented at 11:58 pm on October 5, 2022: contributor

Concept ACK

in src/wallet/spend.cpp:585 in d54606dad0 outdated

584         // or the raw output value based on the 'subtract_fee_outputs' flag.
585-        if (selection_target > 0) return std::nullopt;
586+        if (selection_target > 0) return std::nullopt; // fails only if 'allow_other_inputs=false'
587         SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
588         result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
589         result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);

aureleoules commented at 4:01 pm on October 6, 2022:

in d54606dad097ac050dfdd1c820e3d7172b036146 IMO this boolean logic:

0    // If automatic coin selection was disabled or if we already covered the
1    // target with the preset inputs: try to return the result right away
2    if (!coin_control.m_allow_other_inputs && selection_target > 0) return std::nullopt;
3
4    if (selection_target <= 0) {
5        SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
6        result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
7        result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
8        return result;
9    }

is easier to understand than this one:

 0    // If automatic coin selection was disabled or if we already covered the
 1    // target with the preset inputs: try to return the result right away
 2    if (!coin_control.m_allow_other_inputs || selection_target <= 0) {
 3        // 'selection_target' is computed on `PreSelectedInputs::Insert` which decides whether to use the effective value
 4        // or the raw output value based on the 'subtract_fee_outputs' flag.
 5        if (selection_target > 0) return std::nullopt; // fails only if 'allow_other_inputs=false'
 6        SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
 7        result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
 8        result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
 9        return result;
10    }

furszy commented at 8:44 pm on October 6, 2022:

yeah, better.

aureleoules commented at 4:06 pm on October 6, 2022: member

Left a comment, otherwise LGTM.

furszy force-pushed on Oct 6, 2022

furszy commented at 8:57 pm on October 6, 2022: member

Thanks @aureleoules, feedback applied.

Simplified the SelectCoins early return logic. Small diff.

aureleoules approved

aureleoules commented at 7:45 am on October 7, 2022: member

re-ACK 1c137b577130b9669db9a7b167f44f6304f7df42

furszy force-pushed on Oct 20, 2022

furszy commented at 8:14 pm on October 20, 2022: member

Rebased after #26158 merge.

Only diff is inside the benchmarks, where both of them were marked as low priority to not run them in make check.

Ready for another review round.

achow101 commented at 11:03 pm on October 25, 2022: member

ACK 5bea640cfb29a7d4e2edaa2a24c0457dbda41b0a

aureleoules approved

aureleoules commented at 11:43 pm on October 25, 2022: member

reACK 5bea640cfb29a7d4e2edaa2a24c0457dbda41b0a

in src/wallet/spend.h:125 in a67d26509d outdated

120@@ -121,6 +121,26 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
121 std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
122                         const CoinSelectionParams& coin_selection_params);
123 
124+// User manually selected inputs that must be part of the transaction
125+struct PreSelectedInputs

S3RK commented at 6:55 am on October 26, 2022:

nit: Can we just use SelectionResult here? All we do we preset input is just copy them to SelectionResult anyway.

furszy commented at 5:03 pm on October 26, 2022:

We actually had this convo at the beginning of the PR (https://github.com/bitcoin/bitcoin/pull/25685#discussion_r938067217).

I implemented it two/three months ago and the end result wasn’t so satisfactory.

If my memory doesn’t fail me, the reasons were:

We would be mixing responsibilities if we do that. The SelectionResult class is particular to the coin selection process (in the future, it might even be decoupled into a separated library) and FetchSelectedInputs is a wallet method that we use to obtain a set of outputs (similar to AvailableCoins but instead of traversing the whole wallet tx map, it fetches the specific outputs directly), which could be used in different contexts and not only inside the tx creation process (places where the SelectionResult class wouldn’t make much sense).
The SelectionResult constructor requires a target, which we don’t have up until we fetch and sum-up all the manually selected outputs. So, we would end up doing the same that we do inside SelectCoins but inside FetchSelectedInputs: (1) first fetch and store all the outputs in a vector, then at the end of the method, create a SelectionResult with the accurate target and return it (so there would really be no difference with the current approach unless we add another constructor and a new AddInput method inside SelectionResult that keeps track of target etc or.. pass the tx target to FetchSelectedInputs which I think that is ugly as the tx target has nothing to do with a fetching-only function). 
Plus, the new struct caches the total amount and contemplates the effective/raw amount distinction. Which, for example, made #26203 issue a non-issue here. .

With this I’m not saying that we should marry to this struct (we could also use CoinsResult instead) but I don’t think that changing it to SelectionResult will make the code better (at least for now). It would probably just continue entangling the coin selection code inside wallet general functions that we should be able to re-use in other processes.

Plus, after the next PR in the line #25806, I think that this process will be in a much better shape to re-do this convo once more. Mainly because the coin selection process will not be so attached to the wallet internals. So, we might be able to decouple it into a different module or an external library.

in src/wallet/spend.cpp:583 in 36707c8137 outdated

583+    // Return if automatic coin selection is disabled, and we don't cover the selection target
584+    if (!coin_control.m_allow_other_inputs && selection_target > 0) return std::nullopt;
585+
586+    // Return if we can cover the target only with the preset inputs
587+    if (selection_target <= 0) {
588         SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);

S3RK commented at 7:02 am on October 26, 2022:

nit: I’d suggest to create SelectionResult for preselected inputs unconditionally. This way use can reuse it later when you need to merge it with automatic coin selection.

furszy commented at 6:40 pm on October 26, 2022:

I thought about it but It’s not that straightforward. The SelectionResult target is provided on the constructor and then only modified by the Merge function (which adds the merged selection target to the base one).

So, it’s not clear with which target the selection result should be initialized:

If the preset inputs cover the target amount, then it needs to be initialized to the target amount, not to the preset inputs total amount. So the selection result can be returned early.
If the preset inputs don’t cover the target amount, then it needs to be initialized to the preset inputs total amount, because we need to cover the total amount when we merge it with the SelectionResult that comes from AutomaticCoinSelection (which contains a selection for the target amount minus the preset inputs total).

S3RK commented at 7:01 pm on October 26, 2022:

Yap, agreed.

in src/wallet/spend.cpp:598 in 36707c8137 outdated

593@@ -594,9 +594,6 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
594     SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL);
595     preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
596     op_selection_result->Merge(preselected);
597-    if (op_selection_result->GetAlgo() == SelectionAlgorithm::MANUAL) {
598-        op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);

S3RK commented at 7:03 am on October 26, 2022:

nit: I’d say we need to recalculate waste after adding preselected inputs because otherwise wronge waste score would be passed to USDT

furszy commented at 5:05 pm on October 26, 2022:

funny that I mentioned this here #25685 (review) (last sentence) almost 3 months ago.

Will re-check it.

furszy commented at 6:56 pm on October 26, 2022:

Pushed. Squashed inside a8a75346. Thanks

S3RK commented at 7:35 am on October 26, 2022: contributor

I’m a bit late to the party. Almost code review ACK. Need a bit more time to review. Left a couple nits. The biggest one being about whether we need a new struct about preselected inputs.

furszy commented at 5:07 pm on October 26, 2022: member

I’m a bit late to the party.

It’s never late if you come with beers and/or food.

wallet: skip available coins fetch if "other inputs" are disallowed

no need to waste resources calculating the wallet available coins if
they are not going to be used.

The 'm_allow_other_inputs=true` default value change is to correct
an ugly misleading behavior:

The tx creation process was having a workaround patch to automatically
fall back to select coins from the wallet if `m_allow_other_inputs=false`
(previous default value) and no manual inputs were selected.

This could be seen in master in flows like `sendtoaddress`, `sendmany`
and even the GUI, where the `m_allow_other_inputs` value isn't customized
and the wallet still selects and adds coins to the tx internally.

94c0766b0c

wallet: skip manually selected coins from 'AvailableCoins' result

No need to walk through the entire wallet's txes map just to get
coins that we could have gotten by just doing a simple map.find(out.hash).
(Which is what we are doing inside `SelectCoins` anyway)

37e7887cb4

wallet: encapsulate pre-selected-inputs lookup into its own function

First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.

(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)

295852f619

wallet: remove fetch pre-selected-inputs responsibility from SelectCoins

so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.

----------------------

And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:

1) AutomaticCoinSelection.
2) SelectCoins.

1) AutomaticCoinSelection:
   Receives a set of coins and selects the best subset of them to
   cover the target amount.

2) SelectCoins
   In charge of select all the user manually selected coins first ("pre-set inputs"), and
   if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
   subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
   remaining value.

5baedc3351

wallet: simplify preset inputs selection target check

we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`,
which internally decides whether to use the effective value or the raw output value based on
the 'subtract_fee_outputs' flag.

f41712a734

wallet: SelectCoins, return early if target is covered by preset-inputs a8a75346d7

bench: benchmark transaction creation process

Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.

Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.

-----------------------

Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.

Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.

Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.

3fcb545ab2

furszy force-pushed on Oct 26, 2022

furszy commented at 6:58 pm on October 26, 2022: member

Updated per @S3RK feedback, thanks.

Only change is the ComputeAndSetWaste call after the merge of the two selection results (preset and automatic selection results). So the waste score is properly calculated for the USDT logging. Small diff

aureleoules approved

aureleoules commented at 7:32 pm on October 26, 2022: member

reACK 3fcb545ab26be3e785b5e5654be0bdc77099d827

S3RK commented at 7:26 am on October 27, 2022: contributor

Code Review ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827

achow101 commented at 9:48 pm on October 27, 2022: member

ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827

achow101 merged this on Oct 27, 2022

achow101 closed this on Oct 27, 2022

sidhujag referenced this in commit 1da4c50bd5 on Oct 28, 2022

domob1812 referenced this in commit aadf6733f6 on Nov 7, 2022

achow101 referenced this in commit f0c4807a6a on Dec 5, 2022

sidhujag referenced this in commit 8dde7021c5 on Dec 5, 2022

furszy referenced this in commit 0db926e0e3 on Dec 6, 2022

furszy referenced this in commit abe2b01095 on Dec 14, 2022

furszy referenced this in commit cc3bf0e187 on Jan 18, 2023

furszy referenced this in commit 54ac0f8c2f on Jan 18, 2023

furszy referenced this in commit 072b411550 on Feb 20, 2023

sidhujag referenced this in commit 0a92dce5e4 on Mar 25, 2023

furszy referenced this in commit 8cd7730230 on Apr 2, 2023

furszy referenced this in commit dc1cc1c359 on Apr 3, 2023

furszy referenced this in commit 70227d66d5 on Apr 11, 2023

achow101 referenced this in commit 27dcc07c08 on Apr 11, 2023

furszy deleted the branch on May 27, 2023

RandyMcMillan referenced this in commit c3d17ccafe on May 27, 2023

bitcoin locked this on May 26, 2024

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

# Context (Current Flow on Master)

# Process Workflow Changes

# Summarizing the Improvements:

Result on this PR (tip f6d0bb2d):

Result on master (tip 4a4289e2):

Conflicts

Sources Architecture:

Tangible Improvements:

This is the result on this PR (tip f6d0bb2d):

This is the result on current master (tip 4a4289e2):

Summary

PR

Master

PR

Master

PR

Master