Move code from script to wallet

jtimon commented at 11:14 am on August 23, 2014: contributor

Continues #4754

jtimon force-pushed on Aug 23, 2014

sipa commented at 11:26 am on August 23, 2014: member

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

jtimon commented at 11:43 am on August 23, 2014: contributor

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 ?

sipa commented at 11:45 am on August 23, 2014: member

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

jtimon commented at 11:46 am on August 23, 2014: contributor

class CScript; as shown in the controversial commit.

sipa commented at 11:51 am on August 23, 2014: member

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.

jtimon commented at 12:21 pm on August 23, 2014: contributor

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.

sipa commented at 12:40 pm on August 23, 2014: member

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.

laanwj commented at 10:33 am on August 25, 2014: member

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

jtimon force-pushed on Aug 27, 2014

jtimon commented at 4:20 pm on August 27, 2014: contributor

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.

jtimon force-pushed on Aug 27, 2014

jtimon force-pushed on Aug 28, 2014

laanwj added the label Improvement on Aug 28, 2014

laanwj added the label TX scripts on Aug 28, 2014

jtimon force-pushed on Aug 29, 2014

jtimon commented at 9:43 pm on August 29, 2014: contributor

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

laanwj commented at 8:37 am on August 30, 2014: member

@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.

jtimon commented at 10:25 am on August 30, 2014: contributor

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

sipa commented at 10:42 am on August 30, 2014: member

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

jtimon commented at 10:48 am on August 30, 2014: contributor

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

laanwj commented at 11:21 am on August 30, 2014: member

@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.

jtimon commented at 11:31 am on August 30, 2014: contributor

Fair enough, wallet_ismine.o then

jtimon force-pushed on Aug 30, 2014

jtimon force-pushed on Aug 31, 2014

jtimon commented at 1:08 pm on August 31, 2014: contributor

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

jtimon force-pushed on Aug 31, 2014

sipa commented at 8:15 pm on September 1, 2014: member

@jtimon Sorry for not looking closer at the code wrt the CTxDestination question earlier. I didn’t realize that CScript itself used CTxDestination. I think that the easiest solution is moving CTxDestination to scriptutils.h (or whatever it’s called after renames), and convert the CScript methods SetDestination and SetMultisig to functions in scriptutils.h as well. The CScript data structure has no need to know about standard script types.

jtimon commented at 0:22 am on September 2, 2014: contributor

Don’t worry if I hadn’t insisted on moving it to key.h only by following the includes and I had fully separated instead (depending on key.h but being a script.h dependency), the responses would have been different I think. What you say seems right, it’s not that script/script and script/standard independently depend on script/destination (allow me the redundancy): is that I was right in the sense that CTxDestination doesn’t belong in script/script but I was wrong pushing it down, since it belongs to a higher layer with SetDestination and SetMultisig, didn’t saw that. Anyway, since #4754 needs rebase and rereview I’ll write a PR for destination on top of it. My plan is the following:

Create a new PR with preparations for #4754, a couple of commits with non-move-only stuff.
Rebase #4754 on top of the previous one including all the move-only things that were here.
Rebase this one on top of #4754 with the movements of code from script to wallet
Rebase all my other script-related card castle on top of #4754 instead of this one, the building movements to wallet are independent from the rest.
Come back to CTxDestination and see how high can be lifted.

Postscript: I know that my network of PR is getting messy: I’m slowly cleaning it up. Btw, nice merge 4737, I was considering to rebase 4796 on top of that already. It also solves that inconvenience with clang.

jtimon force-pushed on Sep 2, 2014

jtimon renamed this:
~~Make script even more modular~~
Move code from script to wallet
on Sep 2, 2014

jtimon commented at 10:25 am on September 2, 2014: contributor

Closing until #4754 is merged.

jtimon closed this on Sep 2, 2014

jtimon reopened this on Sep 8, 2014

jtimon force-pushed on Sep 8, 2014

jtimon commented at 9:11 pm on September 8, 2014: contributor

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

TheBlueMatt commented at 7:04 am on September 9, 2014: member

utACK, verified code move.

jtimon force-pushed on Sep 9, 2014

in src/test/multisig_tests.cpp: in c685d4648d outdated

194@@ -195,8 +195,10 @@ BOOST_AUTO_TEST_CASE(multisig_Solver1)
195         CTxDestination addr;
196         BOOST_CHECK(ExtractDestination(s, addr));
197         BOOST_CHECK(addr == keyaddr[0]);
198+#ifdef ENABLE_WALLET

laanwj commented at 2:36 pm on September 9, 2014:

Instead of putting these #ifdefs, I’d prefer to move these to a wallet-specific test cpp (e.g. wallet_tests.cpp or create a new one)

sipa commented at 2:40 pm on September 9, 2014:

+1, but I wouldn’t object to doing that later.

in src/test/multisig_tests.cpp: in c685d4648d outdated

 7@@ -8,7 +8,7 @@
 8 #include "script/script.h"
 9 #include "script/interpreter.h"
10 #include "script/sign.h"
11-#include "scriptutils.h"
12+#include "wallet_ismine.h"

laanwj commented at 2:43 pm on September 9, 2014:

If you keep the ifdefs, this include needs to be in #ifdef ENABLE_WALLET.

in src/Makefile.am: in c685d4648d outdated

102@@ -103,7 +103,7 @@ BITCOIN_CORE_H = \
103   script/script.h \
104   script/sign.h \
105   script/standard.h \
106-  scriptutils.h \
107+  wallet_ismine.h \

theuni commented at 6:28 pm on September 9, 2014:

please keep these alphabetized

jtimon commented at 7:26 pm on September 9, 2014:

Sorry, I always get confused with symbols, should it be:

0   wallet.cpp \
1   wallet_ismine.cpp \
2   walletdb.cpp \

or

0   wallet.cpp \
1   walletdb.cpp \
2   wallet_ismine.cpp \

?

theuni commented at 6:30 pm on September 9, 2014: member

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

Move scriptutils.o to wallet 6c4d5061ed

Move CAffectedKeysVisitor to wallet.cpp (remove ExtractAffectedKeys) f7c299d55b

Rename scriptutils.o to wallet_ismine.o d1e623c444

jtimon force-pushed on Sep 9, 2014

jtimon commented at 7:30 pm on September 9, 2014: contributor

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.

laanwj commented at 7:36 pm on September 9, 2014: member

@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.

BitcoinPullTester commented at 7:40 pm on September 9, 2014: none

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.

sipa commented at 11:28 pm on September 9, 2014: member

ACK

laanwj referenced this in commit fd1caa0961 on Sep 10, 2014

laanwj commented at 10:57 am on September 10, 2014: member

Merged via fd1caa0 (added the #ifdef)

laanwj closed this on Sep 10, 2014

jtimon deleted the branch on Sep 13, 2014

jtimon commented at 3:06 pm on September 13, 2014: contributor

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

MarcoFalke locked this on Sep 8, 2021

Move code from script to wallet #4755