/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Handle getinfo in bitcoin-cli w/ -getinfo (revival of #8843) #10871

Merged
merged 2 commits into from Sep 28, 2017

Conversation

Reviewers

@laanwj laanwj

@TheBlueMatt TheBlueMatt

@jnewbery jnewbery

Assignees
No one assigned
Labels
Scripts and tools
Projects
None yet
Milestone
No milestone
6 participants
@achow101 @jonasschnelli @laanwj @jnewbery @TheBlueMatt @QingjingJing
@achow101
Member

achow101 commented Jul 18, 2017

Since @laanwj doesn't want to maintain these changes anymore, I will.

This PR is a revival of #8843. I have addressed @jnewbery's comments.

Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic.

@jonasschnelli jonasschnelli added the Scripts and tools label Jul 19, 2017

@jonasschnelli
Member

jonasschnelli commented Jul 19, 2017

Concept ACK.
Can you add a tool test for that?
@laanwj
Owner

laanwj commented Jul 20, 2017

Can you add a tool test for that?

A simple tool-test like -tx's wont work here as it requires a running bitcoind.
Better to wait for bitcoin-cli testing infrastructure to be in place I guess (#10798)?
@achow101
Member

achow101 commented Aug 15, 2017

rebased
@jnewbery
Member

jnewbery commented Aug 15, 2017

bad rebase:

bitcoin-cli.cpp: In function ‘int CommandLineRPC(int, char**)’:
bitcoin-cli.cpp:393:41: error: ‘GetBoolArg’ was not declared in this scope
         if (GetBoolArg("-getinfo", false)) {
                                         ^

#10798 is now rebased and ready for merge. It should be fairly straightforward to rebase this on top of that to add a functional test.
@jnewbery
Member

jnewbery commented Aug 23, 2017

Test here if you want it: https://github.com/jnewbery/bitcoin/tree/pr10871_test

Builds on top of #10798. Test currently fails because the output for bitcoin-cli getinfo and bitcoin-cli -getinfo is different in keypoolsize and unlocked_until. See #8843 (comment) for information about keypoolsize discrepancy.
@TheBlueMatt
Contributor

TheBlueMatt commented Aug 23, 2017

I think the docs should at least mention something about atomicity. Every other API to Bitcoin Core is atomic, so it feels very strange to add a new one which is not.
@laanwj
Owner

laanwj commented Aug 23, 2017

I think the docs should at least mention something about atomicity. Every other API to Bitcoin Core is atomic, so it feels very strange to add a new one which is not.

I know you don't like this particular changej, but this is reaching FUD levels here, what exactly are you concerned about happening here? -getinfo pulls the following information:

getnetworkinfo:
"version"
"protocolversion"
"timeoffset"
"connections"
"proxy"
"relayfee"
"errors"

getblockchaininfo:
"blocks"
"difficulty"
"testnet"

getwalletinfo:
"walletversion"
"balance"
"keypoololdest"
"keypoolsize"
"unlocked_until"
"paytxfee"

Within a group, the values are as atomic as ever. Some of these don't change at runtime at all, some hardly change.

Can you indicate exactly between which two you're afraid non-atomicity is going to give a problem, in practice?

If not, let's please leave this behind us.
@TheBlueMatt
Contributor

TheBlueMatt commented Aug 24, 2017

My only real atomicity concern is between blocks and balance - ie that you may call getinfo, see a balance and assume that it is current as of a given block height. It may be of lesser concern in just bitcoin-cli, as its not intended to be used in scripts, hence my note that I'd be ok with just documenting this behavior, though I could still see someone fucking it up in some way.
@laanwj
Owner

laanwj commented Aug 24, 2017

My only real atomicity concern is between blocks and balance - ie that you may call getinfo, see a balance and assume that it is current as of a given block height.

Yes, that's a valid concern. Also outside this PR - there is currently no way to query up to which height the wallet has processed. Especially when the wallet and node are decoupled further w/ threads or even processes. We should add a curheight or such to getwalletinfo.
@jnewbery
Member

jnewbery commented Aug 24, 2017

there is currently no way to query up to which height the wallet has processed

I believe that getinfo itself could be misleading here. balance could be for a different height than blocks.

There's been discussion of adding the wallet's best block to getwalletinfo on IRC: https://botbot.me/freenode/bitcoin-core-dev/2017-08-11/?msg=89723092&page=2 and I have a branch to do that, which I'm going to PR after #10286

@jnewbery jnewbery referenced this pull request Aug 25, 2017

Merged

Add bitcoin-cli -stdin and -stdinrpcpass functional tests #11125

@jnewbery
Member

jnewbery commented Sep 1, 2017

Is this still being actively worked on? Next steps would be:
@achow101
Member

achow101 commented Sep 1, 2017

I am still working on this, just got sidetracked with other stuff.
@achow101
Member

achow101 commented Sep 5, 2017

Took the test and rebased.

fix the discrepencies between getinfo and -getinfo

What discrepancies?
@jnewbery
Member

jnewbery commented Sep 5, 2017

What discrepancies?

See above:

Test currently fails because the output for bitcoin-cli getinfo and bitcoin-cli -getinfo is different in keypoolsize and unlocked_until. See #8843 (comment) for information about keypoolsize discrepancy.

Once #10838 is merged, then that's no longer an issue.

After #10838, I think the way to test this would be to collect the responses from getwalletinfo, getblockchaininfo and getnetworkinfo, merge those dictionaries (hint: https://stackoverflow.com/a/26853961) and then compare that to the response from bitcoin-cli -getinfo.
@achow101
Member

achow101 commented Sep 5, 2017

After #10838, I think the way to test this would be to collect the responses from getwalletinfo, getblockchaininfo and getnetworkinfo, merge those dictionaries (hint: https://stackoverflow.com/a/26853961) and then compare that to the response from bitcoin-cli -getinfo.

I think that we should be doing that instead of comparing against getinfo. This is explicitly a new thing which is similar to getinfo but not exactly replicating it.
@achow101
Member

achow101 commented Sep 5, 2017

I modified the test to compare against the get*info RPCs instead of getinfo
@jnewbery

jnewbery reviewed Sep 5, 2017
View changes

one nit inline.

Please tidy up commits. First two commits should be squashed, and final two commits should be squashed into a second commit.

src/rpc/protocol.cpp
+ std::vector<UniValue> batch(num);
+ for (size_t i=0; i<in.size(); ++i) {
+ const UniValue &rec = in[i];
+ if (!rec.isObject())
@jnewbery

jnewbery Sep 5, 2017

Member

please also add braces to these if blocks

@achow101

achow101 Sep 6, 2017

Member

Done

@achow101
Member

achow101 commented Sep 6, 2017

Tidied up commits
src/bitcoin-cli.cpp
+ }
+};
+
+UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args)
@jnewbery

jnewbery Sep 6, 2017

Member

You've lost the static keyword for this function in your rebase.

@jnewbery

jnewbery reviewed Sep 6, 2017
View changes

Some comments in line.

You seem to have lost the test in your commit squashing.

src/bitcoin-cli.cpp
+ const int ID_WALLETINFO = 2;
+
+ /** Create a simulated `getinfo` request. */
+ UniValue PrepareRequest(const std::string& strMethod, const std::vector<std::string>& args) override
@jnewbery

jnewbery Sep 6, 2017

Member

nit: function arguments should not be hungarian/camel case strMethod -> method

@jnewbery

jnewbery Sep 22, 2017

Member

nit: function arguments should not be hungarian/camel case strMethod -> method

not currently addressed

@achow101

achow101 Sep 26, 2017

Member

Done

src/bitcoin-cli.cpp
+ result.pushKV("balance", batch[ID_WALLETINFO]["result"]["balance"]);
+ result.pushKV("keypoololdest", batch[ID_WALLETINFO]["result"]["keypoololdest"]);
+ result.pushKV("keypoolsize", batch[ID_WALLETINFO]["result"]["keypoolsize"]);
+ if (!batch[ID_WALLETINFO]["result"]["unlocked_until"].isNull())
@jnewbery

jnewbery Sep 6, 2017

Member

nit: braces

src/bitcoin-cli.cpp
+ if (gArgs.GetBoolArg("-getinfo", false)) {
+ rh.reset(new GetinfoRequestHandler());
+ args.clear();
+ args.push_back("getinfo");
@jnewbery

jnewbery Sep 6, 2017

Member

I think it would be clearer to not reset args here. The only reason you're doing this is to pass the if (args.size() < 1) check below and then extract strMethod.

Instead, just move the if (args.size() < 1) check into the else branch. GetinfoRequestHandler.PrepareRequest() doesn't require strMethod or args.

Taking this further, you could add a method to DefaultRequestHandler to update the strMethod and args, and then update the PrepareRequest() method to not take any arguments.

@achow101
Member

achow101 commented Sep 18, 2017

Fixed the test failure.

@laanwj laanwj added this to Blockers in High-priority for review Sep 21, 2017

@jnewbery jnewbery referenced this pull request Sep 21, 2017

Closed

Why is getinfo being removed? #11382

@jnewbery

jnewbery reviewed Sep 22, 2017
View changes

Looks really good. Just a few nitty comments inline

src/bitcoin-cli.cpp
+class BaseRequestHandler
+{
+public:
+ virtual UniValue PrepareRequest(const std::string& strMethod, const std::vector<std::string>& args) = 0;
@jnewbery

jnewbery Sep 22, 2017

Member

nit strMethod -> method

@achow101

achow101 Sep 26, 2017

Member

Done

src/bitcoin-cli.cpp
+ const int ID_WALLETINFO = 2;
+
+ /** Create a simulated `getinfo` request. */
+ UniValue PrepareRequest(const std::string& strMethod, const std::vector<std::string>& args) override
@jnewbery

jnewbery Sep 22, 2017

Member

nit: function arguments should not be hungarian/camel case strMethod -> method

not currently addressed

test/functional/bitcoin_cli.py
+ assert_equal(cli_get_info['paytxfee'], wallet_info['paytxfee'])
+ assert_equal(cli_get_info['relayfee'], network_info['relayfee'])
+
+ if 'unlocked_until' in wallet_info:
@jnewbery

jnewbery Sep 22, 2017

Member

We know that the wallet isn't locked, so this won't be tested. I don't think there's any benefit in adding a test code that won't be executed (in fact it's slightly deleterious since a casual reader might think that we're actually testing this).

Either:

  • lock the wallet and actually test this
  • add a comment saying we're not testing the field because the wallet isn't locked.

I think either is fine.

@achow101

achow101 Sep 26, 2017

Member

Done

@achow101
Member

achow101 commented Sep 26, 2017

Addressed nits (sorry for the delay).
@jnewbery
Member

jnewbery commented Sep 26, 2017

Tested ACK 5ca97e5.

Thanks for sticking with this!
src/bitcoin-cli.cpp
@@ -37,6 +37,7 @@ std::string HelpMessageCli()
strUsage += HelpMessageOpt("-?", _("This help message"));
strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), BITCOIN_CONF_FILENAME));
strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
+ strUsage += HelpMessageOpt("-getinfo", _("Get general information from the remote server"));
@TheBlueMatt

TheBlueMatt Sep 27, 2017

Contributor

Are you going to update the help text to note something about balance/height atomicity? The old getinfo takes cs_main and cs_wallet and returns atomic information, this no longer does. getwalletinfo will likely eventually return something so that you can report if a result was atomic here (probably a follow-up to #10286).

@achow101

achow101 Sep 28, 2017

Member

What do you suggest that the text for such a note be?

@laanwj

laanwj Sep 28, 2017

Owner

as getinfo is misleading here too, see #10871 (comment), I still don't see this as a strong point, but as it seems to bother @TheBlueMatt very much let's just add it...

@TheBlueMatt

TheBlueMatt Sep 28, 2017 •
Edited 1 time

Contributor

@laanwj I do not believe that to be the case in any release nor on master during normal operation (there are special cases for locked wallets). It will be the case after #10286, however there is no getinfo on master either....

@TheBlueMatt
Contributor

TheBlueMatt commented Sep 28, 2017

laanwj and others added some commits Sep 29, 2016

@laanwj @achow101
rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` 
This adds the infrastructure `BaseRequestHandler` class that takes care
of converting bitcoin-cli arguments into a JSON-RPC request object, and
converting the reply into a JSON object that can be shown as result.

This is subsequently used to handle the `-getinfo` option, which sends
a JSON-RPC batch request to the RPC server with
`["getnetworkinfo", "getblockchaininfo", "getwalletinfo"]`,
and after reply combines the result into what looks like a `getinfo`
result.

There have been some requests for a client-side `getinfo` and this
is my PoC of how to do it. If this is considered a good idea
some of the logic could be moved up to rpcclient.cpp and
used in the GUI console as well.

Extra-Author: Andrew Chow <achow101@gmail.com>
3826253
@jnewbery @achow101
Add test for bitcoin-cli -getinfo 
Extra-Author: Andrew Chow <achow101@gmail.com>
5e69a43
@achow101
Member

achow101 commented Sep 28, 2017

Added the atomicity note to the help text.
@laanwj
Owner

laanwj commented Sep 28, 2017

utACK 5e69a43

@laanwj laanwj merged commit 5e69a43 into bitcoin:master Sep 28, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 28, 2017

@laanwj
Merge #10871: Handle getinfo in bitcoin-cli w/ -getinfo (revival of #… 
…8843)


5e69a43 Add test for bitcoin-cli -getinfo (John Newbery)
3826253 rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` (Wladimir J. van der Laan)

Pull request description:

  Since @laanwj doesn't want to maintain these changes anymore, I will.

  This PR is a revival of #8843. I have addressed @jnewbery's comments.

  Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic.

Tree-SHA512: 9664ed13a5557bda8c43f34d6527669a641f260b7830e592409b28c845258fc7e0fdd85dd42bfa88c103fea3ecdfede5f81e3d91870e2accba81c6d6de6b21ff
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
c9a4aa8

@laanwj laanwj removed this from Blockers in High-priority for review Sep 28, 2017

@QingjingJing

QingjingJing commented Sep 28, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 6, 2017

@laanwj @luke-jr
rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` 
This adds the infrastructure `BaseRequestHandler` class that takes care
of converting bitcoin-cli arguments into a JSON-RPC request object, and
converting the reply into a JSON object that can be shown as result.

This is subsequently used to handle the `-getinfo` option, which sends
a JSON-RPC batch request to the RPC server with
`["getnetworkinfo", "getblockchaininfo", "getwalletinfo"]`,
and after reply combines the result into what looks like a `getinfo`
result.

There have been some requests for a client-side `getinfo` and this
is my PoC of how to do it. If this is considered a good idea
some of the logic could be moved up to rpcclient.cpp and
used in the GUI console as well.

Extra-Author: Andrew Chow <achow101@gmail.com>

Github-Pull: #10871
Rebased-From: 3826253
1a42e5c

@fanquake fanquake referenced this pull request Nov 13, 2017

Closed

Why was the 'getinfo' command removed? #11671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment