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

pull jl2012 wants to merge 1 commits into bitcoin:master from jl2012:minimalif changing 8 files +101 −2
  1. jl2012 commented at 3:51 PM on August 16, 2016: contributor

    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.

  2. in src/script/interpreter.h:None in 323a13a084 outdated 
      93 | @@ -94,6 +94,10 @@ enum
  94 |      // Making v1-v16 witness program non-standard
  95 |      //
  96 |      SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM = (1U << 12),
  97 | +
  98 | +    // Require the argument of OP_IF/NOTIF to be exactly 0x01 or empty vector
    sipa commented at 4:27 PM on August 16, 2016:

    Mention that it only applies to witness scripts.

    jl2012 commented at 5:56 PM on August 16, 2016:

    will fix in a squash commit later

  3. MarcoFalke added this to the milestone 0.13.1 on Aug 16, 2016
  4. MarcoFalke added the label Needs backport on Aug 16, 2016
  5. in src/test/data/script_tests.json:None in 323a13a084 outdated 
    2124 | @@ -2125,5 +2125,88 @@
2125 |  ["0", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", "CSV fails if stack top bit 1 << 31 is set and the tx version < 2"],
2126 |  ["4294967296", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME",
2127 |    "CSV fails if stack top bit 1 << 31 is not set, and tx version < 2"],
2128 | -["The End"]
2129 | +
2130 | +["MINIMALIF tests"],
    sipa commented at 4:35 PM on August 16, 2016:

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

    jl2012 commented at 5:56 PM on August 16, 2016:

    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

  6. jl2012 force-pushed on Aug 16, 2016
  7. dcousens commented at 9:09 PM on August 16, 2016: contributor

    concept ACK

  8. luke-jr commented at 9:23 PM on August 27, 2016: member

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

  9. jl2012 commented at 5:11 AM on August 28, 2016: contributor

    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

  10. NicolasDorier commented at 7:24 AM on August 28, 2016: contributor

    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.

  11. btcdrak commented at 8:23 AM on August 28, 2016: contributor

    Relevant discussion https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-August/013014.html

  12. NicolasDorier commented at 11:48 AM on August 28, 2016: contributor

    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.

  13. luke-jr commented at 1:24 PM on August 28, 2016: member

    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...?)

  14. instagibbs commented at 5:28 PM on August 29, 2016: member

    concept ACK as relay rule only

  15. jl2012 commented at 11:41 AM on September 1, 2016: contributor

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

  16. luke-jr commented at 6:34 PM on September 1, 2016: member

    @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).

  17. jl2012 commented at 6:40 PM on September 1, 2016: contributor

    @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

  18. josephpoon commented at 11:59 PM on September 1, 2016: none

    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).

  19. afk11 approved
  20. afk11 commented at 5:55 PM on September 17, 2016: contributor

    Code review / tested ACK

  21. in src/script/interpreter.cpp:None in e0b94de0df outdated 
     427 | @@ -428,6 +428,12 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
 428 |                          if (stack.size() < 1)
 429 |                              return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL);
 430 |                          valtype& vch = stacktop(-1);
 431 | +                        if (sigversion == SIGVERSION_WITNESS_V0 && flags & SCRIPT_VERIFY_MINIMALIF) {
    instagibbs commented at 8:03 PM on September 22, 2016:

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

    jl2012 commented at 5:08 AM on September 23, 2016:

    addressed with c72c5b1

  22. instagibbs approved
  23. instagibbs commented at 8:15 PM on September 22, 2016: member

    utACK e0b94de0dfbec9b305964a594f2e4785de51655d

  24. Make non-minimal OP_IF/NOTIF argument non-standard for P2WSH c72c5b1e3b
  25. jl2012 force-pushed on Sep 23, 2016
  26. btcdrak commented at 2:14 PM on September 27, 2016: contributor

    utACK c72c5b1

  27. laanwj commented at 3:10 PM on September 27, 2016: member

    Code-review ACK c72c5b1

  28. laanwj merged this on Sep 27, 2016
  29. laanwj closed this on Sep 27, 2016
  30. laanwj referenced this in commit 5a4f6d72e6 on Sep 27, 2016
  31. sipa commented at 3:12 PM on September 27, 2016: member

    utACK c72c5b1

  32. laanwj referenced this in commit 0027672c80 on Oct 13, 2016
  33. laanwj removed the label Needs backport on Oct 13, 2016
  34. DrahtBot locked this on Sep 2, 2022
Contributors
jl2012sipadcousensluke-jrNicolasDorierbtcdrakinstagibbsjosephpoonafk11laanwj

Milestone
0.13.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me