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

lnrpc+walletunlocker: extend wallet creation to allow user generated entropy + entropy restore (cipherseed) #719

Merged
merged 5 commits into from Mar 5, 2018

Conversation

@Roasbeef
Copy link
Member

Roasbeef commented Feb 2, 2018

New Functionality

In this PR, we extend the initial lncli create command to allow the user to specify their own entropy when creating the wallet. This capability means that it'll be possible for a user to restore an existing on-chain wallet given their initial source of entropy. Additionally, the lncli create command will now also return a BIP 39 mnemonic generated from lnd's generated entropy in the case that the user opts not to specify their own.

Internally, when accepting a mnemonic from the user, we'll convert that into the original entropy bytes using BIP 39. From there we'll map that to an HD wallet seed and initialize the internal wallet using that value. If the user doesn't specify a seed, we'll generate our own entropy (always 256-bits), and send the generated entropy over RPC. This final step allows any frontends to lnd to display the entropy as 24-word mnemonic for easy backup.

New lncli create workflow

What follows is an interactive session with this PR that illustrates the ability to create a new wallet, then restore the wallet using the return mnemonic:

⛰  lncli --rpcserver=localhost:10017 --macaroonpath=test_lnd/admin.macaroon create
Input wallet password:
Confirm wallet password:
Do you have an existing 24-word BIP 39 mnemonic you want to use? (Enter y/n): n

!!!YOU MUST WRITE DOWN THIS SEED TO BE ABLE TO RESTORE THE WALLET!!!

Generated mnemonic seed: field soup wing canvas survey borrow story analyst useless clap before cheap giggle middle color visa cabbage shell vessel rent deposit exchange coil sample

!!!YOU MUST WRITE DOWN THIS SEED TO BE ABLE TO RESTORE THE WALLET!!!

lnd successfully initialized!

  rpc-seed ✘ ✭  roasbeef  ...github.com/lightningnetwork/lnd 
⛰i  lncli --rpcserver=localhost:10017 --macaroonpath=test_lnd/admin.macaroon newaddress p2wkh
{
    "address": "sb1qkdqvqruhlgep06qq6ga705pkgn8q9gka8ee6lj"
}

<HERE WE CLEAR OUT THE DATA OF LND> 

  rpc-seed ✘ ✭  roasbeef  ...github.com/lightningnetwork/lnd 
⛰ lncli --rpcserver=localhost:10017 --macaroonpath=test_lnd/admin.macaroon create
Input wallet password:
Confirm wallet password:
Do you have an existing 24-word BIP 39 mnemonic you want to use? (Enter y/n): y

