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

feat: notification channel_state_changed #4020

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Sep 8, 2020

Summary

This adds a new plugin notification message channel_state_changed that is called when a channel changes its state.

This PR contains code, test and doc.
Addresses #4018

Example message

{
    "channel_state_changed": {
        "peer_id": "03bc9337c7a28bb784d67742ebedd30a93bacdf7e4ca16436ef3798000242b2251",
        "channel_id": "a2d0851832f0e30a0cf778a826d72f077ca86b69f72677e0267f23f63a0599b4",
        "short_channel_id" : "561820x1020x1",
        "old_state": "CHANNELD_NORMAL",
        "new_state": "CHANNELD_SHUTTING_DOWN"
    }
}

Future thoughts

@fiatjaf added the question if it would be possible to have some kind of a 'reason' for a state changed operation. From a user or plugin developer perspective, this is mostly the distinction if a channel was closed by remote or by the user or for other (internal) reasons. We could add an string indication to this event, i.e. "reason" : "USER" or "reason" : "REMOTE" or "reason" : "OTHER" . Also, this only makes sens for state changes from CHANNELD_NORMAL to some CLOSING state. Maybe AWAITING_LOCKIN can also be caused by the user if its a local funding thats ongoing.

Note

When unilaterally closing a channel it happens that the state changes two times to the same value AWAITING_UNILATERAL.
I don't think this is intended, but current test reflect that.

@m-schmoock m-schmoock force-pushed the feat/notification_channel_state_changed branch 3 times, most recently from 3ff0d64 to 5c4432b Compare September 8, 2020 17:26
@fiatjaf
Copy link
Contributor

fiatjaf commented Sep 8, 2020

How impossible would it be to have the reason of a channel state change? Is there such a concept?

@m-schmoock m-schmoock force-pushed the feat/notification_channel_state_changed branch 6 times, most recently from 898bd51 to cfdeab6 Compare September 9, 2020 07:50
@ZmnSCPxj ZmnSCPxj added this to the v0.9.1 milestone Sep 9, 2020
@cdecker
Copy link
Member

cdecker commented Sep 9, 2020

How impossible would it be to have the reason of a channel state change? Is there such a concept?

We currently only produce logs with the specifics, but we should be able to enumerate the possible causes and pass them in. I think this is a great idea, both for this hook as well as for the status in listpeers.

@cdecker
Copy link
Member

cdecker commented Sep 9, 2020

Opened #4027 to track this, and brainstorm how these details could look like.

@rustyrussell
Copy link
Contributor

Ack cfdeab6

tests/test_plugin.py Outdated Show resolved Hide resolved
This notification will be raised whenever a channel state changes.
The payload includes the channel and peer identifiers and the
old and the new state.

Example payload:

```
{
    "channel_state_changed": {
        "peer_id": "03bc9337c7a28bb784d67742ebedd30a93bacdf7e4ca16436ef3798000242b2251",
        "channel_id": "a2d0851832f0e30a0cf778a826d72f077ca86b69f72677e0267f23f63a0599b4",
        "short_channel_id" : "561820x1020x1",
        "old_state": "CHANNELD_NORMAL",
        "new_state": "AWAITING_UNILATERAL"
    }
}
```

Changelog-Added: Plugins: channel_state_changed notification
@m-schmoock m-schmoock force-pushed the feat/notification_channel_state_changed branch from cfdeab6 to 6fc4f2f Compare September 9, 2020 11:29
@cdecker
Copy link
Member

cdecker commented Sep 9, 2020

LGTM

ACK 6fc4f2f

@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Sep 9, 2020

Note: Pipeline #20 currently ends up in "The job exceeded the maximum time limit for jobs, and has been terminated."
And a lot of errors and stacktraces unrelated to this PR changes... This seems for all other builds an PRs as well.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 6fc4f2f

@rustyrussell rustyrussell merged commit 160c564 into ElementsProject:master Sep 10, 2020
@m-schmoock m-schmoock deleted the feat/notification_channel_state_changed branch September 10, 2020 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants