Add separate bitcoin-rpc client #3082

laanwj · 2013-10-11T21:14:36Z

Rework of #3072. This just adds the new executable.
I gave up on the code movements to remove the dependency on leveldb/BDB as things are just too entangled, see #3072 for details. Hopefully this will also make the pull tester happier. And this is easier to review, too.

This adds an executable bitcoin-rpc that only serves as a Bitcoin RPC client.
The commit does not remove RPC functionality from the bitcoind yet, this functionality should be deprecated but is left for a later version to give users some time to switch.

sipa · 2013-10-12T17:13:39Z

Some comments on the help output:

Usage:
  bitcoin-rpc [options] <command> [params]  Send command to -server or bitcoind

Not sure if it was introduced by this commit, but I have no idea what "sending to -server" would mean.

  -rpcport=<port>        Listen for JSON-RPC connections on <port> (default: 8332 or testnet: 18332)

For bitcoin-rpc, that should probably read "Connect to" instead of "Listen for"

  -rpcsslcertificatechainfile=<file.cert>  Server certificate file (default: server.cert)
  -rpcsslprivatekeyfile=<file.pem>         Server private key (default: server.pem)
  -rpcsslciphers=<ciphers>                 Acceptable ciphers (default: TLSv1+HIGH:!SSLv2:!aNULL:!eNULL:!AH:!3DES:@STRENGTH)

These aren't relevant for RPC clients.

sipa · 2013-10-12T17:17:41Z

src/init.h

@@ -14,6 +14,15 @@
 bool ShutdownRequested();
 void Shutdown();
 bool AppInit2(boost::thread_group& threadGroup);
-std::string HelpMessage();
+
+/* The help message mode determines what help message to show */


Nit: I'm not sure that init should have knowledge about what type of clients it could be built into. How about using bit-position flags for each of the classes instead (HMM_RPC_CLIENT, HMM_OPTIONAL_RPC_SERVER, HMM_NODE, ...), and passing the combination to it?

That's possible. It shouldn't be in init.cpp either. Init contains all kinds of stuff that is unnecessary for the RPC client.

Using a bitfield still implies knowledge of the type of clients, just in another format. It does need to know about the GUI so it can disable -daemon, for example. I'm not sure this is important enough to warrant a lot of thinking/planning though. Having a parameter is already lots better than a global variable!

laanwj · 2013-10-12T22:15:39Z

Not sure if it was introduced by this commit, but I have no idea what "sending to -server" would mean.

That was not introduced in this commit. It is also in bitcoind, I literally took that over. I suppose it means "a bitcoin-qt started with -server".

Agree on the rpcport having different meaning for the client than for the server, and indeed the SSL server settings can go.

laanwj · 2013-10-13T12:20:38Z

Updated the help message accordingly, and added mention in bitcoind help message that using it as client is deprecated.

sipa · 2013-10-13T14:10:56Z

src/init.cpp

-    strUsage += "  -blockminsize=<n>      "   + _("Set minimum block size in bytes (default: 0)") + "\n";
-    strUsage += "  -blockmaxsize=<n>      "   + _("Set maximum block size in bytes (default: 250000)") + "\n";
-    strUsage += "  -blockprioritysize=<n> "   + _("Set maximum size of high-priority/low-fee transactions in bytes (default: 27000)") + "\n";
+    if (hmm == HMM_BITCOIND || hmm == HMM_BITCOIN_RPC)


Shouldn't this be HMM_BITCOIND || HMM_BITCOIN_QT?

Yes, you're right.

sipa · 2013-10-13T15:08:10Z

ACK.

One nit: -rpcsslciphers isn't used by the RPC client either.

super3 · 2013-10-13T23:08:00Z

Good work. Any stats on the performance over Bitcoind?

Any idea on the time period for deprecation and drop of RPC from Bitcoind? A fair amount of codebase in based on Bitcoind, so that might cause some breakage even if it is deprecated for a while.

sipa · 2013-10-13T23:16:56Z

I expect performance to be exactly identical to bitcoind for now.

Once we can create a stripped-down version with less dependencies, startup time may be reduced a bit. But really, it's just thin wrapper to expose an interface intended for computers to humans. If performance matters to you, you should be sending JSON-RPC directly instead of exec()ing a binary for every call.

super3 · 2013-10-14T03:48:51Z

Just curious. Thanks for clarifying.

On Sun, Oct 13, 2013 at 7:17 PM, Pieter Wuille notifications@github.comwrote:

I expect performance to be exactly identical to bitcoind for now.

Once we can create a stripped-down version with less dependencies, startup
time may be reduced a bit. But really, it's just thin wrapper to expose an
interface intended for computers to humans. If performance matters to you,
you should be sending JSON-RPC directly instead of exec()ing a binary for
every call.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3082#issuecomment-26229504
.

Shawn Wilkinson
Student, Morehouse College
Bitcoin Developer/Entrepreneur/Enthusiast
(1P4QkLsujBPBZyUwDezikL4fUSs7JvFhPv)
me@super3.org
http://super3.org

laanwj · 2013-10-14T07:19:56Z

Performance wasn't the concern here, just sanity. Why would you need bitcoind on a system that only does requests? And good luck browsing through the bitcoind help message to find options that concern the client. Even as bitcoin developer I needed three reworks to get those right. There is just no excuse in the world to merge the client and server into one executable.

IMO the deprecation time period should be 0.9 to 0.10 or 0.11. Looking at the large time window between major releases, and the carefulness that merchants already have to switch to new major releases, that's enough. In any case that decision is for later, we don't need to make it now.

@sipa shouldn't the ciphers be used for the client as well? As I understand it, with SSL both the client and server have their say in what cipher is used? Or is that only with SSH?

laanwj · 2013-10-15T10:50:16Z

Ok I've hidden -rpcsslciphers for the client. I still think it would be useful to have it in the client as well (SSL_CTX_set_cipher_list isn't even called in the client), but that is off-topic for this pull.

laanwj · 2013-10-20T10:08:59Z

In case we agree with adding a seperate RPC client, can we please merge this soon? Every change to the help message results in an ugly merge in init.cpp.

sipa · 2013-10-20T11:32:47Z

@gavinandresen @jgarzik @gmaxwell Any general opinion about this?

IMHO, the fact that bitcoind is both a client and a server is confusing, and that alone warrants separating them (though certainly not instantly, too much legacy code relies on bitcoind for now).

laanwj · 2013-10-20T12:08:11Z

Right, a lot of code relies on bitcoind being a client, that's why this doesn't disable or change the functionality in bitcoind, it just adds a new executable with only the RPC functionality. Removing is completely separate and should be left to a further future release when people had the chance to switch over.

sipa · 2013-10-20T13:52:54Z

@laanwj If we ever choose to actually separate the code, there is always the option to make bitcoind exec() bitcoin-cli in case some command is provided still.

jgarzik · 2013-10-20T14:22:29Z

ACK, with optional, feel-free-to-merge-without-this comments,

I would name it "bitcoin-cli" or "bitcoin-remote" mirroring some existing practices.
Remove, rather than deprecate, "bitcoind " usage. Will break some scripts, but so what. I'm more willing to break stuff like this, as 1.0 approaches.

luke-jr · 2013-10-20T14:29:44Z

Is there any reason this should be Bitcoin-specific? Couldn't a generic-JSON-RPC CLI client (perhaps written in Python?) work just as well, without complicating the codebase unnecessarily?

laanwj · 2013-10-20T14:43:38Z

@jgarzik Bitcoin-cli/remote is fine with me, I don't have any opinion on the name.
I still prefer a two-step deprecation process though.

@luke-jr How is this complicating the codebase? If anything, this is the beginning of a clean-up. I did some code movements in the original commit to separate different concerns into different files, which I've left out here for easier review, but they can still be done later.

It needs to be bitcoin specific because of the argument parsing. Only a bitcoin-specific client knows to parse argument 2 of sendtoaddress as a double (for example). Sure, it could be done in a Python script, but that complicates usage on windows or other systems that don't come with Python interpreter by default. And we have the C++ code already...

super3 · 2013-10-20T16:26:22Z

@sipa I like the idea backward compatibility using exec() to call bitcoin-cli. I assume care of course must be taken to prevent malicious injection via Bitcoind.

@jgarzik I agree that it needs to removed, but I'd rather devs have at least one release cycle to use bitcoin-cli and change their core code to match. Then you can remove on ~1.0. Deprecation give devs a fair warning. Removing is just kinda ripping the Bitcoind rug out from under them.

luke-jr · 2013-10-20T16:42:56Z

Complication, for example see all the different conditionals in strUsage now.

Argument parsing might be an issue, I suppose - are there any cases where a Number might be confused with a String or vice-versa? JSON doesn't have doubles or integers, just Numbers - a client or server that makes a distinction is a bug.

As long as we're modularising the code, it makes the most sense to actually modularise it IMO. Separate git repository and all.

laanwj · 2013-10-20T19:59:58Z

@luke-jr yes, the helpmessage function is complicated somewhat by this, but that's the only one. To make it more readable it could be formatted as a table with a bitfield per option (which client kinds it applies to), if anyone cares enough to do that.

A seperate git repository sounds pointless. It's useful to be able to build everything at once. A different directory within src/ would make sense tho.

sipa · 2013-10-20T20:37:23Z

Well I guess actual separate repositories is one potential future, but I doubt we'll get to the point with the current codebase where that makes sense.

I am in favor of a nice and shiny python RPC client, which supports things like tab completion, and inline help, and batch processing, and pipelines of queries. and unicorns. As long as nobody creates one, I think this will do just fine. If someone ever does create a better RPC client in Python (or another language), I'm sure we can replace the bitcoin-cli/remote binary with a self-contained executable compiled from it in distribution packages.

super3 · 2013-10-20T20:47:47Z

@sipa That kinda sounds like a fun project for me, after I do a few more documentation sweeps. I can add unicorns as long as you don't mind them being green.

laanwj · 2013-10-20T20:51:37Z

Wouldn't tab completion ideally be something in the shell instead of in the executable itself? Ie, bash has extensible command completion.

sipa · 2013-10-20T20:53:19Z

Fair enough- I wasn't entirely serious of course. I just mean that a featureful RPC client sounds useful, and Python (or any script language) sounds more appropriate for developing that than C++ - and easier to review.

wtogami · 2013-10-20T21:08:53Z

A python RPC client would definitely have benefits except for ease of cross platform distribution. For example, it appears that py2exe can't be cross-compiled from a Linux host and also relies upon Microsoft DLL's.

super3 · 2013-10-20T21:41:30Z

@sipa Which is why I said it would be a fun project. One or two of those features would be really easy to hack together using existing libs.

@wtogami Hmmm. Didn't know that py2exe had that limitation. Perhaps an alternative might provide the needed cross platform distribution. A quick google search reveals cx_freeze as a possibility.

luke-jr · 2013-10-21T04:09:30Z

FWIW: At least my Python install (on Linux) has a bunch of exes for building Windows stuff with.

jgarzik · 2013-10-21T04:17:55Z

Any sort of python clientage is outside the scope of this pull request. This PR is fine right now, even if a future decision replaces the C++ client with something else down the road.

laanwj · 2013-10-21T06:30:14Z

@sipa I now understand, you're thinking of some kind of interactive client, not so much the rpc client it is now that is used from the shell or shell script. Sure, that'd be a useful project. I played with that idea while making the debug console, but it stayed at that as we don't want a scripting language (js, python) in the main client :-) Let's open a new issue for that.

It'd be a complement to this and not a replacement. For lightweight interaction from shell scripts, a small executable (yes, we'll make it small eventually...) would still be preferable to a py2exe wrapped "monster".

