From a50d9e6c0b8e8144d3deec58ec2e3449ba081151 Mon Sep 17 00:00:00 2001 From: William Casarin Date: Sat, 13 Jul 2019 08:34:49 -0700 Subject: [PATCH 1/4] rpcwallet: default include_watchonly to true for watchonly wallets The logic before would only include watchonly addresses if it was explicitly set in the rpc argument. This changes the logic like so: If the include_watchonly argument is missing, check the WALLET_FLAG_DISABLE_PRIVATE_KEYS flag to determine if we're working with a watchonly wallet. If so, default include_watchonly to true. If the include_watchonly argument is explicit set to false, we still disable them from the listing. Although this would always return nothing, it might be still useful in situations where you want to explicitly filter out watchonly addresses regardless of what wallet you are dealing with. Signed-off-by: William Casarin --- src/wallet/rpcwallet.cpp | 53 +++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 19becf93c7a..e18e3d414af 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa return avoid_reuse; } + +/** Used by RPC commands that have an include_watchonly parameter. + * We default to true for watchonly wallets if include_watchonly isn't + * explicitly set. + */ +static bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& pwallet) +{ + if (include_watchonly.isNull()) { + // if include_watchonly isn't explicitly set, then check if we have a watchonly wallet + return pwallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); + } + + // otherwise return whatever include_watchonly was set to + return include_watchonly.get_bool(); +} + + /** Checks if a CKey is in the given CWallet compressed or otherwise*/ bool HaveKey(const CWallet& wallet, const CKey& key) { @@ -748,10 +765,7 @@ static UniValue getbalance(const JSONRPCRequest& request) min_depth = request.params[1].get_int(); } - bool include_watchonly = false; - if (!request.params[2].isNull() && request.params[2].get_bool()) { - include_watchonly = true; - } + bool include_watchonly = ParseIncludeWatchonly(request.params[2], *pwallet); bool avoid_reuse = GetAvoidReuseFlag(pwallet, request.params[3]); @@ -1033,9 +1047,10 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, CWallet * co fIncludeEmpty = params[1].get_bool(); isminefilter filter = ISMINE_SPENDABLE; - if(!params[2].isNull()) - if(params[2].get_bool()) - filter = filter | ISMINE_WATCH_ONLY; + + if (ParseIncludeWatchonly(params[2], *pwallet)) { + filter |= ISMINE_WATCH_ONLY; + } bool has_filtered_address = false; CTxDestination filtered_address = CNoDestination(); @@ -1434,9 +1449,10 @@ UniValue listtransactions(const JSONRPCRequest& request) if (!request.params[2].isNull()) nFrom = request.params[2].get_int(); isminefilter filter = ISMINE_SPENDABLE; - if(!request.params[3].isNull()) - if(request.params[3].get_bool()) - filter = filter | ISMINE_WATCH_ONLY; + + if (ParseIncludeWatchonly(request.params[3], *pwallet)) { + filter |= ISMINE_WATCH_ONLY; + } if (nCount < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative count"); @@ -1579,8 +1595,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request) } } - if (!request.params[2].isNull() && request.params[2].get_bool()) { - filter = filter | ISMINE_WATCH_ONLY; + if (ParseIncludeWatchonly(request.params[2], *pwallet)) { + filter |= ISMINE_WATCH_ONLY; } bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); @@ -1697,9 +1713,10 @@ static UniValue gettransaction(const JSONRPCRequest& request) uint256 hash(ParseHashV(request.params[0], "txid")); isminefilter filter = ISMINE_SPENDABLE; - if(!request.params[1].isNull()) - if(request.params[1].get_bool()) - filter = filter | ISMINE_WATCH_ONLY; + + if (ParseIncludeWatchonly(request.params[1], *pwallet)) { + filter |= ISMINE_WATCH_ONLY; + } UniValue entry(UniValue::VOBJ); auto it = pwallet->mapWallet.find(hash); @@ -3014,8 +3031,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f } } - if (options.exists("includeWatching")) - coinControl.fAllowWatchOnly = options["includeWatching"].get_bool(); + coinControl.fAllowWatchOnly = ParseIncludeWatchonly(options["includeWatching"], *pwallet); if (options.exists("lockUnspents")) lockUnspents = options["lockUnspents"].get_bool(); @@ -3047,6 +3063,9 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f } } } + } else { + // if options is null and not a bool + coinControl.fAllowWatchOnly = ParseIncludeWatchonly(NullUniValue, *pwallet); } if (tx.vout.size() == 0) From 003a3c73c0450aa18ac2ab2ca47def2b8c53a7df Mon Sep 17 00:00:00 2001 From: William Casarin Date: Sat, 13 Jul 2019 11:48:50 -0700 Subject: [PATCH 2/4] rpcwallet: document include_watchonly default for watchonly wallets Signed-off-by: William Casarin --- src/wallet/rpcwallet.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e18e3d414af..fd4254330c3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -732,7 +732,7 @@ static UniValue getbalance(const JSONRPCRequest& request) { {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."}, {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Also include balance in watch-only addresses (see 'importaddress')"}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also include balance in watch-only addresses (see 'importaddress')"}, {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."}, }, RPCResult{ @@ -1194,7 +1194,7 @@ static UniValue listreceivedbyaddress(const JSONRPCRequest& request) { {"minconf", RPCArg::Type::NUM, /* default */ "1", "The minimum number of confirmations before payments are included."}, {"include_empty", RPCArg::Type::BOOL, /* default */ "false", "Whether to include addresses that haven't received any payments."}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses (see 'importaddress')."}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Whether to include watch-only addresses (see 'importaddress')"}, {"address_filter", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If present, only return information on this address."}, }, RPCResult{ @@ -1245,7 +1245,7 @@ static UniValue listreceivedbylabel(const JSONRPCRequest& request) { {"minconf", RPCArg::Type::NUM, /* default */ "1", "The minimum number of confirmations before payments are included."}, {"include_empty", RPCArg::Type::BOOL, /* default */ "false", "Whether to include labels that haven't received any payments."}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses (see 'importaddress')."}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Whether to include watch-only addresses (see 'importaddress')"}, }, RPCResult{ "[\n" @@ -1386,7 +1386,7 @@ UniValue listtransactions(const JSONRPCRequest& request) " with the specified label, or \"*\" to disable filtering and return all transactions."}, {"count", RPCArg::Type::NUM, /* default */ "10", "The number of transactions to return"}, {"skip", RPCArg::Type::NUM, /* default */ "0", "The number of transactions to skip"}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Include transactions to watch-only addresses (see 'importaddress')"}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Include transactions to watch-only addresses (see 'importaddress')"}, }, RPCResult{ "[\n" @@ -1518,7 +1518,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) { {"blockhash", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, the block hash to list transactions since, otherwise list all transactions."}, {"target_confirmations", RPCArg::Type::NUM, /* default */ "1", "Return the nth block hash from the main chain. e.g. 1 would mean the best block hash. Note: this is not used as a filter, but only affects [lastblock] in the return value"}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Include transactions to watch-only addresses (see 'importaddress')"}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Include transactions to watch-only addresses (see 'importaddress')"}, {"include_removed", RPCArg::Type::BOOL, /* default */ "true", "Show transactions that were removed due to a reorg in the \"removed\" array\n" " (not guaranteed to work on pruned nodes)"}, }, @@ -1658,7 +1658,7 @@ static UniValue gettransaction(const JSONRPCRequest& request) "\nGet detailed information about in-wallet transaction \n", { {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"}, - {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses in balance calculation and details[]"}, + {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Whether to include watch-only addresses in balance calculation and details[]"}, }, RPCResult{ "{\n" @@ -3120,7 +3120,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request) {"changeAddress", RPCArg::Type::STR, /* default */ "pool address", "The bitcoin address to receive the change"}, {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, {"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, - {"includeWatching", RPCArg::Type::BOOL, /* default */ "false", "Also select inputs which are watch only"}, + {"includeWatching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only"}, {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"}, {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n" @@ -4062,7 +4062,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) {"changeAddress", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"}, {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, {"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, - {"includeWatching", RPCArg::Type::BOOL, /* default */ "false", "Also select inputs which are watch only"}, + {"includeWatching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only"}, {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"}, {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n" From 72ffbdc5799c1707ecad674d701b43fb80b031d0 Mon Sep 17 00:00:00 2001 From: William Casarin Date: Tue, 16 Jul 2019 13:23:21 -0700 Subject: [PATCH 3/4] doc: add release note for include_watchonly default changes Signed-off-by: William Casarin --- doc/release-notes-16383.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/release-notes-16383.md diff --git a/doc/release-notes-16383.md b/doc/release-notes-16383.md new file mode 100644 index 00000000000..80157151672 --- /dev/null +++ b/doc/release-notes-16383.md @@ -0,0 +1,8 @@ +RPC changes +----------- + +RPCs which have an `include_watchonly` argument or `includeWatching` +option now default to `true` for watch-only wallets. Affected RPCs +are: `getbalance`, `listreceivedbyaddress`, `listreceivedbylabel`, +`listtransactions`, `listsinceblock`, `gettransaction`, +`walletcreatefundedpsbt`, and `fundrawtransaction`. From 72eaab073bc747425fe551777154b13a6c4c37c9 Mon Sep 17 00:00:00 2001 From: William Casarin Date: Thu, 18 Jul 2019 13:35:23 -0700 Subject: [PATCH 4/4] tests: functional watch-only wallet tests These test the new watch-only defaults for rpcs with include_watchonly and includeWatching options. Signed-off-by: William Casarin --- test/functional/test_runner.py | 2 + test/functional/wallet_watchonly.py | 107 ++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 test/functional/wallet_watchonly.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 462547f44f9..a6688aa362a 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -127,6 +127,8 @@ BASE_SCRIPTS = [ 'wallet_multiwallet.py --usecli', 'wallet_createwallet.py', 'wallet_createwallet.py --usecli', + 'wallet_watchonly.py', + 'wallet_watchonly.py --usecli', 'interface_http.py', 'interface_rpc.py', 'rpc_psbt.py', diff --git a/test/functional/wallet_watchonly.py b/test/functional/wallet_watchonly.py new file mode 100644 index 00000000000..be8d7714fba --- /dev/null +++ b/test/functional/wallet_watchonly.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +# Copyright (c) 2018-2019 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test createwallet arguments. +""" + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error +) + + +class CreateWalletWatchonlyTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = False + self.num_nodes = 1 + self.supports_cli = True + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def run_test(self): + node = self.nodes[0] + + self.nodes[0].createwallet(wallet_name='default') + def_wallet = node.get_wallet_rpc('default') + + a1 = def_wallet.getnewaddress() + wo_change = def_wallet.getnewaddress() + wo_addr = def_wallet.getnewaddress() + + self.nodes[0].createwallet(wallet_name='wo', disable_private_keys=True) + wo_wallet = node.get_wallet_rpc('wo') + + wo_wallet.importpubkey(pubkey=def_wallet.getaddressinfo(wo_addr)['pubkey']) + wo_wallet.importpubkey(pubkey=def_wallet.getaddressinfo(wo_change)['pubkey']) + + # generate some btc for testing + node.generatetoaddress(101, a1) + + # send 1 btc to our watch-only address + txid = def_wallet.sendtoaddress(wo_addr, 1) + self.nodes[0].generate(1) + + # getbalance + self.log.info('include_watchonly should default to true for watch-only wallets') + self.log.info('Testing getbalance watch-only defaults') + assert_equal(wo_wallet.getbalance(), 1) + assert_equal(len(wo_wallet.listtransactions()), 1) + assert_equal(wo_wallet.getbalance(include_watchonly=False), 0) + + self.log.info('Testing listreceivedbyaddress watch-only defaults') + result = wo_wallet.listreceivedbyaddress() + assert_equal(len(result), 1) + assert_equal(result[0]["involvesWatchonly"], True) + result = wo_wallet.listreceivedbyaddress(include_watchonly=False) + assert_equal(len(result), 0) + + self.log.info('Testing listreceivedbylabel watch-only defaults') + result = wo_wallet.listreceivedbylabel() + assert_equal(len(result), 1) + assert_equal(result[0]["involvesWatchonly"], True) + result = wo_wallet.listreceivedbylabel(include_watchonly=False) + assert_equal(len(result), 0) + + self.log.info('Testing listtransactions watch-only defaults') + result = wo_wallet.listtransactions() + assert_equal(len(result), 1) + assert_equal(result[0]["involvesWatchonly"], True) + result = wo_wallet.listtransactions(include_watchonly=False) + assert_equal(len(result), 0) + + self.log.info('Testing listsinceblock watch-only defaults') + result = wo_wallet.listsinceblock() + assert_equal(len(result["transactions"]), 1) + assert_equal(result["transactions"][0]["involvesWatchonly"], True) + result = wo_wallet.listsinceblock(include_watchonly=False) + assert_equal(len(result["transactions"]), 0) + + self.log.info('Testing gettransaction watch-only defaults') + result = wo_wallet.gettransaction(txid) + assert_equal(result["details"][0]["involvesWatchonly"], True) + result = wo_wallet.gettransaction(txid=txid, include_watchonly=False) + assert_equal(len(result["details"]), 0) + + self.log.info('Testing walletcreatefundedpsbt watch-only defaults') + inputs = [] + outputs = [{a1: 0.5}] + options = {'changeAddress': wo_change} + no_wo_options = {'changeAddress': wo_change, 'includeWatching': False} + + result = wo_wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options=options) + assert_equal("psbt" in result, True) + assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.walletcreatefundedpsbt, inputs, outputs, 0, no_wo_options) + + self.log.info('Testing fundrawtransaction watch-only defaults') + rawtx = wo_wallet.createrawtransaction(inputs=inputs, outputs=outputs) + result = wo_wallet.fundrawtransaction(hexstring=rawtx, options=options) + assert_equal("hex" in result, True) + assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.fundrawtransaction, rawtx, no_wo_options) + + + +if __name__ == '__main__': + CreateWalletWatchonlyTest().main()