From 711a4a400d307e1c9ed363032fe51841e79b0855 Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Thu, 30 Apr 2026 10:33:33 -0230 Subject: [PATCH 1/4] chancloser: remove MarkCoopBroadcasted(nil) calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the two call sites that set ChanStatusCoopBroadcasted before a cooperative close transaction exists: - BeginNegotiation in the legacy close path (chancloser.go) - ChannelFlushed handling in the RBF close path (rbf_coop_transitions.go) Both calls passed nil as the close tx, creating a "limbo" state where ChanStatusCoopBroadcasted is set but no close transaction is stored. This is unnecessary because ShutdownInfo — persisted earlier by MarkShutdownSent in initChanShutdown / the RBF ShutdownPending transition — already serves as the durable signal that the shutdown flow was entered. ChanStatusCoopBroadcasted should only be set when a real close transaction exists, which this change preserves. --- channeldb/channel.go | 21 ++++++++++----------- channeldb/channel_test.go | 19 +++++++------------ channeldb/db_test.go | 4 ++-- lnwallet/chancloser/chancloser.go | 14 +------------- lnwallet/chancloser/interface.go | 6 ++++-- lnwallet/chancloser/rbf_coop_states.go | 6 ++++-- lnwallet/chancloser/rbf_coop_test.go | 17 ----------------- lnwallet/chancloser/rbf_coop_transitions.go | 12 ------------ peer/brontide.go | 16 +++++++++------- 9 files changed, 37 insertions(+), 78 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 127e0ac9c4d..676c109af9c 100644 --- a/channeldb/channel.go +++ b/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 diff --git a/channeldb/channel_test.go b/channeldb/channel_test.go index 47504067780..7039abe4659 100644 --- a/channeldb/channel_test.go +++ b/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, diff --git a/channeldb/db_test.go b/channeldb/db_test.go index 277820b10cd..27428a6f934 100644 --- a/channeldb/db_test.go +++ b/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) diff --git a/lnwallet/chancloser/chancloser.go b/lnwallet/chancloser/chancloser.go index 6e5eaf39356..28750424d5d 100644 --- a/lnwallet/chancloser/chancloser.go +++ b/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) diff --git a/lnwallet/chancloser/interface.go b/lnwallet/chancloser/interface.go index f55eec8be4b..1dedeb0b424 100644 --- a/lnwallet/chancloser/interface.go +++ b/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 diff --git a/lnwallet/chancloser/rbf_coop_states.go b/lnwallet/chancloser/rbf_coop_states.go index 9319c1d271e..e359e539012 100644 --- a/lnwallet/chancloser/rbf_coop_states.go +++ b/lnwallet/chancloser/rbf_coop_states.go @@ -274,8 +274,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 diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go index e8bbcc3f40c..05d44f77f72 100644 --- a/lnwallet/chancloser/rbf_coop_test.go +++ b/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{}) @@ -1809,12 +1804,6 @@ func TestRbfChannelFlushingTransitions(t *testing.T) { // 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() - } - // We'll now send in the event which should trigger // this code path. closeHarness.chanCloser.SendEvent( @@ -1858,12 +1847,6 @@ func TestRbfChannelFlushingTransitions(t *testing.T) { localBalance := flushEvent.ShutdownBalances.LocalBalance balanceAfterClose := localBalance.ToSatoshis() - absoluteFee //nolint:ll - // If this is a fresh flush, then we expect the state - // to be marked on disk. - if isFreshFlush { - closeHarness.expectChanPendingClose() - } - // 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 diff --git a/lnwallet/chancloser/rbf_coop_transitions.go b/lnwallet/chancloser/rbf_coop_transitions.go index eb8b1d67968..a651b37cc35 100644 --- a/lnwallet/chancloser/rbf_coop_transitions.go +++ b/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. diff --git a/peer/brontide.go b/peer/brontide.go index f7a01cd11f5..5f6cbe573d1 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -3627,13 +3627,15 @@ func chooseDeliveryScript(upfront, requested lnwire.DeliveryAddress, func (p *Brontide) restartCoopClose(lnChan *lnwallet.LightningChannel) ( *lnwire.Shutdown, error) { - // 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 - // machine and resend Shutdown. BOLT#2 requires that we retransmit - // Shutdown exactly, but doing so would mean persisting the RPC - // provided close script. Instead use the LocalUpfrontShutdownScript - // or generate a script. + // If this channel has status ChanStatusCoopBroadcasted and a closing + // transaction was recorded, we just need to rebroadcast (handled by + // the chain arbitrator) and exit. If the status is set but no closing + // tx exists, fall through and re-drive the close negotiation via + // ShutdownInfo or LocalUpfrontShutdownScript. + // + // BOLT#2 requires that we retransmit Shutdown exactly, but doing so + // would mean persisting the RPC-provided close script. Instead use + // the LocalUpfrontShutdownScript or generate a script. c := lnChan.State() _, err := c.BroadcastedCooperative() if err != nil && err != channeldb.ErrNoCloseTx { From ef7e084d936997c88d68fe7f9fd6faaa45175117 Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Wed, 6 May 2026 11:28:03 -0230 Subject: [PATCH 2/4] chancloser: add nil-tx guard to legacy close test Add call tracking to the legacy mockChannel so that every MarkCoopBroadcasted invocation is recorded. TestTaprootFastClose now asserts that at least one call was made and that every call carried a non-nil tx, guarding against the limbo state described in https://github.com/lightninglabs/taproot-assets/issues/2108. --- lnwallet/chancloser/chancloser_test.go | 27 ++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/lnwallet/chancloser/chancloser_test.go b/lnwallet/chancloser/chancloser_test.go index 5d96b96f31b..16afb656b6f 100644 --- a/lnwallet/chancloser/chancloser_test.go +++ b/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) + } } From 242df46400b84f12dd27f04cc9faabf64a274d3c Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Thu, 30 Apr 2026 12:40:49 -0230 Subject: [PATCH 3/4] docs: add release note for coop close limbo fix --- docs/release-notes/release-notes-0.22.0.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index fb31bad5662..55b0e43bf77 100644 --- a/docs/release-notes/release-notes-0.22.0.md +++ b/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 From c5f3ad33f48b34ff6b4b1b59b42dff32b75950c2 Mon Sep 17 00:00:00 2001 From: Jared Tobin Date: Wed, 6 May 2026 11:31:21 -0230 Subject: [PATCH 4/4] chancloser: remove dead ChannelFlushed.FreshFlush field FreshFlush is never read in any transition handler. The only producer (peer/brontide.go) sets it unconditionally to true, and after the previous commit removed expectChanPendingClose, the test loops that iterated over {true, false} no longer differentiate between the two values. Remove the field, the unconditional assignment, and collapse the test loops into single sub-tests. --- lnwallet/chancloser/rbf_coop_states.go | 4 - lnwallet/chancloser/rbf_coop_test.go | 118 +++++++++++-------------- peer/brontide.go | 1 - 3 files changed, 54 insertions(+), 69 deletions(-) diff --git a/lnwallet/chancloser/rbf_coop_states.go b/lnwallet/chancloser/rbf_coop_states.go index e359e539012..554dca080bb 100644 --- a/lnwallet/chancloser/rbf_coop_states.go +++ b/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. diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go index 05d44f77f72..0c4b937f544 100644 --- a/lnwallet/chancloser/rbf_coop_test.go +++ b/lnwallet/chancloser/rbf_coop_test.go @@ -1780,84 +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) + closeHarness := newCloser(t, &harnessCfg{ + initialState: fn.Some[ProtocolState]( + &firstState, + ), + }) + defer closeHarness.stopAndAssert() - t.Run(testName, func(t *testing.T) { - firstState := *startingState + // 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) - 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) - - // 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) + closeHarness := newCloser(t, &harnessCfg{ + initialState: fn.Some[ProtocolState]( + &firstState, + ), + }) + defer closeHarness.stopAndAssert() - // 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 + localBalance := flushEvent.ShutdownBalances.LocalBalance + balanceAfterClose := localBalance.ToSatoshis() - absoluteFee - closeHarness := newCloser(t, &harnessCfg{ - initialState: fn.Some[ProtocolState]( - &firstState, - ), - }) - defer closeHarness.stopAndAssert() - - localBalance := flushEvent.ShutdownBalances.LocalBalance - balanceAfterClose := localBalance.ToSatoshis() - absoluteFee //nolint:ll - - // 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 diff --git a/peer/brontide.go b/peer/brontide.go index 5f6cbe573d1..2a06e6fb9b5 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -4131,7 +4131,6 @@ func (p *Brontide) chanFlushEventSentinel(chanCloser *chancloser.RbfChanCloser, ctx := context.Background() chanCloser.SendEvent(ctx, &chancloser.ChannelFlushed{ ShutdownBalances: chanBalances, - FreshFlush: true, }) }