LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #13002: Do not treat bare multisig outputs as IsMine unless watched

Browse source 
7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille)
b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille)
9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille)
08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille)
3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille)
ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille)
19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille)
fb1dfbb Remove unused IsMine overload (Pieter Wuille)
952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille)

Pull request description:

  Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet.

  This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them).

  Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it.

  I think there are two options:
  * Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched)
  * Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable

  This PR implements the first option. The second option was explored in #12874.

Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a
This commit is contained in:
Wladimir J. van der Laan 2018-04-26 19:48:50 +02:00
commit 487dcbe80c
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
7 changed files with 91 additions and 45 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
doc/release-notes.md
View file

@ -105,6 +105,16 @@ Low-level RPC changes
with any `-wallet=<path>` options, there is no change in behavior, and the
name of any wallet is just its `<path>` string.
- Bare multisig outputs to our keys are no longer automatically treated as
incoming payments. As this feature was only available for multisig outputs for
which you had all private keys in your wallet, there was generally no use for
them compared to single-key schemes. Furthermore, no address format for such
outputs is defined, and wallet software can't easily send to it. These outputs
will no longer show up in `listtransactions`, `listunspent`, or contribute to
your balance, unless they are explicitly watched (using `importaddress` or
`importmulti` with hex script argument). `signrawtransaction*` also still
works for them.
### Logging
- The log timestamp format is now ISO 8601 (e.g. "2018-02-28T12:34:56Z").

80
src/script/ismine.cpp
View file

@ -13,7 +13,27 @@
typedef std::vector<unsigned char> valtype;
static bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keystore)
namespace {
/**
* This is an enum that tracks the execution context of a script, similar to
* SigVersion in script/interpreter. It is separate however because we want to
* distinguish between top-level scriptPubKey execution and P2SH redeemScript
* execution (a distinction that has no impact on consensus rules).
*/
enum class IsMineSigVersion
{
TOP = 0, //! scriptPubKey execution
P2SH = 1, //! P2SH redeemScript
WITNESS_V0 = 2 //! P2WSH witness script execution
};
bool PermitsUncompressed(IsMineSigVersion sigversion)
{
return sigversion == IsMineSigVersion::TOP || sigversion == IsMineSigVersion::P2SH;
}
bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keystore)
{
for (const valtype& pubkey : pubkeys) {
CKeyID keyID = CPubKey(pubkey).GetID();
@ -22,25 +42,7 @@ static bool HaveKeys(const std::vector<valtype>& pubkeys, const CKeyStore& keyst
return true;
}
isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, SigVersion sigversion)
{
bool isInvalid = false;
return IsMine(keystore, scriptPubKey, isInvalid, sigversion);
}
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest, SigVersion sigversion)
{
bool isInvalid = false;
return IsMine(keystore, dest, isInvalid, sigversion);
}
isminetype IsMine(const CKeyStore &keystore, const CTxDestination& dest, bool& isInvalid, SigVersion sigversion)
{
CScript script = GetScriptForDestination(dest);
return IsMine(keystore, script, isInvalid, sigversion);
}
isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion sigversion)
isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion)
{
isInvalid = false;
@ -61,7 +63,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
break;
case TX_PUBKEY:
keyID = CPubKey(vSolutions[0]).GetID();
if (sigversion != SigVersion::BASE && vSolutions[0].size() != 33) {
if (!PermitsUncompressed(sigversion) && vSolutions[0].size() != 33) {
isInvalid = true;
return ISMINE_NO;
}
@ -70,20 +72,20 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
break;
case TX_WITNESS_V0_KEYHASH:
{
if (!keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
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 = ::IsMine(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, SigVersion::WITNESS_V0);
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;
break;
}
case TX_PUBKEYHASH:
keyID = CKeyID(uint160(vSolutions[0]));
if (sigversion != SigVersion::BASE) {
if (!PermitsUncompressed(sigversion)) {
CPubKey pubkey;
if (keystore.GetPubKey(keyID, pubkey) && !pubkey.IsCompressed()) {
isInvalid = true;
@ -98,7 +100,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
CScript subscript;
if (keystore.GetCScript(scriptID, subscript)) {
isminetype ret = IsMine(keystore, subscript, isInvalid);
isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::P2SH);
if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
return ret;
}
@ -106,7 +108,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
}
case TX_WITNESS_V0_SCRIPTHASH:
{
if (!keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
if (sigversion == IsMineSigVersion::TOP && !keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
break;
}
uint160 hash;
@ -114,7 +116,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
CScriptID scriptID = CScriptID(hash);
CScript subscript;
if (keystore.GetCScript(scriptID, subscript)) {
isminetype ret = IsMine(keystore, subscript, isInvalid, SigVersion::WITNESS_V0);
isminetype ret = IsMineInner(keystore, subscript, isInvalid, IsMineSigVersion::WITNESS_V0);
if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))
return ret;
}
@ -123,13 +125,16 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
case TX_MULTISIG:
{
// Never treat bare multisig outputs as ours (they can still be made watchonly-though)
if (sigversion == IsMineSigVersion::TOP) break;
// Only consider transactions "mine" if we own ALL the
// keys involved. Multi-signature transactions that are
// partially owned (somebody else has a key that can spend
// them) enable spend-out-from-under-you attacks, especially
// in shared-wallet situations.
std::vector<valtype> keys(vSolutions.begin()+1, vSolutions.begin()+vSolutions.size()-1);
if (sigversion != SigVersion::BASE) {
if (!PermitsUncompressed(sigversion)) {
for (size_t i = 0; i < keys.size(); i++) {
if (keys[i].size() != 33) {
isInvalid = true;
@ -150,3 +155,22 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
}
return ISMINE_NO;
}
} // namespace
isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid)
{
return IsMineInner(keystore, scriptPubKey, isInvalid, IsMineSigVersion::TOP);
}
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);
return IsMine(keystore, script);
}

7
src/script/ismine.h
View file

@ -33,9 +33,8 @@ typedef uint8_t isminefilter;
* 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, SigVersion = SigVersion::BASE);
isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, SigVersion = SigVersion::BASE);
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest, bool& isInvalid, SigVersion = SigVersion::BASE);
isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest, SigVersion = SigVersion::BASE);
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);
#endif // BITCOIN_SCRIPT_ISMINE_H

