Rate limiting on client sync state

[TomasArrachea]

While benchmarking the web client's sync state against a local node with a seeded store, the rate limiting default values cause the sync to fail:

Caused by:
    0: grpc request failed for get_notes_by_id: request was rate-limited or the node's resources are exhausted; retry after a delay
    1: code: 'Some resource has been exhausted', message: "Too Many Requests! Wait for 0s"

Setting the node with more relaxed rate limiting allows the sync to complete successfully.

This might differ on testnet due to the network latency on each request. Anyway, we might want to check these values considering the normal client behaviour, or perhaps we should introduce a backoff on the client side to avoid being rate limited, thus making the sync slower.

---

To reproduce it:

On the node side (use branch tomasarrachea-seed-store-consume-notes):

1. Start the seeded node:

cargo run --release --bin miden-node-stress-test -- seed-collector --data-directory ./data --num-blocks 200

cargo run --release --bin miden-node -- bundled start --data-directory ./data --rpc.url http://localhost:57291

2. Check the seeded account id:

cat data/accounts.txt

On the client side (branch tomasarrachea-sync-benchmark):

1. Run the client benchmark against that account:

cargo run --release -p miden-client-bench -- sync --account-id <ACCOUNT_ID>

[Mirko-von-Leipzig]

or perhaps we should introduce a backoff on the client side to avoid being rate limited, thus making the sync slower.

You should ideally always honor the backoff given by a server response.

Rate limiting is something we introduced somewhat recently, so it is fairly new behaviour from the node. But it is essential and is something to expect from any server that you're hitting frequently - it could be overloaded at any point.

iirc we have different settings internally so that we can still benchmark actual through-put e.g. from the stress test.

I guess my question is - what's the question 😁?

[igamigo]

or perhaps we should introduce a backoff on the client side to avoid being rate limited, thus making the sync slower.

You should ideally always honor the backoff given by a server response.

Rate limiting is something we introduced somewhat recently, so it is fairly new behaviour from the node. But it is essential and is something to expect from any server that you're hitting frequently - it could be overloaded at any point.

iirc we have different settings internally so that we can still benchmark actual through-put e.g. from the stress test.

I guess my question is - what's the question 😁?

I think one question here would be if the response contains cooldown info. I think your response implies that it does but I have not confirmed it (I have also not seen the actual implementation). The other question that popped up for me is if the limits had any specific criteria - with the multiple roundtrips that a single sync state incurs, would they be worth revising? We have been getting some issues about some calls taking more time than expected (especially in the webclient); though I have also not seen if these come up on fairly average scenarios, so maybe this is also a question for @TomasArrachea.

[TomasArrachea]

I guess my question is - what's the question 😁?

Yeah sorry, I think that my main concern is whether it makes sense to have a rate limiter set in a way that a normal client sync gets rate limited. Specially since the sync makes several different requests, and we actually want to make them as frequent as possible to improve sync times.

Caused by:
    0: grpc request failed for get_notes_by_id: request was rate-limited or the node's resources are exhausted; retry after a delay
    1: code: 'Some resource has been exhausted', message: "Too Many Requests! Wait for 0s"

Also, I think that the rate limit error shows "Wait for 0s" because tower_governor reports the cooldown in whole seconds via .as_secs(), which truncates sub-second durations to zero. With the default replenish rate of 16, the actual cooldown per token is ~63ms, but the client sees 0s.

[Mirko-von-Leipzig]

Yeah sorry, I think that my main concern is whether it makes sense to have a rate limiter set in a way that a normal client sync gets rate limited. Specially since the sync makes several different requests, and we actually want to make them as frequent as possible to improve sync times.

The current burst size is set to 128 -- that already seems large to me? We also need to consider how to fairly distribute amongst thousands of clients.

Give me a burst size that seems reasonable? I think clients do operate in bursts so if we can come to a sensible default then we're okay.

Something we can also do is add another layer where individual IP rate limiting only kicks in after a generous global burst is exceeded. This would give more generous overall bursts when traffic is low (global bucket still has tokens), and apply rate limiting if traffic is high.

We still need to know what to configure these as though.

Also, I think that the rate limit error shows "Wait for 0s" because tower_governor reports the cooldown in whole seconds via .as_secs(), which truncates sub-second durations to zero. With the default replenish rate of 16, the actual cooldown per token is ~63ms, but the client sees 0s.

That's true, and I guess confusing. I think we should also be setting some return header which gives a non-text based backoff? If not, then we should add that.

[TomasArrachea]

That's true, and I guess confusing. I think we should also be setting some return header which gives a non-text based backoff? If not, then we should add that.

The response contains a retry-after (and x-ratelimit-after) header but it's also truncated as whole seconds, so is set to 0 in this case.

[Mirko-von-Leipzig]

That's true, and I guess confusing. I think we should also be setting some return header which gives a non-text based backoff? If not, then we should add that.

The response contains a retry-after (and x-ratelimit-after) header but it's also truncated as whole seconds, so is set to 0 in this case.

I think its probably not intended to have ms style retries. Even the library API uses replenish as number of seconds per request 😅 So having 16 per second is massive compared to conventional usage.