wallet: avoid redundant prevout rescans in invoice paid detection#10736
Open
sashazykov wants to merge 4 commits into
Open
wallet: avoid redundant prevout rescans in invoice paid detection#10736sashazykov wants to merge 4 commits into
sashazykov wants to merge 4 commits into
Conversation
…ection Follow-up to 63ee2f3 (paid invoice cache de-sloppify): _update_onchain_invoice_paid_detection discards the invoice from the paid-keys cache, scans prevouts (needed for relevant_txs), then calls get_invoice_status(), which redoes the same scan as the cache key is gone. This doubles the per-invoice prevout work at wallet load (_prepare_onchain_invoice_paid_detection) on wallets with many invoices. Re-add the key to the cache when the fresh scan says paid with >=1 conf, so get_invoice_status() short-circuits on its existing cache check instead of rescanning. Reorg demotion is unaffected: the re-add only ever happens from a fresh scan. Lightning invoices are excluded, as their LN status takes precedence in get_invoice_status (e.g. PR_INFLIGHT) and must not be masked by a pre-seeded PR_PAID. On a real wallet with 686 invoices (572 sharing one destination scriptpubkey), this halves the detection pass at wallet open: ~36s -> ~18s.
on_event_adb_added_tx and on_event_adb_added_verified_tx rerun onchain paid detection for every invoice sharing a scriptpubkey with the tx. On a wallet where many invoices pay the same destination, every tx touching that destination rescans prevouts for all of them, synchronously on the asyncio event loop thread: ~35s per tx event on a wallet with 686 invoices, 572 sharing one destination - longer than the 30s network request timeout. A tx addition can only promote an invoice towards paid: demotion (paid->unpaid) can only be caused by a tx/verification removal (e.g. reorg), and those paths still rescan unconditionally. So skip invoices already cached as PR_PAID when the update was triggered by a tx addition, like set_broadcasting already does since d2d4251. On the wallet above, a tx event touching the shared destination goes from ~35s (1176 prevout scans) to ~0ms (0 scans) once the invoices are confirmed-paid. Note this does not help invoices that are not yet confirmed (conf==0 cannot enter the cache), so during initial sync from seed, paid detection still reruns per tx; that cost is addressed in the next commit.
_is_onchain_invoice_paid redoes the prevout lookup and per-prevout get_tx_height for each invoice, even though invoices often share a scriptpubkey (repeat payments to the same destination). One _update_onchain_invoice_paid_detection pass over such invoices does the identical lookup for each of them, and unconfirmed invoices (conf==0, e.g. during initial sync before merkle proofs arrive) cannot enter the paid-keys cache, so during sync every tx addition pays the full cost again, on the asyncio event loop thread. Memoize the per-scriptpubkey lookup for the duration of one detection pass; the per-invoice height filtering still applies on top. The cache only exists while a pass runs, so it needs no invalidation: adb state cannot change mid-pass, as both mutate on the asyncio thread. It also covers the second lookup that get_invoice_status does for invoices that cannot be cache-seeded (unconfirmed/unpaid). On a real wallet (686 invoices, 572 sharing one destination): - detection pass at wallet open: ~18s -> ~0.6s (~36s on master) - one adb_added_tx event while invoices are unconfirmed (initial sync): ~18s -> ~0.6s (~35s on master) A full sync from seed with these invoices present went from 12+ hours (GUI frozen, servers timing out) to ~5 minutes.
Each invoice_status event makes InvoiceList.refresh_item recompute the invoice status, which scans prevouts, on the GUI thread. While the wallet is syncing these events arrive in bulk - one per touched invoice per tx addition - so a wallet with many invoices sharing a destination scriptpubkey freezes the GUI for the duration of the sync (observed via py-spy: the GUI thread inside _is_onchain_invoice_paid under refresh_item, while the event loop thread ran a detection pass). While not up_to_date, just set need_update: update_wallet() already defers update_tabs() until is_up_to_date(), and then rebuilds the invoice list once, in one batch.
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
1 participant
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.
Follow-up to #10658 (paid-invoice keys cache). Four small commits addressing the remaining prevout-rescan hotspots in invoice paid detection, all measured on a real wallet with 686 outgoing invoices, accumulated from a year of daily payments to 40-90 recipients — many of whom appear on the list every day, so their invoices pile up on the same addresses: 572 invoices share the single most-reused destination scriptpubkey.
The ~35s cases run synchronously on the asyncio event loop thread — longer than the 30s request timeout, so during initial sync every in-flight request (merkle proofs, txs) was guaranteed to time out, servers kept disconnecting, and the GUI froze. Both threads were observed inside
_is_onchain_invoice_paidvia py-spy: the event loop underon_event_adb_added_tx, and the GUI thread underInvoiceList.refresh_item.Commit 1 — since 63ee2f3, the detection pass discards the cache key, scans prevouts (for
relevant_txs), then callsget_invoice_status(), which rescans. Re-add the key from the fresh scan result (confirmed-paid only; LN status precedence preserved; reorg demotion unaffected). Halves the wallet-open pass. Side effect: on master the cache was actually empty right after__init__(register_callbacks()runs after_prepare_onchain_invoice_paid_detection()), so this also makes load-time cache population effective.Commit 2 — a tx addition can only promote an invoice towards paid; demotion only comes from the removal paths (reorg/conflict), which still rescan unconditionally. So additions skip invoices already cached as
PR_PAID, likeset_broadcastingdoes since d2d4251. Fixes the ~35s per tx in a synced wallet. Does not help unconfirmed invoices (conf==0cannot be cached) — that is addressed by the next commit.Commit 3 — invoices sharing a scriptpubkey each redid the identical prevouts +
get_tx_heightlookups; one pass over 572 sharing invoices did 572+ lookups of the same data. Memoize the per-scriptpubkey lookup for the duration of one detection pass; per-invoice height filtering applies on top. No invalidation is needed: the cache exists only while a pass runs, adb state cannot change mid-pass, and all access is serialized underwallet.lock. Note the diff is dominated by atry/finallyre-indent —git show -wshows the ~37-line logic change.Commit 4 — each
invoice_statusevent made the Qt invoice list recompute the status (a prevout scan) on the GUI thread; while syncing these arrive per touched invoice per tx. While not up-to-date, just setneed_update—update_wallet()already defersupdate_tabs()untilis_up_to_date()and then rebuilds the list once.Testing: three new unit tests spying on the scan/lookup calls (load pass scans each invoice once; tx additions and verifications don't rescan cached-paid invoices; a shared scriptpubkey is looked up once per pass, with per-invoice height filtering pinned by a height-excluded invoice). The existing reorg test still pins paid->unpaid demotion.
tests.test_invoices,tests.test_wallet,tests.test_wallet_vertical: 131 tests pass. Real-world: watch-only wallet restored from the hw wallet's master public key with the invoices imported — full sync from scratch took 12+ hours on master (timeout spam, disconnections, frozen GUI) vs ~5 minutes on this branch.Known remaining (pre-existing, not introduced here):
timer_actionsrefreshes the visible invoice list every ~0.5s with a per-row status recomputation, so keeping the Send tab open during initial sync still costs GUI-thread scans; therequest_statusand QML paths have the same per-event pattern. Happy to follow up on those if there's interest.Happy to privately share the GPG-encrypted watch-only reproduction wallet if useful for testing.