[wallet] Fix getbalance with minconf

jnewbery commented at 4:20 pm on December 20, 2018: member

When getting balance w/ min_conf, include UTXOs that were spent more recently.

When getting balance w/ min_conf, include UTXOs that were spent more
recently.

14130114d5

jnewbery commented at 4:21 pm on December 20, 2018: member

#14602 has required rebase for a few weeks and has several review comments unanswered.

This PR extracts the non-contentious bugfix from that PR and adds testing and commenting.

DrahtBot commented at 6:30 pm on December 20, 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:

#15596 (rpc: Ignore sendmany::minconf as dummy value by MarcoFalke)
#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.

Empact commented at 10:17 pm on December 20, 2018: member

I may be off base here, but looking at CWallet::AvailableCoins makes me think that some other argument is needed, to separately address:

coins which have received at least N confirmations, relative to the current moment
coins which were spendable M blocks ago

To me, the getbalance docs do not seem to indicate that coins spent in the past N blocks will be included in the balance. I would expect setting an earlier minconf there to uniformy decrease my balance (by windowing down to coins confirmed at greater depth / available with higher confidence), not to provide a window into the past (by excluding more recent spends).

fanquake added the label Wallet on Dec 20, 2018

in test/functional/wallet_balance.py:94 in 14130114d5

93-        # TODO: fix getbalance tracking of coin spentness depth
94-        assert_equal(self.nodes[0].getbalance(minconf=1), Decimal('0'))
95-        assert_equal(self.nodes[1].getbalance(minconf=1), Decimal('0'))
96+        # getbalance with a minconf includes coins that have been spent more recently than the minconf blocks ago
97+        assert_equal(self.nodes[0].getbalance(minconf=1), Decimal('50'))
98+        assert_equal(self.nodes[1].getbalance(minconf=1), Decimal('50'))

luke-jr commented at 3:00 am on December 28, 2018:

This is still the incorrect behaviour, I think. The correct values that should be returned here (at least with dummy="*") are 9.99 and -10.01, respectively. This is because sent transactions are always excluded from the balance, but incoming transactions are only included once they have sufficiently confirmed.

laanwj commented at 1:03 pm on January 2, 2019: member

Can you have a look here @MeshCollider? If this is a non-contentious bugfix we should probably try to get this in.

meshcollider commented at 11:20 am on January 3, 2019: contributor

This change looks sensible to me, but luke’s comment above hasn’t been addressed yet

ryanofsky commented at 1:24 pm on January 3, 2019: member

Catching up on this, but IIRC, the last time this issue was discussed, http://www.erisian.com.au/bitcoin-core-dev/log-2018-11-29.html#l-479, the main problem was considered to be:

your balance suddenly showing zero when you make a small payment.. because now your change is unconfirmed

which would be fixed by implementing:

desired behavior is that balances shows confirmed + trusted-in-mempool-0conf. And that you be able to adjust the threshold for confirmed and turn the trusted-in-mempool-zeroconf on and off. And if you make the confirmed=0, it should still only count in-mempool

It seems like this PR is addressing the problem in a different way. Instead of treating not fully confirmed spends from the wallet as trusted (and therefore including change in the balance), it is treating not fully confirmed spends as actually unspent (and therefore including the whole output value in the balance)?

It’s possible I’m confused, but it would help if there was an explanation of how this fix relates to previous discussion (or doesn’t relate, if it is just a tangential bugfix).

jnewbery commented at 12:32 pm on January 5, 2019: member

This is the fix from https://github.com/bitcoin/bitcoin/pull/14602/commits/1ac717f8d836b16d5cb808c07f0578a309f4e9d0 , but with test cases and commenting added. From that PR:

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.

I don’t understand Luke’s assertion that the getbalance value should be 9.99 and -10.01. Displaying negative balance doesn’t make sense to me.

The behaviour should now be that when calling getbalance with a minconf value:

coins that have received but have fewer than minconf confirmations are excluded
coins that have been spent but have fewer than minconf confirmations are included

Effectively, this gives the balance at (TIP - minconf) height.

