URI: 
       tMerge pull request #4538 from SomberNight/verifier_reorgs_st18 - electrum - Electrum Bitcoin wallet
  HTML git clone https://git.parazyd.org/electrum
   DIR Log
   DIR Files
   DIR Refs
   DIR Submodules
       ---
   DIR commit 32f5feff8f9c3eaabdffd77825c4a31ab79e71a9
   DIR parent 861640949e479689aeb9d30121ba1f9ddb40ae71
  HTML Author: ghost43 <somber.night@protonmail.com>
       Date:   Tue, 31 Jul 2018 19:37:46 +0200
       
       Merge pull request #4538 from SomberNight/verifier_reorgs_st18
       
       verifier: better handle reorgs (and storage upgrade)
       Diffstat:
         M electrum/address_synchronizer.py    |      92 ++++++++++++++++++-------------
         M electrum/gui/kivy/uix/screens.py    |       7 +++----
         M electrum/gui/qt/history_list.py     |      12 +++++++-----
         M electrum/gui/stdio.py               |       6 +++---
         M electrum/gui/text.py                |       6 +++---
         M electrum/storage.py                 |      12 +++++++++++-
         M electrum/util.py                    |      11 +++++++++++
         M electrum/verifier.py                |       7 +++++--
         M electrum/wallet.py                  |      31 ++++++++++++++++++-------------
       
       9 files changed, 115 insertions(+), 69 deletions(-)
       ---
   DIR diff --git a/electrum/address_synchronizer.py b/electrum/address_synchronizer.py
       t@@ -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
       t@@ -45,6 +46,7 @@ class UnrelatedTransactionException(AddTransactionException):
            def __str__(self):
                return _("Transaction is unrelated to this wallet.")
        
       +
        class AddressSynchronizer(PrintError):
            """
            inherited by wallet
       t@@ -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
       t@@ -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
        
       t@@ -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.
       t@@ -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.
       t@@ -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)
        
       t@@ -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)
       t@@ -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:
       t@@ -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'''
       t@@ -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
        
       t@@ -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:
   DIR diff --git a/electrum/gui/kivy/uix/screens.py b/electrum/gui/kivy/uix/screens.py
       t@@ -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 = {}
       t@@ -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
       t@@ -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]
        
        
   DIR diff --git a/electrum/gui/qt/history_list.py b/electrum/gui/qt/history_list.py
       t@@ -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
       t@@ -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)
       t@@ -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:
       t@@ -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
   DIR diff --git a/electrum/gui/stdio.py b/electrum/gui/stdio.py
       t@@ -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:
   DIR diff --git a/electrum/gui/text.py b/electrum/gui/text.py
       t@@ -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:
   DIR diff --git a/electrum/storage.py b/electrum/storage.py
       t@@ -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
        
        
       t@@ -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()
       t@@ -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
   DIR diff --git a/electrum/util.py b/electrum/util.py
       t@@ -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
       t@@ -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)])
   DIR diff --git a/electrum/verifier.py b/electrum/verifier.py
       t@@ -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
       t@@ -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)
        
   DIR diff --git a/electrum/wallet.py b/electrum/wallet.py
       t@@ -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)
       t@@ -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
       t@@ -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)
                    }
       t@@ -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:
       t@@ -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))
       t@@ -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):
       t@@ -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)