From b5c9df81f0a42061463e6401d9d20bda111308ae Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Mon, 21 Apr 2025 17:03:08 -0300 Subject: [PATCH 1/5] lnwallet: fix error message Include the variable of interest (walletAddr), not the outcome of the check (pubKeyAddr) which is always nil. --- lnwallet/interface.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lnwallet/interface.go b/lnwallet/interface.go index 64f854631..767550ec1 100644 --- a/lnwallet/interface.go +++ b/lnwallet/interface.go @@ -663,7 +663,7 @@ func InternalKeyForAddr(wallet WalletController, netParams *chaincfg.Params, pubKeyAddr, ok := walletAddr.(waddrmgr.ManagedPubKeyAddress) if !ok { return none, fmt.Errorf("expected pubkey addr, got %T", - pubKeyAddr) + walletAddr) } _, derivationPath, _ := pubKeyAddr.DerivationInfo() From 429000e360d24c78fcf972a8432b59ff1ff26350 Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Mon, 21 Apr 2025 17:09:17 -0300 Subject: [PATCH 2/5] lnwallet: fix InternalKeyForAddr for imported addr An address imported using ImportTapscript doesn't provide a private key so it can't satisfy waddrmgr.ManagedPubKeyAddress interface. So we don't return an error for imported addresses. Fix https://github.com/lightninglabs/loop/issues/923 --- lnwallet/interface.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lnwallet/interface.go b/lnwallet/interface.go index 767550ec1..9255c513e 100644 --- a/lnwallet/interface.go +++ b/lnwallet/interface.go @@ -660,6 +660,12 @@ 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", From 6451ce495af65794536791871c81fe672960a1ca Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Tue, 22 Apr 2025 10:44:40 -0300 Subject: [PATCH 3/5] itest: use prefixed sub-tests More sub-tests are needed in "coop close with external delivery" itest, which would break the limit for 50 blocks mined. Turning it into prefixed itest. Also used new exclusion approach in test "remote signer" where all the prefixed tests are excluded. --- itest/list_exclude_test.go | 23 ++---- itest/list_on_test.go | 16 +++- .../lnd_coop_close_external_delivery_test.go | 82 +++++++++---------- 3 files changed, 58 insertions(+), 63 deletions(-) diff --git a/itest/list_exclude_test.go b/itest/list_exclude_test.go index fd0e1c03a..77a7d078e 100644 --- a/itest/list_exclude_test.go +++ b/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. diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 320f69f9e..88b7bb395 100644 --- a/itest/list_on_test.go +++ b/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. diff --git a/itest/lnd_coop_close_external_delivery_test.go b/itest/lnd_coop_close_external_delivery_test.go index eea09a48d..e2c84fab1 100644 --- a/itest/lnd_coop_close_external_delivery_test.go +++ b/itest/lnd_coop_close_external_delivery_test.go @@ -2,7 +2,6 @@ package itest import ( "fmt" - "testing" "github.com/btcsuite/btcd/btcutil" "github.com/lightningnetwork/lnd/lnrpc" @@ -11,53 +10,50 @@ import ( "github.com/stretchr/testify/require" ) -func testCoopCloseWithExternalDelivery(ht *lntest.HarnessTest) { - ok := ht.Run("set P2WPKH delivery address at open", func(t *testing.T) { - tt := ht.Subtest(t) - testCoopCloseWithExternalDeliveryImpl( - tt, true, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, - ) - }) - // Abort the test if failed. - if !ok { - return - } - - ok = ht.Run("set P2WPKH delivery address at close", func(t *testing.T) { - tt := ht.Subtest(t) - testCoopCloseWithExternalDeliveryImpl( - tt, false, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, - ) - }) - // Abort the test if failed. - if !ok { - return - } - - ok = ht.Run("set P2TR delivery address at open", func(t *testing.T) { - tt := ht.Subtest(t) - testCoopCloseWithExternalDeliveryImpl( - tt, true, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY, - ) - }) - // Abort the test if failed. - if !ok { - return - } - - ht.Run("set P2TR delivery address at close", func(t *testing.T) { - tt := ht.Subtest(t) - testCoopCloseWithExternalDeliveryImpl( - tt, false, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY, - ) - }) +var coopCloseWithExternalTestCases = []*lntest.TestCase{ + { + Name: "set P2WPKH delivery address at open", + TestFunc: func(ht *lntest.HarnessTest) { + testCoopCloseWithExternalDelivery( + ht, true, + lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, + ) + }, + }, + { + Name: "set P2WPKH delivery address at close", + TestFunc: func(ht *lntest.HarnessTest) { + testCoopCloseWithExternalDelivery( + ht, false, + lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, + ) + }, + }, + { + Name: "set P2TR delivery address at open", + TestFunc: func(ht *lntest.HarnessTest) { + testCoopCloseWithExternalDelivery( + ht, true, + lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY, + ) + }, + }, + { + Name: "set P2TR delivery address at close", + TestFunc: func(ht *lntest.HarnessTest) { + testCoopCloseWithExternalDelivery( + ht, false, + 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, +func testCoopCloseWithExternalDelivery(ht *lntest.HarnessTest, upfrontShutdown bool, deliveryAddressType lnrpc.AddressType) { alice := ht.NewNodeWithCoins("Alice", nil) From c9b597434177e62198f80ff7202bee067b0067a8 Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Tue, 22 Apr 2025 11:03:18 -0300 Subject: [PATCH 4/5] itest: test imported address in coop close Make sure that an address imported to LND via ImportTapscript or ImportPublicKey can be used as a delivery address in coop close. New test cases: - P2TR address imported with ImportTapscript - P2TR address imported with ImportPublicKey - P2WPKH address imported with ImportPublicKey Safeguard against https://github.com/lightninglabs/loop/issues/923 --- .../lnd_coop_close_external_delivery_test.go | 160 ++++++++++++++++-- 1 file changed, 149 insertions(+), 11 deletions(-) diff --git a/itest/lnd_coop_close_external_delivery_test.go b/itest/lnd_coop_close_external_delivery_test.go index e2c84fab1..4c2f0a5c4 100644 --- a/itest/lnd_coop_close_external_delivery_test.go +++ b/itest/lnd_coop_close_external_delivery_test.go @@ -4,7 +4,9 @@ import ( "fmt" "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" @@ -15,7 +17,7 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ Name: "set P2WPKH delivery address at open", TestFunc: func(ht *lntest.HarnessTest) { testCoopCloseWithExternalDelivery( - ht, true, + ht, true, false, false, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, ) }, @@ -24,7 +26,7 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ Name: "set P2WPKH delivery address at close", TestFunc: func(ht *lntest.HarnessTest) { testCoopCloseWithExternalDelivery( - ht, false, + ht, false, false, false, lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, ) }, @@ -33,7 +35,7 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ Name: "set P2TR delivery address at open", TestFunc: func(ht *lntest.HarnessTest) { testCoopCloseWithExternalDelivery( - ht, true, + ht, true, false, false, lnrpc.AddressType_UNUSED_TAPROOT_PUBKEY, ) }, @@ -42,7 +44,61 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ Name: "set P2TR delivery address at close", TestFunc: func(ht *lntest.HarnessTest) { testCoopCloseWithExternalDelivery( - ht, false, + ht, false, false, 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, ) }, @@ -53,20 +109,102 @@ var coopCloseWithExternalTestCases = []*lntest.TestCase{ // 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. +// +// 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 bool, deliveryAddressType lnrpc.AddressType) { + 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{ - Type: deliveryAddressType, - }) + default: + res := bob.RPC.NewAddress(&lnrpc.NewAddressRequest{ + Type: deliveryAddressType, + }) + addr = res.Address + } // Prepare for channel open. openParams := lntest.OpenChannelParams{ @@ -76,7 +214,7 @@ func testCoopCloseWithExternalDelivery(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! @@ -91,14 +229,14 @@ func testCoopCloseWithExternalDelivery(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. From dc2cad9b0f6ecc30013f222798b6056ef5d823e3 Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Mon, 21 Apr 2025 22:29:24 -0300 Subject: [PATCH 5/5] docs: update release-notes for 0.19.0 --- docs/release-notes/release-notes-0.19.0.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index bdbf12735..6e93baf9b 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/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)