wallet: keep height and txpos in sync in wallet.add_input_info()
What the developer wrote
Authored by Sander van Grieken
wallet: keep height and txpos in sync in wallet.add_input_info()
a change in funding tx status (e.g. from verified to back in mempool due to reorg)
can change both height and txpos, but txpos was not kept in sync here. match
what adb.get_transaction() does.
also guard TxInput.short_id on both height and txpos
What changed, and why it matters
This commit fixes a bookkeeping bug in the Electrum Bitcoin wallet. When a transaction's status changes due to a blockchain reorganization (for example, a confirmed transaction temporarily returns to the unconfirmed mempool), the wallet was updating a coin's 'block height' but forgetting to update its 'position within the block' (txpos). The two values were getting out of sync, which could make the wallet display or identify coins incorrectly. The fix keeps both values together and adds a safety check so the short display ID is only used when both height and position are valid.
Treat as a routine correctness fix. Review whether stale txpos could have led to incorrect coin selection, fee estimation, or user-visible transaction identification in edge cases involving reorgs. No immediate emergency response is indicated by the diff alone.
Security signals we found
State inconsistency between block_height and block_txpos after reorg or status transition
UI/model code relied on duplicated, weaker short_id validity check
Fix aligns add_input_info() behavior with adb.get_transaction()
No explicit vulnerability disclosure or CVE referenced in commit
Evidence from the diff
In wallet.add_input_info(), the code previously set txin.block_height from adb.get_tx_height() but left txin.block_txpos stale. Because a funding transaction can move between verified, unverified, and unconfirmed states (especially during reorgs), both height and txpos can change together. The patch now assigns txin.block_txpos from the same TxMinedInfo object. It also adds a has_short_id() helper requiring block_height > 0 and block_txpos >= 0, mirrors that guard in TxInput.short_id, and cleans up an ad-hoc duplicate check in the QML address list model. address_synchronizer.add_unverified_or_unconfirmed_tx() is also tightened to remove a tx_hash from the opposite map when its height category changes.
Changed components
electrum/wallet.pyelectrum/transaction.pyelectrum/address_synchronizer.pyelectrum/gui/qml/qeaddresslistmodel.pyInspect captured patch +34 / −6
### electrum/address_synchronizer.py
@@ -639,8 +639,10 @@ def add_unverified_or_unconfirmed_tx(self, tx_hash: str, tx_height: int) -> None
self.verifier.remove_spv_proof_for_tx(tx_hash)
else:
if tx_height > 0:
+ self.unconfirmed_tx.pop(tx_hash, None)
self.unverified_tx[tx_hash] = tx_height
else:
+ self.unverified_tx.pop(tx_hash, None)
self.unconfirmed_tx[tx_hash] = tx_height
@with_lock
### electrum/gui/qml/qeaddresslistmodel.py
@@ -177,10 +177,8 @@ def addr_to_model(self, addrtype: str, addridx: int, address: str):
def coin_to_model(self, addrtype: str, coin: 'PartialTxInput'):
txid = coin.prevout.txid.hex()
- short_id = ''
- # check below duplicated from TxInput as we cannot get short_id unambiguously
- if coin.block_txpos is not None and coin.block_txpos >= 0:
- short_id = str(coin.short_id)
+ # short_id falls back to the outpoint if the coin is not mined, hence the check
+ short_id = str(coin.short_id) if coin.has_short_id() else ''
item = {
'type': addrtype,
'amount': QEAmount(amount_sat=coin.value_sats()),
### electrum/transaction.py
@@ -363,9 +363,13 @@ def get_block_based_relative_locktime(self) -> Optional[int]:
return self.nsequence & 0xffff
return None
+ def has_short_id(self) -> bool:
+ return (self.block_height is not None and self.block_height > 0
+ and self.block_txpos is not None and self.block_txpos >= 0)
+
@property
def short_id(self):
- if self.block_txpos is not None and self.block_txpos >= 0:
+ if self.has_short_id():
return ShortID.from_components(self.block_height, self.block_txpos, self.prevout.out_idx)
else:
return self.prevout.short_name()
### electrum/wallet.py
@@ -2737,7 +2737,9 @@ def add_input_info(
txin.script_descriptor = desc
txin.is_mine = True
self._add_txinout_derivation_info(txin, address, only_der_suffix=only_der_suffix)
- txin.block_height = self.adb.get_tx_height(txin.prevout.txid.hex()).height()
+ tx_mined_info = self.adb.get_tx_height(txin.prevout.txid.hex())
+ txin.block_height = tx_mined_info.height()
+ txin.block_txpos = tx_mined_info.txpos
def has_support_for_slip_19_ownership_proofs(self) -> bool:
return False
### tests/test_transaction.py
@@ -159,6 +159,28 @@ def test_estimated_output_size(self):
self.assertEqual(estimated_output_size('bc1q3g5tmkmlvxryhh843v4dz026avatc0zzr6h3af'), 31)
self.assertEqual(estimated_output_size('bc1qnvks7gfdu72de8qv6q6rhkkzu70fqz4wpjzuxjf6aydsx7wxfwcqnlxuv3'), 43)
+ def test_txin_short_id(self):
+ prevout = TxOutpoint.from_str(
+ 'db949963c3787c90a40fb689ffdc3146c27a9874a970d1fd20921afbe79a7aa9:0')
+ outpoint_str = 'db949963c3:0'
+
+ def short_id_for(*, block_height, block_txpos):
+ txin = PartialTxInput(prevout=prevout)
+ txin.block_height = block_height
+ txin.block_txpos = block_txpos
+ return str(txin.short_id)
+
+ # mined and SPV-ed:
+ self.assertEqual('600000x7x0', short_id_for(block_height=600000, block_txpos=7))
+ self.assertEqual('600000x0x0', short_id_for(block_height=600000, block_txpos=0))
+ # position in block unknown:
+ self.assertEqual(outpoint_str, short_id_for(block_height=600000, block_txpos=None))
+ self.assertEqual(outpoint_str, short_id_for(block_height=600000, block_txpos=-1))
+ # not mined (or not known to be mined).
+ for height in (0, -1, -2, -3, None):
+ self.assertEqual(outpoint_str, short_id_for(block_height=height, block_txpos=7))
+ self.assertEqual(outpoint_str, short_id_for(block_height=height, block_txpos=None))
+
# TODO other tests for segwit tx
def test_tx_signed_segwit(self):
tx = transaction.Transaction(signed_segwit_blob)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.