Merge #13142: Separate IsMine from solvability

c004ffc9b4 Make handling of invalid in IsMine more uniform (Pieter Wuille)
a53f0feff8 Add some checks for invalid recursion in IsMine (Pieter Wuille)
b5802a9f5f Simplify IsMine logic (Pieter Wuille)
4e91820531 Make IsMine stop distinguishing solvable/unsolvable (Pieter Wuille)
6d714c3419 Make coincontrol use IsSolvable to determine solvability (Pieter Wuille)

Pull request description:

  Our current `IsMine` logic does several things with outputs:
  * Determine "spendability" (roughly corresponding to "could we sign for this")
  * Determine "watching" (is this an output directly or indirectly a watched script)
  * Determine invalidity (is this output definitely not legally spendable, detecting accidental uncompressed pubkeys in witnesses)
  * Determine "solvability" (would we be able to sign for this ignoring the fact that we may be missing some private keys).

  The last item (solvability) is mostly unrelated and only rarely needed (there is just one instance, inside the wallet's coin control logic). This PR changes that instance to use the separate `IsSolvable` function, and stop `IsMine` from distinguishing between solvable and unsolvable.

  As an extra, this also simplifies the `IsMine` logic and adds some extra checks (which wouldn't be hit unless someone adds already invalid scripts to their wallet).

Tree-SHA512: 95a6ef75fbf2eedc5ed938c48a8e5d77dcf09c933372acdd0333129fb7301994a78498f9aacce2c8db74275e19260549dd67a83738e187d40b5090cc04f33adf

src/script/ismine.cpp

@@ -28,6 +28,19 @@ enum class IsMineSigVersion
     WITNESS_V0 = 2  //! P2WSH witness script execution
 };
 
+/**
+ * This is an internal representation of isminetype + invalidity.
+ * Its order is significant, as we return the max of all explored
+ * possibilities.
+ */
+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
+};
+
 bool PermitsUncompressed(IsMineSigVersion sigversion)
 {
     return sigversion == IsMineSigVersion::TOP || sigversion == IsMineSigVersion::P2SH;
@@ -42,17 +55,13 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keystore)
     return true;
 }
 
-isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion)
+IsMineResult IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion)
 {
-    isInvalid = false;
+    IsMineResult ret = IsMineResult::NO;
 
     std::vector<valtype> vSolutions;
     txnouttype whichType;
-    if (!Solver(scriptPubKey, whichType, vSolutions)) {
+    Solver(scriptPubKey, whichType, vSolutions);
-        if (keystore.HaveWatchOnly(scriptPubKey))
-            return ISMINE_WATCH_UNSOLVABLE;
-        return ISMINE_NO;
-    }
 
     CKeyID keyID;
     switch (whichType)
@@ -64,23 +73,25 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
     case TX_PUBKEY:
         keyID = CPubKey(vSolutions[0]).GetID();
         if (!PermitsUncompressed(sigversion) && vSolutions[0].size() != 33) {
-            isInvalid = true;
+            return IsMineResult::INVALID;
-            return ISMINE_NO;
+        }
+        if (keystore.HaveKey(keyID)) {
+            ret = std::max(ret, IsMineResult::SPENDABLE);
         }
-        if (keystore.HaveKey(keyID))
-            return ISMINE_SPENDABLE;
         break;
     case TX_WITNESS_V0_KEYHASH:
     {
+        if (sigversion == IsMineSigVersion::WITNESS_V0) {
+            // P2WPKH inside P2WSH is invalid.
+            return IsMineResult::INVALID;
+        }
         if (sigversion == IsMineSigVersion::TOP && !keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
             // We do not support bare witness outputs unless the P2SH version of it would be
             // acceptable as well. This protects against matching before segwit activates.
             // This also applies to the P2WSH case.
             break;
         }
-        isminetype ret = IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, IsMineSigVersion::WITNESS_V0);
+        ret = std::max(ret, IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), IsMineSigVersion::WITNESS_V0));
-        if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
-            return ret;
         break;
     }
     case TX_PUBKEYHASH:
@@ -88,26 +99,32 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
         if (!PermitsUncompressed(sigversion)) {
             CPubKey pubkey;
             if (keystore.GetPubKey(keyID, pubkey) && !pubkey.IsCompressed()) {
-                isInvalid = true;
+                return IsMineResult::INVALID;
-                return ISMINE_NO;
             }
         }
