walletunlocker: Allow wallet to be created from extended master root key (xprv) #4717

guggero · 2020-10-24T21:55:09Z

In addition to creating a new wallet from an aezeed, we allow specifying
an exteded master root key as the main wallet key directly.

Motivation:
This PR is useful to some users for two reasons:

I know for a fact that there are (mainnet!) nodes out there who run with a wallet.db to which the seed was lost or never known (because of incorrect use of the --noseedbackup). This feature gives those nodes the ability to recover from data loss in combination with the chantools walletinfo command that extracts the xprv from a wallet.db (assuming the wallet password is still known).
Wallets apps that already have a seed for their on-chain wallet and want to add lnd for the off-chain stuff. They possibly don't want to give the user another seed to back up. With BIP85 they could derive a new master root key from their existing seed and give it to lnd. It's still important to make sure the xprv given to lnd is used by lnd exclusively!

Wallet birthday:
Because an exteded key (xprv...) doesn't contain any information about the
creation time of the wallet, we must assume a birthday to start scanning
the chain from. Since lnd only uses SegWit addresses, it makes sense to
choose the date that corresponds to the first mainnet block that
contained SegWit transactions.
Restoring a wallet from an extended master root key will result in a
significantly longer initial wallet rescan time.

guggero · 2021-01-29T09:21:24Z

Rebased this as there seems to be some interest in the wallet community to get this in.

Two questions occurred to me during the rebase:

