Merge pull request #9253 from ziggie1984/fix-chanArb-deadlock

fix chanArb deadlock

Author: guggero

contractcourt/chain_arbitrator.go

@@ -335,11 +335,10 @@ func (a *arbChannel) NewAnchorResolutions() (*lnwallet.AnchorResolutions,
 // ForceCloseChan should force close the contract that this attendant is
 // watching over. We'll use this when we decide that we need to go to chain. It
 // should in addition tell the switch to remove the corresponding link, such
-// that we won't accept any new updates. The returned summary contains all items
-// needed to eventually resolve all outputs on chain.
+// that we won't accept any new updates.
 //
 // NOTE: Part of the ArbChannel interface.
-func (a *arbChannel) ForceCloseChan() (*lnwallet.LocalForceCloseSummary, error) {
+func (a *arbChannel) ForceCloseChan() (*wire.MsgTx, error) {
 	// First, we mark the channel as borked, this ensure
 	// that no new state transitions can happen, and also
 	// that the link won't be loaded into the switch.
@@ -386,7 +385,15 @@ func (a *arbChannel) ForceCloseChan() (*lnwallet.LocalForceCloseSummary, error)
 	if err != nil {
 		return nil, err
 	}
-	return chanMachine.ForceClose()
+
+	closeSummary, err := chanMachine.ForceClose(
+		lnwallet.WithSkipContractResolutions(),
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	return closeSummary.CloseTx, nil
 }
 
 // newActiveChannelArbitrator creates a new instance of an active channel

contractcourt/chain_watcher.go

@@ -1175,16 +1175,29 @@ func (c *chainWatcher) dispatchLocalForceClose(
 		LocalChanConfig:         c.cfg.chanState.LocalChanCfg,
 	}
 
+	resolutions, err := forceClose.ContractResolutions.UnwrapOrErr(
+		fmt.Errorf("resolutions not found"),
+	)
+	if err != nil {
+		return err
+	}
+
 	// If our commitment output isn't dust or we have active HTLC's on the
 	// commitment transaction, then we'll populate the balances on the
 	// close channel summary.
-	if forceClose.CommitResolution != nil {
-		closeSummary.SettledBalance = chanSnapshot.LocalBalance.ToSatoshis()
-		closeSummary.TimeLockedBalance = chanSnapshot.LocalBalance.ToSatoshis()
+	if resolutions.CommitResolution != nil {
+		localBalance := chanSnapshot.LocalBalance.ToSatoshis()
+		closeSummary.SettledBalance = localBalance
+		closeSummary.TimeLockedBalance = localBalance
 	}
-	for _, htlc := range forceClose.HtlcResolutions.OutgoingHTLCs {
-		htlcValue := btcutil.Amount(htlc.SweepSignDesc.Output.Value)
-		closeSummary.TimeLockedBalance += htlcValue
+
+	if resolutions.HtlcResolutions != nil {
+		for _, htlc := range resolutions.HtlcResolutions.OutgoingHTLCs {
+			htlcValue := btcutil.Amount(
+				htlc.SweepSignDesc.Output.Value,
+			)
+			closeSummary.TimeLockedBalance += htlcValue
+		}
 	}
 
 	// Attempt to add a channel sync message to the close summary.

contractcourt/chain_watcher_test.go

@@ -504,14 +504,24 @@ func TestChainWatcherLocalForceCloseDetect(t *testing.T) {
 		// outputs.
 		select {
 		case summary := <-chanEvents.LocalUnilateralClosure:
+			resOpt := summary.LocalForceCloseSummary.
+				ContractResolutions
+
+			resolutions, err := resOpt.UnwrapOrErr(
+				fmt.Errorf("resolutions not found"),
+			)
+			if err != nil {
+				t.Fatalf("unable to get resolutions: %v", err)
+			}
+
 			// Make sure we correctly extracted the commit
 			// resolution if we had a local output.
 			if remoteOutputOnly {
-				if summary.CommitResolution != nil {
+				if resolutions.CommitResolution != nil {
 					t.Fatalf("expected no commit resolution")
 				}
 			} else {
-				if summary.CommitResolution == nil {
+				if resolutions.CommitResolution == nil {
 					t.Fatalf("expected commit resolution")
 				}
 			}

contractcourt/channel_arbitrator.go

@@ -98,7 +98,7 @@ type ArbChannel interface {
 	// corresponding link, such that we won't accept any new updates. The
 	// returned summary contains all items needed to eventually resolve all
 	// outputs on chain.
-	ForceCloseChan() (*lnwallet.LocalForceCloseSummary, error)
+	ForceCloseChan() (*wire.MsgTx, error)
 
 	// NewAnchorResolutions returns the anchor resolutions for currently
 	// valid commitment transactions.
@@ -1098,7 +1098,7 @@ func (c *ChannelArbitrator) stateStep(
 		// We'll tell the switch that it should remove the link for
 		// this channel, in addition to fetching the force close
 		// summary needed to close this channel on chain.
-		closeSummary, err := c.cfg.Channel.ForceCloseChan()
+		forceCloseTx, err := c.cfg.Channel.ForceCloseChan()
 		if err != nil {
 			log.Errorf("ChannelArbitrator(%v): unable to "+
 				"force close: %v", c.cfg.ChanPoint, err)
@@ -1118,7 +1118,7 @@ func (c *ChannelArbitrator) stateStep(
 
 			return StateError, closeTx, err
 		}
-		closeTx = closeSummary.CloseTx
+		closeTx = forceCloseTx
 
 		// Before publishing the transaction, we store it to the
 		// database, such that we can re-publish later in case it
@@ -2812,11 +2812,36 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) {
 			}
 			closeTx := closeInfo.CloseTx
 
+			resolutions, err := closeInfo.ContractResolutions.
+				UnwrapOrErr(
+					fmt.Errorf("resolutions not found"),
+				)
+			if err != nil {
+				log.Errorf("ChannelArbitrator(%v): unable to "+
+					"get resolutions: %v", c.cfg.ChanPoint,
+					err)
+
+				return
+			}
+
+			// We make sure that the htlc resolutions are present
+			// otherwise we would panic dereferencing the pointer.
+			//
+			// TODO(ziggie): Refactor ContractResolutions to use
+			// options.
+			if resolutions.HtlcResolutions == nil {
+				log.Errorf("ChannelArbitrator(%v): htlc "+
+					"resolutions not found",
+					c.cfg.ChanPoint)
+
+				return
+			}
+
 			contractRes := &ContractResolutions{
 				CommitHash:       closeTx.TxHash(),
-				CommitResolution: closeInfo.CommitResolution,
-				HtlcResolutions:  *closeInfo.HtlcResolutions,
-				AnchorResolution: closeInfo.AnchorResolution,
+				CommitResolution: resolutions.CommitResolution,
+				HtlcResolutions:  *resolutions.HtlcResolutions,
+				AnchorResolution: resolutions.AnchorResolution,
 			}
 
 			// When processing a unilateral close event, we'll
@@ -2825,7 +2850,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) {
 			// available to fetch in that state, we'll also write
 			// the commit set so we can reconstruct our chain
 			// actions on restart.
-			err := c.log.LogContractResolutions(contractRes)
+			err = c.log.LogContractResolutions(contractRes)
 			if err != nil {
 				log.Errorf("Unable to write resolutions: %v",
 					err)

contractcourt/channel_arbitrator_test.go

@@ -693,11 +693,15 @@ func TestChannelArbitratorLocalForceClose(t *testing.T) {
 	chanArbCtx.AssertState(StateCommitmentBroadcasted)
 
 	// Now notify about the local force close getting confirmed.
+	//
+	//nolint:lll
 	chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{
 		SpendDetail: &chainntnfs.SpendDetail{},
 		LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{
-			CloseTx:         &wire.MsgTx{},
-			HtlcResolutions: &lnwallet.HtlcResolutions{},
+			CloseTx: &wire.MsgTx{},
+			ContractResolutions: fn.Some(lnwallet.ContractResolutions{
+				HtlcResolutions: &lnwallet.HtlcResolutions{},
+			}),
 		},
 		ChannelCloseSummary: &channeldb.ChannelCloseSummary{},
 	}
@@ -987,15 +991,18 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) {
 		},
 	}
 
+	//nolint:lll
 	chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{
 		SpendDetail: &chainntnfs.SpendDetail{},
 		LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{
 			CloseTx: closeTx,
-			HtlcResolutions: &lnwallet.HtlcResolutions{
-				OutgoingHTLCs: []lnwallet.OutgoingHtlcResolution{
-					outgoingRes,
+			ContractResolutions: fn.Some(lnwallet.ContractResolutions{
+				HtlcResolutions: &lnwallet.HtlcResolutions{
+					OutgoingHTLCs: []lnwallet.OutgoingHtlcResolution{
+						outgoingRes,
+					},
 				},
-			},
+			}),
 		},
 		ChannelCloseSummary: &channeldb.ChannelCloseSummary{},
 		CommitSet: CommitSet{
@@ -1613,12 +1620,15 @@ func TestChannelArbitratorCommitFailure(t *testing.T) {
 		},
 		{
 			closeType: channeldb.LocalForceClose,
+			//nolint:lll
 			sendEvent: func(chanArb *ChannelArbitrator) {
 				chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{
 					SpendDetail: &chainntnfs.SpendDetail{},
 					LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{
-						CloseTx:         &wire.MsgTx{},
-						HtlcResolutions: &lnwallet.HtlcResolutions{},
+						CloseTx: &wire.MsgTx{},
+						ContractResolutions: fn.Some(lnwallet.ContractResolutions{
+							HtlcResolutions: &lnwallet.HtlcResolutions{},
+						}),
 					},
 					ChannelCloseSummary: &channeldb.ChannelCloseSummary{},
 				}
@@ -1946,11 +1956,15 @@ func TestChannelArbitratorDanglingCommitForceClose(t *testing.T) {
 			// being canalled back. Also note that there're no HTLC
 			// resolutions sent since we have none on our
 			// commitment transaction.
+			//
+			//nolint:lll
 			uniCloseInfo := &LocalUnilateralCloseInfo{
 				SpendDetail: &chainntnfs.SpendDetail{},
 				LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{
-					CloseTx:         closeTx,
-					HtlcResolutions: &lnwallet.HtlcResolutions{},
+					CloseTx: closeTx,
+					ContractResolutions: fn.Some(lnwallet.ContractResolutions{
+						HtlcResolutions: &lnwallet.HtlcResolutions{},
+					}),
 				},
 				ChannelCloseSummary: &channeldb.ChannelCloseSummary{},
 				CommitSet: CommitSet{
@@ -2870,12 +2884,15 @@ func TestChannelArbitratorAnchors(t *testing.T) {
 		},
 	}
 
+	//nolint:lll
 	chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{
 		SpendDetail: &chainntnfs.SpendDetail{},
 		LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{
-			CloseTx:          closeTx,
-			HtlcResolutions:  &lnwallet.HtlcResolutions{},
-			AnchorResolution: anchorResolution,
+			CloseTx: closeTx,
+			ContractResolutions: fn.Some(lnwallet.ContractResolutions{
+				HtlcResolutions:  &lnwallet.HtlcResolutions{},
+				AnchorResolution: anchorResolution,
+			}),
 		},
 		ChannelCloseSummary: &channeldb.ChannelCloseSummary{},
 		CommitSet: CommitSet{
@@ -3109,14 +3126,10 @@ func (m *mockChannel) NewAnchorResolutions() (*lnwallet.AnchorResolutions,
 	return &lnwallet.AnchorResolutions{}, nil
 }
 
-func (m *mockChannel) ForceCloseChan() (*lnwallet.LocalForceCloseSummary, error) {
+func (m *mockChannel) ForceCloseChan() (*wire.MsgTx, error) {
 	if m.forceCloseErr != nil {
 		return nil, m.forceCloseErr
 	}
 
-	summary := &lnwallet.LocalForceCloseSummary{
-		CloseTx:         &wire.MsgTx{},
-		HtlcResolutions: &lnwallet.HtlcResolutions{},
-	}
-	return summary, nil
+	return &wire.MsgTx{}, nil
 }

docs/release-notes/release-notes-0.19.0.md

@@ -56,6 +56,9 @@
 
 * [Fixed a case](https://github.com/lightningnetwork/lnd/pull/9258) where the
   confirmation notification may be missed.
+  
+* [Make the contract resolutions for the channel arbitrator optional](
+  https://github.com/lightningnetwork/lnd/pull/9253)
 
 # New Features
 ## Functional Enhancements