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

72eaab0 tests: functional watch-only wallet tests (William Casarin)
72ffbdc doc: add release note for include_watchonly default changes (William Casarin)
003a3c7 rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6 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 72eaab0
  Sjors:
    ACK 72eaab0
  promag:
    ACK 72eaab0, code review only, didn't look closely to the test.
  kallewoof:
    ACK 72eaab0
  fanquake:
    ACK 72eaab0 - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.

Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7

Author: fanquake

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`.

src/wallet/rpcwallet.cpp

@@ -51,6 +51,23 @@
 
 static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
 
+/** 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();
+}
+
+
+
 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
 {
     if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
@@ -780,7 +797,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."},
                     {"addlocked", RPCArg::Type::BOOL, /* default */ "false", "Whether to include transactions locked via InstantSend in the wallet's balance."},
-                    {"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')"},
                 },
                 RPCResult{
             "amount              (numeric) The total amount in " + CURRENCY_UNIT + " received for this wallet.\n"
@@ -818,10 +835,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
         fAddLocked = addlocked.get_bool();
     }
 
-    bool include_watchonly = false;
-    if (!request.params[3].isNull() && request.params[3].get_bool()) {
-        include_watchonly = true;
-    }
+    bool include_watchonly = ParseIncludeWatchonly(request.params[2], *pwallet);
 
     const auto bal = pwallet->GetBalance(min_depth, fAddLocked);
 
@@ -1123,9 +1137,10 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, CWallet * co
         fIncludeEmpty = params[2].get_bool();
 
     isminefilter filter = ISMINE_SPENDABLE;
-    if(!params[3].isNull())
-        if(params[3].get_bool())
-            filter = filter | ISMINE_WATCH_ONLY;
+
+    if (ParseIncludeWatchonly(params[3], *pwallet)) {
+        filter |= ISMINE_WATCH_ONLY;
+    }
 
     bool has_filtered_address = false;
     CTxDestination filtered_address = CNoDestination();
@@ -1275,7 +1290,7 @@ static UniValue listreceivedbyaddress(const JSONRPCRequest& request)
                     {"minconf", RPCArg::Type::NUM, /* default */ "1", "The minimum number of confirmations before payments are included."},
                     {"addlocked", RPCArg::Type::BOOL, /* default */ "false", "Whether to include transactions locked via InstantSend."},
                     {"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{
@@ -1331,7 +1346,7 @@ static UniValue listreceivedbylabel(const JSONRPCRequest& request)
                     {"minconf", RPCArg::Type::NUM, /* default */ "1", "The minimum number of confirmations before payments are included."},
                     {"addlocked", RPCArg::Type::BOOL, /* default */ "false", "Whether to include transactions locked via InstantSend."},
                     {"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"
@@ -1475,7 +1490,7 @@ static 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"
@@ -1543,9 +1558,10 @@ static 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");
@@ -1600,7 +1616,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)"},
                 },
@@ -1674,8 +1690,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());
@@ -1739,7 +1755,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"
@@ -1791,9 +1807,10 @@ static UniValue gettransaction(const JSONRPCRequest& request)
     hash.SetHex(request.params[0].get_str());
 
     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);
@@ -3268,8 +3285,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
         if (options.exists("changePosition"))
             change_position = options["changePosition"].get_int();
 
-        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();
@@ -3297,6 +3313,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)
@@ -3352,7 +3371,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
                         {
                             {"changeAddress", RPCArg::Type::STR, /* default */ "pool address", "The dash address to receive the change"},
                             {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
-                            {"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"
@@ -4072,7 +4091,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
                         {
                             {"changeAddress", RPCArg::Type::STR_HEX, /* default */ "pool address", "The dash address to receive the change"},
                             {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
-                            {"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"

test/functional/test_runner.py

@@ -137,6 +137,8 @@
     'wallet_multiwallet.py',
     'wallet_disableprivatekeys.py',
     'wallet_disableprivatekeys.py --usecli',
+    'wallet_watchonly.py',
+    'wallet_watchonly.py --usecli',
     'interface_http.py',
     'interface_rpc.py',
     'rpc_psbt.py',

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()