Wallet: correctly deprecate accounts in getbalance, re-add minconf / include-watch-only

[jonasschnelli]

It looks like that #9614 accidentally(?) dropped support for min-conf and watch_only in getbalance().

This PR tries to correctly deprecate accounts in getbalance() following the dummy-argument approach.

[promag]

Weird, travis failed with

/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11/build-aux/install-sh: src/qt/bitcoin-qt does not exist.
Makefile:1214: recipe for target 'Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' failed
make: *** [Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt] Error 1
make: *** Waiting for unfinished jobs....

restarted job.

[laanwj]

Concept ACK - added 0.17 tag to avoid API breakage in major version

[jnewbery]

I don't understand. min-conf and watch_only have never been supported when account is not specified:

     if (request.params.size() == 0)
         return  ValueFromAmount(pwallet->GetBalance());

[DrahtBot]

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

• [wallet] getbalance: Add option to include non-mempool UTXOs #11020 ([wallet] getbalance: Add option to include non-mempool UTXOs by kallewoof)
• Refactor: separate wallet from node #10973 (Refactor: separate wallet from node by ryanofsky)

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.

[jonasschnelli]

I don't understand. min-conf and watch_only have never been supported when account is not specified:

@jnewbery:
AFAIK, before #9614, one you get the watch-only balance of a wallet by calling getbalance "*" 0 true which is no longer possible in master.

[jnewbery]

I've taken another look at this and I don't think that #9614 removed the ability to call getbalance "*" 0 true. I think it was accidentally removed by #12953 here: 109e05d#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR891.

Concept ACK fixing that regression and restoring the functionality to call getbalance "*" 0 true.

However, note that calling getbalance "*" 0 true invokes GetLegacyAddress(), which 'calculates the balance in a different way [...], and which can count spends twice when there are conflicting pending transactions (such as those created by the bumpfee command), temporarily resulting in low or even negative balances. In general, account balance calculation is not considered reliable and has resulted in confusing outcomes, so it is recommended to avoid passing this argument.'

This PR changes the API to allow getbalance include_watchonly=true, which will now also call GetLegacyAddress(). I'm not sure if that's desirable. At the very least, the help text should make it clear that the balance may not be as expected.

cc @ryanofsky , who wrote that help text.

[jonasschnelli]

@jnewbery AFAIK there is currently no "better" balance calculation if one wants to filter with watch-only & min-confirmation-count,... and I think keeping the old behaviour rather then removing it is preferable.

But obviously the whole balance calculation and caching system is inefficient (I guess it'�s not buggy?) and needs probably a complete overhaul.

[jnewbery]

I think it's ok to change the API so that getbalance include_watchonly=true works, otherwise there's no way to get the watchonly balance if not using the -deprecatedrpc=accounts argument. However, the help text needs to be updated to explain that the balance is calculated in a different way if the include_watchonly parameter is used.

[ryanofsky]

However, the help text needs to be updated to explain that the balance is calculated in a different way if the include_watchonly parameter is used.

I don't think there's a technical reason why CWallet::GetBalance couldn't support the same isminefilter filter and int minDepth options that CWallet::GetLegacyBalance takes. It should just be matter of forwarding the arguments down to CWalletTx::GetAvailableCredit and adding a few lines of filtering code there. This would probably be less confusing that calculating the balance in a different way when filters are present (and having to explain this in documentation and maybe break compatibility in the future).

[sipa]

There is also CWallet::GetWatchOnlyBalance

[jonasschnelli]

Added min/max conf filter for GetWatchOnlyBalance() and started to use this method for getbalance in conjunction with watchonly. Refactored GetUnconfirmedWatchOnlyBalance().

Not sure if this is better since the main goal of this PR is to avoid an API break in 0.17.
The balance methods probably need refactoring which is not the main intention here.

Ping @sipa @ryanofsky

[ryanofsky]

Didn't look extremely close but it seems like 9bbd407 has a few issues:

• It pretends both minconf and watchonly arguments are null if only one of them is null, instead of warning the user about the ignored option (or simply handling it).
• It interprets "include_watchonly" as just GetWatchOnlyBalance instead of GetWatchOnlyBalance + GetBalance?
• Seems to complicate logic unnecessarily by tying in GetWatchOnlyBalance and introducing unused max_depth option?

I'm not sure if my original suggestion was unclear, unworkable, or just not good, but I wonder why this couldn't just forward filter and minDepth options down to GetBalance GetAvailableCredit methods and slightly just tweak logic inside to implement them.

Also, this is a minor point, but I think it would be clearer to use boost::optional (eventually std::optional) for optional values instead of magic numbers like -1.

[jnewbery]

I think https://github.com/jnewbery/bitcoin/tree/fix_get_balance does what's required:

• minconf and watchonly can be set/unset independently
• if -deprecatedrpc=accounts is set and getbalance "*" <int> <bool> is called, then GetLegacyBalance() is used (unchanged behaviour)
• if getbalance minconf=<int> include_watchonly=<bool> is called without an account, then GetBalance() is called
• GetBalance() takes min_conf and ismine_filter arguments
• when include_watchonly is set, the returned value includes GetWatchOnlyBalance + GetBalance.

@ryanofsky - is that what you had in mind?

@jonasschnelli - if you're happy with that branch, either take it here or let me know and I'll open a PR.

[ryanofsky]

@ryanofsky - is that what you had in mind?

Yes, changes from master...jnewbery:fix_get_balance are what I was suggesting. 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]

Thanks @ryanofsky . @jonasschnelli - if you're amenable, I'm happy to open a new PR for this. Also feel free to take my commits and address Russ's comments if you'd prefer.

[jonasschnelli]

Closing in favour of https://github.com/jnewbery/bitcoin/commits/fix_get_balance