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

kallewoof commented at 8:06 am on August 10, 2017: member

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.

fanquake added the label Wallet on Aug 10, 2017

kallewoof force-pushed on Aug 10, 2017

kallewoof commented at 9:06 am on August 10, 2017: member

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?

MarcoFalke commented at 9:36 am on August 10, 2017: member

The zapwallettxes’ failure is intermittent. Respinned travis.

jnewbery commented at 3:39 pm on August 10, 2017: member

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)

MarcoFalke commented at 8:03 pm on August 10, 2017: member

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

in src/wallet/wallet.h:943 in 5b644c85b7 outdated

939@@ -940,7 +940,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
940     void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override;
941     // ResendWalletTransactionsBefore may only be called if fBroadcastTransactions!
942     std::vector<uint256> ResendWalletTransactionsBefore(int64_t nTime, CConnman* connman);
943-    CAmount GetBalance() const;
944+    CAmount GetBalance(bool ignoreUnspendable = true) const;

promag commented at 0:37 am on August 12, 2017:

Nit, snake case.

in src/wallet/wallet.h:468 in 5b644c85b7 outdated

464@@ -465,7 +465,7 @@ class CWalletTx : public CMerkleTx
465     bool IsEquivalentTo(const CWalletTx& tx) const;
466 
467     bool InMempool() const;
468-    bool IsTrusted() const;
469+    bool IsTrusted(bool ignoreUnspendable = true) const;

promag commented at 0:38 am on August 12, 2017:

Nit, snake case.

kallewoof commented at 1:37 am on August 15, 2017:

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

in test/functional/wallet.py:396 in 5b644c85b7 outdated

391@@ -392,6 +392,8 @@ def run_test(self):
392         extra_txid = self.nodes[0].sendtoaddress(sending_addr, Decimal('0.0001'))
393         assert(extra_txid not in self.nodes[0].getrawmempool())
394         assert(extra_txid in [tx["txid"] for tx in self.nodes[0].listtransactions()])
395+        # The change output for extra_txid should be in the balance if we include unspendable
396+        assert(self.nodes[0].getbalance() < self.nodes[0].getbalance(include_unspendable=True))

promag commented at 0:42 am on August 12, 2017:

Assert exact value?

jnewbery commented at 3:20 pm on August 14, 2017:

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

promag commented at 1:53 am on August 15, 2017:

Asserting on exact values can make the test more brittle.

Is that a bad thing?

kallewoof commented at 3:38 am on August 15, 2017:

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

promag commented at 2:01 pm on August 16, 2017:

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

in src/wallet/wallet.cpp:1826 in 5b644c85b7 outdated

1822@@ -1823,7 +1823,7 @@ bool CWalletTx::InMempool() const
1823     return mempool.exists(GetHash());
1824 }
1825 
1826-bool CWalletTx::IsTrusted() const
1827+bool CWalletTx::IsTrusted(bool ignoreUnspendable) const

promag commented at 0:44 am on August 12, 2017:

Nit, snake case.

jnewbery commented at 3:21 pm on August 14, 2017:

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

in src/wallet/wallet.cpp:1839 in 5b644c85b7 outdated

1836@@ -1837,7 +1837,7 @@ bool CWalletTx::IsTrusted() const
1837         return false;
1838 
1839     // Don't trust unconfirmed transactions from us unless they are in the mempool.

promag commented at 0:59 am on August 12, 2017:

Update comment.

promag commented at 0:59 am on August 12, 2017: member

Should add the same option to getunconfirmedbalance?

in src/wallet/rpcwallet.cpp:758 in 5b644c85b7 outdated

754@@ -755,6 +755,8 @@ UniValue getbalance(const JSONRPCRequest& request)
755             "                     avoid passing this argument.\n"
756             "2. minconf           (numeric, optional, default=1) Only include transactions confirmed at least this many times.\n"
757             "3. include_watchonly (bool, optional, default=false) Also include balance in watch-only addresses (see 'importaddress')\n"
758+            "4. include_unspendable (bool, optional, default=false) Also include balance for UTXOs which are currently not (but\n"

jnewbery commented at 3:11 pm on August 14, 2017:

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.

kallewoof commented at 1:40 am on August 15, 2017:

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

promag commented at 2:03 pm on August 16, 2017:

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

kallewoof commented at 5:16 am on August 17, 2017:

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

in src/wallet/rpcwallet.cpp:774 in 5b644c85b7 outdated

