Merge pull request #9750 from starius/fix-InternalKeyForAddr-for-imported-addresses

lnwallet: fix InternalKeyForAddr for imported addr

docs/release-notes/release-notes-0.19.0.md

@@ -118,6 +118,10 @@ keysend payment validation is stricter.
 * [Fixed](https://github.com/lightningnetwork/lnd/pull/9746) a possible panic
 when running LND with an aux component injected (custom channels).
 
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9750): if a Taproot
+  address is added to LND using the `ImportTapscript` RPC, LND previously failed
+  to perform a cooperative close to that address.
+
 # New Features
 
 * Add support for [archiving channel backup](https://github.com/lightningnetwork/lnd/pull/9232)

itest/list_exclude_test.go

@@ -13,7 +13,7 @@ import (
 // be excluded from the test suite atm.
 //
 // TODO(yy): fix these tests and remove them from this list.
-var excludedTestsWindows = []string{
+var excludedTestsWindows = append(append([]string{
 	"batch channel funding",
 	"zero conf channel open",
 	"open channel with unstable utxos",
@@ -45,26 +45,12 @@ var excludedTestsWindows = []string{
 	"wipe forwarding packages",
 
 	"coop close with htlcs",
-	"coop close with external delivery",
 
 	"forward interceptor restart",
 	"forward interceptor dedup htlcs",
 	"invoice HTLC modifier basic",
 	"lookup htlc resolution",
 
-	"remote signer-taproot",
-	"remote signer-account import",
-	"remote signer-bump fee",
-	"remote signer-funding input types",
-	"remote signer-funding async payments taproot",
-	"remote signer-funding async payments",
-	"remote signer-random seed",
-	"remote signer-verify msg",
-	"remote signer-channel open",
-	"remote signer-shared key",
-	"remote signer-psbt",
-	"remote signer-sign output raw",
-
 	"on chain to blinded",
 	"query blinded route",
 
@@ -76,7 +62,12 @@ var excludedTestsWindows = []string{
 	// more investigation is needed.
 	"channel force close-anchor restart",
 	"channel force close-simple taproot restart",
-}
+}, extractNames(
+	"coop close with external delivery",
+	coopCloseWithExternalTestCases)...,
+),
+	extractNames("remote signer", remoteSignerTestCases)...,
+)
 
 // filterWindowsFlakyTests filters out the flaky tests that are excluded from
 // the test suite on Windows.

itest/list_on_test.go

@@ -5,6 +5,7 @@ package itest
 import (
 	"fmt"
 
+	"github.com/lightningnetwork/lnd/fn/v2"
 	"github.com/lightningnetwork/lnd/lntest"
 )
 
@@ -642,10 +643,6 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "sweep commit output and anchor",
 		TestFunc: testSweepCommitOutputAndAnchor,
 	},
-	{
-		Name:     "coop close with external delivery",
-		TestFunc: testCoopCloseWithExternalDelivery,
-	},
 	{
 		Name:     "payment failed htlc local swept",
 		TestFunc: testPaymentFailedHTLCLocalSwept,
@@ -720,6 +717,13 @@ func appendPrefixed(prefix string, testCases,
 	return testCases
 }
 
+// extractNames is used to extract tests' names from a group of prefixed tests.
+func extractNames(prefix string, subtestCases []*lntest.TestCase) []string {
+	return fn.Map(subtestCases, func(tc *lntest.TestCase) string {
+		return fmt.Sprintf("%s-%s", prefix, tc.Name)
+	})
+}
+
 func init() {
 	// Register subtests.
 	allTestCases = appendPrefixed(
@@ -762,6 +766,10 @@ func init() {
 	allTestCases = appendPrefixed(
 		"wallet", allTestCases, walletTestCases,
 	)
+	allTestCases = appendPrefixed(
+		"coop close with external delivery", allTestCases,
+		coopCloseWithExternalTestCases,
+	)
 
 	// Prepare the test cases for windows to exclude some of the flaky
 	// ones.

itest/lnd_coop_close_external_delivery_test.go

@@ -2,75 +2,209 @@ package itest
 
 import (
 	"fmt"
-	"testing"
 
 	"github.com/btcsuite/btcd/btcutil"
+	"github.com/btcsuite/btcd/txscript"
 	"github.com/lightningnetwork/lnd/lnrpc"
+	"github.com/lightningnetwork/lnd/lnrpc/walletrpc"
 	"github.com/lightningnetwork/lnd/lntest"
 	"github.com/lightningnetwork/lnd/lntest/wait"
 	"github.com/stretchr/testify/require"
 )
 
-func testCoopCloseWithExternalDelivery(ht *lntest.HarnessTest) {
+var coopCloseWithExternalTestCases = []*lntest.TestCase{
-	ok := ht.Run("set P2WPKH delivery address at open", func(t *testing.T) {
+	{
-		tt := ht.Subtest(t)
+		Name: "set P2WPKH delivery address at open",
-		testCoopCloseWithExternalDeliveryImpl(
+		TestFunc: func(ht *lntest.HarnessTest) {
-			tt, true, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
+			testCoopCloseWithExternalDelivery(
-		)
+				ht, true, false, false,
-	})
+				lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
-	// Abort the test if failed.
+			)
-	if !ok {
+		},
-		return
+	},
-	}
+	{
-
+		Name: "set P2WPKH delivery address at close",
-	ok = ht.Run("set P2WPKH delivery address at close", func(t *testing.T) {
+		TestFunc: func(ht *lntest.HarnessTest) {
-		tt := ht.Subtest(t)
+			testCoopCloseWithExternalDelivery(
-		testCoopCloseWithExternalDeliveryImpl(
+				ht, false, false, false,
-			tt, false, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
+				lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
-		)
+			)
-	})
+		},
-	// Abort the test if failed.
+	},
-	if !ok {
+	{
-		return
+		Name: "set P2TR delivery address at open",
-	}
+		TestFunc: func(ht *lntest.HarnessTest) {
-
+			testCoopCloseWithExternalDelivery(
-	ok = ht.Run("set P2TR delivery address at open", func(t *testing.T) {
+				ht, true, false, false,
-		tt := ht.Subtest(t)
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
-		testCoopCloseWithExternalDeliveryImpl(
+			)
-			tt, true, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+		},
-		)
+	},
-	})
+	{
-	// Abort the test if failed.
+		Name: "set P2TR delivery address at close",
-	if !ok {
+		TestFunc: func(ht *lntest.HarnessTest) {
-		return
+			testCoopCloseWithExternalDelivery(
-	}
+				ht, false, false, false,
-
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
-	ht.Run("set P2TR delivery address at close", func(t *testing.T) {
+			)
-		tt := ht.Subtest(t)
+		},
-		testCoopCloseWithExternalDeliveryImpl(
+	},
-			tt, false, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+	{
-		)
+		Name: "set imported P2TR address (ImportTapscript) at open",
-	})
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, true, true, false,
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+			)
+		},
+	},
+	{
+		Name: "set imported P2TR address (ImportTapscript) at close",
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, false, true, false,
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+			)
+		},
+	},
+	{
+		Name: "set imported P2WPKH address at open",
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, true, false, true,
+				lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
+			)
+		},
+	},
+	{
+		Name: "set imported P2WPKH address at close",
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, false, false, true,
+				lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH,
+			)
+		},
+	},
+	{
+		Name: "set imported P2TR address (ImportPublicKey) at open",
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, true, false, true,
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+			)
+		},
+	},
+	{
+		Name: "set imported P2TR address (ImportPublicKey) at close",
+		TestFunc: func(ht *lntest.HarnessTest) {
+			testCoopCloseWithExternalDelivery(
+				ht, false, false, true,
+				lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY,
+			)
+		},
+	},
 }
 
