Added 'immature balance' for miners. Only displayed if the balance is

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

This adds 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. Immature balance comes from mined income that is at least two blocks deep in the chain (same logic as displayed transactions).

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
this makes 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

sipa commented at 3:59 PM on February 15, 2012: member

Doesn't seem to work for me... (the immature transactions are simply not shown)

sje397 commented at 1:38 AM on February 16, 2012: contributor

Hm. I'll try a clean build from that branch tonight, but I don't see why it wouldn't work. The important condition is:

if (pcoin.IsCoinBase() && pcoin.GetBlocksToMaturity() > 0 && pcoin.GetDepthInMainChain() >= 2)

If a transaction in the wallet meets that condition then it will be added to the 'immature balance' shown in the overview.

sipa commented at 12:41 PM on February 16, 2012: member

I was only looking at immature transactions before, not the balance; i must have misread.

The immature balance seems to work fine with this patch; however, newly generated mining transactions are not shown, while the transaction counter is updated.

sje397 commented at 4:39 PM on February 16, 2012: contributor

I think WalletModel::getNumTransactions() is a bit too simple...but, I didn't touch that.

in src/wallet.cpp:None in 8697a15314 outdated

 791 | @@ -793,9 +792,23 @@ int64 CWallet::GetUnconfirmedBalance() const
 792 |          for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
 793 |          {
 794 |              const CWalletTx* pcoin = &(*it).second;
 795 | -            if (pcoin->IsFinal() && pcoin->IsConfirmed())
 796 | -                continue;
 797 | -            nTotal += pcoin->GetAvailableCredit();
 798 | +            if (!pcoin->IsFinal() || !pcoin->IsConfirmed())
 799 | +                nTotal += pcoin->GetAvailableCredit();

luke-jr commented at 2:56 PM on February 17, 2012:

What is the intention of the change here? It looks like it does the same thing...

sje397 commented at 2:58 PM on February 17, 2012:

Yes, I mentioned that in the pull req. It's just easier to read imo (laanwj agreed).

Diapolo commented at 9:17 PM on February 29, 2012:

+1 for easier to read.

sipa commented at 12:50 AM on February 27, 2012: member

ACK

in src/qt/forms/overviewpage.ui:None in 8697a15314 outdated

 106 | +         <widget class="QLabel" name="labelNumTransactions">
 107 |            <property name="text">
 108 | -           <string>&lt;!DOCTYPE HTML PUBLIC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org/TR/REC-html40/strict.dtd&quot;&gt;
 109 | -&lt;html&gt;&lt;head&gt;&lt;meta name=&quot;qrichtext&quot; content=&quot;1&quot; /&gt;&lt;style type=&quot;text/css&quot;&gt;
 110 | -p, li { white-space: pre-wrap; }
 111 | -&lt;/style&gt;&lt;/head&gt;&lt;body style=&quot; font-family:'Ubuntu'; font-size:11pt; font-weight:400; font-style:normal;&quot;&gt;

rebroad commented at 4:55 PM on March 20, 2012:

I wonder how other OSs interpret the Ubuntu font family.

in src/qt/forms/overviewpage.ui:None in 8697a15314 outdated

  33 | @@ -34,56 +34,70 @@
  34 |          <property name="verticalSpacing">
  35 |           <number>12</number>
  36 |          </property>
  37 | -        <item row="2" column="0">
  38 | +        <item row="0" column="0">
  39 | +         <widget class="QLabel" name="label_5">
  40 | +          <property name="text">
  41 | +           <string>&lt;!DOCTYPE HTML PUBLIC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org/TR/REC-html40/strict.dtd&quot;&gt;

Diapolo commented at 5:03 PM on March 20, 2012:

It's a bad idea to include another HTML-string which contains invalid HTML and a font only available on Ubuntu. It's an issue already, see: #945.

sje397 commented at 1:55 AM on March 21, 2012:

Agreed. This was there already, but got moved with my window rearrangement (see 'old' line 82 below).

luke-jr commented at 5:12 PM on March 20, 2012: member

There's really no reason Bitcoin-Qt should be setting fonts/colours in most cases. Let the user configure their OS's font selection...

luke-jr commented at 1:42 PM on April 10, 2012: member

Needs rebasing. Sipa broke everything using locks. >_<

sje397 commented at 2:26 PM on April 10, 2012: contributor

Rebased.

luke-jr commented at 11:01 PM on May 8, 2012: member

ACK

Diapolo commented at 6:35 AM on May 9, 2012: none

It would be nice to move as many settings as possible into the ui file without breaking support for older Qt versions, see: #1212

I also have been told, that "Monospace" is only for addresses and not for BTC amount labels.

in src/qt/overviewpage.cpp:None in a3b8de3595 outdated

 108 | @@ -108,6 +109,11 @@ class TxViewDelegate : public QAbstractItemDelegate
 109 |      ui->labelUnconfirmed->setToolTip(tr("Total of transactions that have yet to be confirmed, and do not yet count toward the current balance"));
 110 |      ui->labelUnconfirmed->setTextInteractionFlags(Qt::TextSelectableByMouse|Qt::TextSelectableByKeyboard);
 111 |  
 112 | +    // Immature balance: <balance>

Diapolo commented at 6:37 AM on May 9, 2012:

setToolTip and setTextInteractionFlags can move to the ui-file and here is that Monospace thing I talked about (you can set bold in the Qt Designer though).

laanwj commented at 9:50 AM on May 11, 2012: member

I'd like to merge this. Can you rebase one more time?

Diapolo commented at 10:02 AM on May 11, 2012: none

General ACK, but it would be nice to update and reflect my suggestions, too keep things easier in the source and use the XML files as much as possible.

sje397 commented at 11:01 AM on May 11, 2012: contributor

No worries :) I'll have a shot at it now. Just have to wrangle this new mac into submission.

