-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
multi: support derived public key import #5047
multi: support derived public key import #5047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work 💯
I tested many different things and only found one small-ish issue (bare pubkey import not being picked up).
UX wise I think we have some challenges ahead of us (rescan logic) and also can improve on a few things. None of them need to go into this PR IMO, just persisting them here for future reference:
- As you've noticed in the itests, the generated addresses need to be in sync between two
lnd
wallets that share the same (public) keys. I'm wondering if we should just go ahead and generate a bunch of addresses up-front to track incoming funds pro-actively? - There's a TODO in the PR about upfront shutdown addresses. It would be nice if we could thread through the account to use for deriving the coop close addresses somehow. Could even be a custom field in the PSBT that's added if
FundPsbt
is called with the--account
flag? - Do we want to support funding a PSBT from multiple accounts? Not sure if it's worth adding, but could do comma separated list of accounts in some of the CLIs/RPCs.
Thanks for the review!
This should be fixed now.
In the integration tests, the addresses are also generated on the signer's side because the wallet is not able to provide signatures for keys that haven't been generated yet. One way to work around this would be extend our sign RPC to accept key descriptors for keys within our default scopes and not just our 1017 scope.
Yeah that was my idea so that funds can go straight from their cold storage into lightning back into cold storage automatically. This would only work with cooperative closes though, so we'd have to extend our sweeper to handle sweeping to the watch-only account to handle the force closes.
We could. They can already do so by using their own coin selection, but maybe they don't want to deal with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work!
Completed an initial pass going from top down, so first focusing on some of the new API changes. Will circle back to take a deeper look at the integration tests w.r.t the envisioned functionality, before following up with some manual tests of my own.
// | ||
// NOTE: Only witness outputs should be included in the computation of | ||
// the total spendable balance of the wallet. We require this as only | ||
// witness inputs can be used for funding channels. | ||
ConfirmedBalance(confs int32) (btcutil.Amount, error) | ||
ConfirmedBalance(confs int32, accountFilter string) (btcutil.Amount, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comment, but we could instead make the accountFilter
itself into a predicate, which would allow callers to filter several accounts at once w/o needing to do repeated calls to the method. We could then have a default higher level function like SingleAccountFilter(acctName string)
for teh more common case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing this would also affect ListUnspentWitness
so I've decided to leave it out for a potential follow-up instead.
// 'maxConfs' implies returning all outputs with at least 'minConfs'. | ||
// | ||
// NOTE: This method requires the global coin selection lock to be held. | ||
func (l *LightningWallet) ListUnspentWitnessFromDefaultAccount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this preferable to just added the account as a default arg using a set of functional arguments? No strong pref on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I had before. Having the name be explicit was suggested and preferred: #5047 (comment).
So you suggest doing this at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest iteration looks pretty good!
Is there anything still in progress concerning the export and BIP49/address type issue we talked about offline? Or should I run another set of manual tests on the current PR stack?
One way to work around this would be extend our sign RPC to accept key descriptors for keys within our default scopes and not just our 1017 scope.
Yes. Since we have the UTXO information available in the PSBT case, we should be able to just sign for that key without needing to look up the unspent output again. Will probably require some more refactoring in the wallet so probably something for a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code/structure looks good to me!
Found a few bugs when testing though (see comments here and in btcsuite/btcwallet#732).
I also think we should add integration test cases for all BIP/address type combinations. We can just hard code some tprv
and uprv
s to import in different constellations in addition to using the other wallet's keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last issue around the hybrid account type, otherwise LGTM, solid work 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK, LGTM 💯
Needs a rebase! |
Rebased. We also have a btcd dependency: btcsuite/btcd#1704. |
@wpaulino btcd PR has been merged! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few clarifying comments, but this PR looks really good. Outstanding job on this series of PRs! I'm sure it was quite the saga to fully grok all necessary flows and test them accordingly as well.
LGTM 🐲
Final thing on my end is to do a few tests of my own, then I think this'll be ready to land!
detect past events will be supported later on. | ||
`, | ||
Flags: []cli.Flag{ | ||
cli.StringFlag{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason the xpub and name don't also have settable flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO flags should only really be used for optional arguments (which the xpub and name aren't), though that hasn't always been consistent across our CLI commands.
This PR introduces support for importing derived public keys, enabling lnd to act as a watch-only wallet. This PR, along with the recently added
FundPsbt
andFinalizePsbt
RPCs inv0.12.0-beta
, enables the use of hardware wallets with lnd. Note that even with all of this, it is still possible for funds to exist within the default lnd wallet upon channel closes.New RPCs
ImportAccount
Imports an account backed by an account extended public key. The master key fingerprint denotes the fingerprint of the root key corresponding to the account public key (also known as the key with derivation path m/). This may be required by some hardware wallets for proper identification and signing.
The address type can usually be inferred from the key's version, but may be required for certain keys to map them into the proper scope. For BIP-0044 keys, an address type must be specified as we intend to not support importing BIP-0044 keys into the wallet using the legacy pay-to-pubkey-hash (P2PKH) scheme. A nested witness address type will force the standard BIP-0049 derivation scheme, while a witness address type will force the standard BIP-0084 derivation scheme. For BIP-0049 keys, an address type must also be specified to make a distinction between the standard BIP-0049 address schema (nested witness pubkeys everywhere) and our own BIP-0049Plus address schema (nested pubkeys externally, witness pubkeys internally).
ImportPublicKey
Imports a single derived public key into the wallet's default imported account. Derived extended public keys must have a derivation path of the form
m/purpose'/coin_type'/account'/branch/index
. Similar toImportAccount
, the address type can usually be inferred from the key's version, but in the case of legacy versions (xpub, tpub), an address type must be specified as we intend to not support importing BIP-44 keys into the wallet using the legacy pay-to-pubkey (P2PK) scheme.ListAccounts
Exposes information for all accounts existing within the wallet by default. A name and key scope filter can be provided to filter through all of the wallet accounts and return only those matching.
Changes to Existing RPCs
Several RPCs and their corresponding CLI commands were modified to include an optional
account
parameter now that watch-only accounts are supported. Users will be able to:The response of the
WalletBalance
RPC now includes an additional field that breaks down the confirmed and unconfirmed balance of each wallet account.Notes for Reviewers/Testers
The size of the diff is due to proto changes, but the overall changes are not as daunting. The majority of the import logic lives within
btcwallet
, so this PR is mostly regarding RPC changes.I recommend not running this on a production node yet as there were some database changes within btcwallet that are currently still in review and may change.
Events (deposits/spends) for imported keys or keys derived from an imported account will only be detected by lnd if they happen after the import. Rescans to detect past events will be supported later on, so make sure to use a new wallet when testing.
Depends on btcsuite/btcwallet#732 and btcsuite/btcwallet#733.
Fixes #3943.