Fix get balance #13566

jnewbery · 2018-06-28T19:05:11Z

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

jnewbery · 2018-06-28T19:06:35Z

Feedback to address from @ryanofsky

Some thoughts looking at code:

It seems like it would be good to raise an error if IsDeprecatedRPCEnabled("accounts") and the account argument is not null or "*". Otherwise the argument would be silently ignored.
Documentation seems to say default minconf is 1, while implementation uses 0 or 1.
Maybe could dedup caching code in GetAvailableCredit a little with something like

CAmount* cache = filter == ISMINE_SPENDABLE  ? nAvailableCreditCached : 
                 filter == ISMINE_WATCH_ONLY ? nAvailableWatchCreditCached :
                 nullptr;

jnewbery · 2018-06-28T21:00:07Z

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.

jnewbery · 2018-06-28T21:59:52Z

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!

ryanofsky

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.

ryanofsky · 2018-06-29T13:39:45Z

src/wallet/rpcwallet.cpp

-                filter = filter | ISMINE_WATCH_ONLY;
-
-        return ValueFromAmount(pwallet->GetLegacyBalance(filter, nMinDepth, account));
+        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 balance

getbalance("*") would return total balance with legacy accounting method

getbalance("account") would return balance associated with a named account

getbalance("") would return leftover balance not associated with any named accounts

#13566 (comment)

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

jnewbery · 2018-06-29T14:25:57Z

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.

ryanofsky

utACK 43937db46edab8c226ce190e185d7a3285b3b364, only change since last review is requiring "*" instead of "" to signify all accounts. Thanks for implementing this and all the changes.

jnewbery · 2018-06-29T15:48:26Z

Thanks for the review @ryanofsky !

Empact · 2018-06-29T16:44:11Z

This calls for a test, no?

…t accounts.

jnewbery · 2018-06-29T19:55:24Z

@Empact - test added. Review plz 🙂

src/wallet/wallet.cpp

Empact · 2018-06-29T20:24:06Z

Should CWallet::GetUnconfirmedBalance use the cache?

src/wallet/wallet.cpp

Empact · 2018-06-29T20:34:52Z

src/wallet/rpcwallet.cpp

+
+    isminefilter filter = ISMINE_SPENDABLE;
+    if (!request.params[2].isNull() && request.params[2].get_bool()) {
+        filter = filter | ISMINE_WATCH_ONLY;


src/wallet/rpcwallet.cpp

Empact · 2018-06-29T20:45:53Z

src/wallet/rpcwallet.cpp

@@ -853,8 +853,9 @@ static UniValue getbalance(const JSONRPCRequest& request)
        return NullUniValue;
    }

-    if (request.fHelp || (request.params.size() > 3 && IsDeprecatedRPCEnabled("accounts")) || (request.params.size() != 0 && !IsDeprecatedRPCEnabled("accounts")))
+    if (request.fHelp || (request.params.size() > 3 ))


nit: whitespace

Empact · 2018-06-29T20:54:24Z

utACK 702ae1e

DrahtBot · 2018-06-30T11:36:36Z

Note to reviewers: This pull request conflicts with the following ones:

wallet: Add GetBalances to calculate all balances #13637 (wallet: Add GetBalances to calculate all balances by promag)
Add CMerkleTx::IsImmatureCoinBase method #13631 (Add CMerkleTx::IsImmatureCoinBase method by Empact)
Add compile time checking for cs_main runtime locking assertions #13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)
wallet: Add missing cs_wallet/cs_KeyStore locks to wallet #11634 (wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift)
[wallet] getbalance: Add option to include non-mempool UTXOs #11020 ([wallet] getbalance: Add option to include non-mempool UTXOs by kallewoof)

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.

laanwj · 2018-07-04T13:06:34Z

utACK 702ae1e

promag

utACK 702ae1e. ~~Consider last comment as a way to not degrade GetBalance performance.~~

promag · 2018-07-04T22:52:37Z

src/wallet/wallet.cpp

+    } else if (filter == ISMINE_WATCH_ONLY) {
+        cache = &nAvailableWatchCreditCached;
+        cache_used = &fAvailableWatchCreditCached;
+    }


What if filter == ISMINE_ALL — which can be when include_watchonly=true in RPC getbalance — could sum both if cached?

Seems like a reasonable optimization, but I don't think it's required. Could be done in a separate PR.

promag · 2018-07-04T23:06:36Z

src/wallet/wallet.h

