Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: read electrum seeds from STDIN or a file #53

Closed
leto opened this issue May 10, 2019 · 10 comments
Closed

Enhancement: read electrum seeds from STDIN or a file #53

leto opened this issue May 10, 2019 · 10 comments
Assignees
Labels
advisory please read enhancement New feature or request resolved Issue has been resolved

Comments

@leto
Copy link

leto commented May 10, 2019

Currently when using

--restore-deterministic-wallet --electrum-seed "the seedphrase"

it means that the seedphrase can be seen in a trivial ps output and will also potentially be in shell history. This is not just a short running process, when recovering a wallet and leaving it running, the seedphrase can be seen in the process arguments until the wallet is exited.

Instead of the current behavior of giving an error if no seedphrase is specified, the wallet would attempt to read it from STDIN. There could also be a new --electrum-seed-file as well.

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

Thanks for letting me know @leto. I'll be pushing back the fork height while I work out a patch. Sorry, I just saw this now.

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

Does the wallet exit properly? The NDEBUG define I just removed from CMakeLists.txt, that we apparently inherited from upstream was blocking a lot of would-be exceptions it looks like... and in some instances things are being thrown up the stack by a different define in p2p Removal mentioned here was improper.

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

In the past, I've always recommended people use the --restore-deterministic option, and follow the prompts, which request entering the seed phrase one line at a time. That's the same behavior you're saying would be best to include with the --electrum-seed option as well, correct?

@who-biz who-biz self-assigned this May 10, 2019
@who-biz who-biz added advisory please read bug Something isn't working fast-track Enhancements that need treated as important, just behind bugs in priority. labels May 10, 2019
@leto
Copy link
Author

leto commented May 10, 2019

@who-biz I didn't really know how to recover/import my wallet to a new machine, so I just read --help and after doing the wrong thing various times, I figured out something that worked.

I was confused about "electrum seed", because I used the seedphrase generated by a blur full node, but I either failed to use --restore-deterministric or it wasn't clear that I should have used that.

If I should just use that, maybe no work is needed. What is the difference between --restore-deterministric and --electrum-seed ?

@leto
Copy link
Author

leto commented May 10, 2019

@who-biz typing exit did work correctly. I am on the f31b8ac commit of master branch

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

If I should just use that, maybe no work is needed. What is the difference between --restore-deterministric and --electrum-seed ?

As nearly as I can tell, not much. Electrum style seeds are used as default in our codebase, by inheritance from XMR, since launch. From that information, they should be the same command.

So the fix, if that's the case, would just be a matter of removing the --electrum-seed option completely, and updating the help text since you've let me know that its lacking proper instruction.

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

Something that was waiting down the pipeline as well, was separating the viewkey and spendkey so that the viewkey is not deterministic (derived from the spendkey, of which the mnemonic is a representation). I think this is a disavowal of established best-practices, and to the best of my understanding, was eliminated because a single mnemonic couldn't be used to generate two distinctly unrelated keypairs. That was a problem for MyMonero mainly, and general usability. But since, we don't have any plans to use any solutions that are like MyMonero's requirement to divulge your keys... I dont see anything but good that could come from a reversion to that practice. A loss in usability maybe, but a worthwhile one for a gain in security.

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

[reverted in master]

@who-biz
Copy link
Contributor

who-biz commented May 10, 2019

@who-biz I didn't really know how to recover/import my wallet to a new machine, so I just read --help and after doing the wrong thing various times, I figured out something that worked.

I was confused about "electrum seed", because I used the seedphrase generated by a blur full node, but I either failed to use --restore-deterministric or it wasn't clear that I should have used that.

If I should just use that, maybe no work is needed. What is the difference between --restore-deterministric and --electrum-seed ?

Lastly, you can find the procedure for the seed phrase restore here #24... with the possible issues in ChaCha20Poly1305, I would recommend just sending those coins to a new wallet, while further investigation into that matter takes place

@who-biz who-biz added enhancement New feature or request resolved Issue has been resolved and removed bug Something isn't working fast-track Enhancements that need treated as important, just behind bugs in priority. labels May 14, 2019
@who-biz
Copy link
Contributor

who-biz commented May 14, 2019

The help text from this original issue has been amended in commit 8a15a76 & in 14777ad. Electrum seed option and prompts (when missing for other options) have been removed.

I am going to leave this issue open, close this, and open new issues for documenting the prospective changes within keypair separation, and chacha20poly1305.
Amended help text:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advisory please read enhancement New feature or request resolved Issue has been resolved
Projects
None yet
Development

No branches or pull requests

2 participants