Merge #16383: rpcwallet: default include_watchonly to true for watchonly wallets

72eaab073bc747425fe551777154b13a6c4c37c9 tests: functional watch-only wallet tests (William Casarin)
72ffbdc5799c1707ecad674d701b43fb80b031d0 doc: add release note for include_watchonly default changes (William Casarin)
003a3c73c0450aa18ac2ab2ca47def2b8c53a7df rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6c0b8e8144d3deec58ec2e3449ba081151 rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)

Pull request description:

  Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.

  Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.

ACKs for top commit:
  achow101:
    Code review ACK 72eaab073bc747425fe551777154b13a6c4c37c9
  Sjors:
    ACK 72eaab073bc747425fe551777154b13a6c4c37c9
  promag:
    ACK 72eaab073bc747425fe551777154b13a6c4c37c9, code review only, didn't look closely to the test.
  kallewoof:
    ACK 72eaab073bc747425fe551777154b13a6c4c37c9
  fanquake:
    ACK 72eaab073bc747425fe551777154b13a6c4c37c9 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.

Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7

4 files changed, 161 insertions, 25 deletions

diff --git a/doc/release-notes-16383.md b/doc/release-notes-16383.md
new file mode 100644
index 0000000000..8015715167
--- /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`.
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index cbab73d612..30e767e489 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)
 {
@@ -710,7 +727,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{
@@ -743,10 +760,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]);
 
@@ -1023,9 +1037,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();
@@ -1169,7 +1184,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{
@@ -1220,7 +1235,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"
@@ -1361,7 +1376,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"
@@ -1424,9 +1439,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");
@@ -1492,7 +1508,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)"},
                 },
@@ -1569,8 +1585,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());
@@ -1632,7 +1648,7 @@ static UniValue gettransaction(const JSONRPCRequest& request)
                 "\nGet detailed information about in-wallet transaction <txid>\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"
@@ -1687,9 +1703,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);
@@ -3015,8 +3032,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();
@@ -3048,6 +3064,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)
@@ -3102,7 +3121,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"
@@ -4047,7 +4066,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"
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index ea8dbbaf95..bca5ac644f 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -129,6 +129,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 0000000000..be8d7714fb
--- /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()