Merge pull request #10962 from jtobin/rbf-guard

peer: never use RBF coop close for aux channels

docs/release-notes/release-notes-0.21.2.md

@@ -26,6 +26,9 @@
   in onion message decoding where messages that should have been rejected
   per BOLT 4 were instead accepted, or a valid TLV was dropped.
 
+* [Fixes a bug](https://github.com/lightningnetwork/lnd/pull/10962) that
+  could allow the RBF closer to be used with incompatible aux channels.
+
 # New Features
 
 ## Functional Enhancements
@@ -69,3 +72,4 @@
 # Contributors (Alphabetical Order)
 
 * bitromortac
+* Jared Tobin

peer/brontide.go

@@ -1089,16 +1089,26 @@ func (p *Brontide) taprootShutdownAllowed() bool {
 		p.LocalFeatures().HasFeature(lnwire.ShutdownAnySegwitOptional)
 }
 
-// rbfCoopCloseAllowed returns true if both parties have negotiated the new RBF
-// coop close feature.
-func (p *Brontide) rbfCoopCloseAllowed() bool {
+// rbfCoopCloseAllowed returns true if the new RBF coop close flow can be
+// used for a channel of the given type: both parties must have negotiated
+// the RBF coop close feature, and the channel must not be an aux channel.
+// Aux channels (taproot overlay channels, marked by a tapscript root) are
+// excluded even when both peers signal the RBF feature bit: the RBF close
+// state machine does not invoke any of the aux closer hooks, so closing an
+// aux channel through it would produce a close transaction without the aux
+// outputs (destroying the committed assets), which the aux closer is then
+// unable to finalize once the transaction confirms. Such channels fall back
+// to the legacy negotiate closer, which is aux-aware.
+func (p *Brontide) rbfCoopCloseAllowed(chanType chanstate.ChannelType) bool {
 	bothHaveBit := func(bit lnwire.FeatureBit) bool {
 		return p.RemoteFeatures().HasFeature(bit) &&
 			p.LocalFeatures().HasFeature(bit)
 	}
 
-	return bothHaveBit(lnwire.RbfCoopCloseOptional) ||
+	featureNegotiated := bothHaveBit(lnwire.RbfCoopCloseOptional) ||
 		bothHaveBit(lnwire.RbfCoopCloseOptionalStaging)
+
+	return featureNegotiated && !chanType.HasTapscriptRoot()
 }
 
 // QuitSignal is a method that should return a channel which will be sent upon
@@ -1377,9 +1387,9 @@ func (p *Brontide) loadActiveChannels(chans []*chanstate.OpenChannel) (
 			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() {
+			// If we can use the new RBF close feature for this
+			// channel, we don't need to create the legacy closer.
+			if p.rbfCoopCloseAllowed(dbChan.ChanType) {
 				return
 			}
 
@@ -1455,9 +1465,9 @@ func (p *Brontide) loadActiveChannels(chans []*chanstate.OpenChannel) (
 
 		p.storeActiveChannel(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() {
+		// We're using the old co-op close for this channel, so we
+		// don't need to init the new RBF chan closer.
+		if !p.rbfCoopCloseAllowed(dbChan.ChanType) {
 			continue
 		}
 
@@ -3640,13 +3650,15 @@ func (p *Brontide) restartCoopClose(lnChan *lnwallet.LightningChannel) (
 	// the LocalUpfrontShutdownScript or generate a script.
 	c := lnChan.State()
 	_, err := c.BroadcastedCooperative()
-	if err != nil && err != channeldb.ErrNoCloseTx {
-		// An error other than ErrNoCloseTx was encountered.
+
+	// Any error other than "no close tx" is a real failure.
+	if err != nil && !errors.Is(err, channeldb.ErrNoCloseTx) {
 		return nil, err
-	} else if err == nil && !p.rbfCoopCloseAllowed() {
-		// This is a channel that doesn't support RBF coop close, and it
-		// already had a coop close txn broadcast. As a result, we can
-		// just exit here as all we can do is wait for it to confirm.
+	}
+
+	// A close tx was already broadcast and this channel can't use RBF
+	// coop close, so all we can do is wait for it to confirm.
+	if err == nil && !p.rbfCoopCloseAllowed(c.ChanType) {
 		return nil, nil
 	}
 
@@ -3681,10 +3693,10 @@ 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() {
+	// If the new RBF co-op close is negotiated and usable for this
+	// channel, then we'll init and start that state machine, skipping the
+	// steps for the negotiate machine below.
+	if p.rbfCoopCloseAllowed(c.ChanType) {
 		_, err := p.initRbfChanCloser(lnChan)
 		if err != nil {
 			return nil, fmt.Errorf("unable to init rbf chan "+
@@ -4172,6 +4184,15 @@ func (p *Brontide) chanFlushEventSentinel(chanCloser *chancloser.RbfChanCloser,
 func (p *Brontide) initRbfChanCloser(
 	channel *lnwallet.LightningChannel) (*chancloser.RbfChanCloser, error) {
 
+	// Aux channels can't use the RBF coop close flow, as the state
+	// machine doesn't invoke the aux closer hooks needed to construct an
+	// aux-aware close transaction.
+	if channel.ChanType().HasTapscriptRoot() {
+		return nil, fmt.Errorf("ChannelPoint(%v): RBF coop close "+
+			"not supported for aux channels",
+			channel.ChannelPoint())
+	}
+
 	chanID := lnwire.NewChanIDFromOutPoint(channel.ChannelPoint())
 
 	link := p.fetchLinkFromKeyAndCid(chanID)
@@ -4595,7 +4616,7 @@ func (p *Brontide) handleLocalCloseReq(req *htlcswitch.ChanClose) {
 		// iteration, in which case we'll be obtaining a new
 		// transaction w/ a higher fee rate.
 		//
-		case p.rbfCoopCloseAllowed():
+		case p.rbfCoopCloseAllowed(channel.ChanType()):
 			err = p.startRbfChanCloser(
 				newRPCShutdownInit(req), channel.ChannelPoint(),
 			)
@@ -5625,9 +5646,9 @@ 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() {
+	// We're using the old co-op close for this channel, so we don't need
+	// to init the new RBF chan closer.
+	if !p.rbfCoopCloseAllowed(lnChan.ChanType()) {
 		return nil
 	}
 

peer/brontide_test.go

@@ -1838,3 +1838,100 @@ func TestHasActiveChannels(t *testing.T) {
 	require.False(t, peer.hasActiveChannels())
 	require.Equal(t, int32(0), peer.numActiveChans.Load())
 }
+
+// TestRbfCoopCloseAllowed asserts that the per-channel RBF coop close
+// predicate excludes aux channels (channel types carrying a tapscript root)
+// even when both peers have negotiated the RBF coop close feature, while
+// permitting it for all other channel types.
+func TestRbfCoopCloseAllowed(t *testing.T) {
+	t.Parallel()
+
+	newPeer := func(local, remote *lnwire.RawFeatureVector) *Brontide {
+		return &Brontide{
+			cfg: Config{
+				Features: lnwire.NewFeatureVector(
+					local, lnwire.Features,
+				),
+			},
+			remoteFeatures: lnwire.NewFeatureVector(
+				remote, lnwire.Features,
+			),
+		}
+	}
+
+	var (
+		noBits = lnwire.NewRawFeatureVector()
+		rbfBit = lnwire.NewRawFeatureVector(
+			lnwire.RbfCoopCloseOptional,
+		)
+		stagingBit = lnwire.NewRawFeatureVector(
+			lnwire.RbfCoopCloseOptionalStaging,
+		)
+
+		overlayChan = chanstate.SimpleTaprootFeatureBit |
+			chanstate.TapscriptRootBit
+	)
+
+	tests := []struct {
+		name     string
+		peer     *Brontide
+		chanType chanstate.ChannelType
+		allowed  bool
+	}{
+		{
+			name:     "both signal, plain channel",
+			peer:     newPeer(rbfBit, rbfBit),
+			chanType: chanstate.SingleFunderTweaklessBit,
+			allowed:  true,
+		},
+		{
+			name:     "both signal staging, plain channel",
+			peer:     newPeer(stagingBit, stagingBit),
+			chanType: chanstate.SingleFunderTweaklessBit,
+			allowed:  true,
+		},
+		{
+			name:     "both signal, simple taproot channel",
+			peer:     newPeer(rbfBit, rbfBit),
+			chanType: chanstate.SimpleTaprootFeatureBit,
+			allowed:  true,
+		},
+		{
+			name:     "both signal, aux (overlay) channel",
+			peer:     newPeer(rbfBit, rbfBit),
+			chanType: overlayChan,
+			allowed:  false,
+		},
+		{
+			name:     "both signal staging, aux (overlay) channel",
+			peer:     newPeer(stagingBit, stagingBit),
+			chanType: overlayChan,
+			allowed:  false,
+		},
+		{
+			name:     "only local signals, plain channel",
+			peer:     newPeer(rbfBit, noBits),
+			chanType: chanstate.SingleFunderTweaklessBit,
+			allowed:  false,
+		},
+		{
+			name:     "neither signals, aux (overlay) channel",
+			peer:     newPeer(noBits, noBits),
+			chanType: overlayChan,
+			allowed:  false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			require.Equal(
+				t, test.allowed,
+				test.peer.rbfCoopCloseAllowed(
+					test.chanType,
+				),
+			)
+		})
+	}
+}

peer/rbf_close_wrapper_actor.go

@@ -131,12 +131,9 @@ func (r *rbfCloseActor) Receive(_ context.Context,
 
 	type retType = *CoopCloseUpdates
 
-	// If RBF coop close isn't permitted, then we'll return an error.
-	if !r.chanPeer.rbfCoopCloseAllowed() {
-		return fn.Errf[retType]("rbf coop close not enabled for " +
-			"channel")
-	}
-
+	// Note that no eligibility check is needed here: an actor is only
+	// ever registered after initRbfChanCloser has vetted the channel for
+	// RBF coop close.
 	closeUpdates := &CoopCloseUpdates{
 		UpdateChan: make(chan interface{}, 1),
 		ErrChan:    make(chan error, 1),