Activity list and payment request correctness fixes - #875
Open
epicleafies wants to merge 18 commits into
Open
Conversation
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.
johnny9
reviewed
Aug 30, 2026
johnny9
left a comment
johnny9
left a comment
Collaborator
There was a problem hiding this comment.
Four additional findings based on the Qt Widgets transaction model and view behavior.
| // columns go through this, so numeric cells keep their minus sign. | ||
| QString GuardedCsvText(QString value) | ||
| { | ||
| static const QString dangerous_leads{QStringLiteral("=+-@\t\r")}; |
Collaborator
There was a problem hiding this comment.
Line feed (\n) is also a dangerous CSV formula prefix. A label starting with it is not neutralized. Please add \n to dangerous_leads and cover it with a test.
| // since data() is a pure read, so a row would keep the wrong confirmation | ||
| // count until the next block. The Widgets table avoids both by comparing | ||
| // each row against the wallet's last processed block on every paint. | ||
| const bool wallet_behind{chain_height >= 0 && wallet_height >= 0 && wallet_height < chain_height}; |
Collaborator
There was a problem hiding this comment.
This only detects a wallet below the node tip. After a tip disconnect, the wallet can be above the new tip, so stale confirmations are accepted. Please treat any known height mismatch as out of sync and retry.
| // Payments the address already received consume its oldest requests | ||
| // first, the order fulfillPendingRequest uses live, so a reload | ||
| // reproduces the same set of still-pending rows. | ||
| QHash<QString, int> received; |
Collaborator
There was a problem hiding this comment.
This matches payments and requests only by count. After restart, an unused old payment can consume a request created later. Please replay payments and requests by timestamp and only match requests that existed when the payment arrived.
| return static_cast<int>(tx->type); | ||
| case TxidRole: | ||
| return tx->isPendingRequest ? QString{} : tx->txid; | ||
| case CanBumpRole: |
Collaborator
There was a problem hiding this comment.
data() runs while QML creates and repaints delegates. canBumpTransaction() can wait for the wallet lock and block the UI. Qt Widgets checks this only when the menu opens. Please query it when transaction details open instead of exposing it as a row role.
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.
The transaction change callback discarded the ChangeType, so a CT_DELETED notification took the same path as any other change: a status read, which can never succeed for a transaction the wallet no longer has, followed by the contention retry. The rows of a deleted transaction therefore lingered with their last cached status (a stale row that predates this branch), and each deletion left a retry chasing a record that will not come back. Pass the ChangeType through and remove every row of a deleted transaction, highest index to lowest so removals do not shift the rows still to be checked. The bounded retry stays for the transient CT_NEW and CT_UPDATED read failures.
The canBump role called WalletQmlModel::canBumpTransaction from data(), which runs while QML creates and repaints delegates. transactionCanBeBumped waits on cs_wallet, so scrolling the activity list could block the GUI thread behind wallet work, once per realized row. The Qt Widgets GUI only checks bump eligibility when the context menu opens, and this model's transactionDetails() invokable already reports canBump when a row's details are opened. Drop the role and route the row click through the same navigateToTransaction path the details page already uses for its transaction links, so bump eligibility is read once per details open instead of on every delegate paint.
epicleafies
force-pushed
the
activity-correctness
branch
from
September 10, 2026 17:26
011e25b to
cdc062b
Compare
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.