diff --git a/config_builder.go b/config_builder.go index 0e8ddfa8d..0e1bc5560 100644 --- a/config_builder.go +++ b/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 "+ diff --git a/docs/release-notes/release-notes-0.14.3.md b/docs/release-notes/release-notes-0.14.3.md index 6b616d7a0..07de969c4 100644 --- a/docs/release-notes/release-notes-0.14.3.md +++ b/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 diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index 971ddac98..1ee37276f 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/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) } diff --git a/lntest/itest/lnd_remote_signer_test.go b/lntest/itest/lnd_remote_signer_test.go index 5768ac8ff..18027f5c2 100644 --- a/lntest/itest/lnd_remote_signer_test.go +++ b/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 { diff --git a/lntest/itest/lnd_signer_test.go b/lntest/itest/lnd_signer_test.go index ce933ff8e..0d4b78185 100644 --- a/lntest/itest/lnd_signer_test.go +++ b/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, diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go index 24b6cd0d7..72c2ed0d0 100644 --- a/lntest/itest/lnd_test_list_on_test.go +++ b/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, diff --git a/lntest/itest/utils.go b/lntest/itest/utils.go index 5c11f9b9b..fdc611eb2 100644 --- a/lntest/itest/utils.go +++ b/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 +} diff --git a/lntest/mock/walletcontroller.go b/lntest/mock/walletcontroller.go index 431c9f885..7dc79e2cc 100644 --- a/lntest/mock/walletcontroller.go +++ b/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) { diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index a8f44a71e..d6a63b0b8 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/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. diff --git a/lnwallet/interface.go b/lnwallet/interface.go index 5475ad62f..f91b1ae81 100644 --- a/lnwallet/interface.go +++ b/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. diff --git a/lnwallet/rpcwallet/healthcheck.go b/lnwallet/rpcwallet/healthcheck.go index 27bcb9c73..7a412959e 100644 --- a/lnwallet/rpcwallet/healthcheck.go +++ b/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 } } diff --git a/lnwallet/rpcwallet/rpcwallet.go b/lnwallet/rpcwallet/rpcwallet.go index ebb11f531..e4ff17ea6 100644 --- a/lnwallet/rpcwallet/rpcwallet.go +++ b/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,