Remove core dependencies from CScript #4981

theuni commented at 7:29 AM on September 25, 2014: member

As an ongoing effort to break out script verification. I hope this doesn't conflict with @sipa's or @jtimon's next steps.

This work moves the logic for writing custom core classes to scripts out of CScript. Those classes can describe how their data should be written to a script, and CScript does so without further knowledge.

Please ignore the entire PR's diffstat while reviewing as it won't be at all helpful. Individual commits will make much more sense.

Also, CScriptBinaryData is a pretty terrible name but I couldn't come up with anything better. Suggestions welcome.

in src/script/binarydata.cpp:None in 2b87d26093 outdated

   0 | @@ -0,0 +1,46 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2014 The Bitcoin developers
   3 | +// Distributed under the MIT software license, see the accompanying
   4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +#include "script/binarydata.h"

Diapolo commented at 11:28 AM on September 25, 2014:

Nit: Can you add a newline above.

in src/script/binarydata.h:None in 2b87d26093 outdated

   0 | @@ -0,0 +1,24 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2014 The Bitcoin developers
   3 | +// Distributed under the MIT software license, see the accompanying
   4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +#include "script.h"

Diapolo commented at 11:29 AM on September 25, 2014:

Same here as above.

Also this is missing here:

#ifndef BITCOIN_BINARYDATA_H #define BITCOIN_BINARYDATA_H

And at the file ending: #endif // BITCOIN_BINARYDATA_H

in src/script/binarydata.h:None in 2b87d26093 outdated

   6 | +
   7 | +#include <vector>
   8 | +
   9 | +class CPubKey;
  10 | +class CScriptID;
  11 | +class CKeyID;

Diapolo commented at 11:29 AM on September 25, 2014:

Nit: This should be at the top of the class stuff (alphabetical ordering).

theuni commented at 7:31 PM on September 25, 2014:

alphabetical fixed. what does "at the top of the class stuff" mean?

in src/script/compressor.h:None in 2b87d26093 outdated

   6 | @@ -7,6 +7,11 @@
   7 |  #define H_BITCOIN_SCRIPT_COMPRESSOR
   8 |  
   9 |  #include "script/script.h"
  10 | +#include "serialize.h"
  11 | +
  12 | +class CKeyID;
  13 | +class CScriptID;
  14 | +class CPubKey;

Diapolo commented at 11:32 AM on September 25, 2014:

Nit: One up (alphabetical ordering).

in src/script/script.h:None in 2b87d26093 outdated

   5 | @@ -6,13 +6,38 @@
   6 |  #ifndef H_BITCOIN_SCRIPT
   7 |  #define H_BITCOIN_SCRIPT
   8 |  
   9 | -#include "key.h"
  10 | -#include "tinyformat.h"
  11 | -#include "utilstrencodings.h"
  12 | -
  13 | +#include <stdint.h>

Diapolo commented at 11:33 AM on September 25, 2014:

Nit: Wrong alphabetical ordering of #includes.

