Make script more modular #4754

jtimon · 2014-08-23T04:28:57Z

While the building system is not ready for #4692 , we can start moving the code already.
When #4692 gets reopened other PRs touching script can be rebased on top of this one and decrease the chances of them being breaking/getting broke by 4692.

sipa · 2014-08-23T10:14:52Z

Idea ACK on splitting script in a base, interpreter and utils module. You'll need to fix a boost foreach include somewhere though, and I haven't verified that it's move only.

theuni · 2014-08-28T17:10:54Z

I'll verify this today.

theuni · 2014-08-28T20:48:21Z

src/script/interpreter.cpp

+/** Wrapper that serializes like CTransaction, but with the modifications
+ *  required for the signature hash done in-place
+ */
+class CTransactionSignatureSerializer {


This was moved out of the anon namespace.

theuni · 2014-08-28T21:29:25Z

I can attest to the code movement, other than the 2 issues raised above.

jtimon · 2014-08-29T13:48:14Z

@theuni Great, thanks. I can leave the std thing for a separate PR, no problem.
I'm confused about the anonymous namespace though. I thought it would be equivalent to just remove empty lines (which you know I did too). I don't understand its purpose, any reason to maintain that?

theuni · 2014-08-29T16:12:24Z

@jtimon See https://github.com/bitcoin/bitcoin/pull/4754/files#diff-be2905e2f5218ecdbe4e55637dac75f3L978 for what changed.

The same question is asked and answered well here: https://stackoverflow.com/questions/1147903/anonymous-namespace-class-definition

It's a rather insignificant functional change, I only pointed it out for the sake of being thorough.

sipa · 2014-08-29T16:15:16Z

Right, anonymous namespaces in C++ are weird and have little to do with actual namespacing... they just prevent symbols from being visible externally, similar to static functions/globals in C.

jtimon · 2014-08-29T17:46:18Z

So should I maintain the anonymous namespace or is it ok to delete it?

jtimon · 2014-08-29T17:54:43Z

Pushed again with the std nit solved, but I left the deletion of the anonymous namespace.
It's not something important so if it's going to hold back this PR I'll just restore it, but that stackoverflow thread doesn't convince me that we need it here.

theuni · 2014-08-29T18:08:57Z

@jtimon for reviews like this where we're strictly verifying code movement, please avoid rebasing until the end, just add the nit fixes on top. Your rebase could've changed anything, so all former reviews would be invalid. In this case, I stashed your pre-rebased branch before pulling. For anyone else who may be checking the movement, the diff is as-expected:

cory@cory-i7:~/dev/bitcoin(code-movement)$ git diff code-movement origin/pr/4754
diff --git a/src/script/script.cpp b/src/script/script.cpp
index be6e849..60d1bea 100644
--- a/src/script/script.cpp
+++ b/src/script/script.cpp
@@ -7,6 +7,8 @@

 #include <boost/foreach.hpp>

+using namespace std;
+
 const char* GetOpName(opcodetype opcode)
 {
     switch (opcode)
@@ -183,7 +185,7 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
     // get the last item that the scriptSig
     // pushes onto the stack:
     const_iterator pc = scriptSig.begin();
-    std::vector<unsigned char> data;
+    vector<unsigned char> data;
     while (pc < scriptSig.end())
     {
         opcodetype opcode;

theuni · 2014-08-29T18:10:54Z

I don't see why you'd remove the anon namespace as it's definitely a good thing, but it's not worth arguing here. ACK.

sipa · 2014-08-29T23:54:25Z

@jtimon They're not pointless. They allow the compiler to omit generated code if every instance of a function/method is inlined, make assumptions about how a function gets called, reduce risk of symbol name collisions, and speed up linking due to fewer exported symbols. I generally try to surround 'private' code in .cpp with an anonymous namespace for that reason. Though it's hardly wrong to remove them.

jtimon · 2014-08-30T08:01:10Z

CheckSig is called by scriptutils::CombineMultisig
CheckSig (and CTransactionSignatureSerializer) is moved above evalscript to prevent a redundant delcaration of checksig just before evalscript, but now that checksig is in the .h this movement was not necessary, sorry.
I guess it will be better to just leave it this way now.

jtimon · 2014-08-30T11:32:47Z

Would it make sense to move CombineMultisig to interpreter to keep CheckSig() private?

sipa · 2014-08-30T14:12:26Z

No, exposing CheckSig seems the right approach for that. Multisig related
stuff doesn't belong in interpreter inho.

jtimon · 2014-08-31T18:19:01Z

Added a fix to the last commit (without rebasing) that restores the order for a simpler diff between the old script.cpp and interpreter.cpp in which I also restore the anonymous namespace.
To avoid wasting the review work (as @theuni explained), it is better that the merger does the rebase himself.

sipa · 2014-08-31T22:45:44Z

You removed the recently added CScript::clear() method.
Apart from that, verified move-only between _1fa679edc5a1eb379049191e8ee18a3b8a0a5a05 and _243fb7a97cf9747a78acd81a66b362d427b1557a.

ACK if you add CScript::clear again.

jtimon · 2014-08-31T23:38:51Z

Restored CScript::clear() (without rebasing)

sipa · 2014-09-01T13:18:28Z

ACK.
Tested testnet reindex.
Verified move-only 1fa679edc5a1eb379049191e8ee18a3b8a0a5a05..2b0f9f9d5c3c4c8a60302951fccad65ed6eb7575.

…cludes)

jtimon · 2014-09-02T14:30:51Z

Rebased after #4812's merge.

BitcoinPullTester · 2014-09-02T14:43:32Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4754_ff51b1a1169c2bb271006620a4b648dc1385f081/ 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 · 2014-09-08T15:36:13Z

ACK. Verified move-only.

This needs a trivial rebase on top of #4865, but let's do that at merge time.

Diapolo · 2014-09-08T17:10:47Z

src/Makefile.am

@@ -98,7 +98,12 @@ BITCOIN_CORE_H = \
  rpcclient.h \
  rpcprotocol.h \
  rpcserver.h \
-  script.h \
+  script/interpreter.h \


Can you follow the alphabetical ordering please? I'm starting to get frustrated... I'm not allowed to cleanup and quite often pulls get in that don't even try to match that.

Let's fix that after merging this.

I really hope someone does it and also at some time our headers and includes reflect this...

Yes, sorry. Maybe #4755 is a good place to fix this?

@sipa @jtimon See #4881 for a cleanup ;). Couldn't resist.... perhaps just base #4755 on that?

laanwj · 2014-09-08T18:03:58Z

Untested ACK

e088d65 Separate script/sign (jtimon) 9294a4b Separate CScriptCompressor (jtimon) c4408a6 Separate script/standard (jtimon) da03e6e Separate script/interpreter (jtimon) cbd22a5 Move CScript class and dependencies to script/script (jtimon) 86dbeea Rename script.h/.cpp to scriptutils.h/.cpp (plus remove duplicated includes) (jtimon) Rebased-by: Pieter Wuille

sipa · 2014-09-08T18:39:13Z

Merged by eecd3c0.

rebroad · 2014-10-24T01:57:50Z

src/script/compressor.h

+#ifndef H_BITCOIN_SCRIPT_COMPRESSOR
+#define H_BITCOIN_SCRIPT_COMPRESSOR
+
+#include "script/script.h"


In file included from ./script/compressor.h:9:
./script/script.h:197:19: error: no matching conversion for functional-style cast from 'const char [57]' to
'scriptnum_error'
throw scriptnum_error("CScriptNum(const std::vector&) : overflow");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./script/script.h:173:7: note: candidate constructor (the implicit copy constructor) not viable: no known
conversion from 'const char [57]' to 'const scriptnum_error' for 1st argument
class scriptnum_error : public std::runtime_error
^
./script/script.h:176:14: note: candidate constructor not viable: no known conversion from
'const char [57]' to 'const std::string' (aka 'const basic_string<char, char_traits,
allocator >') for 1st argument
explicit scriptnum_error(const std::string& str) : std::runtime_error(str) {}
^
1 error generated.
make[1]: *** [qt/qt_libbitcoinqt_a-peertablemodel.o] Error 1
make: *** [src/qt/bitcoin-qt] Error 2

This should be fixed in master by 3a757c5.

jtimon force-pushed the libscript0 branch 2 times, most recently from f2b17e2 to 95d1245 Compare August 23, 2014 11:12

jtimon mentioned this pull request Aug 23, 2014

Move code from script to wallet #4755

Closed

jtimon force-pushed the libscript0 branch 4 times, most recently from 5ee9504 to 164e56b Compare August 27, 2014 23:44

This was referenced Aug 27, 2014

Don't hash what you're not going to sign #4694

Closed

Strictly hash what is going to be signed #4695

Closed

libbitcoinscript #4692

Closed

Don't pass nHashType down to script.h/.cpp #4555

Merged

laanwj added Improvement labels Aug 28, 2014

theuni reviewed Aug 28, 2014
View reviewed changes

jtimon force-pushed the libscript0 branch from 164e56b to 3e9215d Compare August 29, 2014 17:50

jtimon mentioned this pull request Aug 30, 2014

Split up script into a core and utility part. #4344

Closed

jtimon mentioned this pull request Sep 1, 2014

Preview: Compile a standalone VerifyScript() #4809

Closed

jtimon force-pushed the libscript0 branch from 2b0f9f9 to 24eb154 Compare September 2, 2014 00:28

jtimon mentioned this pull request Sep 2, 2014

Little preparations before moving all script code #4812

Merged

jtimon and others added 6 commits September 2, 2014 16:28

Rename script.h/.cpp to scriptutils.h/.cpp (plus remove duplicated in…

086b7c8

…cludes)

Move CScript class and dependencies to script/script

3b64c91

Separate script/interpreter

9e58782

Separate script/standard

fbff412

Separate CScriptCompressor

db89318

Separate script/sign

ff51b1a

jtimon force-pushed the libscript0 branch from 24eb154 to ff51b1a Compare September 2, 2014 14:29

Diapolo reviewed Sep 8, 2014
View reviewed changes

sipa closed this Sep 8, 2014

jtimon mentioned this pull request Sep 8, 2014

Remove CScriptCheck::nHashType (was always 0) #4775

Closed

jtimon deleted the libscript0 branch September 13, 2014 21:05

rebroad reviewed Oct 24, 2014
View reviewed changes

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

Make script more modular #4754

Make script more modular #4754

Uh oh!

Conversation

jtimon commented Aug 23, 2014

Uh oh!

sipa commented Aug 23, 2014

Uh oh!

theuni commented Aug 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Aug 28, 2014

Uh oh!

jtimon commented Aug 29, 2014

Uh oh!

theuni commented Aug 29, 2014

Uh oh!

sipa commented Aug 29, 2014

Uh oh!

jtimon commented Aug 29, 2014

Uh oh!

jtimon commented Aug 29, 2014

Uh oh!

theuni commented Aug 29, 2014

Uh oh!

theuni commented Aug 29, 2014

Uh oh!

sipa commented Aug 29, 2014

Uh oh!

jtimon commented Aug 30, 2014

Uh oh!

jtimon commented Aug 30, 2014

Uh oh!

sipa commented Aug 30, 2014

Uh oh!

jtimon commented Aug 31, 2014

Uh oh!

sipa commented Aug 31, 2014

Uh oh!

jtimon commented Aug 31, 2014

Uh oh!

sipa commented Sep 1, 2014

Uh oh!

jtimon commented Sep 2, 2014

Uh oh!

BitcoinPullTester commented Sep 2, 2014

Uh oh!

sipa commented Sep 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 8, 2014

Uh oh!

sipa commented Sep 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!