wallet: support derived public key import #732

wpaulino · 2021-02-17T01:07:19Z

In this PR, we build upon #667 to introduce support for importing master/derived public keys into the wallet. This has been a much requested feature by lnd users that will allow them to minimize their hot wallet risks. Master public keys, commonly referred to as xpubs, must have a derivation path of the form m/purpose'/coin_type'/account', while derived public keys must have a derivation path of the form m/purpose'/coin_type'/account'/branch/index. As of now, care must be taken to ensure any watch-only accounts/derived public keys imported have not been used, as the functionality to rescan for their previous activity is not supported yet.

Roasbeef · 2021-02-17T01:08:34Z

cc @guggero

guggero

Did a first pass, looks pretty good!
Since I wasn't very familiar with the code base it took me a while to grok all the changes. It is easy to underestimate the complexity here. So great work on this, the diff reads well once one understands the context.

waddrmgr/scoped_manager.go

wallet/import.go

waddrmgr/scoped_manager.go

wallet/import.go

waddrmgr/scoped_manager.go

waddrmgr/address.go

waddrmgr/scoped_manager.go

waddrmgr/db.go

waddrmgr/scoped_manager.go

wallet/import.go

Roasbeef

Really close now! Added a suggestions w.r.t how to handle bip49plus vs the regular bip 49. I think that's the only proper blocking comment I have in this diff.

waddrmgr/manager.go

waddrmgr/scoped_manager.go

waddrmgr/db.go

wallet/import.go

Roasbeef · 2021-03-10T03:44:31Z

I think this is only missing the change to "undo" the creation of the BIP 49+ key scope. If we don't want to break some other callers (like lnd perhaps), then we can just change the mapping in ScopeAddrMap.

wpaulino · 2021-03-11T22:36:56Z

@Roasbeef @guggero PTAL! A few things have changed in the latest diff:

New address schema override per account to support importing standard BIP-0049 keys
BIP-0044 and BIP-0049 keys imported with the NestedWitnessPubkey address type now use the standard BIP-0049 address schema instead of our own (BIP-0049Plus)
BIP-0049 keys imported with the WitnessPubkey address type now use the BIP-0049Plus address schema
Account public keys now use the proper HD version for the default wallet accounts -- this isn't done for imported accounts to maintain the key consistent with what the caller provided.

@Roasbeef could you clear the Travis build cache? I'm not sure why ./goclean.sh is failing there, I don't see any failures locally.

Roasbeef · 2021-03-12T01:52:26Z

@wpaulino oddly, the repo doesn't actually have any build caches...

Roasbeef · 2021-03-12T01:57:25Z

I ran it locally and eventually got this after running it twice:

/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1786:26: should drop = nil from declaration of var privParams; it is the zero value
/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1788:32: should drop = nil from declaration of var cryptoKeyPrivEnc; it is the zero value
/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1789:34: should drop = nil from declaration of var cryptoKeyScriptEnc; it is the zero value
+ test -z '/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1786:26: should drop = nil from declaration of var privParams; it is the zero value
/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1788:32: should drop = nil from declaration of var cryptoKeyPrivEnc; it is the zero value
/Users/roasbeef/gocode/src/github.com/btcsuite/btcwallet/waddrmgr/manager.go:1789:34: should drop = nil from declaration of var cryptoKeyScriptEnc; it is the zero value'

I don't think this has been popping up for any other PRs though?

Roasbeef · 2021-03-12T01:58:27Z

The first time it ran it actually modified a set of checked in protos, then after running it again it finally sent out a proper error.

Roasbeef

LGTM 🏊‍♀️

waddrmgr/db.go

guggero

I think we need to use a default version when serializing instead of returning an error. This currently causes the lnd itests to fail.

waddrmgr/scoped_manager.go

Previously, addresses that belong to a watch-only account would have a derivation path using the internal account number used to identify accounts within the databse, rather than the actual account number based on the account's master public key child index. This wasn't an issue before as only one account would exist within the wallet, the 0 account, which is also the default. To ensure users of the DerivationPath struct can arrive at addresses correctly, we introduce a new field InternalAccount to denote the internal account number and repurpose the existing Account field to its actual meaning.

Watch-only accounts are usually backed by an external signer as they do not contain any private key information. Some external signers require a root key fingerprint for identification and signing purposes. In order to guarantee compatibility with external signers, we need to persist the root key fingerprint within the database. Before this change, watch-only accounts used the default account database structure. In this commit, we introduce a new account type to store different information for watch-only accounts only. This isn't a breaking change as watch-only accounts have yet to be supported by the primary user of the wallet (lnd). With this new account type, we can avoid the empty private key fields, which are irrelevant to watch-only accounts, and we can store the root key fingerprint.

The master fingerprint corresponds to the fingerprint of the root master public key (otherwise known as m/). This is required by some hardware wallets for proper identification and signing. The address schema is an optional field that allows an account to override its corresponding address schema with a custom one.

