storage: fix some madness about get_data_ref() and put() interacting badly

previously load_transactions() had to be called before upgrade();
now we reverse this order.

to reproduce/illustrate issue, before this commit:

try running convert_version_17 and convert_version_18
(e.g. see testcase test_upgrade_from_client_2_9_3_old_seeded_with_realistic_history)
and then in qt console:
>> wallet.storage.db.get_data_ref('spent_outpoints') == wallet.storage.db.spent_outpoints
False
>> wallet.storage.db.get_data_ref('verified_tx3') == wallet.storage.db.verified_tx
False

electrum/address_synchronizer.py

@@ -61,6 +61,9 @@ class AddressSynchronizer(Logger):
     """
 
     def __init__(self, storage: 'WalletStorage'):
+        if not storage.is_ready_to_be_used_by_wallet():
+            raise Exception("storage not ready to be used by AddressSynchronizer")
+
         self.storage = storage
         self.db = self.storage.db
         self.network = None  # type: Network

electrum/json_db.py

@@ -59,12 +59,12 @@ class JsonDB(Logger):
         self.data = {}
         self._modified = False
         self.manual_upgrades = manual_upgrades
-        self._called_load_transactions = False
+        self._called_after_upgrade_tasks = False
         if raw:  # loading existing db
             self.load_data(raw)
         else:  # creating new db
             self.put('seed_version', FINAL_SEED_VERSION)
-            self.load_transactions()
+            self._after_upgrade_tasks()
 
     def set_modified(self, b):
         with self.lock:
@@ -108,12 +108,6 @@ class JsonDB(Logger):
                 self.data[key] = copy.deepcopy(value)
                 return True
         elif key in self.data:
-            # clear current contents in case of references
-            cur_val = self.data[key]
-            clear_method = getattr(cur_val, "clear", None)
-            if callable(clear_method):
-                clear_method()
-            # pop from dict to delete key
             self.data.pop(key)
             return True
         return False
@@ -149,9 +143,9 @@ class JsonDB(Logger):
         if not self.manual_upgrades and self.requires_split():
             raise WalletFileException("This wallet has multiple accounts and must be split")
 
-        self.load_transactions()
+        if not self.requires_upgrade():
-
+            self._after_upgrade_tasks()
-        if not self.manual_upgrades and self.requires_upgrade():
+        elif not self.manual_upgrades:
             self.upgrade()
 
     def requires_split(self):
@@ -204,11 +198,9 @@ class JsonDB(Logger):
     @profiler
     def upgrade(self):
         self.logger.info('upgrading wallet format')
-        if not self._called_load_transactions:
+        if self._called_after_upgrade_tasks:
-            # note: not sure if this is how we should go about this...
+            # we need strict ordering between upgrade() and after_upgrade_tasks()
-            # alternatively, we could make sure load_transactions is always called after upgrade
+            raise Exception("'after_upgrade_tasks' must NOT be called before 'upgrade'")
-            # still, we need strict ordering between the two.
-            raise Exception("'load_transactions' must be called before 'upgrade'")
         self._convert_imported()
         self._convert_wallet_type()
         self._convert_account()
@@ -220,6 +212,12 @@ class JsonDB(Logger):
         self._convert_version_18()
         self.put('seed_version', FINAL_SEED_VERSION)  # just to be sure
 
+        self._after_upgrade_tasks()
+
+    def _after_upgrade_tasks(self):
+        self._called_after_upgrade_tasks = True
+        self._load_transactions()
+
     def _convert_wallet_type(self):
         if not self._is_upgrade_method_needed(0, 13):
             return
@@ -415,9 +413,10 @@ class JsonDB(Logger):
 
         self.put('pruned_txo', None)
 
-        transactions = self.get('transactions', {})  # txid -> Transaction
+        transactions = self.get('transactions', {})  # txid -> raw_tx
         spent_outpoints = defaultdict(dict)
-        for txid, tx in transactions.items():
+        for txid, raw_tx in transactions.items():
+            tx = Transaction(raw_tx)
             for txin in tx.inputs():
                 if txin['type'] == 'coinbase':
                     continue
@@ -475,6 +474,7 @@ class JsonDB(Logger):
         self.put('accounts', None)
 
     def _is_upgrade_method_needed(self, min_version, max_version):
+        assert min_version <= max_version
         cur_version = self.get_seed_version()
         if cur_version > max_version:
             return False
@@ -673,6 +673,8 @@ class JsonDB(Logger):
 
     @locked
     def get_data_ref(self, name):
+        # Warning: interacts un-intuitively with 'put': certain parts
+        # of 'data' will have pointers saved as separate variables.
         if name not in self.data:
             self.data[name] = {}
         return self.data[name]
@@ -745,8 +747,7 @@ class JsonDB(Logger):
                 self._addr_to_addr_index[addr] = (True, i)
 
     @profiler
-    def load_transactions(self):
+    def _load_transactions(self):
-        self._called_load_transactions = True
         # references in self.data
         self.txi = self.get_data_ref('txi')  # txid -> address -> list of (prev_outpoint, value)
         self.txo = self.get_data_ref('txo')  # txid -> address -> list of (output_index, value, is_coinbase)

electrum/storage.py

@@ -226,6 +226,9 @@ class WalletStorage(Logger):
             raise Exception("storage not yet decrypted!")
         return self.db.requires_upgrade()
 
+    def is_ready_to_be_used_by_wallet(self):
+        return not self.requires_upgrade() and self.db._called_after_upgrade_tasks
+
     def upgrade(self):
         self.db.upgrade()
         self.write()
@@ -240,6 +243,7 @@ class WalletStorage(Logger):
             path = self.path + '.' + data['suffix']
             storage = WalletStorage(path)
             storage.db.data = data
+            storage.db._called_after_upgrade_tasks = False
             storage.db.upgrade()
             storage.write()
             out.append(path)

electrum/tests/test_storage_upgrade.py

@@ -305,7 +305,13 @@ class TestStorageUpgrade(WalletTestCase):
             storage2 = self._load_storage_from_json_string(wallet_json=wallet_json,
                                                            path=path2,
                                                            manual_upgrades=False)
+            storage2.write()
             self._sanity_check_upgraded_storage(storage2)
+            # test opening upgraded storages again
+            s1 = WalletStorage(path2, manual_upgrades=False)
+            self._sanity_check_upgraded_storage(s1)
+            s2 = WalletStorage(path2, manual_upgrades=True)
+            self._sanity_check_upgraded_storage(s2)
         else:
             storage = self._load_storage_from_json_string(wallet_json=wallet_json,
                                                           path=self.wallet_path,

electrum/wallet.py

@@ -206,8 +206,8 @@ class Abstract_Wallet(AddressSynchronizer):
     gap_limit_for_change = 6
 
     def __init__(self, storage: WalletStorage):
-        if storage.requires_upgrade():
+        if not storage.is_ready_to_be_used_by_wallet():
-            raise Exception("storage must be upgraded before constructing wallet")
+            raise Exception("storage not ready to be used by Abstract_Wallet")
 
         # load addresses needs to be called before constructor for sanity checks
         storage.db.load_addresses(self.wallet_type)