Support remote signing over RPC #5689

guggero · 2021-09-03T13:49:48Z

Fixes #5486 and #5779.
Also took some ideas from #3944, maybe we can close that as well.

Depends on #5708 and btcsuite/btcwallet#768.

This PR makes it possible to run an instance of lnd in purely watch-only mode (no private key material in the wallet whatsoever). All signing or ECDH requests are then forwarded to a secondary lnd instance (or, in theory, any software that implements the necessary RPC interfaces) for processing.

It is important that the secondary (the remote signing instance with the private) keys is NOT used as a wallet. The keys and addresses are kept in sync between the watch-only and the signing instance, so manually generating addresses or deriving keys on the signing instance will lead to unexpected errors on the watch-only instance!

orijbot · 2021-09-03T13:49:50Z

Visit https://dashboard.github.orijtech.com?back=0&pr=5689&remote=true&repo=guggero%2Flnd to see benchmark details.

config.go

Roasbeef

The final component falls into place! 🍾

Happy to see all the incremental steps we've taken over the past year or so resulted in this PR primary being some refactoring and additional layers of indirection.

Initial pass complete, main comment is the additional layer of indirection needed to allow the "primary" interfaces to be passed into the lnd.Main method rather than lnd initializing the concrete version of the interfaces within itself. Also see the msgs sent offline re some sample directions.

keychain/derivation.go

keychain/btcwallet.go

discovery/gossiper.go

lnwallet/rpcwallet/rpcwallet.go

lnrpc/walletrpc/walletkit.proto

lnwallet/rpcwallet/rpcwallet.go

Roasbeef · 2021-09-04T01:42:14Z

lnrpc/walletunlocker.proto

+    required by some hardware wallets for proper identification and signing. The
+    bytes must be in big-endian order.
+    */
+    bytes master_key_fingerprint = 2;


Is there a strong requirement that this is on this level? In that, can it go one layer below within the WatchOnlyAccount struct?

Ah, you mean in case the accounts are not all derived from the same master key?

guggero · 2021-09-10T15:37:44Z

I've extracted all the refactoring commits into #5708, will address the rest of the comments later.

lntest/itest/lnd_remote_signer_test.go

Roasbeef · 2021-09-14T19:34:36Z

So I think we want something like this:

diff --git a/keychain/btcwallet.go b/keychain/btcwallet.go
index 8d5dab3bd..3110fc960 100644
--- a/keychain/btcwallet.go
+++ b/keychain/btcwallet.go
@@ -64,7 +64,7 @@ type BtcWalletKeyRing struct {
 //
 // NOTE: The passed waddrmgr.Manager MUST be unlocked in order for the keychain
 // to function.
-func NewBtcWalletKeyRing(w *wallet.Wallet, coinType uint32) SecretKeyRing {
+func NewBtcWalletKeyRing(w *wallet.Wallet, coinType uint32) (SecretKeyRing, error) {
 	// Construct the key scope that will be used within the waddrmgr to
 	// create an HD chain for deriving all of our required keys. A different
 	// scope is used for each specific coin type.
@@ -73,10 +73,33 @@ func NewBtcWalletKeyRing(w *wallet.Wallet, coinType uint32) SecretKeyRing {
 		Coin:    coinType,
 	}
 
-	return &BtcWalletKeyRing{
+	// TODO(xxx): create all keyscopes here
+
+	wallet := &BtcWalletKeyRing{
 		wallet:        w,
 		chainKeyScope: chainKeyScope,
 	}
+
+	for _, keyFam := range VersionZeroKeyFamilies {
+		db := wallet.wallet.Database()
+		err := walletdb.Update(db, func(tx walletdb.ReadWriteTx) error {
+			addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey)
+
+			scope, err := wallet.keyScope()
+			if err != nil {
+				return err
+			}
+
+			// If the account doesn't exist, then we may need to create it
+			// for the first time in order to derive the keys that we
+			// require.
+			return wallet.createAccountIfNotExists(addrmgrNs, keyFam, scope)
+		})
+		if err != nil {
+			return nil, err
+		}
+	}
+	return wallet, nil
 }
 
 // keyScope attempts to return the key scope that we'll use to derive all of
diff --git a/keychain/derivation.go b/keychain/derivation.go
index b22d8f70f..15ac954e8 100644
--- a/keychain/derivation.go
+++ b/keychain/derivation.go
@@ -105,6 +105,21 @@ const (
 	KeyFamilyTowerID KeyFamily = 9
 )
 
+// VersionZeroKeyFamilies is a slice of all the known key families for first
+// version of the key derivation schema defined in this package.
+var VersionZeroKeyFamilies = []KeyFamily{
+	KeyFamilyMultiSig,
+	KeyFamilyRevocationBase,
+	KeyFamilyHtlcBase,
+	KeyFamilyPaymentBase,
+	KeyFamilyDelayBase,
+	KeyFamilyRevocationRoot,
+	KeyFamilyNodeKey,
+	KeyFamilyStaticBackup,
+	KeyFamilyTowerSession,
+	KeyFamilyTowerID,
+}
+
 // KeyLocator is a two-tuple that can be used to derive *any* key that has ever
 // been used under the key derivation mechanisms described in this file.
 // Version 0 of our key derivation schema uses the following BIP43-like

Roasbeef · 2021-09-14T19:35:16Z

The other half (lets lncli wallet accounts list) display all the pubkeys:

diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go
index 1db9e0b18..496ec1723 100644
--- a/lnwallet/btcwallet/btcwallet.go
+++ b/lnwallet/btcwallet/btcwallet.go
@@ -552,6 +552,18 @@ func (b *BtcWallet) ListAccounts(name string,
 			account := account
 			res = append(res, &account.AccountProperties)
 		}
+
+		accounts, err = b.wallet.Accounts(waddrmgr.KeyScope{
+			Purpose: keychain.BIP0043Purpose,
+			Coin:    b.cfg.CoinType,
+		})
+		if err != nil {
+			return nil, err
+		}
+		for _, account := range accounts.Accounts {
+			account := account
+			res = append(res, &account.AccountProperties)
+		}
 	}
 
 	return res, nil

guggero · 2021-09-30T16:44:32Z

Rebased and updated with the above suggestions.
Now the only thing needed is a new command (perhaps lncli create-watchonly?) that takes the output of lncli wallet accounts list and converts it into the format that is needed for the InitWallet RPC.

Roasbeef · 2021-10-07T22:17:20Z

Build fails rn:

watchtowerrpc -- unsafe]: exit status 1: go: github.com/btcsuite/btcwallet@v0.12.1-0.20210916213031-d0868cb9dd94 (replaced by ../../btcsuite/btcwallet): reading ../../btcsuite/btcwallet/go.mod: open /home/travis/gopath/src/github.com/btcsuite/btcwallet/go.mod: no such file or directory
go: github.com/btcsuite/btcwallet@v0.12.1-0.20210916213031-d0868cb9dd94 (replaced by ../../btcsuite/btcwallet): reading ../../btcsuite/btcwallet/go.mod: open /home/travis/gopath/src/github.com/btcsuite/btcwallet/go.mod: no such file or directory

Roasbeef

Getting close! Excellent set of docs also, looking forward to having this land in 01.4!

config_builder.go

lnd.go

Roasbeef · 2021-10-07T22:24:02Z

lnwallet/rpcwallet/rpcwallet.go

+		return nil, fmt.Errorf("error connecting to the remote "+
+			"signing node through RPC: %v", err)
+	}
+


