[bugfix] Wait for confirmation before marking channel cooperatively closed #1248

halseth · 2018-05-15T13:56:40Z

This PR fixes an issue where we could go through a cooperative close, and marking the channel as closed in the DB before it was confirmed. This would lead to potential issues as we wouldn't start the necessary submodules to watch for the final close on restarts.

In #1017 we changed the definition of a closed channel to be a channel that has had its closing tx confirmed. This PR makes this true also for cooperative closes.

Roasbeef · 2018-05-15T22:30:46Z

This PR breaks the integration tests as is.

Roasbeef · 2018-05-15T22:34:35Z

I'm not sure what this is attempting to fix, as we'll mark it fully closed on confirmation, but still want the RPC layer to reflect that the channel is about to be closed.

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.

Removes CooperativeCloseCtx and methods.

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.

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.

… PendingClosed

halseth · 2018-05-22T12:38:36Z

While working on fixing the integration tests, I realized that the chancloser could be significantly simplified, as the chainwatcher now handles all possible closes that appears in the chain. As a result, this PR has been reworked to include this simplification, in addition to the original bugfix.

Roasbeef · 2018-05-23T00:05:25Z

@halseth you never responded to the initial comment stating that the initial version of this PR wasn't necessary. Atm, we always mark a channel as closed after we broadcast the co-op close transaction. Only once it's confirmed do we mark it as fully closed.

Roasbeef · 2018-05-23T00:11:23Z

peer.go

