Why do you keep moving CTxDestination to key? IMHO it doesn't belong there at all.

Since this one depends on the other, I'll be showing more controversial things like that one.
And abandon the ones that don't get acked.
I still think CTxDestination doesn't belong to script. CTxDestination is related to CKeyID, CScriptID (in key.h) and CBitcoinAddress (in base58.h).
I thought the changes in the includes would make it more clear than the first time I tried it (#4698).
In particular, I want to avoid scriptutils.h including script/script.h and that's the only type impeding it.
Another way to do that would be by separating CTxDestination to its own file, but when I do that I feel I need to put it in key.h instead, maybe base58.h with CBitcoinAddress ?

How can scriptutils.h not include script.h? It needs the CScript type...?

class CScript; as shown in the controversial commit.

Ah.

So, my opinion about this: using forward declarations and .h files including fewer things than the corresponding .cpp file is nice for one thing only: speeding up compilation.

For all other purposes, I consider it ugly. It hides dependencies between modules, and may cause breaking in case of reorganizations that change types. As far as I'm concerned, the most important thing (and the only thing worth doing refactoring effort to obtain) is dependencies between modules, where a module is defined as the combination of the .h and .cpp file. You can't use the .h file without the .cpp file (typically), so including the .h file means you're depending on that code.

Moving CTxDestination to a place where it makes no sense semantically (scripts, and their identifiers, are much higher level constructs than keys) is IMHO a bad idea, but if the only goal is dropping an #include (without even breaking a module dependency), it's unacceptable.

Of course it all comes down to whether or not it makes sense semantically there.
The includes are just the clues that have led me to believe that an the destination may belong to key.h or base58.h. To be honest I didn't had much hope that the particular commit was going to be accepted, just wanted to present it in a way in which my reasoning is more clear.

Re: forward declarations: I guess if there weren't so many definitions in .h files, forward declarations wouldn't save that much compiling time.

• Key is a wrapper around ECDSA, and has types for private and public keys, and support signature generation and verification [key].
• Script defines a higher-level crypto system. It has private keys consisting of a CKeyStore plus a CScript redeem script, public keys consisting of just the CScript redeem script, and signatures consisting of CScript sigScripts [script].
• In order to help signing in this script crypto system, template matching on redeemscripts is supported, and particular subsets of such scripts can be referred to using a CTxDestination (which encodes a few possible templates in a concise way). [script standard/utils]
• Base58 is built on top of that, providing support for encoding some of the cryptographic data of the layers below in a human-readable way [base58]

Yes, it may seem that moving CTxDestination to CKey simplifies things, but that's just because you ignore the code in the script utils cpp file that is intimately using CTxDestination, while nothing relying on key is.

Really, just consider the .h and .cpp always as a whole when deciding boundaries of responsibilities.

Agree that CTxDestination doesn't belong in key.h. We've had this discussion before, please don't keep doing that.

I'm sorry I shouldn't have pushed that again. I should have at least used the separated destination.h instead of just mentioning it while pushing the change that had been rejected again, or better yet, not push that commit at all since the PR wasn't finished.
Anyway, I also moved ExtractDestination() and ExtractDestinations() to script/standard and added a few more commits.
At this point the only thing preventing me from moving scriptutils to libbitcoin_wallet is that script_P2SH_tests and multisig_tests would break when --disable-wallet.

Rebased on top of #4788
Maybe I should rename scriptutils.o to ismine.o or just move it to wallet.o?

@jtimon I'd say if it is only used by the wallet, move it to the wallet. On the other hand the wallet.cpp is growing quite a lot lately so maybe it makes sense to keep it split up. But make it clear in the name (and build system) that it's part of the wallet.

What about wallet_ismine.o?
Maybe wallet/ismine.o?
I mean, probably wallet should have its own directory too, and although that seems to belong to another PR it makes no sense to rename scriptutils.o to wallet_ismine.o and then wallet_ismine.o to wallet/ismine.o

I wouldn't worry too much about source/object names for now. Just
encapsulate where possible.

Yeah, it is bike-shedding but better do it before I do the rename.
Anyone opposed to wallet/ismine.o?

@jtimon I'd say wallet_ismine.o. Moving it to a directory can always be done later and should happen along with the other files libbitcoin_wallet.

Fair enough, wallet_ismine.o then

Rebased on top of #4754 's latest fix.and after #4788 has being merged.

Closing until #4754 is merged.

I can squash the alphabetical fix into one of the other commits.

utACK, verified code move.

Looks good to me. Agreed about moving out the wallet tests as a next step.

Updated with the ifdef in the includes of the tests fix.
I'm not sure about what's the correct alphabetic order in the makefile.

@jtimon In case of doubt about the sorting you could run the lines through sort. In vim this is a matter of selecting the range with V then :!sort.

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

ACK

Merged via fd1caa0 (added the #ifdef)

Thanks @laanwj. I don't use vim but emacs has the equivalent M-x sort-lines. I could have thought of that.