That behaviour makes sense to me, and I think best reflects the description in the RPC help for minconf: “Only include transactions confirmed at least this many times.” It was what I understood commit 1ac717f8d836b16d5cb808c07f0578a309f4e9d0 in #14602 was attempting.

If people disagree that this is the desired behaviour, then I can amend or close this PR. Also happy to add more documentation if people think that’s required.

luke-jr commented at 1:22 pm on January 5, 2019: member

It’s not supposed to give the balance at (TIP - minconf) height, though. As someone pointed out at the meeting weeks ago, my fix didn’t fix it correctly.

If you receive 50 BTC (unconfirmed), then spend the 50 BTC, this should show -50 because the receive has not confirmed, yet you still sent 50 BTC.

jnewbery commented at 2:52 pm on January 7, 2019: member

If you receive 50 BTC (unconfirmed), then spend the 50 BTC, this should show -50

This really doesn’t make sense to me. How can one have a negative bitcoin balance? What happens if the unconfirmed spend is bumped? Should the balance then be -100, or should it be -50?

To me, the only logically consistent and intuitive behaviour is to show the balance at (TIP - minconf), but this is really a question of concept and design, so I’d appreciate input from others, perhaps @MeshCollider and @laanwj.

promag commented at 2:59 pm on January 7, 2019: member

If you receive 50 BTC (unconfirmed), then spend the 50 BTC, this should show -50

If unconfirmed credit does’t add up, then unconfirmed debit shouldn’t count as well?

sipa commented at 3:14 pm on January 7, 2019: member

Traditionally I think we’ve always counted balance as the sum of incoming UTXOs with sufficient confirmations, but which haven’t been spent afterwards (even with lower confirmations). That means it’s bounded by the balance you had minconf blocks ago, but can be less.

luke-jr commented at 3:57 pm on January 7, 2019: member

From what I can tell, spends have always been counted, even when their inputs aren’t.

ryanofsky commented at 5:27 pm on January 15, 2019: member

It might help if the PR description gave a specific example to think about. E.g. if I received 5 BTC at depth 3 and spent 4 BTC at depth 2, would getbalance(minconf=3) would show a balance of 1 before this PR and 5 after?

If so, this would be changing the interpretation of minconf from “If I only trust blocks with depth >= minconf, how much money can I be sure I have?” to “In a hypothetical universe where only transactions in blocks >= minconf exist and there were no other signed transactions, how much money would I have?”

It does seem like the answer to the first question would be more useful than the answer to the second question.

jnewbery commented at 9:21 pm on March 26, 2019: member

@ryanofsky

It might help if the PR description gave a specific example to think about. E.g. if I received 5 BTC at depth 3 and spent 4 BTC at depth 2, would getbalance(minconf=3) would show a balance of 1 before this PR and 5 after?

You need to consider the UTXO model here, rather than just balances. Here’s your scenario:

depth 3: receive a 5 BTC UTXO (a) depth 2: spend the 5 BTC UTXO (a) - create a 4 BTC payment UTXO (b) - receive a 1 BTC change UTXO (c)

Before this PR, getbalance(minconf=3) would show a balance of 0: (a) is not included because it has been spent, and (c) is not included because it does not have the necessary number of confirmations. After this PR, getbalance(minconf=3) would show a balance of 5, because (a) has the necessary number of confirmations and (b) and (c) don’t. @sipa:

I think we’ve always counted balance as the sum of incoming UTXOs with sufficient confirmations, but which haven’t been spent afterwards (even with lower confirmations).

In the scenario above, would you include the change output (c) in your balance? ie amend your definition to be: (sum of incoming UTXOs with sufficient confirmations which haven’t been spent afterwards) + (sum of change UTXOs). In that case, should I count change transactions where the input UTXO has fewer confirmations than minconf?

To me, a balance in Bitcoin can only make sense at a given block (ie given the UTXO set associated with this block, what is the total amount of UTXOs that I can sign for?) Any alternative definition (eg the amount of UTXOs that I can sign for less those UTXOs where I have evidence they can be spent, etc) leads to confusing inconsistencies like the example above.

ryanofsky commented at 11:21 pm on March 26, 2019: member

Thanks for the explanation.

To me, a balance in Bitcoin can only make sense at a given block (ie given the UTXO set associated with this block, what is the total amount of UTXOs that I can sign for?)

This definition of a balance does make sense, but it seems not very useful, and even dangerous in the example above, where somebody might think they have 5 BTC to spend instead of 1.

I think the definition of a minconf balance that’d be most useful would be the answer to the question in #15010 (comment), “If I only trust blocks with depth >= minconf, how much money can I be sure I have?” It might not be practical to compute this with all conflicting spends that may be in the wallet. But if we only consider non-conflicting spends in the chain and mempool, it seems like we could start including outputs like (c) from the example. Maybe a modified IsTrusted check would work, that makes sure every transaction input is from the wallet and either has depth greater than minconf, or recursively satisfies the same check.

jnewbery commented at 7:53 pm on March 28, 2019: member

“If I only trust blocks with depth >= minconf, how much money can I be sure I have?”

The answer is always 0, since there may be unconfirmed spends that the wallet is not aware of. Perhaps that’s a bit of a legalistic answer, but the only thing that brings us sureness in Bitcoin is confirmations. Talking about a balance outside of confirmed transactions is dangerous in my opinion.

Maybe a modified IsTrusted check would work, that makes sure every transaction input is from the wallet and either has depth greater than minconf, or recursively satisfies the same check.

My view is that getbalance is a user-facing interface, and so the definition of balance should be something that is clear, unambiguous and easily understandable by users. I don’t think the definition above meets that criteria. Saying “we’ll include change outputs, but only if all of the inputs to the tx that created that output have more than minconf confirmations” is too confusing for most users.

ryanofsky commented at 9:36 pm on March 28, 2019: member

The answer is always 0, since there may be unconfirmed spends that the wallet is not aware of. Perhaps that’s a bit of a legalistic answer, but the only thing that brings us sureness in Bitcoin is confirmations. Talking about a balance outside of confirmed transactions is dangerous in my opinion.

That’s fair. I didn’t specify the assumptions I was making with my suggested implementation. It would give the balance assuming you only created normal transactions with the bitcoin wallet and didn’t broadcast conflicting transactions, or export your keys somewhere, etc. It would be possible to do more work and come up with an implementation that made fewer assumptions (like only assuming that the wallet knows about conflicting transactions, not that they were never broadcast) to produce a more useful minconf balance, but this may also be too dangerous, or just not worth the effort to implement.

In any case, I don’t think there is an argument for minconf to be interpreted the way this PR currently interprets it, which is like a request for an old balance from a point in history. So I might suggest closing this PR. Or if you think this behavior is useful, it might be better to expose it as a new RPC like gethistoricbalance and take a height argument rather than a depth argument.

But I guess I still would be curious if @sipa has a different answer to your questions from #15010 (comment).

Also, I was surprised to see the conclusion the last time this topic was discussed in IRC (http://www.erisian.com.au/bitcoin-core-dev/log-2018-11-29.html#l-596), since it seems like it is basically just requesting the current getbalance(minconf=0) behavior:

<gmaxwell> Right, I think the desired behavior is that balances shows confirmed + trusted-in-mempool-0conf

But it was also interesting to read because there are other potential getbalance improvements discussed there.

luke-jr commented at 0:56 am on March 29, 2019: member

Perhaps we should just disable minconf entirely if we’re not going to support the current definition… Changing the meaning of stuff is pretty bad. If a new definition is desired, it can be introduced as a new RPC or something that old software won’t accidentally hit.

jnewbery commented at 9:43 pm on March 29, 2019: member

ok, I’m going to close this for now. I don’t think the existing behaviour is correct, consistent, or easily understandable by users. This offers an improvement because at the very least it can be documented and easily explained: #15010 (comment). I’ve offered to change this PR if we can agree on a clear and full description of what the replacement behaviour should be, but I haven’t seen a response to that, or answers to the questions about edge cases.

jnewbery closed this on Mar 29, 2019

MarcoFalke locked this on Dec 16, 2021

[wallet] Fix getbalance with minconf #15010

Conflicts