What the developer wrote
Authored by SomberNight
keystore: fix memory leak for LRU cache
Using `@functools.lru_cache` on an instance method behaves in interesting ways.
The cache kept a ref around for `self`, so in effect we were never GC-ing keystore objects. Effectively there was a single global cache for derive_pubkey, with keys `(keystore, for_change, n)`.
This PR now changes the caching to be per-keystore:
each ks has a cache, keyed `(for_change, n)`.
GC-ing individual keystores should now be possible, which should result in cleaning up just their own cache.
This also enables the corresponding previously silence flake8-bugbear check for `@functools.lru_cache`. (note that the check can selectively be disabled by adding a comment on the relevant line: `# noqa: B019`)
What changed, and why it matters
This commit fixes a memory leak in Electrum's keystore code. Previously, a caching decorator on key-derivation methods kept a permanent reference to each keystore object, preventing them from being cleaned up by normal garbage collection. The change moves the cache to live inside each keystore object, so when a keystore is no longer needed, its cache is discarded too. This is a reliability/performance bug fix rather than an active security vulnerability, though unbounded memory growth can degrade or destabilize long-running processes.
Treat as a routine bug fix / hardening patch. No immediate security response is indicated, but users running long-lived Electrum processes may benefit from the reduced memory growth. Reviewers should verify the new `LRUCache` has appropriate size limits and thread-safety for Electrum's usage.
Security signals we found
Memory leak / unbounded cache growth
Strong reference retention via functools.lru_cache on instance method
Potential denial-of-service via memory exhaustion in long-running wallet operations
No evidence of confidentiality or integrity compromise
Evidence from the diff
The patch removes @functools.lru_cache(maxsize=None) from derive_pubkey instance methods in MasterPublicKeyMixin subclasses (Xpub, Old_KeyStore). functools.lru_cache on an instance method is attached to the unbound function object and caches (self, *args) tuples, which holds a strong reference to every self ever seen, preventing keystore GC and creating a single effectively-global cache. The fix introduces a per-keystore LRUCache in MasterPublicKeyMixin.__init__, renames the abstract method to _derive_pubkey, and makes derive_pubkey consult the instance cache. It also re-enables the flake8-bugbear B019 lint rule that flags this pattern.
Changed components
electrum/keystore.pyMasterPublicKeyMixinXpubOld_KeyStorederive_pubkey / _derive_pubkeyInspect captured patch +18 / −8
diff --git a/.cirrus.yml b/.cirrus.yml
index e7c68d6..fdc6cf4 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -225,7 +225,7 @@ task:
# - https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes
# - https://github.com/PyCQA/flake8-bugbear/tree/8c0e7eb04217494d48d0ab093bf5b31db0921989#list-of-warnings
ELECTRUM_LINTERS: E9,E101,E129,E273,E274,E703,E71,E722,F5,F6,F7,F8,W191,W29,B,B909
- ELECTRUM_LINTERS_IGNORE: B007,B009,B010,B019,B036,B042,F541,F841
+ ELECTRUM_LINTERS_IGNORE: B007,B009,B010,B036,B042,F541,F841
- name: "linter: Flake8 Non-Mandatory"
env:
ELECTRUM_LINTERS: E,F,W,C90,B
diff --git a/electrum/keystore.py b/electrum/keystore.py
index b7f39c1..e162185 100644
--- a/electrum/keystore.py
+++ b/electrum/keystore.py
@@ -29,7 +29,7 @@ import hashlib
import re
import copy
from typing import Tuple, TYPE_CHECKING, Union, Sequence, Optional, Dict, List, NamedTuple, Any, Type
-from functools import lru_cache, wraps
+from functools import wraps
from abc import ABC, abstractmethod
import electrum_ecc as ecc
@@ -52,6 +52,7 @@ from .util import (InvalidPassword, WalletFileException,
from .mnemonic import Mnemonic, Wordlist, calc_seed_type, is_seed
from .plugin import run_hook
from .logging import Logger
+from .lrucache import LRUCache
if TYPE_CHECKING:
from .gui.common_qt.util import TaskThread
@@ -398,6 +399,9 @@ class Deterministic_KeyStore(Software_KeyStore):
class MasterPublicKeyMixin(ABC):
+ def __init__(self):
+ self._pubkey_cache = LRUCache(maxsize=10**4) # type: LRUCache[Sequence[int], bytes] # path->pubkey
+
@abstractmethod
def get_master_public_key(self) -> str:
pass
@@ -434,8 +438,14 @@ class MasterPublicKeyMixin(ABC):
def get_key_origin_info(self) -> Optional[KeyOriginInfo]:
return None
- @abstractmethod
def derive_pubkey(self, for_change: int, n: int) -> bytes:
+ key = (for_change, n)
+ if key not in self._pubkey_cache:
+ self._pubkey_cache[key] = self._derive_pubkey(*key)
+ return self._pubkey_cache[key]
+
+ @abstractmethod
+ def _derive_pubkey(self, for_change: int, n: int) -> bytes:
"""Returns pubkey at given path.
May raise CannotDerivePubkey.
"""
@@ -501,6 +511,7 @@ class MasterPublicKeyMixin(ABC):
class Xpub(MasterPublicKeyMixin):
def __init__(self, *, derivation_prefix: str = None, root_fingerprint: str = None):
+ MasterPublicKeyMixin.__init__(self)
self.xpub = None
self.xpub_receive = None
self.xpub_change = None
@@ -608,8 +619,7 @@ class Xpub(MasterPublicKeyMixin):
self._derivation_prefix = derivation_prefix
self.is_requesting_to_be_rewritten_to_wallet_file = True
- @lru_cache(maxsize=None)
- def derive_pubkey(self, for_change: int, n: int) -> bytes:
+ def _derive_pubkey(self, for_change: int, n: int) -> bytes:
for_change = int(for_change)
if for_change not in (0, 1):
raise CannotDerivePubkey("forbidden path")
@@ -624,7 +634,7 @@ class Xpub(MasterPublicKeyMixin):
return self.get_pubkey_from_xpub(xpub, (n,))
@classmethod
- def get_pubkey_from_xpub(self, xpub: str, sequence) -> bytes:
+ def get_pubkey_from_xpub(cls, xpub: str, sequence) -> bytes:
node = BIP32Node.from_xkey(xpub).subkey_at_public_derivation(sequence)
return node.eckey.get_public_key_bytes(compressed=True)
@@ -729,6 +739,7 @@ class Old_KeyStore(MasterPublicKeyMixin, Deterministic_KeyStore):
type = 'old'
def __init__(self, d: dict):
+ MasterPublicKeyMixin.__init__(self)
Deterministic_KeyStore.__init__(self, d)
self.mpk = d.get('mpk') # type: Optional[str]
self._root_fingerprint = None
@@ -807,8 +818,7 @@ class Old_KeyStore(MasterPublicKeyMixin, Deterministic_KeyStore):
public_key = master_public_key + z*ecc.GENERATOR
return public_key.get_public_key_bytes(compressed=False)
- @lru_cache(maxsize=None)
- def derive_pubkey(self, for_change, n) -> bytes:
+ def _derive_pubkey(self, for_change, n) -> bytes:
for_change = int(for_change)
if for_change not in (0, 1):
raise CannotDerivePubkey("forbidden path")
Why this scored 25/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.