Errors for all RPC Requests by AgeManning · Pull Request #5867 · sigp/lighthouse

AgeManning · 2024-05-30T07:11:29Z

Sync now requires all RPC messages to either return a success or a failure. A disconnection doesn't count as a failure.

Prior to this PR, there were two known cases where an RPC request can get "lost", i.e no error response. These are:

A peer disconnects just after sync sends an RPC request message - These messages get dropped silently
A peer disconnects whilst we are self rate limiting our own RPC requests

This PR should remedy these cases. In the first case, we instantly send an error response when the peer has disconnected. In the second case, we now watch for disconnection messages and then report an error for all messages that we are currently holding due to self rate limiting.

In the process of doing this PR, I thought I needed an updated version of tokio-util in order to make the code cleaner. It turned out that wasn't the case, but I had already done the work to upgrade it. So I have left these changes in, which imo improve the code by removing a match case in most cases.

jxs

LGTM age

jxs · 2024-05-30T22:13:22Z

beacon_node/lighthouse_network/src/rpc/self_limiter.rs

+        self.delayed_requests
+            .retain(|(map_peer_id, protocol), queue| {
+                if map_peer_id == &peer_id {
+                    // NOTE: Currently cannot remove entries from the DelayQueue, we will just let


I was looking and I think we don't need ready_requests as when next_peer_request is ready we can just return it and keep returning as long as it's ready SelfRateLimiter will keep being polled. I can submit a subsequent PR removing it if you agree.

I had an initial version of this that removed the delalyQueue and used a HashMapDelay. The problem I ran into was that typically there is a queue or backlog per (peer_id, protocol). When we insert them, the delay time is for the first in the queue. When I was using a HashMapDelay, they all expired really quickly which meant we had to keep requeuing them.

The current version just expires the first element in the queue then we check for the next element and queue that, rather than all elements in the queue, if this makes sense.

beacon_node/lighthouse_network/src/service/mod.rs

Squashed commit of the following: commit a3c8e01 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:38:35 2024 +1000 Downgrade to 1.77 commit dbfde08 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:34:11 2024 +1000 Bump rust version to 1.78 commit 2e4fe3f Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:26:50 2024 +1000 Code improvement commit 70e5326 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:05:00 2024 +1000 Report errors for rate limited requests commit 7b0e346 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 13:25:22 2024 +1000 Return and error if peer has disconnected

Co-authored-by: João Oliveira <hello@jxs.pt>

dapplion

LGTM

Squashed commit of the following: commit a3c8e01 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:38:35 2024 +1000 Downgrade to 1.77 commit dbfde08 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:34:11 2024 +1000 Bump rust version to 1.78 commit 2e4fe3f Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:26:50 2024 +1000 Code improvement commit 70e5326 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:05:00 2024 +1000 Report errors for rate limited requests commit 7b0e346 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 13:25:22 2024 +1000 Return and error if peer has disconnected

Squashed commit of the following: commit 9699902 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun Jun 2 20:07:07 2024 +0200 fix fmt commit 05bddb9 Author: Age Manning <Age@AgeManning.com> Date: Sun Jun 2 15:04:02 2024 +1000 Update beacon_node/lighthouse_network/src/service/mod.rs Co-authored-by: João Oliveira <hello@jxs.pt> commit a3c8e01 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:38:35 2024 +1000 Downgrade to 1.77 commit dbfde08 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:34:11 2024 +1000 Bump rust version to 1.78 commit 2e4fe3f Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:26:50 2024 +1000 Code improvement commit 70e5326 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:05:00 2024 +1000 Report errors for rate limited requests commit 7b0e346 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 13:25:22 2024 +1000 Return and error if peer has disconnected

…c-peer-disconnect-error

Squashed commit of the following: commit 638a9bb Author: realbigsean <seananderson33@GMAIL.com> Date: Wed Jun 5 18:11:20 2024 -0400 update lockfile commit 7b2cc6c Merge: 9699902 7a7fc82 Author: realbigsean <seananderson33@GMAIL.com> Date: Wed Jun 5 18:11:03 2024 -0400 Merge branch 'unstable' of https://github.com/sigp/lighthouse into rpc-peer-disconnect-error commit 9699902 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun Jun 2 20:07:07 2024 +0200 fix fmt commit 05bddb9 Author: Age Manning <Age@AgeManning.com> Date: Sun Jun 2 15:04:02 2024 +1000 Update beacon_node/lighthouse_network/src/service/mod.rs Co-authored-by: João Oliveira <hello@jxs.pt> commit a3c8e01 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:38:35 2024 +1000 Downgrade to 1.77 commit dbfde08 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:34:11 2024 +1000 Bump rust version to 1.78 commit 2e4fe3f Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:26:50 2024 +1000 Code improvement commit 70e5326 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 17:05:00 2024 +1000 Report errors for rate limited requests commit 7b0e346 Author: Age Manning <Age@AgeManning.com> Date: Thu May 30 13:25:22 2024 +1000 Return and error if peer has disconnected

michaelsproul · 2024-06-07T00:10:51Z

@Mergifyio queue

mergify · 2024-06-07T00:11:08Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 7b48b0b

AgeManning added 2 commits May 30, 2024 13:25

Return and error if peer has disconnected

7b0e346

Report errors for rate limited requests

70e5326

AgeManning added Networking v5.2.0 Q2 2024 labels May 30, 2024

AgeManning requested review from dapplion and jxs May 30, 2024 07:11

AgeManning added 3 commits May 30, 2024 17:26

Code improvement

2e4fe3f

Bump rust version to 1.78

dbfde08

Downgrade to 1.77

a3c8e01

jxs approved these changes May 30, 2024

View reviewed changes

jxs reviewed May 30, 2024

View reviewed changes

beacon_node/lighthouse_network/src/service/mod.rs Outdated Show resolved Hide resolved

michaelsproul mentioned this pull request May 31, 2024

Update cargo lock #5871

Closed

michaelsproul mentioned this pull request May 31, 2024

Release v5.2.0 #5664

Merged

AgeManning and others added 2 commits June 2, 2024 15:04

Update beacon_node/lighthouse_network/src/service/mod.rs

05bddb9

Co-authored-by: João Oliveira <hello@jxs.pt>

fix fmt

9699902

dapplion approved these changes Jun 2, 2024

View reviewed changes

realbigsean added 2 commits June 5, 2024 18:11

Merge branch 'unstable' of https://github.com/sigp/lighthouse into rp…

7b2cc6c

…c-peer-disconnect-error

update lockfile

638a9bb

ackintosh approved these changes Jun 6, 2024

View reviewed changes

mergify bot merged commit 7b48b0b into sigp:unstable Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errors for all RPC Requests#5867

Errors for all RPC Requests#5867
mergify[bot] merged 9 commits intosigp:unstablefrom
AgeManning:rpc-peer-disconnect-error

AgeManning commented May 30, 2024

Uh oh!

jxs left a comment

Uh oh!

jxs May 30, 2024 •

edited

Loading

Uh oh!

AgeManning Jun 2, 2024

Uh oh!

Uh oh!

dapplion left a comment

Uh oh!

michaelsproul commented Jun 7, 2024

Uh oh!

mergify bot commented Jun 7, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

AgeManning commented May 30, 2024

Uh oh!

jxs left a comment

Choose a reason for hiding this comment

Uh oh!

jxs May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AgeManning Jun 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Jun 7, 2024

Uh oh!

mergify bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jxs May 30, 2024 •

edited

Loading

mergify bot commented Jun 7, 2024 •

edited

Loading