@@ -1671,7 +1644,6 @@ func (p *peer) handleLocalCloseReq(req *htlcswitch.ChanClose) {
 			req.TargetFeePerKw,
 			uint32(startingHeight),
 			req,
-			closeCtx,


Roasbeef · 2018-05-23T00:12:35Z

chancloser.go

-		// TODO(roasbeef): don't need, ChainWatcher will handle
-
-		c.state = closeFinished
+		if c.cfg.channel.MarkCommitmentBroadcasted(); err != nil {


The one side-effect of doing this, is that for a co-op close, we won't show what the proper final balances are (to account for the negotiated fees) until the closing transaction is fully closed.

Roasbeef · 2018-05-23T00:12:51Z

contractcourt/chain_watcher.go

@@ -751,147 +743,3 @@ func (c *chainWatcher) dispatchContractBreach(spendEvent *chainntnfs.SpendDetail

 	return nil
 }
-


Yay code deletion!

Roasbeef · 2018-05-23T00:15:48Z

contractcourt/chain_watcher.go

@@ -465,53 +468,18 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet
 		SettledBalance: localAmt,
 		CloseType:      channeldb.CooperativeClose,
 		ShortChanID:    c.cfg.chanState.ShortChanID(),
-		IsPending:      true,
+		IsPending:      false,


Why false? It's fully closed here as this will only be dispatched once the co-op close transaction hits the chain.

!IsPending means that it is fully closed.

Roasbeef · 2018-05-23T00:17:51Z

contractcourt/chain_arbitrator.go

-				markChanClosed: func() error {
-					// TODO(roasbeef): also need to pass in log?
-					return c.resolveContract(chanPoint, nil)
+				notifyChanClosed: func() error {


Where does the chain watcher mark the channel as fully closed? Even in that case, the resolveContract methods needs to be called in order to stop the channel arbitrator as well.

It is being marked fully closed when IsPending is set to false in the chain watcher.

AFAICT the resolveContract only sets IsPending = false and stops the chain watcher, which we still do here.

It also deletes the state of the chain arbitrator itself. As is now, this state will be left lingering after the channel has been closed cooperatively.

I still can't see where this is happening (arbLog will always be nil):

func (c *ChainArbitrator) resolveContract(chanPoint wire.OutPoint, arbLog ArbitratorLog) error { log.Infof("Marking ChannelPoint(%v) fully resolved", chanPoint) // First, we'll we'll mark the channel as fully closed from the PoV of // the channel source. err := c.chanSource.MarkChanFullyClosed(&chanPoint) if err != nil { log.Errorf("ChainArbitrator: unable to mark ChannelPoint(%v) "+ "fully closed: %v", chanPoint, err) return err } if arbLog != nil { // Once this has been marked as resolved, we'll wipe the log // that the channel arbitrator was using to store its // persistent state. We do this after marking the channel // resolved, as otherwise, the arbitrator would be re-created, // and think it was starting from the default state. if err := arbLog.WipeHistory(); err != nil { return err } } c.Lock() delete(c.activeChannels, chanPoint) chainWatcher, ok := c.activeWatchers[chanPoint] if ok { chainWatcher.Stop() } c.Unlock() return nil }

halseth · 2018-05-23T08:39:54Z

To answer the original question:
We have an assumption that channels marked as closed (meaning they are moved to the closed channel bucket) will never have a different commitment being confirmed then what we initially assumed. This was not handled in the coop case, where we would move the channel to the closed chan bucket after broadcasting our commitment.

This would cause problems in the (unlikely) case where a different commit then what we broadcasted was confirmed, but more importantly if lnd was restarted after the broadcast, we would never start the proper chain watcher again:

lnd/contractcourt/chain_arbitrator.go

Line 319 in 5c42a88

openChannels, err := c.chanSource.FetchAllChannels()

For channels "pending close" we would start a restricted watcher:

lnd/contractcourt/chain_arbitrator.go

Line 371 in 5c42a88

closingChannels, err := c.chanSource.FetchClosedChannels(true)

Since we are only fetching channels from the open chan bucket when starting the proper chain watchers, we would be blind to an unexpected commitment getting confirmed. Since the chan closer also was just all in-memory, it would not spin up its goroutines again. I also saw reports of lnd crashing if a force close was attempted at this stage (#1247 (comment))

A channel in the "waiting close" state is reflected in the RPC layer, since we have no certainty what kind of close it is going to go through.

State machine is as follows:
==== open chan bucket ======

Pending open: funding not confirmed
Open: funding confirmed
waiting close: commitment/close broadcasted

==== closed chan bucket =====

pending close: commitment/close confirmed, but we have outputs pending to be swept
closed: all contracts resolved

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.

Roasbeef · 2018-05-23T22:25:54Z

Since we are only fetching channels from the open chan bucket when starting the proper chain watchers, we would be blind to an unexpected commitment getting confirmed.

Ahh, I see what you're saying: although we expect a co-op close transaction to land in the chain, it may be the case that a force close transaction actually does instead. We look in the pending channel bucket (not the open channel bucket), so we would create an arbitrator in this limited form, but if a user attempted to force close again, then a panic would occur as the pointer wasn't set.

Roasbeef · 2018-05-23T23:18:56Z

contractcourt/chain_arbitrator.go

-				markChanClosed: func() error {
-					// TODO(roasbeef): also need to pass in log?
-					return c.resolveContract(chanPoint, nil)
+				notifyChanClosed: func() error {


It also deletes the state of the chain arbitrator itself. As is now, this state will be left lingering after the channel has been closed cooperatively.

Roasbeef · 2018-05-23T23:27:02Z

Actually resolveContract would still ultimately be called by the channelArbitrator in the case that any data was actually written to the arb log.

…Close

…nnels

halseth · 2018-05-24T10:48:03Z

Ahh, I see what you're saying: although we expect a co-op close transaction to land in the chain, it may be the case that a force close transaction actually does instead. We look in the pending channel bucket (not the open channel bucket), so we would create an arbitrator in this limited form, but if a user attempted to force close again, then a panic would occur as the pointer wasn't set.

Yep

Roasbeef

LGTM 🔥

halseth added 9 commits May 22, 2018 11:51

lnwallet/channel: remove unused method DeleteState

7cd783d

lnwallet/channel: export method MarkCommitmentBroadcasted

9ddd484

peer: don't pass closeCtx to chanCloser

bb611e0

contractcourt/chain_arbitrator: remove BeginCoopChanClose

21d1cc3

contractcourt/chain_watcher: remove CooperativeCloseCtx

af14a2f

Removes CooperativeCloseCtx and methods.

contractcourt/chain_arbitrator: markChanClosed->notifyChanClosed

921f02f

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.

lnd_test: check for coop closed channel among WaitingClose instead of…

74205a6

… PendingClosed

halseth force-pushed the close-channel-fix branch from 9bf4977 to 74205a6 Compare May 22, 2018 12:35

Roasbeef requested changes May 23, 2018

View reviewed changes

halseth added 2 commits May 23, 2018 12:07

breacharbiter: add todo for removing IsPending check

69a76a8

Roasbeef requested changes May 23, 2018

View reviewed changes

Roasbeef added this to the 0.4.2-beta milestone May 23, 2018

halseth added 2 commits May 24, 2018 10:24

contractcourt/chain_arbitrator: add TODO for removing watchForChannel…

8afc7bf

…Close

rpcserver: add TODO for removing coop closes from "pending close" cha…

5cef2ba

…nnels

Roasbeef approved these changes May 25, 2018

View reviewed changes

Roasbeef merged commit fc3d711 into lightningnetwork:master May 25, 2018

halseth deleted the close-channel-fix branch July 10, 2018 10:33

carlaKC mentioned this pull request Mar 11, 2020

local/remote balances and channel phases clarification #4067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Wait for confirmation before marking channel cooperatively closed #1248

[bugfix] Wait for confirmation before marking channel cooperatively closed #1248

halseth commented May 15, 2018

Roasbeef commented May 15, 2018

Roasbeef commented May 15, 2018

halseth commented May 22, 2018 •

edited

Loading

Roasbeef commented May 23, 2018

Roasbeef May 23, 2018

Roasbeef May 23, 2018

Roasbeef May 23, 2018

Roasbeef May 23, 2018

halseth May 23, 2018

Roasbeef May 23, 2018

halseth May 23, 2018

Roasbeef May 23, 2018

halseth May 24, 2018

halseth commented May 23, 2018 •

edited

Loading

Roasbeef commented May 23, 2018

Roasbeef May 23, 2018

Roasbeef commented May 23, 2018

halseth commented May 24, 2018

Roasbeef left a comment

		@@ -751,147 +743,3 @@ func (c chainWatcher) dispatchContractBreach(spendEvent chainntnfs.SpendDetail

		return nil
		}

[bugfix] Wait for confirmation before marking channel cooperatively closed #1248

[bugfix] Wait for confirmation before marking channel cooperatively closed #1248

Conversation

halseth commented May 15, 2018

Roasbeef commented May 15, 2018

Roasbeef commented May 15, 2018

halseth commented May 22, 2018 • edited Loading

Roasbeef commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented May 23, 2018 • edited Loading

Roasbeef commented May 23, 2018

Choose a reason for hiding this comment

Roasbeef commented May 23, 2018

halseth commented May 24, 2018

Roasbeef left a comment

Choose a reason for hiding this comment

halseth commented May 22, 2018 •

edited

Loading

halseth commented May 23, 2018 •

edited

Loading