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.

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

Add closure details to listpeers and channel_state_changed hook #4027

Closed
4 tasks
cdecker opened this issue Sep 9, 2020 · 5 comments
Closed
4 tasks

Add closure details to listpeers and channel_state_changed hook #4027

cdecker opened this issue Sep 9, 2020 · 5 comments

Comments

@cdecker
Copy link
Member

cdecker commented Sep 9, 2020

One of the recurring things we get asked for is "why was this channel closed?"
to which we usually just point to logs, that may not be present anymore due to
a restart. @ZmnSCPxj proposed adding a closure reason in [#4020] that'd
contain the cause of a channel closure (or more generally a state change) to
the listpeers result and to the channel_state_changed hook. I think this
could go a long way towards helping users perform cause analysis for any
unexpected closure.

Open Questions

  • What state changes should be covered? Open, collaborative close,
    unilateral close, feerate adjustment, HTLC addition / removal. Arguably
    the latter two might be too detailed and cause a lot of overhead /
    noise in the status listing.
  • Format of the information
  • Should the information be persisted across restarts? If yes I'd argue
    we should create a single chanenl_state_change table that covers all
    possible fields, and not multiple tables.
  • Would a state change history be desirable, or could that result in too
    much overhead (feerate adjustments, HTLC operations, ...)
@m-schmoock
Copy link
Collaborator

@cdecker
From my side:

  • 'No' to the question of a state change history. Plugins can do that themselves if they need to
  • On the reasons: The main distinction a user/plugin needs to make is if the channel was closed for USER/LOCAL reasons or REMOTE reasons or OTHER internal reasons. We can fiddle around to get more specific, but I think this is pretty much it.

@m-schmoock
Copy link
Collaborator

m-schmoock commented Oct 1, 2020

@cdecker

In my opinion this would require add a simple enum to the notification params like:

enum channel_state_change_reason {
	CSCR_UNKNOWN,  /* anything other than the reasons below */
	CSCR_ONCHAIN,  /* something happened onchain that forces us to change the state */
	CSCR_USER,     /* the operator or a plugin opened or closed a channel by 'hand' */
	CSCR_LOCAL,    /* known internal reasons, i.e. a failed HTLC */
	CSCR_REMOTE    /* the remote bilateral close or funded a channel with us */
};

What do you think of such a generic approach that categorizes the reasons?
Do you want to add/remove a category?
If we want variable string message on-top we can add that later, if required.

Also I think it might be necessary to add this to the enum channel as the 'last known reason'.
Rationale for this is, that a lot of the code path are setting the state as a subsequent update for some initial cause/reason.
Then we only set the last known reason if we really know the underlying cause...

@m-schmoock
Copy link
Collaborator

Can be closed

@jsarenik
Copy link
Contributor

jsarenik commented Nov 8, 2020

ACK close 🎻

@cdecker
Copy link
Member Author

cdecker commented Dec 28, 2020

Yessir 🙃

@cdecker cdecker closed this as completed Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants