New storage api by ecdsa · Pull Request #10693 · spesmilo/electrum

ecdsa · 2026-06-07T13:57:14Z

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:

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
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.
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

- WalletDB no longer inherits from JsonDB, it uses a StoredDict - JsonDB inherits from BaseDB - FileStorage is only seen by JsonDB - calling JSonDB constructor creates the storage note: this commit temporarily breaks DB upgrades

this restores DB upgrades. (DB upgrades require not converting stored classes)

Add unit test of atomicity. Use a write batch for DB upgrades.

f321x · 2026-07-20T08:22:20Z

-    return wrapper
+def normalize_key(x: _FLEX_KEY) -> str:
+    if isinstance(x, int):
+        return int(x)


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 · 2026-07-20T14:22:58Z

+        key = key_to_str(key)
+        return self._db.dict_contains(self.hint, self._path, key)
+
+    def keys(self) -> Iterable[str]:


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 · 2026-07-20T14:53:44Z

        # write file in case there was a db upgrade
-        if self.storage and self.storage.file_exists():
-            self.write_and_force_consolidation()
+        self.write_and_force_consolidation()


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 · 2026-07-20T15:40:33Z

+        b2 = a.pop('b')
+        self.assertEqual(type(b2), dict)
+        # replace item. this must not been written to db
+        with self.assertRaises(KeyError):


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 · 2026-07-20T16:03:49Z

                    # as the htlcs won't get failed due to the new SETTLING state
                    # unless a forwarding error is set.
                    recv_mpp_status[0] = 4  # RecvMPPResolution.SETTLING
+                    mpp_sets[payment_key] = recv_mpp_status


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 · 2026-07-21T07:51:08Z

        channels = self.get('channels', [])
        for c in channels:
            # convert revocation store to dict
            r = c['revocation_store']


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 · 2026-07-21T08:46:36Z

    @with_lock
    def _init_maybe_active_htlc_ids(self):
        # first idx is "side who offered htlc":
        self._maybe_active_htlc_ids = {LOCAL: set(), REMOTE: set()}  # type: Dict[HTLCOwner, Set[int]]


_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 · 2026-07-21T08:51:36Z

    def __init__(self, log: 'StoredDict', *, initiator=None, initial_feerate=None, lock=None):

        if len(log) == 0:
            # note: "htlc_id" keys in dict are str! but due to json_db magic they can *almost* be treated as int...


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 · 2026-07-21T11:23:19Z

            db.put('gap_limit_for_change', gap_limit_for_change)
        wallet = wallet_factory(db, config=config)
-    if db.storage:
-        assert not db.storage.file_exists(), "file was created too soon! plaintext keys might have been written to disk"


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 · 2026-07-21T11:38:40Z

+
+    def dump(self) -> dict:
+        data = {}
+        for k, v in self.items():


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 · 2026-07-21T12:15:40Z

+            return default
+        if isinstance(v, (StoredList, StoredDict)):
+            v = v.dump()
+        del self[key]


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.

f321x · 2026-07-21T12:34:14Z

            invoice = self._invoices.get(key)
            if not invoice:
                continue
            invoice._broadcasting_status = broadcasting_status


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 access?

ecdsa force-pushed the new_storage_api branch 9 times, most recently from ea2886b to 2f25568 Compare June 15, 2026 09:17

ecdsa force-pushed the new_storage_api branch 9 times, most recently from c3f6c61 to 44b951d Compare June 22, 2026 15:36

ecdsa force-pushed the new_storage_api branch 12 times, most recently from 106685f to 9938ba5 Compare June 24, 2026 09:26

ecdsa force-pushed the new_storage_api branch 5 times, most recently from 88e66c9 to ff565a0 Compare June 24, 2026 17:07

ecdsa marked this pull request as ready for review June 24, 2026 17:47

ecdsa force-pushed the new_storage_api branch 3 times, most recently from 452ccba to c40ed7c Compare July 3, 2026 15:17

ecdsa added 5 commits July 14, 2026 12:42

make StoredDict database agnostic

7e38e9c

- WalletDB no longer inherits from JsonDB, it uses a StoredDict - JsonDB inherits from BaseDB - FileStorage is only seen by JsonDB - calling JSonDB constructor creates the storage note: this commit temporarily breaks DB upgrades

performance optimization: cache hint

fe0b963

rename WalletStorage -> DictStorage

f22b226

stored_dict: perform type conversions at request time

de128f9

this restores DB upgrades. (DB upgrades require not converting stored classes)

stored_dict: add support for write batches

93d18a1

Add unit test of atomicity. Use a write batch for DB upgrades.

ecdsa force-pushed the new_storage_api branch from c40ed7c to 93d18a1 Compare July 14, 2026 10:42

f321x reviewed Jul 20, 2026

View reviewed changes

f321x reviewed Jul 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New storage api#10693

New storage api#10693
ecdsa wants to merge 5 commits into
masterfrom
new_storage_api

ecdsa commented Jun 7, 2026 •

edited

Loading

Uh oh!

f321x Jul 20, 2026 •

edited

Loading

Uh oh!

f321x Jul 20, 2026

Uh oh!

f321x Jul 20, 2026

Uh oh!

f321x Jul 20, 2026 •

edited

Loading

Uh oh!

f321x Jul 20, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

f321x Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Uh oh!

Conversation

ecdsa commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

f321x Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f321x Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ecdsa commented Jun 7, 2026 •

edited

Loading

f321x Jul 20, 2026 •

edited

Loading

f321x Jul 20, 2026 •

edited

Loading