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

jonatack commented at 11:59 AM on May 28, 2020: member

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

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

fanquake added the label Tests on May 28, 2020

fanquake added the label Utils/log/libs on May 28, 2020

jonatack force-pushed on May 28, 2020

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.

DrahtBot commented at 9:16 PM on May 28, 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:

#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.

jonatack force-pushed on Jun 5, 2020

hebasto commented at 12:08 PM on June 5, 2020: member

Concept ACK.

jonatack force-pushed on Jun 5, 2020

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.

jonatack commented at 5:52 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.

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 :)

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.

jonatack force-pushed on Jun 6, 2020

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!)

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

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

jonatack force-pushed on Jun 6, 2020

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.

jonatack force-pushed on Jun 6, 2020

luke-jr referenced this in commit 43b301e08f on Jun 9, 2020

luke-jr referenced this in commit 4a0f26c8cd on Jun 9, 2020

luke-jr referenced this in commit 6bfd275ccd on Jun 9, 2020

luke-jr referenced this in commit 53616a0790 on Jun 9, 2020

luke-jr referenced this in commit 91d73f706b on Jun 9, 2020

luke-jr referenced this in commit 2be8ee8f42 on Jun 13, 2020

luke-jr referenced this in commit 4012891950 on Jun 13, 2020

luke-jr referenced this in commit 4b5091fe83 on Jun 13, 2020

luke-jr referenced this in commit b22c2bb2ce on Jun 13, 2020

luke-jr referenced this in commit 158e0cbcbb on Jun 13, 2020

luke-jr referenced this in commit f5c68f78e2 on Jun 14, 2020

luke-jr referenced this in commit 49f6471546 on Jun 14, 2020

luke-jr referenced this in commit 156416eecc on Jun 14, 2020

luke-jr referenced this in commit 4edb3b332a on Jun 14, 2020

luke-jr referenced this in commit 8db280a4ff on Jun 14, 2020

DrahtBot added the label Needs rebase on Jun 16, 2020

jonatack force-pushed on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jun 21, 2020

cli: more robust bitcoin-cli -getinfo command parsing

per Luke Dashjr review feedback in PR 18594

06681102ad

test: add assertion, rm unneeded call in interface_bitcoin_cli.py 76d7af3d80

test: add -getinfo command parsing regression tests

These tests fail without the changes in the first commit of this PR.

06f068dac0

test: add `assert_scale` assertion to test framework 9fa67eecd2

test: add coverage for scale in -getinfo amount values ba336208f1

doc: add release note for -getinfo displaying multiwallet balances 5cb0cb1707

cli: update bitcoin-cli -getinfo help 865d2c32d5

jonatack force-pushed on Jun 21, 2020

DrahtBot removed the label Needs rebase on Jun 21, 2020

jonatack commented at 5:40 PM on June 22, 2020: member

Closing, will propose the changes one by one.

jonatack closed this on Jun 22, 2020

MarcoFalke referenced this in commit d342a45ca7 on Jun 27, 2020

luke-jr referenced this in commit e203242b10 on Dec 9, 2020

luke-jr referenced this in commit 4de07b3ec5 on Dec 9, 2020

luke-jr referenced this in commit 1da782b0fa on Dec 9, 2020

luke-jr referenced this in commit 27104bb324 on Dec 9, 2020

luke-jr referenced this in commit 483d014386 on Dec 9, 2020

PastaPastaPasta referenced this in commit 688eab9354 on Jun 27, 2021

PastaPastaPasta referenced this in commit 318011721f on Jun 28, 2021

PastaPastaPasta referenced this in commit c46412ae2c on Jun 29, 2021

PastaPastaPasta referenced this in commit 0175e1e613 on Jul 1, 2021

PastaPastaPasta referenced this in commit cece4977cd on Jul 1, 2021

PastaPastaPasta referenced this in commit da41cd4a77 on Jul 15, 2021

DrahtBot locked this on Feb 15, 2022