Make script more modular

jtimon commented at 4:28 am on August 23, 2014: contributor

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 commented at 10:14 am on August 23, 2014: member

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.

jtimon force-pushed on Aug 23, 2014

jtimon force-pushed on Aug 27, 2014

laanwj added the label Improvement on Aug 28, 2014

laanwj added the label TX scripts on Aug 28, 2014

theuni commented at 5:10 pm on August 28, 2014: member

I’ll verify this today.

in src/script/interpreter.cpp: in 164e56bf88 outdated

131@@ -301,6 +132,209 @@ bool IsCanonicalSignature(const valtype &vchSig, unsigned int flags) {
132     return true;
133 }
134 
135+/** Wrapper that serializes like CTransaction, but with the modifications
136+ *  required for the signature hash done in-place
137+ */
138+class CTransactionSignatureSerializer {

theuni commented at 8:48 pm on August 28, 2014:

This was moved out of the anon namespace.

in src/script/script.cpp: in 164e56bf88 outdated

181+
182+    // This is a pay-to-script-hash scriptPubKey;
183+    // get the last item that the scriptSig
184+    // pushes onto the stack:
185+    const_iterator pc = scriptSig.begin();
186+    std::vector<unsigned char> data;

theuni commented at 8:59 pm on August 28, 2014:

Being extremely pedantic here:

0-    std::vector<unsigned char> data;
1+    vector<unsigned char> data;

It appears that you did this because you removed the std namespace for script/script.cpp. I agree with that change as it’s good practice, and it bothers me that “using namespace std” is used all over the bitcoin code.

HOWEVER

That makes this no longer just code movement, since that can change seemingly obvious semantics. It’s unlikely, but possible.

Assuming this is intended to be code movement only, please restore the “using namespace std” throughout, then we can nuke em as a follow-up.

theuni commented at 9:29 pm on August 28, 2014: member

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

jtimon commented at 1:48 pm on August 29, 2014: contributor

@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 commented at 4:12 pm on August 29, 2014: member

@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 commented at 4:15 pm on August 29, 2014: member

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 commented at 5:46 pm on August 29, 2014: contributor

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

jtimon force-pushed on Aug 29, 2014

jtimon commented at 5:54 pm on August 29, 2014: contributor

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 commented at 6:08 pm on August 29, 2014: member

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

 0cory@cory-i7:~/dev/bitcoin(code-movement)$ git diff code-movement origin/pr/4754
 1diff --git a/src/script/script.cpp b/src/script/script.cpp
 2index be6e849..60d1bea 100644
 3--- a/src/script/script.cpp
 4+++ b/src/script/script.cpp
 5@@ -7,6 +7,8 @@
 6
 7 #include <boost/foreach.hpp>
 8
 9+using namespace std;
10+
11 const char* GetOpName(opcodetype opcode)
12 {
13     switch (opcode)
14@@ -183,7 +185,7 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
15     // get the last item that the scriptSig
16     // pushes onto the stack:
17     const_iterator pc = scriptSig.begin();
18-    std::vector<unsigned char> data;
19+    vector<unsigned char> data;
20     while (pc < scriptSig.end())
21     {
22         opcodetype opcode;

theuni commented at 6:10 pm on August 29, 2014: member

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 commented at 11:54 pm on August 29, 2014: member

@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 commented at 8:01 am on August 30, 2014: contributor

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 commented at 11:32 am on August 30, 2014: contributor

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

sipa commented at 2:12 pm on August 30, 2014: member

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

jtimon commented at 6:19 pm on August 31, 2014: contributor

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 commented at 10:45 pm on August 31, 2014: member

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 commented at 11:38 pm on August 31, 2014: contributor

Restored CScript::clear() (without rebasing)

sipa commented at 1:18 pm on September 1, 2014: member

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

jtimon force-pushed on Sep 2, 2014

Rename script.h/.cpp to scriptutils.h/.cpp (plus remove duplicated includes) 086b7c85e2

Move CScript class and dependencies to script/script 3b64c914ae

Separate script/interpreter 9e58782428

Separate script/standard fbff412eb2

Separate CScriptCompressor db89318831

Separate script/sign ff51b1a116

jtimon force-pushed on Sep 2, 2014

jtimon commented at 2:30 pm on September 2, 2014: contributor

Rebased after #4812’s merge.

BitcoinPullTester commented at 2:43 pm on September 2, 2014: none

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 commented at 3:36 pm on September 8, 2014: member

ACK. Verified move-only.

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

in src/Makefile.am: in ff51b1a116

 97@@ -98,7 +98,12 @@ BITCOIN_CORE_H = \
 98   rpcclient.h \
 99   rpcprotocol.h \
100   rpcserver.h \
101-  script.h \
102+  script/interpreter.h \

Diapolo commented at 5:10 pm on September 8, 2014:

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.

sipa commented at 5:12 pm on September 8, 2014:

Let’s fix that after merging this.

Diapolo commented at 5:13 pm on September 8, 2014:

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

jtimon commented at 5:17 pm on September 8, 2014:

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

Diapolo commented at 8:05 am on September 9, 2014:

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

laanwj commented at 6:03 pm on September 8, 2014: member

Untested ACK

sipa referenced this in commit eecd3c0fb0 on Sep 8, 2014

sipa commented at 6:39 pm on September 8, 2014: member

Merged by eecd3c0f.

sipa closed this on Sep 8, 2014

jtimon deleted the branch on Sep 13, 2014

in src/script/compressor.h: in ff51b1a116

0@@ -0,0 +1,84 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2013 The Bitcoin developers
3+// Distributed under the MIT/X11 software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#ifndef H_BITCOIN_SCRIPT_COMPRESSOR
7+#define H_BITCOIN_SCRIPT_COMPRESSOR
8+
9+#include "script/script.h"

rebroad commented at 1:57 am on October 24, 2014:

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

theuni commented at 3:34 am on October 24, 2014:

This should be fixed in master by 3a757c52947a5a1beefb456ca7d3ebab7a5a14a2.

MarcoFalke locked this on Sep 8, 2021

Make script more modular #4754