@@ -460,9 +460,8 @@ class CWalletTx : public CMerkleTx
    CAmount GetDebit(const isminefilter& filter) const;
    CAmount GetCredit(const isminefilter& filter) const;
    CAmount GetImmatureCredit(bool fUseCache=true) const;
-    CAmount GetAvailableCredit(bool fUseCache=true) const;
+    CAmount GetAvailableCredit(bool fUseCache=true, const isminefilter& filter=ISMINE_SPENDABLE) const;


nit, drop &?

promag · 2018-07-04T23:12:05Z

src/wallet/wallet.cpp

 {
    CAmount nTotal = 0;
    {
        LOCK2(cs_main, cs_wallet);
        for (const auto& entry : mapWallet)
        {
            const CWalletTx* pcoin = &entry.second;
-            if (pcoin->IsTrusted())
-                nTotal += pcoin->GetAvailableCredit();
+            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.

sipa · 2018-07-14T01:27:54Z

utACK 702ae1e

702ae1e [RPC] [wallet] allow getbalance to use min_conf and watch_only without accounts. (John Newbery) cf15761 [wallet] GetBalance can take a min_depth argument. (John Newbery) 0f3d6e9 [wallet] factor out GetAvailableWatchOnlyBalance() (John Newbery) 7110c83 [wallet] deduplicate GetAvailableCredit logic (John Newbery) ef7bc88 [wallet] Factor out GetWatchOnlyBalance() (John Newbery) 4279da4 [wallet] GetBalance can take an isminefilter filter. (John Newbery) Pull request description: #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. Tree-SHA512: 67e84de9291ed6d34b23c626f4dc5988ba0ae6c99708d02b87dd3aaad3f4b6baa6202a66cc2dadd30dd993a39de8036ee920fcaa8cbb1c5dfe606e6fac183344

jnewbery · 2018-07-16T02:38:46Z

@Empact @promag - thanks for the reviews. I'll look at addressing nits in a follow-up.

jnewbery · 2018-11-01T16:48:28Z

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

Empact · 2018-11-02T14:21:04Z

Yeah I would leave it as-is at this point.

promag · 2018-11-02T14:34:39Z

@jnewbery agree, not worth it.

jnewbery · 2018-11-02T15:34:54Z

Agree. Thanks for the input. Sorry those changes didn't make it!

702ae1e [RPC] [wallet] allow getbalance to use min_conf and watch_only without accounts. (John Newbery) cf15761 [wallet] GetBalance can take a min_depth argument. (John Newbery) 0f3d6e9 [wallet] factor out GetAvailableWatchOnlyBalance() (John Newbery) 7110c83 [wallet] deduplicate GetAvailableCredit logic (John Newbery) ef7bc88 [wallet] Factor out GetWatchOnlyBalance() (John Newbery) 4279da4 [wallet] GetBalance can take an isminefilter filter. (John Newbery) Pull request description: bitcoin#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. Tree-SHA512: 67e84de9291ed6d34b23c626f4dc5988ba0ae6c99708d02b87dd3aaad3f4b6baa6202a66cc2dadd30dd993a39de8036ee920fcaa8cbb1c5dfe606e6fac183344

maflcko added this to the 0.17.0 milestone Jun 28, 2018

jnewbery added 5 commits June 28, 2018 17:50

[wallet] GetBalance can take an isminefilter filter.

4279da4

GetBalance() can now take an ismine filter, which is passed down to GetAvailableCredit. This allows GetBalance to be used to get watch-only balances.

[wallet] Factor out GetWatchOnlyBalance()

ef7bc88

[wallet] deduplicate GetAvailableCredit logic

7110c83

[wallet] factor out GetAvailableWatchOnlyBalance()

0f3d6e9

[wallet] GetBalance can take a min_depth argument.

cf15761

jnewbery force-pushed the fix_get_balance branch from 16e67cc to c57f200 Compare June 28, 2018 21:58

fanquake added the Wallet label Jun 28, 2018

ryanofsky reviewed Jun 29, 2018

View reviewed changes

jnewbery force-pushed the fix_get_balance branch from c57f200 to 43937db Compare June 29, 2018 14:24

ryanofsky reviewed Jun 29, 2018

View reviewed changes

[RPC] [wallet] allow getbalance to use min_conf and watch_only withou…

702ae1e

…t accounts.

jnewbery force-pushed the fix_get_balance branch from 43937db to 702ae1e Compare June 29, 2018 19:54