From e6b9730c49da6a0219453dec8f4df351292e6e07 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 17 Jun 2018 16:28:04 -0700 Subject: [PATCH 1/3] Do not expose invalidity from IsMine --- src/script/ismine.cpp | 12 +--- src/script/ismine.h | 6 -- src/test/script_standard_tests.cpp | 100 ++++++++++------------------- 3 files changed, 35 insertions(+), 83 deletions(-) diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp index 43dd9e582e8..8c26866483d 100644 --- a/src/script/ismine.cpp +++ b/src/script/ismine.cpp @@ -38,7 +38,7 @@ enum class IsMineResult NO = 0, //! Not ours WATCH_ONLY = 1, //! Included in watch-only balance SPENDABLE = 2, //! Included in all balances - INVALID = 3, //! Not spendable by anyone + INVALID = 3, //! Not spendable by anyone (uncompressed pubkey in segwit, P2SH inside P2SH or witness, witness inside witness) }; bool PermitsUncompressed(IsMineSigVersion sigversion) @@ -173,12 +173,10 @@ IsMineResult IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, } // namespace -isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid) +isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey) { - isInvalid = false; switch (IsMineInner(keystore, scriptPubKey, IsMineSigVersion::TOP)) { case IsMineResult::INVALID: - isInvalid = true; case IsMineResult::NO: return ISMINE_NO; case IsMineResult::WATCH_ONLY: @@ -189,12 +187,6 @@ isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& assert(false); } -isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey) -{ - bool isInvalid = false; - return IsMine(keystore, scriptPubKey, isInvalid); -} - isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest) { CScript script = GetScriptForDestination(dest); diff --git a/src/script/ismine.h b/src/script/ismine.h index a15768aecb8..4246da49fe3 100644 --- a/src/script/ismine.h +++ b/src/script/ismine.h @@ -24,12 +24,6 @@ enum isminetype /** used for bitflags of isminetype */ typedef uint8_t isminefilter; -/* isInvalid becomes true when the script is found invalid by consensus or policy. This will terminate the recursion - * and return ISMINE_NO immediately, as an invalid script should never be considered as "mine". This is needed as - * different SIGVERSION may have different network rules. Currently the only use of isInvalid is indicate uncompressed - * keys in SigVersion::WITNESS_V0 script, but could also be used in similar cases in the future - */ -isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid); isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey); isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest); diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index 7ab09782289..ec4eb34b8ae 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -398,7 +398,6 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) CScript scriptPubKey; isminetype result; - bool isInvalid; // P2PK compressed { @@ -407,15 +406,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << ToByteVector(pubkeys[0]) << OP_CHECKSIG; // Keystore does not have key - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has key keystore.AddKey(keys[0]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2PK uncompressed @@ -425,15 +422,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << ToByteVector(uncompressedPubkey) << OP_CHECKSIG; // Keystore does not have key - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has key keystore.AddKey(uncompressedKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2PKH compressed @@ -443,15 +438,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; // Keystore does not have key - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has key keystore.AddKey(keys[0]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2PKH uncompressed @@ -461,15 +454,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_DUP << OP_HASH160 << ToByteVector(uncompressedPubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; // Keystore does not have key - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has key keystore.AddKey(uncompressedKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2SH @@ -483,21 +474,18 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_HASH160 << ToByteVector(CScriptID(redeemScript)) << OP_EQUAL; // Keystore does not have redeemScript or key - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has redeemScript but no key keystore.AddCScript(redeemScript); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has redeemScript and key keystore.AddKey(keys[0]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2WPKH compressed @@ -510,9 +498,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) // Keystore implicitly has key and P2SH redeemScript keystore.AddCScript(scriptPubKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2WPKH uncompressed @@ -524,15 +511,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_0 << ToByteVector(uncompressedPubkey.GetID()); // Keystore has key, but no P2SH redeemScript - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has key and P2SH redeemScript keystore.AddCScript(scriptPubKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(isInvalid); } // scriptPubKey multisig @@ -546,30 +531,26 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) OP_2 << OP_CHECKMULTISIG; // Keystore does not have any keys - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has 1/2 keys keystore.AddKey(uncompressedKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has 2/2 keys keystore.AddKey(keys[1]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has 2/2 keys and the script keystore.AddCScript(scriptPubKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); } // P2SH multisig @@ -588,15 +569,13 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_HASH160 << ToByteVector(CScriptID(redeemScript)) << OP_EQUAL; // Keystore has no redeemScript - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has redeemScript keystore.AddCScript(redeemScript); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2WSH multisig with compressed keys @@ -619,21 +598,18 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_0 << ToByteVector(scriptHash); // Keystore has keys, but no witnessScript or P2SH redeemScript - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has keys and witnessScript, but no P2SH redeemScript keystore.AddCScript(witnessScript); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has keys, witnessScript, P2SH redeemScript keystore.AddCScript(scriptPubKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // P2WSH multisig with uncompressed key @@ -656,21 +632,18 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_0 << ToByteVector(scriptHash); // Keystore has keys, but no witnessScript or P2SH redeemScript - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has keys and witnessScript, but no P2SH redeemScript keystore.AddCScript(witnessScript); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has keys, witnessScript, P2SH redeemScript keystore.AddCScript(scriptPubKey); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(isInvalid); } // P2WSH multisig wrapped in P2SH @@ -694,23 +667,20 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey << OP_HASH160 << ToByteVector(CScriptID(redeemScript)) << OP_EQUAL; // Keystore has no witnessScript, P2SH redeemScript, or keys - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has witnessScript and P2SH redeemScript, but no keys keystore.AddCScript(redeemScript); keystore.AddCScript(witnessScript); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); // Keystore has keys, witnessScript, P2SH redeemScript keystore.AddKey(keys[0]); keystore.AddKey(keys[1]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); - BOOST_CHECK(!isInvalid); } // OP_RETURN @@ -721,9 +691,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey.clear(); scriptPubKey << OP_RETURN << ToByteVector(pubkeys[0]); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); } // witness unspendable @@ -734,9 +703,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey.clear(); scriptPubKey << OP_0 << ToByteVector(ParseHex("aabb")); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); } // witness unknown @@ -747,9 +715,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey.clear(); scriptPubKey << OP_16 << ToByteVector(ParseHex("aabb")); - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); } // Nonstandard @@ -760,9 +727,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) scriptPubKey.clear(); scriptPubKey << OP_9 << OP_ADD << OP_11 << OP_EQUAL; - result = IsMine(keystore, scriptPubKey, isInvalid); + result = IsMine(keystore, scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); - BOOST_CHECK(!isInvalid); } } From eaba1c111e9671cdd6faf4b96c39341bbdd41632 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 17 Jun 2018 17:39:42 -0700 Subject: [PATCH 2/3] Add additional unit tests for invalid IsMine combinations --- src/test/script_standard_tests.cpp | 82 ++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index ec4eb34b8ae..f319ea831a8 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -488,6 +488,88 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine) BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE); } + // (P2PKH inside) P2SH inside P2SH (invalid) + { + CBasicKeyStore keystore; + + CScript redeemscript, redeemscript_inner; + redeemscript_inner << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; + redeemscript << OP_HASH160 << ToByteVector(CScriptID(redeemscript_inner)) << OP_EQUAL; + + scriptPubKey.clear(); + scriptPubKey << OP_HASH160 << ToByteVector(CScriptID(redeemscript)) << OP_EQUAL; + + keystore.AddCScript(redeemscript); + keystore.AddCScript(redeemscript_inner); + keystore.AddCScript(scriptPubKey); + keystore.AddKey(keys[0]); + result = IsMine(keystore, scriptPubKey); + BOOST_CHECK_EQUAL(result, ISMINE_NO); + } + + // (P2PKH inside) P2SH inside P2WSH (invalid) + { + CBasicKeyStore keystore; + + CScript witnessscript, redeemscript; + redeemscript << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; + witnessscript << OP_HASH160 << ToByteVector(CScriptID(redeemscript)) << OP_EQUAL; + + uint256 scripthash; + CSHA256().Write(witnessscript.data(), witnessscript.size()).Finalize(scripthash.begin()); + scriptPubKey.clear(); + scriptPubKey << OP_0 << ToByteVector(scripthash); + + keystore.AddCScript(witnessscript); + keystore.AddCScript(redeemscript); + keystore.AddCScript(scriptPubKey); + keystore.AddKey(keys[0]); + result = IsMine(keystore, scriptPubKey); + BOOST_CHECK_EQUAL(result, ISMINE_NO); + } + + // P2WPKH inside P2WSH (invalid) + { + CBasicKeyStore keystore; + + CScript witnessscript; + witnessscript << OP_0 << ToByteVector(pubkeys[0].GetID()); + + scriptPubKey.clear(); + uint256 scripthash; + CSHA256().Write(witnessscript.data(), witnessscript.size()).Finalize(scripthash.begin()); + scriptPubKey << OP_0 << ToByteVector(scripthash); + + keystore.AddCScript(witnessscript); + keystore.AddCScript(scriptPubKey); + keystore.AddKey(keys[0]); + result = IsMine(keystore, scriptPubKey); + BOOST_CHECK_EQUAL(result, ISMINE_NO); + } + + // (P2PKH inside) P2WSH inside P2WSH (invalid) + { + CBasicKeyStore keystore; + + CScript witnessscript_inner; + witnessscript_inner << OP_DUP << OP_HASH160 << ToByteVector(pubkeys[0].GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; + uint256 scripthash; + CSHA256().Write(witnessscript_inner.data(), witnessscript_inner.size()).Finalize(scripthash.begin()); + CScript witnessscript; + witnessscript << OP_0 << ToByteVector(scripthash); + + scriptPubKey.clear(); + CSHA256().Write(witnessscript.data(), witnessscript.size()).Finalize(scripthash.begin()); + scriptPubKey << OP_0 << ToByteVector(scripthash); + + keystore.AddCScript(witnessscript_inner); + keystore.AddCScript(witnessscript); + keystore.AddCScript(scriptPubKey); + keystore.AddKey(keys[0]); + result = IsMine(keystore, scriptPubKey); + BOOST_CHECK_EQUAL(result, ISMINE_NO); + } + // P2WPKH compressed { CBasicKeyStore keystore; From bb582a59c7532b0e4f647d9dfe50f0d816e81427 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 17 Jun 2018 19:44:50 -0700 Subject: [PATCH 3/3] Add P2WSH destination helper and use it instead of manual hashing --- src/rpc/rawtransaction.cpp | 4 +- src/script/standard.cpp | 10 ++- src/script/standard.h | 1 + src/test/script_standard_tests.cpp | 129 +++++++---------------------- src/wallet/wallet.cpp | 4 +- 5 files changed, 39 insertions(+), 109 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 3b3f43edea7..e7531734dca 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -637,9 +637,7 @@ static UniValue decodescript(const JSONRPCRequest& request) } else { // Scripts that are not fit for P2WPKH are encoded as P2WSH. // Newer segwit program versions should be considered when then become available. - uint256 scriptHash; - CSHA256().Write(script.data(), script.size()).Finalize(scriptHash.begin()); - segwitScr = GetScriptForDestination(WitnessV0ScriptHash(scriptHash)); + segwitScr = GetScriptForDestination(WitnessV0ScriptHash(script)); } ScriptPubKeyToUniv(segwitScr, sr, true); sr.pushKV("p2sh-segwit", EncodeDestination(CScriptID(segwitScr))); diff --git a/src/script/standard.cpp b/src/script/standard.cpp index d9269d6147a..f0b2c62a910 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -5,6 +5,7 @@ #include