Closed BamaHodl closed 4 months ago
I've taken a first look at this pr as well as a single test:
much more to check, but this is interesting for many I suspect. ty @BamaHodl
As of commit 89f6cec,
I have reviewed, and tested, nothing newly broken.
For Address Verification, it appears to search standard native-segwit bip84 path instead of m/0', but I understand that for electrum seeds, not all functionality is expected to work.
For Address Verification, it appears to search standard native-segwit bip84 path instead of m/0', but I understand that for electrum seeds, not all functionality is expected to work.
My mistake, I was unaware it was not working for electrum seeds. The way the derivation path is set for address verification is to take a default derivation path for bip-39 seeds rather than looking it up based on the seed itself so I missed that one. A simple fix was made to simply check if the seed is electrum before doing the search, and override the derivation path if so. Address verification works with this latest change.
Address verification works with this latest change.
as of 1f0a4dd, I confirm.
Very nice BamaHodl!
Without testing and just with a glance of 289b382
I wonder if that commit, all by itself, might better be a refactoring pull-request separate from this one.
I just say that because it's sort of off topic from "electrum support", even if it does reduce the need for "is_electrum". A smaller pull-requests is easier to review and 'ack'. I'd hate to see some progress not go forward because it were waiting for other progress to be reviewed and accepted.
Up to you. Thanks again for nice improvements.
I see your reasoning from the procedure aspect--should probably be a PR on its own. Was just trying to clean it up a bit to avoid needing to do electrum-specific stuff there. I've backed out that commit
Can confirm that this would be a useful comparability feature with a popular open source wallet. Being able to bring people into SS from Electurm would be useful. Thank you
@BamaHodl, can you find a sensible spot in the docs to add info about Electrum seed import? Especially clarification about which Electrum format is supported (I had no idea they updated to a version with the bip39 wordlist; should say that we don't support their older wordlist). Also note any other limitations/caveats when in this mode.
I'm very uncomfortable with the idea of facilitating SeedQR creation for an Electrum seed. I can confirm that it does work to scan back in. But it somewhat muddies the water of what a SeedQR encodes and would likely be wholly unsupported by other software/hardware that has adopted the SeedQR standard.
It seems like EITHER we:
My main interest in supporting this PR is to give users with Electrum seeds the ability to use their seed in SeedSigner, BUT if it's a clunkier, less convenient experience for them, so be it.
SeedSigner w/Electrum seed makes the most sense for rare cases of emergency signing / bailing you out of trouble. And to a lesser extent, external verification (e.g. address explorer). But Electrum seed usage in general should be discouraged. And maybe even more functionality should be disabled (e.g. a bip85 child seed derived from an Electrum seed parent will still yield a bip39 seed... or we can just say, "No, not even gonna go there").
I haven't fully thought this through, but I feel like we should be able to isolate all of the Electrum-specific code in seed.py by subclassing Seed
:
class Seed:
...
class ElectrumSeed(Seed):
...
That would then shift the "is it bip39 or is it Electrum" logic to https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/models/seed_storage.py#L90-L91 where if the Seed
instantiation fails, we can try ElectrumSeed
instead.
Also here if we support Electrum SeedQRs: https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/views/scan_views.py#L75-L77
Perhaps a higher-level util could further abstract that away (e.g. a SeedParser
in seed.py that returns either a Seed
or an ElectrumSeed
).
I had a look at this and do think it's a good idea to support importing these seeds. The main challenge with Electrum seeds is that 1/16 of them is also a valid BIP39 seed, so any support for them should be manually enabled, as opposed to prompting if the checksum is correct for Electrum. (Or should only offer to try Electrum if the BIP39 seed has an invalid checksum, but a valid Electrum version bit, which doesn't seem to be the logic in the PR at the moment.)
The main challenge with Electrum seeds is that 1/16 of them is also a valid BIP39 seed, so any support for them should be manually enabled, as opposed to prompting if the checksum is correct for Electrum.
Oof, agreed. I think maybe an explicit two-step process:
1/16 of them is also a valid BIP39 seed
I believe it's more like 1 out of 16 million? Basically, if the hash of the seed starts with "100" in ascii, not binary (e.g. 3 full bytes must match, 1 in 2^24 probability)
Also, the reverse is not possible--electrum-generated seeds will never have a valid BIP-39 checksum because there is a check to discard those when electrum wallet generates seeds to avoid that confusion. But you are correct that 1 in 2^24 BIP-39 seeds will be a valid electrum seed--that's why I made it require user confirmation rather than just assuming they meant to enter an electrum seed. It's a very small chance, but we don't want to skip any edge cases by making assumptions for sure
1/16 of them is also a valid BIP39 seed
I believe it's more like 1 out of 16 million? Basically, if the hash of the seed starts with "100" in ascii, not binary (e.g. 3 full bytes must match, 1 in 2^24 probability)
Also, the reverse is not possible--electrum-generated seeds will never have a valid BIP-39 checksum because there is a check to discard those when electrum wallet generates seeds to avoid that confusion. But you are correct that 1 in 2^24 BIP-39 seeds will be a valid electrum seed--that's why I made it require user confirmation rather than just assuming they meant to enter an electrum seed. It's a very small chance, but we don't want to skip any edge cases by making assumptions for sure
Electrum only added this check in mid 2021. (And other wallets which generate Electrum compatible seeds may not check it at all) Basically any segwit compatible electrum seed generated before then has a 1/16 chance of being a valid BIP39 seed too.
1/16 of them is also a valid BIP39 seed
I believe it's more like 1 out of 16 million? Basically, if the hash of the seed starts with "100" in ascii, not binary (e.g. 3 full bytes must match, 1 in 2^24 probability) Also, the reverse is not possible--electrum-generated seeds will never have a valid BIP-39 checksum because there is a check to discard those when electrum wallet generates seeds to avoid that confusion. But you are correct that 1 in 2^24 BIP-39 seeds will be a valid electrum seed--that's why I made it require user confirmation rather than just assuming they meant to enter an electrum seed. It's a -very small chance, but we don't want to skip any edge cases by making assumptions for sure
Electrum only added this check in mid 2021. (And other wallets which generate Electrum compatible seeds may not check it at all) Basically any segwit compatible electrum seed generated before then has a 1/16 chance of being a valid BIP39 seed too.
See here: spesmilo/electrum#6001
Thanks, was unaware of the history and assumed the check was there was since integrating segwit. There are many more electrum seeds that are also valid BIP-39 seeds than I had guessed, then, since the check was not always there. There are still many fewer BIP-39 seeds that are valid electrum seeds, however even so it is also much higher than I had estimated. According to that discussion it's (0.439%) and not 1/2^24
I haven't fully thought this through, but I feel like we should be able to isolate all of the Electrum-specific code in seed.py by subclassing
Seed
:class Seed: ... class ElectrumSeed(Seed): ...
That would then shift the "is it bip39 or is it Electrum" logic to https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/models/seed_storage.py#L90-L91 where if the
Seed
instantiation fails, we can tryElectrumSeed
instead.Also here if we support Electrum SeedQRs: https://github.com/BamaHodl/seedsigner/blob/dev/src/seedsigner/views/scan_views.py#L75-L77
Perhaps a higher-level util could further abstract that away (e.g. a
SeedParser
in seed.py that returns either aSeed
or anElectrumSeed
).
I had also considered this, but decided against it, trying to recall why.. I think it was just because I wanted to minimize changes and avoid doing anything that would be seen as changing the architecture of the code (e.g. by moving functionality into the seed class and Electrum seed sub-class). I could take another look at this though if it's an acceptable way to change things and see if it simplifies the code a lot. Will get back on this.
I had also considered this, but decided against it, trying to recall why.. I think it was just because I wanted to minimize changes and avoid doing anything that would be seen as changing the architecture of the code (e.g. by moving functionality into the seed class and Electrum seed sub-class).
My personal preference is to keep any Electrum-specific code off to the side as much as possible. The new code would ideally be a parallel drop-in rather than seeing if seed.is_electrum:
conditionals everywhere. But within reasonable limits of DRY principles, of course.
Thus this suggestion of a ElectrumSeed
child class where there'd be almost zero duplication and obviously meets my goal of keeping those conditionals out of the Seed
class.
For clarity, my understanding of feedback has led to the following plan:
Please let me know if there is additional feedback on this plan or if I have misunderstood anything.
@BamaHodl excellent summary, great job collecting it all together! Also thank you for being so open to all the suggestions/changes/etc.
- A new Advanced Setting option to enable Electrum seed support (necessary?), disabled by default
Yes, I think it's necessary. We've tried to keep the UI as simple as possible. I think every additional option creates exponentially more doubt and confusion for a new user (e.g. the jump from 2 options on a screen to 3). The beauty of our Settings is that we can bury TONS of advanced functionality in there so that the majority of users never see the feature in the UI. It's much better to answer "Can it do X?" ("Yes, just go to Settings") vs "I'm confused, do I select X, Y, or Z?"
Will attempt to add legacy ("Standard", as it's officially named) Electrum seed version support as part of these additional changes if it can be easily done
Based on @3rdIteration's comments, are there effectively three(ish) Electrum types?
Though for our purposes, 2.) and 3.) are effectively the same (w/the updated flow forcing them to explicitly tell us they're trying to enter an Electrum seed, we'll never even notice if it happened to also be bip39 compliant), right?
Create separate ElectrumSeed (parent: Seed) class and to the extent possible use polymorphism to handle differences between Electrum and BIP-39 seed behavior
I think this should be a pretty clean child class that only needs to override a handful of simple methods.
Also with the updated flow, I don't think we'll need the SeedParser
concept I mentioned above. The updated flows always know which Seed type to expect, so they can just directly instantiate the appropriate one.
More food for thought: if possible, see if creating a child class of any existing View
s makes sense to further isolate the Electrum-specific code. I'd love it if SeedExportXpubDetailsView
could get a child class ElectrumSeedExportXpubDetailsView
to keep all the Electrum mods out of that already more complicated parent View. the parent could, for example, be initialized with default embit_util
functions to call. But the child could override those to Electrum version equivalents (it would be a win-win to pull the Electrum logic out of those embit_utils functions anyway. The Electrum logic in them is pretty trivial, I think?).
class SeedExportXpubDetailsView(View):
# I'm not 100% sure about the Callable type hint details...
get_derivation_path: Callable[..., str] = embit_utils.get_standard_derivation_path
def run(self):
derivation_path = self.get_derivation_path(...)
...
class ElectrumSeedExportXpubDetailsView(View):
get_derivation_path: Callable[...,str] = embit_utils.get_electrum_derivation_path
# Maybe that's it? The entire child class is just one or two function assignments...?
With that minimal rewiring of which version of the function gets called, maybe no other overrides are needed AND no if seed.is_electrum
conditionals? I haven't fully tried this out so I could be totally wrong.
Another approach could work for SeedAddPassphraseView
. If the title was an optional arg on __init__
, the calling View could just set "Custom Extension" so that SeedAddPassphraseView
would never care which type of Seed it was working with.
Again, not totally sure it's worth it just to avoid one if seed.is_electrum
test, but the basic point is to look for every opportunity to streamline those away, especially when they intrude into common / important areas of the code.
If user attempts to "Export as QR" on an ElectrumSeed, display a NotYetImplemented screen with a note like "SeedQR for Electrum seeds is deliberately disabled, see docs"
I would just filter the Seed Options list (err, more like in the Backup Options sub-menu) and not even display that button. Hmm... maybe it makes sense to include a note that SeedQR is explicitly disabled for Electrum seeds.
I also think other optional features like bip85 should not be displayed. Maybe someone will make a strong case for enabling the combination (Electrum seed creating bip85 children) in a later release, but for now I'd favor a more conservative / restrictive approach.
Speaking of that, there wasn't a mention of what to do with the warning screen.
I think the flow should probably be:
If there's any strong pushback on that warning, perhaps the Settings option for Electrum support could be:
It would be beneficial to have the Electrum seed specification linked here. I found this: https://electrum.readthedocs.io/en/latest/seedphrase.html
Is there another document?
There's another non-standard seed scheme by Lightning Labs LND open source project. See here https://github.com/lightningnetwork/lnd/blob/master/aezeed/README.md
It has versioning and wallet birthdays.
Electrum seeds are missing birthdays and checksums.
When looking through advanced settings on my 0.7.0 SS I don't see any settings or options like this proposed support for non-standard seeds.
I'm wondering what derivation paths Electrum seeds use and if this can create foot guns for users who use BIP43/44/49/84/86 derivation paths with Electrum seeds that are supposed to capture that information in the seed version prefix.
Thomas V. made a brilliant invention with mnemonic seeds. Upon peer review versions and derivation paths were not baked in for valid flexibility reasons and a word list was standardized and a checksum was added.
Should SS be like Electrum? A break-glass in case of emergency tool. Or should it be a go to, happy path, best practices tool?
Versioning the seed can lead to the version being ignored. Like if you version an Electrum seed for single-sig standard and then reuse the same seed for multisig.
Ethereum made this mistake where the public key has a network version and then people reuse the Ethereum address on Rootstock but keep the Ethereum mainnet version number, essentially ignoring the version number.
Thank you for your questions @pointbiz :
It would be beneficial to have the Electrum seed specification linked here. I found this: https://electrum.readthedocs.io/en/latest/seedphrase.html
Is there another document?
That is a good resource and I also found this excerpt helpful on the history of electrum mnemonics and how they evolved across versions of the software: https://bitcoinelectrum.com/frequently-asked-questions/#why-wont-electrum-accept-my-seed-when-i-attempt-to-restore-my-wallet-from-it "Old" mnemonics in Electrum software prior to version 2.0 (March 02, 2015) are 12 words from a different wordlist from BIP-39. "Standard" mnemonics are from the BIP-39 wordlist in Electrum version 2.0+, and until version 2.7.0 (Oct 02, 2016) they were 13 words, and 12 words thereafter. "Segwit" mnemonics are available in version 3.0+ (Nov 01, 2017) and are 12 words from the BIP-39 wordlist and the address derivation is compatible with BIP-32. These seeds are what this PR is focused on as they are most compatible with existing SS functionality. I am still investigating the possibility to add support for the "Standard" non-segwit seed type.
I'm wondering what derivation paths Electrum seeds use and if this can create foot guns for users who use BIP43/44/49/84/86 derivation paths with Electrum seeds that are supposed to capture that information in the seed version prefix.
Electrum wallet software does not allow selecting derivations for its own mnemonics--they are restricted to "m/0h" as the root node in the wallet software, so that is what is replicated in this PR for the segwit seed version
@kdmukai I have integrated the changes we discussed as follows:
Note: I'm still running tests and might make further changes, just wanted to make sure what has been done is consistent with the discussion.
Today, I tested this promising addition to Seedsigner and managed to sign a transaction. Unfortunately, Electrum, Sparrow and Bluewallet could't parse the animated QR-Code to import the signed transaction. Saved Electrum seeds don't seem to remember their electrum origin... and following transactions cannot be signed. I would like to see the option to add a BIP39 passphrase to an Electrum seed. Looking forward to this valuable addition to Seedsigner!
Finally, I managed to sign a transaction with Sparrow wallet, Seedsigner and an Electrum seed :-) Sending the signed transaction via QR-Code to Sparrow was successful, broadcasting too...
My previous problems may have been caused by a large transaction size and Electrum having trouble reading animated QR-Codes... I forgot Electum calls seed passwords a "seed extension". This change does add an option for a custom extension. I apologise for my ignorance.
Finally, I managed to sign an transaction with Sparrow wallet, Seedsigner and an Electrum seed :-) Sending the signed transaction via QR-Code to Sparrow was successful, broadcasting too...
My previous problems may have been caused by a large transaction size and Electrum having trouble reading animated QR-Codes... I forgot Electum calls seed passwords a "seed extension". This change does add an option for a custom extension. I apologise for my ignorance.
Thanks for the update, and apologies for not seeing your prior comment! Glad you were able to both get the tx signing and custom extension features working using Sparrow as your coordinator. It should be noted that the official Electrum wallet software is not yet a supported coordinator for the SeedSigner, so you are correct that loading and signing transactions with Electrum software will not work currently, in particular because as you mentioned animated QR formats are not yet supported. For my use what I do is load my Electrum seed into the SeedSigner using this new feature, export the xpub into a supported coordinator software (Sparrow is my favorite), and then everything works as expected as any other seed would.
I am taking another look at this PR and will get it merge-ready again at least from a testing and merge conflict standpoint so that we can hopefully get this in the main repo and a future production release if all goes well.
I am taking another look at this PR and will get it merge-ready again at least from a testing and merge conflict standpoint so that we can hopefully get this in the main repo and a future production release if all goes well.
That would be great! Thanks!
@BamaHodl I did some work on adding full legacy support that I think I will bring in another PR. (As there didn't seem to be any objections)
Basically once this is in there, the rest of the legacy Electrum stuff will pretty much "just work" with minimal extra work.
@BamaHodl I did some work on adding full legacy support that I think I will bring in another PR. (As there didn't seem to be any objections)
That's awesome! Just curious what exactly you mean when you refer to legacy, do you mainly mean supporting p2pkh script type? That was my main reason for not attempting any of the other electrum seed types other than native segwit, namely p2pkh not currently implemented in seedsigner. If we can get p2pkh support then adding the older electrum seed types should be relatively straight-forward. These are the electrum seed types I found in my research:
Name | Wordlist | Num Words | Script Type | Electrum version |
---|---|---|---|---|
Native SegWit | BIP-39 | 12 | p2wpkh | >=3.0 |
Standard | BIP-39 | 12 | p2pkh or multi-sig p2sh | >=2.7 and <3.0 |
Standard | BIP-39 | 13 | p2pkh or multi-sig p2sh | >=2.0 and <2.7 |
"Old" | Legacy | 12 | p2pkh | <2.0 |
@BamaHodl we are planning a pre release soon. Hopefully before the end of July. Would you be able to resolve the merge conflict on this PR (It looks like just a whitespace issue) before I do code review and testing? Thanks!
@BamaHodl we are planning a pre release soon. Hopefully before the end of July. Would you be able to resolve the merge conflict on this PR (It looks like just a whitespace issue) before I do code review and testing? Thanks!
Fixed
A lesser known feature of SeedSigner is that it can actually scan and decode a single frame PSBT QR (Base43) from Electrum. This feature has never been advertised or really supported, but I remember working on it. It wasn't really practical since there was not Electrum supported way to get the signed PSBT back into Electrum.
Overall I'm good with this PR minus a few changes. I really hope @kdmukai finds time to update his review of this PR. I have a feeling there might be something he'd want to see updated since all the changes @BamaHodl has made since the beginning of March. I appreciate this great contribution @BamaHodl!
I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)
I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)
I'm also going to attempt integrating the old Standard seed types that use P2PKH. Might be able to accomplish full historical electrum seed support now with your changes. The difficulty with supporting them all will be the UX. Will have to give the user options to use 12 word seed phrases, 13 word seed phrases (with a warning this is NOT the custom extension), and the option to load from the old, pre-2.0 wordlist (i.e. not BIP-39 wordlist) to call it full coverage.
I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)
I'm also going to attempt integrating the old Standard seed types that use P2PKH. Might be able to accomplish full historical electrum seed support now with your changes. The difficulty with supporting them all will be the UX. Will have to give the user options to use 12 word seed phrases, 13 word seed phrases (with a warning this is NOT the custom extension), and the option to load from the old, pre-2.0 wordlist (i.e. not BIP-39 wordlist) to call it full coverage.
Yea the really old seeds are certainly a bit of an issue UX wise. It could be that a separate PR is required which allows users to enable/disable different seed types in a similar manner to how script types are enabled/disabled. (Which would also make it easier to add other things like SLIP39 or other future seed standards which may emerge)
I'm happy to add the code to support legacy Electrum addresses in a separate PR (That is trivial once this and the other Legacy address type PR are merged)
playing around with the older electrum p2pkh seeds here with the addition of your legacy script support PR: https://github.com/BamaHodl/seedsigner/tree/LegacyElectrum
I haven't tested everything yet but so far it's as you said: it just works without having to change much.
tACK
Description
Add support for electrum segwit seeds (Issue #438 ) so that users with electrum seeds will not need to move utxos to a BIP-39 seed to use SeedSigner as their signing device.
It requires one new option to enable Electrum seed support in Advanced settings (disabled by default), which will allow the user the option to enter Electrum seed phrases on screens that allow the user to enter BIP-39 seed phrases. Per the PR comments, SeedQR has been disabled for Electrum seeds.
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: