Merge pull request #9669 from Roasbeef/rbf-taproot-downgrade

multi: downgrade to legacy coop close for taproot channels

Author: Roasbeef

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,

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", flags)
-			bob := ht.NewNodeWithCoins("bob", flags)
+			alice := ht.NewNodeWithCoins("Alice", testCase.flags)
+			bob := ht.NewNodeWithCoins("bob", testCase.flags)
 
-			coopCloseWithHTLCs(tt, alice, bob)
+			runCoopCloseWithHTLCsScenario(tt, alice, bob, testCase)
 		})
 	}
+}
 
-	for _, isRbf := range []bool{true, false} {
-		testName := fmt.Sprintf("with restart is_rbf=%v", isRbf)
-		ht.Run(testName, func(t *testing.T) {
+// 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)
 
-			var flags []string
-			if isRbf {
-				flags = rbfCoopFlags
-			}
-
-			alice := ht.NewNodeWithCoins("Alice", flags)
-			bob := ht.NewNodeWithCoins("bob", flags)
+			alice := ht.NewNodeWithCoins("Alice", testCase.flags)
+			bob := ht.NewNodeWithCoins("bob", testCase.flags)
 
-			coopCloseWithHTLCsWithRestart(tt, alice, bob)
+			runCoopCloseWithHTLCsWithRestartScenario(
+				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) {
+// 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.

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.

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{

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
 	}
 

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(),