Add separate bitcoin-rpc client #3072

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.

To try to remove RPC client dependency on BDB and leveldb I

• Moved HelpMessage from init.cpp to util.cpp
• Split bitcoinrpc.cpp into rpcprotocol.cpp, rpcclient.cpp and rpcserver.cpp

Still this was not successful. It turns out that util.cpp, through Params().DataDir() in GetDataDir(fNetSpecific=true) depends on the block chain parameters, and that way pulls in all the core files (core.cpp, blockchain.cpp) including the ones that need leveldb and BDB. Splitting util.cpp would be possible (as the RPC client only needs the un-specific data dir to get at the configuration file) although somewhat more thinking is required. This would result in many more changes so let's leave it for later.

Note: This leaves the only usage of fHaveGUI left in wallet.cpp: https://github.com/bitcoin/bitcoin/blob/master/src/wallet.cpp#L486 which deals with the default key which we want to remove. After this, the variable can be removed from the core. This is outside the scope of this pull though.

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/f0bc89e011eda6620ec091523773c82d09398c82 for binaries and test log.

This could happen for one of several reasons:

1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

According to the pull tester this has a build problem on win32, though I but have no clue why. I'm missing some include in bitcoinrpc.h, but which one?

@laanwj This seems relevant: http://boost.2283326.n4.nabble.com/boost-asio-UnregisterWaitEx-RegisterWaitForSingleObject-has-not-been-declared-td4633963.html

Review comments: It would be nice if this were split into smaller chunks. At a minimum, I would recommend two commits:

• Code movement of existing RPC client code to new .h and .cpp locations. No external behavior changes.
• The rest of the changes, adding bitcoin-rpc.exe.

Overall the changes look pretty straightforward and correct.

Ok, thanks for a the link re: boost.

Smaller chunks? Usually the question is to squash together the commits not the other way around.

Sure. It is also possible to go too far with "everything in one patch" :) The ideal is a progression of logical code transformation steps, much like the steps in transforming an algebraic equation or math proof. Each step must be buildable and testable, so as to not break "git bisect."

Code movement is an easy thing to separate out, easy to test as a separate commit, and the commit that follows is much smaller and easier for a human to review and test. In the event of a difficult to discover bug, "git bisect" will tell you precisely which commit contains the bug, proving with 100% accuracy that certain commits are OK, or not.

I'll make a version without any code movement. That part was an attempt to split off the parts that don't rely on the database/block chain code, but it turned out pointless anyway.

Closing in favor of #3082

Hmm, in what way does chainparams depend on leveldb/bdb?

EDIT: transitive dependencies from chainparams I can find: bignum, uint256, util, core, protocol, netbase, sync, version, ui_interface, serialize, script, compat, hash, clientversion, allocators, keystore, key, crypter.

Through chainparams it pulls in almost all the implementation files.

The only way to solve this would be to make util no longer depend on chainparams, or split up util into a part for the rpc client and a part for the server (after all, the rpc client doesn't even need the network-specific directory or any other part of util.cpp that depends on the chain params).

Ok, with two simple modifications (adding a dummy main(), and moving uiInterface from init.cpp to noui.cpp), the following compile succeeds:

g++ -o blah noui.o allocators.o netbase.o script.o core.o version.o keystore.o key.o util.o \
protocol.o sync.o chainparams.o hash.o crypter.o -lcrypto -lboost_system \
-lboost_thread -lboost_program_options -lboost_filesystem

The resulting (stripped) binary is "only" 614 KiB here (a significant improvement, compared to bitcoind, which is 3525 KiB). Also note that it doesn't need leveldb, bdb and several boost dependencies.

I guess that means that we could try (as a first step), to separate those objects into a libbitcoincore.a or something, which bitcoin-rpc could link against; bitcoind and bitcoin-qt would use both libbitcoincore.a and libbitcoin.a then.

Arguably, the core should be smaller. Splitting up util into some components (I think e.g. logging and options-handling can be split off) would certainly help. In any case, netbase doesn't belong in core.

The more awkward dependencies are the result of script having both core, validation and wallet logic. I think script should be separated into script-core (the datastructure/serialization definitions), script-eval (used by the validation engine) and script-sign (used by the wallet). That should allow us to drop the dependencies on keystore, key and crypter in core.

But that's still way too much for the bitcoin-rpc client. There is no way in which the RPC client needs the core, script, key, hash, chainparams, sync, protocol or even allocators. The client part doesn't depend on that, it does no handling of keys or blocks.

The only thing it needs is part of the RPC stuff (what I had split into rpcprotocol.cpp and rpcclient.cpp in this commit), and part of util.cpp that has to do with finding the data directory and parsing the options file.

But let's first ACK and merge #3082 and then worry about splitting things up.

@laanwj Agree it's still way too much, and let's merge #3082 first. I was just thinking about modularizing the source code in general :)

Maybe I'm dim, but how can RPC-client live without protocol? I mean, isn't that what it has to use when it asks a remote bitcoind to do anything?

protocol is the P2P protocol nodes talk to eachother. RPC clients just use JSON over HTTP.