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

A0-4318: network rate-limiter for the substrate network #1780

Conversation

fixxxedpoint
Copy link
Contributor

@fixxxedpoint fixxxedpoint commented Jul 10, 2024

Description

Implementation of the rate-limiter algorithm for the substrate-based part of our network code. It limits only the read side of connections. Additionally, now both rate-limit parameters for alephbft and substrate networks apply to all open connection (rate-limit for all nodes, not just single connection). An exploit together with e2e tests will be provided in another PR.

Type of change

  • New feature (non-breaking change which adds functionality)

fixxxedpoint and others added 10 commits July 10, 2024 12:14
- parametrized by TimeProvider instead of taking `now: Instant` as argument for `rate_limit`
- simplified rate_limit method
- re-aligned tests of TokenBucket
- TokenBucket can be set to rate_per_second = 0 - no data will be read
- seperate RateLimiter implementations for [`tokio::io::AsyncRead`] and [`futures::AsyncRead`]
- using [`std::sync::Mutex`] for TokenBucket in RateLimiter - it should allow for `global` rate-limit, not just per connection
… substrate's [`sc_client::network::NetworkWorker`]
- s/alephbft_bit_rate/alephbft_network_bit_rate
@fixxxedpoint fixxxedpoint requested a review from lesniak43 July 10, 2024 13:28
… [`finality-aleph/src/network/build/transport.rs`]
…port.rs`]

- removed references to [`libp2p`] from [`finality-aleph/src/network/build/mod.rs`]
@fixxxedpoint fixxxedpoint force-pushed the A0-4318_rate_limited_substrate_network branch from 04c0718 to 3e7ff62 Compare July 10, 2024 15:36
@fixxxedpoint fixxxedpoint requested review from ggawryal and removed request for Marcin-Radecki July 11, 2024 10:36
Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

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

Nice PR, left one question regarding write rate limiting and some nits.

rate-limiter/src/lib.rs Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
finality-aleph/src/network/substrate.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
bin/node/src/aleph_cli.rs Show resolved Hide resolved
Copy link
Contributor

@timorleph timorleph left a comment

Choose a reason for hiding this comment

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

I only have one miniscule nit, but we should finish the conversation about how much this actually helps before this gets merged.

finality-aleph/src/network/build/mod.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
rate-limiter/src/token_bucket.rs Outdated Show resolved Hide resolved
@fixxxedpoint fixxxedpoint force-pushed the A0-4318_rate_limited_substrate_network branch from 6b21677 to 21b4b73 Compare August 16, 2024 09:59
- added ShareTokenBucket, AsyncTokenBucket and ShredBandwidthManager - used for sharing bandwidth between multiple connections
…and Socket-based networks (sync + alephbft)

- new default values for rate-limiters (sync + alephbft). 768Kib for alephbft and 5Mib for sync. These values were tested using t3a.xlarge aws instances - nodes were not able to handle more.
…docker/docker_entrypoint.sh to allow configuration of rate-limiting for both sync and alephbft networks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants