Merge pull request #1248 from halseth/close-channel-fix

[bugfix] Wait for confirmation before marking channel cooperatively closed

breacharbiter.go

@@ -165,6 +165,8 @@ func (b *breachArbiter) Start() error {
 	// finished our responsibilities. If the removal is successful, we also
 	// remove the entry from our in-memory map, to avoid any further action
 	// for this channel.
+	// TODO(halseth): no need continue on IsPending once closed channels
+	// actually means close transaction is confirmed.
 	for _, chanSummary := range closedChans {
 		if chanSummary.IsPending {
 			continue

chancloser.go

@@ -4,8 +4,6 @@ import (
 	"fmt"
 
 	"github.com/davecgh/go-spew/spew"
-	"github.com/lightningnetwork/lnd/channeldb"
-	"github.com/lightningnetwork/lnd/contractcourt"
 	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwire"
@@ -135,8 +133,6 @@ type channelCloser struct {
 	// TODO(roasbeef): abstract away
 	closeReq *htlcswitch.ChanClose
 
-	closeCtx *contractcourt.CooperativeCloseCtx
-
 	// localDeliveryScript is the script that we'll send our settled
 	// channel funds to.
 	localDeliveryScript []byte
@@ -151,8 +147,7 @@ type channelCloser struct {
 // only be populated iff, we're the initiator of this closing request.
 func newChannelCloser(cfg chanCloseCfg, deliveryScript []byte,
 	idealFeePerKw lnwallet.SatPerKWeight, negotiationHeight uint32,
-	closeReq *htlcswitch.ChanClose,
-	closeCtx *contractcourt.CooperativeCloseCtx) *channelCloser {
+	closeReq *htlcswitch.ChanClose) *channelCloser {
 
 	// Given the target fee-per-kw, we'll compute what our ideal _total_
 	// fee will be starting at for this fee negotiation.
@@ -186,7 +181,6 @@ func newChannelCloser(cfg chanCloseCfg, deliveryScript []byte,
 		cfg:                 cfg,
 		negotiationHeight:   negotiationHeight,
 		idealFeeSat:         idealFeeSat,
-		closeCtx:            closeCtx,
 		localDeliveryScript: deliveryScript,
 		priorFeeOffers:      make(map[btcutil.Amount]*lnwire.ClosingSigned),
 	}
@@ -420,7 +414,7 @@ func (c *channelCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message, b
 		remoteSigBytes := closeSignedMsg.Signature.ToSignatureBytes()
 		remoteSig := append(remoteSigBytes, byte(txscript.SigHashAll))
 
-		closeTx, finalLocalBalance, err := c.cfg.channel.CompleteCooperativeClose(
+		closeTx, _, err := c.cfg.channel.CompleteCooperativeClose(
 			localSig, remoteSig, c.localDeliveryScript,
 			c.remoteDeliveryScript, remoteProposedFee,
 		)
@@ -438,33 +432,16 @@ func (c *channelCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message, b
 		if err := c.cfg.broadcastTx(closeTx); err != nil {
 			return nil, false, err
 		}
-
-		// Clear out the current channel state, marking the channel as
-		// being closed within the database.
-		closingTxid := closeTx.TxHash()
-		chanInfo := c.cfg.channel.StateSnapshot()
-		c.closeCtx.Finalize(&channeldb.ChannelCloseSummary{
-			ChanPoint:      c.chanPoint,
-			ChainHash:      chanInfo.ChainHash,
-			ClosingTXID:    closingTxid,
-			CloseHeight:    c.negotiationHeight,
-			RemotePub:      &chanInfo.RemoteIdentity,
-			Capacity:       chanInfo.Capacity,
-			SettledBalance: finalLocalBalance,
-			CloseType:      channeldb.CooperativeClose,
-			ShortChanID:    c.cfg.channel.ShortChanID(),
-			IsPending:      true,
-		})
-
-		// TODO(roasbeef): don't need, ChainWatcher will handle
-
-		c.state = closeFinished
+		if c.cfg.channel.MarkCommitmentBroadcasted(); err != nil {
+			return nil, false, err
+		}
 
 		// Finally, we'll transition to the closeFinished state, and
 		// also return the final close signed message we sent.
 		// Additionally, we return true for the second argument to
 		// indicate we're finished with the channel closing
 		// negotiation.
+		c.state = closeFinished
 		matchingOffer := c.priorFeeOffers[remoteProposedFee]
 		return []lnwire.Message{matchingOffer}, true, nil
 
@@ -493,7 +470,7 @@ func (c *channelCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message, b
 // current compromise fee.
 func (c *channelCloser) proposeCloseSigned(fee btcutil.Amount) (*lnwire.ClosingSigned, error) {
 
-	rawSig, txid, localAmt, err := c.cfg.channel.CreateCloseProposal(
+	rawSig, _, _, err := c.cfg.channel.CreateCloseProposal(
 		fee, c.localDeliveryScript, c.remoteDeliveryScript,
 	)
 	if err != nil {
@@ -521,20 +498,6 @@ func (c *channelCloser) proposeCloseSigned(fee btcutil.Amount) (*lnwire.ClosingS
 	// accepts our offer. This way, we don't have to re-sign.
 	c.priorFeeOffers[fee] = closeSignedMsg
 
-	chanInfo := c.cfg.channel.StateSnapshot()
-	c.closeCtx.LogPotentialClose(&channeldb.ChannelCloseSummary{
-		ChanPoint:      c.chanPoint,
-		ChainHash:      chanInfo.ChainHash,
-		ClosingTXID:    *txid,
-		CloseHeight:    c.negotiationHeight,
-		RemotePub:      &chanInfo.RemoteIdentity,
-		Capacity:       chanInfo.Capacity,
-		SettledBalance: localAmt,
-		CloseType:      channeldb.CooperativeClose,
-		ShortChanID:    c.cfg.channel.ShortChanID(),
-		IsPending:      true,
-	})
-
 	return closeSignedMsg, nil
 }
 

channeldb/channel.go

@@ -1711,12 +1711,12 @@ type ChannelCloseSummary struct {
 
 	// IsPending indicates whether this channel is in the 'pending close'
 	// state, which means the channel closing transaction has been
-	// broadcast, but not confirmed yet or has not yet been fully resolved.
-	// In the case of a channel that has been cooperatively closed, it will
-	// no longer be considered pending as soon as the closing transaction
-	// has been confirmed. However, for channel that have been force
-	// closed, they'll stay marked as "pending" until _all_ the pending
-	// funds have been swept.
+	// confirmed, but not yet been fully resolved. In the case of a channel
+	// that has been cooperatively closed, it will go straight into the
+	// fully resolved state as soon as the closing transaction has been
+	// confirmed. However, for channel that have been force closed, they'll
+	// stay marked as "pending" until _all_ the pending funds have been
+	// swept.
 	IsPending bool
 
 	// RemoteCurrentRevocation is the current revocation for their

contractcourt/chain_arbitrator.go

@@ -341,9 +341,22 @@ func (c *ChainArbitrator) Start() error {
 				pCache:    c.cfg.PreimageDB,
 				signer:    c.cfg.Signer,
 				isOurAddr: c.cfg.IsOurAddress,
-				markChanClosed: func() error {
-					// TODO(roasbeef): also need to pass in log?
-					return c.resolveContract(chanPoint, nil)
+				notifyChanClosed: func() error {
+					c.Lock()
+					delete(c.activeChannels, chanPoint)
+
+					chainWatcher, ok := c.activeWatchers[chanPoint]
+					if ok {
+						// Since the chainWatcher is
+						// calling notifyChanClosed, we
+						// must stop it in a goroutine
+						// to not deadlock.
+						go chainWatcher.Stop()
+					}
+					delete(c.activeWatchers, chanPoint)
+					c.Unlock()
+
+					return nil
 				},
 				contractBreach: func(retInfo *lnwallet.BreachRetribution) error {
 					return c.cfg.ContractBreach(chanPoint, retInfo)
@@ -379,11 +392,17 @@ func (c *ChainArbitrator) Start() error {
 	}
 
 	// Next, for each channel is the closing state, we'll launch a
-	// corresponding more restricted resolver.
+	// corresponding more restricted resolver, as we don't have to watch
+	// the chain any longer, only resolve the contracts on the confirmed
+	// commitment.
 	for _, closeChanInfo := range closingChannels {
 		// If this is a pending cooperative close channel then we'll
 		// simply launch a goroutine to wait until the closing
 		// transaction has been confirmed.
+		// TODO(halseth): can remove this since no coop close channels
+		// should be "pending close" after the recent changes. Keeping
+		// it for a bit in case someone with a coop close channel in
+		// the pending close state upgrades to the new commit.
 		if closeChanInfo.CloseType == channeldb.CooperativeClose {
 			go c.watchForChannelClose(closeChanInfo)
 
@@ -677,8 +696,21 @@ func (c *ChainArbitrator) WatchNewChannel(newChan *channeldb.OpenChannel) error
 			pCache:    c.cfg.PreimageDB,
 			signer:    c.cfg.Signer,
 			isOurAddr: c.cfg.IsOurAddress,
-			markChanClosed: func() error {
-				return c.resolveContract(chanPoint, nil)
+			notifyChanClosed: func() error {
+				c.Lock()
+				delete(c.activeChannels, chanPoint)
+
+				chainWatcher, ok := c.activeWatchers[chanPoint]
+				if ok {
+					// Since the chainWatcher is calling
+					// notifyChanClosed, we must stop it in
+					// a goroutine to not deadlock.
+					go chainWatcher.Stop()
+				}
+				delete(c.activeWatchers, chanPoint)
+				c.Unlock()
+
+				return nil
 			},
 			contractBreach: func(retInfo *lnwallet.BreachRetribution) error {
 				return c.cfg.ContractBreach(chanPoint, retInfo)
@@ -737,20 +769,5 @@ func (c *ChainArbitrator) SubscribeChannelEvents(
 	return watcher.SubscribeChannelEvents(), nil
 }
 
-// BeginCoopChanClose allows the initiator or responder to a cooperative
-// channel closure to signal to the ChainArbitrator that we're starting close
-// negotiation. The caller can use this context to allow the underlying chain
-// watcher to be prepared to act if *any* of the transactions that may
-// potentially be signed off on during fee negotiation are confirmed.
-func (c *ChainArbitrator) BeginCoopChanClose(chanPoint wire.OutPoint) (*CooperativeCloseCtx, error) {
-	watcher, ok := c.activeWatchers[chanPoint]
-	if !ok {
-		return nil, fmt.Errorf("unable to find watcher for: %v",
-			chanPoint)
-	}
-
-	return watcher.BeginCooperativeClose(), nil
-}
-
 // TODO(roasbeef): arbitration reports
 //  * types: contested, waiting for success conf, etc