Add GetFullBalance() to compute used balance in getbalances RPC and on GUI GUI PR [#775](/bitcoin-bitcoin/775/)
Check #28776 (review) by @achow101
However, I think it would be better to move all of this into
GetBalanceitself and just have it always compute the used balance for us rather than having the caller do this extra computation.
used balance from GetBalance(...)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29062.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
302 | @@ -303,18 +303,23 @@ Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) 303 | const bool is_trusted{CachedTxIsTrusted(wallet, wtx, trusted_parents)}; 304 | const int tx_depth{wallet.GetTxDepthInMainChain(wtx)}; 305 | const CAmount tx_credit_mine{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | reuse_filter)}; 306 | + const CAmount tx_credit_mine_used{ 307 | + avoid_reuse ? CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | ISMINE_USED) : tx_credit_mine};
have it always compute the used balance for us rather than having the caller do this extra computation.
My understanding of this part of @achow101 comment was to move the calculation for the used balance to GetBalance(...)
I have updated the PR and added GetFullBalance() which adds used balance to GetBalance(...)
Concept ACK
289 | @@ -290,6 +290,15 @@ bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx) 290 | return CachedTxIsTrusted(wallet, wtx, trusted_parents); 291 | } 292 | 293 | +Balance GetFullBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) {
This doesn't need to be a separate function, just have GetBalance include m_mine_used. It's up to the caller to figure out what to do with that information.
Fixed: updated GetBalance to include m_mine_used
303 | @@ -304,17 +304,23 @@ Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) 304 | const int tx_depth{wallet.GetTxDepthInMainChain(wtx)}; 305 | const CAmount tx_credit_mine{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | reuse_filter)}; 306 | const CAmount tx_credit_watchonly{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_WATCH_ONLY | reuse_filter)}; 307 | + const CAmount tx_credit_mine_used{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | ISMINE_USED)}; 308 | if (is_trusted && tx_depth >= min_depth) { 309 | ret.m_mine_trusted += tx_credit_mine; 310 | ret.m_watchonly_trusted += tx_credit_watchonly; 311 | } 312 | + if (is_trusted) {
This needs to be in the above if so that the min_depth check is still being done.
My understanding from the above line is min_depth should be zero when calculating full_balance hence I didn't include the statement below in the above if since it uses min_depth provided by the caller
I didn't include the check in this if since tx_depth is always >= 0
I don't think it makes sense to return m_mine_used without respecting min_depth. The original code passes 0 for min_depth because that is the default value but it needs to pass the avoid_reuse flag. It is not because used balance should always calculated with a min depth of 0. Ignoring it is a layer violation here.
318 | + ret.m_mine_used += tx_credit_mine_used; 319 | } 320 | ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE); 321 | ret.m_watchonly_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_WATCH_ONLY); 322 | } 323 | + ret.m_mine_used -= (ret.m_mine_trusted + ret.m_mine_untrusted_pending);
It would be a bit easier to understand if this subtraction was done with each addition during the loop, rather than all at the end. Also a comment for why this is being done would be useful, as it was not immediately obvious to me at first that the purpose of this is to remove the overlap.
@achow101 do the added comments make the PR easy to follow?
309 | + ret.m_mine_used += tx_credit_mine_used; 310 | + } 311 | if (is_trusted && tx_depth >= min_depth) { 312 | ret.m_mine_trusted += tx_credit_mine; 313 | ret.m_watchonly_trusted += tx_credit_watchonly; 314 | + ret.m_mine_used -= tx_credit_mine; // remove overlap from used_balance
When the avoid_reuse flag is passed, tx_credit_mine and tx_credit_mine_used will be the same so the returned used_balance will be 0. That seems incorrect.
Moved to draft, given this doesn't seem to be active. @BrandonOdiwuor are you still working on this?
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.