Add policy: null signature for failed CHECK(MULTI)SIG #8634

jl2012 · 2016-08-31T05:34:23Z

Intended for 0.13.1

This PR adds a new policy to require that signature(s) must be empty vector for failed CHECK(MULTI)SIG operations.

This fixes malleability for some forms of scripts, for example:

"key 1" CHECKSIG IF "key 2" CHECKSIG ELSE "key 3" CHECKSIGVERIFY "locktime" CLTV ENDIF

which is spendable with both sig 1 and sig 2 anytime, or sig 3 only after locktime

Without this new policy, a relay node may inject arbitrary BIP66-compliant data to the scriptSig/witness when the ELSE branch is executed.

This may become a softfork in the future. I will prepare for a BIP later

jl2012 · 2016-09-02T01:41:51Z

Please tag 0.13.1

btcdrak · 2016-09-02T11:00:02Z

Concept ACK

jl2012 · 2016-09-06T12:32:34Z

This is an implementation of NULLFAIL of BIP146, except the activation logic:

https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#NULLFAIL

jl2012 · 2016-09-06T12:40:15Z

src/test/data/script_tests.json

+["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL", "OK", "BIP66 and NULLFAIL-compliant"],
+["1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL", "OK", "BIP66 and NULLFAIL-compliant, not NULLDUMMY-compliant"],
+["1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL,NULLDUMMY", "SIG_NULLDUMMY", "BIP66 and NULLFAIL-compliant, not NULLDUMMY-compliant"],
+["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0x09 0x300602010102010101", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG", "OK", "BIP66 and NULLFAIL-compliant"],


Comments for the following few lines should be read as "BIP66-compliant but not NULLFAIL-compliant". Will fix

NicolasDorier · 2016-09-06T14:36:29Z

CodeReview ACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc

dcousens · 2016-09-11T01:01:50Z

concept ACK

btcdrak · 2016-09-12T16:13:17Z

ACK d071bfd

rubensayshi · 2016-09-13T07:55:18Z

utACK

gmaxwell · 2016-09-15T19:42:40Z

concept ACK, utACK, will review more.

afk11

tACK & code review bb50e76

CodeShark · 2016-09-22T04:46:28Z

utACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc

CodeShark · 2016-09-22T04:29:03Z

src/script/interpreter.cpp

@@ -902,6 +906,7 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                        return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);

                    int nKeysCount = CScriptNum(stacktop(-i), fRequireMinimal).getint();
+                    int ikey2 = nKeysCount + 2;


Small nit: moving this line below the next will avoid performing the addition if the range check on nKeysCount fails. Also, since this is only used for cleanup, I would suggest giving the variable a more descriptive name like nNonSigCleanupItems or adding a comment describing what it's for.

Addressed with 5d23cfe

jtimon · 2016-09-22T19:21:37Z

ut ACK 1912bf1

instagibbs

utACK 1912bf12d06011f989c0db4ae7cbf433cfff18da

I'd like to see the commits squashed though.

instagibbs · 2016-09-22T20:26:07Z

src/script/interpreter.cpp

@@ -880,6 +880,10 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                    bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode, sigversion);

                    popstack(stack);
+
+                    if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && stacktop(-1).size())


nit of nits: just check vchSig.size() instead, move before popstack

Addressed with cd102b5

instagibbs · 2016-09-22T20:47:49Z

src/script/interpreter.cpp

@@ -908,6 +912,9 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                    if (nOpCount > MAX_OPS_PER_SCRIPT)
                        return set_error(serror, SCRIPT_ERR_OP_COUNT);
                    int ikey = ++i;
+                    // ikey2 is the position of last non-signature item in the stack. Top stack item = 1.
+                    // With SCRIPT_VERIFY_NULLFAIL, this is used for cleanup if operation fails.
+                    int ikey2 = nKeysCount + 2;


This code section is a bit confusing already with all these different counters, why not set

int ikey2 = i;

right after the following line:

i += nKeysCount;

below? From there the two values diverge.

I prefer the current way as it's clearly correct. The way that i is calculated looks a bit obscure to me

Oh, I misread. Yes, the i is calculated quite oddly, but it's the value we're calculating yet again here.

CodeShark · 2016-09-23T05:32:04Z

utACK cd102b57a4c148fb93428e656e7282732c65033c

sipa

utACK

btcdrak · 2016-09-27T15:15:33Z

needs rebase

jl2012 · 2016-09-27T15:42:07Z

rebased

e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)

Github-Pull: bitcoin#8634 Rebased-From: e41bd44

…)SIG e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)

fanquake added the TX fees and policy label Aug 31, 2016

