Merge pull request #6341 from guggero/remote-signer-signoutputraw

remote signer: fix SignOutputRaw RPC for incomplete key info, fix healthcheck connection leak

config_builder.go

@@ -691,7 +691,7 @@ func (d *RPCSignerWalletImpl) BuildChainControl(
 
 	rpcKeyRing, err := rpcwallet.NewRPCKeyRing(
 		baseKeyRing, walletController,
-		d.DefaultWalletImpl.cfg.RemoteSigner, walletConfig.CoinType,
+		d.DefaultWalletImpl.cfg.RemoteSigner, walletConfig.NetParams,
 	)
 	if err != nil {
 		err := fmt.Errorf("unable to create RPC remote signing wallet "+

docs/release-notes/release-notes-0.14.3.md

@@ -8,6 +8,13 @@
   off](https://github.com/lightningnetwork/lnd/pull/6359), enabling strict
   HTTP method matching.
 
+* The [`SignOutputRaw` RPC now works properly in remote signing
+  mode](https://github.com/lightningnetwork/lnd/pull/6341), even when
+  only a public key or only a key locator is specified. This allows a Loop or
+  Pool node to be connected to a remote signing node pair.
+  A bug in the remote signer health check was also fixed that lead to too many
+  open connections.
+
 # Contributors (Alphabetical Order)
 
 * Oliver Gugger

lnrpc/walletrpc/walletkit_server.go

@@ -1224,6 +1224,8 @@ func (w *WalletKit) SignPsbt(_ context.Context, req *SignPsbtRequest) (
 		bytes.NewReader(req.FundedPsbt), false,
 	)
 	if err != nil {
+		log.Debugf("Error parsing PSBT: %v, raw input: %x", err,
+			req.FundedPsbt)
 		return nil, fmt.Errorf("error parsing PSBT: %v", err)
 	}
 

lntest/itest/lnd_remote_signer_test.go

@@ -99,6 +99,12 @@ func testRemoteSigner(net *lntest.NetworkHarness, t *harnessTest) {
 			runPsbtChanFunding(net, tt, carol, wo)
 			runSignPsbt(tt, net, wo)
 		},
+	}, {
+		name:      "sign output raw",
+		sendCoins: true,
+		fn: func(tt *harnessTest, wo, carol *lntest.HarnessNode) {
+			runSignOutputRaw(tt, net, wo)
+		},
 	}}
 
 	for _, st := range subTests {

lntest/itest/lnd_signer_test.go

@@ -1,12 +1,18 @@
 package itest
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 
 	"github.com/btcsuite/btcd/btcec/v2"
+	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/txscript"
+	"github.com/btcsuite/btcd/wire"
 	"github.com/lightningnetwork/lnd/keychain"
+	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnrpc/signrpc"
+	"github.com/lightningnetwork/lnd/lnrpc/walletrpc"
 	"github.com/lightningnetwork/lnd/lntest"
 	"github.com/stretchr/testify/require"
 )
@@ -190,6 +196,201 @@ func runDeriveSharedKey(t *harnessTest, alice *lntest.HarnessNode) {
 	assertErrorMatch("use either raw_key_bytes or key_index", req)
 }
 
+// testSignOutputRaw makes sure that the SignOutputRaw RPC can be used with all
+// custom ways of specifying the signing key in the key descriptor/locator.
+func testSignOutputRaw(net *lntest.NetworkHarness, t *harnessTest) {
+	runSignOutputRaw(t, net, net.Alice)
+}
+
+// runSignOutputRaw makes sure that the SignOutputRaw RPC can be used with all
+// custom ways of specifying the signing key in the key descriptor/locator.
+func runSignOutputRaw(t *harnessTest, net *lntest.NetworkHarness,
+	alice *lntest.HarnessNode) {
+
+	ctxb := context.Background()
+	ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+
+	// For the next step, we need a public key. Let's use a special family
+	// for this. We want this to be an index of zero.
+	const testCustomKeyFamily = 44
+	keyDesc, err := alice.WalletKitClient.DeriveNextKey(
+		ctxt, &walletrpc.KeyReq{
+			KeyFamily: testCustomKeyFamily,
+		},
+	)
+	require.NoError(t.t, err)
+	require.Equal(t.t, int32(0), keyDesc.KeyLoc.KeyIndex)
+
+	targetPubKey, err := btcec.ParsePubKey(keyDesc.RawKeyBytes)
+	require.NoError(t.t, err)
+
+	// First, try with a key descriptor that only sets the public key.
+	assertSignOutputRaw(
+		t, net, alice, targetPubKey, &signrpc.KeyDescriptor{
+			RawKeyBytes: keyDesc.RawKeyBytes,
+		},
+	)
+
+	// Now try again, this time only with the (0 index!) key locator.
+	assertSignOutputRaw(
+		t, net, alice, targetPubKey, &signrpc.KeyDescriptor{
+			KeyLoc: &signrpc.KeyLocator{
+				KeyFamily: keyDesc.KeyLoc.KeyFamily,
+				KeyIndex:  keyDesc.KeyLoc.KeyIndex,
+			},
+		},
+	)
+
+	// And now test everything again with a new key where we know the index
+	// is not 0.
+	ctxt, cancel = context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+	keyDesc, err = alice.WalletKitClient.DeriveNextKey(
+		ctxt, &walletrpc.KeyReq{
+			KeyFamily: testCustomKeyFamily,
+		},
+	)
+	require.NoError(t.t, err)
+	require.Equal(t.t, int32(1), keyDesc.KeyLoc.KeyIndex)
+
+	targetPubKey, err = btcec.ParsePubKey(keyDesc.RawKeyBytes)
+	require.NoError(t.t, err)
+
+	// First, try with a key descriptor that only sets the public key.
+	assertSignOutputRaw(
+		t, net, alice, targetPubKey, &signrpc.KeyDescriptor{
+			RawKeyBytes: keyDesc.RawKeyBytes,
+		},
+	)
+
+	// Now try again, this time only with the key locator.
+	assertSignOutputRaw(
+		t, net, alice, targetPubKey, &signrpc.KeyDescriptor{
+			KeyLoc: &signrpc.KeyLocator{
+				KeyFamily: keyDesc.KeyLoc.KeyFamily,
+				KeyIndex:  keyDesc.KeyLoc.KeyIndex,
+			},
+		},
+	)
+}
+
+// assertSignOutputRaw sends coins to a p2wkh address derived from the given
+// target public key and then tries to spend that output again by invoking the
+// SignOutputRaw RPC with the key descriptor provided.
+func assertSignOutputRaw(t *harnessTest, net *lntest.NetworkHarness,
+	alice *lntest.HarnessNode, targetPubKey *btcec.PublicKey,
+	keyDesc *signrpc.KeyDescriptor) {
+
+	ctxb := context.Background()
+	ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+
+	pubKeyHash := btcutil.Hash160(targetPubKey.SerializeCompressed())
+	targetAddr, err := btcutil.NewAddressWitnessPubKeyHash(
+		pubKeyHash, harnessNetParams,
+	)
+	require.NoError(t.t, err)
+	targetScript, err := txscript.PayToAddrScript(targetAddr)
+	require.NoError(t.t, err)
+
+	// Send some coins to the generated p2wpkh address.
+	_, err = alice.SendCoins(ctxt, &lnrpc.SendCoinsRequest{
+		Addr:   targetAddr.String(),
+		Amount: 800_000,
+	})
+	require.NoError(t.t, err)
+
+	// Wait until the TX is found in the mempool.
+	txid, err := waitForTxInMempool(net.Miner.Client, minerMempoolTimeout)
+	require.NoError(t.t, err)
+
+	targetOutputIndex := getOutputIndex(
+		t, net.Miner, txid, targetAddr.String(),
+	)
+
+	// Clear the mempool.
+	mineBlocks(t, net, 1, 1)
+
+	// Try to spend the output now to a new p2wkh address.
+	p2wkhResp, err := alice.NewAddress(ctxt, &lnrpc.NewAddressRequest{
+		Type: lnrpc.AddressType_WITNESS_PUBKEY_HASH,
+	})
+	require.NoError(t.t, err)
+
+	p2wkhAdrr, err := btcutil.DecodeAddress(
+		p2wkhResp.Address, harnessNetParams,
+	)
+	require.NoError(t.t, err)
+
+	p2wkhPkScript, err := txscript.PayToAddrScript(p2wkhAdrr)
+	require.NoError(t.t, err)
+
+	tx := wire.NewMsgTx(2)
+	tx.TxIn = []*wire.TxIn{{
+		PreviousOutPoint: wire.OutPoint{
+			Hash:  *txid,
+			Index: uint32(targetOutputIndex),
+		},
+	}}
+	value := int64(800_000 - 200)
+	tx.TxOut = []*wire.TxOut{{
+		PkScript: p2wkhPkScript,
+		Value:    value,
+	}}
+
+	var buf bytes.Buffer
+	require.NoError(t.t, tx.Serialize(&buf))
+
+	signResp, err := alice.SignerClient.SignOutputRaw(
+		ctxt, &signrpc.SignReq{
+			RawTxBytes: buf.Bytes(),
+			SignDescs: []*signrpc.SignDescriptor{{
+				Output: &signrpc.TxOut{
+					PkScript: targetScript,
+					Value:    800_000,
+				},
+				InputIndex:    0,
+				KeyDesc:       keyDesc,
+				Sighash:       uint32(txscript.SigHashAll),
+				WitnessScript: targetScript,
+			}},
+		},
+	)
+	require.NoError(t.t, err)
+
+	tx.TxIn[0].Witness = wire.TxWitness{
+		append(signResp.RawSigs[0], byte(txscript.SigHashAll)),
+		targetPubKey.SerializeCompressed(),
+	}
+
+	buf.Reset()
+	require.NoError(t.t, tx.Serialize(&buf))
+
+	_, err = alice.WalletKitClient.PublishTransaction(
+		ctxt, &walletrpc.Transaction{
+			TxHex: buf.Bytes(),
+		},
+	)
+	require.NoError(t.t, err)
+
+	// Wait until the spending tx is found.
+	txid, err = waitForTxInMempool(net.Miner.Client, minerMempoolTimeout)
+	require.NoError(t.t, err)
+	p2wkhOutputIndex := getOutputIndex(
+		t, net.Miner, txid, p2wkhAdrr.String(),
+	)
+	op := &lnrpc.OutPoint{
+		TxidBytes:   txid[:],
+		OutputIndex: uint32(p2wkhOutputIndex),
+	}
+	assertWalletUnspent(t, alice, op)
+
+	// Mine another block to clean up the mempool and to make sure the spend
+	// tx is actually included in a block.
+	mineBlocks(t, net, 1, 1)
+}
+
 // deriveCustomizedKey uses the family and index to derive a public key from
 // the node's walletkit client.
 func deriveCustomizedKey(ctx context.Context, node *lntest.HarnessNode,

lntest/itest/lnd_test_list_on_test.go

@@ -157,6 +157,10 @@ var allTestCases = []*testCase{
 		name: "derive shared key",
 		test: testDeriveSharedKey,
 	},
+	{
+		name: "sign output raw",
+		test: testSignOutputRaw,
+	},
 	{
 		name: "async payments benchmark",
 		test: testAsyncPayments,

lntest/itest/utils.go

@@ -8,7 +8,9 @@ import (
 	"time"
 
 	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/rpcclient"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/go-errors/errors"
 	"github.com/lightningnetwork/lnd/input"
@@ -463,3 +465,31 @@ func findTxAtHeight(t *harnessTest, height int32,
 
 	return nil
 }
+
+// getOutputIndex returns the output index of the given address in the given
+// transaction.
+func getOutputIndex(t *harnessTest, miner *lntest.HarnessMiner,
+	txid *chainhash.Hash, addr string) int {
+
+	t.t.Helper()
+
+	// We'll then extract the raw transaction from the mempool in order to
+	// determine the index of the p2tr output.
+	tx, err := miner.Client.GetRawTransaction(txid)
+	require.NoError(t.t, err)
+
+	p2trOutputIndex := -1
+	for i, txOut := range tx.MsgTx().TxOut {
+		_, addrs, _, err := txscript.ExtractPkScriptAddrs(
+			txOut.PkScript, miner.ActiveNet,
+		)
+		require.NoError(t.t, err)
+
+		if addrs[0].String() == addr {
+			p2trOutputIndex = i
+		}
+	}
+	require.Greater(t.t, p2trOutputIndex, -1)
+
+	return p2trOutputIndex
+}

lntest/mock/walletcontroller.go

@@ -90,6 +90,13 @@ func (w *WalletController) IsOurAddress(btcutil.Address) bool {
 	return false
 }
 
+// AddressInfo currently returns a dummy value.
+func (w *WalletController) AddressInfo(
+	btcutil.Address) (waddrmgr.ManagedAddress, error) {
+
+	return nil, nil
+}
+
 // ListAccounts currently returns a dummy value.
 func (w *WalletController) ListAccounts(string,
 	*waddrmgr.KeyScope) ([]*waddrmgr.AccountProperties, error) {

lnwallet/btcwallet/btcwallet.go

@@ -543,6 +543,16 @@ func (b *BtcWallet) IsOurAddress(a btcutil.Address) bool {
 	return result && (err == nil)
 }
 
+// AddressInfo returns the information about an address, if it's known to this
+// wallet.
+//
+// NOTE: This is a part of the WalletController interface.
+func (b *BtcWallet) AddressInfo(a btcutil.Address) (waddrmgr.ManagedAddress,
+	error) {
+
+	return b.wallet.AddressInfo(a)
+}
+
 // ListAccounts retrieves all accounts belonging to 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.

lnwallet/interface.go

@@ -216,6 +216,10 @@ type WalletController interface {
 	// IsOurAddress checks if the passed address belongs to this wallet
 	IsOurAddress(a btcutil.Address) bool
 
+	// AddressInfo returns the information about an address, if it's known
+	// to this wallet.
+	AddressInfo(a btcutil.Address) (waddrmgr.ManagedAddress, error)
+
 	// ListAccounts retrieves all accounts belonging to 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.

lnwallet/rpcwallet/healthcheck.go

@@ -11,7 +11,7 @@ import (
 // configuration.
 func HealthCheck(cfg *lncfg.RemoteSigner, timeout time.Duration) func() error {
 	return func() error {
-		_, err := connectRPC(
+		conn, err := connectRPC(
 			cfg.RPCHost, cfg.TLSCertPath, cfg.MacaroonPath, timeout,
 		)
 		if err != nil {
@@ -19,6 +19,15 @@ func HealthCheck(cfg *lncfg.RemoteSigner, timeout time.Duration) func() error {
 				"signing node through RPC: %v", err)
 		}
 
+		defer func() {
+			err = conn.Close()
+			if err != nil {
+				log.Warnf("Failed to close health check "+
+					"connection to remote signing node: %v",
+					err)
+			}
+		}()
+
 		return nil
 	}
 }

lnwallet/rpcwallet/rpcwallet.go

@@ -14,8 +14,10 @@ import (
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/btcsuite/btcd/btcutil/hdkeychain"
 	"github.com/btcsuite/btcd/btcutil/psbt"
+	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
+	"github.com/btcsuite/btcwallet/waddrmgr"
 	basewallet "github.com/btcsuite/btcwallet/wallet"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/keychain"
@@ -51,7 +53,7 @@ type RPCKeyRing struct {
 
 	watchOnlyKeyRing keychain.SecretKeyRing
 
-	coinType uint32
+	netParams *chaincfg.Params
 
 	rpcTimeout time.Duration
 
@@ -69,7 +71,8 @@ var _ lnwallet.WalletController = (*RPCKeyRing)(nil)
 // delegates any signing or ECDH operations to the remove signer through RPC.
 func NewRPCKeyRing(watchOnlyKeyRing keychain.SecretKeyRing,
 	watchOnlyWalletController lnwallet.WalletController,
-	remoteSigner *lncfg.RemoteSigner, coinType uint32) (*RPCKeyRing, error) {
+	remoteSigner *lncfg.RemoteSigner,
+	netParams *chaincfg.Params) (*RPCKeyRing, error) {
 
 	rpcConn, err := connectRPC(
 		remoteSigner.RPCHost, remoteSigner.TLSCertPath,
@@ -83,7 +86,7 @@ func NewRPCKeyRing(watchOnlyKeyRing keychain.SecretKeyRing,
 	return &RPCKeyRing{
 		WalletController: watchOnlyWalletController,
 		watchOnlyKeyRing: watchOnlyKeyRing,
-		coinType:         coinType,
+		netParams:        netParams,
 		rpcTimeout:       remoteSigner.Timeout,
 		signerClient:     signrpc.NewSignerClient(rpcConn),
 		walletClient:     walletrpc.NewWalletKitClient(rpcConn),
@@ -583,6 +586,79 @@ func (r *RPCKeyRing) remoteSign(tx *wire.MsgTx, signDesc *input.SignDescriptor,
 	in := &packet.Inputs[signDesc.InputIndex]
 	txIn := tx.TxIn[signDesc.InputIndex]
 
+	// Things are a bit tricky with the sign descriptor. There basically are
+	// four ways to describe a key:
+	//   1. By public key only. To match this case both family and index
+	//      must be set to 0.
+	//   2. By family and index only. To match this case the public key
+	//      must be nil and either the family or index must be non-zero.
+	//   3. All values are set and locator is non-empty. To match this case
+	//      the public key must be set and either the family or index must
+	//      be non-zero.
+	//   4. All values are set and locator is empty. This is a special case
+	//      for the very first channel ever created (with the multi-sig key
+	//      family which is 0 and the index which is 0 as well). This looks
+	//      identical to case 1 and will also be handled like that case.
+	// We only really handle case 1 and 2 here, since 3 is no problem and 4
+	// is identical to 1.
+	switch {
+	// Case 1: Public key only. We need to find out the derivation path for
+	// this public key by asking the wallet. This is only possible for our
+	// internal, custom 1017 scope since we know all keys derived there are
+	// internally stored as p2wkh addresses.
+	case signDesc.KeyDesc.PubKey != nil && signDesc.KeyDesc.IsEmpty():
+		pubKeyBytes := signDesc.KeyDesc.PubKey.SerializeCompressed()
+		addr, err := btcutil.NewAddressWitnessPubKeyHash(
+			btcutil.Hash160(pubKeyBytes), r.netParams,
+		)
+		if err != nil {
+			return nil, fmt.Errorf("error deriving address from "+
+				"public key %x: %v", pubKeyBytes, err)
+		}
+
+		managedAddr, err := r.AddressInfo(addr)
+		if err != nil {
+			return nil, fmt.Errorf("error fetching address info "+
+				"for public key %x: %v", pubKeyBytes, err)
+		}
+
+		pubKeyAddr, ok := managedAddr.(waddrmgr.ManagedPubKeyAddress)
+		if !ok {
+			return nil, fmt.Errorf("address derived for public "+
+				"key %x is not a p2wkh address", pubKeyBytes)
+		}
+
+		scope, path, _ := pubKeyAddr.DerivationInfo()
+		if scope.Purpose != keychain.BIP0043Purpose {
+			return nil, fmt.Errorf("address derived for public "+
+				"key %x is not in custom key scope %d'",
+				pubKeyBytes, keychain.BIP0043Purpose)
+		}
+
+		// We now have all the information we need to complete our key
+		// locator information.
+		signDesc.KeyDesc.KeyLocator = keychain.KeyLocator{
+			Family: keychain.KeyFamily(path.InternalAccount),
+			Index:  path.Index,
+		}
+
+	// Case 2: Family and index only. This case is easy, we can just go
+	// ahead and derive the public key from the family and index and then
+	// supply that information in the BIP32 derivation field.
+	case signDesc.KeyDesc.PubKey == nil && !signDesc.KeyDesc.IsEmpty():
+		fullDesc, err := r.watchOnlyKeyRing.DeriveKey(
+			signDesc.KeyDesc.KeyLocator,
+		)
+		if err != nil {
+			return nil, fmt.Errorf("error deriving key with "+
+				"family %d and index %d from the watch-only "+
+				"wallet: %v",
+				signDesc.KeyDesc.KeyLocator.Family,
+				signDesc.KeyDesc.KeyLocator.Index, err)
+		}
+		signDesc.KeyDesc.PubKey = fullDesc.PubKey
+	}
+
 	// Make sure we actually know about the input. We either have been
 	// watching the UTXO on-chain or we have been given all the required
 	// info in the sign descriptor.
@@ -610,7 +686,8 @@ func (r *RPCKeyRing) remoteSign(tx *wire.MsgTx, signDesc *input.SignDescriptor,
 			Bip32Path: []uint32{
 				keychain.BIP0043Purpose +
 					hdkeychain.HardenedKeyStart,
-				r.coinType + hdkeychain.HardenedKeyStart,
+				r.netParams.HDCoinType +
+					hdkeychain.HardenedKeyStart,
 				uint32(signDesc.KeyDesc.Family) +
 					hdkeychain.HardenedKeyStart,
 				0,