Skip to content

Comments

feat(relay): Operation-based rate limiting + Resolve most recent#185

Open
afterburn wants to merge 29 commits intomainfrom
feat/rate-limiting
Open

feat(relay): Operation-based rate limiting + Resolve most recent#185
afterburn wants to merge 29 commits intomainfrom
feat/rate-limiting

Conversation

@afterburn
Copy link
Contributor

@afterburn afterburn commented Sep 18, 2025

Summary
This PR replaces current rate limiting with an operation-based configuration that maps directly to the relay's three core operations: publish, resolve, and resolve_most_recent.

Configuration Example

[relay]
[[relay.rate_limits]]
operation = "resolve"
quota = "10r/s"
burst = 20

[[relay.rate_limits]]
operation = "resolve_most_recent"
quota = "2r/s"
burst = 5

[[relay.rate_limits]]
operation = "publish"
quota = "2r/s"
burst = 10
whitelist = ["127.0.0.1", "192.168.1.0/24"]  # optional, both ips and subnets supported

Operations in this toml file automatically map to:

  • publish → PUT requests (publishing records)
  • resolve → GET requests without most_recent=true (cached lookups)
  • resolve_most_recent → GET requests with most_recent=true (DHT queries)

If no config.toml file is present, system will use defaults, so this is not a breaking change.

@afterburn afterburn requested review from SHAcollision and SeverinAlexB and removed request for SHAcollision September 18, 2025 12:56
@afterburn afterburn marked this pull request as ready for review September 22, 2025 09:17
Copy link
Contributor

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

Need to do a second round later

@afterburn
Copy link
Contributor Author

@SeverinAlexB resolved your feedback:

  • Removed kb, mb, gb quota units, now always uses either r/s or r/m as requested
  • Changed default rate limits to something saner
  • Added behind_proxy flag in config toml, which allows users to respect/ignore proxy headers
  • Cleaned up IP extraction, only happens once per request now
  • Added operation 'Index' so users can specify rate limiting for root path

Copy link
Contributor

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

Two comments, otherwise looks good.

In the Pubky Core team, we have a convention on how to format PR title. We stick to the conventional commit format.

So for this PR, the title could be: feat(relay): Operation-based rate limiting + Resolve most recent

CleanShot 2025-09-25 at 10 37 51@2x

@afterburn afterburn changed the title Add operation-based rate limiting feat(relay): Operation-based rate limiting + Resolve most recent Sep 25, 2025
@afterburn
Copy link
Contributor Author

@SeverinAlexB

  • If no rate limits are defined in config.toml, rate limiting will be disabled completely
  • Added Index to Operation enum so calling GET http://0.0.0.0:6881/ doesn't trigger the incorrect rate limit
  • Added resolve_most_recent_enabled flag so users have the ability to completely ignore the most_recent=true query parameter (defaults to false for backward compatibility)

Copy link
Contributor

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

One NIT, otherwise good to go

@SeverinAlexB
Copy link
Contributor

SeverinAlexB commented Oct 6, 2025

Actually, should this add the most_recent option to the pkarr client too? You are just adding it to the relay in this PR?

@afterburn afterburn requested a review from SHAcollision October 8, 2025 09:29
Copy link
Contributor

@SHAcollision SHAcollision left a comment

Choose a reason for hiding this comment

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

Great progress! Left a few minor comments and nits.

@afterburn
Copy link
Contributor Author

@SeverinAlexB @SHAcollision Restored DHT rate limiting that was previously removed. Since the DHT can be accessed directly, keeping rate limiting in place is necessary. The implementation is now fully backward compatible. Existing [rate_limiter] configurations continue to work as before. New [dht_rate_limit] and [http_rate_limit] options are opt-in. By default, it falls back to the legacy IP-based limiter unless one of the new configs is defined. Also added a comprehensive test to cover the new behavior and ensure backward compat.

afterburn and others added 8 commits November 4, 2025 12:21
* Consolidate simple-dns dependency in workspace root

* Bump simple-dns to latest 0.11.2

* Adapt calls to simple-dns methods

* pkarr: fix port param lookup

* extra: update Endpoint.params type

Params type was updated to better match what simple-dns exposes.

* Throw error if expected SVCParam type doesn't match

* Simplify ipv4 parsing

Co-authored-by: DZ <dzdidi@users.noreply.github.com>

* Use static lifetine for SVCParam in Endpoint.params

* fixed clippy

* A few code cleanups

* fmt

---------

Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com>
Co-authored-by: DZ <dzdidi@users.noreply.github.com>
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.

4 participants