From c5d3d76c33281cc320e52036452781abd500a815 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 1 Apr 2025 16:38:31 -0700 Subject: [PATCH 1/3] peer+server: downgrade to legacy coop close for taproot channels In this commit, we implement logic to downgrade to the legacy coop close for taproot channels. Before this commit, we wouldn't allow nodes to start up with both the taproot flag and the rbf flag activated. In the future, once we implement the spec updates, we'll add support for this combo, and can revert parts of this commit. --- peer/brontide.go | 37 ++++++++++++++++++++++++++----------- server.go | 11 ----------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/peer/brontide.go b/peer/brontide.go index 98414d7ef..bfc603ae8 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -1231,14 +1231,17 @@ func (p *Brontide) loadActiveChannels(chans []*channeldb.OpenChannel) ( return nil, err } + isTaprootChan := lnChan.ChanType().IsTaproot() + var ( shutdownMsg fn.Option[lnwire.Shutdown] shutdownInfoErr error ) shutdownInfo.WhenSome(func(info channeldb.ShutdownInfo) { // If we can use the new RBF close feature, we don't - // need to create the legacy closer. - if p.rbfCoopCloseAllowed() { + // need to create the legacy closer. However for taproot + // channels, we'll continue to use the legacy closer. + if p.rbfCoopCloseAllowed() && !isTaprootChan { return } @@ -1315,8 +1318,10 @@ func (p *Brontide) loadActiveChannels(chans []*channeldb.OpenChannel) ( p.activeChannels.Store(chanID, lnChan) // We're using the old co-op close, so we don't need to init - // the new RBF chan closer. - if !p.rbfCoopCloseAllowed() { + // the new RBF chan closer. If we have a taproot chan, then + // we'll also use the legacy type, so we don't need to make the + // new closer. + if !p.rbfCoopCloseAllowed() || isTaprootChan { continue } @@ -3268,6 +3273,8 @@ func chooseDeliveryScript(upfront, requested lnwire.DeliveryAddress, func (p *Brontide) restartCoopClose(lnChan *lnwallet.LightningChannel) ( *lnwire.Shutdown, error) { + isTaprootChan := lnChan.ChanType().IsTaproot() + // If this channel has status ChanStatusCoopBroadcasted and does not // have a closing transaction, then the cooperative close process was // started but never finished. We'll re-create the chanCloser state @@ -3320,8 +3327,8 @@ func (p *Brontide) restartCoopClose(lnChan *lnwallet.LightningChannel) ( // If the new RBF co-op close is negotiated, then we'll init and start // that state machine, skipping the steps for the negotiate machine - // below. - if p.rbfCoopCloseAllowed() { + // below. We don't support this close type for taproot channels though. + if p.rbfCoopCloseAllowed() && !isTaprootChan { _, err := p.initRbfChanCloser(lnChan) if err != nil { return nil, fmt.Errorf("unable to init rbf chan "+ @@ -4045,7 +4052,7 @@ func (p *Brontide) startRbfChanCloser(shutdown shutdownInit, chanID := lnwire.NewChanIDFromOutPoint(chanPoint) chanCloser, found := p.activeChanCloses.Load(chanID) if !found { - return fmt.Errorf("rbf can closer not found for channel %v", + return fmt.Errorf("rbf chan closer not found for channel %v", chanPoint) } @@ -4165,6 +4172,8 @@ func (p *Brontide) handleLocalCloseReq(req *htlcswitch.ChanClose) { return } + isTaprootChan := channel.ChanType().IsTaproot() + switch req.CloseType { // A type of CloseRegular indicates that the user has opted to close // out this channel on-chain, so we execute the cooperative channel @@ -4176,7 +4185,10 @@ func (p *Brontide) handleLocalCloseReq(req *htlcswitch.ChanClose) { // it to send the shutdown message. This also might be an RBF // iteration, in which case we'll be obtaining a new // transaction w/ a higher fee rate. - case p.rbfCoopCloseAllowed(): + // + // We don't support this close type for taproot channels yet + // however. + case !isTaprootChan && p.rbfCoopCloseAllowed(): err = p.startRbfChanCloser( newRPCShutdownInit(req), channel.ChannelPoint(), ) @@ -5135,9 +5147,12 @@ func (p *Brontide) addActiveChannel(c *lnpeer.NewChannel) error { "peer", chanPoint) } - // We're using the old co-op close, so we don't need to init the new - // RBF chan closer. - if !p.rbfCoopCloseAllowed() { + isTaprootChan := c.ChanType.IsTaproot() + + // We're using the old co-op close, so we don't need to init the new RBF + // chan closer. If this is a taproot channel, then we'll also fall + // through, as we don't support this type yet w/ rbf close. + if !p.rbfCoopCloseAllowed() || isTaprootChan { return nil } diff --git a/server.go b/server.go index 59c084f3a..e816c3ca4 100644 --- a/server.go +++ b/server.go @@ -612,17 +612,6 @@ func newServer(cfg *Config, listenAddrs []net.Addr, "aux controllers") } - // For now, the RBF coop close flag and the taproot channel type cannot - // be used together. - // - // TODO(roasbeef): fix - if cfg.ProtocolOptions.RbfCoopClose && - cfg.ProtocolOptions.TaprootChans { - - return nil, fmt.Errorf("RBF coop close and taproot " + - "channels cannot be used together") - } - //nolint:ll featureMgr, err := feature.NewManager(feature.Config{ NoTLVOnion: cfg.ProtocolOptions.LegacyOnion(), From 1fc2c6477088c0964277e56a3fd1ba4896e07d66 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 1 Apr 2025 16:39:51 -0700 Subject: [PATCH 2/3] itest: test all combos of rbf close and taproot chans In this commit, we test all the combinations of rbf close and taproot chans. This ensures that the downgrade logic works properly. Along the way we refactor the tests slightly, and also split them up, as running all the combos back to back mines more than 50 blocks in a test, which triggers an error in the itest sanity checks. --- itest/list_on_test.go | 4 + itest/lnd_coop_close_with_htlcs_test.go | 140 ++++++++++++++++-------- lntest/node/config.go | 6 + 3 files changed, 106 insertions(+), 44 deletions(-) diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 168465fb8..e4dbc32ca 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -610,6 +610,10 @@ var allTestCases = []*lntest.TestCase{ Name: "coop close with htlcs", TestFunc: testCoopCloseWithHtlcs, }, + { + Name: "coop close with htlcs restart", + TestFunc: testCoopCloseWithHtlcsWithRestart, + }, { Name: "coop close exceeds max fee", TestFunc: testCoopCloseExceedsMaxFee, diff --git a/itest/lnd_coop_close_with_htlcs_test.go b/itest/lnd_coop_close_with_htlcs_test.go index ae9ec9089..06d4dc9ab 100644 --- a/itest/lnd_coop_close_with_htlcs_test.go +++ b/itest/lnd_coop_close_with_htlcs_test.go @@ -18,60 +18,110 @@ import ( "github.com/stretchr/testify/require" ) +type flagCombo struct { + isRbf bool + isTaproot bool + testName string + flags []string + commitType lnrpc.CommitmentType + private bool +} + +// createFlagCombos generates a slice of flagCombo structs representing +// different test configurations for RBF and Taproot channels. +func createFlagCombos() []flagCombo { + var testCases []flagCombo + for _, isRbf := range []bool{true, false} { + for _, isTaproot := range []bool{true, false} { + flagCombo := flagCombo{ + isRbf: isRbf, + isTaproot: isTaproot, + } + + var flags []string + + if isRbf { + flags = append(flags, node.CfgRbfClose...) + } + + if isTaproot { + flags = append(flags, node.CfgSimpleTaproot...) + flagCombo.commitType = lnrpc.CommitmentType_SIMPLE_TAPROOT //nolint:ll + flagCombo.private = true + } + + flagCombo.flags = flags + flagCombo.testName = fmt.Sprintf( + "is_rbf=%v is_taproot=%v", isRbf, isTaproot, + ) + + testCases = append(testCases, flagCombo) + } + } + + return testCases +} + // testCoopCloseWithHtlcs tests whether we can successfully issue a coop close -// request while there are still active htlcs on the link. In all the tests, we +// request while there are still active htlcs on the link. In this test, we // will set up an HODL invoice to suspend settlement. Then we will attempt to // close the channel which should appear as a noop for the time being. Then we // will have the receiver settle the invoice and observe that the channel gets -// torn down after settlement. +// torn down after settlement. This test covers scenarios without node restarts. func testCoopCloseWithHtlcs(ht *lntest.HarnessTest) { - rbfCoopFlags := []string{"--protocol.rbf-coop-close"} + testCases := createFlagCombos() - for _, isRbf := range []bool{true, false} { - testName := fmt.Sprintf("no restart is_rbf=%v", isRbf) - ht.Run(testName, func(t *testing.T) { + for _, testCase := range testCases { + testCase := testCase // Capture range variable. + ht.Run(testCase.testName, func(t *testing.T) { tt := ht.Subtest(t) - var flags []string - if isRbf { - flags = rbfCoopFlags - } + alice := ht.NewNodeWithCoins("Alice", testCase.flags) + bob := ht.NewNodeWithCoins("bob", testCase.flags) - alice := ht.NewNodeWithCoins("Alice", flags) - bob := ht.NewNodeWithCoins("bob", flags) - - coopCloseWithHTLCs(tt, alice, bob) - }) - } - - for _, isRbf := range []bool{true, false} { - testName := fmt.Sprintf("with restart is_rbf=%v", isRbf) - ht.Run(testName, func(t *testing.T) { - tt := ht.Subtest(t) - - var flags []string - if isRbf { - flags = rbfCoopFlags - } - - alice := ht.NewNodeWithCoins("Alice", flags) - bob := ht.NewNodeWithCoins("bob", flags) - - coopCloseWithHTLCsWithRestart(tt, alice, bob) + runCoopCloseWithHTLCsScenario(tt, alice, bob, testCase) }) } } -// coopCloseWithHTLCs tests the basic coop close scenario which occurs when one -// channel party initiates a channel shutdown while an HTLC is still pending on -// the channel. -func coopCloseWithHTLCs(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { +// testCoopCloseWithHtlcsWithRestart tests whether we can successfully issue a +// coop close request while there are still active htlcs on the link, even +// after a node restart. In this test, we will set up an HODL invoice to +// suspend settlement. Then we will attempt to close the channel, restart the +// nodes, and then have the receiver settle the invoice, observing that the +// channel gets torn down after settlement and restart. +func testCoopCloseWithHtlcsWithRestart(ht *lntest.HarnessTest) { + testCases := createFlagCombos() + + for _, testCase := range testCases { + testCase := testCase // Capture range variable. + ht.Run(testCase.testName, func(t *testing.T) { + tt := ht.Subtest(t) + + alice := ht.NewNodeWithCoins("Alice", testCase.flags) + bob := ht.NewNodeWithCoins("bob", testCase.flags) + + runCoopCloseWithHTLCsWithRestartScenario( + tt, alice, bob, testCase, + ) + }) + } +} + +// runCoopCloseWithHTLCsScenario tests the basic coop close scenario which +// occurs when one channel party initiates a channel shutdown while an HTLC is +// still pending on the channel. +func runCoopCloseWithHTLCsScenario(ht *lntest.HarnessTest, alice, + bob *node.HarnessNode, testCase flagCombo) { + ht.ConnectNodes(alice, bob) // Here we set up a channel between Alice and Bob, beginning with a // balance on Bob's side. chanPoint := ht.OpenChannel(bob, alice, lntest.OpenChannelParams{ - Amt: btcutil.Amount(1000000), + Amt: btcutil.Amount(1000000), + CommitmentType: testCase.commitType, + Private: testCase.private, }) // Wait for Bob to understand that the channel is ready to use. @@ -153,12 +203,12 @@ func coopCloseWithHTLCs(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) { ht.AssertStreamChannelCoopClosed(alice, chanPoint, false, closeClient) } -// coopCloseWithHTLCsWithRestart also tests the coop close flow when an HTLC -// is still pending on the channel but this time it ensures that the shutdown -// process continues as expected even if a channel re-establish happens after -// one party has already initiated the shutdown. -func coopCloseWithHTLCsWithRestart(ht *lntest.HarnessTest, alice, - bob *node.HarnessNode) { +// runCoopCloseWithHTLCsWithRestartScenario also tests the coop close flow when +// an HTLC is still pending on the channel but this time it ensures that the +// shutdown process continues as expected even if a channel re-establish +// happens after one party has already initiated the shutdown. +func runCoopCloseWithHTLCsWithRestartScenario(ht *lntest.HarnessTest, alice, + bob *node.HarnessNode, testCase flagCombo) { ht.ConnectNodes(alice, bob) @@ -167,8 +217,10 @@ func coopCloseWithHTLCsWithRestart(ht *lntest.HarnessTest, alice, // so that we can assert that the correct delivery address gets used by // the channel close initiator. chanPoint := ht.OpenChannel(bob, alice, lntest.OpenChannelParams{ - Amt: btcutil.Amount(1000000), - PushAmt: btcutil.Amount(1000000 / 2), + Amt: btcutil.Amount(1000000), + PushAmt: btcutil.Amount(1000000 / 2), + CommitmentType: testCase.commitType, + Private: testCase.private, }) // Wait for Bob to understand that the channel is ready to use. diff --git a/lntest/node/config.go b/lntest/node/config.go index d71723aac..168dc87ec 100644 --- a/lntest/node/config.go +++ b/lntest/node/config.go @@ -68,6 +68,12 @@ var ( "--protocol.simple-taproot-chans", } + // CfgRbfCoopClose specifies the config used to create a node that + // supports the new RBF close protocol. + CfgRbfClose = []string{ + "--protocol.rbf-coop-close", + } + // CfgZeroConf specifies the config used to create a node that uses the // zero-conf channel feature. CfgZeroConf = []string{ From 96662ad43ab910152621ceeb9e169f4e40046308 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Fri, 4 Apr 2025 14:37:19 -0700 Subject: [PATCH 3/3] lncfg: mention that taproot chans aren't supported for rbf close --- lncfg/protocol.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lncfg/protocol.go b/lncfg/protocol.go index 71b81fe75..3c5220d72 100644 --- a/lncfg/protocol.go +++ b/lncfg/protocol.go @@ -37,7 +37,7 @@ type ProtocolOptions struct { // RbfCoopClose should be set if we want to signal that we support for // the new experimental RBF coop close feature. - RbfCoopClose bool `long:"rbf-coop-close" description:"if set, then lnd will signal that it supports the new RBF based coop close protocol"` + RbfCoopClose bool `long:"rbf-coop-close" description:"if set, then lnd will signal that it supports the new RBF based coop close protocol, taproot channels are not supported"` // NoAnchors should be set if we don't want to support opening or accepting // channels having the anchor commitment type.