Skip to content
This repository was archived by the owner on Sep 12, 2023. It is now read-only.

Seedphrase based wallet that can easily be backed up #865

Closed
da-kami opened this issue Dec 10, 2021 · 7 comments · Fixed by #908
Closed

Seedphrase based wallet that can easily be backed up #865

da-kami opened this issue Dec 10, 2021 · 7 comments · Fixed by #908
Assignees

Comments

@da-kami
Copy link
Contributor

da-kami commented Dec 10, 2021

This is something we should really get on Umbrel because it is hard to to achieve it there.

@da-kami da-kami changed the title Seed backup in the UI Seephrase based wallet that can easily be backed up Dec 12, 2021
@thomaseizinger thomaseizinger changed the title Seephrase based wallet that can easily be backed up Seedphrase based wallet that can easily be backed up Dec 12, 2021
@rishflab rishflab self-assigned this Dec 14, 2021
@rishflab
Copy link
Contributor

rishflab commented Dec 14, 2021

Proposed solution: backup/restore through the UI

Features

  1. User can put seed phrase in UI to restore a wallet.
  2. User can let UI create a new wallet, displaying a seed phrase once.
  3. User can ask UI to show seed phrase anytime they are "logged in" (not sure about this one)

Tasks

note: pitfall tasks are in bold

Daemon

  • delay wallet init until seed bytes from rest endpoint arrives (this might drastically change the architecture/actor initialisation)
  • restore from wallet from seed bytes REST endpoint

UI

  • Generate 24-word seed phrase
  • Convert seed phrase to bytes
  • Restore wallet from inputted seed phrase
  • Generate wallet/seed phrase and show seed phrase
  • Prompt/component to restore/generate wallet
  • Reveal seed phrase button/seed phrase display in wallet component
  • Store/wipe stored seed phrase securely?

Follow up features/ Not in scope

  1. User can ask UI to show the descriptor allowing user to export to third party wallets

Daemon

  • export descriptor endpoint
  • export wallet from descriptor methods to wallet actor

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Dec 14, 2021

Thanks for the initial design!

Here are some ideas:

  1. I think we should generate the new seed phrases in the daemon and hand it out to the actor and not generate it in the frontend.
  2. As an initial step, I would change our wallet actor to be initialized by a seed phrase and let it derive the descriptor internally. That is an API we will certainly need, regardless of how we design things around it. For now, you can derive the seed-words deterministically from the seedfile. This can be its own PR already.
  3. Delaying the wallet initialization is a bit trickier because we currently do this as part of the actor system startup.
    • To delay instantiation of the wallet actor, we need to keep its xtra::Context around (we can later call Context::attach with an instance of the actor)
    • In the REST endpoint, we only have access to the addresses of actors but not their contexts. Instead of extending this, I would go towards Too many details about the actor system leak into rocket #774. With that, we can rocket.manage the entire actor system and thus provide an API on it along the lines of TakerActorSystem::initialize_wallet_from_seed_phrase(seed_words: &str) which will internally create a wallet actor and attach it to the context.
    • All other actors shouldn't care much because they only talk to the address of the wallet::Actor which will simply not be connected up until we initialize it.
    • If the user initializes the wallet from a seed phrase, we need to store that one on disk. For now, I would do so un-encrypted. We also store the seed unencrypted currently, so this is not actually a degradation in features.
    • Upon start up, I would check if the file is present. If yes, load the seed words from there, otherwise generate random ones and initialize the wallet with it.
    • We also need to make sure to stop the existing wallet::Actor upon re-initialization, otherwise we will have two running :D

Let me know what you think of this. It should be possible to split quite a few independent PRs out of this. I would do the UI as the last step.

@bonomat
Copy link
Collaborator

bonomat commented Dec 14, 2021

Thanks for writing this up. Two more thoughts:

  • We need to think about if the taker's pubkey depends on the seed or not. If it does, we need to make sure that all CFDs are closed before allowing to change the seed otherwise Authorize taker #576 becomes a problem.
  • even though not explicitly mentioned: I see this feature exclusively for the taker

@thomaseizinger
Copy link
Contributor

We need to think about if the taker's pubkey depends on the seed or not. If it does, we need to make sure that all CFDs are closed before allowing to change the seed otherwise

Good point. I think we should tie everything to the seed-phrase eventually. In an initial step, I would probably have both, just to not bloat the scope of this ticket.

even though not explicitly mentioned: I see this feature exclusively for the taker

Agreed.

@rishflab
Copy link
Contributor

1. I think we should generate the new seed phrases in the daemon and hand it out to the actor and not generate it in the frontend.

2. As an initial step, I would change our wallet actor to be initialized by a seed phrase and let it derive the descriptor internally. That is an API we will certainly need, regardless of how we design things around it. For now, you can derive the seed-words deterministically from the seedfile. This can be its own PR already.

I am not sure about whether or not the backend should learn about the 24 word mnemonic. Is it standard practice to re-use this mnemonic across different products?

I opted for front end because I wanted to minimize the process boundaries the seed phrase crosses for security purposes. Would REST endpoint to generate a wallet also take the seed phrase as an input? Doing a irreversible conversion to a byte seed before sending it across process boundaries will be an additional layer of security. This should make it safer for users who have memorized mnemonic they reuse.

From what I have read/seen it seems like being able to derive the mnemonic from a 256/512 bit seed is anti-pattern.
https://tzumby.github.io/bip32_cheat_sheet/

@rishflab
Copy link
Contributor

rishflab commented Dec 15, 2021

We concluded that it is ok to send the seed words around as it is not standard practice to re-use them.

Given that the initial tasks to do in order are:

  • "Delay" wallet initialization/allow wallet re-initlisation (allow seed/seed words to be passed in after startup)
  • Update wallet to use seed words instead of bytes
  • Implement BIP 39 using these seed words

@rishflab rishflab linked a pull request Dec 16, 2021 that will close this issue
@bors bors bot closed this as completed in #908 Dec 18, 2021
@thomaseizinger
Copy link
Contributor

I am closing this in favor of #978, plus I think we should remove the HTTP API again that was added in #908.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants