add peer_(dis)connected to custom message handler #3105
Conversation
johncantrell97
force-pushed
the
johncantrell97/add-peer-dis/connected-to-cmh
branch
from
501b6dd to
a2a859f
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3105 +/- ##
==========================================
- Coverage 89.88% 89.83% -0.06%
==========================================
Files 119 119
Lines 97551 97561 +10
Branches 97551 97561 +10
==========================================
- Hits 87681 87640 -41
- Misses 7304 7343 +39
- Partials 2566 2578 +12 ☔ View full report in Codecov by Sentry. |
johncantrell97
force-pushed
the
johncantrell97/add-peer-dis/connected-to-cmh
branch
2 times, most recently
from
9fe1260 to
432a95d
Compare
lightning/src/ln/peer_handler.rs
Outdated
| @@ -79,6 +79,17 @@ pub trait CustomMessageHandler: wire::CustomMessageReader { | |||
| /// connection to the node exists, then the message is simply not sent. | |||
| fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)>; | |||
|
|
|||
| // Connection loss/reestablish: | |||
| /// Indicates a connection to the peer failed/an existing connection was lost. | |||
There was a problem hiding this comment.
IIRC this only fires if we previously called peer_connected, so it is not called if a connection "fails".
There was a problem hiding this comment.
Oh ok, might want to change it on ChannelMessageHandler too then?
| @@ -79,6 +79,17 @@ pub trait CustomMessageHandler: wire::CustomMessageReader { | |||
| /// connection to the node exists, then the message is simply not sent. | |||
| fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)>; | |||
|
|
|||
| // Connection loss/reestablish: | |||
| /// Indicates a connection to the peer failed/an existing connection was lost. | |||
| fn peer_disconnected(&self, their_node_id: &PublicKey); | |||
There was a problem hiding this comment.
This is currently not called in PeerManager, we should update the places we currently call the other peer_disconnecteds to also call this.
There was a problem hiding this comment.
Right, whoops. I think I fixed this.
johncantrell97
force-pushed
the
johncantrell97/add-peer-dis/connected-to-cmh
branch
from
432a95d to
1c063e4
Compare
lightning/src/ln/peer_handler.rs
Outdated
| /// Indicates a connection to the peer was lost. | ||
| fn peer_disconnected(&self, their_node_id: &PublicKey); | ||
|
|
||
| /// Handle a peer reconnecting. |
There was a problem hiding this comment.
| /// Handle a peer reconnecting. | |
| /// Handle a peer connecting. |
lightning/src/ln/peer_handler.rs
Outdated
| @@ -79,6 +79,17 @@ pub trait CustomMessageHandler: wire::CustomMessageReader { | |||
| /// connection to the node exists, then the message is simply not sent. | |||
| fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)>; | |||
|
|
|||
| // Connection loss/reestablish: | |||
There was a problem hiding this comment.
nit: Can we remove this non-doc comment? Seems superfluous.
lightning/src/ln/peer_handler.rs
Outdated
| @@ -79,6 +79,17 @@ pub trait CustomMessageHandler: wire::CustomMessageReader { | |||
| /// connection to the node exists, then the message is simply not sent. | |||
| fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)>; | |||
|
|
|||
| // Connection loss/reestablish: | |||
| /// Indicates a connection to the peer was lost. | |||
There was a problem hiding this comment.
nit: "a connection to the peer was lost." sounds like it timed out, but of course this could also be a regular disconnect?
There was a problem hiding this comment.
Hm, I guess lost sort of implies an unintentional disconnection but it's still pretty generic. How about just keeping it simple and saying "Indicates a peer disconnected."
johncantrell97
force-pushed
the
johncantrell97/add-peer-dis/connected-to-cmh
branch
from
1c063e4 to
602921c
Compare
Protocols utilizing the custom message handler might need to know about peer connection and disconnection. At a minimum LSPS5 requires this functionality.