-
Notifications
You must be signed in to change notification settings - Fork 38.7k
wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
@furszy It looks like the qt wallet test is failing in the CI. You can run |
|
Ah, thanks @jonatack 👌. Just noticed that the GUI 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. |
f2c1694 to
2cdd2a1
Compare
Dropped the last two commits that were implementing point (6) described above. They are now part of bitcoin-core/gui#598 |
achow101
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the purpose of the first commit. It doesn't seem like it materially improves anything?
src/wallet/spend.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;|
Hey achow101, thanks for the review!
We are skipping the non-spendable coins that appear in
So, the purpose is not add the non-spendable coins into the Note: this speedup is for all the processes that calls to |
2cdd2a1 to
5b6124d
Compare
|
Rebased after #25083 merge. Conflicts solved. |
src/wallet/rpc/spend.cpp
Outdated
| coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); | ||
| } | ||
|
|
||
| coin_control.m_include_only_spendable_outputs = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 3082f22 "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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 , added it in 8daa58b.
5b6124d to
46ac311
Compare
|
Ready, feedback applied:
|
46ac311 to
5646ccf
Compare
|
Concept ACK |
theStack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/wallet/spend.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, | |
| std::nullopt, /*feerate=*/ | |
| 1, /*nMinimumAmount*/ | |
| MAX_MONEY, /*nMaximumAmount*/ | |
| MAX_MONEY, /*nMinimumSumAmount*/ | |
| 0, /*nMaximumCount*/ | |
| true /*spendable_only*/ | |
| ).total_amount; | |
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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.
…le coin Filtering `AvailableCoins` by spendable outputs only and using the retrieved total_amount.
5646ccf to
fd5c996
Compare
|
thanks for the feedback theStack! Applied the following changes:
|
|
Concept ACK |
|
ACK fd5c996 |
brunoerg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK fd5c996
w0xlt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK fd5c996
Clear improvement in code readability and Available Coins() logic.
stickies-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| }; | ||
| /** | ||
| * populate vCoins with vector of available COutputs. | ||
| * Return vector of available COutputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: CoinsResult is not technically a vector of COutputs, think this could be updated a little bit?
| // Get available coins | ||
| std::vector<COutput> vAvailableCoins; | ||
| AvailableCoins(wallet, vAvailableCoins, &coin_control, coin_selection_params.m_effective_feerate, 1, MAX_MONEY, MAX_MONEY, 0); | ||
| auto res_available_coins = AvailableCoins(wallet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the res_ prefix is not really necessary?
| auto res_available_coins = AvailableCoins(wallet, | |
| auto available_coins = AvailableCoins(wallet, |
|
|
||
| vCoins.clear(); | ||
| CAmount nTotal = 0; | ||
| CoinsResult result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, result is declared as closely as possible to where it's used.
|
|
||
| // Choose coins to use | ||
| std::optional<SelectionResult> result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); | ||
| std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| result.time = wtx.GetTxTime(); | ||
| result.depth_in_main_chain = depth; | ||
| result.is_spent = wallet.IsSpent(wtx.GetHash(), n); | ||
| result.is_spent = wallet.IsSpent(COutPoint(wtx.GetHash(), n)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)); | |
| result.is_spent = wallet.IsSpent(COutPoint{wtx.GetHash(), n}); |
| MAX_MONEY, /*nMaximumAmount*/ | ||
| MAX_MONEY, /*nMinimumSumAmount*/ | ||
| 0 /*nMaximumCount*/ | ||
| ).total_amount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're explicitly relying on only_spendable to be true, I think it would be nice to make the argument explicit?
|
…rate' in comment does not match parameter name" 7ca8726 wallet: fix warning: "argument name 'feerate' in comment does not match parameter name" (furszy) Pull request description: Should solve the tiny bitcoin/bitcoin#25005 (comment). Which merely happens for the extra "=" character after the comma. ACKs for top commit: Empact: Code Review ACK bitcoin/bitcoin@7ca8726 Tree-SHA512: e5368c1114f715bd93cb653c607fd0942ab0b79f709ed7aa627b3fc7e7efd096c92c5c86908c7f26c363b21e391a8faa812727eb32c285e54da3ce0429290361
4584d30 GUI: remove now unneeded 'm_balances' field from overviewpage (furszy) 050e8b1 GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually (furszy) 96e3264 GUI: use cached balance in overviewpage and sendcoinsdialog (furszy) 321335b GUI: add getter for WalletModel::m_cached_balances field (furszy) e62958d GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call (furszy) Pull request description: As per the title says, we are recalculating the entire wallet balance on different situations calling to `wallet().getBalances()`, when should instead make use of the wallet model cached balance. This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed. Changes: 1) Fix: `SendCoinsDialog` was calling `wallet().getBalances()` twice during `setModel`. 2) Use the cached balance if the user did not select any UTXO manually inside the wallet model `getAvailableBalance` call. ----------------------- As an extra note, this work born in [#25005](bitcoin/bitcoin#25005) but grew out of scope of it. ACKs for top commit: jarolrod: ACK 4584d30 hebasto: re-ACK 4584d30, only suggested changes and commit message formatting since my [recent](#598 (review)) review. Tree-SHA512: 6633ce7f9a82a3e46e75aa7295df46c80a4cd4a9f3305427af203c9bc8670573fa8a1927f14a279260c488cc975a08d238faba2e9751588086fea1dcf8ea2b28
3a4f8bc bench: add benchmark for wallet 'AvailableCoins' function. (furszy) Pull request description: #### Rationale `AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog. As we are improving this process in #24699, #25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes). #### Implementation Notes There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types. The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits. ACKs for top commit: achow101: ACK 3a4f8bc hernanmarino: ACK 3a4f8bc aureleoules: ACK 3a4f8bc Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b
… function. 3a4f8bc bench: add benchmark for wallet 'AvailableCoins' function. (furszy) Pull request description: #### Rationale `AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog. As we are improving this process in bitcoin#24699, bitcoin#25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes). #### Implementation Notes There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types. The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits. ACKs for top commit: achow101: ACK 3a4f8bc hernanmarino: ACK 3a4f8bc aureleoules: ACK 3a4f8bc Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b
This started in #24845 but grew out of scope of it.
So, points tackled:
Avoid extra
GetWalletTxlookups insideAvailableCoins -> IsSpentKey.IsSpentKeywas receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all theIsSpentKeyfunction callers already have the wtx available, them can provide thescriptPubKeydirectly.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 insideWallet::AvailableCoinsdirectly.(the non-spendable coins skip examples are inside
AttemptSelection->GroupOutputsandGetAvailableBalance).Refactored
AvailableCoinsin several ways:a) Now it will return a new struct
CoinsResultinstead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from wallet: return error msg for "too-long-mempool-chain" #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 wallet: Improve AvailableCoins performance by reducing duplicated operations #24699 (comment)).
The wallet
IsLockedCoinandIsSpentmethods now accept anOutPointinstead of a hash:index. Which let me cleanup a bunch of extra code.Speeded up the wallet 'GetAvailableBalance': filtering
AvailableCoinsby spendable outputs only and using the 'AvailableCoins' retrievedtotal_amountinstead of looping over all the retrieved coins once more.Side topic, all this process will look even nicer with #25218