channeldb: Indicate cooperative close initator #3961

carlaKC · 2020-01-27T07:09:06Z

Update channeldb to save channel statuses indicating whether the local or remote party initiated a cooperative close. These values are written to db at the time of cooperative close broadcast, and reloaded in chain watcher when we need to dispatch a cooperative close.

channeldb/channel.go

contractcourt/chain_watcher.go

bhandras

Overall solid PR, my only wish is that it'd be great to have test for the added functionality, as in the current form tests are only patched.

carlaKC · 2020-01-30T12:11:09Z

Don't review right now, want to check out the travis failure.

channeldb/channel.go

carlaKC · 2020-02-03T14:43:11Z

Updated to add a new field in the db as @halseth suggested. I think it simplifies the PR nicely, and we don't use us 2 of our 3 remaining status bits.

halseth

Nice, last iteration looks 🔥 Just a few more things and we should be there 🙏

channeldb/db_test.go

rpcserver.go

halseth · 2020-02-13T14:22:23Z

channeldb/channel.go

+
+	// ChanStatusLocalCoopInitiated is set in addition to
+	// ChanStatusCoopBroadcasted when we initiate a cooperative close.
+	ChanStatusLocalCoopInitiated ChannelStatus = 1 << 5


cool, I like this :) Looking at the chan statuses, we don't actually store whether we initiate a force close. This could be useful information also, like if we initiate, but the remote force close ends up in the chain. So we could make this a bit more general, and store it also in the cases where we/them force close?

Updated to include force closes. Not quite the same flow for local vs remote force closes because we have some warning for local force where we can mark broadcast and set the initiator status. We only detect remote forces once they've been broadcast, and we don't mark broadcast (because we use that field for rebroadcasting on startup).

carlaKC · 2020-02-19T12:37:08Z

Ready for another look @bhandras!

A TL;DR on the changes since you last had a look:

Went back to using channel statuses since we are increasing the size of the field
The CloseSummary bucket is nasty to add fields to, channels are keys not buckets, and have a bunch of optional fields tacked on the end, decided not to use this. If we want to change it, a separate PR with full migration is in order IMO.
Since this is just an aesthetic change for rpc, I decided to use the values in the historical channel bucket, since it is now written for all channels

bhandras

lgtm 👍

bhandras · 2020-02-20T13:56:25Z

contractcourt/channel_arbitrator_test.go

+
+// createTestChannelArbitrator returns a channel arbitrator test context which
+// contains a channel arbitrator with default values. These values can be
+// changes by providing options which overwrite the default config.


halseth · 2020-02-19T14:07:43Z

channeldb/channel.go

+	// the channel.
+	ChanStatusLocalCloseInitiator ChannelStatus = 1 << 5
+
+	// ChanStatusRemoteCloseInitiator indicates that we initiated closing


s/we/the remote node

halseth · 2020-02-19T14:10:23Z

channeldb/channel.go

+// channel statuses which will be written to the historical channel bucket.
+// These statuses are used to record close initiators.
+func (c *OpenChannel) CloseChannel(summary *ChannelCloseSummary,
+	statuses ...ChannelStatus) error {


oh, elegant 😀

halseth · 2020-02-19T14:14:05Z

contractcourt/channel_arbitrator_test.go

 			return nil
 		},
 		IsPendingClose:        false,
 		ChainArbitratorConfig: chainArbCfg,
 		ChainEvents:           chanEvents,
 	}

