cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089

pull jonatack wants to merge 7 commits into bitcoin:master from jonatack:cli-getinfo-multiwallet-follow-ups changing 4 files +50 −7
  1. jonatack commented at 11:59 AM on May 28, 2020: contributor

    These are follow-ups to #18594:

    • release note for -getinfo multiwallet balances

    • update the -getinfo help:

    $ bitcoin-cli -h | grep -A3 getinfo
  -getinfo
       Get general information from the remote server, including the balances
       of each loaded wallet when in multiwallet mode. Note that
       -getinfo is the combined result of several RPCs (getnetworkinfo,
       getblockchaininfo, getwalletinfo, getbalances, and in multiwallet
       mode, listwallets), each with potentially different state.

    • more robust bitcoin-cli -getinfo command parsing per review feedback in #18594 (review) and regression test coverage for that change

    • add assert_scale to test_framework/util.py and improve the coverage in interface_bitcoin_cli.py

  2. jonatack cross-referenced this on May 28, 2020 from issue cli: display multiwallet balances in -getinfo by jonatack
  3. in doc/release-notes-18594.md:5 in daab3ced95 outdated 
       0 | @@ -0,0 +1,5 @@
   1 | +## CLI
   2 | +
   3 | +The `bitcoin-cli -getinfo` command now displays the wallet name and balance for
   4 | +each of the loaded wallets when more than one is loaded (e.g. in multiwallet
   5 | +mode) and a wallet is not specified with `-rpcwallet`.
    MarcoFalke commented at 12:10 PM on May 28, 2020: 
    mode) and a wallet is not specified with `-rpcwallet`. (#18594)
    jonatack commented at 12:40 PM on May 28, 2020:

    thanks, done

  4. fanquake added the label Tests on May 28, 2020
  5. fanquake added the label Utils/log/libs on May 28, 2020
  6. jonatack force-pushed on May 28, 2020
  7. in src/bitcoin-cli.cpp:547 in 8ab19a3d44 outdated 
     543 | @@ -544,7 +544,9 @@ static int CommandLineRPC(int argc, char *argv[])
 544 |              args.erase(args.begin()); // Remove trailing method name from arguments vector
 545 |          }
 546 |          Optional<std::string> wallet_name{};
 547 | -        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");
 548 | +        if (!gArgs.GetArgs("-rpcwallet").empty()) {
    luke-jr commented at 1:21 PM on May 28, 2020:

    What is this supposed to improve?

    jonatack commented at 2:42 PM on May 28, 2020:

    This is how -rpcwallet was parsed before (see diff in 7430775). I don't think it changes anything, but I mistakenly thought the same with respect to -getinfo before your feedback in that PR.

    jonatack commented at 2:44 PM on May 28, 2020:

    That said, I added -rpcwallet test coverage recently in #18724, so I think either way works.

    jonatack commented at 9:58 AM on June 5, 2020:

    Reverted to minimal change.

  8. DrahtBot cross-referenced this on May 28, 2020 from issue cli: display multiwallet total balance in -getinfo by jonatack
  9. DrahtBot commented at 9:16 PM on May 28, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19092 (cli: display multiwallet total balance in -getinfo 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.

  10. DrahtBot cross-referenced this on May 29, 2020 from issue cli/gui: support "@height" in place of blockhash for getblock on client side by ajtowns
  11. DrahtBot cross-referenced this on Jun 2, 2020 from issue rpc, cli, test: add bitcoin-cli -generate command by jonatack
  12. jonatack force-pushed on Jun 5, 2020
  13. hebasto commented at 12:08 PM on June 5, 2020: member

    Concept ACK.

  14. jonatack force-pushed on Jun 5, 2020
  15. hebasto commented at 4:31 PM on June 5, 2020: member

    Approach ACK 8019cf2dc7c4cf59ece71a6e7143b3f054a2243e. @jonatack Could you consider to replace s/ArgsManager::ALLOW_ANY/ArgsManager::ALLOW_BOOL/ for -getinfo command-line argument?

    There is a general direction to move from the ArgsManager::ALLOW_ANY flag to more specific ones:

    • 7414d3820c833566b4f48c6c120a18bf53978c55 (-rpcwhitelistdefault)
    • #16545

    If you decide to implement this suggestion, the first commit and the last one could be combined.

  16. jonatack commented at 5:52 PM on June 5, 2020: contributor

    Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

    • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault
    • the change would have no effect on -getinfo command parsing behavior and tests
    • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.
  17. hebasto commented at 5:59 PM on June 5, 2020: member

    Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

    • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault

    • the change would have no effect on -getinfo command parsing behavior and tests

    • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.

    FWIW, ArgsManager::ALLOW_BOOL is designed to use along with ArgsManager::GetBoolArg(). Additional error checking has not been implemented yet, unfortunately.

    In any case, will continue my review :)

  18. in test/functional/test_framework/util.py:38 in 2590e8a24e outdated 
      31 | @@ -32,6 +32,12 @@ def assert_approx(v, vexp, vspan=0.00001):
  32 |      if v > vexp + vspan:
  33 |          raise AssertionError("%s > [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
  34 |  
  35 | +def assert_decimal_places(amount, decimal_places=8):
  36 | +    """Assert an amount has n digits (default 8 for amounts) after the decimal."""
  37 | +    remainder = str(amount).split('.')[-1]
  38 | +    if remainder != '0E-8':
    hebasto commented at 4:44 AM on June 6, 2020:

    What cases are covered by this condition?

    jonatack commented at 7:48 AM on June 6, 2020:

    The condition is for the case that there's no need to run the assert if the remainder string is the Python shorthand for the correct number of zero digits. But good catch: I should pass the param into it: s/'0E-8'/'0E-{}'.format(decimal_places)'

    jonatack commented at 9:15 AM on June 6, 2020:

    Fixed and improved. Thanks @hebasto.

  19. jonatack force-pushed on Jun 6, 2020
  20. jonatack force-pushed on Jun 6, 2020
  21. in test/functional/test_framework/util.py:187 in 46bccb6cad outdated 
     179 | @@ -180,6 +180,16 @@ def assert_array_result(object_array, to_match, expected, should_not_find=False)
 180 |      if num_matched > 0 and should_not_find:
 181 |          raise AssertionError("Objects were found %s" % (str(to_match)))
 182 |  
 183 | +def assert_scale(number, digits=8):
 184 | +    """Assert number has expected scale/fractional digits/number of digits after the decimal (default 8)."""
 185 | +    scale = str(number).split('.')[-1]
 186 | +    if scale == number:  # return early if number has no fractional part
 187 | +        return
    hebasto commented at 12:53 PM on June 6, 2020:

    If a number has no fractional part, it should have digits zeros after the decimal, no?

    jonatack commented at 3:05 PM on June 6, 2020:

    Good idea, done (thanks!)

  22. hebasto commented at 12:53 PM on June 6, 2020: member

    Thanks for a separate commit that introduces the assert_scale() finction.

  23. jonatack force-pushed on Jun 6, 2020
  24. jonatack force-pushed on Jun 6, 2020
  25. in test/functional/test_framework/util.py:189 in 9785b108e5 outdated 
     184 | +    """Assert number has expected scale, e.g. fractional digits; number of
 185 | +    digits after the decimal. The default of 8 corresponds to a Bitcoin amount."""
 186 | +    number = str(number)
 187 | +    mantissa = number.split('.')[-1]
 188 | +    if mantissa[:3] == '0E-':
 189 | +        assert_equal(mantissa, '0E-{}'.format(expected_scale))  # exponent notation
    hebasto commented at 4:04 PM on June 6, 2020:

    I'm not a python expert, but my python 3.6.9 prints numbers in scientific notation using "e" rather "E". Btw, in what cases we could expect values with scientific notation in RPC responses?

    jonatack commented at 4:15 PM on June 6, 2020:

    In every run of interface_bitcoin_cli.py locally, the tests see zero values like 'paytxfee': Decimal('0E-8') in the -getinfo response. I see them as well if I add logging like self.log.info(cli_get_info) in the test. So, the assert needed to be able to handle that case. It passes in the CI, so not sure it needs to deal with lowercase 'e', but I suppose the value could be uppercased to be certain.

    jonatack commented at 4:17 PM on June 6, 2020:

    Example:

    2020-06-06T16:11:48.077000Z TestFramework (INFO): Test -getinfo returns expected network and blockchain info
{
 'balance': Decimal('50.00000000'),
 'blocks': 101,
 'chain': 'regtest',
 'connections': 0,
 'difficulty': Decimal('4.656542373906925E-10'),
 'headers': 101,
 'keypoolsize': 1,
 'paytxfee': Decimal('0E-8'),
 'proxy': '',
 'relayfee': Decimal('0.00001000'),
 'timeoffset': 0,
 'unlocked_until': 0,
 'verificationprogress': 1,
 'version': 209900
}
    jonatack commented at 4:39 PM on June 6, 2020:

    Added upper() just in case.

  26. jonatack force-pushed on Jun 6, 2020
  27. luke-jr referenced this in commit 43b301e08f on Jun 9, 2020
  28. luke-jr referenced this in commit 4a0f26c8cd on Jun 9, 2020
  29. luke-jr referenced this in commit 6bfd275ccd on Jun 9, 2020
  30. luke-jr referenced this in commit 53616a0790 on Jun 9, 2020
  31. luke-jr referenced this in commit 91d73f706b on Jun 9, 2020
  32. luke-jr referenced this in commit 2be8ee8f42 on Jun 13, 2020
  33. luke-jr referenced this in commit 4012891950 on Jun 13, 2020
  34. luke-jr referenced this in commit 4b5091fe83 on Jun 13, 2020
  35. luke-jr referenced this in commit b22c2bb2ce on Jun 13, 2020
  36. luke-jr referenced this in commit 158e0cbcbb on Jun 13, 2020
  37. luke-jr referenced this in commit f5c68f78e2 on Jun 14, 2020
  38. luke-jr referenced this in commit 49f6471546 on Jun 14, 2020
  39. luke-jr referenced this in commit 156416eecc on Jun 14, 2020
  40. luke-jr referenced this in commit 4edb3b332a on Jun 14, 2020
  41. luke-jr referenced this in commit 8db280a4ff on Jun 14, 2020
  42. DrahtBot added the label Needs rebase on Jun 16, 2020
  43. jonatack force-pushed on Jun 17, 2020
  44. DrahtBot removed the label Needs rebase on Jun 17, 2020
  45. DrahtBot added the label Needs rebase on Jun 21, 2020
  46. jonatack cross-referenced this on Jun 21, 2020 from issue rpc: remove deprecated CRPCCommand constructor by MarcoFalke
  47. cli: more robust bitcoin-cli -getinfo command parsing
    per Luke Dashjr review feedback in PR 18594
    06681102ad
  48. test: add assertion, rm unneeded call in interface_bitcoin_cli.py 76d7af3d80
  49. test: add -getinfo command parsing regression tests
    These tests fail without the changes in the first commit of this PR.
    06f068dac0
  50. test: add `assert_scale` assertion to test framework 9fa67eecd2
  51. test: add coverage for scale in -getinfo amount values ba336208f1
  52. doc: add release note for -getinfo displaying multiwallet balances 5cb0cb1707
  53. cli: update bitcoin-cli -getinfo help 865d2c32d5
  54. jonatack force-pushed on Jun 21, 2020
  55. DrahtBot removed the label Needs rebase on Jun 21, 2020
  56. jonatack cross-referenced this on Jun 22, 2020 from issue doc: add release note for -getinfo displaying multiwallet balances by jonatack
  57. jonatack commented at 5:40 PM on June 22, 2020: contributor

    Closing, will propose the changes one by one.

  58. jonatack closed this on Jun 22, 2020
  59. MarcoFalke referenced this in commit d342a45ca7 on Jun 27, 2020
  60. luke-jr referenced this in commit e203242b10 on Dec 9, 2020
  61. luke-jr referenced this in commit 4de07b3ec5 on Dec 9, 2020
  62. luke-jr referenced this in commit 1da782b0fa on Dec 9, 2020
  63. luke-jr referenced this in commit 27104bb324 on Dec 9, 2020
  64. luke-jr referenced this in commit 483d014386 on Dec 9, 2020
  65. PastaPastaPasta referenced this in commit 688eab9354 on Jun 27, 2021
  66. PastaPastaPasta referenced this in commit 318011721f on Jun 28, 2021
  67. PastaPastaPasta referenced this in commit c46412ae2c on Jun 29, 2021
  68. PastaPastaPasta referenced this in commit 0175e1e613 on Jul 1, 2021
  69. PastaPastaPasta referenced this in commit cece4977cd on Jul 1, 2021
  70. PastaPastaPasta referenced this in commit da41cd4a77 on Jul 15, 2021
  71. bitcoin locked this on Feb 15, 2022
Contributors
jonatackMarcoFalkeluke-jrDrahtBothebasto
Labels
TestsUtils/log/libs
Linked (view graph)
#18531 rpc: remove deprecated CRPCCommand constructor#18594 cli: display multiwallet balances in -getinfo#18724 test: add coverage for -rpcwallet cli option#19354 doc: add release note for -getinfo displaying multiwallet balances
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: 2026-05-20 06:54 UTC