Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*")

luke-jr commented at 4:04 pm on October 29, 2018: member

getbalance("*") has always given the final balance ignoring whether the wallet considers coins trusted or not, while getbalance() checked the trusted status.

Furthermore, CWallet::GetBalance (the trusted-only implementation) did not support setting a min_conf parameter. This was added in #13566, but only checked min_conf for UTXO creation, not UTXO spending.

This fixes getbalance to work correctly in both regards.

It also renames the dummy argument to trusted_only and accepts a boolean instead of the fixed string "*" to fit more with the no-accounts stuff.

See also: #6276 fixed the first issue in 2015.

luke-jr force-pushed on Oct 29, 2018

in src/wallet/rpcwallet.cpp:4144 in 0d4ff9b95b outdated

3997@@ -3989,7 +3998,7 @@ static const CRPCCommand commands[] =
3998     { "wallet",             "encryptwallet",                    &encryptwallet,                 {"passphrase"} },
3999     { "wallet",             "getaddressesbylabel",              &getaddressesbylabel,           {"label"} },
4000     { "wallet",             "getaddressinfo",                   &getaddressinfo,                {"address"} },
4001-    { "wallet",             "getbalance",                       &getbalance,                    {"dummy","minconf","include_watchonly"} },
4002+    { "wallet",             "getbalance",                       &getbalance,                    {"trusted_only|account","minconf","include_watchonly"} },

MarcoFalke commented at 4:34 pm on October 29, 2018:

Should keep this as trusted_only|dummy to not break existing clients that use named args?

luke-jr commented at 5:13 pm on October 29, 2018:

Hmm. Probably not, since afaik “dummy” has only ever been used in versions where it’s actually broken anyway?

meshcollider commented at 6:12 am on November 9, 2018:

I agree that dummy as a named arg doesn’t need to be kept around, but if account was already removed as an alias, why add it back? This should just be trusted_only I think?

This is also the cause of the linter error, because trusted_only has an entry in vRPCConvertParams but account doesn’t

luke-jr commented at 10:15 am on November 9, 2018:

Removing account appears to have been a bug, since we still accept "*" as a value?

meshcollider commented at 11:14 am on November 9, 2018:

@luke-jr only as a dummy, hence the name, accounts are completely removed

luke-jr commented at 11:58 am on November 9, 2018:

Existing code calling getbalance(account="*") shouldn’t be broken…

meshcollider commented at 12:50 pm on November 9, 2018:

@luke-jr yes it should in 0.18 and in 0.17 without explicitly enabling deprecated accounts?

luke-jr commented at 1:00 pm on November 9, 2018:

“*” isn’t an account.

meshcollider commented at 10:08 pm on November 9, 2018:

Then why would specifying it using account= make sense?

luke-jr commented at 11:23 pm on November 9, 2018:

Because that’s what the parameter was called when named args were introduced.

meshcollider commented at 10:43 am on November 10, 2018:

Well, that name was already removed, so NACK on adding it back again, it makes no sense

luke-jr commented at 2:30 am on November 22, 2018:

Removing it introduced a bug by breaking existing code that uses the "*" mode.

DrahtBot commented at 5:03 pm on October 29, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14726 (Use RPCHelpMan for all RPCs by MarcoFalke)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
#14459 (More RPC help description fixes by ch4ot1c)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy 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.

luke-jr commented at 5:14 pm on October 29, 2018: member

I don’t understand what the Travis linter is trying to complain about.

MarcoFalke commented at 5:38 pm on October 29, 2018: member

I think the linter doesn’t understand that an option can have two different types (and wants you to specify both conversions similar to { "getblock", 1, "verbosity" }, { "getblock", 1, "verbose" },)

MarcoFalke added the label Wallet on Oct 29, 2018

MarcoFalke added the label RPC/REST/ZMQ on Oct 29, 2018

luke-jr force-pushed on Oct 29, 2018

in src/wallet/rpcwallet.cpp:724 in 5505437e8b outdated

708@@ -709,9 +709,18 @@ static UniValue getbalance(const JSONRPCRequest& request)
709 
710     LOCK2(cs_main, pwallet->cs_wallet);
711 
712-    const UniValue& dummy_value = request.params[0];
713-    if (!dummy_value.isNull() && dummy_value.get_str() != "*") {
714-        throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\".");
715+    const UniValue& trusted_only_uv = request.params[0];
716+    bool trusted_only = true;
717+    if (trusted_only_uv.isBool()) {

jnewbery commented at 8:41 pm on October 29, 2018:

I think we should try to avoid having a single argument accept two different types. It’s messy and I don’t think it’s supported by the CRPConvertTable logic.

luke-jr commented at 9:03 pm on October 29, 2018:

It’s only a boolean. The string stuff is strictly for backward compatibility.

meshcollider commented at 6:24 am on November 9, 2018:

Agree with @jnewbery, because you’ve added the argument to vRPCConvertParams it’ll try and convert it from JSON despite it being a string? Have you tested this backwards compatibility? IMO if this is going to be done, we just drop the string part completely and put it in release notes as a breaking change

luke-jr commented at 10:14 am on November 9, 2018:

bitcoin-cli is just a testing tool. As such, I don’t think we need to worry about backward compatibility there.

This is fully backward compatible with any normal JSON-RPC client.

in src/wallet/rpcwallet.cpp:728 in 5505437e8b outdated

716+    bool trusted_only = true;
717+    if (trusted_only_uv.isBool()) {
718+        trusted_only = trusted_only_uv.get_bool();
719+    } else if (trusted_only_uv.isStr()) {
720+        // backward compatibility
721+        if (trusted_only_uv.get_str() != "*") {

jnewbery commented at 8:46 pm on October 29, 2018:

I’d prefer to drop this undocumented functionality if at all possible. It’s confusing that getbalance and getbalance * should return different values (and this argument has been deprecated since https://github.com/bitcoin/bitcoin/commit/7b782f5b01f4c2d906a28800d01ffd05ad257cbe).

luke-jr commented at 9:04 pm on October 29, 2018:

They always have returned different values. I think the API changes here clear up the confusion?

jnewbery commented at 8:47 pm on October 29, 2018: member

Concept ACK adding an option to return a balance including untrusted transactions.

Thanks for catching the missing min_depth check in IsSpent.

I think this could do with some additional test cases in the wallet_basic.py test file.

practicalswift commented at 9:56 pm on October 29, 2018: contributor

Concept ACK

promag commented at 3:16 pm on November 3, 2018: member

Failing with

0* Checking consistency between dispatch tables and vRPCConvertParams
1ERROR: getbalance argument ['trusted_only', 'account'] has conflicts in vRPCConvertParams conversion specifier [True, False]

luke-jr commented at 6:13 pm on November 3, 2018: member

@promag That’s a linter bug, and IMO outside the scope of this PR to fix.

It is correct that trusted_only is a JSON/converted parameter, and the account alias is a String parameter.

in src/wallet/rpcwallet.cpp:729 in 5505437e8b outdated

717+    if (trusted_only_uv.isBool()) {
718+        trusted_only = trusted_only_uv.get_bool();
719+    } else if (trusted_only_uv.isStr()) {
720+        // backward compatibility
721+        if (trusted_only_uv.get_str() != "*") {
722+            throw JSONRPCError(RPC_METHOD_DEPRECATED, "accounts no longer supported; set 1st field to trusted_only");

meshcollider commented at 6:15 am on November 9, 2018:

Tests are failing because this error message has been changed, you’ll either need to update the tests or just change it back to "dummy first argument must be excluded or set to \"*\"."

DrahtBot added the label Needs rebase on Nov 9, 2018

meshcollider added the label Needs backport on Nov 12, 2018

MarcoFalke added this to the milestone 0.17.1 on Nov 14, 2018

sipa commented at 9:31 pm on November 19, 2018: member

@luke-jr Needs rebase.

sipa commented at 1:11 am on November 22, 2018: member

It’d be great to have this in 0.17.1. @luke-jr Would you mind someone else rebasing this?

luke-jr commented at 1:41 am on November 22, 2018: member

Rebased

luke-jr force-pushed on Nov 22, 2018

meshcollider commented at 2:15 am on November 22, 2018: contributor

Still needs comments above addressed too

DrahtBot removed the label Needs rebase on Nov 22, 2018

luke-jr commented at 2:31 am on November 22, 2018: member

Test issues fixed

Wallet: Add trusted_only flag to GetBalance dfcfa807c9

Bugfix: RPC/Wallet: Restore getbalance("*") behaviour (include untrusted coins), while updating the API to accept a boolean as well 4996d9813e

Bugfix: Wallet: When getting balance w/ min_conf, include UTXOs that were spent more recently cfa948da1c

luke-jr force-pushed on Nov 22, 2018

in src/wallet/rpcwallet.cpp:729 in 4996d9813e outdated

727+    if (trusted_only_uv.isBool()) {
728+        trusted_only = trusted_only_uv.get_bool();
729+    } else if (trusted_only_uv.isStr()) {
730+        // backward compatibility
731+        if (trusted_only_uv.get_str() != "*") {
732+            throw JSONRPCError(RPC_METHOD_DEPRECATED, "accounts no longer supported; set 1st field to trusted_only");

meshcollider commented at 7:36 am on November 22, 2018:

IMO this would be better just to give a type error, that it should be boolean not string

in src/wallet/rpcwallet.cpp:726 in cfa948da1c outdated

724-        throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\".");
725+    const UniValue& trusted_only_uv = request.params[0];
726+    bool trusted_only = true;
727+    if (trusted_only_uv.isBool()) {
728+        trusted_only = trusted_only_uv.get_bool();
729+    } else if (trusted_only_uv.isStr()) {

meshcollider commented at 8:32 am on November 22, 2018:

This still won’t work because of the entry in vRPCConvertParams. I don’t think a single parameter can accept either a string or a boolean in this way, because there is no way to tell otherwise if true is supposed to be a string "true" (which should error) or the boolean value true (which should work). I think the only options are either to break compatibility and drop the "*" case in favor of the boolean only, or not switch to a boolean and just accept "*" and "" (empty string)

luke-jr commented at 9:31 am on November 22, 2018:

It works fine… the lookup table has no effect on the JSON-RPC server, only the bitcoin-cli testing tool.

meshcollider commented at 8:32 am on November 22, 2018: contributor

Thanks for splitting out the backport as we discussed on IRC.

DrahtBot commented at 9:22 am on November 23, 2018: member

DrahtBot added the label Needs rebase on Nov 23, 2018

MarcoFalke commented at 5:54 pm on November 23, 2018: member

Would be nice to have a basic test that covers these changes. Otherwise a future refactoring or “bug fix” will likely break this again and all effort is undone.

in src/wallet/wallet.cpp:2058 in cfa948da1c outdated

2055+                if (!CheckFinalTx(*pcoin->tx)) continue;
2056             }
2057+
2058+            if (pcoin->GetDepthInMainChain(*locked_chain) < min_depth) continue;
2059+
2060+            nTotal += pcoin->GetAvailableCredit(*locked_chain, true, filter, min_depth);

jnewbery commented at 10:57 pm on November 27, 2018:

This won’t work. GetAvailableCredit() uses the nAvailableCreditCached cache, so it doesn’t matter what value is passed in as min_depth here after the first call.

jnewbery commented at 11:05 pm on November 27, 2018: member

I’ve spent several hours reviewing and writing tests for this, and I’m now more convinced that we should try to remove the getbalance * usage. My tests are in this branch: https://github.com/jnewbery/bitcoin/tree/untrusted_balance. When I run them against a slightly modified version of this branch, there’s all kinds of unintuitive behaviour like double counting conflicting transactions, or counting a transaction and its spending transaction. If I run the test against v0.16.3 I see other strange behaviour like negative balances.

The behaviour with this branch is different from v0.16.3, but I don’t think either are good. It makes sense for there to be a method to see untrusted transactions, but summing them into a balance is dangerous and confusing for users.

Can we step back and answer what use case we’re trying to support here? Why do we want to support a non-isTrusted balance? getbalance(minconf=0) already supports showing the unconfirmed balance, with the caveat that it only includes trusted unconfirmed txs (ie ones where all the inputs are from us and the tx is in the mempool).

fixup! Bugfix: Wallet: When getting balance w/ min_conf, include UTXOs that were spent more recently 1ac717f8d8

in src/wallet/wallet.cpp:1877 in 1ac717f8d8

1878-        cache = &nAvailableCreditCached;
1879-        cache_used = &fAvailableCreditCached;
1880-    } else if (filter == ISMINE_WATCH_ONLY) {
1881-        cache = &nAvailableWatchCreditCached;
1882-        cache_used = &fAvailableWatchCreditCached;
1883+    if (min_depth == 0) {

jnewbery commented at 2:13 pm on November 28, 2018:

Please at least add a comment here to explain why you’re not hitting/updating the cache if min_depth != 0

jnewbery commented at 2:21 pm on November 28, 2018: member

This PR needs a few things before I’d consider it ready for merge:

A rationale for why we’d want to provide this ‘feature’. Summing up untrusted coins into a ‘balance’ can lead to very confusing results, such as negative balances and double-counting conflicting transactions. An uninformed user could see money ‘disappear’ or be tricked into thinking they’ve received money which they haven’t.
Full documentation of the usage. I removed getbalance * here: https://github.com/bitcoin/bitcoin/pull/12953/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceL822 because the option had been marked as deprecated. If we want to support this usage, then it should be properly documented, with appropriate warnings and caveats.
Full test coverage, so this doesn’t get regressed again. I wrote some tests here: https://github.com/jnewbery/bitcoin/tree/untrusted_balance in the course of reviewing this PR.
Not gratuitously breaking bitcoin-cli because you don’t happen to use it.

ryanofsky commented at 4:57 pm on November 28, 2018: member

<luke-jr> jnewbery: making it impossible to get the current balance (ie, with untrusted txs included) is IMO not acceptable @luke-jr I agree with this, but I don’t understand if adding up values of getbalance and getunconfirmedbalance as an alternative would give the wrong result, or if it would give the right result, but be less than ideal because it’s inconvenient or not atomic. Do you have thoughts on this?

MarcoFalke removed this from the milestone 0.17.1 on Nov 30, 2018

MarcoFalke added this to the milestone 0.17.2 on Nov 30, 2018

MarcoFalke referenced this in commit 81bd349c9c on Nov 30, 2018

in src/wallet/rpcwallet.cpp:4135 in 1ac717f8d8

4140@@ -4132,7 +4141,7 @@ static const CRPCCommand commands[] =
4141     { "wallet",             "encryptwallet",                    &encryptwallet,                 {"passphrase"} },
4142     { "wallet",             "getaddressesbylabel",              &getaddressesbylabel,           {"label"} },
4143     { "wallet",             "getaddressinfo",                   &getaddressinfo,                {"address"} },
4144-    { "wallet",             "getbalance",                       &getbalance,                    {"dummy","minconf","include_watchonly"} },

promag commented at 5:31 pm on December 3, 2018:

Dummy should stay otherwise it breaks calls using named parameters.

luke-jr commented at 6:00 pm on December 3, 2018:

“dummy” has NEVER worked as a named parameter. The only calls using named parameters where this ever worked, used the name “account”

promag commented at 10:48 pm on December 3, 2018:

Just tested v0.17.0 and the following works:

0bitcoin-cli -regtest --named getbalance account="*"
15097.99999647
2bitcoin-cli -regtest --named getbalance dummy="*"
35097.99999647

luke-jr commented at 11:27 am on January 5, 2019: member

Closing this for now.

luke-jr closed this on Jan 5, 2019

fanquake removed the label Needs backport on Jan 5, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

Munkybooty referenced this in commit 64654ce05d on Jul 30, 2021

Munkybooty referenced this in commit 565ac46233 on Aug 17, 2021

vijaydasmp referenced this in commit 45568a8227 on Sep 8, 2021

vijaydasmp referenced this in commit 8b8a1156cb on Sep 8, 2021

vijaydasmp referenced this in commit 0aa02073e8 on Sep 8, 2021

vijaydasmp referenced this in commit 819e3e4931 on Sep 9, 2021

vijaydasmp referenced this in commit 0a9559cdd9 on Sep 9, 2021

DrahtBot locked this on Dec 16, 2021

Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") #14602

Conflicts