From 4d90ab9ac4e76f507abd7adc66017ef01b0fa8b0 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 4 Aug 2025 16:58:15 +0000 Subject: [PATCH 1/3] WalletDB: add more asserts to {add,get}|_verified_tx to fail early before writing to and right after reading from db ref https://github.com/spesmilo/electrum/issues/10098 --- electrum/wallet_db.py | 14 +++++++++++--- tests/test_txbatcher.py | 13 +++++++++---- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/electrum/wallet_db.py b/electrum/wallet_db.py index 8f37430dc4b8..cb2b93b78f06 100644 --- a/electrum/wallet_db.py +++ b/electrum/wallet_db.py @@ -38,7 +38,7 @@ import attr from . import util, bitcoin -from .util import profiler, WalletFileException, multisig_type, TxMinedInfo, bfh, MyEncoder +from .util import profiler, WalletFileException, multisig_type, TxMinedInfo, bfh, MyEncoder, is_hash256_str from .invoices import Invoice, Request from .keystore import bip44_derivation from .transaction import Transaction, TxOutpoint, tx_from_any, PartialTransaction, PartialTxOutput, BadHeaderMagic @@ -1504,10 +1504,14 @@ def list_verified_tx(self) -> Sequence[str]: @locked def get_verified_tx(self, txid: str) -> Optional[TxMinedInfo]: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid if txid not in self.verified_tx: return None height, timestamp, txpos, header_hash = self.verified_tx[txid] + assert isinstance(height, int) and height > 0, height + assert isinstance(timestamp, int), timestamp + assert isinstance(txpos, int) and txpos >= 0, txpos + assert is_hash256_str(header_hash), header_hash return TxMinedInfo(height=height, conf=None, timestamp=timestamp, @@ -1516,8 +1520,12 @@ def get_verified_tx(self, txid: str) -> Optional[TxMinedInfo]: @modifier def add_verified_tx(self, txid: str, info: TxMinedInfo): - assert isinstance(txid, str) + assert is_hash256_str(txid), txid assert isinstance(info, TxMinedInfo) + assert isinstance(info.height, int) and info.height > 0, info.height + assert isinstance(info.timestamp, int), info.timestamp + assert isinstance(info.txpos, int) and info.txpos >= 0, info.txpos + assert is_hash256_str(info.header_hash), info.header_hash self.verified_tx[txid] = (info.height, info.timestamp, info.txpos, info.header_hash) @modifier diff --git a/tests/test_txbatcher.py b/tests/test_txbatcher.py index d257e5799d96..7690c890b981 100644 --- a/tests/test_txbatcher.py +++ b/tests/test_txbatcher.py @@ -7,6 +7,7 @@ from electrum import storage, bitcoin, keystore, wallet from electrum import SimpleConfig from electrum import util +from electrum.util import TxMinedInfo from electrum.address_synchronizer import TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT, TX_HEIGHT_LOCAL from electrum.transaction import Transaction, PartialTxInput, PartialTxOutput, TxOutpoint from electrum.logging import console_stderr_handler, Logger @@ -151,8 +152,10 @@ async def test_batch_payments(self, mock_save_db): assert output2 in tx1_prime.outputs() # tx1 gets confirmed, tx2 gets removed wallet.adb.receive_tx_callback(tx1, tx_height=1) - tx_mined_status = wallet.adb.get_tx_height(tx1.txid()) - wallet.adb.add_verified_tx(tx1.txid(), tx_mined_status._replace(conf=1)) + wallet.adb.add_verified_tx( + tx1.txid(), + TxMinedInfo(height=1, timestamp=999999, txpos=0, header_hash="aa"*32), + ) assert wallet.adb.get_transaction(tx1.txid()) is not None assert wallet.adb.get_transaction(tx1_prime.txid()) is None # txbatcher creates tx2 @@ -194,8 +197,10 @@ async def test_rbf_batching__cannot_batch_as_would_need_to_use_ismine_outputs_of # tx1 gets confirmed wallet.adb.receive_tx_callback(tx1, tx_height=1) - tx_mined_status = wallet.adb.get_tx_height(tx1.txid()) - wallet.adb.add_verified_tx(tx1.txid(), tx_mined_status._replace(conf=1)) + wallet.adb.add_verified_tx( + tx1.txid(), + TxMinedInfo(height=1, timestamp=999999, txpos=0, header_hash="aa"*32), + ) tx2 = await self.network.next_tx() assert len(tx2.outputs()) == 2 From 38241ff6a54d898a94bc7684d5c15d79c7e940d0 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 4 Aug 2025 17:37:27 +0000 Subject: [PATCH 2/3] wallet: change adb_added_verified_tx evt API, passthrough txminedinfo - call adb.get_tx_height(tx_hash) earlier, and pass result through subsequent events - add assert to sanity-check result right after it is calculated ref https://github.com/spesmilo/electrum/issues/10098 --- electrum/address_synchronizer.py | 5 ++++- electrum/gui/qml/qetransactionlistmodel.py | 4 ++-- electrum/lnwatcher.py | 2 +- electrum/plugins/watchtower/watchtower.py | 4 ++-- electrum/wallet.py | 5 ++--- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/electrum/address_synchronizer.py b/electrum/address_synchronizer.py index 6183f96ee116..88348e34aca7 100644 --- a/electrum/address_synchronizer.py +++ b/electrum/address_synchronizer.py @@ -651,7 +651,10 @@ def add_verified_tx(self, tx_hash: str, info: TxMinedInfo): with self.lock: self.unverified_tx.pop(tx_hash, None) self.db.add_verified_tx(tx_hash, info) - util.trigger_callback('adb_added_verified_tx', self, tx_hash) + del info + info2 = self.get_tx_height(tx_hash) # populates the 'conf' field + assert isinstance(info2.conf, int) and info2.conf > 0, f"{info2.conf=}, {info2.height=}, {self.get_local_height()=}" + util.trigger_callback('adb_added_verified_tx', self, tx_hash, info2) @with_lock def get_unverified_txs(self) -> Dict[str, int]: diff --git a/electrum/gui/qml/qetransactionlistmodel.py b/electrum/gui/qml/qetransactionlistmodel.py index 3b94d25bfa97..6d7ecbe2098b 100644 --- a/electrum/gui/qml/qetransactionlistmodel.py +++ b/electrum/gui/qml/qetransactionlistmodel.py @@ -47,7 +47,7 @@ def on_destroy(self): self.unregister_callbacks() @qt_event_listener - def on_event_verified(self, wallet, txid, info): + def on_event_verified(self, wallet, txid: str, info: TxMinedInfo): if wallet == self.wallet: self._logger.debug('verified event for txid %s' % txid) self.on_tx_verified(txid, info) @@ -228,7 +228,7 @@ def initModel(self, force: bool = False): self._dirty = False - def on_tx_verified(self, txid, info): + def on_tx_verified(self, txid: str, info: TxMinedInfo): for i, tx in enumerate(self.tx_history): if 'txid' in tx and tx['txid'] == txid: tx['height'] = info.height diff --git a/electrum/lnwatcher.py b/electrum/lnwatcher.py index 126fb94f9957..c3ed569d3135 100644 --- a/electrum/lnwatcher.py +++ b/electrum/lnwatcher.py @@ -72,7 +72,7 @@ async def on_event_adb_added_tx(self, adb, tx_hash, tx): await self.trigger_callbacks() @event_listener - async def on_event_adb_added_verified_tx(self, adb, tx_hash): + async def on_event_adb_added_verified_tx(self, adb, tx_hash, info: TxMinedInfo): if adb != self.adb: return await self.trigger_callbacks() diff --git a/electrum/plugins/watchtower/watchtower.py b/electrum/plugins/watchtower/watchtower.py index dd39ebd497c1..3d331ae68063 100644 --- a/electrum/plugins/watchtower/watchtower.py +++ b/electrum/plugins/watchtower/watchtower.py @@ -38,7 +38,7 @@ from electrum.wallet_db import WalletDB from electrum.lnutil import WITNESS_TEMPLATE_RECEIVED_HTLC, WITNESS_TEMPLATE_OFFERED_HTLC from electrum.logging import Logger -from electrum.util import EventListener, event_listener +from electrum.util import EventListener, event_listener, TxMinedInfo from .server import WatchTowerServer @@ -96,7 +96,7 @@ async def on_event_wallet_updated(self, wallet): await self.trigger_callbacks() @event_listener - async def on_event_adb_added_verified_tx(self, adb, tx_hash): + async def on_event_adb_added_verified_tx(self, adb, tx_hash, info: TxMinedInfo): if adb != self.adb: return await self.trigger_callbacks() diff --git a/electrum/wallet.py b/electrum/wallet.py index 1682cb49ed89..9a61bc99c6ed 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -633,12 +633,11 @@ def on_event_adb_removed_tx(self, adb, txid: str, tx: Transaction): util.trigger_callback('removed_transaction', self, tx) @event_listener - def on_event_adb_added_verified_tx(self, adb, tx_hash): + def on_event_adb_added_verified_tx(self, adb, tx_hash, info: TxMinedInfo): if adb != self.adb: return self._update_invoices_and_reqs_touched_by_tx(tx_hash) - tx_mined_status = self.adb.get_tx_height(tx_hash) - util.trigger_callback('verified', self, tx_hash, tx_mined_status) + util.trigger_callback('verified', self, tx_hash, info) @event_listener def on_event_adb_removed_verified_tx(self, adb, tx_hash): From 596fde0d0555368fca761fe7fa2aeb41e06beb27 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 4 Aug 2025 17:51:57 +0000 Subject: [PATCH 3/3] WalletDB: further tighten type-check asserts --- electrum/wallet_db.py | 56 +++++++++++++++++++++---------------------- tests/test_wallet.py | 22 ++++++++--------- 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/electrum/wallet_db.py b/electrum/wallet_db.py index cb2b93b78f06..e5afecd4ae22 100644 --- a/electrum/wallet_db.py +++ b/electrum/wallet_db.py @@ -1312,19 +1312,19 @@ def get_db_metadata(self) -> Optional[DBMetadata]: @locked def get_txi_addresses(self, tx_hash: str) -> List[str]: """Returns list of is_mine addresses that appear as inputs in tx.""" - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash return list(self.txi.get(tx_hash, {}).keys()) @locked def get_txo_addresses(self, tx_hash: str) -> List[str]: """Returns list of is_mine addresses that appear as outputs in tx.""" - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash return list(self.txo.get(tx_hash, {}).keys()) @locked def get_txi_addr(self, tx_hash: str, address: str) -> Iterable[Tuple[str, int]]: """Returns an iterable of (prev_outpoint, value).""" - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash assert isinstance(address, str) d = self.txi.get(tx_hash, {}).get(address, {}) return list(d.items()) @@ -1332,14 +1332,14 @@ def get_txi_addr(self, tx_hash: str, address: str) -> Iterable[Tuple[str, int]]: @locked def get_txo_addr(self, tx_hash: str, address: str) -> Dict[int, Tuple[int, bool]]: """Returns a dict: output_index -> (value, is_coinbase).""" - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash assert isinstance(address, str) d = self.txo.get(tx_hash, {}).get(address, {}) return {int(n): (v, cb) for (n, (v, cb)) in d.items()} @modifier def add_txi_addr(self, tx_hash: str, addr: str, ser: str, v: int) -> None: - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash assert isinstance(addr, str) assert isinstance(ser, str) assert isinstance(v, int) @@ -1353,7 +1353,7 @@ def add_txi_addr(self, tx_hash: str, addr: str, ser: str, v: int) -> None: @modifier def add_txo_addr(self, tx_hash: str, addr: str, n: Union[int, str], v: int, is_coinbase: bool) -> None: n = str(n) - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash assert isinstance(addr, str) assert isinstance(n, str) assert isinstance(v, int) @@ -1375,12 +1375,12 @@ def list_txo(self) -> Sequence[str]: @modifier def remove_txi(self, tx_hash: str) -> None: - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash self.txi.pop(tx_hash, None) @modifier def remove_txo(self, tx_hash: str) -> None: - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash self.txo.pop(tx_hash, None) @locked @@ -1392,18 +1392,18 @@ def list_spent_outpoints(self) -> Sequence[Tuple[str, str]]: @locked def get_spent_outpoints(self, prevout_hash: str) -> Sequence[str]: - assert isinstance(prevout_hash, str) + assert is_hash256_str(prevout_hash), prevout_hash return list(self.spent_outpoints.get(prevout_hash, {}).keys()) @locked def get_spent_outpoint(self, prevout_hash: str, prevout_n: Union[int, str]) -> Optional[str]: - assert isinstance(prevout_hash, str) + assert is_hash256_str(prevout_hash), prevout_hash prevout_n = str(prevout_n) return self.spent_outpoints.get(prevout_hash, {}).get(prevout_n) @modifier def remove_spent_outpoint(self, prevout_hash: str, prevout_n: Union[int, str]) -> None: - assert isinstance(prevout_hash, str) + assert is_hash256_str(prevout_hash), prevout_hash prevout_n = str(prevout_n) self.spent_outpoints[prevout_hash].pop(prevout_n, None) if not self.spent_outpoints[prevout_hash]: @@ -1411,8 +1411,8 @@ def remove_spent_outpoint(self, prevout_hash: str, prevout_n: Union[int, str]) - @modifier def set_spent_outpoint(self, prevout_hash: str, prevout_n: Union[int, str], tx_hash: str) -> None: - assert isinstance(prevout_hash, str) - assert isinstance(tx_hash, str) + assert is_hash256_str(prevout_hash), prevout_hash + assert is_hash256_str(tx_hash), tx_hash prevout_n = str(prevout_n) if prevout_hash not in self.spent_outpoints: self.spent_outpoints[prevout_hash] = {} @@ -1420,7 +1420,7 @@ def set_spent_outpoint(self, prevout_hash: str, prevout_n: Union[int, str], tx_h @modifier def add_prevout_by_scripthash(self, scripthash: str, *, prevout: TxOutpoint, value: int) -> None: - assert isinstance(scripthash, str) + assert is_hash256_str(scripthash) assert isinstance(prevout, TxOutpoint) assert isinstance(value, int) if scripthash not in self._prevouts_by_scripthash: @@ -1429,7 +1429,7 @@ def add_prevout_by_scripthash(self, scripthash: str, *, prevout: TxOutpoint, val @modifier def remove_prevout_by_scripthash(self, scripthash: str, *, prevout: TxOutpoint, value: int) -> None: - assert isinstance(scripthash, str) + assert is_hash256_str(scripthash) assert isinstance(prevout, TxOutpoint) assert isinstance(value, int) self._prevouts_by_scripthash[scripthash].pop(prevout.to_str(), None) @@ -1438,13 +1438,13 @@ def remove_prevout_by_scripthash(self, scripthash: str, *, prevout: TxOutpoint, @locked def get_prevouts_by_scripthash(self, scripthash: str) -> Set[Tuple[TxOutpoint, int]]: - assert isinstance(scripthash, str) + assert is_hash256_str(scripthash) prevouts_and_values = self._prevouts_by_scripthash.get(scripthash, {}) return {(TxOutpoint.from_str(prevout), value) for prevout, value in prevouts_and_values.items()} @modifier def add_transaction(self, tx_hash: str, tx: Transaction) -> None: - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash assert isinstance(tx, Transaction), tx # note that tx might be a PartialTransaction # serialize and de-serialize tx now. this might e.g. convert a complete PartialTx to a Tx @@ -1460,14 +1460,14 @@ def add_transaction(self, tx_hash: str, tx: Transaction) -> None: @modifier def remove_transaction(self, tx_hash: str) -> Optional[Transaction]: - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash return self.transactions.pop(tx_hash, None) @locked def get_transaction(self, tx_hash: Optional[str]) -> Optional[Transaction]: if tx_hash is None: return None - assert isinstance(tx_hash, str) + assert is_hash256_str(tx_hash), tx_hash return self.transactions.get(tx_hash) @locked @@ -1530,16 +1530,16 @@ def add_verified_tx(self, txid: str, info: TxMinedInfo): @modifier def remove_verified_tx(self, txid: str): - assert isinstance(txid, str) + assert is_hash256_str(txid), txid self.verified_tx.pop(txid, None) def is_in_verified_tx(self, txid: str) -> bool: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid return txid in self.verified_tx @modifier def add_tx_fee_from_server(self, txid: str, fee_sat: Optional[int]) -> None: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid # note: when called with (fee_sat is None), rm currently saved value if txid not in self.tx_fees: self.tx_fees[txid] = TxFeesValue() @@ -1550,7 +1550,7 @@ def add_tx_fee_from_server(self, txid: str, fee_sat: Optional[int]) -> None: @modifier def add_tx_fee_we_calculated(self, txid: str, fee_sat: Optional[int]) -> None: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid if fee_sat is None: return assert isinstance(fee_sat, int) @@ -1560,7 +1560,7 @@ def add_tx_fee_we_calculated(self, txid: str, fee_sat: Optional[int]) -> None: @locked def get_tx_fee(self, txid: str, *, trust_server: bool = False) -> Optional[int]: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid """Returns tx_fee.""" tx_fees_value = self.tx_fees.get(txid) if tx_fees_value is None: @@ -1571,7 +1571,7 @@ def get_tx_fee(self, txid: str, *, trust_server: bool = False) -> Optional[int]: @modifier def add_num_inputs_to_tx(self, txid: str, num_inputs: int) -> None: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid assert isinstance(num_inputs, int) if txid not in self.tx_fees: self.tx_fees[txid] = TxFeesValue() @@ -1579,7 +1579,7 @@ def add_num_inputs_to_tx(self, txid: str, num_inputs: int) -> None: @locked def get_num_all_inputs_of_tx(self, txid: str) -> Optional[int]: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid tx_fees_value = self.tx_fees.get(txid) if tx_fees_value is None: return None @@ -1587,13 +1587,13 @@ def get_num_all_inputs_of_tx(self, txid: str) -> Optional[int]: @locked def get_num_ismine_inputs_of_tx(self, txid: str) -> int: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid txins = self.txi.get(txid, {}) return sum([len(tupls) for addr, tupls in txins.items()]) @modifier def remove_tx_fee(self, txid: str) -> None: - assert isinstance(txid, str) + assert is_hash256_str(txid), txid self.tx_fees.pop(txid, None) @locked diff --git a/tests/test_wallet.py b/tests/test_wallet.py index 0fa9d8910fbc..dd18e11c3dcf 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -171,14 +171,14 @@ def __init__(self, fiat_value): self.fiat_value = fiat_value self.db = WalletDB('', storage=None, upgrade=False) self.adb = FakeADB() - self.db.transactions = self.db.verified_tx = {'abc':'Tx'} + self.db.transactions = self.db.verified_tx = {TXID1:'Tx'} default_fiat_value = Abstract_Wallet.default_fiat_value price_at_timestamp = Abstract_Wallet.price_at_timestamp class storage: put = lambda self, x: None -txid = 'abc' +TXID1 = 'aa' * 32 ccy = 'TEST' class TestFiat(ElectrumTestCase): @@ -188,28 +188,28 @@ def setUp(self): self.fiat_value = {} self.wallet = FakeWallet(fiat_value=self.fiat_value) self.fx = FakeFxThread(FakeExchange(Decimal('1000.001'))) - default_fiat = Abstract_Wallet.default_fiat_value(self.wallet, txid, self.fx, self.value_sat) + default_fiat = Abstract_Wallet.default_fiat_value(self.wallet, TXID1, self.fx, self.value_sat) self.assertEqual(Decimal('1000.001'), default_fiat) self.assertEqual('1 000.00', self.fx.ccy_amount_str(default_fiat, add_thousands_sep=True)) def test_save_fiat_and_reset(self): - self.assertEqual(False, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, '1000.01', self.fx, self.value_sat)) - saved = self.fiat_value[ccy][txid] + self.assertEqual(False, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, '1000.01', self.fx, self.value_sat)) + saved = self.fiat_value[ccy][TXID1] self.assertEqual('1 000.01', self.fx.ccy_amount_str(Decimal(saved), add_thousands_sep=True)) - self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, '', self.fx, self.value_sat)) - self.assertNotIn(txid, self.fiat_value[ccy]) + self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, '', self.fx, self.value_sat)) + self.assertNotIn(TXID1, self.fiat_value[ccy]) # even though we are not setting it to the exact fiat value according to the exchange rate, precision is truncated away - self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, '1 000.002', self.fx, self.value_sat)) + self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, '1 000.002', self.fx, self.value_sat)) def test_too_high_precision_value_resets_with_no_saved_value(self): - self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, '1 000.001', self.fx, self.value_sat)) + self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, '1 000.001', self.fx, self.value_sat)) def test_empty_resets(self): - self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, '', self.fx, self.value_sat)) + self.assertEqual(True, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, '', self.fx, self.value_sat)) self.assertNotIn(ccy, self.fiat_value) def test_save_garbage(self): - self.assertEqual(False, Abstract_Wallet.set_fiat_value(self.wallet, txid, ccy, 'garbage', self.fx, self.value_sat)) + self.assertEqual(False, Abstract_Wallet.set_fiat_value(self.wallet, TXID1, ccy, 'garbage', self.fx, self.value_sat)) self.assertNotIn(ccy, self.fiat_value)