Merge pull request #5247

ca81587 Test the exact order of CHECKMULTISIG sig/pubkey evaluation (Peter Todd)
98b135f Make STRICTENC invalid pubkeys fail the script rather than the opcode. (Pieter Wuille)

7 files changed, 102 insertions, 24 deletions

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 760086eaba..5eda23731d 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -207,9 +207,9 @@ bool static CheckSignatureEncoding(const valtype &vchSig, unsigned int flags, Sc
     return true;
 }
 
-bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags) {
+bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags, ScriptError* serror) {
     if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsCompressedOrUncompressedPubKey(vchSig)) {
-        return false;
+        return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);
     }
     return true;
 }
@@ -792,11 +792,11 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                     // Drop the signature, since there's no way for a signature to sign itself
                     scriptCode.FindAndDelete(CScript(vchSig));
 
-                    if (!CheckSignatureEncoding(vchSig, flags, serror)) {
+                    if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
                         //serror is set
                         return false;
                     }
-                    bool fSuccess = CheckPubKeyEncoding(vchPubKey, flags) && checker.CheckSig(vchSig, vchPubKey, scriptCode);
+                    bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode);
 
                     popstack(stack);
                     popstack(stack);
@@ -855,13 +855,16 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                         valtype& vchSig    = stacktop(-isig);
                         valtype& vchPubKey = stacktop(-ikey);
 
-                        if (!CheckSignatureEncoding(vchSig, flags, serror)) {
+                        // Note how this makes the exact order of pubkey/signature evaluation
+                        // distinguishable by CHECKMULTISIG NOT if the STRICTENC flag is set.
+                        // See the script_(in)valid tests for details.
+                        if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
                             // serror is set
                             return false;
                         }
 
                         // Check signature
-                        bool fOk = CheckPubKeyEncoding(vchPubKey, flags) && checker.CheckSig(vchSig, vchPubKey, scriptCode);
+                        bool fOk = checker.CheckSig(vchSig, vchPubKey, scriptCode);
 
                         if (fOk) {
                             isig++;
@@ -871,7 +874,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
                         nKeysCount--;
 
                         // If there are more signatures left than keys left,
-                        // then too many signatures have failed
+                        // then too many signatures have failed. Exit early,
+                        // without checking any further signatures.
                         if (nSigsCount > nKeysCount)
                             fSuccess = false;
                     }
diff --git a/src/script/interpreter.h b/src/script/interpreter.h
index 12b2719414..35b2f6c65a 100644
--- a/src/script/interpreter.h
+++ b/src/script/interpreter.h
@@ -35,8 +35,8 @@ enum
     SCRIPT_VERIFY_P2SH      = (1U << 0),
 
     // Passing a non-strict-DER signature or one with undefined hashtype to a checksig operation causes script failure.
-    // Passing a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) to checksig causes that pubkey to be
-    // skipped (not softfork safe: this flag can widen the validity of OP_CHECKSIG OP_NOT).
+    // Evaluating a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) by checksig causes script failure.
+    // (softfork safe, but not used or intended as a consensus rule).
     SCRIPT_VERIFY_STRICTENC = (1U << 1),
 
     // Passing a non-strict-DER signature to a checksig operation causes script failure (softfork safe, BIP62 rule 1)
diff --git a/src/script/script_error.cpp b/src/script/script_error.cpp
index 793fc0da41..5d24ed98ba 100644
--- a/src/script/script_error.cpp
+++ b/src/script/script_error.cpp
@@ -61,6 +61,8 @@ const char* ScriptErrorString(const ScriptError serror)
             return "Dummy CHECKMULTISIG argument must be zero";
         case SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS:
             return "NOPx reserved for soft-fork upgrades";
+        case SCRIPT_ERR_PUBKEYTYPE:
+            return "Public key is neither compressed or uncompressed";
         case SCRIPT_ERR_UNKNOWN_ERROR:
         case SCRIPT_ERR_ERROR_COUNT:
         default: break;
diff --git a/src/script/script_error.h b/src/script/script_error.h
index 21153f1bd1..ac1f2deae5 100644
--- a/src/script/script_error.h
+++ b/src/script/script_error.h
@@ -42,6 +42,7 @@ typedef enum ScriptError_t
     SCRIPT_ERR_SIG_PUSHONLY,
     SCRIPT_ERR_SIG_HIGH_S,
     SCRIPT_ERR_SIG_NULLDUMMY,
+    SCRIPT_ERR_PUBKEYTYPE,
 
     /* softfork safeness */
     SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS,
