Show immature mined balance #831

sje397 commented at 2:25 PM on February 13, 2012: contributor

...only if greater than zero.

gmaxwell commented at 2:31 PM on February 13, 2012: contributor

Hm. I don't think we should really be showing the balance on blocks that may just get orphaned, should we?

sje397 commented at 2:34 PM on February 13, 2012: contributor

My reasoning is:

as a miner, it's a critical stat I want to see
as a miner, and taking into account the label 'immature', the uncertainty is pretty clearly implied
those numbers are already displayed in the transaction list
now the overview numbers add up to what's in the transaction list
it's not displayed if the immature balance is 0, so won't bother non-miners

gmaxwell commented at 2:36 PM on February 13, 2012: contributor

IIRC unextended blocks are not shown in the transaction list in the gui. They're only show at something like two confirms. If the balance doesn't match the transaction list, it's going to be pretty confusing.

sje397 commented at 2:37 PM on February 13, 2012: contributor

No, they're shown immediately....and by 'now' i meant 'now that I fixed it'. Even if they did take 2 confirms to show, they aren't available for a lot longer than that (120 blocks) so not having them included in either the actual balance or the unconfirmed balance, but still shown in the transaction list, makes it a bit inconsistent.

sje397 commented at 2:52 PM on February 13, 2012: contributor

Of course, you're correct. I've modified (and squashed) the patch to use a consistent method for display - i.e. like the transaction list, calculation of the 'immature' balance now requires transactions to be at least 2 deep in the block chain.

laanwj commented at 5:05 PM on February 13, 2012: member

I agree with this patch (after the fix). Good idea to make it hidden when 0, so only miners will see it. "immature" makes it quite clear that it is still unspendable.

laanwj commented at 5:51 PM on February 13, 2012: member

I think setting an explicit font face for the label is an error (the other labels don't have that either...)

sje397 commented at 9:45 PM on February 13, 2012: contributor

I didn't initially, but then I saw the other labels did have that, so I copied it. I was actually going to make another pull request to fix that - the font and default tooltips can be set in the designer.

sipa commented at 10:16 PM on February 13, 2012: member

What does this change, exactly?

sje397 commented at 10:42 PM on February 13, 2012: contributor

The main effect is the addition of a field labelled 'Immature' in the overview section under the 'unconfirmed' field, which shows mined income that has not yet matured (which is currently not displayed anywhere, even though the transactions exist in the transaction list). To do that I added a 'GetImmatureBalance' method to the wallet, and connected that through to the GUI as per the 'GetBalance' and 'GetUnconfirmedBalance' methods. I did a small 'no-op' change to make the code in adjacent functions a little more readable (imo); it was a change I had made in my repo earlier...but I thought it wouldn't hurt so left it in.

laanwj commented at 8:15 AM on February 14, 2012: member

He replaced the skip - based logic with a more direct boolean condition in CWallet::GetBalance() and CWallet::GetUnconfirmedBalance(), which is a win in clarity IMO.

It does mean it needs some more testing to make sure it's still the same.

sje397 commented at 11:08 AM on February 14, 2012: contributor

Sorry, I found a bug in "WalletModel::update" where I was not checking for a change in the immature balance in the condition for firing the signal to update the GUI. I have fixed it and squashed it back to one commit.

Added 'immature balance' for miners. Only displayed if the balance is greater than zero. 7a3cac0597

sje397 commented at 3:42 PM on February 14, 2012: contributor

Fix for a 'balanceChanged' signal that i'd missed in SendCoinsDialog.

gavinandresen commented at 7:49 PM on February 14, 2012: contributor

Code changes look good, I didn't test though.

sipa commented at 11:47 PM on February 14, 2012: member

Object::connect: No such signal WalletModel::balanceChanged(qint64, qint64, qint64)

sje397 commented at 12:05 AM on February 15, 2012: contributor

sipa, that signal should exist - line 123 of walletmodel.h...maybe you need a rebuild?

sje397 commented at 12:07 AM on February 15, 2012: contributor

Oh, no i put a quint64 there instead of a qint64 - i'll fix it tonight.

OT - that pull request comes from the master branch of my fork - and if I add any commits, they seem to become part of this pull request. Any idea how I can rearrange things so that this pull request comes from another branch, so that I can include all my pull requests in my fork's master? I can live with having an integration branch, and would rather not close this and create another pull request...but if there's an easy way to rearrange things that would be ideal.

sipa commented at 12:31 AM on February 15, 2012: member

I don't think you can edit what branch a pull request refers to, but there is no problem with closing and submitting a new pull request.

sje397 commented at 12:05 PM on February 15, 2012: contributor

Ok I shall close this one and submit a new one including the signal fix.

sje397 closed this on Feb 15, 2012

ptschip referenced this in commit 4052914fcf on Nov 11, 2017

sipa referenced this in commit 77ccaf5cb9 on Apr 2, 2021

sipa referenced this in commit cb8f70f488 on Apr 2, 2021

sipa referenced this in commit a110aca93f on Apr 2, 2021

sipa referenced this in commit bdca9bcb6c on Apr 23, 2021

rebroad referenced this in commit 6e13b4e771 on Jun 23, 2021

DrahtBot locked this on Sep 8, 2021