Implement BIP62 rules 2, 3 and 4 #5065

sipa commented at 1:49 AM on October 9, 2014: member

No description provided.

sipa force-pushed on Oct 9, 2014

in src/script/interpreter.cpp:None in c839843d64 outdated

 155 | @@ -156,6 +156,29 @@ bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags) {
 156 |      return true;
 157 |  }
 158 |  
 159 | +bool static CheckMinimalPush(const valtype& data, opcodetype opcode) {

Diapolo commented at 9:28 AM on October 9, 2014:

Just asking, why you and Gavin like to use

over

AFAIK the clang style will change it.

sipa commented at 8:01 PM on October 9, 2014:

It will.

But whenever I write code I try to mimick the style of the code around it. Once we effectively apply clang-format, that will be the same style everywhere.

in src/script/interpreter.cpp:None in c839843d64 outdated

 234 | +                    return false;
 235 | +                }
 236 |                  stack.push_back(vchPushValue);
 237 | -            else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF))
 238 | +            } else if (fExec || (OP_IF <= opcode && opcode <= OP_ENDIF))
 239 |              switch (opcode)

Diapolo commented at 9:28 AM on October 9, 2014:

Is this now missing some indentation?

sipa commented at 8:01 PM on October 9, 2014:

It always was missing indentation. I'm not changing anything about that.

in src/test/script_tests.cpp:None in c839843d64 outdated

   0 | @@ -1,4 +1,4 @@
   1 | -// Copyright (c) 2011-2013 The Bitcoin Core developers
   2 | +// Copyright (c) 2011-2014 The Bitcoin Core developers
   3 |  // Distributed under the MIT/X11 software license, see the accompanying

Diapolo commented at 9:29 AM on October 9, 2014:

Agreed, should be just MIT then.

sipa force-pushed on Oct 9, 2014

sipa force-pushed on Oct 10, 2014

TheBlueMatt commented at 7:06 AM on October 10, 2014: member

Made a few small comments on the commits d54b8e40908b3e9ca33d728d8f0ab921f856e1dc and 6a1f425942701b916bb5fe39e5ec76b2b5585b40. Other than those, LGTM

sipa force-pushed on Oct 13, 2014

petertodd commented at 7:19 PM on October 14, 2014: contributor

I made some minor improvements to this: https://github.com/petertodd/bitcoin/tree/bip62b-improvements

Better comments for CScriptNum(), OP_PUSHDATA4 handling, changed the invalid unittests to make them more likely to pass, and added bunch of unit tests to get ~100% coverage, and clearly separated them into PUSHDATA and numeric argument categories.

That said, other than those minor improvements, I can't find anything wrong with this patch after a few hours of thinking about it; strong ACK.

Also, I found out something really odd while tearing it apart: if we hadn't disabled OP_RIGHT you could have this valid pair of redeemScript's and scriptSigs:

redeemScript: OP_RIGHT (0x81)
scriptSig: 1 0 OP_1NEGATE

This works because OP_1NEGATE pushes 0x81 to the stack, which happens to also be a perfectly valid redeemScript!

laanwj added the label Tests on Oct 22, 2014

Add SCRIPT_VERIFY_SIGPUSHONLY (BIP62 rule 2) d752ba86c1

Add SCRIPT_VERIFY_MINIMALDATA (BIP62 rules 3 and 4)

Also use the new flag as a standard rule, and replace the IsCanonicalPush
standardness check with it (as it is more complete).

698c6abb25

Improve CScriptNum() comment

Edited-by: Pieter Wuille <pieter.wuille@gmail.com>

6004e77b92

Ensure MINIMALDATA invalid tests can only fail one way

Removes the need for the 'negated' versions of the tests, and ensures
other failures don't mask what we're trying to test.

554147ad9e

Test every numeric-accepting opcode for correct handling of the numeric minimal encoding rule dfeec18b85

Clearly separate PUSHDATA and numeric argument MINIMALDATA tests 2b62e1796b

Add valid invert of invalid every numeric opcode tests 16d78bd68e

sipa force-pushed on Oct 25, 2014

sipa commented at 10:19 AM on October 25, 2014: member

Rebased and included some of @petertodd's commits (fixed typo's in the comment improvement, and skipped the PUSHDATA4 one, because it looks controversial).

sipa commented at 11:41 AM on October 25, 2014: member

Added another test.

EDIT: nevermind; petertodd's cases already cover this.

sipa force-pushed on Oct 25, 2014

gavinandresen commented at 5:55 PM on October 27, 2014: contributor

utACK.

TheBlueMatt commented at 1:04 AM on October 28, 2014: member

laanwj merged this on Oct 28, 2014

laanwj closed this on Oct 28, 2014

laanwj referenced this in commit cd9114e513 on Oct 28, 2014

petertodd commented at 4:00 PM on October 31, 2014: contributor

@sipa FWIW I wasn't actually expecting you to add in all those commits individually; if you had wanted to squash them all or whatever that'd be fine by me. I just had it all split out to make it easy for you to review. But equally, totally ok for you to just pull them in directly too.

DrahtBot locked this on Sep 8, 2021