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,14 +11,23 @@ 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 {
 			return fmt.Errorf("error connecting to the remote "+
 				"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
 	}
 }