From 56048133f2f2e7f8d7e061a348b107c9a72c5418 Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Thu, 30 May 2024 17:44:30 -0700 Subject: [PATCH 1/3] itest+lntest: add itest to reproduce bug #8535 --- itest/list_on_test.go | 4 + .../lnd_coop_close_external_delivery_test.go | 92 +++++++++++++++++++ lntest/harness.go | 5 + 3 files changed, 101 insertions(+) create mode 100644 itest/lnd_coop_close_external_delivery_test.go diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 1c563aa20..3619c2282 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -626,4 +626,8 @@ var allTestCases = []*lntest.TestCase{ Name: "sweep commit output and anchor", TestFunc: testSweepCommitOutputAndAnchor, }, + { + Name: "coop close with external delivery", + TestFunc: testCoopCloseWithExternalDelivery, + }, } diff --git a/itest/lnd_coop_close_external_delivery_test.go b/itest/lnd_coop_close_external_delivery_test.go new file mode 100644 index 000000000..755792303 --- /dev/null +++ b/itest/lnd_coop_close_external_delivery_test.go @@ -0,0 +1,92 @@ +package itest + +import ( + "testing" + + "github.com/btcsuite/btcd/btcutil" + "github.com/lightningnetwork/lnd/lnrpc" + "github.com/lightningnetwork/lnd/lntest" + "github.com/stretchr/testify/require" +) + +func testCoopCloseWithExternalDelivery(ht *lntest.HarnessTest) { + ht.Run("set delivery address at open", func(t *testing.T) { + tt := ht.Subtest(t) + testCoopCloseWithExternalDeliveryImpl(tt, true) + }) + ht.Run("set delivery address at close", func(t *testing.T) { + tt := ht.Subtest(t) + testCoopCloseWithExternalDeliveryImpl(tt, false) + }) +} + +// testCoopCloseWithExternalDeliveryImpl 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) { + + alice, bob := ht.Alice, ht.Bob + ht.ConnectNodes(alice, bob) + + // 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: lnrpc.AddressType_UNUSED_WITNESS_PUBKEY_HASH, + }) + + // Prepare for channel open. + openParams := lntest.OpenChannelParams{ + Amt: btcutil.Amount(1000000), + } + + // 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 + } + + // Open the channel! + chanPoint := ht.OpenChannel(alice, bob, openParams) + + // Prepare for channel close. + closeParams := lnrpc.CloseChannelRequest{ + ChannelPoint: chanPoint, + TargetConf: 6, + } + + // 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 + } + + // Close the channel! + closeClient := alice.RPC.CloseChannel(&closeParams) + + // Assert that we got a channel update when we get a closing txid. + _, err := closeClient.Recv() + require.NoError(ht, err) + + // Mine the closing transaction. + ht.MineClosingTx(chanPoint) + + // Assert that we got a channel update when the closing tx was mined. + _, err = closeClient.Recv() + require.NoError(ht, err) + + // Here we query our closed channels to conduct the final test + // assertion. We want to ensure that even though alice's delivery + // address is set to an address in bob's wallet, we should still show + // the balance as settled. + closed := alice.RPC.ClosedChannels(&lnrpc.ClosedChannelsRequest{ + Cooperative: true, + }) + + // The settled balance should never be zero at this point. + require.NotZero(ht, len(closed.Channels)) + require.NotZero(ht, closed.Channels[0].SettledBalance) +} diff --git a/lntest/harness.go b/lntest/harness.go index 4c26e1aa7..92549427f 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -1007,6 +1007,10 @@ type OpenChannelParams struct { // FundMax flag is specified the entirety of selected funds is // allocated towards channel funding. Outpoints []*lnrpc.OutPoint + + // CloseAddress sets the upfront_shutdown_script parameter during + // channel open. It is expected to be encoded as a bitcoin address. + CloseAddress string } // prepareOpenChannel waits for both nodes to be synced to chain and returns an @@ -1059,6 +1063,7 @@ func (h *HarnessTest) prepareOpenChannel(srcNode, destNode *node.HarnessNode, FundMax: p.FundMax, Memo: p.Memo, Outpoints: p.Outpoints, + CloseAddress: p.CloseAddress, } } From 30e10322b289e54bc92590691962ee486f0d126a Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Fri, 31 May 2024 15:06:01 -0700 Subject: [PATCH 2/3] contractcourt: consider delivery addresses when evaluating toSelfAmount This commit fixes #8535 by changing how we assess toSelfAmount inside the chainWatcher. In certain cases users may wish to close out channel funds to external delivery addresses set either during open or close. Prior to this change we only consider addresses that our wallet is aware of. This change now identifies outputs as to_self outputs if the delivery script matches OR if our wallet is aware of the address. In certain edge cases it can be possible for there to be more than one output that matches these criteria and in that case we will return the sum of those values. --- contractcourt/chain_watcher.go | 74 ++++++++++++++++++++++++++-------- go.mod | 2 +- go.sum | 4 +- 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index 55ac0979c..b17b40aca 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -3,6 +3,7 @@ package contractcourt import ( "bytes" "fmt" + "slices" "sync" "sync/atomic" "time" @@ -16,8 +17,10 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" + "github.com/lightningnetwork/lnd/fn" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwire" ) const ( @@ -970,28 +973,67 @@ func (c *chainWatcher) handleUnknownRemoteState( } // toSelfAmount takes a transaction and returns the sum of all outputs that pay -// to a script that the wallet controls. If no outputs pay to us, then we +// to a script that the wallet controls or the channel defines as its delivery +// script . If no outputs pay to us (determined by these criteria), then we // return zero. This is possible as our output may have been trimmed due to // being dust. func (c *chainWatcher) toSelfAmount(tx *wire.MsgTx) btcutil.Amount { - var selfAmt btcutil.Amount - for _, txOut := range tx.TxOut { - _, addrs, _, err := txscript.ExtractPkScriptAddrs( - // Doesn't matter what net we actually pass in. - txOut.PkScript, &chaincfg.TestNet3Params, - ) - if err != nil { - continue - } + // There are two main cases we have to handle here. First, in the coop + // close case we will always have saved the delivery address we used + // whether it was from the upfront shutdown, from the delivery address + // requested at close time, or even an automatically generated one. All + // coop-close cases can be identified in the following manner: + shutdown, _ := c.cfg.chanState.ShutdownInfo() + oDeliveryAddr := fn.MapOption( + func(i channeldb.ShutdownInfo) lnwire.DeliveryAddress { + return i.DeliveryScript.Val + })(shutdown) - for _, addr := range addrs { - if c.cfg.isOurAddr(addr) { - selfAmt += btcutil.Amount(txOut.Value) - } - } + // Here we define a function capable of identifying whether an output + // corresponds with our local delivery script from a ShutdownInfo if we + // have a ShutdownInfo for this chainWatcher's underlying channel. + // + // isDeliveryOutput :: *TxOut -> bool + isDeliveryOutput := func(o *wire.TxOut) bool { + return fn.ElimOption( + oDeliveryAddr, + // If we don't have a delivery addr, then the output + // can't match it. + func() bool { return false }, + // Otherwise if the PkScript of the TxOut matches our + // delivery script then this is a delivery output. + func(a lnwire.DeliveryAddress) bool { + return slices.Equal(a, o.PkScript) + }, + ) } - return selfAmt + // Here we define a function capable of identifying whether an output + // belongs to the LND wallet. We use this as a heuristic in the case + // where we might be looking for spendable force closure outputs. + // + // isWalletOutput :: *TxOut -> bool + isWalletOutput := func(out *wire.TxOut) bool { + _, addrs, _, err := txscript.ExtractPkScriptAddrs( + // Doesn't matter what net we actually pass in. + out.PkScript, &chaincfg.TestNet3Params, + ) + if err != nil { + return false + } + + return fn.Any(c.cfg.isOurAddr, addrs) + } + + // Grab all of the outputs that correspond with our delivery address + // or our wallet is aware of. + outs := fn.Filter(fn.PredOr(isDeliveryOutput, isWalletOutput), tx.TxOut) + + // Grab the values for those outputs. + vals := fn.Map(func(o *wire.TxOut) int64 { return o.Value }, outs) + + // Return the sum. + return btcutil.Amount(fn.Sum(vals)) } // dispatchCooperativeClose processed a detect cooperative channel closure. diff --git a/go.mod b/go.mod index 55898d79a..8d0683932 100644 --- a/go.mod +++ b/go.mod @@ -35,7 +35,7 @@ require ( github.com/lightningnetwork/lightning-onion v1.2.1-0.20230823005744-06182b1d7d2f github.com/lightningnetwork/lnd/cert v1.2.2 github.com/lightningnetwork/lnd/clock v1.1.1 - github.com/lightningnetwork/lnd/fn v1.0.5 + github.com/lightningnetwork/lnd/fn v1.0.9 github.com/lightningnetwork/lnd/healthcheck v1.2.4 github.com/lightningnetwork/lnd/kvdb v1.4.8 github.com/lightningnetwork/lnd/queue v1.1.1 diff --git a/go.sum b/go.sum index 5a21d5439..2f83baf01 100644 --- a/go.sum +++ b/go.sum @@ -448,8 +448,8 @@ github.com/lightningnetwork/lnd/cert v1.2.2 h1:71YK6hogeJtxSxw2teq3eGeuy4rHGKcFf github.com/lightningnetwork/lnd/cert v1.2.2/go.mod h1:jQmFn/Ez4zhDgq2hnYSw8r35bqGVxViXhX6Cd7HXM6U= github.com/lightningnetwork/lnd/clock v1.1.1 h1:OfR3/zcJd2RhH0RU+zX/77c0ZiOnIMsDIBjgjWdZgA0= github.com/lightningnetwork/lnd/clock v1.1.1/go.mod h1:mGnAhPyjYZQJmebS7aevElXKTFDuO+uNFFfMXK1W8xQ= -github.com/lightningnetwork/lnd/fn v1.0.5 h1:ffDgMSn83avw6rNzxhbt6w5/2oIrwQKTPGfyaLupZtE= -github.com/lightningnetwork/lnd/fn v1.0.5/go.mod h1:P027+0CyELd92H9gnReUkGGAqbFA1HwjHWdfaDFD51U= +github.com/lightningnetwork/lnd/fn v1.0.9 h1:VPljrzHGh0Wfs2NZe/ugUfH0hl6/L2eXW0LLXMUEy3s= +github.com/lightningnetwork/lnd/fn v1.0.9/go.mod h1:P027+0CyELd92H9gnReUkGGAqbFA1HwjHWdfaDFD51U= github.com/lightningnetwork/lnd/healthcheck v1.2.4 h1:lLPLac+p/TllByxGSlkCwkJlkddqMP5UCoawCj3mgFQ= github.com/lightningnetwork/lnd/healthcheck v1.2.4/go.mod h1:G7Tst2tVvWo7cx6mSBEToQC5L1XOGxzZTPB29g9Rv2I= github.com/lightningnetwork/lnd/kvdb v1.4.8 h1:xH0a5Vi1yrcZ5BEeF2ba3vlKBRxrL9uYXlWTjOjbNTY= From 1fea14f69f8ea5b48b3cbfa7cac17b926e26f3ba Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Wed, 5 Jun 2024 16:17:39 -0700 Subject: [PATCH 3/3] docs: update release notes --- docs/release-notes/release-notes-0.18.1.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.1.md b/docs/release-notes/release-notes-0.18.1.md index e2288f166..db01d6222 100644 --- a/docs/release-notes/release-notes-0.18.1.md +++ b/docs/release-notes/release-notes-0.18.1.md @@ -19,6 +19,10 @@ # Bug Fixes +* `closedchannels` now [successfully reports](https://github.com/lightningnetwork/lnd/pull/8800) + settled balances even if the delivery address is set to an address that + LND does not control. + # New Features ## Functional Enhancements ## RPC Additions