Merge pull request #4538 from SomberNight/verifier_reorgs_st18

verifier: better handle reorgs (and storage upgrade)

electrum/address_synchronizer.py

@@ -27,10 +27,11 @@ from collections import defaultdict
 
 from . import bitcoin
 from .bitcoin import COINBASE_MATURITY, TYPE_ADDRESS, TYPE_PUBKEY
-from .util import PrintError, profiler, bfh
+from .util import PrintError, profiler, bfh, VerifiedTxInfo, TxMinedStatus
 from .transaction import Transaction
 from .synchronizer import Synchronizer
 from .verifier import SPV
+from .blockchain import hash_header
 from .i18n import _
 
 TX_HEIGHT_LOCAL = -2
@@ -45,6 +46,7 @@ class UnrelatedTransactionException(AddTransactionException):
     def __str__(self):
         return _("Transaction is unrelated to this wallet.")
 
+
 class AddressSynchronizer(PrintError):
     """
     inherited by wallet
@@ -61,8 +63,11 @@ class AddressSynchronizer(PrintError):
         self.transaction_lock = threading.RLock()
         # address -> list(txid, height)
         self.history = storage.get('addr_history',{})
-        # Verified transactions.  txid -> (height, timestamp, block_pos).  Access with self.lock.
-        self.verified_tx = storage.get('verified_tx3', {})
+        # Verified transactions.  txid -> VerifiedTxInfo.  Access with self.lock.
+        verified_tx = storage.get('verified_tx3', {})
+        self.verified_tx = {}
+        for txid, (height, timestamp, txpos, header_hash) in verified_tx.items():
+            self.verified_tx[txid] = VerifiedTxInfo(height, timestamp, txpos, header_hash)
         # Transactions pending verification.  txid -> tx_height. Access with self.lock.
         self.unverified_tx = defaultdict(int)
         # true when synchronized
@@ -90,7 +95,7 @@ class AddressSynchronizer(PrintError):
         with self.lock, self.transaction_lock:
             related_txns = self._history_local.get(addr, set())
             for tx_hash in related_txns:
-                tx_height = self.get_tx_height(tx_hash)[0]
+                tx_height = self.get_tx_height(tx_hash).height
                 h.append((tx_hash, tx_height))
         return h
 
@@ -193,7 +198,7 @@ class AddressSynchronizer(PrintError):
             # of add_transaction tx, we might learn of more-and-more inputs of
             # being is_mine, as we roll the gap_limit forward
             is_coinbase = tx.inputs()[0]['type'] == 'coinbase'
-            tx_height = self.get_tx_height(tx_hash)[0]
+            tx_height = self.get_tx_height(tx_hash).height
             if not allow_unrelated:
                 # note that during sync, if the transactions are not properly sorted,
                 # it could happen that we think tx is unrelated but actually one of the inputs is is_mine.
@@ -212,10 +217,10 @@ class AddressSynchronizer(PrintError):
             conflicting_txns = self.get_conflicting_transactions(tx)
             if conflicting_txns:
                 existing_mempool_txn = any(
-                    self.get_tx_height(tx_hash2)[0] in (TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT)
+                    self.get_tx_height(tx_hash2).height in (TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT)
                     for tx_hash2 in conflicting_txns)
                 existing_confirmed_txn = any(
-                    self.get_tx_height(tx_hash2)[0] > 0
+                    self.get_tx_height(tx_hash2).height > 0
                     for tx_hash2 in conflicting_txns)
                 if existing_confirmed_txn and tx_height <= 0:
                     # this is a non-confirmed tx that conflicts with confirmed txns; drop.
@@ -393,7 +398,7 @@ class AddressSynchronizer(PrintError):
     def remove_local_transactions_we_dont_have(self):
         txid_set = set(self.txi) | set(self.txo)
         for txid in txid_set:
-            tx_height = self.get_tx_height(txid)[0]
+            tx_height = self.get_tx_height(txid).height
             if tx_height == TX_HEIGHT_LOCAL and txid not in self.transactions:
                 self.remove_transaction(txid)
 
@@ -431,11 +436,11 @@ class AddressSynchronizer(PrintError):
                 self.save_transactions()
 
     def get_txpos(self, tx_hash):
-        "return position, even if the tx is unverified"
+        """Returns (height, txpos) tuple, even if the tx is unverified."""
         with self.lock:
             if tx_hash in self.verified_tx:
-                height, timestamp, pos = self.verified_tx[tx_hash]
-                return height, pos
+                info = self.verified_tx[tx_hash]
+                return info.height, info.txpos
             elif tx_hash in self.unverified_tx:
                 height = self.unverified_tx[tx_hash]
                 return (height, 0) if height > 0 else ((1e9 - height), 0)
@@ -462,16 +467,16 @@ class AddressSynchronizer(PrintError):
         history = []
         for tx_hash in tx_deltas:
             delta = tx_deltas[tx_hash]
-            height, conf, timestamp = self.get_tx_height(tx_hash)
-            history.append((tx_hash, height, conf, timestamp, delta))
+            tx_mined_status = self.get_tx_height(tx_hash)
+            history.append((tx_hash, tx_mined_status, delta))
         history.sort(key = lambda x: self.get_txpos(x[0]))
         history.reverse()
         # 3. add balance
         c, u, x = self.get_balance(domain)
         balance = c + u + x
         h2 = []
-        for tx_hash, height, conf, timestamp, delta in history:
-            h2.append((tx_hash, height, conf, timestamp, delta, balance))
+        for tx_hash, tx_mined_status, delta in history:
+            h2.append((tx_hash, tx_mined_status, delta, balance))
             if balance is None or delta is None:
                 balance = None
             else:
@@ -503,25 +508,27 @@ class AddressSynchronizer(PrintError):
                     self._history_local[addr] = cur_hist
 
     def add_unverified_tx(self, tx_hash, tx_height):
-        if tx_height in (TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT) \
-                and tx_hash in self.verified_tx:
+        if tx_hash in self.verified_tx:
+            if tx_height in (TX_HEIGHT_UNCONFIRMED, TX_HEIGHT_UNCONF_PARENT):
+                with self.lock:
+                    self.verified_tx.pop(tx_hash)
+                if self.verifier:
+                    self.verifier.remove_spv_proof_for_tx(tx_hash)
+        else:
             with self.lock:
-                self.verified_tx.pop(tx_hash)
+                # tx will be verified only if height > 0
+                self.unverified_tx[tx_hash] = tx_height
+            # to remove pending proof requests:
             if self.verifier:
                 self.verifier.remove_spv_proof_for_tx(tx_hash)
 
-        # tx will be verified only if height > 0
-        if tx_hash not in self.verified_tx:
-            with self.lock:
-                self.unverified_tx[tx_hash] = tx_height
-
-    def add_verified_tx(self, tx_hash, info):
+    def add_verified_tx(self, tx_hash: str, info: VerifiedTxInfo):
         # Remove from the unverified map and add to the verified map
         with self.lock:
             self.unverified_tx.pop(tx_hash, None)
-            self.verified_tx[tx_hash] = info  # (tx_height, timestamp, pos)
-        height, conf, timestamp = self.get_tx_height(tx_hash)
-        self.network.trigger_callback('verified', tx_hash, height, conf, timestamp)
+            self.verified_tx[tx_hash] = info
+        tx_mined_status = self.get_tx_height(tx_hash)
+        self.network.trigger_callback('verified', tx_hash, tx_mined_status)
 
     def get_unverified_txs(self):
         '''Returns a map from tx hash to transaction height'''
@@ -532,13 +539,22 @@ class AddressSynchronizer(PrintError):
         '''Used by the verifier when a reorg has happened'''
         txs = set()
         with self.lock:
-            for tx_hash, item in list(self.verified_tx.items()):
-                tx_height, timestamp, pos = item
+            for tx_hash, info in list(self.verified_tx.items()):
+                tx_height = info.height
                 if tx_height >= height:
                     header = blockchain.read_header(tx_height)
-                    # fixme: use block hash, not timestamp
-                    if not header or header.get('timestamp') != timestamp:
+                    if not header or hash_header(header) != info.header_hash:
                         self.verified_tx.pop(tx_hash, None)
+                        # NOTE: we should add these txns to self.unverified_tx,
+                        # but with what height?
+                        # If on the new fork after the reorg, the txn is at the
+                        # same height, we will not get a status update for the
+                        # address. If the txn is not mined or at a diff height,
+                        # we should get a status update. Unless we put tx into
+                        # unverified_tx, it will turn into local. So we put it
+                        # into unverified_tx with the old height, and if we get
+                        # a status update, that will overwrite it.
+                        self.unverified_tx[tx_hash] = tx_height
                         txs.add(tx_hash)
         return txs
 
@@ -546,19 +562,19 @@ class AddressSynchronizer(PrintError):
         """ return last known height if we are offline """
         return self.network.get_local_height() if self.network else self.storage.get('stored_height', 0)
 
-    def get_tx_height(self, tx_hash):
-        """ Given a transaction, returns (height, conf, timestamp) """
+    def get_tx_height(self, tx_hash: str) -> TxMinedStatus:
+        """ Given a transaction, returns (height, conf, timestamp, header_hash) """
         with self.lock:
             if tx_hash in self.verified_tx:
-                height, timestamp, pos = self.verified_tx[tx_hash]
-                conf = max(self.get_local_height() - height + 1, 0)
-                return height, conf, timestamp
+                info = self.verified_tx[tx_hash]
+                conf = max(self.get_local_height() - info.height + 1, 0)
+                return TxMinedStatus(info.height, conf, info.timestamp, info.header_hash)
             elif tx_hash in self.unverified_tx:
                 height = self.unverified_tx[tx_hash]
-                return height, 0, None
+                return TxMinedStatus(height, 0, None, None)
             else:
                 # local transaction
-                return TX_HEIGHT_LOCAL, 0, None
+                return TxMinedStatus(TX_HEIGHT_LOCAL, 0, None, None)
 
     def set_up_to_date(self, up_to_date):
         with self.lock:

electrum/gui/kivy/uix/screens.py

@@ -131,8 +131,8 @@ class HistoryScreen(CScreen):
         d = LabelDialog(_('Enter Transaction Label'), text, callback)
         d.open()
 
-    def get_card(self, tx_hash, height, conf, timestamp, value, balance):
-        status, status_str = self.app.wallet.get_tx_status(tx_hash, height, conf, timestamp)
+    def get_card(self, tx_hash, tx_mined_status, value, balance):
+        status, status_str = self.app.wallet.get_tx_status(tx_hash, tx_mined_status)
         icon = "atlas://electrum/gui/kivy/theming/light/" + TX_ICONS[status]
         label = self.app.wallet.get_label(tx_hash) if tx_hash else _('Pruned transaction outputs')
         ri = {}
@@ -141,7 +141,7 @@ class HistoryScreen(CScreen):
         ri['icon'] = icon
         ri['date'] = status_str
         ri['message'] = label
-        ri['confirmations'] = conf
+        ri['confirmations'] = tx_mined_status.conf
         if value is not None:
             ri['is_mine'] = value < 0
             if value < 0: value = - value
@@ -158,7 +158,6 @@ class HistoryScreen(CScreen):
             return
         history = reversed(self.app.wallet.get_history())
         history_card = self.screen.ids.history_container
-        count = 0
         history_card.data = [self.get_card(*item) for item in history]
 
 

electrum/gui/qt/history_list.py

@@ -29,7 +29,7 @@ import datetime
 from electrum.address_synchronizer import TX_HEIGHT_LOCAL
 from .util import *
 from electrum.i18n import _
-from electrum.util import block_explorer_URL, profiler, print_error
+from electrum.util import block_explorer_URL, profiler, print_error, TxMinedStatus
 
 try:
     from electrum.plot import plot_history, NothingToPlotException
@@ -237,7 +237,8 @@ class HistoryList(MyTreeWidget, AcceptFileDragDrop):
             value = tx_item['value'].value
             balance = tx_item['balance'].value
             label = tx_item['label']
-            status, status_str = self.wallet.get_tx_status(tx_hash, height, conf, timestamp)
+            tx_mined_status = TxMinedStatus(height, conf, timestamp, None)
+            status, status_str = self.wallet.get_tx_status(tx_hash, tx_mined_status)
             has_invoice = self.wallet.invoices.paid.get(tx_hash)
             icon = self.icon_cache.get(":icons/" + TX_ICONS[status])
             v_str = self.parent.format_amount(value, is_diff=True, whitespaces=True)
@@ -304,10 +305,11 @@ class HistoryList(MyTreeWidget, AcceptFileDragDrop):
             label = self.wallet.get_label(txid)
             item.setText(3, label)
 
-    def update_item(self, tx_hash, height, conf, timestamp):
+    def update_item(self, tx_hash, tx_mined_status):
         if self.wallet is None:
             return
-        status, status_str = self.wallet.get_tx_status(tx_hash, height, conf, timestamp)
+        conf = tx_mined_status.conf
+        status, status_str = self.wallet.get_tx_status(tx_hash, tx_mined_status)
         icon = self.icon_cache.get(":icons/" +  TX_ICONS[status])
         items = self.findItems(tx_hash, Qt.UserRole|Qt.MatchContains|Qt.MatchRecursive, column=1)
         if items:
@@ -332,7 +334,7 @@ class HistoryList(MyTreeWidget, AcceptFileDragDrop):
             column_title = self.headerItem().text(column)
             column_data = item.text(column)
         tx_URL = block_explorer_URL(self.config, 'tx', tx_hash)
-        height, conf, timestamp = self.wallet.get_tx_height(tx_hash)
+        height = self.wallet.get_tx_height(tx_hash).height
         tx = self.wallet.transactions.get(tx_hash)
         is_relevant, is_mine, v, fee = self.wallet.get_wallet_delta(tx)
         is_unconfirmed = height <= 0

electrum/gui/stdio.py

@@ -87,9 +87,9 @@ class ElectrumGui:
         + "%d"%(width[2]+delta)+"s"+"%"+"%d"%(width[3]+delta)+"s"
         messages = []
 
-        for item in self.wallet.get_history():
-            tx_hash, height, conf, timestamp, delta, balance = item
-            if conf:
+        for tx_hash, tx_mined_status, delta, balance in self.wallet.get_history():
+            if tx_mined_status.conf:
+                timestamp = tx_mined_status.timestamp
                 try:
                     time_str = datetime.datetime.fromtimestamp(timestamp).isoformat(' ')[:-3]
                 except Exception:

electrum/gui/text.py

@@ -109,9 +109,9 @@ class ElectrumGui:
 
         b = 0
         self.history = []
-        for item in self.wallet.get_history():
-            tx_hash, height, conf, timestamp, value, balance = item
-            if conf:
+        for tx_hash, tx_mined_status, value, balance in self.wallet.get_history():
+            if tx_mined_status.conf:
+                timestamp = tx_mined_status.timestamp
                 try:
                     time_str = datetime.datetime.fromtimestamp(timestamp).isoformat(' ')[:-3]
                 except Exception:

electrum/storage.py

@@ -44,7 +44,7 @@ from .keystore import bip44_derivation
 
 OLD_SEED_VERSION = 4        # electrum versions < 2.0
 NEW_SEED_VERSION = 11       # electrum versions >= 2.0
-FINAL_SEED_VERSION = 17     # electrum >= 2.7 will set this to prevent
+FINAL_SEED_VERSION = 18     # electrum >= 2.7 will set this to prevent
                             # old versions from overwriting new format
 
 
@@ -356,6 +356,7 @@ class WalletStorage(JsonDB):
         self.convert_version_15()
         self.convert_version_16()
         self.convert_version_17()
+        self.convert_version_18()
 
         self.put('seed_version', FINAL_SEED_VERSION)  # just to be sure
         self.write()
@@ -570,6 +571,15 @@ class WalletStorage(JsonDB):
 
         self.put('seed_version', 17)
 
+    def convert_version_18(self):
+        # delete verified_tx3 as its structure changed
+        if not self._is_upgrade_method_needed(17, 17):
+            return
+
+        self.put('verified_tx3', None)
+
+        self.put('seed_version', 18)
+
     def convert_imported(self):
         if not self._is_upgrade_method_needed(0, 13):
             return

electrum/util.py

@@ -23,6 +23,7 @@
 import binascii
 import os, sys, re, json
 from collections import defaultdict
+from typing import NamedTuple
 from datetime import datetime
 import decimal
 from decimal import Decimal
@@ -903,3 +904,13 @@ def make_dir(path, allow_symlink=True):
             raise Exception('Dangling link: ' + path)
         os.mkdir(path)
         os.chmod(path, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
+
+
+TxMinedStatus = NamedTuple("TxMinedStatus", [("height", int),
+                                             ("conf", int),
+                                             ("timestamp", int),
+                                             ("header_hash", str)])
+VerifiedTxInfo = NamedTuple("VerifiedTxInfo", [("height", int),
+                                               ("timestamp", int),
+                                               ("txpos", int),
+                                               ("header_hash", str)])

electrum/verifier.py

@@ -23,9 +23,10 @@
 
 from typing import Sequence, Optional
 
-from .util import ThreadJob, bh2u
+from .util import ThreadJob, bh2u, VerifiedTxInfo
 from .bitcoin import Hash, hash_decode, hash_encode
 from .transaction import Transaction
+from .blockchain import hash_header
 
 
 class MerkleVerificationFailure(Exception): pass
@@ -108,7 +109,9 @@ class SPV(ThreadJob):
             self.requested_merkle.remove(tx_hash)
         except KeyError: pass
         self.print_error("verified %s" % tx_hash)
-        self.wallet.add_verified_tx(tx_hash, (tx_height, header.get('timestamp'), pos))
+        header_hash = hash_header(header)
+        vtx_info = VerifiedTxInfo(tx_height, header.get('timestamp'), pos, header_hash)
+        self.wallet.add_verified_tx(tx_hash, vtx_info)
         if self.is_up_to_date() and self.wallet.is_up_to_date():
             self.wallet.save_verified_tx(write=True)
 

electrum/wallet.py

@@ -318,7 +318,8 @@ class Abstract_Wallet(AddressSynchronizer):
         if tx.is_complete():
             if tx_hash in self.transactions.keys():
                 label = self.get_label(tx_hash)
-                height, conf, timestamp = self.get_tx_height(tx_hash)
+                tx_mined_status = self.get_tx_height(tx_hash)
+                height, conf = tx_mined_status.height, tx_mined_status.conf
                 if height > 0:
                     if conf:
                         status = _("{} confirmations").format(conf)
@@ -368,8 +369,9 @@ class Abstract_Wallet(AddressSynchronizer):
 
     def balance_at_timestamp(self, domain, target_timestamp):
         h = self.get_history(domain)
-        for tx_hash, height, conf, timestamp, value, balance in h:
-            if timestamp > target_timestamp:
+        balance = 0
+        for tx_hash, tx_mined_status, value, balance in h:
+            if tx_mined_status.timestamp > target_timestamp:
                 return balance - value
         # return last balance
         return balance
@@ -384,16 +386,17 @@ class Abstract_Wallet(AddressSynchronizer):
         fiat_income = Decimal(0)
         fiat_expenditures = Decimal(0)
         h = self.get_history(domain)
-        for tx_hash, height, conf, timestamp, value, balance in h:
+        for tx_hash, tx_mined_status, value, balance in h:
+            timestamp = tx_mined_status.timestamp
             if from_timestamp and (timestamp or time.time()) < from_timestamp:
                 continue
             if to_timestamp and (timestamp or time.time()) >= to_timestamp:
                 continue
             item = {
-                'txid':tx_hash,
-                'height':height,
-                'confirmations':conf,
-                'timestamp':timestamp,
+                'txid': tx_hash,
+                'height': tx_mined_status.height,
+                'confirmations': tx_mined_status.conf,
+                'timestamp': timestamp,
                 'value': Satoshis(value),
                 'balance': Satoshis(balance)
             }
@@ -483,9 +486,12 @@ class Abstract_Wallet(AddressSynchronizer):
             return ', '.join(labels)
         return ''
 
-    def get_tx_status(self, tx_hash, height, conf, timestamp):
+    def get_tx_status(self, tx_hash, tx_mined_status):
         from .util import format_time
         extra = []
+        height = tx_mined_status.height
+        conf = tx_mined_status.conf
+        timestamp = tx_mined_status.timestamp
         if conf == 0:
             tx = self.transactions.get(tx_hash)
             if not tx:
@@ -839,8 +845,7 @@ class Abstract_Wallet(AddressSynchronizer):
             txid, n = txo.split(':')
             info = self.verified_tx.get(txid)
             if info:
-                tx_height, timestamp, pos = info
-                conf = local_height - tx_height
+                conf = local_height - info.height
             else:
                 conf = 0
             l.append((conf, v))
@@ -1091,7 +1096,7 @@ class Abstract_Wallet(AddressSynchronizer):
 
     def price_at_timestamp(self, txid, price_func):
         """Returns fiat price of bitcoin at the time tx got confirmed."""
-        height, conf, timestamp = self.get_tx_height(txid)
+        timestamp = self.get_tx_height(txid).timestamp
         return price_func(timestamp if timestamp else time.time())
 
     def unrealized_gains(self, domain, price_func, ccy):
@@ -1258,7 +1263,7 @@ class Imported_Wallet(Simple_Wallet):
                 self.verified_tx.pop(tx_hash, None)
                 self.unverified_tx.pop(tx_hash, None)
                 self.transactions.pop(tx_hash, None)
-            self.storage.put('verified_tx3', self.verified_tx)
+            self.save_verified_tx()
         self.save_transactions()
 
         self.set_label(address, None)