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: add pre check for rate limit #194

Merged
merged 7 commits into from
Jun 27, 2024

Conversation

yjhmelody
Copy link
Contributor

The outer result of until_key_n_ready_with_jitter should be checked before start service, it's static result relatively .
And its meaning is the cap is insufficient for n.

src/server.rs Outdated
@@ -51,6 +51,12 @@ pub async fn build(config: Config) -> anyhow::Result<SubwayServerHandle> {

let rpc_method_weights = MethodWeights::from_config(&config.rpcs.methods);

// pre-check stage
if let Some(r) = &rate_limit_builder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you want to check this then just make sure no weight is greater than burst (ip or connection)

Copy link
Contributor Author

@yjhmelody yjhmelody Jun 24, 2024

Choose a reason for hiding this comment

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

Yeah, The result is fixed if config is not changed, so we should do pre-check.
Oherwise how do we know the config is correct.

Copy link
Collaborator

@ermalkaleci ermalkaleci left a comment

Choose a reason for hiding this comment

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

you can simple verify config file

@yjhmelody
Copy link
Contributor Author

you can simple verify config file

This code is doing what you said.

@ermalkaleci
Copy link
Collaborator

you can simple verify config file

This code is doing what you said.

no it's not. you're adding additional code that needs to be maintained and no necessary at all.
All you need to do is validate config file. assert!(method.rate_limit_weight <= min(ip.burst, connection.burst))

@ermalkaleci
Copy link
Collaborator

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Jun 24, 2024

That's not true, you just assumed period_secs ==1s.

You could try run this branch and master branch, it do need extra check.

Wait, maybe we just need to move this validation logic to there. But we still need such check logic.

Now:

./target/release/subway --config ./configs/config.yml
2024-06-24T10:16:38.652904Z ERROR subway: Config validation failed: `author_pendingExtrinsics` weight config too big for ip rate limit: 10000
2024-06-24T10:16:38.654024Z  INFO subway::extensions::client: Connecting to endpoint: wss://acala-rpc-0.aca-api.network
2024-06-24T10:16:38.655018Z  INFO substrate_prometheus_endpoint: 〽️ Prometheus exporter started at 0.0.0.0:9616    
2024-06-24T10:16:38.655339Z  INFO subway: Server running at 0.0.0.0:9944
2024-06-24T10:16:39.320293Z  INFO subway::extensions::client: Endpoint connected

@yjhmelody
Copy link
Contributor Author

Wonder the behavior design: why still continue the service when validation failed.

@ermalkaleci
Copy link
Collaborator

because it runs on it's own thread. it should work for any period

src/config/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

some tests will be nice

@ermalkaleci
Copy link
Collaborator

ermalkaleci commented Jun 26, 2024

All you need to do is validate config file. assert!(method.rate_limit_weight <= min(ip.burst, connection.burst))

@xlc @yjhmelody In case you missed this

@yjhmelody
Copy link
Contributor Author

All you need to do is validate config file. assert!(method.rate_limit_weight <= min(ip.burst, connection.burst))

@xlc @yjhmelody In case you missed this

Hi, you are right, I have simplify the code.

@xlc xlc merged commit a97673b into AcalaNetwork:master Jun 27, 2024
2 checks passed
fyInALT pushed a commit to fyInALT/subway that referenced this pull request Aug 10, 2024
* feat: pre-check for ip/connection rate limit

* clean up docs

* fix

* do pre-check in `validate` fn

* ok_or

* add tests and simplify validate
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.

3 participants