Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add channel closure details #4126

Merged
merged 11 commits into from
Nov 6, 2020
Merged

feat: add channel closure details #4126

merged 11 commits into from
Nov 6, 2020

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Oct 14, 2020 •
edited
Loading

Ready for review

This adds a state_change cause to a channel in order to tell if it was closed (or opened) for "REMOTE","LOCAL","USER", "PROTOCOL" or "ONCHAIN" reasons. Since a lot of state changes are subsequent results of a prior cause, we store the last known ( not REASON_UNKNOWN ) state_change as cause for subsequent state changes. We add a database table that stores all state changes with their distinct cause and message.

  • adds last known channel state_change cause and enum side closer to struct channel
  • adding char * message
  • persistence state_change cause and message in DB
  • output 'closer' and 'opener' to listpeers channels
  • persistence state_change history with cause, message and timestamp in DB
  • output state_change history to listpeers or a new command?
  • documentation

Addresses #4027

Questions:

  • Instead of recycling htlc.h enum side as closer and initializing it in DB with NUM_SIDES := 2 (kinda whacky t.b.h.) we can introduce an enum closer which can be e.g. CLOSER_NONE, CLOSER_LOCAL, CLOSER_REMOTE and maybe CLOSER_ONCHAIN. Or, if we already want CLOSER_ONCHAIN we can use the enum state_change for this which covers all of the above + 'CLOSER_USER', but then that's another recycling. Please advice.

@m-schmoock m-schmoock changed the title feat: add channel close cause feat: add channel closure details Oct 14, 2020
@m-schmoock m-schmoock force-pushed the feat/closure_details branch 2 times, most recently from 73aa2c8 to 6457b02 Compare October 15, 2020 12:16
@niftynei niftynei added the clightning_twit Tag to nudge @niftynei to post to @clightning_twit label Oct 15, 2020
@m-schmoock m-schmoock force-pushed the feat/closure_details branch 17 times, most recently from ae455c1 to 6ecc5eb Compare October 22, 2020 09:03
@m-schmoock m-schmoock marked this pull request as ready for review October 22, 2020 09:04
@m-schmoock
Copy link
Collaborator Author

@cdecker @rustyrussell
I rebased to make it compatible with recent DB and CLI changes...

@niftynei niftynei added this to the v0.9.2 milestone Oct 27, 2020
@m-schmoock
Copy link
Collaborator Author

added missing documentation.

@m-schmoock m-schmoock force-pushed the feat/closure_details branch 2 times, most recently from 9be595c to 6db7961 Compare October 28, 2020 12:06
cdecker
cdecker reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR. Just some bike-shedding on who is to blame for various state changes and some cleanup suggestions.

lightningd/channel_control.c Outdated Show resolved Hide resolved
lightningd/peer_control.c Outdated Show resolved Hide resolved
lightningd/peer_htlcs.c Outdated Show resolved Hide resolved
lightningd/peer_htlcs.c Outdated Show resolved Hide resolved
wallet/db.c Outdated Show resolved Hide resolved
wallet/wallet.c Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the feat/closure_details branch 3 times, most recently from 9442d1d to 439e0ba Compare November 4, 2020 11:57
@m-schmoock
Copy link
Collaborator Author

@cdecker do you think we should remove REASON_ONCHAIN and use REASON_REMOTE for it? It is extremly unlikely that an onchain force close was caused by us without knowing... Maybe only if one uses some scratching script and then re-using the node again. Currently REASON_ONCHAIN means kinda the same as REASON_REMOTE with the additional information that it was force closed by the remote, while we were offline or for other reason.

@cdecker
Copy link
Member

cdecker commented Nov 4, 2020

Sorry for burying the reply to this in a thread:

I think indeed ONCHAIN is unlikely to be used for much. Aside from maybe channels being forced to close due to feerates moving out of the permissible window, I don't see something that wouldn't be classified either local or remote.

@ElementsProject ElementsProject deleted a comment from m-schmoock Nov 5, 2020
@m-schmoock
Copy link
Collaborator Author

Sorry for burying the reply to this in a thread:

I think indeed ONCHAIN is unlikely to be used for much. Aside from maybe channels being forced to close due to feerates moving out of the permissible window, I don't see something that wouldn't be classified either local or remote.

@cdecker
I think I will remove the REASON_ONCHAIN (as a user can find out the force close by checking the state change history).
If we find an onchain closing tx, this is assumed to be REASON_REMOTE unless we triggered it via RPC or internal reasons (fees), in this case it will be REASON_USER or REASON_LOCAL respectively.

I will also extend on the testcases to also cover some of the itnernal/protocol reasons.

@m-schmoock
feat: adds state change cause and message
59e4ec2 
This adds a `state_change` 'cause' to a channel.
A 'cause' is some initial 'reason' a channel was created or closed by:

  /* Anything other than the reasons below. Should not happen. */
  REASON_UNKNOWN,
  /* Unconscious internal reasons, e.g. dev fail of a channel. */
  REASON_LOCAL,
  /* The operator or a plugin opened or closed a channel by intention. */
  REASON_USER,
  /* The remote closed or funded a channel with us by intention. */
  REASON_REMOTE,
  /* E.g. We need to close a channel because of bad signatures and such. */
  REASON_PROTOCOL,
  /* A channel was closed onchain, while we were offline. */
  /* Note: This is very likely a conscious remote decision. */
  REASON_ONCHAIN

If a 'cause' is known and a subsequent state change is made with
`REASON_UNKNOWN` the preceding cause will be used as reason, since a lot
(all `REASON_UNKNOWN`) state changes are a subsequent consequences of a prior
cause: local, user, remote, protocol or onchain.

Changelog-Added: Plugins: Channel closure resaon/cause to channel_state_changed notification
@m-schmoock
pytest: channel state change cause and message
a6b6714
@m-schmoock
db: persist channel closer and state change cause
2374638
@m-schmoock
rpc: adds opener and closer to listpeers channels
f079636 
Changelog-Added: RPC: Added 'opener' and 'closer' to listpeers channels
@m-schmoock
pytest: test opener and closer of listpeers
20d7d46
@m-schmoock
rpc: adds json_add_timeiso helper
51b5092
@m-schmoock
feat: adds timestamp to state_change notification
75e8a6d
@m-schmoock
db: adds a state_change history to database
e713025
@m-schmoock
feat: adds state_changes to listpeers output
96dcbf5 
Changelog-Added: RCP: Added 'state_changes' history to listpeers channels
@m-schmoock
pytest: state_change history
7289e9e
@m-schmoock
doc: update channel_state_changed notification
edca8cb
@cdecker
Copy link
Member

cdecker commented Nov 6, 2020

ACK edca8cb

@niftynei niftynei merged commit 8613785 into ElementsProject:master Nov 6, 2020
@m-schmoock m-schmoock deleted the feat/closure_details branch November 7, 2020 10:40
fiatjaf pushed a commit to fiatjaf/mcldsp that referenced this pull request Nov 9, 2020
add table channel_state_changes.
dc1308b 
following @m-schmoock's ElementsProject/lightning#4126
fiatjaf pushed a commit to fiatjaf/mcldsp that referenced this pull request Nov 9, 2020
channels: closer and state_change_reason
6a7f712 
also following ElementsProject/lightning#4126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker cdecker left review comments

Assignees
No one assigned
Labels
clightning_twit Tag to nudge @niftynei to post to @clightning_twit
Projects
None yet
Milestone
v0.9.2
Development

Successfully merging this pull request may close these issues.
3 participants
@m-schmoock @cdecker @niftynei