cli: call getbalances.ismine.trusted instead of getwalletinfo.balance #18574

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:getinfo-call-getbalances-instead-of-getwalletinfo-balances changing 2 files +22 −9
  1. jonatack commented at 9:11 am on April 9, 2020: member

    Extracted from #18453 to preserve that PR as a discussion on multiwallet RPC/CLI.

    This PR updates bitcoin-cli -getinfo to fetch the wallet balance from getbalances in order to no longer depend on getwalletinfo.balance which was deprecated a year ago in facfb41.

    I found this when removing the getwalletinfo() balance, unconfirmed_balance, and immature_balance fields to see what broke from depending on them.

    I didn’t see any perceivable change in -getinfo run time from the change.

    Test coverage for this change is provided by test/functional/interface_bitcoin_cli.py, which the second commit updates to (a) no longer depend on getwalletinfo.balances and (b) test the -getinfo blockcount and balance fields against non-default, non-zero values.

  2. fanquake added the label Utils/log/libs on Apr 9, 2020
  3. jonatack commented at 9:19 am on April 9, 2020: member
    This change was ACKed in the original PR by @jonasschnelli.
  4. in src/bitcoin-cli.cpp:231 in baa0691519 outdated 
    227@@ -228,6 +228,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
228     const int ID_NETWORKINFO = 0;
229     const int ID_BLOCKCHAININFO = 1;
230     const int ID_WALLETINFO = 2;
231+    const int ID_BALANCESINFO = 3;
    vasild commented at 9:45 am on April 9, 2020:
    nit: following the convention that ID_foo maps to getfoo this should be named ID_BALANCES.
    jonatack commented at 2:29 pm on April 9, 2020:
    done
  5. in test/functional/interface_bitcoin_cli.py:18 in baa0691519 outdated 
    14+    connect_nodes,
15+    get_auth_cookie,
16+)
17+
18+BLOCKS = 101
19+BALANCE = Decimal('50.00000000')
    vasild commented at 9:54 am on April 9, 2020:

    nit: Maybe a comment is warranted how come after generating 101 blocks the node’s balance becomes 50.

    0# The block reward is 50 but it only matures after 100 (COINBASE_MATURITY) blocks,
1# so the node "gets" the reward for the first block as trusted 100 blocks later.
2BLOCKS = 101
3BALANCE = Decimal('50.00000000')
    jonatack commented at 2:29 pm on April 9, 2020:
    Done. This seems to be a standard convention in the tests, explained in test-shell.md, and used without documentation in wallet-basic.py and rpc_rawtransaction.py, among others… but I added a doc since it’s generally better to do so.
  6. vasild approved
  7. vasild commented at 9:58 am on April 9, 2020: member
    utACK baa069151971f137782e3bd998f07f9f16185637
  8. MarcoFalke renamed this:
    getinfo: call getbalances.ismine.trusted instead of getwalletinfo.balance
    cli: call getbalances.ismine.trusted instead of getwalletinfo.balance
    on Apr 9, 2020
  9. cli -getinfo: use getbalances instead of deprecated getwalletinfo balance 75019774c9
  10. jonatack force-pushed on Apr 9, 2020
  11. jonatack commented at 2:32 pm on April 9, 2020: member
    Thanks for reviewing @vasild! Can you re-ACK?
  12. jonatack force-pushed on Apr 9, 2020
  13. jonatack force-pushed on Apr 9, 2020
  14. jonatack commented at 3:30 pm on April 9, 2020: member
    Force-pushed for good catch by @vasild (thanks!)
  15. vasild commented at 3:31 pm on April 9, 2020: member

    ACK 130abeb93

    Compiles and test/functional/interface_bitcoin_cli.py passes on my end.

  16. vasild approved
  17. DrahtBot commented at 3:32 pm on April 9, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18453 (rpc, cli: add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance by jonatack)

    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.

  18. test: update and harden interface_bitcoin_cli tests 
    - no longer depend on the deprecated balance field of getwalletinfo

- test blockcount and balance against non-default, non-zero expected values
    5df0877f91
  19. jonatack force-pushed on Apr 9, 2020
  20. jonatack commented at 3:35 pm on April 9, 2020: member
    One (hopefully) last fixup :facepalm:
  21. vasild approved
  22. vasild commented at 3:44 pm on April 9, 2020: member

    re-ACK 5df0877f9

    test/lint/lint-python.sh passes locally

  23. robot-visions commented at 7:06 pm on April 9, 2020: contributor

    ACK 5df0877

    • unit tests pass
    • functional tests pass both with and without wallets compiled
    • manual test passes: no user-facing differences to -getinfo behavior for (i) wallet functionality not compiled, (ii) single wallet loaded, (iii) multiple wallets loaded

    And thanks for updating the ID_ naming!

  24. MarcoFalke commented at 7:19 pm on April 9, 2020: member
    ACK 5df0877
  25. in src/bitcoin-cli.cpp:251 in 75019774c9 outdated 
    249     {
250         UniValue result(UniValue::VOBJ);
251-        std::vector<UniValue> batch = JSONRPCProcessBatchReply(batch_in, 3);
252-        // Errors in getnetworkinfo() and getblockchaininfo() are fatal, pass them on
253-        // getwalletinfo() is allowed to fail in case there is no wallet.
254+        std::vector<UniValue> batch = JSONRPCProcessBatchReply(batch_in, batch_in.size());
    promag commented at 11:15 pm on April 9, 2020:
    Could change JSONRPCProcessBatchReply to just take batch_in (drop 2nd arg)?
    jonatack commented at 11:29 pm on April 9, 2020:
    Thanks for reviewing. Was just looking at doing that in the follow-up.
    promag commented at 11:06 am on April 10, 2020:
    TBH I’d change it now. nm this has enough acks.
  26. promag commented at 11:16 pm on April 9, 2020: member
    Code review ACK 5df0877f91f4ed59031b0d7dcd66780df87ac1af.
  27. theStack approved
  29. MarcoFalke merged this on Apr 10, 2020
  30. MarcoFalke closed this on Apr 10, 2020
  31. jonatack deleted the branch on Apr 10, 2020
  32. luke-jr referenced this in commit 6fb7c9bdd8 on Jun 9, 2020
  33. luke-jr referenced this in commit 9fa6dab94b on Jun 9, 2020
  34. luke-jr referenced this in commit 18e5132a62 on Jun 13, 2020
  35. luke-jr referenced this in commit 493b4525a8 on Jun 13, 2020
  36. luke-jr referenced this in commit 5780bc85df on Jun 14, 2020
  37. luke-jr referenced this in commit 32ffa98609 on Jun 14, 2020
  38. Fabcien referenced this in commit e5977fde21 on Jan 14, 2021
  39. PhotoshiNakamoto referenced this in commit d0bdafdc10 on Dec 11, 2021
  40. DrahtBot locked this on Feb 15, 2022


jonatack vasild DrahtBot robot-visions MarcoFalke promag theStack

Labels
Utils/log/libs

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-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me