sje397 commented at 11:45 AM on May 11, 2012: contributor

Ok rebased and cleaned up UI stuff. Please check the layout still looks and acts as you'd like.

Diapolo commented at 11:48 AM on May 11, 2012: none

Is this rebased agains the current master? Sorry to ask, but the changes to overviewpage.cpp / overviewpage.ui look somehow weird in the diff. It seems you change QFormLayout to QGridLayout, but to only add 2 labels...?

Edit: It should only add labelImmatureText and labelImmature to the XML, no?

sje397 commented at 11:56 AM on May 11, 2012: contributor

I do have my disagreements with git, but I'm pretty sure it's against the current master. I changed it to a grid layout because when I removed the HTML from the 'wallet' label at the top of that section, it was no longer centered. The grid layout looked like it worked for me...but I've only tried it on a mac.

Diapolo commented at 12:01 PM on May 11, 2012: none

I would feel much better to only add the labels and I saw at least 2 parts, that are not in the current master, but show up in your diff ... strange.

in src/qt/forms/overviewpage.ui:None in b6d9f6e94e outdated

 167 | +         <widget class="QLabel" name="labelNumTransactions">
 168 | +          <property name="statusTip">
 169 | +           <string>Total number of transactions in wallet</string>
 170 | +          </property>
 171 |            <property name="text">
 172 | -           <string>&lt;!DOCTYPE HTML PUBLIC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org/TR/REC-html40/strict.dtd&quot;&gt;

Diapolo commented at 12:03 PM on May 11, 2012:

This part is not in the master anymore: https://github.com/bitcoin/bitcoin/blob/master/src/qt/forms/overviewpage.ui

