wallet: disable_keystore() not to destroy get_key_origin_info()
What the developer wrote
Authored by SomberNight
wallet: disable_keystore() not to destroy get_key_origin_info()
What changed, and why it matters
This commit fixes a bug in the Electrum wallet where disabling a keystore (a container for private key material) accidentally wiped out important key metadata called 'key origin info.' This metadata tells the wallet where a key came from in a derivation path. Losing it could cause problems when the wallet later tries to sign transactions or interact with hardware wallets, because the wallet might not be able to reconstruct the correct key path. The fix changes the internal field name used when converting a keystore to a watch-only version, and adds tests to make sure the metadata survives.
Treat as a bug-fix commit with low-to-moderate security relevance. Review whether any released versions shipped with this metadata loss and whether it could affect PSBT signing or hardware-wallet compatibility. No immediate emergency response is indicated, but backporting the fix and adding the regression tests is prudent.
Security signals we found
Data-loss bug in key metadata preservation
Incorrect constructor keyword argument (`derivation_prefix` vs `derivation`)
Watch-only keystore conversion may break PSBT/hardware-wallet signing path reconstruction
Regression tests added for disabled keystore metadata integrity
Evidence from the diff
The patch corrects BIP32_KeyStore.only_keystore() and Hardware_KeyStore.only_keystore() so that when a keystore is disabled/converted to watch-only, the resulting BIP32_KeyStore is initialized with derivation rather than derivation_prefix. The constructor expects derivation for the derivation prefix, so using derivation_prefix caused the derivation data to be lost. This in turn destroyed get_key_origin_info() after disable_keystore(). The patch also adds __eq__ and __repr__ to KeyOriginInfo and extends test_wizard.py sanity checks to assert that key origin info is preserved across disable/enable cycles for Electrum seed, BIP39, and hardware-wallet keystores.
Changed components
electrum/keystore.py: BIP32_KeyStore.only_keystore()electrum/keystore.py: Hardware_KeyStore.only_keystore()electrum/bip32.py: KeyOriginInfo equality/repr helperstests/test_wizard.py: KeystoreWizardTestCase disable_keystore sanity checksInspect captured patch +30 / −9
diff --git a/electrum/bip32.py b/electrum/bip32.py
index 9572055..6d36322 100644
--- a/electrum/bip32.py
+++ b/electrum/bip32.py
@@ -525,3 +525,10 @@ class KeyOriginInfo:
xfp.extend(self.path)
return xfp
+ def __eq__(self, other) -> bool:
+ if not isinstance(other, KeyOriginInfo):
+ return False
+ return self.serialize() == other.serialize()
+
+ def __repr__(self) -> str:
+ return f"<KeyOriginInfo {self.to_string()}>"
diff --git a/electrum/keystore.py b/electrum/keystore.py
index 13241f0..0408894 100644
--- a/electrum/keystore.py
+++ b/electrum/keystore.py
@@ -643,7 +643,7 @@ class BIP32_KeyStore(Xpub, Deterministic_KeyStore):
return BIP32_KeyStore({
'xpub': self.xpub,
'root_fingerprint': self.get_root_fingerprint(),
- 'derivation_prefix': self.get_derivation_prefix(),
+ 'derivation': self.get_derivation_prefix(),
})
def format_seed(self, seed):
@@ -903,7 +903,7 @@ class Hardware_KeyStore(Xpub, KeyStore):
return BIP32_KeyStore({
'xpub': self.xpub,
'root_fingerprint': self.get_root_fingerprint(),
- 'derivation_prefix': self.get_derivation_prefix(),
+ 'derivation': self.get_derivation_prefix(),
})
def set_label(self, label: Optional[str]) -> None:
diff --git a/tests/test_wizard.py b/tests/test_wizard.py
index 3ba3840..8eb12fa 100644
--- a/tests/test_wizard.py
+++ b/tests/test_wizard.py
@@ -10,6 +10,7 @@ from electrum.daemon import Daemon
from electrum.wallet import Abstract_Wallet
from electrum import util
from electrum import slip39
+from electrum.bip32 import KeyOriginInfo
from . import ElectrumTestCase
from .test_wallet_vertical import UNICODE_HORROR
@@ -169,13 +170,20 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet = Daemon._load_wallet(wallet_path, password=None, config=self.config)
return wallet
- def _sanity_checks_after_disabling_keystore(self, *, ks: 'KeyStore', xpub: str) -> None:
+ def _sanity_checks_after_disabling_keystore(
+ self,
+ *,
+ ks: 'KeyStore',
+ xpub: str,
+ key_origin_info: KeyOriginInfo,
+ ) -> None:
self.assertTrue(ks.is_watching_only())
self.assertTrue(ks.type in ('bip32', 'old'))
self.assertFalse(ks.has_seed())
self.assertEqual(ks.get_master_public_key(), xpub)
if isinstance(ks, BIP32_KeyStore):
self.assertEqual(ks.xprv, None)
+ self.assertEqual(ks.get_key_origin_info(), key_origin_info)
async def test_haveseed_electrum(self):
w, v = self._wizard_for()
@@ -200,8 +208,9 @@ class KeystoreWizardTestCase(WizardTestCase):
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
self.assertEqual(mypassphrase, wallet.get_keystore().get_passphrase(None))
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
async def test_haveseed_ext_electrum(self):
w, v = self._wizard_for()
@@ -230,8 +239,9 @@ class KeystoreWizardTestCase(WizardTestCase):
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
self.assertEqual(mypassphrase, wallet.get_keystore().get_passphrase(None))
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
async def test_haveseed_electrum_oldseed(self):
w, v = self._wizard_for()
@@ -257,8 +267,9 @@ class KeystoreWizardTestCase(WizardTestCase):
self.assertEqual(myseed, wallet.get_keystore().get_seed(None))
self.assertEqual(mypassphrase, wallet.get_keystore().get_passphrase(None))
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
async def test_haveseed_bip39(self):
w, v = self._wizard_for()
@@ -283,8 +294,9 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
async def test_haveseed_ext_bip39(self):
w, v = self._wizard_for()
@@ -313,8 +325,9 @@ class KeystoreWizardTestCase(WizardTestCase):
wallet.enable_keystore(ks, ishww, None)
self.assertFalse(wallet.get_keystore().is_watching_only())
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
async def test_hww(self):
w, v = self._wizard_for(hww=True)
@@ -354,8 +367,9 @@ class KeystoreWizardTestCase(WizardTestCase):
self.assertFalse(wallet.get_keystore().is_watching_only())
self.assertTrue(isinstance(wallet.get_keystore(), Hardware_KeyStore))
+ my_keyorigininfo = wallet.get_keystore().get_key_origin_info()
wallet.disable_keystore(wallet.get_keystore())
- self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub)
+ self._sanity_checks_after_disabling_keystore(ks=wallet.get_keystore(), xpub=myxpub, key_origin_info=my_keyorigininfo)
class WalletWizardTestCase(WizardTestCase):
Why this scored 28/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.