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.

Sign up for GitHub

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

Jump to bottom

feat: DOS protection of non relay protocols - rate limit phase3 #2897

Merged
merged 5 commits into from
Jul 16, 2024
Merged

feat: DOS protection of non relay protocols - rate limit phase3 #2897

merged 5 commits into from
Jul 16, 2024

Conversation

NagyZoltanPeter
Copy link
Contributor

Description

This PR is the final phase for Rate Limiting non relay protocols.

Aimed three major target:

  • Protect filter service on its specific needs.
    • Allow only a reasonable amount of subscribe/unsubscribe and ping requests for each client peer.
    • Such peer measurement is loosely coupled with maintained subscribed peers.
  • Add fair usage of lightpush and store protocols per peers. Do not allow one/few peers to overload the service.
  • Extend TokenBucket with load balancing.
    • Replenish calculation can consider previous period load and can apply more tokens thus compensating lower resource usage of previous period (capped with 25% threshold)

Remark in this version while lightpush and store protocols' protection can be configured from CLI by default it is switched off.

Notice for reviewers:

  • Filter protocol has an arbitrary limit applied per peers (30 req / 1 min / peer)

  • RequestRateLimiter applies a ratio calculation for managing fair usage of peers.

    • Thus, while main token bucket applies a global token capacity within a certain period of time. Peer allowed usage is managed as a factor of this.

Calculated as "main token capacity" * ratio * 75% over (main time period) * ratio
In other words when a peer used 75% of the previous n periods token capacity it gets rejected in order to let small users go up until main token capacity is not reached.
Of course, big usage peers will be replenished also but only after the "n" period of time.

The ratio is calculated currently as - if main time period is set to:

  • less than a second -> 10x
  • less than a minute -> 3x
  • anything above that -> 2x ratio applied

This is debatable I wanted to apply reasonable scale in time on the applied multiplication factor.

Peer measurement also time capped, non requester peers will be wiped out from the store.

Changes

  • Enhanced TokenBucket to be able to add compensation tokens based on previous usage percentage,
  • per peer rate limiter 'PeerRateLimier' applied on waku_filter_v2 with opinionated default of acceptable request rate
  • Add traffic metrics to filter message push
  • RequestRateLimiter added to combine simple token bucket limiting of request numbers but consider per peer usage over time and prevent some peers to over use the service (although currently rule violating peers will not be disconnected by this time only their requests will get not served)
  • TimedMap utility created (inspired and taken from libp2p TimedCache) which serves as forgiving feature for peers had been overusing the service.
  • Applied new RequestRateLimiter to store and lightpush protocols
  • Added more tests
  • Fix rebase issues

How to test

Unit tests applied.

Issue

#2683 covers #2032

@NagyZoltanPeter
DOS protection of non relay protocols - rate limit phase3:
42401eb 
- Enhanced TokenBucket to be able to add compensation tokens based on previous usage percentage,
- per peer rate limiter 'PeerRateLimier' applied on waku_filter_v2 with opinionated default of acceptable request rate
- Add traffic metrics to filter message push
- RequestRateLimiter added to combine simple token bucket limiting of request numbers but consider per peer usage over time and prevent some peers to over use the service
  (although currently rule violating peers will not be disconnected by this time only their requests will get not served)
- TimedMap utility created (inspired and taken from libp2p TimedCache) which serves as forgiving feature for peers had been overusing the service.
- Added more tests
- Fix rebase issues
- Applied new RequestRateLimiter for store and legacy_store and lightpush
@NagyZoltanPeter NagyZoltanPeter self-assigned this Jul 11, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 11, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2897

Built from 8325b5b

SionoiS
SionoiS approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@SionoiS SionoiS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Amazing work!

As for what settings to use, it's hard to say. Real usage would be how we determine that.

jm-clius
jm-clius approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Really great work! 🎉

Only very minor comments below.

waku/common/utils/timedmap.nim Outdated Show resolved Hide resolved
waku/common/utils/timedmap.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/peerratelimiter.nim Outdated Show resolved Hide resolved
gabrielmer
gabrielmer approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Looks amazing! 🔥 🔥
Thanks so much!

waku/common/ratelimit/simpleratelimiter.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/requestratelimiter.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/requestratelimiter.nim Outdated Show resolved Hide resolved
Ivansete-status
Ivansete-status approved these changes Jul 15, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it! 💯
I just added some comments that I hope you find useful :)

waku/common/ratelimit/ratelimitsetting.nim Outdated Show resolved Hide resolved
tests/common/test_requestratelimiter.nim Outdated Show resolved Hide resolved
tests/common/test_requestratelimiter.nim Show resolved Hide resolved
waku/common/utils/timedmap.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/tokenbucket.nim Outdated Show resolved Hide resolved
waku/waku_filter_v2/subscriptions.nim Show resolved Hide resolved
waku/waku_filter_v2/protocol.nim Show resolved Hide resolved
waku/common/ratelimit/requestratelimiter.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/requestratelimiter.nim Outdated Show resolved Hide resolved
waku/common/ratelimit/simpleratelimiter.nim Outdated Show resolved Hide resolved
@NagyZoltanPeter NagyZoltanPeter merged commit ba418ab into master Jul 16, 2024
8 of 10 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the feat-rate-limit-phase3-for-rebase branch July 16, 2024 13:46
@fbarbu15 fbarbu15 mentioned this pull request Jul 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

@jm-clius jm-clius jm-clius approved these changes

@SionoiS SionoiS SionoiS approved these changes

@alrevuelta alrevuelta Awaiting requested review from alrevuelta

@darshankabariya darshankabariya Awaiting requested review from darshankabariya

Assignees

@NagyZoltanPeter NagyZoltanPeter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NagyZoltanPeter @SionoiS @jm-clius @gabrielmer @Ivansete-status