Skip to content

Separate IsMine from solvability #13142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laanwj merged 5 commits into from
May 29, 2018
Merged

Separate IsMine from solvability #13142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d714c3
Make coincontrol use IsSolvable to determine solvability
sipa May 1, 2018
4e91820
Make IsMine stop distinguishing solvable/unsolvable
sipa May 1, 2018
b5802a9
Simplify IsMine logic
sipa May 1, 2018
a53f0fe
Add some checks for invalid recursion in IsMine
sipa May 1, 2018
c004ffc
Make handling of invalid in IsMine more uniform
sipa May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 61 additions & 35 deletions src/script/ismine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)) {
if (keystore.HaveWatchOnly(scriptPubKey))
return ISMINE_WATCH_UNSOLVABLE;
return ISMINE_NO;
}
Solver(scriptPubKey, whichType, vSolutions);

CKeyID keyID;
switch (whichType)
Expand All @@ -64,50 +73,58 @@ 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)) {
ret = std::max(ret, IsMineResult::SPENDABLE);
Copy link
Contributor

@Empact Empact May 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ret is always NO at this point (and in all such assignments) so the max is unnecessary.
Could be left as a safety measure though if that's what you intend.

Copy link
Member Author

@sipa sipa May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect the compiler to optimize this out; with the max it's more clear what the intention is (going through various combinations and picking the "most" isminish one, unless it's invalid), and less likely to break with future change.

}
if (keystore.HaveKey(keyID))
return ISMINE_SPENDABLE;
break;
case TX_WITNESS_V0_KEYHASH:
{
if (sigversion == IsMineSigVersion::WITNESS_V0) {
// P2WPKH inside P2WSH is invalid.
Copy link
Member

@theuni theuni May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to flip isInvalid ?

Copy link
Member Author

@sipa sipa May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

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);
if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
return ret;
ret = std::max(ret, IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), IsMineSigVersion::WITNESS_V0));
break;
}
case TX_PUBKEYHASH:
keyID = CKeyID(uint160(vSolutions[0]));
if (!PermitsUncompressed(sigversion)) {
CPubKey pubkey;
if (keystore.GetPubKey(keyID, pubkey) && !pubkey.IsCompressed()) {
isInvalid = true;
return ISMINE_NO;
return IsMineResult::INVALID;
}
}
if (keystore.HaveKey(keyID))
return ISMINE_SPENDABLE;
if (keystore.HaveKey(keyID)) {
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);
if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
return ret;
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH));
}
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;
}
Expand All @@ -116,17 +133,17 @@ 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_SOLVABLE || (ret == ISMINE_NO && isInvalid))
return ret;
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0));
}
break;
}

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
Expand All @@ -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 ISMINE_NO;
return IsMineResult::INVALID;
}
}
}
if (HaveKeys(keys, keystore))
return ISMINE_SPENDABLE;
if (HaveKeys(keys, keystore)) {
ret = std::max(ret, IsMineResult::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;
if (ret == IsMineResult::NO && keystore.HaveWatchOnly(scriptPubKey)) {
ret = std::max(ret, IsMineResult::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);
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)
Expand Down
8 changes: 2 additions & 6 deletions src/script/ismine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/coincontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class CCoinControl
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;
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2363,10 +2363,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 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));
Copy link
Contributor

@Empact Empact May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this use of ISMINE_NO seems misplaced. Checking for the existence of the flag in mine, the result should not equal 0, which only incidentally is the value of NO.

Copy link
Member Author

@sipa sipa May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, though that's an existing problem in several places. I prefer to fix it as a follow-up (see #13142 (comment)).


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) {
Expand Down