ensure peer_connected is called before peer_disconnected

[johncantrell97]

Fixes #3108

Makes sure all message handler's peer_connected methods are called instead of returning early on the first to error.

As for whether or not the user has to call back into socket_disconnected after a PeerManager::read_event, I assume you mean after it returns an Err? I think the user does not have to because read_event will call disconnect_event_internal on any error before returning it to the user.

I took a look at lightning-net-tokio and it appears to be the case over there as well. It does:

if let Disconnect::PeerDisconnected = disconnect_type {
    peer_manager.as_ref().socket_disconnected(&our_descriptor);
    peer_manager.as_ref().process_events();
}

Only calling socket_disconnected if the disconnection type is one the user detected. If read_event returns an Err it breaks with a disconnection type of Disconnect::CloseConnection and does not call back into socket_disconnected.

Matt seems to think you do have to so I'm probably misunderstanding the original question. Happy to dig into it a bit more with some clarification if I misunderstood.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.56757% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.78%. Comparing base (9789152) to head (922c31f). Report is 1274 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/peer_handler.rs	92.56%	10 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3110      +/-   ##
==========================================
+ Coverage   89.84%   90.78%   +0.93%     
==========================================
  Files         119      119              
  Lines       97561   103463    +5902     
  Branches    97561   103463    +5902     
==========================================
+ Hits        87655    93925    +6270     
+ Misses       7331     7032     -299     
+ Partials     2575     2506      -69     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[johncantrell97]

Thanks! I think we also need to move the peer_lock.their_features call up - we only call peer_disconnected if that line has been hit (Peer::handshake_complete checks for it) so we want to always hit that immediately before we call peer_connecteds.

Whoops, fixed it.

Can't be before calls to peer_connected because they pass a reference to msg but as long as we do it before returning it should be okay.

[johncantrell97]

Would be nice to get a test (which should be pretty easy), but either way LGTM.

Doesn't look like there's easy way to handle testing it with the existing test message handlers. Should I create new ones that can error on peer_connected and track connected/disconnected have been called or add the functionality to the existing test handlers?

Have used something like mockall for this in the past but without it I'd have to add counters/flags that get updated and a way to check them.

Is this what you had in mind for being able to test it?

[TheBlueMatt]

Yea, I was figuring you'd just create a trivial CustomMessageHandler that asserts that connected/disconnecteds all come in order and then errors on connection.

[johncantrell97]

Yea, I was figuring you'd just create a trivial CustomMessageHandler that asserts that connected/disconnecteds all come in order and then errors on connection.

Hm, using a CustomMessageHandler doesn't really test the fix here since it goes last. One of the issues was the early return causing the later handlers to not get the peer_connected at all. Would have to use multiple new handlers to be able to check the one after an error is returned still gets peer_connected called on it (and disconnected)

I guess at least it would catch the fix for ensuring disconnect is called.

[johncantrell97]

@TheBlueMatt

Added a test that passes but it duplicates a ton of code to handle all of the setup but with the new message handlers :|

not sure if this is okay, looking for feedback on the test and how to do it better if it's not okay.

[tnull]

@johncantrell97 Any interest in finishing this PR?

[TheBlueMatt]

Supersceded by #3580.