What the developer wrote
Authored by kdmukai
Scope the derivation cache to parse()
Nothing outside parse() reads the cache, so it need not be an instance
attribute. As a local, "the cache does not outlive the parse" is a
property of scope rather than a teardown to maintain -- which also
retires the finally block, both initializations, and the test guarding
them.
What changed, and why it matters
This commit is a code-quality and defense-in-depth change, not a fix for an active bug. It moves a temporary cache of derived Bitcoin keys from being a long-lived object property to a short-lived local variable that exists only during parsing. The cache already had a size cap, so unbounded memory growth was already prevented. The change makes it structurally impossible for the cache to leak beyond a single parse, which slightly reduces the risk that sensitive derived key material could remain in memory longer than necessary. It also removes the explicit cleanup code and one related test because the local variable naturally disappears when parsing finishes.
Treat this as a low-risk hardening change. Reviewers should confirm that all call sites that previously read self._child_key_derivation_cache now receive and use the passed local cache, and that no other methods or tests retain references to the removed instance attribute. No urgent security response is warranted because the prior code already capped cache size and explicitly cleared it.
Security signals we found
Reduction of sensitive-data lifetime: derived child keys are no longer stored as an instance attribute beyond the parse call
Defense-in-depth: local scope makes cache lifetime enforceable by the language rather than by manual teardown
Removal of explicit cleanup code and its associated test, justified because the local variable naturally goes out of scope
No change to MAX_CACHED_DERIVATIONS size cap, so the existing anti-DoS memory bound stays in place
Evidence from the diff
The PSBTParser class previously stored child key derivation results in self._child_key_derivation_cache, an instance attribute initialized in init and explicitly cleared in a finally block inside parse(). The commit converts it to a local child_key_derivation_cache dict created at the start of parse() and passed as an argument to _fill_missing_fingerprints(), _parse_inputs(), _parse_outputs(), and the static _get_policy()/_derive_with_cache() helpers. Because the cache is now a local variable, it is automatically reclaimed when parse() returns, eliminating the need for the finally-block teardown, the init initialization, and the test that verified the instance attribute was empty after parsing. The MAX_CACHED_DERIVATIONS bound remains unchanged, so the memory-growth mitigation against malicious PSBTs is preserved.
Changed components
src/seedsigner/models/psbt_parser.pytests/test_psbt_parser.pyInspect captured patch +31 / −54
### src/seedsigner/models/psbt_parser.py
@@ -33,13 +33,13 @@ class PSBTParser():
for and needs no parse.
"""
- # Upper bound on how many levels of derivation a single parse will cache in
- # _child_key_derivation_cache. 1000 is just slightly under a 3-of-5 multisig
- # consolidating 200 inputs and holds the cache to a max of about 600 kilobytes. A
- # psbt that requires more levels will still parse correctly, but may have to derive
- # some levels more than once. Capping the cache at a realistic upper bound protects
- # against a maliciously crafted psbt that would otherwise consume unbounded memory
- # while still providing cache wins for even atypically large real-world psbts.
+ # Upper bound on how many levels of derivation a single parse will cache. 1000 is
+ # just slightly under a 3-of-5 multisig consolidating 200 inputs and holds the cache
+ # to a max of about 600 kilobytes. A psbt that requires more levels will still parse
+ # correctly, but may have to derive some levels more than once. Capping the cache at
+ # a realistic upper bound protects against a maliciously crafted psbt that would
+ # otherwise consume unbounded memory while still providing cache wins for even
+ # atypically large real-world psbts.
MAX_CACHED_DERIVATIONS = 1000
@@ -60,7 +60,6 @@ def __init__(self, p: PSBT, seed: Seed, network: str = SettingsConstants.MAINNET
self.op_return_data: bytes = None
self.root = None
- self._child_key_derivation_cache = {}
if self.seed is not None:
self.parse()
@@ -109,8 +108,10 @@ def parse(self):
traversals overlap heavily: everything in one account shares the same opening
levels, differing only in the address at the end.
- So every level derived during this parse is kept in _child_key_derivation_cache
- and reused. See _derive_with_cache.
+ So every level derived during this parse is kept in a cache and reused. See
+ _derive_with_cache.
+
+ Note that the cache is only useful within a single parse so it is not preserved.
"""
if self.psbt is None:
logger.info(f"self.psbt is None!!")
@@ -122,29 +123,23 @@ def parse(self):
self._set_root()
- self._child_key_derivation_cache = {}
+ child_key_derivation_cache = {}
- try:
- # Try to fix missing fingerprints before parsing
- self._fill_missing_fingerprints()
+ # Try to fix missing fingerprints before parsing
+ self._fill_missing_fingerprints(child_key_derivation_cache)
- rt = self._parse_inputs()
- if rt == False:
- return False
+ rt = self._parse_inputs(child_key_derivation_cache)
+ if rt == False:
+ return False
- rt = self._parse_outputs()
- if rt == False:
- return False
+ rt = self._parse_outputs(child_key_derivation_cache)
+ if rt == False:
+ return False
- return True
- finally:
- # The cache is only useful within a single parse and it holds keys derived
- # from the signing seed, so drop it now rather than letting it live on for
- # as long as this parser does.
- self._child_key_derivation_cache = {}
+ return True
- def _parse_inputs(self):
+ def _parse_inputs(self, child_key_derivation_cache: dict):
self.input_amount = 0
self.num_inputs = len(self.psbt.inputs)
for inp in self.psbt.inputs:
@@ -155,14 +150,14 @@ def _parse_inputs(self):
self.input_amount += inp.utxo.value
script_pubkey = inp.script_pubkey
- inp_policy = PSBTParser._get_policy(inp, script_pubkey, self.psbt.xpubs, self._child_key_derivation_cache)
+ inp_policy = PSBTParser._get_policy(inp, script_pubkey, self.psbt.xpubs, child_key_derivation_cache)
if self.policy == None:
self.policy = inp_policy
else:
if self.policy != inp_policy:
raise RuntimeError("Mixed inputs in the transaction")
- def _parse_outputs(self):
+ def _parse_outputs(self, child_key_derivation_cache: dict):
self.spend_amount = 0
self.change_amount = 0
self.change_data = []
@@ -176,7 +171,7 @@ def _parse_outputs(self):
vout = self.psbt.tx.vout
for i, out in enumerate(self.psbt.outputs):
- out_policy = PSBTParser._get_policy(out, vout[i].script_pubkey, self.psbt.xpubs, self._child_key_derivation_cache)
+ out_policy = PSBTParser._get_policy(out, vout[i].script_pubkey, self.psbt.xpubs, child_key_derivation_cache)
is_change = False
# if policy is the same - probably change
@@ -209,7 +204,7 @@ def _parse_outputs(self):
# should be one or zero for single-key addresses
if len(out.bip32_derivations.values()) > 0:
der = list(out.bip32_derivations.values())[0].derivation
- my_pubkey = PSBTParser._derive_with_cache(self.root, der, self._child_key_derivation_cache)
+ my_pubkey = PSBTParser._derive_with_cache(self.root, der, child_key_derivation_cache)
if self.policy["type"] == "p2pkh" and my_pubkey is not None:
sc = script.p2pkh(my_pubkey)
@@ -230,7 +225,7 @@ def _parse_outputs(self):
# TODO: Support keys in taptree leaves
leaf_hashes, derivation = list(out.taproot_bip32_derivations.values())[0]
der = derivation.derivation
- my_pubkey = PSBTParser._derive_with_cache(self.root, der, self._child_key_derivation_cache)
+ my_pubkey = PSBTParser._derive_with_cache(self.root, der, child_key_derivation_cache)
sc = script.p2tr(my_pubkey)
if sc.data == vout[i].script_pubkey.data:
@@ -523,7 +518,7 @@ def verify_multisig_output(self, descriptor: Descriptor, change_num: int) -> boo
return is_owner
- def _fill_missing_fingerprints(self):
+ def _fill_missing_fingerprints(self, child_key_derivation_cache: dict):
"""
Fix for when fingerprint is missing (defaults to all zeros). Happens when the user
creates a new wallet in an external coordinator but only provides the xpub
@@ -552,7 +547,7 @@ def _get_updated_fingerprint(public_key: PublicKey, derivation_path_obj: Derivat
# fingerprint with the signing seed's master fingerprint so downstream
# parsing/signing can treat it as owned by this seed.
derived_key = PSBTParser._derive_with_cache(
- self.root, derivation_path_obj.derivation, self._child_key_derivation_cache)
+ self.root, derivation_path_obj.derivation, child_key_derivation_cache)
if derived_key.key.sec() == public_key.sec():
return DerivationPath(self.root.my_fingerprint, derivation_path_obj.derivation)
return None
### tests/test_psbt_parser.py
@@ -535,8 +535,8 @@ def cache_size_recorder(self, cache_sizes: list):
Returns a stand-in for _derive_with_cache that derives exactly as the real one
does, but appends the cache's size to cache_sizes on the way out of every call.
- Reading the cache back once the parse is over depends on the parse disposing of
- it by rebinding the attribute; recording sizes as the parse runs does not.
+ The cache is a local inside parse(), so intercepting the calls it gets handed to
+ is the only way to see how large it grew.
"""
real_derive_with_cache = PSBTParser._derive_with_cache
@@ -763,21 +763,3 @@ def build_psbt(case: tuple) -> PSBT:
assert max(capped_sizes) == cap
- def test_cache_is_dropped_when_the_parse_ends(self):
- """
- The cache holds keys derived from the signing seed, so the parser must not still
- be holding it once the parse it belongs to is over.
- """
- psbt = PSBT.parse(a2b_base64(PSBTTestData.MULTISIG_NATIVE_SEGWIT_1_INPUT))
- psbt.outputs.append(create_output(PSBTTestData.MULTISIG_NATIVE_SEGWIT_CHANGE, 10_000))
-
- cache_sizes = []
- with patch.object(PSBTParser, "_derive_with_cache", staticmethod(self.cache_size_recorder(cache_sizes))):
- psbt_parser = PSBTParser(psbt, self.seed, network=SettingsConstants.REGTEST)
-
- # Sanity check: there is something to drop, i.e. the parse really did fill the
- # cache it was handed.
- assert max(cache_sizes) > 0
-
- # But since the parse is done, the PSBTParser should have an empty cache again
- assert psbt_parser._child_key_derivation_cache == {}Why this scored 36/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.