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

jonasschnelli commented at 8:30 pm on June 13, 2018: contributor

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.

Wallet: correctly deprecate getbalance accounts, re-add minconf and include-watch-only 7c1570d6e1

jonasschnelli added the label Wallet on Jun 13, 2018

jonasschnelli added the label RPC/REST/ZMQ on Jun 13, 2018

promag commented at 0:49 am on June 14, 2018: member

Weird, travis failed with

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

restarted job.

laanwj added this to the milestone 0.17.0 on Jun 14, 2018

laanwj commented at 11:18 am on June 14, 2018: member

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

jnewbery commented at 8:00 pm on June 14, 2018: member

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

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

DrahtBot commented at 8:10 pm on June 14, 2018: member

#11020 ([wallet] getbalance: Add option to include non-mempool UTXOs by kallewoof)
#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 commented at 8:15 pm on June 14, 2018: contributor

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 commented at 10:22 pm on June 18, 2018: member

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: https://github.com/bitcoin/bitcoin/commit/109e05dcd163b8ddb7f4b3550db6b9ab833b2c04#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 commented at 7:14 am on June 19, 2018: contributor

@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 commented at 1:58 pm on June 22, 2018: member

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 commented at 4:22 pm on June 22, 2018: member

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 commented at 4:23 pm on June 22, 2018: member

There is also CWallet::GetWatchOnlyBalance

wallet: add min/max conf filter to GetWatchOnlyBalance() 61cb9c8e78

Use GetWatchOnlyBalance instead of GetLegacyBalance in getbalanc 9bbd407936

jonasschnelli commented at 7:07 am on June 27, 2018: contributor

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 commented at 2:03 pm on June 27, 2018: member

Didn’t look extremely close but it seems like 9bbd407936f399c706bbc70c8b5ffa0bba0cfec9 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 commented at 6:45 pm on June 27, 2018: member

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 commented at 7:17 pm on June 27, 2018: member

@ryanofsky - is that what you had in mind?

Yes, changes from https://github.com/bitcoin/bitcoin/compare/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

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

jnewbery commented at 7:30 pm on June 27, 2018: member

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 commented at 6:40 pm on June 28, 2018: contributor

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

jonasschnelli closed this on Jun 28, 2018

MarcoFalke locked this on Sep 8, 2021

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