in src/script/script.h:None in 2b87d26093 outdated

 338 | @@ -314,10 +339,20 @@ class CScriptNum
 339 |  
 340 |  inline std::string ValueString(const std::vector<unsigned char>& vch)
 341 |  {
 342 | -    if (vch.size() <= 4)
 343 | -        return strprintf("%d", CScriptNum(vch).getint());
 344 | -    else
 345 | -        return HexStr(vch);
 346 | +    if (!vch.size())
 347 | +      return "";

Diapolo commented at 11:34 AM on September 25, 2014:

Nit: Indentation

in src/script/script.h:None in 2b87d26093 outdated

 349 | +    static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
 350 | +                                     '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
 351 | +    rv.reserve((vch.size())*2);
 352 | +    for(std::vector<unsigned char>::const_iterator it = vch.begin(); it != vch.end(); ++it)
 353 | +    {
 354 | +        unsigned char val = (unsigned char)(*it);

Diapolo commented at 11:34 AM on September 25, 2014:

Is this cast needed?

in src/script/script.h:None in 2b87d26093 outdated

 400 |  
 401 |      CScript& operator<<(int64_t b) { return push_int64(b); }
 402 |  
 403 | +    CScript& operator<<(const CScriptBinaryDataBase& data)
 404 | +    {
 405 | +      insert(end(), data.begin(), data.end());

Diapolo commented at 11:34 AM on September 25, 2014:

Indentation.

in src/script/sign.cpp:None in 2b87d26093 outdated

   8 | @@ -9,6 +9,7 @@
   9 |  #include "key.h"
  10 |  #include "keystore.h"
  11 |  #include "script/standard.h"
  12 | +#include "script/binarydata.h"

Diapolo commented at 11:35 AM on September 25, 2014:

Nit: Alphabtical ordering.

in src/script/standard.cpp:None in 2b87d26093 outdated

   5 | @@ -6,7 +6,9 @@
   6 |  #include "script/standard.h"
   7 |  
   8 |  #include "script/script.h"
   9 | +#include "script/binarydata.h"

Diapolo commented at 11:35 AM on September 25, 2014:

Nit: Alphabetical ordering.

in src/script/standard.h:None in 2b87d26093 outdated

   5 | @@ -6,9 +6,11 @@
   6 |  #ifndef H_BITCOIN_SCRIPT_STANDARD
   7 |  #define H_BITCOIN_SCRIPT_STANDARD
   8 |  
   9 | +#include "key.h"
  10 |  #include "script/script.h"
  11 |  #include "script/interpreter.h"
  12 |  
  13 | +#include <boost/variant.hpp>

Diapolo commented at 11:36 AM on September 25, 2014:

Needs to go after a newline below stdint.h.

Diapolo commented at 8:37 AM on October 1, 2014:

@theuni This nit is still current, even if very very minor :).

<pre> #include stdint.h #include boost/variant.hpp </pre>

in src/test/multisig_tests.cpp:None in 2b87d26093 outdated

   6 | @@ -7,6 +7,7 @@
   7 |  #include "main.h"
   8 |  #include "script/script.h"
   9 |  #include "script/interpreter.h"
  10 | +#include "script/binarydata.h"

Diapolo commented at 11:36 AM on September 25, 2014:

Nit: Alphabetical ordering.

in src/test/script_P2SH_tests.cpp:None in 2b87d26093 outdated

   6 | @@ -7,6 +7,7 @@
   7 |  #include "main.h"
   8 |  #include "script/script.h"
   9 |  #include "script/sign.h"
  10 | +#include "script/binarydata.h"

Diapolo commented at 11:36 AM on September 25, 2014:

Nit: Alphabetical ordering.

in src/test/script_tests.cpp:None in 2b87d26093 outdated

  10 | @@ -11,6 +11,7 @@
  11 |  #include "script/script.h"
  12 |  #include "script/sign.h"
  13 |  #include "core_io.h"
  14 | +#include "script/binarydata.h"

Diapolo commented at 11:37 AM on September 25, 2014:

This file on a whole seems unordered... well.

in src/test/sigopcount_tests.cpp:None in 2b87d26093 outdated

   4 | @@ -5,6 +5,7 @@
   5 |  #include "key.h"
   6 |  #include "script/script.h"
   7 |  #include "script/standard.h"
   8 | +#include "script/binarydata.h"

Diapolo commented at 11:37 AM on September 25, 2014:

Same here.

in src/test/transaction_tests.cpp:None in 2b87d26093 outdated

   9 | @@ -10,6 +10,7 @@
  10 |  #include "main.h"
  11 |  #include "script/script.h"
  12 |  #include "core_io.h"
  13 | +#include "script/binarydata.h"

Diapolo commented at 11:37 AM on September 25, 2014:

And here...

Diapolo commented at 11:38 AM on September 25, 2014: none

@theuni Sorry for hitting you with that bunch of nits, but they should be fixed IMHO.

in src/script/script.h:None in 2b87d26093 outdated

 344 | -    else
 345 | -        return HexStr(vch);
 346 | +    if (!vch.size())
 347 | +      return "";
 348 | +    std::string rv;
 349 | +    static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',

laanwj commented at 11:53 AM on September 25, 2014:

Please don't implement hexadecimal conversion code here. We've spent a lot of time going from duplicated code like this left around all over the place to the current state. My point of moving the string encoding/decoding to a separate utility file without dependencies (utilstrencodings) was that we can use those in places like here.

in src/script/binarydata.h:None in 65a9c9a619 outdated

   0 | @@ -0,0 +1,28 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2014 The Bitcoin developers
   3 | +// Distributed under the MIT software license, see the accompanying
   4 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +#ifndef BITCOIN_BINARYDATA_H

Diapolo commented at 8:47 PM on September 25, 2014:

Nit: Misses a single new line here still.

in src/script/binarydata.h:None in 65a9c9a619 outdated

  23 | +  explicit CScriptBinaryData(const uint160& hash);
  24 | +private:
  25 | +   void set(const unsigned char* begin, const unsigned char* end);
  26 | +   void set(std::vector<unsigned char>::const_iterator begin, std::vector<unsigned char>::const_iterator end);
  27 | +};
  28 | +#endif

Diapolo commented at 8:48 PM on September 25, 2014:

Nit: Also a new line before the closing endif is missing + the end comment #endif // BITCOIN_BINARYDATA_H

Diapolo commented at 8:52 PM on September 25, 2014: none

@theuni After the last few of my style nits are fixed you've got my style ACK. No, really thanks for updating this pull and taking care of the style stuff :)!

theuni commented at 9:13 PM on September 25, 2014: member

@Diapolo I'd put a rollseyes emoticon in the commit message if I could ;) Keeping things uniform is nice though, so thanks for reviewing. @laanwj What do you suggest? It'd defeat the purpose to depend on Core for the parsing. This seems like a reasonable place to make an exception, but I understand the case against as well.

utilstrencodings.h itself isn't a terrible dependency, as it only depends on tinyformat. I suppose we could call that part of our "shipped" script lib, but it'd be a shame for such a small thing.

The alternative would be using HexStr(script.begin(),script.end()) everywhere in Core that currently uses script.ToString(), but that would be really ugly.

laanwj commented at 7:19 AM on September 26, 2014: member

@theuni Ok if a hexstr is really, truly, the only thing you need in the script lib, I'm fine with making an exception. This is trivially simple code.

One other remark though. The old implementation of that function is:

if (vch.size() <= 4)
    return strprintf("%d", CScriptNum(vch).getint());
else
    return HexStr(vch);

This makes sure that normal scriptnums are printed as numbers instead of hex strings, which people have come to expect. The new one always shows a hex string.

At least move the implementation to the implementation file instead of the header (then the header doesn't have to depend on tinyformat/strencodings/... even if you use it, and the library doesn't have to export those headers).

theuni commented at 5:12 PM on September 26, 2014: member

@laanwj That all sounds perfectly reasonable. How about this, then: Since there's still a good bit of work left before we can break out verification entirely, I'll leave the include for CScript for now. If, at the end, it's still the only thing needed from core, we'll dupe it here as you've specified.

Sound good?

sipa commented at 5:39 PM on September 26, 2014: member

I disagree with including script.h in key; that makes no sense - script is higher level.

I see why you're doing that - the problem is CScriptID being declared in key.h in the first place. I think it should move to script/standard.h instead.

theuni commented at 5:55 PM on September 26, 2014: member

@sipa: I was also unhappy with that include and tried moving CScriptID to a few places, but never found a better home. I'll give that a shot.

theuni commented at 7:31 PM on September 26, 2014: member

Moved CScriptID to standard.h as @sipa requested.

Re-added the string processing deps as described above. When the library can stand on its own, we can evaluate whether it's worth it to dupe the core functions there.

in src/script/script.h:None in 3532a12774 outdated

  18 | +#include <vector>
  19 | +
  20 | +/* Binary script data base class
  21 |  
  22 | -#include <boost/variant.hpp>
  23 | +   Allows arbitrary structures to be written to a script without CScript

sipa commented at 8:18 PM on September 26, 2014:

Is this necessary to be virtual and overridable? Just having a templated method/constructor that takes any type, and copied the vector from x.begin() to x.end() seems like it would suffice. This is really just convenience - the user of CScript can always use his own code to first convert to a vector.

theuni commented at 9:34 PM on September 26, 2014:

Yea, it was done that way so that a user could make a distinction between input types and define his own serializers. If you'd rather avoid the overhead, I'm fine with a templated ctor.

sipa commented at 10:22 PM on September 26, 2014:

Yeah, I understand, I just don't think it's worth it for the cases where we need it.

laanwj commented at 8:08 AM on September 29, 2014:

Agreed with @sipa. Let's avoid virtuals here. There is no need for external parties to add anything here.

theuni force-pushed on Oct 1, 2014

BitcoinPullTester commented at 1:54 AM on October 1, 2014: none

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

theuni commented at 1:57 AM on October 1, 2014: member

Updated, rebased, and squashed. Simplified to avoid the use of virtuals. I believe this has addressed of all previous comments.

Sorry for the squash mid-review, but so much changed that it didn't make sense to read the commits in-order anymore.

in src/script/script.h:None in ea9d733a5a outdated

  33 | +        if(size)
  34 | +        {
  35 | +            assert(size <= UCHAR_MAX);
  36 | +            m_vch.resize(size+1);
  37 | +            m_vch.front() = size;
  38 | +            std::copy(in.begin(),in.end(),m_vch.begin()+1);

sipa commented at 2:12 AM on October 2, 2014:

Coding style (spaces).

sipa commented at 2:13 AM on October 2, 2014: member

Needs rebase, but with the different test/script refactors still going on, it may make sense to wait a bit?

laanwj commented at 7:43 AM on October 2, 2014: member

@sipa Agreed that there are a lot of changes pending to the same area. In this case it may make sense to plan ahead a bit, what makes sense to merge first, to avoid unnecessary non-trivial rebases.

We have at least:

#4981 Remove core dependencies from CScript
#5004 Make SCRIPT_VERIFY_STRICTENC compatible with BIP62
#5000 Blacklist NOPs reserved for soft-fork upgrades
#4954 Don't return an address for invalid pubkeys
~~#4890 Make signature caching optional, and move it out. ~~
#4694 Don't hash what you're not going to sign
#4989 Replace SignatureHash() with class SignatureHasher

jtimon commented at 6:53 PM on October 2, 2014: contributor

There's also #4989 which is built on top #4890. #4311 and #4954 already need rebase.

theuni commented at 7:10 PM on October 2, 2014: member

I'm fine with rebasing this as long as necessary. I'd rather not hold off for too long if there are no objections though, as it does block future reorg work. Though to be fair, future reorg work will only cause more churn, so I suppose that doesn't really help my case :)

Much of https://github.com/theuni/bitcoin/commit/4e4ae57fd28ef0a191ccd8f88192ea53daed9d3f could be moved out to a new PR that likely wouldn't conflict with anyone else's work here, but the other changes would for sure.

As a quick thought, for those of us working on reorg/break-out, we could work in a separate repo and PR back to mainline in stages, more in-line with a git maintainership workflow. Then we'd be working together rather than against eachother. @jtimon @sipa interested, or more trouble than it's worth?

TheBlueMatt commented at 12:58 AM on October 3, 2014: member

Actually I'll wait and review this when things are more clear as to the way forward.

jtimon commented at 3:32 AM on October 3, 2014: contributor

Actually now that things are more separated I don't think this conflicts with #4694 or #4890, and #4989 needs more discussion anyway and I may want to rebase it on top of another PR I'm preparing. So on my part I'm fine with merging this first. I mean, I would oppose to merging #4694 and #4890 first, but as said I'm not concerned about the potential conflicts around script if the order is different.

theuni force-pushed on Oct 3, 2014

theuni commented at 7:35 PM on October 3, 2014: member

rebased and nits addressed (i believe).

jtimon commented at 8:11 PM on October 3, 2014: contributor

Untested ack.

in src/script/script.h:None in 77e5c20e88 outdated

  11 | -#include "utilstrencodings.h"
  12 | -
  13 | +#include <assert.h>
  14 | +#include <climits>
  15 | +#include <limits>
  16 | +#include <string.h>

Diapolo commented at 11:30 PM on October 3, 2014:

Nit: string.h should be after stdint.h

Diapolo commented at 11:31 PM on October 3, 2014: none

Just 2 nits left, but if this gets merged I'll clean them up anyway ;).

sipa commented at 10:26 PM on October 6, 2014: member

Needs rebase.

theuni force-pushed on Oct 6, 2014

theuni commented at 10:58 PM on October 6, 2014: member

Rebased and nits addressed.

sipa commented at 10:54 PM on October 8, 2014: member

Untested ACK, but needs rebase again (sorry...).

theuni commented at 11:21 PM on October 8, 2014: member

No worries, will do.

theuni force-pushed on Oct 9, 2014

theuni force-pushed on Oct 10, 2014

theuni commented at 4:27 PM on October 10, 2014: member

rebased again, should be good to go.

sipa commented at 5:19 PM on October 10, 2014: member

ACK, verified move-only (23b504f).

jgarzik commented at 3:04 PM on October 13, 2014: contributor

ut ACK

jtimon commented at 4:23 PM on October 13, 2014: contributor

ut re-ACK

in src/script/script.h:None in 23b504f9c3 outdated

  31 | +    {
  32 | +        size_t size = in.size();
  33 | +        if (size)
  34 | +        {
  35 | +            assert(size <= UCHAR_MAX);
  36 | +            m_vch.resize(size+1);

laanwj commented at 9:13 AM on October 14, 2014:

From a readability point I'd greatly prefer:

m_vch.reserve(size + 1);
m_vch.push_back(size);
m_vch.insert(m_vch.end(), in.begin(), in.end());

(edit: oeps)

theuni commented at 8:43 PM on October 14, 2014: member

Nit addressed. Will squash when ready.

in src/script/script.h:None in fcbe4e9dda outdated

  30 | +    CScriptBinaryData(const T& in)
  31 | +    {
  32 | +        size_t size = in.size();
  33 | +        if (size)
  34 | +        {
  35 | +            assert(size <= UCHAR_MAX);

sipa commented at 10:17 PM on October 14, 2014:

Hmm, this is not strict enough; the size needs to be at most 75 for this to work.

An alternative is not serializing the size in here, and doing that inside CScript::operator<<, which selects based on the size.

theuni commented at 10:42 PM on October 14, 2014:

will do.

sipa commented at 11:05 PM on October 14, 2014:

And when combined with BIP62, the data pushed should be at least 2 bytes too.

sipa commented at 12:05 AM on October 15, 2014: member

utACK. I'm fine with squashing now.

theuni force-pushed on Oct 15, 2014

theuni commented at 12:26 AM on October 15, 2014: member

squashed and rebased.

theuni force-pushed on Oct 15, 2014

theuni commented at 3:30 AM on October 15, 2014: member

made the const ref change as requested by @sipa and squashed it down.

in src/script/script.h:None in 0ed725f904 outdated

 414 | -
 415 | -    CScript& operator<<(const uint256& b)
 416 | -    {
 417 | -        insert(end(), sizeof(b));
 418 | -        insert(end(), (unsigned char*)&b, (unsigned char*)&b + sizeof(b));
 419 | +        assert(data.size() >= 2 && data.size() <= MAX_SCRIPT_ELEMENT_SIZE);

TheBlueMatt commented at 5:51 AM on October 15, 2014:

It seems the only point of using a CScriptBinaryData over just using vectors is to assert that they are at least 2 bytes in size?

laanwj commented at 7:07 AM on October 15, 2014:

...and that they're prefixed with the size?

Edit: right? I don't see the size being added here, but it used to be the case in the last implementation of CScriptBinaryData

TheBlueMatt commented at 7:28 AM on October 15, 2014:

There should be no size added here. The next line just calls this << std::vector, which will prefix the size properly...I dont see why we need CScriptBinaryData?

laanwj commented at 7:45 AM on October 15, 2014:

Right, the size is added somewhere else.

AFAIK CScriptBinaryData is necessary to convert the argument (which is usually a uint160 or derived type) into a std::vector. It could be a function instead of a class.

theuni commented at 6:36 PM on October 15, 2014:

Well, this has been through many iterations now. Originally it was polymorphic so that eventual users could add their own serialization functionality. Then that was dropped and serialization was done on a per-type basis. Then that was dropped, and it was templated so that it's all done the same way.

So I suppose this should go one of two ways, rather than the hybrid that it is now

keep the class, which allows for implicit conversion from anything with begin/end iterators. That would mean that we could drop nearly all of b25d5f0dd531aaf5618f077c646afa556f6277ad, which should've been done when it was templatized.
Make it a function, and use as in b25d5f0dd531aaf5618f077c646afa556f6277ad.

I really don't care either way. Opinions?

TheBlueMatt commented at 4:32 AM on October 16, 2014:

I would do it in a very explicitly-named CScript function (ie want to make sure callers know that this is gonna go from begin() to end()).

sipa commented at 5:36 AM on October 16, 2014:

See #5091.

laanwj commented at 6:38 AM on October 16, 2014:

I like the idea of having it explicit as in this pull. I don't like the CBinaryData name, maybe simply a function

<< ToByteVector(xxx) <<

I think Cory is on the right track. I don't like the 'everything with a begin() and end() can apply here' as much. It makes the code hard to follow. And for the serialization code it is important to be easy to follow, as people (also those not very skilled in C++ magic) are reading it.

TheBlueMatt commented at 6:39 AM on October 16, 2014:

See-also comment thread on #5091 (review)

sipa commented at 6:42 AM on October 16, 2014:

So #5091 is a change I've wanted to do for a while - the part about replacing the pushers for uint160/CPubKey isn't important there. As this PR already has more work towards have some wrapping, it's probably easier to use something like the suggested ToByteVector here; I can rebase #5091.

theuni commented at 8:03 AM on October 16, 2014:

I still really don't mind either way. I see the merit in being explicit, and I see the merit in letting CScript handle whatever comes its way.

Sounds like everyone's ok with making the change to ToByteVector here, then discussing #5091 as a follow-up. It's fine by me if ToByteVector is essentially reverted there, as long as progress is being made.

I'll push that change.

sipa commented at 8:50 AM on October 16, 2014:

I'm not planning to revert ToByteVector in #5091 - I was just trying to avoid touching too much code there, but as it's exactly code you're already touching, that's fine.

laanwj commented at 10:09 AM on October 16, 2014:

I still really don't mind either way. I see the merit in being explicit, and I see the merit in letting CScript handle whatever comes its way.

I see merit in it, and I wouldn't care either way if this was, for example, GUI code. But for consensus-critical code it's too dangerous to just "handle everything that comes its way in some way". I like to be explicit and clear in what gets handled how and what the representation is. And to have compile-time errors when something is fishy. Remember the fun a while ago with std::vector<unsigned char>(a.begin,e.end()) with a being a std::vector<int>() "just working" by truncating each value?

in src/script/script.cpp:None in 0ed725f904 outdated

   4 | @@ -5,7 +5,18 @@
   5 |  
   6 |  #include "script.h"
   7 |  
   8 | -#include <boost/foreach.hpp>
   9 | +#include "tinyformat.h"
  10 | +#include "utilstrencodings.h"
  11 | +
  12 | +namespace {
  13 | +inline std::string ValueString(const std::vector<unsigned char>& vch)

TheBlueMatt commented at 5:53 AM on October 15, 2014:

Nit: Probably place this at line 267, right before the only function that uses it?

in src/script/script.h:None in 0ed725f904 outdated

  27 | +class CScriptBinaryData
  28 | +{
  29 | +public:
  30 | +    template <typename T>
  31 | +    CScriptBinaryData(const T& in) :
  32 | +    vch( in.size() ? std::vector<unsigned char>(in.begin(), in.end()) : std::vector<unsigned char>())

laanwj commented at 7:09 AM on October 15, 2014:

This expression in.size() ? std::vector<unsigned char>(in.begin(), in.end()) : std::vector<unsigned char>() is unnecessary. std::vector<unsigned char>(in.begin(), in.end()) would work just as well. Passing an empty range to the std::vector constructor is allowed. It won't dereference the iterators if they're equal.

sipa commented at 7:46 PM on October 15, 2014: member

We could of course just have a CScript::operator<<(T begin, T end) { std::vector<unsigned char> vch(begin, end); *this << vch; } ?

theuni force-pushed on Oct 16, 2014

theuni commented at 5:53 PM on October 16, 2014: member

Updated to use ToByteVector, squashed, and rebased.

ToByteVector() is currently in script.h, which is not a good place for it, but I don't see a better one that wouldn't drag in the deps we're working to remove here. I'm hoping we can at least agree to leave it there long enough to get this merged in, then move it around later if desired.

Also, I didn't add any extra checks or conditions to the operator<<(vector), since #5091 reworks that.

sipa commented at 6:05 PM on October 16, 2014: member

utACK

sipa commented at 11:33 PM on October 16, 2014: member

Agree with @jtimon; the first group of 3 commits and the second 3 commits make much more sense when squashed together, for example.

TheBlueMatt commented at 11:57 PM on October 16, 2014: member

theuni commented at 12:20 AM on October 17, 2014: member

Ok. I find it easier to review/digest that way, but I'll keep em more grouped together. Will squash.

script: move CScriptID to standard.h and add a ctor for creating them from CScripts

This allows for a reversal of the current behavior.

This:
CScript foo;
CScriptID bar(foo.GetID());

Becomes:
CScript foo;
CScriptID bar(foo);

This way, CScript is no longer dependent on CScriptID or Hash();

066e2a1403

script: add ToByteVector() for converting anything with begin/end

This should move to a util header once their dependencies are cleaned up.

e9ca4280f3

script: move ToString and ValueString out of the header db8eb54bd7

theuni force-pushed on Oct 17, 2014

script: add a slew of includes all around and drop includes from script.h

Lots of files ended up with indirect includes from script.h.

85c579e3a6

theuni commented at 5:46 PM on October 17, 2014: member

Squished down deeper. The header removal commit remains on top though, that was the purpose of this series. Not re-rebased against master, so this is identical code-wise to previously ACKed changes.

gavinandresen commented at 7:11 PM on October 17, 2014: contributor

utACK

laanwj merged this on Oct 22, 2014

laanwj closed this on Oct 22, 2014

laanwj referenced this in commit 25cc1cf8dc on Oct 22, 2014

MarcoFalke locked this on Sep 8, 2021