-        if (keystore.HaveKey(keyID))
+        if (keystore.HaveKey(keyID)) {
-            return ISMINE_SPENDABLE;
+            ret = std::max(ret, IsMineResult::SPENDABLE);
+        }
         break;
     case TX_SCRIPTHASH:
     {
+        if (sigversion != IsMineSigVersion::TOP) {
+            // P2SH inside P2WSH or P2SH is invalid.
+            return IsMineResult::INVALID;
+        }
         CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
         CScript subscript;
         if (keystore.GetCScript(scriptID, subscript)) {
-            isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH);
+            ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH));
-            if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
-                return ret;
         }
         break;
     }
     case TX_WITNESS_V0_SCRIPTHASH:
     {
+        if (sigversion == IsMineSigVersion::WITNESS_V0) {
+            // P2WSH inside P2WSH is invalid.
+            return IsMineResult::INVALID;
+        }
         if (sigversion == IsMineSigVersion::TOP && !keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
             break;
         }
@@ -116,9 +133,7 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
         CScriptID scriptID = CScriptID(hash);
         CScript subscript;
         if (keystore.GetCScript(scriptID, subscript)) {
-            isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::WITNESS_V0);
+            ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0));
-            if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
-                return ret;
         }
         break;
     }
@@ -126,7 +141,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
     case TX_MULTISIG:
     {
         // Never treat bare multisig outputs as ours (they can still be made watchonly-though)
-        if (sigversion == IsMineSigVersion::TOP) break;
+        if (sigversion == IsMineSigVersion::TOP) {
+            break;
+        }
 
         // Only consider transactions "mine" if we own ALL the
         // keys involved. Multi-signature transactions that are
@@ -137,30 +154,39 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b
         if (!PermitsUncompressed(sigversion)) {
             for (size_t i = 0; i < keys.size(); i++) {
                 if (keys[i].size() != 33) {
-                    isInvalid = true;
+                    return IsMineResult::INVALID;
-                    return ISMINE_NO;
                 }
             }
         }
-        if (HaveKeys(keys, keystore))
+        if (HaveKeys(keys, keystore)) {
-            return ISMINE_SPENDABLE;
+            ret = std::max(ret, IsMineResult::SPENDABLE);
+        }
         break;
     }
     }
 
-    if (keystore.HaveWatchOnly(scriptPubKey)) {
+    if (ret == IsMineResult::NO && keystore.HaveWatchOnly(scriptPubKey)) {
-        // TODO: This could be optimized some by doing some work after the above solver
+        ret = std::max(ret, IsMineResult::WATCH_ONLY);
-        SignatureData sigs;
-        return ProduceSignature(keystore, DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigs) ? ISMINE_WATCH_SOLVABLE : ISMINE_WATCH_UNSOLVABLE;
     }
-    return ISMINE_NO;
+    return ret;
 }
 
 } // namespace
 
 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid)
 {
-    return IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP);
+    isInvalid = false;
+    switch (IsMineInner(keystore, scriptPubKey, IsMineSigVersion::TOP)) {
+    case IsMineResult::INVALID:
+        isInvalid = true;
+    case IsMineResult::NO:
+        return ISMINE_NO;
+    case IsMineResult::WATCH_ONLY:
+        return ISMINE_WATCH_ONLY;
+    case IsMineResult::SPENDABLE:
+        return ISMINE_SPENDABLE;
+    }
+    assert(false);
 }
 
 isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey)

src/script/ismine.h

@@ -17,12 +17,8 @@ class CScript;
 enum isminetype
 {
     ISMINE_NO = 0,
-    //! Indicates that we don't know how to create a scriptSig that would solve this if we were given the appropriate private keys
+    ISMINE_WATCH_ONLY = 1,
-    ISMINE_WATCH_UNSOLVABLE = 1,
+    ISMINE_SPENDABLE = 2,
-    //! Indicates that we know how to create a scriptSig that would solve this if we were given the appropriate private keys
-    ISMINE_WATCH_SOLVABLE = 2,
-    ISMINE_WATCH_ONLY = ISMINE_WATCH_SOLVABLE | ISMINE_WATCH_UNSOLVABLE,
-    ISMINE_SPENDABLE = 4,
     ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE
 };
 /** used for bitflags of isminetype */

src/wallet/coincontrol.h

@@ -22,7 +22,7 @@ public:
     boost::optional<OutputType> m_change_type;
     //! If false, allows unselected inputs, but requires all selected inputs be used
     bool fAllowOtherInputs;
-    //! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
+    //! Includes watch only addresses which are solvable
     bool fAllowWatchOnly;
     //! Override automatic min/max checks on fee, m_feerate must be set if true
     bool fOverrideFeeRate;

src/wallet/wallet.cpp

@@ -2376,10 +2376,10 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
                 continue;
             }
 
-            bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO);
+            bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey);
-            bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO;
+            bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
 
-            vCoins.push_back(COutput(pcoin, i, nDepth, fSpendableIn, fSolvableIn, safeTx));
+            vCoins.push_back(COutput(pcoin, i, nDepth, spendable, solvable, safeTx));
 
             // Checks the sum amount of all UTXO's.
             if (nMinimumSumAmount != MAX_MONEY) {