Input your 24-word mnemonic separated by spaces: (we don't echo the seed on the command line)

lnd successfully initialized!

  rpc-seed ✘ ✭  roasbeef  ...github.com/lightningnetwork/lnd 
⛰ lncli --rpcserver=localhost:10017 --macaroonpath=test_lnd/admin.macaroon newaddress p2wkh
{
    "address": "sb1qkdqvqruhlgep06qq6ga705pkgn8q9gka8ee6lj"
}

Implementation Details

One point of discussion is whether to use the user pass phrase when generating the final seed from the mnemonic. As is now, the contents of the wallet database are encrypted with the passphrase. As is this means that one can't access their wallet (before this PR) if they forgot the passphrase. When generating their seed (atm) we'll use the wallet passphrase as input into the KDF. As a result, if the user forgets their wallet passphrase they won't be able to re-generate the seed.

On one hand, by using the passphrase, this means that for a given mnemonic, any passphrase generates a "valid" wallet. On the other hand, if the user forgets their passphrase, then they won't be able to restore the same wallet.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 2, 2018

Also looking for feedback w.r.t to the wording displayed on lncli, and also the question as to whether or not we should use the user's passphrase to generate a seed.

lnd.go Outdated
case initMsg := <-pwService.InitMsgs:
password := initMsg.Passphrase

// We'll now check to see fit eh user provided any entropy

This comment has been minimized.

Copy link
@alexbosworth

alexbosworth Feb 2, 2018

Contributor

*if the

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 2, 2018

Will also add an initial e2e integration test to start to exercise the basics of on-chain funds recovery.

@junderw

This comment has been minimized.

Copy link
Contributor

junderw commented Feb 2, 2018

  1. 24 words is overkill. 12 words is sufficient as the security of ECDSA Koblitz curves is n/2 of keysize, so 256 bit key size gives 128 bits of security. 12 words and 24 words gives no substantial difference in security. Almost all other wallets use 12 words as well. The user should be able to specify 12 or 24 words if they would like.
  2. Using the encryption password as the BIP39 salt passphrase will get hairy when a user changes their encryption password...... the user will be confused. Copay struggled with the difference between encryption password and salt passphrase in the UI, users would always get confused. I would recommend AGAINST using the salt passphrase feature without the user explicitly being aware of the implications.
@rodasmith

This comment has been minimized.

Copy link
Contributor

rodasmith commented Feb 2, 2018

The BIP 39 passphrase is a great security option for people who store their mnemonic word seed on physical media (e.g. paper or metal) and the passphrase in their brain.

lnd.go Outdated
// mnemonic to the HD seed.
//
// TODO(roasbeef): don't use passphrase?
pass := string(password)

This comment has been minimized.

Copy link
@rodasmith

rodasmith Feb 2, 2018

Contributor

It is kind of confusing to conflate the wallet encrypting password here with the BIP 39 passphrase. Seems like this needs to be an explicit passphrase, ideally supplied just after the 24-word mnemonic.

@Roasbeef Roasbeef changed the title lnrpc+walletunlocker: extend wallet creation to allow user generated entropy + entropy store (BIP 39) lnrpc+walletunlocker: extend wallet creation to allow user generated entropy + entropy restore (BIP 39) Feb 2, 2018
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 2, 2018

Thanks for the feedback concerning the usage of the wallet passphrase. Upon further deliberation, I've concluded that we should actually ditch BIP 39 all together. BIP 39 lacks two important features that are necessary in a modern seed/mneonnic format:

  1. A versioning system
  2. A birthday

The versioning system is required so we can bind the key derivation schema to the version of the seed, allowing future wallet types to properly detect any mismatched between the default key derivation schema of the software and the seed the software has been presented.

A wallet birthday is required so we can properly handle imports of seed format without relying on a 3rd party API service to locate the earliest/oldest instances of address usage.

@dabura667

This comment has been minimized.

Copy link

dabura667 commented Feb 3, 2018

  1. BIP39
  2. Electrum >=2.0 mnemonic
  3. LND mnemonic

And then... there was three.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 3, 2018

Well, the thing is BIP 39 is just flat out lacking. Our needs are very specific, as we'll be using this to deterministically derive all keys we ever use within a channel. This isn't covered under BIP 44, or any of the existing BIPs that govern key derivation. We'll likely document this and put it forth as a standard once it has been implemented (for LN key derivation for channels, including the shachain root derivation).

Even the electrum seed doesn't have a birthday. For wallets that rely on a 3rd party service API to query when the address was first used, this isn't an issue. However, in our case for neutrino we won't be relying on such as service. As a result, we need the seed to be able to tell us the lower bound on the first usage of any addresses or keys.

BIP 39 is rather dated at this point, and should be replaced IMO.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 3, 2018

Here's my proposal for a new seed format/mapping

Alright, so I've thought of a new seed format/mapping that meets all the desired traits above, and also has some cool properties of it's on. The tentative name is cipherseed:

  • A "plaintext" seed is defined as s = version || birthday || entropy.
    • where version is a 1 byte version
    • where birthday is a 8-byte unix timestamp
    • where entropy is 16-bytes of bytes generated from a CSPRNG
  • We then convert this plaintext seed into the ciphertext that will be mapped to mnemonic as follows:
    • Let password be "" if no password is selected, and otherwise a user chosen passord
    • Define k as an invocation of argon2 (assume "suitable" parameters) with password as the input.
    • Using aez (an arbitrary length tweak-able block cipher) we'll encrypt the seed description s with k, the result being the precise length of the ciphertext. Due to the properties of aez if before encrypt s with prepend N (N=8) bytes of redudnancy, then if after decryption, we verify that those bytes are in-tact, then this construction is actually an AEAD, meaning we achieve integrity+confidentiality. As a result, we don't need an explicit checksum, as if the ciphertext is altered, those bytes won't be there upon decryption.
    • Finally, we'll map those bytes of the ciphertext c to a mnemonic using the BIP 39 word list.

The above allows us to ensure the seed description is kept it tact, authenticates the original seed description, and also allows optional usage of a password to enable the mnemonic being kept in plain site. We can modify the level of redundancy we add to tune the number of resulting words, and also the security level of the corresponding MAC construction.

A cool trait of this scheme is that the mnemonic is actually encoding a ciphertext of the initial seed description. By mapping this description through an arbitrary length block cipher, we extract a permuted set of bytes that leaks no information concerning the birthday or version. As the aez is a strong psuedo random permutation, no info of the original plaintext is leaked.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 3, 2018

Building off of the above, we'll need a way to deterministically derive each of the keys we use within a transaction. There are 5 key types for channels that we're concerned with:

  1. multi-sig keys
  2. revocation base points
  3. htlc base points
  4. delay base points
  5. payment basepoints

Using the seed derived from the above cipherseed format, we'll assign distinct branches for each key type. Within a branch a counter will be used to rotate forward to generate a key as needed. The upside of this scheme is that we can then create static descriptions of channels in a standard format to use for backup/recovery. In place of the public key, the format will simply store an index, partitioned from the appropriate sub-tree.

Combined with the seed format above, this is a complete suite for recovery of on-chain funds, as well as keys used in any particular channel, given a static description of the channel.

@junderw

This comment has been minimized.

Copy link
Contributor

junderw commented Feb 3, 2018

BIP 39 is rather dated at this point, and should be replaced IMO.

I agree. But... obligatory: https://xkcd.com/927/

But yeah if we want to do it this way, I HIGHLY suggest making a BOLT so that at LEAST we will have some portability between LND based, CLightning based, and Eclair based client apps.

Some concerns:

  1. version doesn't need to be limited to 1 byte really. we could define the version as a varint so that any value up to 252 will be fine with one byte, but we could support more in the future.
  2. 16 bytes of entropy is sufficient for entropy
  3. version (1-9 bytes) || bday (4 bytes) || entropy (16 bytes) || redundancy (4-12 bytes) would make 33 bytes which is exactly a multiple of 11 bits and can encode 2048 word lists. Resulting in 24 word phrases.
  4. We should probably commit to the bday... plus there should be some light hash iteration... so perhaps pbkdf2(bday || entropy) with a reasonable iteration count. This hash will be the (BIP32) seed used to start the BIP32 process.
  5. One of the biggest problems with BIP39 was that 99% of wallets only supported English wordlists. In Japan, I saw a HUGE number of newbies coming to the meetup saying "OMG I CAN'T RECOVER MY WALLET AFTER RESETTING MY IPHONE! MY PHRASE SHOULD BE RIGHT." and their phrase is filled with words nowhere even close to words on the list/non-words. (remember, English is not their first language and a lot of newbies don't understand the importance of the phrase until they need to recover) In order to fix this, I implemented Japanese wordlist for BIP39, and got it into Copay and Breadwallet. This DRASTICALLY DECREASED the incidence... However, the drawback with supporting multiple languages is that every wallet needs every wordlist to make it work... BIP39 was supposed to fix that by suggesting wallets "check if the wordlist can be found, then if it can be found, check the checksum. If either of those fail, ask the user if they want to use the phrase as is anyways?" (this will allow Japanese phrases to be recovered even if the wallet doesn't know the wordlist. Only drawback being can't check the checksum. However, all wallets decided to HARD FAIL at either of those points... removing that "feature"... It would be nice to have a method that didn't require the wordlist to decode... not sure if that's possible. I guess just make the BOLT STRONGLY SUGGEST supporting every wordlist possible is enough... meh...
  6. channel.db needs to be backed up and can't be recovered from the seed..................... I think that the BOLT should also suggest that all wallets supporting the BOLT should offer a way to "export" the channel info (aka info not deterministically derived from the seed) and that the export should encrypt all information using a leaf key deterministically derived from the seed. Then I could maybe set a cronjob to periodically export the encrypted channel.db to my dropbox folder or something.
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 3, 2018

I HIGHLY suggest making a BOLT so that at LEAST we will have some portability between LND based,

Up to them if they want to support it. I'd favor going ahead with an implementation now, in order for us to have a scheme set for recoverability by 0.4-beta. Can't be concerned with preemptively trying to make it a standard if it'll delay our release. But yes, eventually it would be nice if this was a standard.

version doesn't need to be limited to 1 byte really.

Do we really need more than 255 versions? It must be fixed length as we need to tune the redundancy for our security level.

16 bytes of entropy is sufficient for entropy

With 16 bytes, our redundancy is now 12 bytes. We can shift in either direction to achieve diff security levels. We may bump this to 2 bytes, but in any case it needs to be fixed length.

We should probably commit to the bday... plus there should be some light hash iteration... so perhaps pbkdf2(bday || entropy) with a reasonable iteration count. This hash will be the (BIP32) seed used to start the BIP32 process.

No need to commit to the bday. We're creating an authenticated cipher, it basically rolls a MAC check all into one upon decryption. No need to stretch anything either, as we already use argon2 for the KDF. The BIP 32 seed is actually just the initial entropy. No need to do any other mapping, BIP 39 didn't really make much sense in its construction.

One of the biggest problems with BIP39 was that 99% of wallets only supported English wordlists.

This format doesn't define a pre-defined word list at all. We're using the mnemonic to encode a cipher-text, not the entropy directly. An explicit checksum isn't necessary as if words are transposed, or a mistake is made in the mnemonic mapping, then you'll get a decryption failure, rather than mapping to some possibly empty wallet.

channel.db needs to be backed up and can't be recovered from the seed

Yes, this will be the role of the static channel descriptions. The descriptions themselves could also be encrypted with the same (optional) password and format as the seed description.

@bretton

This comment has been minimized.

Copy link
Contributor

bretton commented Feb 3, 2018

Wallet recovery: can I make a suggestion that something other than a password field be used as this doesn't echo back any characters/words to the user.

This is fine if copy-pasting the 24 words to the prompt, but problematic if typing word by word from a printed store such as paper or cryptosteel record.

In secure environments where the clipboard is disabled due to fears of clipboard hijacking, copy-paste is also not an option.

The seed gets echoed to tty during generation, so I don't see a major problem in seeing the words when inputting them back for wallet recovery.

They won't be stored in the shell history either as the shell records the initial command, not the interaction with it, unless some sort of recording setup.

Dealing with 24 words and spaces, without any visual feedback on where possible input errors might be, is a bit worrying from a user experience perspective.

This issue is addressed on mobile phones where characters are briefly seen before being obscured, and with normal GUI apps the entire seed phrase is visible.

A password type field is not used in other situations, or provides some sort of input-error-catching during entry.

@junderw

This comment has been minimized.

Copy link
Contributor

junderw commented Feb 3, 2018

Yes, this will be the role of the static channel descriptions. The descriptions themselves could also be encrypted with the same (optional) password and format as the seed description.

I don't see the point in adding another password in the mix. The channel.db can not be used with any other wallet (/entropy/BIP32masterkey/whatever/) so why even bother encrypting it with anything other than the BIP32 master key or a specific path derived from it?

(optional) password unlocks the wallet seed, which derives the channel.db key, which decrypts channel.db... makes sense.

As for the encoding, I am indifferent. But I assume it will be much longer than 33 bytes... so while 24 words might be fine... 5000 words would give you carpal tunnel syndrome 🤣 at that point, people will save it to txt files anyways, so an encrypted blob file should be fine. channel.db etc.

@meshcollider meshcollider added the wallet label Feb 4, 2018
@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Feb 6, 2018

I'm currently feeling fairly positive about the combination of (variable length ciphertext, AE mode instead of (checksum or yucky block size + padding) for validity check, ciphertext as encoding, birthday, version).

(Yeah of course no one wants to xkcd themself. But meh. This is not a tight consensus kind of problem. The word list could even remain the same. Sometimes conforming is actually a bad thing, see e.g. the rationale behind BIP49, although I get why people don't agree).

To achieve the crypto properties, I'm wondering about the choice of AEZ, although it does give you what you need, it does seem rather new/unsupported. Does OCB not fit the bill (with AES say)? AFAICT the patent thing shouldn't be an issue.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 7, 2018

To achieve the crypto properties, I'm wondering about the choice of AEZ, although it does give you what you need, it does seem rather new/unsupported.

Yeah the drawbacks of AEZ are: it's newer and not super straight forward to implement (though we do have an impl we can use. On the otherhand, it may very well end up wining the CAESAR competition, and it likely to be adopted by Tor to thwart known tagging attacks.

Does OCB not fit the bill (with AES say)? AFAICT the patent thing shouldn't be an issue.

Hmm, possibly. I don't think it can be used in the same manner (enciphering/deciperhing) which allows use adopt what's the equivalent of an 8-byte MAC (or even tune it as we wish). Also, it seems there'e a set of restricted "free" patents for usage of OCB. However I'm a bit reluctant to use any patented software within the project...

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Feb 7, 2018

I don't think it can be used in the same manner (enciphering/deciperhing) which allows use adopt what's the equivalent of an 8-byte MAC (or even tune it as we wish)

So from the AEZ paper, I think I know what you're referring to when you say "can't be used in .. same ..encipher/decipher", but I don't think there's a significant difference here (there would be for modes like CBC of course):

The ciphertext is equal in length to the plaintext for any length, while the tag for authentication (~=MAC) is allowed to be of variable size such as 64 bit (so same as your case), with its length being a security tradeoff. There's no reason to also include the nonce in the ciphertext as it doesn't have to be secret or random (so can just be a counter). So you just have ciphertext||tag

But yeah patents; it seems like it's not a problem, but who wants to even think about it.

Looked again at GCM, it seems, unless I'm missing something, that you get basically exactly the same properties without any patent concerns (which is presumably why AES-GCM is widely deployed in TLS and OCB isn't); the IV is the nonce which could be a counter. Not sure, maybe missing an important detail, but there are probably quite a lot of people that'd know about that :)

I guess you will stick with AEZ, but it does slightly worry me that it's not widely used.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 7, 2018

With aez, there's no explicit MAC. It comes in the from of "cipher-text expansion". So you can specify a number of bits of redundancy that'll be verified upon decryption. It's even possible to exploit existing redundancy in the plaintext itself (like a message header or something) in order to reduce the number of redundant bits appended before encryption. In the above description, we don't use an explicit nonce at all, as we assume uniqueness of the plaintext.

For traditional cipher modes, we'd need to truncate the max to avoid having too many encoded words (with the params we have in mind, we'd have 24-words). Also for traditional modes, we'd need to add additional padding to ensure the last block was the same width as block cipher, further blowing up the size of the ciphertext.

You can think of aez as a sort of format preserving encryption. We can tightly control the size of the space the plaintexts are mapped to in order to minimize overhead and not force users to type 50 words or something ;)

