[electrum] detect the master private key when loading a proof
ClosedPublic
Actions

Authored by PiRK on Nov 2 2023, 09:38.

Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Commits

rABC5dcccb27716f: [electrum] detect the master private key when loading a proof

Summary

When loading a proof in the wallet that created it, if the master key is one the auxiliary private keys, we can find the private key from the deserialized public key instead of making the user paste the key.

This used to work with the assumption that we always use index 0 to derive the key for proofs. It is broken after D14603 which broke this assumption: the program now does not necessarily suggests key from index 0 when the user generates a second proof or uses keys for other purposes (delegations...)

Fix it by scanning up to 20 recently used auxiliary keys.

Introduce a StorageKeys class to avoid repeating (and possibly mistyping) the storage keys and their default values.

Test Plan

Create a skeleton proof, save it to file or copy the hex to the clipboard.
Close the wallet, reopen the same wallet, open the proof editor, load the proof and check that the master key (WIF) is correctly filled and no warning dialog is displayed.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Nov 2 2023, 09:38

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 2 2023, 09:38

PiRK requested review of this revision.Nov 2 2023, 09:38

Harbormaster completed remote builds in B25515: Diff 42914.Nov 2 2023, 09:42

PiRK added inline comments.Nov 2 2023, 11:50

electrum/electrumabc/storage.py
87 ↗	(On Diff #42914)	the end goal would be to use only the new `storagekey` object everywhere in the codebase, but I'm not sure we can enforce it for plugins.

see inline comments. potentially i'm just misunderstanding a normal pything / electrum thing. if so, please let me know what's going on 🙏

electrum/electrumabc/tests/test_wallet_vertical.py
12 ↗	(On Diff #42914)	ok -- so i get why we have `gap_limit` here that is distinct from `StorageKeys.GAP_LIMIT`
electrum/electrumabc/wallet.py
3364 ↗	(On Diff #42914)	...but now I don't understand why we need this `self.gap_limit` if it's just going to be `StorageKeys.GAP_LIMIT` -- is this used for anything except storage keys?

This revision now requires changes to proceed.Nov 2 2023, 19:21

PiRK requested review of this revision.Nov 2 2023, 21:51

PiRK added inline comments.

electrum/electrumabc/wallet.py
3364 ↗	(On Diff #42914)	So here we initialize a `wallet.gap_limit` attribute with the value from the database (the json file used as wallet files) that is used throughout the wallet as a shortcut when scanning addresses, and can be changed via `wallet.change_gap_limit` (wallet.py line 3377) and committed back to the database. We probably could entirely skip this attribute and always access and update the value directly via `wallet.storage.get...` or `wallet.storage.put...`, but that's outside the scope of this diff. I would need to check if there is a cost for accessing via storage vs accessing the value through this shortcut (and think about how it would work with different database format that does not require rewriting the entire database for each change). I see that `storage.put` triggers a wallet dumping to disk, but as far as I can see everytime we modify this `gap_limit` attribute we also call `storage.put`, so maybe there would not really be any difference.

PiRK added inline comments.Nov 2 2023, 21:54

electrum/electrumabc/wallet.py
3364 ↗	(On Diff #42914)	tl;dr I think your comment is probably correct and warrants a follow-up, but in this diff I don't think it is relevant to the change.

Is this diff in response to user complaints? what's the root cause of wanting to add this functionality?

is "fix it" here from the diff description referring to problem of not knowing the private key and having to enter it manually, or electrum is just always using the 0 key which is now not always correct?

Fabien accepted this revision.Nov 3 2023, 13:45

In D14726#330975, @bytesofman wrote:

Is this diff in response to user complaints? what's the root cause of wanting to add this functionality?

is "fix it" here from the diff description referring to problem of not knowing the private key and having to enter it manually, or electrum is just always using the 0 key which is now not always correct?

It is a regression that I noticed while testing proof loading, the proof editor used to be able to "guess" the key when loading a proof generated with the same wallet. Now as it only tries the 0 index key, it no longer finds it if the user generated more than a single proof or used aux keys for other purposes such as generating a delegation. It significantly degrades the UX and security imo, because it adds a step of having to save wif key to a text file or writing it down, or crawling through aux keys to find the correct one, and pasting it back in the proof editor after loading the proof from file.

PiRK edited the summary of this revision. (Show Details)Nov 3 2023, 14:37

bytesofman accepted this revision.Nov 3 2023, 16:22

This revision is now accepted and ready to land.Nov 3 2023, 16:22

Closed by commit rABC5dcccb27716f: [electrum] detect the master private key when loading a proof (authored by PiRK). · Explain WhyNov 3 2023, 16:28

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC5dcccb27716f: [electrum] detect the master private key when loading a proof.