Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase rpc rate limits #6626

Merged
merged 3 commits into from
Dec 1, 2024
Merged

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Nov 27, 2024

Issue Addressed

N/A

Proposed Changes

In #6093 , we bumped the rate limits for blobs by range significantly presumably for peerdas devnets. This limit is too permissive for mainnet imo, we should reduce it down significantly so as to not get overwhelmed with requests and also keeping the upload bandwidth at check.

I'm proposing to increase the limits by a factor of around 3. Allowing a single epoch of blocks (32 blocks max) to be requested per second or 160 per 5 seconds to account for bursts.
See #6626 (comment) for justification for final numbers on this PR after testing

@michaelsproul michaelsproul added v6.0.0 New major release for hierarchical state diffs Networking labels Nov 27, 2024
michaelsproul added a commit that referenced this pull request Nov 27, 2024
Squashed commit of the following:

commit de4ab55
Author: Pawan Dhananjay <[email protected]>
Date:   Wed Nov 27 15:12:25 2024 -0800

    Increase rate limits for byrange requests
@michaelsproul michaelsproul mentioned this pull request Nov 28, 2024
@pawanjay176
Copy link
Member Author

pawanjay176 commented Nov 28, 2024

After doing some analysis, I think we should adjust the defaults to be 128 tokens/10 secs for blocksbyrange and 512 tokens/10 secs for blobsbyrange (target+1 blobs per block on average to account for high blob usage periods)

Just writing down the thought process for future reference.

With these numbers, our bandwidth usage per peer would look like

  • Base rate blocks: 128 blocks/10s * 100KB = 1,280 KB/s
  • Base rate blobs: 512 blobs/10s * 128KB = 6,553.6 KB/s
  • Total base bandwidth per peer: ~7.8 MB/s
  • with the token bucketing algo we use, this could go max upto ~15.6 MB/s

For a node requesting a batch (32 blocks + associated blobs) from a given peer every 3-5 seconds, this limit allows 4 full batch requests (1 batch = 1 epoch) per 10-second window per peer. Given that we have a big pool of nodes to sync from in mainnet and similar networks, this value makes sense for mainnet/gnosis imo. For devnets, we can raise the limits with cli flags.
The 10-second window provides good spam resistance vs shorter window sizes since an attacker would need to spread out these requests to max out the bandwidth usage.

Not touching data column limits in this PR.

@michaelsproul michaelsproul added the ready-for-review The code is ready for review label Nov 28, 2024
michaelsproul added a commit that referenced this pull request Nov 28, 2024
Squashed commit of the following:

commit 29ab3f5
Author: Pawan Dhananjay <[email protected]>
Date:   Wed Nov 27 20:34:30 2024 -0800

    Update limits

commit 3f25991
Merge: de4ab55 38f5f66
Author: Michael Sproul <[email protected]>
Date:   Thu Nov 28 13:41:35 2024 +1100

    Merge branch 'unstable' into reduce-blob-limits

commit de4ab55
Author: Pawan Dhananjay <[email protected]>
Date:   Wed Nov 27 15:12:25 2024 -0800

    Increase rate limits for byrange requests
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this Pawan! Can we leave a link to #6626 (comment) on the comment for the detailed info?

@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Dec 1, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f8e31f6

mergify bot added a commit that referenced this pull request Dec 1, 2024
@mergify mergify bot merged commit f8e31f6 into sigp:unstable Dec 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants