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

luke-jr · 2018-10-29T16:04:27Z

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.

maflcko · 2018-10-29T16:34:45Z

src/wallet/rpcwallet.cpp

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

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

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

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

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

"*" isn't an account.

Then why would specifying it using account= make sense?

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

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

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

DrahtBot · 2018-10-29T17:03:35Z

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 · 2018-10-29T17:14:48Z

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

maflcko · 2018-10-29T17:38:11Z

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" },)

jnewbery

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.

jnewbery · 2018-10-29T20:41:10Z

src/wallet/rpcwallet.cpp

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.

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

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

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.

jnewbery · 2018-10-29T20:46:15Z

src/wallet/rpcwallet.cpp

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 7b782f5).

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

practicalswift · 2018-10-29T21:56:40Z

Concept ACK

promag · 2018-11-03T15:16:07Z

Failing with

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

luke-jr · 2018-11-03T18:13:24Z

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

meshcollider · 2018-11-09T06:12:08Z

src/wallet/rpcwallet.cpp

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

meshcollider · 2018-11-09T06:15:46Z

src/wallet/rpcwallet.cpp

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

meshcollider · 2018-11-09T06:24:07Z

src/wallet/rpcwallet.cpp

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

sipa · 2018-11-19T21:31:54Z

@luke-jr Needs rebase.

sipa · 2018-11-22T01:11:28Z

It'd be great to have this in 0.17.1.

@luke-jr Would you mind someone else rebasing this?

luke-jr · 2018-11-22T01:41:34Z

Rebased

meshcollider · 2018-11-22T02:15:01Z

Still needs comments above addressed too

luke-jr · 2018-11-22T02:31:13Z

Test issues fixed

meshcollider

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

meshcollider · 2018-11-22T07:36:51Z

src/wallet/rpcwallet.cpp

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


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

meshcollider · 2018-11-22T08:32:04Z

src/wallet/rpcwallet.cpp

+    bool trusted_only = true;
+    if (trusted_only_uv.isBool()) {
+        trusted_only = trusted_only_uv.get_bool();
+    } else if (trusted_only_uv.isStr()) {


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)

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

DrahtBot · 2018-11-23T09:22:50Z

Needs rebase

maflcko · 2018-11-23T17:54:48Z

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.

jnewbery

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

jnewbery · 2018-11-27T22:57:09Z

src/wallet/wallet.cpp

+
+            if (pcoin->GetDepthInMainChain(*locked_chain) < min_depth) continue;
+
+            nTotal += pcoin->GetAvailableCredit(*locked_chain, true, filter, min_depth);


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.

…s that were spent more recently

jnewbery

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.

jnewbery · 2018-11-28T14:13:31Z

src/wallet/wallet.cpp

