#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=<int> include_watchonly=<bool>` 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 likeCAmount* cache = filter == ISMINE_SPENDABLE ? nAvailableCreditCached :
filter == ISMINE_WATCH_ONLY ? nAvailableWatchCreditCached :
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.
utACK 43937db46edab8c226ce190e185d7a3285b3b364, only change since last review is requiring "*" instead of "" to signify all accounts. Thanks for implementing this and all the changes.
Thanks for the review @ryanofsky !
This calls for a test, no?
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);
nit: comment bool arg?
I don't think it's the project's coding style to always comment arguments (at least, I don't see that elsewhere in the codebase as the norm).
Should 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 | + }
nit: could move this into a fUseCache block, connected with 1991 below.
What if filter == ISMINE_ALL — which can be when include_watchonly=true in RPC getbalance — could sum both if cached?
No, cache and cache_used are updated below even if fUseCache is not set.
Seems like a reasonable optimization, but I don't think it's required. Could be done in a separate PR.
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;
nit: |=
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"} },
Maybe show * 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 |
nit: whitespace
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 ))
nit: whitespace
utACK 702ae1e
<!--e57a25ab6845829454e8d69fc972939a-->Note to 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.
utACK 702ae1e21a09d8c31406839c4ea507c5fa276898
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;
nit, drop &?
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:
bool IsTrusted(int min_depth = 1)
Ignore my comment, I tried that refactor and it doesn't look good.
utACK 702ae1e. Consider last comment as a way to not degrade GetBalance performance.
utACK 702ae1e21a09d8c31406839c4ea507c5fa276898
@Empact @promag - I've addressed the remaining nits in https://github.com/jnewbery/bitcoin/tree/13566_nits, but there isn't much in there. I'm not sure if there's much point in opening a new PR.
Yeah I would leave it as-is at this point.
Agree. Thanks for the input. Sorry those changes didn't make it!
Milestone
0.17.0