wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups.

furszy commented at 3:09 pm on April 27, 2022: member

This started in #24845 but grew out of scope of it.

So, points tackled:

Avoid extra GetWalletTx lookups inside AvailableCoins -> IsSpentKey. IsSpentKey was receiving the tx hash and index to internally lookup the tx inside the wallet’s map. As all the IsSpentKey function callers already have the wtx available, them can provide the scriptPubKey directly.
Most of the time, we call Wallet::AvailableCoins, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by “only_spendable” coins inside Wallet::AvailableCoins directly. (the non-spendable coins skip examples are inside AttemptSelection->GroupOutputs and GetAvailableBalance).
Refactored AvailableCoins in several ways:

a) Now it will return a new struct CoinsResult instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). –> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer.

b) Unified all the ‘wtx.tx->vout[I]’ calls into a single call (coming from this comment #24699 (review)).
The wallet IsLockedCoin and IsSpent methods now accept an OutPoint instead of a hash:index. Which let me cleanup a bunch of extra code.
Speeded up the wallet ‘GetAvailableBalance’: filtering AvailableCoins by spendable outputs only and using the ‘AvailableCoins’ retrieved total_amount instead of looping over all the retrieved coins once more.

Side topic, all this process will look even nicer with #25218

fanquake added the label Wallet on Apr 27, 2022

fanquake requested review from achow101 on Apr 27, 2022

DrahtBot commented at 2:00 am on April 28, 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:

#25269 (wallet: re-activate the not triggered “AmountWithFeeExceedsBalance” error by furszy)
#25183 (rpc: Segwit-only inputs option for fundrawtransaction by aureleoules)
#24699 (wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101)
#24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

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.

jonatack commented at 10:45 am on April 28, 2022: contributor

@furszy It looks like the qt wallet test is failing in the CI. You can run ./src/qt/test/test_bitcoin-qt in your dev environment to reproduce and debug the issue.

furszy commented at 2:01 pm on April 28, 2022: member

Ah, thanks @jonatack 👌. Just noticed that the GUI WalletTests isn’t run by default in my OS under make check.

It’s an easy fix; as the test doesn’t start the balance polling timer, the model does not have the balance cached so.. as 5babfb7 make uses of it, the creation process fails with a not-enough balance error.

Will push the fix and expand + improve the test coverage a bit more. And.. most likely will decouple that last commit into a separate PR in few days. Have some further improvements locally for the wallet model area that can package out all together that aren’t really part of the scope of this PR. (like a mutex to be able to access the model cached balance from different threads, connect it to the widgets etc..)

furszy renamed this:
~~wallet: remove extra wtx lookup in 'AvailableCoins' + several code improvements.~~
wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups.
on Apr 29, 2022

furszy force-pushed on May 9, 2022

furszy commented at 8:58 pm on May 9, 2022: member

The GUI WalletModel::prepareTransaction will now use the cached balance to make the funds availability check if the user didn’t manually selected outputs to spend. This will avoid an entire on-demand recalculation of the available balance which is quite expensive as the wallet has to walk through the entire txs map.

Dropped the last two commits that were implementing point (6) described above. They are now part of https://github.com/bitcoin-core/gui/pull/598

in src/wallet/spend.cpp:236 in 2cdd2a1a54 outdated

234-            balance += out.txout.nValue;
235-        }
236-    }
237-    return balance;
238+    // Copy to filter by spendable coins only
239+    CCoinControl coinControl = _coinControl ? *_coinControl : CCoinControl();

achow101 commented at 8:23 pm on May 16, 2022:

In 2cdd2a1a5498cb8767799dd26a4082e9d3b064ee “wallet: speedup ‘GetAvailableBalance’ filtering by spendable outputs only and using the ‘AvailableCoins’ retrieved total_amount.”

What is the purpose of the rename and making an empty CCoinControl here? AvailableCoins can take nullptr for coinControl.

furszy commented at 6:09 pm on May 21, 2022:

The previous flow was:

0std::vector<COutput> vCoins;
1AvailableCoins(wallet, vCoins, coinControl);
2for (const COutput& out : vCoins) {
3     if (out.spendable) { --> // this is the reason
4         balance += out.txout.nValue;
5     }
6}

To remove the second unnecessary for-loop, as the function arg is const CCoinControl*, in order to continue having the same calculation (skipping the non-spendable coins), needed to create a local copy of the coinControl arg and set the new flag m_include_only_spendable_outputs to true (which by default is false).

