rpc, cli: add getbalances#total, and use it for -getinfo #31353

1. 
   jonatack commented at 7:00 pm on November 22, 2024: member

   Add a “total” field in RPC getbalances to be able to easily see the total amount held in the wallet, and use that field for the wallet balances in CLI -getinfo.

   Currently -getinfo only returns getbalances#mine.trusted for wallet balances. It would make sense to instead return the total balance. For instance, to see:

   • watchonly balances
   • reused outputs, like coins returned to a wallet from an exchange or third party service that uses a fixed (reused) address, whether avoid_reuse is set or not, as it can be scary for a user not to see the received coins in the -getinfo summary

   We can keep getbalances#total simple and clear by returning the total of all the getbalances fields. If the -getinfo wallet balances use the total as-is, they would include immature coinbase rewards and unconfirmed transactions, which seems ok to me to see what is coming. Alternatively, depending on review feedback, -getinfo could show e.g. only trusted, used, and watchonly balances without immature coinbase rewards and unconfirmed transactions.

2. wallet: store total balance in Balance struct 19a4892871
3. 
   DrahtBot commented at 7:00 pm on November 22, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31353.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
   • #29062 (Wallet: (Refactor) GetBalance to calculate used balance by BrandonOdiwuor)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #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.

4. rpc, test: add "total" field to RPC getbalances 1b03a82cff
5. cli, test: update -getinfo to use rpc getbalances#total 6cfb73e0e3
6. jonatack force-pushed on Nov 22, 2024
7. DrahtBot added the label CI failed on Nov 22, 2024
8. DrahtBot removed the label CI failed on Nov 22, 2024
9. jonatack marked this as ready for review on Nov 23, 2024
10. test: coverage for getbalances#total field in wallet_avoidreuse e00979167a
11. doc: release notes for rpc getbalances#total and -getinfo balances update 5d0c836294
12. jonatack force-pushed on Nov 23, 2024
13. in src/wallet/rpc/coins.cpp:450 in 5d0c836294

    446@@ -447,6 +447,7 @@ RPCHelpMan getbalances()
    447                     {RPCResult::Type::STR_AMOUNT, "immature", "balance from immature coinbase outputs"},
    448                 }},
    449                 RESULT_LAST_PROCESSED_BLOCK,
    450+                {RPCResult::Type::STR_AMOUNT, "total", "total of all balances returned by this RPC"},
    

    ---

    ---

    

    maflcko commented at 2:34 pm on November 24, 2024:

    Do descriptor wallets even allow watchonly and signable in the same wallet? If not, it seems clearer to keep the nesting?

    In any case, given that this includes untrusted, it seems better to keep the untrusted in the name?

14. DrahtBot added the label CI failed on Dec 6, 2024
15. 
    maflcko commented at 1:05 pm on December 10, 2024: member

    Are you still working on this? Maybe turn into a draft while CI is failing?

Contributors
jonatack DrahtBot maflcko

Labels
CI failed