#12953 inadvertently removed the functionality to call getbalance "*" <int> <bool> to get the wallet’s balance with either minconfs or include_watchonly.
This restores that functionality (when -deprecatedrpc=accounts), and also makes it possible to call ``getbalance minconf= include_watchonly=` when accounts are not being used.
Feedback to address from @ryanofsky
Some thoughts looking at code:
IsDeprecatedRPCEnabled("accounts") and the account argument is not null or “*”. Otherwise the argument would be silently ignored.GetAvailableCredit a little with something like0CAmount* cache = filter == ISMINE_SPENDABLE ? nAvailableCreditCached :
1 filter == ISMINE_WATCH_ONLY ? nAvailableWatchCreditCached :
2 nullptr;
I guess that would not work for min dept of 0 (unconfirmed).
Are you saying that we also need to check pcoin->InMempool() for coins with 0 depth?
GetBalance() can now take an ismine filter, which is passed down to
GetAvailableCredit. This allows GetBalance to be used to get watch-only
balances.
I believe that I’ve addressed all of @ryanofsky’s feedback. Russ - can you rereview and confirm?
Need to address @jonasschnelli’s comment. Jonas - can you please confirm my question above? Thanks!
948- if(!include_watchonly.isNull())
949- if(include_watchonly.get_bool())
950- filter = filter | ISMINE_WATCH_ONLY;
951-
952- return ValueFromAmount(pwallet->GetLegacyBalance(filter, nMinDepth, account));
953+ if (!IsDeprecatedRPCEnabled("accounts") && *account != "") {
In commit “[RPC] [wallet] allow getbalance to use min_conf and watch_only”
I think the safest thing here would be to raise an error whenever accounts are deprecated and account is non-null, because previously getbalance(null), getbalance("*"), and getbalance("") each did different things, and it doesn’t seem good to silently ignore a requested behavior. Another slightly less safe but maybe acceptable option would be to change "" to "*" on this line, since getbalance("*") and getbalance(null) at least were more similar than getbalance("*") and getbalance("")
Previously, and currently with the deprecated accounts feature enabled:
getbalance(null) would return total balancegetbalance("*") would return total balance with legacy accounting methodgetbalance("account") would return balance associated with a named accountgetbalance("") would return leftover balance not associated with any named accounts
Also want to note that another approach we could take here would not be to deprecate the account argument at all, but instead just rename it to label and pass it down as an option to the GetBalance() and GetAvailableCredit methods where it could be used to filter txouts there by address label. This would be a simple code change, just adding if (!label || *label == GetLabelName(txout.scriptPubKey)).
utACK c57f2009f65ffedc7531644243d5d7ddd786edde if getbalance("") is not treated like getbalance(null), (see comment).
It looks like we might have had tests for this previously which were deleted. It could be better in the future to avoid dropping tests for deprecated features until the features are actually removed, though it is sometimes nice to simplify tests.
Thanks @ryanofsky . Updated to treat * as an acceptable dummy argument.
I don’t think we want to entirely remove the possibility of calling this method with positional arguments.
2193@@ -2224,7 +2194,7 @@ CAmount CWallet::GetUnconfirmedWatchOnlyBalance() const
2194 {
2195 const CWalletTx* pcoin = &entry.second;
2196 if (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0 && pcoin->InMempool())
2197- nTotal += pcoin->GetAvailableWatchOnlyCredit();
2198+ nTotal += pcoin->GetAvailableCredit(true, ISMINE_WATCH_ONLY);
CWallet::GetUnconfirmedBalance use the cache?
1986+ cache = &nAvailableCreditCached;
1987+ cache_used = &fAvailableCreditCached;
1988+ } else if (filter == ISMINE_WATCH_ONLY) {
1989+ cache = &nAvailableWatchCreditCached;
1990+ cache_used = &fAvailableWatchCreditCached;
1991+ }
fUseCache block, connected with 1991 below.
filter == ISMINE_ALL — which can be when include_watchonly=true in RPC getbalance — could sum both if cached?
cache and cache_used are updated below even if fUseCache is not set.
931+ min_depth = request.params[1].get_int();
932+ }
933+
934+ isminefilter filter = ISMINE_SPENDABLE;
935+ if (!request.params[2].isNull() && request.params[2].get_bool()) {
936+ filter = filter | ISMINE_WATCH_ONLY;
|=
4422@@ -4416,7 +4423,7 @@ static const CRPCCommand commands[] =
4423 { "wallet", "dumpwallet", &dumpwallet, {"filename"} },
4424 { "wallet", "encryptwallet", &encryptwallet, {"passphrase"} },
4425 { "wallet", "getaddressinfo", &getaddressinfo, {"address"} },
4426- { "wallet", "getbalance", &getbalance, {"account","minconf","include_watchonly"} },
4427+ { "wallet", "getbalance", &getbalance, {"account|dummy","minconf","include_watchonly"} },
* here instead of dummy?
dummy is the name of the argument. * is the value that it needs to be set to.
935+ if (!request.params[2].isNull() && request.params[2].get_bool()) {
936+ filter = filter | ISMINE_WATCH_ONLY;
937+ }
938+
939+ if (!account_value.isNull()) {
940
852@@ -853,8 +853,9 @@ static UniValue getbalance(const JSONRPCRequest& request)
853 return NullUniValue;
854 }
855
856- if (request.fHelp || (request.params.size() > 3 && IsDeprecatedRPCEnabled("accounts")) || (request.params.size() != 0 && !IsDeprecatedRPCEnabled("accounts")))
857+ if (request.fHelp || (request.params.size() > 3 ))
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.
459@@ -460,9 +460,8 @@ class CWalletTx : public CMerkleTx
460 CAmount GetDebit(const isminefilter& filter) const;
461 CAmount GetCredit(const isminefilter& filter) const;
462 CAmount GetImmatureCredit(bool fUseCache=true) const;
463- CAmount GetAvailableCredit(bool fUseCache=true) const;
464+ CAmount GetAvailableCredit(bool fUseCache=true, const isminefilter& filter=ISMINE_SPENDABLE) const;
&?
2148 for (const auto& entry : mapWallet)
2149 {
2150 const CWalletTx* pcoin = &entry.second;
2151- if (pcoin->IsTrusted())
2152- nTotal += pcoin->GetAvailableCredit();
2153+ if (pcoin->IsTrusted() && pcoin->GetDepthInMainChain() >= min_depth) {
IsTrusted() already computes GetDepthInMainChain(). Suggestion:
0bool IsTrusted(int min_depth = 1)
GetBalance performance.