Instead of assuming a static birthday for non-aezeed wallets, should we add another parameter, for example extended_key_birthday to avoid scanning too many blocks?
We need to somehow convey the message that using the exact same on-chain wallet derivation paths (account 0 in the m/49'/0' and m/84'/0') in another wallet at the same time will lead to problems (for example lnd not picking up funds automatically if it didn't register an address as being used in the internal wallet itself). Just adding more text as a comment in the RPC seems easy to miss. Any idea on how to make that clearer?

Roasbeef · 2021-02-02T01:51:07Z

Instead of assuming a static birthday for non-aezeed wallets, should we add another parameter, for example extended_key_birthday to avoid scanning too many blocks?

How would non-aezeed wallets determine what that value should be? We're able to have such a birthday param only because we actually encode that data in the seed.

We need to somehow convey the message that using the exact same on-chain wallet derivation paths (account 0 in the m/49'/0' and m/84'/0') in another wallet at the same time will lead to problems (for example lnd not picking up funds automatically if it didn't register an address as being used in the internal wallet itself)

Seems to be the case for "double importing" into a wallet independent of this use case right? From my PoV, I think I'm missing ab it of a motivating use case for this PR:

Is it for wallet rescue attempts
Or to allow users to use non-aezeed seeds with their lnd wallet? (seems like a bad idea?)
- I guess this would let mobile wallets that want to package lnd without giving their users yet another seed a way forward...

guggero · 2021-02-15T14:48:41Z

How would non-aezeed wallets determine what that value should be? We're able to have such a birthday param only because we actually encode that data in the seed.

The birthday param would be optional and fall back to the hard coded value that is in the PR now. Users that know exactly when the seed was created (perhaps because they wrote it down) can set it and speed up the process. Users that don't know the date leave it blank.

Seems to be the case for "double importing" into a wallet independent of this use case right? From my PoV, I think I'm missing ab it of a motivating use case for this PR:

I added the two main use cases to the main PR description.

carlaKC

Did a very high level pass, nicely broken up PR! Will give this a more thorough look once it's been rebased.

walletunlocker/service.go

carlaKC · 2021-07-15T09:33:33Z

lnd.go

+			// When importing a wallet from its extended private key
+			// we don't know the birthday as that information is not


Decided against an optional birthday param for extended key (as discussed in the PR body)? I think it could be useful, eg in the case of an existing on-chain wallet, they probably know the date when the wallet was created so could have a birthday hint?

Hmm, you're right, it might be useful to have the birthday field. Added it.

lntest/itest/lnd_recovery_test.go

guggero · 2021-07-21T14:20:12Z

Rebased this PR and its dependency.

wpaulino

Looks good! Definitely worthy of an addition given its use cases.

docs/release-notes/release-notes-0.14.0.md

Roasbeef · 2021-08-22T22:30:05Z

The btcwallet PR has been merged, needs a rebase!

guggero · 2021-08-23T09:28:26Z

Rebased after dependent PR was merged.

carlaKC

🏅

carlaKC · 2021-08-24T07:59:17Z

lnd.go

+			// No seed was given, we're importing a wallet from its
+			// extended private key.
+			birthday = initMsg.ExtendedKeyBirthday
+			newWallet, err = loader.CreateNewWalletExtendedKey(


Assuming that this will fail if len(extendedKey)==0, but could be a bit clearer to switch on cipherSeed != nil/len(extendedKey)!=0/default so that the case where we're init-ing the wallet with no seed/key is a bit more obvious?

Yeah, it could be a bit more obvious. Added a switch block instead.

In addition to creating a new wallet from an aezeed, we allow specifying an exteded master root key as the main wallet key directly. Because an exteded key (xprv) doesn't contain any information about the creation time of the wallet, we must assume a birthday to start scanning the chain from (if the user doesn't provide an explicit value). Since lnd only uses SegWit addresses, it makes sense to choose the date that corresponds to the first mainnet block that contained SegWit transactions. Restoring a wallet from an extended master root key will result in a significantly longer initial wallet rescan time if the default value is used.

To allow users to restore a wallet from an existing extended master root key, we accept one of three answers when asking for an existing seed.

To allow testing restoring a node from an extended master root key, we add an extra argument to the RestoreNodeWithSeed function.

We add an additional test case to the on-chain fund recovery test that tries restoring the same wallet from the extended master root key instead of the seed.

guggero added enhancement Improvements to existing features / behaviour rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses recovery Related to the backup/restoration of LND data (e.g. wallet seeds) labels Oct 24, 2020

guggero requested a review from Roasbeef as a code owner October 24, 2020 21:55

guggero removed the request for review from Roasbeef October 26, 2020 08:43

Roasbeef added this to the 0.13.0 milestone Nov 4, 2020

Roasbeef added P2 should be fixed if one has time P3 might get fixed, nice to have and removed P2 should be fixed if one has time labels Jan 28, 2021

guggero force-pushed the xprv-wallet-init branch from b8f0a3f to db42b6f Compare January 29, 2021 09:13

guggero mentioned this pull request Feb 1, 2021

Wallet password should be optional #899

Open

Roasbeef modified the milestones: 0.13.0, v0.14.0 Mar 31, 2021

Roasbeef requested review from carlaKC and wpaulino July 1, 2021 01:48

carlaKC reviewed Jul 15, 2021

View reviewed changes

guggero force-pushed the xprv-wallet-init branch 2 times, most recently from da0f2fe to a6ed0a4 Compare July 21, 2021 14:19

wpaulino approved these changes Jul 30, 2021

View reviewed changes

docs/release-notes/release-notes-0.14.0.md Outdated Show resolved Hide resolved

mod: update to latest btcwallet dep

d329655

lnrpc: add extended master key to walletunlocker

6d70468

guggero force-pushed the xprv-wallet-init branch 2 times, most recently from 2d4ff89 to 96b1b34 Compare August 23, 2021 09:28

guggero requested a review from carlaKC August 23, 2021 09:28

carlaKC approved these changes Aug 24, 2021

View reviewed changes

guggero added 7 commits August 24, 2021 10:37

lncli: extract askRecoveryWindow

bdae671

lncli: extract printCipherSeedWords

370d056

lncli: add extended master root key to create command

bbd5980

To allow users to restore a wallet from an existing extended master root key, we accept one of three answers when asking for an existing seed.

lntest: allow node restore by extended root key

3fd944e

To allow testing restoring a node from an extended master root key, we add an extra argument to the RestoreNodeWithSeed function.

itest: test wallet recovery from extended master root key

0822573

We add an additional test case to the on-chain fund recovery test that tries restoring the same wallet from the extended master root key instead of the seed.

docs: update release notes

8632fc9

guggero force-pushed the xprv-wallet-init branch from 96b1b34 to 8632fc9 Compare August 24, 2021 08:38

guggero merged commit 23cd231 into lightningnetwork:master Aug 24, 2021

guggero deleted the xprv-wallet-init branch August 24, 2021 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

walletunlocker: Allow wallet to be created from extended master root key (xprv) #4717

walletunlocker: Allow wallet to be created from extended master root key (xprv) #4717

guggero commented Oct 24, 2020 •

edited

Loading

guggero commented Jan 29, 2021

Roasbeef commented Feb 2, 2021

guggero commented Feb 15, 2021

carlaKC left a comment

carlaKC Jul 15, 2021

guggero Jul 21, 2021

guggero commented Jul 21, 2021

wpaulino left a comment

Roasbeef commented Aug 22, 2021

guggero commented Aug 23, 2021

carlaKC left a comment

carlaKC Aug 24, 2021

guggero Aug 24, 2021

		// When importing a wallet from its extended private key
		// we don't know the birthday as that information is not

walletunlocker: Allow wallet to be created from extended master root key (xprv) #4717

walletunlocker: Allow wallet to be created from extended master root key (xprv) #4717

Conversation

guggero commented Oct 24, 2020 • edited Loading

guggero commented Jan 29, 2021

Roasbeef commented Feb 2, 2021

guggero commented Feb 15, 2021

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC Jul 15, 2021

Choose a reason for hiding this comment

guggero Jul 21, 2021

Choose a reason for hiding this comment

guggero commented Jul 21, 2021

wpaulino left a comment

Choose a reason for hiding this comment

Roasbeef commented Aug 22, 2021

guggero commented Aug 23, 2021

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC Aug 24, 2021

Choose a reason for hiding this comment

guggero Aug 24, 2021

Choose a reason for hiding this comment

guggero commented Oct 24, 2020 •

edited

Loading