Remove all unnecessary dependencies for bitcoin-cli #4368

This pull removes all the unnecessary dependencies (key, core, netbase, protocol, ...) of bitcoin-cli.

To do this:

• Shards the chain parameters into BaseParams, which contains just the RPC port and data directory (as used by util and bitcoin-cli) and Params, with the rest.
• The network time-offset-mangement functions from util.cpp are moved to timedata.(cpp|h). This breaks the dependency of util on netbase.
• To avoid potential confusing scenarios, CChainParams does no longer assume the main chain by default. A chain must explicitly be set with SelectParams / SelectParamsFromCommandLine (or the Base counterpart). In practice this changes nothing, except in the tests.

Untested ACK.

I like breaking dependencies.

ACK most.

NAK lib change: it is flat out wrong to move low level, library objects from libbitcoin_common.a to libbitcoin_server.a.

That moves us in the opposite of the correct direction. We want to move towards a generic C++ bitcoin lib. For example, this immediately breaks #4332

General rule: fill up libbitcoin_common.a with as many objects as possible.

@jgarzik Right, the point I think is that we're introducing 3 levels here:

• what is needed by everything (formerly common)
• what is needed by bitcoind and potential future binaries that do require core code
• what is needed only by bitcoind (formerly server)

I noticed the move back, but as there exists nothing in the middle level we can always split again later. #4332 is probably a good reason to keep that level around.

Suggestion: libbitcoin_common, libbitcoin_server, libbitcoin_node.

Due to the way libraries work, I question the value of multiple libraries. libbitcoin_core.a and libbitcoin_server.a are quite sufficient. And even that split is unnecessary. It's not like the entire lib is linked in; compiler picks and chooses needed objects. Having fewer libraries becomes more valuable if a lib is ever made a shared object, too.

Also, we don't want to merge something, just to immediately undo/redo it.

@jgarzik:

• Having crypto/network/base58/script code in a dumb RPC client is silly.
• Having validation/blockstorage/rpc/mining code in your bitcoin-tx utility is silly.

That implies 3 levels.

@sipa That's just it. There is no crypto/network/base58/script code going into a dumb RPC client!

Look at how ar archives work.

@jgarzik And that's terribly fragile assurance. The minute someone accidentally places code in the wrong object and uses it, it may trigger global constructors being called, and other dependencies pulled in.

I care much more about the code organization part than what ends up in the binary. Having several layers brings clear separation of each part's responsibilities and interactions, and makes it compiler-enforced.

To shift the discussion a bit: I'd love to see the code that ends up in these separate layers (whether they're actually separate libraries or not) move to separate source tree directories.

That is also a way of making the separation clear - without compiler-enforcement, but at least made obvious in the code.

I agree with @sipa here. Having the split explicit is nice, it makes the linker/compiler enforce sw architecture constraints. I know about ar semantics but I don't want it to be possible to accidentally pull in more than necessary (believe me, that's too easy with the current code...).

I expected this concern, that is why I added comments such as # common: shared between bitcoin-cli, bitcoind, and bitcoin-qt to the Makefile.

We may want to rename the libraries to clarify further. If you think common implies more than it does after this change, then maybe it should be renamed to base or util or such! Then keep common for the stuff you want to share with the rawtx tool.

I agree with @jgarzik's analysis for the most part, but I wasn't going to complain because IMO this particular change doesn't set us back any. Breaking dependencies is crucial, but splitting internal libs is entirely arbitrary until an external api is defined and there is a logical scope for each lib. So while I'm not a fan of combining the libs, it would be no more than a lateral move really. @sipa's recently merged crypto stuff is a good example of how the libs should be broken up going forward. It has a single scope, a well-defined (though obviously not set-in-stone or exposed at the moment) api, and could be entirely swapped out with a new implementation in the future without changing any usage semantics. Granted that's an overly-simple case, but still example-worthy imo.

The thing is, without the Makefile.am changes it's hard to verify that this pull actually does what it says to do. And breaking dependencies without having it enforced somehow means it is easy to revert to the previous state. Until there is a well-defined API for the outside world, that's what matters.

Anyhow - I'm going to rename the library with just chainparamsbase.cpp, rpcprotocol.cpp, sync.cpp, util.cpp and version.cpp to libbbitcoin_util.a. These are the utilities and can be included by all tools that need access to bitcoind's configuration and data directory.

I'm going to keep libbitcoin_common.a around with the rest of the objects that are now in it (except for those moved to libbitcoin_util). @jgarzik can add the ones he needs from libbitcoin_server for the tx tool.

Done; also already moved coins.cpp and keystore.cpp to libbitcoin_common in preparation for #4332.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4368_75c82d4923174a126e1b87be6a43f274d2cc4c9b/ 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.

OK, ACK let's go ahead and get this merged so I can clean up the damage on my side