wallet: cache paid-invoice keys to fix GUI freeze after broadcast#10658
Open
sashazykov wants to merge 2 commits into
Open
wallet: cache paid-invoice keys to fix GUI freeze after broadcast#10658sashazykov wants to merge 2 commits into
sashazykov wants to merge 2 commits into
Conversation
On wallets with many outgoing invoices that share output scriptpubkeys,
broadcasting a new transaction froze the GUI for several seconds — up
to a minute on bigger wallets. The hot loops in set_broadcasting() and
_update_onchain_invoice_paid_detection() iterate every invoice touched
by the new tx's outputs, which is large when scriptpubkeys are shared
via _invoices_from_scriptpubkey_map. For each touched invoice they call
_is_onchain_invoice_paid(), which scans all of the invoice's output
scripthashes against _prevouts_by_scripthash and does a get_tx_height
per prevout. Both inner dimensions blow up together when invoices share
inputs/outputs across many past payments.
Add _paid_invoice_keys: Set[str], an in-memory cache of outgoing
invoice ids known to be PR_PAID, maintained incrementally:
- populated at wallet load by _prepare_onchain_invoice_paid_detection
- updated in _update_onchain_invoice_paid_detection (discard then
recompute, so reorgs still demote PAID->UNPAID)
- updated in save_invoice, delete_invoice, clear_invoices
- new on_event_invoice_status listener keeps it in sync with
LN-driven transitions from LNWallet.set_invoice_status
get_invoice_status() short-circuits to PR_PAID on cache hit, skipping
the prevout scan. set_broadcasting() skips already-paid invoices
entirely — broadcasting_status has no effect on a paid invoice and the
callback churn is what made the GUI freeze.
The full invoice-list rebuild after a successful broadcast is unnecessary: the set_broadcasting(PR_BROADCAST) call immediately after fires the invoice_status callback for any touched invoices, and main_window.on_event_invoice_status updates the affected rows incrementally via refresh_item / delete_item. Tx ingestion also sets need_update, which causes update_tabs() to refresh. On wallets with many invoices this rebuild iterated all unpaid invoices and called get_invoice_status() per entry on the GUI thread, contributing to the post-broadcast freeze fixed in the previous commit.
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
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.
Fix for a long-standing GUI freeze when broadcasting a transaction from a wallet with many already-paid outgoing invoices that share output scriptpubkeys.
This issue has bothered me personally for nearly ten years — I originally reported it back in 2018 in #4183. The wallet I use most has accumulated 500+ invoices over the years (typical for repeat payments to the same set of addresses), and every time I broadcast a new transaction the GUI would lock up for several seconds — up to a minute on bigger wallets. The workaround I'd been using is
wallet.clear_invoices(); wallet.save_db()in the console, but that throws away history I'd rather keep.Symptom
After clicking Send, the entire Qt GUI becomes unresponsive for several seconds (tens of seconds on larger wallets). Scales with the number of stored invoices, not with the size of the new transaction.
Root cause
_invoices_from_scriptpubkey_mapmapsscriptpubkey -> set[invoice_key]. When many invoices share an output scriptpubkey (very common in real wallets — repeat payments to the same destination), this set is large. On broadcast:set_broadcasting(tx, PR_BROADCASTING)iterates every touched invoice and callsget_invoice_statuson each._update_onchain_invoice_paid_detectionruns the same loop again after the tx is added to the address synchronizer.set_broadcasting(tx, PR_BROADCAST)runs it a third time on success.Each
get_invoice_statuson an onchain invoice calls_is_onchain_invoice_paid, which scans every output scripthash against_prevouts_by_scripthashand does aget_tx_heightper prevout. With shared inputs/outputs and many past payments, both inner dimensions blow up together. All of this runs on the GUI thread.Fix
Two commits:
wallet: cache paid-invoice keys to avoid prevout rescanAdds
_paid_invoice_keys: Set[str]toAbstract_Wallet, an in-memory cache of outgoing invoice ids known to bePR_PAID, maintained incrementally:_prepare_onchain_invoice_paid_detection_update_onchain_invoice_paid_detection(discard then recompute, so reorgs still demote PAID->UNPAID)save_invoice,delete_invoice,clear_invoices,clear_historyon_event_invoice_statuslistener keeps it in sync with LN-driven transitions fromLNWallet.set_invoice_statusBoth
save_invoiceand_update_onchain_invoice_paid_detectionreuse the result of the existing(is_paid, conf)computation instead of triggering a second prevout scan viaget_invoice_status.get_invoice_statusshort-circuits toPR_PAIDon cache hit.set_broadcastingskips already-paid invoices entirely —broadcasting_statushas no effect on a paid invoice and the callback churn is what made the GUI freeze.qt/send_tab: drop redundant invoice_list.update() after broadcastThe explicit full rebuild after a successful broadcast is unnecessary: the
set_broadcasting(PR_BROADCAST)call immediately after fires theinvoice_statuscallback for any touched invoices, andmain_window.on_event_invoice_statusupdates the affected rows incrementally. Tx ingestion also setsneed_update, which causesupdate_tabs()to refresh.Testing
Unit tests: added
TestOutgoingInvoicesPaidCache(9 tests) covering: empty/populated cache state, invoice-paid event flow, delete/clear cleanup, the short-circuit inget_invoice_status,set_broadcastingskipping cached-paid invoices, the shared-scriptpubkey regression scenario, cache population at wallet load, cache demotion on reorg (verified tx unverified), and cache reset onclear_history.Also ran
tests.test_lnpeerandtests.test_lnwallet— 143/143 pass.Real-world testing: I ran a patched build on macOS against my actual long-lived wallet — the one with the 500+ invoices that motivated the fix. The freeze is gone; sending a transaction is now instant.
Notes for reviewers
on_event_adb_removed_verified_tx, and history wipe viaclear_history) still flows through code that discards the cache entry before any subsequent read. Both transitions have explicit regression tests._paid_invoice_keysis only ever touched with single set operations (add/discard/in/clear), no iteration, which is safe under the GIL.on_event_invoice_statuslistener firing on the wallet's own callbacks is idempotent and intentional (defense in depth for any future callsite that firesinvoice_statuswithout going through the existing paths)._update_onchain_invoice_paid_detectionnow derives status from the already-computed(is_paid, conf_needed)tuple instead of callingget_invoice_status, which previously re-ranis_onchain_invoice_paid— halving the per-invoice prevout work in_prepare_onchain_invoice_paid_detection.