diff --git a/channeldb/channel.go b/channeldb/channel.go index 127e0ac9c4..676c109af9 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 4750406778..7039abe465 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 277820b10c..27428a6f93 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/docs/release-notes/release-notes-0.22.0.md b/docs/release-notes/release-notes-0.22.0.md index fb31bad566..55b0e43bf7 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 diff --git a/lnwallet/chancloser/chancloser.go b/lnwallet/chancloser/chancloser.go index 6e5eaf3935..28750424d5 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/chancloser_test.go b/lnwallet/chancloser/chancloser_test.go index 5d96b96f31..16afb656b6 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) + } } diff --git a/lnwallet/chancloser/interface.go b/lnwallet/chancloser/interface.go index f55eec8be4..1dedeb0b42 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 9319c1d271..554dca080b 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. @@ -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 diff --git a/lnwallet/chancloser/rbf_coop_test.go b/lnwallet/chancloser/rbf_coop_test.go index e8bbcc3f40..0c4b937f54 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{}) @@ -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 diff --git a/lnwallet/chancloser/rbf_coop_transitions.go b/lnwallet/chancloser/rbf_coop_transitions.go index eb8b1d6796..a651b37cc3 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 f7a01cd11f..2a06e6fb9b 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 { @@ -4129,7 +4131,6 @@ func (p *Brontide) chanFlushEventSentinel(chanCloser *chancloser.RbfChanCloser, ctx := context.Background() chanCloser.SendEvent(ctx, &chancloser.ChannelFlushed{ ShutdownBalances: chanBalances, - FreshFlush: true, }) }