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.
51@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
52 return avoid_reuse;
53 }
54
55+
56+/* Used by RPC commands that include an include_watchonly parameter.
57+ * We default to true for watchonly wallets if include_watchonly isn't
58+ * explicitly set
59+ */
60+static bool IncludeWatchonly(const UniValue& include_watchonly, CWallet* const pwallet)
0static bool IncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet)
include_watchonly default to true for actual watchonly wallets is super useful.
51@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
52 return avoid_reuse;
53 }
54
55+
56+/* Used by RPC commands that include an include_watchonly parameter.
/**-starting comments are prefered
@MarcoFalke I’m a bit new, which docs specifically would need to be updated?
0- {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses in balance calculation and details[]"},
1+ {"include_watchonly", RPCArg::Type::BOOL, /* default */ ">>>>>>>>>>> update here<<<<<<<<", "Whether to include watch-only addresses in balance calculation and details[]"},
51@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
52 return avoid_reuse;
53 }
54
55+
56+/* Used by RPC commands that include an include_watchonly parameter.
57+ * We default to true for watchonly wallets if include_watchonly isn't
58+ * explicitly set
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
731@@ -732,7 +732,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
732 {
733 {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
734 {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
735- {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Also include balance in watch-only addresses (see 'importaddress')"},
736+ {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false, true for watchonly wallets", "Also include balance in watch-only addresses (see 'importaddress')"},
Suggestion for the RPCHelpMan default messages: "true for watch-only wallets, otherwise false"
Grepping related cases in the codebase comes up with similar, e.g. src/wallet/rpcdump.cpp:120
As for “watchonly” vs “watch-only”, in the descriptions on the same line it is dasherized and git grepping shows the same for user-facing messages. Unless other reviewers have a different opinion, it might be more clear to do the same?
watch-only as well.
51@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
52 return avoid_reuse;
53 }
54
55+
56+/** Used by RPC commands that have an include_watchonly parameter.
57+ * We default to true for watchonly wallets if include_watchonly isn't
58+ * explicitly set.
59+ */
60+static bool IncludeWatchonly(const UniValue& include_watchonly, const CWallet& pwallet)
ParseIncludeWatchonly.
1447@@ -1434,9 +1448,9 @@ UniValue listtransactions(const JSONRPCRequest& request)
1448 if (!request.params[2].isNull())
1449 nFrom = request.params[2].get_int();
1450 isminefilter filter = ISMINE_SPENDABLE;
1451- if(!request.params[3].isNull())
1452- if(request.params[3].get_bool())
1453- filter = filter | ISMINE_WATCH_ONLY;
1454+
1455+ if (IncludeWatchonly(request.params[3], *pwallet))
{ }.
1592@@ -1579,9 +1593,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1593 }
1594 }
1595
1596- if (!request.params[2].isNull() && request.params[2].get_bool()) {
1597- filter = filter | ISMINE_WATCH_ONLY;
1598- }
1599+ if (IncludeWatchonly(request.params[2], *pwallet))
{ }.
1709@@ -1697,9 +1710,9 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1710 uint256 hash(ParseHashV(request.params[0], "txid"));
1711
1712 isminefilter filter = ISMINE_SPENDABLE;
1713- if(!request.params[1].isNull())
1714- if(request.params[1].get_bool())
1715- filter = filter | ISMINE_WATCH_ONLY;
1716+
1717+ if (IncludeWatchonly(request.params[1], *pwallet))
{ }.
includeWatching option such as fundrawtransaction and walletcreatefundedpsbt. I think it makes sense to add the default on those as well.
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 <jb55@jb55.com>
Signed-off-by: William Casarin <jb55@jb55.com>
Latest updates:
walletcreatefundedpsbt and fundrawtransaction’s includeWatching option.This is the first time I’ve done the last two things, so let me know if I did anything wrong.
0@@ -0,0 +1,8 @@
1+RPC changes
2+-----------
3+
4+RPCs which have an `include_watchonly` argument or `includeWatching`
5+option now default to `true` for watch-only wallets. affected RPCs
Concept ACK
Good idea!
Signed-off-by: William Casarin <jb55@jb55.com>
These test the new watch-only defaults for rpcs with include_watchonly
and includeWatching options.
Signed-off-by: William Casarin <jb55@jb55.com>
0@@ -0,0 +1,8 @@
1+RPC changes
2+-----------
3+
4+RPCs which have an `include_watchonly` argument or `includeWatching`
5+option now default to `true` for watch-only wallets. Affected RPCs
6+are: `getbalance`, `listreceivedbyaddress`, `listreceivedbylabel`,
100+ result = wo_wallet.fundrawtransaction(hexstring=rawtx, options=options)
101+ assert_equal("hex" in result, True)
102+ assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.fundrawtransaction, rawtx, no_wo_options)
103+
104+
105+
42+
43+ # send 1 btc to our watch-only address
44+ txid = def_wallet.sendtoaddress(wo_addr, 1)
45+ self.nodes[0].generate(1)
46+
47+ # getbalance
51@@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa
52 return avoid_reuse;
53 }
54
55+
64+ return pwallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
65+ }
66+
67+ // otherwise return whatever include_watchonly was set to
68+ return include_watchonly.get_bool();
69+}
Can do
0return include_watchonly.isNull()
1 ? pwallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)
2 : include_watchonly.get_bool();
66+
67+ // otherwise return whatever include_watchonly was set to
68+ return include_watchonly.get_bool();
69+}
70+
71+
ACK 72eaab073bc747425fe551777154b13a6c4c37c9
Lots of unnecessary newlines all over, and a few nits, but merge-worthy IMO!
ACK 72eaab073bc747425fe551777154b13a6c4c37c9 - I’ve looked over the changes, they make sense to me. Compiled and ran the tests etc.
Thanks to @kallewoof and @ajtowns (he said he sanity checked on his lunch break) for following up my review prod.
I know there are some nits here, however this has ACKs, and has been sitting for a while, so I’m going to merge it. The nits can be scooped up by someone in either a follow up PR, or a related change.