Rebased and renamed to bitcoin-cli.

This adds an executable `bitcoin-rpc` that only serves as a Bitcoin RPC client. The commit does not remove RPC functionality from the `bitcoind` yet, this functionality should be deprecated but is left for a later version to give users some time to switch.

BitcoinPullTester · 2013-10-21T07:40:13Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2a03a39020da5ae38f05c38a5bf92c023acdeb90 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

Add separate bitcoin-rpc client

* remove liquidity provider. The reason behind this is two fold: 1. it is very likely nobody is currently running this on the mainnet (due to already strong liquidity), it provides no use for testnet as there you can just enable mixing and get the same affect. Because of this reason I don't find it okay to just remove it outright. 2. even if people did use it (or if some do as a novelty) it likely hurts the privacy of the system. This is because as a liquidity provider your outputs will always take many (sometimes hundreds) blocks before being used in a new mixing round, whereas real user's funds will normally be mixed again very rapidly. Signed-off-by: Pasta <pasta@dashboost.org> * remove unused function Signed-off-by: Pasta <pasta@dashboost.org>

laanwj mentioned this pull request Oct 11, 2013

Add separate bitcoin-rpc client #3072

Closed

sipa reviewed Oct 12, 2013
View reviewed changes

sipa reviewed Oct 13, 2013
View reviewed changes

super3 mentioned this pull request Oct 21, 2013

Adding an Interactive RPC Client. #3122

Closed

laanwj added a commit that referenced this pull request Oct 22, 2013

Merge pull request #3082 from laanwj/2013_10_rpccli3

b3dd90c

Add separate bitcoin-rpc client

laanwj merged commit b3dd90c into bitcoin:master Oct 22, 2013

Diapolo mentioned this pull request Oct 22, 2013

remove duplicate -rpcsslciphers help message from init #3129

Merged

laanwj deleted the 2013_10_rpccli3 branch April 9, 2014 14:19

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

Add separate bitcoin-rpc client #3082

Add separate bitcoin-rpc client #3082

Uh oh!

Conversation

laanwj commented Oct 11, 2013

Uh oh!

sipa commented Oct 12, 2013

Uh oh!

sipa Oct 12, 2013

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 12, 2013

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 12, 2013

Uh oh!

laanwj commented Oct 13, 2013

Uh oh!

sipa Oct 13, 2013

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 13, 2013

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 13, 2013

Uh oh!

super3 commented Oct 13, 2013

Uh oh!

sipa commented Oct 13, 2013

Uh oh!

super3 commented Oct 14, 2013

Uh oh!

laanwj commented Oct 14, 2013

Uh oh!

laanwj commented Oct 15, 2013

Uh oh!

laanwj commented Oct 20, 2013

Uh oh!

sipa commented Oct 20, 2013

Uh oh!

laanwj commented Oct 20, 2013

Uh oh!

sipa commented Oct 20, 2013

Uh oh!

jgarzik commented Oct 20, 2013

Uh oh!

luke-jr commented Oct 20, 2013

Uh oh!

laanwj commented Oct 20, 2013

Uh oh!

super3 commented Oct 20, 2013

Uh oh!

luke-jr commented Oct 20, 2013

Uh oh!

laanwj commented Oct 20, 2013

Uh oh!

sipa commented Oct 20, 2013

Uh oh!

super3 commented Oct 20, 2013

Uh oh!

laanwj commented Oct 20, 2013

Uh oh!

sipa commented Oct 20, 2013

Uh oh!

wtogami commented Oct 20, 2013

Uh oh!

super3 commented Oct 20, 2013

Uh oh!

luke-jr commented Oct 21, 2013

Uh oh!

jgarzik commented Oct 21, 2013

Uh oh!

laanwj commented Oct 21, 2013

Uh oh!

BitcoinPullTester commented Oct 21, 2013

Uh oh!

Uh oh!