Handle getinfo in bitcoin-cli w/ -getinfo (revival of #8843) by achow101 · Pull Request #10871 · bitcoin/bitcoin

achow101 · 2017-07-18T22:19:36Z

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 · 2017-07-19T08:15:07Z

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

laanwj · 2017-07-20T08:31:04Z

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 · 2017-08-15T21:39:03Z

rebased

jnewbery · 2017-08-15T22:05:43Z

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 · 2017-08-23T18:32:43Z

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 · 2017-08-23T18:59:34Z

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 · 2017-08-23T19:26:23Z

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 · 2017-08-24T16:38:16Z

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 · 2017-08-24T16:47:32Z

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 · 2017-08-24T17:17:03Z

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 · 2017-09-01T16:57:24Z

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

take the functional test in https://github.com/jnewbery/bitcoin/tree/pr10871_test
fix the discrepencies between getinfo and -getinfo
rebase

achow101 · 2017-09-01T17:03:52Z

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

achow101 · 2017-09-05T17:10:12Z

Took the test and rebased.

fix the discrepencies between getinfo and -getinfo

What discrepancies?

jnewbery · 2017-09-05T17:21:58Z

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 · 2017-09-05T17:29:17Z

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 · 2017-09-05T21:33:20Z

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

jnewbery

one nit inline.

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

jnewbery · 2017-09-05T23:26:27Z

src/rpc/protocol.cpp

please also add braces to these if blocks

achow101 · 2017-09-06T00:08:54Z

Tidied up commits

jnewbery · 2017-09-06T00:47:59Z

src/bitcoin-cli.cpp

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

jnewbery

Some comments in line.

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

jnewbery · 2017-09-06T16:44:29Z

src/bitcoin-cli.cpp

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.

jnewbery · 2017-09-06T16:44:38Z

src/bitcoin-cli.cpp

nit: braces

jnewbery · 2017-09-06T16:45:21Z

src/bitcoin-cli.cpp

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

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

not currently addressed

achow101 · 2017-09-18T15:36:09Z

Fixed the test failure.

jnewbery

Looks really good. Just a few nitty comments inline

jnewbery · 2017-09-22T21:12:34Z

src/bitcoin-cli.cpp

nit strMethod -> method

jnewbery · 2017-09-22T21:12:51Z

src/bitcoin-cli.cpp

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

not currently addressed

jnewbery · 2017-09-22T21:21:29Z

test/functional/bitcoin_cli.py

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 · 2017-09-26T15:52:19Z

Addressed nits (sorry for the delay).

jnewbery · 2017-09-26T16:25:22Z

Tested ACK 5ca97e5f03d7352558991e8eb26bb58dc4eec453.

Thanks for sticking with this!

TheBlueMatt · 2017-09-27T20:28:33Z

src/bitcoin-cli.cpp

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

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

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

@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 · 2017-09-28T01:39:28Z

Something like "Note that unlike server-side RPC calls, the results of getinfo is the result of multiple non-atomic requests. Some entries in the result may represent results from different states (eg wallet balance may be as of a different block from the chain state reported)"

…

On September 27, 2017 8:06:29 PM EDT, Andrew Chow ***@***.***> wrote: achow101 commented on this pull request. > @@ -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")); What do you suggest that the text for such a note be? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #10871 (comment)

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 <[email protected]>

Extra-Author: Andrew Chow <[email protected]>

achow101 · 2017-09-28T01:53:30Z

Added the atomicity note to the help text.

laanwj · 2017-09-28T06:33:33Z

utACK 5e69a43

@laanwj

…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

QingjingJing · 2017-09-28T22:10:19Z

Thanks, helped.

…

On Thu, Sep 28, 2017 at 2:54 AM, Andrew Chow ***@***.***> wrote: Added the atomicity note to the help text. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#10871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AdBhR6LWhfiCahFF4ZmMTHD0vw0tUAmcks5smvxWgaJpZM4Ob_vu> .

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 <[email protected]> Github-Pull: bitcoin#10871 Rebased-From: 3826253

crombiecrunch · 2018-02-09T18:52:50Z

can someone please tell me how to apply this so that getinfo can work with the yiimp opensource pool files?

maflcko · 2018-02-09T19:55:30Z

@crombiecrunch Please submit a pull request upstream, replacing getinfo['balance'] with getwalletinfo['balance'] in the open source code.

Summary: 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 <[email protected]> Add test for bitcoin-cli -getinfo Extra-Author: Andrew Chow <[email protected]> Backport of PR10871 bitcoin/bitcoin#10871 Completes T586 Test Plan: make check test_runner.py bitcoin-cli -getinfo Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D2787

@laanwj

…al of bitcoin#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 bitcoin#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

jonasschnelli added the Scripts and tools label Jul 19, 2017

achow101 force-pushed the cli-getinfo branch from 6432850 to ecde516 Compare August 15, 2017 21:38

achow101 force-pushed the cli-getinfo branch from ecde516 to e697d54 Compare August 15, 2017 22:11

jnewbery mentioned this pull request Aug 25, 2017

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

Merged

achow101 force-pushed the cli-getinfo branch from e697d54 to c432fc4 Compare September 5, 2017 17:09

achow101 force-pushed the cli-getinfo branch from c432fc4 to 5d2341c Compare September 5, 2017 21:32

jnewbery reviewed Sep 5, 2017

View reviewed changes

achow101 force-pushed the cli-getinfo branch from 5d2341c to d1e4ea8 Compare September 6, 2017 00:06

achow101 force-pushed the cli-getinfo branch from d1e4ea8 to 226fa64 Compare September 6, 2017 00:10

jnewbery reviewed Sep 6, 2017

View reviewed changes

src/bitcoin-cli.cpp Outdated

Copy link

Contributor

jnewbery Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

achow101 force-pushed the cli-getinfo branch from 226fa64 to af61f69 Compare September 6, 2017 16:44

jnewbery reviewed Sep 6, 2017

View reviewed changes

achow101 force-pushed the cli-getinfo branch from af61f69 to 47458fe Compare September 6, 2017 17:06

achow101 force-pushed the cli-getinfo branch from f2fde56 to 2cf0866 Compare September 18, 2017 15:36

jnewbery mentioned this pull request Sep 21, 2017

Why is getinfo being removed? #11382

Closed

jnewbery reviewed Sep 22, 2017

View reviewed changes

achow101 force-pushed the cli-getinfo branch from 2cf0866 to 5ca97e5 Compare September 26, 2017 15:51

TheBlueMatt reviewed Sep 27, 2017

View reviewed changes

laanwj and others added 2 commits September 27, 2017 21:53

Add test for bitcoin-cli -getinfo

5e69a43

Extra-Author: Andrew Chow <[email protected]>

achow101 force-pushed the cli-getinfo branch from 5ca97e5 to 5e69a43 Compare September 28, 2017 01:53

laanwj merged commit 5e69a43 into bitcoin:master Sep 28, 2017

fanquake mentioned this pull request Nov 13, 2017

Why was the 'getinfo' command removed? #11671

Closed

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

meshcollider referenced this pull request Jan 24, 2018

(finally) remove getinfo in favor of more module-specific infos

aece8a4

crombiecrunch mentioned this pull request Feb 9, 2018

ERROR error -32601: method not found yiimp RavenProject/Ravencoin#23

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

achow101 commented Jul 18, 2017

Uh oh!

jonasschnelli commented Jul 19, 2017

Uh oh!

laanwj commented Jul 20, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

jnewbery commented Aug 15, 2017

Uh oh!

jnewbery commented Aug 23, 2017

Uh oh!

TheBlueMatt commented Aug 23, 2017

Uh oh!

laanwj commented Aug 23, 2017

Uh oh!

TheBlueMatt commented Aug 24, 2017

Uh oh!

laanwj commented Aug 24, 2017

Uh oh!

jnewbery commented Aug 24, 2017

Uh oh!

jnewbery commented Sep 1, 2017

Uh oh!

achow101 commented Sep 1, 2017

Uh oh!

achow101 commented Sep 5, 2017

Uh oh!

jnewbery commented Sep 5, 2017

Uh oh!

achow101 commented Sep 5, 2017

Uh oh!

achow101 commented Sep 5, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 18, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 26, 2017

Uh oh!

jnewbery commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 28, 2017 •

edited

Loading