diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -7,3 +7,12 @@ which is replaced by a `dummy` argument for backwards compatibility with clients using positional arguments. The RPC is still used to change the apparent fee-rate of the transaction by using the `fee_delta` argument. + - 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. diff --git a/src/script/ismine.h b/src/script/ismine.h --- a/src/script/ismine.h +++ b/src/script/ismine.h @@ -41,8 +41,6 @@ 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, - bool &isInvalid); isminetype IsMine(const CKeyStore &keystore, const CTxDestination &dest); #endif // BITCOIN_SCRIPT_ISMINE_H diff --git a/src/script/ismine.cpp b/src/script/ismine.cpp --- a/src/script/ismine.cpp +++ b/src/script/ismine.cpp @@ -13,8 +13,20 @@ typedef std::vector valtype; -static bool HaveKeys(const std::vector &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 +}; + +bool HaveKeys(const std::vector &pubkeys, const CKeyStore &keystore) { for (const valtype &pubkey : pubkeys) { CKeyID keyID = CPubKey(pubkey).GetID(); if (!keystore.HaveKey(keyID)) { @@ -24,24 +36,8 @@ return true; } -isminetype IsMine(const CKeyStore &keystore, const CScript &scriptPubKey) { - bool isInvalid = false; - return IsMine(keystore, scriptPubKey, isInvalid); -} - -isminetype IsMine(const CKeyStore &keystore, const CTxDestination &dest) { - bool isInvalid = false; - return IsMine(keystore, dest, isInvalid); -} - -isminetype IsMine(const CKeyStore &keystore, const CTxDestination &dest, - bool &isInvalid) { - CScript script = GetScriptForDestination(dest); - return IsMine(keystore, script, isInvalid); -} - -isminetype IsMine(const CKeyStore &keystore, const CScript &scriptPubKey, - bool &isInvalid) { +isminetype IsMineInner(const CKeyStore &keystore, const CScript &scriptPubKey, + bool &isInvalid, IsMineSigVersion sigversion) { isInvalid = false; std::vector vSolutions; @@ -69,7 +65,8 @@ 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; @@ -77,6 +74,12 @@ break; } 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 @@ -103,3 +106,21 @@ } 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); +} diff --git a/src/script/standard.h b/src/script/standard.h --- a/src/script/standard.h +++ b/src/script/standard.h @@ -24,7 +24,7 @@ class CScriptID : public uint160 { public: CScriptID() : uint160() {} - CScriptID(const CScript &in); + explicit CScriptID(const CScript &in); CScriptID(const uint160 &in) : uint160(in) {} }; diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -597,7 +597,14 @@ 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); } diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -267,11 +267,12 @@ } 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)) { @@ -1125,14 +1126,14 @@ "Error adding address to wallet"); } - if (!pwallet->HaveCScript(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, diff --git a/test/functional/abc-wallet-standardness.py b/test/functional/abc-wallet-standardness.py --- a/test/functional/abc-wallet-standardness.py +++ b/test/functional/abc-wallet-standardness.py @@ -49,13 +49,17 @@ nonstd_node.generate(120) sync_blocks(self.nodes) - def fund_and_test_wallet(scriptPubKey, shouldBeStandard, shouldBeInWallet, amount=10000, spendfee=500, nonstd_error="scriptpubkey (code 64)"): + def fund_and_test_wallet(scriptPubKey, shouldBeStandard, shouldBeInWallet, + amount=10000, spendfee=500, nonstd_error="scriptpubkey (code 64)", canSign=None): """ Get the nonstandard node to fund a transaction, test its standardness by trying to broadcast on the standard node, then mine it and see if it ended up in the standard node's wallet. Finally, it attempts to spend the coin. """ + if canSign is None: + canSign = shouldBeInWallet + self.log.info("Trying script {}".format(scriptPubKey.hex(),)) # get nonstandard node to fund the script @@ -99,6 +103,12 @@ # calculate wallet balance change just as a double check balance_change = std_node.getbalance() - balance_initial + if shouldBeInWallet: + assert (txid, 0) in wallet_outpoints + assert balance_change == amount * SATOSHI + else: + assert (txid, 0) not in wallet_outpoints + assert balance_change == 0 # try spending the funds using the wallet. outamount = (amount - spendfee) * SATOSHI @@ -112,9 +122,7 @@ [{'txid': txid, 'vout': 0}], outputs) signresult = std_node.signrawtransactionwithwallet(spendtx) - if shouldBeInWallet: - assert (txid, 0) in wallet_outpoints - assert balance_change == amount * SATOSHI + if canSign: assert_equal(signresult['complete'], True) txid = std_node.sendrawtransaction(signresult['hex']) [blockhash] = std_node.generate(1) @@ -122,8 +130,6 @@ assert txid in std_node.getblock(blockhash)["tx"] sync_blocks(self.nodes) else: - assert (txid, 0) not in wallet_outpoints - assert balance_change == 0 # signresult['errors'] will vary depending on input script. What # occurs is that in sign.cpp, ProduceSignature gets back # solved=false since SignStep sees a nonstandard input. Then, @@ -155,7 +161,7 @@ # Bare multisig fund_and_test_wallet( - CScript([OP_1, pubkey, OP_1, OP_CHECKMULTISIG]), True, True) + CScript([OP_1, pubkey, OP_1, OP_CHECKMULTISIG]), True, False, canSign=True) fund_and_test_wallet( CScript([OP_1, OP_PUSHDATA1, pubkey, OP_1, OP_CHECKMULTISIG]), False, False) fund_and_test_wallet( @@ -164,7 +170,7 @@ CScript([b'\x01', pubkey, OP_1, OP_CHECKMULTISIG]), False, False) # Note: 1-of-5 is nonstandard to fund but standard to spend. fund_and_test_wallet( - CScript([OP_1, pubkey, pubkey, pubkey, pubkey, pubkey, OP_5, OP_CHECKMULTISIG]), False, True) + CScript([OP_1, pubkey, pubkey, pubkey, pubkey, pubkey, OP_5, OP_CHECKMULTISIG]), False, False, canSign=True) fund_and_test_wallet( CScript([OP_1, pubkey, pubkey, pubkey, OP_PUSHDATA1, pubkey, pubkey, OP_5, OP_CHECKMULTISIG]), False, False)