Implement BIP62 rules 2, 3 and 4

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

sipa force-pushed on Oct 9, 2014

in src/script/interpreter.cpp: 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: 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: 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:

0redeemScript: OP_RIGHT (0x81)
1scriptSig: 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

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA1
 2
 3utACK commithash 16d78bd68e1488a2a84e368b50bf511c77392d2e
 4-----BEGIN PGP SIGNATURE-----
 5Version: GnuPG v2
 6
 7iQIcBAEBAgAGBQJUTuuqAAoJEIm7uGY+LmXOV1oQAJYu8QNCcxs9XkCkmgxnqc7v
 8ahhN52JRPx037WdH/pHFUCif0ZFnOXCNuhh/vHEZI8M6hVrSyb051nkzioRqPQYx
 9TYzDqzzcsCGbDuLuJfNkXgEbipgluodvCI6ktgiEBgLkl0rxLKNJR3Li7qSFrUxq
10q8TAHi5k+amNfGfejyeFT5HEJ0N/hwteExnTuGX0xH5yLYD3O1+W/geYTimU21WT
11ed444EcgG8nIDB/VIQ/3i2j8w1RkMY30v5uG0/FDAzaPRIM2x3nEQ/tsWMRgTVN4
122zqYITmgHvH5167SkIbmj3L5XoJhn0eu861rMDSE7AWhzbpQEzKoz/ws1EopK4KJ
13WPkgAP9bGUhTDqwBdKAnMQFN3wG0UY3PE1iXK81xNQAwH1YghTJTO/zDJ+YIakuD
147ApHR9BQwh4tGGuBpsByyIw9xwnLs/lQsWhOVJqjiGfAOi1apUu2lvFk91menUHz
15bRmBQWCEUMMNrEPal/q+ipLNe3JAB/Evam7/6nYRAZvb9NgD3Mx3KMrsTjdnlIZ+
165MbXsFq1WqbodsfSJY1LrF65jkBDHsznqnX1YA1HDvDux+Bgcs+1na2SrxptJ489
17B9fXmAQ7BvoCHcTyx96h2/gTxjiqc2swwOaBYJGKm8dfD9gpXJQbRSdGkuJp5Nbm
1878iHeo9dkcd9s0F7QmWy
19=Z6yq
20-----END PGP SIGNATURE-----

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

Implement BIP62 rules 2, 3 and 4 #5065