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

feat: rate limit with rln configuration #1262

Merged
merged 5 commits into from
Dec 24, 2024
Merged

Conversation

kaichaosun
Copy link
Contributor

@kaichaosun kaichaosun commented Dec 3, 2024

Description

Support RLN contract's configuration like: allow 100 messages per 1 minute.

[DO NOT MERGE] Use this PR to test 1:1 with RLN
The default rate limiter is used when not configured, so this PR is safe to be merged.

Changes

  • create a new custom rate limiter with refill at certain interval strategy

@status-im-auto
Copy link

status-im-auto commented Dec 3, 2024

Jenkins Builds

Click to see older builds (4)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 37affd2 #1 2024-12-03 07:25:39 ~2 min unknown 📄log
✔️ 4908959 #2 2024-12-03 07:27:51 ~2 min unknown 📄log
✔️ b6cf713 #3 2024-12-03 07:32:23 ~2 min unknown 📄log
✔️ 76309b2 #4 2024-12-05 07:20:31 ~2 min unknown 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1bcb412 #5 2024-12-17 01:25:15 ~2 min unknown 📄log
✔️ 6b6cd5f #6 2024-12-18 06:45:14 ~2 min unknown 📄log

@kaichaosun kaichaosun requested a review from plopezlpz December 3, 2024 07:31
@chaitanyaprem
Copy link
Collaborator

@kaichaosun i am guessing this is for some sort of testing and analysis because RLN already the logic to accept/reject msgs based on rate limit, if so better not to merge it. Would it be required for both lightclient and relay?

Would be helpful if you can give more context as to what this change is meant to be used for.

Also iiuc RLN integration is supposed to be done for 1:1 chats first right. if so, wondering if we should introduce this limiter at such lower layer or a higher layer in status-go where we can distinguish 1:1 chats and community chats.

@kaichaosun
Copy link
Contributor Author

@chaitanyaprem this PR/branch is used for 1:1 chat with RLN, updated the description to reflect its purpose.
Also it would be good to get feedback about the implementation.

@kaichaosun kaichaosun changed the title feat: rate limit with rln configuration feat: rate limit with rln configuration [DO NOT MERGE] Dec 4, 2024
Copy link
Collaborator

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

Rate limiter logic looks good to me, but i am surprised golang's default rate-limiter doesn't support any other interval other than rate/sec.

waku/v2/api/publish/message_sender.go Outdated Show resolved Hide resolved
@kaichaosun
Copy link
Contributor Author

maybe we can use some build flag to choose between default or RLN Rate limiter?

Using a config in status-go seems better, and pass the rate limiter as needed.

@richard-ramos
Copy link
Member

Interesting implementation of a rate limiter!

@kaichaosun kaichaosun changed the title feat: rate limit with rln configuration [DO NOT MERGE] feat: rate limit with rln configuration Dec 17, 2024
@kaichaosun
Copy link
Contributor Author

cc @richard-ramos for another review.

Copy link
Collaborator

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

LGTM

wondering if default rate-limit is fine for community owner node or will it run into any issues?

@kaichaosun
Copy link
Contributor Author

kaichaosun commented Dec 17, 2024

wondering if default rate-limit is fine for community owner node or will it run into any issues?

The default rate limit is 5 messages per second and burst into 10, which was added previously and already used by status-go. Actually the old default config is 2 messages per second before #1267, and deployed via status-im/status-go#5523.

waku/v2/api/publish/rln_rate_limiting.go Outdated Show resolved Hide resolved
@kaichaosun kaichaosun merged commit 6dcf177 into master Dec 24, 2024
12 checks passed
@kaichaosun kaichaosun deleted the custom-rate-limit branch December 24, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants