Adds non-mempool wallet balance to overview - #952
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy 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. |
Use getBalances when testing the current balance in OverviewPage to take into account non_mempool balances in next commits. Co-authored-by: Anthony Towns <aj@erisian.com.au>
The assert removal is necesary for a next commit to introduce nonmempoolbalance as that balance should always be negative thus making this assert no longer true. Co-authored-by: Anthony Towns <aj@erisian.com.au>
fce681b to
db32a1b
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Key differences with #911 are:
|
pablomartin4btc
left a comment
There was a problem hiding this comment.
Concept ACK
I've suggested some release notes in previous attempt #911 and it'd be nice to include them here if you're up to ofc.
Suggested Qt test snippet for wallettests.cpp...
The PR touches wallettests.cpp but only to fix the existing call — worth adding a UI-level test for the new label visibility and formatting. Since there are no actual non-mempool transactions in the test wallet, calling setBalance() directly with crafted data is the right approach here rather than going through pollBalanceChanged().
// Test non-mempool balance display
interfaces::WalletBalances nonmempool_balances;
nonmempool_balances.balance = walletModel.wallet().getBalances().balance;
nonmempool_balances.nonmempool_balance = -50000;
overviewPage.setBalance(nonmempool_balances);
QVERIFY(overviewPage.findChild<QLabel*>("labelNonMempool")->isVisible());
QVERIFY(overviewPage.findChild<QLabel*>("labelNonMempoolText")->isVisible());
CompareBalance(walletModel, -50000, overviewPage.findChild<QLabel*>("labelNonMempool"));
nonmempool_balances.nonmempool_balance = 0;
overviewPage.setBalance(nonmempool_balances);
QVERIFY(!overviewPage.findChild<QLabel*>("labelNonMempool")->isVisible());
QVERIFY(!overviewPage.findChild<QLabel*>("labelNonMempoolText")->isVisible());|
Added the tests with you as a co-author and added some release notes. |
eee7f87 to
1907923
Compare
|
Could you please weigh in on the UI/UX design choices in this PR? |
|
Yes, I'd be happy to. Overall, I think a label like "On hold" or "Needs attention" would be a more clear than "Not in mempool" and go better with "Available" and "Pending". Users have mental models for those. Not sure if this is done already, and maybe not part of this PR, but carrying this state through the rest of the UI is likely also a good idea. So a transaction in the payment history should also indicate this state. And the transaction details view can ideally let the user know why the transaction is "on hold", and provide information or options on how to either cancel it or fix the respective problem. Hope that's helpful. |
1907923 to
4894a79
Compare
pablomartin4btc
left a comment
There was a problem hiding this comment.
Regarding the label choice: I think "On hold" implies something or someone intentionally paused the transaction — but could be that the network doesn't even know it exists. For the policy-rejection cases, it's closer to a draft: created, saved locally, but rejected or never submitted. Nothing is on hold. "Pending" is already taken mentally by "in mempool, waiting for confirmation." And "Unbroadcast" isn't accurate either — the tx may have been broadcast and rejected by policy.
The Core RPC in bitcoin/bitcoin#33671 uses nonmempool — so I think the GUI label should mirror that for consistency, it matches what users would see in getbalances RPC.
The nonmempool state actually covers a wider range of situations than the PR description suggests:
| Case | Duration | Voluntary? | Tx fate |
|---|---|---|---|
| Private broadcast in-flight | ~1 second | Yes | Will enter mempool |
walletbroadcast=0 |
Indefinite | Yes | User controls |
| Low feerate / too many ancestors | Indefinite | No | May enter if conditions change |
| Too large OP_RETURN | Indefinite | No | Will never enter standard mempool |
| Mempool eviction | Indefinite | No | May re-enter if feerate improves |
Perhaps this also means the tooltip needs updating. "Balance for wallet transactions that currently don't fit node's mempool policies" is not completely right, for the first two rows — a private broadcast tx fits mempool policies fine, it's just being relayed privately via Tor/I2P. Notably, the nonmempool balance was needed as a next step once private broadcast was implemented — without it, I think funds would silently disappear during the ~1 second broadcast window.
The tooltip is also misleading on the accounting side: per Core's #33671 description, inputs consumed by non-mempool txs are added back into trusted (Available) as if the transaction doesn't exist, with nonmempool as the offsetting debit — keeping the total balance unchanged. It's not a separate pool of frozen funds.
A more accurate tooltip I think, could be something like: "Wallet transactions not currently in the mempool — due to policy rejection, pending private broadcast, or deliberate delay."
4894a79 to
0351124
Compare
Although I like the mirroring for consistency, I don't think the RPC is though for users, the GUI is. Imho mirroring should be the first option as long as the naming is user friendly, and here I agree with Christoph that
Force pushed your tool-tip suggestion. |
pablomartin4btc
left a comment
There was a problem hiding this comment.
ACK 0351124.
Fair points from @GBKS, I agree.
My earlier thinking actually started from a similar place: should this fold under Pending? After thinking through it I leaned toward accuracy, but I think the right frame is honest-but-readable rather than technically precise. One reason I moved away from it is that Pending is positive (incoming, unconfirmed receipts), while this amount is always negative — funds tied up on the outgoing side. Mixing them arithmetically would be confusing without a more elaborate breakdown like the Arké design.
I ended up thinking through a few alternatives along honest-but-readable lines:
- Stalled — movement stopped, no guarantee it restarts; handles the policy-rejection cases well but sounds harsh;
- Delayed — friendlier, but leans toward "will happen eventually," which isn't always true.
None of these are clearly better. "On hold" is fine — my only mild concern was that it implies something intentionally paused that will eventually release, while some cases just sit until the user acts. But that's a subtle edge case and not worth over-engineering the label for.
Thanks @GBKS for your help here.
The assert in formatWithPrivacy is removed because m_mine_nonmempool is always negative, making this assert no longer true. Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Pablo Martin <pablomartin4btc@gmail.com>
0351124 to
4d5c099
Compare




Just grabbing #911
Copying the motivation from AJ's description:
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.
The changes look like:
How to test
To test, start with
-datacarriersize=10, and runfrom the console. "Non-mempool: " should appear in the Balances pane on the Overview window with the change from the transaction. Restarting with
-datacarriersize=100000should 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)