diff --git a/src/test/data/script_invalid.json b/src/test/data/script_invalid.json
index 0356d0be1c..71e757714c 100644
--- a/src/test/data/script_invalid.json
+++ b/src/test/data/script_invalid.json
@@ -593,11 +593,48 @@ nSequences are max.
     "P2PK NOT with hybrid pubkey but no STRICTENC"
 ],
 [
+    "0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
+    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
+    "STRICTENC",
+    "P2PK NOT with hybrid pubkey"
+],
+[
+    "0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
+    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
+    "STRICTENC",
+    "P2PK NOT with invalid hybrid pubkey"
+],
+[
+    "0 0x47 0x304402206797289d3dc81692edae58430276d04641ea5d86967be557163f8494da32fd78022006fc6ab77aaed4ac11ea69cd878ab26e3e24290f47a43e9adf34075d52b7142c01",
+    "1 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 2 CHECKMULTISIG",
+    "STRICTENC",
+    "1-of-2 with the first 1 hybrid pubkey"
+],
+[
     "0x47 0x304402201f82b99a813c9c48c8dee8d2c43b8f637b72353fe9bdcc084537bc17e2ab770402200c43b96a5f7e115f0114eabda32e068145965cb6c7b5ef64833bb4fcf9fc1b3b05",
     "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",
     "STRICTENC",
     "P2PK with undefined hashtype"
 ],
+
+["
+Order of CHECKMULTISIG evaluation tests, inverted by swapping the order of
+pubkeys/signatures so they fail due to the STRICTENC rules on validly encoded
+signatures and pubkeys.
+"],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "2-of-2 CHECKMULTISIG NOT with the first pubkey invalid, and both signatures validly encoded."
+],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but first signature invalid."
+],
+
 [
     "0x47 0x30440220166848cd5b82a32b5944d90de3c35249354b43773c2ece1844ee8d1103e2f6c602203b6b046da4243c77adef80ada9201b27bbfdf7f9d5428f40434b060432afd62005",
     "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG NOT",
diff --git a/src/test/data/script_valid.json b/src/test/data/script_valid.json
index 0cf1563163..ada45a64ed 100644
--- a/src/test/data/script_valid.json
+++ b/src/test/data/script_valid.json
@@ -744,24 +744,49 @@ nSequences are max.
     "P2PK with hybrid pubkey but no STRICTENC"
 ],
 [
-    "0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
-    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
-    "STRICTENC",
-    "P2PK NOT with hybrid pubkey"
-],
-[
     "0x47 0x3044022078d6c447887e88dcbe1bc5b613645280df6f4e5935648bc226e9d91da71b3216022047d6b7ef0949b228fc1b359afb8d50500268711354298217b983c26970790c7601",
     "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
     "",
     "P2PK NOT with invalid hybrid pubkey but no STRICTENC"
 ],
 [
-    "0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
-    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
+    "0 0x47 0x304402203b269b9fbc0936877bf855b5fb41757218d9548b246370d991442a5f5bd1c3440220235268a4eaa8c67e543c6e37da81dd36d3b1be2de6b4fef04113389ca6ddc04501",
+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
+    "",
+    "1-of-2 with the second 1 hybrid pubkey and no STRICTENC"
+],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
+    "STRICTENC",
+    "1-of-2 with the second 1 hybrid pubkey"
+],
+
+["
+CHECKMULTISIG evaluation order tests. CHECKMULTISIG evaluates signatures and
+pubkeys in a specific order, and will exit early if the number of signatures
+left to check is greater than the number of keys left. As STRICTENC fails the
+script when it reaches an invalidly encoded signature or pubkey, we can use it
+to test the exact order in which signatures and pubkeys are evaluated by
+distinguishing CHECKMULTISIG returning false on the stack and the script as a
+whole failing.
+
+See also the corresponding inverted versions of these tests in script_invalid.json
+"],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
     "STRICTENC",
-    "P2PK NOT with invalid hybrid pubkey"
+    "2-of-2 CHECKMULTISIG NOT with the second pubkey invalid, and both signatures validly encoded. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid pubkey."
 ],
 [
+    "0 0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but second signature invalid. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid signature."
+],
+
+[
     "0x47 0x304402204649e9517ef0377a8f8270bd423053fd98ddff62d74ea553e9579558abbb75e4022044a2b2344469c12e35ed898987711272b634733dd0f5e051288eceb04bd4669e05",
     "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",
     "",
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index 36aaa6903f..6952f4c584 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -428,15 +428,24 @@ BOOST_AUTO_TEST_CASE(script_build)
     bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
                               "P2PK NOT with hybrid pubkey but no STRICTENC", 0
                              ).PushSig(keys.key0, SIGHASH_ALL));
-    good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
-                               "P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
-                              ).PushSig(keys.key0, SIGHASH_ALL));
+    bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
+                              "P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).PushSig(keys.key0, SIGHASH_ALL));
     good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
                                "P2PK NOT with invalid hybrid pubkey but no STRICTENC", 0
                               ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
-    good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
-                               "P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
-                              ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
+    bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
+                              "P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
+    good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
+                               "1-of-2 with the second 1 hybrid pubkey and no STRICTENC", 0
+                              ).Num(0).PushSig(keys.key1, SIGHASH_ALL));
+    good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
+                               "1-of-2 with the second 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                              ).Num(0).PushSig(keys.key1, SIGHASH_ALL));
+    bad.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey0H) << OP_2 << OP_CHECKMULTISIG,
+                              "1-of-2 with the first 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).Num(0).PushSig(keys.key1, SIGHASH_ALL));
 
     good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey1) << OP_CHECKSIG,
                                "P2PK with undefined hashtype but no STRICTENC", 0