Activity list and payment request correctness fixes - #875
Open
epicleafies wants to merge 16 commits into
Open
Conversation
The activity model sorted by confirmation depth on refresh but pushed live transactions and pending receive requests to the front of the list, so the order a user saw while the app was running differed from the order after a restart, and rows receiving the same block time had no stable relative order at all (bitcoin-core#848). Share one comparator between the refresh sort and the live paths: newest first, with pending requests, txid, output index, and request id as tie-breakers. New rows are inserted at their sorted position, and a pending request row is repositioned once a real transaction fulfills it, since fulfillment changes its sort key.
ActivityListModel::data() refreshed each row's status from inside the const accessor, one wallet tryGetTxStatus call per role read, and fetched address labels the same way. Views call data() constantly (delegate creation, repaints, proxy filtering), so wallet work ran on the render path and rows mutated without dataChanged semantics. Keep data() a pure read. Rows get their status and label when they are created, statuses refresh once per new block through a blockTipHeight connection in bitcoin.cpp (the QML counterpart of the Widgets numBlocksChanged refresh), and labels refresh when the address book changes. Pending request rows keep their request label, which updateReceiveRequest already maintains. A refresh keeps a row's cached status when tryGetTxStatus fails: the interface try-locks cs_wallet, so a failed read means lock contention as often as a missing transaction, and without the lazy re-read in data() a downgraded status would stick until the next block. The Widgets table keeps its cached status on a failed read the same way. The per-tip refresh is skipped while blocks are still syncing (one refresh when the sync completes catches the list up, mirroring the Widgets throttle during IBD), and a newly selected wallet is refreshed on selection, since only the selected wallet follows the per-block refresh. Also assign Transaction::countsForBalance in updateStatus, mirroring the Widgets TransactionRecord logic. It was never written before, so fulfillPendingRequest copied an uninitialized bool.
Zero-confirmation transactions rendered like confirmed ones apart from an icon color change, so nothing told a user that received funds were not final yet, and color alone does not survive every theme or reader. Give unconfirmed rows the dashed pending icon (already shipped in the icon set but unused) and replace the date column with a Pending label until the first confirmation, matching the Pending receive treatment request rows already get. The transaction detail page says Pending confirmation instead of 0 confirmations. Confirmed and confirming rows are unchanged, and the icon recolors through Theme tokens so both themes keep contrast.
Editing a saved payment request allowed changing its expected amount, so a slip while relabeling could silently rewrite what a request is waiting for and corrupt the record of what was asked of the payer (bitcoin-core#847). Disable the amount field in the update state (name and note stay editable) and, since a disabled field is not a guarantee, enforce it in the model: saveCurrentPaymentRequest keeps the stored amount on updates regardless of what the in-memory request holds, and re-syncs the editor object so the UI shows the enforced value. New requests are unaffected.
The address details action always started a fresh request draft seeded from the address book label, so an address that already had a saved request silently got a second one on commit. Duplicate requests per address break the one-request-per-address assumption the label sync and pending-row fulfillment rely on. setCurrentPaymentRequestAddress now loads the most recent saved request for the address into the editor when one exists, so committing updates in place, and the details button reads Edit payment request in that case. A used address keeps the action as long as it still has a saved request.
Core stores any number of receive requests per address, but the activity model gated fulfillment on a per-address set and dropped the address after fulfilling one row. With two requests on one address the second payment could no longer match, so its pending row stayed stuck forever, and on a reload any transaction on the address hid every request row for it (the address-reuse gap raised on PR bitcoin-core#708). Drop the address set. A payment now fulfills the oldest still-pending row for its address, older requests are consumed in the order they were made, and later payments keep fulfilling the remaining rows one at a time. The reload path applies the same rule: payments the address already received consume its oldest requests first, so a restart shows the same still-pending rows as the live session. Only incoming parts may fulfill a request, the same types the reload path counts, so the debit part of a payment to one of this wallet's own requested addresses cannot consume the row. Request ids compare numerically (and newest first) in the row order, and equal-date request history entries tie-break on the id, so same-second requests keep a deterministic order and oldest-first fulfillment agrees between a live session and a reload.
WriteCsvValue quote-escapes correctly but leaves formula interpretation to the spreadsheet: a received payment whose label (or address-book entry) begins with =, +, -, or @ runs as a formula when the user opens their own export in Excel or LibreOffice, enough for HYPERLINK or WEBSERVICE exfiltration and DDE command execution. The label is sender-influenced, so this is remote data in a local file. Prefix such cells with a single quote before quoting. The guard is deliberately scoped to the text columns (label and address): the amount column is wallet-generated and must keep its leading minus sign, which the test pins down.
Two small correctness riders on the activity model. Transaction::dateTimeString formatted month names with QDateTime's format strings, which in Qt 6 always render English regardless of the user's locale, so every transaction date in the app was English-only. Format through QLocale like the ban list dates already do. The Status and Type roles returned raw C++ enum values, so QML delegate comparisons against Transaction enum values worked only through QVariant's enum coercion. Cast both to int at the model boundary (data() and transactionDetails) to make the contract explicit.
Extend the receive flow test past request creation: an unpaid request shows a pending activity row, paying it from a funded miner wallet on the same node fulfills that row in place (no extra row) with the zero-conf Pending treatment, and mining a block clears the Pending state without leaving the page. Extend the addresses flow test past the create step: once a request is committed for an address, the address details action reads Edit payment request, opens the saved request in its update state with the amount locked, and re-committing updates in place instead of duplicating the request.
The wallet fires handleTransactionChanged callbacks on the node's notification thread, and ActivityListModel's subscriber runs row inserts and moves directly in that callback. Qt models must only be mutated on the thread that owns them, and the unmarshalled delivery showed up in manual testing as live activity rows landing at the wrong position (a pending transaction below an older confirmed one) until the next resort or reload. Queue the delivery onto the model's thread, the same marshalling the Widgets transaction table does in NotifyTransactionChanged. Direct delivery is kept when the callback already runs on the model's thread, so synchronous test notifications stay synchronous.
ActivityFilterProxyModel::lessThan compared only the timestamp role, so transactions sharing a timestamp displayed in arrival order while a reload displays them in the source model's fully tie-broken order. Same-second transactions are common (a burst of incoming payments), so the displayed order could change across a restart, which is the nondeterminism the source ordering exists to prevent. Delegate the proxy comparison to the source model's comparator so the view shows one deterministic order both live and after a reload. The timestamp-only fallback stays for source models that are not an ActivityListModel (the proxy unit tests use a stub source).
The wallet often fires transaction change notifications while it still holds cs_wallet, so tryGetTxStatus can fail transiently both in the notification handler and in the per-block refreshStatuses sweep, and both paths dropped the update. The Widgets table heals such drops lazily because its data() re-reads the wallet on every paint, but the QML model's data() is a pure read, so a dropped update left a just confirmed row showing Pending until the next block (observed in manual testing), and a notification for a new transaction could be lost entirely. Retry the hash-specific update and the refresh sweep on a short timer until a pass reads the wallet cleanly. Notifications only carry wallet transactions and rows only come from the wallet, so the retries terminate once the lock frees up.
Transaction::dateTimeString renders a row's date as "N minutes ago" relative to the moment the row is read, and nothing re-reads a row once the view has it. A row therefore keeps the age it was first drawn with: it shows "0 minutes ago" until an unrelated event happens to refresh the list, and then jumps several minutes at once. Confirmed rows sat wrong for as long as the user left the wallet open. Re-emit the date role on a timer so a row ages on its own. The sweep runs several times a minute rather than once: it has its own phase, unrelated to when any row was created, so refreshing at the resolution of the string itself would still leave a row reading "0 minutes ago" for nearly two minutes. Only the rendered string changes, the timestamps behind it are fixed, so the sweep is a signal with no wallet work behind it and the view only re-reads the delegates it has realized. Also note next to the date formatting that QLocale() is the system locale, which the in-app language setting does not change, matching how the Widgets GUI formats its dates.
A pending payment request rendered its requested amount in the same green a confirmed receive gets, while an unconfirmed receive, where the money has actually been seen on the network, was greyed. That put the two in the wrong order: the row where nothing has arrived looked more final than the row where something did. Drop the payment-request exemption from the amount color so a request greys for the reason every other row greys, that the amount does not count toward the balance. The purple icon and the "Pending receive" date keep the row identifiable as a request.
The per-block refresh runs off the node's tip, but the wallet processes that block on its own schedule, and tryGetTxStatus answers with the wallet's height rather than the node's. A refresh that lands in between reads a depth one short, and since the read succeeded the existing retry never fired, so rows kept a confirmation count one behind until the next block arrived. Only the transaction that just confirmed recovered, from its own wallet notification; every other row stayed wrong. Report the wallet height out of the status read and pass the height being refreshed to in from the block hook, so a refresh can tell a read taken before the wallet caught up from one that is current, and retry the same way a read lost to lock contention already does. The Widgets table gets this for free by re-reading each row against the wallet's last processed block on every paint, which a pure data() cannot do. Back the retries off and cap them. The wallet is normally a moment behind, so the first retry comes quickly, and a wallet still behind after the budget is rescanning rather than mid-block, where the next tip refreshes it anyway. Each retry re-reads every row, so the sweep is kept off the critical path during a burst of blocks.
The address details button now reads "Edit payment request" when the address already has one, but the same action in the row's menu still read "Create payment request", so the two entry points into the same editor described it differently. The menu item was also hidden as soon as the address was used, which left a used address with a saved request offering the action in the details popup and not in the row. Give the row the saved-request fact when its menu opens, resolved there rather than bound per row so no lookup runs while the list scrolls, and key the wording and availability off it the way the details button does.
johnny9
reviewed
Aug 28, 2026
| const int delay{std::min(STATUS_RETRY_INTERVAL_MS << m_status_retry_attempts, | ||
| MAX_STATUS_RETRY_INTERVAL_MS)}; | ||
| ++m_status_retry_attempts; | ||
| QTimer::singleShot(delay, this, [this, chain_height] { |
Collaborator
There was a problem hiding this comment.
chain_height is captured here. If tip H+1 arrives while the retry for H is already scheduled, H+1 cannot schedule another retry. The old callback can finish at H and leave every row one confirmation behind until another block arrives. Please store the latest target height in a member and make the pending timer read that member instead of capturing an old height. A test should cover H+1 arriving while the H retry is pending.
| int num_blocks; | ||
| int64_t block_time; | ||
| if (m_wallet_model->tryGetTxStatus(hash, wtx, num_blocks, block_time)) { | ||
| updateTransaction(hash, wtx, num_blocks, block_time); |
Collaborator
There was a problem hiding this comment.
updateTransaction() updates only the first row returned by findTransactionIndex(). One wallet transaction can produce several activity rows, such as a multi-recipient or self-payment transaction. Now that data() is a pure read, the other rows no longer refresh themselves after a non-block CT_UPDATED, such as abandoning a transaction. Please find all rows with this hash and update each one. A single O(n) scan is fine here. A regression test should verify that both rows of one transaction become Abandoned.
| // the event-driven counterpart of the Widgets lazy status re-read on | ||
| // paint. Notifications only carry wallet transactions, so the read | ||
| // eventually succeeds. | ||
| QTimer::singleShot(250, this, [this, hash] { applyTransactionChanged(hash); }); |
Collaborator
There was a problem hiding this comment.
ChangeType is discarded before this retry path. For CT_DELETED, tryGetTxStatus() can never succeed because the wallet record is gone, so this schedules another read every 250 ms for the lifetime of the model. Please pass ChangeType into this function, remove all matching rows from highest to lowest for CT_DELETED without reading status, and keep only bounded or deduplicated retries for transient CT_NEW and CT_UPDATED reads. The stale deleted row was pre-existing; the endless polling is introduced here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a set of correctness problems in the Activity list and payment requests.
Fixes #848. Fixes #849. Fixes #847.
Added unit, QML and functional tests.
Verified headed on Wayland including first-run onboarding.