Unify and simplify CScript push methods #5091

sipa · 2014-10-16T05:35:29Z

Make CScript::operator<<(const std::vector&) cover all push operations (including OP_0, OP_1..OP_16 and OP_1NEGATE), and make the push for numbers use that.
Convert the uint256, uint160 and CPubKey into a single templated one using the vector push code.
Get rid of the int64_t operator<<; better be explicit about what needs pushing, so change all number pushes to use CScriptNum (which, due to the generic vector push now also correctly uses OP_n).
Get rid of some unused methods and constructors as a result.

TheBlueMatt · 2014-10-16T05:55:01Z

src/chainparams.cpp

Because the number push operator doesn't work anymore - it uses OP_4 intead of 1-byte push of 0x04. As this is one of the very few cases where you need the byte-level ability, I figured it's just shorter to turn it into a single byte sequence here.

Ugh, ok then...maybe put in a comment of the old code?

Why does the old code matter? The coinbase input is just a byte array.

I suppose for historical nostalgia? In any case, it matters not.

TheBlueMatt · 2014-10-16T06:31:41Z

utACK...hope no one ever writes a class with unsigned char* begin() and unsigned char* end()

laanwj · 2014-10-16T06:46:43Z

hope no one ever writes a class with unsigned char* begin() and unsigned char* end()

Why not? That would still be transparently converted to a std::vector, right?

TheBlueMatt · 2014-10-16T07:00:09Z

OK, hope no one ever writes a class with unsigned char* begin() and
unsigned char* end() and doesnt make those map to a consecutive memory
region they want turned into a vector :)

On 10/16/14 06:46, Wladimir J. van der Laan wrote:

hope no one ever writes a class with unsigned char* begin() and
unsigned char* end()
Why not? That would still be transparently converted to a std::vector,
right?

—
Reply to this email directly or view it on GitHub
#5091 (comment).

sipa · 2014-10-17T00:11:40Z

Rebased on top of #4981.

sipa · 2014-10-17T01:18:50Z

Also added a move-only commit to move a lot of the larger script methods to the .cpp file.

TheBlueMatt · 2014-10-18T00:12:07Z

theuni · 2014-10-18T00:34:01Z

I'm not sure this makes things any clearer. I'll delay judgement until giving it a re-read this weekend.

TheBlueMatt · 2014-10-18T00:35:44Z

It makes it marginally clearer, and the move to the .cpp is nice.

sipa · 2014-10-18T02:21:24Z

I should probably clarify a bit why I think this is nicer. Previously, there were some different ways of pushing vectors/numbers:

operator<<(vector): pushes the vector using OP_0, direct push, OP_PUSHDATA1 or OP_PUSHDATA2, but not using OP_1..OP_16 or OP_1NEGATE.
opterator<<(int64_t) pushes the vector representing the number using OP_1NEGATE, OP_0..OP_16 or a direct push.
operator<<(CSCriptNum) pushes the vector representing the number using OP_0 or direct push.

So the logic for number conversion and vector-to-opcode conversion was being done in several places, and pushing a vector or scriptnum would never result in using OP_1..OP_16 or OP_1NEGATE, even if that was the shortest way of doing it (which would violate BIP62).

This pull request moves all number-to-vector logic to CScriptNum, and all vector-to-opcode logic to operator<<(vector), and makes the rest wrappers around it. Also, it removes a bunch of unnecessary methods.

sipa · 2014-10-25T09:46:12Z

Rebased.

@theuni Do you understand my motivation now for this?

theuni · 2014-10-26T19:01:16Z

@sipa Yes, It's fine by me. My iffyness stemmed from the fact that this changes behavior somewhat from "I'll blindly add whatever you tell me to" to "just tell me what you want and I'll clean it up". I'd usually prefer to allow people to shoot themselves in the foot if they're really trying, but this really isn't the place for that.

Fwiw, I really meant for the ScriptBinaryData change (what eventually turned into the ToByteVector() function) to be used for this. I had hoped to leave a method around for adding exact data to a script. That would've made the chainparams change a good bit cleaner. But I suppose it's possible there aren't enough of those cases to justify easy the foot-shooting.

sipa · 2014-10-27T11:33:10Z

@laanwj comments?

gavinandresen · 2014-10-27T17:50:08Z

utACK

sipa · 2014-10-29T15:22:30Z

Rebased.

* Make CScript::operator<<(const std::vector<unsigned char>&) cover all push operations (including OP_0, OP_1..OP_16 and OP_1NEGATE), and make the push for numbers use that. * Convert the uint256, uint160 and CPubKey into a single templated one using the vector push code. * Get rid of the int64_t operator<<; better be explicit about what needs pushing, so change all number pushes to use CScriptNum (which, due to the generic vector push now also correctly uses OP_n). * Get rid of some unused methods and constructors as a result.

