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)
static bool IncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet)
Would have to update the documentation as well for the "smart" default value.
Concept ACK. I think having include_watchonly default to true for actual watchonly wallets is super useful.
@MarcoFalke I'm a bit new, which docs specifically would need to be updated?
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.
nit: /**-starting comments are prefered
nit: for readability perhaps employ a different verb than "include" here (have, take, etc.)
done
done
@MarcoFalke I'm a bit new, which docs specifically would need to be updated?
- {"include_watchonly", RPCArg::Type::BOOL, /* default */ "false", "Whether to include watch-only addresses in balance calculation and details[]"},
+ {"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
nit: s/set/set./
done
Concept ACK. Will test when help docs are updated. Seems this change would merit test coverage?
@MarcoFalke I wasn't quite sure what to do for documenting the smart default, let me know if that looks ok
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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?
yeah that message sounds better. I'll go with watch-only as well.
done
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)
nit, I'd name this 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))
nit, add { }.
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))
nit, add { }.
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))
nit, add { }.
Concept ACK, this definitely seems like the only sensible default to have in that case.
concept ACK, needs tests(as others have said)
One thing I missed was rpcs with the includeWatching option such as fundrawtransaction and walletcreatefundedpsbt. I think it makes sense to add the default on those as well.
Concept ACK
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
Nit: Should be "Affected"? :-)
done
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>
Code review ACK 72eaab073bc747425fe551777154b13a6c4c37c9
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`,
nit, sort.
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 | +
nit, remove extra line.
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
nit, drop comment.
ACK 72eaab073bc747425fe551777154b13a6c4c37c9, code review only, didn't look closely to the test.
This makes a lot of sense, Concept ACK
ACK 72eaab073bc747425fe551777154b13a6c4c37c9
51 | @@ -52,6 +52,23 @@ static inline bool GetAvoidReuseFlag(CWallet * const pwallet, const UniValue& pa 52 | return avoid_reuse; 53 | } 54 | 55 | +
Nit: Extraneous newline.
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
return include_watchonly.isNull()
? pwallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)
: include_watchonly.get_bool();
66 | + 67 | + // otherwise return whatever include_watchonly was set to 68 | + return include_watchonly.get_bool(); 69 | +} 70 | + 71 | +
Nit: Extraneous newline.
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.