@AdamISZ

This comment has been minimized.

Copy link
Contributor

AdamISZ commented Feb 8, 2018

With aez, there's no explicit MAC.

Of course.

Also for traditional modes, we'd need to add additional padding to ensure the last block was the same width as block cipher, further blowing up the size of the ciphertext.

Yes, I referred to that (CBC as example), I was specifically talking about OCB (and possibly GCM) because they don't have this problem; they are block ciphers that function as stream ciphers, and do not require more ciphertext than plaintext. To give an example, I can encrypt 53 bytes of plaintext in AES OCB and get 53 bytes of ciphertext. The tag functions as authentication so you append ciphertext||tag ; this, as I was trying to assert above, is not really different from appending redundancy before the enciphering step. And it's tunable so e.g. 8 bytes of it is fine. So the length arithmetic would be basically the same, with N bytes redundancy in tag instead of plaintext.

However your point that you can exploit existing redundancy in the plaintext format to reduce the length expansion required for authentication is a good one, I think.

Btw this does remind me a bit of the old "cryptographic doom principle" of Marlinspike, aka the old mac-then-encrypt vs encrypt-then-mac. I think there's a subtlety here I'm missing though.

@AbdussamadA

This comment has been minimized.

Copy link

AbdussamadA commented Feb 26, 2018