-// testCoopCloseWithExternalDeliveryImpl ensures that we have a valid settled
+// testCoopCloseWithExternalDelivery ensures that we have a valid settled
 // balance irrespective of whether the delivery address is in LND's wallet or
 // not. Some users set this value to be an address in a different wallet and
 // this should not affect our ability to accurately report the settled balance.
-func testCoopCloseWithExternalDeliveryImpl(ht *lntest.HarnessTest,
+//
-	upfrontShutdown bool, deliveryAddressType lnrpc.AddressType) {
+// If importTapscript is set, it imports a Taproot script and internal key to
+// Alice's LND using ImportTapscript to make sure it doesn't interfere with
+// delivery address. If importPubkey is set, the address is imported using
+// ImportPublicKey.
+func testCoopCloseWithExternalDelivery(ht *lntest.HarnessTest,
+	upfrontShutdown, importTapscript, importPubkey bool,
+	deliveryAddressType lnrpc.AddressType) {
 
 	alice := ht.NewNodeWithCoins("Alice", nil)
 	bob := ht.NewNodeWithCoins("bob", nil)
 	ht.ConnectNodes(alice, bob)
 
+	// Make fake taproot internal public key (equal to the final).
+	// This is only used if importTapscript or importPubkey is set.
+	taprootPubkey := [32]byte{1, 2, 3}
+	if upfrontShutdown {
+		// Make new address for second sub-test not to import
+		// the same address twice causing an error.
+		taprootPubkey[3] = 1
+	}
+	pkScriptBytes := append(
+		[]byte{txscript.OP_1, txscript.OP_DATA_32},
+		taprootPubkey[:]...,
+	)
+	pkScript, err := txscript.ParsePkScript(pkScriptBytes)
+	require.NoError(ht, err)
+	taprootAddress, err := pkScript.Address(harnessNetParams)
+	require.NoError(ht, err)
+
+	var addr string
+	switch {
+	// Use ImportTapscript.
+	case importTapscript:
+		addr = taprootAddress.String()
+
+		// Import the taproot address to LND.
+		req := &walletrpc.ImportTapscriptRequest{
+			InternalPublicKey: taprootPubkey[:],
+			Script: &walletrpc.ImportTapscriptRequest_FullKeyOnly{
+				FullKeyOnly: true,
+			},
+		}
+		res := alice.RPC.ImportTapscript(req)
+		require.Equal(ht, addr, res.P2TrAddress)
+
+	// Use ImportPublicKey.
+	case importPubkey:
+		var (
+			address     btcutil.Address
+			pubKey      []byte
+			addressType walletrpc.AddressType
+		)
+		switch deliveryAddressType {
+		case lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH:
+			// Make fake public key hash.
+			pk := [33]byte{2, 3, 4}
+			if upfrontShutdown {
+				// Make new address for second sub-test.
+				pk[1]++
+			}
+			address, err = btcutil.NewAddressWitnessPubKeyHash(
+				btcutil.Hash160(pk[:]), harnessNetParams,
+			)
+			require.NoError(ht, err)
+			pubKey = pk[:]
+			addressType = walletrpc.AddressType_WITNESS_PUBKEY_HASH
+
+		case lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY:
+			address = taprootAddress
+			pubKey = taprootPubkey[:]
+			addressType = walletrpc.AddressType_TAPROOT_PUBKEY
+
+		default:
+			ht.Fatalf("not allowed address type: %v",
+				deliveryAddressType)
+		}
+
+		addr = address.String()
+
+		// Import the address to LND.
+		alice.RPC.ImportPublicKey(&walletrpc.ImportPublicKeyRequest{
+			PublicKey:   pubKey,
+			AddressType: addressType,
+		})
+
 	// Here we generate a final delivery address in bob's wallet but set by
 	// alice. We do this to ensure that the address is not in alice's LND
 	// wallet. We already correctly track settled balances when the address
 	// is in the LND wallet.
-	addr := bob.RPC.NewAddress(&lnrpc.NewAddressRequest{
+	default:
-		Type: deliveryAddressType,
+		res := bob.RPC.NewAddress(&lnrpc.NewAddressRequest{
-	})
+			Type: deliveryAddressType,
+		})
+		addr = res.Address
+	}
 
 	// Prepare for channel open.
 	openParams := lntest.OpenChannelParams{
@@ -80,7 +214,7 @@ func testCoopCloseWithExternalDeliveryImpl(ht *lntest.HarnessTest,
 	// If we are testing the case where we set it on open then we'll set the
 	// upfront shutdown script in the channel open parameters.
 	if upfrontShutdown {
-		openParams.CloseAddress = addr.Address
+		openParams.CloseAddress = addr
 	}
 
 	// Open the channel!
@@ -95,14 +229,14 @@ func testCoopCloseWithExternalDeliveryImpl(ht *lntest.HarnessTest,
 	// If we are testing the case where we set the delivery address on
 	// channel close then we will set it in the channel close parameters.
 	if !upfrontShutdown {
-		closeParams.DeliveryAddress = addr.Address
+		closeParams.DeliveryAddress = addr
 	}
 
 	// Close the channel!
 	closeClient := alice.RPC.CloseChannel(&closeParams)
 
 	// Assert that we got a channel update when we get a closing txid.
-	_, err := closeClient.Recv()
+	_, err = closeClient.Recv()
 	require.NoError(ht, err)
 
 	// Mine the closing transaction.

lnwallet/interface.go

@@ -660,10 +660,16 @@ func InternalKeyForAddr(wallet WalletController, netParams *chaincfg.Params,
 		return none, nil
 	}
 
+	// Imported addresses do not provide private keys, so they do not
+	// implement waddrmgr.ManagedPubKeyAddress. See RPC ImportTapscript.
+	if walletAddr.Imported() {
+		return none, nil
+	}
+
 	pubKeyAddr, ok := walletAddr.(waddrmgr.ManagedPubKeyAddress)
 	if !ok {
 		return none, fmt.Errorf("expected pubkey addr, got %T",
-			pubKeyAddr)
+			walletAddr)
 	}
 
 	_, derivationPath, _ := pubKeyAddr.DerivationInfo()