Fix calculation of balances and available coins.

morcos commented at 1:58 pm on March 18, 2016: member

No longer consider coins which aren’t in our mempool as adding to our spendable but unconfirmed balance. Add testing of this in abandonconflict.py. Fixes #7690 (sort of)

This PR only changes the result of GetUnconfirmedBalance and AvailableCoins. Balances returned via GetAccountBalance such as the rpc call getbalance "" 0 were already unreliable in 0.11 and suffered a further regression in 0.12. It is recommended to use rpc call getunconfirmedbalance to get the unconfirmed portion of the balance.

Summary of effect on balances for various transaction types: :white_check_mark: - Reduces balance for coins spent and increases balance for coins received. :ballot_box_with_check: - Reduces balance for coins spent and increases unconfirmed balance for coins received :arrow_down: - Reduces balance for coins spent, does not affect balance for any coins received :fearful: - Increases unconfirmed balance for coins received, but does not reduce balance for coins spent :heavy_minus_sign: - no effect on balance

tx status	0.11	0.12	#7715
Confirmed	:white_check_mark:	:white_check_mark:	:white_check_mark:
Unconfirmed, in mempool	:ballot_box_with_check:	:ballot_box_with_check:	:ballot_box_with_check:
Unconfirmed, not in mempool	:heavy_minus_sign:	:ballot_box_with_check:	:arrow_down:
Unconfirmed, not in mempool, abandoned	:heavy_minus_sign:	:fearful:	:heavy_minus_sign:
Unconfirmed, not in mempool, known to conflict	:heavy_minus_sign:	:heavy_minus_sign:	:heavy_minus_sign:
Unconfirmed, not in mempool, non-final	:fearful:	:ballot_box_with_check:	:arrow_down:
Unconfirmed, not in mempool, non-final, abandoned	:fearful:	:fearful:	:heavy_minus_sign:
Unconfirmed, not in mempool, non-final, known to conflict	:fearful:	:fearful:	:heavy_minus_sign:

Fix calculation of balances and available coins.

No longer consider coins which aren't in our mempool.

Add test for regression in abandonconflict.py

68d4282774

laanwj added the label Wallet on Mar 18, 2016

morcos commented at 2:54 pm on March 21, 2016: member

@laanwj I think this PR is small enough and a clear enough improvement to a regression that it should be backported for 0.12.1.

bittylicious commented at 4:48 pm on March 21, 2016: none

I can confirm that this no longer shows the transaction that were abandoned on my client, so the patch seems to work as expected - thank you.

As you stated, getbalance doesn’t work, and I can confirm “listaccounts 0” also doesn’t work (it still shows this abandoned amount). Is there any possibility of allowing these commands to discount the abandoned transactions in another patch as I sadly rely on some deprecated accounting functionality at the moment (yes, I know I shouldn’t).

laanwj added the label Needs backport on Mar 23, 2016

laanwj commented at 12:19 pm on March 23, 2016: member

utACK 68d4282

laanwj merged this on Mar 23, 2016

laanwj closed this on Mar 23, 2016

laanwj referenced this in commit 3bdc583b3f on Mar 23, 2016

laanwj referenced this in commit 19866c1ffc on Mar 23, 2016

morcos commented at 5:57 pm on March 23, 2016: member

@bittylicious Unfortunately I don’t see the situation improving for the account balance calculations. It’s not related to abandontransaction technically, which was never meant to affect whether or not new coins are considered received, but it is related to whether newly received coins are countable in unconfirmed balance or not based on whether they are in the mempool which changed for 0.12. It’s very difficult, maybe even impossible, to do account balance calculations properly, so I would expect them to just get more unreliable until they finally disappear. This particular problem should only affect balances for 0 confirms.

laanwj commented at 7:41 am on March 24, 2016: member

Unfortunately I don’t see the situation improving for the account balance calculations

Unfortunately, I don’t see so either.

Going forward on the account deprecation is #7729, which adds a label API. Labels behave almost as accounts do, except that they have no balance. I’d like to include that for 0.13. Then at some later time (possibly 0.14), the old account API, along with everything to do with account balance computation is going to be removed.

There’s not much chance of anyone maintaining it in the mean time - some of the reasons for removing it in the first place are that the code is a mess, no one knows exactly how it behaves (nor should behave), it has some long-running problems, and thus no one wants to maintain it.

Accounts have been marked DEPRECATED since 0.11, and there was talk of doing so far before that. I’d urge you to start working on an implementation that doesn’t need them.

If there is specific functionality that doesn’t involve computing balances, but that you feel you cannot achieve without making use of accounts, do let us know (comment in #7729 or so).

bittylicious commented at 2:14 pm on March 24, 2016: none

Thanks @morcos and @laanwj for the analysis on this one. Naturally it’s a shame the unconfirmed balance will seemingly always be off, but it’s not a showstopper.

Thanks also for the estimated timescale for accounts being removed. I’m well aware of the deprecation of this functionality and there is an internal accounting mechanism there. It just needs more testing as it has a few bugs.

Ultimately, accounts are used indeed mainly for their balance. It also serves as a last chance emergency stop in case a user tries to send more coins than they have, enforced by bitcoind because the account balance would drop to zero. These can, of course, both be coded for outside of bitcoind, but it’s always been nice having this emergency. Issues like this one though do show how the accounting functionality has suffered bit rot though so I understand the need to remove it eventually.

MarcoFalke removed the label Needs backport on Apr 25, 2016

MarcoFalke commented at 1:37 pm on April 25, 2016: member

Backported in 19866c1; Removing label; Assuming this is not meant to be backported to 0.11?

thokon00 referenced this in commit 164c5197f7 on Jun 28, 2016

codablock referenced this in commit a26f97aae7 on Sep 16, 2017

codablock referenced this in commit a12bd14f4d on Sep 19, 2017

codablock referenced this in commit 924f3833d3 on Dec 9, 2017

codablock referenced this in commit f933c67266 on Dec 19, 2017

random-zebra referenced this in commit 0f1764a3db on Oct 9, 2019

MarcoFalke locked this on Sep 8, 2021

Fix calculation of balances and available coins. #7715