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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

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
… [`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 Show resolved Hide resolved
bin/node/src/aleph_cli.rs Outdated 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 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
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