An explicit checksum isn't necessary as if words are transposed, or a mistake is made in the mnemonic mapping, then you'll get a decryption failure, rather than mapping to some possibly empty wallet.

A decryption failure doesn't tell you whether you made a mistake writing down or entering the seed mnemonic or you forgot the password. This can be an important distinction when you are trying to recover your wallet.

Copy link
Collaborator

cfromknecht left a comment

Nice! Getting close to not having to completely wipe our nodes before upgrading :) Couple small things, but changes look solid on first pass!

// We'll now prompt the user to enter in their 24-word
// mnemonic.
fmt.Printf("Input your 24-word mnemonic separated by spaces: ")
mnemonic, err := terminal.ReadPassword(int(syscall.Stdin))

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

I don't think this requires ReadPassword since the mnemonic is itself a ciphertext

This comment has been minimized.

Copy link
@junderw

junderw Feb 27, 2018

Contributor

Also, it's hard to double check you typed it in correctly. No one will copy paste, and typing a long phrase is screaming for typos... so being able to see what you typed on the screen is a must... if there's a way to securely delete the stdin/terminal window from the app after they hit Enter that would be best.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

just remembered that in this context this is actually a bip 39 mnemonic, so it is semi private

@@ -757,8 +759,26 @@ func listPeers(ctx *cli.Context) error {
}