jl2012 mentioned this pull request Sep 1, 2016

Implement LOW_S and NULLDUMMY softfork (BIP146) #8533

Closed

fanquake added this to the 0.13.1 milestone Sep 2, 2016

jl2012 reviewed Sep 6, 2016
View reviewed changes

maflcko added the Needs backport label Sep 15, 2016

afk11 approved these changes Sep 17, 2016

View reviewed changes

CodeShark reviewed Sep 22, 2016

View reviewed changes

jl2012 force-pushed the nullfail branch 2 times, most recently from be29b35 to 1912bf1 Compare September 22, 2016 07:59

instagibbs suggested changes Sep 22, 2016

View reviewed changes

jl2012 force-pushed the nullfail branch from 1912bf1 to cd102b5 Compare September 23, 2016 03:12

sipa approved these changes Sep 27, 2016

View reviewed changes

Add policy: null signature for failed CHECK(MULTI)SIG

e41bd44

jl2012 force-pushed the nullfail branch from cd102b5 to e41bd44 Compare September 27, 2016 15:41

laanwj merged commit e41bd44 into bitcoin:master Sep 27, 2016

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

Merge #8634: Add policy: null signature for failed CHECK(MULTI)SIG

fc4f454

e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)

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

Add policy: null signature for failed CHECK(MULTI)SIG

3e80ab7

Github-Pull: bitcoin#8634 Rebased-From: e41bd44

laanwj removed the Needs backport label Oct 13, 2016

codablock pushed a commit to codablock/dash that referenced this pull request Jan 12, 2018

Merge bitcoin#8634: Add policy: null signature for failed CHECK(MULTI…

a75d611

…)SIG e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#8634: Add policy: null signature for failed CHECK(MULTI…

8b60b63

…)SIG e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add policy: null signature for failed CHECK(MULTI)SIG #8634

Add policy: null signature for failed CHECK(MULTI)SIG #8634

jl2012 commented Aug 31, 2016 •

edited

Loading

jl2012 commented Sep 2, 2016

btcdrak commented Sep 2, 2016

jl2012 commented Sep 6, 2016

jl2012 Sep 6, 2016

NicolasDorier commented Sep 6, 2016 •

edited

Loading

dcousens commented Sep 11, 2016

btcdrak commented Sep 12, 2016

rubensayshi commented Sep 13, 2016

gmaxwell commented Sep 15, 2016

afk11 left a comment

CodeShark commented Sep 22, 2016

CodeShark Sep 22, 2016 •

edited

Loading

jl2012 Sep 22, 2016

jtimon commented Sep 22, 2016

instagibbs left a comment

instagibbs Sep 22, 2016

jl2012 Sep 23, 2016

instagibbs Sep 22, 2016 •

edited

Loading

jl2012 Sep 23, 2016

instagibbs Sep 23, 2016 •

edited

Loading

CodeShark commented Sep 23, 2016

sipa left a comment

btcdrak commented Sep 27, 2016

jl2012 commented Sep 27, 2016

Add policy: null signature for failed CHECK(MULTI)SIG #8634

Add policy: null signature for failed CHECK(MULTI)SIG #8634

Conversation

jl2012 commented Aug 31, 2016 • edited Loading

jl2012 commented Sep 2, 2016

btcdrak commented Sep 2, 2016

jl2012 commented Sep 6, 2016

jl2012 Sep 6, 2016

Choose a reason for hiding this comment

NicolasDorier commented Sep 6, 2016 • edited Loading

dcousens commented Sep 11, 2016

btcdrak commented Sep 12, 2016

rubensayshi commented Sep 13, 2016

gmaxwell commented Sep 15, 2016

afk11 left a comment

Choose a reason for hiding this comment

CodeShark commented Sep 22, 2016

CodeShark Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

jl2012 Sep 22, 2016

Choose a reason for hiding this comment

jtimon commented Sep 22, 2016

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs Sep 22, 2016

Choose a reason for hiding this comment

jl2012 Sep 23, 2016

Choose a reason for hiding this comment

instagibbs Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

jl2012 Sep 23, 2016

Choose a reason for hiding this comment

instagibbs Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

CodeShark commented Sep 23, 2016

sipa left a comment

Choose a reason for hiding this comment

btcdrak commented Sep 27, 2016

jl2012 commented Sep 27, 2016

jl2012 commented Aug 31, 2016 •

edited

Loading

NicolasDorier commented Sep 6, 2016 •

edited

Loading

CodeShark Sep 22, 2016 •

edited

Loading

instagibbs Sep 22, 2016 •

edited

Loading

instagibbs Sep 23, 2016 •

edited

Loading