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: Added flexible rate limit checks for store, legacy store and lightpush #2668

Merged
merged 6 commits into from
May 9, 2024
Merged

feat: Added flexible rate limit checks for store, legacy store and lightpush #2668

merged 6 commits into from
May 9, 2024

Conversation

NagyZoltanPeter
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter commented May 3, 2024 •
edited
Loading

Also added rate and traffic metrics.

Description

This PR addresses most of the weakness of phase1 implementation and extend it with bandwidth metrics for the protocols.
For convenience it fixes some nuances on other aspects.

Changes

  • Added template for ease and generalize the use of tokenbucket.
  • Rate check is done before anything would happen to request lib2p2 stream (no read/copy/decode)
  • Added counting of the individual protocols in/outbound bandwidths.
  • Renamed legacy store codec global const to avoid misunderstanding
  • Fixed missing legacy store protocol in GET /admin/peers query

Missing

Will be applied in a separate PR:

  • Rate limiting Filter
    • It might need a bit different approach due to its nature of subscribe and ping
  • Trace top contributor peers of over limit use and ban them

Issue

#2589
#2032

@NagyZoltanPeter NagyZoltanPeter self-assigned this May 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2668-rln-v2

Built from f1345b3

@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2668-rln-v1

Built from f1345b3

@NagyZoltanPeter NagyZoltanPeter marked this pull request as ready for review May 6, 2024 12:11
@gabrielmer gabrielmer self-requested a review May 7, 2024 23:48
gabrielmer
gabrielmer approved these changes May 7, 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.

Loved this PR! 😍 Thanks so much!

Now I noticed that we don't really write tests to make sure counters work correctly. Maybe it would be good idea to start doing it in the future, counters are underrated IMO :))

Ivansete-status
Ivansete-status approved these changes May 9, 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! Super professional PR and a great use of templates 🥳
I just added some comments that I hope you find useful

waku/common/ratelimit.nim Outdated Show resolved Hide resolved
waku/common/ratelimit.nim Outdated Show resolved Hide resolved
waku/common/ratelimit.nim Outdated Show resolved Hide resolved
tests/waku_store_legacy/test_resume.nim Show resolved Hide resolved
waku/waku_store/protocol.nim Outdated Show resolved Hide resolved
waku/waku_store/protocol.nim Show resolved Hide resolved
waku/waku_store_legacy/protocol.nim Show resolved Hide resolved
waku/waku_store_legacy/protocol.nim Outdated Show resolved Hide resolved
@NagyZoltanPeter NagyZoltanPeter merged commit 026d804 into master May 9, 2024
14 of 15 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the feat-rate-limit-phase2 branch May 9, 2024 18:07
Ivansete-status added a commit that referenced this pull request May 14, 2024
@NagyZoltanPeter @Ivansete-status
feat: Added flexible rate limit checks for store, legacy store and li…
4936df6 
…ghtpush (#2668)

* Added flexible rate limit checks for store, legacy store and lightpush. Also added rate and traffic metrics.

* Fix chat2 after WakuLegacyStoreCodec rename

* Update waku/common/ratelimit.nim

Co-authored-by: Ivan FB <[email protected]>

* Update waku/common/ratelimit.nim

Co-authored-by: Ivan FB <[email protected]>

* Update waku/waku_store_legacy/protocol.nim

Co-authored-by: Ivan FB <[email protected]>

* Fix review findings, added limit to debug logs

---------

Co-authored-by: Ivan FB <[email protected]>
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

@alrevuelta alrevuelta Awaiting requested review from alrevuelta

Assignees

@NagyZoltanPeter NagyZoltanPeter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NagyZoltanPeter @gabrielmer @Ivansete-status