in src/qt/overviewpage.cpp:None in b6d9f6e94e outdated

  93 | @@ -94,22 +94,11 @@ class TxViewDelegate : public QAbstractItemDelegate
  94 |      ui(new Ui::OverviewPage),
  95 |      currentBalance(-1),
  96 |      currentUnconfirmedBalance(-1),
  97 | +    currentImmatureBalance(-1),
  98 |      txdelegate(new TxViewDelegate())
  99 |  {
 100 |      ui->setupUi(this);
 101 |  
 102 | -    // Balance: <balance>

Diapolo commented at 12:03 PM on May 11, 2012:

And this one was also removed by a commit, so why should it show up here?

sje397 commented at 12:13 PM on May 11, 2012: contributor

No you're right - missed a fetch. I'll redo it. Sorry.

in src/qt/forms/overviewpage.ui:None in 066d070f19 outdated

  17 | @@ -18,30 +18,41 @@
  18 |      <layout class="QVBoxLayout" name="verticalLayout_2">
  19 |       <item>
  20 |        <widget class="QFrame" name="frame">
  21 | +       <property name="sizePolicy">

Diapolo commented at 12:44 PM on May 11, 2012:

Looking much better, but what does this do / change?

Diapolo commented at 12:44 PM on May 11, 2012: none

The overviewpage.cpp is fine now.

sje397 commented at 12:53 PM on May 11, 2012: contributor

I started with the fresh master copy of the xml and just added those two fields. The rest of the changes were done automatically by the editor.

Diapolo commented at 12:58 PM on May 11, 2012: none

As @laanwj is the one, who is able to merge this he should give the final ACK and comment :). Thanks for your work!

sje397 commented at 1:03 PM on May 11, 2012: contributor

My pleasure. That should have been much easier.

luke-jr commented at 8:01 PM on May 18, 2012: member

Blerg, needs rebasing again :(

laanwj commented at 8:31 PM on May 18, 2012: member

Oops we should have merged this first before the (out of sync) stuff...

Diapolo commented at 9:25 PM on May 18, 2012: none

But at least we now have a clean ui-file, should be pretty easy to just add the 2 labels in.

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

This adds 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. Immature balance comes from mined income that is
at least two blocks deep in the chain (same logic as displayed transactions).

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
- this makes 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

I also 'cleaned' the overview UI a little, moving code to the XML and removing HTML.

8fdb7e108f

sje397 commented at 1:18 PM on May 25, 2012: contributor

Rebased again.

laanwj referenced this in commit 882ba0e752 on Jun 2, 2012

laanwj merged this on Jun 2, 2012

laanwj closed this on Jun 2, 2012

laanwj commented at 9:39 AM on June 2, 2012: member

Finally merged...

Agreed with @sje397, the ui file output is just a data file, and should be opaque. No need for manually shuffling around xml unless something is broken in the designer and it produces nonfunctional code as a result.

Diapolo commented at 5:43 AM on June 6, 2012: none

@sje397 Scott, today I observed sth. strange, the displayed Immature balance was displaying a higher value, than it should. In the transaction list the mined ammounts were already displayed as mature, but did still count towards immature. A client restart fixed this, any idea for that?

sje397 commented at 6:16 AM on June 6, 2012: contributor

@Diapolo I'll look into it. All I can think of is that perhaps the signal to update those values isn't working correctly. It should update along with the balance above, which should also change when amounts mature.

sje397 commented at 9:05 AM on June 20, 2012: contributor

I can't see why that would happen. The signal ('balancedChanged') is called from the wallet model whenever 'updateTransaction' is called...which AFAIK should be called when mined amounts mature. I'll keep looking.

sje397 commented at 11:05 AM on June 20, 2012: contributor

This seems to be a more general issue, and applies to the 'unconfirmed' balance as well and not just the 'immature' balance that I added. I just started my client after receiving a 'payout confirmation' email from my pool, and after sync it showed the new transaction in the transaction list as confirmed (8 confirmations) but still showed that amount 'unconfirmed' in the overview. As with your case, a restart fixed it.

Diapolo commented at 1:21 PM on June 20, 2012: none

Have a look at the discussion in #1475. AFAIK @laanwj is currently working on that general fix.

sje397 commented at 1:28 PM on June 20, 2012: contributor

Thanks Philip. Sounds like progress has been made. :)

coblee referenced this in commit be39fc02a5 on Jul 17, 2012

destenson referenced this in commit fb1ffded70 on Jun 26, 2016

ptschip referenced this in commit 234cbe1f10 on Dec 5, 2017

lateminer referenced this in commit d241b5ed77 on Apr 6, 2019

DrahtBot locked this on Sep 8, 2021

Added 'immature balance' for miners. Only displayed if the balance is > 0 #837