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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | pablomartin4btc, polespinasa |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
CAmount a class by hodlinator)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
To test, start with -datacarriersize=10, and run
send '{"data": "4368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73"}' null "unset" 1
from the console. "Non-mempool: <change amount>" should appear in the Balances pane on the Overview window with the change from the transaction. Restarting with -datacarriersize=100000 should allow the tx to enter the mempool, and move the amount into the Available balance. (Presumably, do this on -regtest after generating 100 blocks; or perhaps signet after grabbing some funds from a faucet)
Screenshot:
<img width="318" height="218" alt="non-mempool-gui" src="https://github.com/user-attachments/assets/885411f4-d01a-4574-a341-98b962c5c2ec" />
The negative sign for the non-memool stuff isn't really very clear, perhaps.
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? If that is a transaction we created we will not broadcast it. If that is a transaction we are receiving we will not be able to spend the outputs until it is confirmed as the child will be rejected by our mempool.
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.
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...
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.
I see, didn't check the code when asking, had the same concerns as here https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3554858459, but I saw the approach taken there was pretty nice tbh.
Concept ACK
The negative sign for the non-memool stuff isn't really very clear, perhaps.
It is not :)
I don't think we show in the balance overview the "negative" (outgoing) balance, we just reduce the available amount. <img width="1063" height="197" alt="imagen" src="https://github.com/user-attachments/assets/0d62bb02-32f5-4cf0-acb6-92b0799f9d82" />
204 | + </widget> 205 | + </item> 206 | + <item row="3" column="0"> 207 | + <widget class="QLabel" name="labelNonMempoolText"> 208 | + <property name="text"> 209 | + <string>Non-mempool:</string>
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?
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.
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.
The label has a tooltip already, I left a suggestion there.
141 | @@ -142,7 +142,6 @@ QString BitcoinUnits::formatHtmlWithUnit(Unit unit, const CAmount& amount, bool 142 | 143 | QString BitcoinUnits::formatWithPrivacy(Unit unit, const CAmount& amount, SeparatorStyle separators, bool privacy) 144 | { 145 | - assert(amount >= 0);
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.
199 | 200 | ui->labelImmature->setVisible(showImmature); 201 | ui->labelImmatureText->setVisible(showImmature); 202 | + 203 | + // likewise for non-mempool balances 204 | + bool showNonMempool = balances.nonmempool_balance != 0;
maybe < 0 ?
Maybe we can use this PR to update this to also use:
walletModel.wallet().getBalances().balance
Like you did below?
188 | + <widget class="QLabel" name="labelNonMempool"> 189 | + <property name="cursor"> 190 | + <cursorShape>IBeamCursor</cursorShape> 191 | + </property> 192 | + <property name="toolTip"> 193 | + <string>Balance for wallet transactions not in the mempool</string>
nit: I think the tooltip text doesn't tell the user what that means in practice, or why it's negative...
<string>Balance about to be spent by wallet transactions not currently in the mempool. May never confirm.</string>
189 | @@ -190,13 +190,20 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances) 190 | ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); 191 | ui->labelUnconfirmed->setText(BitcoinUnits::formatWithPrivacy(unit, balances.unconfirmed_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); 192 | ui->labelImmature->setText(BitcoinUnits::formatWithPrivacy(unit, balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); 193 | - ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); 194 | + ui->labelNonMempool->setText(BitcoinUnits::formatWithPrivacy(unit, balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); 195 | + ui->labelTotal->setText(BitcoinUnits::formatWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance + balances.nonmempool_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy));
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->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));
@ajtowns, I've added a test that perhaps you may want to include here, it checks the label visibility and its value, please check e746d9ef6225b1c4281ce514fb4e21fc98b4c56a (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.