wallet: de-sloppify d2d4251c8 (paid invoice cache)
What the developer wrote
Authored by SomberNight
wallet: de-sloppify d2d4251c8 (paid invoice cache)
arghhhhhh
What changed, and why it matters
This commit is a cleanup of a previous change that added a cache for paid invoices. The developer admits the earlier code was sloppy ('arghhhhhh'). The patch removes redundant logic that tried to update the paid-invoice cache in multiple places and instead lets a single method, get_invoice_status, decide whether an invoice is paid. The risk is that if the cache is not updated correctly, Electrum might wrongly report an invoice as paid or unpaid, which could mislead users or merchants about whether money was actually received. There is no direct evidence this is exploitable by an attacker, but it is a correctness fix in payment handling.
Reviewers should verify that get_invoice_status correctly updates _paid_invoice_keys_cache in all code paths, especially for on-chain invoices with 0 confirmations, expired invoices, and after blockchain reorgs. Users should update to the version containing this commit if they rely on accurate invoice payment status, but no urgent security patch is indicated by the diff alone.
Security signals we found
Cache consistency refactoring in payment status logic
Developer self-described as 'de-sloppify' of prior commit
Payment status correctness affects merchant/user funds
Reorg handling test updated to verify paid->unpaid transition
Removal of duplicated paid-status computation paths
Evidence from the diff
The commit refactors the _paid_invoice_keys_cache management in electrum/wallet.py. Previously, save_invoice and _update_invoices_and_paid_cache both independently computed paid status and added/discarded keys from the cache. The new code removes that duplicated logic from save_invoice and relies on get_invoice_status to update the cache consistently. A test is updated to trigger the ‘adb_removed_verified_tx’ callback directly instead of calling a removed event handler. The change is defensive refactoring rather than a clear vulnerability fix, but it addresses potential cache inconsistency bugs that could cause incorrect payment status reporting after reorgs or invoice saves.
Changed components
electrum/wallet.pytests/test_invoices.pyAbstract_Wallet._paid_invoice_keys_cacheAbstract_Wallet.save_invoiceAbstract_Wallet._update_invoices_and_paid_cacheInspect captured patch +6 / −21
diff --git a/electrum/wallet.py b/electrum/wallet.py
index 20e7d20..601879a 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -672,7 +672,6 @@ class Abstract_Wallet(ABC, Logger, EventListener):
def clear_history(self):
self.adb.clear_history()
- # the paid-keys cache is derived from on-chain prevouts that adb.clear_history just wiped
self._paid_invoice_keys_cache.clear()
self.save_db()
@@ -1294,21 +1293,13 @@ class Abstract_Wallet(ABC, Logger, EventListener):
def save_invoice(self, invoice: Invoice, *, write_to_disk: bool = True) -> None:
key = invoice.get_id()
- if invoice.is_lightning():
- is_paid = bool(self.lnworker and self.lnworker.get_invoice_status(invoice) == PR_PAID)
- else:
- is_paid_onchain, conf = self.is_onchain_invoice_paid(invoice)
- if is_paid_onchain:
+ if not invoice.is_lightning():
+ if self.is_onchain_invoice_paid(invoice)[0]:
_logger.info("saving invoice... but it is already paid!")
with self.lock:
for txout in invoice.get_outputs():
self._invoices_from_scriptpubkey_map[txout.scriptpubkey].add(key)
- is_paid = is_paid_onchain and conf is not None and conf >= 1
self._invoices[key] = invoice
- if is_paid:
- self._paid_invoice_keys_cache.add(key)
- else:
- self._paid_invoice_keys_cache.discard(key)
if write_to_disk:
self.save_db()
@@ -1388,9 +1379,8 @@ class Abstract_Wallet(ABC, Logger, EventListener):
for invoice_key in invoice_keys:
invoice = self._invoices.get(invoice_key)
if not invoice:
- self._paid_invoice_keys_cache.discard(invoice_key)
continue
- # clear the cache first so get_invoice_status takes the slow path,
+ # clear the cache first so self.get_invoice_status takes the slow path,
# which is needed to detect paid->unpaid transitions (e.g. reorgs)
self._paid_invoice_keys_cache.discard(invoice_key)
if invoice.is_lightning():
@@ -1401,17 +1391,12 @@ class Abstract_Wallet(ABC, Logger, EventListener):
continue
is_paid, conf_needed, relevant_txs = self._is_onchain_invoice_paid(invoice)
if is_paid:
- assert conf_needed is not None and conf_needed >= 0
- status = PR_UNCONFIRMED if conf_needed == 0 else PR_PAID
for txid in relevant_txs:
self._invoices_from_txid_map[txid].add(invoice_key)
- else:
- status = invoice.get_broadcasting_status() or PR_UNPAID
for txout in invoice.get_outputs():
self._invoices_from_scriptpubkey_map[txout.scriptpubkey].add(invoice_key)
- status = self.check_expired_status(invoice, status)
- if status == PR_PAID:
- self._paid_invoice_keys_cache.add(invoice_key)
+ # update invoice status
+ status = self.get_invoice_status(invoice)
util.trigger_callback('invoice_status', self, invoice_key, status)
def _is_onchain_invoice_paid(self, invoice: BaseInvoice) -> Tuple[bool, Optional[int], Sequence[str]]:
diff --git a/tests/test_invoices.py b/tests/test_invoices.py
index 5e42cf3..f188559 100644
--- a/tests/test_invoices.py
+++ b/tests/test_invoices.py
@@ -447,7 +447,7 @@ class TestOutgoingInvoicesPaidCache(ElectrumTestCase):
self.assertIn(inv.get_id(), wallet._paid_invoice_keys_cache)
# Simulate reorg: unverify the tx and fire the same event the verifier would.
wallet.adb.db.remove_verified_tx(tx.txid())
- wallet.on_event_adb_removed_verified_tx(wallet.adb, tx.txid())
+ util.trigger_callback('adb_removed_verified_tx', wallet.adb, tx.txid())
self.assertNotIn(inv.get_id(), wallet._paid_invoice_keys_cache)
self.assertNotEqual(PR_PAID, wallet.get_invoice_status(inv))
Why this scored 35/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.