sipa · 2014-11-04T13:56:00Z

Rebased.

jtimon · 2014-11-05T10:54:45Z

Untested ACK

sipa · 2014-11-06T17:40:27Z

src/script/interpreter.cpp

@petertodd I think this would actually be a consensus change...

Yep! Well, maybe, probably not. I think this may only matter in the case of a valid signature, so you'd need a valid signature that is representable by a non-push opcode.

@TheBlueMatt If STRICTENC was written such that it caused the script to immediately fail, this could matter as CHECKMULTISIG evaluation exits early; the fork would be the difference between a passing CHECKMULTISIG NOT and a failed script.

True, though I think its rather orthogonal to this issue...and we knew (its in the src!) that STRICTENC was not a softfork.

That's not the point. It's an example of how a very subtle change could have lead to a fork because of a known consensus-critical dependency. Just because you don't know how to exploit it right now doesn't mean you can't.

Huh? My point was that STRICTENC is already a hardfork, and while I agree 100% with everything you said in your last statement, its again not connected to the previous discussion.

Again, you're comment gives the impression that you're arguing this change is ok, on the basis that it can't be exploited right now. I'm arguing this change isn't ok, because we have solid evidence that it changes behavior that could be used for an exploit. I'm not sure how, and maybe it isn't possible, but it's obviously a risky change.

Adding a separate method to serialize a unsigned char vector to a PUSHDATA and nothing else, and changing FindAndDelete() calls to use that method, may be one way to mitigate the risk.

Note how all of the above also suggests that conversions of data to CScripts() is something that should ideally be kept separate of the consensus-critical code.

This is also a good argument for incorporating hashed instruction-by-instruction execution traces into any future OP_EVAL proposal, where failure to match the trace hash exactly fails immediately. Merkelized abstract syntax tree ideas lend themselves naturally to this.

My point was not that this is OK, but that arguing based on STRICTENC is a completely invalid argument. In any case, we clearly agree, so...whatever.

There's nothing invalid about arguing that we had a close call. We should follow development practices that prevent close calls, given it's only a matter of time before one of those close calls turns into an actual exploit.

sipa · 2014-11-06T23:43:31Z

Too risky. Closing.

jtimon · 2014-11-07T15:05:56Z

Maybe we can push a subset of the changes that is not risky?

sipa mentioned this pull request Oct 16, 2014

Remove core dependencies from CScript #4981

Merged

sipa force-pushed the unipush branch from 0712c7c to c4156c6 Compare October 16, 2014 05:52

TheBlueMatt reviewed Oct 16, 2014
View reviewed changes

sipa force-pushed the unipush branch 2 times, most recently from 06d2bca to 4ad85ab Compare October 16, 2014 06:08

sipa force-pushed the unipush branch from 4ad85ab to 3ed1cb3 Compare October 17, 2014 00:10

sipa force-pushed the unipush branch from 3ed1cb3 to c5a758d Compare October 17, 2014 01:16

laanwj added the Refactoring label Oct 22, 2014

sipa force-pushed the unipush branch from c5a758d to 8620641 Compare October 25, 2014 09:44

sipa force-pushed the unipush branch from 8620641 to c378fd8 Compare October 29, 2014 15:22

sipa added 2 commits November 4, 2014 05:55

Move some CScript methods to script/script.cpp

6f95254

sipa force-pushed the unipush branch from c378fd8 to 6f95254 Compare November 4, 2014 13:55

sipa reviewed Nov 6, 2014
View reviewed changes

sipa closed this Nov 6, 2014

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

Unify and simplify CScript push methods #5091

Unify and simplify CScript push methods #5091

Uh oh!

Conversation

sipa commented Oct 16, 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!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 16, 2014

Uh oh!

laanwj commented Oct 16, 2014

Uh oh!

TheBlueMatt commented Oct 16, 2014

Uh oh!

sipa commented Oct 17, 2014

Uh oh!

sipa commented Oct 17, 2014

Uh oh!

TheBlueMatt commented Oct 18, 2014

Uh oh!

theuni commented Oct 18, 2014

Uh oh!

TheBlueMatt commented Oct 18, 2014

Uh oh!

sipa commented Oct 18, 2014

Uh oh!

sipa commented Oct 25, 2014

Uh oh!

theuni commented Oct 26, 2014

Uh oh!

sipa commented Oct 27, 2014

Uh oh!

gavinandresen commented Oct 27, 2014

Uh oh!

sipa commented Oct 29, 2014

Uh oh!

sipa commented Nov 4, 2014

Uh oh!

jtimon commented Nov 5, 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!

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!

sipa commented Nov 6, 2014

Uh oh!

jtimon commented Nov 7, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development