verifier: fix logic bug. after reorg, some verifs were not undone

after a reorg, in a many fork/orphan chains scenario,
we would sometimes not undo SPV for enough blocks

functions in blockchain.py somewhat based on kyuupichan/bitcoinX@5126bd15ef0c9ba36e17a455513452ebed7b2328

electrum/address_synchronizer.py

@@ -525,14 +525,14 @@ class AddressSynchronizer(PrintError):
         with self.lock:
             return dict(self.unverified_tx)  # copy
 
-    def undo_verifications(self, blockchain, height):
+    def undo_verifications(self, blockchain, above_height):
         '''Used by the verifier when a reorg has happened'''
         txs = set()
         with self.lock:
             for tx_hash in self.db.list_verified_tx():
                 info = self.db.get_verified_tx(tx_hash)
                 tx_height = info.height
-                if tx_height >= height:
+                if tx_height > above_height:
                     header = blockchain.read_header(tx_height)
                     if not header or hash_header(header) != info.header_hash:
                         self.db.remove_verified_tx(tx_hash)

electrum/blockchain.py

@@ -201,6 +201,27 @@ class Blockchain(util.PrintError):
         with blockchains_lock:
             return list(filter(lambda y: y.parent==self, blockchains.values()))
 
+    def get_parent_heights(self) -> Mapping['Blockchain', int]:
+        """Returns map: (parent chain -> height of last common block)"""
+        with blockchains_lock:
+            result = {self: self.height()}
+            chain = self
+            while True:
+                parent = chain.parent
+                if parent is None: break
+                result[parent] = chain.forkpoint - 1
+                chain = parent
+            return result
+
+    def get_height_of_last_common_block_with_chain(self, other_chain: 'Blockchain') -> int:
+        last_common_block_height = 0
+        our_parents = self.get_parent_heights()
+        their_parents = other_chain.get_parent_heights()
+        for chain in our_parents:
+            if chain in their_parents:
+                h = min(our_parents[chain], their_parents[chain])
+                last_common_block_height = max(last_common_block_height, h)
+        return last_common_block_height
 
     @with_lock
     def get_branch_size(self) -> int:

electrum/tests/test_blockchain.py

@@ -70,6 +70,62 @@ class TestBlockchain(SequentialTestCase):
         self.assertTrue(chain.can_connect(header))
         chain.save_header(header)
 
+    def test_get_height_of_last_common_block_with_chain(self):
+        blockchain.blockchains[constants.net.GENESIS] = chain_u = Blockchain(
+            config=self.config, forkpoint=0, parent=None,
+            forkpoint_hash=constants.net.GENESIS, prev_hash=None)
+        open(chain_u.path(), 'w+').close()
+        self._append_header(chain_u, self.HEADERS['A'])
+        self._append_header(chain_u, self.HEADERS['B'])
+        self._append_header(chain_u, self.HEADERS['C'])
+        self._append_header(chain_u, self.HEADERS['D'])
+        self._append_header(chain_u, self.HEADERS['E'])
+        self._append_header(chain_u, self.HEADERS['F'])
+        self._append_header(chain_u, self.HEADERS['O'])
+        self._append_header(chain_u, self.HEADERS['P'])
+        self._append_header(chain_u, self.HEADERS['Q'])
+
+        chain_l = chain_u.fork(self.HEADERS['G'])
+        self._append_header(chain_l, self.HEADERS['H'])
+        self._append_header(chain_l, self.HEADERS['I'])
+        self._append_header(chain_l, self.HEADERS['J'])
+        self._append_header(chain_l, self.HEADERS['K'])
+        self._append_header(chain_l, self.HEADERS['L'])
+
+        self.assertEqual({chain_u:  8, chain_l: 5}, chain_u.get_parent_heights())
+        self.assertEqual({chain_l: 11},             chain_l.get_parent_heights())
+
+        chain_z = chain_l.fork(self.HEADERS['M'])
+        self._append_header(chain_z, self.HEADERS['N'])
+        self._append_header(chain_z, self.HEADERS['X'])
+        self._append_header(chain_z, self.HEADERS['Y'])
+        self._append_header(chain_z, self.HEADERS['Z'])
+
+        self.assertEqual({chain_u:  8, chain_z: 5}, chain_u.get_parent_heights())
+        self.assertEqual({chain_l: 11, chain_z: 8}, chain_l.get_parent_heights())
+        self.assertEqual({chain_z: 13},             chain_z.get_parent_heights())
+        self.assertEqual(5, chain_u.get_height_of_last_common_block_with_chain(chain_l))
+        self.assertEqual(5, chain_l.get_height_of_last_common_block_with_chain(chain_u))
+        self.assertEqual(5, chain_u.get_height_of_last_common_block_with_chain(chain_z))
+        self.assertEqual(5, chain_z.get_height_of_last_common_block_with_chain(chain_u))
+        self.assertEqual(8, chain_l.get_height_of_last_common_block_with_chain(chain_z))
+        self.assertEqual(8, chain_z.get_height_of_last_common_block_with_chain(chain_l))
+
+        self._append_header(chain_u, self.HEADERS['R'])
+        self._append_header(chain_u, self.HEADERS['S'])
+        self._append_header(chain_u, self.HEADERS['T'])
+        self._append_header(chain_u, self.HEADERS['U'])
+
+        self.assertEqual({chain_u: 12, chain_z: 5}, chain_u.get_parent_heights())
+        self.assertEqual({chain_l: 11, chain_z: 8}, chain_l.get_parent_heights())
+        self.assertEqual({chain_z: 13},             chain_z.get_parent_heights())
+        self.assertEqual(5, chain_u.get_height_of_last_common_block_with_chain(chain_l))
+        self.assertEqual(5, chain_l.get_height_of_last_common_block_with_chain(chain_u))
+        self.assertEqual(5, chain_u.get_height_of_last_common_block_with_chain(chain_z))
+        self.assertEqual(5, chain_z.get_height_of_last_common_block_with_chain(chain_u))
+        self.assertEqual(8, chain_l.get_height_of_last_common_block_with_chain(chain_z))
+        self.assertEqual(8, chain_z.get_height_of_last_common_block_with_chain(chain_l))
+
     def test_parents_after_forking(self):
         blockchain.blockchains[constants.net.GENESIS] = chain_u = Blockchain(
             config=self.config, forkpoint=0, parent=None,

electrum/verifier.py

@@ -168,18 +168,17 @@ class SPV(NetworkJobOnDefaultServer):
             raise InnerNodeOfSpvProofIsValidTx()
 
     async def _maybe_undo_verifications(self):
-        def undo_verifications():
+        old_chain = self.blockchain
-            height = self.blockchain.get_max_forkpoint()
+        cur_chain = self.network.blockchain()
-            self.print_error("undoing verifications back to height {}".format(height))
+        if cur_chain != old_chain:
-            tx_hashes = self.wallet.undo_verifications(self.blockchain, height)
+            self.blockchain = cur_chain
+            above_height = cur_chain.get_height_of_last_common_block_with_chain(old_chain)
+            self.print_error(f"undoing verifications above height {above_height}")
+            tx_hashes = self.wallet.undo_verifications(self.blockchain, above_height)
             for tx_hash in tx_hashes:
                 self.print_error("redoing", tx_hash)
                 self.remove_spv_proof_for_tx(tx_hash)
 
-        if self.network.blockchain() != self.blockchain:
-            self.blockchain = self.network.blockchain()
-            undo_verifications()
-
     def remove_spv_proof_for_tx(self, tx_hash):
         self.merkle_roots.pop(tx_hash, None)
         try: