[wallet] getbalance: Add option to include non-mempool UTXOs #11020

kallewoof · 2017-08-10T08:06:51Z

Currently, if the wallet generates a transaction that cannot currently go into the mempool (e.g. due to too-long-mempool-chain), the wallet UTXOs related to this will vanish from getbalance, giving the appearance that the user has less funds than they actually do.

From a "get spendable balance" perspective, this is perfectly valid, but users may sometimes want to see their actual balance, regardless of whether they can spend it or not at that time.

This PR adds an include_unspendable option (default=false) to getbalance which, when true, will consider non-mempool transactions as trusted, and thus display these in the tally.

Note: this flag does nothing when a user specifies an account (i.e. GetLegacyBalance), and the legacy balance in fact already does what include_unspendable=true does, given the right arguments.

kallewoof · 2017-08-10T09:06:56Z

Not sure why zapwallettxes.py is failing. It's not touching anything I changed (and the test passes on my fork's travis & when running the test locally / via make check ...). @jnewbery thoughts?

maflcko · 2017-08-10T09:36:28Z

The zapwallettxes' failure is intermittent. Respinned travis.

jnewbery · 2017-08-10T15:39:34Z

I'd hoped that #10330 had fixed the zapwallettxes intermittency. @MarcoFalke do you have an example of it failing (the failure on this PR is no longer available since the task was restarted in Travis).

(apologies for being off-topic)

maflcko · 2017-08-10T20:03:57Z

@jnewbery Sorry I didn't keep track of the exact failure this time, I assume it was one of the known issues that popped up recently.

promag

Should add the same option to getunconfirmedbalance?

promag · 2017-08-12T00:37:55Z

src/wallet/wallet.h

Nit, snake case.

promag · 2017-08-12T00:38:00Z

src/wallet/wallet.h

Nit, snake case.

Not sure what you mean -- most other argument names are written in this style, e.g. redeemScript, mapKeyBirth, etc.

promag · 2017-08-12T00:42:55Z

test/functional/wallet.py

Assert exact value?

Asserting on exact values can make the test more brittle. I think it's fine to test strict lesser than here.

Asserting on exact values can make the test more brittle.

Is that a bad thing?

It's bad if the test will intermittently fail for unrelated reasons (rounding errors, for example).

That should not happen. There are lots of exact asserts through out this file.

promag · 2017-08-12T00:44:34Z

src/wallet/wallet.cpp

Nit, snake case.

promag · 2017-08-12T00:59:24Z

src/wallet/wallet.cpp

Update comment.

jnewbery

Concept ACK. A few questions about the interface and naming inline.

jnewbery · 2017-08-14T15:11:54Z

src/wallet/rpcwallet.cpp

I think this argument name is a bit confusing. Perhaps include_not_yet_spendable. It's a bit of a mouthful, but 'unspendable' sounds too permanent to me.

I was hoping the description of the argument would alleviate that, but you may be right.

Also, please align other arguments as this is currently the longest.

Actually, include_watchonly is slightly longer, but I think realigning is a good idea yeah.

jnewbery · 2017-08-14T15:17:22Z

src/wallet/rpcwallet.cpp

This is confusing for me. include_unspendable only has effect if:

account is not specified; and

minconf is not specified; and

include_watchonly is not specified

Additionally, if account is specified as "" (ie balance not associated with any account), the returned balance will use GetBalance() if include_unspendable is not set, but GetLegacyBalance() if it is set.

Can you explain the reasoning here?