769@@ -768,8 +770,10 @@ UniValue getbalance(const JSONRPCRequest& request)
770 
771     LOCK2(cs_main, pwallet->cs_wallet);
772 
773-    if (request.params.size() == 0)
774-        return  ValueFromAmount(pwallet->GetBalance());
775+    bool ignoreUnspendable = request.params[3].isNull() || !request.params[3].get_bool();
776+    if ((request.params[0].isNull() || (request.params[0].get_str() == "" && !ignoreUnspendable)) && request.params[1].isNull() && request.params[2].isNull()) {

jnewbery commented at 3:17 pm on August 14, 2017:

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?

kallewoof commented at 1:43 am on August 15, 2017:

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="*".

jnewbery commented at 2:02 pm on August 15, 2017:

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.

in src/wallet/rpcwallet.cpp:773 in 5b644c85b7 outdated

769@@ -768,8 +770,10 @@ UniValue getbalance(const JSONRPCRequest& request)
770 
771     LOCK2(cs_main, pwallet->cs_wallet);
772 
773-    if (request.params.size() == 0)
774-        return  ValueFromAmount(pwallet->GetBalance());
775+    bool ignoreUnspendable = request.params[3].isNull() || !request.params[3].get_bool();

jnewbery commented at 3:18 pm on August 14, 2017:

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?

kallewoof commented at 1:41 am on August 15, 2017:

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

jnewbery commented at 3:22 pm on August 14, 2017: member

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

kallewoof commented at 2:27 am on August 15, 2017: member

@promag @jnewbery Thanks for the review!

https://github.com/bitcoin/bitcoin/pull/11020/commits/28e8271424b8642e825889bfaa8e51ae6a2a60dd fixes comment
https://github.com/bitcoin/bitcoin/pull/11020/commits/df7228d5dec184c92f7190265afb2c380da0a9ae 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.
https://github.com/bitcoin/bitcoin/pull/11020/commits/80acc02b0e77791605eb8f0cb20263986ea87399 adds the feature to getunconfirmedbalance (tested in https://github.com/bitcoin/bitcoin/pull/11020/commits/36423983a1b1a473151056bb5d4e3159975c9b18). 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 commented at 5:21 am on August 16, 2017: member

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

promag commented at 2:18 pm on August 16, 2017: member

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 commented at 5:12 am on August 17, 2017: member

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 force-pushed on Aug 17, 2017

kallewoof force-pushed on Oct 11, 2017

kallewoof force-pushed on Dec 5, 2017

kallewoof force-pushed on Feb 20, 2018

kallewoof force-pushed on Mar 6, 2018

kallewoof force-pushed on Apr 24, 2018

kallewoof force-pushed on May 22, 2018

kallewoof commented at 2:52 am on May 22, 2018: member

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.

kallewoof force-pushed on May 22, 2018

jnewbery commented at 3:10 pm on May 22, 2018: member

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

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

kallewoof force-pushed on May 22, 2018

kallewoof commented at 11:33 pm on May 22, 2018: member

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

kallewoof force-pushed on May 23, 2018

in src/wallet/wallet.cpp:2045 in fdbf735a7c outdated

2041@@ -2042,7 +2042,7 @@ bool CWalletTx::InMempool() const
2042     return fInMempool;
2043 }
2044 
2045-bool CWalletTx::IsTrusted() const
2046+bool CWalletTx::IsTrusted(bool trustUnavailable) const

jnewbery commented at 3:32 pm on May 23, 2018:

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

in src/wallet/wallet.cpp:2169 in fdbf735a7c outdated

2168         LOCK2(cs_main, cs_wallet);
2169         for (const auto& entry : mapWallet)
2170         {
2171             const CWalletTx* pcoin = &entry.second;
2172-            if (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0 && pcoin->InMempool())
2173+            if (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0 && (includeUnavailable || pcoin->InMempool()))

jnewbery commented at 3:33 pm on May 23, 2018:

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.

kallewoof commented at 4:47 am on May 24, 2018:

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

in src/wallet/rpcwallet.cpp:899 in fdbf735a7c outdated

929     // the user could have gotten from another RPC command prior to now
930     pwallet->BlockUntilSyncedToCurrentChain();
931 
932     LOCK2(cs_main, pwallet->cs_wallet);
933 
934+    // params[0] is not converted from JSON until account stuff is removed, so we need to manually convert it here

jnewbery commented at 3:37 pm on May 23, 2018:

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.

kallewoof commented at 4:48 am on May 24, 2018:

Good point!

in src/wallet/rpcwallet.cpp:839 in fdbf735a7c outdated

869-        );
870+    if (request.fHelp || (request.params.size() > 3 && IsDeprecatedRPCEnabled("accounts")) || (request.params.size() > 1 && !IsDeprecatedRPCEnabled("accounts"))) {
871+        if (IsDeprecatedRPCEnabled("accounts")) {
872+            throw std::runtime_error(
873+                "getbalance ( \"account\" minconf include_watchonly )\n"
874+                "\nIf account is not specified, returns the server's total available balance.\n"

jnewbery commented at 4:29 pm on May 23, 2018:

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 …”

in src/wallet/rpcwallet.cpp:901 in fdbf735a7c outdated

931 
932     LOCK2(cs_main, pwallet->cs_wallet);
933 
934+    // params[0] is not converted from JSON until account stuff is removed, so we need to manually convert it here
935+    // TODO: add param[0] to clients.cpp for getbalance and switch to get_bool() here, once account stuff is removed
936+    const bool include_unavailable = !IsDeprecatedRPCEnabled("accounts") && !request.params[0].isNull() && (request.params[0].get_str() == "1" || request.params[0].get_str() == "true");

jnewbery commented at 4:45 pm on May 23, 2018:

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.

kallewoof commented at 4:51 am on May 24, 2018:

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.

[wallet] Add option to get(unconfirmed)balance to include currently unspendable 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.

7070fcc08c

[test] Test for get(unconfirmed)balance include_unavailable in wallet.py test. a73ecd6ca6

kallewoof force-pushed on May 24, 2018

kallewoof commented at 4:57 am on May 24, 2018: member

@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 commented at 11:23 am on July 14, 2018: member

DrahtBot added the label Needs rebase on Jul 14, 2018

jnewbery commented at 2:42 am on July 16, 2018: member

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 commented at 10:45 am on July 17, 2018: member

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

kallewoof commented at 3:16 am on August 10, 2018: member

Happy birthday. Closing due to lack of interest.

kallewoof closed this on Aug 10, 2018

jnewbery commented at 6:30 pm on August 10, 2018: member

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

kallewoof commented at 5:02 am on August 11, 2018: member

@jnewbery OK!

kallewoof reopened this on Aug 11, 2018

kallewoof commented at 1:54 am on September 10, 2018: member

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

kallewoof closed this on Sep 10, 2018

kallewoof deleted the branch on Sep 10, 2018

jnewbery commented at 2:26 am on September 10, 2018: member

Sorry for wasting your time!

Not wasted. No need to apologise!

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

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