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.

[f321x]

This crashes in to_default() as r['buckets'] is a StoredList.

Suggested change

	r = c['revocation_store'].dump()

Could to_default() handle StoredDict/StoredList by calling dump() on them so this would be prevented?

[f321x]

_maybe_active_htlc_ids contains Set[int] but StoredDict.__iter__() returns Iterator[str] because it doesn't call _convert_key() on the yielded keys.
So this might mixes up int and str htlc ids in the same set.
This is also fixed by the patch in comment #10693 (comment)

[f321x]

Is this comment still up-to-date, shouldn't they should be converted to int on exposure as the keys are registered in wallet_db.py?

[f321x]

The wallet file will already get created when instantiating db = WalletDB(storage) above due to the upgrader setting the initial seed version etc and write_batch() then writing the file.
If wallet recovery then fails later on here the user ends up with an unusable wallet file.
Would it be safer to just not create a new file/call self.write() in JsonDB.write_batch() if the path doesn't exist yet and rely on the explicit call to wallet.save_db() below to create the file?

[f321x]

Should StoredDict|StoredList.dump() take self.lock to prevent self from changing during the iteration? Otherwise things like Abstract_Wallet.save_backup() might end up with a weird state if something from the asyncio thread modifies the dict/list while the GUI dumps it.

Suggested change

	for k, v in self.items():
	with self.lock:
	for k, v in self.items():

[f321x]

As this is not done under a lock i guess it could raise even if a default is given if multiple threads access/modify the dict simultaneously.

[f321x]

Now that objects are re-instantiated on every access this probably won't work anymore as ._broadcasting_status is not persisted and then lost on the next db access when a fresh Invoice is instantiated?

[f321x]

Removing the json_db import prevents the monkeypatch from redacting the secrets, causing the test to fail if test_jsondb.py is tested on its own (otherwise jsondb gets imported in test_commands.py and the monkeypatch applies).

====================================================================== short test summary info ======================================================================
SUBFAILED[replace_dict_inner_key_missing] tests/test_jsondb.py::TestJsonpatch::test_patch_does_not_leak_privatekeys - AssertionError: 'dictlevel' unexpectedly found in "can't replace a non-existent object 'dictlevelX2'"
...

[f321x]

Could BaseDB be made a proper abstract base class defining all the methods that it is supposed to implement (get/put/replace/remove/clear/iter_keys/dict_len/dict_contains/get_hint/list_append/etc...)?

This would add some boilerplate code but would also be nice to understand what BaseDB is actually supposed to support when implementing support for a new DB and would look better with a type checking IDE.

[f321x]

When storing a tuple in a StoredDict the to_default() call doesn't convert it to a json array but keeps it as tuple.
When the value is still a list after loading the data from file this comparison
will be False as [1, 2] != (1, 2) and a useless replace patch is appended.
Could to_default() simply return a list for passed tuples?

[f321x]

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

I think this sounds good as StoredDict.get() already is a bit ambiguous with the add_if_missing parameter. However it might be worth to change the name to something else, e.g. get_copy(), to prevent confusion and make it explicit that the returned object is not persisted.

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

It seems cleaner, I think it would make it easier to reason about what happens when and might allow for better error handling. E.g. when reading the file fails, the caller could still check db.file_exists() as there is a instantiated JsonDB object, if the constructor raises there is no object at all. Might also be better for testability.
Also right now there are 3 places where _is_closed gets set False, with an open() method that could be consolidated in a single place and the API would be simpler as open()/decrypt() could be a single thing.

[f321x]

This could be simplified by using the builtin .indices() method of the slices type instead of manually calculating start, stop and step. Also the builtin list __getitem__ raises IndexError for out of bound access.

Suggested change

	def __getitem__(self, s: slice) -> Any:
	n = self._db.list_len(self.hint, self._path)
	if type(s) is int:
	s = n + s if s < 0 else s
	return self._get_list_item(s)
	elif type(s) is slice:
	start = 0 if s.start is None else s.start if s.start >= 0 else n + s.start
	stop = n if s.stop is None else s.stop if s.stop >= 0 else n + s.stop
	step = 1 if s.step is None else s.step
	return [self._get_list_item(i) for i in range(start, stop, step)]
	else:
	raise Exception()
	def __getitem__(self, s: int | slice) -> Any:
	n = self._db.list_len(self.hint, self._path)
	if isinstance(s, int):
	if s < 0:
	s += n
	if not 0 <= s < n:
	raise IndexError(f'list index out of range: {s}')
	return self._get_list_item(s)
	elif isinstance(s, slice):
	return [self._get_list_item(i) for i in range(*s.indices(n))]
	raise TypeError(f'indices must be integers or slices, not {type(s).__name__}')