The argument is the last in the list, and there's no way to give a default value for account because it's not being JSON parsed (you can't pass "null" as it would become the string "null"). Using named arguments, you can do this, but otherwise I have to make the "special case" of you passing defaults in.

The legacy balance seems to not be very used and possibly even about to be deprecated, so I didn't feel the need to update it. That, and it already kind of does what the PR is doing when you do a 0 minconf and account="*".

I find it surprising that minconf and watchonly only have effect if account is specified, and now include_unspendable only has effect if account is not specified. There doesn't appear to be any user documentation explaining that.

I'm afraid I don't know the history or motivation for that. @ryanofsky made changes in this area recently. Perhaps he can shed light on this.

jnewbery · 2017-08-14T15:18:13Z

src/wallet/rpcwallet.cpp

I find it confusing that the API uses include_unspendable, but internally you use ignoreUnspendable which is the inverse. Any reason not to be consistent?

It ended up that way. You're right, will fix.

jnewbery · 2017-08-14T15:20:48Z

test/functional/wallet.py

Asserting on exact values can make the test more brittle. I think it's fine to test strict lesser than here.

jnewbery · 2017-08-14T15:21:54Z

src/wallet/wallet.cpp

Again, I think the name ignoreUnspendable could be made clearer.

kallewoof · 2017-08-15T02:27:15Z

@promag @jnewbery Thanks for the review!

28e8271 fixes comment
df7228d fixes the inconsistency in variable names, but currently retains the include_unspendable name. I'm not sure I agree with the longer version @jnewbery suggested, but I'm up for changing this to something more intuitive if people feel it's worthwhile.
80acc02 adds the feature to getunconfirmedbalance (tested in 3642398). I was not entirely sure about the logic on this one, but I went with !IsTrusted() & depth=0 & (includeUnspendable || InMempool()), changed from !IsTrusted() & depth=0 & InMempool(). This means some balance will overlap between getbalance(include_unspendable=true) and getunconfirmedbalance(include_unspendable=true) but I think that's ok.

kallewoof · 2017-08-16T05:21:07Z

zapwallettxes.py again, @jnewbery: https://pastebin.com/70bQ7CLq

promag · 2017-08-16T14:18:43Z

IMO from the wallet point of view those UTXO should always be part of the balance, and in that case this should be a bug fix and not require more options. WDYT?

Edit: unless getbalance defaults to available balance. If that's the case the option can be include_unavailable (default false) or only_available (default true).

kallewoof · 2017-08-17T05:12:06Z

Yes getbalance is available balance, I think, so I don't think it should default to including these. Of course, there is the option of making the coin select include these coins even though they're not in the mempool, in which case getbalance could include them, but that's for another time (and PR).

Hm, include_unavailable seems like a better name for it than include_unspendable.

kallewoof · 2018-05-22T02:52:41Z

Rebased this, but now it is not available unless the user enables deprecated account. I could

Add a new "index 0" parameter to the non-deprecated variant, replacing "account", and allowing up to 1 argument for the new style. This obviously could be potentially confusing.
Keep the index and allow up to 4 indices in new style, but require that items at index 0..2 are all null. Fine for -named style but ughy otherwise.

Ping @jnewbery.

Edit: I went with the second option in c8c147a.

jnewbery · 2018-05-22T15:10:59Z

I think I prefer option (1). Overloading parameters can be confusing, but I think it's better in this case because:

the meaning of argument 0 is not ambiguous - it's fixed by setting the deprecaterrpc argument.
include_unavailable is a bool and account is a string
the account, minconf and include_watchonly parameters will be removed in the next release.
going with option (2) will lock us in to having 3 dummy arguments for future releases.

kallewoof · 2018-05-22T23:23:56Z

@jnewbery I definitely prefer option 1 but I wasn't sure it would be disruptive. Will switch.

kallewoof · 2018-05-22T23:33:08Z

@jnewbery See 0c28d8f -- I split up the descriptions based on deprecation, and fixed several text issues (e.g. RPC examples using the deprecated stuff). I think this change alone is worth a PR, to be honest.

FWIW I also removed include_unavailable from the deprecated variant since we're removing it anyway.

Since "accounts" is not JSON-parsed and "include_unavailable" wants to be, I am temporarily doing the conversion manually (testing for "true" or "1"). Once accounts are removed, we can add param 0 to clients.cpp and remove the manual stuff.

jnewbery · 2018-05-23T15:32:46Z

src/wallet/wallet.cpp

nit: variables and arguments should be in snake_case (same for all args below)

jnewbery · 2018-05-23T15:33:38Z

src/wallet/wallet.cpp

Should this call IsTrusted() with includeUnavailable? If not, coins that are in the mempool could show up in both the balance and the unconfirmed balance.

I'm not sure. I'll add tests to check that this is not the case.

jnewbery · 2018-05-23T15:37:08Z

src/wallet/rpcwallet.cpp

nit: can be moved below the if (IsDeprecatedRPCEnabled("accounts")) { block and the !IsDeprecatedRPCEnabled("accounts") condition removed (since the code below the if (IsDeprecatedRPCEnabled("accounts")) { block is effectively the else branch.

Good point!

jnewbery · 2018-05-23T16:29:54Z

src/wallet/rpcwallet.cpp

nit: since you're changing the help text for this and getunconfirmedbalance, perhaps correct it to say "the wallet's ..." instead of "the server's ..."

jnewbery · 2018-05-23T16:45:55Z

src/wallet/rpcwallet.cpp

It's a bit ugly to replicate the univalue parsing here, but I think it's ok since it's only going to be in here for one release.

I think you should remove the request.params[0].get_str() == "1" (1 isn't interpreted as true in univalue). You may also throw an error if request.params[0].get_str() is not false or true (so passing True for example doesn't silently fail and leave include_unavailable as false.

I thought 1 was true. E.g. getrawtransaction <id> 1. But that seems to be a special case for getrawtransaction, actually. Changing as you suggest.

…nspendable UTXOs. In some cases, a transaction will fail to go into the mempool, e.g. if it is in a too-long chain of transactions. getbalance will ignore the change UTXOs for these, giving you the impression that you have less balance than you actually do. This commit adds a flag include_unavailable to get(unconfirmed)balance, which, if enabled, will also account for transactions NOT in the mempool but in your wallet.

….py test.

kallewoof · 2018-05-24T04:57:21Z

@jnewbery

Should this call IsTrusted() with includeUnavailable? If not, coins that are in the mempool could show up in both the balance and the unconfirmed balance.

Looking at the tests (it was a while since I wrote this), I think the idea was that it should be counted in both (see wallet_basic.py changes). Is this problematic?

DrahtBot · 2018-07-14T11:23:31Z

Needs rebase

jnewbery · 2018-07-16T02:42:53Z

Needs rebase.

@kallewoof - how urgently do you want this? If it can wait until after v0.17, then it'll probably be easier and a smaller diff once the accounts API code has been removed. I intend to do that as soon as 0.17 is branched.

kallewoof · 2018-07-17T10:45:00Z

@jnewbery Not urgent. I will wait until 0.17 branches off.

kallewoof · 2018-08-10T03:16:02Z

Happy birthday. Closing due to lack of interest.

jnewbery · 2018-08-10T18:30:53Z

I commit to reviewing this after it's rebased on #13825

kallewoof · 2018-08-11T05:02:29Z

@jnewbery OK!

kallewoof · 2018-09-10T01:54:00Z

@jnewbery I began to rebase this, but self-reflectively decided this is a bit too invasive for the improvement it provides. I think it's a bit weird that 'getbalance' will suddenly drop significantly just because you ran out of chain length, but I'm no longer confident this is the right approach. Sorry for wasting your time!

jnewbery · 2018-09-10T02:26:33Z

Sorry for wasting your time!

Not wasted. No need to apologise!

fanquake added the Wallet label Aug 10, 2017

kallewoof force-pushed the unspendable-utxo-handling branch from d1ec4d5 to 5b644c8 Compare August 10, 2017 08:28

promag reviewed Aug 12, 2017

View reviewed changes

jnewbery reviewed Aug 14, 2017

View reviewed changes

kallewoof force-pushed the unspendable-utxo-handling branch 2 times, most recently from 3fd1c22 to 2812048 Compare August 17, 2017 05:26

kallewoof force-pushed the unspendable-utxo-handling branch from 2812048 to 8d6a264 Compare October 11, 2017 09:56

kallewoof force-pushed the unspendable-utxo-handling branch from 8d6a264 to bc5fc21 Compare December 5, 2017 04:56

ryanofsky mentioned this pull request Jan 9, 2018

Could the wallet count unconfirmed non-mempool change? #11887

Open

kallewoof force-pushed the unspendable-utxo-handling branch from bc5fc21 to ae468c1 Compare February 20, 2018 02:01

kallewoof force-pushed the unspendable-utxo-handling branch from ae468c1 to 6f4b15c Compare March 6, 2018 18:35

kallewoof force-pushed the unspendable-utxo-handling branch from 6f4b15c to d30f03e Compare April 24, 2018 05:39

kallewoof force-pushed the unspendable-utxo-handling branch from d30f03e to 97f4399 Compare May 22, 2018 02:50

kallewoof force-pushed the unspendable-utxo-handling branch from 7752812 to c8c147a Compare May 22, 2018 03:56

kallewoof force-pushed the unspendable-utxo-handling branch from c8c147a to b93fbc4 Compare May 22, 2018 23:31

kallewoof force-pushed the unspendable-utxo-handling branch from b93fbc4 to fdbf735 Compare May 23, 2018 00:26

jnewbery reviewed May 23, 2018

View reviewed changes

[test] Test for get(unconfirmed)balance include_unavailable in wallet…

a73ecd6

….py test.

kallewoof force-pushed the unspendable-utxo-handling branch from fdbf735 to a73ecd6 Compare May 24, 2018 04:54

DrahtBot mentioned this pull request Jun 30, 2018

Fix get balance #13566

Merged

DrahtBot mentioned this pull request Jul 11, 2018

wallet: Add GetBalances to calculate all balances #13637

Closed

DrahtBot added the Needs rebase label Jul 14, 2018

kallewoof closed this Aug 10, 2018

kallewoof reopened this Aug 11, 2018

kallewoof closed this Sep 10, 2018

kallewoof deleted the unspendable-utxo-handling branch September 10, 2018 01:54

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[wallet] getbalance: Add option to include non-mempool UTXOs #11020

[wallet] getbalance: Add option to include non-mempool UTXOs #11020

Uh oh!

Conversation

kallewoof commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 10, 2017

Uh oh!

maflcko commented Aug 10, 2017

Uh oh!

jnewbery commented Aug 10, 2017

Uh oh!

maflcko commented Aug 10, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Aug 15, 2017

Uh oh!

kallewoof commented Aug 16, 2017

Uh oh!

promag commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 17, 2017

Uh oh!

kallewoof commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 22, 2018

Uh oh!

kallewoof commented May 22, 2018

Uh oh!

kallewoof commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kallewoof commented Aug 10, 2017 •

edited

Loading

promag commented Aug 16, 2017 •

edited

Loading

kallewoof commented May 22, 2018 •

edited

Loading

kallewoof commented May 22, 2018 •

edited

Loading