Adds non-mempool wallet balance to overview#911
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
To test, start with from the console. "Non-mempool: " should appear in the Balances pane on the Overview window with the change from the transaction. Restarting with |
029dc0d to
4be5d3d
Compare
4be5d3d to
bc85b4a
Compare
bc85b4a to
026088b
Compare
|
Rebased after merge of parent PR, should be ready to review. Happy to close if some someone wants to take over. |
|
Maybe I'm not understanding the issue, but does this matter? So I am not really getting the issue, we just need to wait for it to be confirmed. IIUC this only makes sense for private-broadcast. |
|
Worst case, I guess: if you make a transaction that doesn't hit the mempool, it won't be visible in your balance, which may lead you to try to make the same transaction again. That might happen if the first transaction was spending some unconfirmed outputs from large transactions, so your new transaction would have exceeded the cluster size limits. Once those large transactions confirm, your nonmempool transaction will be broadcast and able to be confirmed. |
pablomartin4btc
left a comment
There was a problem hiding this comment.
Concept ACK
The negative sign for the non-mempool stuff isn't really very clear, perhaps.
I think for negative amounts we use COLOR_NEGATIVE - defined in guiconstants.h (at least for transactions).
Thinking out loud... shouldn't be the non-mempool balance shown under the pending one like a subitem...
I see, didn't check the code when asking, had the same concerns as here bitcoin/bitcoin#33671 (comment), but I saw the approach taken there was pretty nice tbh. Concept ACK |
| <item row="3" column="0"> | ||
| <widget class="QLabel" name="labelNonMempoolText"> | ||
| <property name="text"> | ||
| <string>Non-mempool:</string> |
There was a problem hiding this comment.
just a silly feel-free-to-ignore nit: in the rpc we use nonmempool no non_mempool. Maybe we don't want to use the - for consistency?
Also maybe mempool can be tooo techincal for some users? What about Pending (unbroadcast) or something similar?
There was a problem hiding this comment.
I was thinking the same and have expressed it in my previous comment but I've checked in the code and it seems that unbroadcast has a different meaning I think (transactions that ARE in the mempool but haven't been relayed to peers yet - m_unbroadcast_txids in txmempool.h), and for the pending section, there's a key difference: Pending = mempool → almost certain to confirm soon; Non-mempool = might never confirm, so it would be also misleading.
There was a problem hiding this comment.
Maybe non-standard and then add a ? icon next to it, if putting the mouse on top it shows a small explanation? IIRC we used that ? in the past for some rbf comments.
There was a problem hiding this comment.
The label has a tooltip already, I left a suggestion there.
|
|
||
| QString BitcoinUnits::formatWithPrivacy(Unit unit, const CAmount& amount, SeparatorStyle separators, bool privacy) | ||
| { | ||
| assert(amount >= 0); |
There was a problem hiding this comment.
nit: Maybe this change motivation should be documented in the commit message. It is not clear why is needed.
Just a simple line saying that m_mine_nonmempool is always negative so this assert is no longer true should be enough.
| ui->labelImmatureText->setVisible(showImmature); | ||
|
|
||
| // likewise for non-mempool balances | ||
| bool showNonMempool = balances.nonmempool_balance != 0; |
| // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel. | ||
| walletModel.pollBalanceChanged(); | ||
| // Check balance in send dialog | ||
| CompareBalance(walletModel, walletModel.wallet().getBalance(), sendCoinsDialog.findChild<QLabel*>("labelBalance")); |
There was a problem hiding this comment.
Maybe we can use this PR to update this to also use:
walletModel.wallet().getBalances().balance
Like you did below?
pablomartin4btc
left a comment
There was a problem hiding this comment.
@ajtowns, I've added a test that perhaps you may want to include here, it checks the label visibility and its value, please check e746d9e (based on top of your changes).
I think this PR should include a doc/release-notes-911.md file. Suggested text:
GUI Changes
-----------
- The Overview page now displays a "Non-mempool:" balance row showing funds that would be spent if wallet transactions currently outside the mempool were to confirm. This row is only shown when non-mempool wallet transactions are present.
| <cursorShape>IBeamCursor</cursorShape> | ||
| </property> | ||
| <property name="toolTip"> | ||
| <string>Balance for wallet transactions not in the mempool</string> |
There was a problem hiding this comment.
nit: I think the tooltip text doesn't tell the user what that means in practice, or why it's negative...
| <string>Balance for wallet transactions not in the mempool</string> | |
| <string>Balance about to be spent by wallet transactions not currently in the mempool. May never confirm.</string> |
| <item row="3" column="0"> | ||
| <widget class="QLabel" name="labelNonMempoolText"> | ||
| <property name="text"> | ||
| <string>Non-mempool:</string> |
There was a problem hiding this comment.
I was thinking the same and have expressed it in my previous comment but I've checked in the code and it seems that unbroadcast has a different meaning I think (transactions that ARE in the mempool but haven't been relayed to peers yet - m_unbroadcast_txids in txmempool.h), and for the pending section, there's a key difference: Pending = mempool → almost certain to confirm soon; Non-mempool = might never confirm, so it would be also misleading.
| ui->labelImmature->setText(BitcoinUnits::formatWithPrivacy(unit, balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); | ||
| ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); | ||
| ui->labelNonMempool->setText(BitcoinUnits::formatWithPrivacy(unit, balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); | ||
| ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance + balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); |
There was a problem hiding this comment.
nit: the non-mempool balance is currently a plain text with no visual emphasis, perhaps since it's always negative, it should be red (like negative amounts in the transaction list)... (haven't tested it)
| ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance + balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); | |
| ui->labelNonMempool->setStyleSheet("QLabel { color: " + COLOR_NEGATIVE.name() + "; }"); | |
| ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance + balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); |


The wallet can contain transactions that are not accepted into the node's mempool (eg due to containing a too large OP_RETURN output, due to too low a feerate, or due to too many unconfirmed ancestors). In the event you end up in this situation, it can appear as if funds have gone missing from your wallet due to the non-mempool balance not being reported. Correct this by reporting the non-mempool balance.
See bitcoin/bitcoin#33671 for further context.