2
src/script/standard.h
View file

@ -23,7 +23,7 @@ class CScriptID : public uint160
{
public:
CScriptID() : uint160() {}
CScriptID(const CScript& in);
explicit CScriptID(const CScript& in);
CScriptID(const uint160& in) : uint160(in) {}
};

9
src/test/script_standard_tests.cpp
View file

@ -561,7 +561,14 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine)
keystore.AddKey(keys[1]);
result = IsMine(keystore, scriptPubKey, isInvalid);
BOOST_CHECK_EQUAL(result, ISMINE_SPENDABLE);
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);
BOOST_CHECK_EQUAL(result, ISMINE_NO);
BOOST_CHECK(!isInvalid);
}

16
src/wallet/rpcdump.cpp
View file

@ -224,10 +224,11 @@ void ImportScript(CWallet* const pwallet, const CScript& script, const std::stri
}
if (isRedeemScript) {
if (!pwallet->HaveCScript(script) && !pwallet->AddCScript(script)) {
const CScriptID id(script);
if (!pwallet->HaveCScript(id) && !pwallet->AddCScript(script)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
}
ImportAddress(pwallet, CScriptID(script), strLabel);
ImportAddress(pwallet, id, strLabel);
} else {
CTxDestination destination;
if (ExtractDestination(script, destination)) {
@ -602,7 +603,8 @@ UniValue importwallet(const JSONRPCRequest& request)
} else if(IsHex(vstr[0])) {
std::vector<unsigned char> vData(ParseHex(vstr[0]));
CScript script = CScript(vData.begin(), vData.end());
if (pwallet->HaveCScript(script)) {
CScriptID id(script);
if (pwallet->HaveCScript(id)) {
LogPrintf("Skipping import of %s (script already present)\n", vstr[0]);
continue;
}
@ -613,7 +615,7 @@ UniValue importwallet(const JSONRPCRequest& request)
}
int64_t birth_time = DecodeDumpTime(vstr[1]);
if (birth_time > 0) {
pwallet->m_script_metadata[CScriptID(script)].nCreateTime = birth_time;
pwallet->m_script_metadata[id].nCreateTime = birth_time;
nTimeBegin = std::min(nTimeBegin, birth_time);
}
}
@ -899,12 +901,12 @@ UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int6
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
}
if (!pwallet->HaveCScript(redeemScript) && !pwallet->AddCScript(redeemScript)) {
CScriptID redeem_id(redeemScript);
if (!pwallet->HaveCScript(redeem_id) && !pwallet->AddCScript(redeemScript)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
}
CTxDestination redeem_dest = CScriptID(redeemScript);
CScript redeemDestination = GetScriptForDestination(redeem_dest);
CScript redeemDestination = GetScriptForDestination(redeem_id);
if (::IsMine(*pwallet, redeemDestination) == ISMINE_SPENDABLE) {
throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");

12
test/functional/feature_segwit.py
View file

@ -303,8 +303,10 @@ class SegWitTest(BitcoinTestFramework):
v = self.nodes[0].getaddressinfo(i)
if (v['isscript']):
[bare, p2sh, p2wsh, p2sh_p2wsh] = self.p2sh_address_to_script(v)
# bare and p2sh multisig with compressed keys should always be spendable
spendable_anytime.extend([bare, p2sh])
# p2sh multisig with compressed keys should always be spendable
spendable_anytime.extend([p2sh])
# bare multisig can be watched and signed, but is not treated as ours
solvable_after_importaddress.extend([bare])
# P2WSH and P2SH(P2WSH) multisig with compressed keys are spendable after direct importaddress
spendable_after_importaddress.extend([p2wsh, p2sh_p2wsh])
else:
@ -320,8 +322,10 @@ class SegWitTest(BitcoinTestFramework):
v = self.nodes[0].getaddressinfo(i)
if (v['isscript']):
[bare, p2sh, p2wsh, p2sh_p2wsh] = self.p2sh_address_to_script(v)
# bare and p2sh multisig with uncompressed keys should always be spendable
spendable_anytime.extend([bare, p2sh])
# p2sh multisig with uncompressed keys should always be spendable
spendable_anytime.extend([p2sh])
# bare multisig can be watched and signed, but is not treated as ours
solvable_after_importaddress.extend([bare])
# P2WSH and P2SH(P2WSH) multisig with uncompressed keys are never seen
unseen_anytime.extend([p2wsh, p2sh_p2wsh])
else: