chancloser: don't set ChanStatusCoopBroadcasted before close tx exists

channeldb/channel.go

@@ -2208,21 +2208,20 @@ func (c *OpenChannel) MarkCoopBroadcasted(closeTx *wire.MsgTx,
 func (c *OpenChannel) markBroadcasted(status ChannelStatus, key []byte,
 	closeTx *wire.MsgTx, closer lntypes.ChannelParty) error {
 
+	if closeTx == nil {
+		return fmt.Errorf("closeTx must be non-nil")
+	}
+
 	c.Lock()
 	defer c.Unlock()
 
-	// If a closing tx is provided, we'll generate a closure to write the
-	// transaction in the appropriate bucket under the given key.
-	var putClosingTx func(kvdb.RwBucket) error
-	if closeTx != nil {
-		var b bytes.Buffer
-		if err := WriteElement(&b, closeTx); err != nil {
-			return err
-		}
+	var b bytes.Buffer
+	if err := WriteElement(&b, closeTx); err != nil {
+		return err
+	}
 
-		putClosingTx = func(chanBucket kvdb.RwBucket) error {
-			return chanBucket.Put(key, b.Bytes())
-		}
+	putClosingTx := func(chanBucket kvdb.RwBucket) error {
+		return chanBucket.Put(key, b.Bytes())
 	}
 
 	// Add the initiator status to the status provided. These statuses are

channeldb/channel_test.go

@@ -1239,21 +1239,16 @@ func TestFetchWaitingCloseChannels(t *testing.T) {
 			t.Fatalf("unable to mark commitment broadcast: %v", err)
 		}
 
-		// Now try to marking a coop close with a nil tx. This should
-		// succeed, but it shouldn't exit when queried.
-		if err = channel.MarkCoopBroadcasted(
+		// A nil close tx must be rejected.
+		err = channel.MarkCoopBroadcasted(
 			nil, lntypes.Local,
-		); err != nil {
-			t.Fatalf("unable to mark nil coop broadcast: %v", err)
-		}
-		_, err := channel.BroadcastedCooperative()
-		if err != ErrNoCloseTx {
-			t.Fatalf("expected no closing tx error, got: %v", err)
-		}
+		)
+		require.Error(t, err, "nil tx should be rejected")
 
-		// Finally, modify the close tx deterministically  and also mark
+		// Modify the close tx deterministically and also mark
 		// it as coop closed. Later we will test that distinct
-		// transactions are returned for both coop and force closes.
+		// transactions are returned for both coop and force
+		// closes.
 		closeTx.TxIn[0].PreviousOutPoint.Index ^= 1
 		if err := channel.MarkCoopBroadcasted(
 			closeTx, lntypes.Local,

channeldb/db_test.go

@@ -621,7 +621,7 @@ func TestFetchChannels(t *testing.T) {
 			)
 
 			err = pendingClosing.MarkCoopBroadcasted(
-				nil, lntypes.Local,
+				wire.NewMsgTx(2), lntypes.Local,
 			)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
@@ -643,7 +643,7 @@ func TestFetchChannels(t *testing.T) {
 				openChannelOption(),
 			)
 			err = openClosing.MarkCoopBroadcasted(
-				nil, lntypes.Local,
+				wire.NewMsgTx(2), lntypes.Local,
 			)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)

docs/release-notes/release-notes-0.22.0.md

@@ -26,6 +26,14 @@
   [clarifies](https://github.com/lightningnetwork/lnd/issues/10568) the ZMQ
   port-mismatch warnings so they no longer suggest that the connection failed.
 
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/10782)
+  that could be encountered during co-op closes whereby
+  `ChanStatusCoopBroadcasted` was set before a close transaction
+  actually existed. As a side effect, channels in shutdown
+  negotiation now remain in `ListChannels` (as inactive) until
+  the close transaction is actually broadcast, and
+  `WaitingCloseChannel.ClosingTx` is never empty.
+
 # New Features
 
 ## Functional Enhancements

lnwallet/chancloser/chancloser.go

@@ -742,18 +742,6 @@ func (c *ChanCloser) BeginNegotiation() (fn.Option[lnwire.ClosingSigned],
 		// compute what our max/ideal fee will be.
 		c.initFeeBaseline()
 
-		// Before continuing, mark the channel as cooperatively closed
-		// with a nil txn. Even though we haven't negotiated the final
-		// txn, this guarantees that our listchannels rpc will be
-		// externally consistent, and reflect that the channel is being
-		// shutdown by the time the closing request returns.
-		err := c.cfg.Channel.MarkCoopBroadcasted(
-			nil, c.closer,
-		)
-		if err != nil {
-			return noClosingSigned, err
-		}
-
 		// At this point, we can now start the fee negotiation state, by
 		// constructing and sending our initial signature for what we
 		// think the closing transaction should look like.
@@ -764,7 +752,7 @@ func (c *ChanCloser) BeginNegotiation() (fn.Option[lnwire.ClosingSigned],
 			// to check if we have a cached remote offer to process.
 			// If we do, we'll process it here.
 			res := noClosingSigned
-			err = nil
+			var err error
 			c.cachedClosingSigned.WhenSome(
 				func(cs lnwire.ClosingSigned) {
 					res, err = c.ReceiveClosingSigned(cs)

lnwallet/chancloser/chancloser_test.go

@@ -147,6 +147,8 @@ type mockChannel struct {
 	chanType  channeldb.ChannelType
 	localKey  keychain.KeyDescriptor
 	remoteKey keychain.KeyDescriptor
+
+	coopBroadcastTxns []*wire.MsgTx
 }
 
 func (m *mockChannel) ChannelPoint() wire.OutPoint {
@@ -161,8 +163,10 @@ func (m *mockChannel) FundingBlob() fn.Option[tlv.Blob] {
 	return fn.None[tlv.Blob]()
 }
 
-func (m *mockChannel) MarkCoopBroadcasted(*wire.MsgTx,
-	lntypes.ChannelParty) error {
+func (m *mockChannel) MarkCoopBroadcasted(tx *wire.MsgTx,
+	_ lntypes.ChannelParty) error {
+
+	m.coopBroadcastTxns = append(m.coopBroadcastTxns, tx)
 
 	return nil
 }
@@ -643,4 +647,23 @@ func TestTaprootFastClose(t *testing.T) {
 	tx, _ = bobCloser.ClosingTx()
 	require.NotNil(t, tx)
 	require.True(t, oClosingSigned.IsNone())
+
+	// Every MarkCoopBroadcasted call must have a real close tx.
+	// A nil tx would set ChanStatusCoopBroadcasted without a
+	// stored transaction, creating the limbo state described in
+	// https://github.com/lightninglabs/taproot-assets/issues/2108.
+	require.NotEmpty(t, aliceChan.coopBroadcastTxns,
+		"expected at least one MarkCoopBroadcasted call "+
+			"from alice")
+	require.NotEmpty(t, bobChan.coopBroadcastTxns,
+		"expected at least one MarkCoopBroadcasted call "+
+			"from bob")
+	for i, broadcastTx := range aliceChan.coopBroadcastTxns {
+		require.NotNilf(t, broadcastTx,
+			"alice MarkCoopBroadcasted call %d had nil tx", i)
+	}
+	for i, broadcastTx := range bobChan.coopBroadcastTxns {
+		require.NotNilf(t, broadcastTx,
+			"bob MarkCoopBroadcasted call %d had nil tx", i)
+	}
 }

lnwallet/chancloser/interface.go

@@ -41,8 +41,10 @@ type Channel interface { //nolint:interfacebloat
 	// funding details for the channel.
 	FundingBlob() fn.Option[tlv.Blob]
 
-	// MarkCoopBroadcasted persistently marks that the channel close
-	// transaction has been broadcast.
+	// MarkCoopBroadcasted persistently marks that the channel
+	// close transaction has been broadcast. The tx MUST be
+	// non-nil; callers must not invoke this until a concrete
+	// close tx has been constructed.
 	MarkCoopBroadcasted(*wire.MsgTx, lntypes.ChannelParty) error
 
 	// MarkShutdownSent persists the given ShutdownInfo. The existence of

lnwallet/chancloser/rbf_coop_states.go

@@ -175,10 +175,6 @@ var unknownBalance = ShutdownBalances{}
 //   - fromState: ChannelFlushing
 //   - toState: ClosingNegotiation
 type ChannelFlushed struct {
-	// FreshFlush indicates if this is the first time the channel has been
-	// flushed, or if this is a flush as part of an RBF iteration.
-	FreshFlush bool
-
 	// ShutdownBalances is the balances of the channel once it has been
 	// flushed. We tie this to the ChannelFlushed state as this may not be
 	// the same as the starting value.
@@ -274,8 +270,10 @@ type ChanStateObserver interface {
 	// channel.
 	DisableChannel() error
 
-	// MarkCoopBroadcasted persistently marks that the channel close
-	// transaction has been broadcast.
+	// MarkCoopBroadcasted persistently marks that the channel
+	// close transaction has been broadcast. The tx MUST be
+	// non-nil; callers must not invoke this until a concrete
+	// close tx has been constructed.
 	MarkCoopBroadcasted(*wire.MsgTx, bool) error
 
 	// MarkShutdownSent persists the given ShutdownInfo. The existence of

lnwallet/chancloser/rbf_coop_test.go

@@ -523,11 +523,6 @@ func (r *rbfCloserTestHarness) expectCloseFinalized(
 	).Return(nil)
 }
 
-func (r *rbfCloserTestHarness) expectChanPendingClose() {
-	var nilTx *wire.MsgTx
-	r.chanObserver.On("MarkCoopBroadcasted", nilTx, true).Return(nil)
-}
-
 func (r *rbfCloserTestHarness) assertLocalClosePending() {
 	// We should then remain in the outer close negotiation state.
 	r.assertStateTransitions(&ClosingNegotiation{})
@@ -1785,96 +1780,74 @@ func TestRbfChannelFlushingTransitions(t *testing.T) {
 		},
 	}
 
-	// If send in the channel flushed event, but the local party can't pay
-	// for fees, then we should just head to the negotiation state.
-	for _, isFreshFlush := range []bool{true, false} {
+	// When the channel is flushed but the local party cannot cover
+	// the closing fee, we should transition directly to
+	// ClosingNegotiation without any further intermediate state
+	// transitions.
+	t.Run("local_cannot_pay_for_fee", func(t *testing.T) {
+		firstState := *startingState
 		chanFlushedEvent := *flushTemplate
-		chanFlushedEvent.FreshFlush = isFreshFlush
 
-		testName := fmt.Sprintf("local_cannot_pay_for_fee/"+
-			"fresh_flush=%v", isFreshFlush)
-
-		t.Run(testName, func(t *testing.T) {
-			firstState := *startingState
+		closeHarness := newCloser(t, &harnessCfg{
+			initialState: fn.Some[ProtocolState](
+				&firstState,
+			),
+		})
+		defer closeHarness.stopAndAssert()
 
-			closeHarness := newCloser(t, &harnessCfg{
-				initialState: fn.Some[ProtocolState](
-					&firstState,
-				),
-			})
-			defer closeHarness.stopAndAssert()
-
-			// As part of the set up for this state, we'll have the
-			// final absolute fee required be greater than the
-			// balance of the local party.
-			closeHarness.expectFeeEstimate(absoluteFee, 1)
-
-			// If this is a fresh flush, then we expect the state
-			// to be marked on disk.
-			if isFreshFlush {
-				closeHarness.expectChanPendingClose()
-			}
+		// As part of the set up for this state, we'll have the
+		// final absolute fee required be greater than the
+		// balance of the local party.
+		closeHarness.expectFeeEstimate(absoluteFee, 1)
 
-			// We'll now send in the event which should trigger
-			// this code path.
-			closeHarness.chanCloser.SendEvent(
-				ctx, &chanFlushedEvent,
-			)
+		// We'll now send in the event which should trigger
+		// this code path.
+		closeHarness.chanCloser.SendEvent(
+			ctx, &chanFlushedEvent,
+		)
 
-			// With the event sent, we should now transition
-			// straight to the ClosingNegotiation state, with no
-			// further state transitions.
-			closeHarness.assertStateTransitions(
-				&ClosingNegotiation{},
-			)
-		})
-	}
+		// With the event sent, we should now transition
+		// straight to the ClosingNegotiation state, with no
+		// further state transitions.
+		closeHarness.assertStateTransitions(
+			&ClosingNegotiation{},
+		)
+	})
 
-	for _, isFreshFlush := range []bool{true, false} {
+	// When the local party can cover the closing fee,
+	// ChannelFlushed drives a normal half-signer iteration: we
+	// move to ClosingNegotiation and send a ClosingComplete
+	// message.
+	t.Run("local_can_pay_for_fee", func(t *testing.T) {
+		firstState := *startingState
 		flushEvent := *flushTemplate
-		flushEvent.FreshFlush = isFreshFlush
 
-		// We'll modify the starting balance to be 3x the required fee
-		// to ensure that we can pay for the fee.
+		// We'll modify the starting balance to be 3x the required
+		// fee to ensure that we can pay for the fee.
 		flushEvent.ShutdownBalances.LocalBalance = lnwire.NewMSatFromSatoshis( //nolint:ll
 			absoluteFee * 3,
 		)
 
-		testName := fmt.Sprintf("local_can_pay_for_fee/"+
-			"fresh_flush=%v", isFreshFlush)
-
-		// This scenario, we'll have the local party be able to pay for
-		// the fees, which will trigger additional state transitions.
-		t.Run(testName, func(t *testing.T) {
-			firstState := *startingState
-
-			closeHarness := newCloser(t, &harnessCfg{
-				initialState: fn.Some[ProtocolState](
-					&firstState,
-				),
-			})
-			defer closeHarness.stopAndAssert()
-
-			localBalance := flushEvent.ShutdownBalances.LocalBalance
-			balanceAfterClose := localBalance.ToSatoshis() - absoluteFee //nolint:ll
+		closeHarness := newCloser(t, &harnessCfg{
+			initialState: fn.Some[ProtocolState](
+				&firstState,
+			),
+		})
+		defer closeHarness.stopAndAssert()
 
-			// If this is a fresh flush, then we expect the state
-			// to be marked on disk.
-			if isFreshFlush {
-				closeHarness.expectChanPendingClose()
-			}
+		localBalance := flushEvent.ShutdownBalances.LocalBalance
+		balanceAfterClose := localBalance.ToSatoshis() - absoluteFee
 
-			// From here, we expect the state transition to go
-			// back to closing negotiated, for a ClosingComplete
-			// message to be sent and then for us to terminate at
-			// that state. This is 1/2 of the normal RBF signer
-			// flow.
-			closeHarness.expectHalfSignerIteration(
-				&flushEvent, balanceAfterClose, absoluteFee,
-				noDustExpect, false,
-			)
-		})
-	}
+		// From here, we expect the state transition to go
+		// back to closing negotiated, for a ClosingComplete
+		// message to be sent and then for us to terminate at
+		// that state. This is 1/2 of the normal RBF signer
+		// flow.
+		closeHarness.expectHalfSignerIteration(
+			&flushEvent, balanceAfterClose, absoluteFee,
+			noDustExpect, false,
+		)
+	})
 
 	// This tests that if we receive an `OfferReceivedEvent` while in the
 	// flushing state, then we'll cache that, and once we receive

lnwallet/chancloser/rbf_coop_transitions.go

@@ -585,18 +585,6 @@ func (c *ChannelFlushing) ProcessEvent(event ProtocolEvent, env *Environment,
 		chancloserLog.Infof("ChannelPoint(%v): channel flushed! "+
 			"proceeding with co-op close", env.ChanPoint)
 
-		// Now that the channel has been flushed, we'll mark on disk
-		// that we're approaching the point of no return where we'll
-		// send a new signature to the remote party.
-		//
-		// TODO(roasbeef): doesn't actually matter if initiator here?
-		if msg.FreshFlush {
-			err := env.ChanObserver.MarkCoopBroadcasted(nil, true)
-			if err != nil {
-				return nil, err
-			}
-		}
-
 		// If an ideal fee rate was specified, then we'll use that,
 		// otherwise we'll fall back to the default value given in the
 		// env.