Wallet: (Refactor) GetBalance to calculate used balance #29062
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29062. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
src/wallet/receive.cpp
Outdated
| @@ -303,18 +303,23 @@ Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) | |||
| const bool is_trusted{CachedTxIsTrusted(wallet, wtx, trusted_parents)}; | |||
| const int tx_depth{wallet.GetTxDepthInMainChain(wtx)}; | |||
| const CAmount tx_credit_mine{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | reuse_filter)}; | |||
| const CAmount tx_credit_mine_used{ | |||
| avoid_reuse ? CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | ISMINE_USED) : tx_credit_mine}; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
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(...)
19437c3 to
6a0272b
Compare
6a0272b to
c69dd7b
Compare
src/wallet/receive.cpp
Outdated
| if (is_trusted && tx_depth >= min_depth) { | ||
| ret.m_mine_trusted += tx_credit_mine; | ||
| ret.m_watchonly_trusted += tx_credit_watchonly; | ||
| } | ||
| if (is_trusted) { |
There was a problem hiding this comment.
This needs to be in the above if so that the min_depth check is still being done.
There was a problem hiding this comment.
bitcoin/src/wallet/rpc/coins.cpp
Line 480 in a46065e
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
There was a problem hiding this comment.
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.
c69dd7b to
1cc2a81
Compare
| if (is_trusted && tx_depth >= min_depth) { | ||
| ret.m_mine_trusted += tx_credit_mine; | ||
| ret.m_watchonly_trusted += tx_credit_watchonly; | ||
| ret.m_mine_used -= tx_credit_mine; // remove overlap from used_balance |
There was a problem hiding this comment.
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? |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Add
GetFullBalance()to computeusedbalance ingetbalances RPCand on GUI GUI PR #775Check #28776 (comment) by @achow101
Update:
usedbalance fromGetBalance(...)