So the coins vector and the total balance returned by AvailableCoins has all, and only, the spendable coins directly.

In the new flow:

0CCoinControl coinControl = _coinControl ? *_coinControl : CCoinControl();
1coinControl.m_include_only_spendable_outputs = true; // --> this enforces that non-spendable coins are filtered from `AvailableCoins` result.
2return AvailableCoins(wallet, &coinControl).total_amount;

achow101 commented at 8:27 pm on May 16, 2022: member

I don’t quite understand the purpose of the first commit. It doesn’t seem like it materially improves anything?

furszy commented at 5:54 pm on May 21, 2022: member

Hey achow101, thanks for the review!

I don’t quite understand the purpose of the first commit. It doesn’t seem like it materially improves anything?

We are skipping the non-spendable coins that appear in vCoins (AvailableCoins result) later, in several parts of the CreateTransaction and GetBalance flows:

GetAvailableBalance was (1) getting all the available coins calling AvailableCoins and, right away, walking through the entire vector, skipping the non-spendable coins, to calculate the total balance. 
Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.

So, the purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.

Note: this speedup is for all the processes that calls to CreateTransaction and GetBalance* internally.

furszy force-pushed on May 23, 2022

furszy commented at 6:18 pm on May 23, 2022: member

Rebased after #25083 merge. Conflicts solved.

in src/wallet/rpc/spend.cpp:399 in 3082f2270f outdated

395@@ -395,6 +396,8 @@ RPCHelpMan sendmany()
396         coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
397     }
398 
399+    coin_control.m_include_only_spendable_outputs = true;

achow101 commented at 3:53 pm on June 2, 2022:

In 3082f2270fa96eaf2bd1d5e69d56fce9588a4f79 “wallet: add ’m_include_only_spendable_outputs’ filter to CoinControl”

Instead of setting this parameter all the way out here in the RPC, I think it would make more sense to set it closer to the places where it actually matters, e.g. in CreateTransactionInternal right before AvailableCoins is called. This way we won’t have to remember to always set this option for every caller to CreateTransaction.

furszy commented at 4:25 pm on June 2, 2022:

I was doing it there first but the CCoinControl arg of CreateTransactionInternal is const. Options were (1) modify it by copying the coin control object to a temp variable or (2) change the function signature.

And at least in this case, I wasn’t totally good with those two options (not against neither, it was a “maybe”). But open for new thoughts, would you prefer option 1 more?

achow101 commented at 5:08 pm on June 2, 2022:

Actually, since this option is really just for AvailableCoins itself, it might make more sense to make it a parameter to AvailableCoins rather than having it in CCoinControl.

furszy commented at 6:02 pm on June 2, 2022:

👍🏼 , added it in 8daa58b.

in src/wallet/spend.cpp:170 in aa0be53fcd outdated

165@@ -166,24 +166,27 @@ CoinsResult AvailableCoins(const CWallet& wallet,
166         bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL);
167 
168         for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
169+            const CTxOut& output = wtx.tx->vout[i];
170+            COutPoint outpoint(wtxid, i);

achow101 commented at 3:55 pm on June 2, 2022:

In aa0be53fcd85f8ad8102c5843baeec9d0740297a “wallet::AvailableCoins, stop calling ‘wtx.tx->vout[i]’ multiple times.”

nit: const

0            const COutPoint outpoint(wtxid, i);

in src/wallet/receive.cpp:209 in da875e6e80 outdated

203@@ -204,9 +204,9 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
204     bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
205     CAmount nCredit = 0;
206     uint256 hashTx = wtx.GetHash();
207-    for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)
208-    {
209-        if (!wallet.IsSpent(hashTx, i) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {
210+    for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)  {
211+        COutPoint outpoint(hashTx, i);
212+        if (!wallet.IsSpent(outpoint) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {

achow101 commented at 3:58 pm on June 2, 2022:

In da875e6e8008f9b7053b5ce42fade49a322e0a50 “wallet::IsSpent accepting ‘COutPoint’ instead of hash:index.”

nit: inline

0        if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {

furszy force-pushed on Jun 2, 2022

furszy commented at 6:06 pm on June 2, 2022: member

Ready, feedback applied:

Moved only_spendable from be a CoinControl field to be a AvailableCoins param.
Nits tackled.

furszy force-pushed on Jun 5, 2022

theStack commented at 8:18 pm on June 5, 2022: contributor

Concept ACK

in src/wallet/spend.h:43 in 5646ccf4af outdated

39+    // Sum of all the coins amounts
40+    CAmount total_amount{0};
41+};
42 /**
43- * populate vCoins with vector of available COutputs.
44+ * Populate vCoins with vector of available COutputs.

theStack commented at 4:44 pm on June 7, 2022:

The vCoins parameter doesn’t exist anymore, i.e. the comment should be adapted accordingly. E.g.

0 * Return vector of available COutputs.

in src/wallet/receive.cpp:207 in 5646ccf4af outdated

203@@ -204,10 +204,9 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx,
204     bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
205     CAmount nCredit = 0;
206     uint256 hashTx = wtx.GetHash();
207-    for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)
208-    {
209-        if (!wallet.IsSpent(hashTx, i) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {
210-            const CTxOut &txout = wtx.tx->vout[i];
211+    for (unsigned int i = 0; i < wtx.tx->vout.size(); i++)  {

theStack commented at 4:48 pm on June 7, 2022:

nit: unnecessary whitespace

0    for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {

in src/wallet/spend.cpp:242 in 5646ccf4af outdated

267+                          1,            /*nMinimumAmount*/
268+                          MAX_MONEY,    /*nMaximumAmount*/
269+                          MAX_MONEY,    /*nMinimumSumAmount*/
270+                          0,            /*nMaximumCount*/
271+                          true          /*spendable_only*/
272+                          ).total_amount;

theStack commented at 5:05 pm on June 7, 2022:

Not tested, but would it make sense to set the default parameter of spendable_only of AvailableCoins to true rather than false? Then this call would simply be

0    return AvailableCoins(wallet, coinControl).total_amount;

As far as I can see, there are only 4 instances in total that call AvailableCoins, so it should be quite easy to find out where false has to be passed explicitly for the spendable_only parameter.

furszy commented at 2:14 pm on June 8, 2022:

You found an existing issue in the RPC sendall command :).

When send_max is set, sendall uses all the coins returned from AvailableCoins as inputs for a new transaction, without checking if the coin are spendable/solvable first.

So.. this command should be always failing in master if the user sets the send_max param and the wallet has at least one non-spendable or non-solvable coin.

Note: This is easily solved passing the new spendable_only=false flag to AvailableCoins here.

theStack commented at 5:12 pm on June 7, 2022: contributor

LGTM, changes all make sense to me, also thanks to chat we had in IRC :)

Left a few nits below. Also, can you change the commit texts to have a shorter line length? This makes the git history more readable. Breaking at 72-80 characters seem to be good values.

wallet: return 'CoinsResult' struct in `AvailableCoins`

Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.

Note:
This new struct, down the commits line, will contain other `AvailableCoins` useful results.

4ce235ef8f

wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times 9472ca0a65

wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) 91902b7720

wallet: IsSpent, 'COutPoint' arg instead of (hash, index) a06fa94ff8

wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n)

This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.

3d8a282257

wallet: avoid extra IsSpentKey -> GetWalletTx lookups 4b83bf8dbc

wallet: remove unused IsSpentKey(hash, index) method cdf185ccfb

wallet: add 'only_spendable' filter to AvailableCoins

We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows:

GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance.

Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.

So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.

Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally.

162d4ad10f

wallet: GetAvailableBalance, remove double walk-through every available coin

Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount.

fd5c996d16

furszy force-pushed on Jun 8, 2022

furszy commented at 2:36 pm on June 8, 2022: member

thanks for the feedback theStack!

Applied the following changes:

Re-wrote commits titles to be 72-80 characters long.
Moved the only_spendable arg of AvailableCoins to be true by default.
By having spendable_only=true by default, fixed an already existent hidden issue in sendall RPC command -> description

brunoerg commented at 8:42 pm on June 15, 2022: contributor

Concept ACK

achow101 commented at 8:52 pm on June 16, 2022: member

ACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa

brunoerg approved

brunoerg commented at 9:22 pm on June 16, 2022: contributor

crACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa

fanquake requested review from theStack on Jun 16, 2022

fanquake requested review from murchandamus on Jun 16, 2022

w0xlt approved

w0xlt commented at 4:54 am on June 17, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/25005/commits/fd5c996d1609e6f88769f6f3ef0c322e3435b3aa

Clear improvement in code readability and Available Coins() logic.

in src/wallet/spend.h:43 in fd5c996d16

39+    // Sum of all the coins amounts
40+    CAmount total_amount{0};
41+};
42 /**
43- * populate vCoins with vector of available COutputs.
44+ * Return vector of available COutputs.

stickies-v commented at 8:41 pm on June 17, 2022:

nit: CoinsResult is not technically a vector of COutputs, think this could be updated a little bit?

in src/wallet/spend.cpp:802 in fd5c996d16

798@@ -791,11 +799,16 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
799     CAmount selection_target = recipients_sum + not_input_fees;
800 
801     // Get available coins
802-    std::vector<COutput> vAvailableCoins;
803-    AvailableCoins(wallet, vAvailableCoins, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0);
804+    auto res_available_coins = AvailableCoins(wallet,

stickies-v commented at 9:13 pm on June 17, 2022:

nit: I think the res_ prefix is not really necessary?

0    auto available_coins = AvailableCoins(wallet,

in src/wallet/spend.cpp:98 in fd5c996d16

 96 {
 97     AssertLockHeld(wallet.cs_wallet);
 98 
 99-    vCoins.clear();
100-    CAmount nTotal = 0;
101+    CoinsResult result;

stickies-v commented at 9:16 pm on June 17, 2022:

Ideally, result is declared as closely as possible to where it’s used.

in src/wallet/spend.cpp:811 in fd5c996d16

809+                                              MAX_MONEY,    /*nMinimumSumAmount*/
810+                                              0);           /*nMaximumCount*/
811 
812     // Choose coins to use
813-    std::optional<SelectionResult> result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
814+    std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);

stickies-v commented at 9:36 pm on June 17, 2022:

nit: since you’re already touching this, maybe you could update result to something more helpful like selected_coins? It does affect a few other lines so would understand if out of scope.

achow101 merged this on Jun 17, 2022

achow101 closed this on Jun 17, 2022

in src/wallet/interfaces.cpp:113 in fd5c996d16

109@@ -110,7 +110,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
110     result.txout = wtx.tx->vout[n];
111     result.time = wtx.GetTxTime();
112     result.depth_in_main_chain = depth;
113-    result.is_spent = wallet.IsSpent(wtx.GetHash(), n);
114+    result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n));

stickies-v commented at 11:47 pm on June 17, 2022:

I think it’s preferred to use {} list initialization - any reason not to? (Here and in quite a few other places)

0    result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n});

in src/wallet/spend.cpp:242 in fd5c996d16

266+                          std::nullopt, /*feerate=*/
267+                          1,            /*nMinimumAmount*/
268+                          MAX_MONEY,    /*nMaximumAmount*/
269+                          MAX_MONEY,    /*nMinimumSumAmount*/
270+                          0             /*nMaximumCount*/
271+                          ).total_amount;

stickies-v commented at 1:15 am on June 18, 2022:

Since we’re explicitly relying on only_spendable to be true, I think it would be nice to make the argument explicit?

stickies-v commented at 1:17 am on June 18, 2022: contributor

Post-merge approach ACK fd5c996, nice refactor!

I did have a couple of nits/style suggestions during review, none of them crucial. I’ll post them anyway since you may want to consider them in future PRs.

One thing I wonder (just out of curiosity) is why for IsSpent() and IsLockedCoin() you refactored the function signature in a single commit, whereas for IsSpentKey() you did it in three commits? I think both are done elegantly, just curious if there’s a reason behind the (apparent) inconsistency.

maflcko commented at 9:11 am on June 18, 2022: member

https://cirrus-ci.com/task/4885942589718528?logs=ci#L6616

0wallet/spend.cpp:237:41: error: argument name 'feerate' in comment does not match parameter name 'nMinimumAmount' [bugprone-argument-comment,-warnings-as-errors]
1                          std::nullopt, /*feerate=*/
2                                        ^
3./wallet/spend.h:49:43: note: 'nMinimumAmount' declared here
4                           const CAmount& nMinimumAmount = 1,
5                                          ^
6wallet/spend.cpp:87:13: note: actual callee ('AvailableCoins') is declared here
7CoinsResult AvailableCoins(const CWallet& wallet,
8            ^

maflcko referenced this in commit 8e7eeb5971 on Jun 18, 2022

sidhujag referenced this in commit 581c6a74be on Jun 19, 2022

hebasto referenced this in commit 6d4889a694 on Aug 15, 2022

achow101 referenced this in commit 139ba2bf12 on Jan 4, 2023

sidhujag referenced this in commit b23c80f51c on Jan 4, 2023

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

wallet: remove extra wtx lookup in ‘AvailableCoins’ + several code cleanups. #25005

Conflicts