Delayed RPC Send Using Tokens

[ackintosh]

Issue Addressed

closes #5785

Proposed Changes

The diagram below shows the differences in how the receiver (responder) behaves before and after this PR. The following sentences will detail the changes.

flowchart TD

subgraph "*** After ***"
    Start2([START]) --> AA[Receive request]
    AA --> COND1{Is there already an active request <br> with the same protocol?}
    COND1 --> |Yes| CC[Send error response]
    CC --> End2([END])
    %% COND1 --> |No| COND2{Request is too large?}
    %% COND2 --> |Yes| CC
    COND1 --> |No| DD[Process request]
    DD --> EE{Rate limit reached?}
    EE --> |Yes| FF[Wait until tokens are regenerated]
    FF --> EE
    EE --> |No| GG[Send response]
    GG --> End2
end

subgraph "*** Before ***"
    Start([START]) --> A[Receive request]
    A --> B{Rate limit reached <br> or <br> request is too large?}
    B -->|Yes| C[Send error response]
    C --> End([END])
    B -->|No| E[Process request]
    E --> F[Send response]
    F --> End
end

Loading

Is there already an active request with the same protocol?

This check is not performed in Before. This is taken from the PR in the consensus-spec, which proposes updates regarding rate limiting and response timeout.
https://github.com/ethereum/consensus-specs/pull/3767/files

The requester MUST NOT make more than two concurrent requests with the same ID.

The PR mentions the requester side. In this PR, I introduced the ActiveRequestsLimiter for the responder side to restrict more than two requests from running simultaneously on the same protocol per peer. If the limiter disallows a request, the responder sends a rate-limited error and penalizes the requester.

Rate limit reached? and Wait until tokens are regenerated

UPDATE: I moved the limiter logic to the behaviour side. #5923 (comment)

The rate limiter is shared between the behaviour and the handler. (Arc<Mutex<RateLimiter>>>) The handler checks the rate limit and queues the response if the limit is reached. The behaviour handles pruning.

I considered not sharing the rate limiter between the behaviour and the handler, and performing all of these either within the behaviour or handler. However, I decided against this for the following reasons:

• Regarding performing everything within the behaviour: The behaviour is unable to recognize the response protocol when RPC::send_response() is called, especially when the response is RPCCodedResponse::Error. Therefore, the behaviour can't rate limit responses based on the response protocol.
• Regarding performing everything within the handler: When multiple connections are established with a peer, there could be multiple handlers interacting with that peer. Thus, we cannot enforce rate limiting per peer solely within the handler. (Any ideas? 🤔 )

Additional Info

Naming

I have renamed the fields of the behaviour to make them more intuitive:

• limiter -> response_limiter
• self_limiter -> outbound_request_limiter

Testing

I have run beacon node with this changes for 24hours, it looks work fine.

The rate-limited error has not occurred anymore while running this branch.

[ackintosh]

https://github.com/sigp/lighthouse/actions/runs/12109786536/job/33759312718?pr=5923

The network-tests failure was not reproduced locally. It might be related to #6646.

[ackintosh]

@jxs @pawanjay176 @AgeManning

This is ready for another review. 🙏

I have added a concurrency limit on the self-lmiter. Now, the self-limiter limits outbound requests based on both the number of concurrent requests and tokens (optional). Whether we also need to limit tokens in the self-limiter is still under duscussion. Let me know if you have any ideas.

---

(FYI)

I also ran lighthouse (this branch) on the testnet for ~24hours. During this time, the LH node responded with 21 RateLimited errors due to the number of active requests. These errors appear in the logs like the example below. Note that this is about inbound requests, not the self-limiter (outbound requests).

Dec 09 13:38:56.806 DEBG There is an active request with the same protocol, protocol: beacon_blocks_by_range, request: Blocks by range: Start Slot: 2738468, Count: 64, Step: 1, peer_id: 16Uiu2HAmERvtCC321A2Nu1pH6QmhXgvqALxgCnrp4qxr2qeVWr2P, service: libp2p_rpc, service: libp2p, module: lighthouse_network::rpc:491

[AgeManning]

@pawanjay176 @jxs @dapplion @jimmygchen - If anyone has any spare time, I think this is a good one to get in.

[ackintosh]

I removed RPC::is_request_size_too_large in cfea9d2 because such invalid requests are now handled in the Handler, as implemented in the following PRs:

• Fork aware max values in rpc #6847
• Penalize peers that send an invalid rpc request #6986

[dapplion]

This means we can have at most two blocks_by_range ReqResp requests per peer?

[dapplion]

Suggested change

	"Rate limited. There is an active request with the same protocol"
	format!("Rate limited. There are already {MAX_CONCURRENT_REQUESTS} active requests with the same protocol")

[dapplion]

Overall architecture looks good! Just some comments.

Could you add a metric to track the time our own requests are idling in the self-rate limiter? It will help inform is sync performance is hindered by this new rate limit policy. We should also track the time outbound responses are idling in the rate limiter.

[dapplion]

We consider the memory cost of storing responses too low to worry? The worst case is buffering 2 x data_columns_by_range for all possible indices for all peers. For 9 blobs per block, that's 131072*2 * 9 * 100 / 1e6 = 235 MB

[dapplion]

Is the point of this PR to not have an additional global rate limiter? It would help to reduce the worst case scenario

[dapplion]

If you remove a QueuedRequest item from queued_requests and the limiter doesn't allow to send, is this request lost?