feat: rate_limiting token bucket sub module

[geekbrother]

Description

The rate-limiting token bucket mechanism (based on #231) uses Redis and Moka in-memory cache.

The initial implementation was updated to use the local app cache in Moka and omit the Redis RTT calls (for single key calls) because we know the TTL when the token will be refilled in cases when the key is rate-limited. So we don't need to call the Redis until the TTL expires and we can use the local memory Moka cache. This should significantly eliminate Redis RTT calls in the case of DOS/flood. The initial Lua script was a bit optimized and minified.

How Has This Been Tested?

• New integration tests were introduced for the CI loop.

Due Diligence

• Breaking change
• Requires a documentation update
• Requires a e2e/integration test update

[chris13524]

This test has significantly less coverage than the one from Notify Server. Can we copy those cases directly so we can keep the level of coverage?

Missing:

• refill_in calculation
• checking that tokens recover 1 at a time rather than all at once
• test for separate keys

Why did you remove the ability to mock the clock? That helps ensure that tests aren't flaky as well as I think making the refill_in test case possible.

[geekbrother]

Makes sense to update the test coverage with the missing ones, thanks for pointing out this!

Why did you remove the ability to mock the clock? That helps ensure that tests aren't flaky as well as I think making the refill_in test case possible.

Maybe it's better to pass the timestamp to the function instead of hacking the clock for the test (it looks a bit hacky to me). We can just pass Utc::now().timestamp_millis() to the function instead of getting it inside of it.

I have updated tests and the function in the 729ec96.

The test coverage now has:

• refill_in calculation assertions,
• checking that tokens recover 1 at a time,
• test for multiple keys running in parallel.

[chris13524]

Are you planning to replace the Notify Server implementation with this standard one? If so, the mocked clock is was necessary to be able to test that rate limits are applied to regular endpoints.

It's common practice to mock clocks, I don't see what's "hacky" about that.

[geekbrother]

I want to apply this unified implementation to the Blockchain-api and Echo server and then probably to the Notify (not on the table yet).

[chris13524]

☹️ I would prefer if we touched the Lua script as minimally as possible. It's a direct copy of the linked script above with only the tweaks necessary to make it work with multiple keys. You may have introduced bugs and made it harder to keep in-sync with the implementation above. I found a bug before in the implementation and contributed it back. Do you really think you've made significant performance improvements worth risking these changes? Looks to me that these changes might actually be less performant due to the removal of locals and I don't think one-time complication or hashing speed really has an effect.

[geekbrother]

Reverted it in b9ecde9 if you find it hard to debug the minified version of the Lua script since it's not a huge optimization. Let's stick to the initial one and move forward with it.

[chris13524]

mem_cache TTL must be set to the same value as the refill interval.

What does this mean? Does Cache support different keys with different refill intervals e.g. for what Notify Server needs?

[geekbrother]

The TTL is for all records. If there are requirements to refill different rates for different cases it can be produced with the single refill interval (lowest) and different refill rates by tuning the refill rate to the single refill interval, without adding additional TTL per key.