One thing we may want to add here (doesn't necessarily need to be done here, can be follow up work) is a consistency check on start up. We can use lncli wallet accounts to see if all the key indexes line up, and advance them forward as needed.

Yes, definitely! Though this sounds like a good thing to split off into its own PR. Going to try and do this during the RC phase, or worst case for 0.14.1.

go.mod

Roasbeef · 2021-10-07T22:27:00Z

lnwallet/rpcwallet/rpcwallet.go

+	feeRate chainfee.SatPerKWeight, minConfs int32,
+	label string) (*wire.MsgTx, error) {
+
+	tx, err := r.WalletController.SendOutputs(


This should be a dry run instance right?

Why do we want to have this step? To check that it's indeed watch only?

No, we want the watch-only wallet to actually lock the UTXOs and assume they're spent. Because we currently don't sync the UTXO locks, we need to make sure the watch-only wallet is in charge of doing coin selection and UTXO locking.
Or did you mean something else?

Got it. Didn't know the locking UTXO are wrapped inside SendOutputs.

Roasbeef · 2021-10-07T23:56:20Z

docs/remote-signing.md

+remotesigner.enable=true
+remotesigner.rpchost=zane.example.internal:10019
+remotesigner.tlscertpath=/home/watch-only/example/signer.tls.cert
+remotesigner.macaroonpath=/home/watch-only/example/signer.admin.macaroon


Perhaps instead guide the user to use a signer macaroon or a custom one that includes just the perms we need to interact w/ the remote signer?

Great idea! I'm going to recommend the macaroon recipe for that in the docs.

Roasbeef · 2021-10-07T23:56:39Z

docs/remote-signing.md

+the public node using NodeJS.
+
+To use this example, first initialize the "signer" wallet with the root key
+`tprv8ZgxMBicQKsPe6jS4vDm2n7s42Q6MpvghUQqMmSKG7bTZvGKtjrcU3PGzMNG37yzxywrcdvgkwrr8eYXJmbwdvUNVT4Ucv7ris4jvA7BUmg`


Not clear how the user is meant to do this?

The lncli create now has the option to import an xprv if you press x here:

Do you have an existing cipher seed mnemonic or extended master root key you want to use? Enter 'y' to use an existing cipher seed mnemonic, 'x' to use an extended master root key or 'n' to create a new seed (Enter y/x/n):

But I can make that more explicit in the docs.

docs/remote-signing.md

Roasbeef · 2021-10-07T23:57:46Z

docs/remote-signing.md

+Input an optional wallet birthday unix timestamp of first block to start scanning from (default 0): 
+
+
+Input an optional address look-ahead used to scan for used keys (default 2500):


So we've implemented this now in another btcwallet fork?

There is no additional btcwallet fork. Because we create the wallet for the first time and start/sync to chain after importing the accounts, we actually do get a full rescan of all addresses. I tested this locally and even address with index 9 is being detected as being used.
But this will only work in this case where we import an account before the initial chain sync (where we also set recovery_window). Importing an account later will not cause a rescan.
I think that's actually a nice thing to get an initial syncup when setting up the node pair.

Roasbeef · 2021-10-08T00:04:35Z

docs/remote-signing.md

+client.initWallet({
+    wallet_password: Buffer.from(WATCH_ONLY_WALLET_PASSWORD, 'utf-8'),
+    recovery_window: 2500,
+    watch_only: {


Same comment here re not hard coding (can get from the RPC?)

Sure, we could do that. But it's just an example script to show what content the call needs to have. Can also remove the example script if it's more confusing than helping.

yyforyongyu

Fantastic feature! And love to see the great documentation! I think it's looking gooood, a major question is do we plan to patch unit tests for those newly added functions? Plus few questions in the comments.

config_builder.go

yyforyongyu · 2021-10-12T07:10:28Z

config_builder.go

+		*walletConfig, partialChainControl.Cfg.BlockCache,
+	)
+	if err != nil {
+		fmt.Printf("unable to create wallet controller: %v\n", err)


Leftover? Do we want to print to console?

Just mirroring the code style in the rest of the file. We should probably clean that up in another PR.

yyforyongyu · 2021-10-12T07:20:47Z

config_builder.go

+	// DefaultWalletImpl is the embedded instance of the default
+	// implementation that the remote signer uses as its watch-only wallet
+	// for keeping track of addresses and UTXOs.
+	*DefaultWalletImpl


Why do we embed by pointer here?

lnwallet/rpcwallet/rpcwallet.go

yyforyongyu · 2021-10-12T07:27:27Z

lnwallet/rpcwallet/rpcwallet.go

+	// have all keys we have locally. Only if the remote wallet falls behind
+	// the local we might have problems that the remote wallet won't know
+	// outputs we're giving it to sign.
+	if uint32(remoteDesc.KeyLoc.KeyIndex) < localDesc.Index {


I think they must be equal if using the same key locator?

yyforyongyu · 2021-10-12T07:42:33Z

lnwallet/rpcwallet/rpcwallet.go

+		},
+	}
+
+	if keyDesc.Index == 0 && keyDesc.PubKey != nil {


nit: might add some docs explaining why this check.

lnwallet/rpcwallet/rpcwallet.go

yyforyongyu · 2021-10-12T08:56:09Z

lnwallet/rpcwallet/rpcwallet.go

+	feeRate chainfee.SatPerKWeight, minConfs int32,
+	label string) (*wire.MsgTx, error) {
+
+	tx, err := r.WalletController.SendOutputs(


Why do we want to have this step? To check that it's indeed watch only?

yyforyongyu · 2021-10-12T09:19:33Z

walletunlocker/service.go

+		// for it. Since lnd only uses SegWit addresses, we pick the
+		// date of the first block that contained SegWit transactions
+		// (481824).
+		initMsg.WatchOnlyBirthday = time.Date(


I think we use birthday to locate a birthdayBlock when we do syncWithChain inside btcwallet.

lnrpc/walletrpc/walletkit_util.go

To make it possible to supply our own implementation of a secret key ring, we extract that part from the chain control and split the whole chain control creation into two parts.

In order to support the full range of on-chain functionality, including importing watch-only accounts in the watch-only instance, we need to forward some calls like creating new addresses or importing accounts to the remote signing instance.

Roasbeef

LGTM 🍣

Roasbeef · 2021-10-19T01:33:56Z

cmd/lncli/main.go

@@ -327,6 +327,7 @@ func main() {
 	}
 	app.Commands = []cli.Command{
 		createCommand,


Might move this to reside under lncli wallet in a follow up change.

guggero added rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses signrpc labels Sep 3, 2021

guggero added this to the v0.14.0 milestone Sep 3, 2021

guggero added this to In progress in v0.14.0-beta via automation Sep 3, 2021

v0.14.0-beta automation moved this from In progress to Review in progress Sep 3, 2021

Roasbeef requested changes Sep 3, 2021

View reviewed changes

config.go Show resolved Hide resolved

Roasbeef requested changes Sep 4, 2021

View reviewed changes

guggero force-pushed the remote-signer branch from 7b46ef7 to 9370f8c Compare September 10, 2021 15:36

guggero mentioned this pull request Sep 10, 2021

refactor: move towards more configurable implementation details #5708

Merged

Roasbeef mentioned this pull request Sep 12, 2021

wallet: mirrored node doesn't automatically watch and progress keychain of active node #5685

Closed

guggero force-pushed the remote-signer branch 4 times, most recently from d8f03d2 to c8ed48e Compare September 13, 2021 15:43

Roasbeef reviewed Sep 14, 2021

View reviewed changes

lntest/itest/lnd_remote_signer_test.go Show resolved Hide resolved

guggero force-pushed the remote-signer branch 2 times, most recently from b95e738 to 0aaaf83 Compare September 24, 2021 13:39

Roasbeef requested a review from yyforyongyu September 28, 2021 19:01

guggero force-pushed the remote-signer branch from 0aaaf83 to 1a55f90 Compare September 30, 2021 16:43

guggero force-pushed the remote-signer branch 2 times, most recently from c710794 to 23fa101 Compare October 7, 2021 12:23

guggero changed the title ~~WIP: Support remote signing over RPC~~ Support remote signing over RPC Oct 7, 2021

guggero requested a review from Roasbeef October 7, 2021 19:35

Roasbeef reviewed Oct 8, 2021

View reviewed changes

guggero force-pushed the remote-signer branch 3 times, most recently from c8b0c70 to 78f616b Compare October 11, 2021 09:53

yyforyongyu requested changes Oct 12, 2021

View reviewed changes

guggero force-pushed the remote-signer branch from 78f616b to cc46a19 Compare October 12, 2021 09:50

guggero added 11 commits October 14, 2021 15:42

multi: refactor SignMessage to specify hashing

6093393

multi: allow internal wallet to be watch-only

1309c6a

multi: extract key ring creation from chain control

9fa9dd8

To make it possible to supply our own implementation of a secret key ring, we extract that part from the chain control and split the whole chain control creation into two parts.

signrpc: add compact_sig flag to SignMessage

917cf4e

multi: add remote RPC signing wallet implementation

9cae7ad

multi: forward SendCoins call over RPC

a3addcc

rpcwallet: forward FinalizePsbt call over RPC

6d339f3

multi: allow watch-only wallet creation

ceb31f9

multi: create and list all default internal accounts

1541b2e

cmd/lncli+walletrpc: add createwatchonly command

78b7003

guggero force-pushed the remote-signer branch from cc46a19 to 0195196 Compare October 14, 2021 13:42

guggero added 3 commits October 14, 2021 15:42

itest: refactor tests to be re-used

36dc4b3

lntest: add itest for remote signing

a6ea019

docs: add remote signing documentation and release notes

d43854a

guggero force-pushed the remote-signer branch from 0195196 to d43854a Compare October 14, 2021 14:14

Roasbeef approved these changes Oct 19, 2021

View reviewed changes

Roasbeef merged commit 94b7b71 into lightningnetwork:master Oct 19, 2021

v0.14.0-beta automation moved this from Review in progress to Done Oct 19, 2021

guggero deleted the remote-signer branch October 20, 2021 11:00

Roasbeef mentioned this pull request Oct 28, 2021

Allow to sign a message directly #4474

Closed

bjarnemagnussen mentioned this pull request Nov 5, 2021

gRPC FundPsbt ignores MinConfs field and defaults to 1 #5779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support remote signing over RPC #5689

Support remote signing over RPC #5689

guggero commented Sep 3, 2021 •

edited

Loading

orijbot commented Sep 3, 2021

Roasbeef left a comment

Roasbeef Sep 4, 2021

guggero Oct 7, 2021

guggero commented Sep 10, 2021

Roasbeef commented Sep 14, 2021

Roasbeef commented Sep 14, 2021 •

edited

Loading

guggero commented Sep 30, 2021

Roasbeef commented Oct 7, 2021

Roasbeef left a comment

Roasbeef Oct 7, 2021

guggero Oct 12, 2021

Roasbeef Oct 7, 2021

yyforyongyu Oct 12, 2021

guggero Oct 12, 2021

yyforyongyu Oct 12, 2021

Roasbeef Oct 7, 2021

guggero Oct 11, 2021

Roasbeef Oct 7, 2021

guggero Oct 11, 2021

Roasbeef Oct 7, 2021

guggero Oct 12, 2021

Roasbeef Oct 8, 2021

guggero Oct 11, 2021

yyforyongyu left a comment

yyforyongyu Oct 12, 2021

guggero Oct 12, 2021

yyforyongyu Oct 12, 2021

yyforyongyu Oct 12, 2021

yyforyongyu Oct 12, 2021

yyforyongyu Oct 12, 2021

yyforyongyu Oct 12, 2021

Roasbeef left a comment

Roasbeef Oct 19, 2021

		Input an optional wallet birthday unix timestamp of first block to start scanning from (default 0):


		Input an optional address look-ahead used to scan for used keys (default 2500):

Support remote signing over RPC #5689

Support remote signing over RPC #5689

Conversation

guggero commented Sep 3, 2021 • edited Loading

orijbot commented Sep 3, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Sep 10, 2021

Roasbeef commented Sep 14, 2021

Roasbeef commented Sep 14, 2021 • edited Loading

guggero commented Sep 30, 2021

Roasbeef commented Oct 7, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Sep 3, 2021 •

edited

Loading

Roasbeef commented Sep 14, 2021 •

edited

Loading