Merge #13491: Improve handling of INVALID in IsMine

bb582a59c Add P2WSH destination helper and use it instead of manual hashing (Pieter Wuille)
eaba1c111 Add additional unit tests for invalid IsMine combinations (Pieter Wuille)
e6b9730c4 Do not expose invalidity from IsMine (Pieter Wuille)

Pull request description:

  This improves the handling of INVALID in IsMine:
  * Extra INVALID conditions were added to `IsMine` (following https://github.com/bitcoin/bitcoin/pull/13142/files#r185349057), but these were untested. Add unit tests for them.
  * In https://github.com/bitcoin/bitcoin/pull/13142#issuecomment-386396975 it was suggested to merge `isInvalid` into the return status. This PR takes a different approach, and removes the `isInvalid` entirely. It was only ever used inside tests, as normal users of IsMine don't care about the reason for non-mine-ness, only whether it is or not. As the unit tests are extensive enough, it seems sufficient to have a black box text (with tests for both compressed and uncompressed keys).

  Some addition code simplification is done as well.

Tree-SHA512: 3267f8846f3fa4e994f57504b155b0e1bbdf13808c4c04dab7c6886c2c0b88716169cee9c5b350513297e0ca2a00812e3401acf30ac9cde5d892f9fb59ad7fef

4 files changed, 10 insertions, 19 deletions

diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp
index 43dd9e582..8c2686648 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 a15768aec..4246da49f 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/script/standard.cpp b/src/script/standard.cpp
index d9269d614..f0b2c62a9 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -5,6 +5,7 @@
 
 #include <script/standard.h>
 
+#include <crypto/sha256.h>
 #include <pubkey.h>
 #include <script/script.h>
 #include <util.h>
@@ -18,6 +19,11 @@ unsigned nMaxDatacarrierBytes = MAX_OP_RETURN_RELAY;
 
 CScriptID::CScriptID(const CScript& in) : uint160(Hash160(in.begin(), in.end())) {}
 
+WitnessV0ScriptHash::WitnessV0ScriptHash(const CScript& in)
+{
+    CSHA256().Write(in.data(), in.size()).Finalize(begin());
+}
+
 const char* GetTxnOutputType(txnouttype t)
 {
     switch (t)
@@ -329,9 +335,7 @@ CScript GetScriptForWitness(const CScript& redeemscript)
             return GetScriptForDestination(WitnessV0KeyHash(vSolutions[0]));
         }
     }
-    uint256 hash;
-    CSHA256().Write(&redeemscript[0], redeemscript.size()).Finalize(hash.begin());
-    return GetScriptForDestination(WitnessV0ScriptHash(hash));
+    return GetScriptForDestination(WitnessV0ScriptHash(redeemscript));
 }
 
 bool IsValidDestination(const CTxDestination& dest) {
diff --git a/src/script/standard.h b/src/script/standard.h
index 4922b7236..138003087 100644
--- a/src/script/standard.h
+++ b/src/script/standard.h
@@ -77,6 +77,7 @@ struct WitnessV0ScriptHash : public uint256
 {
     WitnessV0ScriptHash() : uint256() {}
     explicit WitnessV0ScriptHash(const uint256& hash) : uint256(hash) {}
+    explicit WitnessV0ScriptHash(const CScript& script);
     using uint256::uint256;
 };