var createCommand = cli.Command{
Name: "create",
Usage: "used to set the wallet password at lnd startup",
Name: "create",

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

I know this command was first created in a previous PR, but just wanted to propose renaming to init? i think it's more descriptive of how/what the command does

Name: "unlock",
Description: `
The unlock command is used to decrypt lnd's wallet state in order to
start up. This command MUST be run after booting up lnd before it's

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

Add caveat about --noencryptwallet?

lnd.go Outdated
return nil, nil, err
}

// If we generated the seed to create he wallet ourselves, then

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

create the*

@@ -11,13 +11,49 @@ import (
"gopkg.in/macaroon-bakery.v2/bakery"
)

// SeedEntropySize is the default number of bytes that will be used to generate
// a seed if the user hasn't specified one themselves. We'll also enforce that
// the see is at least this large

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 27, 2018

Collaborator

seed* is at least


fmt.Println("Generated mnemonic seed:", mnemonic)

fmt.Println("\n!!!YOU MUST WRITE DOWN THIS SEED TO BE ABLE TO " +

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 28, 2018

Collaborator

Was thinking a little more on how we display the mnemonic to the user in lncli. There's a lot going on there, and think it needs to be more obvious as to what exactly needs to be copied. I would propose printing out the mnemonic in a format similar to text encodings for keys/certs:

-----BEGIN LND CIPHER SEED-----
field   soup     wing   canvas
survey  borrow   story  analyst
useless clap     before cheap
giggle  middle   color  visa
cabbage shell    vessel rent
deposit exchange coil   sample
------END LND CIPHER SEED------

I think this will greatly reduce the errors people make in writing down the seed. Humans are best at remembering things in 3's and 4's, plus the formatting allows users to check if they're missing a word in each line, instead of hoping they correctly copied all 24 words in a single run-on sentence.

This comment has been minimized.

Copy link
@junderw

junderw Feb 28, 2018

Contributor

I would also say to add numbers...

Japanese users sometimes try to write top to bottom first.

-----BEGIN LND CIPHER SEED-----
 1. field    2. soup      3. wing    4. canvas
 5. survey   6. borrow    7. story   8. analyst
 9. useless 10. clap     11. before 12. cheap
13. giggle  14. middle   15. color  16. visa
17. cabbage 18. shell    19. vessel 20. rent
21. deposit 22. exchange 23. coil   24. sample
------END LND CIPHER SEED------
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Feb 28, 2018

@AbdussamadA good point, we can improve. I'll be updating the aezeed PR, so we'll be able to detect a case of an invalid mnemonic, and then an invalid password distinctly. This provides the ultimate level of protection against a loss of user funds.

@Roasbeef Roasbeef force-pushed the Roasbeef:rpc-seed branch from afc1d6e to 7be0d22 Mar 1, 2018
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 1, 2018

About to push a new version with the feedback incorporated: we'll now display the mnemonic (so echo it back), and also the seed is formatted as such:

Input wallet password:
Confirm wallet password:
Do you have an existing 24-word BIP 39 mnemonic you want to use? (Enter y/n): n

!!!YOU MUST WRITE DOWN THIS SEED TO BE ABLE TO RESTORE THE WALLET!!!

-------------BEGIN LND CIPHER SEED-------------
1. carpet   2. pluck   3. bring   4. typical
5. tunnel   6. bring   7. affair   8. until
9. curtain  10. pole  11. light  12. seed
13. naive  14. federal  15. win  16. tube
17. distance  18. ten  19. ecology  20. ethics
21. dragon  22. mask  23. buddy  24. gather
--------------END LND CIPHER SEED--------------

!!!YOU MUST WRITE DOWN THIS SEED TO BE ABLE TO RESTORE THE WALLET!!!

lnd successfully initialized!

Twiddled for a while to try to get up to line up really nicely column wise, but this current formatting is a stop gap for now. Here's a playground to adjust some of the spacing: https://play.golang.org/p/cPBPo8KgN7m

@Roasbeef Roasbeef force-pushed the Roasbeef:rpc-seed branch from a3eeab8 to eb3df2a Mar 3, 2018
@dabura667

This comment has been minimized.

Copy link

dabura667 commented Mar 3, 2018

Which part of the two step does the WALLET encryption password get set? (not the aezeed optional passphrase)

During the ACK step, I hope?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 3, 2018

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Mar 3, 2018

@Roasbeef the user doesn’t need to call genseed manually, right? Sounds like they can though?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 3, 2018

@dabura667

This comment has been minimized.

Copy link

dabura667 commented Mar 3, 2018

AWESOME!!!!!!!

Killing it! Totally...

Now all we need is a method for encrypting+exporting channel.db and all info for recovering the node in a fatal crash scenario are covered.

  1. The export procedure will use a specific path of the HD key's private key to AES encrypt the channel.db before exporting.
  2. The import function will use the same path to decrypt after importing, and try to merge the current channel.db (if any) and the imported db (where later state takes precedence)
  3. Maybe offer an option in config/somewhere to set auto-backups for channel.db...
  4. Definitely offer a CLI command so people can at least make a cronjob. (ie. lncli backupchannels /path/to/my/backup etc.)
@junderw

This comment has been minimized.

Copy link
Contributor

junderw commented Mar 5, 2018

Great feature!

I agree an export/import feature for channel.db that encrypts/decrypts with a key from the wallet and will merge latest-takes-precedence when importing would be great.

Roasbeef added 5 commits Feb 2, 2018
In this commit, we revamp the WalletUnlocker service to now have a
two-stage init process.

The first (optional) is for the user instantiating a new lnd instance to
call the GenSeed method with an optional aezeed passphrase. The response
to this will be a freshly generated aezeed mnemonic along with the
original enciphered seed.

The second step will be the actual wallet initaliztion. By separating
this step from seed generation, UI's will be able to ensure that the
user has written down the seed, before proceeding and committing the
seed to the internal wallet. The new method InitWallet accepts a wallet
passphrase, the aezeed mnemonic, and the optional passphrase.
In this commit, we extend the UnlockerService to account for the new
changes in the lnrpc definition. Setting up the daemon for the first
time is now two step process: first the user will generate a new seed
via the GenSeed method, then the user will present this new seed (and
optional pass) to the InitWallet method which then will finalize the
wallet creation.

This two step process ensures that we don't commit the wallet changes
in the case that the user doesn't actually "ACK" the new seed.

In the case that the user already has an existing seed, they can
re-enter it and skip straight to the InitWallet step.

We also update the tests to account for the new API changes.
…ntropy

In this commit, we extend the initial wallet creation set up case with
the goal of giving the user the ability to restore a prior wallet from
seed, or obtain the mnemonic for a newly generated wallet.

As the WalletUnlocker has been extended to allow passing a user source
of entropy, if this is detected, then we’ll use BIP39 to covert it into
an HD wallet seed. Otherwise, we’ll generate our own entropy, then
convert that into the wallet seed.

In order to make this change, we’ll now manually create the default
wallet ourselves. In the case that the user didn’t provide their own
seed, we’ll send the seed we generated back to the user. This will allow
frontends to display the newly generated seed to the end user.
…ting wallet

In this commit, due to the recent changes within lnd itself, it may be
possible that a wallet already exists when the wallet has been signaled
to be created. As a result, *always* open the wallet ourselves, but
allow an existing wallet to already be in place.
In this commit, we extend the `lncli create` command to allow users to
specify their own side (if they want). In the case that the user
*doesn’t* specify their own seed, we’ll return the entropy generated by
the wallet in a 24-word mnemonic format for easy backup.

With this change, it’s now possible for users to restore an existing lnd
wallet seed.
@Roasbeef Roasbeef force-pushed the Roasbeef:rpc-seed branch from eb3df2a to 3356a37 Mar 5, 2018
Copy link
Collaborator

cfromknecht left a comment

I like the last minute change to make genseed stateful, user onboarding flow is now much clearer! LGTM 🌱

@Roasbeef Roasbeef merged commit 61a7556 into lightningnetwork:master Mar 5, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 64.895%
Details
@Roasbeef Roasbeef changed the title lnrpc+walletunlocker: extend wallet creation to allow user generated entropy + entropy restore (BIP 39) lnrpc+walletunlocker: extend wallet creation to allow user generated entropy + entropy restore (cipherseed) Mar 11, 2018
@homakov

This comment has been minimized.

Copy link

homakov commented Mar 11, 2018

@dabura667 why do you think it should be cron for auto backup? If you missed a single state update and then crashed, you lose all of your money. It must be something ongoing after every payment.

So it must support backup of every new state out of box.

@dabura667

This comment has been minimized.

Copy link

dabura667 commented Mar 11, 2018

It must be something ongoing after every payment.

And we could do that if we had an easy command like:

lncli backupchannels /path/to/backup/channels-yyyymmdd-hhmmss.db

and the equivalent in GRPC.

No, copying channel.db from the folder directly does not give certain guarantees.

So I could say in my app:

Every time I send a payment, immediately after, make a backup.

Or I could say in my server:

cron please run the backup command every 5 minutes

Or I could say anything I want.

Currently we have no method for backing up channel.db... if my disk blows up, ALL my channels are borked, and even if I remembered their info, I could get funds stolen from ALL channels.

Loading a backup that has all my channels but one or two of them are in an old state will only risk funds in one or two channels.

Right now the only way you could "back up" would be to set your .lnd folder to be inside a dropbox folder or something.

@homakov

This comment has been minimized.

Copy link

homakov commented Mar 11, 2018

Right now the only way you could "back up" would be to set your .lnd folder to be inside a dropbox folder or something.

Yes, and this is not production ready for sure. Some people don't like dropbox etc.

Also even though channel db is tiny, if you have 1000 channels it makes sense to only backup some encrypted diff instead of everything.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 11, 2018

@homakov

This comment has been minimized.

Copy link

homakov commented Mar 11, 2018

@Roasbeef I didn't exactly understand the process, who stores the backup, and how often backups are made... Do you plan to explain it in RFC? All implementations/wallets would need it anyway.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 11, 2018

@dabura667

This comment has been minimized.

Copy link

dabura667 commented Mar 11, 2018

So... the answer for now is “don’t break your disk, or kiss all your channels and money goodbye!”

What is the current recommendation for managing channel.db backup and recovery?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Mar 11, 2018

The answer for now is don't put 1 billion USD into a channel ;)

Automated tools are in development to manage the backup, recovery, etc. That PR open that just slings backups isn't safe (with current commitment design), so it won't be merged.

@marsrobertson

This comment has been minimized.

Copy link

marsrobertson commented Jun 26, 2019

#RANDOM

I’m sorry, it feels like the ship has sailed and I’m talking about last year snow, water under the bridge.

This: rootzoll/raspiblitz#500 (comment)

But work needs to be done as priority to make this seed recoverable in any bip39 compatible wallets if we really want to make this project a huge success.

Also this: https://twitter.com/slush/status/974584070603706368

Breaking BIP39 is uncool

#719 (comment)

BIP 39 lacks two important features

Versioning system

bind the key derivation schema to the version of the seed

Why not just use a single, standard, default derivation path?

https://bitcoin.stackexchange.com/questions/78993/default-derivation-paths

(it’s pretty well established)

Wallet birthday

Why don’t we assume that in the future running a full node will be a common place.

It’s unfairly easy these days to run the node: https://medium.com/lightning-power-users/windows-macos-lightning-network-284bd5034340


Upon expressing my concerns about breaking standards, I'm thinking about creating a tool that would convert one format into another. Also a tool that would seamlessly migrate from on-chain BTC into a lightning-enabled wallet in a one go. Currently, to start using lightning network:

  1. Send BTC to a lightning enabled wallet (on-chain)
  2. Open a channel (on-chain)
  3. Spend sats to enable inbound capacity

That's quite a lot of determination and I'm happy to see tools to make it easier - https://blog.lightning.engineering/posts/2019/03/20/loop.html - I'm on the side of adoption.

Breaking BIP39 was (in my opinion) a bad decision and I do not understand the benefits (birthday, versioning) as they seem to me negligible.

I can be wrong, just like Craig Wright can be Satoshi, as simple as that.

image


EDIT: To avoid spamming.

Dude, you need to stop spamming this thread and everywhere else. This is not only crazy but you are very rude and disrespectful to the devs. Just totally uncool yourself, so stop it.

Well... The issue was bothering me so much, I was editing notes, doing research, thinking about it. I even posted the screenshot on Twitter: https://twitter.com/marsxrobertson/status/1143916109516738560

image

In no place, my intention was to be disrespectful. If that is your perception then please acknowledge we live in a free-will universe and anyone has the right to feel whatever they want.

@molxyz

This comment has been minimized.

Copy link

molxyz commented Jun 26, 2019

@marsrobertson Dude, you need to stop spamming this thread and everywhere else. This is not only crazy but you are very rude and disrespectful to the devs. Just totally uncool yourself, so stop it.

@junderw

This comment has been minimized.

Copy link
Contributor

junderw commented Jun 27, 2019

BIP39 was not "carefully engineered"

it was obviously rushed to the point where ThomasV had them take his name off the BIP because they had to rush and push it out the door to make money on selling Trezors.

SatoshiLabs does tons of good for Bitcoin, and BIP39 was overall a good for Bitcoin, yes, but it was rushed. Acting otherwise is disingenuous.

Also "dig up my cryptosteel for every feature" is disingenuous.

The lack of versioning might be great for a developer. But for my mom, she called me in a panic because she restored her BIP39, and suddenly Ledger played 20 questions with her. She didn't understand, it shows the wrong wallet "p2sh-p2wpkh instead of p2pkh" and she literally said "I lost my Bitcoins"

Some people might throw away their HW wallet and phrase if they thought they lost everything anyway.

with versioning, all you need is to create a new phrase. No need to dig up.

If you want a new feature, you're going to move to a new address anyways.

and if done correctly, you could even add an hidden advanced "version override" feature on your wallet if the version is not tied to the entropy.

tl:dr Aezeed > BIP39 and SLIP39

@molxyz

This comment has been minimized.

Copy link

molxyz commented Jun 27, 2019

@junderw Hi Jon, thanks for this interesting piece of history which I didn't even know... I saw that tweet over a year ago and wondered why someone who's not a dev of this project, cares less about it, but got upset that bip39 isn't used... LOL and roasbeef had to explain to him again...

image

Just for the reference if anyone else hasn't read why aezeed is used, it's explained on this thread and specifically from here:
#719 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.