Allow all RPC messages on disconnect #5876
Conversation
| }; | ||
| if !self.peer_manager().is_connected(&peer_id) | ||
| // Do not permit Inbound events from peers that are being disconnected | ||
| && matches!(event.event, HandlerEvent::Err(HandlerErr::Inbound { .. })) |
There was a problem hiding this comment.
Why do you want to suppress Inbound errors? Is there any known scoring issue that can happen here? We should document why if that's the case
|
@AgeManning we can move the is_connected check to the main match. I am not convinced of having to ignore inbound errors. I've this commit moving the check to the match + only ignoring inbound requests 32c8fce |
|
Yeah fair point. I don't recall off the top of my head, I imagine there are few sneaky edge cases, like peers instantly dropping connections or something when we send a goodbye. Your commit does look cleaner, but I would test it before merging it in here. You can follow this guide: https://github.com/sigp/lighthouse/tree/stable/testing/network_testing To run a fresh node on mainnet without syncing or anything and see if it has any weird errors. If not, lets do it. |
Squashed commit of the following: commit 6d42ebc Author: Age Manning <[email protected]> Date: Fri May 31 20:21:26 2024 +1000 Permit rpc messages on disconnect
Squashed commit of the following: commit 6d42ebc Author: Age Manning <[email protected]> Date: Fri May 31 20:21:26 2024 +1000 Permit rpc messages on disconnect
Squashed commit of the following: commit 6d42ebc Author: Age Manning <[email protected]> Date: Fri May 31 20:21:26 2024 +1000 Permit rpc messages on disconnect
| && matches!(event.event, HandlerEvent::Err(HandlerErr::Inbound { .. })) | ||
| && matches!(event.event, HandlerEvent::Ok(RPCReceived::Request(..))) |
There was a problem hiding this comment.
looks like this condition only hits if event.event is both an Err and Ok at the same time
There was a problem hiding this comment.
I'm guessing the intent was && should be ||
Squashed commit of the following: commit de97efc Author: Michael Sproul <[email protected]> Date: Thu Jun 6 12:13:28 2024 +1000 Use or instead of and commit 6d42ebc Author: Age Manning <[email protected]> Date: Fri May 31 20:21:26 2024 +1000 Permit rpc messages on disconnect
michaelsproul
force-pushed
the
permit-rpc-on-disconnect
branch
from
c4f7811
to
de97efc
Compare
|
I pushed a commit here and then force pushed it out of existence, because I misread some logs on our infra thinking that the |
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 22fe0a6 |
This PR makes it more permissive for outbound RPC messages to reach the application layer.
Previously, RPC requests were not getting stream terminations in race conditions between a peer disconnecting and the RPC handler propagating the message up.
This PR now allows messages to pass up to the application layer in the events that we start disconnecting from a peer. This will even allow slower messages to be sent to the application layer after peers have been disconnected.
This should help resolve further sync lookup bugs.