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.
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)?
bad rebase:
0bitcoin-cli.cpp: In function ‘int CommandLineRPC(int, char**)’:
1bitcoin-cli.cpp:393:41: error: ‘GetBoolArg’ was not declared in this scope
2 if (GetBoolArg("-getinfo", false)) {
3 ^
#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.
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 (review) for information about keypoolsize discrepancy.
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:
0getnetworkinfo:
1"version"
2"protocolversion"
3"timeoffset"
4"connections"
5"proxy"
6"relayfee"
7"errors"
8
9getblockchaininfo:
10"blocks"
11"difficulty"
12"testnet"
13
14getwalletinfo:
15"walletversion"
16"balance"
17"keypoololdest"
18"keypoolsize"
19"unlocked_until"
20"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.
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.
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
Is this still being actively worked on? Next steps would be:
getinfo and -getinfo
Took the test and rebased.
fix the discrepencies between
getinfoand-getinfo
What discrepancies?
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.
After #10838, I think the way to test this would be to collect the responses from
getwalletinfo,getblockchaininfoandgetnetworkinfo, merge those dictionaries (hint: https://stackoverflow.com/a/26853961) and then compare that to the response frombitcoin-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.
get*info RPCs instead of getinfo
141+ throw std::runtime_error("Batch must be an array");
142+ }
143+ std::vector<UniValue> batch(num);
144+ for (size_t i=0; i<in.size(); ++i) {
145+ const UniValue &rec = in[i];
146+ if (!rec.isObject())
one nit inline.
Please tidy up commits. First two commits should be squashed, and final two commits should be squashed into a second commit.
279+ {
280+ return reply.get_obj();
281+ }
282+};
283+
284+UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args)
395@@ -307,23 +396,24 @@ int CommandLineRPC(int argc, char *argv[])
396 while (std::getline(std::cin,line))
397 args.push_back(line);
398 }
399+ std::unique_ptr<BaseRequestHandler> rh;
400+ if (gArgs.GetBoolArg("-getinfo", false)) {
401+ rh.reset(new GetinfoRequestHandler());
402+ args.clear();
403+ args.push_back("getinfo");
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.
249+ if (!batch[ID_WALLETINFO].isNull()) {
250+ result.pushKV("walletversion", batch[ID_WALLETINFO]["result"]["walletversion"]);
251+ result.pushKV("balance", batch[ID_WALLETINFO]["result"]["balance"]);
252+ result.pushKV("keypoololdest", batch[ID_WALLETINFO]["result"]["keypoololdest"]);
253+ result.pushKV("keypoolsize", batch[ID_WALLETINFO]["result"]["keypoolsize"]);
254+ if (!batch[ID_WALLETINFO]["result"]["unlocked_until"].isNull())
210+ const int ID_NETWORKINFO = 0;
211+ const int ID_BLOCKCHAININFO = 1;
212+ const int ID_WALLETINFO = 2;
213+
214+ /** Create a simulated `getinfo` request. */
215+ UniValue PrepareRequest(const std::string& strMethod, const std::vector<std::string>& args) override
strMethod -> method
nit: function arguments should not be hungarian/camel case strMethod -> method
not currently addressed
Some comments in line.
You seem to have lost the test in your commit squashing.
197+ * as well as converting back to a JSON object that can be shown as result.
198+ */
199+class BaseRequestHandler
200+{
201+public:
202+ virtual UniValue PrepareRequest(const std::string& strMethod, const std::vector<std::string>& args) = 0;
strMethod -> method
55+ assert_equal(cli_get_info['keypoololdest'], wallet_info['keypoololdest'])
56+ assert_equal(cli_get_info['keypoolsize'], wallet_info['keypoolsize'])
57+ assert_equal(cli_get_info['paytxfee'], wallet_info['paytxfee'])
58+ assert_equal(cli_get_info['relayfee'], network_info['relayfee'])
59+
60+ if 'unlocked_until' in wallet_info:
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:
I think either is fine.
Tested ACK 5ca97e5f03d7352558991e8eb26bb58dc4eec453.
Thanks for sticking with this!
36@@ -37,6 +37,7 @@ std::string HelpMessageCli()
37 strUsage += HelpMessageOpt("-?", _("This help message"));
38 strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), BITCOIN_CONF_FILENAME));
39 strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
40+ strUsage += HelpMessageOpt("-getinfo", _("Get general information from the remote server"));
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 notifications@github.com wrote:
achow101 commented on this pull request.
@@ -37,6 +37,7 @@ std::string HelpMessageCli() strUsage += HelpMessageOpt("-?", (“This help message”)); strUsage += HelpMessageOpt("-conf=", strprintf((“Specify configuration file (default: %s)”), BITCOIN_CONF_FILENAME)); strUsage += HelpMessageOpt("-datadir=", _(“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: https://github.com/bitcoin/bitcoin/pull/10871#discussion_r141498012
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>
Extra-Author: Andrew Chow <achow101@gmail.com>
Thanks, helped.
On Thu, Sep 28, 2017 at 2:54 AM, Andrew Chow notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/10871#issuecomment-332704361, or mute the thread https://github.com/notifications/unsubscribe-auth/AdBhR6LWhfiCahFF4ZmMTHD0vw0tUAmcks5smvxWgaJpZM4Ob_vu .
