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

pull furszy wants to merge 9 commits into bitcoin:master from furszy:2022_availableBalance_improvements changing 9 files +118 −107
  1. 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:

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

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

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

    4. The wallet IsLockedCoin and IsSpent methods now accept an OutPoint instead of a hash:index. Which let me cleanup a bunch of extra code.

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

  2. fanquake added the label Wallet on Apr 27, 2022
  3. fanquake requested review from achow101 on Apr 27, 2022
  4. DrahtBot commented at 2:00 AM on April 28, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

  7. 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
  8. furszy force-pushed on May 9, 2022
  9. furszy commented at 8:58 PM on May 9, 2022: member
    1. 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

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

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

    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:

    CCoinControl coinControl = _coinControl ? *_coinControl : CCoinControl();
coinControl.m_include_only_spendable_outputs = true; // --> this enforces that non-spendable coins are filtered from `AvailableCoins` result.
return AvailableCoins(wallet, &coinControl).total_amount;
  11. 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?

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

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


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

  13. furszy force-pushed on May 23, 2022
  14. furszy commented at 6:18 PM on May 23, 2022: member

    Rebased after #25083 merge. Conflicts solved.

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

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

                const COutPoint outpoint(wtxid, i);
  17. 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

            if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(hashTx, i))) {
  18. furszy force-pushed on Jun 2, 2022
  19. 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.
  20. furszy force-pushed on Jun 5, 2022
  21. theStack commented at 8:18 PM on June 5, 2022: contributor

    Concept ACK

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

     * Return vector of available COutputs.
  23. 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

        for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
  24. 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

        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.

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

  26. 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
  27. wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times 9472ca0a65
  28. wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) 91902b7720
  29. wallet: IsSpent, 'COutPoint' arg instead of (hash, index) a06fa94ff8
  30. 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
  31. wallet: avoid extra IsSpentKey -> GetWalletTx lookups 4b83bf8dbc
  32. wallet: remove unused IsSpentKey(hash, index) method cdf185ccfb
  33. 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
  34. wallet: GetAvailableBalance, remove double walk-through every available coin
    Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount.
    fd5c996d16
  35. furszy force-pushed on Jun 8, 2022
  36. furszy commented at 2:36 PM on June 8, 2022: member

    thanks for the feedback theStack!

    Applied the following changes:

    1. Re-wrote commits titles to be 72-80 characters long.
    2. Moved the only_spendable arg of AvailableCoins to be true by default.
    3. By having spendable_only=true by default, fixed an already existent hidden issue in sendall RPC command -> description
  37. brunoerg commented at 8:42 PM on June 15, 2022: contributor

    Concept ACK

  38. achow101 commented at 8:52 PM on June 16, 2022: member

    ACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa

  39. brunoerg approved
  40. brunoerg commented at 9:22 PM on June 16, 2022: contributor

    crACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa

  41. fanquake requested review from theStack on Jun 16, 2022
  42. fanquake requested review from murchandamus on Jun 16, 2022
  43. w0xlt approved
  44. 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.

  45. 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?

  46. 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?

        auto available_coins = AvailableCoins(wallet,
  47. 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.

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

  49. achow101 merged this on Jun 17, 2022
  50. achow101 closed this on Jun 17, 2022
  51. 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)

        result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n});
  52. 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?

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

  54. maflcko commented at 9:11 AM on June 18, 2022: member

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

    wallet/spend.cpp:237:41: error: argument name 'feerate' in comment does not match parameter name 'nMinimumAmount' [bugprone-argument-comment,-warnings-as-errors]
                          std::nullopt, /*feerate=*/
                                        ^
./wallet/spend.h:49:43: note: 'nMinimumAmount' declared here
                           const CAmount& nMinimumAmount = 1,
                                          ^
wallet/spend.cpp:87:13: note: actual callee ('AvailableCoins') is declared here
CoinsResult AvailableCoins(const CWallet& wallet,
            ^
  55. maflcko referenced this in commit 8e7eeb5971 on Jun 18, 2022
  56. sidhujag referenced this in commit 581c6a74be on Jun 19, 2022
  57. hebasto referenced this in commit 6d4889a694 on Aug 15, 2022
  58. achow101 referenced this in commit 139ba2bf12 on Jan 4, 2023
  59. sidhujag referenced this in commit b23c80f51c on Jan 4, 2023
  60. furszy deleted the branch on May 27, 2023
  61. bitcoin locked this on May 26, 2024
Contributors
furszyDrahtBotjonatackachow101theStackbrunoergw0xltstickies-vmaflcko


theStackmurchandamus

Labels
Wallet
Linked (view graph)
#24699 wallet: Improve AvailableCoins performance by reducing duplicated operations#24845 wallet: return error msg for "too-long-mempool-chain"#25218 refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination#25234 bench: add benchmark for wallet 'AvailableCoins' function.#25410 wallet: fix warning: "argument name 'feerate' in comment does not match parameter name"
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-16 00:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me