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(waku)_: add lightpush rate-limiter #5504

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented Jul 10, 2024

This PR enables rate-limiting on Lightpush protocol with 1 msg/s rate.

Important changes:

  • Enable 1 msg/s rate limit on Lightpush by default

Related to #4857

@status-im-auto
Copy link
Member

status-im-auto commented Jul 10, 2024

Jenkins Builds

Click to see older builds (33)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e1febab #1 2024-07-10 14:25:05 ~2 min tests-rpc 📄log
✖️ e1febab #1 2024-07-10 14:25:26 ~2 min tests 📄log
✔️ e1febab #1 2024-07-10 14:26:29 ~4 min linux 📦zip
✔️ e1febab #1 2024-07-10 14:26:43 ~4 min ios 📦zip
✔️ e1febab #1 2024-07-10 14:28:16 ~5 min android 📦aar
✔️ fc7b7e4 #2 2024-07-10 14:38:16 ~2 min ios 📦zip
✖️ fc7b7e4 #2 2024-07-10 14:38:18 ~2 min tests 📄log
✔️ fc7b7e4 #2 2024-07-10 14:38:25 ~2 min tests-rpc 📄log
✔️ fc7b7e4 #2 2024-07-10 14:39:32 ~3 min linux 📦zip
✔️ fc7b7e4 #2 2024-07-10 14:41:13 ~5 min android 📦aar
✔️ 355ec24 #3 2024-07-10 14:45:56 ~1 min android 📦aar
✔️ 355ec24 #3 2024-07-10 14:48:09 ~3 min linux 📦zip
✔️ 355ec24 #3 2024-07-10 14:49:21 ~4 min ios 📦zip
✖️ 355ec24 #3 2024-07-10 14:49:57 ~5 min tests 📄log
✔️ 71d41c0 #4 2024-07-10 14:57:32 ~1 min linux 📦zip
✔️ 71d41c0 #4 2024-07-10 14:59:17 ~3 min ios 📦zip
✔️ 71d41c0 #4 2024-07-10 15:01:03 ~5 min android 📦aar
✖️ 71d41c0 #4 2024-07-10 15:31:49 ~36 min tests 📄log
8c2e930 #5 2024-07-11 16:01:44 ~31 sec ios 📄log
8c2e930 #5 2024-07-11 16:04:06 ~2 min android 📄log
8c2e930 #5 2024-07-11 16:04:38 ~3 min linux 📄log
✖️ 8c2e930 #5 2024-07-11 16:05:13 ~4 min tests-rpc 📄log
✖️ 8c2e930 #5 2024-07-11 16:06:59 ~5 min tests 📄log
✖️ 8ad9b1d #6 2024-07-12 07:47:55 ~47 sec tests 📄log
✔️ 8ad9b1d #6 2024-07-12 07:49:27 ~2 min linux 📦zip
✔️ 8ad9b1d #6 2024-07-12 07:49:33 ~2 min tests-rpc 📄log
✔️ 8ad9b1d #6 2024-07-12 07:49:59 ~2 min ios 📦zip
✔️ 8ad9b1d #6 2024-07-12 07:52:22 ~5 min android 📦aar
5ea72f3 #7 2024-07-12 07:51:28 ~25 sec ios 📄log
✖️ 5ea72f3 #7 2024-07-12 07:52:08 ~57 sec tests 📄log
✖️ 5ea72f3 #7 2024-07-12 07:52:15 ~1 min tests-rpc 📄log
5ea72f3 #7 2024-07-12 07:52:38 ~1 min linux 📄log
5ea72f3 #7 2024-07-12 07:53:09 ~29 sec android 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 62b8105 #8 2024-07-12 07:53:36 ~13 sec tests-rpc 📄log
62b8105 #8 2024-07-12 07:53:48 ~24 sec ios 📄log
62b8105 #8 2024-07-12 07:53:55 ~26 sec android 📄log
62b8105 #8 2024-07-12 07:53:59 ~35 sec linux 📄log
✖️ 62b8105 #8 2024-07-12 07:54:47 ~1 min tests 📄log
✔️ 20c88f8 #9 2024-07-12 08:55:42 ~2 min tests-rpc 📄log
✔️ 20c88f8 #9 2024-07-12 08:55:43 ~2 min android 📦aar
✔️ 20c88f8 #9 2024-07-12 08:55:50 ~2 min linux 📦zip
✔️ 20c88f8 #9 2024-07-12 08:56:20 ~2 min ios 📦zip
✔️ 20c88f8 #9 2024-07-12 09:36:45 ~43 min tests 📄log

@vpavlin vpavlin force-pushed the feat/add-lightpush-ratelimit branch 3 times, most recently from 355ec24 to 71d41c0 Compare July 10, 2024 14:55
@chaitanyaprem
Copy link
Contributor

Wondering if we should follow rate-limits similar to how they are sent in nwaku or use a smaller size than fleet nodes.
@jm-clius @richard-ramos any thoughts?

@vpavlin
Copy link
Member Author

vpavlin commented Jul 11, 2024

IIUC this is mainly for the Status Desktop instances - hence the rate limits should be probably smaller than a fleet node - although 1 msg/s might be a bit too low:)

@jm-clius
Copy link

Actually I can't see rate limits being set anywhere (yet) for nwaku fleets. --request-rate-limit is not yet set for any fleet deployment and is in fact not yet part of the role: https://github.com/status-im/infra-role-nim-waku/blob/master/templates/config.toml.j2 (or am I missing something @NagyZoltanPeter?)

In other words, I'm not sure what the intended rate limits for the fleets are. 1 msg/second for Status Desktop nominally sounds reasonable, though this may require real world feedback to confirm. :)

@NagyZoltanPeter
Copy link

Actually I can't see rate limits being set anywhere (yet) for nwaku fleets. --request-rate-limit is not yet set for any fleet deployment and is in fact not yet part of the role: https://github.com/status-im/infra-role-nim-waku/blob/master/templates/config.toml.j2 (or am I missing something @NagyZoltanPeter?)

In other words, I'm not sure what the intended rate limits for the fleets are. 1 msg/second for Status Desktop nominally sounds reasonable, though this may require real world feedback to confirm. :)

Yes, it is not applied yet.
We may need to open discussion on this topic as I'm just finished with the last phase of request rate limitation.
That will apply default per peer request rate for filter service.
lightpush and store and store legacy are default switched off.

There are number of questions how to apply DOS protection properly.

  • I think we cannot apply one single rule for lightpush and store.
    • Do we need separate CLI arguments or defaults?
    • RLN and dos protection are a bit competing feature: for lightpush we may consider different approach for DOS protection. The current solution is flexible on that.
    • What are the acceptable query rates for store service?
      ... probably there come more questions to discuss.

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

LGTM. We can always change the rate limit values if needed

Copy link
Contributor

@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

@vpavlin vpavlin force-pushed the feat/add-lightpush-ratelimit branch from 71d41c0 to 8c2e930 Compare July 11, 2024 16:00
@vpavlin vpavlin force-pushed the feat/add-lightpush-ratelimit branch 2 times, most recently from 5ea72f3 to 62b8105 Compare July 12, 2024 07:53
@vpavlin vpavlin force-pushed the feat/add-lightpush-ratelimit branch from 62b8105 to 20c88f8 Compare July 12, 2024 08:53
@vpavlin vpavlin merged commit ea35803 into develop Jul 12, 2024
10 checks passed
@vpavlin vpavlin deleted the feat/add-lightpush-ratelimit branch July 12, 2024 10:34
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.

6 participants