-    } else if (filter == ISMINE_WATCH_ONLY) {
-        cache = &nAvailableWatchCreditCached;
-        cache_used = &fAvailableWatchCreditCached;
+    if (min_depth == 0) {


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

ryanofsky · 2018-11-28T16:57:15Z

<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?

c1825b9 [tests] Add wallet_balance.py (John Newbery) Pull request description: Adds a test specifically to test the wallet's getbalance and getunconfirmedbalance RPCs. `wallet_basic.py` is too large and should be broken down into more focused test cases. I wrote `wallet_balance.py` to test the changes in bitcoin#14602. Offering as a PR in case people think it's more generally useful. Tree-SHA512: 573ae8faf377df3d87d5112870b40690efb285fc5578fff8acc2ac1a0e4625ae65d3dfa8abbac577c87bec015038f425833783fa09f014f87906e8d098ed30d7

promag · 2018-12-03T17:31:37Z

src/wallet/rpcwallet.cpp

    { "wallet",             "encryptwallet",                    &encryptwallet,                 {"passphrase"} },
    { "wallet",             "getaddressesbylabel",              &getaddressesbylabel,           {"label"} },
    { "wallet",             "getaddressinfo",                   &getaddressinfo,                {"address"} },
-    { "wallet",             "getbalance",                       &getbalance,                    {"dummy","minconf","include_watchonly"} },


Dummy should stay otherwise it breaks calls using named parameters.

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

Just tested v0.17.0 and the following works:

bitcoin-cli -regtest --named getbalance account="*" 5097.99999647 bitcoin-cli -regtest --named getbalance dummy="*" 5097.99999647

luke-jr · 2019-01-05T11:27:50Z

Closing this for now.

luke-jr force-pushed the bugfix_rpc_getbalance_untrusted branch 2 times, most recently from 7809c34 to 0d4ff9b Compare October 29, 2018 16:06

maflcko reviewed Oct 29, 2018

View reviewed changes

DrahtBot mentioned this pull request Oct 29, 2018

docs: Consistent type names in RPC help descriptions #14459

Closed

DrahtBot mentioned this pull request Oct 29, 2018

wallet: "avoid_reuse" wallet flag for improved privacy #13756

Merged

maflcko added Wallet RPC/REST/ZMQ labels Oct 29, 2018

luke-jr force-pushed the bugfix_rpc_getbalance_untrusted branch 3 times, most recently from 34e258f to 5505437 Compare October 29, 2018 20:19

jnewbery reviewed Oct 29, 2018

View reviewed changes

luke-jr mentioned this pull request Oct 29, 2018

Wallet/RPC: sweepprivkeys method to scan UTXO set and send to local wallet #9152

Closed

meshcollider reviewed Nov 9, 2018

View reviewed changes

DrahtBot added the Needs rebase label Nov 9, 2018

meshcollider added the Needs backport label Nov 12, 2018

maflcko added this to the 0.17.1 milestone Nov 14, 2018

luke-jr force-pushed the bugfix_rpc_getbalance_untrusted branch from 5505437 to cdc9814 Compare November 22, 2018 01:41

DrahtBot removed the Needs rebase label Nov 22, 2018

Wallet: Add trusted_only flag to GetBalance

dfcfa80

This was referenced Nov 22, 2018

[0.17] Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") #14781

Closed

[0.17] Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") #14782

Closed

meshcollider reviewed Nov 22, 2018

View reviewed changes

DrahtBot added the Needs rebase label Nov 23, 2018

jnewbery reviewed Nov 27, 2018

View reviewed changes

fixup! Bugfix: Wallet: When getting balance w/ min_conf, include UTXO…

1ac717f

…s that were spent more recently

jnewbery reviewed Nov 28, 2018

View reviewed changes

jnewbery mentioned this pull request Nov 29, 2018

[tests] Add wallet_balance.py #14845

Merged

maflcko modified the milestones: 0.17.1, 0.17.2 Nov 30, 2018

promag reviewed Dec 3, 2018

View reviewed changes

gmaxwell mentioned this pull request Dec 12, 2018

rpc: Add min_conf option to fund transaction calls #14641

Closed

jnewbery mentioned this pull request Dec 20, 2018

[wallet] Fix getbalance with minconf #15010

Closed

luke-jr closed this Jan 5, 2019

fanquake removed the Needs backport label Jan 5, 2019

jnewbery mentioned this pull request Jul 2, 2019

rpc: Raise error in getbalance if minconf is not zero #15729

Closed

laanwj removed the Needs rebase label Oct 24, 2019

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


		if (pcoin->GetDepthInMainChain(*locked_chain) < min_depth) continue;

		nTotal += pcoin->GetAvailableCredit(*locked_chain, true, filter, min_depth);

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

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

Uh oh!

Conversation

luke-jr commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

meshcollider Nov 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Oct 29, 2018

Uh oh!

maflcko commented Oct 29, 2018

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!

practicalswift commented Oct 29, 2018

Uh oh!

promag commented Nov 3, 2018

Uh oh!

luke-jr commented Nov 3, 2018

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!

sipa commented Nov 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

meshcollider commented Nov 22, 2018

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Oct 29, 2018 •

edited

Loading

meshcollider Nov 10, 2018 •

edited

Loading

DrahtBot commented Oct 29, 2018 •

edited

Loading

sipa commented Nov 22, 2018 •

edited

Loading

meshcollider Nov 22, 2018 •

edited

Loading