This change was motivated by the need to support importing BIP-0049 keys that use the standard address derivation scheme, where nested witness pubkeys are used for both the external and internal branches. Our BIP-0049 key scope is slightly different, in that addresses derived from the internal branch use the witness pubkey address type. By having the option of overriding the address schema for a particular account, we can support importing standard BIP-0049 keys.

Co-authored-by: Oliver Gugger <[email protected]>

For key scopes which have an address schema where the external and internal branches differ, we always assume that imported keys use the external address type defined in the scope's address schema. This may not always be the case however, and should be handled correctly. Ideally, we generate two addresses per imported key (only if the external and internal address types differ) and scan for both in the chain.

guggero

Travis is failing, otherwise LGTM 💯

wpaulino · 2021-03-23T22:52:03Z

The linter doesn't produce any failures locally, and it doesn't seem to return what's wrong on Travis. Waiting for @Roasbeef to report back.

Roasbeef · 2021-03-24T01:39:10Z

I can confirm that ./goclean.sh passes for me locally, not sure what's up with Travis....

Merging so we can see if the issue perists once it's running against master.

wpaulino mentioned this pull request Feb 18, 2021

waddrmgr+wallet: support transaction creation and signing for watch-only accounts #733

Merged

guggero reviewed Feb 19, 2021

View reviewed changes

waddrmgr/scoped_manager.go Outdated Show resolved Hide resolved

wallet/import.go Outdated Show resolved Hide resolved

waddrmgr/scoped_manager.go Outdated Show resolved Hide resolved

waddrmgr/scoped_manager.go Show resolved Hide resolved

wallet/import.go Outdated Show resolved Hide resolved

wpaulino mentioned this pull request Feb 20, 2021

multi: support derived public key import lightningnetwork/lnd#5047

Merged

Roasbeef requested changes Feb 20, 2021

View reviewed changes

wpaulino changed the title ~~wallet: support master/derived public key import~~ wallet: support derived public key import Feb 23, 2021

wpaulino requested a review from Roasbeef February 24, 2021 00:12

Roasbeef requested changes Feb 25, 2021

View reviewed changes

waddrmgr/manager.go Show resolved Hide resolved

waddrmgr/scoped_manager.go Show resolved Hide resolved

waddrmgr/db.go Outdated Show resolved Hide resolved

wallet/import.go Show resolved Hide resolved

wallet/import.go Outdated Show resolved Hide resolved

Roasbeef approved these changes Mar 12, 2021

View reviewed changes

waddrmgr/db.go Show resolved Hide resolved

guggero requested changes Mar 12, 2021

View reviewed changes

waddrmgr/scoped_manager.go Show resolved Hide resolved

wpaulino and others added 15 commits March 15, 2021 19:08

wtxmgr: prevent race condition by using local vars

6ff982c

multi: fix linter

742d879

travis: bump go version to 1.15.x

967f663

build: update btcutil dependency

02c4a1a

waddrmgr: refactor code from ImportPrivateKey into methods

ee9a1fb

waddrmgr: add ImportPublicKey

dead1a8

wallet: move ImportPrivateKey to import.go

0b5eca1

waddrmgr: return err when priv key is requested for watch-only addr

283f914

waddrmgr: extend AccountProperties with watch-only account properties

adb3d77

waddrmgr: use proper version for account key of non-watch-only accounts

9d90911

wallet: add derived public key import

b0a4956

Co-authored-by: Oliver Gugger <[email protected]>

guggero approved these changes Mar 23, 2021

View reviewed changes

Roasbeef merged commit 5c08d49 into btcsuite:master Mar 24, 2021

wpaulino deleted the import-account-or-pubkey branch March 24, 2021 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: support derived public key import #732

wallet: support derived public key import #732

wpaulino commented Feb 17, 2021

Roasbeef commented Feb 17, 2021

guggero left a comment

Roasbeef left a comment

Roasbeef commented Mar 10, 2021

wpaulino commented Mar 11, 2021 •

edited

Loading

Roasbeef commented Mar 12, 2021

Roasbeef commented Mar 12, 2021

Roasbeef commented Mar 12, 2021

Roasbeef left a comment

guggero left a comment

guggero left a comment

wpaulino commented Mar 23, 2021

Roasbeef commented Mar 24, 2021

wallet: support derived public key import #732

wallet: support derived public key import #732

Conversation

wpaulino commented Feb 17, 2021

Roasbeef commented Feb 17, 2021

guggero left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef commented Mar 10, 2021

wpaulino commented Mar 11, 2021 • edited Loading

Roasbeef commented Mar 12, 2021

Roasbeef commented Mar 12, 2021

Roasbeef commented Mar 12, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

wpaulino commented Mar 23, 2021

Roasbeef commented Mar 24, 2021

wpaulino commented Mar 11, 2021 •

edited

Loading