+	// Apply all custom options to the config struct.
+	for _, o := range opts {
+		o(arbCfg)


halseth · 2020-02-19T14:19:59Z

contractcourt/channel_arbitrator_test.go

+			}
+
+			chanArbCtx, err := createTestChannelArbitrator(
+				t, log, withMarkClosed(alice.CloseChannel),


instead of making the dependency on the OpenChannel here, could we instead mock the CloseChannel method, and assert it is called with the expected states set.

nice, cuts down the test length too 👍

halseth · 2020-02-20T14:12:13Z

rpcserver.go

+
+	// The channel was closed before we started storing historical
+	// channels. Do  not return an error, initiator values are unknown.
+	case channeldb.ErrChannelNotFound:


return immediately to avoid indentation below

This commit adds two new channel statuses which indicate the party that initatited closing the channel. These statuses are set in conjunction with the existing commit broadcast status so that we do not need to migrate existing logic to handle multiple types of closes. This status is set for locally initiated force closes in this commit because they follow a similar pattern to cooparative closes, marking the commitment broadcast then proceeding with tx broadcast. Remote force closes are added in the following commit, as they are handled differently.

Add an optional channel status CloseChannel which will be stored on the hitsorical channel which is persisted at channel close. This status is used to set the close initiator for channels that do not complete the funding flow or we abandon. In follow up commits, this status will be used to record force and breach closes. The value is written to the historical channel bucket for diplay over rpc.

Add an initiator enum which allows up to display an unknown value for channels that are not in the historical chan bucket, rather than having and ambiguous false value also representing no-value. A both option is added to cover the case where both parties initiated a close on chain.

Update channel updates and subscription itest to check that close initiator is appropriately set for cooperative and force closes for the local and remote party.

halseth

LGTM 🔥

carlaKC requested review from cfromknecht, halseth, joostjager and Roasbeef as code owners January 27, 2020 07:09

carlaKC requested review from wpaulino and removed request for Roasbeef, halseth and joostjager January 27, 2020 07:09

carlaKC added accounting channels v0.10 labels Jan 27, 2020

Roasbeef requested review from bhandras and removed request for cfromknecht January 27, 2020 23:16

Roasbeef added this to the 0.10.0 milestone Jan 27, 2020

carlaKC force-pushed the 3709-cooperativecloseinitiator branch 2 times, most recently from 68e6073 to 2d4cdad Compare January 28, 2020 11:40

wpaulino reviewed Jan 29, 2020

View reviewed changes

channeldb/channel.go Outdated Show resolved Hide resolved

channeldb/channel.go Outdated Show resolved Hide resolved

contractcourt/chain_watcher.go Outdated Show resolved Hide resolved

bhandras requested changes Jan 29, 2020

View reviewed changes

carlaKC force-pushed the 3709-cooperativecloseinitiator branch from 2d4cdad to 13b88cd Compare January 30, 2020 09:52

carlaKC requested review from wpaulino and bhandras January 30, 2020 09:53

halseth reviewed Jan 31, 2020

View reviewed changes

channeldb/channel.go Outdated Show resolved Hide resolved

carlaKC force-pushed the 3709-cooperativecloseinitiator branch from 13b88cd to 858313a Compare February 3, 2020 11:02

carlaKC commented Feb 3, 2020

View reviewed changes

channeldb/channel.go Outdated Show resolved Hide resolved

carlaKC requested a review from halseth February 3, 2020 14:43

carlaKC requested a review from halseth February 10, 2020 10:32

carlaKC force-pushed the 3709-cooperativecloseinitiator branch from 49c85e8 to 9402e5e Compare February 13, 2020 10:19

carlaKC requested a review from bhandras February 13, 2020 10:27

halseth suggested changes Feb 13, 2020

View reviewed changes

carlaKC force-pushed the 3709-cooperativecloseinitiator branch 6 times, most recently from aebafec to 94368d5 Compare February 19, 2020 12:34

carlaKC requested a review from halseth February 19, 2020 12:37

bhandras approved these changes Feb 20, 2020

View reviewed changes

halseth suggested changes Feb 20, 2020

View reviewed changes

carlaKC added 8 commits February 21, 2020 13:23

channeldb: add historical channel bucket lookup

8d632b8

contractcourt/test: replace timeouts with constants

c9915e0

contractcourt: record force and breach close initiator

4eb3036

rpcserver: display open and close initiator

e74b6f8

lntest: check close initiator in basic channel itest

b3e6395

Update channel updates and subscription itest to check that close initiator is appropriately set for cooperative and force closes for the local and remote party.

carlaKC force-pushed the 3709-cooperativecloseinitiator branch from 94368d5 to b3e6395 Compare February 21, 2020 11:54

carlaKC requested a review from halseth February 21, 2020 12:50

halseth approved these changes Feb 21, 2020

View reviewed changes

halseth merged commit 38b521d into lightningnetwork:master Feb 21, 2020

carlaKC mentioned this pull request Feb 21, 2020

The closed channels list should return the initiator of channels #2594

Closed

carlaKC mentioned this pull request Mar 27, 2020

Initiated channels do not show total fees for closing txs in getTransactions #2936

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb: Indicate cooperative close initator #3961

channeldb: Indicate cooperative close initator #3961

carlaKC commented Jan 27, 2020

bhandras left a comment

carlaKC commented Jan 30, 2020

carlaKC commented Feb 3, 2020

halseth left a comment

halseth Feb 13, 2020

carlaKC Feb 18, 2020

carlaKC commented Feb 19, 2020

bhandras left a comment

bhandras Feb 20, 2020

halseth Feb 19, 2020

halseth Feb 19, 2020

halseth Feb 19, 2020

halseth Feb 19, 2020

carlaKC Feb 21, 2020

halseth Feb 20, 2020

halseth left a comment

channeldb: Indicate cooperative close initator #3961

channeldb: Indicate cooperative close initator #3961

Conversation

carlaKC commented Jan 27, 2020

bhandras left a comment

Choose a reason for hiding this comment

carlaKC commented Jan 30, 2020

carlaKC commented Feb 3, 2020

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Feb 19, 2020

bhandras left a 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 left a comment

Choose a reason for hiding this comment