From 6d714c3419b368671bd071a8992950c3dc00e613 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 1 May 2018 12:05:55 -0700 Subject: [PATCH 1/5] Make coincontrol use IsSolvable to determine solvability --- src/wallet/coincontrol.h | 2 +- src/wallet/wallet.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 2f08162ee49..98b4298507f 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -22,7 +22,7 @@ public: boost::optional 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; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6e0f49f136e..ab351871de3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2363,10 +2363,10 @@ void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const continue; } - bool fSpendableIn = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (coinControl && coinControl->fAllowWatchOnly && (mine & ISMINE_WATCH_SOLVABLE) != ISMINE_NO); - bool fSolvableIn = (mine & (ISMINE_SPENDABLE | ISMINE_WATCH_SOLVABLE)) != ISMINE_NO; + bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey); + 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) { From 4e91820531889e309dc4335fe0de8229c6426040 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 1 May 2018 12:19:47 -0700 Subject: [PATCH 2/5] Make IsMine stop distinguishing solvable/unsolvable --- src/script/ismine.cpp | 22 ++++++++++++---------- src/script/ismine.h | 8 ++------ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp index fefa02fdef1..043756829bc 100644 --- a/src/script/ismine.cpp +++ b/src/script/ismine.cpp @@ -49,9 +49,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b std::vector vSolutions; txnouttype whichType; if (!Solver(scriptPubKey, whichType, vSolutions)) { - if (keystore.HaveWatchOnly(scriptPubKey)) - return ISMINE_WATCH_UNSOLVABLE; - return ISMINE_NO; + if (keystore.HaveWatchOnly(scriptPubKey)) { + return ISMINE_WATCH_ONLY; + } } CKeyID keyID; @@ -79,8 +79,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b break; } isminetype ret = IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, IsMineSigVersion::WITNESS_V0); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid)) + if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { return ret; + } break; } case TX_PUBKEYHASH: @@ -101,8 +102,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid)) + if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { return ret; + } } break; } @@ -117,8 +119,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::WITNESS_V0); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid)) + if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { return ret; + } } break; } @@ -142,16 +145,15 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b } } } - if (HaveKeys(keys, keystore)) + if (HaveKeys(keys, keystore)) { return ISMINE_SPENDABLE; + } break; } } if (keystore.HaveWatchOnly(scriptPubKey)) { - // TODO: This could be optimized some by doing some work after the above solver - SignatureData sigs; - return ProduceSignature(keystore, DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigs) ? ISMINE_WATCH_SOLVABLE : ISMINE_WATCH_UNSOLVABLE; + return ISMINE_WATCH_ONLY; } return ISMINE_NO; } diff --git a/src/script/ismine.h b/src/script/ismine.h index 8573bdfbd24..a15768aecb8 100644 --- a/src/script/ismine.h +++ b/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_UNSOLVABLE = 1, - //! 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_WATCH_ONLY = 1, + ISMINE_SPENDABLE = 2, ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE }; /** used for bitflags of isminetype */ From b5802a9f5f69815d3290361fd8c96d76a037832f Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 1 May 2018 12:51:30 -0700 Subject: [PATCH 3/5] Simplify IsMine logic --- src/script/ismine.cpp | 54 ++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp index 043756829bc..bebaf9ea827 100644 --- a/src/script/ismine.cpp +++ b/src/script/ismine.cpp @@ -42,17 +42,20 @@ bool HaveKeys(const std::vector& pubkeys, const CKeyStore& keystore) return true; } +void Update(isminetype& val, isminetype update) +{ + if (val == ISMINE_NO) val = update; + if (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE) val = update; +} + isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion) { + isminetype ret = ISMINE_NO; isInvalid = false; std::vector vSolutions; txnouttype whichType; - if (!Solver(scriptPubKey, whichType, vSolutions)) { - if (keystore.HaveWatchOnly(scriptPubKey)) { - return ISMINE_WATCH_ONLY; - } - } + Solver(scriptPubKey, whichType, vSolutions); CKeyID keyID; switch (whichType) @@ -67,8 +70,9 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b isInvalid = true; return ISMINE_NO; } - if (keystore.HaveKey(keyID)) - return ISMINE_SPENDABLE; + if (keystore.HaveKey(keyID)) { + Update(ret, ISMINE_SPENDABLE); + } break; case TX_WITNESS_V0_KEYHASH: { @@ -78,10 +82,7 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b // This also applies to the P2WSH case. break; } - isminetype ret = IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, IsMineSigVersion::WITNESS_V0); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { - return ret; - } + Update(ret, IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, IsMineSigVersion::WITNESS_V0)); break; } case TX_PUBKEYHASH: @@ -93,18 +94,16 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b return ISMINE_NO; } } - if (keystore.HaveKey(keyID)) - return ISMINE_SPENDABLE; + if (keystore.HaveKey(keyID)) { + Update(ret, ISMINE_SPENDABLE); + } break; case TX_SCRIPTHASH: { CScriptID scriptID = CScriptID(uint160(vSolutions[0])); CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { - isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { - return ret; - } + Update(ret, IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH)); } break; } @@ -118,10 +117,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); - if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_ONLY || (ret == ISMINE_NO && isInvalid)) { - return ret; - } + Update(ret, IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::WITNESS_V0)); } break; } @@ -129,7 +125,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 @@ -146,23 +144,27 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b } } if (HaveKeys(keys, keystore)) { - return ISMINE_SPENDABLE; + Update(ret, ISMINE_SPENDABLE); } break; } } - if (keystore.HaveWatchOnly(scriptPubKey)) { + if (ret == ISMINE_NO && keystore.HaveWatchOnly(scriptPubKey)) { return ISMINE_WATCH_ONLY; } - return ISMINE_NO; + return ret; } } // namespace isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid) { - return IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP); + isminetype ret = IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP); + if (isInvalid) { + ret = ISMINE_NO; + } + return ret; } isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey) From a53f0feff8d42b7a40d417f77dc8de682dd88fd9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 1 May 2018 13:18:51 -0700 Subject: [PATCH 4/5] Add some checks for invalid recursion in IsMine --- src/script/ismine.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp index bebaf9ea827..2f710782fb4 100644 --- a/src/script/ismine.cpp +++ b/src/script/ismine.cpp @@ -76,6 +76,11 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b break; case TX_WITNESS_V0_KEYHASH: { + if (sigversion == IsMineSigVersion::WITNESS_V0) { + // P2WPKH inside P2WSH is invalid. + isInvalid = true; + return ISMINE_NO; + } 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. @@ -100,6 +105,11 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b break; case TX_SCRIPTHASH: { + if (sigversion != IsMineSigVersion::TOP) { + // P2SH inside P2WSH or P2SH is invalid. + isInvalid = true; + return ISMINE_NO; + } CScriptID scriptID = CScriptID(uint160(vSolutions[0])); CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { @@ -109,6 +119,11 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b } case TX_WITNESS_V0_SCRIPTHASH: { + if (sigversion == IsMineSigVersion::WITNESS_V0) { + // P2WSH inside P2WSH is invalid. + isInvalid = true; + return ISMINE_NO; + } if (sigversion == IsMineSigVersion::TOP && !keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) { break; } From c004ffc9b42a738043e19e4c812fc7e0566119c5 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 3 May 2018 10:55:40 -0700 Subject: [PATCH 5/5] Make handling of invalid in IsMine more uniform --- src/script/ismine.cpp | 73 ++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp index 2f710782fb4..43dd9e582e8 100644 --- a/src/script/ismine.cpp +++ b/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,16 +55,9 @@ bool HaveKeys(const std::vector& pubkeys, const CKeyStore& keystore) return true; } -void Update(isminetype& val, isminetype update) +IsMineResult IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion) { - if (val == ISMINE_NO) val = update; - if (val == ISMINE_WATCH_ONLY && update == ISMINE_SPENDABLE) val = update; -} - -isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion) -{ - isminetype ret = ISMINE_NO; - isInvalid = false; + IsMineResult ret = IsMineResult::NO; std::vector vSolutions; txnouttype whichType; @@ -67,19 +73,17 @@ 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 ISMINE_NO; + return IsMineResult::INVALID; } if (keystore.HaveKey(keyID)) { - Update(ret, ISMINE_SPENDABLE); + ret = std::max(ret, IsMineResult::SPENDABLE); } break; case TX_WITNESS_V0_KEYHASH: { if (sigversion == IsMineSigVersion::WITNESS_V0) { // P2WPKH inside P2WSH is invalid. - isInvalid = true; - return ISMINE_NO; + 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 @@ -87,7 +91,7 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b // This also applies to the P2WSH case. break; } - Update(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)); break; } case TX_PUBKEYHASH: @@ -95,25 +99,23 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b if (!PermitsUncompressed(sigversion)) { CPubKey pubkey; if (keystore.GetPubKey(keyID, pubkey) && !pubkey.IsCompressed()) { - isInvalid = true; - return ISMINE_NO; + return IsMineResult::INVALID; } } if (keystore.HaveKey(keyID)) { - Update(ret, ISMINE_SPENDABLE); + ret = std::max(ret, IsMineResult::SPENDABLE); } break; case TX_SCRIPTHASH: { if (sigversion != IsMineSigVersion::TOP) { // P2SH inside P2WSH or P2SH is invalid. - isInvalid = true; - return ISMINE_NO; + return IsMineResult::INVALID; } CScriptID scriptID = CScriptID(uint160(vSolutions[0])); CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { - Update(ret, IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH)); + ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH)); } break; } @@ -121,8 +123,7 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b { if (sigversion == IsMineSigVersion::WITNESS_V0) { // P2WSH inside P2WSH is invalid. - isInvalid = true; - return ISMINE_NO; + return IsMineResult::INVALID; } if (sigversion == IsMineSigVersion::TOP && !keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) { break; @@ -132,7 +133,7 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b CScriptID scriptID = CScriptID(hash); CScript subscript; if (keystore.GetCScript(scriptID, subscript)) { - Update(ret, IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::WITNESS_V0)); + ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0)); } break; } @@ -153,20 +154,19 @@ 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 ISMINE_NO; + return IsMineResult::INVALID; } } } if (HaveKeys(keys, keystore)) { - Update(ret, ISMINE_SPENDABLE); + ret = std::max(ret, IsMineResult::SPENDABLE); } break; } } - if (ret == ISMINE_NO && keystore.HaveWatchOnly(scriptPubKey)) { - return ISMINE_WATCH_ONLY; + if (ret == IsMineResult::NO && keystore.HaveWatchOnly(scriptPubKey)) { + ret = std::max(ret, IsMineResult::WATCH_ONLY); } return ret; } @@ -175,11 +175,18 @@ isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, b isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid) { - isminetype ret = IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP); - if (isInvalid) { - ret = ISMINE_NO; + 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; } - return ret; + assert(false); } isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey)