From 7cd783d7bbb0c8c4ab039297a0c774ba42f81e97 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 15 May 2018 15:15:42 +0200 Subject: [PATCH 01/13] lnwallet/channel: remove unused method DeleteState --- lnwallet/channel.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 8e6ec7f7..45f26bc4 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5580,13 +5580,6 @@ func (lc *LightningChannel) CompleteCooperativeClose(localSig, remoteSig []byte, return closeTx, ourBalance, nil } -// DeleteState deletes all state concerning the channel from the underlying -// database, only leaving a small summary describing metadata of the -// channel's lifetime. -func (lc *LightningChannel) DeleteState(c *channeldb.ChannelCloseSummary) error { - return lc.channelState.CloseChannel(c) -} - // AvailableBalance returns the current available balance within the channel. // By available balance, we mean that if at this very instance s new commitment // were to be created which evals all the log entries, what would our available From 9ddd4845241a600488dc451e66d971b0d83eb0d6 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 18 May 2018 15:19:40 +0200 Subject: [PATCH 02/13] lnwallet/channel: export method MarkCommitmentBroadcasted --- lnwallet/channel.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 45f26bc4..8ccb0ded 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5896,6 +5896,16 @@ func (lc *LightningChannel) State() *channeldb.OpenChannel { return lc.channelState } +// MarkCommitmentBroadcasted marks the channel as a commitment transaction has +// been broadcast, either our own or the remote, and we should watch the chain +// for it to confirm before taking any further action. +func (lc *LightningChannel) MarkCommitmentBroadcasted() error { + lc.Lock() + defer lc.Unlock() + + return lc.channelState.MarkCommitmentBroadcasted() +} + // ActiveHtlcs returns a slice of HTLC's which are currently active on *both* // commitment transactions. func (lc *LightningChannel) ActiveHtlcs() []channeldb.HTLC { From 0809880426629ce2796676a2ef862803b9d1bc2f Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 15 May 2018 15:44:33 +0200 Subject: [PATCH 03/13] chancloser: don't log potential closes to the chainwatcher This commit stops the chan closer from sending the potential coop close transactions to the chainwatcher, as this is no longer needed. The chainwatcher recently was modified to watch for any potential close, and will because of this handle the close regardless of which one appears in chain. When the chancloser broadcast the final close transaction, we mark it as CommitmentBroadcasted in the database. --- chancloser.go | 51 +++++++-------------------------------------------- 1 file changed, 7 insertions(+), 44 deletions(-) diff --git a/chancloser.go b/chancloser.go index a9df9fa9..257b6db1 100644 --- a/chancloser.go +++ b/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 } From bb611e065d70695b6ef91ae5562a3637389da26c Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 21 May 2018 15:04:23 +0200 Subject: [PATCH 04/13] peer: don't pass closeCtx to chanCloser --- peer.go | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/peer.go b/peer.go index 9a6b792e..a260a65c 100644 --- a/peer.go +++ b/peer.go @@ -1572,18 +1572,6 @@ func (p *peer) fetchActiveChanCloser(chanID lnwire.ChannelID) (*channelCloser, e return nil, fmt.Errorf("cannot obtain best block") } - // Before we create the chan closer, we'll start a new - // cooperative channel closure transaction from the chain arb. - // With this context, we'll ensure that we're able to respond - // if *any* of the transactions we sign off on are ever - // broadcast. - closeCtx, err := p.server.chainArb.BeginCoopChanClose( - *channel.ChannelPoint(), - ) - if err != nil { - return nil, err - } - chanCloser = newChannelCloser( chanCloseCfg{ channel: channel, @@ -1595,7 +1583,6 @@ func (p *peer) fetchActiveChanCloser(chanID lnwire.ChannelID) (*channelCloser, e targetFeePerKw, uint32(startingHeight), nil, - closeCtx, ) p.activeChanCloses[chanID] = chanCloser } @@ -1638,20 +1625,6 @@ func (p *peer) handleLocalCloseReq(req *htlcswitch.ChanClose) { return } - // Before we create the chan closer, we'll start a new - // cooperative channel closure transaction from the chain arb. - // With this context, we'll ensure that we're able to respond - // if *any* of the transactions we sign off on are ever - // broadcast. - closeCtx, err := p.server.chainArb.BeginCoopChanClose( - *channel.ChannelPoint(), - ) - if err != nil { - peerLog.Errorf(err.Error()) - req.Err <- err - return - } - // Next, we'll create a new channel closer state machine to // handle the close negotiation. _, startingHeight, err := p.server.cc.chainIO.GetBestBlock() @@ -1671,7 +1644,6 @@ func (p *peer) handleLocalCloseReq(req *htlcswitch.ChanClose) { req.TargetFeePerKw, uint32(startingHeight), req, - closeCtx, ) p.activeChanCloses[chanID] = chanCloser From 21d1cc3fe888b80903abc3b846126c403ebfdbd0 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 21 May 2018 15:06:07 +0200 Subject: [PATCH 05/13] contractcourt/chain_arbitrator: remove BeginCoopChanClose --- contractcourt/chain_arbitrator.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index aa9a956a..1e2079b7 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -737,20 +737,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 From af14a2fc57d29f443f8d4535fb64adfc593b8fd7 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 22 May 2018 14:21:43 +0200 Subject: [PATCH 06/13] contractcourt/chain_watcher: remove CooperativeCloseCtx Removes CooperativeCloseCtx and methods. --- contractcourt/chain_watcher.go | 152 --------------------------------- 1 file changed, 152 deletions(-) diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index 606ce980..2163ee92 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -10,7 +10,6 @@ import ( "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lnwallet" "github.com/roasbeef/btcd/chaincfg" - "github.com/roasbeef/btcd/chaincfg/chainhash" "github.com/roasbeef/btcd/txscript" "github.com/roasbeef/btcd/wire" "github.com/roasbeef/btcutil" @@ -123,12 +122,6 @@ type chainWatcher struct { // clientSubscriptions is a map that keeps track of all the active // client subscriptions for events related to this channel. clientSubscriptions map[uint64]*ChainEventSubscription - - // possibleCloses is a map from cooperative closing transaction txid to - // a close summary that describes the nature of the channel closure. - // We'll use this map to keep track of all possible channel closures to - // ensure out db state is correct in the end. - possibleCloses map[chainhash.Hash]*channeldb.ChannelCloseSummary } // newChainWatcher returns a new instance of a chainWatcher for a channel given @@ -158,7 +151,6 @@ func newChainWatcher(cfg chainWatcherConfig) (*chainWatcher, error) { stateHintObfuscator: stateHint, quit: make(chan struct{}), clientSubscriptions: make(map[uint64]*ChainEventSubscription), - possibleCloses: make(map[chainhash.Hash]*channeldb.ChannelCloseSummary), }, nil } @@ -751,147 +743,3 @@ func (c *chainWatcher) dispatchContractBreach(spendEvent *chainntnfs.SpendDetail return nil } - -// CooperativeCloseCtx is a transactional object that's used by external -// parties to initiate a cooperative closure negotiation. During the -// negotiation, we sign multiple versions of a closing transaction, either of -// which may be counter signed and broadcast by the remote party at any time. -// As a result, we'll need to watch the chain to see if any of these confirm, -// only afterwards will we mark the channel as fully closed. -type CooperativeCloseCtx struct { - // potentialCloses is a channel will be used by the party negotiating - // the cooperative closure to send possible closing states to the chain - // watcher to ensure we detect all on-chain spends. - potentialCloses chan *channeldb.ChannelCloseSummary - - // activeCloses keeps track of all the txid's that we're currently - // watching for. - activeCloses map[chainhash.Hash]struct{} - - // watchCancel will be closed once *one* of the txid's in the map above - // is confirmed. This will cause all the lingering goroutines to exit. - watchCancel chan struct{} - - watcher *chainWatcher - - sync.Mutex -} - -// BeginCooperativeClose should be called by the party negotiating the -// cooperative closure before the first signature is sent to the remote party. -// This will return a context that should be used to communicate possible -// closing states so we can act on them. -func (c *chainWatcher) BeginCooperativeClose() *CooperativeCloseCtx { - // We'll simply return a new close context that will be used be the - // caller to notify us of potential closes. - return &CooperativeCloseCtx{ - potentialCloses: make(chan *channeldb.ChannelCloseSummary), - watchCancel: make(chan struct{}), - activeCloses: make(map[chainhash.Hash]struct{}), - watcher: c, - } -} - -// LogPotentialClose should be called by the party negotiating the cooperative -// closure once they signed a new state, but *before* they transmit it to the -// remote party. This will ensure that the chain watcher is able to log the new -// state it should watch the chain for. -func (c *CooperativeCloseCtx) LogPotentialClose(potentialClose *channeldb.ChannelCloseSummary) { - c.Lock() - defer c.Unlock() - - // We'll check to see if we're already watching for a close of this - // channel, if so, then we'll exit early to avoid launching a duplicate - // goroutine. - if _, ok := c.activeCloses[potentialClose.ClosingTXID]; ok { - return - } - - // Otherwise, we'll mark this txid as currently being watched. - c.activeCloses[potentialClose.ClosingTXID] = struct{}{} - - // We'll take this potential close, and launch a goroutine which will - // wait until it's confirmed, then update the database state. When a - // potential close gets confirmed, we'll cancel out all other launched - // goroutines. - go func() { - confNtfn, err := c.watcher.cfg.notifier.RegisterConfirmationsNtfn( - &potentialClose.ClosingTXID, 1, - uint32(potentialClose.CloseHeight), - ) - if err != nil { - log.Errorf("unable to register for conf: %v", err) - return - } - - log.Infof("closeCtx: waiting for txid=%v to close "+ - "ChannelPoint(%v) on chain", potentialClose.ClosingTXID, - c.watcher.cfg.chanState.FundingOutpoint) - - select { - case confInfo, ok := <-confNtfn.Confirmed: - if !ok { - log.Errorf("notifier exiting") - return - } - - log.Infof("closeCtx: ChannelPoint(%v) is fully closed, at "+ - "height: %v", c.watcher.cfg.chanState.FundingOutpoint, - confInfo.BlockHeight) - - close(c.watchCancel) - - c.watcher.Lock() - for _, sub := range c.watcher.clientSubscriptions { - select { - case sub.CooperativeClosure <- struct{}{}: - case <-c.watcher.quit: - } - } - c.watcher.Unlock() - - err := c.watcher.cfg.chanState.CloseChannel(potentialClose) - if err != nil { - log.Warnf("closeCtx: unable to update latest "+ - "close for ChannelPoint(%v): %v", - c.watcher.cfg.chanState.FundingOutpoint, err) - } - - err = c.watcher.cfg.markChanClosed() - if err != nil { - log.Errorf("closeCtx: unable to mark chan fully "+ - "closed: %v", err) - return - } - - case <-c.watchCancel: - log.Debugf("Exiting watch for close of txid=%v for "+ - "ChannelPoint(%v)", potentialClose.ClosingTXID, - c.watcher.cfg.chanState.FundingOutpoint) - - case <-c.watcher.quit: - return - } - }() -} - -// Finalize should be called once both parties agree on a final transaction to -// close out the channel. This method will immediately mark the channel as -// pending closed in the database, then launch a goroutine to mark the channel -// fully closed upon confirmation. -func (c *CooperativeCloseCtx) Finalize(preferredClose *channeldb.ChannelCloseSummary) error { - chanPoint := c.watcher.cfg.chanState.FundingOutpoint - - log.Infof("Finalizing chan close for ChannelPoint(%v)", chanPoint) - - err := c.watcher.cfg.chanState.CloseChannel(preferredClose) - if err != nil { - log.Errorf("closeCtx: unable to close ChannelPoint(%v): %v", - chanPoint, err) - return err - } - - go c.LogPotentialClose(preferredClose) - - return nil -} From 0f077fcb545ce732ed679f42a7ea48e3a670e0fe Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 22 May 2018 14:26:02 +0200 Subject: [PATCH 07/13] contractcourt/chain_watcher: mark fully closed in dispatchCoopClose This commit makes the dispatchCooperativeClose method mark the channel fully closed directly, without registering for confirmation notifications first. We can do this as recent changes to the contractcourt changed the definition of a closed channel in the database to have had its closing tx confirmed, and we only dispatch the cooperative close once the transaction has 1 confirmation. We also rename the markChanClosed method to notifyChanClosed, to more clearly indicate that the ChainArbitrator no longer has to mark the channel fully closed in the database. --- contractcourt/chain_watcher.go | 68 +++++++++++----------------------- 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index 2163ee92..feb18fa0 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -78,10 +78,12 @@ type chainWatcherConfig struct { // machine. signer lnwallet.Signer - // markChanClosed is a method that will be called by the watcher if it - // detects that a cooperative closure transaction has successfully been - // confirmed. - markChanClosed func() error + // notifyChanClosed is a method that will be called by the watcher when + // it has detected a close on-chain and performed all necessary + // actions, like marking the channel closed in the database and + // notified all its subcribers. It lets the chain arbitrator know that + // the chain watcher chan be stopped. + notifyChanClosed func() error // contractBreach is a method that will be called by the watcher if it // detects that a contract breach transaction has been confirmed. Only @@ -453,8 +455,9 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet // ours. localAmt := c.toSelfAmount(broadcastTx) - // Once this is known, we'll mark the state as pending close in the - // database. + // Once this is known, we'll mark the state as fully closed in the + // database. We can do this as a cooperatively closed channel has all + // its outputs resolved after only one confirmation. closeSummary := &channeldb.ChannelCloseSummary{ ChanPoint: c.cfg.chanState.FundingOutpoint, ChainHash: c.cfg.chanState.ChainHash, @@ -465,7 +468,7 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet SettledBalance: localAmt, CloseType: channeldb.CooperativeClose, ShortChanID: c.cfg.chanState.ShortChanID(), - IsPending: true, + IsPending: false, } err := c.cfg.chanState.CloseChannel(closeSummary) if err != nil && err != channeldb.ErrNoActiveChannels && @@ -473,45 +476,10 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet return fmt.Errorf("unable to close chan state: %v", err) } - // Finally, we'll launch a goroutine to mark the channel as fully - // closed once the transaction confirmed. - go func() { - confNtfn, err := c.cfg.notifier.RegisterConfirmationsNtfn( - commitSpend.SpenderTxHash, 1, - uint32(commitSpend.SpendingHeight), - ) - if err != nil { - log.Errorf("unable to register for conf: %v", err) - return - } - - log.Infof("closeObserver: waiting for txid=%v to close "+ - "ChannelPoint(%v) on chain", commitSpend.SpenderTxHash, - c.cfg.chanState.FundingOutpoint) - - select { - case confInfo, ok := <-confNtfn.Confirmed: - if !ok { - log.Errorf("notifier exiting") - return - } - - log.Infof("closeObserver: ChannelPoint(%v) is fully "+ - "closed, at height: %v", - c.cfg.chanState.FundingOutpoint, - confInfo.BlockHeight) - - err := c.cfg.markChanClosed() - if err != nil { - log.Errorf("unable to mark chan fully "+ - "closed: %v", err) - return - } - - case <-c.quit: - return - } - }() + log.Infof("closeObserver: ChannelPoint(%v) is fully "+ + "closed, at height: %v", + c.cfg.chanState.FundingOutpoint, + commitSpend.SpendingHeight) c.Lock() for _, sub := range c.clientSubscriptions { @@ -524,6 +492,14 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet } c.Unlock() + // Now notify the ChainArbitrator that the watcher's job is done, such + // that it can shut it down and clean up. + if err := c.cfg.notifyChanClosed(); err != nil { + log.Errorf("unable to notify channel closed for "+ + "ChannelPoint(%v): %v", + c.cfg.chanState.FundingOutpoint, err) + } + return nil } From 921f02fe22749148b42839489880711bd2ec270f Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 22 May 2018 14:04:53 +0200 Subject: [PATCH 08/13] contractcourt/chain_arbitrator: markChanClosed->notifyChanClosed We no longer have to mark the channel as fully closed in the database, as it is done directly in the chainWatcher. Instead, we stop the watcher and delete it from the set of active watchers. --- contractcourt/chain_arbitrator.go | 36 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index 1e2079b7..944b6cc8 100644 --- a/contractcourt/chain_arbitrator.go +++ b/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) @@ -677,8 +690,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) From 74205a64fd34879f031b85722d8fc3549889463a Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 18 May 2018 15:21:31 +0200 Subject: [PATCH 09/13] lnd_test: check for coop closed channel among WaitingClose instead of PendingClosed --- lnd_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index 3cc478bf..7105676e 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -216,7 +216,7 @@ func closeChannelAndAssert(ctx context.Context, t *harnessTest, chanPointStr := fmt.Sprintf("%v:%v", txid, fundingChanPoint.OutputIndex) // If we didn't force close the transaction, at this point, the channel - // should now be marked as being in the state of "pending close". + // should now be marked as being in the state of "waiting close". if !force { pendingChansRequest := &lnrpc.PendingChannelsRequest{} pendingChanResp, err := node.PendingChannels(ctx, pendingChansRequest) @@ -224,14 +224,14 @@ func closeChannelAndAssert(ctx context.Context, t *harnessTest, t.Fatalf("unable to query for pending channels: %v", err) } var found bool - for _, pendingClose := range pendingChanResp.PendingClosingChannels { + for _, pendingClose := range pendingChanResp.WaitingCloseChannels { if pendingClose.Channel.ChannelPoint == chanPointStr { found = true break } } if !found { - t.Fatalf("channel not marked as pending close") + t.Fatalf("channel not marked as waiting close") } } @@ -247,7 +247,7 @@ func closeChannelAndAssert(ctx context.Context, t *harnessTest, assertTxInBlock(t, block, closingTxid) - // Finally, the transaction should no longer be in the pending close + // Finally, the transaction should no longer be in the waiting close // state as we've just mined a block that should include the closing // transaction. This only applies for co-op close channels though. if !force { @@ -260,7 +260,7 @@ func closeChannelAndAssert(ctx context.Context, t *harnessTest, return false } - for _, pendingClose := range pendingChanResp.PendingClosingChannels { + for _, pendingClose := range pendingChanResp.WaitingCloseChannels { if pendingClose.Channel.ChannelPoint == chanPointStr { return false } From 72d9726e7fee678fd8e815f02b751e7fc5a4a49c Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 23 May 2018 12:07:35 +0200 Subject: [PATCH 10/13] channeldb/channel: update IsPending godoc The pending state definitin in ChannelCloseSummary was slightly changed in such a way that channels that has had their commitment broadcasted now is no longer considered "pending close". They now instead stay in the open chan bucket with the ChanStatus "CommitmentBroadcasted" until their commitment is confirmed. This commit updates the IsPending godoc to reflect this. --- channeldb/channel.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 2dec4741..dfa436b9 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -1673,12 +1673,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 From 69a76a808fdf86c8354f24d7ddab3ae306a72f24 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 23 May 2018 12:11:19 +0200 Subject: [PATCH 11/13] breacharbiter: add todo for removing IsPending check --- breacharbiter.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/breacharbiter.go b/breacharbiter.go index a5d12c88..938240aa 100644 --- a/breacharbiter.go +++ b/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 From 8afc7bf66e1bd6469c1d5cc8ab5e7f2f188213b1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 24 May 2018 10:24:31 +0200 Subject: [PATCH 12/13] contractcourt/chain_arbitrator: add TODO for removing watchForChannelClose --- contractcourt/chain_arbitrator.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index 944b6cc8..c567dcf9 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -392,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) From 5cef2bacdee207e6ef07f0e3b35287dc6eb74076 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 24 May 2018 10:29:20 +0200 Subject: [PATCH 13/13] rpcserver: add TODO for removing coop closes from "pending close" channels --- rpcserver.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rpcserver.go b/rpcserver.go index 6f0d01e1..59ce3af4 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1455,6 +1455,10 @@ func (r *rpcServer) PendingChannels(ctx context.Context, // If the channel was closed cooperatively, then we'll only // need to tack on the closing txid. + // TODO(halseth): remove. After recent changes, a coop closed + // channel should never be in the "pending close" state. + // Keeping for now to let someone that upgraded in the middle + // of a close let their closing tx confirm. case channeldb.CooperativeClose: resp.PendingClosingChannels = append( resp.PendingClosingChannels,