Open johncantrell97 opened 3 months ago
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 35 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
lightning/src/ln/peer_handler.rs | 92.56% | 10 Missing and 1 partial :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks! I think we also need to move the
peer_lock.their_features
call up - we only callpeer_disconnected
if that line has been hit (Peer::handshake_complete
checks for it) so we want to always hit that immediately before we callpeer_connected
s.
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.
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?
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.
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.
@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.
@johncantrell97 Any interest in finishing this PR?
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 becauseread_event
will calldisconnect_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:Only calling
socket_disconnected
if the disconnection type is one the user detected. Ifread_event
returns an Err it breaks with a disconnection type of Disconnect::CloseConnection and does not call back intosocket_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.