New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make non-minimal OP_IF/NOTIF argument non-standard for P2WSH #8526

Merged
merged 1 commit into from Sep 27, 2016

Conversation

@jl2012
Copy link
Member

jl2012 commented Aug 16, 2016

The argument for OP_IF/NOTIF should be exactly 0x01 or empty vector in P2WSH. Otherwise, a relay node may replace the argument with arbitrary data. This is now applied as a policy but eventually should become a softfork.

This is not applied to non-segwit scripts because people may have P2SH UTXOs that are not complying with the new rules.

@@ -94,6 +94,10 @@ enum
// Making v1-v16 witness program non-standard
//
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM = (1U << 12),

// Require the argument of OP_IF/NOTIF to be exactly 0x01 or empty vector

This comment has been minimized.

@sipa

sipa Aug 16, 2016

Member

Mention that it only applies to witness scripts.

This comment has been minimized.

@jl2012

jl2012 Aug 16, 2016

Member

will fix in a squash commit later

@MarcoFalke MarcoFalke added this to the 0.13.1 milestone Aug 16, 2016

@@ -2125,5 +2125,88 @@
["0", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", "CSV fails if stack top bit 1 << 31 is set and the tx version < 2"],
["4294967296", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME",
"CSV fails if stack top bit 1 << 31 is not set, and tx version < 2"],
["The End"]

["MINIMALIF tests"],

This comment has been minimized.

@sipa

sipa Aug 16, 2016

Member

I don't think we need to include any cases whose behaviour is not changed by minimalif.

This comment has been minimized.

@jl2012

jl2012 Aug 16, 2016

Member

I'm not sure what should(not) be tested. I want to show that the new rules are not applied to non segwit scripts by accident

@jl2012 jl2012 force-pushed the jl2012:minimalif branch from 323a13a to e0b94de Aug 16, 2016

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Aug 16, 2016

concept ACK

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 27, 2016

Prefer to just have an OP_CASTTOBOOL added. This breaks conditionals on OP_DEPTH and OP_SIZE for example.

@jl2012

This comment has been minimized.

Copy link
Member

jl2012 commented Aug 28, 2016

OP_CASTTOBOOL can't solve the problem. CastToBool in OP_IF is exactly the source of malleability. We needs to turn a OP_NOP into OP_ISBOOLVERIFY.

In any case, that requires a softfork which can't be done any time soon. What we need now is a quick policy patch. An alternative is to make SIZE IF and DEPTH IF excluded from this policy

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Aug 28, 2016

we did segwit so that we don't have to care about malleability in inputs, but now we care about malleability inside witness inputs? o_O

I would prefer to not touch that as DEPTH IF and SIZE IF are still useful. I'm not fan making special case either.

@btcdrak

This comment has been minimized.

Copy link
Member

btcdrak commented Aug 28, 2016

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Aug 28, 2016

ok read the conversation, I like the proposition of @jl2012 of making it a relay rule for now and deciding whether to make it a SF later. Concept ACK.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 28, 2016

No objection to a policy rule, just wanted to point that out here so it didn't get lost. :)

(OP_ISBOOLVERIFY could be policy-enforced just as well though...?)

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Aug 29, 2016

concept ACK as relay rule only

@jl2012

This comment has been minimized.

Copy link
Member

jl2012 commented Sep 1, 2016

@luke-jr , a "policy opcode" means we could never use that NOP for other purpose again, even if we abandon the softfork plan.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Sep 1, 2016

@jl2012 Not really. It simply means we'd need to wait a few releases from when it's policy-redefined to a straight-disallowed NOP, before it is safe to reuse. Along those lines, I wonder if it would make sense to redefine other unallocated opcodes as NOPs inside segwit now (although I doubt it will be useful in the long run).

@jl2012

This comment has been minimized.

Copy link
Member

jl2012 commented Sep 1, 2016

@luke-jr you can't redefine policy opcode because people are likely to have utxos with that.

and it won't make sense to create more NOPs. The use of NOPs is really limited as they can't manipulate the stack at all. For example, my most wanted code, OP_CAT, could not be done with and NOP

@josephpoon

This comment has been minimized.

Copy link

josephpoon commented Sep 1, 2016

Concept ACK. OK with either policy rule or consensus rule, finalized LN bitcoin scripts will be compatible.

To echo jl2012 's comment about having UTXOs, policy rules to an eventual SF is complicated because doing something as simple as OP_DUP OP_IF OP_SHA256 <PUSHDATA> OP_EQUAL
can invalidate scripts (you're computing on the same data in two places, one of which would be constrained in the SF when activated and the other constrained in the script itself). It is theoretically plausible to construct this kind of script for an actual useful purpose (e.g. if OP_FALSE as input, the other branch would compute on the next item on the stack, albeit it may be possible to construct those scripts more optimally without this problem in the first place, the possibility that it can happen with some kind of script is a concern).

@afk11

afk11 approved these changes Sep 17, 2016

Copy link
Contributor

afk11 left a comment

Code review / tested ACK

@instagibbs
Copy link
Member

instagibbs left a comment

utACK e0b94de

@@ -428,6 +428,12 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
if (stack.size() < 1)
return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL);
valtype& vch = stacktop(-1);
if (sigversion == SIGVERSION_WITNESS_V0 && flags & SCRIPT_VERIFY_MINIMALIF) {

This comment has been minimized.

@instagibbs

instagibbs Sep 22, 2016

Member

nit: put some () around the flag check for ease of reading

This comment has been minimized.

@jl2012

jl2012 Sep 23, 2016

Member

addressed with c72c5b1

@jl2012 jl2012 force-pushed the jl2012:minimalif branch from e0b94de to c72c5b1 Sep 23, 2016

@btcdrak

This comment has been minimized.

Copy link
Member

btcdrak commented Sep 27, 2016

utACK c72c5b1

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 27, 2016

Code-review ACK c72c5b1

@laanwj laanwj merged commit c72c5b1 into bitcoin:master Sep 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 27, 2016

Merge #8526: Make non-minimal OP_IF/NOTIF argument non-standard for P…
…2WSH

c72c5b1 Make non-minimal OP_IF/NOTIF argument non-standard for P2WSH (Johnson Lau)
@sipa
Copy link
Member

sipa left a comment

utACK c72c5b1

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 13, 2016

@laanwj laanwj removed the Needs backport label Oct 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment