New storage api

[ecdsa]

I am marking this as ready because it is advanced enough that it could use some review.

My hope was to make the first commit pass all the tests, but that seems to be impossible. Commit 4 (types conversions) restores functionality. Note that commit 2 (perf optimization) and 3 (rename) are trivial, and they could potentially be reordered after commit 4.

There are 3 remaining design issues:

1. should StoredDict.get() return dict/list instead of StoredList/StoredDict ?

   • pro: get() would be consistent with pop()
   • con: get() would be inconsistent with __getitem__ , which must return StoredList/StoredDict
   • pro: in order to access/create StoredDict/StoredList, we would move get_dict, get_list from wallet_db to stored_dict. This would make the returned types more explicit.
   • pro: this would simplify DB upgrades, and code in general

2. should we remove the init_db parameter in the constructor, and use an explicit DictStorage.open() call instead?

   • probably cleaner, there is already a close() method
   • open would take the password as parameter, and behave as decrypt(). Howver, it would need to be called even for non-encrypted wallets.

3. currently if an exception is raised during a batch write, memory and disk become inconsistent (see FIXME)

   • ideally, we should get rid of save_db, and always write to disk if we are not in a batch.
   • this would be easy if we had partial writes always enabled (not implemented yet with encrypted wallets)

EDIT: point 3 is now fixed in #10339

[f321x]

Is this intended to cast x as str instead? Otherwise the annotated return type is incorrect.

Suggested change

	return int(x)
	return str(x)

[f321x]

The iterator returned by the keys()/values()/items() methods is exhausted after one iteration, unlike the dictionary view objects returned by a regular dict.

This seems like a footgun as it behaves unexpected in situations like this:

diff --git a/tests/test_stored_dict.py b/tests/test_stored_dict.py
index 26e96fc63..cf8f92701 100644
--- a/tests/test_stored_dict.py
+++ b/tests/test_stored_dict.py
@@ -98,3 +98,14 @@ class TestStorage(ElectrumTestCase):
         storage = DictStorage(self.path)
         self.assertEqual(storage.dump(), {'a':{}})
 
+    async def test_show_iterator_type(self):
+        storage = DictStorage(self.path)
+        storage['a'] = {'k1': 1, 'k2': 2, 'k3': 3}
+        a = storage.get('a')
+        # len(a.keys())  # TypeError: object of type 'generator' has no len()
+        keys = a.keys()
+        b = list(keys)
+        c = list(keys)
+        self.assertEqual(len(b), len(c))
+        storage['b'] = {}
+        self.assertFalse(storage.get('b').keys())  # iterator is always True

In fact, _convert_version_14() already surfaces this issue:

                pubkeys = self.get('keystore').get('keypairs').keys()
                assert len(addresses) == len(list(pubkeys))  # drains pubkeys
                d = {}
                for pubkey in pubkeys:  # pubkeys is empty here
                    addr = bitcoin.pubkey_to_address('p2pkh', pubkey)

A simple and nice fix seems to be making StoredDict inherit from collections.abc.MutableMapping:

See patch (click to expand)

diff --git a/electrum/stored_dict.py b/electrum/stored_dict.py
index 1917ca478..41df28a2f 100644
--- a/electrum/stored_dict.py
+++ b/electrum/stored_dict.py
@@ -27,7 +27,8 @@ import threading
 import os
 from enum import IntEnum
 from collections import defaultdict
-from typing import Any, Optional, Tuple, Union, Iterator, Iterable, List, Sequence
+from collections.abc import MutableMapping
+from typing import Any, Optional, Union, Iterator, List, Sequence
 from .logging import Logger
 
 
@@ -272,7 +273,7 @@ class StoredObject(BaseStoredObject):
         return d
 
 
-class StoredDict(BaseStoredObject):
+class StoredDict(BaseStoredObject, MutableMapping):
     """
     dict-like object that queries the DB
     type conversions are performed here
@@ -327,8 +328,9 @@ class StoredDict(BaseStoredObject):
         key = key_to_str(key)
         self._db.remove(self.hint, self._path, key)
 
-    def __iter__(self) -> Iterator[str]:
-        return self._db.iter_keys(self.hint, self._path)
+    def __iter__(self) -> Iterator[_FLEX_KEY]:
+        for k in self._db.iter_keys(self.hint, self._path):
+            yield self._convert_key(k)
 
     def __len__(self) -> int:
         return self._db.dict_len(self.hint, self._path)
@@ -339,18 +341,6 @@ class StoredDict(BaseStoredObject):
         key = key_to_str(key)
         return self._db.dict_contains(self.hint, self._path, key)
 
-    def keys(self) -> Iterable[str]:
-        for k in self._db.iter_keys(self.hint, self._path):
-            yield self._convert_key(k)
-
-    def values(self) -> Iterator[Any]:
-        for k in self._db.iter_keys(self.hint, self._path):
-            yield self[k]
-
-    def items(self) -> Iterator[Tuple[str, Any]]:
-        for k in self._db.iter_keys(self.hint, self._path):
-            yield (self._convert_key(k), self[k])
-
     def get(self, key: _FLEX_KEY, default: Any = None, add_if_missing=False) -> Any:
         # If add_if_missing is True, create DB entry if it does not exist.
         # This will return StoredDict/StoredList if default is dict/list
@@ -378,15 +368,6 @@ class StoredDict(BaseStoredObject):
         del self[key]
         return v
 
-    def update(self, other=(), /, **kwargs) -> None:
-        if isinstance(other, dict):
-            pairs = list(other.items())
-        else:
-            pairs = list(other)
-        pairs.extend(kwargs.items())
-        for k, v in pairs:
-            self[k] = v
-
     def as_dict(self) -> dict:
         """used by keystore"""
         return self.dump()

And StoredList could similarly inherit from collections.abc.MutableSequence.

[f321x]

On master self.write_and_force_consolidation() was only called after the wallet upgrade, so if the upgrade failed the patches weren't applied. Now it is called before applying the upgrade.
This causes test_mainnet_testnet_mixup to apply the patches in /tests/test_storage_upgrade/client_4_5_2_9dk_with_ln on each test run which modifies the test wallet and shows up in the git diff.
Can the db write be delayed until the wallet upgrade succeeds?

[f321x]

This only raises if self.hint is not yet populated. When e.g. accessing (__getitem__) StoredDict first and the hint property sets self.hint, subsequent __setitem__ don't raise.
This fails e.g.:

diff --git a/tests/test_stored_dict.py b/tests/test_stored_dict.py
index 26e96fc63..0e25480ad 100644
--- a/tests/test_stored_dict.py
+++ b/tests/test_stored_dict.py
@@ -88,6 +88,7 @@ class TestStorage(ElectrumTestCase):
         a = storage.get('a')
         b = a['b']
         self.assertEqual(type(b), StoredDict)
+        print(b['c'])  # reads from b, populates cache
         b2 = a.pop('b')
         self.assertEqual(type(b2), dict)
         # replace item. this must not been written to db

[f321x]

Should this be moved one intendation layer out?
Otherwise the new type mpp htlcs will only be saved if a forwarding_key is set.