Wallet: (Refactor) GetBalance to calculate used balance #29062

pull BrandonOdiwuor wants to merge 1 commits into bitcoin:master from BrandonOdiwuor:getbalance-add-used changing 3 files +9 −4
  1. BrandonOdiwuor commented at 2:38 pm on December 12, 2023: contributor

    Add GetFullBalance() to compute used balance in getbalances RPC and on GUI [GUI PR #775](https://github.com/bitcoin-core/gui/pull/775)

    Check #28776 (review) by @achow101

    However, I think it would be better to move all of this into GetBalance itself and just have it always compute the used balance for us rather than having the caller do this extra computation.

    Update:

    • Compute used balance from GetBalance(...)
  2. DrahtBot commented at 2:38 pm on December 12, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    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.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

    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.

  3. BrandonOdiwuor renamed this:
    Calculate used balance from GetBalance(...)
    Wallet: Calculate used balance from GetBalance(...)
    on Dec 12, 2023
  4. DrahtBot added the label Wallet on Dec 12, 2023
  5. BrandonOdiwuor marked this as ready for review on Dec 13, 2023
  6. in src/wallet/receive.cpp:307 in 19437c3a1c outdated
    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};
    


    pablomartin4btc commented at 9:12 pm on December 16, 2023:
    What about caculating avoid_reuse locally (e.g. const bool avoid = !(m_wallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)); at the beginning of the function), instead of passing it? I thought @achow101’s comment was referring to include that too but maybe I’m wrong.

    BrandonOdiwuor commented at 4:40 am on December 17, 2023:

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

  7. pablomartin4btc commented at 9:14 pm on December 16, 2023: member
    Concept ACK
  8. BrandonOdiwuor force-pushed on Dec 17, 2023
  9. BrandonOdiwuor renamed this:
    Wallet: Calculate used balance from GetBalance(...)
    Wallet: Add GetFullBalance(...) which included used balance
    on Dec 17, 2023
  10. DrahtBot added the label CI failed on Jan 16, 2024
  11. achow101 requested review from achow101 on Apr 9, 2024
  12. in src/wallet/receive.cpp:293 in 6a0272b87e outdated
    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) {
    


    achow101 commented at 6:35 pm on April 16, 2024:
    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.

    BrandonOdiwuor commented at 2:51 pm on April 22, 2024:
    Fixed: updated GetBalance to include m_mine_used
  13. BrandonOdiwuor force-pushed on Apr 22, 2024
  14. BrandonOdiwuor renamed this:
    Wallet: Add GetFullBalance(...) which included used balance
    Wallet: (Refactor) GetBalance to calculate used balance
    on Apr 22, 2024
  15. BrandonOdiwuor requested review from achow101 on Apr 22, 2024
  16. BrandonOdiwuor requested review from pablomartin4btc on Apr 22, 2024
  17. DrahtBot removed the label CI failed on Apr 23, 2024
  18. in src/wallet/receive.cpp:312 in c69dd7ba84 outdated
    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) {
    


    achow101 commented at 6:22 pm on April 25, 2024:
    This needs to be in the above if so that the min_depth check is still being done.

    BrandonOdiwuor commented at 10:21 am on April 29, 2024:

    https://github.com/bitcoin/bitcoin/blob/a46065e36cf868265c909dc5edf29dc17be53c1f/src/wallet/rpc/coins.cpp#L480

    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

  19. in src/wallet/receive.cpp:323 in c69dd7ba84 outdated
    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);
    


    achow101 commented at 6:24 pm on April 25, 2024:
    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.

    BrandonOdiwuor commented at 3:52 pm on May 11, 2024:
    @achow101 do the added comments make the PR easy to follow?
  20. wallet: (Refactor) Compute used balance from GetBalance() Function 1cc2a81e9f
  21. BrandonOdiwuor force-pushed on May 11, 2024
  22. BrandonOdiwuor requested review from achow101 on May 11, 2024

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: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me